* [PATCH v3 1/4] Grab the EFI System Resource Table and check it
@ 2022-04-19 15:34 Demi Marie Obenour
0 siblings, 0 replies; 7+ messages in thread
From: Demi Marie Obenour @ 2022-04-19 15:34 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu
[-- Attachment #1: Type: text/plain, Size: 8494 bytes --]
The EFI System Resource Table (ESRT) is necessary for fwupd to identify
firmware updates to install. According to the UEFI specification §23.4,
the table shall be stored in memory of type EfiBootServicesData.
Therefore, Xen must avoid reusing that memory for other purposes, so
that Linux can access the ESRT. Additionally, Xen must mark the memory
as reserved, so that Linux knows accessing it is safe.
See https://lore.kernel.org/xen-devel/20200818184018.GN1679@mail-itl/T/
for details.
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
xen/arch/arm/efi/efi-boot.h | 1 +
xen/arch/x86/efi/efi-boot.h | 2 +-
xen/common/efi/boot.c | 50 ++++++++++++++++++++++++++++++++++---
xen/common/efi/efi.h | 18 +++++++++++++
xen/common/efi/runtime.c | 3 ++-
xen/include/efi/efiapi.h | 3 +++
6 files changed, 72 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index e452b687d8..ab2ad3dfe0 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -188,6 +188,7 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
desc_ptr->Type == EfiLoaderCode ||
desc_ptr->Type == EfiLoaderData ||
(!map_bs &&
+ desc_ptr != esrt_desc &&
(desc_ptr->Type == EfiBootServicesCode ||
desc_ptr->Type == EfiBootServicesData))) )
{
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 6e65b569b0..75937c8a11 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -171,7 +171,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
{
case EfiBootServicesCode:
case EfiBootServicesData:
- if ( map_bs )
+ if ( map_bs || desc == (EFI_MEMORY_DESCRIPTOR *)esrt_desc )
{
default:
type = E820_RESERVED;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index ac1b235372..31664818c1 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -567,6 +567,38 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
}
#endif
+static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
+
+static bool __init is_esrt_valid(
+ const EFI_MEMORY_DESCRIPTOR *const desc)
+{
+ size_t available_len, len;
+ const UINTN physical_start = desc->PhysicalStart;
+ const ESRT *esrt_ptr;
+
+ len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+ if ( esrt == EFI_INVALID_TABLE_ADDR )
+ return false;
+ if ( physical_start > esrt || esrt - physical_start >= len )
+ return false;
+ /*
+ * The specification requires EfiBootServicesData, but accept
+ * EfiRuntimeServicesData for compatibility
+ */
+ if ( (desc->Type != EfiRuntimeServicesData) &&
+ (desc->Type != EfiBootServicesData) )
+ return false;
+ available_len = len - (esrt - physical_start);
+ if ( available_len < sizeof(*esrt_ptr) )
+ return false;
+ esrt_ptr = (const ESRT *)esrt;
+ if ( esrt_ptr->Version != 1 || !esrt_ptr->Count )
+ return false;
+ return esrt_ptr->Count <=
+ (available_len - sizeof(*esrt_ptr)) /
+ sizeof(esrt_ptr->Entries[0]);
+}
+
/*
* Include architecture specific implementation here, which references the
* static globals defined above.
@@ -857,6 +889,7 @@ static void __init efi_tables(void)
static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID;
static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID;
static EFI_GUID __initdata smbios3_guid = SMBIOS3_TABLE_GUID;
+ static EFI_GUID __initdata esrt_guid = ESRT_GUID;
if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) )
efi.acpi20 = (unsigned long)efi_ct[i].VendorTable;
@@ -868,6 +901,8 @@ static void __init efi_tables(void)
efi.smbios = (unsigned long)efi_ct[i].VendorTable;
if ( match_guid(&smbios3_guid, &efi_ct[i].VendorGuid) )
efi.smbios3 = (unsigned long)efi_ct[i].VendorTable;
+ if ( match_guid(&esrt_guid, &efi_ct[i].VendorGuid) )
+ esrt = (UINTN)efi_ct[i].VendorTable;
}
#ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
@@ -1056,19 +1091,19 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
EFI_STATUS status;
UINTN info_size = 0, map_key;
bool retry;
-#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
unsigned int i;
-#endif
efi_bs->GetMemoryMap(&info_size, NULL, &map_key,
&efi_mdesc_size, &mdesc_ver);
- info_size += 8 * efi_mdesc_size;
+ info_size += 8 * (efi_mdesc_size + 1);
efi_memmap = efi_arch_allocate_mmap_buffer(info_size);
if ( !efi_memmap )
blexit(L"Unable to allocate memory for EFI memory map");
for ( retry = false; ; retry = true )
{
+ esrt_desc = (const EFI_MEMORY_DESCRIPTOR *)EFI_INVALID_TABLE_ADDR;
+
efi_memmap_size = info_size;
status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
efi_memmap, &map_key,
@@ -1077,6 +1112,15 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
if ( EFI_ERROR(status) )
PrintErrMesg(L"Cannot obtain memory map", status);
+ for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
+ {
+ if ( is_esrt_valid(efi_memmap + i) )
+ {
+ esrt_desc = efi_memmap + i;
+ break;
+ }
+ }
+
efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
efi_mdesc_size, mdesc_ver);
diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index c9aa65d506..02f499071a 100644
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -10,6 +10,23 @@
#include <xen/spinlock.h>
#include <asm/page.h>
+typedef struct _ESRT_ENTRY {
+ EFI_GUID FwClass;
+ UINT32 FwType;
+ UINT32 FwVersion;
+ UINT32 FwLowestSupportedVersion;
+ UINT32 FwCapsuleFlags;
+ UINT32 FwLastAttemptVersion;
+ UINT32 FwLastAttemptStatus;
+} ESRT_ENTRY;
+
+typedef struct _ESRT {
+ UINT32 Count;
+ UINT32 Max;
+ UINT64 Version;
+ ESRT_ENTRY Entries[];
+} ESRT;
+
struct efi_pci_rom {
const struct efi_pci_rom *next;
u16 vendor, devid, segment;
@@ -28,6 +45,7 @@ extern const EFI_RUNTIME_SERVICES *efi_rs;
extern UINTN efi_memmap_size, efi_mdesc_size;
extern void *efi_memmap;
+extern const EFI_MEMORY_DESCRIPTOR *esrt_desc;
#ifdef CONFIG_X86
extern mfn_t efi_l4_mfn;
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 13b0975866..0d09647952 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -52,6 +52,7 @@ static unsigned int efi_rs_on_cpu = NR_CPUS;
UINTN __read_mostly efi_memmap_size;
UINTN __read_mostly efi_mdesc_size;
void *__read_mostly efi_memmap;
+const EFI_MEMORY_DESCRIPTOR *__read_mostly esrt_desc;
UINT64 __read_mostly efi_boot_max_var_store_size;
UINT64 __read_mostly efi_boot_remain_var_store_size;
@@ -269,7 +270,7 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
case XEN_FW_EFI_MEM_INFO:
for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
{
- EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
+ const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
if ( info->mem.addr >= desc->PhysicalStart &&
diff --git a/xen/include/efi/efiapi.h b/xen/include/efi/efiapi.h
index a616d1238a..42ef3e1c8c 100644
--- a/xen/include/efi/efiapi.h
+++ b/xen/include/efi/efiapi.h
@@ -882,6 +882,9 @@ typedef struct _EFI_BOOT_SERVICES {
#define SAL_SYSTEM_TABLE_GUID \
{ 0xeb9d2d32, 0x2d88, 0x11d3, {0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d} }
+#define ESRT_GUID \
+ { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} }
+
typedef struct _EFI_CONFIGURATION_TABLE {
EFI_GUID VendorGuid;
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 0/4] EFI System Resource Table support
@ 2022-04-19 15:32 Demi Marie Obenour
2022-04-19 15:40 ` [PATCH v3 1/4] Grab the EFI System Resource Table and check it Demi Marie Obenour
0 siblings, 1 reply; 7+ messages in thread
From: Demi Marie Obenour @ 2022-04-19 15:32 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu, George Dunlap
[-- Attachment #1: Type: text/plain, Size: 1590 bytes --]
This adds support for the EFI System Resource Table. This involves
reserving the table in Xen and adding a new hypercall so that dom0 can
access it.
Changes since v2:
- Use the esrt_desc global variable instead of passing it as a function
parameter
- Add an overflow check for the ESRT size
- Create a new memory region for the ESRT to avoid wasting memory
- Add hypercall to retrieve the ESRT
- Add file local variables used during development
- Remove extra consts
- Follow the EFI naming convention in struct definitions
- Move struct definitions to header file
- Fix inverted logic in overflow check
- Remove BUILD_BUG_ON()s
- Avoid overriding attribute of memory descriptor containing ESRT
Changes since v1:
- Remove the esrt_status enum
- Use EFI types
- Fix style nits
- Remove an unused overflow check
Demi Marie Obenour (4):
Grab the EFI System Resource Table and check it
Add a dedicated memory region for the ESRT
Add a new hypercall to get the ESRT
Add emacs file-local variables
xen/arch/arm/efi/efi-boot.h | 1 +
xen/arch/x86/efi/efi-boot.h | 67 +++++++++++++++++++++++++--------
xen/arch/x86/include/asm/e820.h | 2 +-
xen/common/efi/boot.c | 65 ++++++++++++++++++++++++++++++--
xen/common/efi/efi.h | 20 ++++++++++
xen/common/efi/runtime.c | 27 ++++++++++++-
xen/include/efi/efiapi.h | 3 ++
xen/include/public/platform.h | 7 ++++
8 files changed, 172 insertions(+), 20 deletions(-)
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 1/4] Grab the EFI System Resource Table and check it
2022-04-19 15:32 [PATCH v3 0/4] EFI System Resource Table support Demi Marie Obenour
@ 2022-04-19 15:40 ` Demi Marie Obenour
2022-04-27 8:23 ` Jan Beulich
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Demi Marie Obenour @ 2022-04-19 15:40 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu
[-- Attachment #1: Type: text/plain, Size: 8494 bytes --]
The EFI System Resource Table (ESRT) is necessary for fwupd to identify
firmware updates to install. According to the UEFI specification §23.4,
the table shall be stored in memory of type EfiBootServicesData.
Therefore, Xen must avoid reusing that memory for other purposes, so
that Linux can access the ESRT. Additionally, Xen must mark the memory
as reserved, so that Linux knows accessing it is safe.
See https://lore.kernel.org/xen-devel/20200818184018.GN1679@mail-itl/T/
for details.
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
xen/arch/arm/efi/efi-boot.h | 1 +
xen/arch/x86/efi/efi-boot.h | 2 +-
xen/common/efi/boot.c | 50 ++++++++++++++++++++++++++++++++++---
xen/common/efi/efi.h | 18 +++++++++++++
xen/common/efi/runtime.c | 3 ++-
xen/include/efi/efiapi.h | 3 +++
6 files changed, 72 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index e452b687d8..ab2ad3dfe0 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -188,6 +188,7 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *
desc_ptr->Type == EfiLoaderCode ||
desc_ptr->Type == EfiLoaderData ||
(!map_bs &&
+ desc_ptr != esrt_desc &&
(desc_ptr->Type == EfiBootServicesCode ||
desc_ptr->Type == EfiBootServicesData))) )
{
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 6e65b569b0..75937c8a11 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -171,7 +171,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
{
case EfiBootServicesCode:
case EfiBootServicesData:
- if ( map_bs )
+ if ( map_bs || desc == (EFI_MEMORY_DESCRIPTOR *)esrt_desc )
{
default:
type = E820_RESERVED;
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index ac1b235372..31664818c1 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -567,6 +567,38 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
}
#endif
+static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
+
+static bool __init is_esrt_valid(
+ const EFI_MEMORY_DESCRIPTOR *const desc)
+{
+ size_t available_len, len;
+ const UINTN physical_start = desc->PhysicalStart;
+ const ESRT *esrt_ptr;
+
+ len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+ if ( esrt == EFI_INVALID_TABLE_ADDR )
+ return false;
+ if ( physical_start > esrt || esrt - physical_start >= len )
+ return false;
+ /*
+ * The specification requires EfiBootServicesData, but accept
+ * EfiRuntimeServicesData for compatibility
+ */
+ if ( (desc->Type != EfiRuntimeServicesData) &&
+ (desc->Type != EfiBootServicesData) )
+ return false;
+ available_len = len - (esrt - physical_start);
+ if ( available_len < sizeof(*esrt_ptr) )
+ return false;
+ esrt_ptr = (const ESRT *)esrt;
+ if ( esrt_ptr->Version != 1 || !esrt_ptr->Count )
+ return false;
+ return esrt_ptr->Count <=
+ (available_len - sizeof(*esrt_ptr)) /
+ sizeof(esrt_ptr->Entries[0]);
+}
+
/*
* Include architecture specific implementation here, which references the
* static globals defined above.
@@ -857,6 +889,7 @@ static void __init efi_tables(void)
static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID;
static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID;
static EFI_GUID __initdata smbios3_guid = SMBIOS3_TABLE_GUID;
+ static EFI_GUID __initdata esrt_guid = ESRT_GUID;
if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) )
efi.acpi20 = (unsigned long)efi_ct[i].VendorTable;
@@ -868,6 +901,8 @@ static void __init efi_tables(void)
efi.smbios = (unsigned long)efi_ct[i].VendorTable;
if ( match_guid(&smbios3_guid, &efi_ct[i].VendorGuid) )
efi.smbios3 = (unsigned long)efi_ct[i].VendorTable;
+ if ( match_guid(&esrt_guid, &efi_ct[i].VendorGuid) )
+ esrt = (UINTN)efi_ct[i].VendorTable;
}
#ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */
@@ -1056,19 +1091,19 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
EFI_STATUS status;
UINTN info_size = 0, map_key;
bool retry;
-#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
unsigned int i;
-#endif
efi_bs->GetMemoryMap(&info_size, NULL, &map_key,
&efi_mdesc_size, &mdesc_ver);
- info_size += 8 * efi_mdesc_size;
+ info_size += 8 * (efi_mdesc_size + 1);
efi_memmap = efi_arch_allocate_mmap_buffer(info_size);
if ( !efi_memmap )
blexit(L"Unable to allocate memory for EFI memory map");
for ( retry = false; ; retry = true )
{
+ esrt_desc = (const EFI_MEMORY_DESCRIPTOR *)EFI_INVALID_TABLE_ADDR;
+
efi_memmap_size = info_size;
status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size,
efi_memmap, &map_key,
@@ -1077,6 +1112,15 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
if ( EFI_ERROR(status) )
PrintErrMesg(L"Cannot obtain memory map", status);
+ for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
+ {
+ if ( is_esrt_valid(efi_memmap + i) )
+ {
+ esrt_desc = efi_memmap + i;
+ break;
+ }
+ }
+
efi_arch_process_memory_map(SystemTable, efi_memmap, efi_memmap_size,
efi_mdesc_size, mdesc_ver);
diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h
index c9aa65d506..02f499071a 100644
--- a/xen/common/efi/efi.h
+++ b/xen/common/efi/efi.h
@@ -10,6 +10,23 @@
#include <xen/spinlock.h>
#include <asm/page.h>
+typedef struct _ESRT_ENTRY {
+ EFI_GUID FwClass;
+ UINT32 FwType;
+ UINT32 FwVersion;
+ UINT32 FwLowestSupportedVersion;
+ UINT32 FwCapsuleFlags;
+ UINT32 FwLastAttemptVersion;
+ UINT32 FwLastAttemptStatus;
+} ESRT_ENTRY;
+
+typedef struct _ESRT {
+ UINT32 Count;
+ UINT32 Max;
+ UINT64 Version;
+ ESRT_ENTRY Entries[];
+} ESRT;
+
struct efi_pci_rom {
const struct efi_pci_rom *next;
u16 vendor, devid, segment;
@@ -28,6 +45,7 @@ extern const EFI_RUNTIME_SERVICES *efi_rs;
extern UINTN efi_memmap_size, efi_mdesc_size;
extern void *efi_memmap;
+extern const EFI_MEMORY_DESCRIPTOR *esrt_desc;
#ifdef CONFIG_X86
extern mfn_t efi_l4_mfn;
diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c
index 13b0975866..0d09647952 100644
--- a/xen/common/efi/runtime.c
+++ b/xen/common/efi/runtime.c
@@ -52,6 +52,7 @@ static unsigned int efi_rs_on_cpu = NR_CPUS;
UINTN __read_mostly efi_memmap_size;
UINTN __read_mostly efi_mdesc_size;
void *__read_mostly efi_memmap;
+const EFI_MEMORY_DESCRIPTOR *__read_mostly esrt_desc;
UINT64 __read_mostly efi_boot_max_var_store_size;
UINT64 __read_mostly efi_boot_remain_var_store_size;
@@ -269,7 +270,7 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
case XEN_FW_EFI_MEM_INFO:
for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
{
- EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
+ const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
if ( info->mem.addr >= desc->PhysicalStart &&
diff --git a/xen/include/efi/efiapi.h b/xen/include/efi/efiapi.h
index a616d1238a..42ef3e1c8c 100644
--- a/xen/include/efi/efiapi.h
+++ b/xen/include/efi/efiapi.h
@@ -882,6 +882,9 @@ typedef struct _EFI_BOOT_SERVICES {
#define SAL_SYSTEM_TABLE_GUID \
{ 0xeb9d2d32, 0x2d88, 0x11d3, {0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d} }
+#define ESRT_GUID \
+ { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} }
+
typedef struct _EFI_CONFIGURATION_TABLE {
EFI_GUID VendorGuid;
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/4] Grab the EFI System Resource Table and check it
2022-04-19 15:40 ` [PATCH v3 1/4] Grab the EFI System Resource Table and check it Demi Marie Obenour
@ 2022-04-27 8:23 ` Jan Beulich
2022-04-27 8:42 ` Jan Beulich
2022-04-27 9:00 ` Jan Beulich
2 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2022-04-27 8:23 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 19.04.2022 17:40, Demi Marie Obenour wrote:
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -171,7 +171,7 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
> {
> case EfiBootServicesCode:
> case EfiBootServicesData:
> - if ( map_bs )
> + if ( map_bs || desc == (EFI_MEMORY_DESCRIPTOR *)esrt_desc )
No need for the cast afaics, even more so that it casts away const-ness.
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -567,6 +567,38 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image)
> }
> #endif
>
> +static UINTN __initdata esrt = EFI_INVALID_TABLE_ADDR;
> +
> +static bool __init is_esrt_valid(
> + const EFI_MEMORY_DESCRIPTOR *const desc)
As indicated elsewhere before, while we want to have const on pointed-to
types whenever possible, the 2nd const here is unusual in our code base
and hence would imo better be omitted.
> @@ -1056,19 +1091,19 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> EFI_STATUS status;
> UINTN info_size = 0, map_key;
> bool retry;
> -#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
> unsigned int i;
> -#endif
>
> efi_bs->GetMemoryMap(&info_size, NULL, &map_key,
> &efi_mdesc_size, &mdesc_ver);
> - info_size += 8 * efi_mdesc_size;
> + info_size += 8 * (efi_mdesc_size + 1);
What is this needed for? Does this perhaps belong into a later patch?
> --- a/xen/common/efi/efi.h
> +++ b/xen/common/efi/efi.h
> @@ -10,6 +10,23 @@
> #include <xen/spinlock.h>
> #include <asm/page.h>
>
> +typedef struct _ESRT_ENTRY {
> + EFI_GUID FwClass;
> + UINT32 FwType;
> + UINT32 FwVersion;
> + UINT32 FwLowestSupportedVersion;
> + UINT32 FwCapsuleFlags;
> + UINT32 FwLastAttemptVersion;
> + UINT32 FwLastAttemptStatus;
> +} ESRT_ENTRY;
> +
> +typedef struct _ESRT {
> + UINT32 Count;
> + UINT32 Max;
> + UINT64 Version;
> + ESRT_ENTRY Entries[];
> +} ESRT;
The names in the spec, which (as said before) we're trying to follow along
with the gnu-efi package, where we would generally be taking things from,
are EFI_SYSTEM_RESOURCE_ENTRY and EFI_SYSTEM_RESOURCE_TABLE. The field
names of the former also don't all start with "Fw". The field names of the
latter are still quite far off of what the spec says.
Also, why did you move this here? There's no need to expose things in a
header which are used by a single CU.
> @@ -269,7 +270,7 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info *info)
> case XEN_FW_EFI_MEM_INFO:
> for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size )
> {
> - EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
> + const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i;
While I don't mind this change, it also looks unrelated. Perhaps again
needed by (and then supposed to be in) a later patch?
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/4] Grab the EFI System Resource Table and check it
2022-04-19 15:40 ` [PATCH v3 1/4] Grab the EFI System Resource Table and check it Demi Marie Obenour
2022-04-27 8:23 ` Jan Beulich
@ 2022-04-27 8:42 ` Jan Beulich
2022-05-30 8:47 ` Henry Wang
2022-04-27 9:00 ` Jan Beulich
2 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2022-04-27 8:42 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 19.04.2022 17:40, Demi Marie Obenour wrote:
> @@ -1056,19 +1091,19 @@ static void __init efi_exit_boot(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *Syste
> EFI_STATUS status;
> UINTN info_size = 0, map_key;
> bool retry;
> -#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
> unsigned int i;
> -#endif
>
> efi_bs->GetMemoryMap(&info_size, NULL, &map_key,
> &efi_mdesc_size, &mdesc_ver);
> - info_size += 8 * efi_mdesc_size;
> + info_size += 8 * (efi_mdesc_size + 1);
> efi_memmap = efi_arch_allocate_mmap_buffer(info_size);
> if ( !efi_memmap )
> blexit(L"Unable to allocate memory for EFI memory map");
>
> for ( retry = false; ; retry = true )
> {
> + esrt_desc = (const EFI_MEMORY_DESCRIPTOR *)EFI_INVALID_TABLE_ADDR;
Sorry, one more question here: Why is NULL not good enough?
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v3 1/4] Grab the EFI System Resource Table and check it
2022-04-27 8:42 ` Jan Beulich
@ 2022-05-30 8:47 ` Henry Wang
2022-05-30 18:22 ` Demi Marie Obenour
0 siblings, 1 reply; 7+ messages in thread
From: Henry Wang @ 2022-05-30 8:47 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu, xen-devel
Hi,
It seems that this series is stale for more than one month with maintainers
comments given for [1][2] and some discussions between maintainer and author
for [3]. So this email is a gentle reminder for the author about this series (no hurries
and pressure though, please take your time :) ). Thanks!
Kind regards,
Henry
[1] https://patchwork.kernel.org/project/xen-devel/patch/Yl7X3mAJhR5ENSpl@itl-email/
[2] https://patchwork.kernel.org/project/xen-devel/patch/Yl7X/dT39vvhZmho@itl-email/
[3] https://patchwork.kernel.org/project/xen-devel/patch/Yl7aC2a+TtOaFtqZ@itl-email/
> -----Original Message-----
> On 19.04.2022 17:40, Demi Marie Obenour wrote:
> > @@ -1056,19 +1091,19 @@ static void __init efi_exit_boot(EFI_HANDLE
> ImageHandle, EFI_SYSTEM_TABLE *Syste
> > EFI_STATUS status;
> > UINTN info_size = 0, map_key;
> > bool retry;
> > -#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP
> > unsigned int i;
> > -#endif
> >
> > efi_bs->GetMemoryMap(&info_size, NULL, &map_key,
> > &efi_mdesc_size, &mdesc_ver);
> > - info_size += 8 * efi_mdesc_size;
> > + info_size += 8 * (efi_mdesc_size + 1);
> > efi_memmap = efi_arch_allocate_mmap_buffer(info_size);
> > if ( !efi_memmap )
> > blexit(L"Unable to allocate memory for EFI memory map");
> >
> > for ( retry = false; ; retry = true )
> > {
> > + esrt_desc = (const EFI_MEMORY_DESCRIPTOR
> *)EFI_INVALID_TABLE_ADDR;
>
> Sorry, one more question here: Why is NULL not good enough?
>
> Jan
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/4] Grab the EFI System Resource Table and check it
2022-05-30 8:47 ` Henry Wang
@ 2022-05-30 18:22 ` Demi Marie Obenour
0 siblings, 0 replies; 7+ messages in thread
From: Demi Marie Obenour @ 2022-05-30 18:22 UTC (permalink / raw)
To: Henry Wang
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu, xen-devel
[-- Attachment #1: Type: text/plain, Size: 522 bytes --]
On Mon, May 30, 2022 at 08:47:39AM +0000, Henry Wang wrote:
> Hi,
>
> It seems that this series is stale for more than one month with maintainers
> comments given for [1][2] and some discussions between maintainer and author
> for [3]. So this email is a gentle reminder for the author about this series (no hurries
> and pressure though, please take your time :) ). Thanks!
Thanks for the reminder. This series has been superseded by a later
one.
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/4] Grab the EFI System Resource Table and check it
2022-04-19 15:40 ` [PATCH v3 1/4] Grab the EFI System Resource Table and check it Demi Marie Obenour
2022-04-27 8:23 ` Jan Beulich
2022-04-27 8:42 ` Jan Beulich
@ 2022-04-27 9:00 ` Jan Beulich
2 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2022-04-27 9:00 UTC (permalink / raw)
To: Demi Marie Obenour
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, Roger Pau Monné,
Wei Liu, xen-devel
On 19.04.2022 17:40, Demi Marie Obenour wrote:
> --- a/xen/include/efi/efiapi.h
> +++ b/xen/include/efi/efiapi.h
> @@ -882,6 +882,9 @@ typedef struct _EFI_BOOT_SERVICES {
> #define SAL_SYSTEM_TABLE_GUID \
> { 0xeb9d2d32, 0x2d88, 0x11d3, {0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d} }
>
> +#define ESRT_GUID \
> + { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} }
> +
I'm sorry, yet one more remark: This should go here only if the gnu-efi
package also has it there. Otherwise it should be added next to the
other GUIDs in efi/boot.c. This is to make updating of this header from
newer gnu-efi versions as straightforward as possible.
Also please once again use the name from the spec,
EFI_SYSTEM_RESOURCE_TABLE_GUID.
Jan
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-05-30 18:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-19 15:34 [PATCH v3 1/4] Grab the EFI System Resource Table and check it Demi Marie Obenour
-- strict thread matches above, loose matches on Subject: below --
2022-04-19 15:32 [PATCH v3 0/4] EFI System Resource Table support Demi Marie Obenour
2022-04-19 15:40 ` [PATCH v3 1/4] Grab the EFI System Resource Table and check it Demi Marie Obenour
2022-04-27 8:23 ` Jan Beulich
2022-04-27 8:42 ` Jan Beulich
2022-05-30 8:47 ` Henry Wang
2022-05-30 18:22 ` Demi Marie Obenour
2022-04-27 9:00 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.