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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a125cfc58ef09367dc4557fd8a854ed9f21c1675.camel@intel.com>
Date:   Sat, 14 May 2022 00:34:55 +1200
From:   Kai Huang <kai.huang@...el.com>
To:     Isaku Yamahata <isaku.yamahata@...il.com>,
        Xiaoyao Li <xiaoyao.li@...el.com>
Cc:     isaku.yamahata@...el.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        erdemaktas@...gle.com, Sean Christopherson <seanjc@...gle.com>,
        Sagi Shahar <sagis@...gle.com>
Subject: Re: [RFC PATCH v6 025/104] KVM: TDX: initialize VM with TDX
 specific parameters

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