linux-kernel - Re: [PATCH 11/16] KVM: TDX: Always block INIT/SIPI

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <473c1a20-11c8-4e4e-8ff1-e2e5c5d68332@intel.com>
Date: Wed, 8 Jan 2025 15:21:26 +0800
From: Xiaoyao Li <xiaoyao.li@...el.com>
To: Binbin Wu <binbin.wu@...ux.intel.com>, pbonzini@...hat.com,
 seanjc@...gle.com, kvm@...r.kernel.org
Cc: 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 12/9/2024 9:07 AM, Binbin Wu wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
> 
> Always block INIT and SIPI events for the TDX guest because the TDX module
> doesn't provide API for VMM to inject INIT IPI or SIPI.
> 
> TDX defines its own vCPU creation and initialization sequence including
> multiple seamcalls.  Also, it's only allowed during TD build time.
> 
> Given that TDX guest is para-virtualized to boot BSP/APs, normally there
> shouldn't be any INIT/SIPI event for TDX guest.  If any, three options to
> handle them:
> 1. Always block INIT/SIPI request.
> 2. (Silently) ignore INIT/SIPI request during delivery.
> 3. Return error to guest TDs somehow.
> 
> Choose option 1 for simplicity. Since INIT and SIPI are always blocked,
> INIT handling and the OP vcpu_deliver_sipi_vector() won't be called, no
> need to add new interface or helper function for INIT/SIPI delivery.
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> Co-developed-by: Binbin Wu <binbin.wu@...ux.intel.com>
> Signed-off-by: Binbin Wu <binbin.wu@...ux.intel.com>
> ---
> TDX interrupts breakout:
> - Renamed from "KVM: TDX: Silently ignore INIT/SIPI" to
>    "KVM: TDX: Always block INIT/SIPI".
> - Remove KVM_BUG_ON() in tdx_vcpu_reset(). (Rick)
> - Drop tdx_vcpu_reset() and move the comment to vt_vcpu_reset().
> - Remove unnecessary interface and helpers to delivery INIT/SIPI
>    because INIT/SIPI events are always blocked for TDX. (Binbin)
> - Update changelog.
> ---
>   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.

>   	if (is_td_vcpu(vcpu))
>   		return;
>   
> @@ -211,6 +216,18 @@ static void vt_enable_smi_window(struct kvm_vcpu *vcpu)
>   }
>   #endif
>   
> +static bool vt_apic_init_signal_blocked(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * INIT and SIPI are always blocked for TDX, i.e., INIT handling and
> +	 * the OP vcpu_deliver_sipi_vector() won't be called.
> +	 */
> +	if (is_td_vcpu(vcpu))
> +		return true;
> +
> +	return vmx_apic_init_signal_blocked(vcpu);
> +}
> +
>   static void vt_apicv_pre_state_restore(struct kvm_vcpu *vcpu)
>   {
>   	struct pi_desc *pi = vcpu_to_pi_desc(vcpu);
> @@ -565,7 +582,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
>   #endif
>   
>   	.check_emulate_instruction = vmx_check_emulate_instruction,
> -	.apic_init_signal_blocked = vmx_apic_init_signal_blocked,
> +	.apic_init_signal_blocked = vt_apic_init_signal_blocked,
>   	.migrate_timers = vmx_migrate_timers,
>   
>   	.msr_filter_changed = vmx_msr_filter_changed,