[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 15/23] i386: add bios linker/loader
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH v4 15/23] i386: add bios linker/loader |
Date: |
Mon, 23 Sep 2013 16:36:36 +0300 |
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?
> 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
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.
> > 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;
Why do you prefer this? It just makes code more verbose.
> Also perhaps add QEMU_PACKED here too, just in case someone makes
> typedefs out of the structs and forgets it.
I don't see why.
Even the QEMU_PACKED at the end is for documentation purposes only:
there's no real need for it in any of the structs here.
So let's document it all in one place.
> > + /*
> > + * 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 ;)
> >
[Qemu-devel] [PATCH v4 13/23] acpi: pre-compiled ASL files, Michael S. Tsirkin, 2013/09/22
[Qemu-devel] [PATCH v4 16/23] loader: allow adding ROMs in done callbacks, Michael S. Tsirkin, 2013/09/22
[Qemu-devel] [PATCH v4 17/23] i386: define pc guest info, Michael S. Tsirkin, 2013/09/22
[Qemu-devel] [PATCH v4 18/23] acpi/piix: add macros for acpi property names, Michael S. Tsirkin, 2013/09/22
[Qemu-devel] [PATCH v4 19/23] piix: APIs for pc guest info, Michael S. Tsirkin, 2013/09/22