Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a125cfc58ef09367dc4557fd8a854ed9f21c1675.camel@intel.com>
Subject: Re: [RFC PATCH v6 025/104] KVM: TDX: initialize VM with TDX
 specific parameters
From:   Kai Huang <kai.huang@intel.com>
To:     Isaku Yamahata <isaku.yamahata@gmail.com>,
        Xiaoyao Li <xiaoyao.li@intel.com>
Cc:     isaku.yamahata@intel.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        erdemaktas@google.com, Sean Christopherson <seanjc@google.com>,
        Sagi Shahar <sagis@google.com>
Date:   Sat, 14 May 2022 00:34:55 +1200
In-Reply-To: <20220509151829.GC2789321@ls.amr.corp.intel.com>
References: <cover.1651774250.git.isaku.yamahata@intel.com>
         <fbc23565f7556e7b33227bcad95441195bb4758d.1651774250.git.isaku.yamahata@intel.com>
         <b3d587fd-1bf2-411c-96a9-6750e9aeefa2@intel.com>
         <20220509151829.GC2789321@ls.amr.corp.intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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