Re: [PATCH v9 07/18] x86/apic: Add support to send IPI for Secure AVIC

["Upadhyay, Neeraj" <neeraj.upadhyay@amd.com>]

On 8/22/2025 10:44 PM, Borislav Petkov wrote:
> On Thu, Aug 21, 2025 at 10:57:24AM +0530, Upadhyay, Neeraj wrote:
>> Is below better?
> 
> I was only reacting to that head-spinning, conglomerate of abbreviations "AVIC
> GHCB APIC MSR".
> 

Ah ok. I thought you were not happy with the commit message 
wording/structure.

>> x86/apic: Add support to send IPI for Secure AVIC
>>
>> Secure AVIC hardware only accelerates Self-IPI, i.e. on WRMSR to
>> APIC_SELF_IPI and APIC_ICR (with destination shorthand equal to Self)
>> registers, hardware takes care of updating the APIC_IRR in the APIC
>> backing page of the vCPU. For other IPI types (cross-vCPU, broadcast IPIs),
>> software needs to take care of updating the APIC_IRR state of the target
>> CPUs and to ensure that the target vCPUs notice the new pending interrupt.
>>
>> Add new callbacks in the Secure AVIC driver for sending IPI requests. These
>> callbacks update the IRR in the target guest vCPU's APIC backing page. To
>> ensure that the remote vCPU notices the new pending interrupt, reuse the
>> GHCB MSR handling code in vc_handle_msr() to issue APIC_ICR MSR-write GHCB
>> protocol event to the hypervisor. For Secure AVIC guests, on APIC_ICR write
>> MSR exits, the hypervisor notifies the target vCPU by either sending an AVIC
>> doorbell (if target vCPU is running) or by waking up the non-running target
>> vCPU.
> 
> But I'll take a definitely better commit message too! :-)
> 

Cool!

>> Ok moving it to x2apic_savic.c requires below 4 sev-internal declarations to
>> be moved to arch/x86/include/asm/sev.h
>>
>> struct ghcb_state;
>> struct ghcb *__sev_get_ghcb(struct ghcb_state *state);
>> void __sev_put_ghcb(struct ghcb_state *state);
>> enum es_result sev_es_ghcb_handle_msr(...);
> 
> Well, do you anticipate needing any more sev* facilities for SAVIC?
>

At this point I do not anticipate adding new functions for new SAVIC
features.

> If so, you probably should carve them out into arch/x86/coco/sev/savic.c
> 
> If only 4 functions, I guess they're probably still ok in .../sev/core.c
> 

Ok. I will keep them in sev/core.c for now and move to sev/savic.c if
anything new comes up in future.

>> This comment explains why WRMSR is sufficient for sending SELF_IPI. On
>> WRMSR by vCPU, Secure AVIC hardware takes care of updating APIC_IRR in
>> backing page. Hardware also ensures that new APIC_IRR state is evaluated
>> for new pending interrupts. So, WRMSR is hardware-accelerated.
>>
>> For non-self-IPI case, software need to do APIC_IRR update and sending of
>> wakeup-request/doorbell to the target vCPU.
> 
> Yeah, you need to rewrite it like the commit message above - it needs to say
> that upon the MSR write, hw does this and that and therefore accelerates this
> type of IPI.
> 
> Then it is clear what you mean by "acceleration."
> 

Got it. Will update. Thanks!

- Neeraj

> Thx.
>