diff mbox series

[v3,2/4] Add a dedicated memory region for the ESRT

Message ID Yl7X/dT39vvhZmho@itl-email (mailing list archive)
State New
Headers show
Series EFI System Resource Table support | expand

Commit Message

Demi Marie Obenour April 19, 2022, 3:40 p.m. UTC
This allows the ESRT to be marked as reserved without having to waste a
potentially large amount of memory.  This patch assumes that Xen can
handle memory regions that are not page-aligned.  If it cannot,
additional code will need to be added to align the regions.
---
 xen/arch/x86/efi/efi-boot.h     | 69 +++++++++++++++++++++++++--------
 xen/arch/x86/include/asm/e820.h |  2 +-
 2 files changed, 54 insertions(+), 17 deletions(-)

Comments

Jan Beulich April 27, 2022, 8:40 a.m. UTC | #1
On 19.04.2022 17:40, Demi Marie Obenour wrote:
> This allows the ESRT to be marked as reserved without having to waste a
> potentially large amount of memory.  This patch assumes that Xen can
> handle memory regions that are not page-aligned.  If it cannot,
> additional code will need to be added to align the regions.
> ---

This lacks an S-o-b and perhaps also a Suggested-by or Requested-by.

As to the mentioned assumption, I'm of the opinion that you as the
author would need to check whether the assumption holds or whether,
as you say, more code needs to be added. Or else I think such a
change would want tagging as RFC.

> @@ -198,21 +199,57 @@ static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
>              type = E820_NVS;
>              break;
>          }
> -        if ( e820_raw.nr_map && type == e->type &&
> -             desc->PhysicalStart == e->addr + e->size )
> -            e->size += len;
> -        else if ( !len || e820_raw.nr_map >= ARRAY_SIZE(e820_raw.map) )
> -            continue;
> -        else
> +
> +#define ADD_ENTRY(len, type_, physical_start)                           \

I think the order would be less unexpected as (start, len, type),
especially when actually seeing the macro in use further down.

> +        if ( len )                                                      \
> +        {                                                               \
> +            if ( e820_raw.nr_map && (type_) == e->type &&               \
> +                 (physical_start) == e->addr + e->size )                \
> +                e->size += (len);                                       \
> +            else if ( e820_raw.nr_map < ARRAY_SIZE(e820_raw.map) )      \
> +                continue;                                               \
> +            else                                                        \
> +            {                                                           \
> +                ++e;                                                    \
> +                e->addr = (physical_start);                             \
> +                e->size = (len);                                        \
> +                e->type = (type_);                                      \
> +                ++e820_raw.nr_map;                                      \
> +            }                                                           \
> +        }                                                               \
> +        else                                                            \
> +            do {} while (0)

This is odd to see. What we usually do in such cases is to enclose
the whole construct in do { ... } while (0), or to convert the
statement to an expression, by enclosing it in ({ }).

> +        if ( desc == (EFI_MEMORY_DESCRIPTOR *)esrt_desc )
>          {
> -            ++e;
> -            e->addr = desc->PhysicalStart;
> -            e->size = len;
> -            e->type = type;
> -            ++e820_raw.nr_map;
> +            const ESRT *esrt_ptr;
> +            UINTN esrt_offset, esrt_len;
> +
> +            BUG_ON(physical_start > esrt);
> +            BUG_ON(len < sizeof(*esrt_ptr));
> +            esrt_offset = esrt - physical_start;
> +
> +            BUG_ON(len - sizeof(*esrt_ptr) < esrt_offset);
> +            esrt_ptr = (const ESRT *)esrt;
> +
> +            BUG_ON(esrt_ptr->Version != 1);
> +            BUG_ON(esrt_ptr->Count < 1);
> +
> +            esrt_len = (esrt_ptr->Count + 1) * sizeof(*esrt_ptr);
> +
> +            BUG_ON( len - esrt_offset < esrt_len );

Nit: Excess blanks immediately inside the parentheses.

> --- a/xen/arch/x86/include/asm/e820.h
> +++ b/xen/arch/x86/include/asm/e820.h
> @@ -16,7 +16,7 @@ struct __packed e820entry {
>      uint32_t type;
>  };
>  
> -#define E820MAX	1024
> +#define E820MAX	1026

Why?

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 75937c8a11..edf1fea3e0 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -165,13 +165,14 @@  static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
     {
         EFI_MEMORY_DESCRIPTOR *desc = map + i;
         u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT;
+        UINTN physical_start = desc->PhysicalStart;
         u32 type;
 
         switch ( desc->Type )
         {
         case EfiBootServicesCode:
         case EfiBootServicesData:
-            if ( map_bs || desc == (EFI_MEMORY_DESCRIPTOR *)esrt_desc )
+            if ( map_bs )
             {
         default:
                 type = E820_RESERVED;
@@ -179,9 +180,9 @@  static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
             }
             /* fall through */
         case EfiConventionalMemory:
-            if ( !trampoline_phys && desc->PhysicalStart + len <= 0x100000 &&
-                 len >= cfg.size && desc->PhysicalStart + len > cfg.addr )
-                cfg.addr = (desc->PhysicalStart + len - cfg.size) & PAGE_MASK;
+            if ( !trampoline_phys && physical_start + len <= 0x100000 &&
+                 len >= cfg.size && physical_start + len > cfg.addr )
+                cfg.addr = (physical_start + len - cfg.size) & PAGE_MASK;
             /* fall through */
         case EfiLoaderCode:
         case EfiLoaderData:
@@ -198,21 +199,57 @@  static void __init efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable,
             type = E820_NVS;
             break;
         }
-        if ( e820_raw.nr_map && type == e->type &&
-             desc->PhysicalStart == e->addr + e->size )
-            e->size += len;
-        else if ( !len || e820_raw.nr_map >= ARRAY_SIZE(e820_raw.map) )
-            continue;
-        else
+
+#define ADD_ENTRY(len, type_, physical_start)                           \
+        if ( len )                                                      \
+        {                                                               \
+            if ( e820_raw.nr_map && (type_) == e->type &&               \
+                 (physical_start) == e->addr + e->size )                \
+                e->size += (len);                                       \
+            else if ( e820_raw.nr_map < ARRAY_SIZE(e820_raw.map) )      \
+                continue;                                               \
+            else                                                        \
+            {                                                           \
+                ++e;                                                    \
+                e->addr = (physical_start);                             \
+                e->size = (len);                                        \
+                e->type = (type_);                                      \
+                ++e820_raw.nr_map;                                      \
+            }                                                           \
+        }                                                               \
+        else                                                            \
+            do {} while (0)
+
+        if ( desc == (EFI_MEMORY_DESCRIPTOR *)esrt_desc )
         {
-            ++e;
-            e->addr = desc->PhysicalStart;
-            e->size = len;
-            e->type = type;
-            ++e820_raw.nr_map;
+            const ESRT *esrt_ptr;
+            UINTN esrt_offset, esrt_len;
+
+            BUG_ON(physical_start > esrt);
+            BUG_ON(len < sizeof(*esrt_ptr));
+            esrt_offset = esrt - physical_start;
+
+            BUG_ON(len - sizeof(*esrt_ptr) < esrt_offset);
+            esrt_ptr = (const ESRT *)esrt;
+
+            BUG_ON(esrt_ptr->Version != 1);
+            BUG_ON(esrt_ptr->Count < 1);
+
+            esrt_len = (esrt_ptr->Count + 1) * sizeof(*esrt_ptr);
+
+            BUG_ON( len - esrt_offset < esrt_len );
+
+            ADD_ENTRY(esrt_offset, type, physical_start);
+
+            ADD_ENTRY(esrt_len, E820_RESERVED, esrt);
+
+            physical_start = esrt + esrt_len;
+            len -= esrt_offset + esrt_len;
         }
-    }
 
+        ADD_ENTRY(len, type, physical_start);
+    }
+#undef ADD_ENTRY
 }
 
 static void *__init efi_arch_allocate_mmap_buffer(UINTN map_size)
diff --git a/xen/arch/x86/include/asm/e820.h b/xen/arch/x86/include/asm/e820.h
index 92f5efa4f5..98eca96425 100644
--- a/xen/arch/x86/include/asm/e820.h
+++ b/xen/arch/x86/include/asm/e820.h
@@ -16,7 +16,7 @@  struct __packed e820entry {
     uint32_t type;
 };
 
-#define E820MAX	1024
+#define E820MAX	1026
 
 struct e820map {
     unsigned int nr_map;