Message ID | 1379857006-17451-16-git-send-email-mst@redhat.com |
---|---|
State | New |
Headers | show |
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 <mst@redhat.com> > --- > 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 <mst@redhat.com> > + * > + * 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 ;) >
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 <mst@redhat.com> > > --- > > 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 <mst@redhat.com> > > + * > > + * 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 ;) > >
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 <mst@redhat.com> >>> --- >>> 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. 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 >>> + /* >>> + * 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 ;) >>>
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 <mst@redhat.com> > >>> --- > >>> 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 ;) > >>>
Il 23/09/2013 15:47, Michael S. Tsirkin ha scritto: > > It doesn't, the names were already prefixed with alloc_/pointer_/cksum_. > > All it does is change underscores to periods. > > 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. Fair enough. Thanks! Paolo
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 <mst@redhat.com> + * + * 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; + }; + + /* + * 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); + 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 ;)
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 <mst@redhat.com> --- include/hw/i386/bios-linker-loader.h | 26 ++++++ hw/i386/bios-linker-loader.c | 156 +++++++++++++++++++++++++++++++++++ hw/i386/Makefile.objs | 1 + 3 files changed, 183 insertions(+) create mode 100644 include/hw/i386/bios-linker-loader.h create mode 100644 hw/i386/bios-linker-loader.c