Re: [PATCH 7/7] efi: change heap allocation type to GRUB_EFI_LOADER_CODE

[Daniel Kiper]

On Fri, Jul 28, 2017 at 10:35:38PM +0100, Leif Lindholm wrote:
> On Thu, Jul 27, 2017 at 06:06:18PM +0200, Daniel Kiper wrote:
> > > > > > > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > > > > > > index 7b1763bc5..f27a48e68 100644
> > > > > > > --- a/grub-core/kern/efi/mm.c
> > > > > > > +++ b/grub-core/kern/efi/mm.c
> > > > > > > @@ -404,7 +404,9 @@ add_memory_regions 
> > > > > > > (grub_efi_memory_descriptor_t
> > > > > > *memory_map,
> > > > > > >         pages = required_pages;
> > > > > > >       }
> > > > > > >
> > > > > > > -      addr = grub_efi_allocate_pages (start, pages);
> > > > > > > +      addr = grub_efi_allocate_pages_real (start, pages,
> > > > > > > +                                        
> > > > > > > GRUB_EFI_ALLOCATE_ADDRESS,
> > > > > > > +                                        GRUB_EFI_LOADER_CODE);
> > > > > >
> > > > > > Is it really needed? Is any module exectued in place or does it 
> > > > > > contain
> > > > > > code ready to run out of the box?
> > > > > >
> > > > > All the modules are loaded to heap
> > > >
> > > > I do not see why modules have to be loaded into memory region with
> > > > GRUB_EFI_LOADER_CODE type.
> > >
> > > He means grub modules not initrd or multiboot modules
> >
> > Ahhh... Right, then it should be correct. Though I would double
> > check it applies to GRUB2 modules only.
>
> It applies to any executable code running until an operating system
> takes over.

It looks that I was misunderstood. I hope that this change only influence
allocations for GRUB2 modules and does not change allocation behavior for
others, i.e. OS kernels, Multiboot proto, Multiboo2 proto, etc.

> Don't get me wrong - this is more of a workaround than a fix for the
> grub module loading. But properly separating code/data and ro/rw
> regions is a much more invasive change which deserves more discussion.

Yep, you are right. Added to TODO list.

Daniel