Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

[Edgecombe, Rick P]

On Mon, 2024-08-12 at 15:48 -0700, Rick Edgecombe wrote:
> +static inline bool is_td_vcpu_created(struct vcpu_tdx *tdx)
> +{
> +       return tdx->td_vcpu_created;
> +}

This and is_td_finalized() seem like unneeded helpers. The field name is clear
enough.

> +
>  static inline bool is_td_created(struct kvm_tdx *kvm_tdx)
>  {
>         return kvm_tdx->tdr_pa;

Not this one though, the helper makes the caller code clearer.

> @@ -105,6 +110,11 @@ static inline bool is_hkid_assigned(struct kvm_tdx
> *kvm_tdx)
>         return kvm_tdx->hkid > 0;
>  }
>  
> +static inline bool is_td_finalized(struct kvm_tdx *kvm_tdx)
> +{
> +       return kvm_tdx->finalized;
> +}
> +

Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

[Tony Lindgren]

On Wed, Aug 28, 2024 at 02:34:21PM +0000, Edgecombe, Rick P wrote:
> On Mon, 2024-08-12 at 15:48 -0700, Rick Edgecombe wrote:
> > +static inline bool is_td_vcpu_created(struct vcpu_tdx *tdx)
> > +{
> > +       return tdx->td_vcpu_created;
> > +}
> 
> This and is_td_finalized() seem like unneeded helpers. The field name is clear
> enough.

I'll do a patch for this.

> >  static inline bool is_td_created(struct kvm_tdx *kvm_tdx)
> >  {
> >         return kvm_tdx->tdr_pa;
> 
> Not this one though, the helper makes the caller code clearer.

Yes this makes things more readable.

Regards,

Tony

Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

[Tony Lindgren]

On Tue, Aug 13, 2024 at 04:00:09PM +0800, Yuan Yao wrote:
> On Mon, Aug 12, 2024 at 03:48:13PM -0700, Rick Edgecombe wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > @@ -293,6 +303,15 @@ static inline u8 tdx_sysinfo_nr_tdcs_pages(void)
> >  	return tdx_sysinfo->td_ctrl.tdcs_base_size / PAGE_SIZE;
> >  }
> >
> > +static inline u8 tdx_sysinfo_nr_tdcx_pages(void)
> 
> tdx_sysinfo_nr_tdcx_pages() is very similar to
> tdx_sysinfo_nr_tdcs_pages() which is introduced in patch 13.
> 
> It's easy to use either of them in wrong place and hard to
> review, these 2 functions have same signature so compiler
> has no way to prevent us from using them incorrectly.
> TDX 1.5 spec defines these additional pages for TD and vCPU to
> "TDCX" pages, so how about we name them like:
> 
> u8 tdx_sysinfo_nr_td_tdcx_pages(void);
> u8 tdx_sysinfo_nr_vcpu_tdcx_pages(void);
> 
> Above name matchs spec more, and easy to distinguish and review.

Good idea to clarify the naming. For patch 13/25, Nikolay suggested
precalculating the values and dropping the helpers. So we could have
kvm_tdx->nr_td_tdcx_pages and kvm_tdx->nr_vcpu_tdcx_pages following
your naming suggestion.

Regards,

Tony

Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

[Adrian Hunter]

On 13/08/24 11:00, Yuan Yao wrote:
> On Mon, Aug 12, 2024 at 03:48:13PM -0700, Rick Edgecombe wrote:
>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>
>> TD guest vcpu needs TDX specific initialization before running.  Repurpose
>> KVM_MEMORY_ENCRYPT_OP to vcpu-scope, add a new sub-command
>> KVM_TDX_INIT_VCPU, and implement the callback for it.
>>
>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>> ---

<SNIP>

>> @@ -884,6 +930,149 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>>  	return r;
>>  }
>>
>> +/* VMM can pass one 64bit auxiliary data to vcpu via RCX for guest BIOS. */
>> +static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
>> +{

<SNIP>

>> +	if (modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)
>> +		err = tdh_vp_init_apicid(tdx, vcpu_rcx, vcpu->vcpu_id);
>> +	else
>> +		err = tdh_vp_init(tdx, vcpu_rcx);
> 
> This can cause incorrect topology information to guest
> silently:
> 
> A user space VMM uses "-smp 8,threads=4,cores=2" but doesn't
> pass any 0x1f leaf data to KVM, means no 0x1f value to TDX
> module for this TD. The topology TD guest observed is:
> 
> Thread(s) per core:                 2
> Core(s) per socket:                 4
> 
> I suggest to use tdh_vp_init_apicid() only when 0x1f is
> valid. This will disable the 0x1f/0xb topology feature per
> the spec, but leaf 0x1/0x4 still are available to present
> right topology in this example. It presents correct topology
> information to guest if user space VMM doesn't use 0x1f for
> simple topology and run on TDX module w/ FEATURES0_TOPOLOGY.

tdh_vp_init_apicid() passes x2APIC ID to TDH.VP.INIT which
is one of the steps for the TDX Module to support topology
information for the guest i.e. CPUID leaf 0xB and CPUID leaf 0x1F.

If the host VMM does not provide CPUID leaf 0x1F values
(i.e. the values are 0), then the TDX Module will use native
values for both CPUID leaf 0x1F and CPUID leaf 0xB.

To get 0x1F/0xB the guest must also opt-in by setting
TDCS.TD_CTLS.ENUM_TOPOLOGY to 1.  AFAICT currently Linux
does not do that.

In the tdh_vp_init() case, topology information will not be
supported.

If topology information is not supported CPUID leaf 0xB and
CPUID leaf 0x1F will #VE, and a Linux guest will return zeros.

So, yes, it seems like tdh_vp_init_apicid() should only
be called if there is non-zero CPUID leaf 0x1F values provided
by host VMM. e.g. add a helper function

bool tdx_td_enum_topology(struct kvm_cpuid2 *cpuid)
{
	const struct tdx_sys_info_features *modinfo = &tdx_sysinfo->features;
	const struct kvm_cpuid_entry2 *entry;

	if (!(modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM))
		return false;

	entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent, 0x1f, 0);
	if (!entry)
		return false;

	return entry->eax || entry->ebx || entry->ecx || entry->edx;
}

Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

[Edgecombe, Rick P]

On Wed, 2024-10-09 at 18:01 +0300, Adrian Hunter wrote:
> tdh_vp_init_apicid() passes x2APIC ID to TDH.VP.INIT which
> is one of the steps for the TDX Module to support topology
> information for the guest i.e. CPUID leaf 0xB and CPUID leaf 0x1F.
> 
> If the host VMM does not provide CPUID leaf 0x1F values
> (i.e. the values are 0), then the TDX Module will use native
> values for both CPUID leaf 0x1F and CPUID leaf 0xB.
> 
> To get 0x1F/0xB the guest must also opt-in by setting
> TDCS.TD_CTLS.ENUM_TOPOLOGY to 1.  AFAICT currently Linux
> does not do that.
> 
> In the tdh_vp_init() case, topology information will not be
> supported.
> 
> If topology information is not supported CPUID leaf 0xB and
> CPUID leaf 0x1F will #VE, and a Linux guest will return zeros.
> 
> So, yes, it seems like tdh_vp_init_apicid() should only
> be called if there is non-zero CPUID leaf 0x1F values provided
> by host VMM. e.g. add a helper function
> 
> bool tdx_td_enum_topology(struct kvm_cpuid2 *cpuid)
> {
> 	const struct tdx_sys_info_features *modinfo = &tdx_sysinfo->features;
> 	const struct kvm_cpuid_entry2 *entry;
> 
> 	if (!(modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM))
> 		return false;
> 
> 	entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent, 0x1f, 0);
> 	if (!entry)
> 		return false;
> 
> 	return entry->eax || entry->ebx || entry->ecx || entry->edx;
> }

KVM usually leaves it up to userspace to not create nonsensical VMs. So I think
we can skip the check in KVM.

In that case, do you see a need for the vanilla tdh_vp_init() SEAMCALL wrapper?

The TDX module version we need already supports enum_topology, so the code:
	if (modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)
		err = tdh_vp_init_apicid(tdx, vcpu_rcx, vcpu->vcpu_id);
	else
		err = tdh_vp_init(tdx, vcpu_rcx);

The tdh_vp_init() branch shouldn't be hit.

Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

[Xiaoyao Li]

On 10/17/2024 1:42 AM, Edgecombe, Rick P wrote:
> On Wed, 2024-10-09 at 18:01 +0300, Adrian Hunter wrote:
>> tdh_vp_init_apicid() passes x2APIC ID to TDH.VP.INIT which
>> is one of the steps for the TDX Module to support topology
>> information for the guest i.e. CPUID leaf 0xB and CPUID leaf 0x1F.
>>
>> If the host VMM does not provide CPUID leaf 0x1F values
>> (i.e. the values are 0), then the TDX Module will use native
>> values for both CPUID leaf 0x1F and CPUID leaf 0xB.
>>
>> To get 0x1F/0xB the guest must also opt-in by setting
>> TDCS.TD_CTLS.ENUM_TOPOLOGY to 1.  AFAICT currently Linux
>> does not do that.
>>
>> In the tdh_vp_init() case, topology information will not be
>> supported.
>>
>> If topology information is not supported CPUID leaf 0xB and
>> CPUID leaf 0x1F will #VE, and a Linux guest will return zeros.
>>
>> So, yes, it seems like tdh_vp_init_apicid() should only
>> be called if there is non-zero CPUID leaf 0x1F values provided
>> by host VMM. e.g. add a helper function
>>
>> bool tdx_td_enum_topology(struct kvm_cpuid2 *cpuid)
>> {
>> 	const struct tdx_sys_info_features *modinfo = &tdx_sysinfo->features;
>> 	const struct kvm_cpuid_entry2 *entry;
>>
>> 	if (!(modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM))
>> 		return false;
>>
>> 	entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent, 0x1f, 0);
>> 	if (!entry)
>> 		return false;
>>
>> 	return entry->eax || entry->ebx || entry->ecx || entry->edx;
>> }
> 
> KVM usually leaves it up to userspace to not create nonsensical VMs. So I think
> we can skip the check in KVM.

It's not nonsensical unless KVM announces its own requirement for TD 
guest that userspace VMM must provide valid CPUID leaf 0x1f value for 
topology.

It's architectural valid that userspace VMM creates a TD with legacy 
topology, i.e., topology enumerated via CPUID 0x1 and 0x4.

> In that case, do you see a need for the vanilla tdh_vp_init() SEAMCALL wrapper?
> 
> The TDX module version we need already supports enum_topology, so the code:
> 	if (modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)
> 		err = tdh_vp_init_apicid(tdx, vcpu_rcx, vcpu->vcpu_id);
> 	else
> 		err = tdh_vp_init(tdx, vcpu_rcx);
> 
> The tdh_vp_init() branch shouldn't be hit.

We cannot know what version of TDX module user might use thus we cannot 
assume enum_topology is always there unless we make it a hard 
requirement in KVM that TDX fails being enabled when

   !(modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)

Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

[Edgecombe, Rick P]

On Fri, 2024-10-18 at 10:21 +0800, Xiaoyao Li wrote:
> > KVM usually leaves it up to userspace to not create nonsensical VMs. So I
> > think
> > we can skip the check in KVM.
> 
> It's not nonsensical unless KVM announces its own requirement for TD 
> guest that userspace VMM must provide valid CPUID leaf 0x1f value for 
> topology.

How about adding it to the docs?

> 
> It's architectural valid that userspace VMM creates a TD with legacy 
> topology, i.e., topology enumerated via CPUID 0x1 and 0x4.
> 
> > In that case, do you see a need for the vanilla tdh_vp_init() SEAMCALL
> > wrapper?
> > 
> > The TDX module version we need already supports enum_topology, so the code:
> >  	if (modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)
> >  		err = tdh_vp_init_apicid(tdx, vcpu_rcx, vcpu->vcpu_id);
> >  	else
> >  		err = tdh_vp_init(tdx, vcpu_rcx);
> > 
> > The tdh_vp_init() branch shouldn't be hit.
> 
> We cannot know what version of TDX module user might use thus we cannot 
> assume enum_topology is always there unless we make it a hard 
> requirement in KVM that TDX fails being enabled when
> 
>    !(modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)

We will depend on bugs that are fixed in TDX Modules after enum topology, so it
shouldn't be required in the normal case. So I think it would be simpler to add
this tdx_features0 conditional. We can then export one less SEAMCALL and will
have less configurations flows to worry about on the KVM side.

Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

[Xiaoyao Li]

On 10/18/2024 10:20 PM, Edgecombe, Rick P wrote:
> On Fri, 2024-10-18 at 10:21 +0800, Xiaoyao Li wrote:
>>> KVM usually leaves it up to userspace to not create nonsensical VMs. So I
>>> think
>>> we can skip the check in KVM.
>>
>> It's not nonsensical unless KVM announces its own requirement for TD
>> guest that userspace VMM must provide valid CPUID leaf 0x1f value for
>> topology.
> 
> How about adding it to the docs?

OK for me.

>>
>> It's architectural valid that userspace VMM creates a TD with legacy
>> topology, i.e., topology enumerated via CPUID 0x1 and 0x4.
>>
>>> In that case, do you see a need for the vanilla tdh_vp_init() SEAMCALL
>>> wrapper?
>>>
>>> The TDX module version we need already supports enum_topology, so the code:
>>>   	if (modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)
>>>   		err = tdh_vp_init_apicid(tdx, vcpu_rcx, vcpu->vcpu_id);
>>>   	else
>>>   		err = tdh_vp_init(tdx, vcpu_rcx);
>>>
>>> The tdh_vp_init() branch shouldn't be hit.
>>
>> We cannot know what version of TDX module user might use thus we cannot
>> assume enum_topology is always there unless we make it a hard
>> requirement in KVM that TDX fails being enabled when
>>
>>     !(modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)
> 
> We will depend on bugs that are fixed in TDX Modules after enum topology, so it
> shouldn't be required in the normal case. So I think it would be simpler to add
> this tdx_features0 conditional. We can then export one less SEAMCALL and will
> have less configurations flows to worry about on the KVM side.

I'm a little bit confused. what does "add this tdx_feature0 conditional" 
mean?

Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

[Edgecombe, Rick P]

On Mon, 2024-10-21 at 16:35 +0800, Xiaoyao Li wrote:
> > 
> > How about adding it to the docs?
> 
> OK for me.

Can you propose something?

> 
> > > 
> > > It's architectural valid that userspace VMM creates a TD with legacy
> > > topology, i.e., topology enumerated via CPUID 0x1 and 0x4.
> > > 
> > > > In that case, do you see a need for the vanilla tdh_vp_init() SEAMCALL
> > > > wrapper?
> > > > 
> > > > The TDX module version we need already supports enum_topology, so the
> > > > code:
> > > >    	if (modinfo->tdx_features0 &
> > > > MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)
> > > >    		err = tdh_vp_init_apicid(tdx, vcpu_rcx, vcpu->vcpu_id);
> > > >    	else
> > > >    		err = tdh_vp_init(tdx, vcpu_rcx);
> > > > 
> > > > The tdh_vp_init() branch shouldn't be hit.
> > > 
> > > We cannot know what version of TDX module user might use thus we cannot
> > > assume enum_topology is always there unless we make it a hard
> > > requirement in KVM that TDX fails being enabled when
> > > 
> > >      !(modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)
> > 
> > We will depend on bugs that are fixed in TDX Modules after enum topology, so
> > it
> > shouldn't be required in the normal case. So I think it would be simpler to
> > add
> > this tdx_features0 conditional. We can then export one less SEAMCALL and
> > will
> > have less configurations flows to worry about on the KVM side.
> 
> I'm a little bit confused. what does "add this tdx_feature0 conditional" 
> mean?

I was talking about your suggestion to check for
MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM.

Re: [PATCH 03/25] KVM: TDX: Add TDX "architectural" error codes

[Tony Lindgren]

On Tue, Aug 13, 2024 at 02:08:40PM +0800, Binbin Wu wrote:
> On 8/13/2024 6:47 AM, Rick Edgecombe wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > --- a/arch/x86/include/asm/shared/tdx.h
> > +++ b/arch/x86/include/asm/shared/tdx.h
> > @@ -28,6 +28,12 @@
> >   #define TDVMCALL_STATUS_RETRY		1
> > +/*
> > + * TDG.VP.VMCALL Status Codes (returned in R10)
> > + */
> > +#define TDVMCALL_SUCCESS		0x0000000000000000ULL
> > +#define TDVMCALL_INVALID_OPERAND	0x8000000000000000ULL
> > +
> TDX guest code has already defined/uses "TDVMCALL_STATUS_RETRY", which is
> one
> of the TDG.VP.VMCALL Status Codes.
> 
> IMHO, the style of the macros should be unified.
> How about using TDVMALL_STATUS_* for TDG.VP.VMCALL Status Codes?
> 
> +/*
> + * TDG.VP.VMCALL Status Codes (returned in R10)
> + */
> +#define TDVMCALL_STATUS_SUCCESS 0x0000000000000000ULL
> -#define TDVMCALL_STATUS_RETRY                  1
> +#define TDVMCALL_STATUS_RETRY 0x0000000000000001ULL
> +#define TDVMCALL_STATUS_INVALID_OPERAND 0x8000000000000000ULL

Makes sense as they are the hardware status codes.

Regards,

Tony

Re: [PATCH 03/25] KVM: TDX: Add TDX "architectural" error codes

[Tony Lindgren]

On Thu, Aug 29, 2024 at 08:24:25AM +0300, Tony Lindgren wrote:
> On Tue, Aug 13, 2024 at 02:08:40PM +0800, Binbin Wu wrote:
> > On 8/13/2024 6:47 AM, Rick Edgecombe wrote:
> > > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > > --- a/arch/x86/include/asm/shared/tdx.h
> > > +++ b/arch/x86/include/asm/shared/tdx.h
> > > @@ -28,6 +28,12 @@
> > >   #define TDVMCALL_STATUS_RETRY		1
> > > +/*
> > > + * TDG.VP.VMCALL Status Codes (returned in R10)
> > > + */
> > > +#define TDVMCALL_SUCCESS		0x0000000000000000ULL
> > > +#define TDVMCALL_INVALID_OPERAND	0x8000000000000000ULL
> > > +
> > TDX guest code has already defined/uses "TDVMCALL_STATUS_RETRY", which is
> > one
> > of the TDG.VP.VMCALL Status Codes.
> > 
> > IMHO, the style of the macros should be unified.
> > How about using TDVMALL_STATUS_* for TDG.VP.VMCALL Status Codes?
> > 
> > +/*
> > + * TDG.VP.VMCALL Status Codes (returned in R10)
> > + */
> > +#define TDVMCALL_STATUS_SUCCESS 0x0000000000000000ULL
> > -#define TDVMCALL_STATUS_RETRY                  1
> > +#define TDVMCALL_STATUS_RETRY 0x0000000000000001ULL
> > +#define TDVMCALL_STATUS_INVALID_OPERAND 0x8000000000000000ULL
> 
> Makes sense as they are the hardware status codes.

I'll do a patch against the CoCo queue for the TDVMCALL_STATUS prefix FYI.

Regards,

Tony

Re: [PATCH 03/25] KVM: TDX: Add TDX "architectural" error codes

[Paolo Bonzini]

On 8/30/24 07:52, Tony Lindgren wrote:
>>> +#define TDVMCALL_STATUS_SUCCESS 0x0000000000000000ULL
>>> -#define TDVMCALL_STATUS_RETRY                  1
>>> +#define TDVMCALL_STATUS_RETRY 0x0000000000000001ULL
>>> +#define TDVMCALL_STATUS_INVALID_OPERAND 0x8000000000000000ULL
>> Makes sense as they are the hardware status codes.
> I'll do a patch against the CoCo queue for the TDVMCALL_STATUS prefix FYI.

Just squash it in the next version of this series.

Paolo

Re: [PATCH 03/25] KVM: TDX: Add TDX "architectural" error codes

[Tony Lindgren]

On Tue, Sep 10, 2024 at 06:22:52PM +0200, Paolo Bonzini wrote:
> On 8/30/24 07:52, Tony Lindgren wrote:
> > > > +#define TDVMCALL_STATUS_SUCCESS 0x0000000000000000ULL
> > > > -#define TDVMCALL_STATUS_RETRY                  1
> > > > +#define TDVMCALL_STATUS_RETRY 0x0000000000000001ULL
> > > > +#define TDVMCALL_STATUS_INVALID_OPERAND 0x8000000000000000ULL
> > > Makes sense as they are the hardware status codes.
> > I'll do a patch against the CoCo queue for the TDVMCALL_STATUS prefix FYI.
> 
> Just squash it in the next version of this series.

Sure no problem.

Regards,

Tony

Re: [PATCH 17/25] KVM: TDX: create/free TDX vcpu structure

[Tony Lindgren]

On Mon, Aug 19, 2024 at 07:46:13PM +0300, Nikolay Borisov wrote:
> On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -377,6 +377,47 @@ int tdx_vm_init(struct kvm *kvm)
> >   	return 0;
> >   }
> > +int tdx_vcpu_create(struct kvm_vcpu *vcpu)
> > +{
> > +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> > +
> > +	/* TDX only supports x2APIC, which requires an in-kernel local APIC. */
> > +	if (!vcpu->arch.apic)
> > +		return -EINVAL;
> 
> nit: Use kvm_apic_present()

Thanks will do a patch for this.

Regards,

Tony

Re: [PATCH 17/25] KVM: TDX: create/free TDX vcpu structure

[Yan Zhao]

On Mon, Aug 12, 2024 at 03:48:12PM -0700, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> +int tdx_vcpu_create(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
As explained in [1], could we add a check of TD initialization status here?

+       if (!kvm_tdx->initialized)
+               return -EIO;
+

[1] https://lore.kernel.org/kvm/ZtAU7FIV2Xkw+L3O@yzhao56-desk.sh.intel.com/

> +
> +	/* TDX only supports x2APIC, which requires an in-kernel local APIC. */
> +	if (!vcpu->arch.apic)
> +		return -EINVAL;
> +
> +	fpstate_set_confidential(&vcpu->arch.guest_fpu);
> +
> +	vcpu->arch.efer = EFER_SCE | EFER_LME | EFER_LMA | EFER_NX;
> +
> +	vcpu->arch.cr0_guest_owned_bits = -1ul;
> +	vcpu->arch.cr4_guest_owned_bits = -1ul;
> +
> +	vcpu->arch.tsc_offset = kvm_tdx->tsc_offset;
> +	vcpu->arch.l1_tsc_offset = vcpu->arch.tsc_offset;
> +	vcpu->arch.guest_state_protected =
> +		!(to_kvm_tdx(vcpu->kvm)->attributes & TDX_TD_ATTR_DEBUG);
> +
> +	if ((kvm_tdx->xfam & XFEATURE_MASK_XTILE) == XFEATURE_MASK_XTILE)
> +		vcpu->arch.xfd_no_write_intercept = true;
> +
> +	return 0;
> +}
> +

Re: [PATCH 17/25] KVM: TDX: create/free TDX vcpu structure

[Tony Lindgren]

On Tue, Aug 13, 2024 at 05:15:28PM +0800, Binbin Wu wrote:
> On 8/13/2024 6:48 AM, Rick Edgecombe wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > +void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> > +{
> > +
> > +	/* Ignore INIT silently because TDX doesn't support INIT event. */
> > +	if (init_event)
> > +		return;
> > +
> > +	/* This is stub for now. More logic will come here. */
> > +}
> > +
> For TDX, it actually doesn't do any thing meaningful in vcpu reset.
> Maybe we can drop the helper and move the comments to vt_vcpu_reset()?

Good point, will do a patch to drop tdx_vcpu_reset().

> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -488,6 +488,7 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> >   	kvm_recalculate_apic_map(vcpu->kvm);
> >   	return 0;
> >   }
> > +EXPORT_SYMBOL_GPL(kvm_set_apic_base);
> >   /*
> >    * Handle a fault on a hardware virtualization (VMX or SVM) instruction.
> > @@ -12630,6 +12631,7 @@ bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu)
> >   {
> >   	return vcpu->kvm->arch.bsp_vcpu_id == vcpu->vcpu_id;
> >   }
> > +EXPORT_SYMBOL_GPL(kvm_vcpu_is_reset_bsp);
> >   bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu)
> >   {
> 
> kvm_set_apic_base() and kvm_vcpu_is_reset_bsp() is not used in
> this patch. The symbol export should move to the next patch, which
> uses them.

Yes that should have been in the following patch.

Regards,

Tony

Re: [PATCH 02/25] KVM: TDX: Define TDX architectural definitions

[Xiaoyao Li]

On 8/13/2024 6:47 AM, Rick Edgecombe wrote:
> +/*
> + * TD_PARAMS is provided as an input to TDH_MNG_INIT, the size of which is 1024B.
> + */
> +struct td_params {
> +	u64 attributes;
> +	u64 xfam;
> +	u16 max_vcpus;
> +	u8 reserved0[6];
> +
> +	u64 eptp_controls;
> +	u64 exec_controls;

TDX 1.5 renames 'exec_controls' to 'config_flags', maybe we need update 
it to match TDX 1.5 since the minimum supported TDX module of linux 
starts from 1.5.

Besides, TDX 1.5 defines more fields that was reserved in TDX 1.0, but 
most of them are not used by current TDX enabling patches. If we update 
TD_PARAMS to match with TDX 1.5, should we add them as well?

This leads to another topic that defining all the TDX structure in this 
patch seems unfriendly for review. It seems better to put the 
introduction of definition and its user in a single patch.

> +	u16 tsc_frequency;
> +	u8  reserved1[38];
> +
> +	u64 mrconfigid[6];
> +	u64 mrowner[6];
> +	u64 mrownerconfig[6];
> +	u64 reserved2[4];
> +
> +	union {
> +		DECLARE_FLEX_ARRAY(struct tdx_cpuid_value, cpuid_values);
> +		u8 reserved3[768];
> +	};
> +} __packed __aligned(1024);

Re: [PATCH 02/25] KVM: TDX: Define TDX architectural definitions

[Edgecombe, Rick P]

On Thu, 2024-08-29 at 21:25 +0800, Xiaoyao Li wrote:
> On 8/13/2024 6:47 AM, Rick Edgecombe wrote:
> > +/*
> > + * TD_PARAMS is provided as an input to TDH_MNG_INIT, the size of which is
> > 1024B.
> > + */
> > +struct td_params {
> > +       u64 attributes;
> > +       u64 xfam;
> > +       u16 max_vcpus;
> > +       u8 reserved0[6];
> > +
> > +       u64 eptp_controls;
> > +       u64 exec_controls;
> 
> TDX 1.5 renames 'exec_controls' to 'config_flags', maybe we need update 
> it to match TDX 1.5 since the minimum supported TDX module of linux 
> starts from 1.5.

Agreed.

> 
> Besides, TDX 1.5 defines more fields that was reserved in TDX 1.0, but 
> most of them are not used by current TDX enabling patches. If we update 
> TD_PARAMS to match with TDX 1.5, should we add them as well?

You mean config_flags or supported "features0"? For config_flags, it seems just
one is missing. I don't think we need to add it.

> 
> This leads to another topic that defining all the TDX structure in this 
> patch seems unfriendly for review. It seems better to put the 
> introduction of definition and its user in a single patch.

Yea.

Re: [PATCH 02/25] KVM: TDX: Define TDX architectural definitions

[Xiaoyao Li]

On 8/30/2024 3:46 AM, Edgecombe, Rick P wrote:
> On Thu, 2024-08-29 at 21:25 +0800, Xiaoyao Li wrote:
>> On 8/13/2024 6:47 AM, Rick Edgecombe wrote:
>>> +/*
>>> + * TD_PARAMS is provided as an input to TDH_MNG_INIT, the size of which is
>>> 1024B.
>>> + */
>>> +struct td_params {
>>> +       u64 attributes;
>>> +       u64 xfam;
>>> +       u16 max_vcpus;
>>> +       u8 reserved0[6];
>>> +
>>> +       u64 eptp_controls;
>>> +       u64 exec_controls;
>>
>> TDX 1.5 renames 'exec_controls' to 'config_flags', maybe we need update
>> it to match TDX 1.5 since the minimum supported TDX module of linux
>> starts from 1.5.
> 
> Agreed.
> 
>>
>> Besides, TDX 1.5 defines more fields that was reserved in TDX 1.0, but
>> most of them are not used by current TDX enabling patches. If we update
>> TD_PARAMS to match with TDX 1.5, should we add them as well?
> 
> You mean config_flags or supported "features0"? For config_flags, it seems just
> one is missing. I don't think we need to add it.

No. I meant NUM_L2_VMS, MSR_CONFIG_CTLS, IA32_ARCH_CAPABILITIES_CONFIG, 
MRCONFIGSVN and MROWNERCONFIGSVN introduced in TD_PARAMS from TDX 1.5.

Only MSR_CONFIG_CTLS and IA32_ARCH_CAPABILITIES_CONFIG likely need 
enabling for now since they relates to MSR_IA32_ARCH_CAPABILITIES 
virtualization of TDs.

>>
>> This leads to another topic that defining all the TDX structure in this
>> patch seems unfriendly for review. It seems better to put the
>> introduction of definition and its user in a single patch.
> 
> Yea.

Re: [PATCH 02/25] KVM: TDX: Define TDX architectural definitions

[Tony Lindgren]

On Fri, Aug 30, 2024 at 09:29:19AM +0800, Xiaoyao Li wrote:
> On 8/30/2024 3:46 AM, Edgecombe, Rick P wrote:
> > On Thu, 2024-08-29 at 21:25 +0800, Xiaoyao Li wrote:
> > > On 8/13/2024 6:47 AM, Rick Edgecombe wrote:
> > > > +/*
> > > > + * TD_PARAMS is provided as an input to TDH_MNG_INIT, the size of which is
> > > > 1024B.
> > > > + */
> > > > +struct td_params {
> > > > +       u64 attributes;
> > > > +       u64 xfam;
> > > > +       u16 max_vcpus;
> > > > +       u8 reserved0[6];
> > > > +
> > > > +       u64 eptp_controls;
> > > > +       u64 exec_controls;
> > > 
> > > TDX 1.5 renames 'exec_controls' to 'config_flags', maybe we need update
> > > it to match TDX 1.5 since the minimum supported TDX module of linux
> > > starts from 1.5.
> > 
> > Agreed.

I'm doing a patch for this FYI.
 
> > > Besides, TDX 1.5 defines more fields that was reserved in TDX 1.0, but
> > > most of them are not used by current TDX enabling patches. If we update
> > > TD_PARAMS to match with TDX 1.5, should we add them as well?
> > 
> > You mean config_flags or supported "features0"? For config_flags, it seems just
> > one is missing. I don't think we need to add it.
> 
> No. I meant NUM_L2_VMS, MSR_CONFIG_CTLS, IA32_ARCH_CAPABILITIES_CONFIG,
> MRCONFIGSVN and MROWNERCONFIGSVN introduced in TD_PARAMS from TDX 1.5.
> 
> Only MSR_CONFIG_CTLS and IA32_ARCH_CAPABILITIES_CONFIG likely need enabling
> for now since they relates to MSR_IA32_ARCH_CAPABILITIES virtualization of
> TDs.

Seems these changes can be separate additional patches.

Regards,

Tony

Re: [PATCH 02/25] KVM: TDX: Define TDX architectural definitions

[Paolo Bonzini]

On 8/29/24 21:46, Edgecombe, Rick P wrote:
>> This leads to another topic that defining all the TDX structure in this
>> patch seems unfriendly for review. It seems better to put the
>> introduction of definition and its user in a single patch.
>
> Yea.

I don't know, it's easier to check a single patch against the manual.  I 
don't have any objection to leaving everything here instead of 
scattering it over multiple patches, in fact I think I prefer it.

Paolo

Re: [PATCH 02/25] KVM: TDX: Define TDX architectural definitions

[Sean Christopherson]

On Tue, Sep 10, 2024, Paolo Bonzini wrote:
> On 8/29/24 21:46, Edgecombe, Rick P wrote:
> > > This leads to another topic that defining all the TDX structure in this
> > > patch seems unfriendly for review. It seems better to put the
> > > introduction of definition and its user in a single patch.
> > 
> > Yea.
> 
> I don't know, it's easier to check a single patch against the manual.  I
> don't have any objection to leaving everything here instead of scattering it
> over multiple patches, in fact I think I prefer it.

+1.  There is so much to understand with TDX that trying to provide some form of
temporal locality between architectural definitions and the first code to use a
given definition seems futile/pointless.

Re: [PATCH 05/25] KVM: TDX: Add helper functions to print TDX SEAMCALL error

[Tony Lindgren]

On Tue, Aug 13, 2024 at 05:31:31PM -0700, Isaku Yamahata wrote:
> On Wed, Aug 14, 2024 at 10:34:11AM +1200,
> "Huang, Kai" <kai.huang@intel.com> wrote:
> 
> > 
> > > > +#define pr_tdx_error(__fn, __err)	\
> > > > +	pr_err_ratelimited("SEAMCALL %s failed: 0x%llx\n", #__fn, __err)
> > > > +
> > > > +#define pr_tdx_error_N(__fn, __err, __fmt, ...)		\
> > > > +	pr_err_ratelimited("SEAMCALL %s failed: 0x%llx, " __fmt, #__fn, __err,  __VA_ARGS__)
> > > 
> > > Stringify in the inner macro results in expansion of __fn.  It means value
> > > itself, not symbolic string.  Stringify should be in the outer macro.
> > > "SEAMCALL 7 failed" vs "SEAMCALL TDH_MEM_RANGE_BLOCK failed"
> > > 
> > > #define __pr_tdx_error_N(__fn_str, __err, __fmt, ...)           \
> > >          pr_err_ratelimited("SEAMCALL " __fn_str " failed: 0x%llx, " __fmt,  __err,  __VA_ARGS__)
> > > 
> > > #define pr_tdx_error_N(__fn, __err, __fmt, ...)         \
> > >          __pr_tdx_error_N(#__fn, __err, __fmt, __VA_ARGS__)
> > > 
> > > #define pr_tdx_error_1(__fn, __err, __rcx)              \
> > >          __pr_tdx_error_N(#__fn, __err, "rcx 0x%llx\n", __rcx)
> > > 
> > > #define pr_tdx_error_2(__fn, __err, __rcx, __rdx)       \
> > >          __pr_tdx_error_N(#__fn, __err, "rcx 0x%llx, rdx 0x%llx\n", __rcx, __rdx)
> > > 
> > > #define pr_tdx_error_3(__fn, __err, __rcx, __rdx, __r8) \
> > >          __pr_tdx_error_N(#__fn, __err, "rcx 0x%llx, rdx 0x%llx, r8 0x%llx\n", __rcx, __rdx, __r8)
> > > 
> > 
> > You are right.  Thanks for catching this!
> > 
> > The above code looks good to me, except we don't need pr_tdx_error_N()
> > anymore.
> > 
> > I think we can just replace the old pr_tdx_error_N() with your
> > __pr_tdx_error_N().
> 
> Agreed, we don't have the direct user of pr_tdx_error_N().

I'll do a patch for these changes.

Tony

Re: [PATCH 08/25] KVM: TDX: Add place holder for TDX VM specific mem_enc_op ioctl

[Tony Lindgren]

On Tue, Aug 13, 2024 at 09:37:07AM -0700, Isaku Yamahata wrote:
> On Mon, Aug 12, 2024 at 03:48:03PM -0700,
> Rick Edgecombe <rick.p.edgecombe@intel.com> wrote:
> 
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index b1c885ce8c9c..de14e80d8f3a 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -2,6 +2,7 @@
> >  #include <linux/cpu.h>
> >  #include <asm/tdx.h>
> >  #include "capabilities.h"
> > +#include "x86_ops.h"
> >  #include "tdx.h"
> >  
> >  #undef pr_fmt
> > @@ -29,6 +30,37 @@ static void __used tdx_guest_keyid_free(int keyid)
> >  	ida_free(&tdx_guest_keyid_pool, keyid);
> >  }
> >  
> > +int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
> > +{
> > +	struct kvm_tdx_cmd tdx_cmd;
> > +	int r;
> > +
> > +	if (copy_from_user(&tdx_cmd, argp, sizeof(struct kvm_tdx_cmd)))
> > +		return -EFAULT;
> > +
> > +	/*
> > +	 * Userspace should never set @error. It is used to fill
> 
> nitpick: @hw_error

Thanks will do a patch for this.

Tony

Re: [PATCH 09/25] KVM: TDX: Get system-wide info about TDX module on initialization

[Tony Lindgren]

On Tue, Aug 13, 2024 at 02:47:17PM +0800, Binbin Wu wrote:
> On 8/13/2024 6:48 AM, Rick Edgecombe wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -3,6 +3,7 @@
> >   #include <asm/tdx.h>
> >   #include "capabilities.h"
> >   #include "x86_ops.h"
> > +#include "mmu.h"
> 
> Is this needed by this patch?

Needed but looks like it should have been introduced only in patch
"KVM: x86: Introduce KVM_TDX_GET_CPUID" for kvm_gfn_direct_bits().

Regards,

Tony

Re: [PATCH 09/25] KVM: TDX: Get system-wide info about TDX module on initialization

[Tony Lindgren]

On Thu, Aug 15, 2024 at 03:59:26PM +0800, Xu Yilun wrote:
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -3,6 +3,7 @@
> >  #include <asm/tdx.h>
> >  #include "capabilities.h"
> >  #include "x86_ops.h"
> > +#include "mmu.h"
> 
> Is the header file still needed?

It's needed for kvm_gfn_direct_bits(), but should have been added in a
later patch.

> > +static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
> > +{
> > +	const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
> > +	struct kvm_tdx_capabilities __user *user_caps;
> > +	struct kvm_tdx_capabilities *caps = NULL;
> > +	int i, ret = 0;
> > +
> > +	/* flags is reserved for future use */
> > +	if (cmd->flags)
> > +		return -EINVAL;
> > +
> > +	caps = kmalloc(sizeof(*caps), GFP_KERNEL);
> > +	if (!caps)
> > +		return -ENOMEM;
> > +
> > +	user_caps = u64_to_user_ptr(cmd->data);
> > +	if (copy_from_user(caps, user_caps, sizeof(*caps))) {
> > +		ret = -EFAULT;
> > +		goto out;
> > +	}
> > +
> > +	if (caps->nr_cpuid_configs < td_conf->num_cpuid_config) {
> > +		ret = -E2BIG;
> 
> How about output the correct num_cpuid_config to userspace as a hint,
> to avoid user blindly retries.

Hmm do we want to add also positive numbers for errors for this function?

> > +	for (i = 0; i < td_conf->num_cpuid_config; i++) {
> > +		struct kvm_tdx_cpuid_config cpuid_config = {
> > +			.leaf = (u32)td_conf->cpuid_config_leaves[i],
> > +			.sub_leaf = td_conf->cpuid_config_leaves[i] >> 32,
> > +			.eax = (u32)td_conf->cpuid_config_values[i].eax_ebx,
> > +			.ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32,
> > +			.ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx,
> > +			.edx = td_conf->cpuid_config_values[i].ecx_edx >> 32,
> > +		};
> > +
> > +		if (copy_to_user(&(user_caps->cpuid_configs[i]), &cpuid_config,
>                                   ^                           ^
> 
> I think the brackets could be removed.
> 
> > +					sizeof(struct kvm_tdx_cpuid_config))) {
> 
> sizeof(cpuid_config) could be better.

Looks like these both already changed in a later patch
"KVM: TDX: Report kvm_tdx_caps in KVM_TDX_CAPABILITIES".

Regards,

Tony

Re: [PATCH 09/25] KVM: TDX: Get system-wide info about TDX module on initialization

[Xu Yilun]

> > > +static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
> > > +{
> > > +	const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
> > > +	struct kvm_tdx_capabilities __user *user_caps;
> > > +	struct kvm_tdx_capabilities *caps = NULL;
> > > +	int i, ret = 0;
> > > +
> > > +	/* flags is reserved for future use */
> > > +	if (cmd->flags)
> > > +		return -EINVAL;
> > > +
> > > +	caps = kmalloc(sizeof(*caps), GFP_KERNEL);
> > > +	if (!caps)
> > > +		return -ENOMEM;
> > > +
> > > +	user_caps = u64_to_user_ptr(cmd->data);
> > > +	if (copy_from_user(caps, user_caps, sizeof(*caps))) {
> > > +		ret = -EFAULT;
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (caps->nr_cpuid_configs < td_conf->num_cpuid_config) {
> > > +		ret = -E2BIG;
> > 
> > How about output the correct num_cpuid_config to userspace as a hint,
> > to avoid user blindly retries.
> 
> Hmm do we want to add also positive numbers for errors for this function?

No. I think maybe update the user_caps->nr_cpuid_configs when returning
-E2BIG. Similar to KVM_GET_MSR_INDEX_LIST.

Thanks,
Yilun

Re: [PATCH 09/25] KVM: TDX: Get system-wide info about TDX module on initialization

[Tony Lindgren]

On Mon, Sep 02, 2024 at 09:25:00AM +0800, Xu Yilun wrote:
> > > > +static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
> > > > +{
> > > > +	const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
> > > > +	struct kvm_tdx_capabilities __user *user_caps;
> > > > +	struct kvm_tdx_capabilities *caps = NULL;
> > > > +	int i, ret = 0;
> > > > +
> > > > +	/* flags is reserved for future use */
> > > > +	if (cmd->flags)
> > > > +		return -EINVAL;
> > > > +
> > > > +	caps = kmalloc(sizeof(*caps), GFP_KERNEL);
> > > > +	if (!caps)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	user_caps = u64_to_user_ptr(cmd->data);
> > > > +	if (copy_from_user(caps, user_caps, sizeof(*caps))) {
> > > > +		ret = -EFAULT;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	if (caps->nr_cpuid_configs < td_conf->num_cpuid_config) {
> > > > +		ret = -E2BIG;
> > > 
> > > How about output the correct num_cpuid_config to userspace as a hint,
> > > to avoid user blindly retries.
> > 
> > Hmm do we want to add also positive numbers for errors for this function?
> 
> No. I think maybe update the user_caps->nr_cpuid_configs when returning
> -E2BIG. Similar to KVM_GET_MSR_INDEX_LIST.

OK thanks for clarifying, yes that sounds nice.

Regards,

Tony

Re: [PATCH 06/25] x86/virt/tdx: Export TDX KeyID information

[Dave Hansen]

On 8/12/24 15:48, Rick Edgecombe wrote:
> Each TDX guest has a root control structure called "Trust Domain
> Root" (TDR).  Unlike the rest of the TDX guest, the TDR is protected
> by the TDX global KeyID.  When tearing down the TDR, KVM will need to
> pass the TDX global KeyID explicitly to the TDX module to flush cache
> associated to the TDR.

What does that end up looking like?

In other words, should we export the global KeyID, or export a function
to do the flush and then never actually expose the KeyID?

> -static u32 tdx_global_keyid __ro_after_init;
> -static u32 tdx_guest_keyid_start __ro_after_init;
> -static u32 tdx_nr_guest_keyids __ro_after_init;
> +u32 tdx_global_keyid __ro_after_init;
> +EXPORT_SYMBOL_GPL(tdx_global_keyid);
> +
> +u32 tdx_guest_keyid_start __ro_after_init;
> +EXPORT_SYMBOL_GPL(tdx_guest_keyid_start);
> +
> +u32 tdx_nr_guest_keyids __ro_after_init;
> +EXPORT_SYMBOL_GPL(tdx_nr_guest_keyids);

I know the KVM folks aren't maniacs that will start writing to these or
anything.

But, in general, just exporting global variables isn't super nice.  If
these are being used to set up the key allocator, I'd kinda just rather
that the allocator be in core code and have its alloc/free functions
exported.

Re: [PATCH 06/25] x86/virt/tdx: Export TDX KeyID information

[Edgecombe, Rick P]

On Fri, 2024-08-30 at 11:45 -0700, Dave Hansen wrote:
> On 8/12/24 15:48, Rick Edgecombe wrote:
> > Each TDX guest has a root control structure called "Trust Domain
> > Root" (TDR).  Unlike the rest of the TDX guest, the TDR is protected
> > by the TDX global KeyID.  When tearing down the TDR, KVM will need to
> > pass the TDX global KeyID explicitly to the TDX module to flush cache
> > associated to the TDR.
> 
> What does that end up looking like?

The global key id callers looks like:
	err = tdh_phymem_page_wbinvd(set_hkid_to_hpa(kvm_tdx->tdr_pa,
						     tdx_global_keyid));
	if (KVM_BUG_ON(err, kvm)) {
		pr_tdx_error(TDH_PHYMEM_PAGE_WBINVD, err);
		return;
	}

The TD keyid callers looks like:
	hpa_with_hkid = set_hkid_to_hpa(hpa, (u16)kvm_tdx->hkid);
	do {
		/*
		 * TDX_OPERAND_BUSY can happen on locking PAMT entry.  Because
		 * this page was removed above, other thread shouldn't be
		 * repeatedly operating on this page.  Just retry loop.
		 */
		err = tdh_phymem_page_wbinvd(hpa_with_hkid);
	} while (unlikely(err == (TDX_OPERAND_BUSY | TDX_OPERAND_ID_RCX)));

> 
> In other words, should we export the global KeyID, or export a function
> to do the flush and then never actually expose the KeyID?

We could split it into two helpers if we wanted to remove the export of
tdx_global_keyid. One for global key id and one that only takes TD range key
ids. Adding more layers is a downside.

Separate from Dave's question, I wonder if we should open code set_hkid_to_hpa()
inside tdh_phymem_page_wbinvd(). The signature could change to
tdh_phymem_page_wbinvd(hpa_t pa, u16 hkid). set_hkid_to_hpa() is very
lightweight, so I don't think doing it outside the loop is much gain. It makes
the code cleaner.

> 
> > -static u32 tdx_global_keyid __ro_after_init;
> > -static u32 tdx_guest_keyid_start __ro_after_init;
> > -static u32 tdx_nr_guest_keyids __ro_after_init;
> > +u32 tdx_global_keyid __ro_after_init;
> > +EXPORT_SYMBOL_GPL(tdx_global_keyid);
> > +
> > +u32 tdx_guest_keyid_start __ro_after_init;
> > +EXPORT_SYMBOL_GPL(tdx_guest_keyid_start);
> > +
> > +u32 tdx_nr_guest_keyids __ro_after_init;
> > +EXPORT_SYMBOL_GPL(tdx_nr_guest_keyids);
> 
> I know the KVM folks aren't maniacs that will start writing to these or
> anything.

Yea. ro_after_init would stop most mischief as well.

> 
> But, in general, just exporting global variables isn't super nice.  If
> these are being used to set up the key allocator, I'd kinda just rather
> that the allocator be in core code and have its alloc/free functions
> exported.
> 

Makes sense. We could remove tdx_guest_keyid_start/tdx_nr_guest_keyids then in
any case. But if we want to remove tdx_global_keyid too, we could add a
tdh_phymem_page_wbinvd_global_keyid(void). I'm split on that one, but I'd lean
towards doing it.

Re: [PATCH 06/25] x86/virt/tdx: Export TDX KeyID information

[Dave Hansen]

On 8/30/24 12:16, Edgecombe, Rick P wrote:
...>> In other words, should we export the global KeyID, or export a
function
>> to do the flush and then never actually expose the KeyID?
> 
> We could split it into two helpers if we wanted to remove the export of
> tdx_global_keyid. One for global key id and one that only takes TD range key
> ids. Adding more layers is a downside.

I do like the idea of exporting a couple of helpers that are quite hard
to misuse instead of exporting the variable.

> Separate from Dave's question, I wonder if we should open code set_hkid_to_hpa()
> inside tdh_phymem_page_wbinvd(). The signature could change to
> tdh_phymem_page_wbinvd(hpa_t pa, u16 hkid). set_hkid_to_hpa() is very
> lightweight, so I don't think doing it outside the loop is much gain. It makes
> the code cleaner.

Yeah, do what's cleanest.  This is all super cold code.

Re: [PATCH 06/25] x86/virt/tdx: Export TDX KeyID information

[Paolo Bonzini]

On 8/30/24 23:18, Dave Hansen wrote:
> I do like the idea of exporting a couple of helpers that are quite hard
> to misuse instead of exporting the variable.

Yeah, that's nicer.

Paolo

>> Separate from Dave's question, I wonder if we should open code set_hkid_to_hpa()
>> inside tdh_phymem_page_wbinvd(). The signature could change to
>> tdh_phymem_page_wbinvd(hpa_t pa, u16 hkid). set_hkid_to_hpa() is very
>> lightweight, so I don't think doing it outside the loop is much gain. It makes
>> the code cleaner.
> Yeah, do what's cleanest.  This is all super cold code.

Re: [PATCH 13/25] KVM: TDX: create/destroy VM structure

[Tony Lindgren]

On Fri, Aug 16, 2024 at 03:31:46PM +0800, Xu Yilun wrote:
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 84cd9b4f90b5..a0954c3928e2 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -5,6 +5,7 @@
> >  #include "x86_ops.h"
> >  #include "mmu.h"
> >  #include "tdx.h"
> > +#include "tdx_ops.h"
> 
> I remember patch #4 says "C files should never include this header
> directly"
> 
>   +++ b/arch/x86/kvm/vmx/tdx_ops.h
>   @@ -0,0 +1,387 @@
>   +/* SPDX-License-Identifier: GPL-2.0 */
>   +/*
>   + * Constants/data definitions for TDX SEAMCALLs
>   + *
>   + * This file is included by "tdx.h" after declarations of 'struct
>   + * kvm_tdx' and 'struct vcpu_tdx'.  C file should never include
>   + * this header directly.
>   + */
> 
> maybe just remove it?

Yes doing patch to drop it thanks.

Tony

Re: [PATCH 13/25] KVM: TDX: create/destroy VM structure

[Tony Lindgren]

On Wed, Aug 21, 2024 at 04:52:14PM +0000, Edgecombe, Rick P wrote:
> On Wed, 2024-08-21 at 08:39 +0300, Tony Lindgren wrote:
> > > Hmm, we would have to make SEAMCALLs to spin on that lock, where as mutexes
> > > can
> > > sleep. I suspect that is where it came from. But we are trying to make the
> > > code
> > > simple and obviously correct and add optimizations later. This might fit
> > > that
> > > pattern, especially since it is just used during VM creation and teardown.
> > 
> > For handling the busy retries for SEAMCALL callers, we could just use
> > iopoll.h read_poll_timeout(). I think it can handle toggling the resume
> > bit while looping, need to test that though. See for example the
> > smp_func_do_phymem_cache_wb() for toggling the resume variable.
> 
> Nice. It seems worth trying to me.

To recap on this, using iopoll for smp_func_do_phymem_cache_wb() would look like:

static void smp_func_do_phymem_cache_wb(void *unused)
{
	u64 status = 0;
	int err;

	err = read_poll_timeout_atomic(tdh_phymem_cache_wb, status,
				       status != TDX_INTERRUPTED_RESUMABLE,
				       1, 1000, 0, !!status);
	if (WARN_ON_ONCE(err)) {
		pr_err("TDH_PHYMEM_CACHE_WB timed out: 0x%llx\n", status);
		return;
	}
	...
}

For the retry flag toggling with the !!status, I think it's best to add a TDX
specific tdx_read_poll_timeout_atomic() macro.

Regards,

Tony

Re: [PATCH 13/25] KVM: TDX: create/destroy VM structure

[Tony Lindgren]

On Mon, Aug 19, 2024 at 06:09:06PM +0300, Nikolay Borisov wrote:
> On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > +static u64 ____tdx_reclaim_page(hpa_t pa, u64 *rcx, u64 *rdx, u64 *r8)
> 
> Just inline this into its sole caller. Yes each specific function is rather
> small but if you have to go through several levels of indirection then
> there's no point in splitting it...

Makes sense, will do a patch for this.

> > +static inline u8 tdx_sysinfo_nr_tdcs_pages(void)
> > +{
> > +	return tdx_sysinfo->td_ctrl.tdcs_base_size / PAGE_SIZE;
> > +}
> 
> Just add a nr_tdcs_pages to struct tdx_sysinfo_td_ctrl and claculate this
> value in get_tdx_td_ctrl() rather than having this long-named non-sense.
> This value can't be calculated at compiletime anyway.

The struct tdx_sysinfo_td_ctrl is defined in the TDX module API json files.
Probably best to add nr_tdcs_pages to struct kvm_tdx.

> > +void tdx_vm_free(struct kvm *kvm)
> > +{
> > +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > +	u64 err;
> > +	int i;
> > +
> > +	/*
> > +	 * tdx_mmu_release_hkid() failed to reclaim HKID.  Something went wrong
> > +	 * heavily with TDX module.  Give up freeing TD pages.  As the function
> > +	 * already warned, don't warn it again.
> > +	 */
> > +	if (is_hkid_assigned(kvm_tdx))
> > +		return;
> > +
> > +	if (kvm_tdx->tdcs_pa) {
> > +		for (i = 0; i < tdx_sysinfo_nr_tdcs_pages(); i++) {
> > +			if (!kvm_tdx->tdcs_pa[i])
> > +				continue;
> > +
> > +			tdx_reclaim_control_page(kvm_tdx->tdcs_pa[i]);
> > +		}
> > +		kfree(kvm_tdx->tdcs_pa);
> > +		kvm_tdx->tdcs_pa = NULL;
> > +	}
> > +
> > +	if (!kvm_tdx->tdr_pa)
> > +		return;
> 
> Use is_td_created() helper. Also isn't this check redundant since you've
> already executed is_hkid_assigned() and if the VM is not properly created
> i.e __tdx_td_init() has failed for whatever reason then the is_hkid_assigned
> check will also fail?

On the error path __tdx_td_init() calls tdx_mmu_release_hkid().

I'll do a patch to change to use is_td_created(). The error path is a bit
hard to follow so likely needs some more patches :)

Regards,

Tony

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Tony Lindgren]

On Mon, Aug 26, 2024 at 02:04:27PM +0300, Nikolay Borisov wrote:
> On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote:
> > From: Xiaoyao Li <xiaoyao.li@intel.com>
> >   static int tdx_online_cpu(unsigned int cpu)
> >   {
> >   	unsigned long flags;
> > @@ -217,11 +292,16 @@ static int __init __tdx_bringup(void)
> >   		goto get_sysinfo_err;
> >   	}
> > +	r = setup_kvm_tdx_caps();
> 
> nit: Since there are other similarly named functions that come later how
> about rename this to init_kvm_tdx_caps, so that it's clear that the
> functions that are executed ones are prefixed with "init_" and those that
> will be executed on every TDV boot up can be named prefixed with "setup_"

We can call setup_kvm_tdx_caps() from from tdx_get_kvm_supported_cpuid(),
and drop the struct kvm_tdx_caps. So then the setup_kvm_tdx_caps() should
be OK.

Regards,

Tony

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Paolo Bonzini]

On 8/29/24 06:51, Tony Lindgren wrote:
>> nit: Since there are other similarly named functions that come later how
>> about rename this to init_kvm_tdx_caps, so that it's clear that the
>> functions that are executed ones are prefixed with "init_" and those that
>> will be executed on every TDV boot up can be named prefixed with "setup_"
> We can call setup_kvm_tdx_caps() from from tdx_get_kvm_supported_cpuid(),
> and drop the struct kvm_tdx_caps. So then the setup_kvm_tdx_caps() should
> be OK.

I don't understand this suggestion since tdx_get_capabilities() also 
needs kvm_tdx_caps.  I think the code is okay as it is with just the 
rename that Nik suggested (there are already some setup_*() functions in 
KVM but for example setup_vmcs_config() is called from hardware_setup()).

Paolo

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Tony Lindgren]

On Tue, Sep 10, 2024 at 07:15:12PM +0200, Paolo Bonzini wrote:
> On 8/29/24 06:51, Tony Lindgren wrote:
> > > nit: Since there are other similarly named functions that come later how
> > > about rename this to init_kvm_tdx_caps, so that it's clear that the
> > > functions that are executed ones are prefixed with "init_" and those that
> > > will be executed on every TDV boot up can be named prefixed with "setup_"
> > We can call setup_kvm_tdx_caps() from from tdx_get_kvm_supported_cpuid(),
> > and drop the struct kvm_tdx_caps. So then the setup_kvm_tdx_caps() should
> > be OK.
> 
> I don't understand this suggestion since tdx_get_capabilities() also needs
> kvm_tdx_caps.  I think the code is okay as it is with just the rename that
> Nik suggested (there are already some setup_*() functions in KVM but for
> example setup_vmcs_config() is called from hardware_setup()).

Oh sorry for the confusion, looks like I pasted the function names wrong
way around above and left out where setup_kvm_tdx_caps() can be called
from.

I meant only tdx_get_capabilities() needs to call setup_kvm_tdx_caps().
And setup_kvm_tdx_caps() calls tdx_get_kvm_supported_cpuid().

The data in kvm_tdx_caps is only needed for tdx_get_capabilities(). It can
be generated from the data already in td_conf.

At least that's what it looks like to me, but maybe I'm missing something.

Regards,

Tony

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Xiaoyao Li]

On 9/11/2024 7:04 PM, Tony Lindgren wrote:
> On Tue, Sep 10, 2024 at 07:15:12PM +0200, Paolo Bonzini wrote:
>> On 8/29/24 06:51, Tony Lindgren wrote:
>>>> nit: Since there are other similarly named functions that come later how
>>>> about rename this to init_kvm_tdx_caps, so that it's clear that the
>>>> functions that are executed ones are prefixed with "init_" and those that
>>>> will be executed on every TDV boot up can be named prefixed with "setup_"
>>> We can call setup_kvm_tdx_caps() from from tdx_get_kvm_supported_cpuid(),
>>> and drop the struct kvm_tdx_caps. So then the setup_kvm_tdx_caps() should
>>> be OK.
>>
>> I don't understand this suggestion since tdx_get_capabilities() also needs
>> kvm_tdx_caps.  I think the code is okay as it is with just the rename that
>> Nik suggested (there are already some setup_*() functions in KVM but for
>> example setup_vmcs_config() is called from hardware_setup()).
> 
> Oh sorry for the confusion, looks like I pasted the function names wrong
> way around above and left out where setup_kvm_tdx_caps() can be called
> from.
> 
> I meant only tdx_get_capabilities() needs to call setup_kvm_tdx_caps().
> And setup_kvm_tdx_caps() calls tdx_get_kvm_supported_cpuid().
> 
> The data in kvm_tdx_caps is only needed for tdx_get_capabilities(). It can
> be generated from the data already in td_conf.
> 
> At least that's what it looks like to me, but maybe I'm missing something.

kvm_tdx_caps is setup in __tdx_bringup() because it also serves the 
purpose to validate the KVM's capabilities against the specific TDX 
module. If KVM and TDX module are incompatible, it needs to fail the 
bring up of TDX in KVM. It's too late to validate it when 
KVM_TDX_CAPABILITIES issued.  E.g., if the TDX module reports some 
fixed-1 attribute bit while KVM isn't aware of, in such case KVM needs 
to set enable_tdx to 0 to reflect that TDX cannot be enabled/brought up.

> Regards,
> 
> Tony

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Tony Lindgren]

On Thu, Oct 10, 2024 at 04:25:30PM +0800, Xiaoyao Li wrote:
> On 9/11/2024 7:04 PM, Tony Lindgren wrote:
> > On Tue, Sep 10, 2024 at 07:15:12PM +0200, Paolo Bonzini wrote:
> > > On 8/29/24 06:51, Tony Lindgren wrote:
> > > > > nit: Since there are other similarly named functions that come later how
> > > > > about rename this to init_kvm_tdx_caps, so that it's clear that the
> > > > > functions that are executed ones are prefixed with "init_" and those that
> > > > > will be executed on every TDV boot up can be named prefixed with "setup_"
> > > > We can call setup_kvm_tdx_caps() from from tdx_get_kvm_supported_cpuid(),
> > > > and drop the struct kvm_tdx_caps. So then the setup_kvm_tdx_caps() should
> > > > be OK.
> > > 
> > > I don't understand this suggestion since tdx_get_capabilities() also needs
> > > kvm_tdx_caps.  I think the code is okay as it is with just the rename that
> > > Nik suggested (there are already some setup_*() functions in KVM but for
> > > example setup_vmcs_config() is called from hardware_setup()).
> > 
> > Oh sorry for the confusion, looks like I pasted the function names wrong
> > way around above and left out where setup_kvm_tdx_caps() can be called
> > from.
> > 
> > I meant only tdx_get_capabilities() needs to call setup_kvm_tdx_caps().
> > And setup_kvm_tdx_caps() calls tdx_get_kvm_supported_cpuid().
> > 
> > The data in kvm_tdx_caps is only needed for tdx_get_capabilities(). It can
> > be generated from the data already in td_conf.
> > 
> > At least that's what it looks like to me, but maybe I'm missing something.
> 
> kvm_tdx_caps is setup in __tdx_bringup() because it also serves the purpose
> to validate the KVM's capabilities against the specific TDX module. If KVM
> and TDX module are incompatible, it needs to fail the bring up of TDX in
> KVM. It's too late to validate it when KVM_TDX_CAPABILITIES issued.  E.g.,
> if the TDX module reports some fixed-1 attribute bit while KVM isn't aware
> of, in such case KVM needs to set enable_tdx to 0 to reflect that TDX cannot
> be enabled/brought up.

OK makes sense, thanks for clarifying the use case for __tdx_bringup().

We can check the attributes_fixed1 and xfam_fixed1 also on __tdx_bringup()
no problem.

Regards,

Tony

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Xiaoyao Li]

On 8/19/2024 9:33 AM, Tao Su wrote:
> On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote:
>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>
>> While TDX module reports a set of capabilities/features that it
>> supports, what KVM currently supports might be a subset of them.
>> E.g., DEBUG and PERFMON are supported by TDX module but currently not
>> supported by KVM.
>>
>> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX.
>> supported_attrs and suppported_xfam are validated against fixed0/1
>> values enumerated by TDX module. Configurable CPUID bits derive from TDX
>> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID),
>> i.e., mask off the bits that are configurable in the view of TDX module
>> but not supported by KVM yet.
>>
> 
> But this mask is not implemented in this patch, which should be in patch24?

yes, the commit message needs to be updated. Even more the patches need 
to be re-organized.

>> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0
>> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> 
> [...]

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Tony Lindgren]

On Tue, Aug 13, 2024 at 11:25:37AM +0800, Chao Gao wrote:
> On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote:
> >From: Xiaoyao Li <xiaoyao.li@intel.com>
> >+static int __init setup_kvm_tdx_caps(void)
> >+{
> >+	const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
> >+	u64 kvm_supported;
> >+	int i;
> >+
> >+	kvm_tdx_caps = kzalloc(sizeof(*kvm_tdx_caps) +
> >+			       sizeof(struct kvm_tdx_cpuid_config) * td_conf->num_cpuid_config,
> 
> struct_size()
> 
> >+			       GFP_KERNEL);
> >+	if (!kvm_tdx_caps)
> >+		return -ENOMEM;

This will go away with the dropping of struct kvm_tdx_caps. Should be checked
for other places though.

Regards,

Tony

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Paolo Bonzini]

On 8/30/24 10:34, Tony Lindgren wrote:
> On Tue, Aug 13, 2024 at 11:25:37AM +0800, Chao Gao wrote:
>> On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote:
>>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>> +static int __init setup_kvm_tdx_caps(void)
>>> +{
>>> +	const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
>>> +	u64 kvm_supported;
>>> +	int i;
>>> +
>>> +	kvm_tdx_caps = kzalloc(sizeof(*kvm_tdx_caps) +
>>> +			       sizeof(struct kvm_tdx_cpuid_config) * td_conf->num_cpuid_config,
>>
>> struct_size()
>>
>>> +			       GFP_KERNEL);
>>> +	if (!kvm_tdx_caps)
>>> +		return -ENOMEM;
> 
> This will go away with the dropping of struct kvm_tdx_caps. Should be checked
> for other places though.

What do you mean exactly by dropping of struct kvm_tdx_caps?

Paolo

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Tony Lindgren]

On Tue, Sep 10, 2024 at 06:58:06PM +0200, Paolo Bonzini wrote:
> On 8/30/24 10:34, Tony Lindgren wrote:
> > On Tue, Aug 13, 2024 at 11:25:37AM +0800, Chao Gao wrote:
> > > On Mon, Aug 12, 2024 at 03:48:05PM -0700, Rick Edgecombe wrote:
> > > > From: Xiaoyao Li <xiaoyao.li@intel.com>
> > > > +static int __init setup_kvm_tdx_caps(void)
> > > > +{
> > > > +	const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
> > > > +	u64 kvm_supported;
> > > > +	int i;
> > > > +
> > > > +	kvm_tdx_caps = kzalloc(sizeof(*kvm_tdx_caps) +
> > > > +			       sizeof(struct kvm_tdx_cpuid_config) * td_conf->num_cpuid_config,
> > > 
> > > struct_size()
> > > 
> > > > +			       GFP_KERNEL);
> > > > +	if (!kvm_tdx_caps)
> > > > +		return -ENOMEM;
> > 
> > This will go away with the dropping of struct kvm_tdx_caps. Should be checked
> > for other places though.
> 
> What do you mean exactly by dropping of struct kvm_tdx_caps?

I think we can initialize the data as needed based on td_conf.

Regards,

Tony

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Tony Lindgren]

On Tue, Aug 13, 2024 at 05:26:06AM +0000, Huang, Kai wrote:
> On Tue, 2024-08-13 at 11:25 +0800, Chao Gao wrote:
> > > +	for (i = 0; i < td_conf->num_cpuid_config; i++) {
> > > +		struct kvm_tdx_cpuid_config source = {
> > > +			.leaf = (u32)td_conf->cpuid_config_leaves[i],
> > > +			.sub_leaf = td_conf->cpuid_config_leaves[i] >> 32,
> > > +			.eax = (u32)td_conf->cpuid_config_values[i].eax_ebx,
> > > +			.ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32,
> > > +			.ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx,
> > > +			.edx = td_conf->cpuid_config_values[i].ecx_edx >> 32,
> > > +		};
> > > +		struct kvm_tdx_cpuid_config *dest =
> > > +			&kvm_tdx_caps->cpuid_configs[i];
> > > +
> > > +		memcpy(dest, &source, sizeof(struct kvm_tdx_cpuid_config));
> > 
> > this memcpy() looks superfluous. does this work?
> > 
> > 		kvm_tdx_caps->cpuid_configs[i] = {
> > 			.leaf = (u32)td_conf->cpuid_config_leaves[i],
> > 			.sub_leaf = td_conf->cpuid_config_leaves[i] >> 32,
> > 			.eax = (u32)td_conf->cpuid_config_values[i].eax_ebx,
> > 			.ebx = td_conf->cpuid_config_values[i].eax_ebx >> 32,
> > 			.ecx = (u32)td_conf->cpuid_config_values[i].ecx_edx,
> > 			.edx = td_conf->cpuid_config_values[i].ecx_edx >> 32,
> > 		};
> 
> This looks good to me.  I didn't try to optimize because it's done in the
> module loading time.

I'll do a patch to initialize dest directly without a memcpy().

Tony

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Edgecombe, Rick P]

On Tue, 2024-08-13 at 11:25 +0800, Chao Gao wrote:
> > +       /*
> > +        * PT and CET can be exposed to TD guest regardless of KVM's XSS, PT
> > +        * and, CET support.
> > +        */
> > +       kvm_supported |= XFEATURE_MASK_PT | XFEATURE_MASK_CET_USER |
> > +                        XFEATURE_MASK_CET_KERNEL;
> 
> I prefer to add PT/CET bits in separate patches because PT/CET related MSRs
> may
> need save/restore. Putting them in separate patches can give us the chance to
> explain this in detail.

I think we should just drop them from the base series to save required testing.
We can leave them for the future.

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Nikolay Borisov]

On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote:
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> While TDX module reports a set of capabilities/features that it
> supports, what KVM currently supports might be a subset of them.
> E.g., DEBUG and PERFMON are supported by TDX module but currently not
> supported by KVM.
> 
> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX.
> supported_attrs and suppported_xfam are validated against fixed0/1
> values enumerated by TDX module. Configurable CPUID bits derive from TDX
> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID),
> i.e., mask off the bits that are configurable in the view of TDX module
> but not supported by KVM yet.
> 
> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0
> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> uAPI breakout v1:
>   - Change setup_kvm_tdx_caps() to use the exported 'struct tdx_sysinfo'
>     pointer.
>   - Change how to copy 'kvm_tdx_cpuid_config' since 'struct tdx_sysinfo'
>     doesn't have 'kvm_tdx_cpuid_config'.
>   - Updates for uAPI changes
> ---
>   arch/x86/include/uapi/asm/kvm.h |  2 -
>   arch/x86/kvm/vmx/tdx.c          | 81 +++++++++++++++++++++++++++++++++
>   2 files changed, 81 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index 47caf508cca7..c9eb2e2f5559 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -952,8 +952,6 @@ struct kvm_tdx_cmd {
>   	__u64 hw_error;
>   };
>   
> -#define KVM_TDX_CPUID_NO_SUBLEAF	((__u32)-1)
> -
>   struct kvm_tdx_cpuid_config {
>   	__u32 leaf;
>   	__u32 sub_leaf;
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 90b44ebaf864..d89973e554f6 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -31,6 +31,19 @@ static void __used tdx_guest_keyid_free(int keyid)
>   	ida_free(&tdx_guest_keyid_pool, keyid);
>   }
>   
> +#define KVM_TDX_CPUID_NO_SUBLEAF	((__u32)-1)
> +
> +struct kvm_tdx_caps {
> +	u64 supported_attrs;
> +	u64 supported_xfam;
> +
> +	u16 num_cpuid_config;
> +	/* This must the last member. */
> +	DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
> +};
> +
> +static struct kvm_tdx_caps *kvm_tdx_caps;
> +
>   static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
>   {
>   	const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
> @@ -131,6 +144,68 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>   	return r;
>   }
>   
> +#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)

Why isn't TDX_TD_ATTR_DEBUG added as well?

<snip>

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Xiaoyao Li]

On 9/4/2024 7:58 PM, Nikolay Borisov wrote:
> 
> 
> On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote:
>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>
>> While TDX module reports a set of capabilities/features that it
>> supports, what KVM currently supports might be a subset of them.
>> E.g., DEBUG and PERFMON are supported by TDX module but currently not
>> supported by KVM.
>>
>> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX.
>> supported_attrs and suppported_xfam are validated against fixed0/1
>> values enumerated by TDX module. Configurable CPUID bits derive from TDX
>> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID),
>> i.e., mask off the bits that are configurable in the view of TDX module
>> but not supported by KVM yet.
>>
>> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0
>> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM.
>>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>> ---
>> uAPI breakout v1:
>>   - Change setup_kvm_tdx_caps() to use the exported 'struct tdx_sysinfo'
>>     pointer.
>>   - Change how to copy 'kvm_tdx_cpuid_config' since 'struct tdx_sysinfo'
>>     doesn't have 'kvm_tdx_cpuid_config'.
>>   - Updates for uAPI changes
>> ---
>>   arch/x86/include/uapi/asm/kvm.h |  2 -
>>   arch/x86/kvm/vmx/tdx.c          | 81 +++++++++++++++++++++++++++++++++
>>   2 files changed, 81 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/include/uapi/asm/kvm.h 
>> b/arch/x86/include/uapi/asm/kvm.h
>> index 47caf508cca7..c9eb2e2f5559 100644
>> --- a/arch/x86/include/uapi/asm/kvm.h
>> +++ b/arch/x86/include/uapi/asm/kvm.h
>> @@ -952,8 +952,6 @@ struct kvm_tdx_cmd {
>>       __u64 hw_error;
>>   };
>> -#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>> -
>>   struct kvm_tdx_cpuid_config {
>>       __u32 leaf;
>>       __u32 sub_leaf;
>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>> index 90b44ebaf864..d89973e554f6 100644
>> --- a/arch/x86/kvm/vmx/tdx.c
>> +++ b/arch/x86/kvm/vmx/tdx.c
>> @@ -31,6 +31,19 @@ static void __used tdx_guest_keyid_free(int keyid)
>>       ida_free(&tdx_guest_keyid_pool, keyid);
>>   }
>> +#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>> +
>> +struct kvm_tdx_caps {
>> +    u64 supported_attrs;
>> +    u64 supported_xfam;
>> +
>> +    u16 num_cpuid_config;
>> +    /* This must the last member. */
>> +    DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
>> +};
>> +
>> +static struct kvm_tdx_caps *kvm_tdx_caps;
>> +
>>   static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
>>   {
>>       const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
>> @@ -131,6 +144,68 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>>       return r;
>>   }
>> +#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
> 
> Why isn't TDX_TD_ATTR_DEBUG added as well?

Because so far KVM doesn't support all the features of a DEBUG TD for 
userspace. e.g., KVM doesn't provide interface for userspace to 
read/write private memory of DEBUG TD.

> <snip>

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Nikolay Borisov]

On 5.09.24 г. 16:36 ч., Xiaoyao Li wrote:
> On 9/4/2024 7:58 PM, Nikolay Borisov wrote:
>>
>>
>> On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote:
>>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>>
>>> While TDX module reports a set of capabilities/features that it
>>> supports, what KVM currently supports might be a subset of them.
>>> E.g., DEBUG and PERFMON are supported by TDX module but currently not
>>> supported by KVM.
>>>
>>> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX.
>>> supported_attrs and suppported_xfam are validated against fixed0/1
>>> values enumerated by TDX module. Configurable CPUID bits derive from TDX
>>> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID),
>>> i.e., mask off the bits that are configurable in the view of TDX module
>>> but not supported by KVM yet.
>>>
>>> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0
>>> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM.
>>>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>>> ---
>>> uAPI breakout v1:
>>>   - Change setup_kvm_tdx_caps() to use the exported 'struct tdx_sysinfo'
>>>     pointer.
>>>   - Change how to copy 'kvm_tdx_cpuid_config' since 'struct tdx_sysinfo'
>>>     doesn't have 'kvm_tdx_cpuid_config'.
>>>   - Updates for uAPI changes
>>> ---
>>>   arch/x86/include/uapi/asm/kvm.h |  2 -
>>>   arch/x86/kvm/vmx/tdx.c          | 81 +++++++++++++++++++++++++++++++++
>>>   2 files changed, 81 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/include/uapi/asm/kvm.h 
>>> b/arch/x86/include/uapi/asm/kvm.h
>>> index 47caf508cca7..c9eb2e2f5559 100644
>>> --- a/arch/x86/include/uapi/asm/kvm.h
>>> +++ b/arch/x86/include/uapi/asm/kvm.h
>>> @@ -952,8 +952,6 @@ struct kvm_tdx_cmd {
>>>       __u64 hw_error;
>>>   };
>>> -#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>>> -
>>>   struct kvm_tdx_cpuid_config {
>>>       __u32 leaf;
>>>       __u32 sub_leaf;
>>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>>> index 90b44ebaf864..d89973e554f6 100644
>>> --- a/arch/x86/kvm/vmx/tdx.c
>>> +++ b/arch/x86/kvm/vmx/tdx.c
>>> @@ -31,6 +31,19 @@ static void __used tdx_guest_keyid_free(int keyid)
>>>       ida_free(&tdx_guest_keyid_pool, keyid);
>>>   }
>>> +#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>>> +
>>> +struct kvm_tdx_caps {
>>> +    u64 supported_attrs;
>>> +    u64 supported_xfam;
>>> +
>>> +    u16 num_cpuid_config;
>>> +    /* This must the last member. */
>>> +    DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
>>> +};
>>> +
>>> +static struct kvm_tdx_caps *kvm_tdx_caps;
>>> +
>>>   static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
>>>   {
>>>       const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
>>> @@ -131,6 +144,68 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user 
>>> *argp)
>>>       return r;
>>>   }
>>> +#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
>>
>> Why isn't TDX_TD_ATTR_DEBUG added as well?
> 
> Because so far KVM doesn't support all the features of a DEBUG TD for 
> userspace. e.g., KVM doesn't provide interface for userspace to 
> read/write private memory of DEBUG TD.

But this means that you can't really run a TDX with SEPT_VE_DISABLE 
disabled for debugging purposes, so perhaps it might be necessary to 
rethink the condition allowing SEPT_VE_DISABLE to be disabled. Without 
the debug flag and SEPT_VE_DISABLE disabled the code refuses to start 
the VM, what if one wants to debug some SEPT issue by having an oops 
generated inside the vm ?

> 
>> <snip>
> 
> 

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Xiaoyao Li]

On 9/12/2024 4:04 PM, Nikolay Borisov wrote:
> 
> 
> On 5.09.24 г. 16:36 ч., Xiaoyao Li wrote:
>> On 9/4/2024 7:58 PM, Nikolay Borisov wrote:
>>>
>>>
>>> On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote:
>>>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>
>>>> While TDX module reports a set of capabilities/features that it
>>>> supports, what KVM currently supports might be a subset of them.
>>>> E.g., DEBUG and PERFMON are supported by TDX module but currently not
>>>> supported by KVM.
>>>>
>>>> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of TDX.
>>>> supported_attrs and suppported_xfam are validated against fixed0/1
>>>> values enumerated by TDX module. Configurable CPUID bits derive from 
>>>> TDX
>>>> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID),
>>>> i.e., mask off the bits that are configurable in the view of TDX module
>>>> but not supported by KVM yet.
>>>>
>>>> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it to 0
>>>> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM.
>>>>
>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>>>> ---
>>>> uAPI breakout v1:
>>>>   - Change setup_kvm_tdx_caps() to use the exported 'struct 
>>>> tdx_sysinfo'
>>>>     pointer.
>>>>   - Change how to copy 'kvm_tdx_cpuid_config' since 'struct 
>>>> tdx_sysinfo'
>>>>     doesn't have 'kvm_tdx_cpuid_config'.
>>>>   - Updates for uAPI changes
>>>> ---
>>>>   arch/x86/include/uapi/asm/kvm.h |  2 -
>>>>   arch/x86/kvm/vmx/tdx.c          | 81 
>>>> +++++++++++++++++++++++++++++++++
>>>>   2 files changed, 81 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/include/uapi/asm/kvm.h 
>>>> b/arch/x86/include/uapi/asm/kvm.h
>>>> index 47caf508cca7..c9eb2e2f5559 100644
>>>> --- a/arch/x86/include/uapi/asm/kvm.h
>>>> +++ b/arch/x86/include/uapi/asm/kvm.h
>>>> @@ -952,8 +952,6 @@ struct kvm_tdx_cmd {
>>>>       __u64 hw_error;
>>>>   };
>>>> -#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>>>> -
>>>>   struct kvm_tdx_cpuid_config {
>>>>       __u32 leaf;
>>>>       __u32 sub_leaf;
>>>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>>>> index 90b44ebaf864..d89973e554f6 100644
>>>> --- a/arch/x86/kvm/vmx/tdx.c
>>>> +++ b/arch/x86/kvm/vmx/tdx.c
>>>> @@ -31,6 +31,19 @@ static void __used tdx_guest_keyid_free(int keyid)
>>>>       ida_free(&tdx_guest_keyid_pool, keyid);
>>>>   }
>>>> +#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>>>> +
>>>> +struct kvm_tdx_caps {
>>>> +    u64 supported_attrs;
>>>> +    u64 supported_xfam;
>>>> +
>>>> +    u16 num_cpuid_config;
>>>> +    /* This must the last member. */
>>>> +    DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
>>>> +};
>>>> +
>>>> +static struct kvm_tdx_caps *kvm_tdx_caps;
>>>> +
>>>>   static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
>>>>   {
>>>>       const struct tdx_sysinfo_td_conf *td_conf = 
>>>> &tdx_sysinfo->td_conf;
>>>> @@ -131,6 +144,68 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user 
>>>> *argp)
>>>>       return r;
>>>>   }
>>>> +#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
>>>
>>> Why isn't TDX_TD_ATTR_DEBUG added as well?
>>
>> Because so far KVM doesn't support all the features of a DEBUG TD for 
>> userspace. e.g., KVM doesn't provide interface for userspace to 
>> read/write private memory of DEBUG TD.
> 
> But this means that you can't really run a TDX with SEPT_VE_DISABLE 
> disabled for debugging purposes, so perhaps it might be necessary to 
> rethink the condition allowing SEPT_VE_DISABLE to be disabled. Without 
> the debug flag and SEPT_VE_DISABLE disabled the code refuses to start 
> the VM, what if one wants to debug some SEPT issue by having an oops 
> generated inside the vm ?

sept_ve_disable is allowed to be disable, i.e., set to 0.

I think there must be some misunderstanding.

>>
>>> <snip>
>>
>>

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Nikolay Borisov]

On 12.09.24 г. 11:37 ч., Xiaoyao Li wrote:
> On 9/12/2024 4:04 PM, Nikolay Borisov wrote:
>>
>>
>> On 5.09.24 г. 16:36 ч., Xiaoyao Li wrote:
>>> On 9/4/2024 7:58 PM, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote:
>>>>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>>
>>>>> While TDX module reports a set of capabilities/features that it
>>>>> supports, what KVM currently supports might be a subset of them.
>>>>> E.g., DEBUG and PERFMON are supported by TDX module but currently not
>>>>> supported by KVM.
>>>>>
>>>>> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of 
>>>>> TDX.
>>>>> supported_attrs and suppported_xfam are validated against fixed0/1
>>>>> values enumerated by TDX module. Configurable CPUID bits derive 
>>>>> from TDX
>>>>> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID),
>>>>> i.e., mask off the bits that are configurable in the view of TDX 
>>>>> module
>>>>> but not supported by KVM yet.
>>>>>
>>>>> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it 
>>>>> to 0
>>>>> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of KVM.
>>>>>
>>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>>>>> ---
>>>>> uAPI breakout v1:
>>>>>   - Change setup_kvm_tdx_caps() to use the exported 'struct 
>>>>> tdx_sysinfo'
>>>>>     pointer.
>>>>>   - Change how to copy 'kvm_tdx_cpuid_config' since 'struct 
>>>>> tdx_sysinfo'
>>>>>     doesn't have 'kvm_tdx_cpuid_config'.
>>>>>   - Updates for uAPI changes
>>>>> ---
>>>>>   arch/x86/include/uapi/asm/kvm.h |  2 -
>>>>>   arch/x86/kvm/vmx/tdx.c          | 81 
>>>>> +++++++++++++++++++++++++++++++++
>>>>>   2 files changed, 81 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/include/uapi/asm/kvm.h 
>>>>> b/arch/x86/include/uapi/asm/kvm.h
>>>>> index 47caf508cca7..c9eb2e2f5559 100644
>>>>> --- a/arch/x86/include/uapi/asm/kvm.h
>>>>> +++ b/arch/x86/include/uapi/asm/kvm.h
>>>>> @@ -952,8 +952,6 @@ struct kvm_tdx_cmd {
>>>>>       __u64 hw_error;
>>>>>   };
>>>>> -#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>>>>> -
>>>>>   struct kvm_tdx_cpuid_config {
>>>>>       __u32 leaf;
>>>>>       __u32 sub_leaf;
>>>>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>>>>> index 90b44ebaf864..d89973e554f6 100644
>>>>> --- a/arch/x86/kvm/vmx/tdx.c
>>>>> +++ b/arch/x86/kvm/vmx/tdx.c
>>>>> @@ -31,6 +31,19 @@ static void __used tdx_guest_keyid_free(int keyid)
>>>>>       ida_free(&tdx_guest_keyid_pool, keyid);
>>>>>   }
>>>>> +#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>>>>> +
>>>>> +struct kvm_tdx_caps {
>>>>> +    u64 supported_attrs;
>>>>> +    u64 supported_xfam;
>>>>> +
>>>>> +    u16 num_cpuid_config;
>>>>> +    /* This must the last member. */
>>>>> +    DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
>>>>> +};
>>>>> +
>>>>> +static struct kvm_tdx_caps *kvm_tdx_caps;
>>>>> +
>>>>>   static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
>>>>>   {
>>>>>       const struct tdx_sysinfo_td_conf *td_conf = 
>>>>> &tdx_sysinfo->td_conf;
>>>>> @@ -131,6 +144,68 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user 
>>>>> *argp)
>>>>>       return r;
>>>>>   }
>>>>> +#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
>>>>
>>>> Why isn't TDX_TD_ATTR_DEBUG added as well?
>>>
>>> Because so far KVM doesn't support all the features of a DEBUG TD for 
>>> userspace. e.g., KVM doesn't provide interface for userspace to 
>>> read/write private memory of DEBUG TD.
>>
>> But this means that you can't really run a TDX with SEPT_VE_DISABLE 
>> disabled for debugging purposes, so perhaps it might be necessary to 
>> rethink the condition allowing SEPT_VE_DISABLE to be disabled. Without 
>> the debug flag and SEPT_VE_DISABLE disabled the code refuses to start 
>> the VM, what if one wants to debug some SEPT issue by having an oops 
>> generated inside the vm ?
> 
> sept_ve_disable is allowed to be disable, i.e., set to 0.
> 
> I think there must be some misunderstanding.

There isn't, the current code is:

   201         if (!(td_attr & ATTR_SEPT_VE_DISABLE)) { 

     1                 const char *msg = "TD misconfiguration: 
SEPT_VE_DISABLE attribute must be set.";
     2 

     3                 /* Relax SEPT_VE_DISABLE check for debug TD. */ 

     4                 if (td_attr & ATTR_DEBUG) 

     5                         pr_warn("%s\n", msg); 

     6                 else 

     7                         tdx_panic(msg); 

     8         }


I.e if we disable SEPT_VE_DISABLE without having ATTR_DEBUG it results 
in a panic.

> 
>>>
>>>> <snip>
>>>
>>>
> 

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Xiaoyao Li]

On 9/12/2024 4:43 PM, Nikolay Borisov wrote:
> 
> 
> On 12.09.24 г. 11:37 ч., Xiaoyao Li wrote:
>> On 9/12/2024 4:04 PM, Nikolay Borisov wrote:
>>>
>>>
>>> On 5.09.24 г. 16:36 ч., Xiaoyao Li wrote:
>>>> On 9/4/2024 7:58 PM, Nikolay Borisov wrote:
>>>>>
>>>>>
>>>>> On 13.08.24 г. 1:48 ч., Rick Edgecombe wrote:
>>>>>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>>>
>>>>>> While TDX module reports a set of capabilities/features that it
>>>>>> supports, what KVM currently supports might be a subset of them.
>>>>>> E.g., DEBUG and PERFMON are supported by TDX module but currently not
>>>>>> supported by KVM.
>>>>>>
>>>>>> Introduce a new struct kvm_tdx_caps to store KVM's capabilities of 
>>>>>> TDX.
>>>>>> supported_attrs and suppported_xfam are validated against fixed0/1
>>>>>> values enumerated by TDX module. Configurable CPUID bits derive 
>>>>>> from TDX
>>>>>> module plus applying KVM's capabilities (KVM_GET_SUPPORTED_CPUID),
>>>>>> i.e., mask off the bits that are configurable in the view of TDX 
>>>>>> module
>>>>>> but not supported by KVM yet.
>>>>>>
>>>>>> KVM_TDX_CPUID_NO_SUBLEAF is the concept from TDX module, switch it 
>>>>>> to 0
>>>>>> and use KVM_CPUID_FLAG_SIGNIFCANT_INDEX, which are the concept of 
>>>>>> KVM.
>>>>>>
>>>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>>>> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>>>>>> ---
>>>>>> uAPI breakout v1:
>>>>>>   - Change setup_kvm_tdx_caps() to use the exported 'struct 
>>>>>> tdx_sysinfo'
>>>>>>     pointer.
>>>>>>   - Change how to copy 'kvm_tdx_cpuid_config' since 'struct 
>>>>>> tdx_sysinfo'
>>>>>>     doesn't have 'kvm_tdx_cpuid_config'.
>>>>>>   - Updates for uAPI changes
>>>>>> ---
>>>>>>   arch/x86/include/uapi/asm/kvm.h |  2 -
>>>>>>   arch/x86/kvm/vmx/tdx.c          | 81 
>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>   2 files changed, 81 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/include/uapi/asm/kvm.h 
>>>>>> b/arch/x86/include/uapi/asm/kvm.h
>>>>>> index 47caf508cca7..c9eb2e2f5559 100644
>>>>>> --- a/arch/x86/include/uapi/asm/kvm.h
>>>>>> +++ b/arch/x86/include/uapi/asm/kvm.h
>>>>>> @@ -952,8 +952,6 @@ struct kvm_tdx_cmd {
>>>>>>       __u64 hw_error;
>>>>>>   };
>>>>>> -#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>>>>>> -
>>>>>>   struct kvm_tdx_cpuid_config {
>>>>>>       __u32 leaf;
>>>>>>       __u32 sub_leaf;
>>>>>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>>>>>> index 90b44ebaf864..d89973e554f6 100644
>>>>>> --- a/arch/x86/kvm/vmx/tdx.c
>>>>>> +++ b/arch/x86/kvm/vmx/tdx.c
>>>>>> @@ -31,6 +31,19 @@ static void __used tdx_guest_keyid_free(int keyid)
>>>>>>       ida_free(&tdx_guest_keyid_pool, keyid);
>>>>>>   }
>>>>>> +#define KVM_TDX_CPUID_NO_SUBLEAF    ((__u32)-1)
>>>>>> +
>>>>>> +struct kvm_tdx_caps {
>>>>>> +    u64 supported_attrs;
>>>>>> +    u64 supported_xfam;
>>>>>> +
>>>>>> +    u16 num_cpuid_config;
>>>>>> +    /* This must the last member. */
>>>>>> +    DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
>>>>>> +};
>>>>>> +
>>>>>> +static struct kvm_tdx_caps *kvm_tdx_caps;
>>>>>> +
>>>>>>   static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
>>>>>>   {
>>>>>>       const struct tdx_sysinfo_td_conf *td_conf = 
>>>>>> &tdx_sysinfo->td_conf;
>>>>>> @@ -131,6 +144,68 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user 
>>>>>> *argp)
>>>>>>       return r;
>>>>>>   }
>>>>>> +#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
>>>>>
>>>>> Why isn't TDX_TD_ATTR_DEBUG added as well?
>>>>
>>>> Because so far KVM doesn't support all the features of a DEBUG TD 
>>>> for userspace. e.g., KVM doesn't provide interface for userspace to 
>>>> read/write private memory of DEBUG TD.
>>>
>>> But this means that you can't really run a TDX with SEPT_VE_DISABLE 
>>> disabled for debugging purposes, so perhaps it might be necessary to 
>>> rethink the condition allowing SEPT_VE_DISABLE to be disabled. 
>>> Without the debug flag and SEPT_VE_DISABLE disabled the code refuses 
>>> to start the VM, what if one wants to debug some SEPT issue by having 
>>> an oops generated inside the vm ?
>>
>> sept_ve_disable is allowed to be disable, i.e., set to 0.
>>
>> I think there must be some misunderstanding.
> 
> There isn't, the current code is:
> 
>    201         if (!(td_attr & ATTR_SEPT_VE_DISABLE)) {
>      1                 const char *msg = "TD misconfiguration: 
> SEPT_VE_DISABLE attribute must be set.";
>      2
>      3                 /* Relax SEPT_VE_DISABLE check for debug TD. */
>      4                 if (td_attr & ATTR_DEBUG)
>      5                         pr_warn("%s\n", msg);
>      6                 else
>      7                         tdx_panic(msg);
>      8         }
> 
> 
> I.e if we disable SEPT_VE_DISABLE without having ATTR_DEBUG it results 
> in a panic.

I see now.

It's linux TD guest's implementation, which requires SEPT_VE_DISABLE 
must be set unless it's a debug TD.

Yes, it can be the motivation to request KVM to add the support of 
ATTRIBUTES.DEBUG. But the support of ATTRIBUTES.DEBUG is not just 
allowing this bit to be set to 1. For DEBUG TD, VMM is allowed to 
read/write the private memory content, cpu registers, and MSRs, VMM is 
allowed to trap the exceptions in TD, VMM is allowed to manipulate the 
VMCS of TD vcpu, etc.

IMHO, for upstream, no need to support all the debug capability as 
described above. But we need firstly define a subset of them as the 
starter of supporting ATTRIBUTES.DEBUG. Otherwise, what is the meaning 
of KVM to allow the DEBUG to be set without providing any debug capability?

For debugging purpose, you can just hack guest kernel to allow 
spet_ve_disable to be 0 without DEBUG bit set, or hack KVM to allow 
DEBUG bit to be set.

>>
>>>>
>>>>> <snip>
>>>>
>>>>
>>

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Edgecombe, Rick P]

On Thu, 2024-09-12 at 17:07 +0800, Xiaoyao Li wrote:
> > I.e if we disable SEPT_VE_DISABLE without having ATTR_DEBUG it results 
> > in a panic.
> 
> I see now.
> 
> It's linux TD guest's implementation, which requires SEPT_VE_DISABLE 
> must be set unless it's a debug TD.
> 
> Yes, it can be the motivation to request KVM to add the support of 
> ATTRIBUTES.DEBUG. But the support of ATTRIBUTES.DEBUG is not just 
> allowing this bit to be set to 1. For DEBUG TD, VMM is allowed to 
> read/write the private memory content, cpu registers, and MSRs, VMM is 
> allowed to trap the exceptions in TD, VMM is allowed to manipulate the 
> VMCS of TD vcpu, etc.
> 
> IMHO, for upstream, no need to support all the debug capability as 
> described above. 

I think you mean for the first upstream support. I don't see why it would not be
suitable for upstream if we have upstream users doing it.

Nikolay, is this hypothetical or something that you have been doing with some
other TDX tree? We can factor it into the post-base support roadmap.

> But we need firstly define a subset of them as the 
> starter of supporting ATTRIBUTES.DEBUG. Otherwise, what is the meaning 
> of KVM to allow the DEBUG to be set without providing any debug capability?
> 
> For debugging purpose, you can just hack guest kernel to allow 
> spet_ve_disable to be 0 without DEBUG bit set, or hack KVM to allow 
> DEBUG bit to be set.

Re: [PATCH 10/25] KVM: TDX: Initialize KVM supported capabilities when module setup

[Nikolay Borisov]

On 12.09.24 г. 18:12 ч., Edgecombe, Rick P wrote:
> On Thu, 2024-09-12 at 17:07 +0800, Xiaoyao Li wrote:
>>> I.e if we disable SEPT_VE_DISABLE without having ATTR_DEBUG it results
>>> in a panic.
>>
>> I see now.
>>
>> It's linux TD guest's implementation, which requires SEPT_VE_DISABLE
>> must be set unless it's a debug TD.
>>
>> Yes, it can be the motivation to request KVM to add the support of
>> ATTRIBUTES.DEBUG. But the support of ATTRIBUTES.DEBUG is not just
>> allowing this bit to be set to 1. For DEBUG TD, VMM is allowed to
>> read/write the private memory content, cpu registers, and MSRs, VMM is
>> allowed to trap the exceptions in TD, VMM is allowed to manipulate the
>> VMCS of TD vcpu, etc.
>>
>> IMHO, for upstream, no need to support all the debug capability as
>> described above.
> 
> I think you mean for the first upstream support. I don't see why it would not be
> suitable for upstream if we have upstream users doing it.
> 
> Nikolay, is this hypothetical or something that you have been doing with some
> other TDX tree? We can factor it into the post-base support roadmap.

The real world use case here is a report comes and says " Hey, our TVM 
locks up on certain event". Turns out it happens due to the hypervisor 
not handling correctly some TD exit event caused by a SEPT violation. So 
then I can instruct the person to simply disable SEPT_VE_DISABLE so that 
instead of a TD exit we get a nice oops inside the guest which can serve 
further debugging.


> 
>> But we need firstly define a subset of them as the
>> starter of supporting ATTRIBUTES.DEBUG. Otherwise, what is the meaning
>> of KVM to allow the DEBUG to be set without providing any debug capability?
>>
>> For debugging purpose, you can just hack guest kernel to allow
>> spet_ve_disable to be 0 without DEBUG bit set, or hack KVM to allow
>> DEBUG bit to be set.
> 

Re: [PATCH 01/25] KVM: TDX: Add placeholders for TDX VM/vCPU structures

[Paolo Bonzini]

On 8/13/24 00:47, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> Add TDX's own VM and vCPU structures as placeholder to manage and run
> TDX guests.  Also add helper functions to check whether a VM/vCPU is
> TDX or normal VMX one, and add helpers to convert between TDX VM/vCPU
> and KVM VM/vCPU.
> 
> TDX protects guest VMs from malicious host.  Unlike VMX guests, TDX
> guests are crypto-protected.  KVM cannot access TDX guests' memory and
> vCPU states directly.  Instead, TDX requires KVM to use a set of TDX
> architecture-defined firmware APIs (a.k.a TDX module SEAMCALLs) to
> manage and run TDX guests.
> 
> In fact, the way to manage and run TDX guests and normal VMX guests are
> quite different.  Because of that, the current structures
> ('struct kvm_vmx' and 'struct vcpu_vmx') to manage VMX guests are not
> quite suitable for TDX guests.  E.g., the majority of the members of
> 'struct vcpu_vmx' don't apply to TDX guests.
> 
> Introduce TDX's own VM and vCPU structures ('struct kvm_tdx' and 'struct
> vcpu_tdx' respectively) for KVM to manage and run TDX guests.  And
> instead of building TDX's VM and vCPU structures based on VMX's, build
> them directly based on 'struct kvm'.
> 
> As a result, TDX and VMX guests will have different VM size and vCPU
> size/alignment.
> 
> Currently, kvm_arch_alloc_vm() uses 'kvm_x86_ops::vm_size' to allocate
> enough space for the VM structure when creating guest.  With TDX guests,
> ideally, KVM should allocate the VM structure based on the VM type so
> that the precise size can be allocated for VMX and TDX guests.  But this
> requires more extensive code change.  For now, simply choose the maximum
> size of 'struct kvm_tdx' and 'struct kvm_vmx' for VM structure
> allocation for both VMX and TDX guests.  This would result in small
> memory waste for each VM which has smaller VM structure size but this is
> acceptable.
> 
> For simplicity, use the same way for vCPU allocation too.  Otherwise KVM
> would need to maintain a separate 'kvm_vcpu_cache' for each VM type.
> 
> Note, updating the 'vt_x86_ops::vm_size' needs to be done before calling
> kvm_ops_update(), which copies vt_x86_ops to kvm_x86_ops.  However this
> happens before TDX module initialization.  Therefore theoretically it is
> possible that 'kvm_x86_ops::vm_size' is set to size of 'struct kvm_tdx'
> (when it's larger) but TDX actually fails to initialize at a later time.
> 
> Again the worst case of this is wasting couple of bytes memory for each
> VM.  KVM could choose to update 'kvm_x86_ops::vm_size' at a later time
> depending on TDX's status but that would require base KVM module to
> export either kvm_x86_ops or kvm_ops_update().
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

The ugly part here is the type-unsafety of to_vmx/to_tdx.  We probably 
should add some "#pragma poison" of to_vmx/to_tdx: for example both can 
be poisoned in pmu_intel.c after the definition of 
vcpu_to_lbr_records(), while one of them can be poisoned in 
sgx.c/posted_intr.c/vmx.c/tdx.c.  Not a strict requirement though.

Paolo

Re: [PATCH 07/25] KVM: TDX: Add helper functions to allocate/free TDX private host key id

[Paolo Bonzini]

On 8/13/24 00:48, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> Add helper functions to allocate/free TDX private host key id (HKID).
> 
> The memory controller encrypts TDX memory with the assigned HKIDs. Each TDX
> guest must be protected by its own unique TDX HKID.
> 
> The HW has a fixed set of these HKID keys. Out of those, some are set aside
> for use by for other TDX components, but most are saved for guest use. The
> code that does this partitioning, records the range chosen to be available
> for guest use in the tdx_guest_keyid_start and tdx_nr_guest_keyids
> variables.
> 
> Use this range of HKIDs reserved for guest use with the kernel's IDA
> allocator library helper to create a mini TDX HKID allocator that can be
> called when setting up a TD. This way it can have an exclusive HKID, as is
> required. This allocator will be used in future changes.

This is basically what Dave was asking for, isn't it?

Paolo

> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> uAPI breakout v1:
>   - Update the commit message
>   - Delete stale comment on global hkdi
>   - Deleted WARN_ON_ONCE() as it doesn't seemed very usefull
> 
> v19:
>   - Removed stale comment in tdx_guest_keyid_alloc() by Binbin
>   - Update sanity check in tdx_guest_keyid_free() by Binbin
> 
> v18:
>   - Moved the functions to kvm tdx from arch/x86/virt/vmx/tdx/
>   - Drop exporting symbols as the host tdx does.
> ---
>   arch/x86/kvm/vmx/tdx.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index dbcc1ed80efa..b1c885ce8c9c 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -14,6 +14,21 @@ static enum cpuhp_state tdx_cpuhp_state;
>   
>   static const struct tdx_sysinfo *tdx_sysinfo;
>   
> +/* TDX KeyID pool */
> +static DEFINE_IDA(tdx_guest_keyid_pool);
> +
> +static int __used tdx_guest_keyid_alloc(void)
> +{
> +	return ida_alloc_range(&tdx_guest_keyid_pool, tdx_guest_keyid_start,
> +			       tdx_guest_keyid_start + tdx_nr_guest_keyids - 1,
> +			       GFP_KERNEL);
> +}
> +
> +static void __used tdx_guest_keyid_free(int keyid)
> +{
> +	ida_free(&tdx_guest_keyid_pool, keyid);
> +}
> +
>   static int tdx_online_cpu(unsigned int cpu)
>   {
>   	unsigned long flags;

Re: [PATCH 07/25] KVM: TDX: Add helper functions to allocate/free TDX private host key id

[Edgecombe, Rick P]

On Tue, 2024-09-10 at 18:27 +0200, Paolo Bonzini wrote:
> On 8/13/24 00:48, Rick Edgecombe wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> > Add helper functions to allocate/free TDX private host key id (HKID).
> > 
> > The memory controller encrypts TDX memory with the assigned HKIDs. Each TDX
> > guest must be protected by its own unique TDX HKID.
> > 
> > The HW has a fixed set of these HKID keys. Out of those, some are set aside
> > for use by for other TDX components, but most are saved for guest use. The
> > code that does this partitioning, records the range chosen to be available
> > for guest use in the tdx_guest_keyid_start and tdx_nr_guest_keyids
> > variables.
> > 
> > Use this range of HKIDs reserved for guest use with the kernel's IDA
> > allocator library helper to create a mini TDX HKID allocator that can be
> > called when setting up a TD. This way it can have an exclusive HKID, as is
> > required. This allocator will be used in future changes.
> 
> This is basically what Dave was asking for, isn't it?

This patch has the allocator in KVM code, and the keyid ranges exported from
arch/x86. Per the discussion with Dave we will export the allocator functions
and keep the keyid ranges in arch/x86 code.

Re: [PATCH 07/25] KVM: TDX: Add helper functions to allocate/free TDX private host key id

[Paolo Bonzini]

On 9/10/24 18:39, Edgecombe, Rick P wrote:
>>> Use this range of HKIDs reserved for guest use with the kernel's IDA
>>> allocator library helper to create a mini TDX HKID allocator that can be
>>> called when setting up a TD. This way it can have an exclusive HKID, as is
>>> required. This allocator will be used in future changes.
>> This is basically what Dave was asking for, isn't it?
> This patch has the allocator in KVM code, and the keyid ranges exported from
> arch/x86. Per the discussion with Dave we will export the allocator functions
> and keep the keyid ranges in arch/x86 code.

Yes, I meant this is the code and it just has to be moved to arch/x86. 
The only other function that is needed is a wrapper for ida_is_empty(), 
which is used in tdx_offline_cpu():

         /* No TD is running.  Allow any cpu to be offline. */
         if (ida_is_empty(&tdx_guest_keyid_pool))
                 return 0;

Paolo

Re: [PATCH 07/25] KVM: TDX: Add helper functions to allocate/free TDX private host key id

[Edgecombe, Rick P]

On Tue, 2024-09-10 at 18:42 +0200, Paolo Bonzini wrote:
> Yes, I meant this is the code and it just has to be moved to arch/x86. 
> The only other function that is needed is a wrapper for ida_is_empty(), 
> which is used in tdx_offline_cpu():
> 
>          /* No TD is running.  Allow any cpu to be offline. */
>          if (ida_is_empty(&tdx_guest_keyid_pool))
>                  return 0;

Oh, good point.

Re: [PATCH 15/25] KVM: TDX: Make pmu_intel.c ignore guest TD case

[Paolo Bonzini]

On 8/13/24 00:48, Rick Edgecombe wrote:
> From: Isaku Yamahata<isaku.yamahata@intel.com>
> 
> Because TDX KVM doesn't support PMU yet (it's future work of TDX KVM
> support as another patch series) and pmu_intel.c touches vmx specific
> structure in vcpu initialization, as workaround add dummy structure to
> struct vcpu_tdx and pmu_intel.c can ignore TDX case.
> 
> Signed-off-by: Isaku Yamahata<isaku.yamahata@intel.com>
> Signed-off-by: Rick Edgecombe<rick.p.edgecombe@intel.com>

Would be nicer not to have this dummy member at all if possible.

Could vcpu_to_lbr_desc() return NULL, and then lbr_desc can be checked 
in intel_pmu_init() and intel_pmu_refresh()?  Then the checks for 
is_td_vcpu(vcpu), both inside WARN_ON_ONCE() and outside, can also be 
changed to check NULL-ness of vcpu_to_lbr_desc().

Also please add a WARN_ON_ONCE(is_td_vcpu(vcpu)), or 
WARN_ON_ONCE(!lbr_desc) given the above suggestion, to return early from 
vmx_passthrough_lbr_msrs().

Thanks,

Paolo

Re: [PATCH 15/25] KVM: TDX: Make pmu_intel.c ignore guest TD case

[Tony Lindgren]

On Tue, Sep 10, 2024 at 07:23:10PM +0200, Paolo Bonzini wrote:
> On 8/13/24 00:48, Rick Edgecombe wrote:
> > From: Isaku Yamahata<isaku.yamahata@intel.com>
> > 
> > Because TDX KVM doesn't support PMU yet (it's future work of TDX KVM
> > support as another patch series) and pmu_intel.c touches vmx specific
> > structure in vcpu initialization, as workaround add dummy structure to
> > struct vcpu_tdx and pmu_intel.c can ignore TDX case.
> > 
> > Signed-off-by: Isaku Yamahata<isaku.yamahata@intel.com>
> > Signed-off-by: Rick Edgecombe<rick.p.edgecombe@intel.com>
> 
> Would be nicer not to have this dummy member at all if possible.
>
> Could vcpu_to_lbr_desc() return NULL, and then lbr_desc can be checked in
> intel_pmu_init() and intel_pmu_refresh()?  Then the checks for
> is_td_vcpu(vcpu), both inside WARN_ON_ONCE() and outside, can also be
> changed to check NULL-ness of vcpu_to_lbr_desc().

Just catching up on this one, returning NULL works nice. Also for
vcpu_to_lbr_records() we need to return NULL.

Also the ifdefs around the is_td_vcpu() checks should not be needed as
is_td_vcpu() returns false unless CONFIG_INTEL_TDX_HOST is set.

> Also please add a WARN_ON_ONCE(is_td_vcpu(vcpu)), or WARN_ON_ONCE(!lbr_desc)
> given the above suggestion, to return early from vmx_passthrough_lbr_msrs().

Yes will add.

Regards,

Tony

Re: [PATCH 22/25] KVM: TDX: Use guest physical address to configure EPT level and GPAW

[Paolo Bonzini]

On 8/13/24 00:48, Rick Edgecombe wrote:
> @@ -576,6 +575,9 @@ static int setup_tdparams_cpuids(struct kvm_cpuid2 *cpuid,
>   		value->ebx = entry->ebx;
>   		value->ecx = entry->ecx;
>   		value->edx = entry->edx;
> +
> +		if (c->leaf == 0x80000008)
> +			value->eax &= 0xff00ffff;
>   	}
>   
>   	return 0;

Ah, this answers my question in 21/25.  It definitely needs a comment 
though!  Also to explain what will future support in the TDX module look 
like (a new feature bit I guess).

Paolo

Re: [PATCH 22/25] KVM: TDX: Use guest physical address to configure EPT level and GPAW

[Xiaoyao Li]

On 8/13/2024 6:48 AM, Rick Edgecombe wrote:
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> KVM reports guest physical address in CPUID.0x800000008.EAX[23:16],
> which is similar to TDX's GPAW. Use this field as the interface for
> userspace to configure the GPAW and EPT level for TDs.
> 
> Note,
> 
> 1. only value 48 and 52 are supported. 52 means GPAW-52 and EPT level
>     5, and 48 means GPAW-48 and EPT level 4.
> 2. value 48, i.e., GPAW-48 is always supported. value 52 is only
>     supported when the platform supports 5 level EPT.
> 
> Current TDX module doesn't support max_gpa configuration. However
> current implementation relies on max_gpa to configure  EPT level and
> GPAW. Hack KVM to make it work.

This patch needs to be squashed into patch 14.

> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> uAPI breakout v1:
>   - New patch
> ---
>   arch/x86/kvm/vmx/tdx.c | 32 +++++++++++++++++++-------------
>   1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index fe2bbc2ced41..c6bfeb0b3cc9 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -514,23 +514,22 @@ static int setup_tdparams_eptp_controls(struct kvm_cpuid2 *cpuid,
>   					struct td_params *td_params)
>   {
>   	const struct kvm_cpuid_entry2 *entry;
> -	int max_pa = 36;
> +	int guest_pa;
>   
>   	entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent, 0x80000008, 0);
> -	if (entry)
> -		max_pa = entry->eax & 0xff;
> +	if (!entry)
> +		return -EINVAL;
> +
> +	guest_pa = (entry->eax >> 16) & 0xff;
> +
> +	if (guest_pa != 48 && guest_pa != 52)
> +		return -EINVAL;
> +
> +	if (guest_pa == 52 && !cpu_has_vmx_ept_5levels())
> +		return -EINVAL;
>   
>   	td_params->eptp_controls = VMX_EPTP_MT_WB;
> -	/*
> -	 * No CPU supports 4-level && max_pa > 48.
> -	 * "5-level paging and 5-level EPT" section 4.1 4-level EPT
> -	 * "4-level EPT is limited to translating 48-bit guest-physical
> -	 *  addresses."
> -	 * cpu_has_vmx_ept_5levels() check is just in case.
> -	 */
> -	if (!cpu_has_vmx_ept_5levels() && max_pa > 48)
> -		return -EINVAL;
> -	if (cpu_has_vmx_ept_5levels() && max_pa > 48) {
> +	if (guest_pa == 52) {
>   		td_params->eptp_controls |= VMX_EPTP_PWL_5;
>   		td_params->exec_controls |= TDX_EXEC_CONTROL_MAX_GPAW;
>   	} else {
> @@ -576,6 +575,9 @@ static int setup_tdparams_cpuids(struct kvm_cpuid2 *cpuid,
>   		value->ebx = entry->ebx;
>   		value->ecx = entry->ecx;
>   		value->edx = entry->edx;
> +
> +		if (c->leaf == 0x80000008)
> +			value->eax &= 0xff00ffff;
>   	}
>   
>   	return 0;
> @@ -1277,6 +1279,10 @@ static int __init setup_kvm_tdx_caps(void)
>   		memcpy(dest, &source, sizeof(struct kvm_tdx_cpuid_config));
>   		if (dest->sub_leaf == KVM_TDX_CPUID_NO_SUBLEAF)
>   			dest->sub_leaf = 0;
> +
> +		/* Work around missing support on old TDX modules */
> +		if (dest->leaf == 0x80000008)
> +			dest->eax |= 0x00ff0000;
>   	}
>   
>   	return 0;

Re: [PATCH 22/25] KVM: TDX: Use guest physical address to configure EPT level and GPAW

[Tony Lindgren]

On Thu, Oct 10, 2024 at 05:13:43PM +0800, Xiaoyao Li wrote:
> On 8/13/2024 6:48 AM, Rick Edgecombe wrote:
> > From: Xiaoyao Li <xiaoyao.li@intel.com>
> > 
> > KVM reports guest physical address in CPUID.0x800000008.EAX[23:16],
> > which is similar to TDX's GPAW. Use this field as the interface for
> > userspace to configure the GPAW and EPT level for TDs.
> > 
> > Note,
> > 
> > 1. only value 48 and 52 are supported. 52 means GPAW-52 and EPT level
> >     5, and 48 means GPAW-48 and EPT level 4.
> > 2. value 48, i.e., GPAW-48 is always supported. value 52 is only
> >     supported when the platform supports 5 level EPT.
> > 
> > Current TDX module doesn't support max_gpa configuration. However
> > current implementation relies on max_gpa to configure  EPT level and
> > GPAW. Hack KVM to make it work.
> 
> This patch needs to be squashed into patch 14.

Yes agreed that makes sense.

Regards,

Tony

Re: [PATCH 23/25] KVM: x86/mmu: Taking guest pa into consideration when calculate tdp level

[Paolo Bonzini]

On 8/13/24 00:48, Rick Edgecombe wrote:
> +	int maxpa = 0;
> +
> +	if (vcpu->kvm->arch.vm_type == KVM_X86_TDX_VM)
> +		maxpa = cpuid_query_maxguestphyaddr(vcpu);

I think the "if" should be in cpuid_query_maxguestphyaddr(), or 
alternatively the test below should become an "else".  The current 
combination is not the clearest.

Otherwise,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> +	if (!maxpa)
> +		maxpa = cpuid_maxphyaddr(vcpu);
> +

Re: [PATCH 24/25] KVM: x86: Filter directly configurable TDX CPUID bits

[Tony Lindgren]

On Mon, Aug 19, 2024 at 01:02:41PM +0800, Xu Yilun wrote:
> On Mon, Aug 12, 2024 at 03:48:19PM -0700, Rick Edgecombe wrote:
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -1086,8 +1086,9 @@ static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
> >  	return ret;
> >  }
> >  
> > -static int __maybe_unused tdx_get_kvm_supported_cpuid(struct kvm_cpuid2 **cpuid)
> > +static int tdx_get_kvm_supported_cpuid(struct kvm_cpuid2 **cpuid)
> 
> This func is already used in patch #21, put the change in that patch.
> 
> >  {
> > +
> 
> remove the blank line.
> 
> >  	int r;

Yes looks like that got removed in patch 25/25.

Tony

Re: [PATCH 24/25] KVM: x86: Filter directly configurable TDX CPUID bits

[Paolo Bonzini]

On 8/13/24 00:48, Rick Edgecombe wrote:
> +
> +		cpuid_e = kvm_find_cpuid_entry2(supported_cpuid->entries, supported_cpuid->nent,
> +						dest->leaf, dest->sub_leaf);
> +		if (!cpuid_e) {
> +			dest->eax = dest->ebx = dest->ecx = dest->edx = 0;
> +		} else {
> +			dest->eax &= cpuid_e->eax;
> +			dest->ebx &= cpuid_e->ebx;
> +			dest->ecx &= cpuid_e->ecx;
> +			dest->edx &= cpuid_e->edx;
> +		}

This can only work with CPUID entries that consists of 4*32 features, so 
it has to be done specifically for each leaf, unfortunately.  I suggest 
defining a kvm_merge_cpuid_entries in cpuid.c that takes two struct 
cpuid_entry2* that refer to the same leaf and subleaf.

Paolo

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Paolo Bonzini]

On 8/13/24 00:48, Rick Edgecombe wrote:
> Originally, the plan was to filter the directly configurable CPUID bits
> exposed by KVM_TDX_CAPABILITIES, and the final configured bit values
> provided by KVM_TDX_GET_CPUID. However, several issues were found with
> this. Both the filtering done with KVM_TDX_CAPABILITIES and
> KVM_TDX_GET_CPUID had the issue that the get_supported_cpuid() provided
> default values instead of supported masks for multi-bit fields (i.e. those
> encoding a multi-bit number).
> 
> For KVM_TDX_CAPABILITIES, there was also the problem of bits that are
> actually supported by KVM, but missing from get_supported_cpuid() for one
> reason or another. These include X86_FEATURE_MWAIT, X86_FEATURE_HT and
> X86_FEATURE_TSC_DEADLINE_TIMER. This is currently worked around in QEMU by
> adjusting which features are expected. Some of these are going to be added
> to get_supported_cpuid(), and that is probably the right long term fix.

There are several cases here:

- MWAIT is hidden because it's hard to virtualize its C-state parameters

- HT is hidden because it depends on the topology, and cannot be added 
blindly.

- TSC_DEADLINE_TIMER is queried with KVM_CHECK_EXTENSION for historical 
reasons

There are basically two kinds of userspace:

- those that fetch KVM_GET_SUPPORED_CPUID and pass it blindly to 
KVM_SET_CPUID2.  These mostly work, though they may miss a feature or 
three (e.g. the TSC deadline timer).

- those that know each bit and make an informed decision on what to 
enable; for those, KVM_GET_SUPPORTED_CPUID is just guidance.

Because of this, KVM_GET_SUPPORTED_CPUID doesn't return bits that are 
one; it returns a mix of:

- maximum supported values (e.g. CPUID[7,0].EAX)

- values from the host (e.g. FMS or model name)

- supported features

It's an awfully defined API but it is easier to use than it sounds (some 
of the quirks are being documented in 
Documentation/virt/kvm/x86/errata.rst and 
Documentation/virt/kvm/x86/api.rst).  The idea is that, if userspace 
manages individual CPUID bits, it already knows what can be one anyway.

This is the kind of API that we need to present for TDX, even if the 
details on how to get the supported CPUID are different.  Not because 
it's a great API, but rather because it's a known API.

The difference between this and KVM_GET_SUPPORTED_CPUID are small, but 
the main one is X86_FEATURE_HYPERVISOR (I am not sure whether to make it 
different with respect to X86_FEATURE_TSC_DEADLINE_TIMER; leaning 
towards no).

We may also need a second ioctl specifically to return the fixed-1 bits. 
  Asking Xiaoyao for input with regard to what he'd like to have in QEMU.

> +	entry = kvm_find_cpuid_entry2((*cpuid)->entries, (*cpuid)->nent, 0x0, 0);
> +	if (WARN_ON(!entry))
> +		goto err;
> +	/* Fixup of maximum basic leaf */
> +	entry->eax |= 0x000000FF;
> +
> +	entry = kvm_find_cpuid_entry2((*cpuid)->entries, (*cpuid)->nent, 0x1, 0);
> +	if (WARN_ON(!entry))
> +		goto err;
> +	/* Fixup of FMS */
> +	entry->eax |= 0x0fff3fff;
> +	/* Fixup of maximum logical processors per package */
> +	entry->ebx |= 0x00ff0000;
> +

I see now why you could blindly AND things in patch 24.

However, the right mode of operation is still to pick manually which 
bits to AND.

Paolo

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Xiaoyao Li]

On 9/11/2024 1:52 AM, Paolo Bonzini wrote:
> On 8/13/24 00:48, Rick Edgecombe wrote:
>> Originally, the plan was to filter the directly configurable CPUID bits
>> exposed by KVM_TDX_CAPABILITIES, and the final configured bit values
>> provided by KVM_TDX_GET_CPUID. However, several issues were found with
>> this. Both the filtering done with KVM_TDX_CAPABILITIES and
>> KVM_TDX_GET_CPUID had the issue that the get_supported_cpuid() provided
>> default values instead of supported masks for multi-bit fields (i.e. 
>> those
>> encoding a multi-bit number).
>>
>> For KVM_TDX_CAPABILITIES, there was also the problem of bits that are
>> actually supported by KVM, but missing from get_supported_cpuid() for one
>> reason or another. These include X86_FEATURE_MWAIT, X86_FEATURE_HT and
>> X86_FEATURE_TSC_DEADLINE_TIMER. This is currently worked around in 
>> QEMU by
>> adjusting which features are expected. 

I'm not sure what issuee/problem can be worked around in QEMU.

QEMU doesn't expect these bit are reported by KVM as supported for TDX. 
QEMU just accepts the result reported by KVM.

The problem is, TDX module and the hardware allow these bits be 
configured for TD guest, but KVM doesn't allow. It leads to users cannot 
create a TD with these bits on.

QEMU cannot work around this problem.

>> Some of these are going to be 
>> added
>> to get_supported_cpuid(), and that is probably the right long term fix.
> 
> There are several cases here:
> 
> - MWAIT is hidden because it's hard to virtualize its C-state parameters
> 
> - HT is hidden because it depends on the topology, and cannot be added 
> blindly.
> 
> - TSC_DEADLINE_TIMER is queried with KVM_CHECK_EXTENSION for historical 
> reasons
> 
> There are basically two kinds of userspace:
> 
> - those that fetch KVM_GET_SUPPORED_CPUID and pass it blindly to 
> KVM_SET_CPUID2.  These mostly work, though they may miss a feature or 
> three (e.g. the TSC deadline timer).
> 
> - those that know each bit and make an informed decision on what to 
> enable; for those, KVM_GET_SUPPORTED_CPUID is just guidance.
> 
> Because of this, KVM_GET_SUPPORTED_CPUID doesn't return bits that are 
> one; it returns a mix of:
> 
> - maximum supported values (e.g. CPUID[7,0].EAX)
> 
> - values from the host (e.g. FMS or model name)
> 
> - supported features
> 
> It's an awfully defined API but it is easier to use than it sounds (some 
> of the quirks are being documented in 
> Documentation/virt/kvm/x86/errata.rst and 
> Documentation/virt/kvm/x86/api.rst).  The idea is that, if userspace 
> manages individual CPUID bits, it already knows what can be one anyway.
> 
> This is the kind of API that we need to present for TDX, even if the 
> details on how to get the supported CPUID are different.  Not because 
> it's a great API, but rather because it's a known API.

However there are differences for TDX. For legacy VMs, the result of 
KVM_GET_SUPPORTED_CPUID isn't used to filter the input of KVM_SET_CPUID2.

But for TDX, it needs to filter the input of KVM_TDX_VM_INIT.CPUID[] 
because TDX module only allows the bits that are reported as 
configurable to be set to 1.

> The difference between this and KVM_GET_SUPPORTED_CPUID are small, but 
> the main one is X86_FEATURE_HYPERVISOR (I am not sure whether to make it 
> different with respect to X86_FEATURE_TSC_DEADLINE_TIMER; leaning 
> towards no).
> 
> We may also need a second ioctl specifically to return the fixed-1 bits. 
>   Asking Xiaoyao for input with regard to what he'd like to have in QEMU.

With current designed API, QEMU can only know which bits are 
configurable before KVM_TDX_VM_INIT, i.e., which bits can be set to 1 or 
0 freely.

For other bits not reported as configurable, QEMU can know the exact 
value of them via KVM_TDX_GET_CPUID, after KVM_TDX_VM_INIT and before 
TD's running. With it, QEMU can validate the return value is matched 
with what QEMU wants to set that determined by users input. If not 
matched, QEMU can provide some warnings like what for legacy VMs:

   - TDX doesn't support requested feature: CPUID.01H.ECX.tsc-deadline 
[bit 24]
   - TDX forcibly sets features: CPUID.01H:ECX.hypervisor [bit 31]

If there are ioctls to report the fixed0 bits and fixed1 bits for TDX, 
QEMU can validate the user's configuration earlier.

>> +    entry = kvm_find_cpuid_entry2((*cpuid)->entries, (*cpuid)->nent, 
>> 0x0, 0);
>> +    if (WARN_ON(!entry))
>> +        goto err;
>> +    /* Fixup of maximum basic leaf */
>> +    entry->eax |= 0x000000FF;
>> +
>> +    entry = kvm_find_cpuid_entry2((*cpuid)->entries, (*cpuid)->nent, 
>> 0x1, 0);
>> +    if (WARN_ON(!entry))
>> +        goto err;
>> +    /* Fixup of FMS */
>> +    entry->eax |= 0x0fff3fff;
>> +    /* Fixup of maximum logical processors per package */
>> +    entry->ebx |= 0x00ff0000;
>> +
> 
> I see now why you could blindly AND things in patch 24.
> 
> However, the right mode of operation is still to pick manually which 
> bits to AND.
> 
> Paolo
> 

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Paolo Bonzini]

On Thu, Sep 12, 2024 at 9:48 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> On 9/11/2024 1:52 AM, Paolo Bonzini wrote:
> > On 8/13/24 00:48, Rick Edgecombe wrote:
> >> For KVM_TDX_CAPABILITIES, there was also the problem of bits that are
> >> actually supported by KVM, but missing from get_supported_cpuid() for one
> >> reason or another. These include X86_FEATURE_MWAIT, X86_FEATURE_HT and
> >> X86_FEATURE_TSC_DEADLINE_TIMER. This is currently worked around in
> >> QEMU by
> >> adjusting which features are expected.
>
> I'm not sure what issue/problem can be worked around in QEMU.
> QEMU doesn't expect these bit are reported by KVM as supported for TDX.
> QEMU just accepts the result reported by KVM.

QEMU already adds some extra bits, for example:

        ret |= CPUID_EXT_HYPERVISOR;
        if (kvm_irqchip_in_kernel() &&
                kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
            ret |= CPUID_EXT_TSC_DEADLINE_TIMER;
        }

> The problem is, TDX module and the hardware allow these bits be
> configured for TD guest, but KVM doesn't allow. It leads to users cannot
> create a TD with these bits on.

KVM is not going to have any checks, it's only going to pass the
CPUID to the TDX module and return an error if the check fails
in the TDX module.

KVM can have a TDX-specific version of KVM_GET_SUPPORTED_CPUID, so
that we can keep a variant of the "get supported bits and pass them
to KVM_SET_CPUID2" logic, but that's it.

> > This is the kind of API that we need to present for TDX, even if the
> > details on how to get the supported CPUID are different.  Not because
> > it's a great API, but rather because it's a known API.
>
> However there are differences for TDX. For legacy VMs, the result of
> KVM_GET_SUPPORTED_CPUID isn't used to filter the input of KVM_SET_CPUID2.
> But for TDX, it needs to filter the input of KVM_TDX_VM_INIT.CPUID[]
> because TDX module only allows the bits that are reported as
> configurable to be set to 1.

Yes, that's userspace's responsibility.

> With current designed API, QEMU can only know which bits are
> configurable before KVM_TDX_VM_INIT, i.e., which bits can be set to 1 or
> 0 freely.

The API needs userspace to have full knowledge of the
requirements of the TDX module, if it wants to change the
defaults provided by KVM.

This is the same as for non-TDX VMs (including SNP).  The only
difference is that TDX and SNP fails, while non-confidential VMs
get slightly garbage CPUID.

> For other bits not reported as configurable, QEMU can know the exact
> value of them via KVM_TDX_GET_CPUID, after KVM_TDX_VM_INIT and before
> TD's running. With it, QEMU can validate the return value is matched
> with what QEMU wants to set that determined by users input. If not
> matched, QEMU can provide some warnings like what for legacy VMs:
>
>    - TDX doesn't support requested feature: CPUID.01H.ECX.tsc-deadline
> [bit 24]
>    - TDX forcibly sets features: CPUID.01H:ECX.hypervisor [bit 31]
>
> If there are ioctls to report the fixed0 bits and fixed1 bits for TDX,
> QEMU can validate the user's configuration earlier.

Yes, that's fine.

Paolo

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Xiaoyao Li]

On 9/12/2024 10:09 PM, Paolo Bonzini wrote:
> On Thu, Sep 12, 2024 at 9:48 AM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>> On 9/11/2024 1:52 AM, Paolo Bonzini wrote:
>>> On 8/13/24 00:48, Rick Edgecombe wrote:
>>>> For KVM_TDX_CAPABILITIES, there was also the problem of bits that are
>>>> actually supported by KVM, but missing from get_supported_cpuid() for one
>>>> reason or another. These include X86_FEATURE_MWAIT, X86_FEATURE_HT and
>>>> X86_FEATURE_TSC_DEADLINE_TIMER. This is currently worked around in
>>>> QEMU by
>>>> adjusting which features are expected.
>>
>> I'm not sure what issue/problem can be worked around in QEMU.
>> QEMU doesn't expect these bit are reported by KVM as supported for TDX.
>> QEMU just accepts the result reported by KVM.
> 
> QEMU already adds some extra bits, for example:
> 
>          ret |= CPUID_EXT_HYPERVISOR;
>          if (kvm_irqchip_in_kernel() &&
>                  kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
>              ret |= CPUID_EXT_TSC_DEADLINE_TIMER;
>          }
> 
>> The problem is, TDX module and the hardware allow these bits be
>> configured for TD guest, but KVM doesn't allow. It leads to users cannot
>> create a TD with these bits on.
> 
> KVM is not going to have any checks, it's only going to pass the
> CPUID to the TDX module and return an error if the check fails
> in the TDX module.

If so, new feature can be enabled for TDs out of KVM's control.

Is it acceptable?

> KVM can have a TDX-specific version of KVM_GET_SUPPORTED_CPUID, so
> that we can keep a variant of the "get supported bits and pass them
> to KVM_SET_CPUID2" logic, but that's it.
> 
>>> This is the kind of API that we need to present for TDX, even if the
>>> details on how to get the supported CPUID are different.  Not because
>>> it's a great API, but rather because it's a known API.
>>
>> However there are differences for TDX. For legacy VMs, the result of
>> KVM_GET_SUPPORTED_CPUID isn't used to filter the input of KVM_SET_CPUID2.
>> But for TDX, it needs to filter the input of KVM_TDX_VM_INIT.CPUID[]
>> because TDX module only allows the bits that are reported as
>> configurable to be set to 1.
> 
> Yes, that's userspace's responsibility.
> 
>> With current designed API, QEMU can only know which bits are
>> configurable before KVM_TDX_VM_INIT, i.e., which bits can be set to 1 or
>> 0 freely.
> 
> The API needs userspace to have full knowledge of the
> requirements of the TDX module, if it wants to change the
> defaults provided by KVM.
> 
> This is the same as for non-TDX VMs (including SNP).  The only
> difference is that TDX and SNP fails, while non-confidential VMs
> get slightly garbage CPUID.
> 
>> For other bits not reported as configurable, QEMU can know the exact
>> value of them via KVM_TDX_GET_CPUID, after KVM_TDX_VM_INIT and before
>> TD's running. With it, QEMU can validate the return value is matched
>> with what QEMU wants to set that determined by users input. If not
>> matched, QEMU can provide some warnings like what for legacy VMs:
>>
>>     - TDX doesn't support requested feature: CPUID.01H.ECX.tsc-deadline
>> [bit 24]
>>     - TDX forcibly sets features: CPUID.01H:ECX.hypervisor [bit 31]
>>
>> If there are ioctls to report the fixed0 bits and fixed1 bits for TDX,
>> QEMU can validate the user's configuration earlier.
> 
> Yes, that's fine.
> 
> Paolo
> 

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Paolo Bonzini]

On Thu, Sep 12, 2024 at 4:45 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> > KVM is not going to have any checks, it's only going to pass the
> > CPUID to the TDX module and return an error if the check fails
> > in the TDX module.
>
> If so, new feature can be enabled for TDs out of KVM's control.
>
> Is it acceptable?

It's the same as for non-TDX VMs, I think it's acceptable.

Paolo

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Xiaoyao Li]

On 9/12/2024 10:48 PM, Paolo Bonzini wrote:
> On Thu, Sep 12, 2024 at 4:45 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>>> KVM is not going to have any checks, it's only going to pass the
>>> CPUID to the TDX module and return an error if the check fails
>>> in the TDX module.
>>
>> If so, new feature can be enabled for TDs out of KVM's control.
>>
>> Is it acceptable?
> 
> It's the same as for non-TDX VMs, I think it's acceptable.

another question is for patch 24, will we keep the filtering of the 
configurable CPUDIDs in KVM_TDX_CAPABILITIES with KVM_GET_SUPPORTED_CPUID?

> Paolo
> 

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Sean Christopherson]

On Thu, Sep 12, 2024, Paolo Bonzini wrote:
> On Thu, Sep 12, 2024 at 4:45 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> > > KVM is not going to have any checks, it's only going to pass the
> > > CPUID to the TDX module and return an error if the check fails
> > > in the TDX module.
> >
> > If so, new feature can be enabled for TDs out of KVM's control.
> >
> > Is it acceptable?
> 
> It's the same as for non-TDX VMs, I think it's acceptable.

No?  IIUC, it's not the same.

E.g. KVM doesn't yet support CET, and while userspace can enumerate CET support
to VMs all it wants, guests will never be able to set CR4.CET and thus can't
actually enable CET.

IIUC, the proposal here is to allow userspace to configure the features that are
exposed _and enabled_ for a TDX VM without any enforcement from KVM.

CET might be a bad example because it looks like it's controlled by TDCS.XFAM, but
presumably there are other CPUID-based features that would actively enable some
feature for a TDX VM.

For HYPERVISOR and TSC_DEADLINE_TIMER, I would much prefer to fix those KVM warts,
and have already posted patches[1][2] to do exactly that.

With those out of the way, are there any other CPUID-based features that KVM
supports, but doesn't advertise?  Ignore MWAIT, it's a special case and isn't
allowed in TDX VMs anyways.

[1] https://lore.kernel.org/all/20240517173926.965351-34-seanjc@google.com
[2] https://lore.kernel.org/all/20240517173926.965351-35-seanjc@google.com

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Paolo Bonzini]

On Thu, Sep 12, 2024 at 6:42 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Sep 12, 2024, Paolo Bonzini wrote:
> > On Thu, Sep 12, 2024 at 4:45 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> > > > KVM is not going to have any checks, it's only going to pass the
> > > > CPUID to the TDX module and return an error if the check fails
> > > > in the TDX module.
> > >
> > > If so, new feature can be enabled for TDs out of KVM's control.
> > >
> > > Is it acceptable?
> >
> > It's the same as for non-TDX VMs, I think it's acceptable.
>
> No?  IIUC, it's not the same.
>
> E.g. KVM doesn't yet support CET, and while userspace can enumerate CET support
> to VMs all it wants, guests will never be able to set CR4.CET and thus can't
> actually enable CET.
>
> IIUC, the proposal here is to allow userspace to configure the features that are
> exposed _and enabled_ for a TDX VM without any enforcement from KVM.

Yeah, that's correct, on the other hand a lot of features are just
new instructions and no new registers.  Those pass under the radar
and in fact you can even use them if the CPUID bit is 0 (of course).
Others are just data, and again you can pass any crap you'd like.

And for SNP we had the case where we are forced to leave features
enabled if their state is in the VMSA, because we cannot block
writes to XCR0 and XSS that we'd like to be invalid.

> CET might be a bad example because it looks like it's controlled by TDCS.XFAM, but
> presumably there are other CPUID-based features that would actively enable some
> feature for a TDX VM.

XFAM is controlled by userspace though, not KVM, so we've got no
control on that either.

> For HYPERVISOR and TSC_DEADLINE_TIMER, I would much prefer to fix those KVM warts,
> and have already posted patches[1][2] to do exactly that.
>
> With those out of the way, are there any other CPUID-based features that KVM
> supports, but doesn't advertise?  Ignore MWAIT, it's a special case and isn't
> allowed in TDX VMs anyways.

I don't think so.

Paolo

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Sean Christopherson]

On Thu, Sep 12, 2024, Paolo Bonzini wrote:
> On Thu, Sep 12, 2024 at 6:42 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Sep 12, 2024, Paolo Bonzini wrote:
> > > On Thu, Sep 12, 2024 at 4:45 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
> > > > > KVM is not going to have any checks, it's only going to pass the
> > > > > CPUID to the TDX module and return an error if the check fails
> > > > > in the TDX module.
> > > >
> > > > If so, new feature can be enabled for TDs out of KVM's control.
> > > >
> > > > Is it acceptable?
> > >
> > > It's the same as for non-TDX VMs, I think it's acceptable.
> >
> > No?  IIUC, it's not the same.
> >
> > E.g. KVM doesn't yet support CET, and while userspace can enumerate CET support
> > to VMs all it wants, guests will never be able to set CR4.CET and thus can't
> > actually enable CET.
> >
> > IIUC, the proposal here is to allow userspace to configure the features that are
> > exposed _and enabled_ for a TDX VM without any enforcement from KVM.
> 
> Yeah, that's correct, on the other hand a lot of features are just
> new instructions and no new registers.  Those pass under the radar
> and in fact you can even use them if the CPUID bit is 0 (of course).
> Others are just data, and again you can pass any crap you'd like.

Right, I don't care about those precisely because there's nothing KVM can or
_needs_ to do for features that don't have interception controls.

> And for SNP we had the case where we are forced to leave features
> enabled if their state is in the VMSA, because we cannot block
> writes to XCR0 and XSS that we'd like to be invalid.

Oh, I'm well aware :-)

> > CET might be a bad example because it looks like it's controlled by TDCS.XFAM, but
> > presumably there are other CPUID-based features that would actively enable some
> > feature for a TDX VM.
> 
> XFAM is controlled by userspace though, not KVM, so we've got no
> control on that either.

I assume it's plain text though?  I.e. whatever ioctl() sets TDCS.XFAM can be
rejected by KVM if it attempts to enable unsupported features?

I don't expect that we'll want KVM to gatekeep many, if any features, but I do
think we should require explicit enabling in KVM whenever possible, even if the
enabling is boring and largely ceremonial.

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Xiaoyao Li]

On 9/13/2024 2:41 AM, Sean Christopherson wrote:
>>> CET might be a bad example because it looks like it's controlled by TDCS.XFAM, but
>>> presumably there are other CPUID-based features that would actively enable some
>>> feature for a TDX VM.
>> XFAM is controlled by userspace though, not KVM, so we've got no
>> control on that either.
> I assume it's plain text though?  I.e. whatever ioctl() sets TDCS.XFAM can be
> rejected by KVM if it attempts to enable unsupported features?

yes. XFAM is validated by KVM actually in this series.

KVM reports supported_xfam via KVM_TDX_CAPABILITIES and userspace sets 
XFAM via ioctl(KVM_TDX_VM_INIT). If userspace sets any bits beyond the 
supported_xfam, KVM returns -EINVAL.

The same for attributes.

> I don't expect that we'll want KVM to gatekeep many, if any features, but I do
> think we should require explicit enabling in KVM whenever possible, even if the
> enabling is boring and largely ceremonial.
+1

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Edgecombe, Rick P]

On Thu, 2024-09-12 at 20:29 +0200, Paolo Bonzini wrote:
> > IIUC, the proposal here is to allow userspace to configure the features that
> > are
> > exposed _and enabled_ for a TDX VM without any enforcement from KVM.
> 
> Yeah, that's correct, on the other hand a lot of features are just
> new instructions and no new registers.  Those pass under the radar
> and in fact you can even use them if the CPUID bit is 0 (of course).
> Others are just data, and again you can pass any crap you'd like.
> 
> And for SNP we had the case where we are forced to leave features
> enabled if their state is in the VMSA, because we cannot block
> writes to XCR0 and XSS that we'd like to be invalid.
> 
> > CET might be a bad example because it looks like it's controlled by
> > TDCS.XFAM, but
> > presumably there are other CPUID-based features that would actively enable
> > some
> > feature for a TDX VM.
> 
> XFAM is controlled by userspace though, not KVM, so we've got no
> control on that either.

There are some ATTRIBUTES (the non-xsave features like PKS get bucketed in
there), which can affect the host. So we have to filter this config in KVM. I'd
just assume not trust future XFAM bits because it's easy to implement.


> 
> > For HYPERVISOR and TSC_DEADLINE_TIMER, I would much prefer to fix those KVM
> > warts,
> > and have already posted patches[1][2] to do exactly that.
> > 
> > With those out of the way, are there any other CPUID-based features that KVM
> > supports, but doesn't advertise?  Ignore MWAIT, it's a special case and
> > isn't
> > allowed in TDX VMs anyways.
> 
> I don't think so.

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Xiaoyao Li]

On 9/13/2024 12:42 AM, Sean Christopherson wrote:
> On Thu, Sep 12, 2024, Paolo Bonzini wrote:
>> On Thu, Sep 12, 2024 at 4:45 PM Xiaoyao Li <xiaoyao.li@intel.com> wrote:
>>>> KVM is not going to have any checks, it's only going to pass the
>>>> CPUID to the TDX module and return an error if the check fails
>>>> in the TDX module.
>>>
>>> If so, new feature can be enabled for TDs out of KVM's control.
>>>
>>> Is it acceptable?
>>
>> It's the same as for non-TDX VMs, I think it's acceptable.
> 
> No?  IIUC, it's not the same.
> 
> E.g. KVM doesn't yet support CET, and while userspace can enumerate CET support
> to VMs all it wants, guests will never be able to set CR4.CET and thus can't
> actually enable CET.
> 
> IIUC, the proposal here is to allow userspace to configure the features that are
> exposed _and enabled_ for a TDX VM without any enforcement from KVM.
> 
> CET might be a bad example because it looks like it's controlled by TDCS.XFAM, but
> presumably there are other CPUID-based features that would actively enable some
> feature for a TDX VM.
> 
> For HYPERVISOR and TSC_DEADLINE_TIMER, I would much prefer to fix those KVM warts,
> and have already posted patches[1][2] to do exactly that.
> 
> With those out of the way, are there any other CPUID-based features that KVM
> supports, but doesn't advertise?  Ignore MWAIT, it's a special case and isn't
> allowed in TDX VMs anyways.

Actually MWAIT becoems allowed by TDX and it's configurable.

> [1] https://lore.kernel.org/all/20240517173926.965351-34-seanjc@google.com
> [2] https://lore.kernel.org/all/20240517173926.965351-35-seanjc@google.com
> 

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Edgecombe, Rick P]

On Thu, 2024-09-12 at 16:09 +0200, Paolo Bonzini wrote:
> 
> > The problem is, TDX module and the hardware allow these bits be
> > configured for TD guest, but KVM doesn't allow. It leads to users cannot
> > create a TD with these bits on.
> 
> KVM is not going to have any checks, it's only going to pass the
> CPUID to the TDX module and return an error if the check fails
> in the TDX module.

Ok. 

> 
> KVM can have a TDX-specific version of KVM_GET_SUPPORTED_CPUID, so
> that we can keep a variant of the "get supported bits and pass them
> to KVM_SET_CPUID2" logic, but that's it.

Can you clarify what you mean here when you say TDX-specific version of
KVM_GET_SUPPORTED_CPUID?

We have two things kind of like that implemented in this series:
1. KVM_TDX_GET_CPUID, which returns the CPUID bits actually set in the TD
2. KVM_TDX_CAPABILITIES, which returns CPUID bits that TDX module allows full
control over (i.e. what we have been calling directly configurable CPUID bits)

KVM_TDX_GET_CPUID->KVM_SET_CPUID2 kind of works like
KVM_GET_SUPPORTED_CPUID->KVM_SET_CPUID2, so I think that is what you mean, but
just want to confirm.

We can't get the needed information (fixed bits, etc) to create a TDX
KVM_GET_SUPPORTED_CPUID today from the TDX module, so we would have to encode it
into KVM. This was NAKed by Sean at some point. We have started looking into
exposing the needed info in the TDX module, but it is just starting.

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Paolo Bonzini]

On Thu, Sep 12, 2024 at 5:08 PM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
> > KVM can have a TDX-specific version of KVM_GET_SUPPORTED_CPUID, so
> > that we can keep a variant of the "get supported bits and pass them
> > to KVM_SET_CPUID2" logic, but that's it.
>
> Can you clarify what you mean here when you say TDX-specific version of
> KVM_GET_SUPPORTED_CPUID?
>
> We have two things kind of like that implemented in this series:
> 1. KVM_TDX_GET_CPUID, which returns the CPUID bits actually set in the TD
> 2. KVM_TDX_CAPABILITIES, which returns CPUID bits that TDX module allows full
> control over (i.e. what we have been calling directly configurable CPUID bits)
>
> KVM_TDX_GET_CPUID->KVM_SET_CPUID2 kind of works like
> KVM_GET_SUPPORTED_CPUID->KVM_SET_CPUID2, so I think that is what you mean, but
> just want to confirm.

Yes, that's correct.

> We can't get the needed information (fixed bits, etc) to create a TDX
> KVM_GET_SUPPORTED_CPUID today from the TDX module, so we would have to encode it
> into KVM. This was NAKed by Sean at some point. We have started looking into
> exposing the needed info in the TDX module, but it is just starting.

I think a bare minimum of this API is needed (adding HYPERVISOR,
and masking TDX-supported features against what KVM supports).
It's too much of a fundamental step in KVM's configuration API.

I am not sure if there are other fixed-1 bits than HYPERVISOR as of
today.  But in any case, if the TDX module breaks it unilaterally by
adding more fixed-1 bits, that's a problem for Intel not for KVM.

On the other hand is KVM_TDX_CAPABILITIES even needed?  If userspace
can replace that with hardcoded logic or info from the infamous JSON
file, that would work.

Paolo

Re: [PATCH 25/25] KVM: x86: Add CPUID bits missing from KVM_GET_SUPPORTED_CPUID

[Edgecombe, Rick P]

On Thu, 2024-09-12 at 17:37 +0200, Paolo Bonzini wrote:
> Yes, that's correct.

Thanks.

> 
> > We can't get the needed information (fixed bits, etc) to create a TDX
> > KVM_GET_SUPPORTED_CPUID today from the TDX module, so we would have to
> > encode it
> > into KVM. This was NAKed by Sean at some point. We have started looking into
> > exposing the needed info in the TDX module, but it is just starting.
> 
> I think a bare minimum of this API is needed (adding HYPERVISOR,
> and masking TDX-supported features against what KVM supports).
> It's too much of a fundamental step in KVM's configuration API.

Ok so we want KVM_TDX_CAPABILITIES to filter bits, but not KVM_TDX_GET_CPUID.

> 
> I am not sure if there are other fixed-1 bits than HYPERVISOR as of
> today.  But in any case, if the TDX module breaks it unilaterally by
> adding more fixed-1 bits, that's a problem for Intel not for KVM.
> 
> On the other hand is KVM_TDX_CAPABILITIES even needed?  If userspace
> can replace that with hardcoded logic or info from the infamous JSON
> file, that would work.

The directly configurable CPUID bits will grow over time. So if we don't expose
the supported ones, userspace will have to guess which ones it can set at that
point.

But as long as the list doesn't shrink we could encode the directly configurable
data in userspace for now, then add an API later when the list of bits grows. If
the API is not present, userspace can assume it's only the original list.

Re: [PATCH 12/25] KVM: TDX: Allow userspace to configure maximum vCPUs for TDX guests

[Tony Lindgren]

On Mon, Aug 19, 2024 at 09:17:10AM +0800, Tao Su wrote:
> On Mon, Aug 12, 2024 at 03:48:07PM -0700, Rick Edgecombe wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -44,6 +44,35 @@ struct kvm_tdx_caps {
> >  
> >  static struct kvm_tdx_caps *kvm_tdx_caps;
> >  
> > +int tdx_vm_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
> > +{
> > +	int r;
> > +
> > +	switch (cap->cap) {
> > +	case KVM_CAP_MAX_VCPUS: {
> 
> How about delete the curly braces on the case?

Thanks I'll do a patch to drop these. And there's an unpaired if else
bracket cosmetic issue there too.

> > +		if (cap->flags || cap->args[0] == 0)
> > +			return -EINVAL;
> > +		if (cap->args[0] > KVM_MAX_VCPUS ||
> > +		    cap->args[0] > tdx_sysinfo->td_conf.max_vcpus_per_td)
> > +			return -E2BIG;
> > +
> > +		mutex_lock(&kvm->lock);
> > +		if (kvm->created_vcpus)
> > +			r = -EBUSY;
> > +		else {
> > +			kvm->max_vcpus = cap->args[0];
> > +			r = 0;
> > +		}
> > +		mutex_unlock(&kvm->lock);
> > +		break;
> > +	}
> > +	default:
> > +		r = -EINVAL;
> > +		break;
> > +	}

And adding a line break here after the switch.

> > +	return r;
> > +}

Regards,

Tony

Re: [PATCH 12/25] KVM: TDX: Allow userspace to configure maximum vCPUs for TDX guests

[Xiaoyao Li]

On 8/13/2024 6:48 AM, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> TDX has its own mechanism to control the maximum number of vCPUs that
> the TDX guest can use.  When creating a TDX guest, the maximum number of
> vCPUs of the guest needs to be passed to the TDX module as part of the
> measurement of the guest.  Depending on TDX module's version, it may
> also report the maximum vCPUs it can support for all TDX guests.
> 
> Because the maximum number of vCPUs is part of the measurement, thus
> part of attestation, it's better to allow the userspace to be able to
> configure it.  E.g. the users may want to precisely control the maximum
> number of vCPUs their precious VMs can use.
> 
> The actual control itself must be done via the TDH.MNG.INIT SEAMCALL,
> where the number of maximum cpus is part of the input to the TDX module,
> but KVM needs to support the "per-VM maximum number of vCPUs" and
> reflect that in the KVM_CAP_MAX_VCPUS.
> 
> Currently, the KVM x86 always reports KVM_MAX_VCPUS for all VMs but
> doesn't allow to enable KVM_CAP_MAX_VCPUS to configure the number of
> maximum vCPUs on VM-basis.
> 
> Add "per-VM maximum number of vCPUs" to KVM x86/TDX to accommodate TDX's
> needs.
> 
> Specifically, use KVM's existing KVM_ENABLE_CAP IOCTL() to allow the
> userspace to configure the maximum vCPUs by making KVM x86 support
> enabling the KVM_CAP_MAX_VCPUS cap on VM-basis.
> 
> For that, add a new 'kvm_x86_ops::vm_enable_cap()' callback and call
> it from kvm_vm_ioctl_enable_cap() as a placeholder to handle the
> KVM_CAP_MAX_VCPUS for TDX guests (and other KVM_CAP_xx for TDX and/or
> other VMs if needed in the future).
> 
> Implement the callback for TDX guest to check whether the maximum vCPUs
> passed from usrspace can be supported by TDX, and if it can, override
> the 'struct kvm::max_vcpus'.  Leave VMX guests and all AMD guests
> unsupported to avoid any side-effect for those VMs.
> 
> Accordingly, in the KVM_CHECK_EXTENSION IOCTL(), change to return the
> 'struct kvm::max_vcpus' for a given VM for the KVM_CAP_MAX_VCPUS.

The implementation of this patch should be dropped in next version and 
be replaced with something suggested in
https://lore.kernel.org/kvm/ZmzaqRy2zjvlsDfL@google.com/

> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> uAPI breakout v1:
>   - Change to use exported 'struct tdx_sysinfo' pointer.
>   - Remove the code to read 'max_vcpus_per_td' since it is now done in
>     TDX host code.
>   - Drop max_vcpu ops to use kvm.max_vcpus
>   - Remove TDX_MAX_VCPUS (Kai)
>   - Use type cast (u16) instead of calling memcpy() when reading the
>     'max_vcpus_per_td' (Kai)
>   - Improve change log and change patch title from "KVM: TDX: Make
>     KVM_CAP_MAX_VCPUS backend specific" (Kai)
> ---
>   arch/x86/include/asm/kvm-x86-ops.h |  1 +
>   arch/x86/include/asm/kvm_host.h    |  1 +
>   arch/x86/kvm/vmx/main.c            | 10 ++++++++++
>   arch/x86/kvm/vmx/tdx.c             | 29 +++++++++++++++++++++++++++++
>   arch/x86/kvm/vmx/x86_ops.h         |  5 +++++
>   arch/x86/kvm/x86.c                 |  4 ++++
>   6 files changed, 50 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 538f50eee86d..bd7434fe5d37 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -19,6 +19,7 @@ KVM_X86_OP(hardware_disable)
>   KVM_X86_OP(hardware_unsetup)
>   KVM_X86_OP(has_emulated_msr)
>   KVM_X86_OP(vcpu_after_set_cpuid)
> +KVM_X86_OP_OPTIONAL(vm_enable_cap)
>   KVM_X86_OP(vm_init)
>   KVM_X86_OP_OPTIONAL(vm_destroy)
>   KVM_X86_OP_OPTIONAL_RET0(vcpu_precreate)
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index c754183e0932..9d15f810f046 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1648,6 +1648,7 @@ struct kvm_x86_ops {
>   	void (*vcpu_after_set_cpuid)(struct kvm_vcpu *vcpu);
>   
>   	unsigned int vm_size;
> +	int (*vm_enable_cap)(struct kvm *kvm, struct kvm_enable_cap *cap);
>   	int (*vm_init)(struct kvm *kvm);
>   	void (*vm_destroy)(struct kvm *kvm);
>   
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index 59f4d2d42620..cd53091ddaab 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -7,6 +7,7 @@
>   #include "pmu.h"
>   #include "posted_intr.h"
>   #include "tdx.h"
> +#include "tdx_arch.h"
>   
>   static __init int vt_hardware_setup(void)
>   {
> @@ -41,6 +42,14 @@ static __init int vt_hardware_setup(void)
>   	return 0;
>   }
>   
> +static int vt_vm_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
> +{
> +	if (is_td(kvm))
> +		return tdx_vm_enable_cap(kvm, cap);
> +
> +	return -EINVAL;
> +}
> +
>   static int vt_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
>   {
>   	if (!is_td(kvm))
> @@ -72,6 +81,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
>   	.has_emulated_msr = vmx_has_emulated_msr,
>   
>   	.vm_size = sizeof(struct kvm_vmx),
> +	.vm_enable_cap = vt_vm_enable_cap,
>   	.vm_init = vmx_vm_init,
>   	.vm_destroy = vmx_vm_destroy,
>   
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index f9faec217ea9..84cd9b4f90b5 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -44,6 +44,35 @@ struct kvm_tdx_caps {
>   
>   static struct kvm_tdx_caps *kvm_tdx_caps;
>   
> +int tdx_vm_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
> +{
> +	int r;
> +
> +	switch (cap->cap) {
> +	case KVM_CAP_MAX_VCPUS: {
> +		if (cap->flags || cap->args[0] == 0)
> +			return -EINVAL;
> +		if (cap->args[0] > KVM_MAX_VCPUS ||
> +		    cap->args[0] > tdx_sysinfo->td_conf.max_vcpus_per_td)
> +			return -E2BIG;
> +
> +		mutex_lock(&kvm->lock);
> +		if (kvm->created_vcpus)
> +			r = -EBUSY;
> +		else {
> +			kvm->max_vcpus = cap->args[0];
> +			r = 0;
> +		}
> +		mutex_unlock(&kvm->lock);
> +		break;
> +	}
> +	default:
> +		r = -EINVAL;
> +		break;
> +	}
> +	return r;
> +}
> +
>   static int tdx_get_capabilities(struct kvm_tdx_cmd *cmd)
>   {
>   	const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
> diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
> index c69ca640abe6..c1bdf7d8fee3 100644
> --- a/arch/x86/kvm/vmx/x86_ops.h
> +++ b/arch/x86/kvm/vmx/x86_ops.h
> @@ -119,8 +119,13 @@ void vmx_cancel_hv_timer(struct kvm_vcpu *vcpu);
>   void vmx_setup_mce(struct kvm_vcpu *vcpu);
>   
>   #ifdef CONFIG_INTEL_TDX_HOST
> +int tdx_vm_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap);
>   int tdx_vm_ioctl(struct kvm *kvm, void __user *argp);
>   #else
> +static inline int tdx_vm_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
> +{
> +	return -EINVAL;
> +};
>   static inline int tdx_vm_ioctl(struct kvm *kvm, void __user *argp) { return -EOPNOTSUPP; }
>   #endif
>   
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 7914ea50fd04..751b3841c48f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4754,6 +4754,8 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>   		break;
>   	case KVM_CAP_MAX_VCPUS:
>   		r = KVM_MAX_VCPUS;
> +		if (kvm)
> +			r = kvm->max_vcpus;
>   		break;
>   	case KVM_CAP_MAX_VCPU_ID:
>   		r = KVM_MAX_VCPU_IDS;
> @@ -6772,6 +6774,8 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>   	}
>   	default:
>   		r = -EINVAL;
> +		if (kvm_x86_ops.vm_enable_cap)
> +			r = static_call(kvm_x86_vm_enable_cap)(kvm, cap);
>   		break;
>   	}
>   	return r;

Re: [PATCH 21/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

[Edgecombe, Rick P]

On Tue, 2024-08-27 at 15:19 +0300, Nikolay Borisov wrote:

> > 
> > But beyond checking for supported features, there are also bug fixes that
> > can
> > affect usability. In the NO_RBP_MOD case we need a specific recent TDX
> > module in
> > order to remove the RBP workaround patches.
> 
> My point was that if having the NO_RPB_MOD implied that the CPUID 
> 0x8000000 configuration capability is also there (not that there is a 
> direct connection between the too but it seems the TDX module isn't 
> being updated that often, I might be wrong of course!), there is no 
> point in having the workaround as NO_RPB_MOD is the minimum required 
> version.

Having NO_RBP_MOD won't imply 0x80000008 configuration capability. We will have
to check for a new feature bit for that. We should wait until it's finalized to
add the code.

Re: [PATCH 21/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

[Tony Lindgren]

On Mon, Aug 19, 2024 at 10:59:49AM +0800, Tao Su wrote:
> On Mon, Aug 12, 2024 at 03:48:16PM -0700, Rick Edgecombe wrote:
> > From: Xiaoyao Li <xiaoyao.li@intel.com>
> > +		/*
> > +		 * Work around missing support on old TDX modules, fetch
> > +		 * guest maxpa from gfn_direct_bits.
> > +		 */
> > +		if (output_e->function == 0x80000008) {
> > +			gpa_t gpa_bits = gfn_to_gpa(kvm_gfn_direct_bits(vcpu->kvm));
> > +			unsigned int g_maxpa = __ffs(gpa_bits) + 1;
> > +
> > +			output_e->eax &= ~0x00ff0000;
> > +			output_e->eax |= g_maxpa << 16;
> > +		}
> 
> I suggest putting all guest_phys_bits related WA in a WA-only patch, which will
> be clearer.

The 80000008 workaround needs to be tidied up for sure, it's hard to follow.

> > --- a/arch/x86/kvm/vmx/tdx.h
> > +++ b/arch/x86/kvm/vmx/tdx.h
> > @@ -25,6 +25,11 @@ struct kvm_tdx {
> >  	bool finalized;
> >  
> >  	u64 tsc_offset;
> > +
> > +	/* For KVM_MAP_MEMORY and KVM_TDX_INIT_MEM_REGION. */
> > +	atomic64_t nr_premapped;
> 
> I don't see it is used in this patch set.

Yes that should have been in a later patch.

Regards,

Tony

Re: [PATCH 21/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

[Paolo Bonzini]

On 9/3/24 08:21, Tony Lindgren wrote:
> On Mon, Aug 19, 2024 at 10:59:49AM +0800, Tao Su wrote:
>> On Mon, Aug 12, 2024 at 03:48:16PM -0700, Rick Edgecombe wrote:
>>> From: Xiaoyao Li <xiaoyao.li@intel.com>
>>> +		/*
>>> +		 * Work around missing support on old TDX modules, fetch
>>> +		 * guest maxpa from gfn_direct_bits.
>>> +		 */
>>> +		if (output_e->function == 0x80000008) {
>>> +			gpa_t gpa_bits = gfn_to_gpa(kvm_gfn_direct_bits(vcpu->kvm));
>>> +			unsigned int g_maxpa = __ffs(gpa_bits) + 1;
>>> +
>>> +			output_e->eax &= ~0x00ff0000;
>>> +			output_e->eax |= g_maxpa << 16;
>>> +		}
>>
>> I suggest putting all guest_phys_bits related WA in a WA-only patch, which will
>> be clearer.
> 
> The 80000008 workaround needs to be tidied up for sure, it's hard to follow.

I think it's okay if you just add a separate 
tdx_get_guest_phys_addr_bits(struct kvm *kvm).

>>> --- a/arch/x86/kvm/vmx/tdx.h
>>> +++ b/arch/x86/kvm/vmx/tdx.h
>>> @@ -25,6 +25,11 @@ struct kvm_tdx {
>>>   	bool finalized;
>>>   
>>>   	u64 tsc_offset;
>>> +
>>> +	/* For KVM_MAP_MEMORY and KVM_TDX_INIT_MEM_REGION. */
>>> +	atomic64_t nr_premapped;
>>
>> I don't see it is used in this patch set.
> 
> Yes that should have been in a later patch.

Yes, it's used in the MMU prep part 2 series.

Paolo

Re: [PATCH 21/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

[Tony Lindgren]

On Mon, Aug 19, 2024 at 01:02:02PM +0800, Xu Yilun wrote:
> On Mon, Aug 12, 2024 at 03:48:16PM -0700, Rick Edgecombe wrote:
> > From: Xiaoyao Li <xiaoyao.li@intel.com>
> > +static int tdx_mask_cpuid(struct kvm_tdx *tdx, struct kvm_cpuid_entry2 *entry)
> > +{
> > +	u64 field_id = TD_MD_FIELD_ID_CPUID_VALUES;
> > +	u64 ebx_eax, edx_ecx;
> > +	u64 err = 0;
> > +
> > +	if (entry->function & TDX_MD_UNREADABLE_LEAF_MASK ||
> > +	    entry->index & TDX_MD_UNREADABLE_SUBLEAF_MASK)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * bit 23:17, REVSERVED: reserved, must be 0;
> > +	 * bit 16,    LEAF_31: leaf number bit 31;
> > +	 * bit 15:9,  LEAF_6_0: leaf number bits 6:0, leaf bits 30:7 are
> > +	 *                      implicitly 0;
> > +	 * bit 8,     SUBLEAF_NA: sub-leaf not applicable flag;
> > +	 * bit 7:1,   SUBLEAF_6_0: sub-leaf number bits 6:0. If SUBLEAF_NA is 1,
> > +	 *                         the SUBLEAF_6_0 is all-1.
> > +	 *                         sub-leaf bits 31:7 are implicitly 0;
> > +	 * bit 0,     ELEMENT_I: Element index within field;
> > +	 */
> > +	field_id |= ((entry->function & 0x80000000) ? 1 : 0) << 16;
> > +	field_id |= (entry->function & 0x7f) << 9;
> > +	if (entry->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX)
> > +		field_id |= (entry->index & 0x7f) << 1;
> > +	else
> > +		field_id |= 0x1fe;
> > +
> > +	err = tdx_td_metadata_field_read(tdx, field_id, &ebx_eax);
> > +	if (err) //TODO check for specific errors
> > +		goto err_out;
> > +
> > +	entry->eax &= (u32) ebx_eax;
> > +	entry->ebx &= (u32) (ebx_eax >> 32);
> 
> Some fields contains a N-bits wide value instead of a bitmask, why a &=
> just work?

There's the CPUID 0x80000008 workaround, I wonder if we are missing some
other handling though. Do you have some specific CPUIDs bits in mind to
check?

The handling for the supported CPUID values mask from the TDX module is
a bit unclear for sure :)

> > +static int tdx_vcpu_get_cpuid(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
> > +{
> > +	struct kvm_cpuid2 __user *output, *td_cpuid;
> > +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> > +	struct kvm_cpuid2 *supported_cpuid;
> > +	int r = 0, i, j = 0;
> > +
> > +	output = u64_to_user_ptr(cmd->data);
> > +	td_cpuid = kzalloc(sizeof(*td_cpuid) +
> > +			sizeof(output->entries[0]) * KVM_MAX_CPUID_ENTRIES,
> > +			GFP_KERNEL);
> > +	if (!td_cpuid)
> > +		return -ENOMEM;
> > +
> > +	r = tdx_get_kvm_supported_cpuid(&supported_cpuid);
> 
> Personally I don't like the definition of this function. I need to look
> into the inner implementation to see if kfree(supported_cpuid); is needed
> or safe. How about:
> 
>   supported_cpuid = tdx_get_kvm_supported_cpuid();
>   if (!supported_cpuid)
> 	goto out_td_cpuid;

So allocate in tdx_get_kvm_supported_cpuid() and the caller frees. Sounds
cleaner to me.

> > +		/*
> > +		 * Work around missing support on old TDX modules, fetch
> > +		 * guest maxpa from gfn_direct_bits.
> > +		 */
> > +		if (output_e->function == 0x80000008) {
> > +			gpa_t gpa_bits = gfn_to_gpa(kvm_gfn_direct_bits(vcpu->kvm));
> > +			unsigned int g_maxpa = __ffs(gpa_bits) + 1;
> > +
> > +			output_e->eax &= ~0x00ff0000;
> > +			output_e->eax |= g_maxpa << 16;
> 
> Is it possible this workaround escapes the KVM supported bits check?

Yes it might need a mask for (g_maxpa << 16) & 0x00ff0000 to avoid setting
the wrong bits, will check.

...
> > +out:
> > +	kfree(td_cpuid);
> > +	kfree(supported_cpuid);
> 
> Traditionally we do:
> 
>   out_supported_cpuid:
> 	kfree(supported_cpuid);
>   out_td_cpuid:
> 	kfree(td_cpuid);
> 
> I'm not sure what's the advantage to make people think more about whether
> kfree is safe.

I'll do a patch for this thanks.

> > --- a/arch/x86/kvm/vmx/tdx.h
> > +++ b/arch/x86/kvm/vmx/tdx.h
> > @@ -25,6 +25,11 @@ struct kvm_tdx {
> >  	bool finalized;
> >  
> >  	u64 tsc_offset;
> > +
> > +	/* For KVM_MAP_MEMORY and KVM_TDX_INIT_MEM_REGION. */
> > +	atomic64_t nr_premapped;
> 
> This doesn't belong to this patch.
> 
> > +
> > +	struct kvm_cpuid2 *cpuid;
> 
> Didn't find the usage of this field.

Thanks will check and drop.

Regards,

Tony

Re: [PATCH 21/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

[Paolo Bonzini]

On 9/3/24 09:19, Tony Lindgren wrote:
>>> +			gpa_t gpa_bits = gfn_to_gpa(kvm_gfn_direct_bits(vcpu->kvm));
>>> +			unsigned int g_maxpa = __ffs(gpa_bits) + 1;
>>> +
>>> +			output_e->eax &= ~0x00ff0000;
>>> +			output_e->eax |= g_maxpa << 16;
>> Is it possible this workaround escapes the KVM supported bits check?
>
> Yes it might need a mask for (g_maxpa << 16) & 0x00ff0000 to avoid setting
> the wrong bits, will check.

The mask is okay, __ffs(gpa_bits) + 1 will be between 1 and 64.

The question is whether the TDX module will accept nonzero bits 16..23 
of CPUID[0x80000008].EAX.

Paolo

Re: [PATCH 21/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

[Tony Lindgren]

On Tue, Sep 10, 2024 at 07:29:13PM +0200, Paolo Bonzini wrote:
> On 9/3/24 09:19, Tony Lindgren wrote:
> > > > +			gpa_t gpa_bits = gfn_to_gpa(kvm_gfn_direct_bits(vcpu->kvm));
> > > > +			unsigned int g_maxpa = __ffs(gpa_bits) + 1;
> > > > +
> > > > +			output_e->eax &= ~0x00ff0000;
> > > > +			output_e->eax |= g_maxpa << 16;
> > > Is it possible this workaround escapes the KVM supported bits check?
> > 
> > Yes it might need a mask for (g_maxpa << 16) & 0x00ff0000 to avoid setting
> > the wrong bits, will check.
> 
> The mask is okay, __ffs(gpa_bits) + 1 will be between 1 and 64.

OK

> The question is whether the TDX module will accept nonzero bits 16..23 of
> CPUID[0x80000008].EAX.

Just for reference, that's the 0x80000008 quirk as you noticed in 22/25.

Regards,

Tony

Re: [PATCH 21/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

[Xiaoyao Li]

On 8/13/2024 6:48 AM, Rick Edgecombe wrote:
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> 
> Implement an IOCTL to allow userspace to read the CPUID bit values for a
> configured TD.
> 
> The TDX module doesn't provide the ability to set all CPUID bits. Instead
> some are configured indirectly, or have fixed values. But it does allow
> for the final resulting CPUID bits to be read. This information will be
> useful for userspace to understand the configuration of the TD, and set
> KVM's copy via KVM_SET_CPUID2.
> 
> To prevent userspace from starting to use features that might not have KVM
> support yet, filter the reported values by KVM's support CPUID bits.

This patch lacks the documentation of KVM_TDX_GET_CPUID to describe what 
it returns and how the returned data is used or expected to be used by 
userspace.

Set aside the filtering of KVM's support CPUID bits, KVM_TDX_GET_CPUID 
only returns the CPUID leaves that can be readable by TDX module (in 
tdx_mask_cpuid()). So what about the leaves that aren't readable? e.g., 
CPUID leaf 6,9,0xc,etc, and leaf 0x40000000 and 0x40000001.

Should userspace to intercept it as the leaves that aren't covered by 
KVM_TDX_GET_CPUID are totally controlled by userspace itself and it's 
userspace's responsibility to set a sane and valid value?

> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> uAPI breakout v1:
>   - New patch
> ---
>   arch/x86/include/uapi/asm/kvm.h |   1 +
>   arch/x86/kvm/vmx/tdx.c          | 131 ++++++++++++++++++++++++++++++++
>   arch/x86/kvm/vmx/tdx.h          |   5 ++
>   arch/x86/kvm/vmx/tdx_arch.h     |   5 ++
>   arch/x86/kvm/vmx/tdx_errno.h    |   1 +
>   5 files changed, 143 insertions(+)
> 
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index b4f12997052d..39636be5c891 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -931,6 +931,7 @@ enum kvm_tdx_cmd_id {
>   	KVM_TDX_CAPABILITIES = 0,
>   	KVM_TDX_INIT_VM,
>   	KVM_TDX_INIT_VCPU,
> +	KVM_TDX_GET_CPUID,
>   
>   	KVM_TDX_CMD_NR_MAX,
>   };
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index b2ed031ac0d6..fe2bbc2ced41 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -813,6 +813,76 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
>   	return ret;
>   }
>   
> +static u64 tdx_td_metadata_field_read(struct kvm_tdx *tdx, u64 field_id,
> +				      u64 *data)
> +{
> +	u64 err;
> +
> +	err = tdh_mng_rd(tdx, field_id, data);
> +
> +	return err;
> +}
> +
> +#define TDX_MD_UNREADABLE_LEAF_MASK	GENMASK(30, 7)
> +#define TDX_MD_UNREADABLE_SUBLEAF_MASK	GENMASK(31, 7)
> +
> +static int tdx_mask_cpuid(struct kvm_tdx *tdx, struct kvm_cpuid_entry2 *entry)
> +{
> +	u64 field_id = TD_MD_FIELD_ID_CPUID_VALUES;
> +	u64 ebx_eax, edx_ecx;
> +	u64 err = 0;
> +
> +	if (entry->function & TDX_MD_UNREADABLE_LEAF_MASK ||
> +	    entry->index & TDX_MD_UNREADABLE_SUBLEAF_MASK)
> +		return -EINVAL;
> +
> +	/*
> +	 * bit 23:17, REVSERVED: reserved, must be 0;
> +	 * bit 16,    LEAF_31: leaf number bit 31;
> +	 * bit 15:9,  LEAF_6_0: leaf number bits 6:0, leaf bits 30:7 are
> +	 *                      implicitly 0;
> +	 * bit 8,     SUBLEAF_NA: sub-leaf not applicable flag;
> +	 * bit 7:1,   SUBLEAF_6_0: sub-leaf number bits 6:0. If SUBLEAF_NA is 1,
> +	 *                         the SUBLEAF_6_0 is all-1.
> +	 *                         sub-leaf bits 31:7 are implicitly 0;
> +	 * bit 0,     ELEMENT_I: Element index within field;
> +	 */
> +	field_id |= ((entry->function & 0x80000000) ? 1 : 0) << 16;
> +	field_id |= (entry->function & 0x7f) << 9;
> +	if (entry->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX)
> +		field_id |= (entry->index & 0x7f) << 1;
> +	else
> +		field_id |= 0x1fe;
> +
> +	err = tdx_td_metadata_field_read(tdx, field_id, &ebx_eax);
> +	if (err) //TODO check for specific errors
> +		goto err_out;
> +
> +	entry->eax &= (u32) ebx_eax;
> +	entry->ebx &= (u32) (ebx_eax >> 32);
> +
> +	field_id++;
> +	err = tdx_td_metadata_field_read(tdx, field_id, &edx_ecx);
> +	/*
> +	 * It's weird that reading edx_ecx fails while reading ebx_eax
> +	 * succeeded.
> +	 */
> +	if (WARN_ON_ONCE(err))
> +		goto err_out;
> +
> +	entry->ecx &= (u32) edx_ecx;
> +	entry->edx &= (u32) (edx_ecx >> 32);
> +	return 0;
> +
> +err_out:
> +	entry->eax = 0;
> +	entry->ebx = 0;
> +	entry->ecx = 0;
> +	entry->edx = 0;
> +
> +	return -EIO;
> +}
> +
>   static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
>   {
>   	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> @@ -1038,6 +1108,64 @@ static int __maybe_unused tdx_get_kvm_supported_cpuid(struct kvm_cpuid2 **cpuid)
>   	return r;
>   }
>   
> +static int tdx_vcpu_get_cpuid(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
> +{
> +	struct kvm_cpuid2 __user *output, *td_cpuid;
> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> +	struct kvm_cpuid2 *supported_cpuid;
> +	int r = 0, i, j = 0;
> +
> +	output = u64_to_user_ptr(cmd->data);
> +	td_cpuid = kzalloc(sizeof(*td_cpuid) +
> +			sizeof(output->entries[0]) * KVM_MAX_CPUID_ENTRIES,
> +			GFP_KERNEL);
> +	if (!td_cpuid)
> +		return -ENOMEM;
> +
> +	r = tdx_get_kvm_supported_cpuid(&supported_cpuid);
> +	if (r)
> +		goto out;
> +
> +	for (i = 0; i < supported_cpuid->nent; i++) {
> +		struct kvm_cpuid_entry2 *supported = &supported_cpuid->entries[i];
> +		struct kvm_cpuid_entry2 *output_e = &td_cpuid->entries[j];
> +
> +		*output_e = *supported;
> +
> +		/* Only allow values of bits that KVM's supports to be exposed */
> +		if (tdx_mask_cpuid(kvm_tdx, output_e))
> +			continue;
> +
> +		/*
> +		 * Work around missing support on old TDX modules, fetch
> +		 * guest maxpa from gfn_direct_bits.
> +		 */
> +		if (output_e->function == 0x80000008) {
> +			gpa_t gpa_bits = gfn_to_gpa(kvm_gfn_direct_bits(vcpu->kvm));
> +			unsigned int g_maxpa = __ffs(gpa_bits) + 1;
> +
> +			output_e->eax &= ~0x00ff0000;
> +			output_e->eax |= g_maxpa << 16;
> +		}
> +
> +		j++;
> +	}
> +	td_cpuid->nent = j;
> +
> +	if (copy_to_user(output, td_cpuid, sizeof(*output))) {
> +		r = -EFAULT;
> +		goto out;
> +	}
> +	if (copy_to_user(output->entries, td_cpuid->entries,
> +			 td_cpuid->nent * sizeof(struct kvm_cpuid_entry2)))
> +		r = -EFAULT;
> +
> +out:
> +	kfree(td_cpuid);
> +	kfree(supported_cpuid);
> +	return r;
> +}
> +
>   static int tdx_vcpu_init(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
>   {
>   	struct msr_data apic_base_msr;
> @@ -1089,6 +1217,9 @@ int tdx_vcpu_ioctl(struct kvm_vcpu *vcpu, void __user *argp)
>   	case KVM_TDX_INIT_VCPU:
>   		ret = tdx_vcpu_init(vcpu, &cmd);
>   		break;
> +	case KVM_TDX_GET_CPUID:
> +		ret = tdx_vcpu_get_cpuid(vcpu, &cmd);
> +		break;
>   	default:
>   		ret = -EINVAL;
>   		break;
> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> index 8349b542836e..7eeb54fbcae1 100644
> --- a/arch/x86/kvm/vmx/tdx.h
> +++ b/arch/x86/kvm/vmx/tdx.h
> @@ -25,6 +25,11 @@ struct kvm_tdx {
>   	bool finalized;
>   
>   	u64 tsc_offset;
> +
> +	/* For KVM_MAP_MEMORY and KVM_TDX_INIT_MEM_REGION. */
> +	atomic64_t nr_premapped;
> +
> +	struct kvm_cpuid2 *cpuid;
>   };
>   
>   struct vcpu_tdx {
> diff --git a/arch/x86/kvm/vmx/tdx_arch.h b/arch/x86/kvm/vmx/tdx_arch.h
> index d2d7f9cab740..815e74408a34 100644
> --- a/arch/x86/kvm/vmx/tdx_arch.h
> +++ b/arch/x86/kvm/vmx/tdx_arch.h
> @@ -157,4 +157,9 @@ struct td_params {
>   
>   #define MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM	BIT_ULL(20)
>   
> +/*
> + * TD scope metadata field ID.
> + */
> +#define TD_MD_FIELD_ID_CPUID_VALUES		0x9410000300000000ULL
> +
>   #endif /* __KVM_X86_TDX_ARCH_H */
> diff --git a/arch/x86/kvm/vmx/tdx_errno.h b/arch/x86/kvm/vmx/tdx_errno.h
> index dc3fa2a58c2c..f9dbb3a065cc 100644
> --- a/arch/x86/kvm/vmx/tdx_errno.h
> +++ b/arch/x86/kvm/vmx/tdx_errno.h
> @@ -23,6 +23,7 @@
>   #define TDX_FLUSHVP_NOT_DONE			0x8000082400000000ULL
>   #define TDX_EPT_WALK_FAILED			0xC0000B0000000000ULL
>   #define TDX_EPT_ENTRY_STATE_INCORRECT		0xC0000B0D00000000ULL
> +#define TDX_METADATA_FIELD_NOT_READABLE		0xC0000C0200000000ULL
>   
>   /*
>    * TDX module operand ID, appears in 31:0 part of error code as

Re: [PATCH 21/25] KVM: x86: Introduce KVM_TDX_GET_CPUID

[Edgecombe, Rick P]

On Mon, 2024-09-30 at 14:26 +0800, Xiaoyao Li wrote:
> This patch lacks the documentation of KVM_TDX_GET_CPUID to describe what 
> it returns and how the returned data is used or expected to be used by 
> userspace.

Yes, we should add some docs.

> 
> Set aside the filtering of KVM's support CPUID bits, KVM_TDX_GET_CPUID 
> only returns the CPUID leaves that can be readable by TDX module (in 
> tdx_mask_cpuid()). So what about the leaves that aren't readable? e.g., 
> CPUID leaf 6,9,0xc,etc, and leaf 0x40000000 and 0x40000001.

Hmm. The purpose of this is to IOCTL is to read the values that the TDX module
knows about so it can set the values KVM knows about. So I think we should just
let it read straight from the TDX module.

> 
> Should userspace to intercept it as the leaves that aren't covered by 
> KVM_TDX_GET_CPUID are totally controlled by userspace itself and it's 
> userspace's responsibility to set a sane and valid value?

Not sure what you mean by "userspace to intercept it". But letting userspace be
responsible to "set a sane and valid value" seems consistent with general KVM
philosophy, and how we are planning to handle the issues around the TDX fixed
bits.

Can you elaborate? And on the consequences from QEMU's side?

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Yan Zhao]

On Mon, Aug 12, 2024 at 03:48:09PM -0700, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
...
> +static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
> +{
> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> +	struct kvm_tdx_init_vm *init_vm;
> +	struct td_params *td_params = NULL;
> +	int ret;
> +
> +	BUILD_BUG_ON(sizeof(*init_vm) != 256 + sizeof_field(struct kvm_tdx_init_vm, cpuid));
> +	BUILD_BUG_ON(sizeof(struct td_params) != 1024);
> +
> +	if (is_hkid_assigned(kvm_tdx))
> +		return -EINVAL;
> +
> +	if (cmd->flags)
> +		return -EINVAL;
> +
> +	init_vm = kmalloc(sizeof(*init_vm) +
> +			  sizeof(init_vm->cpuid.entries[0]) * KVM_MAX_CPUID_ENTRIES,
> +			  GFP_KERNEL);
> +	if (!init_vm)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(init_vm, u64_to_user_ptr(cmd->data), sizeof(*init_vm))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	if (init_vm->cpuid.nent > KVM_MAX_CPUID_ENTRIES) {
> +		ret = -E2BIG;
> +		goto out;
> +	}
> +
> +	if (copy_from_user(init_vm->cpuid.entries,
> +			   u64_to_user_ptr(cmd->data) + sizeof(*init_vm),
> +			   flex_array_size(init_vm, cpuid.entries, init_vm->cpuid.nent))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	if (memchr_inv(init_vm->reserved, 0, sizeof(init_vm->reserved))) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (init_vm->cpuid.padding) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	td_params = kzalloc(sizeof(struct td_params), GFP_KERNEL);
> +	if (!td_params) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = setup_tdparams(kvm, td_params, init_vm);
> +	if (ret)
> +		goto out;
> +
> +	ret = __tdx_td_init(kvm, td_params, &cmd->hw_error);
> +	if (ret)
> +		goto out;
> +
> +	kvm_tdx->tsc_offset = td_tdcs_exec_read64(kvm_tdx, TD_TDCS_EXEC_TSC_OFFSET);
> +	kvm_tdx->attributes = td_params->attributes;
> +	kvm_tdx->xfam = td_params->xfam;
> +
> +	if (td_params->exec_controls & TDX_EXEC_CONTROL_MAX_GPAW)
> +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(51));
> +	else
> +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(47));
> +
Could we introduce a initialized field in struct kvm_tdx and set it true
here? e.g
+       kvm_tdx->initialized = true;

Then reject vCPU creation in tdx_vcpu_create() before KVM_TDX_INIT_VM is
executed successfully? e.g.

@@ -584,6 +589,9 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
        struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
        struct vcpu_tdx *tdx = to_tdx(vcpu);

+       if (!kvm_tdx->initialized)
+               return -EIO;
+
        /* TDX only supports x2APIC, which requires an in-kernel local APIC. */
        if (!vcpu->arch.apic)
                return -EINVAL;

Allowing vCPU creation only after TD is initialized can prevent unexpected
userspace access to uninitialized TD primitives.
See details in the next comment.

> +out:
> +	/* kfree() accepts NULL. */
> +	kfree(init_vm);
> +	kfree(td_params);
> +
> +	return ret;
> +}
> +
>  int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>  {
>  	struct kvm_tdx_cmd tdx_cmd;
> @@ -613,6 +827,9 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>  	case KVM_TDX_CAPABILITIES:
>  		r = tdx_get_capabilities(&tdx_cmd);
>  		break;
> +	case KVM_TDX_INIT_VM:
> +		r = tdx_td_init(kvm, &tdx_cmd);
> +		break;
>  	default:
>  		r = -EINVAL;
>  		goto out;


QEMU should invoke VM ioctl KVM_TDX_INIT_VM in tdx_pre_create_vcpu() before
creating vCPUs via VM ioctl KVM_CREATE_VCPU, but KVM should not count on
userspace always doing the right thing.
e.g. running below selftest would produce warning in KVM due to
td_vmcs_write64() error in tdx_load_mmu_pgd().

void verify_td_negative_test(void)
{
        struct kvm_vm *vm;
        struct kvm_vcpu *vcpu;

        vm = td_create();
        vm_enable_cap(vm, KVM_CAP_SPLIT_IRQCHIP, 24);
        vcpu = __vm_vcpu_add(vm, 0);
        vcpu_run(vcpu);
        kvm_vm_free(vm);
}


[ 5600.721996] WARNING: CPU: 116 PID: 7914 at arch/x86/kvm/vmx/tdx.h:237 tdx_load_mmu_pgd+0x55/0xa0 [kvm_intel] 
[ 5600.735999] Modules linked in: kvm_intel kvm idxd i2c_i801 nls_iso8859_1 i2c_smbus i2c_ismt nls_cp437 squashfs hid_generic crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel crypto_simd cryptd [last unloaded: kvm]
[ 5600.762904] CPU: 116 PID: 7914 Comm: tdx_vm_tests Not tainted 6.10.0-rc7-upstream+ #278 5e882f76313c2b130a0f7525b7eda06f47d8ea02
[ 5600.779772] Hardware name: Intel Corporation ArcherCity/ArcherCity, BIOS EGSDCRB1.SYS.0101.D29.2303301937 03/30/2023
[ 5600.795940] RIP: 0010:tdx_load_mmu_pgd+0x55/0xa0 [kvm_intel]                  
[ 5600.805013] Code: 00 e8 8f b4 ff ff 48 85 c0 74 52 49 89 c5 48 8b 03 44 0f b6 b0 89 a3 00 00 41 80 fe 01 0f 87 ae 74 00 00 41 83 e6 01 75 1d 90 <0f> 0b 90 48 8b 3b b8 01 01 00 00 be 01 03 00 00 66 89 87 89 a3 00
[ 5600.833286] RSP: 0018:ff3550cf49297c78 EFLAGS: 00010246                       
[ 5600.842233] RAX: ff3550cf4dfd9000 RBX: ff2c5edc10600000 RCX: 0000000000000000 
[ 5600.853400] RDX: 0000000000000000 RSI: ff3550cf49297be8 RDI: 000000000000002b 
[ 5600.864609] RBP: ff3550cf49297c98 R08: 0000000000000000 R09: ffffffffffffffff 
[ 5600.875915] R10: 0000000000000000 R11: 0000000000000000 R12: 000000048d10c000 
[ 5600.887255] R13: c000030000000001 R14: 0000000000000000 R15: 0000000000000000 
[ 5600.898584] FS:  00007f9597799740(0000) GS:ff2c5ee7ad700000(0000) knlGS:0000000000000000
[ 5600.911113] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033                 
[ 5600.921064] CR2: 00007f959759b8c0 CR3: 000000010b83e005 CR4: 0000000000773ef0 
[ 5600.932675] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 
[ 5600.944319] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: 0000000000000400 
[ 5600.955987] PKRU: 55555554                                                    
[ 5600.962665] Call Trace:                                                       
[ 5600.969084]  <TASK>                                                           
[ 5600.975079]  ? show_regs+0x64/0x70                                            
[ 5600.982536]  ? __warn+0x8a/0x100                                              
[ 5600.989840]  ? tdx_load_mmu_pgd+0x55/0xa0 [kvm_intel b63d7b2e0213930160302a21a156d5f897483840]
[ 5601.006321]  ? report_bug+0x1b6/0x220                                         
[ 5601.014351]  ? handle_bug+0x43/0x80                                           
[ 5601.022248]  ? exc_invalid_op+0x18/0x70                                       
[ 5601.030554]  ? asm_exc_invalid_op+0x1b/0x20                                   
[ 5601.039297]  ? tdx_load_mmu_pgd+0x55/0xa0 [kvm_intel b63d7b2e0213930160302a21a156d5f897483840]
[ 5601.056276]  ? tdx_load_mmu_pgd+0x31/0xa0 [kvm_intel b63d7b2e0213930160302a21a156d5f897483840]
[ 5601.073270]  vt_load_mmu_pgd+0x57/0x70 [kvm_intel b63d7b2e0213930160302a21a156d5f897483840]
[ 5601.089991]  kvm_mmu_load+0xa4/0xc0 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.102708]  vcpu_enter_guest+0xbe2/0x1140 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.116042]  ? __this_cpu_preempt_check+0x13/0x20                             
[ 5601.125373]  ? debug_smp_processor_id+0x17/0x20                               
[ 5601.134400]  vcpu_run+0x4d/0x280 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.146657]  ? vcpu_run+0x4d/0x280 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.159108]  kvm_arch_vcpu_ioctl_run+0x224/0x680 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.175943]  kvm_vcpu_ioctl+0x238/0x750 [kvm 2979fa2240d2f299e1c4576243100dec1104b4cd]
[ 5601.188912]  ? __ct_user_exit+0xd1/0x120 
[ 5601.197305]  ? __lock_release.isra.0+0x61/0x160
[ 5601.206432]  ? __ct_user_exit+0xd1/0x120
[ 5601.214791]  __x64_sys_ioctl+0x98/0xd0
[ 5601.222980]  x64_sys_call+0x1222/0x2040
[ 5601.231268]  do_syscall_64+0xc3/0x220
[ 5601.239321]  entry_SYSCALL_64_after_hwframe+0x77/0x7f
[ 5601.248913] RIP: 0033:0x7f9597524ded
[ 5601.256781] Code: 04 25 28 00 00 00 48 89 45 c8 31 c0 48 8d 45 10 c7 45 b0 10 00 00 00 48 89 45 b8 48 8d 45 d0 48 89 45 c0 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1a 48 8b 45 c8 64 48 2b 04 25 28 00 00 00
[ 5601.288181] RSP: 002b:00007ffd117315c0 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[ 5601.300843] RAX: ffffffffffffffda RBX: 00000000108a32a0 RCX: 00007f9597524ded
[ 5601.313108] RDX: 0000000000000000 RSI: 000000000000ae80 RDI: 0000000000000005
[ 5601.325411] RBP: 00007ffd11731610 R08: 0000000000422078 R09: 0000000000428e48
[ 5601.337721] R10: 0000000000000001 R11: 0000000000000246 R12: 00000000108a54a0
[ 5601.349965] R13: 0000000000000000 R14: 0000000000434e00 R15: 00007f95977eb000
[ 5601.362131]  </TASK>


> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> index 268959d0f74f..8912cb6d5bc2 100644
> --- a/arch/x86/kvm/vmx/tdx.h
> +++ b/arch/x86/kvm/vmx/tdx.h
> @@ -16,7 +16,11 @@ struct kvm_tdx {
>  	unsigned long tdr_pa;
>  	unsigned long *tdcs_pa;
>  
> +	u64 attributes;
> +	u64 xfam;
>  	int hkid;
> +
> +	u64 tsc_offset;
>  };

> 
> 

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Tony Lindgren]

On Thu, Aug 29, 2024 at 02:27:56PM +0800, Yan Zhao wrote:
> On Mon, Aug 12, 2024 at 03:48:09PM -0700, Rick Edgecombe wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > 
> ...
> > +static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
> > +{
...

> > +	kvm_tdx->tsc_offset = td_tdcs_exec_read64(kvm_tdx, TD_TDCS_EXEC_TSC_OFFSET);
> > +	kvm_tdx->attributes = td_params->attributes;
> > +	kvm_tdx->xfam = td_params->xfam;
> > +
> > +	if (td_params->exec_controls & TDX_EXEC_CONTROL_MAX_GPAW)
> > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(51));
> > +	else
> > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(47));
> > +
> Could we introduce a initialized field in struct kvm_tdx and set it true
> here? e.g
> +       kvm_tdx->initialized = true;
> 
> Then reject vCPU creation in tdx_vcpu_create() before KVM_TDX_INIT_VM is
> executed successfully? e.g.
> 
> @@ -584,6 +589,9 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
>         struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
>         struct vcpu_tdx *tdx = to_tdx(vcpu);
> 
> +       if (!kvm_tdx->initialized)
> +               return -EIO;
> +
>         /* TDX only supports x2APIC, which requires an in-kernel local APIC. */
>         if (!vcpu->arch.apic)
>                 return -EINVAL;
> 
> Allowing vCPU creation only after TD is initialized can prevent unexpected
> userspace access to uninitialized TD primitives.

Makes sense to check for initialized TD before allowing other calls. Maybe
the check is needed in other places too in additoin to the tdx_vcpu_create().

How about just a function to check for one or more of the already existing
initialized struct kvm_tdx values?

Regards,

Tony

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Yan Zhao]

On Mon, Sep 02, 2024 at 01:31:29PM +0300, Tony Lindgren wrote:
> On Thu, Aug 29, 2024 at 02:27:56PM +0800, Yan Zhao wrote:
> > On Mon, Aug 12, 2024 at 03:48:09PM -0700, Rick Edgecombe wrote:
> > > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > > 
> > ...
> > > +static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
> > > +{
> ...
> 
> > > +	kvm_tdx->tsc_offset = td_tdcs_exec_read64(kvm_tdx, TD_TDCS_EXEC_TSC_OFFSET);
> > > +	kvm_tdx->attributes = td_params->attributes;
> > > +	kvm_tdx->xfam = td_params->xfam;
> > > +
> > > +	if (td_params->exec_controls & TDX_EXEC_CONTROL_MAX_GPAW)
> > > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(51));
> > > +	else
> > > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(47));
> > > +
> > Could we introduce a initialized field in struct kvm_tdx and set it true
> > here? e.g
> > +       kvm_tdx->initialized = true;
> > 
> > Then reject vCPU creation in tdx_vcpu_create() before KVM_TDX_INIT_VM is
> > executed successfully? e.g.
> > 
> > @@ -584,6 +589,9 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
> >         struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> >         struct vcpu_tdx *tdx = to_tdx(vcpu);
> > 
> > +       if (!kvm_tdx->initialized)
> > +               return -EIO;
> > +
> >         /* TDX only supports x2APIC, which requires an in-kernel local APIC. */
> >         if (!vcpu->arch.apic)
> >                 return -EINVAL;
> > 
> > Allowing vCPU creation only after TD is initialized can prevent unexpected
> > userspace access to uninitialized TD primitives.
> 
> Makes sense to check for initialized TD before allowing other calls. Maybe
> the check is needed in other places too in additoin to the tdx_vcpu_create().
Do you mean in places checking is_hkid_assigned()?
> 
> How about just a function to check for one or more of the already existing
> initialized struct kvm_tdx values?
Instead of checking multiple individual fields in kvm_tdx or vcpu_tdx, could we
introduce a single state field in the two strutures and utilize a state machine
for check (as Chao Gao pointed out at [1]) ?

e.g.
Now TD can have 5 states: (1)created, (2)initialized, (3)finalized,
                          (4)destroyed, (5)freed.
Each vCPU has 3 states: (1) created, (2) initialized, (3)freed

All the states are updated by a user operation (e.g. KVM_TDX_INIT_VM,
KVM_TDX_FINALIZE_VM, KVM_TDX_INIT_VCPU) or a x86 op (e.g. vm_init, vm_destroy,
vm_free, vcpu_create, vcpu_free).


     TD                                   vCPU
(1) created(set in op vm_init)
(2) initialized
(indicate tdr_pa != 0 && HKID assigned)

                                          (1) created (set in op vcpu_create)

                                          (2) initialized

                                    (can call INIT_MEM_REGION, GET_CPUID here)


(3) finalized

                                 (tdx_vcpu_run(), tdx_handle_exit() can be here)


(4) destroyed (indicate HKID released)

                                         (3) freed

(5) freed


[1] https://lore.kernel.org/kvm/ZfvI8t7SlfIsxbmT@chao-email/#t

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Tony Lindgren]

On Thu, Sep 05, 2024 at 02:59:25PM +0800, Yan Zhao wrote:
> On Mon, Sep 02, 2024 at 01:31:29PM +0300, Tony Lindgren wrote:
> > On Thu, Aug 29, 2024 at 02:27:56PM +0800, Yan Zhao wrote:
> > > On Mon, Aug 12, 2024 at 03:48:09PM -0700, Rick Edgecombe wrote:
> > > > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > > > 
> > > ...
> > > > +static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
> > > > +{
> > ...
> > 
> > > > +	kvm_tdx->tsc_offset = td_tdcs_exec_read64(kvm_tdx, TD_TDCS_EXEC_TSC_OFFSET);
> > > > +	kvm_tdx->attributes = td_params->attributes;
> > > > +	kvm_tdx->xfam = td_params->xfam;
> > > > +
> > > > +	if (td_params->exec_controls & TDX_EXEC_CONTROL_MAX_GPAW)
> > > > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(51));
> > > > +	else
> > > > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(47));
> > > > +
> > > Could we introduce a initialized field in struct kvm_tdx and set it true
> > > here? e.g
> > > +       kvm_tdx->initialized = true;
> > > 
> > > Then reject vCPU creation in tdx_vcpu_create() before KVM_TDX_INIT_VM is
> > > executed successfully? e.g.
> > > 
> > > @@ -584,6 +589,9 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
> > >         struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> > >         struct vcpu_tdx *tdx = to_tdx(vcpu);
> > > 
> > > +       if (!kvm_tdx->initialized)
> > > +               return -EIO;
> > > +
> > >         /* TDX only supports x2APIC, which requires an in-kernel local APIC. */
> > >         if (!vcpu->arch.apic)
> > >                 return -EINVAL;
> > > 
> > > Allowing vCPU creation only after TD is initialized can prevent unexpected
> > > userspace access to uninitialized TD primitives.
> > 
> > Makes sense to check for initialized TD before allowing other calls. Maybe
> > the check is needed in other places too in additoin to the tdx_vcpu_create().
> Do you mean in places checking is_hkid_assigned()?

Sounds like the state needs to be checked in multiple places to handle
out-of-order ioctls to that's not enough.

> > How about just a function to check for one or more of the already existing
> > initialized struct kvm_tdx values?
> Instead of checking multiple individual fields in kvm_tdx or vcpu_tdx, could we
> introduce a single state field in the two strutures and utilize a state machine
> for check (as Chao Gao pointed out at [1]) ?

OK

> e.g.
> Now TD can have 5 states: (1)created, (2)initialized, (3)finalized,
>                           (4)destroyed, (5)freed.
> Each vCPU has 3 states: (1) created, (2) initialized, (3)freed
> 
> All the states are updated by a user operation (e.g. KVM_TDX_INIT_VM,
> KVM_TDX_FINALIZE_VM, KVM_TDX_INIT_VCPU) or a x86 op (e.g. vm_init, vm_destroy,
> vm_free, vcpu_create, vcpu_free).
> 
> 
>      TD                                   vCPU
> (1) created(set in op vm_init)
> (2) initialized
> (indicate tdr_pa != 0 && HKID assigned)
> 
>                                           (1) created (set in op vcpu_create)
> 
>                                           (2) initialized
> 
>                                     (can call INIT_MEM_REGION, GET_CPUID here)
> 
> 
> (3) finalized
> 
>                                  (tdx_vcpu_run(), tdx_handle_exit() can be here)
> 
> 
> (4) destroyed (indicate HKID released)
> 
>                                          (3) freed
> 
> (5) freed

So an enum for the TD state, and also for the vCPU state?

Regards,

Tony
 
> [1] https://lore.kernel.org/kvm/ZfvI8t7SlfIsxbmT@chao-email/#t

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Yan Zhao]

On Thu, Sep 05, 2024 at 12:27:54PM +0300, Tony Lindgren wrote:
> On Thu, Sep 05, 2024 at 02:59:25PM +0800, Yan Zhao wrote:
> > On Mon, Sep 02, 2024 at 01:31:29PM +0300, Tony Lindgren wrote:
> > > On Thu, Aug 29, 2024 at 02:27:56PM +0800, Yan Zhao wrote:
> > > > On Mon, Aug 12, 2024 at 03:48:09PM -0700, Rick Edgecombe wrote:
> > > > > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > > > > 
> > > > ...
> > > > > +static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
> > > > > +{
> > > ...
> > > 
> > > > > +	kvm_tdx->tsc_offset = td_tdcs_exec_read64(kvm_tdx, TD_TDCS_EXEC_TSC_OFFSET);
> > > > > +	kvm_tdx->attributes = td_params->attributes;
> > > > > +	kvm_tdx->xfam = td_params->xfam;
> > > > > +
> > > > > +	if (td_params->exec_controls & TDX_EXEC_CONTROL_MAX_GPAW)
> > > > > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(51));
> > > > > +	else
> > > > > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(47));
> > > > > +
> > > > Could we introduce a initialized field in struct kvm_tdx and set it true
> > > > here? e.g
> > > > +       kvm_tdx->initialized = true;
> > > > 
> > > > Then reject vCPU creation in tdx_vcpu_create() before KVM_TDX_INIT_VM is
> > > > executed successfully? e.g.
> > > > 
> > > > @@ -584,6 +589,9 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
> > > >         struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> > > >         struct vcpu_tdx *tdx = to_tdx(vcpu);
> > > > 
> > > > +       if (!kvm_tdx->initialized)
> > > > +               return -EIO;
> > > > +
> > > >         /* TDX only supports x2APIC, which requires an in-kernel local APIC. */
> > > >         if (!vcpu->arch.apic)
> > > >                 return -EINVAL;
> > > > 
> > > > Allowing vCPU creation only after TD is initialized can prevent unexpected
> > > > userspace access to uninitialized TD primitives.
> > > 
> > > Makes sense to check for initialized TD before allowing other calls. Maybe
> > > the check is needed in other places too in additoin to the tdx_vcpu_create().
> > Do you mean in places checking is_hkid_assigned()?
> 
> Sounds like the state needs to be checked in multiple places to handle
> out-of-order ioctls to that's not enough.
> 
> > > How about just a function to check for one or more of the already existing
> > > initialized struct kvm_tdx values?
> > Instead of checking multiple individual fields in kvm_tdx or vcpu_tdx, could we
> > introduce a single state field in the two strutures and utilize a state machine
> > for check (as Chao Gao pointed out at [1]) ?
> 
> OK
> 
> > e.g.
> > Now TD can have 5 states: (1)created, (2)initialized, (3)finalized,
> >                           (4)destroyed, (5)freed.
> > Each vCPU has 3 states: (1) created, (2) initialized, (3)freed
> > 
> > All the states are updated by a user operation (e.g. KVM_TDX_INIT_VM,
> > KVM_TDX_FINALIZE_VM, KVM_TDX_INIT_VCPU) or a x86 op (e.g. vm_init, vm_destroy,
> > vm_free, vcpu_create, vcpu_free).
> > 
> > 
> >      TD                                   vCPU
> > (1) created(set in op vm_init)
> > (2) initialized
> > (indicate tdr_pa != 0 && HKID assigned)
> > 
> >                                           (1) created (set in op vcpu_create)
> > 
> >                                           (2) initialized
> > 
> >                                     (can call INIT_MEM_REGION, GET_CPUID here)
> > 
> > 
> > (3) finalized
> > 
> >                                  (tdx_vcpu_run(), tdx_handle_exit() can be here)
> > 
> > 
> > (4) destroyed (indicate HKID released)
> > 
> >                                          (3) freed
> > 
> > (5) freed
> 
> So an enum for the TD state, and also for the vCPU state?

A state for TD, and a state for each vCPU.
Each vCPU needs to check TD state and vCPU state of itself for vCPU state
transition.

Does it make sense?

>  
> > [1] https://lore.kernel.org/kvm/ZfvI8t7SlfIsxbmT@chao-email/#t

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Tony Lindgren]

On Fri, Sep 06, 2024 at 12:05:41PM +0800, Yan Zhao wrote:
> On Thu, Sep 05, 2024 at 12:27:54PM +0300, Tony Lindgren wrote:
> > On Thu, Sep 05, 2024 at 02:59:25PM +0800, Yan Zhao wrote:
> > > On Mon, Sep 02, 2024 at 01:31:29PM +0300, Tony Lindgren wrote:
> > > > On Thu, Aug 29, 2024 at 02:27:56PM +0800, Yan Zhao wrote:
> > > > > On Mon, Aug 12, 2024 at 03:48:09PM -0700, Rick Edgecombe wrote:
> > > > > > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > > > > > 
> > > > > ...
> > > > > > +static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
> > > > > > +{
> > > > ...
> > > > 
> > > > > > +	kvm_tdx->tsc_offset = td_tdcs_exec_read64(kvm_tdx, TD_TDCS_EXEC_TSC_OFFSET);
> > > > > > +	kvm_tdx->attributes = td_params->attributes;
> > > > > > +	kvm_tdx->xfam = td_params->xfam;
> > > > > > +
> > > > > > +	if (td_params->exec_controls & TDX_EXEC_CONTROL_MAX_GPAW)
> > > > > > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(51));
> > > > > > +	else
> > > > > > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(47));
> > > > > > +
> > > > > Could we introduce a initialized field in struct kvm_tdx and set it true
> > > > > here? e.g
> > > > > +       kvm_tdx->initialized = true;
> > > > > 
> > > > > Then reject vCPU creation in tdx_vcpu_create() before KVM_TDX_INIT_VM is
> > > > > executed successfully? e.g.
> > > > > 
> > > > > @@ -584,6 +589,9 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
> > > > >         struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> > > > >         struct vcpu_tdx *tdx = to_tdx(vcpu);
> > > > > 
> > > > > +       if (!kvm_tdx->initialized)
> > > > > +               return -EIO;
> > > > > +
> > > > >         /* TDX only supports x2APIC, which requires an in-kernel local APIC. */
> > > > >         if (!vcpu->arch.apic)
> > > > >                 return -EINVAL;
> > > > > 
> > > > > Allowing vCPU creation only after TD is initialized can prevent unexpected
> > > > > userspace access to uninitialized TD primitives.
> > > > 
> > > > Makes sense to check for initialized TD before allowing other calls. Maybe
> > > > the check is needed in other places too in additoin to the tdx_vcpu_create().
> > > Do you mean in places checking is_hkid_assigned()?
> > 
> > Sounds like the state needs to be checked in multiple places to handle
> > out-of-order ioctls to that's not enough.
> > 
> > > > How about just a function to check for one or more of the already existing
> > > > initialized struct kvm_tdx values?
> > > Instead of checking multiple individual fields in kvm_tdx or vcpu_tdx, could we
> > > introduce a single state field in the two strutures and utilize a state machine
> > > for check (as Chao Gao pointed out at [1]) ?
> > 
> > OK
> > 
> > > e.g.
> > > Now TD can have 5 states: (1)created, (2)initialized, (3)finalized,
> > >                           (4)destroyed, (5)freed.
> > > Each vCPU has 3 states: (1) created, (2) initialized, (3)freed
> > > 
> > > All the states are updated by a user operation (e.g. KVM_TDX_INIT_VM,
> > > KVM_TDX_FINALIZE_VM, KVM_TDX_INIT_VCPU) or a x86 op (e.g. vm_init, vm_destroy,
> > > vm_free, vcpu_create, vcpu_free).
> > > 
> > > 
> > >      TD                                   vCPU
> > > (1) created(set in op vm_init)
> > > (2) initialized
> > > (indicate tdr_pa != 0 && HKID assigned)
> > > 
> > >                                           (1) created (set in op vcpu_create)
> > > 
> > >                                           (2) initialized
> > > 
> > >                                     (can call INIT_MEM_REGION, GET_CPUID here)
> > > 
> > > 
> > > (3) finalized
> > > 
> > >                                  (tdx_vcpu_run(), tdx_handle_exit() can be here)
> > > 
> > > 
> > > (4) destroyed (indicate HKID released)
> > > 
> > >                                          (3) freed
> > > 
> > > (5) freed
> > 
> > So an enum for the TD state, and also for the vCPU state?
> 
> A state for TD, and a state for each vCPU.
> Each vCPU needs to check TD state and vCPU state of itself for vCPU state
> transition.
> 
> Does it make sense?

That sounds good to me :)

Regards,

Tony
 
> > > [1] https://lore.kernel.org/kvm/ZfvI8t7SlfIsxbmT@chao-email/#t

RE: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Wang, Wei W]

On Friday, September 6, 2024 12:33 PM, Tony Lindgren wrote:
> On Fri, Sep 06, 2024 at 12:05:41PM +0800, Yan Zhao wrote:
> > On Thu, Sep 05, 2024 at 12:27:54PM +0300, Tony Lindgren wrote:
> > > On Thu, Sep 05, 2024 at 02:59:25PM +0800, Yan Zhao wrote:
> > > > On Mon, Sep 02, 2024 at 01:31:29PM +0300, Tony Lindgren wrote:
> > > > > On Thu, Aug 29, 2024 at 02:27:56PM +0800, Yan Zhao wrote:
> > > > > > On Mon, Aug 12, 2024 at 03:48:09PM -0700, Rick Edgecombe wrote:
> > > > > > > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > > > > > >
> > > > > > ...
> > > > > > > +static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd
> > > > > > > +*cmd) {
> > > > > ...
> > > > >
> > > > > > > +	kvm_tdx->tsc_offset = td_tdcs_exec_read64(kvm_tdx,
> TD_TDCS_EXEC_TSC_OFFSET);
> > > > > > > +	kvm_tdx->attributes = td_params->attributes;
> > > > > > > +	kvm_tdx->xfam = td_params->xfam;
> > > > > > > +
> > > > > > > +	if (td_params->exec_controls &
> TDX_EXEC_CONTROL_MAX_GPAW)
> > > > > > > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(51));
> > > > > > > +	else
> > > > > > > +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(47));
> > > > > > > +
> > > > > > Could we introduce a initialized field in struct kvm_tdx and
> > > > > > set it true here? e.g
> > > > > > +       kvm_tdx->initialized = true;
> > > > > >
> > > > > > Then reject vCPU creation in tdx_vcpu_create() before
> > > > > > KVM_TDX_INIT_VM is executed successfully? e.g.
> > > > > >
> > > > > > @@ -584,6 +589,9 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
> > > > > >         struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
> > > > > >         struct vcpu_tdx *tdx = to_tdx(vcpu);
> > > > > >
> > > > > > +       if (!kvm_tdx->initialized)
> > > > > > +               return -EIO;
> > > > > > +
> > > > > >         /* TDX only supports x2APIC, which requires an in-kernel local
> APIC. */
> > > > > >         if (!vcpu->arch.apic)
> > > > > >                 return -EINVAL;
> > > > > >
> > > > > > Allowing vCPU creation only after TD is initialized can
> > > > > > prevent unexpected userspace access to uninitialized TD primitives.
> > > > >
> > > > > Makes sense to check for initialized TD before allowing other
> > > > > calls. Maybe the check is needed in other places too in additoin to the
> tdx_vcpu_create().
> > > > Do you mean in places checking is_hkid_assigned()?
> > >
> > > Sounds like the state needs to be checked in multiple places to
> > > handle out-of-order ioctls to that's not enough.
> > >
> > > > > How about just a function to check for one or more of the
> > > > > already existing initialized struct kvm_tdx values?
> > > > Instead of checking multiple individual fields in kvm_tdx or
> > > > vcpu_tdx, could we introduce a single state field in the two
> > > > strutures and utilize a state machine for check (as Chao Gao pointed out
> at [1]) ?
> > >
> > > OK
> > >
> > > > e.g.
> > > > Now TD can have 5 states: (1)created, (2)initialized, (3)finalized,
> > > >                           (4)destroyed, (5)freed.
> > > > Each vCPU has 3 states: (1) created, (2) initialized, (3)freed
> > > >
> > > > All the states are updated by a user operation (e.g.
> > > > KVM_TDX_INIT_VM, KVM_TDX_FINALIZE_VM, KVM_TDX_INIT_VCPU) or
> a x86
> > > > op (e.g. vm_init, vm_destroy, vm_free, vcpu_create, vcpu_free).
> > > >
> > > >
> > > >      TD                                   vCPU
> > > > (1) created(set in op vm_init)
> > > > (2) initialized
> > > > (indicate tdr_pa != 0 && HKID assigned)
> > > >
> > > >                                           (1) created (set in op
> > > > vcpu_create)
> > > >
> > > >                                           (2) initialized
> > > >
> > > >                                     (can call INIT_MEM_REGION,
> > > > GET_CPUID here)
> > > >
> > > >
> > > > (3) finalized
> > > >
> > > >                                  (tdx_vcpu_run(),
> > > > tdx_handle_exit() can be here)
> > > >
> > > >
> > > > (4) destroyed (indicate HKID released)
> > > >
> > > >                                          (3) freed
> > > >
> > > > (5) freed
> > >
> > > So an enum for the TD state, and also for the vCPU state?
> >
> > A state for TD, and a state for each vCPU.
> > Each vCPU needs to check TD state and vCPU state of itself for vCPU
> > state transition.
> >
> > Does it make sense?
> 
> That sounds good to me :)

+1 sounds good.
I also thought about this. KVM could create a shadow of the TD and vCPU
states that are already defined and maintained by the TDX module.
This should also be more extensible for adding the TD migration support later
(compared to adding various Booleans).

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Chenyi Qiang]

On 8/13/2024 6:48 AM, Rick Edgecombe wrote:
> From: Isaku Yamahata <isaku.yamahata@intel.com>
> 
> After the crypto-protection key has been configured, TDX requires a
> VM-scope initialization as a step of creating the TDX guest.  This
> "per-VM" TDX initialization does the global configurations/features that
> the TDX guest can support, such as guest's CPUIDs (emulated by the TDX
> module), the maximum number of vcpus etc.
> 
> This "per-VM" TDX initialization must be done before any "vcpu-scope" TDX
> initialization.  To match this better, require the KVM_TDX_INIT_VM IOCTL()
> to be done before KVM creates any vcpus.
> 
> Co-developed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> uAPI breakout v1:
>  - Drop TDX_TD_XFAM_CET and use XFEATURE_MASK_CET_{USER, KERNEL}.
>  - Update for the wrapper functions for SEAMCALLs. (Sean)
>  - Move gfn_shared_mask settings into this patch due to MMU section move
>  - Fix bisectability issues in headers (Kai)
>  - Updates from seamcall overhaul (Kai)
>  - Allow userspace configure xfam directly
>  - Check if user sets non-configurable bits in CPUIDs
>  - Rename error->hw_error
>  - Move code change to tdx_module_setup() to __tdx_bringup() due to
>    initializing is done in post hardware_setup() now and
>    tdx_module_setup() is removed.  Remove the code to use API to read
>    global metadata but use exported 'struct tdx_sysinfo' pointer.
>  - Replace 'tdx_info->nr_tdcs_pages' with a wrapper
>    tdx_sysinfo_nr_tdcs_pages() because the 'struct tdx_sysinfo' doesn't
>    have nr_tdcs_pages directly.
>  - Replace tdx_info->max_vcpus_per_td with the new exported pointer in
>    tdx_vm_init().
>  - Decrease the reserved space for struct kvm_tdx_init_vm (Kai)
>  - Use sizeof_field() for struct kvm_tdx_init_vm cpuids (Tony)
>  - No need to init init_vm, it gets copied over in tdx_td_init() (Chao)
>  - Use kmalloc() instead of () kzalloc for init_vm in tdx_td_init() (Chao)
>  - Add more line breaks to tdx_td_init() to make code easier to read (Tony)
>  - Clarify patch description (Kai)
> 
> v19:
>  - Check NO_RBP_MOD of feature0 and set it
>  - Update the comment for PT and CET
> 
> v18:
>  - remove the change of tools/arch/x86/include/uapi/asm/kvm.h
>  - typo in comment. sha348 => sha384
>  - updated comment in setup_tdparams_xfam()
>  - fix setup_tdparams_xfam() to use init_vm instead of td_params
> 
> v16:
>  - Removed AMX check as the KVM upstream supports AMX.
>  - Added CET flag to guest supported xss
> ---
>  arch/x86/include/uapi/asm/kvm.h |  24 ++++
>  arch/x86/kvm/cpuid.c            |   7 +
>  arch/x86/kvm/cpuid.h            |   2 +
>  arch/x86/kvm/vmx/tdx.c          | 237 ++++++++++++++++++++++++++++++--
>  arch/x86/kvm/vmx/tdx.h          |   4 +
>  arch/x86/kvm/vmx/tdx_ops.h      |  12 ++
>  6 files changed, 276 insertions(+), 10 deletions(-)
> 

...

> +static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
> +			 u64 *seamcall_err)
>  {
>  	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
>  	cpumask_var_t packages;
> @@ -427,8 +547,9 @@ static int __tdx_td_init(struct kvm *kvm)
>  	unsigned long tdr_pa = 0;
>  	unsigned long va;
>  	int ret, i;
> -	u64 err;
> +	u64 err, rcx;
>  
> +	*seamcall_err = 0;
>  	ret = tdx_guest_keyid_alloc();
>  	if (ret < 0)
>  		return ret;
> @@ -543,10 +664,23 @@ static int __tdx_td_init(struct kvm *kvm)
>  		}
>  	}
>  
> -	/*
> -	 * Note, TDH_MNG_INIT cannot be invoked here.  TDH_MNG_INIT requires a dedicated
> -	 * ioctl() to define the configure CPUID values for the TD.
> -	 */
> +	err = tdh_mng_init(kvm_tdx, __pa(td_params), &rcx);
> +	if ((err & TDX_SEAMCALL_STATUS_MASK) == TDX_OPERAND_INVALID) {
> +		/*
> +		 * Because a user gives operands, don't warn.
> +		 * Return a hint to the user because it's sometimes hard for the
> +		 * user to figure out which operand is invalid.  SEAMCALL status
> +		 * code includes which operand caused invalid operand error.
> +		 */
> +		*seamcall_err = err;

I'm wondering if we could return or output more hint (i.e. the value of
rcx) in the case of invalid operand. For example, if seamcall returns
with INVALID_OPERAND_CPUID_CONFIG, rcx will contain the CPUID
leaf/sub-leaf info.

> +		ret = -EINVAL;
> +		goto teardown;
> +	} else if (WARN_ON_ONCE(err)) {
> +		pr_tdx_error_1(TDH_MNG_INIT, err, rcx);
> +		ret = -EIO;
> +		goto teardown;
> +	}
> +
>  	return 0;
>  
>  	/*
> @@ -592,6 +726,86 @@ static int __tdx_td_init(struct kvm *kvm)
>  	return ret;
>  }
>  
> +static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
> +{
> +	struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> +	struct kvm_tdx_init_vm *init_vm;
> +	struct td_params *td_params = NULL;
> +	int ret;
> +
> +	BUILD_BUG_ON(sizeof(*init_vm) != 256 + sizeof_field(struct kvm_tdx_init_vm, cpuid));
> +	BUILD_BUG_ON(sizeof(struct td_params) != 1024);
> +
> +	if (is_hkid_assigned(kvm_tdx))
> +		return -EINVAL;
> +
> +	if (cmd->flags)
> +		return -EINVAL;
> +
> +	init_vm = kmalloc(sizeof(*init_vm) +
> +			  sizeof(init_vm->cpuid.entries[0]) * KVM_MAX_CPUID_ENTRIES,
> +			  GFP_KERNEL);
> +	if (!init_vm)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(init_vm, u64_to_user_ptr(cmd->data), sizeof(*init_vm))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	if (init_vm->cpuid.nent > KVM_MAX_CPUID_ENTRIES) {
> +		ret = -E2BIG;
> +		goto out;
> +	}
> +
> +	if (copy_from_user(init_vm->cpuid.entries,
> +			   u64_to_user_ptr(cmd->data) + sizeof(*init_vm),
> +			   flex_array_size(init_vm, cpuid.entries, init_vm->cpuid.nent))) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	if (memchr_inv(init_vm->reserved, 0, sizeof(init_vm->reserved))) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	if (init_vm->cpuid.padding) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	td_params = kzalloc(sizeof(struct td_params), GFP_KERNEL);
> +	if (!td_params) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	ret = setup_tdparams(kvm, td_params, init_vm);
> +	if (ret)
> +		goto out;
> +
> +	ret = __tdx_td_init(kvm, td_params, &cmd->hw_error);
> +	if (ret)
> +		goto out;
> +
> +	kvm_tdx->tsc_offset = td_tdcs_exec_read64(kvm_tdx, TD_TDCS_EXEC_TSC_OFFSET);
> +	kvm_tdx->attributes = td_params->attributes;
> +	kvm_tdx->xfam = td_params->xfam;
> +
> +	if (td_params->exec_controls & TDX_EXEC_CONTROL_MAX_GPAW)
> +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(51));
> +	else
> +		kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(47));
> +
> +out:
> +	/* kfree() accepts NULL. */
> +	kfree(init_vm);
> +	kfree(td_params);
> +
> +	return ret;
> +}
> +
>  int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>  {
>  	struct kvm_tdx_cmd tdx_cmd;
> @@ -613,6 +827,9 @@ int tdx_vm_ioctl(struct kvm *kvm, void __user *argp)
>  	case KVM_TDX_CAPABILITIES:
>  		r = tdx_get_capabilities(&tdx_cmd);
>  		break;
> +	case KVM_TDX_INIT_VM:
> +		r = tdx_td_init(kvm, &tdx_cmd);
> +		break;
>  	default:
>  		r = -EINVAL;
>  		goto out;
> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> index 268959d0f74f..8912cb6d5bc2 100644
> --- a/arch/x86/kvm/vmx/tdx.h
> +++ b/arch/x86/kvm/vmx/tdx.h
> @@ -16,7 +16,11 @@ struct kvm_tdx {
>  	unsigned long tdr_pa;
>  	unsigned long *tdcs_pa;
>  
> +	u64 attributes;
> +	u64 xfam;
>  	int hkid;
> +
> +	u64 tsc_offset;
>  };
>  
>  struct vcpu_tdx {
> diff --git a/arch/x86/kvm/vmx/tdx_ops.h b/arch/x86/kvm/vmx/tdx_ops.h
> index 3f64c871a3f2..0363d8544f42 100644
> --- a/arch/x86/kvm/vmx/tdx_ops.h
> +++ b/arch/x86/kvm/vmx/tdx_ops.h
> @@ -399,4 +399,16 @@ static inline u64 tdh_vp_wr(struct vcpu_tdx *tdx, u64 field, u64 val, u64 mask)
>  	return seamcall(TDH_VP_WR, &in);
>  }
>  
> +static __always_inline u64 td_tdcs_exec_read64(struct kvm_tdx *kvm_tdx, u32 field)
> +{
> +	u64 err, data;
> +
> +	err = tdh_mng_rd(kvm_tdx, TDCS_EXEC(field), &data);
> +	if (unlikely(err)) {
> +		pr_err("TDH_MNG_RD[EXEC.0x%x] failed: 0x%llx\n", field, err);
> +		return 0;
> +	}
> +	return data;
> +}
> +
>  #endif /* __KVM_X86_TDX_OPS_H */

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Tony Lindgren]

On Tue, Sep 03, 2024 at 10:58:11AM +0800, Chenyi Qiang wrote:
> On 8/13/2024 6:48 AM, Rick Edgecombe wrote:
> > From: Isaku Yamahata <isaku.yamahata@intel.com>
> > @@ -543,10 +664,23 @@ static int __tdx_td_init(struct kvm *kvm)
> >  		}
> >  	}
> >  
> > -	/*
> > -	 * Note, TDH_MNG_INIT cannot be invoked here.  TDH_MNG_INIT requires a dedicated
> > -	 * ioctl() to define the configure CPUID values for the TD.
> > -	 */
> > +	err = tdh_mng_init(kvm_tdx, __pa(td_params), &rcx);
> > +	if ((err & TDX_SEAMCALL_STATUS_MASK) == TDX_OPERAND_INVALID) {
> > +		/*
> > +		 * Because a user gives operands, don't warn.
> > +		 * Return a hint to the user because it's sometimes hard for the
> > +		 * user to figure out which operand is invalid.  SEAMCALL status
> > +		 * code includes which operand caused invalid operand error.
> > +		 */
> > +		*seamcall_err = err;
> 
> I'm wondering if we could return or output more hint (i.e. the value of
> rcx) in the case of invalid operand. For example, if seamcall returns
> with INVALID_OPERAND_CPUID_CONFIG, rcx will contain the CPUID
> leaf/sub-leaf info.

Printing a decriptive error here would be nice when things go wrong.
Probably no need to return that information.

Sounds like you have a patch already in mind though :) Care to post a
patch against the current kvm-coco branch? If not, I can do it after all
the obvious comment changes are out of the way.

Regards,

Tony

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Chenyi Qiang]

On 9/3/2024 1:44 PM, Tony Lindgren wrote:
> On Tue, Sep 03, 2024 at 10:58:11AM +0800, Chenyi Qiang wrote:
>> On 8/13/2024 6:48 AM, Rick Edgecombe wrote:
>>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>> @@ -543,10 +664,23 @@ static int __tdx_td_init(struct kvm *kvm)
>>>  		}
>>>  	}
>>>  
>>> -	/*
>>> -	 * Note, TDH_MNG_INIT cannot be invoked here.  TDH_MNG_INIT requires a dedicated
>>> -	 * ioctl() to define the configure CPUID values for the TD.
>>> -	 */
>>> +	err = tdh_mng_init(kvm_tdx, __pa(td_params), &rcx);
>>> +	if ((err & TDX_SEAMCALL_STATUS_MASK) == TDX_OPERAND_INVALID) {
>>> +		/*
>>> +		 * Because a user gives operands, don't warn.
>>> +		 * Return a hint to the user because it's sometimes hard for the
>>> +		 * user to figure out which operand is invalid.  SEAMCALL status
>>> +		 * code includes which operand caused invalid operand error.
>>> +		 */
>>> +		*seamcall_err = err;
>>
>> I'm wondering if we could return or output more hint (i.e. the value of
>> rcx) in the case of invalid operand. For example, if seamcall returns
>> with INVALID_OPERAND_CPUID_CONFIG, rcx will contain the CPUID
>> leaf/sub-leaf info.
> 
> Printing a decriptive error here would be nice when things go wrong.
> Probably no need to return that information.
> 
> Sounds like you have a patch already in mind though :) Care to post a
> patch against the current kvm-coco branch? If not, I can do it after all
> the obvious comment changes are out of the way.

According to the comment above, this patch wants to return the hint to
user as the user gives operands. I'm still uncertain if we should follow
this to return value in some way or special-case the
INVALID_OPERAND_CPUID_CONFIG like:

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index c00c73b2ad4c..dd6e3149ff5a 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -2476,8 +2476,14 @@ static int __tdx_td_init(struct kvm *kvm, struct
td_params *td_params,
                 * Return a hint to the user because it's sometimes hard
for the
                 * user to figure out which operand is invalid.
SEAMCALL status
                 * code includes which operand caused invalid operand error.
+                *
+                * TDX_OPERAND_INVALID_CPUID_CONFIG contains more info
+                * in rcx (i.e. leaf/sub-leaf), warn it to help figure
+                * out the invalid CPUID config.
                 */
                *seamcall_err = err;
+               if (err == (TDX_OPERAND_INVALID |
TDX_OPERAND_ID_CPUID_CONFIG))
+                       pr_tdx_error_1(TDH_MNG_INIT, err, rcx);
                ret = -EINVAL;
                goto teardown;
        } else if (WARN_ON_ONCE(err)) {
diff --git a/arch/x86/kvm/vmx/tdx_errno.h b/arch/x86/kvm/vmx/tdx_errno.h
index f9dbb3a065cc..311c3f03d398 100644
--- a/arch/x86/kvm/vmx/tdx_errno.h
+++ b/arch/x86/kvm/vmx/tdx_errno.h
@@ -30,6 +30,7 @@
  * detail information
  */
 #define TDX_OPERAND_ID_RCX                     0x01
+#define TDX_OPERAND_ID_CPUID_CONFIG            0x45
 #define TDX_OPERAND_ID_TDR                     0x80
 #define TDX_OPERAND_ID_SEPT                    0x92
 #define TDX_OPERAND_ID_TD_EPOCH                        0xa9

> 
> Regards,
> 
> Tony

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Tony Lindgren]

On Tue, Sep 03, 2024 at 04:04:47PM +0800, Chenyi Qiang wrote:
> 
> 
> On 9/3/2024 1:44 PM, Tony Lindgren wrote:
> > On Tue, Sep 03, 2024 at 10:58:11AM +0800, Chenyi Qiang wrote:
> >> On 8/13/2024 6:48 AM, Rick Edgecombe wrote:
> >>> From: Isaku Yamahata <isaku.yamahata@intel.com>
> >>> @@ -543,10 +664,23 @@ static int __tdx_td_init(struct kvm *kvm)
> >>>  		}
> >>>  	}
> >>>  
> >>> -	/*
> >>> -	 * Note, TDH_MNG_INIT cannot be invoked here.  TDH_MNG_INIT requires a dedicated
> >>> -	 * ioctl() to define the configure CPUID values for the TD.
> >>> -	 */
> >>> +	err = tdh_mng_init(kvm_tdx, __pa(td_params), &rcx);
> >>> +	if ((err & TDX_SEAMCALL_STATUS_MASK) == TDX_OPERAND_INVALID) {
> >>> +		/*
> >>> +		 * Because a user gives operands, don't warn.
> >>> +		 * Return a hint to the user because it's sometimes hard for the
> >>> +		 * user to figure out which operand is invalid.  SEAMCALL status
> >>> +		 * code includes which operand caused invalid operand error.
> >>> +		 */
> >>> +		*seamcall_err = err;
> >>
> >> I'm wondering if we could return or output more hint (i.e. the value of
> >> rcx) in the case of invalid operand. For example, if seamcall returns
> >> with INVALID_OPERAND_CPUID_CONFIG, rcx will contain the CPUID
> >> leaf/sub-leaf info.
> > 
> > Printing a decriptive error here would be nice when things go wrong.
> > Probably no need to return that information.
> > 
> > Sounds like you have a patch already in mind though :) Care to post a
> > patch against the current kvm-coco branch? If not, I can do it after all
> > the obvious comment changes are out of the way.
> 
> According to the comment above, this patch wants to return the hint to
> user as the user gives operands. I'm still uncertain if we should follow
> this to return value in some way or special-case the
> INVALID_OPERAND_CPUID_CONFIG like:
> 
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index c00c73b2ad4c..dd6e3149ff5a 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -2476,8 +2476,14 @@ static int __tdx_td_init(struct kvm *kvm, struct
> td_params *td_params,
>                  * Return a hint to the user because it's sometimes hard
> for the
>                  * user to figure out which operand is invalid.
> SEAMCALL status
>                  * code includes which operand caused invalid operand error.
> +                *
> +                * TDX_OPERAND_INVALID_CPUID_CONFIG contains more info
> +                * in rcx (i.e. leaf/sub-leaf), warn it to help figure
> +                * out the invalid CPUID config.
>                  */
>                 *seamcall_err = err;
> +               if (err == (TDX_OPERAND_INVALID |
> TDX_OPERAND_ID_CPUID_CONFIG))
> +                       pr_tdx_error_1(TDH_MNG_INIT, err, rcx);
>                 ret = -EINVAL;
>                 goto teardown;
>         } else if (WARN_ON_ONCE(err)) {
> diff --git a/arch/x86/kvm/vmx/tdx_errno.h b/arch/x86/kvm/vmx/tdx_errno.h
> index f9dbb3a065cc..311c3f03d398 100644
> --- a/arch/x86/kvm/vmx/tdx_errno.h
> +++ b/arch/x86/kvm/vmx/tdx_errno.h
> @@ -30,6 +30,7 @@
>   * detail information
>   */
>  #define TDX_OPERAND_ID_RCX                     0x01
> +#define TDX_OPERAND_ID_CPUID_CONFIG            0x45
>  #define TDX_OPERAND_ID_TDR                     0x80
>  #define TDX_OPERAND_ID_SEPT                    0x92
>  #define TDX_OPERAND_ID_TD_EPOCH                        0xa9
> 

OK yes that should take care of the issue, I doubt that this can be
automatically be handled by the caller even a better error code
was returned.

Regards,

Tony

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Edgecombe, Rick P]

On Thu, 2024-09-05 at 12:31 +0300, Tony Lindgren wrote:
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index c00c73b2ad4c..dd6e3149ff5a 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -2476,8 +2476,14 @@ static int __tdx_td_init(struct kvm *kvm, struct
> > td_params *td_params,
> >                   * Return a hint to the user because it's sometimes hard
> > for the
> >                   * user to figure out which operand is invalid.
> > SEAMCALL status
> >                   * code includes which operand caused invalid operand
> > error.
> > +                *
> > +                * TDX_OPERAND_INVALID_CPUID_CONFIG contains more info
> > +                * in rcx (i.e. leaf/sub-leaf), warn it to help figure
> > +                * out the invalid CPUID config.
> >                   */
> >                  *seamcall_err = err;
> > +               if (err == (TDX_OPERAND_INVALID |
> > TDX_OPERAND_ID_CPUID_CONFIG))
> > +                       pr_tdx_error_1(TDH_MNG_INIT, err, rcx);
> >                  ret = -EINVAL;
> >                  goto teardown;

Currently we filter by supported CPUID bits. But if we drop that filter and just
allow the TDX module to reject (based on discussion
https://lore.kernel.org/kvm/CABgObfbyd-a_bD-3fKmF3jVgrTiCDa3SHmrmugRji8BB-vs5GA@mail.gmail.com)

...then I guess this could be useful for userspace debugging. I'd say let's
leave this for a follow on patch. It's not critical for now.

Re: [PATCH 14/25] KVM: TDX: initialize VM with TDX specific parameters

[Edgecombe, Rick P]

Xiaoyao,

On Mon, 2024-08-12 at 15:48 -0700, Rick Edgecombe wrote:
> +static int setup_tdparams_cpuids(struct kvm_cpuid2 *cpuid,
> +				 struct td_params *td_params)
> +{
> +	const struct tdx_sysinfo_td_conf *td_conf = &tdx_sysinfo->td_conf;
> +	const struct kvm_tdx_cpuid_config *c;
> +	const struct kvm_cpuid_entry2 *entry;
> +	struct tdx_cpuid_value *value;
> +	int i;
> +
> +	/*
> +	 * td_params.cpuid_values: The number and the order of cpuid_value must
> +	 * be same to the one of struct tdsysinfo.{num_cpuid_config, cpuid_configs}
> +	 * It's assumed that td_params was zeroed.
> +	 */
> +	for (i = 0; i < td_conf->num_cpuid_config; i++) {
> +		c = &kvm_tdx_caps->cpuid_configs[i];
> +		entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent,
> +					      c->leaf, c->sub_leaf);
> +		if (!entry)
> +			continue;
> +
> +		/*
> +		 * Check the user input value doesn't set any non-configurable
> +		 * bits reported by kvm_tdx_caps.
> +		 */
> +		if ((entry->eax & c->eax) != entry->eax ||
> +		    (entry->ebx & c->ebx) != entry->ebx ||
> +		    (entry->ecx & c->ecx) != entry->ecx ||
> +		    (entry->edx & c->edx) != entry->edx)
> +			return -EINVAL;
> +
> +		value = &td_params->cpuid_values[i];
> +		value->eax = entry->eax;
> +		value->ebx = entry->ebx;
> +		value->ecx = entry->ecx;
> +		value->edx = entry->edx;
> +	}
> +
> +	return 0;
> +}

We agreed to let the TDX module reject CPUID bits that are not supported instead
of having KVM do it. While removing conditional above I found that we actually
still need some filtering.

The problem is that the filtering here only filters bits for leafs that are in
kvm_tdx_caps, the other leafs are just ignored. But we can't pass those other
leafs to the TDX module for it to do verification on because the index they are
supposed to go in is determined by kvm_tdx_caps->cpuid_configs, so there is no
place to pass them.

So KVM still needs to make sure no leafs are provided that are not in
kvm_tdx_caps, otherwise it will accept bits from userspace and ignore them. It
turns out this is already happening because QEMU is not filtering the CPUID
leafs that it passes. After I changed KVM to reject the other leafs, I needed
the following QEMU change to not pass leafs not in tdx caps:

diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
index 29ff7d2f7e..990960ec27 100644
--- a/target/i386/kvm/tdx.c
+++ b/target/i386/kvm/tdx.c
@@ -648,22 +648,29 @@ static struct kvm_tdx_cpuid_config
*tdx_find_cpuid_config(uint32_t leaf, uint32_
 
 static void tdx_filter_cpuid(struct kvm_cpuid2 *cpuids)
 {
-    int i;
-    struct kvm_cpuid_entry2 *e;
+    int i, dest_cnt = 0;
+    struct kvm_cpuid_entry2 *src, *dest;
     struct kvm_tdx_cpuid_config *config;
 
     for (i = 0; i < cpuids->nent; i++) {
-        e = cpuids->entries + i;
-        config = tdx_find_cpuid_config(e->function, e->index);
+        src = cpuids->entries + i;
+        config = tdx_find_cpuid_config(src->function, src->index);
         if (!config) {
             continue;
         }
+        dest = cpuids->entries + dest_cnt;
+
+        dest->function = src->function;
+        dest->index = src->index;
+        dest->flags = src->flags;
+        dest->eax = src->eax & config->eax;
+        dest->ebx = src->ebx & config->ebx;
+        dest->ecx = src->ecx & config->ecx;
+        dest->edx = src->edx & config->edx;
 
-        e->eax &= config->eax;
-        e->ebx &= config->ebx;
-        e->ecx &= config->ecx;
-        e->edx &= config->edx;
+        dest_cnt++;
     }
+    cpuids->nent = dest_cnt;
 }
 
 int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)