search

coconut-svsm / svsm

COCONUT-SVSM
MIT License
122 stars 43 forks source link

svsm: implement SVSM Alternate Injection protocol for APIC emulation #368

Closed msft-jlange closed 5 months ago

msft-jlange commented 6 months ago

This change includes an APIC emulator and all of the logic required to enable Alternate Injection, including the extensions to the GHCB protocol to accept interrupts from the hypervisor, as well as an SVSM protocol to enable guest interaction with APIC state.

This change does not include support for APIC timers nor INIT/SIPI. APIC timers will come in a future change. INIT/SIPI will likely not be implemented for SNP because processor startup is already controlled through the VCPU creation elements of the SVSM Core Protocol.

msft-jlange commented 6 months ago

In the alternate Injection spec, Bytes 64-97 (here should be a typo in spec as 95) is for VMPL! and 128-159 for VMPL2, 192-223 for VMPL3, so each of them should be 32 bytes, but HVExtIntInfo for each VMPL is defined as 64 bytes here, should it be 32 bytes?

I think you left this comment twice. See my response to your other comment.

MelodyHuibo commented 6 months ago

In the alternate Injection spec, Bytes 64-97 (here should be a typo in spec as 95) is for VMPL! and 128-159 for VMPL2, 192-223 for VMPL3, so each of them should be 32 bytes, but HVExtIntInfo for each VMPL is defined as 64 bytes here, should it be 32 bytes?

I think you left this comment twice. See my response to your other comment.

yes, sorry about that. I see your response, thank you for that

msft-jlange commented 5 months ago

Also, can we get some tests for LocalApic?

I've spent some time trying to get some tests built, but in practice it is going to be extremely challenging. The workhorse of LocalApic is LocalApic::present_interrupts(). But that function takes a PerCpuShared as a parameter, and it is not possible to construct a standalone PerCpuShared (its new function is not public); it can only be constructed as part of a PerCpu, which requires the static per-CPU virtual address map. In addition, the function LocalApic::consume_host_interrupts() - which is central to APIC emulation - expects to be able to call this_cpu(), which is not available in the test environment.

It might be possible to create some traits that abstract these away - such that the SVSM environment supplies trait implementations based on real logic and the test environment supplies trait implementations based on test logic - but this would require the introduction of several abstractions that get in the way both of readability and of performance. Perhaps someday we will decide that this is a worthwhile tradeoff, but in the first PR, I'm reluctant to bite that off.

It looks like there may be some support for building test code into the SVSM environment, as I see a test_in_svsm configuration in multiple places, but I am unable to find any documentation for how this can be used for development or testing purposes.

As much as I would like to see test code, I think it's going to have to wait for a future PR.

00xc commented 5 months ago

Damn, I submitted the review after the PR was merged. @msft-jlange could you address some of the comments to see if they'd warrant a follow-up PR?

msft-jlange commented 5 months ago

Damn, I submitted the review after the PR was merged. @msft-jlange could you address some of the comments to see if they'd warrant a follow-up PR?

These should be addressed in #386.