On Fri, May 23, 2025, Chao Gao wrote:
> +static __maybe_unused int seamldr_call(u64 fn, struct tdx_module_args *args)
> +{
> + u64 vmcs;
> + int ret;
> +
> + if (!is_seamldr_call(fn))
> + return -EINVAL;
> +
> + /*
> + * SEAMRET from P-SEAMLDR invalidates the current-VMCS pointer.
I'd rather this use human-friendly language as opposed to the SDM's pedantic
terminology, e.g. just "current VMCS".
> + * Save/restore current-VMCS pointer across P-SEAMLDR SEAMCALLs so
> + * that VMX instructions won't fail due to an invalid current-VMCS.
> + *
> + * Disable interrupt to prevent SMP call functions from seeing the
I would rather we establish a rule that KVM is allowed to do VMREAD/VMWRITE in
IRQ context, i.e. don't single out SMP function calls.
> + * invalid current-VMCS.
> + */
> + guard(irqsave)();
> +
> + ret = cpu_vmcs_store(&vmcs);
> + if (ret)
> + return ret;
> +
> + ret = seamldr_prerr(fn, args);
> +
> + /* Restore current-VMCS pointer */
> +#define INVALID_VMCS -1ULL
> + if (vmcs != INVALID_VMCS)
> + WARN_ON_ONCE(cpu_vmcs_load(vmcs));
> +
> + return ret;
> +}
> diff --git a/arch/x86/virt/vmx/vmx.h b/arch/x86/virt/vmx/vmx.h
> new file mode 100644
> index 000000000000..51e6460fd1fd
> --- /dev/null
> +++ b/arch/x86/virt/vmx/vmx.h
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef ARCH_X86_VIRT_VMX_H
> +#define ARCH_X86_VIRT_VMX_H
> +
> +#include <linux/printk.h>
> +
> +static inline int cpu_vmcs_load(u64 vmcs_pa)
> +{
> + asm goto("1: vmptrld %0\n\t"
> + ".byte 0x2e\n\t" /* branch not taken hint */
Heh, don't copy paste the crappy indentation, that was a result of Linus' tree-wide
changes from 4356e9f841f7 ("work around gcc bugs with 'asm goto' with outputs"),
i.e. not intentional.
Regarding question #3 from the cover letter:
3. Two helpers, cpu_vmcs_load() and cpu_vmcs_store(), are added in patch 3
to save and restore the current VMCS. KVM has a variant of cpu_vmcs_load(),
i.e., vmcs_load(). Extracting KVM's version would cause a lot of code
churn, and I don't think that can be justified for reducing ~16 LoC
duplication. Please let me know if you disagree.
I'm fine with the SEAMLDR code having its own code, because I agree it's not worth
extracting KVM's macro maze just to get at VMPTRLD. But I'm not fine with creating
a new, inferior framework. So if we elect to leave KVM alone for the time being,
I would prefer to simply open code VMPTRST and VMPTRLD in seamldr.c, e.g.
static inline int seamldr_call(u64 fn, struct tdx_module_args *args)
{
u64 vmcs;
int ret;
if (!is_seamldr_call(fn))
return -EINVAL;
/*
* SEAMRET from P-SEAMLDR invalidates the current VMCS. Save/restore
* the VMCS across P-SEAMLDR SEAMCALLs to avoid clobbering KVM state.
* Disable interrupts as KVM is allowed to do VMREAD/VMWRITE in IRQ
* context (but not NMI context).
*/
guard(irqsave)();
asm goto("1: vmptrst %0\n\t"
_ASM_EXTABLE(1b, %l[error])
: "=m" (&vmcs) : "cc" : error);
ret = seamldr_prerr(fn, args);
/*
* Restore the current VMCS pointer. VMPTSTR "returns" all ones if the
* current VMCS is invalid.
*/
if (vmcs != -1ULL) {
asm goto("1: vmptrld %0\n\t"
"jna %l[error]\n\t"
_ASM_EXTABLE(1b, %l[error])
: : "m" (&vmcs) : "cc" : error);
}
return ret;
error:
WARN_ONCE(1, "Failed to save/restore the current VMCS");
return -EIO;
}
