Re: [PATCH 2/7] x86/apic: Add an SVSM APIC driver

[Tom Lendacky]

On 8/20/26 22:19, Melody Wang wrote:
> Hi Tom,
> 
> On 8/20/26 8:43 AM, Tom Lendacky wrote:
>> Why not move svsm_get_caa() and svsm_perform_call_protocol() from
>> internal.h then and make them available? Or create a callable function
>> that lives in arch/x86/coco/sev/svsm.c that builds the svsm_call struct
>> and performs the SVSM call?
> 
> The current svsm_do_call() actually is a callable function living in arch/
> x86/coco/sev/svsm.c except it doesn't build the call struct inside. I feel
> this way it is more clear.
> 
> What would be the advantage of building the svsm call?

Isolation of the svsm_call struct usage. Similar to what was done for the
vTPM support (see snp_svsm_vtpm_send_command() and snp_svsm_vtpm_probe()).

> 
>> Isn't that all that is needed? If the attribute is set none of the other
>> injection methods can be used. If anything, you should terminate if
>> alternate injection is enabled and you are running at VMPL0, because
>> nothing can update the VMSA to set the injection/irq fields.
> 
> Ok, agreed.
> 
>> If the VMRUN fails because both are set, how can you possibly be running
>> in the guest with both set? So I see no need to check for Secure AVIC.
> 
> Do you mean there are already enough places to prevent this fault, so the
> guest does not need to enforce it too?
> 
> I feel it does not hurt to do it there too, in the probe function. That
> belongs there anyway.

Sure, it just seems unnecessary since Secure AVIC won't work if Alternate
Injection is enabled. So giving it priority is meaningless.

> 
>> What if the version of the SVSM that is running doesn't have the APIC
>> emulation protocol?
> 
> The Alternate Injection spec says:
> 
> "The APIC Protocol is supported only as long as Alternate Injection is
> enabled."
> 
> To me, this reads like the APIC protocol is always there when Alternate
> Injection is enabled so I can assume it's present in that case.
> 
> So I don't need to query it.
> 
> Or am I misreading the spec?

Sure, you can assume that. The worst that will happen is your call will
fail if the SVSM hasn't implemented the spec correctly.

> 
>>> needed to query the APIC protocol now. In the future, when the SVSM code
>>> changes with different set, we can adjust the guest code accordingly.
>>>
> 
>>>> The read and write are very similar. Can you have a common function that
>>>> takes a reg paramter, value parameter (that is input and output), and a
>>>> mode parameter (read/write) and then have small read and write functions?
>>>
>>> Yes, and Sashiko pointed that I need to prevent preemption for the caa
>>> call, I agree with it, but I think I should prevent interrupts here - I
>>> should do native_local_irq_save(), because there should not be any
>>> interrupts during a caa call as those things are not reentrant. Thoughts?
>>
>> svsm_perform_call_protocol() already disables interrupts.
> 
> The problem is in svsm_get_caa(), when the task is preempted and migrated
> to a different CPU after fetching the per-CPU caa. Since this is
> preemptible code, after migration it is already wrong.
> 
> The hypercall will execute on the new CPU but use the original CPU's caa.
> This was found by Sashiko's review, I think this is a problem, so I need
> to disable preemption until svsm_perform_call_protocol() is finished.

If that is the case, then the fix would be to move the setting of the CAA
into the svsm_perform_call_protocol() function and just make that function
external, which will eliminate the need for svsm_do_call().

You would be a pre-patch then for fixing all the callers of
svsm_perform_call_protocol().

Thanks,
Tom

>