Re: [OpenBIOS] [PATCH 18/24] aout: implement load/init-program as per IEEE-1275 specification

[BALATON Zoltan]

On Mon, 5 Sep 2016, Mark Cave-Ayland wrote:
> load should place the file at load-base, whilst init-program should parse
> the memory at load-base and set up the context accordingly.
> Signed-off-by: Mark Cave-Ayland
>
> libopenbios/aout_load.c |   44 +++++++++++++++++++++++---------------------
> libopenbios/load.c      |   15 ++++++++-------
> 2 files changed, 31 insertions(+), 28 deletions(-)
>
> diff --git a/libopenbios/aout_load.c b/libopenbios/aout_load.c
> index 2433106..4dc9ac3 100644
> --- a/libopenbios/aout_load.c
> +++ b/libopenbios/aout_load.c
> @@ -10,15 +10,10 @@
> #define CONFIG_SPARC64_PAGE_SIZE_8KB
> #endif
>
> -/* NextStep bootloader on SPARC32 expects the a.out header directly
> below load-base (0x4000) */
>
> -#ifdef CONFIG_SPARC32
> -#define AOUT_HEADER_COPY
> -#endif
>
> #include "libopenbios/sys_info.h"
> #include "libopenbios/bindings.h"
> #include "libopenbios/aout_load.h"
> +#include "libopenbios/initprogram.h"
> #include "libc/diskio.h"
> #define printf printk
> #define debug printk
> @@ -85,7 +80,7 @@ aout_load(struct sys_info *info, ihandle_t dev)
> /* Mark the saved-program-state as invalid */
> feval("0 state-valid !");
> fd = open_ih(dev);
>   if (fd == -1) {
>  goto out;
>
> @@ -123,15 +118,16 @@ aout_load(struct sys_info *info, ihandle_t dev)
>     fword("load-base");
>     start = POP(); // N_TXTADDR(ehdr);
>
> memcpy((void *)start, &ehdr, sizeof(ehdr));
>
> if (!check_mem_ranges(info, start, size))
>  goto out;
>
> printf("Loading a.out %s...\n", image_name ? image_name : "image");
>
> seek_io(fd, offset + N_TXTOFF(ehdr));
>
> if (N_MAGIC(ehdr) == NMAGIC) {
>    if ((size_t)read_io(fd, (void *)start, ehdr.a_text) != ehdr.a_text) {
>    if ((size_t)read_io(fd, (void *)(start + N_TXTOFF(ehdr)), ehdr.a_text) != ehdr.a_text) {
>       printf("Can't read program text segment (size 0x" FMT_aout_ehdr ")\n", ehdr.a_text);
>       goto out;
>   }
>
> @@ -140,7 +136,7 @@ aout_load(struct sys_info *info, ihandle_t dev)
>             goto out;
>         }
>     } else {
>    if ((size_t)read_io(fd, (void *)start, size) != size) {
>    if ((size_t)read_io(fd, (void *)(start + N_TXTOFF(ehdr)), size) != size) {
>       printf("Can't read program (size 0x" FMT_sizet ")\n", size);
>       goto out;
>   }
>
> @@ -149,20 +145,11 @@ aout_load(struct sys_info *info, ihandle_t dev)
>     debug("Loaded %lu bytes\n", size);
>     debug("entry point is %#lx\n", start);
>
> -#ifdef AOUT_HEADER_COPY
> // Copy the a.out header just before start
> memcpy((char *)(start - 0x20), &ehdr, 0x20);
>
> -#endif
>
> // Initialise saved-program-state
>
> PUSH(addr_fixup(start));
>
> feval("load-state >ls.entry !");
> PUSH(size);
> feval("load-state >ls.file-size !");
> feval("aout load-state >ls.file-type !");
>
> feval("-1 state-valid !");
>
> out:
>     close_io(fd);
>     return retval;
> @@ -171,6 +158,21 @@ out:
> void
> aout_init_program(void)
> {
> // Currently not implemented
> feval("0 state-valid !");
> ucell start, size;
> // Relocate a.out text down from load-base to load-base - header. This
> // is similar to what OBP and is needed for NextStep.
> fword("load-base");
> start = POP();
> feval("load-state >ls.file-size @");
> size = POP();
> memcpy((char *)start - sizeof(struct exec), (char *)start, size);

Shouldn't this be memmove() to handle overlapping regions correctly? (I 
assume size is often bigger than sizeof(struct exec).) Maybe it works with 
the current memcpy implementation in OpenBIOS but in case it's ever 
replaced memmove may be better.

Regards,
BALATON Zoltan
> PUSH(start);
> feval("load-state >ls.entry !");
> arch_init_program();
> feval("-1 state-valid !");
>
> }
> diff --git a/libopenbios/load.c b/libopenbios/load.c
> index 1c54d19..f43679d 100644
> --- a/libopenbios/load.c
> +++ b/libopenbios/load.c
> @@ -17,6 +17,7 @@
> #include "config.h"
> #include "kernel/kernel.h"
> #include "libopenbios/bindings.h"
> +#include "libopenbios/initprogram.h"
> #include "libopenbios/sys_info.h"
> #include "libopenbios/load.h"
>
> @@ -50,12 +51,15 @@ void load(ihandle_t dev)
> {
>    /* Invoke the loaders on the specified device */
>    char *param;
> ucell valid;
>
> ucell valid = 0;
>
> /* TODO: Currently the internal loader APIs use load-base directly, so
>  drop the address */
> POP();
>
> /* Temporarily keep compiler quiet during transition */
>
> valid = valid;
>
> #ifdef CONFIG_LOADER_ELF
>
> /* Grab the boot arguments */
> @@ -77,12 +81,9 @@ void load(ihandle_t dev)
> #endif
>
> #ifdef CONFIG_LOADER_AOUT
> aout_load(&sys_info, dev);
>    feval("state-valid @");
>    valid = POP();
>    if (valid) {
>            feval("load-state >ls.file-size @");
>            return;
> if (aout_load(&sys_info, dev) != LOADER_NOT_SUPPORT) {
>        feval("load-state >ls.file-size @");
>        return;
>   }
>
> #endif