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

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?

> 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.

> 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?

>> 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.

-- 
Thanks,
Melody