| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230117160330.GA2959778@ls.amr.corp.intel.com>
Date: Tue, 17 Jan 2023 08:03:30 -0800
From: Isaku Yamahata <isaku.yamahata@...il.com>
To: Zhi Wang <zhi.wang.linux@...il.com>
Cc: isaku.yamahata@...el.com, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, isaku.yamahata@...il.com,
Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com,
Sean Christopherson <seanjc@...gle.com>,
Sagi Shahar <sagis@...gle.com>,
David Matlack <dmatlack@...gle.com>
Subject: Re: [PATCH v11 004/113] KVM: TDX: Initialize the TDX module when
loading the KVM intel kernel module
On Fri, Jan 13, 2023 at 02:31:58PM +0200,
Zhi Wang <zhi.wang.linux@...il.com> wrote:
> On Thu, 12 Jan 2023 08:31:12 -0800
> isaku.yamahata@...el.com wrote:
>
> > From: Isaku Yamahata <isaku.yamahata@...el.com>
> >
> > TDX requires several initialization steps for KVM to create guest TDs.
> > Detect CPU feature, enable VMX (TDX is based on VMX), detect the TDX
> > module availability, and initialize it. This patch implements those
> > steps.
> >
> > There are several options on when to initialize the TDX module. A.)
> > kernel module loading time, B.) the first guest TD creation time. A.)
> > was chosen. With B.), a user may hit an error of the TDX initialization
> > when trying to create the first guest TD. The machine that fails to
> > initialize the TDX module can't boot any guest TD further. Such failure
> > is undesirable and a surprise because the user expects that the machine
> > can accommodate guest TD, but actually not. So A.) is better than B.).
> >
> > Introduce a module parameter, enable_tdx, to explicitly enable TDX KVM
> > support. It's off by default to keep same behavior for those who don't
> > use TDX. Implement hardware_setup method to detect TDX feature of CPU.
> > Because TDX requires all present CPUs to enable VMX (VMXON). The x86
> > specific kvm_arch_post_hardware_enable_setup overrides the existing weak
> > symbol of kvm_arch_post_hardware_enable_setup which is called at the KVM
> > module initialization.
> >
> > Suggested-by: Sean Christopherson <seanjc@...gle.com>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> > ---
> > arch/x86/kvm/Makefile | 1 +
> > arch/x86/kvm/vmx/main.c | 33 +++++++++++++++++++++++-----
> > arch/x86/kvm/vmx/tdx.c | 44 ++++++++++++++++++++++++++++++++++++++
> > arch/x86/kvm/vmx/vmx.c | 39 +++++++++++++++++++++++++++++++++
> > arch/x86/kvm/vmx/x86_ops.h | 10 +++++++++
> > 5 files changed, 122 insertions(+), 5 deletions(-)
> > create mode 100644 arch/x86/kvm/vmx/tdx.c
> >
> > diff --git a/arch/x86/kvm/Makefile b/arch/x86/kvm/Makefile
> > index 0e894ae23cbc..4b01ab842ab7 100644
> > --- a/arch/x86/kvm/Makefile
> > +++ b/arch/x86/kvm/Makefile
> > @@ -25,6 +25,7 @@ kvm-$(CONFIG_KVM_SMM) += smm.o
> > kvm-intel-y += vmx/vmx.o vmx/vmenter.o vmx/pmu_intel.o
> > vmx/vmcs12.o \ vmx/hyperv.o vmx/nested.o vmx/posted_intr.o vmx/main.o
> > kvm-intel-$(CONFIG_X86_SGX_KVM) += vmx/sgx.o
> > +kvm-intel-$(CONFIG_INTEL_TDX_HOST) += vmx/tdx.o
> >
> > kvm-amd-y += svm/svm.o svm/vmenter.o svm/pmu.o
> > svm/nested.o svm/avic.o \ svm/sev.o svm/hyperv.o
> > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> > index 18f659d1d456..f5d1166d2718 100644
> > --- a/arch/x86/kvm/vmx/main.c
> > +++ b/arch/x86/kvm/vmx/main.c
> > @@ -7,6 +7,22 @@
> > #include "pmu.h"
> > #include "tdx.h"
> >
> > +static bool enable_tdx __ro_after_init =
> > IS_ENABLED(CONFIG_INTEL_TDX_HOST); +module_param_named(tdx, enable_tdx,
> > bool, 0444); +
>
> The comments says "TDX is off by default". It seems default on/off is controlled
> by the kernel configuration here.
I'll update the comments.
> > +static __init int vt_hardware_setup(void)
> > +{
> > + int ret;
> > +
> > + ret = vmx_hardware_setup();
> > + if (ret)
> > + return ret;
> > +
> > + enable_tdx = enable_tdx && !tdx_hardware_setup(&vt_x86_ops);
> > +
> > + return 0;
> > +}
> > +
> > struct kvm_x86_ops vt_x86_ops __initdata = {
> > .name = KBUILD_MODNAME,
> >
> > @@ -149,7 +165,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
> > };
> >
> > struct kvm_x86_init_ops vt_init_ops __initdata = {
> > - .hardware_setup = vmx_hardware_setup,
> > + .hardware_setup = vt_hardware_setup,
> > .handle_intel_pt_intr = NULL,
> >
> > .runtime_ops = &vt_x86_ops,
> > @@ -182,10 +198,17 @@ static int __init vt_init(void)
> > * Common KVM initialization _must_ come last, after this,
> > /dev/kvm is
> > * exposed to userspace!
> > */
> > - vt_x86_ops.vm_size = max(sizeof(struct kvm_vmx), sizeof(struct
> > kvm_tdx));
> > - vcpu_size = max(sizeof(struct vcpu_vmx), sizeof(struct
> > vcpu_tdx));
> > - vcpu_align = max(__alignof__(struct vcpu_vmx),
> > - __alignof__(struct vcpu_tdx));
> > + vt_x86_ops.vm_size = sizeof(struct kvm_vmx);
> > + vcpu_size = sizeof(struct vcpu_vmx);
> > + vcpu_align = __alignof__(struct vcpu_vmx);
> > + if (enable_tdx) {
> > + vt_x86_ops.vm_size = max_t(unsigned int,
> > vt_x86_ops.vm_size,
> > + sizeof(struct kvm_tdx));
> > + vcpu_size = max_t(unsigned int, vcpu_size,
> > + sizeof(struct vcpu_tdx));
> > + vcpu_align = max_t(unsigned int, vcpu_align,
> > + __alignof__(struct vcpu_tdx));
> > + }
> > r = kvm_init(vcpu_size, vcpu_align, THIS_MODULE);
> > if (r)
> > goto err_kvm_init;
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > new file mode 100644
> > index 000000000000..d7a276118940
> > --- /dev/null
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -0,0 +1,44 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include <linux/cpu.h>
> > +
> > +#include <asm/tdx.h>
> > +
> > +#include "capabilities.h"
> > +#include "x86_ops.h"
> > +#include "tdx.h"
> > +#include "x86.h"
> > +
> > +#undef pr_fmt
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +static int __init tdx_module_setup(void)
> > +{
> > + int ret;
> > +
>
> Better mention the tdx_enable() is implemented in another patch? But I guess
> we need a wrapper here so that the compilation would succeed.
The coverletter mentions it. I'll make the commit message of this patch
mention it anyway.
> > + ret = tdx_enable();
> > + if (ret) {
> > + pr_info("Failed to initialize TDX module.\n");
> > + return ret;
> > + }
> > +
> > + pr_info("TDX is supported.\n");
> > + return 0;
> > +}
> > +
> > +int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
> > +{
> > + int r;
> > +
> > + if (!enable_ept) {
> > + pr_warn("Cannot enable TDX with EPT disabled\n");
> > + return -EINVAL;
> > + }
> > +
> > + /* TDX requires VMX. */
> > + r = vmxon_all();
> > + if (!r)
> > + r = tdx_module_setup();
> > + vmxoff_all();
> > +
> > + return r;
> > +}
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index 5de1792c9902..5dc7687dcf16 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -8147,6 +8147,45 @@ static unsigned int vmx_handle_intel_pt_intr(void)
> > return 1;
> > }
> >
> > +static __init void vmxon(void *arg)
> > +{
> > + int cpu = raw_smp_processor_id();
> > + u64 phys_addr = __pa(per_cpu(vmxarea, cpu));
> > + atomic_t *failed = arg;
> > + int r;
> > +
> > + if (cr4_read_shadow() & X86_CR4_VMXE) {
> > + r = -EBUSY;
> > + goto out;
> > + }
> > +
> > + r = kvm_cpu_vmxon(phys_addr);
> > +out:
> > + if (r)
> > + atomic_inc(failed);
> > +}
> > +
> > +__init int vmxon_all(void)
> > +{
> > + atomic_t failed = ATOMIC_INIT(0);
> > +
> > + on_each_cpu(vmxon, &failed, 1);
> > +
> > + if (atomic_read(&failed))
> > + return -EBUSY;
> > + return 0;
> > +}
> > +
> > +static __init void vmxoff(void *junk)
> > +{
> > + cpu_vmxoff();
> > +}
> > +
> > +__init void vmxoff_all(void)
> > +{
> > + on_each_cpu(vmxoff, NULL, 1);
> > +}
> > +
> > static __init void vmx_setup_user_return_msrs(void)
> > {
> >
> > diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
> > index 051b5c4b5c2f..fbc57fcbdd21 100644
> > --- a/arch/x86/kvm/vmx/x86_ops.h
> > +++ b/arch/x86/kvm/vmx/x86_ops.h
> > @@ -20,6 +20,10 @@ bool kvm_is_vmx_supported(void);
> > int __init vmx_init(void);
> > void vmx_exit(void);
> >
> > +__init int vmxon_all(void);
> > +__init void vmxoff_all(void);
> > +__init int vmx_hardware_setup(void);
> > +
> > extern struct kvm_x86_ops vt_x86_ops __initdata;
> > extern struct kvm_x86_init_ops vt_init_ops __initdata;
> >
> > @@ -133,4 +137,10 @@ void vmx_cancel_hv_timer(struct kvm_vcpu *vcpu);
> > #endif
> > void vmx_setup_mce(struct kvm_vcpu *vcpu);
> >
> > +#ifdef CONFIG_INTEL_TDX_HOST
> > +int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops);
> > +#else
> > +static inline int tdx_hardware_setup(struct kvm_x86_ops *x86_ops) {
> > return 0; } +#endif
> > +
> > #endif /* __KVM_X86_VMX_X86_OPS_H */
>
--
Isaku Yamahata <isaku.yamahata@...il.com>
Powered by blists - more mailing lists