On Mon, 2022-05-09 at 08:18 -0700, Isaku Yamahata wrote:
> > > +struct kvm_tdx_init_vm {
> > > + __u64 attributes;
> > > + __u32 max_vcpus;
> > > + __u32 tsc_khz;
> > > + __u64 mrconfigid[6]; /* sha384 digest */
> > > + __u64 mrowner[6]; /* sha384 digest */
> > > + __u64 mrownerconfig[6]; /* sha348 digest */
> > > + union {
> > > + /*
> > > + * KVM_TDX_INIT_VM is called before vcpu creation, thus
> > > before
> > > + * KVM_SET_CPUID2. CPUID configurations needs to be
> > > passed.
> > > + *
> > > + * This configuration supersedes KVM_SET_CPUID{,2}.
> > > + * The user space VMM, e.g. qemu, should make them
> > > consistent
> > > + * with this values.
> > > + * sizeof(struct kvm_cpuid_entry2) *
> > > KVM_MAX_CPUID_ENTRIES(256)
> > > + * = 8KB.
> > > + */
> > > + struct {
> > > + struct kvm_cpuid2 cpuid;
> > > + /* 8KB with KVM_MAX_CPUID_ENTRIES. */
> > > + struct kvm_cpuid_entry2 entries[];
> > > + };
> > > + /*
> > > + * For future extensibility.
> > > + * The size(struct kvm_tdx_init_vm) = 16KB.
> > > + * This should be enough given sizeof(TD_PARAMS) = 1024
> > > + */
> > > + __u64 reserved[2028];
> >
> > I don't think it's a good idea to put the CPUID configs at the end of this
> > structure and put it into a union.
> >
> > 1. The union makes the Array of Length zero entries[] pointless.
> > 2. It wastes memory that when new field to be added in the future, it has to
> > be put after union instead of inside union.
>
> Hmm, I checked this as there was a suggestion to do so.
> I have to admit that it's ugly for future reserved area. The options I can
> think of are
>
> A. add a pointer to struct kvm_cpuid2 (previous v5 patch)
> B. this patch.
Why can't we just use kvm_cpuid2 here to replace the union? We can add
additional reserved space before kvm_cpuid2 for future extension. Is there any
problem?
I don't see there's fundamental difference between putting kvm_cpuid2 directly
here vs putting a 'cpuid' pointer here. My personal feeling is the former is
clearer than the latter.
--
Thanks,
-Kai
