On Fri, Feb 4, 2022 at 2:55 AM Joerg Roedel <jroedel@xxxxxxx> wrote: > > Hi Marc, > > On Sun, Jan 30, 2022 at 12:36:48PM -0800, Marc Orr wrote: > > Please let me know if I'm mis-understanding this rationale or missing > > any reasons for why folks want a built-in #VC handler. > > There are a couple of reasons which come all come down to one goal: > Robustnes of the kvm-unit-tests. > > If kvm-unit-tests continue to use the firmware #VC handler after > ExitBootServices there needs to be a contract between the test > framework and the firmware about: > > 1) Page-table layout - The page table needs to map the firmware > and the shared GHCB used by the firmware. > > 2) The firmware is required to keep its #VC handler in the > current IDT for kvm-unit-tests to find it and copy the #VC > entry into its own IDT. Yeah. I think we already resolved these first two issues in the initial patch set. > 3) The firmware #VC handler might use state which is not > available anymore after ExitBootServices. Of all the issues listed, this one seems the most serious. > 4) If the firmware uses the kvm-unit-test GHCB after > ExitBootServices, it has the get the GHCB address from the > GHCB MSR, requiring an identity mapping. > Moreover it requires to keep the address of the GHCB in the > MSR at all times where a #VC could happen. This could be a > problem when we start to add SEV-ES specific tests to the > unit-tests, explcitily testing the MSR protocol. Ack. I'd think we could require tests to save/restore the GHCB MSR. > It is easy to violate this implicit protocol and breaking kvm-unit-tests > just by a new version of OVMF being used. I think that is not a very > robust approach and a separate #VC handler in the unit-test framework > makes sense even now. Thanks for the explanation! I hope we can keep the UEFI #VC handler working, because like I mentioned, I think this work can be used to test that code inside of UEFI. But I guess time will tell. Of all the points listed above, I think point #3 is the most concerning. The others seem like they can be managed. Nonetheless, based on this explanation plus prior mailing list discussions, it is clear that the preference is to make the built-in #VC handler the default. My only request to Varad is to update the cover letter/patch descriptions with a summary of this discussion. Also, it might be worth adding a comment in the configure script mentioning that the built-in #VC handler is the default due to robustness and future-proofing concerns. Regarding code review and testing, I can help with the following: - Compare the patches being pulled into kvm-unit-tests to what's in the Linux kernel and add my Reviewed-by tags if I don't see any meaningful discrepancies. - Test the entire series on Google's setup, which doesn't use QEMU and add my Tested-by tag accordingly. My previous Tested-by tags were on individual patches. I have not yet tested the entire series. Please let me know if this is useful. If not, I wouldn't spend the time :-). > Regards, > > -- > Jörg Rödel > jroedel@xxxxxxx > > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5 > 90409 Nürnberg > Germany > > (HRB 36809, AG Nürnberg) > Geschäftsführer: Ivo Totev >
