| 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: <20230228195509.000073da@intel.com>
Date: Tue, 28 Feb 2023 19:55:09 +0200
From: Zhi Wang <zhi.wang.linux@...il.com>
To: Isaku Yamahata <isaku.yamahata@...il.com>
Cc: Zhi Wang <zhi.wang.linux@...il.com>, isaku.yamahata@...el.com,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
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 023/113] KVM: TDX: allocate/free TDX vcpu structure
On Mon, 27 Feb 2023 15:49:14 -0800
Isaku Yamahata <isaku.yamahata@...il.com> wrote:
> On Mon, Jan 16, 2023 at 12:46:06PM +0200,
> Zhi Wang <zhi.wang.linux@...il.com> wrote:
>
> > On Thu, 12 Jan 2023 08:31:31 -0800
> > isaku.yamahata@...el.com wrote:
> >
> > > From: Isaku Yamahata <isaku.yamahata@...el.com>
> > >
> > > The next step of TDX guest creation is to create vcpu. Allocate TDX vcpu
> > > structures, partially initialize it. Allocate pages of TDX vcpu for the
> > > TDX module. Actual donation TDX vcpu pages to the TDX module is not done
> > > yet.
> > >
> > > In the case of the conventional case, cpuid is empty at the initialization.
> > > and cpuid is configured after the vcpu initialization. Because TDX
> > > supports only X2APIC mode, cpuid is forcibly initialized to support X2APIC
> > > on the vcpu initialization.
> > >
> > > Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> > > ---
> > > Changes v10 -> v11:
> > > - NULL check of kvmalloc_array() in tdx_vcpu_reset. Move it to
> > > tdx_vcpu_create()
> > >
> > > Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> > > ---
> > > arch/x86/kvm/vmx/main.c | 40 ++++++++++++++++++--
> > > arch/x86/kvm/vmx/tdx.c | 75 ++++++++++++++++++++++++++++++++++++++
> > > arch/x86/kvm/vmx/x86_ops.h | 10 +++++
> > > arch/x86/kvm/x86.c | 2 +
> > > 4 files changed, 123 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> > > index ddf0742f1f67..59813ca05f36 100644
> > > --- a/arch/x86/kvm/vmx/main.c
> > > +++ b/arch/x86/kvm/vmx/main.c
> > > @@ -63,6 +63,38 @@ static void vt_vm_free(struct kvm *kvm)
> > > tdx_vm_free(kvm);
> > > }
> > >
> > > +static int vt_vcpu_precreate(struct kvm *kvm)
> > > +{
> > > + if (is_td(kvm))
> > > + return 0;
> > > +
> > > + return vmx_vcpu_precreate(kvm);
> > > +}
> > > +
> > > +static int vt_vcpu_create(struct kvm_vcpu *vcpu)
> > > +{
> > > + if (is_td_vcpu(vcpu))
> > > + return tdx_vcpu_create(vcpu);
> > > +
> > > + return vmx_vcpu_create(vcpu);
> > > +}
> > > +
> >
> > -----
> > > +static void vt_vcpu_free(struct kvm_vcpu *vcpu)
> > > +{
> > > + if (is_td_vcpu(vcpu))
> > > + return tdx_vcpu_free(vcpu);
> > > +
> > > + return vmx_vcpu_free(vcpu);
> > > +}
> > > +
> > > +static void vt_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> > > +{
> > > + if (is_td_vcpu(vcpu))
> > > + return tdx_vcpu_reset(vcpu, init_event);
> > > +
> > > + return vmx_vcpu_reset(vcpu, init_event);
> > > +}
> > > +
> > ----
> >
> > It seems a little strange to use return in this style. Would it be better like:
> >
> > -----
> > if (xxx) {
> > tdx_vcpu_reset(xxx);
> > return;
> > }
> >
> > vmx_vcpu_reset(xxx);
> > ----
> >
> > ?
>
> It's C11. I updated the code to not use the feature.
>
>
> > > static int vt_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
> > > {
> > > if (!is_td(kvm))
> > > @@ -90,10 +122,10 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
> > > .vm_destroy = vt_vm_destroy,
> > > .vm_free = vt_vm_free,
> > >
> > > - .vcpu_precreate = vmx_vcpu_precreate,
> > > - .vcpu_create = vmx_vcpu_create,
> > > - .vcpu_free = vmx_vcpu_free,
> > > - .vcpu_reset = vmx_vcpu_reset,
> > > + .vcpu_precreate = vt_vcpu_precreate,
> > > + .vcpu_create = vt_vcpu_create,
> > > + .vcpu_free = vt_vcpu_free,
> > > + .vcpu_reset = vt_vcpu_reset,
> > >
> > > .prepare_switch_to_guest = vmx_prepare_switch_to_guest,
> > > .vcpu_load = vmx_vcpu_load,
> > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > > index 557a609c5147..099f0737a5aa 100644
> > > --- a/arch/x86/kvm/vmx/tdx.c
> > > +++ b/arch/x86/kvm/vmx/tdx.c
> > > @@ -281,6 +281,81 @@ int tdx_vm_init(struct kvm *kvm)
> > > return 0;
> > > }
> > >
> > > +int tdx_vcpu_create(struct kvm_vcpu *vcpu)
> > > +{
> > > + struct kvm_cpuid_entry2 *e;
> > > +
> > > + /*
> > > + * On cpu creation, cpuid entry is blank. Forcibly enable
> > > + * X2APIC feature to allow X2APIC.
> > > + * Because vcpu_reset() can't return error, allocation is done here.
> > > + */
> > > + WARN_ON_ONCE(vcpu->arch.cpuid_entries);
> > > + WARN_ON_ONCE(vcpu->arch.cpuid_nent);
> > > + e = kvmalloc_array(1, sizeof(*e), GFP_KERNEL_ACCOUNT);
> > > + if (!e)
> > > + return -ENOMEM;
> > > + *e = (struct kvm_cpuid_entry2) {
> > > + .function = 1, /* Features for X2APIC */
> > > + .index = 0,
> > > + .eax = 0,
> > > + .ebx = 0,
> > > + .ecx = 1ULL << 21, /* X2APIC */
> > > + .edx = 0,
> > > + };
> > > + vcpu->arch.cpuid_entries = e;
> > > + vcpu->arch.cpuid_nent = 1;
> > > +
> > > + /* TDX only supports x2APIC, which requires an in-kernel local APIC. */
> > > + if (!vcpu->arch.apic)
> > > + return -EINVAL;
> > > +
> > > + fpstate_set_confidential(&vcpu->arch.guest_fpu);
> > > +
> > > + vcpu->arch.efer = EFER_SCE | EFER_LME | EFER_LMA | EFER_NX;
> > > +
> > > + vcpu->arch.cr0_guest_owned_bits = -1ul;
> > > + vcpu->arch.cr4_guest_owned_bits = -1ul;
> > > +
> > > + vcpu->arch.tsc_offset = to_kvm_tdx(vcpu->kvm)->tsc_offset;
> > > + vcpu->arch.l1_tsc_offset = vcpu->arch.tsc_offset;
> > > + vcpu->arch.guest_state_protected =
> > > + !(to_kvm_tdx(vcpu->kvm)->attributes & TDX_TD_ATTRIBUTE_DEBUG);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +void tdx_vcpu_free(struct kvm_vcpu *vcpu)
> > > +{
> > > + /* This is stub for now. More logic will come. */
> > > +}
> > > +
> > > +void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> > > +{
> > > + struct msr_data apic_base_msr;
> > > +
> > > + /* TDX doesn't support INIT event. */
> > > + if (WARN_ON_ONCE(init_event))
> > > + goto td_bugged;
> > > +
> > > + /* TDX rquires X2APIC. */
> > ^
> > requires
> > > + apic_base_msr.data = APIC_DEFAULT_PHYS_BASE | LAPIC_MODE_X2APIC;
> > > + if (kvm_vcpu_is_reset_bsp(vcpu))
> > > + apic_base_msr.data |= MSR_IA32_APICBASE_BSP;
> > > + apic_base_msr.host_initiated = true;
> > > + if (WARN_ON_ONCE(kvm_set_apic_base(vcpu, &apic_base_msr)))
> > > + goto td_bugged;
> > > +
> > > + /*
> > > + * Don't update mp_state to runnable because more initialization
> > > + * is needed by TDX_VCPU_INIT.
> > > + */
> > > + return;
> > > +
> > > +td_bugged:
> > > + vcpu->kvm->vm_bugged = true;
> > > +}
> > > +
> >
> > 1) Using vm_bugged to terminate the VM creation feels off. When
> > using it in creation path, the termination still happens in xx_vcpu_run().
> >
> > Thus, even something wrong happens at a certain point of the creation path,
> > the VM creation still continues. Until the xxx_vcpu_run(), the VM termination
> > finally happens.
> >
> > Why not just fail in the creation path?
>
> I converted vm_bugged to KVM_BUG_ON. Because the td_bugged case shouldn't
> happen for TDX case, it's worthwhile for KVM_BUG_ON()
>
>
> > 2) Move
> >
> > > + apic_base_msr.data = APIC_DEFAULT_PHYS_BASE | LAPIC_MODE_X2APIC;
> > > + if (kvm_vcpu_is_reset_bsp(vcpu))
> > > + apic_base_msr.data |= MSR_IA32_APICBASE_BSP;
> > > + apic_base_msr.host_initiated = true;
> >
> > to:
> >
> > void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
> > {
> > struct kvm_lapic *apic = vcpu->arch.apic;
> > u64 msr_val;
> > int i;
> >
> > if (!init_event) {
> > msr_val = APIC_DEFAULT_PHYS_BASE | MSR_IA32_APICBASE_ENABLE;
> >
> > /* here */
> > if (is_td_vcpu(vcpu))
> > msr_val = xxxx;
> > if (kvm_vcpu_is_reset_bsp(vcpu))
> > msr_val |= MSR_IA32_APICBASE_BSP;
> > kvm_lapic_set_base(vcpu, msr_val);
> > }
>
> No. Because I'm trying to contain is_td/is_td_vcpu in vmx specific and not use
> in common x86 code.
>
I guess so. Centeralizing the initialization would be the nice and greatly
improve the readablity of the code. Maybe adding a new callback in kvm x86_ops
like .get_default_msr_val instead.
>
> > PS: Is there any reason that APIC MSR in TDX doesn't need
> > MSR_IA32_APICBASE_ENABLE?
>
> because LAPIC_MODE_X2APIC includes MSR_IA32_APICBASE_ENABLE.
> In lapic.h
> LAPIC_MODE_X2APIC = MSR_IA32_APICBASE_ENABLE | X2APIC_ENABLE,
>
>
>
> > 3) Change the following:
> >
> > > +
> > > + /* TDX doesn't support INIT event. */
> > > + if (WARN_ON_ONCE(init_event))
> > > + goto td_bugged;
> > > +
> >
> > to
> > WARN_ON_ONCE(init_event);
> >
> > kvm_cpu_deliver_init() will trigger a kvm_vcpu_reset(xxx, init_event=true),
> > but you have already avoided this in vt_vcpu_deliver_init(). A warn
> > is good enough to remind people.
>
> I converted it into KVM_BUG_ON().
>
>
> > With these changes, tdx_vcpu_reset() will only contain the CPUID configuration
> > , using the vm_bugged to terminate the VM in tdx_vcpu_reset() can be removed.
> >
> > > int tdx_dev_ioctl(void __user *argp)
> > > {
> > > struct kvm_tdx_capabilities __user *user_caps;
> > > diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
> > > index 6c40dda1cc2f..37ab2cfd35bc 100644
> > > --- a/arch/x86/kvm/vmx/x86_ops.h
> > > +++ b/arch/x86/kvm/vmx/x86_ops.h
> > > @@ -147,7 +147,12 @@ int tdx_offline_cpu(void);
> > > int tdx_vm_init(struct kvm *kvm);
> > > void tdx_mmu_release_hkid(struct kvm *kvm);
> > > void tdx_vm_free(struct kvm *kvm);
> > > +
> > > int tdx_vm_ioctl(struct kvm *kvm, void __user *argp);
> > > +
> > > +int tdx_vcpu_create(struct kvm_vcpu *vcpu);
> > > +void tdx_vcpu_free(struct kvm_vcpu *vcpu);
> > > +void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event);
> > > #else
> > > static inline int tdx_hardware_setup(struct kvm_x86_ops *x86_ops) { return 0; }
> > > static inline void tdx_hardware_unsetup(void) {}
> > > @@ -159,7 +164,12 @@ static inline int tdx_vm_init(struct kvm *kvm) { return -EOPNOTSUPP; }
> > > static inline void tdx_mmu_release_hkid(struct kvm *kvm) {}
> > > static inline void tdx_flush_shadow_all_private(struct kvm *kvm) {}
> > > static inline void tdx_vm_free(struct kvm *kvm) {}
> > > +
> > > static inline int tdx_vm_ioctl(struct kvm *kvm, void __user *argp) { return -EOPNOTSUPP; }
> > > +
> > > +static inline int tdx_vcpu_create(struct kvm_vcpu *vcpu) { return -EOPNOTSUPP; }
> > > +static inline void tdx_vcpu_free(struct kvm_vcpu *vcpu) {}
> > > +static inline void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event) {}
> > > #endif
> > >
> > > #endif /* __KVM_X86_VMX_X86_OPS_H */
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 1fb135e0c98f..e8bc66031a1d 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -492,6 +492,7 @@ int kvm_set_apic_base(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > > kvm_recalculate_apic_map(vcpu->kvm);
> > > return 0;
> > > }
> > > +EXPORT_SYMBOL_GPL(kvm_set_apic_base);
> > >
> > > /*
> > > * Handle a fault on a hardware virtualization (VMX or SVM) instruction.
> > > @@ -12109,6 +12110,7 @@ bool kvm_vcpu_is_reset_bsp(struct kvm_vcpu *vcpu)
> > > {
> > > return vcpu->kvm->arch.bsp_vcpu_id == vcpu->vcpu_id;
> > > }
> > > +EXPORT_SYMBOL_GPL(kvm_vcpu_is_reset_bsp);
> > >
> >
> > The symbols don't need to be exported with the changes mentioned above.
> >
> > > bool kvm_vcpu_is_bsp(struct kvm_vcpu *vcpu)
> > > {
> >
>
Powered by blists - more mailing lists