On Mon, 27 Feb 2023 15:49:14 -0800 Isaku Yamahata <isaku.yamahata@xxxxxxxxx> wrote: > On Mon, Jan 16, 2023 at 12:46:06PM +0200, > Zhi Wang <zhi.wang.linux@xxxxxxxxx> wrote: > > > On Thu, 12 Jan 2023 08:31:31 -0800 > > isaku.yamahata@xxxxxxxxx wrote: > > > > > From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx> > > > > > > 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@xxxxxxxxx> > > > --- > > > 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@xxxxxxxxx> > > > --- > > > 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) > > > { > > >