kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Jones <drjones@redhat.com>
To: Zixuan Wang <zixuanwang@google.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, marcorr@google.com,
	baekhw@google.com, tmroeder@google.com, erdemaktas@google.com,
	rientjes@google.com, seanjc@google.com, brijesh.singh@amd.com,
	Thomas.Lendacky@amd.com, varad.gautam@suse.com, jroedel@suse.de,
	bp@suse.de
Subject: Re: [kvm-unit-tests PATCH v2 08/17] x86 UEFI: Set up RSDP after UEFI boot up
Date: Mon, 4 Oct 2021 15:21:02 +0200	[thread overview]
Message-ID: <20211004132102.lfsyabk2daeiejkx@gator> (raw)
In-Reply-To: <20210827031222.2778522-9-zixuanwang@google.com>

On Fri, Aug 27, 2021 at 03:12:13AM +0000, Zixuan Wang wrote:
> Root system description pointer (RSDP) is a data structure used in the
> ACPI programming interface. In BIOS, RSDP is located within a
> predefined memory area, so a program can scan the memory area and find
> RSDP. But in UEFI, RSDP may not appear in that memory area, instead, a
> program should find it in the EFI system table.
> 
> This commit provides RSDP set up code in UEFI:
>    1. Read RSDP from EFI system table
>    2. Pass RSDP pointer to find_acpi_table_attr() function
> 
> From this commit, the `x86/s3.c` test can run in UEFI and generates
> similar output as in Seabios, note that:
>    1. In its output, memory addresses are different than Seabios's, this
>       is because EFI application starts from a dynamic runtime address,
>       not a fixed predefined memory address
>    2. There is a short delay (~5 secs) after the test case prints "PM1a
>       event registers" line. This test case sleeps for a few seconds
>       and then wakes up, so give it a few seconds to run.
> 
> Signed-off-by: Zixuan Wang <zixuanwang@google.com>
> ---
>  lib/efi.c           | 15 +++++++++++++++
>  lib/efi.h           |  1 +
>  lib/linux/uefi.h    | 15 +++++++++++++++
>  lib/x86/acpi.c      | 38 +++++++++++++++++++++++++++++++-------
>  lib/x86/acpi.h      | 11 +++++++++++
>  lib/x86/asm/setup.h |  2 ++
>  lib/x86/setup.c     | 13 +++++++++++++
>  7 files changed, 88 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/efi.c b/lib/efi.c
> index b7a69d3..a0d4476 100644
> --- a/lib/efi.c
> +++ b/lib/efi.c
> @@ -69,6 +69,21 @@ efi_status_t efi_exit_boot_services(void *handle, unsigned long mapkey)
>  	return efi_bs_call(exit_boot_services, handle, mapkey);
>  }
>  
> +efi_status_t efi_get_system_config_table(efi_guid_t table_guid, void **table)
> +{
> +	size_t i;
> +	efi_config_table_t *tables;
> +
> +	tables = (efi_config_table_t *)efi_system_table->tables;
> +	for (i = 0; i < efi_system_table->nr_tables; i++) {
> +		if (!memcmp(&table_guid, &tables[i].guid, sizeof(efi_guid_t))) {
> +			*table = tables[i].table;
> +			return EFI_SUCCESS;
> +		}
> +	}
> +	return EFI_NOT_FOUND;
> +}
> +
>  efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
>  {
>  	int ret;
> diff --git a/lib/efi.h b/lib/efi.h
> index 2d3772c..dbb8159 100644
> --- a/lib/efi.h
> +++ b/lib/efi.h
> @@ -12,6 +12,7 @@
>  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_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);
>  
>  #endif /* _EFI_H_ */
> diff --git a/lib/linux/uefi.h b/lib/linux/uefi.h
> index 9adc7ab..d1d599f 100644
> --- a/lib/linux/uefi.h
> +++ b/lib/linux/uefi.h
> @@ -58,6 +58,21 @@ typedef guid_t efi_guid_t;
>  	(b) & 0xff, ((b) >> 8) & 0xff,						\
>  	(c) & 0xff, ((c) >> 8) & 0xff, d } }
>  
> +#define ACPI_TABLE_GUID EFI_GUID(0xeb9d2d30, 0x2d88, 0x11d3, 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
> +
> +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;

Can't we drop all the mixed_mode stuff? Or do we really want to support
32-bit UEFI kvm-unit-tests?

> +} efi_config_table_t;
> +
>  /*
>   * Generic EFI table header
>   */
> diff --git a/lib/x86/acpi.c b/lib/x86/acpi.c
> index 4373106..0f75d79 100644
> --- a/lib/x86/acpi.c
> +++ b/lib/x86/acpi.c
> @@ -1,9 +1,37 @@
>  #include "libcflat.h"
>  #include "acpi.h"
>  
> +#ifdef TARGET_EFI
> +struct rsdp_descriptor *efi_rsdp = NULL;
> +
> +void setup_efi_rsdp(struct rsdp_descriptor *rsdp) {
> +	efi_rsdp = rsdp;
> +}

{ on its own line please

> +
> +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");
> +	}
> +	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;
> +    }

When moving code please take the opportunity to clean up its style.

> +    if (addr == 0x100000) {
> +        return NULL;
> +    }
> +    return rsdp;
> +}
> +#endif /* TARGET_EFI */
> +
>  void* find_acpi_table_addr(u32 sig)
>  {
> -    unsigned long addr;
>      struct rsdp_descriptor *rsdp;
>      struct rsdt_descriptor_rev1 *rsdt;
>      void *end;
> @@ -19,12 +47,8 @@ void* find_acpi_table_addr(u32 sig)
>          return (void*)(ulong)fadt->firmware_ctrl;
>      }
>  
> -    for(addr = 0xf0000; addr < 0x100000; addr += 16) {
> -	rsdp = (void*)addr;
> -	if (rsdp->signature == 0x2052545020445352LL)
> -          break;
> -    }
> -    if (addr == 0x100000) {
> +    rsdp = get_rsdp();
> +    if (rsdp == NULL) {
>          printf("Can't find RSDP\n");
>          return 0;
>      }
> diff --git a/lib/x86/acpi.h b/lib/x86/acpi.h
> index 1b80374..db8ee56 100644
> --- a/lib/x86/acpi.h
> +++ b/lib/x86/acpi.h
> @@ -11,6 +11,13 @@
>  #define FACP_SIGNATURE ACPI_SIGNATURE('F','A','C','P')
>  #define FACS_SIGNATURE ACPI_SIGNATURE('F','A','C','S')
>  
> +
> +#define ACPI_SIGNATURE_8BYTE(c1, c2, c3, c4, c5, c6, c7, c8) \
> +	((uint64_t)(ACPI_SIGNATURE(c1, c2, c3, c4))) |       \
> +	((uint64_t)(ACPI_SIGNATURE(c5, c6, c7, c8)) << 32)
> +
> +#define RSDP_SIGNATURE_8BYTE (ACPI_SIGNATURE_8BYTE('R', 'S', 'D', ' ', 'P', 'T', 'R', ' '))
> +
>  struct rsdp_descriptor {        /* Root System Descriptor Pointer */
>      u64 signature;              /* ACPI signature, contains "RSD PTR " */
>      u8  checksum;               /* To make sum of struct == 0 */
> @@ -101,4 +108,8 @@ struct facs_descriptor_rev1
>  
>  void* find_acpi_table_addr(u32 sig);
>  
> +#ifdef TARGET_EFI
> +void setup_efi_rsdp(struct rsdp_descriptor *rsdp);
> +#endif /* TARGET_EFI */

Unnecessary ifdef.

> +
>  #endif
> diff --git a/lib/x86/asm/setup.h b/lib/x86/asm/setup.h
> index 8ff31ef..40fd963 100644
> --- a/lib/x86/asm/setup.h
> +++ b/lib/x86/asm/setup.h
> @@ -2,6 +2,7 @@
>  #define _X86_ASM_SETUP_H_
>  
>  #ifdef TARGET_EFI
> +#include "x86/acpi.h"
>  #include "x86/apic.h"
>  #include "x86/smp.h"
>  #include "efi.h"
> @@ -15,6 +16,7 @@
>  typedef struct {
>  	phys_addr_t free_mem_start;
>  	phys_addr_t free_mem_size;
> +	struct rsdp_descriptor *rsdp;
>  } efi_bootinfo_t;
>  
>  void setup_efi_bootinfo(efi_bootinfo_t *efi_bootinfo);
> diff --git a/lib/x86/setup.c b/lib/x86/setup.c
> index a49e0d4..1ddfb8c 100644
> --- a/lib/x86/setup.c
> +++ b/lib/x86/setup.c
> @@ -135,6 +135,7 @@ void setup_efi_bootinfo(efi_bootinfo_t *efi_bootinfo)
>  {
>  	efi_bootinfo->free_mem_size = 0;
>  	efi_bootinfo->free_mem_start = 0;
> +	efi_bootinfo->rsdp = NULL;
>  }
>  
>  static efi_status_t setup_pre_boot_memory(unsigned long *mapkey, efi_bootinfo_t *efi_bootinfo)
> @@ -185,6 +186,11 @@ static efi_status_t setup_pre_boot_memory(unsigned long *mapkey, efi_bootinfo_t
>  	return EFI_SUCCESS;
>  }
>  
> +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);
> +}
> +
>  efi_status_t setup_efi_pre_boot(unsigned long *mapkey, efi_bootinfo_t *efi_bootinfo)
>  {
>  	efi_status_t status;
> @@ -203,6 +209,12 @@ efi_status_t setup_efi_pre_boot(unsigned long *mapkey, efi_bootinfo_t *efi_booti
>  		return status;
>  	}
>  
> +	status = setup_pre_boot_rsdp(efi_bootinfo);
> +	if (status != EFI_SUCCESS) {
> +		printf("Cannot find RSDP in EFI system table\n");
> +		return status;
> +	}
> +
>  	return EFI_SUCCESS;
>  }
>  
> @@ -255,6 +267,7 @@ void setup_efi(efi_bootinfo_t *efi_bootinfo)
>  	enable_x2apic();
>  	smp_init();
>  	phys_alloc_init(efi_bootinfo->free_mem_start, efi_bootinfo->free_mem_size);
> +	setup_efi_rsdp(efi_bootinfo->rsdp);

What memory region is this table in? We should make sure it's reserved or
copy the table out to somewhere that is reserved.

>  }
>  
>  #endif /* TARGET_EFI */
> -- 
> 2.33.0.259.gc128427fd7-goog
>

I'd much prefer we avoid too much of this split setup where we have a bit
of setup in a common efi lib and then an x86 specific part that populates
an x86 specific info structure before exiting boot services and then more
x86 specific setup that uses that later... 

Can't we do almost everything in lib/efi.c and only call out once into an
arch_efi_setup function after exiting boot services?

Thanks,
drew


  reply	other threads:[~2021-10-04 13:22 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-27  3:12 [kvm-unit-tests PATCH v2 00/17] x86_64 UEFI and AMD SEV/SEV-ES support Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 01/17] x86 UEFI: Copy code from Linux Zixuan Wang
2021-09-20 14:33   ` Paolo Bonzini
2021-09-21  3:58     ` Zixuan Wang
2021-09-21  6:37       ` Varad Gautam
2021-09-21 16:33   ` Andrew Jones
2021-09-22 20:10     ` Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 02/17] x86 UEFI: Implement UEFI function calls Zixuan Wang
2021-09-21 16:43   ` Andrew Jones
2021-09-22 20:17     ` Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 03/17] x86 UEFI: Copy code from GNU-EFI Zixuan Wang
2021-10-04 12:44   ` Andrew Jones
2021-10-04 22:09     ` Zixuan Wang
2021-10-05  5:58       ` Andrew Jones
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 04/17] x86 UEFI: Boot from UEFI Zixuan Wang
2021-10-04 12:55   ` Andrew Jones
2021-10-04 21:30     ` Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 05/17] x86 UEFI: Load IDT after UEFI boot up Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 06/17] x86 UEFI: Load GDT and TSS " Zixuan Wang
2021-09-20 15:40   ` Paolo Bonzini
2021-09-21  4:15     ` Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 07/17] x86 UEFI: Set up memory allocator Zixuan Wang
2021-10-04 13:06   ` Andrew Jones
2021-10-04 21:43     ` Zixuan Wang
2021-10-05  6:05       ` Andrew Jones
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 08/17] x86 UEFI: Set up RSDP after UEFI boot up Zixuan Wang
2021-10-04 13:21   ` Andrew Jones [this message]
2021-10-04 21:58     ` Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 09/17] x86 UEFI: Set up page tables Zixuan Wang
2021-09-20 15:43   ` Paolo Bonzini
2021-09-21  4:31     ` Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 10/17] x86 UEFI: Convert x86 test cases to PIC Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 11/17] x86 AMD SEV: Initial support Zixuan Wang
2021-08-27 14:51   ` Tom Lendacky
2021-08-31 19:36     ` Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 12/17] x86 AMD SEV: Page table with c-bit Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 13/17] x86 AMD SEV-ES: Check SEV-ES status Zixuan Wang
2021-08-27 14:55   ` Tom Lendacky
2021-08-31 19:38     ` Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 14/17] x86 AMD SEV-ES: Load GDT with UEFI segments Zixuan Wang
2021-09-20 16:00   ` Paolo Bonzini
2021-09-21  4:41     ` Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 15/17] x86 AMD SEV-ES: Copy UEFI #VC IDT entry Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 16/17] x86 AMD SEV-ES: Set up GHCB page Zixuan Wang
2021-08-27  3:12 ` [kvm-unit-tests PATCH v2 17/17] x86 AMD SEV-ES: Add test cases Zixuan Wang
2021-10-04 13:27 ` [kvm-unit-tests PATCH v2 00/17] x86_64 UEFI and AMD SEV/SEV-ES support Andrew Jones
2021-10-04 20:54   ` Zixuan Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211004132102.lfsyabk2daeiejkx@gator \
    --to=drjones@redhat.com \
    --cc=Thomas.Lendacky@amd.com \
    --cc=baekhw@google.com \
    --cc=bp@suse.de \
    --cc=brijesh.singh@amd.com \
    --cc=erdemaktas@google.com \
    --cc=jroedel@suse.de \
    --cc=kvm@vger.kernel.org \
    --cc=marcorr@google.com \
    --cc=pbonzini@redhat.com \
    --cc=rientjes@google.com \
    --cc=seanjc@google.com \
    --cc=tmroeder@google.com \
    --cc=varad.gautam@suse.com \
    --cc=zixuanwang@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).