Re: [PATCH v4] KVM: x86: Add x2APIC "features" to control EOI broadcast suppression

[Khushit Shah]

> On 12 Dec 2025, at 12:29 AM, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> 
> A bunch of nits, but I'll fix them up when applying, assuming on one else has
> feedback.
> 
> On Thu, Dec 11, 2025, Khushit Shah wrote:
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 48598d017d6f..4a6d94dc7a2a 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -1229,6 +1229,12 @@ enum kvm_irqchip_mode {
>> KVM_IRQCHIP_SPLIT,        /* created with KVM_CAP_SPLIT_IRQCHIP */
>> };
>> 
>> +enum kvm_suppress_eoi_broadcast_mode {
>> + KVM_SUPPRESS_EOI_BROADCAST_QUIRKED, /* Legacy behavior */
>> + KVM_SUPPRESS_EOI_BROADCAST_ENABLED, /* Enable Suppress EOI broadcast */
>> + KVM_SUPPRESS_EOI_BROADCAST_DISABLED /* Disable Suppress EOI broadcast */
>> +};
>> +
>> struct kvm_x86_msr_filter {
>> u8 count;
>> bool default_allow:1;
>> @@ -1480,6 +1486,7 @@ struct kvm_arch {
>> 
>> bool x2apic_format;
>> bool x2apic_broadcast_quirk_disabled;
>> + enum kvm_suppress_eoi_broadcast_mode suppress_eoi_broadcast_mode;
> 
> For brevity, I vote for eoi_broadcast_mode here, i.e.:
> 
> enum kvm_suppress_eoi_broadcast_mode eoi_broadcast_mode;
> 
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 0ae7f913d782..1ef0bd3eff1e 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -105,6 +105,34 @@ bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector)
>> apic_test_vector(vector, apic->regs + APIC_IRR);
>> }
>> 
>> +static inline bool kvm_lapic_advertise_suppress_eoi_broadcast(struct kvm *kvm)
> 
> Formletter...
> 
> Do not use "inline" for functions that are visible only to the local compilation
> unit.  "inline" is just a hint, and modern compilers are smart enough to inline
> functions when appropriate without a hint.
> 
> A longer explanation/rant here: https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_all_ZAdfX-2BS323JVWNZC-40google.com&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=PGWMyignA0NiDmTlyP7vOTHozBws_VN86yrVmSMkBp0&m=1PkTHZB5A4wRs2jiasH5pBNS7SQ5m_NStqhLjdC0QPvPXhu0bw71ANp5LrOuh4Hi&s=PbeEM0u06z_0DEK26Y1gV17VlAsY7WX98Bv7xj6s9GU&e=
> 
>> +{
>> + /*
>> +  * Advertise Suppress EOI broadcast support to the guest unless the VMM
>> +  * explicitly disabled it.
>> +  *
>> +  * Historically, KVM advertised this capability even though it did not
>> +  * actually suppress EOIs.
>> +  */
>> + return kvm->arch.suppress_eoi_broadcast_mode !=
>> + KVM_SUPPRESS_EOI_BROADCAST_DISABLED;
> 
> With a shorter field name, this can more comfortably be:
> 
> return kvm->arch.eoi_broadcast_mode != KVM_SUPPRESS_EOI_BROADCAST_DISABLED;
> 
>> +}
>> +
>> +static inline bool kvm_lapic_ignore_suppress_eoi_broadcast(struct kvm *kvm)
>> +{
>> + /*
>> +  * Returns true if KVM should ignore the suppress EOI broadcast bit set by
>> +  * the guest and broadcast EOIs anyway.
>> +  *
>> +  * Only returns false when the VMM explicitly enabled Suppress EOI
>> +  * broadcast. If disabled by VMM, the bit should be ignored as it is not
>> +  * supported. Legacy behavior was to ignore the bit and broadcast EOIs
>> +  * anyway.
>> +  */
>> + return kvm->arch.suppress_eoi_broadcast_mode !=
>> + KVM_SUPPRESS_EOI_BROADCAST_ENABLED;
> 
> And then...
> 
> return kvm->arch.eoi_broadcast_mode != KVM_SUPPRESS_EOI_BROADCAST_ENABLED;
> 
>> +}
>> +
>> __read_mostly DEFINE_STATIC_KEY_FALSE(kvm_has_noapic_vcpu);
>> EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_has_noapic_vcpu);
>> 
>> @@ -562,6 +590,7 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu)
>>  * IOAPIC.
>>  */
>> if (guest_cpu_cap_has(vcpu, X86_FEATURE_X2APIC) &&
>> + kvm_lapic_advertise_suppress_eoi_broadcast(vcpu->kvm) &&
> 
> Align indentation.
> 
>>     !ioapic_in_kernel(vcpu->kvm))
>> v |= APIC_LVR_DIRECTED_EOI;
>> kvm_lapic_set_reg(apic, APIC_LVR, v);
>> @@ -1517,6 +1546,17 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
>> 
>> /* Request a KVM exit to inform the userspace IOAPIC. */
>> if (irqchip_split(apic->vcpu->kvm)) {
>> + /*
>> +  * Don't exit to userspace if the guest has enabled Directed
>> +  * EOI, a.k.a. Suppress EOI Broadcasts, in which case the local
>> +  * APIC doesn't broadcast EOIs (the guest must EOI the target
>> +  * I/O APIC(s) directly).  Ignore the suppression if userspace
>> +  * has NOT explicitly enabled Suppress EOI broadcast.
>> +  */
>> + if ((kvm_lapic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) &&
>> +      !kvm_lapic_ignore_suppress_eoi_broadcast(apic->vcpu->kvm))
>> + return;
>> +
>> apic->vcpu->arch.pending_ioapic_eoi = vector;
>> kvm_make_request(KVM_REQ_IOAPIC_EOI_EXIT, apic->vcpu);
>> return;
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index c9c2aa6f4705..81b40fdb5f5f 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -121,8 +121,11 @@ static u64 __read_mostly efer_reserved_bits = ~((u64)EFER_SCE);
>> 
>> #define KVM_CAP_PMU_VALID_MASK KVM_PMU_CAP_DISABLE
>> 
>> -#define KVM_X2APIC_API_VALID_FLAGS (KVM_X2APIC_API_USE_32BIT_IDS | \
>> -                                    KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK)
>> +#define KVM_X2APIC_API_VALID_FLAGS \
>> + (KVM_X2APIC_API_USE_32BIT_IDS | \
>> + KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK | \
>> + KVM_X2APIC_ENABLE_SUPPRESS_EOI_BROADCAST | \
>> + KVM_X2APIC_DISABLE_SUPPRESS_EOI_BROADCAST)
> 
> Unless someone feels strongly, I think I'd prefer to keep the existing style, e.g.
> 
> #define KVM_X2APIC_API_VALID_FLAGS (KVM_X2APIC_API_USE_32BIT_IDS | \
>     KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK | \
>     KVM_X2APIC_ENABLE_SUPPRESS_EOI_BROADCAST | \
>     KVM_X2APIC_DISABLE_SUPPRESS_EOI_BROADCAST)
> 
>> 
>> static void update_cr8_intercept(struct kvm_vcpu *vcpu);
>> static void process_nmi(struct kvm_vcpu *vcpu);
>> @@ -6777,12 +6780,22 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
>> r = -EINVAL;
>> if (cap->args[0] & ~KVM_X2APIC_API_VALID_FLAGS)
>> break;
>> + if ((cap->args[0] & KVM_X2APIC_ENABLE_SUPPRESS_EOI_BROADCAST) &&
>> +     (cap->args[0] & KVM_X2APIC_DISABLE_SUPPRESS_EOI_BROADCAST))
>> + break;
>> + if (!irqchip_split(kvm) &&
>> +     ((cap->args[0] & KVM_X2APIC_ENABLE_SUPPRESS_EOI_BROADCAST) ||
>> +      (cap->args[0] & KVM_X2APIC_DISABLE_SUPPRESS_EOI_BROADCAST)))
>> + break;
> 
> Again, unless someone feels strongly, I'd prefer to have some newlines here, i.e.
> 
> r = -EINVAL;
> if (cap->args[0] & ~KVM_X2APIC_API_VALID_FLAGS)
> break;
> 
> if ((cap->args[0] & KVM_X2APIC_ENABLE_SUPPRESS_EOI_BROADCAST) &&
>     (cap->args[0] & KVM_X2APIC_DISABLE_SUPPRESS_EOI_BROADCAST))
> break;
> 
> if (!irqchip_split(kvm) &&
>     ((cap->args[0] & KVM_X2APIC_ENABLE_SUPPRESS_EOI_BROADCAST) ||
>      (cap->args[0] & KVM_X2APIC_DISABLE_SUPPRESS_EOI_BROADCAST)))
> break;
> 
> if (cap->args[0] & KVM_X2APIC_API_USE_32BIT_IDS)

Seems like there will be v5, so I will incorporate these fixes.