Re: [PATCH 01/16] KVM: TDX: Add support for find pending IRQ in a protected local APIC

[Nikolay Borisov <nik.borisov@suse.com>]

On 9.12.24 г. 3:07 ч., Binbin Wu wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> Add flag and hook to KVM's local APIC management to support determining
> whether or not a TDX guest as a pending IRQ.  For TDX vCPUs, the virtual
> APIC page is owned by the TDX module and cannot be accessed by KVM.  As a
> result, registers that are virtualized by the CPU, e.g. PPR, cannot be
> read or written by KVM.  To deliver interrupts for TDX guests, KVM must
> send an IRQ to the CPU on the posted interrupt notification vector.  And
> to determine if TDX vCPU has a pending interrupt, KVM must check if there
> is an outstanding notification.
> 
> Return "no interrupt" in kvm_apic_has_interrupt() if the guest APIC is
> protected to short-circuit the various other flows that try to pull an
> IRQ out of the vAPIC, the only valid operation is querying _if_ an IRQ is
> pending, KVM can't do anything based on _which_ IRQ is pending.
> 
> Intentionally omit sanity checks from other flows, e.g. PPR update, so as
> not to degrade non-TDX guests with unnecessary checks.  A well-behaved KVM
> and userspace will never reach those flows for TDX guests, but reaching
> them is not fatal if something does go awry.
> 
> Note, this doesn't handle interrupts that have been delivered to the vCPU
> but not yet recognized by the core, i.e. interrupts that are sitting in
> vmcs.GUEST_INTR_STATUS.  Querying that state requires a SEAMCALL and will
> be supported in a future patch.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
> TDX interrupts breakout:
>   - Dropped vt_protected_apic_has_interrupt() with KVM_BUG_ON(), wire in
>     tdx_protected_apic_has_interrupt() directly. (Rick)
>   - Add {} on else in vt_hardware_setup()
> ---
>   arch/x86/include/asm/kvm-x86-ops.h | 1 +
>   arch/x86/include/asm/kvm_host.h    | 1 +
>   arch/x86/kvm/irq.c                 | 3 +++
>   arch/x86/kvm/lapic.c               | 3 +++
>   arch/x86/kvm/lapic.h               | 2 ++
>   arch/x86/kvm/vmx/main.c            | 3 +++
>   arch/x86/kvm/vmx/tdx.c             | 6 ++++++
>   arch/x86/kvm/vmx/x86_ops.h         | 2 ++
>   8 files changed, 21 insertions(+)
> 
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index ec1b1b39c6b3..d5faaaee6ac0 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -114,6 +114,7 @@ KVM_X86_OP_OPTIONAL(pi_start_assignment)
>   KVM_X86_OP_OPTIONAL(apicv_pre_state_restore)
>   KVM_X86_OP_OPTIONAL(apicv_post_state_restore)
>   KVM_X86_OP_OPTIONAL_RET0(dy_apicv_has_pending_interrupt)
> +KVM_X86_OP_OPTIONAL(protected_apic_has_interrupt)
>   KVM_X86_OP_OPTIONAL(set_hv_timer)
>   KVM_X86_OP_OPTIONAL(cancel_hv_timer)
>   KVM_X86_OP(setup_mce)
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 37dc7edef1ca..32c7d58a5d68 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1811,6 +1811,7 @@ struct kvm_x86_ops {
>   	void (*apicv_pre_state_restore)(struct kvm_vcpu *vcpu);
>   	void (*apicv_post_state_restore)(struct kvm_vcpu *vcpu);
>   	bool (*dy_apicv_has_pending_interrupt)(struct kvm_vcpu *vcpu);
> +	bool (*protected_apic_has_interrupt)(struct kvm_vcpu *vcpu);
>   
>   	int (*set_hv_timer)(struct kvm_vcpu *vcpu, u64 guest_deadline_tsc,
>   			    bool *expired);
> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> index 63f66c51975a..f0644d0bbe11 100644
> --- a/arch/x86/kvm/irq.c
> +++ b/arch/x86/kvm/irq.c
> @@ -100,6 +100,9 @@ int kvm_cpu_has_interrupt(struct kvm_vcpu *v)
>   	if (kvm_cpu_has_extint(v))
>   		return 1;
>   
> +	if (lapic_in_kernel(v) && v->arch.apic->guest_apic_protected)
> +		return static_call(kvm_x86_protected_apic_has_interrupt)(v);
> +
>   	return kvm_apic_has_interrupt(v) != -1;	/* LAPIC */
>   }
>   EXPORT_SYMBOL_GPL(kvm_cpu_has_interrupt);
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 65412640cfc7..684777c2f0a4 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -2920,6 +2920,9 @@ int kvm_apic_has_interrupt(struct kvm_vcpu *vcpu)
>   	if (!kvm_apic_present(vcpu))
>   		return -1;
>   
> +	if (apic->guest_apic_protected)
> +		return -1;
> +
>   	__apic_update_ppr(apic, &ppr);
>   	return apic_has_interrupt_for_ppr(apic, ppr);
>   }
> diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h
> index 1b8ef9856422..82355faf8c0d 100644
> --- a/arch/x86/kvm/lapic.h
> +++ b/arch/x86/kvm/lapic.h
> @@ -65,6 +65,8 @@ struct kvm_lapic {
>   	bool sw_enabled;
>   	bool irr_pending;
>   	bool lvt0_in_nmi_mode;
> +	/* Select registers in the vAPIC cannot be read/written. */
> +	bool guest_apic_protected;

Can't this member be eliminated and instead  is_td_vcpu() used as it 
stands currently that member is simply a proxy value for "is this a tdx 
vcpu"?

<snip>