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

[Paolo Bonzini]

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".

Also, the API is (understandably) bios_linker_*, not
bios_linker_loader_*, with the exception of BiosLinkerLoaderEntry,
BIOS_LINKER_LOADER_FILESZ and perhaps a few others I've missed.  I would
just drop the loader part, and if you go for the BIOS -> ACPI rename,
make the filenames include/hw/acpi/linker.h and hw/acpi/linker.c.

Just post v5 of this patch in any case, I don't need to see the matching
changes in patch 23.

Paolo

>  3 files changed, 183 insertions(+)
>  create mode 100644 include/hw/i386/bios-linker-loader.h
>  create mode 100644 hw/i386/bios-linker-loader.c
> 
> diff --git a/include/hw/i386/bios-linker-loader.h 
> b/include/hw/i386/bios-linker-loader.h
> new file mode 100644
> index 0000000..18c3868
> --- /dev/null
> +++ b/include/hw/i386/bios-linker-loader.h
> @@ -0,0 +1,26 @@
> +#ifndef BIOS_LINKER_LOADER_H
> +#define BIOS_LINKER_LOADER_H
> +
> +#include <glib.h>
> +#include <stdbool.h>
> +#include <inttypes.h>
> +
> +GArray *bios_linker_init(void);
> +
> +void bios_linker_alloc(GArray *linker,
> +                       const char *file,
> +                       uint32_t alloc_align,
> +                       bool alloc_fseg);
> +
> +void bios_linker_add_checksum(GArray *linker, const char *file, void *table,
> +                              void *start, unsigned size, uint8_t *checksum);
> +
> +
> +void bios_linker_add_pointer(GArray *linker,
> +                             const char *dest_file,
> +                             const char *src_file,
> +                             GArray *table, void *pointer,
> +                             uint8_t pointer_size);
> +
> +void *bios_linker_cleanup(GArray *linker);
> +#endif
> diff --git a/hw/i386/bios-linker-loader.c b/hw/i386/bios-linker-loader.c
> new file mode 100644
> index 0000000..644016b
> --- /dev/null
> +++ b/hw/i386/bios-linker-loader.c
> @@ -0,0 +1,156 @@
> +/* Dynamic linker/loader of ACPI tables
> + *
> + * Copyright (C) 2013 Red Hat Inc
> + *
> + * Author: Michael S. Tsirkin <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/i386/bios-linker-loader.h"
> +#include "hw/nvram/fw_cfg.h"
> +
> +#include <string.h>
> +#include <assert.h>
> +#include "qemu/bswap.h"
> +
> +#define BIOS_LINKER_LOADER_FILESZ FW_CFG_MAX_FILE_PATH
> +
> +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;

Also perhaps add QEMU_PACKED here too, just in case someone makes
typedefs out of the structs and forgets it.

> +        /*
> +         * 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).

> +    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 ;)
>