Re: [RFC PATCH v3 04/27] x86/apic: Use AVIC_INCOMPLETE_IPI VMGEXIT for Secure AVIC IPI handling

[Tom Lendacky <thomas.lendacky@amd.com>]

On 7/8/26 01:32, Naveen N Rao (AMD) wrote:
> x2apic_savic driver currently uses SVM_EXIT_MSR to have the hypervisor
> emulate ICR writes and to be able to deliver IPIs. However, the driver
> also sets APIC_IRR in the APIC backing page of the target vCPU, so the
> expectation is only for the hypervisor to notify/wake up the target
> vCPU.
> 
> This is incorrect since SVM_EXIT_MSR is for requesting full emulation of
> a certain MSR access -- and hypervisors expect to be able to _inject_
> (set APIC_IRR) *and* deliver the interrupt to the target. This can
> result in duplicate interrupts at the guest (assuming the guest is
> allowing that vector to be injected by the hypervisor).
> 
> Instead, have the driver do AVIC_INCOMPLETE_IPI exit so that it is clear
> on what the hypervisor needs to do. This is the same exit used by SVM
> AVIC when it has already set APIC_IRR in the target vCPU APIC backing
> page to request the hypervisor to send an AVIC doorbell/wake up the
> target vCPU. Add the newly added exit ID AVIC_IPI_FAILURE_UNACCELERATED
> (ID number 5) and use the same to signal that this is a Secure AVIC
> access that is not accelerated by hardware.

Except that the GHCB specification doesn't have this support, so you can't
really do this. And if you want it to be part of the GHCB spec it will
need to be a new feature bit at this point so that the guest knows that
the hypervisor can support this new exit code.

With the current guest code, can't the hypervisor support know that for
Secure AVIC it only needs to schedule the vCPU since it is assumed the
guest updated the target backing page?

Thanks,
Tom

> 
> Fixes: 2c6978ea1a85 ("x86/apic: Add support to send IPI for Secure AVIC")
> Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
> ---
>  arch/x86/include/asm/svm.h |  1 +
>  arch/x86/coco/sev/core.c   | 18 +++++++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index aa63431ba92c..42ececa8963d 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -284,6 +284,7 @@ enum avic_ipi_failure_cause {
>  	AVIC_IPI_FAILURE_INVALID_TARGET,
>  	AVIC_IPI_FAILURE_INVALID_BACKING_PAGE,
>  	AVIC_IPI_FAILURE_INVALID_IPI_VECTOR,
> +	AVIC_IPI_FAILURE_UNACCELERATED,
>  };
>  
>  #define AVIC_PHYSICAL_MAX_INDEX_MASK	GENMASK_ULL(11, 0)
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 7ed3da998489..69c75ea543a3 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -1014,13 +1014,29 @@ void savic_ghcb_msr_write(u32 reg, u64 value)
>  	struct ghcb_state state;
>  	enum es_result res;
>  	struct ghcb *ghcb;
> +	u64 exit_info_2;
>  
>  	guard(irqsave)();
>  
>  	ghcb = __sev_get_ghcb(&state);
>  	vc_ghcb_invalidate(ghcb);
>  
> -	res = __vc_handle_msr(ghcb, &ctxt, true);
> +	if (reg == APIC_ICR) {
> +		/*
> +		 * Exit with AVIC_INCOMPLETE_IPI to request hypervisor to notify
> +		 * the target vCPU(s). exit_info_1 is just the ICR value.
> +		 * exit_info_2 encodes exit id in the upper 32-bits, and icrh
> +		 * (IPI dest, since Secure AVIC is x2APIC-only) in the lower 32-bits.
> +		 */
> +		exit_info_2 = (u64)(AVIC_IPI_FAILURE_UNACCELERATED) << 32;
> +		exit_info_2 |= upper_32_bits(value);
> +
> +		res = sev_es_ghcb_hv_call(ghcb, &ctxt, SVM_EXIT_AVIC_INCOMPLETE_IPI,
> +					  value, exit_info_2);
> +	} else {
> +		res = __vc_handle_msr(ghcb, &ctxt, true);
> +	}
> +
>  	if (res != ES_OK) {
>  		pr_err("Secure AVIC MSR (0x%llx) write returned error (%d)\n", msr, res);
>  		/* MSR writes should never fail. Any failure is fatal error for SNP guest */