1. Patchew
  2. linux
  3. [PATCH 0/2] TDX: Clean up the definitions of TDX ATTRIBUTES
  4. View patch

[PATCH 2/2] KVM: TDX: Remove redundant definitions of TDX_TD_ATTR_*

Xiaoyao Li posted 2 patches 3 months ago
There is a newer version of this series
[PATCH 2/2] KVM: TDX: Remove redundant definitions of TDX_TD_ATTR_*
Posted by Xiaoyao Li 3 months ago 
There are definitions of TD attributes bits inside asm/shared/tdx.h as
TDX_ATTR_*.

Remove KVM's definitions and use the ones in asm/shared/tdx.h

Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 arch/x86/kvm/vmx/tdx.c      | 4 ++--
 arch/x86/kvm/vmx/tdx_arch.h | 6 ------
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index c539c2e6109f..efb7d589b672 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -62,7 +62,7 @@ void tdh_vp_wr_failed(struct vcpu_tdx *tdx, char *uclass, char *op, u32 field,
 	pr_err("TDH_VP_WR[%s.0x%x]%s0x%llx failed: 0x%llx\n", uclass, field, op, val, err);
 }
 
-#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
+#define KVM_SUPPORTED_TD_ATTRS (TDX_ATTR_SEPT_VE_DISABLE)
 
 static __always_inline struct kvm_tdx *to_kvm_tdx(struct kvm *kvm)
 {
@@ -700,7 +700,7 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
 	vcpu->arch.l1_tsc_scaling_ratio = kvm_tdx->tsc_multiplier;
 
 	vcpu->arch.guest_state_protected =
-		!(to_kvm_tdx(vcpu->kvm)->attributes & TDX_TD_ATTR_DEBUG);
+		!(to_kvm_tdx(vcpu->kvm)->attributes & TDX_ATTR_DEBUG);
 
 	if ((kvm_tdx->xfam & XFEATURE_MASK_XTILE) == XFEATURE_MASK_XTILE)
 		vcpu->arch.xfd_no_write_intercept = true;
diff --git a/arch/x86/kvm/vmx/tdx_arch.h b/arch/x86/kvm/vmx/tdx_arch.h
index a30e880849e3..350143b9b145 100644
--- a/arch/x86/kvm/vmx/tdx_arch.h
+++ b/arch/x86/kvm/vmx/tdx_arch.h
@@ -75,12 +75,6 @@ struct tdx_cpuid_value {
 	u32 edx;
 } __packed;
 
-#define TDX_TD_ATTR_DEBUG		BIT_ULL(0)
-#define TDX_TD_ATTR_SEPT_VE_DISABLE	BIT_ULL(28)
-#define TDX_TD_ATTR_PKS			BIT_ULL(30)
-#define TDX_TD_ATTR_KL			BIT_ULL(31)
-#define TDX_TD_ATTR_PERFMON		BIT_ULL(63)
-
 #define TDX_EXT_EXIT_QUAL_TYPE_MASK	GENMASK(3, 0)
 #define TDX_EXT_EXIT_QUAL_TYPE_PENDING_EPT_VIOLATION  6
 /*
-- 
2.43.0
Re: [PATCH 2/2] KVM: TDX: Remove redundant definitions of TDX_TD_ATTR_*
Posted by Sean Christopherson 3 months ago 
On Tue, Jul 08, 2025, Xiaoyao Li wrote:
> There are definitions of TD attributes bits inside asm/shared/tdx.h as
> TDX_ATTR_*.
> 
> Remove KVM's definitions and use the ones in asm/shared/tdx.h
> 
> Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  arch/x86/kvm/vmx/tdx.c      | 4 ++--
>  arch/x86/kvm/vmx/tdx_arch.h | 6 ------
>  2 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index c539c2e6109f..efb7d589b672 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -62,7 +62,7 @@ void tdh_vp_wr_failed(struct vcpu_tdx *tdx, char *uclass, char *op, u32 field,
>  	pr_err("TDH_VP_WR[%s.0x%x]%s0x%llx failed: 0x%llx\n", uclass, field, op, val, err);
>  }
>  
> -#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
> +#define KVM_SUPPORTED_TD_ATTRS (TDX_ATTR_SEPT_VE_DISABLE)

Would it make sense to rename KVM_SUPPORTED_TD_ATTRS to KVM_SUPPORTED_TDX_ATTRS?
The names from common code lack the TD qualifier, and I think it'd be helpful for
readers to have have TDX in the name (even though I agree "TD" is more precise).
Re: [PATCH 2/2] KVM: TDX: Remove redundant definitions of TDX_TD_ATTR_*
Posted by Edgecombe, Rick P 3 months ago 
On Tue, 2025-07-08 at 07:03 -0700, Sean Christopherson wrote:
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index c539c2e6109f..efb7d589b672 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -62,7 +62,7 @@ void tdh_vp_wr_failed(struct vcpu_tdx *tdx, char *uclass,
> > char *op, u32 field,
> >   	pr_err("TDH_VP_WR[%s.0x%x]%s0x%llx failed: 0x%llx\n", uclass,
> > field, op, val, err);
> >   }
> >   
> > -#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
> > +#define KVM_SUPPORTED_TD_ATTRS (TDX_ATTR_SEPT_VE_DISABLE)
> 
> Would it make sense to rename KVM_SUPPORTED_TD_ATTRS to
> KVM_SUPPORTED_TDX_ATTRS?
> The names from common code lack the TD qualifier, and I think it'd be helpful
> for
> readers to have have TDX in the name (even though I agree "TD" is more
> precise).

It's useful to know that these are per-TD attributes and not per-TDX module.
Especially for TDX_TD_ATTR_DEBUG. I kind of prefer the KVM naming scheme that is
removed in this patch.
Re: [PATCH 2/2] KVM: TDX: Remove redundant definitions of TDX_TD_ATTR_*
Posted by Sean Christopherson 3 months ago 
On Tue, Jul 08, 2025, Rick P Edgecombe wrote:
> On Tue, 2025-07-08 at 07:03 -0700, Sean Christopherson wrote:
> > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > > index c539c2e6109f..efb7d589b672 100644
> > > --- a/arch/x86/kvm/vmx/tdx.c
> > > +++ b/arch/x86/kvm/vmx/tdx.c
> > > @@ -62,7 +62,7 @@ void tdh_vp_wr_failed(struct vcpu_tdx *tdx, char *uclass,
> > > char *op, u32 field,
> > >   	pr_err("TDH_VP_WR[%s.0x%x]%s0x%llx failed: 0x%llx\n", uclass,
> > > field, op, val, err);
> > >   }
> > >   
> > > -#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
> > > +#define KVM_SUPPORTED_TD_ATTRS (TDX_ATTR_SEPT_VE_DISABLE)
> > 
> > Would it make sense to rename KVM_SUPPORTED_TD_ATTRS to
> > KVM_SUPPORTED_TDX_ATTRS?
> > The names from common code lack the TD qualifier, and I think it'd be helpful
> > for
> > readers to have have TDX in the name (even though I agree "TD" is more
> > precise).
> 
> It's useful to know that these are per-TD attributes and not per-TDX module.
> Especially for TDX_TD_ATTR_DEBUG. I kind of prefer the KVM naming scheme that is
> removed in this patch.

Heh, as does Xiaoyao, and me too.  I thought I was just being nitpicky :-)

Though in that case, I think I'd prefer KVM_SUPPORTED_TDX_TD_ATTRS.
Re: [PATCH 2/2] KVM: TDX: Remove redundant definitions of TDX_TD_ATTR_*
Posted by Xiaoyao Li 3 months ago 
On 7/8/2025 10:44 PM, Sean Christopherson wrote:
> On Tue, Jul 08, 2025, Rick P Edgecombe wrote:
>> On Tue, 2025-07-08 at 07:03 -0700, Sean Christopherson wrote:
>>>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>>>> index c539c2e6109f..efb7d589b672 100644
>>>> --- a/arch/x86/kvm/vmx/tdx.c
>>>> +++ b/arch/x86/kvm/vmx/tdx.c
>>>> @@ -62,7 +62,7 @@ void tdh_vp_wr_failed(struct vcpu_tdx *tdx, char *uclass,
>>>> char *op, u32 field,
>>>>    	pr_err("TDH_VP_WR[%s.0x%x]%s0x%llx failed: 0x%llx\n", uclass,
>>>> field, op, val, err);
>>>>    }
>>>>    
>>>> -#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
>>>> +#define KVM_SUPPORTED_TD_ATTRS (TDX_ATTR_SEPT_VE_DISABLE)
>>>
>>> Would it make sense to rename KVM_SUPPORTED_TD_ATTRS to
>>> KVM_SUPPORTED_TDX_ATTRS?
>>> The names from common code lack the TD qualifier, and I think it'd be helpful
>>> for
>>> readers to have have TDX in the name (even though I agree "TD" is more
>>> precise).
>>
>> It's useful to know that these are per-TD attributes and not per-TDX module.
>> Especially for TDX_TD_ATTR_DEBUG. I kind of prefer the KVM naming scheme that is
>> removed in this patch.
> 
> Heh, as does Xiaoyao, and me too.  I thought I was just being nitpicky :-)
> 
> Though in that case, I think I'd prefer KVM_SUPPORTED_TDX_TD_ATTRS.

To me, since the MACRO is only used inside kvm/vmx/tdx.c, it's OK 
without the _TDX_ prefix.

However, doing the rename is simple. So I'm going to rename it in a 
separate patch in the v2 unless being told unnecessary.
Re: [PATCH 2/2] KVM: TDX: Remove redundant definitions of TDX_TD_ATTR_*
Posted by Xiaoyao Li 3 months ago 
On 7/8/2025 10:03 PM, Sean Christopherson wrote:
> On Tue, Jul 08, 2025, Xiaoyao Li wrote:
>> There are definitions of TD attributes bits inside asm/shared/tdx.h as
>> TDX_ATTR_*.
>>
>> Remove KVM's definitions and use the ones in asm/shared/tdx.h
>>
>> Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> ---
>>   arch/x86/kvm/vmx/tdx.c      | 4 ++--
>>   arch/x86/kvm/vmx/tdx_arch.h | 6 ------
>>   2 files changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
>> index c539c2e6109f..efb7d589b672 100644
>> --- a/arch/x86/kvm/vmx/tdx.c
>> +++ b/arch/x86/kvm/vmx/tdx.c
>> @@ -62,7 +62,7 @@ void tdh_vp_wr_failed(struct vcpu_tdx *tdx, char *uclass, char *op, u32 field,
>>   	pr_err("TDH_VP_WR[%s.0x%x]%s0x%llx failed: 0x%llx\n", uclass, field, op, val, err);
>>   }
>>   
>> -#define KVM_SUPPORTED_TD_ATTRS (TDX_TD_ATTR_SEPT_VE_DISABLE)
>> +#define KVM_SUPPORTED_TD_ATTRS (TDX_ATTR_SEPT_VE_DISABLE)
> 
> Would it make sense to rename KVM_SUPPORTED_TD_ATTRS to KVM_SUPPORTED_TDX_ATTRS?
> The names from common code lack the TD qualifier, and I think it'd be helpful for
> readers to have have TDX in the name (even though I agree "TD" is more precise).

Personally, I prefer adding _TD_ to the common header, i.e., rename 
TDX_ATTR_SEPT_VE_DISABLE to TDX_TD_ATTR_SEPT_VE_DISABLE, or just 
TD_ATTR_SEPT_VE_DISABLE if dropping TDX prefix is acceptable.

Because TDX_ATTR OR TDX_ATTRIBUTES is ambiguous to me. There are other 
attributes defined in TDX spec, e.g., TDSYSINFO_STRUCT.ATTRIBUTES, GPA 
attributes.
Re: [PATCH 2/2] KVM: TDX: Remove redundant definitions of TDX_TD_ATTR_*
Posted by Huang, Kai 3 months ago 
On Tue, 2025-07-08 at 16:03 +0800, Xiaoyao Li wrote:
> There are definitions of TD attributes bits inside asm/shared/tdx.h as
> TDX_ATTR_*.
> 
> Remove KVM's definitions and use the ones in asm/shared/tdx.h

Nit: Missing period at the end of the sentence.

> 
> Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> 

Reviewed-by: Kai Huang <kai.huang@intel.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.