Re: [PATCH v3 6/6] x86/vmware: Add TDX hypercall support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 12/19/23 5:00 PM, kirill.shutemov@xxxxxxxxxxxxxxx wrote:
On Tue, Dec 19, 2023 at 04:27:51PM -0800, Alexey Makhalov wrote:


On 12/19/23 3:23 PM, kirill.shutemov@xxxxxxxxxxxxxxx wrote:
On Tue, Dec 19, 2023 at 01:57:51PM -0800, Alexey Makhalov wrote:
diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 3aa1adaed18f..ef07ab7a07e1 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -428,6 +428,30 @@ static bool __init vmware_legacy_x2apic_available(void)
   		(eax & BIT(VCPU_LEGACY_X2APIC));
   }
+#ifdef CONFIG_INTEL_TDX_GUEST
+unsigned long vmware_tdx_hypercall(unsigned long cmd,
+				   struct tdx_module_args *args)
+{
+	if (!hypervisor_is_type(X86_HYPER_VMWARE))
+		return 0;

BTW, don't you want to warn here to? We don't expect vmware hypercalls to
be called by non-vmware guest, do we?

The answer is below...


+
+	if (cmd & ~VMWARE_CMD_MASK) {
+		pr_warn("Out of range command %x\n", cmd);
+		return 0;

Is zero success? Shouldn't it be an error?

VMware hypercalls do not have a standard way of signalling an error.
To generalize expectations from the caller perspective of any existing
hypercalls: error (including hypercall is not supported or disabled) is when
return value is 0 and out1/2 are unchanged or equal to in1/in2.

You are talking about signaling errors over hypercall transport. But if
kernel can see that something is wrong why cannot it signal the issue
clearly to caller. It is going to be in-kernel convention.These "return 0" blocks were introduced to protect against non-vmware
guest or arbitrary modules trying to use __tdx_hypercall via exported vmware_tdx_hypercall function. In this case, it will be NOOP behavior with no or minor side effects.

From valid vmware_hypercall callers point of view, there is no such thing as a hypercall not available. Once guest detection code recognizes VMWare hypervisor via cpuid, it will start using hypercalls in accordance to per-call API.

Valid VMware guest code will never go into first return, no warning required. Second return can be hit in rare cases for example during development phase, or, hypothetical case, when cmd was dynamically generated.
That's why we have a warning warning only for the second condition.

While speaking about it, I'm started to lean towards your recommendation. Yes, we can return standard error code such as -EINVAL or just -1 instead of "return 0" in this function. And it will be algorithmically correct. As if Vmware guest caller provide out of range cmd - it is not documented behavior.

Speaking of additional in-kernel convention for passing additional parameter if error happens, it does not makes sense for me because: 1. existing caller codes analyze output argument to recognize error error response from the hypervisor. Adding one additional check for in-kernel errors just for TDX path which will be never hit by valid code in production is an unnecessary overhead. 2. It will definitely add an overhead as an error code will require one more output value, or out0 should be moved from return in-register value to return by pointer function argument.

Summarizing, overloading vmware_tdx_hypercall return value by arg0 (from the hypervisor) and kernel error (-1 or any other) seems like reasonable change.


And to very least, it has to be pr_warn_once().

Good catch! Will change it.

Thanks,
--Alexey




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux