| 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: <8fccaab6-fda3-489c-866d-f0463ebbad65@linux.intel.com>
Date: Thu, 9 Jan 2025 10:26:36 +0800
From: Binbin Wu <binbin.wu@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Xiaoyao Li <xiaoyao.li@...el.com>, pbonzini@...hat.com,
kvm@...r.kernel.org, rick.p.edgecombe@...el.com, kai.huang@...el.com,
adrian.hunter@...el.com, reinette.chatre@...el.com,
tony.lindgren@...ux.intel.com, isaku.yamahata@...el.com,
yan.y.zhao@...el.com, chao.gao@...el.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 11/16] KVM: TDX: Always block INIT/SIPI
On 1/8/2025 10:40 PM, Sean Christopherson wrote:
> On Wed, Jan 08, 2025, Binbin Wu wrote:
>> On 1/8/2025 3:21 PM, Xiaoyao Li wrote:
>>> On 12/9/2024 9:07 AM, Binbin Wu wrote:
> ...
>
>>>> ---
>>>> arch/x86/kvm/lapic.c | 2 +-
>>>> arch/x86/kvm/vmx/main.c | 19 ++++++++++++++++++-
>>>> 2 files changed, 19 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>>>> index 474e0a7c1069..f93c382344ee 100644
>>>> --- a/arch/x86/kvm/lapic.c
>>>> +++ b/arch/x86/kvm/lapic.c
>>>> @@ -3365,7 +3365,7 @@ int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
>>>> if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
>>>> kvm_vcpu_reset(vcpu, true);
>>>> - if (kvm_vcpu_is_bsp(apic->vcpu))
>>>> + if (kvm_vcpu_is_bsp(vcpu))
>>>> vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
>>>> else
>>>> vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
>>>> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
>>>> index 8ec96646faec..7f933f821188 100644
>>>> --- a/arch/x86/kvm/vmx/main.c
>>>> +++ b/arch/x86/kvm/vmx/main.c
>>>> @@ -115,6 +115,11 @@ static void vt_vcpu_free(struct kvm_vcpu *vcpu)
>>>> static void vt_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>>>> {
>>>> + /*
>>>> + * TDX has its own sequence to do init during TD build time (by
>>>> + * KVM_TDX_INIT_VCPU) and it doesn't support INIT event during TD
>>>> + * runtime.
>>>> + */
>>> The first half is confusing. It seems to mix up init(ialization) with INIT
>>> event.
>>>
>>> And this callback is about *reset*, which can be due to INIT event or not.
>>> That's why it has a second parameter of init_event. The comment needs to
>>> clarify why reset is not needed for both cases.
>>>
>>> I think we can just say TDX doesn't support vcpu reset no matter due to
>>> INIT event or not.
> That's not entirely accurate either though. TDX does support KVM's version of
> RESET, because KVM's RESET is "power-on", i.e. vCPU creation. Emulation of
> runtime RESET is userspace's responsibility.
>
> The real reason why KVM doesn't do anything during KVM's RESET is that what
> little setup KVM does/can do needs to be defered until after guest CPUID is
> configured.
>
> KVM should also WARN if a TDX vCPU gets INIT, no?
There was a KVM_BUG_ON() if a TDX vCPU gets INIT in v19, and later it was
removed during the cleanup about removing WARN_ON_ONCE() and KVM_BUG_ON().
Since INIT/SIPI are always blocked for TDX guests, a delivery of INIT
event is a KVM bug and a WARN_ON_ONCE() is appropriate for this case.
>
> Side topic, the comment about x2APIC in tdx_vcpu_init() is too specific, e.g.
> calling out that x2APIC support is enumerated in CPUID.0x1.ECX isn't necessary,
> and stating that userspace must use KVM_SET_CPUID2 is flat out wrong. Very
> technically, KVM_SET_CPUID is also a valid option, it's just not used in practice
> because it doesn't support setting non-zero indices (but in theory it could be
> used to enable x2APIC).
Indeed, it is too specific.
>
> E.g. something like this?
LGTM.
Thanks for the suggestion!
>
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index d2e78e6675b9..e36fba94fa14 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -115,13 +115,10 @@ static void vt_vcpu_free(struct kvm_vcpu *vcpu)
>
> static void vt_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> {
> - /*
> - * TDX has its own sequence to do init during TD build time (by
> - * KVM_TDX_INIT_VCPU) and it doesn't support INIT event during TD
> - * runtime.
> - */
> - if (is_td_vcpu(vcpu))
> + if (is_td_vcpu(vcpu)) {
> + tdx_vcpu_reset(vcpu, init_event);
> return;
> + }
>
> vmx_vcpu_reset(vcpu, init_event);
> }
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 9e490fccf073..a587f59167a7 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -2806,9 +2806,8 @@ static int tdx_vcpu_init(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
> return -EINVAL;
>
> /*
> - * As TDX requires X2APIC, set local apic mode to X2APIC. User space
> - * VMM, e.g. qemu, is required to set CPUID[0x1].ecx.X2APIC=1 by
> - * KVM_SET_CPUID2. Otherwise kvm_apic_set_base() will fail.
> + * TDX requires x2APIC, userspace is responsible for configuring guest
> + * CPUID accordingly.
> */
> apic_base = APIC_DEFAULT_PHYS_BASE | LAPIC_MODE_X2APIC |
> (kvm_vcpu_is_reset_bsp(vcpu) ? MSR_IA32_APICBASE_BSP : 0);
> @@ -2827,6 +2826,19 @@ static int tdx_vcpu_init(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
> return 0;
> }
>
> +void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> +{
> + /*
> + * Yell on INIT, as TDX doesn't support INIT, i.e. KVM should drop all
> + * INIT events.
> + *
> + * Defer initializing vCPU for RESET state until KVM_TDX_INIT_VCPU, as
> + * userspace needs to define the vCPU model before KVM can initialize
> + * vCPU state, e.g. to enable x2APIC.
> + */
> + WARN_ON_ONCE(init_event);
> +}
> +
> struct tdx_gmem_post_populate_arg {
> struct kvm_vcpu *vcpu;
> __u32 flags;
>
Powered by blists - more mailing lists