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

[kvm-unit-tests PATCH v3 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>
Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
---
 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 v3 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. Keep freeing
the old memory map before obtaining new memory map via
efi_get_memory_map() in case of exit boot services failure.

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

diff --git a/lib/efi.c b/lib/efi.c
index 8a74a22834a4..d2569b22b4f2 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -406,8 +406,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 */
@@ -446,13 +446,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) {
@@ -461,11 +454,24 @@ 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);
+	status = EFI_INVALID_PARAMETER;
+	while (status == EFI_INVALID_PARAMETER) {
+		/* 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;
+		}
+		/*
+		 * 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_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;
-- 
2.34.1

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

[Andrew Jones]

On Thu, Mar 28, 2024 at 10:21:10AM -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. Keep freeing
> the old memory map before obtaining new memory map via
> efi_get_memory_map() in case of exit boot services failure.
> 
> Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
> ---
>  lib/efi.c | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/efi.c b/lib/efi.c
> index 8a74a22834a4..d2569b22b4f2 100644
> --- a/lib/efi.c
> +++ b/lib/efi.c
> @@ -406,8 +406,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 */
> @@ -446,13 +446,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) {
> @@ -461,11 +454,24 @@ 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);
> +	status = EFI_INVALID_PARAMETER;
> +	while (status == EFI_INVALID_PARAMETER) {
> +		/* Get EFI memory map */

I tried to get rid of this comment since it states the exact same thing
as the function name below does.

> +		status = efi_get_memory_map(&efi_bootinfo.mem_map);
> +		if (status != EFI_SUCCESS) {
> +			printf("Failed to get memory map\n");
> +			goto efi_main_error;
> +		}
> +		/*
> +		 * Exit EFI boot services, let kvm-unit-tests take full
> +		 * control of the guest.
> +		 */
> +		status = efi_exit_boot_services(handle,
> +						&efi_bootinfo.mem_map);

We have 100 char lines (and that's just a soft limit) so this would look
better sticking out.

> +		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;
> -- 
> 2.34.1
>

Besides the nits,

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

(A general comment for the series is that we're on v3 but there's no
changelog anywhere. Please use cover letters for a series and then
put the changelog in the cover letter.)

Thanks,
drew

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

[Paluri, PavanKumar]

On 3/28/2024 11:33 AM, Andrew Jones wrote:
> On Thu, Mar 28, 2024 at 10:21:10AM -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. Keep freeing
>> the old memory map before obtaining new memory map via
>> efi_get_memory_map() in case of exit boot services failure.
>>
>> Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
>> ---
>>  lib/efi.c | 34 ++++++++++++++++++++--------------
>>  1 file changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/efi.c b/lib/efi.c
>> index 8a74a22834a4..d2569b22b4f2 100644
>> --- a/lib/efi.c
>> +++ b/lib/efi.c
>> @@ -406,8 +406,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 */
>> @@ -446,13 +446,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) {
>> @@ -461,11 +454,24 @@ 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);
>> +	status = EFI_INVALID_PARAMETER;
>> +	while (status == EFI_INVALID_PARAMETER) {
>> +		/* Get EFI memory map */
> 
> I tried to get rid of this comment since it states the exact same thing
> as the function name below does.
>
Will make the change.

>> +		status = efi_get_memory_map(&efi_bootinfo.mem_map);
>> +		if (status != EFI_SUCCESS) {
>> +			printf("Failed to get memory map\n");
>> +			goto efi_main_error;
>> +		}
>> +		/*
>> +		 * Exit EFI boot services, let kvm-unit-tests take full
>> +		 * control of the guest.
>> +		 */
>> +		status = efi_exit_boot_services(handle,
>> +						&efi_bootinfo.mem_map);
> 
> We have 100 char lines (and that's just a soft limit) so this would look
> better sticking out.
> 
Will do.
>> +		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;
>> -- 
>> 2.34.1
>>
> 
> Besides the nits,
> 
> Reviewed-by: Andrew Jones <andrew.jones@linux.dev>
> 
> (A general comment for the series is that we're on v3 but there's no
> changelog anywhere. Please use cover letters for a series and then
> put the changelog in the cover letter.)
> 
Ah, I missed out on that. I will include a cover-letter for v4 and also
plan to drop patches 3 & 4 and send them separately as they aren't very
relevant to UEFI fixes and for easier upstreaming.

Thanks,
Pavan
> Thanks,
> drew

[kvm-unit-tests PATCH v3 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 v3 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