Re: [PATCH v16 19/20] KVM: selftests: Add support for TDX ucalls, via TDVMCALL_REPORT_FATAL_ERROR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] kvm: selftests: ucall_arch_init() permanently overwrites global ucall_x86_ops
--

commit e57af1ee0994af97f6ae447fc9a5abebc0287e67
Author: Sean Christopherson <seanjc@google.com>

KVM: selftests: Add support for TDX ucalls, via TDVMCALL_REPORT_FATAL_ERROR

This patch adds TDX ucall support in KVM selftests by passing the ucall
payload address and a magic value via TDVMCALL_REPORT_FATAL_ERROR. When a
TDX VM is initialized in ucall_arch_init(), ucall_x86_ops is updated to
use the TDX ucall callbacks and synced to the guest.

> diff --git a/tools/testing/selftests/kvm/lib/x86/ucall.c b/tools/testing/selftests/kvm/lib/x86/ucall.c
> index 3aec4dd46d1b2..2a1c61785e5fb 100644
> --- a/tools/testing/selftests/kvm/lib/x86/ucall.c
> +++ b/tools/testing/selftests/kvm/lib/x86/ucall.c

[ ... ]

> @@ -37,6 +61,11 @@ static struct {
>  
>  void ucall_arch_init(struct kvm_vm *vm, gpa_t mmio_gpa)
>  {
> +	if (is_tdx_vm(vm)) {
> +		ucall_x86_ops.do_ucall = ucall_tdx_do_ucall;
> +		ucall_x86_ops.get_ucall = ucall_tdx_get_ucall;
> +	}
> +
>  	sync_global_to_guest(vm, ucall_x86_ops);
>  }

[Severity: Medium]
As reported by Sashiko on v15 [1], since ucall_x86_ops is a process-wide
global structure in tools/testing/selftests/kvm/lib/x86/ucall.c:

static struct {
	void (*do_ucall)(gva_t uc);
	void *(*get_ucall)(struct kvm_vcpu *vcpu);
} ucall_x86_ops = {
	.do_ucall = ucall_x86_do_ucall,
	.get_ucall = ucall_x86_get_ucall,
};

does overwriting ucall_x86_ops in ucall_arch_init() when is_tdx_vm(vm) is
true without restoring the default handlers when !is_tdx_vm(vm) break tests
that create a non-TDX VM after a TDX VM (or run both concurrently) in the
same process?

If a non-TDX VM is initialized after a TDX VM via ucall_init() ->
ucall_arch_init(), sync_global_to_guest(vm, ucall_x86_ops) will copy
ucall_tdx_do_ucall into the non-TDX guest, causing the guest to execute
TDCALL and fault with #UD on its first ucall.

Similarly, on the host side, ucall_arch_get_ucall() dispatches through the
single global ucall_x86_ops.get_ucall pointer:

void *ucall_arch_get_ucall(struct kvm_vcpu *vcpu)
{
	return ucall_x86_ops.get_ucall(vcpu);
}

so get_ucall() -> ucall_arch_get_ucall() will check for
KVM_SYSTEM_EVENT_TDX_FATAL instead of KVM_EXIT_IO for any non-TDX VM once a
TDX VM has been initialized. Should ucall_arch_init() restore the default x86
callbacks when !is_tdx_vm(vm) (or should ucall_arch_get_ucall() dispatch
based on is_tdx_vm(vcpu->vm))?

[1] https://lore.kernel.org/r/20261002091337.9BDF91F0089A@smtp.kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-tdx-selftests-v16-0-2024e8fa517a@google.com?part=19