[PATCH 0/2] x86/virt/tdx: Minor sparse fixups

[PATCH 0/2] x86/virt/tdx: Minor sparse fixups

[Dave Hansen]

Sean recently suggested relying on sparse to add type safety in TDX code,
hoping that the robots would notice and complain. Well, that plan is not
working out so great. TDX is not even sparse clean today and nobody seems
to have noticed or cared.

I can see how folks might ignore the 0 vs. NULL complaints. But the
misplaced __user is actually bad enough it should be fixed no matter
what.

Might as well fix it all up.

Cc: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: "Kirill A. Shutemov" <kas@kernel.org>
Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

[PATCH 1/2] x86/virt/tdx: Remove __user annotation from kernel pointer

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

There are two 'kvm_cpuid2' pointers involved here. There's an "input"
side: 'td_cpuid' which is a normal kernel pointer and an 'output'
side. The output here is userspace and there is an attempt at properly
annotating the variable with __user:

	struct kvm_cpuid2 __user *output, *td_cpuid;

But, alas, this is wrong. The __user in the definition applies to both
'output' and 'td_cpuid'.

Fix it up by completely separating the two definitions so that it is
obviously correct without even having to know what the C syntax rules
even are.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: "Kirill A. Shutemov" <kas@kernel.org>
Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/kvm/vmx/tdx.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -puN arch/x86/kvm/vmx/tdx.c~tdx-sparse-fix-3 arch/x86/kvm/vmx/tdx.c
--- a/arch/x86/kvm/vmx/tdx.c~tdx-sparse-fix-3	2025-10-29 12:10:10.375383704 -0700
+++ b/arch/x86/kvm/vmx/tdx.c	2025-10-29 12:10:10.379384154 -0700
@@ -3054,7 +3054,8 @@ static int tdx_vcpu_get_cpuid_leaf(struc
 
 static int tdx_vcpu_get_cpuid(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
 {
-	struct kvm_cpuid2 __user *output, *td_cpuid;
+	struct kvm_cpuid2 __user *output;
+	struct kvm_cpuid2 *td_cpuid;
 	int r = 0, i = 0, leaf;
 	u32 level;
 
_

[PATCH 2/2] x86/virt/tdx: Fix sparse warnings from using 0 for NULL

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

sparse moans:

	... arch/x86/kvm/vmx/tdx.c:859:38: warning: Using plain integer as NULL pointer

for several TDX pointer initializations. While I love a good ptr=0
now and then, it's good to have quiet sparse builds.

Fix the sites up.

Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: x86@kernel.org
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: "Kirill A. Shutemov" <kas@kernel.org>
Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: kvm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---

 b/arch/x86/kvm/vmx/tdx.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff -puN arch/x86/kvm/vmx/tdx.c~tdx-sparse-fix-1 arch/x86/kvm/vmx/tdx.c
--- a/arch/x86/kvm/vmx/tdx.c~tdx-sparse-fix-1	2025-10-29 12:10:11.114466713 -0700
+++ b/arch/x86/kvm/vmx/tdx.c	2025-10-29 12:10:11.119467274 -0700
@@ -856,7 +856,7 @@ void tdx_vcpu_free(struct kvm_vcpu *vcpu
 	}
 	if (tdx->vp.tdvpr_page) {
 		tdx_reclaim_control_page(tdx->vp.tdvpr_page);
-		tdx->vp.tdvpr_page = 0;
+		tdx->vp.tdvpr_page = NULL;
 		tdx->vp.tdvpr_pa = 0;
 	}
 
@@ -2642,7 +2642,7 @@ free_tdcs:
 free_tdr:
 	if (tdr_page)
 		__free_page(tdr_page);
-	kvm_tdx->td.tdr_page = 0;
+	kvm_tdx->td.tdr_page = NULL;
 
 free_hkid:
 	tdx_hkid_free(kvm_tdx);
@@ -3016,7 +3016,7 @@ free_tdcx:
 free_tdvpr:
 	if (tdx->vp.tdvpr_page)
 		__free_page(tdx->vp.tdvpr_page);
-	tdx->vp.tdvpr_page = 0;
+	tdx->vp.tdvpr_page = NULL;
 	tdx->vp.tdvpr_pa = 0;
 
 	return ret;
_

Re: [PATCH 0/2] x86/virt/tdx: Minor sparse fixups

[Edgecombe, Rick P]

On Wed, 2025-10-29 at 12:48 -0700, Dave Hansen wrote:
> Sean recently suggested relying on sparse to add type safety in TDX code,
> hoping that the robots would notice and complain. Well, that plan is not
> working out so great. TDX is not even sparse clean today and nobody seems
> to have noticed or cared.
> 
> I can see how folks might ignore the 0 vs. NULL complaints. But the
> misplaced __user is actually bad enough it should be fixed no matter
> what.
> 
> Might as well fix it all up.

Oof. But my local distro version of sparse spit out a bunch of noise related to
"warning: unreplaced symbol" in "include/asm-generic/bitops/generic-non-
atomic.h". It also did not find these. But checking with the latest sparse build
from source did, and this fixes them.

Re: [PATCH 1/2] x86/virt/tdx: Remove __user annotation from kernel pointer

[Edgecombe, Rick P]

For the KVM side of tdx, the commits are getting prefixed with "KVM: TDX: ", and
"x86/virt/tdx" is being used arch/x86/virt/vmx/tdx/tdx.c. It's probably not too
late to adopt the one true naming scheme. I don't have a strong preference
except some consistency and that the maintainers agree :)


On Wed, 2025-10-29 at 12:48 -0700, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>

Pretty much the only difference between tip style logs and kvm/x86 style logs is
to lead with a short "what is the change" blurb before launching into the
background. Like:

KVM: TDX: Remove __user annotation from kernel pointer

Fix sparse warning in tdx_vcpu_get_cpuid().

There are two 'kvm_cpuid2' pointers involved here...

> 
> There are two 'kvm_cpuid2' pointers involved here. There's an "input"
> side: 'td_cpuid' which is a normal kernel pointer and an 'output'
> side. The output here is userspace and there is an attempt at properly
> annotating the variable with __user:
> 
> 	struct kvm_cpuid2 __user *output, *td_cpuid;
> 
> But, alas, this is wrong. The __user in the definition applies to both
> 'output' and 'td_cpuid'.
> 
> Fix it up by completely separating the two definitions so that it is
> obviously correct without even having to know what the C syntax rules
> even are.

If we want it:
Fixes: 488808e682e7 ("KVM: x86: Introduce KVM_TDX_GET_CPUID")

TIL on the syntax association here.

Re: [PATCH 2/2] x86/virt/tdx: Fix sparse warnings from using 0 for NULL

[Edgecombe, Rick P]

On Wed, 2025-10-29 at 12:48 -0700, Dave Hansen wrote:
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> sparse moans:
> 
> 	... arch/x86/kvm/vmx/tdx.c:859:38: warning: Using plain integer as NULL pointer
> 
> for several TDX pointer initializations. While I love a good ptr=0
> now and then, it's good to have quiet sparse builds.
> 
> Fix the sites up.
> 
If we want it:

Fixes: a50f673f25e0 ("KVM: TDX: Do TDX specific vcpu initialization")
Fixes: 8d032b683c29 ("KVM: TDX: create/destroy VM structure")

Re: [PATCH 0/2] x86/virt/tdx: Minor sparse fixups

[Edgecombe, Rick P]

On Wed, 2025-10-29 at 13:53 -0700, Rick Edgecombe wrote:
> But checking with the latest sparse build from source did, and this fixes them.

Minus the log questions:

Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

Re: [PATCH 1/2] x86/virt/tdx: Remove __user annotation from kernel pointer

[Dave Hansen]

On 10/29/25 14:06, Edgecombe, Rick P wrote:
> For the KVM side of tdx, the commits are getting prefixed with "KVM: TDX: ", and
> "x86/virt/tdx" is being used arch/x86/virt/vmx/tdx/tdx.c. It's probably not too
> late to adopt the one true naming scheme. I don't have a strong preference
> except some consistency and that the maintainers agree :)

Yeah, I just picked one. I honestly don't care what we do in the end. I
was also probably just going to send these in the tip/x86/tdx branch
unless anyone screams, so I did it the tip-ish way.

>> There are two 'kvm_cpuid2' pointers involved here. There's an "input"
>> side: 'td_cpuid' which is a normal kernel pointer and an 'output'
>> side. The output here is userspace and there is an attempt at properly
>> annotating the variable with __user:
>>
>> 	struct kvm_cpuid2 __user *output, *td_cpuid;
>>
>> But, alas, this is wrong. The __user in the definition applies to both
>> 'output' and 'td_cpuid'.
>>
>> Fix it up by completely separating the two definitions so that it is
>> obviously correct without even having to know what the C syntax rules
>> even are.
> 
> If we want it:
> Fixes: 488808e682e7 ("KVM: x86: Introduce KVM_TDX_GET_CPUID")

Thanks for that. I might dump in in, but sometimes folks can get all hot
and bothered about getting true fixes upstream fast. I was planning to
go go the slow next-merge-window route with these.

Re: [PATCH 1/2] x86/virt/tdx: Remove __user annotation from kernel pointer

[Sean Christopherson]

On Wed, Oct 29, 2025, Dave Hansen wrote:
> On 10/29/25 14:06, Edgecombe, Rick P wrote:
> > For the KVM side of tdx, the commits are getting prefixed with "KVM: TDX: ", and
> > "x86/virt/tdx" is being used arch/x86/virt/vmx/tdx/tdx.c. It's probably not too
> > late to adopt the one true naming scheme. I don't have a strong preference
> > except some consistency and that the maintainers agree :)
> 
> Yeah, I just picked one. I honestly don't care what we do in the end.

I do.  Being able to quickly determine if something touches KVM is valuable.  TDX
blurs the line since much of the code is split across KVM and x86/virt, but I
still find value in differentiating when possible.

> I was also probably just going to send these in the tip/x86/tdx branch unless
> anyone screams, so I did it the tip-ish way.

But this doesn't have anything to do with what tree the patches get routed through.
Scopes are always about what files/content is changing.

I also don't undertand why you would take these through tip.  They only touch
KVM (which is annoying hard to see since there's no shortlog in the cover letter).
Sure, they're minor changes and _probably_ won't conflict with anything, but again
I don't see how that matters.  These are clearly KVM patches.

Re: [PATCH 1/2] x86/virt/tdx: Remove __user annotation from kernel pointer

[Dave Hansen]

On 10/29/25 16:00, Sean Christopherson wrote:
...
> I also don't undertand why you would take these through tip.  They only touch
> KVM (which is annoying hard to see since there's no shortlog in the cover letter).
> Sure, they're minor changes and _probably_ won't conflict with anything, but again
> I don't see how that matters.  These are clearly KVM patches.

Hey, the more patches off my plate, the better.

I'm happy to make them more KVM-ish if you want and put the problem
statements in backwards. ;)

Re: [PATCH 1/2] x86/virt/tdx: Remove __user annotation from kernel pointer

[Sean Christopherson]

On Wed, Oct 29, 2025, Dave Hansen wrote:
> On 10/29/25 16:00, Sean Christopherson wrote:
> ...
> > I also don't undertand why you would take these through tip.  They only touch
> > KVM (which is annoying hard to see since there's no shortlog in the cover letter).
> > Sure, they're minor changes and _probably_ won't conflict with anything, but again
> > I don't see how that matters.  These are clearly KVM patches.
> 
> Hey, the more patches off my plate, the better.
> 
> I'm happy to make them more KVM-ish if you want and put the problem
> statements in backwards. ;)

LOL, I'm not _that_ particular.  Feel free to send a v2, but I'm also a-ok doing
fixups on the shortlogs when applying.  I'll also add the Fixes: tags Rick suggested,
as KVM x86 policy is to be liberal with Fixes, and only backport explicit stable@
patches.