[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Wed, 27 Apr 2022 10:40:06 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Y5QjRMoLiHAlv2i4iqi7JlyFkL8HZ1i6/D6f562725Y=; b=Qb8xsDx8jJWf3K6qAD6OsTIrP3Cs0+0xfHRCWQjatkHPIJ5LsbPDIz+NCkzWTrWQ6sdkSv+fewd7jHXL9g7Wz2ZdIgz+b2ZZJC/Pfv7A5+P9a/imOdSN82uZgJ9ugRVdX8yjBzxuJTO7AZfb9pB5hXT+6qSMPdaE2suKw6DXaK2FA69UxcSf6zaLB6a4lzX5eqeqcrEAtIZnDGq2DSjn2YljW2wlgUwvU1T1a+oHL5gc4hiVgZ2ZypZQ3JcDcr1Qyybw+rzyOTtCMHh1evd/Bg5MqoOMNlh0vv1PPN0b54IoM2HxqhWDNOUlODHd6YNiiIm8vl4E8sGrlwoNzfOozQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=PHRZL/zTLQJd/RfbH5zDXMZBvOZUoTJN/P+HOnDi+QZynkZ6LlZ4kZ+6rKcNk8ZqMsFLbC0+VjnZ0pYyIFpzV41BO19faN3cxnU5GA2VB/b3whG46/b/CM7LIkcM8m+/fQOlI8qWOxMX3ha7ixBoDIR80MDmMXCD3VCEqCtINRd8S6RapVVv4wnjyvoaluwfeJ3nEIT0x9i2AxJu/tK5pLIVinkn5MIlaMwdeLwW1oKlI7MUnf3q24kYXy3IWflDbWPxNBL3d6KLy4hYOy6ffZzMl6ZGSVTWotYcqnJVwUMBN6/BC6bdL3f1bsR4Slit/gyIrSLxmTyGvZF32S1aYQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Wed, 27 Apr 2022 08:40:14 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.