Re: Multiboot ELF segment handling patch

[Daniel Kiper]

On Thu, Apr 26, 2018 at 11:55:22PM +0200, Alexander Boettcher wrote:
> On 24.04.2018 12:42, Daniel Kiper wrote:
> > On Mon, Apr 23, 2018 at 08:26:34PM +0200, Alexander Boettcher wrote:
> >> On 17.04.2018 21:40, Daniel Kiper wrote
> >>>> The overriden memory may contain device memory (vga text mode e.g.), 
> >>>> which
> >>>> leads to strange boot behaviour.
> >>>
> >>> Have you been able to take a look at memory allocator/relocator code why
> >>> this happened?
> >>
> >> for me it looks like, that either already
> >> grub_relocator_alloc_chunk_addr or at latest
> >> grub_relocator_prepare_relocs should bail out with an error, if the
> >> memory range of the VGA memory (0xa0000 ++) is covered.
> >
> > Exactly!
> >
> >> What I don't know, respectively didn't found in the code, how and where
> >> this information about this VGA text memory area is excluded/reserved
> >> from the allocator/reallocator. Maybe you can give me a hint ?
> >
> > I have skimmed through the code and I think that you should take
> > a look at grub-core/kern/i386/pc/init.c, grub-core/kern/mm.c and
> > grub-core/lib/relocator.c. At first sight it looks that
> > grub-core/kern/i386/pc/init.c:grub_machine_init() is the
> > most interesting thing.
> >
> > I hope that helps. If you have further questions drop me a line.
> >
> > Thank you for taking a stab at this.
>
> The whole lower 1M physical memory is reserved and not available for use
> (done by grub-core/kern/i386/pc/init.c), but effectively ignored by
> grub_relocator_alloc_chunk_addr for the non relocatable case during
> multiboot segment load.
>
> The adjust_limits call in grub_relocator_alloc_chunk_addr mainly sets
> the min_addr to 0 and max_addr to end of physical memory. So
> grub_relocator_alloc_chunk_addr will ever find a piece of (higher)
> memory, mainly ignoring the target address (which is reserved, e.g.
> below 1M).
>
> The showcase code snippet below catches the case now. I don't know
> whether it is correct nor do I like it ...
>
> What do you think ?
>
>
> Alex
>
>
> --- a/grub-core/lib/relocator.c
> +++ b/grub-core/lib/relocator.c
> @@ -1226,7 +1237,7 @@ adjust_limits (struct grub_relocator *rel,
>  grub_err_t
>  grub_relocator_alloc_chunk_addr (struct grub_relocator *rel,
>                                grub_relocator_chunk_t *out,
> -                              grub_phys_addr_t target, grub_size_t size)
> +                              grub_phys_addr_t target, grub_size_t size, int 
> relocatable)
>  {
>    struct grub_relocator_chunk *chunk;
>    grub_phys_addr_t min_addr = 0, max_addr;
> @@ -1250,6 +1263,10 @@ grub_relocator_alloc_chunk_addr (struct
> grub_relocator *rel,
>               (unsigned long long) min_addr, (unsigned long long) max_addr,
>               (unsigned long long) target);
>
> +  if (!relocatable &&
> +      !malloc_in_range (rel, target, target + size, 1, size, chunk, 0, 1))
> +    return grub_error (GRUB_ERR_BUG, "target memory not available");
> +
>    do
>      {
>        /* A trick to improve Linux allocation.  */

AIUI grub_relocator_alloc_chunk_addr() should allocate memory region
with exact properties, i.e, target and size. So, I think that relocatable
argument does not make sense here. However, I have a feeling that this
piece of code is a problem:

1255          /* A trick to improve Linux allocation.  */
1256    #if defined (__i386__) || defined (__x86_64__)
1257          if (target < 0x100000)
1258            if (malloc_in_range (rel, rel->highestnonpostaddr, 
~(grub_addr_t)0, 1,
1259                                 size, chunk, 0, 1))
1260              {
1261                if (rel->postchunks > chunk->src)
1262                  rel->postchunks = chunk->src;
1263                break;
1264              }
1265    #endif

There is a chance that if you comment it out then Multiboot/Multiboot2
will work as expected. If it is true then we have to think how to
fix it and do not break Linux boot.

Daniel