Re: [PATCH v2 01/22] ieee1275: drop HEAP_MAX_ADDR, HEAP_MIN_SIZE

[Stefan Berger]

On 6/30/21 4:40 AM, Daniel Axtens wrote:
> HEAP_MAX_ADDR is confusing. Currently it is set to 32MB, except
> on ieee1275 on x86, where it is 64MB.
>
> There is a comment which purports to explain it:
>
> /* If possible, we will avoid claiming heap above this address, because it
>     seems to cause relocation problems with OSes that link at 4 MiB */
>
> This doesn't make a lot of sense when the constants are well above 4MB
> already. It was not always this way. Prior to
> commit 7b5d0fe4440c ("Increase heap limit") in 2010, HEAP_MAX_SIZE and
> HEAP_MAX_ADDR were indeed 4MB. However, when the constants were increased
> the comment was left unchanged.
>
> It's been over a decade. It doesn't seem like we have problems with
> claims over 4MB on powerpc or x86 ieee1275. (sparc does things completely
> differently and never used the constant.)
>
> Drop the constant and the check.
>
> The only use of HEAP_MIN_SIZE was to potentially override the
> HEAP_MAX_ADDR check. It is now unused. Remove it.
>
> Signed-off-by: Daniel Axtens <dja@axtens.net>


I tested this patch and the other 2 memory related patches in a ppc64 
QEMU VM where I need them for getting more memory for trusted boot 
enablement on ppc64. Without these patches it runs out of memory. From 
what I can see they work fine.

Tested-by: Stefan Berger <stefanb@linux.ibm.com>


> ---
>   grub-core/kern/ieee1275/init.c | 17 -----------------
>   1 file changed, 17 deletions(-)
>
> diff --git a/grub-core/kern/ieee1275/init.c b/grub-core/kern/ieee1275/init.c
> index d483e35eed2b..c5d091689f29 100644
> --- a/grub-core/kern/ieee1275/init.c
> +++ b/grub-core/kern/ieee1275/init.c
> @@ -45,9 +45,6 @@
>   #include <grub/machine/kernel.h>
>   #endif
>   
> -/* The minimal heap size we can live with. */
> -#define HEAP_MIN_SIZE          (unsigned long) (2 * 1024 * 1024)
> -
>   /* The maximum heap size we're going to claim */
>   #ifdef __i386__
>   #define HEAP_MAX_SIZE         (unsigned long) (64 * 1024 * 1024)
> @@ -55,14 +52,6 @@
>   #define HEAP_MAX_SIZE         (unsigned long) (32 * 1024 * 1024)
>   #endif
>   
> -/* If possible, we will avoid claiming heap above this address, because it
> -   seems to cause relocation problems with OSes that link at 4 MiB */
> -#ifdef __i386__
> -#define HEAP_MAX_ADDR          (unsigned long) (64 * 1024 * 1024)
> -#else
> -#define HEAP_MAX_ADDR          (unsigned long) (32 * 1024 * 1024)
> -#endif
> -
>   extern char _start[];
>   extern char _end[];
>   
> @@ -183,12 +172,6 @@ heap_init (grub_uint64_t addr, grub_uint64_t len, grub_memory_type_t type,
>     if (*total + len > HEAP_MAX_SIZE)
>       len = HEAP_MAX_SIZE - *total;
>   
> -  /* Avoid claiming anything above HEAP_MAX_ADDR, if possible. */
> -  if ((addr < HEAP_MAX_ADDR) &&                             /* if it's too 
> late, don't bother */
> -      (addr + len > HEAP_MAX_ADDR) &&                               /* if it 
> wasn't available anyway, don't bother */
> -      (*total + (HEAP_MAX_ADDR - addr) > HEAP_MIN_SIZE))    /* only limit 
> ourselves when we can afford to */
> -     len = HEAP_MAX_ADDR - addr;
> -
>     /* In theory, firmware should already prevent this from happening by not
>        listing our own image in /memory/available.  The check below is intended
>        as a safeguard in case that doesn't happen.  However, it doesn't protect