Re: [PATCH 1/9] kern/ieee1275/init: ppc64: Introduce a request for regions_claim

[Daniel Kiper]

On Tue, Oct 31, 2023 at 01:50:23PM -0400, Stefan Berger wrote:
> The regions_claim() function limits the allocation of memory regions
> by excluding certain memory areas from being used by grub. This for
> example includes a gap between 640MB and 768MB as well as an upper
> limit beyond which no memory may be used when an fadump is present.
> However, the ieee1275 loader for kernel and initrd currently does not
> current use regions_claim() for memory allocation on PowerVM and
> PowerKVM and therefore may allocate memory in those areas that it
> should not use.
>
> To make the regions_claim() function more flexible and ulimately usable
> for the ieee1275 loader, introduce a request structure to pass various
> parameters to the regions_claim() function that describe the properties
> of requested memory chunks. In a first step, move the total and flags
> variables into this structure.
>
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Cc: Hari Bathini <hbathini@linux.ibm.com>
> Cc: Pavithra Prakash <pavrampu@in.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Carolyn Scherrer <cpscherr@us.ibm.com>
> Cc: Mahesh Salgaonkar <mahesh@linux.ibm.com>
> Cc: Sourabh Jain <sourabhjain@linux.ibm.com>
> ---
>  grub-core/Makefile.am                 |  1 +
>  grub-core/kern/ieee1275/init.c        | 43 ++++++++++++++++++++-------
>  include/grub/powerpc/ieee1275/alloc.h | 11 +++++++
>  3 files changed, 45 insertions(+), 10 deletions(-)
>  create mode 100644 include/grub/powerpc/ieee1275/alloc.h
>
> diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
> index f0cb2f2cc..4f9f3f963 100644
> --- a/grub-core/Makefile.am
> +++ b/grub-core/Makefile.am
> @@ -240,6 +240,7 @@ endif
>
>  if COND_powerpc_ieee1275
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/ieee1275/ieee1275.h
> +KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/powerpc/ieee1275/alloc.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/terminfo.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/extcmd.h
>  KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/lib/arg.h
> diff --git a/grub-core/kern/ieee1275/init.c b/grub-core/kern/ieee1275/init.c
> index d6c9c9049..061c28c98 100644
> --- a/grub-core/kern/ieee1275/init.c
> +++ b/grub-core/kern/ieee1275/init.c
> @@ -46,6 +46,9 @@
>  #ifdef __sparc__
>  #include <grub/machine/kernel.h>
>  #endif
> +#ifdef __powerpc__
> +#include <grub/powerpc/ieee1275/alloc.h>
> +#endif
>
>  /* The maximum heap size we're going to claim at boot. Not used by sparc. */
>  #ifdef __i386__
> @@ -317,9 +320,9 @@ count_free (grub_uint64_t addr, grub_uint64_t len, 
> grub_memory_type_t type,
>
>  static int
>  regions_claim (grub_uint64_t addr, grub_uint64_t len, grub_memory_type_t 
> type,
> -           unsigned int flags, void *data)
> +            void *data)
>  {
> -  grub_uint32_t total = *(grub_uint32_t *) data;
> +  struct regions_claim_request *rcr = data;
>    grub_uint64_t linux_rmo_save;
>
>    if (type != GRUB_MEMORY_AVAILABLE)
> @@ -499,11 +502,11 @@ regions_claim (grub_uint64_t addr, grub_uint64_t len, 
> grub_memory_type_t type,
>              }
>          }
>      }
> -  if (flags & GRUB_MM_ADD_REGION_CONSECUTIVE && len < total)
> +  if (rcr->flags & GRUB_MM_ADD_REGION_CONSECUTIVE && len < rcr->total)
>      return 0;
>
> -  if (len > total)
> -    len = total;
> +  if (len > rcr->total)
> +    len = rcr->total;
>
>    if (len)
>      {
> @@ -513,12 +516,12 @@ regions_claim (grub_uint64_t addr, grub_uint64_t len, 
> grub_memory_type_t type,
>        if (err)
>       return err;
>        grub_mm_init_region ((void *) (grub_addr_t) addr, len);
> -      total -= len;
> +      rcr->total -= len;
>      }
>
> -  *(grub_uint32_t *) data = total;
> +  *(grub_uint32_t *) data = rcr->total;
>
> -  if (total == 0)
> +  if (rcr->total == 0)
>      return 1;
>
>    return 0;
> @@ -528,14 +531,34 @@ static int
>  heap_init (grub_uint64_t addr, grub_uint64_t len, grub_memory_type_t type,
>          void *data)
>  {
> -  return regions_claim (addr, len, type, GRUB_MM_ADD_REGION_NONE, data);
> +  struct regions_claim_request rcr = {
> +    .flags = GRUB_MM_ADD_REGION_NONE,
> +    .total = *(grub_uint32_t *) data,
> +  };
> +  int ret;
> +
> +  ret = regions_claim (addr, len, type, &rcr);
> +
> +  *(grub_uint32_t *) data = rcr.total;
> +
> +  return ret;
>  }
>
>  static int
>  region_claim (grub_uint64_t addr, grub_uint64_t len, grub_memory_type_t type,
>          void *data)
>  {
> -  return regions_claim (addr, len, type, GRUB_MM_ADD_REGION_CONSECUTIVE, 
> data);
> +  struct regions_claim_request rcr = {
> +    .flags = GRUB_MM_ADD_REGION_CONSECUTIVE,
> +    .total = *(grub_uint32_t *) data,
> +  };
> +  int ret;
> +
> +  ret = regions_claim (addr, len, type, &rcr);
> +
> +  *(grub_uint32_t *) data = rcr.total;
> +
> +  return ret;
>  }
>
>  static grub_err_t
> diff --git a/include/grub/powerpc/ieee1275/alloc.h 
> b/include/grub/powerpc/ieee1275/alloc.h
> new file mode 100644
> index 000000000..97b8c2683
> --- /dev/null
> +++ b/include/grub/powerpc/ieee1275/alloc.h
> @@ -0,0 +1,11 @@

Missing standard license header. Please add it.

> +#ifndef GRUB_POWERPC_IEEE1275_ALLOC_HEADER
> +#define GRUB_POWERPC_IEEE1275_ALLOC_HEADER   1
> +
> +#include <grub/memory.h>
> +
> +struct regions_claim_request {
> +  unsigned int flags;     /* GRUB_MM_ADD_REGION_(NONE|CONSECUTIVE) */
> +  grub_uint32_t total;    /* number of requested bytes */
> +};
> +
> +#endif /* GRUB_POWERPC_IEEE1275_ALLOC_HEADER */

Daniel