Re: [Qemu-devel] [PATCH v2 repost 5/9] i386: add bios linker/loader

[Laszlo Ersek]

On 07/10/13 15:51, Michael S. Tsirkin wrote:
> This add a dynamic bios linker/loader.

s/add/adds/

> This will be used by acpi table generation
> code to:
>     - load each table in the appropriate memory egment

s/egment/segment/


> diff --git a/hw/i386/bios-linker-loader.c b/hw/i386/bios-linker-loader.c
> new file mode 100644
> index 0000000..b2c87d7
> --- /dev/null
> +++ b/hw/i386/bios-linker-loader.c
> @@ -0,0 +1,155 @@
> +/* 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 <string.h>
> +#include <assert.h>
> +#include "qemu/bswap.h"
> +
> +#define BIOS_LINKER_LOADER_FILESZ 56

Perhaps share a macro between this file, "hw/core/loader.c", and
"include/hw/nvram/fw_cfg.h"?

> +
> +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 think in OVMF we won't rely on the alloc_zone / alloc_align members,
but that's OVMF's private business.

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

What do you mean by addition? Hm... I vaguely remember something from
our earlier discussion. I'll probably understand again when looking at
the next patches.

> +         */
> +        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;
> +        };

I wonder if we can implement this full flexibility in OVMF. The default
edk2 ACPI table installation protocol special-cases the tables that
commonly need pointers, and automatically links them together (and
re-checksums them) during table-wise installation. So for those cases
this linker command is a no-op, which is good. Arbitrary cross-linking
may not be possible in OVMF though.

> +
> +        /*
> +         * 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;
> +        };

IIRC in OVMF part of this is automatic again, but in any case this
command doesn't seem impossible to implement.

> +
> +        /* padding */
> +        char pad[124];
> +    };

The unnamed union member is a gcc-ism. I'd give it a short name (like
"u"), but feel free to ignore this.


> +};
> +typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;

Probably not needed in practice, but for documentation purposes I
suggest QEMU_PACKED from "include/qemu/compiler.h".

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

Yes, the fw_cfg filenames appear NUL-terminated.

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

So "table" is actually "dest_table" (the table to patch), and "pointer"
points to the pointer in it to patch.


Qemu, seabios and OVMF must all agree upon this interface. Since you're
moving tables from seabios to qemu, that part of the agreement is a
given. Without actually trying to consume these tables in OVMF I don't
know now if I'll run into any problems, but it doesn't immediately look
threatening.

I think you might want to fix the commit message typos and add the
QEMU_PACKED macro, so postponing my R-b until your answer.

Thanks
Laszlo