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

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: <aTsUo9Fc6uu2A7rs@google.com>
Date: Thu, 11 Dec 2025 10:59:47 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Khushit Shah <khushit.shah@...anix.com>
Cc: pbonzini@...hat.com, kai.huang@...el.com, mingo@...hat.com, x86@...nel.org, 
	bp@...en8.de, hpa@...or.com, linux-kernel@...r.kernel.org, 
	kvm@...r.kernel.org, dave.hansen@...ux.intel.com, tglx@...utronix.de, 
	jon@...anix.com, shaju.abraham@...anix.com, dwmw2@...radead.org, 
	stable@...r.kernel.org
Subject: Re: [PATCH v4] KVM: x86: Add x2APIC "features" to control EOI
 broadcast suppression

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://lore.kernel.org/all/ZAdfX+S323JVWNZC@google.com

> +{
> +	/*
> +	 * 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)