lkml.org 
[lkml]   [2023]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v11 023/113] KVM: TDX: allocate/free TDX vcpu structure
    On Mon, Jan 16, 2023 at 12:46:06PM +0200,
    Zhi Wang <zhi.wang.linux@gmail.com> wrote:

    > On Thu, 12 Jan 2023 08:31:31 -0800
    > isaku.yamahata@intel.com wrote:
    >
    > > From: Isaku Yamahata <isaku.yamahata@intel.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@intel.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@intel.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.


    > 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)
    > > {
    >

    --
    Isaku Yamahata <isaku.yamahata@gmail.com>

    \
     
     \ /
      Last update: 2023-03-27 00:37    [W:4.284 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site