Re: [kvm-unit-tests RFC 14/16] x86 AMD SEV-ES: Copy UEFI #VC IDT entry

[Marc Orr <marcorr@google.com>]

On Fri, Aug 20, 2021 at 4:50 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Aug 18, 2021, Zixuan Wang wrote:
> > AMD SEV-ES introduces a new #VC exception that handles the communication
> > between guest and host.  UEFI already implements a #VC handler so there
> > is no need to re-implement it in KVM-Unit-Tests. To reuse this #VC
> > handler, this commit reads UEFI's IDT, copy the #VC IDT entry into
> > KVM-Unit-Tests' IDT.
> >
> > In this commit, load_idt() can work and now guest crashes in
> > setup_page_table(), which will be fixed by follow-up commits.
>
> As a stop gap to get SEV testing of any kind enabled, I think piggybacking the
> vBIOS #VC handler is a great idea.  But long term, kvm-unit-tests absolutely
> needs to have its own #VC handler.
>
> In addition to the downsides Joerg pointed out[*], relying on an external #VC
> introduces the possibility of test failures that are tied to the vBIOS being
> used.  Such dependencies already exist to some extent, e.g. using a buggy QEMU or
> SeaBIOS could certainly introduce failures, but those components are far more
> mature and less likely to break in weird ways unique to a specific test.
>
> Another potential issue is that it's possible vBIOS will be enlightened to the
> point where it _never_ expects a #VC, e.g. does #VMGEXIT directly, and thus panics
> on any #VC instead of requesting the necessary emulation.
>
> Fixing the vBIOS image in the repo would mostly solve those problems, but it
> wouldn't solve the lack of flexibility for the #VC handler, and debugging a third
> party #VC handler would likely be far more difficult to debug when failures
> inevitably occur.
>
> So, if these shenanigans give us test coverage now instead of a few months from
> now, than I say go for it.  But, we need clear line of sight to a "native" #VC
> handler, confidence that it will actually get written in a timely manner, and an
> easily reverted set of commits to unwind all of the UEFI stuff.
>
> [*] https://lkml.kernel.org/r/YRuURERGp8CQ1jAX@suse.de

Understood. I must admit, we didn’t have this long term perspective
when drafting these patches. But after reading this feedback, we see
your point. Luckily, unwinding the code to install the UEFI #VC
handler is trivial.

Also, we do believe that completing and submitting this patch series
such that it uses the UEFI’s #VC handler is the best next step, even
with the understanding that it’s not where we want to be six months to
one year from now. The reason is that adding a new #VC handler is
non-trivial. It seems like a separate patch set. At the same time,
using the UEFI #VC handler unlocks a lot of testing (that’s totally
non-existent now) and it should be trivial to plumb in the (to be
written) kvm-unit-tests #VC handler. In other words, with this patch
the community can start using and adding to the tests that are
unlocked by the UEFI #VC handler while someone (in parallel) works on
a follow-on patch set to add a #VC handler to kvm-unit-tests.