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

[Binbin Wu]

On 1/9/2025 11:38 PM, Nikolay Borisov wrote:
> On 9.12.24 г. 3:07 ч., Binbin Wu wrote:
> > From: Sean Christopherson <seanjc@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
> > Signed-off-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>
> > ---
> > 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"?

By using this member, the code in the common path can be more generic,
instead of using is_td_vcpu(). I.e, in the future, if other VM types has
the same characteristic, no need to modify the common code.

> <snip>