| Message ID | 20220120125122.4633-3-varad.gautam@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add #VC exception handling for AMD SEV-ES | expand |
On Thu, Jan 20, 2022 at 4:52 AM Varad Gautam <varad.gautam@suse.com> wrote: > > AMD SEV-ES defines a new guest exception that gets triggered on > some vmexits to allow the guest to control what state gets shared > with the host. kvm-unit-tests currently relies on UEFI to provide > this #VC exception handler. > > Switch the tests to install their own #VC handler on early bootup > to process these exits, just after GHCB has been mapped. > > If --amdsev-efi-vc is passed during ./configure, the tests will > continue using the UEFI #VC handler. > > Signed-off-by: Varad Gautam <varad.gautam@suse.com> > --- > Makefile | 3 +++ > configure | 16 ++++++++++++++++ > lib/x86/amd_sev.c | 3 ++- > lib/x86/amd_sev.h | 1 + > lib/x86/amd_sev_vc.c | 14 ++++++++++++++ > lib/x86/desc.c | 15 +++++++++++++++ > lib/x86/desc.h | 1 + > lib/x86/setup.c | 10 ++++++++++ > x86/Makefile.common | 1 + > 9 files changed, 63 insertions(+), 1 deletion(-) > create mode 100644 lib/x86/amd_sev_vc.c > > diff --git a/Makefile b/Makefile > index 4f4ad23..94a0162 100644 > --- a/Makefile > +++ b/Makefile > @@ -46,6 +46,9 @@ else > $(error Cannot build $(ARCH_NAME) tests as EFI apps) > endif > EFI_CFLAGS := -DTARGET_EFI > +ifeq ($(AMDSEV_EFI_VC),y) > +EFI_CFLAGS += -DAMDSEV_EFI_VC > +endif > # The following CFLAGS and LDFLAGS come from: > # - GNU-EFI/Makefile.defaults > # - GNU-EFI/apps/Makefile > diff --git a/configure b/configure > index 41372ef..c687d9f 100755 > --- a/configure > +++ b/configure > @@ -29,6 +29,7 @@ host_key_document= > page_size= > earlycon= > target_efi= > +amdsev_efi_vc= > > usage() { > cat <<-EOF > @@ -71,6 +72,8 @@ usage() { > Specify a PL011 compatible UART at address ADDR. Supported > register stride is 32 bit only. > --target-efi Boot and run from UEFI > + --amdsev-efi-vc Use UEFI-provided #VC handlers on AMD SEV/ES. Requires > + --target-efi. What's the rationale to make the built-in #VC handler default? Note: I'm not actually saying I have an opinion, one way or the other. But I think the commit description (and cover letter) should explain why we prefer what we're doing. > EOF > exit 1 > } > @@ -138,6 +141,9 @@ while [[ "$1" = -* ]]; do > --target-efi) > target_efi=y > ;; > + --amdsev-efi-vc) > + amdsev_efi_vc=y > + ;; > --help) > usage > ;; > @@ -197,8 +203,17 @@ elif [ "$processor" = "arm" ]; then > processor="cortex-a15" > fi > > +if [ "$amdsev_efi_vc" ] && [ "$arch" != "x86_64" ]; then > + echo "--amdsev-efi-vc requires arch x86_64." > + usage > +fi > + > if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then > testdir=x86 > + if [ "$amdsev_efi_vc" ] && [ -z "$target_efi" ]; then > + echo "--amdsev-efi-vc requires --target-efi." > + usage > + fi > elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then > testdir=arm > if [ "$target" = "qemu" ]; then > @@ -356,6 +371,7 @@ WA_DIVIDE=$wa_divide > GENPROTIMG=${GENPROTIMG-genprotimg} > HOST_KEY_DOCUMENT=$host_key_document > TARGET_EFI=$target_efi > +AMDSEV_EFI_VC=$amdsev_efi_vc > EOF > if [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then > echo "TARGET=$target" >> config.mak > diff --git a/lib/x86/amd_sev.c b/lib/x86/amd_sev.c > index 6672214..bde126b 100644 > --- a/lib/x86/amd_sev.c > +++ b/lib/x86/amd_sev.c > @@ -14,6 +14,7 @@ > #include "x86/vm.h" > > static unsigned short amd_sev_c_bit_pos; > +phys_addr_t ghcb_addr; > > bool amd_sev_enabled(void) > { > @@ -126,7 +127,7 @@ void setup_ghcb_pte(pgd_t *page_table) > * function searches GHCB's L1 pte, creates corresponding L1 ptes if not > * found, and unsets the c-bit of GHCB's L1 pte. > */ > - phys_addr_t ghcb_addr, ghcb_base_addr; > + phys_addr_t ghcb_base_addr; > pteval_t *pte; > > /* Read the current GHCB page addr */ > diff --git a/lib/x86/amd_sev.h b/lib/x86/amd_sev.h > index 6a10f84..afbacf3 100644 > --- a/lib/x86/amd_sev.h > +++ b/lib/x86/amd_sev.h > @@ -54,6 +54,7 @@ efi_status_t setup_amd_sev(void); > bool amd_sev_es_enabled(void); > efi_status_t setup_amd_sev_es(void); > void setup_ghcb_pte(pgd_t *page_table); > +void handle_sev_es_vc(struct ex_regs *regs); > > unsigned long long get_amd_sev_c_bit_mask(void); > unsigned long long get_amd_sev_addr_upperbound(void); > diff --git a/lib/x86/amd_sev_vc.c b/lib/x86/amd_sev_vc.c > new file mode 100644 > index 0000000..8226121 > --- /dev/null > +++ b/lib/x86/amd_sev_vc.c > @@ -0,0 +1,14 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > + > +#include "amd_sev.h" > + > +extern phys_addr_t ghcb_addr; > + > +void handle_sev_es_vc(struct ex_regs *regs) > +{ > + struct ghcb *ghcb = (struct ghcb *) ghcb_addr; > + if (!ghcb) { > + /* TODO: kill guest */ > + return; > + } > +} > diff --git a/lib/x86/desc.c b/lib/x86/desc.c > index 16b7256..73aa866 100644 > --- a/lib/x86/desc.c > +++ b/lib/x86/desc.c > @@ -3,6 +3,9 @@ > #include "processor.h" > #include <setjmp.h> > #include "apic-defs.h" > +#ifdef TARGET_EFI > +#include "amd_sev.h" > +#endif > > /* Boot-related data structures */ > > @@ -228,6 +231,9 @@ EX_E(ac, 17); > EX(mc, 18); > EX(xm, 19); > EX_E(cp, 21); > +#ifdef TARGET_EFI > +EX_E(vc, 29); > +#endif > > asm (".pushsection .text \n\t" > "__handle_exception: \n\t" > @@ -293,6 +299,15 @@ void setup_idt(void) > handle_exception(13, check_exception_table); > } > > +void setup_amd_sev_es_vc(void) > +{ > + if (!amd_sev_es_enabled()) > + return; > + > + set_idt_entry(29, &vc_fault, 0); > + handle_exception(29, handle_sev_es_vc); > +} > + > unsigned exception_vector(void) > { > unsigned char vector; > diff --git a/lib/x86/desc.h b/lib/x86/desc.h > index b65539e..4fcbf9f 100644 > --- a/lib/x86/desc.h > +++ b/lib/x86/desc.h > @@ -220,6 +220,7 @@ void set_intr_alt_stack(int e, void *fn); > void print_current_tss_info(void); > handler handle_exception(u8 v, handler fn); > void unhandled_exception(struct ex_regs *regs, bool cpu); > +void setup_amd_sev_es_vc(void); > > bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data), > void *data); > diff --git a/lib/x86/setup.c b/lib/x86/setup.c > index bbd3468..6013602 100644 > --- a/lib/x86/setup.c > +++ b/lib/x86/setup.c > @@ -327,6 +327,16 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo) > smp_init(); > setup_page_table(); > > +#ifndef AMDSEV_EFI_VC > + if (amd_sev_es_enabled()) { This is a tautology. `setup_amd_sev_es_vc()` returns immediately upon `!amd_sev_es_enabled()`. Therefore, I think we should remove the check here. > + /* > + * Switch away from the UEFI-installed #VC handler. > + * GHCB has already been mapped at this point. > + */ > + setup_amd_sev_es_vc(); > + } > +#endif /* AMDSEV_EFI_VC */ > + > return EFI_SUCCESS; > } > > diff --git a/x86/Makefile.common b/x86/Makefile.common > index 984444e..65d16e7 100644 > --- a/x86/Makefile.common > +++ b/x86/Makefile.common > @@ -24,6 +24,7 @@ cflatobjs += lib/x86/fault_test.o > cflatobjs += lib/x86/delay.o > ifeq ($(TARGET_EFI),y) > cflatobjs += lib/x86/amd_sev.o > +cflatobjs += lib/x86/amd_sev_vc.o > cflatobjs += lib/efi.o > cflatobjs += x86/efi/reloc_x86_64.o > endif > -- > 2.32.0 > Thank you for adding this patch to the series! I left a comment above asking about the default #VC handler, above. And I'd like to expand on that question with my thoughts here. As far as I understand, mostly from offline conversations with Sean, a big motivation for the built-in #VC handler is that in the (very) long term, people will build a guest firmware without a #VC handler. In other words, eventually (I assume this is many years down the road), the guest firmware will be enlightened enough to avoid NAEs altogether, and instead explicitly call VMGEXIT explicitly, rather than implicitly after triggering a #VC exception. To me, this all sounds very far (e.g., 5-10 years) out. And thus, I still question the value in adding the built-in #VC handler with this as the justification now. However, it's clear from prior discussions that this is the direction preferred by the community. So I'm not pushing back. I'm just making sure we're all on the same page here. Please let me know if I'm mis-understanding this rationale or missing any reasons for why folks want a built-in #VC handler. And again, I think the cover letter should be updated with all the reasons why we're doing what we're doing. Where I think this work could be useful in the shorter term is testing. We have three pieces of code to test: 1. Host-side VMGEXIT handler. 2. Guest-side firmware #VC handler. 3. Guest-side kernel #VC handler. Technically, I guess there are multiple #VC handlers within each guest-side component, depending on the boot stage. But let's consider the steady state handlers that operate after initial guest-side boot stages for now. Using the UEFI's builtin #VC handler tests: - Guest-side firmware #VC handler + host-side VMGEXIT handler. Notably, it does _not_ test the guest-side kernel #VC handler. Looking ahead in the series, I can see that the code for the new builtin kvm-unit-tests #VC handler is basically copied from the Linux kernel #VC handler. Therefore, we could use this work to test: - Guest-side kernel #VC handler + host-side VMGEXIT handler. The difficulty with using this approach to test the guest-side #VC handler is that we now have two copies of the code that can get out of sync: one in the kernel and a 2nd in kvm-unit-tests. However, maybe this is OK. And maybe we can even try to ameliorate this issue with some scripts to compare the two codes or some policy that when we update the Linux kernel proper we send a follow-up patch to kvm-unit-tests. Another approach to test the guest-side kernel #VC handler would be to add tests to run from within a Linux guest. E.g., maybe Linux selftests could work here. We actually have something like this internally, But the way it was written would make it non-trivial to upstream. Thoughts? Tested-by: Marc Orr <marcorr@google.com>
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. 3) The firmware #VC handler might use state which is not available anymore after ExitBootServices. 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. 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. Regards,
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 >
On Fri, Feb 04, 2022, Marc Orr wrote: > On Fri, Feb 4, 2022 at 2:55 AM Joerg Roedel <jroedel@suse.de> wrote: > > 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. 5) Debug. I don't want to have to reverse engineer assembly code to understand why a #VC handler isn't doing what I expect, or to a debug the exchanges between guest and host. On Thu, Jan 20, 2022 at 4:52 AM Varad Gautam <varad.gautam@suse.com> wrote: > If --amdsev-efi-vc is passed during ./configure, the tests will > continue using the UEFI #VC handler. Why bother? I would prefer we ditch the UEFI #VC handler entirely and not give users the option to using anything but the built-in handler. What do we gain other than complexity?
Hi Marc, On Fri, Feb 04, 2022 at 07:57:39AM -0800, Marc Orr wrote: > 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 :-). I think it is definitly useful to run this in Googles environment too to get it tested and possible bugs ruled out. That can only help the upstream integration of these patches :) Varad discussed an idea with me today where the core VC handling code could be put into a library and used by the kernel and unit-tests and possibly others as well. The has the benefit that the kvm-unit-tests would also test the kernels VC handler paths. But it is probably something for the future. Regards,
On Fri, Feb 4, 2022 at 8:30 AM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Feb 04, 2022, Marc Orr wrote: > > On Fri, Feb 4, 2022 at 2:55 AM Joerg Roedel <jroedel@suse.de> wrote: > > > 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. > > 5) Debug. I don't want to have to reverse engineer assembly code to understand > why a #VC handler isn't doing what I expect, or to a debug the exchanges > between guest and host. Ack. But this can also be viewed as a benefit. Because the bug is probably something that should be followed up and fixed inside of UEFI. And that's my end goal. Can we reuse this work to test the #VC handler in the UEFI? This shouldn't be onerous. Because the #VC should follow the APM and GHCB specs. And both UEFI and kvm-unit-tests #VC handlers should be coded to those specs. If they're not, then one of them has a bug. > On Thu, Jan 20, 2022 at 4:52 AM Varad Gautam <varad.gautam@suse.com> wrote: > > If --amdsev-efi-vc is passed during ./configure, the tests will > > continue using the UEFI #VC handler. > > Why bother? I would prefer we ditch the UEFI #VC handler entirely and not give > users the option to using anything but the built-in handler. What do we gain > other than complexity? See above. If we keep the ability to run off the UEFI #VC handler then we can get continuous testing running inside of Google to verify the UEFI used to launch every SEV VM on Google cloud.
On Fri, Feb 4, 2022 at 9:15 AM Joerg Roedel <jroedel@suse.de> wrote: > > Hi Marc, > > On Fri, Feb 04, 2022 at 07:57:39AM -0800, Marc Orr wrote: > > 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 :-). > > I think it is definitly useful to run this in Googles environment too > to get it tested and possible bugs ruled out. That can only help the > upstream integration of these patches :) SGTM. Thank you for the feedback. > Varad discussed an idea with me today where the core VC handling code > could be put into a library and used by the kernel and unit-tests and > possibly others as well. The has the benefit that the kvm-unit-tests > would also test the kernels VC handler paths. But it is probably > something for the future. Yes, I like this idea a lot. This is basically what our internal test framework does. So if we can eventually convert this code to be a #VC as a library, that can be used from kvm-unit-tests, Linux kernel, UEFI, etc., that would be great.
On Fri, Feb 04, 2022, Marc Orr wrote: > On Fri, Feb 4, 2022 at 8:30 AM Sean Christopherson <seanjc@google.com> wrote: > > > > On Fri, Feb 04, 2022, Marc Orr wrote: > > > On Fri, Feb 4, 2022 at 2:55 AM Joerg Roedel <jroedel@suse.de> wrote: > > > > 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. > > > > 5) Debug. I don't want to have to reverse engineer assembly code to understand > > why a #VC handler isn't doing what I expect, or to a debug the exchanges > > between guest and host. > > Ack. But this can also be viewed as a benefit. Because the bug is > probably something that should be followed up and fixed inside of > UEFI. But how would we know it's a bug? E.g. IMO, UEFI should be enlightened to _never_ take a #VC, at which point its #VC handle can be changed to panic and using such a UEIF would cause KUT to fail. > And that's my end goal. Can we reuse this work to test the #VC handler > in the UEFI? > > This shouldn't be onerous. Because the #VC should follow the APM and > GHCB specs. And both UEFI and kvm-unit-tests #VC handlers should be > coded to those specs. If they're not, then one of them has a bug. > > > On Thu, Jan 20, 2022 at 4:52 AM Varad Gautam <varad.gautam@suse.com> wrote: > > > If --amdsev-efi-vc is passed during ./configure, the tests will > > > continue using the UEFI #VC handler. > > > > Why bother? I would prefer we ditch the UEFI #VC handler entirely and not give > > users the option to using anything but the built-in handler. What do we gain > > other than complexity? > > See above. If we keep the ability to run off the UEFI #VC handler then > we can get continuous testing running inside of Google to verify the > UEFI used to launch every SEV VM on Google cloud. I'm not super opposed to the idea, but I really do think that taking a #VC in guest UEFI is a bug in and of itself.
On Mon, Feb 7, 2022 at 1:11 PM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Feb 04, 2022, Marc Orr wrote: > > On Fri, Feb 4, 2022 at 8:30 AM Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Fri, Feb 04, 2022, Marc Orr wrote: > > > > On Fri, Feb 4, 2022 at 2:55 AM Joerg Roedel <jroedel@suse.de> wrote: > > > > > 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. > > > > > > 5) Debug. I don't want to have to reverse engineer assembly code to understand > > > why a #VC handler isn't doing what I expect, or to a debug the exchanges > > > between guest and host. > > > > Ack. But this can also be viewed as a benefit. Because the bug is > > probably something that should be followed up and fixed inside of > > UEFI. > > But how would we know it's a bug? E.g. IMO, UEFI should be enlightened to _never_ > take a #VC, at which point its #VC handle can be changed to panic and using such a > UEIF would cause KUT to fail. Yeah. If we end up with enlightened UEFIs that skip handling some/all #VC exceptions, then using the UEFI #VC handler from kvm-unit-tests doesn't make any sense :-). > > And that's my end goal. Can we reuse this work to test the #VC handler > > in the UEFI? > > > > This shouldn't be onerous. Because the #VC should follow the APM and > > GHCB specs. And both UEFI and kvm-unit-tests #VC handlers should be > > coded to those specs. If they're not, then one of them has a bug. > > > > > On Thu, Jan 20, 2022 at 4:52 AM Varad Gautam <varad.gautam@suse.com> wrote: > > > > If --amdsev-efi-vc is passed during ./configure, the tests will > > > > continue using the UEFI #VC handler. > > > > > > Why bother? I would prefer we ditch the UEFI #VC handler entirely and not give > > > users the option to using anything but the built-in handler. What do we gain > > > other than complexity? > > > > See above. If we keep the ability to run off the UEFI #VC handler then > > we can get continuous testing running inside of Google to verify the > > UEFI used to launch every SEV VM on Google cloud. > > I'm not super opposed to the idea, but I really do think that taking a #VC in > guest UEFI is a bug in and of itself. Understood.
diff --git a/Makefile b/Makefile index 4f4ad23..94a0162 100644 --- a/Makefile +++ b/Makefile @@ -46,6 +46,9 @@ else $(error Cannot build $(ARCH_NAME) tests as EFI apps) endif EFI_CFLAGS := -DTARGET_EFI +ifeq ($(AMDSEV_EFI_VC),y) +EFI_CFLAGS += -DAMDSEV_EFI_VC +endif # The following CFLAGS and LDFLAGS come from: # - GNU-EFI/Makefile.defaults # - GNU-EFI/apps/Makefile diff --git a/configure b/configure index 41372ef..c687d9f 100755 --- a/configure +++ b/configure @@ -29,6 +29,7 @@ host_key_document= page_size= earlycon= target_efi= +amdsev_efi_vc= usage() { cat <<-EOF @@ -71,6 +72,8 @@ usage() { Specify a PL011 compatible UART at address ADDR. Supported register stride is 32 bit only. --target-efi Boot and run from UEFI + --amdsev-efi-vc Use UEFI-provided #VC handlers on AMD SEV/ES. Requires + --target-efi. EOF exit 1 } @@ -138,6 +141,9 @@ while [[ "$1" = -* ]]; do --target-efi) target_efi=y ;; + --amdsev-efi-vc) + amdsev_efi_vc=y + ;; --help) usage ;; @@ -197,8 +203,17 @@ elif [ "$processor" = "arm" ]; then processor="cortex-a15" fi +if [ "$amdsev_efi_vc" ] && [ "$arch" != "x86_64" ]; then + echo "--amdsev-efi-vc requires arch x86_64." + usage +fi + if [ "$arch" = "i386" ] || [ "$arch" = "x86_64" ]; then testdir=x86 + if [ "$amdsev_efi_vc" ] && [ -z "$target_efi" ]; then + echo "--amdsev-efi-vc requires --target-efi." + usage + fi elif [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then testdir=arm if [ "$target" = "qemu" ]; then @@ -356,6 +371,7 @@ WA_DIVIDE=$wa_divide GENPROTIMG=${GENPROTIMG-genprotimg} HOST_KEY_DOCUMENT=$host_key_document TARGET_EFI=$target_efi +AMDSEV_EFI_VC=$amdsev_efi_vc EOF if [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then echo "TARGET=$target" >> config.mak diff --git a/lib/x86/amd_sev.c b/lib/x86/amd_sev.c index 6672214..bde126b 100644 --- a/lib/x86/amd_sev.c +++ b/lib/x86/amd_sev.c @@ -14,6 +14,7 @@ #include "x86/vm.h" static unsigned short amd_sev_c_bit_pos; +phys_addr_t ghcb_addr; bool amd_sev_enabled(void) { @@ -126,7 +127,7 @@ void setup_ghcb_pte(pgd_t *page_table) * function searches GHCB's L1 pte, creates corresponding L1 ptes if not * found, and unsets the c-bit of GHCB's L1 pte. */ - phys_addr_t ghcb_addr, ghcb_base_addr; + phys_addr_t ghcb_base_addr; pteval_t *pte; /* Read the current GHCB page addr */ diff --git a/lib/x86/amd_sev.h b/lib/x86/amd_sev.h index 6a10f84..afbacf3 100644 --- a/lib/x86/amd_sev.h +++ b/lib/x86/amd_sev.h @@ -54,6 +54,7 @@ efi_status_t setup_amd_sev(void); bool amd_sev_es_enabled(void); efi_status_t setup_amd_sev_es(void); void setup_ghcb_pte(pgd_t *page_table); +void handle_sev_es_vc(struct ex_regs *regs); unsigned long long get_amd_sev_c_bit_mask(void); unsigned long long get_amd_sev_addr_upperbound(void); diff --git a/lib/x86/amd_sev_vc.c b/lib/x86/amd_sev_vc.c new file mode 100644 index 0000000..8226121 --- /dev/null +++ b/lib/x86/amd_sev_vc.c @@ -0,0 +1,14 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#include "amd_sev.h" + +extern phys_addr_t ghcb_addr; + +void handle_sev_es_vc(struct ex_regs *regs) +{ + struct ghcb *ghcb = (struct ghcb *) ghcb_addr; + if (!ghcb) { + /* TODO: kill guest */ + return; + } +} diff --git a/lib/x86/desc.c b/lib/x86/desc.c index 16b7256..73aa866 100644 --- a/lib/x86/desc.c +++ b/lib/x86/desc.c @@ -3,6 +3,9 @@ #include "processor.h" #include <setjmp.h> #include "apic-defs.h" +#ifdef TARGET_EFI +#include "amd_sev.h" +#endif /* Boot-related data structures */ @@ -228,6 +231,9 @@ EX_E(ac, 17); EX(mc, 18); EX(xm, 19); EX_E(cp, 21); +#ifdef TARGET_EFI +EX_E(vc, 29); +#endif asm (".pushsection .text \n\t" "__handle_exception: \n\t" @@ -293,6 +299,15 @@ void setup_idt(void) handle_exception(13, check_exception_table); } +void setup_amd_sev_es_vc(void) +{ + if (!amd_sev_es_enabled()) + return; + + set_idt_entry(29, &vc_fault, 0); + handle_exception(29, handle_sev_es_vc); +} + unsigned exception_vector(void) { unsigned char vector; diff --git a/lib/x86/desc.h b/lib/x86/desc.h index b65539e..4fcbf9f 100644 --- a/lib/x86/desc.h +++ b/lib/x86/desc.h @@ -220,6 +220,7 @@ void set_intr_alt_stack(int e, void *fn); void print_current_tss_info(void); handler handle_exception(u8 v, handler fn); void unhandled_exception(struct ex_regs *regs, bool cpu); +void setup_amd_sev_es_vc(void); bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data), void *data); diff --git a/lib/x86/setup.c b/lib/x86/setup.c index bbd3468..6013602 100644 --- a/lib/x86/setup.c +++ b/lib/x86/setup.c @@ -327,6 +327,16 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo) smp_init(); setup_page_table(); +#ifndef AMDSEV_EFI_VC + if (amd_sev_es_enabled()) { + /* + * Switch away from the UEFI-installed #VC handler. + * GHCB has already been mapped at this point. + */ + setup_amd_sev_es_vc(); + } +#endif /* AMDSEV_EFI_VC */ + return EFI_SUCCESS; } diff --git a/x86/Makefile.common b/x86/Makefile.common index 984444e..65d16e7 100644 --- a/x86/Makefile.common +++ b/x86/Makefile.common @@ -24,6 +24,7 @@ cflatobjs += lib/x86/fault_test.o cflatobjs += lib/x86/delay.o ifeq ($(TARGET_EFI),y) cflatobjs += lib/x86/amd_sev.o +cflatobjs += lib/x86/amd_sev_vc.o cflatobjs += lib/efi.o cflatobjs += x86/efi/reloc_x86_64.o endif
AMD SEV-ES defines a new guest exception that gets triggered on some vmexits to allow the guest to control what state gets shared with the host. kvm-unit-tests currently relies on UEFI to provide this #VC exception handler. Switch the tests to install their own #VC handler on early bootup to process these exits, just after GHCB has been mapped. If --amdsev-efi-vc is passed during ./configure, the tests will continue using the UEFI #VC handler. Signed-off-by: Varad Gautam <varad.gautam@suse.com> --- Makefile | 3 +++ configure | 16 ++++++++++++++++ lib/x86/amd_sev.c | 3 ++- lib/x86/amd_sev.h | 1 + lib/x86/amd_sev_vc.c | 14 ++++++++++++++ lib/x86/desc.c | 15 +++++++++++++++ lib/x86/desc.h | 1 + lib/x86/setup.c | 10 ++++++++++ x86/Makefile.common | 1 + 9 files changed, 63 insertions(+), 1 deletion(-) create mode 100644 lib/x86/amd_sev_vc.c