Re: [RFC PATCH v3 02/27] x86/apic: Drop savic_eoi() in favor of native_apic_msr_eoi() for Secure AVIC

[Tom Lendacky]

On 7/8/26 01:32, Naveen N Rao (AMD) wrote:
> Drop savic_eoi() in favor of using the native helper that writes to the
> APIC_EOI MSR. savic_eoi() was added mainly to be able to handle
> level-triggered interrupts. However, it relies on APIC_TMR indicating a
> vector to be level-triggered, but APIC_TMR can never have a bit set
> since it is only updated when the LAPIC accepts a level-triggered
> interrupt. In the case of a Secure AVIC SEV-SNP guest, all
> level-triggered interrupt sources are in the VMM (emulated IOAPIC
> primarily) and KVM accepts them on behalf of the guest resulting in the
> APIC_TMR in KVM APIC backing page having a bit set. This is never seen
> by the guest, which has its own private APIC backing page. As such, the
> savic_eoi() handler is dead code. Remove it.
> 
> Fixes: 43b6687ac877 ("x86/apic: Handle EOI writes for Secure AVIC guests")
> Signed-off-by: Naveen N Rao (AMD) <naveen@xxxxxxxxxx>

Another guest change which should be separate from this series.

Is this causing issues with this hypervisor support or is just code that
is never invoked? Do other hypervisors behave the same way and will
removing this break them?

Thanks,
Tom

> ---
>  arch/x86/kernel/apic/x2apic_savic.c | 31 +----------------------------
>  1 file changed, 1 insertion(+), 30 deletions(-)
> 
> diff --git a/arch/x86/kernel/apic/x2apic_savic.c b/arch/x86/kernel/apic/x2apic_savic.c
> index cb3b55225ab6..f2af149c6005 100644
> --- a/arch/x86/kernel/apic/x2apic_savic.c
> +++ b/arch/x86/kernel/apic/x2apic_savic.c
> @@ -304,35 +304,6 @@ static void savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
>  	update_vector(cpu, SAVIC_ALLOWED_IRR, vector, set);
>  }
>  
> -static void savic_eoi(void)
> -{
> -	unsigned int cpu;
> -	int vec;
> -
> -	cpu = raw_smp_processor_id();
> -	vec = apic_find_highest_vector(get_reg_bitmap(cpu, APIC_ISR));
> -	if (WARN_ONCE(vec == -1, "EOI write while no active interrupt in APIC_ISR"))
> -		return;
> -
> -	/* Is level-triggered interrupt? */
> -	if (apic_test_vector(vec, get_reg_bitmap(cpu, APIC_TMR))) {
> -		update_vector(cpu, APIC_ISR, vec, false);
> -		/*
> -		 * Propagate the EOI write to the hypervisor for level-triggered
> -		 * interrupts. Return to the guest from GHCB protocol event takes
> -		 * care of re-evaluating interrupt state.
> -		 */
> -		savic_ghcb_msr_write(APIC_EOI, 0);
> -	} else {
> -		/*
> -		 * Hardware clears APIC_ISR and re-evaluates the interrupt state
> -		 * to determine if there is any pending interrupt which can be
> -		 * delivered to CPU.
> -		 */
> -		native_apic_msr_eoi();
> -	}
> -}
> -
>  static void savic_teardown(void)
>  {
>  	/* Disable Secure AVIC */
> @@ -421,7 +392,7 @@ static struct apic apic_x2apic_savic __ro_after_init = {
>  
>  	.read				= savic_read,
>  	.write				= savic_write,
> -	.eoi				= savic_eoi,
> +	.eoi				= native_apic_msr_eoi,
>  	.icr_read			= native_x2apic_icr_read,
>  	.icr_write			= savic_icr_write,
>