| Message ID | Yl7X/dT39vvhZmho@itl-email (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | EFI System Resource Table support | expand |
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 --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;