Re: [patch] rough Mac OS X loader

[Hollis Blanchard]

Thanks for your comments.

On Jan 2, 2006, at 11:29 AM, Marco Gerards wrote:
> > Index: include/grub/xcoff.h
> > ===================================================================
>
> Is it possible to use the grub_ namespace in this file?  In elf.h this
> is not done because it should be possible to update it from glibc,
> AFAIK.

Ok, that should be fine.

> > Index: include/grub/powerpc/ieee1275/kernel.h
> > ===================================================================
> > RCS file: /cvsroot/grub/grub2/include/grub/powerpc/ieee1275/kernel.h,v
> > retrieving revision 1.3
> > diff -u -p -r1.3 kernel.h
> > --- include/grub/powerpc/ieee1275/kernel.h	3 Aug 2005 22:53:50 
> > -0000	1.3
> > +++ include/grub/powerpc/ieee1275/kernel.h      2 Jan 2006 00:34:11 -0000
> > @@ -26,6 +26,9 @@ void EXPORT_FUNC (abort) (void);
> >  void EXPORT_FUNC (grub_reboot) (void);
> >  void EXPORT_FUNC (grub_halt) (void);
> >
> > +void EXPORT_FUNC (grub_jump) (unsigned long text, unsigned long 
> > stack,
> > +                             unsigned long arg1, unsigned long arg2);
>
> Shouldn't a pointer be used here?

Used where?

> > +/* BootX, the Mac OS X bootloader, is an XCOFF executable with a 
> > CHRP script
> > + * prepended to it.  We skip the script and load the XCOFF file.  */
>
> What is in this script?  Are you completely sure it can be skipped?

It loads the XCOFF into memory. Yes.

> Can you please remove the `*' on the second line so the comment
> matches the style of the other sourcecode?

Yes.

> > +static grub_err_t
> > +grub_macosx_release_mem (void)
> > +{
> > +  /* XXX write me */
>
> I assume you encountered a bug in the firmware while writing this
> function? :-)

No, just haven't written it yet. :) I think the XCOFF sections in BootX 
are contiguous though, so if that's true it should not be difficult.

> > +  /* Load all section headers.  */
> > +  scnhdr_bytes = filehdr->f_nscns * sizeof (struct xcoff_scnhdr);
> > +  scnhdrs = grub_malloc (scnhdr_bytes);
> > +  if (! scnhdrs)
> > +    return grub_errno;
> > +
> > +  if (grub_file_read (file, (void *)scnhdrs, scnhdr_bytes) != 
> > scnhdr_bytes)
> > +    return grub_error (GRUB_ERR_READ_ERROR,
> > +                      "could not read XCOFF section headers");
>
> You don't free scnhdrs here, is that intentional?

Accidental, thanks.

> > +/* Find NULL-terminated `needle' in non-terminated `haystack'.  */
> > +static void *
> > +grub_memstr (void *haystack, int len, char *needle)
>
> Perhaps it is better to move this to kern/misc.c?

I had that thought, but "memstr" is not a standard POSIX function so I 
wasn't sure.

> > +/* grub_chain_trampoline is copied somewhere other than its link 
> > address
> > + * and executes after all other code has been blown away. In order 
> > to call
> > + * other functions from here, they must have been copied into a safe 
> > place and
> > + * their addresses passed as parameters. */
>
> Same as above.
>
> > +typedef void (*kernel_entry_t) (unsigned long, unsigned long, int 
> > (void *));
> > +typedef int (*release_t) (grub_addr_t addr, grub_size_t size);
> > +static void
> > +grub_chain_trampoline (kernel_entry_t entry, release_t release)
> > +{
> > +  release (0x00003000, 0x06000000 - 0x3000);
> > +  entry (0, 0, grub_ieee1275_entry_fn);
> > +}
> > +
> > +static int
> > +grub_macosx_install_trampoline (void)
> > +{
> > +  int rc;
> > +
> > +  /* XXX don't use magic numbers here */
> > +  rc = grub_claimmap (GRUB_MACOSX_CHAIN_AREA, 0x200);
> > +  rc |= grub_claimmap (GRUB_MACOSX_RELEASE_AREA, 0x200);
> > +  rc |= grub_claimmap (GRUB_MACOSX_STACK_AREA, 0x200);
> > +  if (rc)
> > +    return rc;
> > +
> > +  grub_memcpy ((void *) GRUB_MACOSX_CHAIN_AREA, 
> > grub_chain_trampoline, 0x200);
> > +  grub_arch_sync_caches ((void *) GRUB_MACOSX_CHAIN_AREA, 0x200);
> > +  grub_memcpy ((void *) GRUB_MACOSX_RELEASE_AREA, 
> > grub_ieee1275_release, 0x200);
> > +  grub_arch_sync_caches ((void *) GRUB_MACOSX_RELEASE_AREA, 0x200);
>
> What is 0x200?  The maximum size of grub_chain_trampoline?  Perhaps
> you can do something like: `grub_macosx_release_area -
> grub_chain_trampoline', or do you think that is too unpredictable and
> too ugly?

We're copying several functions elsewhere. The problem is this: how big 
is that function? In other words, how many bytes do we need to copy? I 
don't have an answer.

-Hollis