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

[Binbin Wu <binbin.wu@linux.intel.com>]

On 1/9/2025 11:38 PM, Nikolay Borisov wrote:
>
>
> 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"?

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>