Re: [Qemu-devel] [PATCH v4 15/23] i386: add bios linker/loader

[Michael S. Tsirkin]

On Mon, Sep 23, 2013 at 03:39:10PM +0200, Paolo Bonzini wrote:
> Il 23/09/2013 15:36, Michael S. Tsirkin ha scritto:
> > On Mon, Sep 23, 2013 at 02:48:57PM +0200, Paolo Bonzini wrote:
> >> Il 22/09/2013 15:37, Michael S. Tsirkin ha scritto:
> >>> This adds a dynamic bios linker/loader.
> >>> This will be used by acpi table generation
> >>> code to:
> >>>     - load each table in the appropriate memory segment
> >>>     - link tables to each other
> >>>     - fix up checksums after said linking
> >>>
> >>> Signed-off-by: Michael S. Tsirkin <address@hidden>
> >>> ---
> >>>  include/hw/i386/bios-linker-loader.h |  26 ++++++
> >>>  hw/i386/bios-linker-loader.c         | 156 
> >>> +++++++++++++++++++++++++++++++++++
> >>>  hw/i386/Makefile.objs                |   1 +
> >>
> >> Just a naming question...
> >>
> >> If this is ACPI-specific (or if there are no other uses in sight), it
> >> should go in include/hw/acpi and hw/acpi, and renamed with s/bios/acpi/ 
> >> etc.
> >>
> >> If this is i386-specific, it should go entirely in hw/i386 and the
> >> headers should be included as "bios-linker-loader.h".
> > 
> > Definitely not ACPI specific.
> > It's only used on i386 so I put it all under i386.
> > You think we should replace
> > #include "hw/i386/bios-linker-loader.h" with
> > #include "bios-linker-loader.h"?
> > 
> > I don't really care, even though this makes headers
> > hard to find. What's the benefit?
> 
> Private headers are outside include/, public headers within it.

Ah you mean move it outside include/. OK.

>  Same
> conventions as Linux.  But if it's not ACPI-specific, I don't feel too
> strongly about it.
> 
> > It's not easy to find a good name for it.
> > For now I'll just make all names bios_linker_loader_ consistently.
> > We can always rename it later, it's not set in stone.
> 
> Ok.
> 
> >>> +struct BiosLinkerLoaderEntry {
> >>> +    uint32_t command;
> >>> +    union {
> >>> +        /*
> >>> +         * COMMAND_ALLOCATE - allocate a table from @alloc_file
> >>> +         * subject to @alloc_align alignment (must be power of 2)
> >>> +         * and @alloc_zone (can be HIGH or FSEG) requirements.
> >>> +         *
> >>> +         * Must appear exactly once for each file, and before
> >>> +         * this file is referenced by any other command.
> >>> +         */
> >>> +        struct {
> >>> +            char alloc_file[BIOS_LINKER_LOADER_FILESZ];
> >>> +            uint32_t alloc_align;
> >>> +            uint8_t alloc_zone;
> >>> +        };
> >>
> >> I would prefer to have named structs, like
> >>
> >>         struct {
> >>             char file[BIOS_LINKER_LOADER_FILESZ];
> >>             uint32_t align;
> >>             uint8_t zone;
> >>         } alloc;
> > 
> > Why do you prefer this? It just makes code more verbose.
> 
> It doesn't, the names were already prefixed with alloc_/pointer_/cksum_.
>  All it does is change underscores to periods.
> 
> Paolo

You are right, that's cleaner. I'll do this.
I don't want to sprinkle QEMU_PACKED around though,
it's just there for documentation purposes, so one
place is enough.

> >>> +        /*
> >>> +         * COMMAND_ADD_POINTER - patch the table (originating from
> >>> +         * @dest_file) at @pointer_offset, by adding a pointer to the 
> >>> table
> >>> +         * originating from @src_file. 1,2,4 or 8 byte unsigned
> >>> +         * addition is used depending on @pointer_size.
> >>> +         */
> >>> +        struct {
> >>> +            char pointer_dest_file[BIOS_LINKER_LOADER_FILESZ];
> >>> +            char pointer_src_file[BIOS_LINKER_LOADER_FILESZ];
> >>> +            uint32_t pointer_offset;
> >>> +            uint8_t pointer_size;
> >>> +        };
> >>> +
> >>> +        /*
> >>> +         * COMMAND_ADD_CHECKSUM - calculate checksum of the range 
> >>> specified by
> >>> +         * @cksum_start and @cksum_length fields,
> >>> +         * and then add the value at @cksum_offset.
> >>> +         * Checksum simply sums -X for each byte X in the range
> >>> +         * using 8-bit math.
> >>> +         */
> >>> +        struct {
> >>> +            char cksum_file[BIOS_LINKER_LOADER_FILESZ];
> >>> +            uint32_t cksum_offset;
> >>> +            uint32_t cksum_start;
> >>> +            uint32_t cksum_length;
> >>> +        };
> >>> +
> >>> +        /* padding */
> >>> +        char pad[124];
> >>> +    };
> >>> +} QEMU_PACKED;
> >>> +typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
> >>> +
> >>> +enum {
> >>> +    BIOS_LINKER_LOADER_COMMAND_ALLOCATE     = 0x1,
> >>> +    BIOS_LINKER_LOADER_COMMAND_ADD_POINTER  = 0x2,
> >>> +    BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3,
> >>> +};
> >>> +
> >>> +enum {
> >>> +    BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH = 0x1,
> >>> +    BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG = 0x2,
> >>> +};
> >>> +
> >>> +GArray *bios_linker_init(void)
> >>> +{
> >>> +    return g_array_new(false, true /* clear */, 
> >>> sizeof(BiosLinkerLoaderEntry));
> >>> +}
> >>> +
> >>> +/* Free linker wrapper and return the linker array. */
> >>> +void *bios_linker_cleanup(GArray *linker)
> >>> +{
> >>> +    return g_array_free(linker, false);
> >>> +}
> >>> +
> >>> +void bios_linker_alloc(GArray *linker,
> >>> +                       const char *file,
> >>> +                       uint32_t alloc_align,
> >>> +                       bool alloc_fseg)
> >>> +{
> >>> +    BiosLinkerLoaderEntry entry;
> >>> +
> >>> +    memset(&entry, 0, sizeof entry);
> >>> +    strncpy(entry.alloc_file, file, sizeof entry.alloc_file - 1);
> >>
> >> Please use pstrcpy (also below).
> > 
> > OK.
> > 
> >>> +    entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ALLOCATE);
> >>> +    entry.alloc_align = cpu_to_le32(alloc_align);
> >>> +    entry.alloc_zone = cpu_to_le32(alloc_fseg ?
> >>> +                                    BIOS_LINKER_LOADER_ALLOC_ZONE_FSEG :
> >>> +                                    BIOS_LINKER_LOADER_ALLOC_ZONE_HIGH);
> >>> +
> >>> +    /* Alloc entries must come first, so prepend them */
> >>> +    g_array_prepend_val(linker, entry);
> >>> +}
> >>> +
> >>> +void bios_linker_add_checksum(GArray *linker, const char *file, void 
> >>> *table,
> >>> +                              void *start, unsigned size, uint8_t 
> >>> *checksum)
> >>> +{
> >>> +    BiosLinkerLoaderEntry entry;
> >>> +
> >>> +    memset(&entry, 0, sizeof entry);
> >>> +    strncpy(entry.cksum_file, file, sizeof entry.cksum_file - 1);
> >>> +    entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ADD_CHECKSUM);
> >>> +    entry.cksum_offset = cpu_to_le32(checksum - (uint8_t *)table);
> >>> +    entry.cksum_start = cpu_to_le32((uint8_t *)start - (uint8_t *)table);
> >>> +    entry.cksum_length = cpu_to_le32(size);
> >>> +
> >>> +    g_array_append_val(linker, entry);
> >>> +}
> >>> +
> >>> +void bios_linker_add_pointer(GArray *linker,
> >>> +                             const char *dest_file,
> >>> +                             const char *src_file,
> >>> +                             GArray *table, void *pointer,
> >>> +                             uint8_t pointer_size)
> >>> +{
> >>> +    BiosLinkerLoaderEntry entry;
> >>> +
> >>> +    memset(&entry, 0, sizeof entry);
> >>> +    strncpy(entry.pointer_dest_file, dest_file,
> >>> +            sizeof entry.pointer_dest_file - 1);
> >>> +    strncpy(entry.pointer_src_file, src_file,
> >>> +            sizeof entry.pointer_src_file - 1);
> >>> +    entry.command = cpu_to_le32(BIOS_LINKER_LOADER_COMMAND_ADD_POINTER);
> >>> +    entry.pointer_offset = cpu_to_le32((gchar *)pointer - table->data);
> >>> +    entry.pointer_size = pointer_size;
> >>> +    assert(pointer_size == 1 || pointer_size == 2 ||
> >>> +           pointer_size == 4 || pointer_size == 8);
> >>> +
> >>> +    g_array_append_val(linker, entry);
> >>> +}
> >>> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> >>> index f950707..b9ca380 100644
> >>> --- a/hw/i386/Makefile.objs
> >>> +++ b/hw/i386/Makefile.objs
> >>> @@ -5,6 +5,7 @@ obj-y += pc_sysfw.o
> >>>  obj-$(CONFIG_XEN) += xen_domainbuild.o xen_machine_pv.o
> >>>  
> >>>  obj-y += kvmvapic.o
> >>> +obj-y += bios-linker-loader.o
> >>>  
> >>>  iasl-option=$(shell if test -z "`$(1) $(2) 2>&1 > /dev/null`" \
> >>>      ; then echo "$(2)"; else echo "$(3)"; fi ;)
> >>>