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

[Daniel Kiper <dkiper@net-space.pl>]

On Wed, Jun 30, 2021 at 06:40:10PM +1000, 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>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.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
> --
> 2.30.2

Daniel