Re: [RFC PATCH v6 025/104] KVM: TDX: initialize VM with TDX specific parameters

[Kai Huang <kai.huang@intel.com>]

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