[kvm-unit-tests PATCH v2 1/4] x86 EFI: Bypass call to fdt_check_header()

[kvm-unit-tests PATCH v2 1/4] x86 EFI: Bypass call to fdt_check_header()

[Pavan Kumar Paluri]

Issuing a call to fdt_check_header() prevents running any of x86 UEFI
enabled tests. Bypass this call for x86 and also calls to
efi_load_image(), efi_grow_buffer(), efi_get_var() in order to enable
UEFI supported tests for KUT x86 arch.

Fixes: 9632ce446b8f ("arm64: efi: Improve device tree discovery")
Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
---
 lib/efi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/lib/efi.c b/lib/efi.c
index 5314eaa81e66..8a74a22834a4 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -204,6 +204,7 @@ static char *efi_convert_cmdline(struct efi_loaded_image_64 *image, int *cmd_lin
 	return (char *)cmdline_addr;
 }
 
+#if defined(__aarch64__) || defined(__riscv)
 /*
  * Open the file and read it into a buffer.
  */
@@ -330,6 +331,12 @@ static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
 
 	return fdt_check_header(fdt) == 0 ? fdt : NULL;
 }
+#else
+static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
+{
+	return NULL;
+}
+#endif
 
 static const struct {
 	struct efi_vendor_dev_path	vendor;
-- 
2.34.1

[kvm-unit-tests PATCH v2 2/4] x86/efi: Retry call to efi exit boot services

[Pavan Kumar Paluri]

In some cases, KUT guest might fail to exit boot services due to a
possible memory map update that might have taken place between
efi_get_memory_map() and efi_exit_boot_services() calls. As per UEFI
spec 2.10 (Section 7.4.6 EFI_BOOT_SERVICES.ExitBootServices()), we need
to keep trying to update the memory map and calls to exit boot
services as long as case status is EFI_INVALID_PARAMETER.

Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
---
 lib/efi.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/lib/efi.c b/lib/efi.c
index 8a74a22834a4..c98bc5c0a022 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -461,16 +461,35 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
 	}
 #endif
 
-	/* 
+	/*
 	 * 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) {
+	if (status != EFI_SUCCESS && status != EFI_INVALID_PARAMETER) {
 		printf("Failed to exit boot services\n");
 		goto efi_main_error;
 	}
 
+	/*
+	 * There is a possibility that memory map might have changed
+	 * between efi_get_memory_map() and efi_exit_boot_services in
+	 * which case status is EFI_INVALID_PARAMETER. As per UEFI spec
+	 * 2.10, we need to get the updated memory map and keep trying
+	 * until status is not EFI_INVALID_PARAMETER.
+	 */
+	while (status == EFI_INVALID_PARAMETER) {
+		efi_get_memory_map(&efi_bootinfo.mem_map);
+
+		status = efi_exit_boot_services(handle,
+						&efi_bootinfo.mem_map);
+		if (status != EFI_SUCCESS &&
+		    status != EFI_INVALID_PARAMETER) {
+			printf("Failed to exit boot services\n");
+			goto efi_main_error;
+		}
+	}
+
 	/* Set up arch-specific resources */
 	status = setup_efi(&efi_bootinfo);
 	if (status != EFI_SUCCESS) {
-- 
2.34.1

[kvm-unit-tests PATCH v2 3/4] x86 AMD SEV-ES: Set GHCB page attributes for a new page table

[Pavan Kumar Paluri]

SEV-ES/SNP guest uses GHCB page to communicate with the host. Such a
page should remain unencrypted (its c-bit should be unset). Therefore,
call setup_ghcb_pte() in the path of setup_vm() to make sure c-bit of
GHCB's pte is unset, for a new page table that will be setup as a part
of page allocation for SEV-ES/SNP tests later on.

Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
---
 lib/x86/vm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/x86/vm.c b/lib/x86/vm.c
index 90f73fbb2dfd..ce2063aee75d 100644
--- a/lib/x86/vm.c
+++ b/lib/x86/vm.c
@@ -3,6 +3,7 @@
 #include "vmalloc.h"
 #include "alloc_page.h"
 #include "smp.h"
+#include "amd_sev.h"
 
 static pteval_t pte_opt_mask;
 
@@ -197,6 +198,11 @@ void *setup_mmu(phys_addr_t end_of_memory, void *opt_mask)
     init_alloc_vpage((void*)(3ul << 30));
 #endif
 
+#ifdef CONFIG_EFI
+	if (amd_sev_es_enabled())
+		setup_ghcb_pte(cr3);
+#endif
+
     write_cr3(virt_to_phys(cr3));
 #ifndef __x86_64__
     write_cr4(X86_CR4_PSE);
-- 
2.34.1

[kvm-unit-tests PATCH v2 4/4] x86 AMD SEV-ES: Setup a new page table and install level-1 PTEs

[Pavan Kumar Paluri]

KUT's UEFI tests don't currently have support for page allocation.
SEV-ES/SNP tests will need this later, so the support for page
allocation is provided via setup_vm().

Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
---
 x86/amd_sev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/x86/amd_sev.c b/x86/amd_sev.c
index 7757d4f85b7a..bdf14055e46a 100644
--- a/x86/amd_sev.c
+++ b/x86/amd_sev.c
@@ -14,6 +14,8 @@
 #include "x86/processor.h"
 #include "x86/amd_sev.h"
 #include "msr.h"
+#include "x86/vm.h"
+#include "alloc_page.h"
 
 #define EXIT_SUCCESS 0
 #define EXIT_FAILURE 1
@@ -89,9 +91,14 @@ static void test_stringio(void)
 int main(void)
 {
 	int rtn;
+	unsigned long *vaddr;
 	rtn = test_sev_activation();
 	report(rtn == EXIT_SUCCESS, "SEV activation test.");
 	test_sev_es_activation();
 	test_stringio();
+	setup_vm();
+	vaddr = alloc_page();
+	if (!vaddr)
+		assert_msg(vaddr, "Page allocation failure");
 	return report_summary();
 }
-- 
2.34.1

Re: [kvm-unit-tests PATCH v2 1/4] x86 EFI: Bypass call to fdt_check_header()

[Andrew Jones]

On Tue, Mar 26, 2024 at 12:33:57PM -0500, Pavan Kumar Paluri wrote:
> Issuing a call to fdt_check_header() prevents running any of x86 UEFI
> enabled tests. Bypass this call for x86 and also calls to
> efi_load_image(), efi_grow_buffer(), efi_get_var() in order to enable
> UEFI supported tests for KUT x86 arch.
> 
> Fixes: 9632ce446b8f ("arm64: efi: Improve device tree discovery")
> Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
> ---
>  lib/efi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/efi.c b/lib/efi.c
> index 5314eaa81e66..8a74a22834a4 100644
> --- a/lib/efi.c
> +++ b/lib/efi.c
> @@ -204,6 +204,7 @@ static char *efi_convert_cmdline(struct efi_loaded_image_64 *image, int *cmd_lin
>  	return (char *)cmdline_addr;
>  }
>  
> +#if defined(__aarch64__) || defined(__riscv)
>  /*
>   * Open the file and read it into a buffer.
>   */
> @@ -330,6 +331,12 @@ static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
>  
>  	return fdt_check_header(fdt) == 0 ? fdt : NULL;
>  }
> +#else
> +static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
> +{
> +	return NULL;
> +}
> +#endif
>  
>  static const struct {
>  	struct efi_vendor_dev_path	vendor;
> -- 
> 2.34.1
>

It's a pity that the suggestion I made (which I obviously hadn't even
compile tested...) isn't sufficient, because what I was going for was
a way to annotate that specifically efi_get_fdt() was for architectures
which use a DT (and link libfdt). It's not as nice to indicate that
the other functions also don't apply to x86 (x86 doesn't use them
now, but there's no reason it couldn't). With that in mind, I think I
like your original patch better, but with a tweak to ensure we don't
generate the undefined reference to fdt_check_header(),

diff --git a/lib/efi.c b/lib/efi.c
index 5314eaa81e66..dfbadea60411 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -328,7 +328,12 @@ static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
                return NULL;
        }

+#ifdef __x86_64__
+       /* x86 is ACPI-only */
+       return NULL;
+#else
        return fdt_check_header(fdt) == 0 ? fdt : NULL;
+#endif
 }

 static const struct {

That said, I could go either way, since it also makes sense to not
compile code for x86 that it doesn't currently use. So, unless you
need to respin for other reasons,

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Thanks,
drew

Re: [kvm-unit-tests PATCH v2 2/4] x86/efi: Retry call to efi exit boot services

[Andrew Jones]

On Tue, Mar 26, 2024 at 12:33:58PM -0500, Pavan Kumar Paluri wrote:
> In some cases, KUT guest might fail to exit boot services due to a
> possible memory map update that might have taken place between
> efi_get_memory_map() and efi_exit_boot_services() calls. As per UEFI
> spec 2.10 (Section 7.4.6 EFI_BOOT_SERVICES.ExitBootServices()), we need
> to keep trying to update the memory map and calls to exit boot
> services as long as case status is EFI_INVALID_PARAMETER.
> 
> Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
> ---
>  lib/efi.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/efi.c b/lib/efi.c
> index 8a74a22834a4..c98bc5c0a022 100644
> --- a/lib/efi.c
> +++ b/lib/efi.c
> @@ -461,16 +461,35 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
>  	}
>  #endif
>  
> -	/* 
> +	/*
>  	 * 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) {
> +	if (status != EFI_SUCCESS && status != EFI_INVALID_PARAMETER) {
>  		printf("Failed to exit boot services\n");
>  		goto efi_main_error;
>  	}
>  
> +	/*
> +	 * There is a possibility that memory map might have changed
> +	 * between efi_get_memory_map() and efi_exit_boot_services in

nit: Either add the () to all function names or to none.

> +	 * which case status is EFI_INVALID_PARAMETER. As per UEFI spec
> +	 * 2.10, we need to get the updated memory map and keep trying

Even older specs have this, but calling out 2.10 makes it sound like it's
something from 2.10. It's probably not worth looking for when it first
appeared, but normally we'd want to reference the oldest version. If you
don't want to dig that up, then I'm fine with just dropping the number,
or even the whole comment.

> +	 * until status is not EFI_INVALID_PARAMETER.
> +	 */
> +	while (status == EFI_INVALID_PARAMETER) {
> +		efi_get_memory_map(&efi_bootinfo.mem_map);
> +
> +		status = efi_exit_boot_services(handle,
> +						&efi_bootinfo.mem_map);
> +		if (status != EFI_SUCCESS &&
> +		    status != EFI_INVALID_PARAMETER) {
> +			printf("Failed to exit boot services\n");
> +			goto efi_main_error;
> +		}
> +	}
> +
>  	/* Set up arch-specific resources */
>  	status = setup_efi(&efi_bootinfo);
>  	if (status != EFI_SUCCESS) {
> -- 
> 2.34.1
>

We're leaking the old maps when we need to try again. Also, I see
another issue (which I introduced with efi_get_boot_hartid()) which is
to have a call between getting the memory map and exiting boot services,
which the spec recommends not doing. I think for this patch we should
do something like below (which is untested).

Thanks,
drew

diff --git a/lib/efi.c b/lib/efi.c
index dfbadea60411..d876afefc614 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -404,8 +404,8 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
        efi_system_table = sys_tab;

        /* Memory map struct values */
-       efi_memory_desc_t *map = NULL;
-       unsigned long map_size = 0, desc_size = 0, key = 0, buff_size = 0;
+       efi_memory_desc_t *map;
+       unsigned long map_size, desc_size, key, buff_size;
        u32 desc_ver;

        /* Helper variables needed to get the cmdline */
@@ -444,13 +444,6 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
        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 get memory map\n");
-               goto efi_main_error;
-       }
-
 #ifdef __riscv
        status = efi_get_boot_hartid();
        if (status != EFI_SUCCESS) {
@@ -463,7 +456,18 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
         * Exit EFI boot services, let kvm-unit-tests take full control of the
         * guest
         */
-       status = efi_exit_boot_services(handle, &efi_bootinfo.mem_map);
+       status = EFI_INVALID_PARAMETER;
+       while (status == EFI_INVALID_PARAMETER) {
+               status = efi_get_memory_map(&efi_bootinfo.mem_map);
+               if (status != EFI_SUCCESS) {
+                       printf("Failed to get memory map\n");
+                       goto efi_main_error;
+               }
+
+               status = efi_exit_boot_services(handle, &efi_bootinfo.mem_map);
+               if (status == EFI_INVALID_PARAMETER)
+                       efi_free_pool(*efi_bootinfo.mem_map.map);
+       }
        if (status != EFI_SUCCESS) {
                printf("Failed to exit boot services\n");
                goto efi_main_error;

Re: [kvm-unit-tests PATCH v2 1/4] x86 EFI: Bypass call to fdt_check_header()

[Paluri, PavanKumar]

On 3/27/2024 3:08 AM, Andrew Jones wrote:
> On Tue, Mar 26, 2024 at 12:33:57PM -0500, Pavan Kumar Paluri wrote:
>> Issuing a call to fdt_check_header() prevents running any of x86 UEFI
>> enabled tests. Bypass this call for x86 and also calls to
>> efi_load_image(), efi_grow_buffer(), efi_get_var() in order to enable
>> UEFI supported tests for KUT x86 arch.
>>
>> Fixes: 9632ce446b8f ("arm64: efi: Improve device tree discovery")
>> Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
>> ---
>>  lib/efi.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/lib/efi.c b/lib/efi.c
>> index 5314eaa81e66..8a74a22834a4 100644
>> --- a/lib/efi.c
>> +++ b/lib/efi.c
>> @@ -204,6 +204,7 @@ static char *efi_convert_cmdline(struct efi_loaded_image_64 *image, int *cmd_lin
>>  	return (char *)cmdline_addr;
>>  }
>>  
>> +#if defined(__aarch64__) || defined(__riscv)
>>  /*
>>   * Open the file and read it into a buffer.
>>   */
>> @@ -330,6 +331,12 @@ static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
>>  
>>  	return fdt_check_header(fdt) == 0 ? fdt : NULL;
>>  }
>> +#else
>> +static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
>> +{
>> +	return NULL;
>> +}
>> +#endif
>>  
>>  static const struct {
>>  	struct efi_vendor_dev_path	vendor;
>> -- 
>> 2.34.1
>>
> 
> It's a pity that the suggestion I made (which I obviously hadn't even
> compile tested...) isn't sufficient, because what I was going for was
> a way to annotate that specifically efi_get_fdt() was for architectures
> which use a DT (and link libfdt). It's not as nice to indicate that
> the other functions also don't apply to x86 (x86 doesn't use them
> now, but there's no reason it couldn't). With that in mind, I think I
> like your original patch better, but with a tweak to ensure we don't
> generate the undefined reference to fdt_check_header(),
> 
> diff --git a/lib/efi.c b/lib/efi.c
> index 5314eaa81e66..dfbadea60411 100644
> --- a/lib/efi.c
> +++ b/lib/efi.c
> @@ -328,7 +328,12 @@ static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
>                 return NULL;
>         }
> 
> +#ifdef __x86_64__
> +       /* x86 is ACPI-only */
> +       return NULL;
> +#else
>         return fdt_check_header(fdt) == 0 ? fdt : NULL;
> +#endif
>  }
> 
>  static const struct {
> 
> That said, I could go either way, since it also makes sense to not
> compile code for x86 that it doesn't currently use. So, unless you
> need to respin for other reasons,
> 
Tested the above diff as well. Both work for x86. So maybe it is better
to not compile code that x86 currently does not use for now and later
make this change when x86 makes use of efi_load_image(), etc.. ? I am
fine with either ways.

> Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
> 
Thanks,
Pavan

> Thanks,
> drew