| 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: <76cd219cadf3f5e06eb10b592de121ed0db056eb.camel@intel.com>
Date: Thu, 19 Jan 2023 00:45:09 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Yamahata, Isaku" <isaku.yamahata@...el.com>
CC: "pbonzini@...hat.com" <pbonzini@...hat.com>,
"Shahar, Sagi" <sagis@...gle.com>,
"Aktas, Erdem" <erdemaktas@...gle.com>,
"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
"dmatlack@...gle.com" <dmatlack@...gle.com>,
"Christopherson,, Sean" <seanjc@...gle.com>
Subject: Re: [PATCH v11 023/113] KVM: TDX: allocate/free TDX vcpu structure
On Thu, 2023-01-12 at 08: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.
>
Why partially initialize it? Shouldn't a better way be either: 1) not
initialize at all, or; 2) fully initialize?
Can you put more _why_ here?
> Allocate pages of TDX vcpu for the
> TDX module. Actual donation TDX vcpu pages to the TDX module is not done
> yet.
Also, can you explain _why_ it is not done here?
>
> 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.
Don't quite understand here. As you said CPUID entries are configured later in
KVM_SET_CPUID2, so what's the point of initializing CPUID to support x2apic
here?
Are you suggesting KVM_SET_CPUID2 will be somehow rejected for TDX guest, or
there will be special handling to make sure the CPUID initialized here won't be
overwritten later?
Please explain clearly here.
>
> 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);
> +}
> +
> 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);
You don't need to use kvmalloc_array() when only allocating one entry.
> + 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;
As mentioned above, why doing it here? Won't be this be overwritten later in
KVM_SET_CPUID2?
> +
> + /* TDX only supports x2APIC, which requires an in-kernel local APIC. */
> + if (!vcpu->arch.apic)
> + return -EINVAL;
If this is hit, what happens to the CPUID entry allocated above? It's
absolutely not clear here in this patch.
> +
> + 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;
Should we use KVM_BUG_ON()?
Again, it appears this depends on how KVM handles INIT, which is done in a later
patch far way:
[PATCH v11 102/113] KVM: TDX: Silently ignore INIT/SIPI
And there's no material explaining how it is handled in either changelog or
comment, so to me it's not reviewable.
> +
> + /* TDX rquires X2APIC. */
> + 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;
I think we have KVM_BUG_ON()?
TDX requires a lot more staff then just x2apic, why only x2apic is done here,
particularly in _this_ patch?
> +
> + /*
> + * Don't update mp_state to runnable because more initialization
> + * is needed by TDX_VCPU_INIT.
> + */
> + return;
> +
> +td_bugged:
> + vcpu->kvm->vm_bugged = true;
> +}
> +
>
[snip]
Powered by blists - more mailing lists