kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Marc Orr <marcorr@google.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Zixuan Wang <zixuanwang@google.com>,
	kvm list <kvm@vger.kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Andrew Jones <drjones@redhat.com>,
	"Hyunwook (Wooky) Baek" <baekhw@google.com>,
	Tom Roeder <tmroeder@google.com>,
	Erdem Aktas <erdemaktas@google.com>,
	David Rientjes <rientjes@google.com>,
	"Singh, Brijesh" <brijesh.singh@amd.com>,
	"Lendacky, Thomas" <Thomas.Lendacky@amd.com>,
	Varad Gautam <varad.gautam@suse.com>,
	Joerg Roedel <jroedel@suse.de>,
Subject: Re: [kvm-unit-tests RFC 14/16] x86 AMD SEV-ES: Copy UEFI #VC IDT entry
Date: Fri, 20 Aug 2021 17:37:40 -0700	[thread overview]
Message-ID: <CAA03e5FRT7TgtDaV3mrTjaWF8njnFuRre0id8FFCDDdPbMeFPA@mail.gmail.com> (raw)
In-Reply-To: <YSA/sYhGgMU72tn+@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.

  reply	other threads:[~2021-08-21  0:37 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
2021-08-18  0:08 ` [kvm-unit-tests RFC 01/16] x86 UEFI: Copy code from GNU-EFI Zixuan Wang
2021-08-18  0:08 ` [kvm-unit-tests RFC 02/16] x86 UEFI: Boot from UEFI Zixuan Wang
2021-08-18  0:08 ` [kvm-unit-tests RFC 03/16] x86 UEFI: Move setjmp.h out of desc.h Zixuan Wang
2021-08-18  0:08 ` [kvm-unit-tests RFC 04/16] x86 UEFI: Load KVM-Unit-Tests IDT after UEFI boot up Zixuan Wang
2021-08-18  0:08 ` [kvm-unit-tests RFC 05/16] x86 UEFI: Load GDT and TSS " Zixuan Wang
2021-08-18  0:08 ` [kvm-unit-tests RFC 06/16] x86 UEFI: Set up memory allocator Zixuan Wang
2021-08-18  0:08 ` [kvm-unit-tests RFC 07/16] x86 UEFI: Set up RSDP after UEFI boot up Zixuan Wang
2021-08-18  0:08 ` [kvm-unit-tests RFC 08/16] x86 UEFI: Set up page tables Zixuan Wang
2021-08-18  0:08 ` [kvm-unit-tests RFC 09/16] x86 UEFI: Convert x86 test cases to PIC Zixuan Wang
2021-08-18  0:08 ` [kvm-unit-tests RFC 10/16] x86 AMD SEV: Initial support Zixuan Wang
2021-08-18  0:09 ` [kvm-unit-tests RFC 11/16] x86 AMD SEV: Page table with c-bit Zixuan Wang
2021-08-18  0:09 ` [kvm-unit-tests RFC 12/16] x86 AMD SEV-ES: Check SEV-ES status Zixuan Wang
2021-08-18  0:09 ` [kvm-unit-tests RFC 13/16] x86 AMD SEV-ES: Load GDT with UEFI segments Zixuan Wang
2021-08-18  0:09 ` [kvm-unit-tests RFC 14/16] x86 AMD SEV-ES: Copy UEFI #VC IDT entry Zixuan Wang
2021-08-20 23:50   ` Sean Christopherson
2021-08-21  0:37     ` Marc Orr [this message]
2021-08-21  0:47     ` Zixuan Wang
2021-08-18  0:09 ` [kvm-unit-tests RFC 15/16] x86 AMD SEV-ES: Set up GHCB page Zixuan Wang
2021-08-18  0:09 ` [kvm-unit-tests RFC 16/16] x86 AMD SEV-ES: Add test cases Zixuan Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAA03e5FRT7TgtDaV3mrTjaWF8njnFuRre0id8FFCDDdPbMeFPA@mail.gmail.com \
    --to=marcorr@google.com \
    --cc=Thomas.Lendacky@amd.com \
    --cc=baekhw@google.com \
    --cc=bp@suse.de \
    --cc=brijesh.singh@amd.com \
    --cc=drjones@redhat.com \
    --cc=erdemaktas@google.com \
    --cc=jroedel@suse.de \
    --cc=kvm@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=rientjes@google.com \
    --cc=seanjc@google.com \
    --cc=tmroeder@google.com \
    --cc=varad.gautam@suse.com \
    --cc=zixuanwang@google.com \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).