Re: [PATCH v3 2/4] Add a dedicated memory region for the ESRT

[Jan Beulich <jbeulich@xxxxxxxx>]

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