Re: grub-mkimage and module loading

[Hollis Blanchard]

On Jan 2, 2005, at 3:05 PM, Marco Gerards wrote:

> Hollis Blanchard <address@hidden> writes:
>
> > This is a more complete patch for PPC grub-mkimage that I posted a 
> > while
> > back. Important points:
> > - move grub_load_modules() into i386/pc as grub_arch_load_modules()
> > - implement PPC's grub_arch_load_modules() using an in-memory 
> > structure
> >   telling us how many modules to expect
>
> I would prefer a grub_load_modules and a function to return the
> address and size of the modules in memory.  I explained this in my
> other email.

I suggested the same thing. But I'd really rather have someone who 
understands the PC side to make such changes. I don't have the 
equipment or the interest to rework the PC code. This patch has almost 
no impact on the PC side; sharing the code will be more invasive.

> Anyway, I'd prefer this to work the same on every machine and this
> code to be shared.

I understand. I will see what I can do.

> > - the structure contains a version field to avoid grub-mkimage/grub
> >   binary layout mismatches
>
> I don't think versioning is required.

Fine.

> > I think there is still some PC cleanup needed, e.g. grub_end_addr and
> > grub_add_unused_region() should also be moved into i386 code, but my
> > patch is already fairly large, and I'd prefer a PC person were 
> > involved
> > in this additional cleanup anyways.
>
> Ok.  Can you at least make sure it compiles on the PC and try not to
> break it?

I have tried not to break it. Even compile-testing is going to be 
pretty inconvenient I think, but I'll see...
> > This patch has been tested and is needed to load modules on PPC. 
> > Please
> > comment soon so we can get PPC modules working in the main tree...
>
> I assume you will make a changelog entry when the patch is more or
> less ok?  It's easier to review a patch using a changelog entry.

Yes, but there was no sense in me making a changelog description when 
the patch was just going to be ripped apart anyways. :)

> > Index: include/grub/kernel.h
> > ===================================================================
> > RCS file: /cvsroot/grub/grub2/include/grub/kernel.h,v
> > retrieving revision 1.4
> > diff -u -p -r1.4 kernel.h
> > --- include/grub/kernel.h       4 Apr 2004 13:46:00 -0000       1.4
> > +++ include/grub/kernel.h       2 Jan 2005 19:01:53 -0000
> > @@ -31,8 +31,14 @@ struct grub_module_header
> >    grub_size_t size;
> >  };
> >
> > -/* The start address of the kernel.  */
> > -extern grub_addr_t grub_start_addr;
>
> ...
>
> > -/* Return the end address of the core image.  */
> > -grub_addr_t grub_get_end_addr (void);
>
> Doesn't this break the PC port?

grub_get_end_addr() has only a single caller, and was moved with that 
caller.

grub_start_addr is never used anywhere.

> > +/* TODO: endianness */
>
> Better move this to the TODO list.

Ok.

> > +void load_note (Elf32_Phdr *phdr, const char *dir, FILE *out)
>
> GCS nitpick: put the return type on a separate line:
>
> void
> load_note (Elf32_Phdr *phdr, const char *dir, FILE *out)
>
> Same for all other functions, btw.

Ok.

> > +void load_modules (Elf32_Phdr *phdr, const char *dir, char *mods[], 
> > FILE *out)
> > +{
> > +  char *module_img;
> > +  struct grub_util_path_list *path_list, *p;
>
> Please use separate lines for every declaration:

Ok.

-Hollis