* [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes
@ 2021-10-31 5:56 Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 1/7] x86 UEFI: Remove mixed_mode Zixuan Wang
` (7 more replies)
0 siblings, 8 replies; 19+ messages in thread
From: Zixuan Wang @ 2021-10-31 5:56 UTC (permalink / raw)
To: kvm, pbonzini, drjones
Cc: marcorr, erdemaktas, rientjes, seanjc, brijesh.singh,
Thomas.Lendacky, varad.gautam, jroedel, bp
Hello,
This patch series refactors the x86_64 UEFI set up process and fixes the
`run-tests.sh` script to run under UEFI. The patches are organized as
three parts.
The first part (patches 1-2) refactors the x86_64 UEFI set up process.
The previous UEFI setup calls arch-specific setup functions twice and
generates arch-specific data structure. As Andrew suggested [1], we
refactor this process to make only one call to the arch-specific
function and generate arch-neutral data structures. This simplifies the
set up process and makes it easier to develop UEFI support for other
architectures.
The second part (patch 3) converts several x86 test cases to
Position-Independent Code (PIC) to run under UEFI. This patch is ported
from the initial UEFI support patchset [2] with fixes to the 32-bit
compilation.
The third part (patches 4-7) fixes the UEFI runner scripts. Patch 4 sets
UEFI OVMF image as readonly. Patch 5 fixes test cases' return code under
UEFI, enabling Patch 6-7 to fix the `run-tests.sh` script under UEFI.
This patch set is based on the `uefi` branch.
Best regards,
Zixuan Wang and Marc Orr
[1] https://lore.kernel.org/kvm/20211005060549.clar5nakynz2zecl@gator.home/
[2] https://lore.kernel.org/kvm/20211004204931.1537823-1-zxwang42@gmail.com/
Marc Orr (2):
scripts: Generalize EFI check
x86 UEFI: Make run_tests.sh (mostly) work under UEFI
Zixuan Wang (5):
x86 UEFI: Remove mixed_mode
x86 UEFI: Refactor set up process
x86 UEFI: Convert x86 test cases to PIC
x86 UEFI: Set UEFI OVMF as readonly
x86 UEFI: Exit QEMU with return code
lib/efi.c | 54 ++++++--
lib/efi.h | 19 ++-
lib/linux/efi.h | 317 ++++++++++++++-----------------------------
lib/x86/acpi.c | 36 +++--
lib/x86/acpi.h | 5 +-
lib/x86/asm/setup.h | 16 +--
lib/x86/setup.c | 153 ++++++++++-----------
lib/x86/usermode.c | 3 +-
scripts/common.bash | 9 +-
scripts/runtime.bash | 15 +-
x86/Makefile.common | 10 +-
x86/Makefile.x86_64 | 7 +-
x86/access.c | 9 +-
x86/cet.c | 8 +-
x86/efi/run | 27 +++-
x86/emulator.c | 5 +-
x86/eventinj.c | 8 ++
x86/run | 6 +-
x86/smap.c | 13 +-
x86/umip.c | 26 +++-
20 files changed, 360 insertions(+), 386 deletions(-)
--
2.33.0
^ permalink raw reply [flat|nested] 19+ messages in thread
* [kvm-unit-tests PATCH v1 1/7] x86 UEFI: Remove mixed_mode
2021-10-31 5:56 [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Zixuan Wang
@ 2021-10-31 5:56 ` Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 2/7] x86 UEFI: Refactor set up process Zixuan Wang
` (6 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Zixuan Wang @ 2021-10-31 5:56 UTC (permalink / raw)
To: kvm, pbonzini, drjones
Cc: marcorr, erdemaktas, rientjes, seanjc, brijesh.singh,
Thomas.Lendacky, varad.gautam, jroedel, bp
From: Zixuan Wang <zxwang42@gmail.com>
Remove the mixed_mode code from efi.h as we are not supporting i386 UEFI
for now.
Signed-off-by: Zixuan Wang <zxwang42@gmail.com>
---
lib/linux/efi.h | 317 +++++++++++++++---------------------------------
1 file changed, 100 insertions(+), 217 deletions(-)
diff --git a/lib/linux/efi.h b/lib/linux/efi.h
index 7ac1082..455625a 100644
--- a/lib/linux/efi.h
+++ b/lib/linux/efi.h
@@ -62,15 +62,7 @@ typedef guid_t efi_guid_t;
typedef struct {
efi_guid_t guid;
- u32 table;
-} efi_config_table_32_t;
-
-typedef union {
- struct {
- efi_guid_t guid;
- void *table;
- };
- efi_config_table_32_t mixed_mode;
+ void *table;
} efi_config_table_t;
/*
@@ -251,127 +243,76 @@ typedef struct efi_generic_dev_path efi_device_path_protocol_t;
/*
* EFI Boot Services table
*/
-union efi_boot_services {
- struct {
- efi_table_hdr_t hdr;
- void *raise_tpl;
- void *restore_tpl;
- efi_status_t (__efiapi *allocate_pages)(int, int, unsigned long,
- efi_physical_addr_t *);
- efi_status_t (__efiapi *free_pages)(efi_physical_addr_t,
- unsigned long);
- efi_status_t (__efiapi *get_memory_map)(unsigned long *, void *,
- unsigned long *,
- unsigned long *, u32 *);
- efi_status_t (__efiapi *allocate_pool)(int, unsigned long,
- void **);
- efi_status_t (__efiapi *free_pool)(void *);
- efi_status_t (__efiapi *create_event)(u32, unsigned long,
- efi_event_notify_t, void *,
- efi_event_t *);
- efi_status_t (__efiapi *set_timer)(efi_event_t,
- EFI_TIMER_DELAY, u64);
- efi_status_t (__efiapi *wait_for_event)(unsigned long,
- efi_event_t *,
- unsigned long *);
- void *signal_event;
- efi_status_t (__efiapi *close_event)(efi_event_t);
- void *check_event;
- void *install_protocol_interface;
- void *reinstall_protocol_interface;
- void *uninstall_protocol_interface;
- efi_status_t (__efiapi *handle_protocol)(efi_handle_t,
- efi_guid_t *, void **);
- void *__reserved;
- void *register_protocol_notify;
- efi_status_t (__efiapi *locate_handle)(int, efi_guid_t *,
- void *, unsigned long *,
- efi_handle_t *);
- efi_status_t (__efiapi *locate_device_path)(efi_guid_t *,
- efi_device_path_protocol_t **,
- efi_handle_t *);
- efi_status_t (__efiapi *install_configuration_table)(efi_guid_t *,
- void *);
- void *load_image;
- void *start_image;
- efi_status_t (__efiapi *exit)(efi_handle_t,
- efi_status_t,
- unsigned long,
- efi_char16_t *);
- void *unload_image;
- efi_status_t (__efiapi *exit_boot_services)(efi_handle_t,
- unsigned long);
- void *get_next_monotonic_count;
- efi_status_t (__efiapi *stall)(unsigned long);
- void *set_watchdog_timer;
- void *connect_controller;
- efi_status_t (__efiapi *disconnect_controller)(efi_handle_t,
- efi_handle_t,
- efi_handle_t);
- void *open_protocol;
- void *close_protocol;
- void *open_protocol_information;
- void *protocols_per_handle;
- void *locate_handle_buffer;
- efi_status_t (__efiapi *locate_protocol)(efi_guid_t *, void *,
- void **);
- void *install_multiple_protocol_interfaces;
- void *uninstall_multiple_protocol_interfaces;
- void *calculate_crc32;
- void *copy_mem;
- void *set_mem;
- void *create_event_ex;
- };
- struct {
- efi_table_hdr_t hdr;
- u32 raise_tpl;
- u32 restore_tpl;
- u32 allocate_pages;
- u32 free_pages;
- u32 get_memory_map;
- u32 allocate_pool;
- u32 free_pool;
- u32 create_event;
- u32 set_timer;
- u32 wait_for_event;
- u32 signal_event;
- u32 close_event;
- u32 check_event;
- u32 install_protocol_interface;
- u32 reinstall_protocol_interface;
- u32 uninstall_protocol_interface;
- u32 handle_protocol;
- u32 __reserved;
- u32 register_protocol_notify;
- u32 locate_handle;
- u32 locate_device_path;
- u32 install_configuration_table;
- u32 load_image;
- u32 start_image;
- u32 exit;
- u32 unload_image;
- u32 exit_boot_services;
- u32 get_next_monotonic_count;
- u32 stall;
- u32 set_watchdog_timer;
- u32 connect_controller;
- u32 disconnect_controller;
- u32 open_protocol;
- u32 close_protocol;
- u32 open_protocol_information;
- u32 protocols_per_handle;
- u32 locate_handle_buffer;
- u32 locate_protocol;
- u32 install_multiple_protocol_interfaces;
- u32 uninstall_multiple_protocol_interfaces;
- u32 calculate_crc32;
- u32 copy_mem;
- u32 set_mem;
- u32 create_event_ex;
- } mixed_mode;
-};
-
-typedef union efi_boot_services efi_boot_services_t;
+typedef struct {
+ efi_table_hdr_t hdr;
+ void *raise_tpl;
+ void *restore_tpl;
+ efi_status_t(__efiapi *allocate_pages)(int, int, unsigned long,
+ efi_physical_addr_t *);
+ efi_status_t(__efiapi *free_pages)(efi_physical_addr_t,
+ unsigned long);
+ efi_status_t(__efiapi *get_memory_map)(unsigned long *, void *,
+ unsigned long *,
+ unsigned long *, u32 *);
+ efi_status_t(__efiapi *allocate_pool)(int, unsigned long,
+ void **);
+ efi_status_t(__efiapi *free_pool)(void *);
+ efi_status_t(__efiapi *create_event)(u32, unsigned long,
+ efi_event_notify_t, void *,
+ efi_event_t *);
+ efi_status_t(__efiapi *set_timer)(efi_event_t,
+ EFI_TIMER_DELAY, u64);
+ efi_status_t(__efiapi *wait_for_event)(unsigned long,
+ efi_event_t *,
+ unsigned long *);
+ void *signal_event;
+ efi_status_t(__efiapi *close_event)(efi_event_t);
+ void *check_event;
+ void *install_protocol_interface;
+ void *reinstall_protocol_interface;
+ void *uninstall_protocol_interface;
+ efi_status_t(__efiapi *handle_protocol)(efi_handle_t,
+ efi_guid_t *, void **);
+ void *__reserved;
+ void *register_protocol_notify;
+ efi_status_t(__efiapi *locate_handle)(int, efi_guid_t *,
+ void *, unsigned long *,
+ efi_handle_t *);
+ efi_status_t(__efiapi *locate_device_path)(efi_guid_t *,
+ efi_device_path_protocol_t **,
+ efi_handle_t *);
+ efi_status_t(__efiapi *install_configuration_table)(efi_guid_t *,
+ void *);
+ void *load_image;
+ void *start_image;
+ efi_status_t(__efiapi *exit)(efi_handle_t,
+ efi_status_t,
+ unsigned long,
+ efi_char16_t *);
+ void *unload_image;
+ efi_status_t(__efiapi *exit_boot_services)(efi_handle_t,
+ unsigned long);
+ void *get_next_monotonic_count;
+ efi_status_t(__efiapi *stall)(unsigned long);
+ void *set_watchdog_timer;
+ void *connect_controller;
+ efi_status_t(__efiapi *disconnect_controller)(efi_handle_t,
+ efi_handle_t,
+ efi_handle_t);
+ void *open_protocol;
+ void *close_protocol;
+ void *open_protocol_information;
+ void *protocols_per_handle;
+ void *locate_handle_buffer;
+ efi_status_t(__efiapi *locate_protocol)(efi_guid_t *, void *,
+ void **);
+ void *install_multiple_protocol_interfaces;
+ void *uninstall_multiple_protocol_interfaces;
+ void *calculate_crc32;
+ void *copy_mem;
+ void *set_mem;
+ void *create_event_ex;
+} efi_boot_services_t;
/*
* Types and defines for EFI ResetSystem
@@ -386,24 +327,6 @@ typedef union efi_boot_services efi_boot_services_t;
#define EFI_RUNTIME_SERVICES_SIGNATURE ((u64)0x5652453544e5552ULL)
#define EFI_RUNTIME_SERVICES_REVISION 0x00010000
-typedef struct {
- efi_table_hdr_t hdr;
- u32 get_time;
- u32 set_time;
- u32 get_wakeup_time;
- u32 set_wakeup_time;
- u32 set_virtual_address_map;
- u32 convert_pointer;
- u32 get_variable;
- u32 get_next_variable;
- u32 set_variable;
- u32 get_next_high_mono_count;
- u32 reset_system;
- u32 update_capsule;
- u32 query_capsule_caps;
- u32 query_variable_info;
-} efi_runtime_services_32_t;
-
typedef efi_status_t efi_get_time_t (efi_time_t *tm, efi_time_cap_t *tc);
typedef efi_status_t efi_set_time_t (efi_time_t *tm);
typedef efi_status_t efi_get_wakeup_time_t (efi_bool_t *enabled, efi_bool_t *pending,
@@ -438,25 +361,22 @@ typedef efi_status_t efi_query_variable_store_t(u32 attributes,
unsigned long size,
bool nonblocking);
-typedef union {
- struct {
- efi_table_hdr_t hdr;
- efi_get_time_t __efiapi *get_time;
- efi_set_time_t __efiapi *set_time;
- efi_get_wakeup_time_t __efiapi *get_wakeup_time;
- efi_set_wakeup_time_t __efiapi *set_wakeup_time;
- efi_set_virtual_address_map_t __efiapi *set_virtual_address_map;
- void *convert_pointer;
- efi_get_variable_t __efiapi *get_variable;
- efi_get_next_variable_t __efiapi *get_next_variable;
- efi_set_variable_t __efiapi *set_variable;
- efi_get_next_high_mono_count_t __efiapi *get_next_high_mono_count;
- efi_reset_system_t __efiapi *reset_system;
- efi_update_capsule_t __efiapi *update_capsule;
- efi_query_capsule_caps_t __efiapi *query_capsule_caps;
- efi_query_variable_info_t __efiapi *query_variable_info;
- };
- efi_runtime_services_32_t mixed_mode;
+typedef struct {
+ efi_table_hdr_t hdr;
+ efi_get_time_t __efiapi *get_time;
+ efi_set_time_t __efiapi *set_time;
+ efi_get_wakeup_time_t __efiapi *get_wakeup_time;
+ efi_set_wakeup_time_t __efiapi *set_wakeup_time;
+ efi_set_virtual_address_map_t __efiapi *set_virtual_address_map;
+ void *convert_pointer;
+ efi_get_variable_t __efiapi *get_variable;
+ efi_get_next_variable_t __efiapi *get_next_variable;
+ efi_set_variable_t __efiapi *set_variable;
+ efi_get_next_high_mono_count_t __efiapi *get_next_high_mono_count;
+ efi_reset_system_t __efiapi *reset_system;
+ efi_update_capsule_t __efiapi *update_capsule;
+ efi_query_capsule_caps_t __efiapi *query_capsule_caps;
+ efi_query_variable_info_t __efiapi *query_variable_info;
} efi_runtime_services_t;
#define EFI_SYSTEM_TABLE_SIGNATURE ((u64)0x5453595320494249ULL)
@@ -468,60 +388,23 @@ typedef union {
#define EFI_1_10_SYSTEM_TABLE_REVISION ((1 << 16) | (10))
#define EFI_1_02_SYSTEM_TABLE_REVISION ((1 << 16) | (02))
-typedef struct {
- efi_table_hdr_t hdr;
- u64 fw_vendor; /* physical addr of CHAR16 vendor string */
- u32 fw_revision;
- u32 __pad1;
- u64 con_in_handle;
- u64 con_in;
- u64 con_out_handle;
- u64 con_out;
- u64 stderr_handle;
- u64 stderr;
- u64 runtime;
- u64 boottime;
- u32 nr_tables;
- u32 __pad2;
- u64 tables;
-} efi_system_table_64_t;
+typedef union efi_simple_text_input_protocol efi_simple_text_input_protocol_t;
+typedef union efi_simple_text_output_protocol efi_simple_text_output_protocol_t;
typedef struct {
efi_table_hdr_t hdr;
- u32 fw_vendor; /* physical addr of CHAR16 vendor string */
+ unsigned long fw_vendor; /* physical addr of CHAR16 vendor string */
u32 fw_revision;
- u32 con_in_handle;
- u32 con_in;
- u32 con_out_handle;
- u32 con_out;
- u32 stderr_handle;
- u32 stderr;
- u32 runtime;
- u32 boottime;
- u32 nr_tables;
- u32 tables;
-} efi_system_table_32_t;
-
-typedef union efi_simple_text_input_protocol efi_simple_text_input_protocol_t;
-typedef union efi_simple_text_output_protocol efi_simple_text_output_protocol_t;
-
-typedef union {
- struct {
- efi_table_hdr_t hdr;
- unsigned long fw_vendor; /* physical addr of CHAR16 vendor string */
- u32 fw_revision;
- unsigned long con_in_handle;
- efi_simple_text_input_protocol_t *con_in;
- unsigned long con_out_handle;
- efi_simple_text_output_protocol_t *con_out;
- unsigned long stderr_handle;
- unsigned long stderr;
- efi_runtime_services_t *runtime;
- efi_boot_services_t *boottime;
- unsigned long nr_tables;
- unsigned long tables;
- };
- efi_system_table_32_t mixed_mode;
+ unsigned long con_in_handle;
+ efi_simple_text_input_protocol_t *con_in;
+ unsigned long con_out_handle;
+ efi_simple_text_output_protocol_t *con_out;
+ unsigned long stderr_handle;
+ unsigned long stderr;
+ efi_runtime_services_t *runtime;
+ efi_boot_services_t *boottime;
+ unsigned long nr_tables;
+ unsigned long tables;
} efi_system_table_t;
struct efi_boot_memmap {
--
2.33.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [kvm-unit-tests PATCH v1 2/7] x86 UEFI: Refactor set up process
2021-10-31 5:56 [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 1/7] x86 UEFI: Remove mixed_mode Zixuan Wang
@ 2021-10-31 5:56 ` Zixuan Wang
2021-11-05 18:54 ` Sean Christopherson
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 3/7] x86 UEFI: Convert x86 test cases to PIC Zixuan Wang
` (5 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Zixuan Wang @ 2021-10-31 5:56 UTC (permalink / raw)
To: kvm, pbonzini, drjones
Cc: marcorr, erdemaktas, rientjes, seanjc, brijesh.singh,
Thomas.Lendacky, varad.gautam, jroedel, bp
From: Zixuan Wang <zxwang42@gmail.com>
Refactor the EFI set up process. The previous set up process calls
multiple arch-specific functions, now it's simplified to call
only one arch-specific function:
1. (Arch neutral ) Extract EFI data structures, e.g., memory maps
2. (Arch neutral ) Exit EFI boot services
3. (Arch specific) Parse EFI data structures and set up arch-specific
resources
4. (Arch neutral ) Run test cases' main functions
Signed-off-by: Zixuan Wang <zxwang42@gmail.com>
---
lib/efi.c | 50 ++++++++++++---
lib/efi.h | 19 ++++--
lib/x86/acpi.c | 36 ++++++-----
lib/x86/acpi.h | 5 +-
lib/x86/asm/setup.h | 16 +----
lib/x86/setup.c | 153 +++++++++++++++++++++-----------------------
6 files changed, 149 insertions(+), 130 deletions(-)
diff --git a/lib/efi.c b/lib/efi.c
index 9506830..99eb00c 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -65,9 +65,9 @@ out:
return status;
}
-efi_status_t efi_exit_boot_services(void *handle, unsigned long mapkey)
+efi_status_t efi_exit_boot_services(void *handle, struct efi_boot_memmap *map)
{
- return efi_bs_call(exit_boot_services, handle, mapkey);
+ return efi_bs_call(exit_boot_services, handle, *map->key_ptr);
}
efi_status_t efi_get_system_config_table(efi_guid_t table_guid, void **table)
@@ -88,31 +88,61 @@ efi_status_t efi_get_system_config_table(efi_guid_t table_guid, void **table)
efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
{
int ret;
- unsigned long mapkey = 0;
efi_status_t status;
efi_bootinfo_t efi_bootinfo;
efi_system_table = sys_tab;
- setup_efi_bootinfo(&efi_bootinfo);
- status = setup_efi_pre_boot(&mapkey, &efi_bootinfo);
+ /* Memory map struct values */
+ efi_memory_desc_t *map = NULL;
+ unsigned long map_size = 0, desc_size = 0, key = 0, buff_size = 0;
+ u32 desc_ver;
+
+ /* Set up efi_bootinfo */
+ efi_bootinfo.mem_map.map = ↦
+ efi_bootinfo.mem_map.map_size = &map_size;
+ efi_bootinfo.mem_map.desc_size = &desc_size;
+ efi_bootinfo.mem_map.desc_ver = &desc_ver;
+ efi_bootinfo.mem_map.key_ptr = &key;
+ efi_bootinfo.mem_map.buff_size = &buff_size;
+
+ /* Get EFI memory map */
+ status = efi_get_memory_map(&efi_bootinfo.mem_map);
if (status != EFI_SUCCESS) {
- printf("Failed to set up before ExitBootServices, exiting.\n");
- return status;
+ printf("Failed to get memory map\n");
+ goto efi_main_error;
}
- status = efi_exit_boot_services(handle, mapkey);
+ /*
+ * Exit EFI boot services, let kvm-unit-tests take full control of the
+ * guest
+ */
+ status = efi_exit_boot_services(handle, &efi_bootinfo.mem_map);
if (status != EFI_SUCCESS) {
printf("Failed to exit boot services\n");
- return status;
+ goto efi_main_error;
}
- setup_efi(&efi_bootinfo);
+ /* Set up arch-specific resources */
+ status = setup_efi(&efi_bootinfo);
+ if (status != EFI_SUCCESS) {
+ printf("Failed to set up arch-specific resources\n");
+ goto efi_main_error;
+ }
+
+ /* Run the test case */
ret = main(__argc, __argv, __environ);
/* Shutdown the guest VM */
efi_rs_call(reset_system, EFI_RESET_SHUTDOWN, ret, 0, NULL);
+ /* Unreachable */
+ return EFI_UNSUPPORTED;
+
+efi_main_error:
+ /* Shutdown the guest with error EFI status */
+ efi_rs_call(reset_system, EFI_RESET_SHUTDOWN, status, 0, NULL);
+
/* Unreachable */
return EFI_UNSUPPORTED;
}
diff --git a/lib/efi.h b/lib/efi.h
index 1b3abd0..ce8b74d 100644
--- a/lib/efi.h
+++ b/lib/efi.h
@@ -2,9 +2,7 @@
#define _EFI_H_
/*
- * EFI-related functions in . This file's name "efi.h" is in
- * conflict with GNU-EFI library's "efi.h", but does not include
- * GNU-EFI headers or links against GNU-EFI.
+ * EFI-related functions.
*
* Copyright (c) 2021, Google Inc, Zixuan Wang <zixuanwang@google.com>
*
@@ -13,9 +11,20 @@
#include "linux/efi.h"
#include <elf.h>
-efi_status_t _relocate(long ldbase, Elf64_Dyn *dyn, efi_handle_t handle, efi_system_table_t *sys_tab);
+/*
+ * efi_bootinfo_t: stores EFI-related machine info retrieved before exiting EFI
+ * boot services, and is then used by setup_efi(). setup_efi() cannot retrieve
+ * this info as it is called after ExitBootServices and thus some EFI resources
+ * and functions are not available.
+ */
+typedef struct {
+ struct efi_boot_memmap mem_map;
+} efi_bootinfo_t;
+
+efi_status_t _relocate(long ldbase, Elf64_Dyn *dyn, efi_handle_t handle,
+ efi_system_table_t *sys_tab);
efi_status_t efi_get_memory_map(struct efi_boot_memmap *map);
-efi_status_t efi_exit_boot_services(void *handle, unsigned long mapkey);
+efi_status_t efi_exit_boot_services(void *handle, struct efi_boot_memmap *map);
efi_status_t efi_get_system_config_table(efi_guid_t table_guid, void **table);
efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab);
diff --git a/lib/x86/acpi.c b/lib/x86/acpi.c
index 0f75d79..c523dac 100644
--- a/lib/x86/acpi.c
+++ b/lib/x86/acpi.c
@@ -4,29 +4,35 @@
#ifdef TARGET_EFI
struct rsdp_descriptor *efi_rsdp = NULL;
-void setup_efi_rsdp(struct rsdp_descriptor *rsdp) {
+void set_efi_rsdp(struct rsdp_descriptor *rsdp)
+{
efi_rsdp = rsdp;
}
-static struct rsdp_descriptor *get_rsdp(void) {
+static struct rsdp_descriptor *get_rsdp(void)
+{
if (efi_rsdp == NULL) {
- printf("Can't find RSDP from UEFI, maybe setup_efi_rsdp() was not called\n");
+ printf("Can't find RSDP from UEFI, maybe set_efi_rsdp() was 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 == RSDP_SIGNATURE_8BYTE)
- break;
- }
- if (addr == 0x100000) {
- return NULL;
- }
- return rsdp;
+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 == RSDP_SIGNATURE_8BYTE)
+ break;
+ }
+
+ if (addr == 0x100000) {
+ return NULL;
+ }
+
+ return rsdp;
}
#endif /* TARGET_EFI */
diff --git a/lib/x86/acpi.h b/lib/x86/acpi.h
index db8ee56..67ba389 100644
--- a/lib/x86/acpi.h
+++ b/lib/x86/acpi.h
@@ -106,10 +106,7 @@ struct facs_descriptor_rev1
u8 reserved3 [40]; /* Reserved - must be zero */
};
+void set_efi_rsdp(struct rsdp_descriptor *rsdp);
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 73ff4a3..dbfb2a2 100644
--- a/lib/x86/asm/setup.h
+++ b/lib/x86/asm/setup.h
@@ -12,21 +12,7 @@ unsigned long setup_tss(u8 *stacktop);
#include "efi.h"
#include "x86/amd_sev.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;
- struct rsdp_descriptor *rsdp;
-} efi_bootinfo_t;
-
-void setup_efi_bootinfo(efi_bootinfo_t *efi_bootinfo);
-void setup_efi(efi_bootinfo_t *efi_bootinfo);
-efi_status_t setup_efi_pre_boot(unsigned long *mapkey, efi_bootinfo_t *efi_bootinfo);
+efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo);
void setup_5level_page_table(void);
#endif /* TARGET_EFI */
diff --git a/lib/x86/setup.c b/lib/x86/setup.c
index 24fe74e..57b9b5e 100644
--- a/lib/x86/setup.c
+++ b/lib/x86/setup.c
@@ -173,34 +173,14 @@ void setup_multiboot(struct mbi_bootinfo *bi)
extern void load_idt(void);
extern void load_gdt_tss(size_t tss_offset);
-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_t setup_pre_boot_memory(unsigned long *mapkey, efi_bootinfo_t *efi_bootinfo)
+static efi_status_t setup_memory_allocator(efi_bootinfo_t *efi_bootinfo)
{
int i;
- unsigned long free_mem_total_pages;
- efi_status_t status;
- struct efi_boot_memmap map;
- efi_memory_desc_t *buffer, *d;
- unsigned long map_size, desc_size, buff_size;
- u32 desc_ver;
-
- map.map = &buffer;
- map.map_size = &map_size;
- map.desc_size = &desc_size;
- map.desc_ver = &desc_ver;
- map.buff_size = &buff_size;
- map.key_ptr = mapkey;
-
- status = efi_get_memory_map(&map);
- if (status != EFI_SUCCESS) {
- return status;
- }
+ unsigned long free_mem_pages = 0;
+ unsigned long free_mem_start = 0;
+ struct efi_boot_memmap *map = &(efi_bootinfo->mem_map);
+ efi_memory_desc_t *buffer = *map->map;
+ efi_memory_desc_t *d = NULL;
/*
* The 'buffer' contains multiple descriptors that describe memory
@@ -209,77 +189,42 @@ static efi_status_t setup_pre_boot_memory(unsigned long *mapkey, efi_bootinfo_t
* memory allocator, so that the memory allocator can work in the
* largest free continuous memory region.
*/
- free_mem_total_pages = 0;
- for (i = 0; i < map_size; i += desc_size) {
+ for (i = 0; i < *(map->map_size); i += *(map->desc_size)) {
d = (efi_memory_desc_t *)(&((u8 *)buffer)[i]);
if (d->type == EFI_CONVENTIONAL_MEMORY) {
- if (free_mem_total_pages < d->num_pages) {
- free_mem_total_pages = d->num_pages;
- efi_bootinfo->free_mem_size = free_mem_total_pages << EFI_PAGE_SHIFT;
- efi_bootinfo->free_mem_start = d->phys_addr;
+ if (free_mem_pages < d->num_pages) {
+ free_mem_pages = d->num_pages;
+ free_mem_start = d->phys_addr;
}
}
}
- if (efi_bootinfo->free_mem_size == 0) {
+ if (free_mem_pages == 0) {
return EFI_OUT_OF_RESOURCES;
}
- return EFI_SUCCESS;
-}
+ phys_alloc_init(free_mem_start, free_mem_pages << EFI_PAGE_SHIFT);
-static efi_status_t setup_pre_boot_rsdp(efi_bootinfo_t *efi_bootinfo)
-{
- return efi_get_system_config_table(ACPI_TABLE_GUID, (void **)&efi_bootinfo->rsdp);
+ return EFI_SUCCESS;
}
-efi_status_t setup_efi_pre_boot(unsigned long *mapkey, efi_bootinfo_t *efi_bootinfo)
+static efi_status_t setup_rsdp(efi_bootinfo_t *efi_bootinfo)
{
efi_status_t status;
+ struct rsdp_descriptor *rsdp;
- status = setup_pre_boot_memory(mapkey, efi_bootinfo);
- if (status != EFI_SUCCESS) {
- printf("setup_pre_boot_memory() failed: ");
- switch (status) {
- case EFI_OUT_OF_RESOURCES:
- printf("No free memory region\n");
- break;
- default:
- printf("Unknown error\n");
- break;
- }
- return status;
- }
-
- status = setup_pre_boot_rsdp(efi_bootinfo);
+ /*
+ * RSDP resides in an EFI_ACPI_RECLAIM_MEMORY region, which is not used
+ * by kvm-unit-tests x86's memory allocator. So it is not necessary to
+ * copy the data structure to another memory region to prevent
+ * unintentional overwrite.
+ */
+ status = efi_get_system_config_table(ACPI_TABLE_GUID, (void **)&rsdp);
if (status != EFI_SUCCESS) {
- printf("Cannot find RSDP in EFI system table\n");
return status;
}
- status = setup_amd_sev();
- if (status != EFI_SUCCESS) {
- switch (status) {
- case EFI_UNSUPPORTED:
- /* Continue if AMD SEV is not supported */
- break;
- default:
- printf("Set up AMD SEV failed\n");
- return status;
- }
- }
-
- status = setup_amd_sev_es();
- if (status != EFI_SUCCESS) {
- switch (status) {
- case EFI_UNSUPPORTED:
- /* Continue if AMD SEV-ES is not supported */
- break;
- default:
- printf("Set up AMD SEV-ES failed\n");
- return status;
- }
- }
+ set_efi_rsdp(rsdp);
return EFI_SUCCESS;
}
@@ -333,8 +278,54 @@ static void setup_gdt_tss(void)
load_gdt_tss(tss_offset);
}
-void setup_efi(efi_bootinfo_t *efi_bootinfo)
+efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo)
{
+ efi_status_t status;
+
+ status = setup_memory_allocator(efi_bootinfo);
+ if (status != EFI_SUCCESS) {
+ printf("Failed to set up memory allocator: ");
+ switch (status) {
+ case EFI_OUT_OF_RESOURCES:
+ printf("No free memory region\n");
+ break;
+ default:
+ printf("Unknown error\n");
+ break;
+ }
+ return status;
+ }
+
+ status = setup_rsdp(efi_bootinfo);
+ if (status != EFI_SUCCESS) {
+ printf("Cannot find RSDP in EFI system table\n");
+ return status;
+ }
+
+ status = setup_amd_sev();
+ if (status != EFI_SUCCESS) {
+ switch (status) {
+ case EFI_UNSUPPORTED:
+ /* Continue if AMD SEV is not supported */
+ break;
+ default:
+ printf("Set up AMD SEV failed\n");
+ return status;
+ }
+ }
+
+ status = setup_amd_sev_es();
+ if (status != EFI_SUCCESS) {
+ switch (status) {
+ case EFI_UNSUPPORTED:
+ /* Continue if AMD SEV-ES is not supported */
+ break;
+ default:
+ printf("Set up AMD SEV-ES failed\n");
+ return status;
+ }
+ }
+
reset_apic();
setup_gdt_tss();
setup_idt();
@@ -343,9 +334,9 @@ void setup_efi(efi_bootinfo_t *efi_bootinfo)
enable_apic();
enable_x2apic();
smp_init();
- phys_alloc_init(efi_bootinfo->free_mem_start, efi_bootinfo->free_mem_size);
- setup_efi_rsdp(efi_bootinfo->rsdp);
setup_page_table();
+
+ return EFI_SUCCESS;
}
#endif /* TARGET_EFI */
--
2.33.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [kvm-unit-tests PATCH v1 3/7] x86 UEFI: Convert x86 test cases to PIC
2021-10-31 5:56 [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 1/7] x86 UEFI: Remove mixed_mode Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 2/7] x86 UEFI: Refactor set up process Zixuan Wang
@ 2021-10-31 5:56 ` Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 4/7] x86 UEFI: Set UEFI OVMF as readonly Zixuan Wang
` (4 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Zixuan Wang @ 2021-10-31 5:56 UTC (permalink / raw)
To: kvm, pbonzini, drjones
Cc: marcorr, erdemaktas, rientjes, seanjc, brijesh.singh,
Thomas.Lendacky, varad.gautam, jroedel, bp
From: Zixuan Wang <zixuanwang@google.com>
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. 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 | 9 +++++----
x86/cet.c | 8 +++++---
x86/emulator.c | 5 +++--
x86/eventinj.c | 8 ++++++++
x86/smap.c | 13 +++++++++----
x86/umip.c | 26 +++++++++++++++++++++++---
9 files changed, 64 insertions(+), 25 deletions(-)
diff --git a/lib/x86/usermode.c b/lib/x86/usermode.c
index 2e77831..5b657fd 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 deaa386..119f4b2 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 fe6457c..3963840 100644
--- a/x86/Makefile.x86_64
+++ b/x86/Makefile.x86_64
@@ -30,6 +30,8 @@ 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)
ifeq ($(TARGET_EFI),y)
tests += $(TEST_DIR)/amd_sev.$(exe)
@@ -40,14 +42,13 @@ endif
# 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 911f0e3..a2ec049 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, %[rsp0] \n\t"
"cmp $0, %[user] \n\t"
"jz do_access \n\t"
@@ -719,7 +719,8 @@ 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), %%r8\n\t"
+ "pushq %%r8\n\t"
"iretq \n"
"do_access: \n\t"
"cmp $0, %[fetch] \n\t"
@@ -750,7 +751,7 @@ static int ac_test_do_access(ac_test_t *at)
[user_cs]"i"(USER_CS),
[user_stack_top]"r"(user_stack + sizeof user_stack),
[kernel_entry_vector]"i"(0x20)
- : "rsi");
+ : "rsi", "r8");
asm volatile (".section .text.pf \n\t"
"page_fault: \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 c5f584a..22a518f 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_pass("ret");
stack_top[-1] = 0x778899;
diff --git a/x86/eventinj.c b/x86/eventinj.c
index 46593c9..3c0db56 100644
--- a/x86/eventinj.c
+++ b/x86/eventinj.c
@@ -155,9 +155,17 @@ asm("do_iret:"
"pushf"W" \n\t"
"mov %cs, %ecx \n\t"
"push"W" %"R "cx \n\t"
+#ifndef __x86_64__
"push"W" $2f \n\t"
"cmpb $0, no_test_device\n\t" // see if need to flush
+#else
+ "leaq 2f(%rip), %rbx \n\t"
+ "pushq %rbx \n\t"
+
+ "mov no_test_device(%rip), %bl \n\t"
+ "cmpb $0, %bl\n\t" // see if need to flush
+#endif
"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 c6ddf38..0994c29 100644
--- a/x86/smap.c
+++ b/x86/smap.c
@@ -159,12 +159,17 @@ int main(int ac, char **av)
init_test(i);
stac();
test = -1;
+#ifndef __x86_64__
+ #define TEST "test"
+#else
+ #define TEST "test(%rip)"
+#endif
asm("or $(" xstr(USER_BASE) "), %"R "sp \n"
"push $44 \n "
- "decl test\n"
+ "decl " TEST "\n"
"and $~(" xstr(USER_BASE) "), %"R "sp \n"
"pop %"R "ax\n"
- "movl %eax, test");
+ "movl %eax, " TEST "\n");
report(pf_count == 0 && test == 44,
"write to user stack with AC=1");
@@ -173,10 +178,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 "\n"
"and $~(" xstr(USER_BASE) "), %"R "sp \n"
"pop %"R "ax\n"
- "movl %eax, test");
+ "movl %eax, " TEST "\n");
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 af8db59..fccdedc 100644
--- a/x86/umip.c
+++ b/x86/umip.c
@@ -20,10 +20,20 @@ static void gp_handler(struct ex_regs *regs)
}
}
+#ifndef __x86_64__
+#define GP_ASM_MOVE_TO_RIP \
+ "mov" W " $1f, %[expected_rip]\n\t"
+#else
+#define GP_ASM_MOVE_TO_RIP \
+ "pushq %%rax\n\t" \
+ "lea 1f(%%rip), %%rax\n\t" \
+ "mov %%rax, %[expected_rip]\n\t" \
+ "popq %%rax\n\t"
+#endif
#define GP_ASM(stmt, in, clobber) \
asm volatile ( \
- "mov" W " $1f, %[expected_rip]\n\t" \
+ GP_ASM_MOVE_TO_RIP \
"movl $2f-1f, %[skip_count]\n\t" \
"1: " stmt "\n\t" \
"2: " \
@@ -125,12 +135,18 @@ static noinline int do_ring3(void (*fn)(const char *), const char *arg)
"mov %%dx, %%fs\n\t"
"mov %%dx, %%gs\n\t"
"mov %%" R "sp, %[sp0]\n\t" /* kernel sp for exception handlers */
+ "mov %[sp0], %%" R "bx\n\t" /* ebx/rbx is preserved before and after 'call' instruction */
"push" W " %%" R "dx \n\t"
"lea %[user_stack_top], %%" R "dx \n\t"
"push" W " %%" R "dx \n\t"
"pushf" W "\n\t"
"push" W " %[user_cs] \n\t"
+#ifndef __x86_64__
"push" W " $1f \n\t"
+#else
+ "lea 1f(%%rip), %%rdx \n\t"
+ "pushq %%rdx \n\t"
+#endif
"iret" W "\n"
"1: \n\t"
#ifndef __x86_64__
@@ -140,12 +156,16 @@ static noinline int do_ring3(void (*fn)(const char *), const char *arg)
#ifndef __x86_64__
"pop %%ecx\n\t"
#endif
+#ifndef __x86_64__
"mov $1f, %%" R "dx\n\t"
+#else
+ "lea 1f(%%" R "ip), %%" R "dx\n\t"
+#endif
"int %[kernel_entry_vector]\n\t"
".section .text.entry \n\t"
"kernel_entry: \n\t"
#ifdef __x86_64__
- "mov %[sp0], %%" R "sp\n\t"
+ "mov %%rbx, %%rsp\n\t"
#else
"add $(5 * " S "), %%esp\n\t"
#endif
@@ -171,7 +191,7 @@ static noinline int do_ring3(void (*fn)(const char *), const char *arg)
[arg]"D"(arg),
[kernel_ds]"i"(KERNEL_DS),
[kernel_entry_vector]"i"(0x20)
- : "rcx", "rdx");
+ : "rcx", "rdx", "rbx");
return ret;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [kvm-unit-tests PATCH v1 4/7] x86 UEFI: Set UEFI OVMF as readonly
2021-10-31 5:56 [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Zixuan Wang
` (2 preceding siblings ...)
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 3/7] x86 UEFI: Convert x86 test cases to PIC Zixuan Wang
@ 2021-10-31 5:56 ` Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 5/7] x86 UEFI: Exit QEMU with return code Zixuan Wang
` (3 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Zixuan Wang @ 2021-10-31 5:56 UTC (permalink / raw)
To: kvm, pbonzini, drjones
Cc: marcorr, erdemaktas, rientjes, seanjc, brijesh.singh,
Thomas.Lendacky, varad.gautam, jroedel, bp
From: Zixuan Wang <zxwang42@gmail.com>
Set readonly for UEFI OVMF image.
Signed-off-by: Zixuan Wang <zxwang42@gmail.com>
---
x86/efi/run | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/x86/efi/run b/x86/efi/run
index a47c0d5..922b266 100755
--- a/x86/efi/run
+++ b/x86/efi/run
@@ -54,7 +54,7 @@ popd || exit 2
# 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="$EFI_UEFI",format=raw,if=pflash,readonly=on \
-drive file.dir="$EFI_TEST/$EFI_CASE/",file.driver=vvfat,file.rw=on,format=raw,if=virtio \
-net none \
-nographic \
--
2.33.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [kvm-unit-tests PATCH v1 5/7] x86 UEFI: Exit QEMU with return code
2021-10-31 5:56 [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Zixuan Wang
` (3 preceding siblings ...)
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 4/7] x86 UEFI: Set UEFI OVMF as readonly Zixuan Wang
@ 2021-10-31 5:56 ` Zixuan Wang
2021-10-31 10:01 ` Paolo Bonzini
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 6/7] scripts: Generalize EFI check Zixuan Wang
` (2 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Zixuan Wang @ 2021-10-31 5:56 UTC (permalink / raw)
To: kvm, pbonzini, drjones
Cc: marcorr, erdemaktas, rientjes, seanjc, brijesh.singh,
Thomas.Lendacky, varad.gautam, jroedel, bp
From: Zixuan Wang <zxwang42@gmail.com>
kvm-unit-tests runner scripts parse QEMU exit code to determine if a
test case runs successfully. But the UEFI 'reset_system' function always
exits QEMU with code 0, even if the test case returns a non-zero code.
This commit fixes this issue by replacing the 'reset_system' call with
an 'exit' call, which ensures QEMU exit with the correct code.
Signed-off-by: Zixuan Wang <zxwang42@gmail.com>
---
lib/efi.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/efi.c b/lib/efi.c
index 99eb00c..cc0386c 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -87,7 +87,7 @@ efi_status_t efi_get_system_config_table(efi_guid_t table_guid, void **table)
efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
{
- int ret;
+ unsigned long ret;
efi_status_t status;
efi_bootinfo_t efi_bootinfo;
@@ -134,14 +134,14 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
ret = main(__argc, __argv, __environ);
/* Shutdown the guest VM */
- efi_rs_call(reset_system, EFI_RESET_SHUTDOWN, ret, 0, NULL);
+ exit(ret);
/* Unreachable */
return EFI_UNSUPPORTED;
efi_main_error:
/* Shutdown the guest with error EFI status */
- efi_rs_call(reset_system, EFI_RESET_SHUTDOWN, status, 0, NULL);
+ exit(status);
/* Unreachable */
return EFI_UNSUPPORTED;
--
2.33.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [kvm-unit-tests PATCH v1 6/7] scripts: Generalize EFI check
2021-10-31 5:56 [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Zixuan Wang
` (4 preceding siblings ...)
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 5/7] x86 UEFI: Exit QEMU with return code Zixuan Wang
@ 2021-10-31 5:56 ` Zixuan Wang
2021-10-31 7:13 ` Paolo Bonzini
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 7/7] x86 UEFI: Make run_tests.sh (mostly) work under UEFI Zixuan Wang
2021-10-31 7:28 ` [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Paolo Bonzini
7 siblings, 1 reply; 19+ messages in thread
From: Zixuan Wang @ 2021-10-31 5:56 UTC (permalink / raw)
To: kvm, pbonzini, drjones
Cc: marcorr, erdemaktas, rientjes, seanjc, brijesh.singh,
Thomas.Lendacky, varad.gautam, jroedel, bp
From: Marc Orr <marcorr@google.com>
Previously, the scripts distinguish between seabios and UEFI via a
hard-coded env var in the EFI run script, `arch/x86/efi/run`.
Furthermore, this var is passed to the x86 run script, `arch/x86/run`,
and then not available in other scripts (or to other architectures).
Replace the previous approach with a common helper function to check
whether the repo has been configured to run under EFI. The helper does
this by probing the `config.mak` file generated by `configure`.
Signed-off-by: Marc Orr <marcorr@google.com>
---
scripts/common.bash | 5 +++++
x86/efi/run | 1 -
x86/run | 6 ++++--
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/scripts/common.bash b/scripts/common.bash
index 7b983f7..6f45843 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -1,5 +1,10 @@
source config.mak
+function running_under_efi()
+{
+ [[ ${TARGET_EFI} == "y" ]] && echo "y" || echo ""
+}
+
function for_each_unittest()
{
local unittests="$1"
diff --git a/x86/efi/run b/x86/efi/run
index 922b266..aacc691 100755
--- a/x86/efi/run
+++ b/x86/efi/run
@@ -52,7 +52,6 @@ popd || exit 2
# 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,readonly=on \
-drive file.dir="$EFI_TEST/$EFI_CASE/",file.driver=vvfat,file.rw=on,format=raw,if=virtio \
diff --git a/x86/run b/x86/run
index 4eba2b9..95b56b6 100755
--- a/x86/run
+++ b/x86/run
@@ -1,5 +1,7 @@
#!/usr/bin/env bash
+source scripts/common.bash
+
if [ -z "$STANDALONE" ]; then
if [ ! -f config.mak ]; then
echo "run ./configure && make first. See ./configure -h"
@@ -39,12 +41,12 @@ fi
command="${qemu} --no-reboot -nodefaults $pc_testdev -vnc none -serial stdio $pci_testdev"
command+=" -machine accel=$ACCEL"
-if ! [ "$EFI_RUN" ]; then
+if ! [ "$(running_under_efi)" ]; then
command+=" -kernel"
fi
command="$(timeout_cmd) $command"
-if [ "$EFI_RUN" ]; then
+if [ "$(running_under_efi)" ]; 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
--
2.33.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [kvm-unit-tests PATCH v1 7/7] x86 UEFI: Make run_tests.sh (mostly) work under UEFI
2021-10-31 5:56 [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Zixuan Wang
` (5 preceding siblings ...)
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 6/7] scripts: Generalize EFI check Zixuan Wang
@ 2021-10-31 5:56 ` Zixuan Wang
2021-10-31 7:28 ` [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Paolo Bonzini
7 siblings, 0 replies; 19+ messages in thread
From: Zixuan Wang @ 2021-10-31 5:56 UTC (permalink / raw)
To: kvm, pbonzini, drjones
Cc: marcorr, erdemaktas, rientjes, seanjc, brijesh.singh,
Thomas.Lendacky, varad.gautam, jroedel, bp
From: Marc Orr <marcorr@google.com>
Make several fixes so that run_tests.sh can run the test cases when
the repo is configured to run under UEFI. Specifically:
1. The scripts are enlighted to run the .efi test cases rather than the
.flat test cases.
2. The scripts are enlighted to run an empty file, _NO_FILE_4Uhere_,
which is used to sanity check QEMU and QEMU flags.
3. The scripts are updated to always run with `-smp 1`, since SMP is not
yet supported under UEFI.
Notably, QEMU's `-append` flag still does not work. This will need to be
fixed in future commits. For now, test cases that use `-append` are
marked `SKIP`.
Signed-off-by: Marc Orr <marcorr@google.com>
---
scripts/common.bash | 4 ++--
scripts/runtime.bash | 15 +++++++++++++--
x86/efi/run | 24 ++++++++++++++++++++----
3 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/scripts/common.bash b/scripts/common.bash
index 6f45843..c42df95 100644
--- a/scripts/common.bash
+++ b/scripts/common.bash
@@ -26,7 +26,7 @@ function for_each_unittest()
if [[ "$line" =~ ^\[(.*)\]$ ]]; then
rematch=${BASH_REMATCH[1]}
if [ -n "${testname}" ]; then
- $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+ $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" "$(running_under_efi)"
fi
testname=$rematch
smp=1
@@ -56,7 +56,7 @@ function for_each_unittest()
fi
done
if [ -n "${testname}" ]; then
- $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
+ $(arch_cmd) "$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout" "$(running_under_efi)"
fi
exec {fd}<&-
}
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 132389c..9c89a74 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -81,6 +81,11 @@ function run()
local check="${CHECK:-$7}"
local accel="$8"
local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the default
+ local running_under_efi="${10}"
+
+ if [ "$running_under_efi" ]; then
+ kernel=$(basename $kernel .flat)
+ fi
if [ -z "$testname" ]; then
return
@@ -127,8 +132,14 @@ function run()
fi
last_line=$(premature_failure > >(tail -1)) && {
- print_result "SKIP" $testname "" "$last_line"
- return 77
+ skip=true
+ if [ "${running_under_efi}" ] && [[ "${last_line}" =~ "Reset" ]]; then
+ skip=false
+ fi
+ if [ ${skip} == true ]; then
+ print_result "SKIP" $testname "" "$last_line"
+ return 77
+ fi
}
cmdline=$(get_cmdline $kernel)
diff --git a/x86/efi/run b/x86/efi/run
index aacc691..e6486ed 100755
--- a/x86/efi/run
+++ b/x86/efi/run
@@ -34,16 +34,27 @@ shift 1
# 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/"
+if [ $EFI_CASE != "_NO_FILE_4Uhere_" ]; then
+ cp "$EFI_SRC/$EFI_CASE.efi" "$EFI_TEST/$EFI_CASE/"
+else
+ touch "$EFI_TEST/$EFI_CASE/$EFI_CASE.efi"
+fi
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
+if [ $EFI_CASE != "_NO_FILE_4Uhere_" ]; then
+ cat << EOF >startup.nsh
@echo -off
fs0:
"$EFI_CASE.efi"
EOF
+else
+ cat << EOF >startup.nsh
+@echo -off
+reset -s
+EOF
+fi
popd || exit 2
# Run test case with 256MiB QEMU memory. QEMU default memory size is 128MiB.
@@ -52,11 +63,16 @@ popd || exit 2
# 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.
+#
+# Also, pass in an EFI-specific smp count (i.e., `-smp 1`) as the last argument
+# to x86/run. This `smp` flag overrides any previous `smp` flags (e.g.,
+# `-smp 4`). This is necessary because KVM-Unit-Tests do not currently support
+# SMP under UEFI. This last flag should be removed when this issue is resolved.
"$TEST_DIR/run" \
-drive file="$EFI_UEFI",format=raw,if=pflash,readonly=on \
-drive file.dir="$EFI_TEST/$EFI_CASE/",file.driver=vvfat,file.rw=on,format=raw,if=virtio \
-net none \
-nographic \
- -smp "$EFI_SMP" \
-m 256 \
- "$@"
+ "$@" \
+ -smp "$EFI_SMP"
--
2.33.0
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH v1 6/7] scripts: Generalize EFI check
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 6/7] scripts: Generalize EFI check Zixuan Wang
@ 2021-10-31 7:13 ` Paolo Bonzini
2021-10-31 15:35 ` Marc Orr
0 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2021-10-31 7:13 UTC (permalink / raw)
To: Zixuan Wang, kvm, drjones
Cc: marcorr, erdemaktas, rientjes, seanjc, brijesh.singh,
Thomas.Lendacky, varad.gautam, jroedel, bp
On 31/10/21 06:56, Zixuan Wang wrote:
> From: Marc Orr<marcorr@google.com>
>
> Previously, the scripts distinguish between seabios and UEFI via a
> hard-coded env var in the EFI run script, `arch/x86/efi/run`.
> Furthermore, this var is passed to the x86 run script, `arch/x86/run`,
> and then not available in other scripts (or to other architectures).
>
> Replace the previous approach with a common helper function to check
> whether the repo has been configured to run under EFI. The helper does
> this by probing the `config.mak` file generated by `configure`.
It should be possible to just use
[ "${TARGET_EFI}" == "y" ]
as the test:
diff --git a/x86/efi/run b/x86/efi/run
index 922b266..aacc691 100755
--- a/x86/efi/run
+++ b/x86/efi/run
@@ -52,7 +52,6 @@ popd || exit 2
# 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,readonly=on \
-drive file.dir="$EFI_TEST/$EFI_CASE/",file.driver=vvfat,file.rw=on,format=raw,if=virtio \
diff --git a/x86/run b/x86/run
index 4eba2b9..0a4dda9 100755
--- a/x86/run
+++ b/x86/run
@@ -39,12 +39,12 @@ fi
command="${qemu} --no-reboot -nodefaults $pc_testdev -vnc none -serial stdio $pci_testdev"
command+=" -machine accel=$ACCEL"
-if ! [ "$EFI_RUN" ]; then
+if [ ${TARGET_EFI} != "y" ]; then
command+=" -kernel"
fi
command="$(timeout_cmd) $command"
-if [ "$EFI_RUN" ]; then
+if [ ${TARGET_EFI} = "y" ]; 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
Paolo
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes
2021-10-31 5:56 [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Zixuan Wang
` (6 preceding siblings ...)
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 7/7] x86 UEFI: Make run_tests.sh (mostly) work under UEFI Zixuan Wang
@ 2021-10-31 7:28 ` Paolo Bonzini
2021-10-31 16:14 ` Marc Orr
7 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2021-10-31 7:28 UTC (permalink / raw)
To: Zixuan Wang, kvm, drjones
Cc: marcorr, erdemaktas, rientjes, seanjc, brijesh.singh,
Thomas.Lendacky, varad.gautam, jroedel, bp
On 31/10/21 06:56, Zixuan Wang wrote:
> Hello,
>
> This patch series refactors the x86_64 UEFI set up process and fixes the
> `run-tests.sh` script to run under UEFI. The patches are organized as
> three parts.
>
> The first part (patches 1-2) refactors the x86_64 UEFI set up process.
> The previous UEFI setup calls arch-specific setup functions twice and
> generates arch-specific data structure. As Andrew suggested [1], we
> refactor this process to make only one call to the arch-specific
> function and generate arch-neutral data structures. This simplifies the
> set up process and makes it easier to develop UEFI support for other
> architectures.
>
> The second part (patch 3) converts several x86 test cases to
> Position-Independent Code (PIC) to run under UEFI. This patch is ported
> from the initial UEFI support patchset [2] with fixes to the 32-bit
> compilation.
>
> The third part (patches 4-7) fixes the UEFI runner scripts. Patch 4 sets
> UEFI OVMF image as readonly. Patch 5 fixes test cases' return code under
> UEFI, enabling Patch 6-7 to fix the `run-tests.sh` script under UEFI.
>
> This patch set is based on the `uefi` branch.
Thank you, for patches 1-6 I have squashed the patches when applicable
(1, 4, 5, 6) and queued the others (2 and 3).
I did not queue patch 7 yet, it seems okay but I want to understand
better the changes it needs in the harness and what is missing. I'll
take a look during the week.
Paolo
> Best regards,
> Zixuan Wang and Marc Orr
>
> [1] https://lore.kernel.org/kvm/20211005060549.clar5nakynz2zecl@gator.home/
> [2] https://lore.kernel.org/kvm/20211004204931.1537823-1-zxwang42@gmail.com/
>
> Marc Orr (2):
> scripts: Generalize EFI check
> x86 UEFI: Make run_tests.sh (mostly) work under UEFI
>
> Zixuan Wang (5):
> x86 UEFI: Remove mixed_mode
> x86 UEFI: Refactor set up process
> x86 UEFI: Convert x86 test cases to PIC
> x86 UEFI: Set UEFI OVMF as readonly
> x86 UEFI: Exit QEMU with return code
>
> lib/efi.c | 54 ++++++--
> lib/efi.h | 19 ++-
> lib/linux/efi.h | 317 ++++++++++++++-----------------------------
> lib/x86/acpi.c | 36 +++--
> lib/x86/acpi.h | 5 +-
> lib/x86/asm/setup.h | 16 +--
> lib/x86/setup.c | 153 ++++++++++-----------
> lib/x86/usermode.c | 3 +-
> scripts/common.bash | 9 +-
> scripts/runtime.bash | 15 +-
> x86/Makefile.common | 10 +-
> x86/Makefile.x86_64 | 7 +-
> x86/access.c | 9 +-
> x86/cet.c | 8 +-
> x86/efi/run | 27 +++-
> x86/emulator.c | 5 +-
> x86/eventinj.c | 8 ++
> x86/run | 6 +-
> x86/smap.c | 13 +-
> x86/umip.c | 26 +++-
> 20 files changed, 360 insertions(+), 386 deletions(-)
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH v1 5/7] x86 UEFI: Exit QEMU with return code
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 5/7] x86 UEFI: Exit QEMU with return code Zixuan Wang
@ 2021-10-31 10:01 ` Paolo Bonzini
2021-10-31 21:36 ` Zixuan Wang
0 siblings, 1 reply; 19+ messages in thread
From: Paolo Bonzini @ 2021-10-31 10:01 UTC (permalink / raw)
To: Zixuan Wang, kvm, drjones
Cc: marcorr, erdemaktas, rientjes, seanjc, brijesh.singh,
Thomas.Lendacky, varad.gautam, jroedel, bp
On 31/10/21 06:56, Zixuan Wang wrote:
> From: Zixuan Wang <zxwang42@gmail.com>
>
> kvm-unit-tests runner scripts parse QEMU exit code to determine if a
> test case runs successfully. But the UEFI 'reset_system' function always
> exits QEMU with code 0, even if the test case returns a non-zero code.
>
> This commit fixes this issue by replacing the 'reset_system' call with
> an 'exit' call, which ensures QEMU exit with the correct code.
>
> Signed-off-by: Zixuan Wang <zxwang42@gmail.com>
> ---
> lib/efi.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/efi.c b/lib/efi.c
> index 99eb00c..cc0386c 100644
> --- a/lib/efi.c
> +++ b/lib/efi.c
> @@ -87,7 +87,7 @@ efi_status_t efi_get_system_config_table(efi_guid_t table_guid, void **table)
>
> efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
> {
> - int ret;
> + unsigned long ret;
Why this change?
> efi_status_t status;
> efi_bootinfo_t efi_bootinfo;
>
> @@ -134,14 +134,14 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
> ret = main(__argc, __argv, __environ);
>
> /* Shutdown the guest VM */
> - efi_rs_call(reset_system, EFI_RESET_SHUTDOWN, ret, 0, NULL);
> + exit(ret);
>
> /* Unreachable */
> return EFI_UNSUPPORTED;
>
> efi_main_error:
> /* Shutdown the guest with error EFI status */
> - efi_rs_call(reset_system, EFI_RESET_SHUTDOWN, status, 0, NULL);
> + exit(status);
>
> /* Unreachable */
> return EFI_UNSUPPORTED;
It's better to keep the exit() *and* the efi_rs_call(), I think, in case
the testdev is missing and therefore the exit() does not work.
Paolo
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH v1 6/7] scripts: Generalize EFI check
2021-10-31 7:13 ` Paolo Bonzini
@ 2021-10-31 15:35 ` Marc Orr
0 siblings, 0 replies; 19+ messages in thread
From: Marc Orr @ 2021-10-31 15:35 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Zixuan Wang, kvm, drjones, erdemaktas, rientjes, seanjc,
brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp
On Sun, Oct 31, 2021 at 12:13 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 31/10/21 06:56, Zixuan Wang wrote:
> > From: Marc Orr<marcorr@google.com>
> >
> > Previously, the scripts distinguish between seabios and UEFI via a
> > hard-coded env var in the EFI run script, `arch/x86/efi/run`.
> > Furthermore, this var is passed to the x86 run script, `arch/x86/run`,
> > and then not available in other scripts (or to other architectures).
> >
> > Replace the previous approach with a common helper function to check
> > whether the repo has been configured to run under EFI. The helper does
> > this by probing the `config.mak` file generated by `configure`.
>
> It should be possible to just use
>
> [ "${TARGET_EFI}" == "y" ]
>
> as the test:
>
> diff --git a/x86/efi/run b/x86/efi/run
> index 922b266..aacc691 100755
> --- a/x86/efi/run
> +++ b/x86/efi/run
> @@ -52,7 +52,6 @@ popd || exit 2
> # 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,readonly=on \
> -drive file.dir="$EFI_TEST/$EFI_CASE/",file.driver=vvfat,file.rw=on,format=raw,if=virtio \
> diff --git a/x86/run b/x86/run
> index 4eba2b9..0a4dda9 100755
> --- a/x86/run
> +++ b/x86/run
> @@ -39,12 +39,12 @@ fi
>
> command="${qemu} --no-reboot -nodefaults $pc_testdev -vnc none -serial stdio $pci_testdev"
> command+=" -machine accel=$ACCEL"
> -if ! [ "$EFI_RUN" ]; then
> +if [ ${TARGET_EFI} != "y" ]; then
> command+=" -kernel"
> fi
> command="$(timeout_cmd) $command"
>
> -if [ "$EFI_RUN" ]; then
> +if [ ${TARGET_EFI} = "y" ]; 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
>
> Paolo
>
Agreed. That SGTM.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes
2021-10-31 7:28 ` [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Paolo Bonzini
@ 2021-10-31 16:14 ` Marc Orr
2021-10-31 21:54 ` Zixuan Wang
0 siblings, 1 reply; 19+ messages in thread
From: Marc Orr @ 2021-10-31 16:14 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Zixuan Wang, kvm, drjones, erdemaktas, rientjes, seanjc,
brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp
On Sun, Oct 31, 2021 at 12:28 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 31/10/21 06:56, Zixuan Wang wrote:
> > Hello,
> >
> > This patch series refactors the x86_64 UEFI set up process and fixes the
> > `run-tests.sh` script to run under UEFI. The patches are organized as
> > three parts.
> >
> > The first part (patches 1-2) refactors the x86_64 UEFI set up process.
> > The previous UEFI setup calls arch-specific setup functions twice and
> > generates arch-specific data structure. As Andrew suggested [1], we
> > refactor this process to make only one call to the arch-specific
> > function and generate arch-neutral data structures. This simplifies the
> > set up process and makes it easier to develop UEFI support for other
> > architectures.
> >
> > The second part (patch 3) converts several x86 test cases to
> > Position-Independent Code (PIC) to run under UEFI. This patch is ported
> > from the initial UEFI support patchset [2] with fixes to the 32-bit
> > compilation.
> >
> > The third part (patches 4-7) fixes the UEFI runner scripts. Patch 4 sets
> > UEFI OVMF image as readonly. Patch 5 fixes test cases' return code under
> > UEFI, enabling Patch 6-7 to fix the `run-tests.sh` script under UEFI.
> >
> > This patch set is based on the `uefi` branch.
>
> Thank you, for patches 1-6 I have squashed the patches when applicable
> (1, 4, 5, 6) and queued the others (2 and 3).
>
> I did not queue patch 7 yet, it seems okay but I want to understand
> better the changes it needs in the harness and what is missing. I'll
> take a look during the week.
SGTM, thank you! Zixuan and I discussed a few things that are missing:
1. Test cases that take the `-append` arg are currently marked `SKIP`.
Two issues need to be resolved here. First, we're not using QEMU's
`-kernel` flag for EFI test cases [1]. And the `-append` flag does not
work without the `-kernel` flag. I don't understand the details on why
we don't use the `-kernel` flag myself. Maybe Zixuan can elaborate.
Second, assuming we fix the first issue, then we need to enlighten the
KVM-Unit-Tests under UEFI to parse kernel command line arguments and
pass them down to the test cases via `argv`. Zixuan pointed out to me
that there is some prior work from Drew [2] that we should be able to
follow to make this work. So I'm hoping that Zixuan and I can work
together on solving these issues to get the argument passing working
next.
2. We need a way to annotate test cases in `x86/unittests.cfg` as
known to work under SEV. I'm thinking of doing this via new (very
broad) test groups in `unittests.cfg`. I _think_ SEV is the primary
scenario we care about. However, folks may care about running the test
cases under UEFI outside of SEV. For example, last time I checked,
emulator runs OK under UEFI minus SEV-ES but fails under SEV-ES. And
similarly, while most test cases work under UEFI minus SEV, there are
a few that do mis-behave -- and it probably makes sense to document
this (e.g., via annotations in `unittests.cfg`). Also, there are many
variations of SEV (SEV, SEV-ES, SEV-SNP)... And hopefully some of this
will eventually be applicable to TDX as well. So many testgroups is
not a good solution. I'm not sure.
3. Multi-CPU needs to be made to work under UEFI. For now, patch #7
forces all EFI test cases to run with 1 vCPU. I chatted with Brijesh,
and he mentioned that Varad would like to work on this. However, if
anything here changes, please let me know, because we can work on this
as well. But for now, I'm not planning to work on it so we can avoid
duplicating work.
4. UEFI runs a lot slower than SEABIOS. It doesn't help that the test
harness launches QEMU more than once for each test case (i.e., it runs
the `_NO_FILE_4Uhere_` scenario to check QEMU arguments). I'm not sure
how much of an issue this is in practice. Depending on the answer, I
know Zixuan had some ideas on how to speed this up in the current test
harness. Or maybe we can explore an alternative to the
`_NO_FILE_4Uhere_` approach instead.
Zixuan: Please add/correct anything as needed!
[1] https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/blob/uefi/x86/run#L42-44
[2] https://github.com/rhdrjones/kvm-unit-tests/blob/target-efi/scripts/mkefi.sh
Thanks,
Marc
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH v1 5/7] x86 UEFI: Exit QEMU with return code
2021-10-31 10:01 ` Paolo Bonzini
@ 2021-10-31 21:36 ` Zixuan Wang
0 siblings, 0 replies; 19+ messages in thread
From: Zixuan Wang @ 2021-10-31 21:36 UTC (permalink / raw)
To: Paolo Bonzini
Cc: kvm list, Andrew Jones, Marc Orr, Erdem Aktas, David Rientjes,
Sean Christopherson, Singh, Brijesh, Lendacky, Thomas,
Varad Gautam, Joerg Roedel, bp
On Sun, Oct 31, 2021 at 3:02 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 31/10/21 06:56, Zixuan Wang wrote:
> > From: Zixuan Wang <zxwang42@gmail.com>
> > efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
> > {
> > - int ret;
> > + unsigned long ret;
>
> Why this change?
Didn't notice this, it should be int, thanks for pointing it out!
> > efi_status_t status;
> > efi_bootinfo_t efi_bootinfo;
> >
> > @@ -134,14 +134,14 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
> > ret = main(__argc, __argv, __environ);
> >
> > /* Shutdown the guest VM */
> > - efi_rs_call(reset_system, EFI_RESET_SHUTDOWN, ret, 0, NULL);
> > + exit(ret);
> >
> > /* Unreachable */
> > return EFI_UNSUPPORTED;
> >
> > efi_main_error:
> > /* Shutdown the guest with error EFI status */
> > - efi_rs_call(reset_system, EFI_RESET_SHUTDOWN, status, 0, NULL);
> > + exit(status);
> >
> > /* Unreachable */
> > return EFI_UNSUPPORTED;
>
> It's better to keep the exit() *and* the efi_rs_call(), I think, in case
> the testdev is missing and therefore the exit() does not work.
>
> Paolo
>
I agree, I think there are three possible solutions:
1. keep both exit() and efi_rs_call() here, or
2. define a new function efi_exit() that calls both exit() and efi_rs_call(), or
3. add efi_rs_call() to the end of exit() function (defined in
lib/x86/io.c), so many other calls to exit() can utilize EFI exit as a
backup
Best regards,
Zixuan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes
2021-10-31 16:14 ` Marc Orr
@ 2021-10-31 21:54 ` Zixuan Wang
2021-11-01 7:11 ` Andrew Jones
0 siblings, 1 reply; 19+ messages in thread
From: Zixuan Wang @ 2021-10-31 21:54 UTC (permalink / raw)
To: Marc Orr
Cc: Paolo Bonzini, kvm list, Andrew Jones, Erdem Aktas,
David Rientjes, Sean Christopherson, Singh, Brijesh, Lendacky,
Thomas, Varad Gautam, Joerg Roedel, bp
On Sun, Oct 31, 2021 at 9:14 AM Marc Orr <marcorr@google.com> wrote:
>
> On Sun, Oct 31, 2021 at 12:28 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 31/10/21 06:56, Zixuan Wang wrote:
> > > Hello,
> > >
> > > This patch series refactors the x86_64 UEFI set up process and fixes the
> > > `run-tests.sh` script to run under UEFI. The patches are organized as
> > > three parts.
> > >
> > > The first part (patches 1-2) refactors the x86_64 UEFI set up process.
> > > The previous UEFI setup calls arch-specific setup functions twice and
> > > generates arch-specific data structure. As Andrew suggested [1], we
> > > refactor this process to make only one call to the arch-specific
> > > function and generate arch-neutral data structures. This simplifies the
> > > set up process and makes it easier to develop UEFI support for other
> > > architectures.
> > >
> > > The second part (patch 3) converts several x86 test cases to
> > > Position-Independent Code (PIC) to run under UEFI. This patch is ported
> > > from the initial UEFI support patchset [2] with fixes to the 32-bit
> > > compilation.
> > >
> > > The third part (patches 4-7) fixes the UEFI runner scripts. Patch 4 sets
> > > UEFI OVMF image as readonly. Patch 5 fixes test cases' return code under
> > > UEFI, enabling Patch 6-7 to fix the `run-tests.sh` script under UEFI.
> > >
> > > This patch set is based on the `uefi` branch.
> >
> > Thank you, for patches 1-6 I have squashed the patches when applicable
> > (1, 4, 5, 6) and queued the others (2 and 3).
> >
> > I did not queue patch 7 yet, it seems okay but I want to understand
> > better the changes it needs in the harness and what is missing. I'll
> > take a look during the week.
>
> SGTM, thank you! Zixuan and I discussed a few things that are missing:
>
> 1. Test cases that take the `-append` arg are currently marked `SKIP`.
> Two issues need to be resolved here. First, we're not using QEMU's
> `-kernel` flag for EFI test cases [1]. And the `-append` flag does not
> work without the `-kernel` flag. I don't understand the details on why
> we don't use the `-kernel` flag myself. Maybe Zixuan can elaborate.
> Second, assuming we fix the first issue, then we need to enlighten the
> KVM-Unit-Tests under UEFI to parse kernel command line arguments and
> pass them down to the test cases via `argv`. Zixuan pointed out to me
> that there is some prior work from Drew [2] that we should be able to
> follow to make this work. So I'm hoping that Zixuan and I can work
> together on solving these issues to get the argument passing working
> next.
Thank you for the detailed summary!
Current kvm-unit-tests pass an EFI binary as part of a disk image,
instead of using the `-kernel` argument.
I just tested the `-kernel` argument and it seems to work with EFI
binaries, and more importantly, it's really fast (bypassing the
default 5-second user input waiting). I will update the `x86/efi/run`
to use `-kernel` argument to pass the EFI binaries.
Since `-kernel` is working, I can start to investigate how to use
`-append` to pass arguments. If that doesn't work well, an alternative
approach could be:
1. (host) create a file `args.txt` in the disk image, which contains
all the arguments needed
2. (guest) call UEFI filesystem interface to read this `args.txt` from
the disk image, parse it and pass the arguments to `main()`
> 2. We need a way to annotate test cases in `x86/unittests.cfg` as
> known to work under SEV. I'm thinking of doing this via new (very
> broad) test groups in `unittests.cfg`. I _think_ SEV is the primary
> scenario we care about. However, folks may care about running the test
> cases under UEFI outside of SEV. For example, last time I checked,
> emulator runs OK under UEFI minus SEV-ES but fails under SEV-ES. And
> similarly, while most test cases work under UEFI minus SEV, there are
> a few that do mis-behave -- and it probably makes sense to document
> this (e.g., via annotations in `unittests.cfg`). Also, there are many
> variations of SEV (SEV, SEV-ES, SEV-SNP)... And hopefully some of this
> will eventually be applicable to TDX as well. So many testgroups is
> not a good solution. I'm not sure.
Adding an `efi` group seems helpful. E.g., the current `x86/smap.c`
does not work under UEFI; but the `run-tests.sh` still tries to run
this test case, even if this test case is not compiled.
> 3. Multi-CPU needs to be made to work under UEFI. For now, patch #7
> forces all EFI test cases to run with 1 vCPU. I chatted with Brijesh,
> and he mentioned that Varad would like to work on this. However, if
> anything here changes, please let me know, because we can work on this
> as well. But for now, I'm not planning to work on it so we can avoid
> duplicating work.
> 4. UEFI runs a lot slower than SEABIOS. It doesn't help that the test
> harness launches QEMU more than once for each test case (i.e., it runs
> the `_NO_FILE_4Uhere_` scenario to check QEMU arguments). I'm not sure
> how much of an issue this is in practice. Depending on the answer, I
> know Zixuan had some ideas on how to speed this up in the current test
> harness. Or maybe we can explore an alternative to the
> `_NO_FILE_4Uhere_` approach instead.
As the `-kernel` argument now works with the EFI binaries and is
significantly faster, this should not be an issue anymore. We just
need to update the runner scripts to use `-kernel` argument.
> Zixuan: Please add/correct anything as needed!
>
> [1] https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/blob/uefi/x86/run#L42-44
> [2] https://github.com/rhdrjones/kvm-unit-tests/blob/target-efi/scripts/mkefi.sh
>
> Thanks,
> Marc
Best regards,
Zixuan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes
2021-10-31 21:54 ` Zixuan Wang
@ 2021-11-01 7:11 ` Andrew Jones
2021-11-01 22:35 ` Zixuan Wang
0 siblings, 1 reply; 19+ messages in thread
From: Andrew Jones @ 2021-11-01 7:11 UTC (permalink / raw)
To: Zixuan Wang
Cc: Marc Orr, Paolo Bonzini, kvm list, Erdem Aktas, David Rientjes,
Sean Christopherson, Singh, Brijesh, Lendacky, Thomas,
Varad Gautam, Joerg Roedel, bp
On Sun, Oct 31, 2021 at 02:54:00PM -0700, Zixuan Wang wrote:
> On Sun, Oct 31, 2021 at 9:14 AM Marc Orr <marcorr@google.com> wrote:
> >
> > On Sun, Oct 31, 2021 at 12:28 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >
> > > On 31/10/21 06:56, Zixuan Wang wrote:
> > > > Hello,
> > > >
> > > > This patch series refactors the x86_64 UEFI set up process and fixes the
> > > > `run-tests.sh` script to run under UEFI. The patches are organized as
> > > > three parts.
> > > >
> > > > The first part (patches 1-2) refactors the x86_64 UEFI set up process.
> > > > The previous UEFI setup calls arch-specific setup functions twice and
> > > > generates arch-specific data structure. As Andrew suggested [1], we
> > > > refactor this process to make only one call to the arch-specific
> > > > function and generate arch-neutral data structures. This simplifies the
> > > > set up process and makes it easier to develop UEFI support for other
> > > > architectures.
> > > >
> > > > The second part (patch 3) converts several x86 test cases to
> > > > Position-Independent Code (PIC) to run under UEFI. This patch is ported
> > > > from the initial UEFI support patchset [2] with fixes to the 32-bit
> > > > compilation.
> > > >
> > > > The third part (patches 4-7) fixes the UEFI runner scripts. Patch 4 sets
> > > > UEFI OVMF image as readonly. Patch 5 fixes test cases' return code under
> > > > UEFI, enabling Patch 6-7 to fix the `run-tests.sh` script under UEFI.
> > > >
> > > > This patch set is based on the `uefi` branch.
> > >
> > > Thank you, for patches 1-6 I have squashed the patches when applicable
> > > (1, 4, 5, 6) and queued the others (2 and 3).
> > >
> > > I did not queue patch 7 yet, it seems okay but I want to understand
> > > better the changes it needs in the harness and what is missing. I'll
> > > take a look during the week.
> >
> > SGTM, thank you! Zixuan and I discussed a few things that are missing:
> >
> > 1. Test cases that take the `-append` arg are currently marked `SKIP`.
> > Two issues need to be resolved here. First, we're not using QEMU's
> > `-kernel` flag for EFI test cases [1]. And the `-append` flag does not
> > work without the `-kernel` flag. I don't understand the details on why
> > we don't use the `-kernel` flag myself. Maybe Zixuan can elaborate.
> > Second, assuming we fix the first issue, then we need to enlighten the
> > KVM-Unit-Tests under UEFI to parse kernel command line arguments and
> > pass them down to the test cases via `argv`. Zixuan pointed out to me
> > that there is some prior work from Drew [2] that we should be able to
> > follow to make this work. So I'm hoping that Zixuan and I can work
> > together on solving these issues to get the argument passing working
> > next.
>
> Thank you for the detailed summary!
>
> Current kvm-unit-tests pass an EFI binary as part of a disk image,
> instead of using the `-kernel` argument.
>
> I just tested the `-kernel` argument and it seems to work with EFI
> binaries, and more importantly, it's really fast (bypassing the
> default 5-second user input waiting). I will update the `x86/efi/run`
> to use `-kernel` argument to pass the EFI binaries.
>
> Since `-kernel` is working, I can start to investigate how to use
> `-append` to pass arguments. If that doesn't work well, an alternative
> approach could be:
>
> 1. (host) create a file `args.txt` in the disk image, which contains
> all the arguments needed
> 2. (guest) call UEFI filesystem interface to read this `args.txt` from
> the disk image, parse it and pass the arguments to `main()`
>
> > 2. We need a way to annotate test cases in `x86/unittests.cfg` as
> > known to work under SEV. I'm thinking of doing this via new (very
> > broad) test groups in `unittests.cfg`. I _think_ SEV is the primary
> > scenario we care about. However, folks may care about running the test
> > cases under UEFI outside of SEV. For example, last time I checked,
> > emulator runs OK under UEFI minus SEV-ES but fails under SEV-ES. And
> > similarly, while most test cases work under UEFI minus SEV, there are
> > a few that do mis-behave -- and it probably makes sense to document
> > this (e.g., via annotations in `unittests.cfg`). Also, there are many
> > variations of SEV (SEV, SEV-ES, SEV-SNP)... And hopefully some of this
> > will eventually be applicable to TDX as well. So many testgroups is
> > not a good solution. I'm not sure.
>
> Adding an `efi` group seems helpful. E.g., the current `x86/smap.c`
> does not work under UEFI; but the `run-tests.sh` still tries to run
> this test case, even if this test case is not compiled.
>
> > 3. Multi-CPU needs to be made to work under UEFI. For now, patch #7
> > forces all EFI test cases to run with 1 vCPU. I chatted with Brijesh,
> > and he mentioned that Varad would like to work on this. However, if
> > anything here changes, please let me know, because we can work on this
> > as well. But for now, I'm not planning to work on it so we can avoid
> > duplicating work.
> > 4. UEFI runs a lot slower than SEABIOS. It doesn't help that the test
> > harness launches QEMU more than once for each test case (i.e., it runs
> > the `_NO_FILE_4Uhere_` scenario to check QEMU arguments). I'm not sure
> > how much of an issue this is in practice. Depending on the answer, I
> > know Zixuan had some ideas on how to speed this up in the current test
> > harness. Or maybe we can explore an alternative to the
> > `_NO_FILE_4Uhere_` approach instead.
>
> As the `-kernel` argument now works with the EFI binaries and is
> significantly faster, this should not be an issue anymore. We just
> need to update the runner scripts to use `-kernel` argument.
You can add an additional '-kernel' + EFI binary runner if you want, but
the goal of being able to run kvm-unit-tests on bare-metal means we
shouldn't be counting on QEMU/OVMF to do magic stuff with the kernel. We
need to build disk images. Argument passing works with EFI apps, when
implemented, so that's not a problem. I also created a script that uses
the framework's for_each_unittest to generate an EFI script that allowed
each test to be easily run with its arguments.
Thanks,
drew
>
> > Zixuan: Please add/correct anything as needed!
> >
> > [1] https://gitlab.com/kvm-unit-tests/kvm-unit-tests/-/blob/uefi/x86/run#L42-44
> > [2] https://github.com/rhdrjones/kvm-unit-tests/blob/target-efi/scripts/mkefi.sh
> >
> > Thanks,
> > Marc
>
> Best regards,
> Zixuan
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes
2021-11-01 7:11 ` Andrew Jones
@ 2021-11-01 22:35 ` Zixuan Wang
0 siblings, 0 replies; 19+ messages in thread
From: Zixuan Wang @ 2021-11-01 22:35 UTC (permalink / raw)
To: Andrew Jones
Cc: Marc Orr, Paolo Bonzini, kvm list, Erdem Aktas, David Rientjes,
Sean Christopherson, Singh, Brijesh, Lendacky, Thomas,
Varad Gautam, Joerg Roedel, bp
On Mon, Nov 1, 2021 at 12:11 AM Andrew Jones <drjones@redhat.com> wrote:
>
> On Sun, Oct 31, 2021 at 02:54:00PM -0700, Zixuan Wang wrote:
> > On Sun, Oct 31, 2021 at 9:14 AM Marc Orr <marcorr@google.com> wrote:
> > >
> > > On Sun, Oct 31, 2021 at 12:28 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > >
> > > > On 31/10/21 06:56, Zixuan Wang wrote:
> > > > > Hello,
> > > > >
> > > > > This patch series refactors the x86_64 UEFI set up process and fixes the
> > > > > `run-tests.sh` script to run under UEFI. The patches are organized as
> > > > > three parts.
> > > > >
> > > > > The first part (patches 1-2) refactors the x86_64 UEFI set up process.
> > > > > The previous UEFI setup calls arch-specific setup functions twice and
> > > > > generates arch-specific data structure. As Andrew suggested [1], we
> > > > > refactor this process to make only one call to the arch-specific
> > > > > function and generate arch-neutral data structures. This simplifies the
> > > > > set up process and makes it easier to develop UEFI support for other
> > > > > architectures.
> > > > >
> > > > > The second part (patch 3) converts several x86 test cases to
> > > > > Position-Independent Code (PIC) to run under UEFI. This patch is ported
> > > > > from the initial UEFI support patchset [2] with fixes to the 32-bit
> > > > > compilation.
> > > > >
> > > > > The third part (patches 4-7) fixes the UEFI runner scripts. Patch 4 sets
> > > > > UEFI OVMF image as readonly. Patch 5 fixes test cases' return code under
> > > > > UEFI, enabling Patch 6-7 to fix the `run-tests.sh` script under UEFI.
> > > > >
> > > > > This patch set is based on the `uefi` branch.
> > > >
> > > > Thank you, for patches 1-6 I have squashed the patches when applicable
> > > > (1, 4, 5, 6) and queued the others (2 and 3).
> > > >
> > > > I did not queue patch 7 yet, it seems okay but I want to understand
> > > > better the changes it needs in the harness and what is missing. I'll
> > > > take a look during the week.
> > >
> > > SGTM, thank you! Zixuan and I discussed a few things that are missing:
> > >
> > > 1. Test cases that take the `-append` arg are currently marked `SKIP`.
> > > Two issues need to be resolved here. First, we're not using QEMU's
> > > `-kernel` flag for EFI test cases [1]. And the `-append` flag does not
> > > work without the `-kernel` flag. I don't understand the details on why
> > > we don't use the `-kernel` flag myself. Maybe Zixuan can elaborate.
> > > Second, assuming we fix the first issue, then we need to enlighten the
> > > KVM-Unit-Tests under UEFI to parse kernel command line arguments and
> > > pass them down to the test cases via `argv`. Zixuan pointed out to me
> > > that there is some prior work from Drew [2] that we should be able to
> > > follow to make this work. So I'm hoping that Zixuan and I can work
> > > together on solving these issues to get the argument passing working
> > > next.
> >
> > Thank you for the detailed summary!
> >
> > Current kvm-unit-tests pass an EFI binary as part of a disk image,
> > instead of using the `-kernel` argument.
> >
> > I just tested the `-kernel` argument and it seems to work with EFI
> > binaries, and more importantly, it's really fast (bypassing the
> > default 5-second user input waiting). I will update the `x86/efi/run`
> > to use `-kernel` argument to pass the EFI binaries.
> >
> > Since `-kernel` is working, I can start to investigate how to use
> > `-append` to pass arguments. If that doesn't work well, an alternative
> > approach could be:
> >
> > 1. (host) create a file `args.txt` in the disk image, which contains
> > all the arguments needed
> > 2. (guest) call UEFI filesystem interface to read this `args.txt` from
> > the disk image, parse it and pass the arguments to `main()`
> >
> > > 2. We need a way to annotate test cases in `x86/unittests.cfg` as
> > > known to work under SEV. I'm thinking of doing this via new (very
> > > broad) test groups in `unittests.cfg`. I _think_ SEV is the primary
> > > scenario we care about. However, folks may care about running the test
> > > cases under UEFI outside of SEV. For example, last time I checked,
> > > emulator runs OK under UEFI minus SEV-ES but fails under SEV-ES. And
> > > similarly, while most test cases work under UEFI minus SEV, there are
> > > a few that do mis-behave -- and it probably makes sense to document
> > > this (e.g., via annotations in `unittests.cfg`). Also, there are many
> > > variations of SEV (SEV, SEV-ES, SEV-SNP)... And hopefully some of this
> > > will eventually be applicable to TDX as well. So many testgroups is
> > > not a good solution. I'm not sure.
> >
> > Adding an `efi` group seems helpful. E.g., the current `x86/smap.c`
> > does not work under UEFI; but the `run-tests.sh` still tries to run
> > this test case, even if this test case is not compiled.
> >
> > > 3. Multi-CPU needs to be made to work under UEFI. For now, patch #7
> > > forces all EFI test cases to run with 1 vCPU. I chatted with Brijesh,
> > > and he mentioned that Varad would like to work on this. However, if
> > > anything here changes, please let me know, because we can work on this
> > > as well. But for now, I'm not planning to work on it so we can avoid
> > > duplicating work.
> > > 4. UEFI runs a lot slower than SEABIOS. It doesn't help that the test
> > > harness launches QEMU more than once for each test case (i.e., it runs
> > > the `_NO_FILE_4Uhere_` scenario to check QEMU arguments). I'm not sure
> > > how much of an issue this is in practice. Depending on the answer, I
> > > know Zixuan had some ideas on how to speed this up in the current test
> > > harness. Or maybe we can explore an alternative to the
> > > `_NO_FILE_4Uhere_` approach instead.
> >
> > As the `-kernel` argument now works with the EFI binaries and is
> > significantly faster, this should not be an issue anymore. We just
> > need to update the runner scripts to use `-kernel` argument.
>
> You can add an additional '-kernel' + EFI binary runner if you want, but
> the goal of being able to run kvm-unit-tests on bare-metal means we
> shouldn't be counting on QEMU/OVMF to do magic stuff with the kernel. We
> need to build disk images. Argument passing works with EFI apps, when
> implemented, so that's not a problem. I also created a script that uses
> the framework's for_each_unittest to generate an EFI script that allowed
> each test to be easily run with its arguments.
>
> Thanks,
> drew
I see, I think an alternative approach is to rename test case binaries
to UEFI default binary filename, which is EFI/BOOT/BOOTX64.EFI for
x86_64. This should work just like the `-kernel` argument. I will
explore this approach with the argument passing mechanisms.
Best regards,
Zixuan
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH v1 2/7] x86 UEFI: Refactor set up process
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 2/7] x86 UEFI: Refactor set up process Zixuan Wang
@ 2021-11-05 18:54 ` Sean Christopherson
2021-11-09 17:16 ` Zixuan Wang
0 siblings, 1 reply; 19+ messages in thread
From: Sean Christopherson @ 2021-11-05 18:54 UTC (permalink / raw)
To: Zixuan Wang
Cc: kvm, pbonzini, drjones, marcorr, erdemaktas, rientjes,
brijesh.singh, Thomas.Lendacky, varad.gautam, jroedel, bp
On Sat, Oct 30, 2021, Zixuan Wang wrote:
> +efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo)
> {
> + efi_status_t status;
> +
> + status = setup_memory_allocator(efi_bootinfo);
> + if (status != EFI_SUCCESS) {
> + printf("Failed to set up memory allocator: ");
> + switch (status) {
> + case EFI_OUT_OF_RESOURCES:
> + printf("No free memory region\n");
> + break;
> + default:
> + printf("Unknown error\n");
> + break;
> + }
> + return status;
> + }
> +
> + status = setup_rsdp(efi_bootinfo);
> + if (status != EFI_SUCCESS) {
> + printf("Cannot find RSDP in EFI system table\n");
> + return status;
> + }
> +
> + status = setup_amd_sev();
> + if (status != EFI_SUCCESS) {
> + switch (status) {
> + case EFI_UNSUPPORTED:
> + /* Continue if AMD SEV is not supported */
> + break;
> + default:
> + printf("Set up AMD SEV failed\n");
> + return status;
> + }
> + }
Looks like this is pre-existing behavior, but the switch is quite gratuituous,
and arguably does the wrong thing for EFI_UNSUPPORTED here as attempting to setup
SEV-ES without SEV is guaranteed to fail. And it'd be really nice if the printf()
actually provided the error (below might be wrong, I don't know the type of
efi_status-t).
status = setup_amd_sev();
/* Continue on if AMD SEV isn't supported, but skip SEV-ES setup. */
if (status == EFI_UNSUPPORTED)
goto continue_setup;
if (status != EFI_SUCCESS) {
printf("AMD SEV setup failed, error = %d\n", status);
return status;
}
/* Same as above, lack of SEV-ES is not a fatal error. */
status = setup_amd_sev_es();
if (status != EFI_SUCCESS && status != EFI_UNSUPPORTED) {
printf("AMD SEV-ES setup failed, error = %d\n", status);
return status;
}
continue_setup:
> +
> + status = setup_amd_sev_es();
> + if (status != EFI_SUCCESS) {
> + switch (status) {
> + case EFI_UNSUPPORTED:
> + /* Continue if AMD SEV-ES is not supported */
> + break;
> + default:
> + printf("Set up AMD SEV-ES failed\n");
> + return status;
> + }
> + }
> +
> reset_apic();
> setup_gdt_tss();
> setup_idt();
> @@ -343,9 +334,9 @@ void setup_efi(efi_bootinfo_t *efi_bootinfo)
> enable_apic();
> enable_x2apic();
> smp_init();
> - phys_alloc_init(efi_bootinfo->free_mem_start, efi_bootinfo->free_mem_size);
> - setup_efi_rsdp(efi_bootinfo->rsdp);
> setup_page_table();
> +
> + return EFI_SUCCESS;
> }
>
> #endif /* TARGET_EFI */
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [kvm-unit-tests PATCH v1 2/7] x86 UEFI: Refactor set up process
2021-11-05 18:54 ` Sean Christopherson
@ 2021-11-09 17:16 ` Zixuan Wang
0 siblings, 0 replies; 19+ messages in thread
From: Zixuan Wang @ 2021-11-09 17:16 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvm list, Paolo Bonzini, Andrew Jones, Marc Orr, Erdem Aktas,
David Rientjes, Singh, Brijesh, Lendacky, Thomas, Varad Gautam,
Joerg Roedel, bp
On Fri, Nov 5, 2021 at 11:54 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Sat, Oct 30, 2021, Zixuan Wang wrote:
> > +efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo)
> > {
> > + efi_status_t status;
> > +
> > + status = setup_memory_allocator(efi_bootinfo);
> > + if (status != EFI_SUCCESS) {
> > + printf("Failed to set up memory allocator: ");
> > + switch (status) {
> > + case EFI_OUT_OF_RESOURCES:
> > + printf("No free memory region\n");
> > + break;
> > + default:
> > + printf("Unknown error\n");
> > + break;
> > + }
> > + return status;
> > + }
> > +
> > + status = setup_rsdp(efi_bootinfo);
> > + if (status != EFI_SUCCESS) {
> > + printf("Cannot find RSDP in EFI system table\n");
> > + return status;
> > + }
> > +
> > + status = setup_amd_sev();
> > + if (status != EFI_SUCCESS) {
> > + switch (status) {
> > + case EFI_UNSUPPORTED:
> > + /* Continue if AMD SEV is not supported */
> > + break;
> > + default:
> > + printf("Set up AMD SEV failed\n");
> > + return status;
> > + }
> > + }
>
> Looks like this is pre-existing behavior, but the switch is quite gratuituous,
> and arguably does the wrong thing for EFI_UNSUPPORTED here as attempting to setup
> SEV-ES without SEV is guaranteed to fail. And it'd be really nice if the printf()
> actually provided the error (below might be wrong, I don't know the type of
> efi_status-t).
>
> status = setup_amd_sev();
>
> /* Continue on if AMD SEV isn't supported, but skip SEV-ES setup. */
> if (status == EFI_UNSUPPORTED)
> goto continue_setup;
>
> if (status != EFI_SUCCESS) {
> printf("AMD SEV setup failed, error = %d\n", status);
> return status;
> }
>
> /* Same as above, lack of SEV-ES is not a fatal error. */
> status = setup_amd_sev_es();
> if (status != EFI_SUCCESS && status != EFI_UNSUPPORTED) {
> printf("AMD SEV-ES setup failed, error = %d\n", status);
> return status;
> }
>
> continue_setup:
>
I agree. The current setup_amd_sev_es() checks if SEV is available and
returns EFI_UNSUPPORTED if not, so it does no harm if called after
setup_amd_sev() fails. But I think we should not rely on these
underlying implementation details. I will include this part in the
next version.
Best regards,
Zixuan
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-11-09 17:16 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-31 5:56 [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 1/7] x86 UEFI: Remove mixed_mode Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 2/7] x86 UEFI: Refactor set up process Zixuan Wang
2021-11-05 18:54 ` Sean Christopherson
2021-11-09 17:16 ` Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 3/7] x86 UEFI: Convert x86 test cases to PIC Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 4/7] x86 UEFI: Set UEFI OVMF as readonly Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 5/7] x86 UEFI: Exit QEMU with return code Zixuan Wang
2021-10-31 10:01 ` Paolo Bonzini
2021-10-31 21:36 ` Zixuan Wang
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 6/7] scripts: Generalize EFI check Zixuan Wang
2021-10-31 7:13 ` Paolo Bonzini
2021-10-31 15:35 ` Marc Orr
2021-10-31 5:56 ` [kvm-unit-tests PATCH v1 7/7] x86 UEFI: Make run_tests.sh (mostly) work under UEFI Zixuan Wang
2021-10-31 7:28 ` [kvm-unit-tests PATCH v1 0/7] x86_64 UEFI set up process refactor and scripts fixes Paolo Bonzini
2021-10-31 16:14 ` Marc Orr
2021-10-31 21:54 ` Zixuan Wang
2021-11-01 7:11 ` Andrew Jones
2021-11-01 22:35 ` 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).