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

[Michael S. Tsirkin]

On Fri, Jul 12, 2013 at 04:17:28PM +0200, Laszlo Ersek wrote:
> 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"?

I'm not sure - it doesn't have to match exactly and
I also want to avoid dependency between loader and fw_cfg.h.
If yes we'll need a new header for this macro.

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

RSDP must be in FSEG though

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

This isn't a gcc-ism. It's in C1x:

An unnamed member whose type specifier is a structure specifier with no
tag is called an anonymous structure; an unnamed member whose type
specifier is a union specifier with no tag is called an anonymous union.
The members of an anonymous structure or union are considered to be
members of the containing structure or union. This applies recursively
if the containing structure or union is also anonymous.


> 
> > +};
> > +typedef struct BiosLinkerLoaderEntry BiosLinkerLoaderEntry;
> 
> Probably not needed in practice, but for documentation purposes I
> suggest QEMU_PACKED from "include/qemu/compiler.h".

It's not required in practice but I can add this though I'm not sure -
what does this document?

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

Yes.

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