kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support
@ 2021-08-18  0:08 Zixuan Wang
  2021-08-18  0:08 ` [kvm-unit-tests RFC 01/16] x86 UEFI: Copy code from GNU-EFI Zixuan Wang
                   ` (15 more replies)
  0 siblings, 16 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:08 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

Hello,

This patch series updates the x86_64 KVM-Unit-Tests to run under UEFI
and culminates in enabling AMD SEV/SEV-ES. We are sending it out for
early review as it provides basic support to run test cases in UEFI,
and also enables AMD SEV and SEV-ES features.

The patches are organized as two parts. The first part (patches 1-9)
enables the x86_64 test cases to run under UEFI. In particular, these
patches allow the x86_64 test cases to be built as EFI applications.
The efi_main() function sets up the KVM-Unit-Tests framework to run
under UEFI and then launches the test cases' main() function. To date,
we have 38/43 test cases running with UEFI using this approach.

The second part of the series (patches 10-16) focuses on SEV. In
particular, these patches introduce SEV/SEV-ES set up code into the EFI
set up process, including checking if SEV is supported, setting c-bits
for page table entries, and (notably) reusing the UEFI #VC handler so
that the set up process does not need to re-implement it (a test case
can always implement a new #VC handler and load it after set up is
finished). Using this approach, we are able to launch the x86_64 test
cases under SEV-ES and exercise KVM's VMGEXIT handler.

See the Part 1 and Part 2 summaries, below, for a high-level breakdown
of how the patches are organized.

Part 1 Summary:
Commits 1-3 introduce support to build test cases as EFI applications
(with the GNU-EFI library).

Commits 4-8 set up KVM-Unit-Tests to run under UEFI. In doing so, these
patches incrementally enable most existing x86_64 test cases to run
under UEFI.

Commit 9 fixes several test cases that fail to compile with GNU-EFI due
to UEFI's position independent code (PIC) requirement.

Part 2 Summary:
Commits 10-11 introduce support for SEV by adding new configuration
flags and set up code to set the SEV c-bit in page table entries.
SEV-related code is currently injected by configuration flags and C
macros, it is also possible to remove these flags and macros and
implement runtime SEV check and set up functions.

Commits 12-15 introduce support for SEV-ES by reusing the UEFI #VC
handler in KVM-Unit-Tests. They also fix GDT and IDT issues that occur
when reusing UEFI functions in KVM-Unit-Tests.

Commit 16 adds additional test cases for SEV-ES.

Regards,
Zixuan Wang

Zixuan Wang (16):
  x86 UEFI: Copy code from GNU-EFI
  x86 UEFI: Boot from UEFI
  x86 UEFI: Move setjmp.h out of desc.h
  x86 UEFI: Load KVM-Unit-Tests IDT after UEFI boot up
  x86 UEFI: Load GDT and TSS after UEFI boot up
  x86 UEFI: Set up memory allocator
  x86 UEFI: Set up RSDP after UEFI boot up
  x86 UEFI: Set up page tables
  x86 UEFI: Convert x86 test cases to PIC
  x86 AMD SEV: Initial support
  x86 AMD SEV: Page table with c-bit
  x86 AMD SEV-ES: Check SEV-ES status
  x86 AMD SEV-ES: Load GDT with UEFI segments
  x86 AMD SEV-ES: Copy UEFI #VC IDT entry
  x86 AMD SEV-ES: Set up GHCB page
  x86 AMD SEV-ES: Add test cases

 .gitignore                 |   3 +
 Makefile                   |  47 ++++++-
 README.md                  |   6 +
 configure                  |  29 +++++
 lib/efi.c                  |  60 +++++++++
 lib/string.c               |   3 +
 lib/x86/acpi.c             |  38 +++++-
 lib/x86/acpi.h             |   4 +
 lib/x86/amd_sev.c          | 147 +++++++++++++++++++++
 lib/x86/amd_sev.h          |  59 +++++++++
 lib/x86/asm/page.h         |  14 +-
 lib/x86/asm/setup.h        |  37 ++++++
 lib/x86/desc.c             |   4 +
 lib/x86/desc.h             |   5 -
 lib/x86/setup.c            | 259 +++++++++++++++++++++++++++++++++++++
 lib/x86/usermode.c         |   3 +-
 lib/x86/vm.c               |  18 ++-
 x86/Makefile.common        |  75 ++++++++---
 x86/Makefile.i386          |   5 +-
 x86/Makefile.x86_64        |  58 ++++++---
 x86/access.c               |   6 +-
 x86/amd_sev.c              |  97 ++++++++++++++
 x86/cet.c                  |   8 +-
 x86/efi/README.md          |  72 +++++++++++
 x86/efi/efistart64.S       | 141 ++++++++++++++++++++
 x86/efi/elf_x86_64_efi.lds |  81 ++++++++++++
 x86/efi/run                |  63 +++++++++
 x86/emulator.c             |   5 +-
 x86/eventinj.c             |   6 +-
 x86/run                    |  16 ++-
 x86/smap.c                 |   8 +-
 x86/umip.c                 |  10 +-
 x86/vmx.c                  |   1 +
 33 files changed, 1311 insertions(+), 77 deletions(-)
 create mode 100644 lib/efi.c
 create mode 100644 lib/x86/amd_sev.c
 create mode 100644 lib/x86/amd_sev.h
 create mode 100644 lib/x86/asm/setup.h
 create mode 100644 x86/amd_sev.c
 create mode 100644 x86/efi/README.md
 create mode 100644 x86/efi/efistart64.S
 create mode 100644 x86/efi/elf_x86_64_efi.lds
 create mode 100755 x86/efi/run

--
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 01/16] x86 UEFI: Copy code from GNU-EFI
  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 ` Zixuan Wang
  2021-08-18  0:08 ` [kvm-unit-tests RFC 02/16] x86 UEFI: Boot from UEFI Zixuan Wang
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:08 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

To build x86 test cases with UEFI, we need to borrow a linker script
from GNU-EFI. This commit only copies the source code, without any
modification. The linker script will be used by KVM-Unit-Tests in the
following commits in this patch series.

The following source code is copied from GNU-EFI:
   1. x86/efi/elf_x86_64_efi.lds

We will put EFI-specific files under a new dir `x86/efi` because:
   1. EFI-related code is easy to find
   2. EFI-related code is separated from the original code in `x86/`
   3. EFI-related code can still reuse the Makefile and test case code
      in its parent dir `x86/`

GNU-EFI repo and version:
   GIT URL: https://git.code.sf.net/p/gnu-efi/code
   Commit ID: 4fe83e102674
   Website: https://sourceforge.net/p/gnu-efi/code/ci/4fe83e/tree/

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 x86/efi/README.md          | 25 +++++++++++++
 x86/efi/elf_x86_64_efi.lds | 77 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)
 create mode 100644 x86/efi/README.md
 create mode 100644 x86/efi/elf_x86_64_efi.lds

diff --git a/x86/efi/README.md b/x86/efi/README.md
new file mode 100644
index 0000000..4deba6e
--- /dev/null
+++ b/x86/efi/README.md
@@ -0,0 +1,25 @@
+# EFI Startup Code and Linker Script
+
+This dir contains a linker script copied from
+[GNU-EFI](https://sourceforge.net/projects/gnu-efi/):
+   - elf_x86_64_efi.lds: linker script to build an EFI application
+
+The following pre-compiled object files ship with GNU-EFI library, and are used
+to build KVM-Unit-Tests with GNU-EFI:
+   - crt0-efi-x86_64.o: startup code of an EFI application
+   - libgnuefi.a: position independent x86_64 ELF shared object relocator
+
+EFI application binaries should be relocatable as UEFI loads binaries to dynamic
+runtime addresses. To build such relocatable binaries, GNU-EFI utilizes the
+above-mentioned files in its build process:
+
+   1. build an ELF shared object and link it using linker script
+      `elf_x86_64_efi.lds` to organize the sections in a way UEFI recognizes
+   2. link the shared object with self-relocator `libgnuefi.a` that applies
+      dynamic relocations that may be present in the shared object
+   3. link the entry point code `crt0-efi-x86_64.o` that invokes self-relocator
+      and then jumps to EFI application's `efi_main()` function
+   4. convert the shared object to an EFI binary
+
+More details can be found in `GNU-EFI/README.gnuefi`, section "Building
+Relocatable Binaries".
diff --git a/x86/efi/elf_x86_64_efi.lds b/x86/efi/elf_x86_64_efi.lds
new file mode 100644
index 0000000..5eae376
--- /dev/null
+++ b/x86/efi/elf_x86_64_efi.lds
@@ -0,0 +1,77 @@
+/* Copied from GNU-EFI/gnuefi/elf_x86_64_efi.lds, licensed under GNU GPL */
+/* Same as elf_x86_64_fbsd_efi.lds, except for OUTPUT_FORMAT below - KEEP IN SYNC */
+OUTPUT_FORMAT("elf64-x86-64", "elf64-x86-64", "elf64-x86-64")
+OUTPUT_ARCH(i386:x86-64)
+ENTRY(_start)
+SECTIONS
+{
+  . = 0;
+  ImageBase = .;
+  /* .hash and/or .gnu.hash MUST come first! */
+  .hash : { *(.hash) }
+  .gnu.hash : { *(.gnu.hash) }
+  . = ALIGN(4096);
+  .eh_frame : 
+  { 
+    *(.eh_frame)
+  }
+  . = ALIGN(4096);
+  .text :
+  {
+   _text = .;
+   *(.text)
+   *(.text.*)
+   *(.gnu.linkonce.t.*)
+   . = ALIGN(16);
+  }
+  _etext = .;
+  _text_size = . - _text;
+  . = ALIGN(4096);
+  .reloc :
+  {
+   *(.reloc)
+  }
+  . = ALIGN(4096);
+  .data :
+  {
+   _data = .;
+   *(.rodata*)
+   *(.got.plt)
+   *(.got)
+   *(.data*)
+   *(.sdata)
+   /* the EFI loader doesn't seem to like a .bss section, so we stick
+      it all into .data: */
+   *(.sbss)
+   *(.scommon)
+   *(.dynbss)
+   *(.bss)
+   *(COMMON)
+   *(.rel.local)
+  }
+  .note.gnu.build-id : { *(.note.gnu.build-id) }
+
+  _edata = .;
+  _data_size = . - _etext;
+  . = ALIGN(4096);
+  .dynamic  : { *(.dynamic) }
+  . = ALIGN(4096);
+  .rela :
+  {
+    *(.rela.data*)
+    *(.rela.got)
+    *(.rela.stab)
+  }
+  . = ALIGN(4096);
+  .dynsym   : { *(.dynsym) }
+  . = ALIGN(4096);
+  .dynstr   : { *(.dynstr) }
+  . = ALIGN(4096);
+  .ignored.reloc :
+  {
+    *(.rela.reloc)
+    *(.eh_frame)
+    *(.note.GNU-stack)
+  }
+  .comment 0 : { *(.comment) }
+}
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 02/16] x86 UEFI: Boot from UEFI
  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 ` Zixuan Wang
  2021-08-18  0:08 ` [kvm-unit-tests RFC 03/16] x86 UEFI: Move setjmp.h out of desc.h Zixuan Wang
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:08 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

This commit provides initial support for x86 test cases to boot from
UEFI:

   1. UEFI compiler flags are added to Makefile
   2. A new TARGET_EFI macro is added to turn on/off UEFI startup code
   3. Previous Multiboot setup code is refactored and updated for
      supporting UEFI, including the following changes:
      1. GNU-EFI/crt0-efi-x86_64.o: provides entry point and jumps to
         setup code in lib/efi.c. This file comes from GNU-EFI library
         package and is not included in this commit
      2. lib/efi.c: performs UEFI setup, calls arch-related setup
         functions, then jumps to test case main() function
      3. lib/x86/setup.c: provides arch-related setup under UEFI

To build test cases for UEFI, please first install the GNU-EFI library.
Check x86/efi/README.md for more details.

This commit is tested by a simple test calling report() and
report_summayr(). This commit does not include such a test to avoid
unnecessary files added into git history. To build and run this test in
UEFI (assuming file name is x86/dummy.c):

   ./configure --target-efi
   make x86/dummy.efi
   ./x86/efi/run ./x86/dummy.efi

To use the default Multiboot instead of UEFI:

   ./configure
   make x86/dummy.flat
   ./x86/run ./x86/dummy.flat

Some x86 test cases require additional fixes to work in UEFI, e.g.,
converting to position independent code (PIC), setting up page tables,
etc. This commit does not provide these fixes, so compiling and running
UEFI test cases other than x86/dummy.c may trigger compiler errors or
QEMU crashes.

The following code is ported from github.com/rhdrjones/kvm-unit-tests
   - ./configure: 'target-efi'-related code

See original code:
   - Repo: https://github.com/rhdrjones/kvm-unit-tests
   - Branch: target-efi

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 .gitignore          |  3 +++
 Makefile            | 47 +++++++++++++++++++++++++++++++--
 README.md           |  6 +++++
 configure           | 20 ++++++++++++++
 lib/efi.c           | 44 +++++++++++++++++++++++++++++++
 lib/string.c        |  3 +++
 lib/x86/asm/setup.h | 11 ++++++++
 lib/x86/setup.c     | 14 ++++++++++
 x86/Makefile.common | 64 +++++++++++++++++++++++++++++++++------------
 x86/Makefile.i386   |  5 ++--
 x86/Makefile.x86_64 | 54 ++++++++++++++++++++++++--------------
 x86/efi/README.md   | 49 +++++++++++++++++++++++++++++++++-
 x86/efi/run         | 63 ++++++++++++++++++++++++++++++++++++++++++++
 x86/run             | 16 ++++++++++--
 14 files changed, 357 insertions(+), 42 deletions(-)
 create mode 100644 lib/efi.c
 create mode 100644 lib/x86/asm/setup.h
 create mode 100755 x86/efi/run

diff --git a/.gitignore b/.gitignore
index b3cf2cb..dca6d29 100644
--- a/.gitignore
+++ b/.gitignore
@@ -3,7 +3,9 @@ tags
 *.a
 *.d
 *.o
+*.so
 *.flat
+*.efi
 *.elf
 .pc
 patches
@@ -24,3 +26,4 @@ cscope.*
 /api/dirty-log-perf
 /s390x/*.bin
 /s390x/snippets/*/*.gbin
+/efi-tests/*
diff --git a/Makefile b/Makefile
index f7b9f28..7084bac 100644
--- a/Makefile
+++ b/Makefile
@@ -38,6 +38,39 @@ LIBFDT_archive = $(LIBFDT_objdir)/libfdt.a
 
 OBJDIRS += $(LIBFDT_objdir)
 
+# EFI App
+ifeq ($(TARGET_EFI),y)
+ifeq ($(ARCH_NAME),x86_64)
+EFI_ARCH = x86_64
+else
+$(error Cannot build $(ARCH_NAME) tests as EFI apps)
+endif
+ifeq ($(EFI_INCLUDE_PATH),)
+EFI_INCLUDE_PATH:= /usr/include/efi
+endif
+ifeq ($(EFI_LIBS_PATH),)
+EFI_LIBS_PATH := /usr/lib/
+endif
+EFI_LIBS := -L $(EFI_LIBS_PATH) -lgnuefi -lefi
+EFI_CFLAGS := -DTARGET_EFI
+EFI_CFLAGS += -I $(EFI_INCLUDE_PATH) -I $(EFI_INCLUDE_PATH)/$(EFI_ARCH)
+# The following CFLAGS and LDFLAGS come from:
+#   - GNU-EFI/Makefile.defaults
+#   - GNU-EFI/apps/Makefile
+# Tell GNU-EFI to create an ABI that UEFI recognizes
+EFI_CFLAGS += -DGNU_EFI_USE_MS_ABI
+# Function calls must include the number of arguments passed to the functions
+# More details: https://wiki.osdev.org/GNU-EFI
+EFI_CFLAGS += -maccumulate-outgoing-args
+# GCC defines wchar to be 32 bits, but EFI expects 16 bits
+EFI_CFLAGS += -fshort-wchar
+# EFI applications use PIC as they are loaded to dynamic addresses, not a fixed
+# starting address
+EFI_CFLAGS += -fPIC
+# Create shared library
+EFI_LDFLAGS := -Bsymbolic -shared -nostdlib
+endif
+
 #include architecture specific make rules
 include $(SRCDIR)/$(TEST_DIR)/Makefile
 
@@ -62,14 +95,24 @@ COMMON_CFLAGS += $(fno_stack_protector)
 COMMON_CFLAGS += $(fno_stack_protector_all)
 COMMON_CFLAGS += $(wno_frame_address)
 COMMON_CFLAGS += $(if $(U32_LONG_FMT),-D__U32_LONG_FMT__,)
+ifeq ($(TARGET_EFI),y)
+COMMON_CFLAGS += $(EFI_CFLAGS)
+else
 COMMON_CFLAGS += $(fno_pic) $(no_pie)
+endif
 COMMON_CFLAGS += $(wclobbered)
 COMMON_CFLAGS += $(wunused_but_set_parameter)
 
 CFLAGS += $(COMMON_CFLAGS)
 CFLAGS += $(wmissing_parameter_type)
 CFLAGS += $(wold_style_declaration)
-CFLAGS += -Woverride-init -Wmissing-prototypes -Wstrict-prototypes
+CFLAGS += -Woverride-init
+CFLAGS += -Wmissing-prototypes
+ifeq ($(TARGET_EFI),y)
+# GNU-EFI library header does not pass the strict-prototype check
+else
+CFLAGS += -Wstrict-prototypes
+endif
 
 autodepend-flags = -MMD -MF $(dir $*).$(notdir $*).d
 
@@ -113,7 +156,7 @@ clean: arch_clean libfdt_clean
 
 distclean: clean
 	$(RM) lib/asm lib/config.h config.mak $(TEST_DIR)-run msr.out cscope.* build-head
-	$(RM) -r tests logs logs.old
+	$(RM) -r tests logs logs.old efi-tests
 
 cscope: cscope_dirs = lib lib/libfdt lib/linux $(TEST_DIR) $(ARCH_LIBDIRS) lib/asm-generic
 cscope:
diff --git a/README.md b/README.md
index b498aaf..6edacfe 100644
--- a/README.md
+++ b/README.md
@@ -17,6 +17,8 @@ in this directory.  Test images are created in ./ARCH/\*.flat
 
 NOTE: GCC cross-compiler is required for [build on macOS](README.macOS.md).
 
+To build with UEFI, check [build and run with UEFI](./x86/efi/README.md).
+
 ## Standalone tests
 
 The tests can be built as standalone.  To create and use standalone tests do:
@@ -54,6 +56,10 @@ ACCEL=name environment variable:
 
     ACCEL=kvm ./x86-run ./x86/msr.flat
 
+## Running the tests with UEFI
+
+Check [build and run with UEFI](./x86/efi/README.md).
+
 # Tests configuration file
 
 The test case may need specific runtime configurations, for
diff --git a/configure b/configure
index 1d4d855..3094375 100755
--- a/configure
+++ b/configure
@@ -28,6 +28,9 @@ erratatxt="$srcdir/errata.txt"
 host_key_document=
 page_size=
 earlycon=
+target_efi=
+efi_include_path=
+efi_libs_path=
 
 usage() {
     cat <<-EOF
@@ -69,6 +72,11 @@ usage() {
 	               pl011,mmio32,ADDR
 	                           Specify a PL011 compatible UART at address ADDR. Supported
 	                           register stride is 32 bit only.
+	    --target-efi           Boot and run from UEFI
+	    --efi-include-path     Path to GNU-EFI headers, e.g. "/usr/include/efi"
+	    --efi-libs-path        Path to GNU-EFI libraries, e.g. "/usr/lib/". This dir should
+	                           contain 3 files from GNU-EFI: crt0-efi-x86_64.o, libefi.a,
+	                           and libgnuefi.a
 EOF
     exit 1
 }
@@ -133,6 +141,15 @@ while [[ "$1" = -* ]]; do
 	--earlycon)
 	    earlycon="$arg"
 	    ;;
+	--target-efi)
+	    target_efi=y
+	    ;;
+	--efi-include-path)
+	    efi_include_path="$arg"
+	    ;;
+	--efi-libs-path)
+	    efi_libs_path="$arg"
+	    ;;
 	--help)
 	    usage
 	    ;;
@@ -341,6 +358,9 @@ U32_LONG_FMT=$u32_long
 WA_DIVIDE=$wa_divide
 GENPROTIMG=${GENPROTIMG-genprotimg}
 HOST_KEY_DOCUMENT=$host_key_document
+TARGET_EFI=$target_efi
+EFI_INCLUDE_PATH=$efi_include_path
+EFI_LIBS_PATH=$efi_libs_path
 EOF
 if [ "$arch" = "arm" ] || [ "$arch" = "arm64" ]; then
     echo "TARGET=$target" >> config.mak
diff --git a/lib/efi.c b/lib/efi.c
new file mode 100644
index 0000000..018918a
--- /dev/null
+++ b/lib/efi.c
@@ -0,0 +1,44 @@
+/*
+ * efi_main() function to set up and run test cases in EFI
+ *
+ * Copyright (c) 2021, Google Inc
+ *
+ * Authors:
+ *   Zixuan Wang <zixuanwang@google.com>
+ *
+ * SPDX-License-Identifier: LGPL-2.0-or-later
+ */
+
+#include <libcflat.h>
+#include <asm/setup.h>
+
+#ifdef ALIGN
+#undef ALIGN
+#endif
+#include <efi.h>
+#include <efilib.h>
+
+/* From lib/argv.c */
+extern int __argc, __envc;
+extern char *__argv[100];
+extern char *__environ[200];
+
+extern int main(int argc, char **argv, char **envp);
+
+EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *systab);
+
+EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *systab)
+{
+	int ret;
+
+	InitializeLib(image_handle, systab);
+
+	setup_efi();
+	ret = main(__argc, __argv, __environ);
+
+	/* Shutdown the Guest VM */
+	uefi_call_wrapper(RT->ResetSystem, 4, EfiResetShutdown, ret, 0, NULL);
+
+	/* Unreachable */
+	return EFI_UNSUPPORTED;
+}
diff --git a/lib/string.c b/lib/string.c
index ffc7c7e..060955a 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -100,6 +100,8 @@ char *strstr(const char *s1, const char *s2)
     return NULL;
 }
 
+#ifndef TARGET_EFI
+/* GNU-EFI already defines memset and memcpy */
 void *memset(void *s, int c, size_t n)
 {
     size_t i;
@@ -122,6 +124,7 @@ void *memcpy(void *dest, const void *src, size_t n)
 
     return dest;
 }
+#endif /* TARGET_EFI */
 
 int memcmp(const void *s1, const void *s2, size_t n)
 {
diff --git a/lib/x86/asm/setup.h b/lib/x86/asm/setup.h
new file mode 100644
index 0000000..eb1cf73
--- /dev/null
+++ b/lib/x86/asm/setup.h
@@ -0,0 +1,11 @@
+#ifndef _X86_ASM_SETUP_H_
+#define _X86_ASM_SETUP_H_
+
+#ifdef TARGET_EFI
+#include "x86/apic.h"
+#include "x86/smp.h"
+
+void setup_efi(void);
+#endif /* TARGET_EFI */
+
+#endif /* _X86_ASM_SETUP_H_ */
diff --git a/lib/x86/setup.c b/lib/x86/setup.c
index 7befe09..e3faf00 100644
--- a/lib/x86/setup.c
+++ b/lib/x86/setup.c
@@ -9,6 +9,7 @@
 #include "fwcfg.h"
 #include "alloc_phys.h"
 #include "argv.h"
+#include "asm/setup.h"
 
 extern char edata;
 
@@ -118,6 +119,19 @@ void setup_multiboot(struct mbi_bootinfo *bi)
 	initrd_size = mods->end - mods->start;
 }
 
+#ifdef TARGET_EFI
+
+void setup_efi(void)
+{
+	reset_apic();
+	mask_pic_interrupts();
+	enable_apic();
+	enable_x2apic();
+	smp_init();
+}
+
+#endif /* TARGET_EFI */
+
 void setup_libcflat(void)
 {
 	if (initrd) {
diff --git a/x86/Makefile.common b/x86/Makefile.common
index 52bb7aa..00adddd 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -22,6 +22,10 @@ cflatobjs += lib/x86/acpi.o
 cflatobjs += lib/x86/stack.o
 cflatobjs += lib/x86/fault_test.o
 cflatobjs += lib/x86/delay.o
+ifeq ($(TARGET_EFI),y)
+cflatobjs += lib/x86/setup.o
+cflatobjs += lib/efi.o
+endif
 
 OBJDIRS += lib/x86
 
@@ -37,10 +41,25 @@ COMMON_CFLAGS += -O1
 # stack.o relies on frame pointers.
 KEEP_FRAME_POINTER := y
 
+FLATLIBS = lib/libcflat.a
+
+ifeq ($(TARGET_EFI),y)
+.PRECIOUS: %.efi %.so
+
+%.so: %.o $(FLATLIBS) $(SRCDIR)/x86/efi/elf_x86_64_efi.lds $(cstart.o)
+	$(LD) -T $(SRCDIR)/x86/efi/elf_x86_64_efi.lds $(EFI_LDFLAGS) -o $@ \
+		$(filter %.o, $^) $(FLATLIBS) $(EFI_LIBS)
+	@chmod a-x $@
+
+%.efi: %.so
+	$(OBJCOPY) \
+		-j .text -j .sdata -j .data -j .dynamic -j .dynsym -j .rel \
+		-j .rela -j .reloc -S --target=$(FORMAT) $< $@
+	@chmod a-x $@
+else
 # We want to keep intermediate file: %.elf and %.o 
 .PRECIOUS: %.elf %.o
 
-FLATLIBS = lib/libcflat.a
 %.elf: %.o $(FLATLIBS) $(SRCDIR)/x86/flat.lds $(cstart.o)
 	$(CC) $(CFLAGS) -nostdlib -o $@ -Wl,-T,$(SRCDIR)/x86/flat.lds \
 		$(filter %.o, $^) $(FLATLIBS)
@@ -49,18 +68,29 @@ FLATLIBS = lib/libcflat.a
 %.flat: %.elf
 	$(OBJCOPY) -O elf32-i386 $^ $@
 	@chmod a-x $@
+endif
 
-tests-common = $(TEST_DIR)/vmexit.flat $(TEST_DIR)/tsc.flat \
-               $(TEST_DIR)/smptest.flat  \
-               $(TEST_DIR)/realmode.flat $(TEST_DIR)/msr.flat \
-               $(TEST_DIR)/hypercall.flat $(TEST_DIR)/sieve.flat \
-               $(TEST_DIR)/kvmclock_test.flat  $(TEST_DIR)/eventinj.flat \
-               $(TEST_DIR)/s3.flat $(TEST_DIR)/pmu.flat $(TEST_DIR)/setjmp.flat \
-               $(TEST_DIR)/tsc_adjust.flat $(TEST_DIR)/asyncpf.flat \
-               $(TEST_DIR)/init.flat $(TEST_DIR)/smap.flat \
-               $(TEST_DIR)/hyperv_synic.flat $(TEST_DIR)/hyperv_stimer.flat \
-               $(TEST_DIR)/hyperv_connections.flat \
-               $(TEST_DIR)/umip.flat $(TEST_DIR)/tsx-ctrl.flat
+tests-common = $(TEST_DIR)/vmexit.$(exe) $(TEST_DIR)/tsc.$(exe) \
+               $(TEST_DIR)/smptest.$(exe)  \
+               $(TEST_DIR)/msr.$(exe) \
+               $(TEST_DIR)/hypercall.$(exe) $(TEST_DIR)/sieve.$(exe) \
+               $(TEST_DIR)/kvmclock_test.$(exe) \
+               $(TEST_DIR)/s3.$(exe) $(TEST_DIR)/pmu.$(exe) $(TEST_DIR)/setjmp.$(exe) \
+               $(TEST_DIR)/tsc_adjust.$(exe) $(TEST_DIR)/asyncpf.$(exe) \
+               $(TEST_DIR)/init.$(exe) \
+               $(TEST_DIR)/hyperv_synic.$(exe) $(TEST_DIR)/hyperv_stimer.$(exe) \
+               $(TEST_DIR)/hyperv_connections.$(exe) \
+               $(TEST_DIR)/tsx-ctrl.$(exe)
+
+# The following test cases are disabled when building EFI tests because they
+# use absolute addresses in their inline assembly code, which cannot compile
+# with the '-fPIC' flag
+ifneq ($(TARGET_EFI),y)
+tests-common += $(TEST_DIR)/eventinj.$(exe) \
+                $(TEST_DIR)/smap.$(exe) \
+                $(TEST_DIR)/realmode.$(exe) \
+                $(TEST_DIR)/umip.$(exe)
+endif
 
 test_cases: $(tests-common) $(tests)
 
@@ -72,14 +102,16 @@ $(TEST_DIR)/realmode.elf: $(TEST_DIR)/realmode.o
 
 $(TEST_DIR)/realmode.o: bits = $(if $(call cc-option,-m16,""),16,32)
 
-$(TEST_DIR)/kvmclock_test.elf: $(TEST_DIR)/kvmclock.o
+$(TEST_DIR)/kvmclock_test.$(bin): $(TEST_DIR)/kvmclock.o
 
-$(TEST_DIR)/hyperv_synic.elf: $(TEST_DIR)/hyperv.o
+$(TEST_DIR)/hyperv_synic.$(bin): $(TEST_DIR)/hyperv.o
 
-$(TEST_DIR)/hyperv_stimer.elf: $(TEST_DIR)/hyperv.o
+$(TEST_DIR)/hyperv_stimer.$(bin): $(TEST_DIR)/hyperv.o
 
-$(TEST_DIR)/hyperv_connections.elf: $(TEST_DIR)/hyperv.o
+$(TEST_DIR)/hyperv_connections.$(bin): $(TEST_DIR)/hyperv.o
 
 arch_clean:
 	$(RM) $(TEST_DIR)/*.o $(TEST_DIR)/*.flat $(TEST_DIR)/*.elf \
 	$(TEST_DIR)/.*.d lib/x86/.*.d \
+	$(TEST_DIR)/efi/*.o $(TEST_DIR)/efi/.*.d \
+	$(TEST_DIR)/*.so $(TEST_DIR)/*.efi
diff --git a/x86/Makefile.i386 b/x86/Makefile.i386
index 960e274..340c561 100644
--- a/x86/Makefile.i386
+++ b/x86/Makefile.i386
@@ -1,11 +1,12 @@
 cstart.o = $(TEST_DIR)/cstart.o
 bits = 32
 ldarch = elf32-i386
+exe = flat
 COMMON_CFLAGS += -mno-sse -mno-sse2
 
 cflatobjs += lib/x86/setjmp32.o lib/ldiv32.o
 
-tests = $(TEST_DIR)/taskswitch.flat $(TEST_DIR)/taskswitch2.flat \
-	$(TEST_DIR)/cmpxchg8b.flat $(TEST_DIR)/la57.flat
+tests = $(TEST_DIR)/taskswitch.$(exe) $(TEST_DIR)/taskswitch2.$(exe) \
+	$(TEST_DIR)/cmpxchg8b.$(exe) $(TEST_DIR)/la57.$(exe)
 
 include $(SRCDIR)/$(TEST_DIR)/Makefile.common
diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
index 8134952..7063ba1 100644
--- a/x86/Makefile.x86_64
+++ b/x86/Makefile.x86_64
@@ -1,6 +1,15 @@
 cstart.o = $(TEST_DIR)/cstart64.o
 bits = 64
 ldarch = elf64-x86-64
+ifeq ($(TARGET_EFI),y)
+exe = efi
+bin = so
+FORMAT = efi-app-x86_64
+cstart.o = $(EFI_LIBS_PATH)/crt0-efi-x86_64.o
+else
+exe = flat
+bin = elf
+endif
 
 fcf_protection_full := $(call cc-option, -fcf-protection=full,)
 COMMON_CFLAGS += -mno-red-zone -mno-sse -mno-sse2 $(fcf_protection_full)
@@ -9,29 +18,36 @@ cflatobjs += lib/x86/setjmp64.o
 cflatobjs += lib/x86/intel-iommu.o
 cflatobjs += lib/x86/usermode.o
 
-tests = $(TEST_DIR)/access.flat $(TEST_DIR)/apic.flat \
-	  $(TEST_DIR)/emulator.flat $(TEST_DIR)/idt_test.flat \
-	  $(TEST_DIR)/xsave.flat $(TEST_DIR)/rmap_chain.flat \
-	  $(TEST_DIR)/pcid.flat $(TEST_DIR)/debug.flat \
-	  $(TEST_DIR)/ioapic.flat $(TEST_DIR)/memory.flat \
-	  $(TEST_DIR)/pku.flat $(TEST_DIR)/hyperv_clock.flat
-tests += $(TEST_DIR)/syscall.flat
-tests += $(TEST_DIR)/svm.flat
-tests += $(TEST_DIR)/vmx.flat
-tests += $(TEST_DIR)/tscdeadline_latency.flat
-tests += $(TEST_DIR)/intel-iommu.flat
-tests += $(TEST_DIR)/vmware_backdoors.flat
-tests += $(TEST_DIR)/rdpru.flat
-tests += $(TEST_DIR)/pks.flat
-tests += $(TEST_DIR)/pmu_lbr.flat
+tests = $(TEST_DIR)/apic.$(exe) \
+	  $(TEST_DIR)/idt_test.$(exe) \
+	  $(TEST_DIR)/xsave.$(exe) $(TEST_DIR)/rmap_chain.$(exe) \
+	  $(TEST_DIR)/pcid.$(exe) $(TEST_DIR)/debug.$(exe) \
+	  $(TEST_DIR)/ioapic.$(exe) $(TEST_DIR)/memory.$(exe) \
+	  $(TEST_DIR)/pku.$(exe) $(TEST_DIR)/hyperv_clock.$(exe)
+tests += $(TEST_DIR)/syscall.$(exe)
+tests += $(TEST_DIR)/tscdeadline_latency.$(exe)
+tests += $(TEST_DIR)/intel-iommu.$(exe)
+tests += $(TEST_DIR)/rdpru.$(exe)
+tests += $(TEST_DIR)/pks.$(exe)
+tests += $(TEST_DIR)/pmu_lbr.$(exe)
 
+# The following test cases are disabled when building EFI tests because they
+# use absolute addresses in their inline assembly code, which cannot compile
+# with the '-fPIC' flag
+ifneq ($(TARGET_EFI),y)
+tests += $(TEST_DIR)/access.$(exe)
+tests += $(TEST_DIR)/emulator.$(exe)
+tests += $(TEST_DIR)/svm.$(exe)
+tests += $(TEST_DIR)/vmx.$(exe)
+tests += $(TEST_DIR)/vmware_backdoors.$(exe)
 ifneq ($(fcf_protection_full),)
-tests += $(TEST_DIR)/cet.flat
+tests += $(TEST_DIR)/cet.$(exe)
+endif
 endif
 
 include $(SRCDIR)/$(TEST_DIR)/Makefile.common
 
-$(TEST_DIR)/hyperv_clock.elf: $(TEST_DIR)/hyperv_clock.o
+$(TEST_DIR)/hyperv_clock.$(bin): $(TEST_DIR)/hyperv_clock.o
 
-$(TEST_DIR)/vmx.elf: $(TEST_DIR)/vmx_tests.o
-$(TEST_DIR)/svm.elf: $(TEST_DIR)/svm_tests.o
+$(TEST_DIR)/vmx.$(bin): $(TEST_DIR)/vmx_tests.o
+$(TEST_DIR)/svm.$(bin): $(TEST_DIR)/svm_tests.o
diff --git a/x86/efi/README.md b/x86/efi/README.md
index 4deba6e..b7f760a 100644
--- a/x86/efi/README.md
+++ b/x86/efi/README.md
@@ -1,4 +1,46 @@
-# EFI Startup Code and Linker Script
+# Build KVM-Unit-Tests with GNU-EFI
+
+## Introduction
+
+This dir provides code to build KVM-Unit-Tests with GNU-EFI and run the test
+cases with QEMU and UEFI.
+
+### Install dependencies
+
+The following dependencies should be installed:
+
+- [GNU-EFI](https://sourceforge.net/projects/gnu-efi): to build test cases as
+  EFI applications
+- [UEFI firmware](https://github.com/tianocore/edk2): to run test cases in QEMU
+
+### Build with GNU-EFI
+
+To build with GNU-EFI, do:
+
+    ./configure --target-efi
+    make
+
+Building UEFI tests requires the
+[GNU-EFI](https://sourceforge.net/projects/gnu-efi) library: the Makefile
+searches GNU-EFI headers under `/usr/include/efi` and static libraries under
+`/usr/lib/` by default. These paths can be overridden by `./configure` flags
+`efi-include-path` and `efi-libs-path`.
+
+### Run test cases with UEFI
+
+To run a test case with UEFI:
+
+    ./x86/efi/run ./x86/dummy.efi
+
+By default the runner script loads the UEFI firmware `/usr/share/ovmf/OVMF.fd`;
+please install UEFI firmware to this path, or specify the correct path through
+the env variable `EFI_UEFI`:
+
+    EFI_UEFI=/path/to/OVMF.fd ./x86/efi/run ./x86/dummy.efi
+
+## Code structure
+
+### Code from GNU-EFI
 
 This dir contains a linker script copied from
 [GNU-EFI](https://sourceforge.net/projects/gnu-efi/):
@@ -23,3 +65,8 @@ above-mentioned files in its build process:
 
 More details can be found in `GNU-EFI/README.gnuefi`, section "Building
 Relocatable Binaries".
+
+### Startup code for KVM-Unit-Tests in UEFI
+
+This dir also contains KVM-Unit-Tests startup code in UEFI:
+   - efistart64.S: startup code for KVM-Unit-Tests in UEFI
diff --git a/x86/efi/run b/x86/efi/run
new file mode 100755
index 0000000..72ad4a9
--- /dev/null
+++ b/x86/efi/run
@@ -0,0 +1,63 @@
+#!/bin/bash
+
+set -e
+
+if [ $# -eq 0 ]; then
+	echo "Usage $0 TEST_CASE [QEMU_ARGS]"
+	exit 2
+fi
+
+if [ ! -f config.mak ]; then
+	echo "run './configure --target-efi && make' first. See ./configure -h"
+	exit 2
+fi
+source config.mak
+
+: "${EFI_SRC:=$(realpath "$(dirname "$0")/../")}"
+: "${EFI_UEFI:=/usr/share/ovmf/OVMF.fd}"
+: "${EFI_TEST:=efi-tests}"
+: "${EFI_SMP:=1}"
+: "${EFI_CASE:=$(basename $1 .efi)}"
+
+if [ ! -f "$EFI_UEFI" ]; then
+	echo "UEFI firmware not found: $EFI_UEFI"
+	echo "Please install the UEFI firmware to this path"
+	echo "Or specify the correct path with the env variable EFI_UEFI"
+	exit 2
+fi
+
+# Remove the TEST_CASE from $@
+shift 1
+
+# Prepare EFI boot file system
+#   - Copy .efi file to host dir $EFI_TEST/$EFI_CASE/
+#     This host dir will be loaded by QEMU as a FAT32 image
+#   - Make UEFI startup script that runs the .efi on boot
+mkdir -p "$EFI_TEST/$EFI_CASE/"
+cp "$EFI_SRC/$EFI_CASE.efi" "$EFI_TEST/$EFI_CASE/"
+
+pushd "$EFI_TEST/$EFI_CASE" || exit 2
+# 'startup.nsh' is the default script executed by UEFI on boot
+# Use this script to run the test binary automatically
+cat << EOF >startup.nsh
+@echo -off
+fs0:
+"$EFI_CASE.efi"
+EOF
+popd || exit 2
+
+# Run test case with 256MiB QEMU memory. QEMU default memory size is 128MiB.
+# After UEFI boot up and we call `LibMemoryMap()`, the largest consecutive
+# memory region is ~42MiB. Although this is sufficient for many test cases to
+# run in UEFI, some test cases, e.g. `x86/pmu.c`, require more free memory. A
+# simple fix is to increase the QEMU default memory size to 256MiB so that
+# UEFI's largest allocatable memory region is large enough.
+EFI_RUN=y \
+"$TEST_DIR/run" \
+	-drive file="$EFI_UEFI",format=raw,if=pflash \
+	-drive file.dir="$EFI_TEST/$EFI_CASE/",file.driver=vvfat,file.rw=on,format=raw \
+	-net none \
+	-nographic \
+	-smp "$EFI_SMP" \
+	-m 256 \
+	"$@"
diff --git a/x86/run b/x86/run
index 8b2425f..4eba2b9 100755
--- a/x86/run
+++ b/x86/run
@@ -38,7 +38,19 @@ else
 fi
 
 command="${qemu} --no-reboot -nodefaults $pc_testdev -vnc none -serial stdio $pci_testdev"
-command+=" -machine accel=$ACCEL -kernel"
+command+=" -machine accel=$ACCEL"
+if ! [ "$EFI_RUN" ]; then
+	command+=" -kernel"
+fi
 command="$(timeout_cmd) $command"
 
-run_qemu ${command} "$@"
+if [ "$EFI_RUN" ]; then
+	# Set ENVIRON_DEFAULT=n to remove '-initrd' flag for QEMU (see
+	# 'scripts/arch-run.bash' for more details). This is because when using
+	# UEFI, the test case binaries are passed to QEMU through the disk
+	# image, not through the '-kernel' flag. And QEMU reports an error if it
+	# gets '-initrd' without a '-kernel'
+	ENVIRON_DEFAULT=n run_qemu ${command} "$@"
+else
+	run_qemu ${command} "$@"
+fi
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 03/16] x86 UEFI: Move setjmp.h out of desc.h
  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 ` 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
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:08 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

Previous desc.h includes setjmp.h, and this causes duplicate definition
when compiling with UEFI header efi.h. This is because setjmp() function
is defined both in KVM-Unit-Tests and UEFI. When including both desc.h
and efi.h, the setjmp() is found in both headers and causes this error.

The easy solution is to move setjmp.h from desc.h to desc.c, so
including desc.h does not bring in setjmp.h, so that we can include
both desc.h and efi.h in lib/x86/setup.c.

This commit also includes setjmp.h in x86/vmx.c, because this test case
previously assumed setjmp.h is included by desc.h and did not include
setjmp.h explicitly.

This commit does not change any test case behavior.

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/x86/desc.c | 4 ++++
 lib/x86/desc.h | 5 -----
 x86/vmx.c      | 1 +
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index e7378c1..eb4d2bc 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -3,6 +3,10 @@
 #include "processor.h"
 #include <setjmp.h>
 
+void __set_exception_jmpbuf(jmp_buf *addr);
+#define set_exception_jmpbuf(jmpbuf) \
+	(setjmp(jmpbuf) ? : (__set_exception_jmpbuf(&(jmpbuf)), 0))
+
 #ifndef __x86_64__
 __attribute__((regparm(1)))
 #endif
diff --git a/lib/x86/desc.h b/lib/x86/desc.h
index a6ffb38..9fda20d 100644
--- a/lib/x86/desc.h
+++ b/lib/x86/desc.h
@@ -1,8 +1,6 @@
 #ifndef _X86_DESC_H_
 #define _X86_DESC_H_
 
-#include <setjmp.h>
-
 void setup_idt(void);
 void setup_alt_stack(void);
 
@@ -226,9 +224,6 @@ void unhandled_exception(struct ex_regs *regs, bool cpu);
 
 bool test_for_exception(unsigned int ex, void (*trigger_func)(void *data),
 			void *data);
-void __set_exception_jmpbuf(jmp_buf *addr);
-#define set_exception_jmpbuf(jmpbuf) \
-	(setjmp(jmpbuf) ? : (__set_exception_jmpbuf(&(jmpbuf)), 0))
 
 static inline void *get_idt_addr(idt_entry_t *entry)
 {
diff --git a/x86/vmx.c b/x86/vmx.c
index f0b853a..4469b31 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -38,6 +38,7 @@
 #include "msr.h"
 #include "smp.h"
 #include "apic.h"
+#include "setjmp.h"
 
 u64 *bsp_vmxon_region;
 struct vmcs *vmcs_root;
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 04/16] x86 UEFI: Load KVM-Unit-Tests IDT after UEFI boot up
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (2 preceding siblings ...)
  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 ` Zixuan Wang
  2021-08-18  0:08 ` [kvm-unit-tests RFC 05/16] x86 UEFI: Load GDT and TSS " Zixuan Wang
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:08 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

Interrupt descriptor table (IDT) is used by x86 arch to describe the
interrupt handlers. UEFI already setup an IDT before running the test
binaries, but this IDT is not compatible with the existing
KVM-Unit-Tests test cases, e.g., x86/msr.c triggers a #GP fault when
using UEFI IDT. This is because test cases setup new interrupt handlers
and register them into KVM-Unit-Tests' IDT, but it takes no effect if we
do not load KVM-Unit-Tests' IDT.

This commit fixes this issue:

   1. Port the IDT definition from cstart64.S to efistart64.S
   2. Update IDT descriptor with runtime IDT address and load it

In this commit, the x86/msr.c can run in UEFI and generates same output
as the default Seabios version.

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/x86/setup.c            |  6 ++++++
 x86/Makefile.common        |  3 ++-
 x86/Makefile.x86_64        |  2 +-
 x86/efi/README.md          |  4 ++--
 x86/efi/efistart64.S       | 32 ++++++++++++++++++++++++++++++++
 x86/efi/elf_x86_64_efi.lds |  6 +++++-
 6 files changed, 48 insertions(+), 5 deletions(-)
 create mode 100644 x86/efi/efistart64.S

diff --git a/lib/x86/setup.c b/lib/x86/setup.c
index e3faf00..9548c5b 100644
--- a/lib/x86/setup.c
+++ b/lib/x86/setup.c
@@ -9,6 +9,7 @@
 #include "fwcfg.h"
 #include "alloc_phys.h"
 #include "argv.h"
+#include "x86/desc.h"
 #include "asm/setup.h"
 
 extern char edata;
@@ -121,9 +122,14 @@ void setup_multiboot(struct mbi_bootinfo *bi)
 
 #ifdef TARGET_EFI
 
+/* From x86/efi/efistart64.S */
+extern void load_idt(void);
+
 void setup_efi(void)
 {
 	reset_apic();
+	setup_idt();
+	load_idt();
 	mask_pic_interrupts();
 	enable_apic();
 	enable_x2apic();
diff --git a/x86/Makefile.common b/x86/Makefile.common
index 00adddd..314bf47 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -48,7 +48,8 @@ ifeq ($(TARGET_EFI),y)
 
 %.so: %.o $(FLATLIBS) $(SRCDIR)/x86/efi/elf_x86_64_efi.lds $(cstart.o)
 	$(LD) -T $(SRCDIR)/x86/efi/elf_x86_64_efi.lds $(EFI_LDFLAGS) -o $@ \
-		$(filter %.o, $^) $(FLATLIBS) $(EFI_LIBS)
+		$(EFI_LIBS_PATH)/crt0-efi-x86_64.o $(filter %.o, $^) \
+		$(FLATLIBS) $(EFI_LIBS)
 	@chmod a-x $@
 
 %.efi: %.so
diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
index 7063ba1..aa23b22 100644
--- a/x86/Makefile.x86_64
+++ b/x86/Makefile.x86_64
@@ -5,7 +5,7 @@ ifeq ($(TARGET_EFI),y)
 exe = efi
 bin = so
 FORMAT = efi-app-x86_64
-cstart.o = $(EFI_LIBS_PATH)/crt0-efi-x86_64.o
+cstart.o = $(TEST_DIR)/efi/efistart64.o
 else
 exe = flat
 bin = elf
diff --git a/x86/efi/README.md b/x86/efi/README.md
index b7f760a..d31993e 100644
--- a/x86/efi/README.md
+++ b/x86/efi/README.md
@@ -30,13 +30,13 @@ searches GNU-EFI headers under `/usr/include/efi` and static libraries under
 
 To run a test case with UEFI:
 
-    ./x86/efi/run ./x86/dummy.efi
+    ./x86/efi/run ./x86/msr.efi
 
 By default the runner script loads the UEFI firmware `/usr/share/ovmf/OVMF.fd`;
 please install UEFI firmware to this path, or specify the correct path through
 the env variable `EFI_UEFI`:
 
-    EFI_UEFI=/path/to/OVMF.fd ./x86/efi/run ./x86/dummy.efi
+    EFI_UEFI=/path/to/OVMF.fd ./x86/efi/run ./x86/msr.efi
 
 ## Code structure
 
diff --git a/x86/efi/efistart64.S b/x86/efi/efistart64.S
new file mode 100644
index 0000000..e8d5ad6
--- /dev/null
+++ b/x86/efi/efistart64.S
@@ -0,0 +1,32 @@
+/* Startup code and pre-defined data structures */
+
+.globl boot_idt
+.globl idt_descr
+
+.data
+
+boot_idt:
+	.rept 256
+	.quad 0
+	.quad 0
+	.endr
+end_boot_idt:
+
+idt_descr:
+	.word end_boot_idt - boot_idt - 1
+	.quad 0 /* To be filled with runtime addr of boot_idt(%rip) */
+
+.section .init
+.code64
+.text
+
+.globl load_idt
+load_idt:
+	/* Set IDT runtime address */
+	lea boot_idt(%rip), %rax
+	mov %rax, idt_descr+2(%rip)
+
+	/* Load IDT */
+	lidtq idt_descr(%rip)
+
+	retq
diff --git a/x86/efi/elf_x86_64_efi.lds b/x86/efi/elf_x86_64_efi.lds
index 5eae376..3d92c86 100644
--- a/x86/efi/elf_x86_64_efi.lds
+++ b/x86/efi/elf_x86_64_efi.lds
@@ -1,4 +1,4 @@
-/* Copied from GNU-EFI/gnuefi/elf_x86_64_efi.lds, licensed under GNU GPL */
+/* Developed based on GNU-EFI/gnuefi/elf_x86_64_efi.lds, licensed under GNU GPL */
 /* Same as elf_x86_64_fbsd_efi.lds, except for OUTPUT_FORMAT below - KEEP IN SYNC */
 OUTPUT_FORMAT("elf64-x86-64", "elf64-x86-64", "elf64-x86-64")
 OUTPUT_ARCH(i386:x86-64)
@@ -38,6 +38,10 @@ SECTIONS
    *(.rodata*)
    *(.got.plt)
    *(.got)
+   /* Expected by lib/x86/desc.c to store exception_table */
+   exception_table_start = .;
+   *(.data.ex)
+   exception_table_end = .;
    *(.data*)
    *(.sdata)
    /* the EFI loader doesn't seem to like a .bss section, so we stick
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 05/16] x86 UEFI: Load GDT and TSS after UEFI boot up
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (3 preceding siblings ...)
  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 ` Zixuan Wang
  2021-08-18  0:08 ` [kvm-unit-tests RFC 06/16] x86 UEFI: Set up memory allocator Zixuan Wang
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:08 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

Global Descriptor Table (GDT) defines x86 memory areas, e.g. memory area
for data or code. UEFI has a different GDT compared to KVM-Unit-Tests,
e.g., in UEFI, 3rd GDT entry defines a code segment, while in
KVM-Unit-Tests, 3rd GDT entry is data segment.

Without loading KVM-Unit-Tests GDT, the UEFI GDT is used and is
incompatible with KVM-Unit-Tests. This causes QEMU to silently crash
when a test case changes segments.

This commit fixes this issue by loading KVM-Unit-Tests GDT to replace
UEFI GDT. And since Task State Segment (TSS) is tightly coupled with
GDT, this commit also loads TSS on boot-up.

The GDT and TSS loading function is originally written in assembly code,
see cstart64.S load_tss function. This commit provides a similar C
function setup_gdt_tss() which is more readable and easier to modify.

In this commit, x86/debug.c can run in UEFI and pass all sub-tests.

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/x86/setup.c      | 44 ++++++++++++++++++++++
 x86/efi/efistart64.S | 88 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+)

diff --git a/lib/x86/setup.c b/lib/x86/setup.c
index 9548c5b..51be241 100644
--- a/lib/x86/setup.c
+++ b/lib/x86/setup.c
@@ -124,10 +124,54 @@ void setup_multiboot(struct mbi_bootinfo *bi)
 
 /* From x86/efi/efistart64.S */
 extern void load_idt(void);
+extern void load_gdt_tss(size_t tss_offset);
+extern phys_addr_t tss_descr;
+extern phys_addr_t ring0stacktop;
+extern gdt_entry_t gdt64[];
+extern size_t ring0stacksize;
+
+static void setup_gdt_tss(void)
+{
+	gdt_entry_t *tss_lo, *tss_hi;
+	tss64_t *curr_tss;
+	phys_addr_t curr_tss_addr;
+	u32 id;
+	size_t tss_offset;
+	size_t pre_tss_entries;
+
+	/* Get APIC ID, see also x86/cstart64.S:load_tss */
+	id = apic_id();
+
+	/* Get number of GDT entries before TSS-related GDT entry */
+	pre_tss_entries = (size_t)((u8 *)&(tss_descr) - (u8 *)gdt64) / sizeof(gdt_entry_t);
+
+	/* Each TSS descriptor takes up 2 GDT entries */
+	tss_offset = (pre_tss_entries + id * 2) * sizeof(gdt_entry_t);
+	tss_lo = &(gdt64[pre_tss_entries + id * 2 + 0]);
+	tss_hi = &(gdt64[pre_tss_entries + id * 2 + 1]);
+
+	/* Runtime address of current TSS */
+	curr_tss_addr = (((phys_addr_t)&tss) + (phys_addr_t)(id * sizeof(tss64_t)));
+
+	/* Use runtime address for ring0stacktop, see also x86/cstart64.S:tss */
+	curr_tss = (tss64_t *)curr_tss_addr;
+	curr_tss->rsp0 = (u64)((u8*)&ring0stacktop - id * ring0stacksize);
+
+	/* Update TSS descriptors */
+	tss_lo->limit_low = sizeof(tss64_t);
+	tss_lo->base_low = (u16)(curr_tss_addr & 0xffff);
+	tss_lo->base_middle = (u8)((curr_tss_addr >> 16) & 0xff);
+	tss_lo->base_high = (u8)((curr_tss_addr >> 24) & 0xff);
+	tss_hi->limit_low = (u16)((curr_tss_addr >> 32) & 0xffff);
+	tss_hi->base_low = (u16)((curr_tss_addr >> 48) & 0xffff);
+
+	load_gdt_tss(tss_offset);
+}
 
 void setup_efi(void)
 {
 	reset_apic();
+	setup_gdt_tss();
 	setup_idt();
 	load_idt();
 	mask_pic_interrupts();
diff --git a/x86/efi/efistart64.S b/x86/efi/efistart64.S
index e8d5ad6..cc993e5 100644
--- a/x86/efi/efistart64.S
+++ b/x86/efi/efistart64.S
@@ -1,7 +1,23 @@
 /* Startup code and pre-defined data structures */
 
+#include "apic-defs.h"
+#include "asm-generic/page.h"
+
 .globl boot_idt
 .globl idt_descr
+.globl tss_descr
+.globl gdt64_desc
+.globl ring0stacksize
+
+max_cpus = MAX_TEST_CPUS
+ring0stacksize = PAGE_SIZE
+
+.bss
+
+.globl ring0stacktop
+	. = . + ring0stacksize * max_cpus
+	.align 16
+ring0stacktop:
 
 .data
 
@@ -16,6 +32,48 @@ idt_descr:
 	.word end_boot_idt - boot_idt - 1
 	.quad 0 /* To be filled with runtime addr of boot_idt(%rip) */
 
+gdt64_desc:
+	.word gdt64_end - gdt64 - 1
+	.quad 0 /* To be filled with runtime addr of gdt64(%rip) */
+
+.globl gdt64
+gdt64:
+	.quad 0
+	.quad 0x00af9b000000ffff /* 64-bit code segment */
+	.quad 0x00cf93000000ffff /* 32/64-bit data segment */
+	.quad 0x00af1b000000ffff /* 64-bit code segment, not present */
+	.quad 0x00cf9b000000ffff /* 32-bit code segment */
+	.quad 0x008f9b000000FFFF /* 16-bit code segment */
+	.quad 0x008f93000000FFFF /* 16-bit data segment */
+	.quad 0x00cffb000000ffff /* 32-bit code segment (user) */
+	.quad 0x00cff3000000ffff /* 32/64-bit data segment (user) */
+	.quad 0x00affb000000ffff /* 64-bit code segment (user) */
+
+	.quad 0			 /* 6 spare selectors */
+	.quad 0
+	.quad 0
+	.quad 0
+	.quad 0
+	.quad 0
+
+tss_descr:
+	.rept max_cpus
+	.quad 0x000089000000ffff /* 64-bit avail tss */
+	.quad 0                  /* tss high addr */
+	.endr
+gdt64_end:
+
+.globl tss
+tss:
+	.rept max_cpus
+	.long 0
+	.quad 0
+	.quad 0, 0
+	.quad 0, 0, 0, 0, 0, 0, 0, 0
+	.long 0, 0, 0
+	.endr
+tss_end:
+
 .section .init
 .code64
 .text
@@ -30,3 +88,33 @@ load_idt:
 	lidtq idt_descr(%rip)
 
 	retq
+
+.globl load_gdt_tss
+load_gdt_tss:
+	/* Set GDT runtime address */
+	lea gdt64(%rip), %rax
+	mov %rax, gdt64_desc+2(%rip)
+
+	/* Load GDT */
+	lgdt gdt64_desc(%rip)
+
+	/* Load TSS */
+	mov %rdi, %rax
+	ltr %ax
+
+	/* Update data segments */
+	mov $0x10, %ax /* 3rd entry in gdt64: 32/64-bit data segment */
+	mov %ax, %ds
+	mov %ax, %es
+	mov %ax, %fs
+	mov %ax, %gs
+	mov %ax, %ss
+
+	/* Update the code segment by putting it on the stack before the return
+	 * address, then doing a far return: this will use the new code segment
+	 * along with the address.
+	 */
+	popq %rdi
+	pushq $0x08 /* 2nd entry in gdt64: 64-bit code segment */
+	pushq %rdi
+	lretq
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 06/16] x86 UEFI: Set up memory allocator
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (4 preceding siblings ...)
  2021-08-18  0:08 ` [kvm-unit-tests RFC 05/16] x86 UEFI: Load GDT and TSS " Zixuan Wang
@ 2021-08-18  0:08 ` Zixuan Wang
  2021-08-18  0:08 ` [kvm-unit-tests RFC 07/16] x86 UEFI: Set up RSDP after UEFI boot up Zixuan Wang
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:08 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

KVM-Unit-Tests library implements a memory allocator which requires
two arguments to set up (See `lib/alloc_phys.c:phys_alloc_init()` for
more details):
   1. A base (start) physical address
   2. Size of available memory for allocation

To get this memory info, we scan all the memory regions returned by
`LibMemoryMap()`, find out the largest free memory region and use it for
memory allocation.

After retrieving this memory info, we call `ExitBootServices` so that
KVM-Unit-Tests has full control of the machine, and UEFI will not touch
the memory after this point.

Starting from this commit, `x86/hypercall.c` test case can run in UEFI
and generates the same output as in Seabios.

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/efi.c           | 18 ++++++++++-
 lib/x86/asm/setup.h | 20 +++++++++++-
 lib/x86/setup.c     | 79 ++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 114 insertions(+), 3 deletions(-)

diff --git a/lib/efi.c b/lib/efi.c
index 018918a..a644913 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -30,10 +30,26 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *systab);
 EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *systab)
 {
 	int ret;
+	EFI_STATUS status;
+	UINTN mapkey = 0;
+	efi_bootinfo_t efi_bootinfo;
 
 	InitializeLib(image_handle, systab);
 
-	setup_efi();
+	setup_efi_bootinfo(&efi_bootinfo);
+	status = setup_efi_pre_boot(&mapkey, &efi_bootinfo);
+	if (EFI_ERROR(status)) {
+		printf("Failed to set up before ExitBootServices, exiting.\n");
+		return status;
+	}
+
+	status = uefi_call_wrapper(BS->ExitBootServices, 2, image_handle, mapkey);
+	if (EFI_ERROR(status)) {
+		printf("Failed to exit boot services\n");
+		return status;
+	}
+
+	setup_efi(&efi_bootinfo);
 	ret = main(__argc, __argv, __environ);
 
 	/* Shutdown the Guest VM */
diff --git a/lib/x86/asm/setup.h b/lib/x86/asm/setup.h
index eb1cf73..c22c999 100644
--- a/lib/x86/asm/setup.h
+++ b/lib/x86/asm/setup.h
@@ -5,7 +5,25 @@
 #include "x86/apic.h"
 #include "x86/smp.h"
 
-void setup_efi(void);
+#ifdef ALIGN
+#undef ALIGN
+#endif
+#include <efi.h>
+#include <efilib.h>
+
+/* efi_bootinfo_t: stores EFI-related machine info retrieved by
+ * setup_efi_pre_boot(), and is then used by setup_efi(). setup_efi() cannot
+ * retrieve this info as it is called after ExitBootServices and thus some EFI
+ * resources are not available.
+ */
+typedef struct {
+	phys_addr_t free_mem_start;
+	phys_addr_t free_mem_size;
+} efi_bootinfo_t;
+
+void setup_efi_bootinfo(efi_bootinfo_t *efi_bootinfo);
+void setup_efi(efi_bootinfo_t *efi_bootinfo);
+EFI_STATUS setup_efi_pre_boot(UINTN *mapkey, efi_bootinfo_t *efi_bootinfo);
 #endif /* TARGET_EFI */
 
 #endif /* _X86_ASM_SETUP_H_ */
diff --git a/lib/x86/setup.c b/lib/x86/setup.c
index 51be241..0f6e376 100644
--- a/lib/x86/setup.c
+++ b/lib/x86/setup.c
@@ -130,6 +130,81 @@ extern phys_addr_t ring0stacktop;
 extern gdt_entry_t gdt64[];
 extern size_t ring0stacksize;
 
+void setup_efi_bootinfo(efi_bootinfo_t *efi_bootinfo)
+{
+	efi_bootinfo->free_mem_size = 0;
+	efi_bootinfo->free_mem_start = 0;
+}
+
+static EFI_STATUS setup_pre_boot_memory(UINTN *mapkey, efi_bootinfo_t *efi_bootinfo)
+{
+	UINTN total_entries, desc_size;
+	UINT32 desc_version;
+	char *buffer;
+	int i;
+	UINT64 free_mem_total_pages = 0;
+
+	/* Although buffer entries are later converted to EFI_MEMORY_DESCRIPTOR,
+	 * we cannot simply define buffer as 'EFI_MEMORY_DESCRIPTOR *buffer'.
+	 * Because the actual buffer entry size 'desc_size' is bigger than
+	 * 'sizeof(EFI_MEMORY_DESCRIPTOR)', i.e. there are padding data after
+	 * each EFI_MEMORY_DESCRIPTOR. So defining 'EFI_MEMORY_DESCRIPTOR
+	 * *buffer' leads to wrong buffer entries fetched.
+	 */
+	buffer = (char *)LibMemoryMap(&total_entries, mapkey, &desc_size, &desc_version);
+	if (desc_version != 1) {
+		return EFI_INCOMPATIBLE_VERSION;
+	}
+
+	/* The 'buffer' contains multiple descriptors that describe memory
+	 * regions maintained by UEFI. This code records the largest free
+	 * EfiConventionalMemory region which will be used to set up the memory
+	 * allocator, so that the memory allocator can work in the largest free
+	 * continuous memory region.
+	 */
+	for (i = 0; i < total_entries * desc_size; i += desc_size) {
+		EFI_MEMORY_DESCRIPTOR *d = (EFI_MEMORY_DESCRIPTOR *)&buffer[i];
+
+		if (d->Type == EfiConventionalMemory) {
+			if (free_mem_total_pages < d->NumberOfPages) {
+				free_mem_total_pages = d->NumberOfPages;
+				efi_bootinfo->free_mem_size = free_mem_total_pages * EFI_PAGE_SIZE;
+				efi_bootinfo->free_mem_start = d->PhysicalStart;
+			}
+		}
+	}
+
+	if (efi_bootinfo->free_mem_size == 0) {
+		return EFI_OUT_OF_RESOURCES;
+	}
+
+	return EFI_SUCCESS;
+}
+
+EFI_STATUS setup_efi_pre_boot(UINTN *mapkey, efi_bootinfo_t *efi_bootinfo)
+{
+	EFI_STATUS status;
+
+	status = setup_pre_boot_memory(mapkey, efi_bootinfo);
+	if (EFI_ERROR(status)) {
+		printf("setup_pre_boot_memory() failed: ");
+		switch (status) {
+		case EFI_INCOMPATIBLE_VERSION:
+			printf("Unsupported descriptor version\n");
+			break;
+		case EFI_OUT_OF_RESOURCES:
+			printf("No free memory region\n");
+			break;
+		default:
+			printf("Unknown error\n");
+			break;
+		}
+		return status;
+	}
+
+	return EFI_SUCCESS;
+}
+
 static void setup_gdt_tss(void)
 {
 	gdt_entry_t *tss_lo, *tss_hi;
@@ -168,7 +243,7 @@ static void setup_gdt_tss(void)
 	load_gdt_tss(tss_offset);
 }
 
-void setup_efi(void)
+void setup_efi(efi_bootinfo_t *efi_bootinfo)
 {
 	reset_apic();
 	setup_gdt_tss();
@@ -178,6 +253,8 @@ void setup_efi(void)
 	enable_apic();
 	enable_x2apic();
 	smp_init();
+	phys_alloc_init(efi_bootinfo->free_mem_start,
+			efi_bootinfo->free_mem_size);
 }
 
 #endif /* TARGET_EFI */
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 07/16] x86 UEFI: Set up RSDP after UEFI boot up
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (5 preceding siblings ...)
  2021-08-18  0:08 ` [kvm-unit-tests RFC 06/16] x86 UEFI: Set up memory allocator Zixuan Wang
@ 2021-08-18  0:08 ` Zixuan Wang
  2021-08-18  0:08 ` [kvm-unit-tests RFC 08/16] x86 UEFI: Set up page tables Zixuan Wang
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:08 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

Root system description pointer (RSDP) is a data structure used in the
ACPI programming interface. In BIOS, RSDP is located within a
predefined memory area, so a program can scan the memory area and find
RSDP. But in UEFI, RSDP may not appear in that memory area, instead, a
program should find it in the EFI system table.

This commit provides RSDP set up code in UEFI:
   1. Read RSDP from EFI system table
   2. Pass RSDP pointer to find_acpi_table_attr() function

From this commit, the `x86/s3.c` test can run in UEFI and generates
similar output as in Seabios, note that:
   1. In its output, memory addresses are different than Seabios's, this
      is because EFI application starts from a dynamic runtime address,
      not a fixed predefined memory address
   2. There is a short delay (~5 secs) after the test case prints "PM1a
      event registers" line. This test case sleeps for a few seconds
      and then wakes up, so give it a few seconds to run.

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/x86/acpi.c      | 38 +++++++++++++++++++++++++++++++-------
 lib/x86/acpi.h      |  4 ++++
 lib/x86/asm/setup.h |  2 ++
 lib/x86/setup.c     | 13 +++++++++++++
 4 files changed, 50 insertions(+), 7 deletions(-)

diff --git a/lib/x86/acpi.c b/lib/x86/acpi.c
index 4373106..4b7dc60 100644
--- a/lib/x86/acpi.c
+++ b/lib/x86/acpi.c
@@ -1,9 +1,37 @@
 #include "libcflat.h"
 #include "acpi.h"
 
+#ifdef TARGET_EFI
+struct rsdp_descriptor *efi_rsdp = NULL;
+
+void setup_efi_rsdp(struct rsdp_descriptor *rsdp) {
+	efi_rsdp = rsdp;
+}
+
+static struct rsdp_descriptor *get_rsdp(void) {
+	if (efi_rsdp == NULL) {
+		printf("Can't find RSDP from UEFI, maybe setup_efi_rsdp() is not called\n");
+	}
+	return efi_rsdp;
+}
+#else
+static struct rsdp_descriptor *get_rsdp(void) {
+    struct rsdp_descriptor *rsdp;
+    unsigned long addr;
+    for(addr = 0xf0000; addr < 0x100000; addr += 16) {
+	rsdp = (void*)addr;
+	if (rsdp->signature == 0x2052545020445352LL)
+          break;
+    }
+    if (addr == 0x100000) {
+        return NULL;
+    }
+    return rsdp;
+}
+#endif /* TARGET_EFI */
+
 void* find_acpi_table_addr(u32 sig)
 {
-    unsigned long addr;
     struct rsdp_descriptor *rsdp;
     struct rsdt_descriptor_rev1 *rsdt;
     void *end;
@@ -19,12 +47,8 @@ void* find_acpi_table_addr(u32 sig)
         return (void*)(ulong)fadt->firmware_ctrl;
     }
 
-    for(addr = 0xf0000; addr < 0x100000; addr += 16) {
-	rsdp = (void*)addr;
-	if (rsdp->signature == 0x2052545020445352LL)
-          break;
-    }
-    if (addr == 0x100000) {
+    rsdp = get_rsdp();
+    if (rsdp == NULL) {
         printf("Can't find RSDP\n");
         return 0;
     }
diff --git a/lib/x86/acpi.h b/lib/x86/acpi.h
index 1b80374..da57043 100644
--- a/lib/x86/acpi.h
+++ b/lib/x86/acpi.h
@@ -101,4 +101,8 @@ struct facs_descriptor_rev1
 
 void* find_acpi_table_addr(u32 sig);
 
+#ifdef TARGET_EFI
+void setup_efi_rsdp(struct rsdp_descriptor *rsdp);
+#endif /* TARGET_EFI */
+
 #endif
diff --git a/lib/x86/asm/setup.h b/lib/x86/asm/setup.h
index c22c999..2115a75 100644
--- a/lib/x86/asm/setup.h
+++ b/lib/x86/asm/setup.h
@@ -2,6 +2,7 @@
 #define _X86_ASM_SETUP_H_
 
 #ifdef TARGET_EFI
+#include "x86/acpi.h"
 #include "x86/apic.h"
 #include "x86/smp.h"
 
@@ -19,6 +20,7 @@
 typedef struct {
 	phys_addr_t free_mem_start;
 	phys_addr_t free_mem_size;
+	struct rsdp_descriptor *rsdp;
 } efi_bootinfo_t;
 
 void setup_efi_bootinfo(efi_bootinfo_t *efi_bootinfo);
diff --git a/lib/x86/setup.c b/lib/x86/setup.c
index 0f6e376..3695c4a 100644
--- a/lib/x86/setup.c
+++ b/lib/x86/setup.c
@@ -134,6 +134,7 @@ void setup_efi_bootinfo(efi_bootinfo_t *efi_bootinfo)
 {
 	efi_bootinfo->free_mem_size = 0;
 	efi_bootinfo->free_mem_start = 0;
+	efi_bootinfo->rsdp = NULL;
 }
 
 static EFI_STATUS setup_pre_boot_memory(UINTN *mapkey, efi_bootinfo_t *efi_bootinfo)
@@ -181,6 +182,11 @@ static EFI_STATUS setup_pre_boot_memory(UINTN *mapkey, efi_bootinfo_t *efi_booti
 	return EFI_SUCCESS;
 }
 
+static EFI_STATUS setup_pre_boot_rsdp(efi_bootinfo_t *efi_bootinfo)
+{
+	return LibGetSystemConfigurationTable(&AcpiTableGuid, (VOID **)&efi_bootinfo->rsdp);
+}
+
 EFI_STATUS setup_efi_pre_boot(UINTN *mapkey, efi_bootinfo_t *efi_bootinfo)
 {
 	EFI_STATUS status;
@@ -202,6 +208,12 @@ EFI_STATUS setup_efi_pre_boot(UINTN *mapkey, efi_bootinfo_t *efi_bootinfo)
 		return status;
 	}
 
+	status = setup_pre_boot_rsdp(efi_bootinfo);
+	if (EFI_ERROR(status)) {
+		printf("Cannot find RSDP in EFI system table\n");
+		return status;
+	}
+
 	return EFI_SUCCESS;
 }
 
@@ -255,6 +267,7 @@ void setup_efi(efi_bootinfo_t *efi_bootinfo)
 	smp_init();
 	phys_alloc_init(efi_bootinfo->free_mem_start,
 			efi_bootinfo->free_mem_size);
+	setup_efi_rsdp(efi_bootinfo->rsdp);
 }
 
 #endif /* TARGET_EFI */
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 08/16] x86 UEFI: Set up page tables
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (6 preceding siblings ...)
  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 ` Zixuan Wang
  2021-08-18  0:08 ` [kvm-unit-tests RFC 09/16] x86 UEFI: Convert x86 test cases to PIC Zixuan Wang
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:08 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

UEFI sets up page tables before executing EFI application binaries.
These page tables do not allow user space code to access kernel space
memory. But `x86/syscall.c` test case places a user space function
`syscall_tf_user32` inside kernel space memory. When using UEFI page
tables, fetching this kernel memory from user space triggers a #PF
fault, which is not expected by this test case.

KVM-Unit-Tests defines page tables that allow such behavior. So the
solution to this problem is to load KVM-Unit-Tests' page tables:

   1. Copy the page table definition from `x86/cstart64.S`
   2. Update page table entries with runtime memory addresses
   3. Update CR3 register with the new page table root address

Since this commit, `x86/syscall.c` can run in UEFI and generate same
output as in Seabios, using the following command:

   ./x86/efi/run ./x86/syscall.efi --cpu Opteron_G1,vendor=AuthenticAMD

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/x86/asm/page.h   |  1 +
 lib/x86/asm/setup.h  |  3 +++
 lib/x86/setup.c      | 57 ++++++++++++++++++++++++++++++++++++++++++++
 x86/efi/efistart64.S | 21 ++++++++++++++++
 4 files changed, 82 insertions(+)

diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
index fc14160..f6f740b 100644
--- a/lib/x86/asm/page.h
+++ b/lib/x86/asm/page.h
@@ -31,6 +31,7 @@ typedef unsigned long pgd_t;
 #define PT_ACCESSED_MASK	(1ull << 5)
 #define PT_DIRTY_MASK		(1ull << 6)
 #define PT_PAGE_SIZE_MASK	(1ull << 7)
+#define PT_GLOBAL_MASK		(1ull << 8)
 #define PT64_NX_MASK		(1ull << 63)
 #define PT_ADDR_MASK		GENMASK_ULL(51, 12)
 
diff --git a/lib/x86/asm/setup.h b/lib/x86/asm/setup.h
index 2115a75..c32168e 100644
--- a/lib/x86/asm/setup.h
+++ b/lib/x86/asm/setup.h
@@ -4,7 +4,9 @@
 #ifdef TARGET_EFI
 #include "x86/acpi.h"
 #include "x86/apic.h"
+#include "x86/processor.h"
 #include "x86/smp.h"
+#include "asm/page.h"
 
 #ifdef ALIGN
 #undef ALIGN
@@ -26,6 +28,7 @@ typedef struct {
 void setup_efi_bootinfo(efi_bootinfo_t *efi_bootinfo);
 void setup_efi(efi_bootinfo_t *efi_bootinfo);
 EFI_STATUS setup_efi_pre_boot(UINTN *mapkey, efi_bootinfo_t *efi_bootinfo);
+void setup_5level_page_table(void);
 #endif /* TARGET_EFI */
 
 #endif /* _X86_ASM_SETUP_H_ */
diff --git a/lib/x86/setup.c b/lib/x86/setup.c
index 3695c4a..0b0dbea 100644
--- a/lib/x86/setup.c
+++ b/lib/x86/setup.c
@@ -217,6 +217,62 @@ EFI_STATUS setup_efi_pre_boot(UINTN *mapkey, efi_bootinfo_t *efi_bootinfo)
 	return EFI_SUCCESS;
 }
 
+/* Defined in cstart64.S or efistart64.S */
+extern phys_addr_t ptl5;
+extern phys_addr_t ptl4;
+extern phys_addr_t ptl3;
+extern phys_addr_t ptl2;
+
+static void setup_page_table(void)
+{
+	pgd_t *curr_pt;
+	phys_addr_t flags;
+	int i;
+
+	/* Set default flags */
+	flags = PT_PRESENT_MASK | PT_WRITABLE_MASK | PT_USER_MASK;
+
+	/* Level 5 */
+	curr_pt = (pgd_t *)&ptl5;
+	curr_pt[0] = ((phys_addr_t)&ptl4) | flags;
+	/* Level 4 */
+	curr_pt = (pgd_t *)&ptl4;
+	curr_pt[0] = ((phys_addr_t)&ptl3) | flags;
+	/* Level 3 */
+	curr_pt = (pgd_t *)&ptl3;
+	for (i = 0; i < 4; i++) {
+		curr_pt[i] = (((phys_addr_t)&ptl2) + i * PAGE_SIZE) | flags;
+	}
+	/* Level 2 */
+	curr_pt = (pgd_t *)&ptl2;
+	flags |= PT_ACCESSED_MASK | PT_DIRTY_MASK | PT_PAGE_SIZE_MASK | PT_GLOBAL_MASK;
+	for (i = 0; i < 4 * 512; i++)	{
+		curr_pt[i] = ((phys_addr_t)(i << 21)) | flags;
+	}
+
+	/* Load 4-level page table */
+	write_cr3((ulong)&ptl4);
+}
+
+void setup_5level_page_table(void)
+{
+	/*  Check if 5-level page table is already enabled */
+	if (read_cr4() & X86_CR4_LA57) {
+		return;
+	}
+
+	/* Disable CR4.PCIDE */
+	write_cr4(read_cr4() & ~(X86_CR4_PCIDE));
+	/* Disable CR0.PG */
+	write_cr0(read_cr0() & ~(X86_CR0_PG));
+
+	/* Load new page table */
+	write_cr3((ulong)&ptl5);
+
+	/* Enable CR4.LA57 */
+	write_cr4(read_cr4() | X86_CR4_LA57);
+}
+
 static void setup_gdt_tss(void)
 {
 	gdt_entry_t *tss_lo, *tss_hi;
@@ -268,6 +324,7 @@ void setup_efi(efi_bootinfo_t *efi_bootinfo)
 	phys_alloc_init(efi_bootinfo->free_mem_start,
 			efi_bootinfo->free_mem_size);
 	setup_efi_rsdp(efi_bootinfo->rsdp);
+	setup_page_table();
 }
 
 #endif /* TARGET_EFI */
diff --git a/x86/efi/efistart64.S b/x86/efi/efistart64.S
index cc993e5..315f8d3 100644
--- a/x86/efi/efistart64.S
+++ b/x86/efi/efistart64.S
@@ -21,6 +21,27 @@ ring0stacktop:
 
 .data
 
+.align PAGE_SIZE
+.globl ptl2
+ptl2:
+	. = . + 4 * PAGE_SIZE
+.align PAGE_SIZE
+
+.globl ptl3
+ptl3:
+	. = . + PAGE_SIZE
+.align PAGE_SIZE
+
+.globl ptl4
+ptl4:
+	. = . + PAGE_SIZE
+.align PAGE_SIZE
+
+.globl ptl5
+ptl5:
+	. = . + PAGE_SIZE
+.align PAGE_SIZE
+
 boot_idt:
 	.rept 256
 	.quad 0
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 09/16] x86 UEFI: Convert x86 test cases to PIC
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (7 preceding siblings ...)
  2021-08-18  0:08 ` [kvm-unit-tests RFC 08/16] x86 UEFI: Set up page tables Zixuan Wang
@ 2021-08-18  0:08 ` Zixuan Wang
  2021-08-18  0:08 ` [kvm-unit-tests RFC 10/16] x86 AMD SEV: Initial support Zixuan Wang
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:08 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

UEFI loads EFI applications to dynamic runtime addresses, so it requires
all applications to be compiled as PIC (position independent code). PIC
does not allow the usage of compile time absolute address.

This commit converts multiple x86 test cases to PIC so they can compile
and run in UEFI:

- x86/cet.efi

- x86/emulator.c: x86/emulator.c depends on lib/x86/usermode.c. But
usermode.c contains non-PIC inline assembly code and thus blocks the
compilation with GNU-EFI. This commit converts lib/x86/usermode.c and
x86/emulator.c to PIC, so x86/emulator.c can compile and run in UEFI.

- x86/vmware_backdoors.c: it depends on lib/x86/usermode.c and now works
without modifications

- x86/eventinj.c

- x86/smap.c

- x86/access.c

- x86/umip.c

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/x86/usermode.c  |  3 ++-
 x86/Makefile.common | 10 +++++-----
 x86/Makefile.x86_64 |  7 ++++---
 x86/access.c        |  6 +++---
 x86/cet.c           |  8 +++++---
 x86/emulator.c      |  5 +++--
 x86/eventinj.c      |  6 ++++--
 x86/smap.c          |  8 ++++----
 x86/umip.c          | 10 +++++++---
 9 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
index f032523..c550545 100644
--- a/lib/x86/usermode.c
+++ b/lib/x86/usermode.c
@@ -58,7 +58,8 @@ uint64_t run_in_user(usermode_func func, unsigned int fault_vector,
 			"pushq %[user_stack_top]\n\t"
 			"pushfq\n\t"
 			"pushq %[user_cs]\n\t"
-			"pushq $user_mode\n\t"
+			"lea user_mode(%%rip), %%rdx\n\t"
+			"pushq %%rdx\n\t"
 			"iretq\n"
 
 			"user_mode:\n\t"
diff --git a/x86/Makefile.common b/x86/Makefile.common
index 314bf47..e77de6b 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -81,16 +81,16 @@ tests-common = $(TEST_DIR)/vmexit.$(exe) $(TEST_DIR)/tsc.$(exe) \
                $(TEST_DIR)/init.$(exe) \
                $(TEST_DIR)/hyperv_synic.$(exe) $(TEST_DIR)/hyperv_stimer.$(exe) \
                $(TEST_DIR)/hyperv_connections.$(exe) \
-               $(TEST_DIR)/tsx-ctrl.$(exe)
+               $(TEST_DIR)/tsx-ctrl.$(exe) \
+	       $(TEST_DIR)/eventinj.$(exe) \
+               $(TEST_DIR)/umip.$(exe)
 
 # The following test cases are disabled when building EFI tests because they
 # use absolute addresses in their inline assembly code, which cannot compile
 # with the '-fPIC' flag
 ifneq ($(TARGET_EFI),y)
-tests-common += $(TEST_DIR)/eventinj.$(exe) \
-                $(TEST_DIR)/smap.$(exe) \
-                $(TEST_DIR)/realmode.$(exe) \
-                $(TEST_DIR)/umip.$(exe)
+tests-common += $(TEST_DIR)/smap.$(exe) \
+                $(TEST_DIR)/realmode.$(exe)
 endif
 
 test_cases: $(tests-common) $(tests)
diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
index aa23b22..7e8a57a 100644
--- a/x86/Makefile.x86_64
+++ b/x86/Makefile.x86_64
@@ -30,20 +30,21 @@ tests += $(TEST_DIR)/intel-iommu.$(exe)
 tests += $(TEST_DIR)/rdpru.$(exe)
 tests += $(TEST_DIR)/pks.$(exe)
 tests += $(TEST_DIR)/pmu_lbr.$(exe)
+tests += $(TEST_DIR)/emulator.$(exe)
+tests += $(TEST_DIR)/vmware_backdoors.$(exe)
 
 # The following test cases are disabled when building EFI tests because they
 # use absolute addresses in their inline assembly code, which cannot compile
 # with the '-fPIC' flag
 ifneq ($(TARGET_EFI),y)
 tests += $(TEST_DIR)/access.$(exe)
-tests += $(TEST_DIR)/emulator.$(exe)
 tests += $(TEST_DIR)/svm.$(exe)
 tests += $(TEST_DIR)/vmx.$(exe)
-tests += $(TEST_DIR)/vmware_backdoors.$(exe)
+endif
+
 ifneq ($(fcf_protection_full),)
 tests += $(TEST_DIR)/cet.$(exe)
 endif
-endif
 
 include $(SRCDIR)/$(TEST_DIR)/Makefile.common
 
diff --git a/x86/access.c b/x86/access.c
index 4725bbd..d0c84ca 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -700,7 +700,7 @@ static int ac_test_do_access(ac_test_t *at)
 
     if (F(AC_ACCESS_TWICE)) {
 	asm volatile (
-	    "mov $fixed2, %%rsi \n\t"
+	    "lea fixed2(%%rip), %%rsi \n\t"
 	    "mov (%[addr]), %[reg] \n\t"
 	    "fixed2:"
 	    : [reg]"=r"(r), [fault]"=a"(fault), "=b"(e)
@@ -710,7 +710,7 @@ static int ac_test_do_access(ac_test_t *at)
 	fault = 0;
     }
 
-    asm volatile ("mov $fixed1, %%rsi \n\t"
+    asm volatile ("lea fixed1(%%rip), %%rsi \n\t"
 		  "mov %%rsp, %%rdx \n\t"
 		  "cmp $0, %[user] \n\t"
 		  "jz do_access \n\t"
@@ -719,7 +719,7 @@ static int ac_test_do_access(ac_test_t *at)
 		  "pushq %[user_stack_top] \n\t"
 		  "pushfq \n\t"
 		  "pushq %[user_cs] \n\t"
-		  "pushq $do_access \n\t"
+		  "lea do_access(%%rip), %%rsi; pushq %%rsi; lea fixed1(%%rip), %%rsi\n\t"
 		  "iretq \n"
 		  "do_access: \n\t"
 		  "cmp $0, %[fetch] \n\t"
diff --git a/x86/cet.c b/x86/cet.c
index a21577a..a4b79cb 100644
--- a/x86/cet.c
+++ b/x86/cet.c
@@ -52,7 +52,7 @@ static u64 cet_ibt_func(void)
 	printf("No endbr64 instruction at jmp target, this triggers #CP...\n");
 	asm volatile ("movq $2, %rcx\n"
 		      "dec %rcx\n"
-		      "leaq 2f, %rax\n"
+		      "leaq 2f(%rip), %rax\n"
 		      "jmp *%rax \n"
 		      "2:\n"
 		      "dec %rcx\n");
@@ -67,7 +67,8 @@ void test_func(void) {
 			"pushq %[user_stack_top]\n\t"
 			"pushfq\n\t"
 			"pushq %[user_cs]\n\t"
-			"pushq $user_mode\n\t"
+			"lea user_mode(%%rip), %%rax\n\t"
+			"pushq %%rax\n\t"
 			"iretq\n"
 
 			"user_mode:\n\t"
@@ -77,7 +78,8 @@ void test_func(void) {
 			[user_ds]"i"(USER_DS),
 			[user_cs]"i"(USER_CS),
 			[user_stack_top]"r"(user_stack +
-					sizeof(user_stack)));
+					sizeof(user_stack))
+			: "rax");
 }
 
 #define SAVE_REGS() \
diff --git a/x86/emulator.c b/x86/emulator.c
index 9fda1a0..4d2de24 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -262,12 +262,13 @@ static void test_pop(void *mem)
 
 	asm volatile("mov %%rsp, %[tmp] \n\t"
 		     "mov %[stack_top], %%rsp \n\t"
-		     "push $1f \n\t"
+		     "lea 1f(%%rip), %%rax \n\t"
+		     "push %%rax \n\t"
 		     "ret \n\t"
 		     "2: jmp 2b \n\t"
 		     "1: mov %[tmp], %%rsp"
 		     : [tmp]"=&r"(tmp) : [stack_top]"r"(stack_top)
-		     : "memory");
+		     : "memory", "rax");
 	report(1, "ret");
 
 	stack_top[-1] = 0x778899;
diff --git a/x86/eventinj.c b/x86/eventinj.c
index 46593c9..0cd68e8 100644
--- a/x86/eventinj.c
+++ b/x86/eventinj.c
@@ -155,9 +155,11 @@ asm("do_iret:"
 	"pushf"W" \n\t"
 	"mov %cs, %ecx \n\t"
 	"push"W" %"R "cx \n\t"
-	"push"W" $2f \n\t"
+	"lea"W" 2f(%"R "ip), %"R "bx \n\t"
+	"push"W" %"R "bx \n\t"
 
-	"cmpb $0, no_test_device\n\t"	// see if need to flush
+	"mov no_test_device(%"R "ip), %bl \n\t"
+	"cmpb $0, %bl\n\t"		// see if need to flush
 	"jnz 1f\n\t"
 	"outl %eax, $0xe4 \n\t"		// flush page
 	"1: \n\t"
diff --git a/x86/smap.c b/x86/smap.c
index ac2c8d5..b3ee16f 100644
--- a/x86/smap.c
+++ b/x86/smap.c
@@ -161,10 +161,10 @@ int main(int ac, char **av)
 		test = -1;
 		asm("or $(" xstr(USER_BASE) "), %"R "sp \n"
 		    "push $44 \n "
-		    "decl test\n"
+		    "decl test(%"R "ip)\n"
 		    "and $~(" xstr(USER_BASE) "), %"R "sp \n"
 		    "pop %"R "ax\n"
-		    "movl %eax, test");
+		    "movl %eax, test(%"R "ip)");
 		report(pf_count == 0 && test == 44,
 		       "write to user stack with AC=1");
 
@@ -173,10 +173,10 @@ int main(int ac, char **av)
 		test = -1;
 		asm("or $(" xstr(USER_BASE) "), %"R "sp \n"
 		    "push $45 \n "
-		    "decl test\n"
+		    "decl test(%"R "ip)\n"
 		    "and $~(" xstr(USER_BASE) "), %"R "sp \n"
 		    "pop %"R "ax\n"
-		    "movl %eax, test");
+		    "movl %eax, test(%"R "ip)");
 		report(pf_count == 1 && test == 45 && save == -1,
 		       "write to user stack with AC=0");
 
diff --git a/x86/umip.c b/x86/umip.c
index c5700b3..8b4e798 100644
--- a/x86/umip.c
+++ b/x86/umip.c
@@ -23,7 +23,10 @@ static void gp_handler(struct ex_regs *regs)
 
 #define GP_ASM(stmt, in, clobber)                  \
     asm volatile (                                 \
-          "mov" W " $1f, %[expected_rip]\n\t"      \
+          "push" W " %%" R "ax\n\t"                \
+	  "lea 1f(%%" R "ip), %%" R "ax\n\t"       \
+          "mov %%" R "ax, %[expected_rip]\n\t"     \
+          "pop" W " %%" R "ax\n\t"                 \
           "movl $2f-1f, %[skip_count]\n\t"         \
           "1: " stmt "\n\t"                        \
           "2: "                                    \
@@ -130,7 +133,8 @@ static int do_ring3(void (*fn)(const char *), const char *arg)
 		  "push" W " %%" R "dx \n\t"
 		  "pushf" W "\n\t"
 		  "push" W " %[user_cs] \n\t"
-		  "push" W " $1f \n\t"
+		  "lea 1f(%%" R "ip), %%" R "dx \n\t"
+		  "push" W " %%" R "dx \n\t"
 		  "iret" W "\n"
 		  "1: \n\t"
 		  "push %%" R "cx\n\t"   /* save kernel SP */
@@ -144,7 +148,7 @@ static int do_ring3(void (*fn)(const char *), const char *arg)
 #endif
 
 		  "pop %%" R "cx\n\t"
-		  "mov $1f, %%" R "dx\n\t"
+		  "lea 1f(%%" R "ip), %%" R "dx\n\t"
 		  "int %[kernel_entry_vector]\n\t"
 		  ".section .text.entry \n\t"
 		  "kernel_entry: \n\t"
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 10/16] x86 AMD SEV: Initial support
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (8 preceding siblings ...)
  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 ` Zixuan Wang
  2021-08-18  0:09 ` [kvm-unit-tests RFC 11/16] x86 AMD SEV: Page table with c-bit Zixuan Wang
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:08 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

AMD Secure Encrypted Virtualization (SEV) is a hardware accelerated
memory encryption feature that protects guest VMs from host attacks.

This commit provides set up code and a test case for AMD SEV. The set up
code checks if SEV is supported and enabled, and then sets SEV c-bit for
each page table entry.

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
Co-developed-by: Hyunwook (Wooky) Baek <baekhw@google.com>
---
 configure           |  9 +++++++
 lib/x86/amd_sev.c   | 50 +++++++++++++++++++++++++++++++++++
 lib/x86/amd_sev.h   | 41 +++++++++++++++++++++++++++++
 lib/x86/asm/setup.h |  3 +++
 lib/x86/setup.c     | 24 +++++++++++++++++
 x86/Makefile.common | 10 +++++++
 x86/Makefile.x86_64 |  3 +++
 x86/amd_sev.c       | 64 +++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 204 insertions(+)
 create mode 100644 lib/x86/amd_sev.c
 create mode 100644 lib/x86/amd_sev.h
 create mode 100644 x86/amd_sev.c

diff --git a/configure b/configure
index 3094375..c165ba7 100755
--- a/configure
+++ b/configure
@@ -31,6 +31,8 @@ earlycon=
 target_efi=
 efi_include_path=
 efi_libs_path=
+amd_sev=
+amd_sev_variant=
 
 usage() {
     cat <<-EOF
@@ -77,6 +79,7 @@ usage() {
 	    --efi-libs-path        Path to GNU-EFI libraries, e.g. "/usr/lib/". This dir should
 	                           contain 3 files from GNU-EFI: crt0-efi-x86_64.o, libefi.a,
 	                           and libgnuefi.a
+	    --amd-sev=VARIANT      AMD SEV variant, [SEV|ES|SNP]
 EOF
     exit 1
 }
@@ -150,6 +153,10 @@ while [[ "$1" = -* ]]; do
 	--efi-libs-path)
 	    efi_libs_path="$arg"
 	    ;;
+	--amd-sev)
+	    amd_sev=y
+	    [ "${arg^^}" != "SEV" ] && amd_sev_variant="${arg^^}"
+	    ;;
 	--help)
 	    usage
 	    ;;
@@ -361,6 +368,8 @@ HOST_KEY_DOCUMENT=$host_key_document
 TARGET_EFI=$target_efi
 EFI_INCLUDE_PATH=$efi_include_path
 EFI_LIBS_PATH=$efi_libs_path
+AMD_SEV=$amd_sev
+AMD_SEV_VARIANT=$amd_sev_variant
 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
new file mode 100644
index 0000000..bd8d536
--- /dev/null
+++ b/lib/x86/amd_sev.c
@@ -0,0 +1,50 @@
+/*
+ * AMD SEV support in KVM-Unit-Tests
+ *
+ * Copyright (c) 2021, Google Inc
+ *
+ * Authors:
+ *   Zixuan Wang <zixuanwang@google.com>
+ *
+ * SPDX-License-Identifier: LGPL-2.0-or-later
+ */
+
+#include "amd_sev.h"
+#include "x86/processor.h"
+
+static unsigned long long amd_sev_c_bit_pos;
+
+EFI_STATUS setup_amd_sev(void)
+{
+	struct cpuid cpuid_out;
+
+	/* Test if we can query SEV features */
+	cpuid_out = cpuid(CPUID_FN_LARGEST_EXT_FUNC_NUM);
+	if (cpuid_out.a < CPUID_FN_ENCRYPT_MEM_CAPAB) {
+		return EFI_UNSUPPORTED;
+	}
+
+	/* Test if SEV is supported */
+	cpuid_out = cpuid(CPUID_FN_ENCRYPT_MEM_CAPAB);
+	if (!(cpuid_out.a & SEV_SUPPORT_MASK)) {
+		return EFI_UNSUPPORTED;
+	}
+
+	/* Test if SEV is enabled */
+	if (!(rdmsr(MSR_SEV_STATUS) & SEV_ENABLED_MASK)) {
+		return EFI_NOT_READY;
+	}
+
+	/* Extract C-Bit position from ebx[5:0]
+	 * AMD64 Architecture Programmer's Manual Volume 3
+	 *   - Section " Function 8000_001Fh - Encrypted Memory Capabilities"
+	 */
+	amd_sev_c_bit_pos = (unsigned long long)(cpuid_out.b & 0x3f);
+
+	return EFI_SUCCESS;
+}
+
+unsigned long long get_amd_sev_c_bit_mask(void)
+{
+	return 1ull << amd_sev_c_bit_pos;
+}
diff --git a/lib/x86/amd_sev.h b/lib/x86/amd_sev.h
new file mode 100644
index 0000000..c1b08e8
--- /dev/null
+++ b/lib/x86/amd_sev.h
@@ -0,0 +1,41 @@
+/*
+ * AMD SEV support in KVM-Unit-Tests
+ *
+ * Copyright (c) 2021, Google Inc
+ *
+ * Authors:
+ *   Zixuan Wang <zixuanwang@google.com>
+ *
+ * SPDX-License-Identifier: LGPL-2.0-or-later
+ */
+
+#ifndef _X86_AMD_SEV_H_
+#define _X86_AMD_SEV_H_
+
+#include "libcflat.h"
+#include "desc.h"
+#include "asm/page.h"
+
+#ifdef ALIGN
+#undef ALIGN
+#endif
+#include <efi.h>
+
+/* AMD Programmer's Manual Volume 3
+ *   - Section "Function 8000_0000h - Maximum Extended Function Number and Vendor String"
+ *   - Section "Function 8000_001Fh - Encrypted Memory Capabilities"
+ */
+#define CPUID_FN_LARGEST_EXT_FUNC_NUM 0x80000000
+#define CPUID_FN_ENCRYPT_MEM_CAPAB    0x8000001f
+#define SEV_SUPPORT_MASK              0b10
+
+/* AMD Programmer's Manual Volume 2
+ *   - Section "SEV_STATUS MSR"
+ */
+#define MSR_SEV_STATUS   0xc0010131
+#define SEV_ENABLED_MASK 0b1
+
+EFI_STATUS setup_amd_sev(void);
+unsigned long long get_amd_sev_c_bit_mask(void);
+
+#endif /* _X86_AMD_SEV_H_ */
diff --git a/lib/x86/asm/setup.h b/lib/x86/asm/setup.h
index c32168e..1d69c6e 100644
--- a/lib/x86/asm/setup.h
+++ b/lib/x86/asm/setup.h
@@ -7,6 +7,9 @@
 #include "x86/processor.h"
 #include "x86/smp.h"
 #include "asm/page.h"
+#ifdef CONFIG_AMD_SEV
+#include "x86/amd_sev.h"
+#endif /* CONFIG_AMD_SEV */
 
 #ifdef ALIGN
 #undef ALIGN
diff --git a/lib/x86/setup.c b/lib/x86/setup.c
index 0b0dbea..aaa1cce 100644
--- a/lib/x86/setup.c
+++ b/lib/x86/setup.c
@@ -214,6 +214,25 @@ EFI_STATUS setup_efi_pre_boot(UINTN *mapkey, efi_bootinfo_t *efi_bootinfo)
 		return status;
 	}
 
+#ifdef CONFIG_AMD_SEV
+	status = setup_amd_sev();
+	if (EFI_ERROR(status)) {
+		printf("setup_amd_sev() failed: ");
+		switch (status) {
+		case EFI_UNSUPPORTED:
+			printf("SEV is not supported\n");
+			break;
+		case EFI_NOT_READY:
+			printf("SEV is not enabled\n");
+			break;
+		default:
+			printf("Unknown error\n");
+			break;
+		}
+		return status;
+	}
+#endif /* CONFIG_AMD_SEV */
+
 	return EFI_SUCCESS;
 }
 
@@ -232,6 +251,11 @@ static void setup_page_table(void)
 	/* Set default flags */
 	flags = PT_PRESENT_MASK | PT_WRITABLE_MASK | PT_USER_MASK;
 
+#ifdef CONFIG_AMD_SEV
+	/* Set AMD SEV C-Bit for page table entries */
+	flags |= get_amd_sev_c_bit_mask();
+#endif /* CONFIG_AMD_SEV */
+
 	/* Level 5 */
 	curr_pt = (pgd_t *)&ptl5;
 	curr_pt[0] = ((phys_addr_t)&ptl4) | flags;
diff --git a/x86/Makefile.common b/x86/Makefile.common
index e77de6b..8f9ddad 100644
--- a/x86/Makefile.common
+++ b/x86/Makefile.common
@@ -25,6 +25,9 @@ cflatobjs += lib/x86/delay.o
 ifeq ($(TARGET_EFI),y)
 cflatobjs += lib/x86/setup.o
 cflatobjs += lib/efi.o
+ifeq ($(AMD_SEV),y)
+cflatobjs += lib/x86/amd_sev.o
+endif
 endif
 
 OBJDIRS += lib/x86
@@ -38,6 +41,13 @@ COMMON_CFLAGS += -Wa,--divide
 endif
 COMMON_CFLAGS += -O1
 
+ifeq ($(AMD_SEV),y)
+COMMON_CFLAGS += -DCONFIG_AMD_SEV
+ifneq ($(AMD_SEV_VARIANT),)
+COMMON_CFLAGS += -DCONFIG_AMD_SEV_$(AMD_SEV_VARIANT)
+endif
+endif
+
 # stack.o relies on frame pointers.
 KEEP_FRAME_POINTER := y
 
diff --git a/x86/Makefile.x86_64 b/x86/Makefile.x86_64
index 7e8a57a..c1af80c 100644
--- a/x86/Makefile.x86_64
+++ b/x86/Makefile.x86_64
@@ -32,6 +32,9 @@ tests += $(TEST_DIR)/pks.$(exe)
 tests += $(TEST_DIR)/pmu_lbr.$(exe)
 tests += $(TEST_DIR)/emulator.$(exe)
 tests += $(TEST_DIR)/vmware_backdoors.$(exe)
+ifeq ($(AMD_SEV),y)
+tests += $(TEST_DIR)/amd_sev.$(exe)
+endif
 
 # The following test cases are disabled when building EFI tests because they
 # use absolute addresses in their inline assembly code, which cannot compile
diff --git a/x86/amd_sev.c b/x86/amd_sev.c
new file mode 100644
index 0000000..a07a48f
--- /dev/null
+++ b/x86/amd_sev.c
@@ -0,0 +1,64 @@
+/*
+ * AMD SEV test cases
+ *
+ * Copyright (c) 2021, Google Inc
+ *
+ * Authors:
+ *   Hyunwook (Wooky) Baek <baekhw@google.com>
+ *   Zixuan Wang <zixuanwang@google.com>
+ *
+ * SPDX-License-Identifier: LGPL-2.0-or-later
+ */
+
+#include "libcflat.h"
+#include "x86/processor.h"
+#include "x86/amd_sev.h"
+
+#define EXIT_SUCCESS 0
+#define EXIT_FAILURE 1
+
+static int test_sev_activation(void)
+{
+	struct cpuid cpuid_out;
+	u64 msr_out;
+
+	printf("SEV activation test is loaded.\n");
+
+	/* Tests if CPUID function to check SEV is implemented */
+	cpuid_out = cpuid(CPUID_FN_LARGEST_EXT_FUNC_NUM);
+	printf("CPUID Fn8000_0000[EAX]: 0x%08x\n", cpuid_out.a);
+	if (cpuid_out.a < CPUID_FN_ENCRYPT_MEM_CAPAB) {
+		printf("CPUID does not support FN%08x\n",
+		       CPUID_FN_ENCRYPT_MEM_CAPAB);
+		return EXIT_FAILURE;
+	}
+
+	/* Tests if SEV is supported */
+	cpuid_out = cpuid(CPUID_FN_ENCRYPT_MEM_CAPAB);
+	printf("CPUID Fn8000_001F[EAX]: 0x%08x\n", cpuid_out.a);
+	printf("CPUID Fn8000_001F[EBX]: 0x%08x\n", cpuid_out.b);
+	if (!(cpuid_out.a & SEV_SUPPORT_MASK)) {
+		printf("SEV is not supported.\n");
+		return EXIT_FAILURE;
+	}
+	printf("SEV is supported\n");
+
+	/* Tests if SEV is enabled */
+	msr_out = rdmsr(MSR_SEV_STATUS);
+	printf("MSR C001_0131[EAX]: 0x%08lx\n", msr_out & 0xffffffff);
+	if (!(msr_out & SEV_ENABLED_MASK)) {
+		printf("SEV is not enabled.\n");
+		return EXIT_FAILURE;
+	}
+	printf("SEV is enabled\n");
+
+	return EXIT_SUCCESS;
+}
+
+int main(void)
+{
+	int rtn;
+	rtn = test_sev_activation();
+	report(rtn == EXIT_SUCCESS, "SEV activation test.");
+	return report_summary();
+}
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 11/16] x86 AMD SEV: Page table with c-bit
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (9 preceding siblings ...)
  2021-08-18  0:08 ` [kvm-unit-tests RFC 10/16] x86 AMD SEV: Initial support Zixuan Wang
@ 2021-08-18  0:09 ` Zixuan Wang
  2021-08-18  0:09 ` [kvm-unit-tests RFC 12/16] x86 AMD SEV-ES: Check SEV-ES status Zixuan Wang
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:09 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

AMD SEV introduces c-bit to page table entries. To work with AMD SEV:

   1. c-bit should be set for new page table entries
   2. address calculation should not use c-bit as part of address

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/x86/amd_sev.c  |  5 +++++
 lib/x86/amd_sev.h  |  1 +
 lib/x86/asm/page.h | 13 +++++++++++--
 lib/x86/vm.c       | 18 ++++++++++++++----
 4 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/lib/x86/amd_sev.c b/lib/x86/amd_sev.c
index bd8d536..535f0e8 100644
--- a/lib/x86/amd_sev.c
+++ b/lib/x86/amd_sev.c
@@ -48,3 +48,8 @@ unsigned long long get_amd_sev_c_bit_mask(void)
 {
 	return 1ull << amd_sev_c_bit_pos;
 }
+
+unsigned long long get_amd_sev_c_bit_pos(void)
+{
+	return amd_sev_c_bit_pos;
+}
diff --git a/lib/x86/amd_sev.h b/lib/x86/amd_sev.h
index c1b08e8..e1ef777 100644
--- a/lib/x86/amd_sev.h
+++ b/lib/x86/amd_sev.h
@@ -37,5 +37,6 @@
 
 EFI_STATUS setup_amd_sev(void);
 unsigned long long get_amd_sev_c_bit_mask(void);
+unsigned long long get_amd_sev_c_bit_pos(void);
 
 #endif /* _X86_AMD_SEV_H_ */
diff --git a/lib/x86/asm/page.h b/lib/x86/asm/page.h
index f6f740b..c49b259 100644
--- a/lib/x86/asm/page.h
+++ b/lib/x86/asm/page.h
@@ -33,10 +33,19 @@ typedef unsigned long pgd_t;
 #define PT_PAGE_SIZE_MASK	(1ull << 7)
 #define PT_GLOBAL_MASK		(1ull << 8)
 #define PT64_NX_MASK		(1ull << 63)
-#define PT_ADDR_MASK		GENMASK_ULL(51, 12)
+#ifndef CONFIG_AMD_SEV
+#define PT_ADDR_UPPER_BOUND	(51)
+#else
+/* lib/x86/amd_sev.c */
+extern unsigned long long get_amd_sev_c_bit_mask(void);
+extern unsigned long long get_amd_sev_c_bit_pos(void);
+#define PT_ADDR_UPPER_BOUND	(get_amd_sev_c_bit_pos()-1)
+#endif /* CONFIG_AMD_SEV */
+#define PT_ADDR_LOWER_BOUND	(PAGE_SHIFT)
+#define PT_ADDR_MASK		GENMASK_ULL(PT_ADDR_UPPER_BOUND, PT_ADDR_LOWER_BOUND)
 
 #define PDPTE64_PAGE_SIZE_MASK	  (1ull << 7)
-#define PDPTE64_RSVD_MASK	  GENMASK_ULL(51, cpuid_maxphyaddr())
+#define PDPTE64_RSVD_MASK	  GENMASK_ULL(PT_ADDR_UPPER_BOUND, cpuid_maxphyaddr())
 
 #define PT_AD_MASK              (PT_ACCESSED_MASK | PT_DIRTY_MASK)
 
diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index 5cd2ee4..b482b87 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -26,6 +26,9 @@ pteval_t *install_pte(pgd_t *cr3,
                 pt_page = 0;
 	    memset(new_pt, 0, PAGE_SIZE);
 	    pt[offset] = virt_to_phys(new_pt) | PT_PRESENT_MASK | PT_WRITABLE_MASK | pte_opt_mask;
+#ifdef CONFIG_AMD_SEV
+	    pt[offset] |= get_amd_sev_c_bit_mask();
+#endif /* CONFIG_AMD_SEV */
 	}
 	pt = phys_to_virt(pt[offset] & PT_ADDR_MASK);
     }
@@ -63,7 +66,7 @@ struct pte_search find_pte_level(pgd_t *cr3, void *virt,
 		if (r.level == lowest_level)
 			return r;
 
-		pt = phys_to_virt(pte & 0xffffffffff000ull);
+		pt = phys_to_virt(pte & PT_ADDR_MASK);
 	}
 }
 
@@ -94,13 +97,20 @@ pteval_t *get_pte_level(pgd_t *cr3, void *virt, int pte_level)
 
 pteval_t *install_large_page(pgd_t *cr3, phys_addr_t phys, void *virt)
 {
-    return install_pte(cr3, 2, virt,
-		       phys | PT_PRESENT_MASK | PT_WRITABLE_MASK | pte_opt_mask | PT_PAGE_SIZE_MASK, 0);
+    phys_addr_t flags = PT_PRESENT_MASK | PT_WRITABLE_MASK | pte_opt_mask | PT_PAGE_SIZE_MASK;
+#ifdef CONFIG_AMD_SEV
+    flags |= get_amd_sev_c_bit_mask();
+#endif /* CONFIG_AMD_SEV */
+    return install_pte(cr3, 2, virt, phys | flags, 0);
 }
 
 pteval_t *install_page(pgd_t *cr3, phys_addr_t phys, void *virt)
 {
-    return install_pte(cr3, 1, virt, phys | PT_PRESENT_MASK | PT_WRITABLE_MASK | pte_opt_mask, 0);
+    phys_addr_t flags = PT_PRESENT_MASK | PT_WRITABLE_MASK | pte_opt_mask;
+#ifdef CONFIG_AMD_SEV
+    flags |= get_amd_sev_c_bit_mask();
+#endif /* CONFIG_AMD_SEV */
+    return install_pte(cr3, 1, virt, phys | flags, 0);
 }
 
 void install_pages(pgd_t *cr3, phys_addr_t phys, size_t len, void *virt)
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 12/16] x86 AMD SEV-ES: Check SEV-ES status
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (10 preceding siblings ...)
  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 ` Zixuan Wang
  2021-08-18  0:09 ` [kvm-unit-tests RFC 13/16] x86 AMD SEV-ES: Load GDT with UEFI segments Zixuan Wang
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:09 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

This commit provides initial start up code for KVM-Unit-Tests to run in
an SEV-ES guest VM. This start up code checks if SEV-ES feature is
supported and enabled for the guest.

In this commit, KVM-Unit-Tests can pass the SEV-ES check and enter
setup_efi() function, but crashes in setup_gdt_tss(), which will be
fixed by follow-up commits.

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/x86/amd_sev.c | 11 +++++++++++
 lib/x86/amd_sev.h |  9 +++++++--
 lib/x86/setup.c   | 16 ++++++++++++++++
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/lib/x86/amd_sev.c b/lib/x86/amd_sev.c
index 535f0e8..a31d352 100644
--- a/lib/x86/amd_sev.c
+++ b/lib/x86/amd_sev.c
@@ -44,6 +44,17 @@ EFI_STATUS setup_amd_sev(void)
 	return EFI_SUCCESS;
 }
 
+#ifdef CONFIG_AMD_SEV_ES
+EFI_STATUS setup_amd_sev_es(void){
+	/* Test if SEV-ES is enabled */
+	if (!(rdmsr(MSR_SEV_STATUS) & SEV_ES_ENABLED_MASK)) {
+		return EFI_UNSUPPORTED;
+	}
+
+	return EFI_SUCCESS;
+}
+#endif /* CONFIG_AMD_SEV_ES */
+
 unsigned long long get_amd_sev_c_bit_mask(void)
 {
 	return 1ull << amd_sev_c_bit_pos;
diff --git a/lib/x86/amd_sev.h b/lib/x86/amd_sev.h
index e1ef777..a2eccfc 100644
--- a/lib/x86/amd_sev.h
+++ b/lib/x86/amd_sev.h
@@ -32,10 +32,15 @@
 /* AMD Programmer's Manual Volume 2
  *   - Section "SEV_STATUS MSR"
  */
-#define MSR_SEV_STATUS   0xc0010131
-#define SEV_ENABLED_MASK 0b1
+#define MSR_SEV_STATUS      0xc0010131
+#define SEV_ENABLED_MASK    0b1
+#define SEV_ES_ENABLED_MASK 0b10
 
 EFI_STATUS setup_amd_sev(void);
+#ifdef CONFIG_AMD_SEV_ES
+EFI_STATUS setup_amd_sev_es(void);
+#endif /* CONFIG_AMD_SEV_ES */
+
 unsigned long long get_amd_sev_c_bit_mask(void);
 unsigned long long get_amd_sev_c_bit_pos(void);
 
diff --git a/lib/x86/setup.c b/lib/x86/setup.c
index aaa1cce..d29f415 100644
--- a/lib/x86/setup.c
+++ b/lib/x86/setup.c
@@ -231,6 +231,22 @@ EFI_STATUS setup_efi_pre_boot(UINTN *mapkey, efi_bootinfo_t *efi_bootinfo)
 		}
 		return status;
 	}
+
+#ifdef CONFIG_AMD_SEV_ES
+	status = setup_amd_sev_es();
+	if (EFI_ERROR(status)) {
+		printf("setup_amd_sev_es() failed: ");
+		switch (status) {
+		case EFI_UNSUPPORTED:
+			printf("SEV-ES is not supported\n");
+			break;
+		default:
+			printf("Unknown error\n");
+			break;
+		}
+		return status;
+	}
+#endif /* CONFIG_AMD_SEV_ES */
 #endif /* CONFIG_AMD_SEV */
 
 	return EFI_SUCCESS;
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 13/16] x86 AMD SEV-ES: Load GDT with UEFI segments
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (11 preceding siblings ...)
  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 ` Zixuan Wang
  2021-08-18  0:09 ` [kvm-unit-tests RFC 14/16] x86 AMD SEV-ES: Copy UEFI #VC IDT entry Zixuan Wang
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:09 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

Before this commit, KVM-Unit-Tests set up process crashes when executing
'lgdt' instruction under SEV-ES. This is because lgdt triggers UEFI
procedures (e.g. UEFI #VC handler) that require UEFI's code and data
segments. But these segments are are not compatible with KVM-Unit-Tests
GDT:

UEFI uses 0x30 as code segment and 0x38 as data segment, but in
KVM-Unit-Tests' GDT, 0x30 is a data segment, and 0x38 is a code segment.
This discrepancy crashes the UEFI procedures and thus crashes the 'lgdt'
execution.

This commit fixes this issue by copying UEFI GDT's code and data
segments into KVM-Unit-Tests GDT, so that UEFI procedures (e.g. UEFI #VC
handler) can work.

In this commit, the guest VM passes setup_gdt_tss() but crashes in
load_idt(), which will be fixed by follow-up commits.

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/x86/amd_sev.c | 36 ++++++++++++++++++++++++++++++++++++
 lib/x86/amd_sev.h |  1 +
 lib/x86/setup.c   |  4 ++++
 3 files changed, 41 insertions(+)

diff --git a/lib/x86/amd_sev.c b/lib/x86/amd_sev.c
index a31d352..c2aebdf 100644
--- a/lib/x86/amd_sev.c
+++ b/lib/x86/amd_sev.c
@@ -53,6 +53,42 @@ EFI_STATUS setup_amd_sev_es(void){
 
 	return EFI_SUCCESS;
 }
+
+static void copy_gdt_entry(gdt_entry_t *dst, gdt_entry_t *src, unsigned segment)
+{
+	unsigned index;
+
+	index = segment / sizeof(gdt_entry_t);
+	dst[index] = src[index];
+}
+
+/* Defined in x86/efi/efistart64.S */
+extern gdt_entry_t gdt64[];
+
+/* Copy UEFI's code and data segments to KVM-Unit-Tests GDT.
+ *
+ * This is because KVM-Unit-Tests reuses UEFI #VC handler that requires UEFI
+ * code and data segments to run. The UEFI #VC handler crashes the guest VM if
+ * these segments are not available. So we need to copy these two UEFI segments
+ * into KVM-Unit-Tests GDT.
+ *
+ * UEFI uses 0x30 as code segment and 0x38 as data segment. Fortunately, these
+ * segments can be safely overridden in KVM-Unit-Tests as they are used as
+ * protected mode and real mode segments (see x86/efi/efistart64.S for more
+ * details), which are not used in EFI set up process.
+ */
+void copy_uefi_segments(void)
+{
+	/* GDT and GDTR in current UEFI */
+	gdt_entry_t *gdt_curr;
+	struct descriptor_table_ptr gdtr_curr;
+
+	/* Copy code and data segments from UEFI */
+	sgdt(&gdtr_curr);
+	gdt_curr = (gdt_entry_t *)gdtr_curr.base;
+	copy_gdt_entry(gdt64, gdt_curr, read_cs());
+	copy_gdt_entry(gdt64, gdt_curr, read_ds());
+}
 #endif /* CONFIG_AMD_SEV_ES */
 
 unsigned long long get_amd_sev_c_bit_mask(void)
diff --git a/lib/x86/amd_sev.h b/lib/x86/amd_sev.h
index a2eccfc..4d81cae 100644
--- a/lib/x86/amd_sev.h
+++ b/lib/x86/amd_sev.h
@@ -39,6 +39,7 @@
 EFI_STATUS setup_amd_sev(void);
 #ifdef CONFIG_AMD_SEV_ES
 EFI_STATUS setup_amd_sev_es(void);
+void copy_uefi_segments(void);
 #endif /* CONFIG_AMD_SEV_ES */
 
 unsigned long long get_amd_sev_c_bit_mask(void);
diff --git a/lib/x86/setup.c b/lib/x86/setup.c
index d29f415..d828638 100644
--- a/lib/x86/setup.c
+++ b/lib/x86/setup.c
@@ -348,6 +348,10 @@ static void setup_gdt_tss(void)
 	tss_hi->limit_low = (u16)((curr_tss_addr >> 32) & 0xffff);
 	tss_hi->base_low = (u16)((curr_tss_addr >> 48) & 0xffff);
 
+#ifdef CONFIG_AMD_SEV_ES
+	copy_uefi_segments();
+#endif /* CONFIG_AMD_SEV_ES */
+
 	load_gdt_tss(tss_offset);
 }
 
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 14/16] x86 AMD SEV-ES: Copy UEFI #VC IDT entry
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (12 preceding siblings ...)
  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 ` Zixuan Wang
  2021-08-20 23:50   ` Sean Christopherson
  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
  15 siblings, 1 reply; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:09 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

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.

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/x86/amd_sev.c | 10 ++++++++++
 lib/x86/amd_sev.h |  5 +++++
 2 files changed, 15 insertions(+)

diff --git a/lib/x86/amd_sev.c b/lib/x86/amd_sev.c
index c2aebdf..04b6912 100644
--- a/lib/x86/amd_sev.c
+++ b/lib/x86/amd_sev.c
@@ -46,11 +46,21 @@ EFI_STATUS setup_amd_sev(void)
 
 #ifdef CONFIG_AMD_SEV_ES
 EFI_STATUS setup_amd_sev_es(void){
+	struct descriptor_table_ptr idtr;
+	idt_entry_t *idt;
+
 	/* Test if SEV-ES is enabled */
 	if (!(rdmsr(MSR_SEV_STATUS) & SEV_ES_ENABLED_MASK)) {
 		return EFI_UNSUPPORTED;
 	}
 
+	/* Copy UEFI's #VC IDT entry, so KVM-Unit-Tests can reuse it and does
+	 * not have to re-implement a #VC handler
+	 */
+	sidt(&idtr);
+	idt = (idt_entry_t *)idtr.base;
+	boot_idt[SEV_ES_VC_HANDLER_VECTOR] = idt[SEV_ES_VC_HANDLER_VECTOR];
+
 	return EFI_SUCCESS;
 }
 
diff --git a/lib/x86/amd_sev.h b/lib/x86/amd_sev.h
index 4d81cae..5ebd4a6 100644
--- a/lib/x86/amd_sev.h
+++ b/lib/x86/amd_sev.h
@@ -36,6 +36,11 @@
 #define SEV_ENABLED_MASK    0b1
 #define SEV_ES_ENABLED_MASK 0b10
 
+/* AMD Programmer's Manual Volume 2
+ *   - Section "#VC Exception"
+ */
+#define SEV_ES_VC_HANDLER_VECTOR 29
+
 EFI_STATUS setup_amd_sev(void);
 #ifdef CONFIG_AMD_SEV_ES
 EFI_STATUS setup_amd_sev_es(void);
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 15/16] x86 AMD SEV-ES: Set up GHCB page
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (13 preceding siblings ...)
  2021-08-18  0:09 ` [kvm-unit-tests RFC 14/16] x86 AMD SEV-ES: Copy UEFI #VC IDT entry Zixuan Wang
@ 2021-08-18  0:09 ` Zixuan Wang
  2021-08-18  0:09 ` [kvm-unit-tests RFC 16/16] x86 AMD SEV-ES: Add test cases Zixuan Wang
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:09 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

AMD SEV-ES introduces a GHCB page for guest/host communication. This
page should be unencrypted, i.e. its c-bit should be unset, otherwise
the guest VM may crash when #VC exception happens.

By default, KVM-Unit-Tests only sets up 2MiB pages, i.e. only Level 2
page table entries are provided. Unsetting GHCB Level 2 pte's c-bit
still crashes the guest VM. The solution is to unset only its Level 1
pte's c-bit.

This commit provides GHCB page set up code that:

   1. finds GHCB Level 1 pte
   2. if not found, installs corresponding Level 1 pages
   3. unsets GHCB Level 1 pte's c-bit

In this commit, KVM-Unit-Tests can run in an SEV-ES VM and boot into
test cases' main().

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 lib/x86/amd_sev.c | 35 +++++++++++++++++++++++++++++++++++
 lib/x86/amd_sev.h |  6 ++++++
 lib/x86/setup.c   |  4 ++++
 3 files changed, 45 insertions(+)

diff --git a/lib/x86/amd_sev.c b/lib/x86/amd_sev.c
index 04b6912..7f6265a 100644
--- a/lib/x86/amd_sev.c
+++ b/lib/x86/amd_sev.c
@@ -11,6 +11,7 @@
 
 #include "amd_sev.h"
 #include "x86/processor.h"
+#include "x86/vm.h"
 
 static unsigned long long amd_sev_c_bit_pos;
 
@@ -64,6 +65,40 @@ EFI_STATUS setup_amd_sev_es(void){
 	return EFI_SUCCESS;
 }
 
+void setup_ghcb_pte(pgd_t *page_table)
+{
+	/* SEV-ES guest uses GHCB page to communicate with host. This page must
+	 * be unencrypted, i.e. its c-bit should be unset.
+	 */
+	phys_addr_t ghcb_addr, ghcb_base_addr;
+	pteval_t *pte;
+
+	/* Read the current GHCB page addr */
+	ghcb_addr = rdmsr(SEV_ES_GHCB_MSR_INDEX);
+
+	/* Find Level 1 page table entry for GHCB page */
+	pte = get_pte_level(page_table, (void *)ghcb_addr, 1);
+
+	/* Create Level 1 pte for GHCB page if not found */
+	if (pte == NULL) {
+		/* Find Level 2 page base address */
+		ghcb_base_addr = ghcb_addr & ~(LARGE_PAGE_SIZE-1);
+		/* Install Level 1 ptes */
+		install_pages(page_table, ghcb_base_addr, LARGE_PAGE_SIZE,
+			      (void *)ghcb_base_addr);
+		/* Find Level 2 pte, set as 4KB pages */
+		pte = get_pte_level(page_table, (void *)ghcb_addr, 2);
+		assert(pte);
+		*pte &= ~(PT_PAGE_SIZE_MASK);
+		/* Find Level 1 GHCB pte */
+		pte = get_pte_level(page_table, (void *)ghcb_addr, 1);
+		assert(pte);
+	}
+
+	/* Unset c-bit in Level 1 GHCB pte */
+	*pte &= ~(get_amd_sev_c_bit_mask());
+}
+
 static void copy_gdt_entry(gdt_entry_t *dst, gdt_entry_t *src, unsigned segment)
 {
 	unsigned index;
diff --git a/lib/x86/amd_sev.h b/lib/x86/amd_sev.h
index 5ebd4a6..2f08cdb 100644
--- a/lib/x86/amd_sev.h
+++ b/lib/x86/amd_sev.h
@@ -41,9 +41,15 @@
  */
 #define SEV_ES_VC_HANDLER_VECTOR 29
 
+/* AMD Programmer's Manual Volume 2
+ *   - Section "GHCB"
+ */
+#define SEV_ES_GHCB_MSR_INDEX 0xc0010130
+
 EFI_STATUS setup_amd_sev(void);
 #ifdef CONFIG_AMD_SEV_ES
 EFI_STATUS setup_amd_sev_es(void);
+void setup_ghcb_pte(pgd_t *page_table);
 void copy_uefi_segments(void);
 #endif /* CONFIG_AMD_SEV_ES */
 
diff --git a/lib/x86/setup.c b/lib/x86/setup.c
index d828638..d782c39 100644
--- a/lib/x86/setup.c
+++ b/lib/x86/setup.c
@@ -290,6 +290,10 @@ static void setup_page_table(void)
 		curr_pt[i] = ((phys_addr_t)(i << 21)) | flags;
 	}
 
+#ifdef CONFIG_AMD_SEV_ES
+	setup_ghcb_pte((pgd_t *)&ptl4);
+#endif /* CONFIG_AMD_SEV_ES */
+
 	/* Load 4-level page table */
 	write_cr3((ulong)&ptl4);
 }
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [kvm-unit-tests RFC 16/16] x86 AMD SEV-ES: Add test cases
  2021-08-18  0:08 [kvm-unit-tests RFC 00/16] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
                   ` (14 preceding siblings ...)
  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 ` Zixuan Wang
  15 siblings, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-18  0:09 UTC (permalink / raw)
  To: kvm, pbonzini, drjones
  Cc: marcorr, baekhw, tmroeder, erdemaktas, rientjes, seanjc,
	brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp,
	Zixuan Wang

SEV-ES introduces #VC handler for guest/host communications, e.g.,
accessing MSR, executing CPUID. This commit provides test cases to check
if SEV-ES is enabled and if rdmsr/wrmsr are handled correctly in SEV-ES.

Signed-off-by: Zixuan Wang <zixuanwang@google.com>
---
 x86/amd_sev.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/x86/amd_sev.c b/x86/amd_sev.c
index a07a48f..660196e 100644
--- a/x86/amd_sev.c
+++ b/x86/amd_sev.c
@@ -13,6 +13,7 @@
 #include "libcflat.h"
 #include "x86/processor.h"
 #include "x86/amd_sev.h"
+#include "msr.h"
 
 #define EXIT_SUCCESS 0
 #define EXIT_FAILURE 1
@@ -55,10 +56,42 @@ static int test_sev_activation(void)
 	return EXIT_SUCCESS;
 }
 
+#ifdef CONFIG_AMD_SEV_ES
+static int test_sev_es_activation(void)
+{
+	if (!(rdmsr(MSR_SEV_STATUS) & SEV_ES_ENABLED_MASK)) {
+		return EXIT_FAILURE;
+	}
+
+	return EXIT_SUCCESS;
+}
+
+static int test_sev_es_msr(void)
+{
+	/* With SEV-ES, rdmsr/wrmsr trigger #VC exception. If #VC is handled
+	 * correctly, rdmsr/wrmsr should work like without SEV-ES and not crash
+	 * the guest VM.
+	 */
+	u64 val = 0x1234;
+	wrmsr(MSR_TSC_AUX, val);
+	if(val != rdmsr(MSR_TSC_AUX)) {
+		return EXIT_FAILURE;
+	}
+
+	return EXIT_SUCCESS;
+}
+#endif /* CONFIG_AMD_SEV_ES */
+
 int main(void)
 {
 	int rtn;
 	rtn = test_sev_activation();
 	report(rtn == EXIT_SUCCESS, "SEV activation test.");
+#ifdef CONFIG_AMD_SEV_ES
+	rtn = test_sev_es_activation();
+	report(rtn == EXIT_SUCCESS, "SEV-ES activation test.");
+	rtn = test_sev_es_msr();
+	report(rtn == EXIT_SUCCESS, "SEV-ES MSR test.");
+#endif /* CONFIG_AMD_SEV_ES */
 	return report_summary();
 }
-- 
2.33.0.rc1.237.g0d66db33f3-goog


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [kvm-unit-tests RFC 14/16] x86 AMD SEV-ES: Copy UEFI #VC IDT entry
  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
  2021-08-21  0:47     ` Zixuan Wang
  0 siblings, 2 replies; 20+ messages in thread
From: Sean Christopherson @ 2021-08-20 23:50 UTC (permalink / raw)
  To: Zixuan Wang
  Cc: kvm, pbonzini, drjones, marcorr, baekhw, tmroeder, erdemaktas,
	rientjes, brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel,
	bp

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

> Signed-off-by: Zixuan Wang <zixuanwang@google.com>
> ---
>  lib/x86/amd_sev.c | 10 ++++++++++
>  lib/x86/amd_sev.h |  5 +++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/lib/x86/amd_sev.c b/lib/x86/amd_sev.c
> index c2aebdf..04b6912 100644
> --- a/lib/x86/amd_sev.c
> +++ b/lib/x86/amd_sev.c
> @@ -46,11 +46,21 @@ EFI_STATUS setup_amd_sev(void)
>  
>  #ifdef CONFIG_AMD_SEV_ES
>  EFI_STATUS setup_amd_sev_es(void){
> +	struct descriptor_table_ptr idtr;
> +	idt_entry_t *idt;
> +
>  	/* Test if SEV-ES is enabled */
>  	if (!(rdmsr(MSR_SEV_STATUS) & SEV_ES_ENABLED_MASK)) {
>  		return EFI_UNSUPPORTED;
>  	}
>  
> +	/* Copy UEFI's #VC IDT entry, so KVM-Unit-Tests can reuse it and does

Nit, multiline comments should have a leading bare /*, i.e.

	/*
	 * Copy ....
	 * not have to ...

> +	 * not have to re-implement a #VC handler
> +	 */
> +	sidt(&idtr);
> +	idt = (idt_entry_t *)idtr.base;
> +	boot_idt[SEV_ES_VC_HANDLER_VECTOR] = idt[SEV_ES_VC_HANDLER_VECTOR];
> +
>  	return EFI_SUCCESS;
>  }
>  
> diff --git a/lib/x86/amd_sev.h b/lib/x86/amd_sev.h
> index 4d81cae..5ebd4a6 100644
> --- a/lib/x86/amd_sev.h
> +++ b/lib/x86/amd_sev.h
> @@ -36,6 +36,11 @@
>  #define SEV_ENABLED_MASK    0b1
>  #define SEV_ES_ENABLED_MASK 0b10
>  
> +/* AMD Programmer's Manual Volume 2
> + *   - Section "#VC Exception"
> + */
> +#define SEV_ES_VC_HANDLER_VECTOR 29
> +
>  EFI_STATUS setup_amd_sev(void);
>  #ifdef CONFIG_AMD_SEV_ES
>  EFI_STATUS setup_amd_sev_es(void);
> -- 
> 2.33.0.rc1.237.g0d66db33f3-goog
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kvm-unit-tests RFC 14/16] x86 AMD SEV-ES: Copy UEFI #VC IDT entry
  2021-08-20 23:50   ` Sean Christopherson
@ 2021-08-21  0:37     ` Marc Orr
  2021-08-21  0:47     ` Zixuan Wang
  1 sibling, 0 replies; 20+ messages in thread
From: Marc Orr @ 2021-08-21  0:37 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Zixuan Wang, kvm list, Paolo Bonzini, Andrew Jones,
	Hyunwook (Wooky) Baek, Tom Roeder, Erdem Aktas, David Rientjes,
	Singh, Brijesh, Lendacky, Thomas, Varad Gautam, Joerg Roedel, bp

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [kvm-unit-tests RFC 14/16] x86 AMD SEV-ES: Copy UEFI #VC IDT entry
  2021-08-20 23:50   ` Sean Christopherson
  2021-08-21  0:37     ` Marc Orr
@ 2021-08-21  0:47     ` Zixuan Wang
  1 sibling, 0 replies; 20+ messages in thread
From: Zixuan Wang @ 2021-08-21  0:47 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: kvm, pbonzini, drjones, marcorr, baekhw, tmroeder, erdemaktas,
	rientjes, brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel,
	bp

> On Fri, Aug 20, 2021 at 4:50 PM Sean Christopherson <seanjc@google.com> wrote:
> > Signed-off-by: Zixuan Wang <zixuanwang@google.com>
> > ---
> >  lib/x86/amd_sev.c | 10 ++++++++++
> >  lib/x86/amd_sev.h |  5 +++++
> >  2 files changed, 15 insertions(+)
> >
> > diff --git a/lib/x86/amd_sev.c b/lib/x86/amd_sev.c
> > index c2aebdf..04b6912 100644
> > --- a/lib/x86/amd_sev.c
> > +++ b/lib/x86/amd_sev.c
> > @@ -46,11 +46,21 @@ EFI_STATUS setup_amd_sev(void)
> >
> >  #ifdef CONFIG_AMD_SEV_ES
> >  EFI_STATUS setup_amd_sev_es(void){
> > +     struct descriptor_table_ptr idtr;
> > +     idt_entry_t *idt;
> > +
> >       /* Test if SEV-ES is enabled */
> >       if (!(rdmsr(MSR_SEV_STATUS) & SEV_ES_ENABLED_MASK)) {
> >               return EFI_UNSUPPORTED;
> >       }
> >
> > +     /* Copy UEFI's #VC IDT entry, so KVM-Unit-Tests can reuse it and does
>
> Nit, multiline comments should have a leading bare /*, i.e.
>
>         /*
>          * Copy ....
>          * not have to ...

Thanks for pointing this out, I will fix this issue in the next version.

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2021-08-21  0:48 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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).