All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marc Orr <marcorr@google.com>
To: Joerg Roedel <jroedel@suse.de>
Cc: Varad Gautam <varad.gautam@suse.com>,
	kvm list <kvm@vger.kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Andrew Jones <drjones@redhat.com>,
	Zixuan Wang <zxwang42@gmail.com>,
	Erdem Aktas <erdemaktas@google.com>,
	David Rientjes <rientjes@google.com>,
	Sean Christopherson <seanjc@google.com>,
	"Singh, Brijesh" <brijesh.singh@amd.com>,
	"Lendacky, Thomas" <Thomas.Lendacky@amd.com>,
	bp@suse.de
Subject: Re: [kvm-unit-tests 02/13] x86: AMD SEV-ES: Setup #VC exception handler for AMD SEV-ES
Date: Fri, 4 Feb 2022 07:57:39 -0800	[thread overview]
Message-ID: <CAA03e5HnyqZqDOyK8cbJgq_-zMPYEcrAuKr_CF8+=3DeykfV5A@mail.gmail.com> (raw)
In-Reply-To: <Yf0GO8EydyQSdZvu@suse.de>

On Fri, Feb 4, 2022 at 2:55 AM Joerg Roedel <jroedel@suse.de> 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@suse.de
>
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5
> 90409 Nürnberg
> Germany
>
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Ivo Totev
>

  reply	other threads:[~2022-02-04 15:57 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-20 12:51 [kvm-unit-tests 00/13] Add #VC exception handling for AMD SEV-ES Varad Gautam
2022-01-20 12:51 ` [kvm-unit-tests 01/13] x86/efi: Allow specifying AMD SEV/SEV-ES guest launch policy Varad Gautam
2022-01-20 16:18   ` Tom Lendacky
2022-01-30 20:04   ` Marc Orr
2022-01-20 12:51 ` [kvm-unit-tests 02/13] x86: AMD SEV-ES: Setup #VC exception handler for AMD SEV-ES Varad Gautam
2022-01-30 20:36   ` Marc Orr
2022-02-04 10:55     ` Joerg Roedel
2022-02-04 15:57       ` Marc Orr [this message]
2022-02-04 16:30         ` Sean Christopherson
2022-02-04 20:09           ` Marc Orr
2022-02-07 21:11             ` Sean Christopherson
2022-02-08  1:58               ` Marc Orr
2022-02-04 17:15         ` Joerg Roedel
2022-02-04 20:12           ` Marc Orr
2022-01-20 12:51 ` [kvm-unit-tests 03/13] x86: Move svm.h to lib/x86/ Varad Gautam
2022-01-20 12:51 ` [kvm-unit-tests 04/13] lib: x86: Import insn decoder from Linux Varad Gautam
2022-01-20 12:51 ` [kvm-unit-tests 05/13] x86: AMD SEV-ES: Pull related GHCB definitions and helpers " Varad Gautam
2022-01-20 12:51 ` [kvm-unit-tests 06/13] x86: AMD SEV-ES: Prepare for #VC processing Varad Gautam
2022-01-20 12:51 ` [kvm-unit-tests 07/13] x86: AMD SEV-ES: Handle WBINVD #VC Varad Gautam
2022-02-07 21:13   ` Sean Christopherson
2022-01-20 12:51 ` [kvm-unit-tests 08/13] lib/x86: Move xsave helpers to lib/ Varad Gautam
2022-01-20 12:51 ` [kvm-unit-tests 09/13] x86: AMD SEV-ES: Handle CPUID #VC Varad Gautam
2022-01-20 12:51 ` [kvm-unit-tests 10/13] x86: AMD SEV-ES: Handle RDTSC/RDTSCP #VC Varad Gautam
2022-02-07 21:17   ` Sean Christopherson
2022-01-20 12:51 ` [kvm-unit-tests 11/13] x86: AMD SEV-ES: Handle MSR #VC Varad Gautam
2022-01-20 12:51 ` [kvm-unit-tests 12/13] x86: AMD SEV-ES: Handle IOIO #VC Varad Gautam
2022-01-20 12:51 ` [kvm-unit-tests 13/13] x86: AMD SEV-ES: Handle string IO for " Varad Gautam

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to='CAA03e5HnyqZqDOyK8cbJgq_-zMPYEcrAuKr_CF8+=3DeykfV5A@mail.gmail.com' \
    --to=marcorr@google.com \
    --cc=Thomas.Lendacky@amd.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=varad.gautam@suse.com \
    --cc=zxwang42@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.