| Message ID | 1373211589-8097-2-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Jul 07, 2013 at 06:42:35PM +0300, Michael S. Tsirkin wrote: > Add ability for a ROM file to point to > it's image in memory. When file is in memory, > add utility that can patch it, storing > pointers to one file within another file. Thanks. See my comments below. [...] > --- /dev/null > +++ b/src/linker.c [...] > +void linker_loader_execute(const char *name) > +{ > + struct linker_loader_entry_s *entry; > + int size, offset = 0; > + void *data = romfile_loadfile(name, &size); > + if (!data) > + return; > + > + for (offset = 0; offset < size; offset += sizeof *entry) { For consistent style, please treat sizeof like a function (ie, sizeof(*entry) ). > + entry = data + offset; > + /* Check that entry fits in buffer. */ > + if (offset + sizeof *entry > size) { > + warn_internalerror(); > + break; > + } > + switch (le32_to_cpu(entry->command)) { > + case LINKER_LOADER_COMMAND_ALLOCATE: > + linker_loader_allocate(entry); SeaBIOS uses 4 spaces for indentation, and no tabs. [...] > --- a/src/util.h > +++ b/src/util.h > @@ -436,6 +436,7 @@ struct romfile_s { > char name[128]; > u32 size; > int (*copy)(struct romfile_s *file, void *dest, u32 maxlen); > + void *data; > }; I'd prefer to see this tracked within the "linker" code and not in the generic romfile struct. Also, is there another name besides "linker" that could be used? SeaBIOS has code to self-relocate and fixup code relocations. I think having code in the repo called "linker" could cause confusion. -Kevin
On Sun, Jul 14, 2013 at 02:24:52PM -0400, Kevin O'Connor wrote: > On Sun, Jul 07, 2013 at 06:42:35PM +0300, Michael S. Tsirkin wrote: > > Add ability for a ROM file to point to > > it's image in memory. When file is in memory, > > add utility that can patch it, storing > > pointers to one file within another file. > > Thanks. See my comments below. > > [...] > > --- /dev/null > > +++ b/src/linker.c > [...] > > +void linker_loader_execute(const char *name) > > +{ > > + struct linker_loader_entry_s *entry; > > + int size, offset = 0; > > + void *data = romfile_loadfile(name, &size); > > + if (!data) > > + return; > > + > > + for (offset = 0; offset < size; offset += sizeof *entry) { > > For consistent style, please treat sizeof like a function (ie, > sizeof(*entry) ). > > > + entry = data + offset; > > + /* Check that entry fits in buffer. */ > > + if (offset + sizeof *entry > size) { > > + warn_internalerror(); > > + break; > > + } > > + switch (le32_to_cpu(entry->command)) { > > + case LINKER_LOADER_COMMAND_ALLOCATE: > > + linker_loader_allocate(entry); > > SeaBIOS uses 4 spaces for indentation, and no tabs. > > [...] > > --- a/src/util.h > > +++ b/src/util.h > > @@ -436,6 +436,7 @@ struct romfile_s { > > char name[128]; > > u32 size; > > int (*copy)(struct romfile_s *file, void *dest, u32 maxlen); > > + void *data; > > }; > > I'd prefer to see this tracked within the "linker" code and not in the > generic romfile struct. A way to associate a romfile instance with a value seems generally useful, no? Still, that's not too hard - it would only mean an extra linked list of struct linker { char name[56] void *data; struct hlist_node node; } is this preferable? > Also, is there another name besides "linker" that could be used? > SeaBIOS has code to self-relocate and fixup code relocations. I think > having code in the repo called "linker" could cause confusion. > > -Kevin romfile_loader?
On Mon, Jul 15, 2013 at 11:01:02AM +0300, Michael S. Tsirkin wrote: > On Sun, Jul 14, 2013 at 02:24:52PM -0400, Kevin O'Connor wrote: > > On Sun, Jul 07, 2013 at 06:42:35PM +0300, Michael S. Tsirkin wrote: > > > Add ability for a ROM file to point to > > > it's image in memory. When file is in memory, > > > add utility that can patch it, storing > > > pointers to one file within another file. > > > > Thanks. See my comments below. > > > > [...] > > > --- /dev/null > > > +++ b/src/linker.c > > [...] > > > +void linker_loader_execute(const char *name) > > > +{ > > > + struct linker_loader_entry_s *entry; > > > + int size, offset = 0; > > > + void *data = romfile_loadfile(name, &size); > > > + if (!data) > > > + return; > > > + > > > + for (offset = 0; offset < size; offset += sizeof *entry) { > > > > For consistent style, please treat sizeof like a function (ie, > > sizeof(*entry) ). > > > > > + entry = data + offset; > > > + /* Check that entry fits in buffer. */ > > > + if (offset + sizeof *entry > size) { > > > + warn_internalerror(); > > > + break; > > > + } > > > + switch (le32_to_cpu(entry->command)) { > > > + case LINKER_LOADER_COMMAND_ALLOCATE: > > > + linker_loader_allocate(entry); > > > > SeaBIOS uses 4 spaces for indentation, and no tabs. > > > > [...] > > > --- a/src/util.h > > > +++ b/src/util.h > > > @@ -436,6 +436,7 @@ struct romfile_s { > > > char name[128]; > > > u32 size; > > > int (*copy)(struct romfile_s *file, void *dest, u32 maxlen); > > > + void *data; > > > }; > > > > I'd prefer to see this tracked within the "linker" code and not in the > > generic romfile struct. > > A way to associate a romfile instance with a value seems generally > useful, no? Still, that's not too hard - it would only mean an extra > linked list of > > struct linker { > char name[56] > void *data; > struct hlist_node node; > } > > is this preferable? > > > Also, is there another name besides "linker" that could be used? > > SeaBIOS has code to self-relocate and fixup code relocations. I think > > having code in the repo called "linker" could cause confusion. > > > > -Kevin > > romfile_loader? Could you respond on above please? > -- > MST
On Thu, Jul 25, 2013 at 03:55:56PM +0300, Michael S. Tsirkin wrote: > On Mon, Jul 15, 2013 at 11:01:02AM +0300, Michael S. Tsirkin wrote: > > On Sun, Jul 14, 2013 at 02:24:52PM -0400, Kevin O'Connor wrote: > > > I'd prefer to see this tracked within the "linker" code and not in the > > > generic romfile struct. > > > > A way to associate a romfile instance with a value seems generally > > useful, no? Still, that's not too hard - it would only mean an extra > > linked list of > > > > struct linker { > > char name[56] > > void *data; > > struct hlist_node node; > > } > > > > is this preferable? Sure, but it's probably easier to do something like: struct linkfiles { char *name; void *data; }; void linker_loader_execute(const char *name) { int size; struct linker_loader_entry_s *entries = romfile_loadfile(name, &size); int numentries = size/sizeof(entries[0]); if (! entries) return; struct linkfiles *files = malloc_tmp(sizeof(files[0]) * numentries); and then just populate and use the array of filenames. > > > Also, is there another name besides "linker" that could be used? > > > SeaBIOS has code to self-relocate and fixup code relocations. I think > > > having code in the repo called "linker" could cause confusion. > > > > > > > romfile_loader? Shrug. How about "tabledeploy"? -Kevin
On Thu, Jul 25, 2013 at 08:06:27PM -0400, Kevin O'Connor wrote: > On Thu, Jul 25, 2013 at 03:55:56PM +0300, Michael S. Tsirkin wrote: > > On Mon, Jul 15, 2013 at 11:01:02AM +0300, Michael S. Tsirkin wrote: > > > On Sun, Jul 14, 2013 at 02:24:52PM -0400, Kevin O'Connor wrote: > > > > I'd prefer to see this tracked within the "linker" code and not in the > > > > generic romfile struct. > > > > > > A way to associate a romfile instance with a value seems generally > > > useful, no? Still, that's not too hard - it would only mean an extra > > > linked list of > > > > > > struct linker { > > > char name[56] > > > void *data; > > > struct hlist_node node; > > > } > > > > > > is this preferable? > > Sure, but it's probably easier to do something like: > > struct linkfiles { char *name; void *data; }; > > void linker_loader_execute(const char *name) > { > int size; > struct linker_loader_entry_s *entries = romfile_loadfile(name, &size); > int numentries = size/sizeof(entries[0]); > if (! entries) > return; > struct linkfiles *files = malloc_tmp(sizeof(files[0]) * numentries); > > and then just populate and use the array of filenames. OK I'll do this but it's more code as I can't use plain romfile_find anymore, and have to code up my own lookup. > > > > Also, is there another name besides "linker" that could be used? > > > > SeaBIOS has code to self-relocate and fixup code relocations. I think > > > > having code in the repo called "linker" could cause confusion. > > > > > > > > > > romfile_loader? > > Shrug. How about "tabledeploy"? > > -Kevin
diff --git a/Makefile b/Makefile index 759bbbb..020fb2f 100644 --- a/Makefile +++ b/Makefile @@ -18,7 +18,7 @@ SRC16=$(SRCBOTH) system.c disk.c font.c SRC32FLAT=$(SRCBOTH) post.c shadow.c memmap.c pmm.c coreboot.c boot.c \ acpi.c smm.c mptable.c pirtable.c smbios.c pciinit.c optionroms.c mtrr.c \ lzmadecode.c bootsplash.c jpeg.c usb-hub.c paravirt.c \ - biostables.c xen.c bmp.c romfile.c csm.c + biostables.c xen.c bmp.c romfile.c csm.c linker.c SRC32SEG=util.c output.c pci.c pcibios.c apm.c stacks.c # Default compiler flags diff --git a/src/linker.c b/src/linker.c new file mode 100644 index 0000000..3caa97b --- /dev/null +++ b/src/linker.c @@ -0,0 +1,132 @@ +#include "linker.h" +#include "byteorder.h" // leXX_to_cpu/cpu_to_leXX +#include "util.h" // checksum + +static struct romfile_s *linker_loader_find(const char *name) +{ + if (name[LINKER_LOADER_FILESZ - 1]) + return NULL; + return romfile_find(name); +} + +static void linker_loader_allocate(struct linker_loader_entry_s *entry) +{ + struct zone_s *zone; + struct romfile_s *file; + void *data; + int ret; + unsigned alloc_align = le32_to_cpu(entry->alloc_align); + + if (alloc_align & (alloc_align - 1)) + goto err; + + switch (entry->alloc_zone) { + case LINKER_LOADER_ALLOC_ZONE_HIGH: + zone = &ZoneHigh; + break; + case LINKER_LOADER_ALLOC_ZONE_FSEG: + zone = &ZoneFSeg; + break; + default: + goto err; + } + if (alloc_align < MALLOC_MIN_ALIGN) + alloc_align = MALLOC_MIN_ALIGN; + file = linker_loader_find(entry->alloc_file); + if (!file || file->data) + goto err; + if (!file->size) + return; + data = pmm_malloc(zone, PMM_DEFAULT_HANDLE, file->size, alloc_align); + if (!data) { + warn_noalloc(); + return; + } + ret = file->copy(file, data, file->size); + if (ret != file->size) + goto file_err; + file->data = data; + return; + +file_err: + free(data); +err: + warn_internalerror(); +} + +static void linker_loader_add_pointer(struct linker_loader_entry_s *entry) +{ + struct romfile_s *dest_file = linker_loader_find(entry->pointer_dest_file); + struct romfile_s *src_file = linker_loader_find(entry->pointer_src_file); + unsigned offset = le32_to_cpu(entry->pointer_offset); + u64 pointer = 0; + + if (!dest_file || !src_file || !dest_file->data || !src_file->data || + offset + entry->pointer_size < offset || + offset + entry->pointer_size > dest_file->size || + entry->pointer_size < 1 || entry->pointer_size > 8 || + entry->pointer_size & (entry->pointer_size - 1)) + goto err; + + memcpy(&pointer, dest_file->data + offset, entry->pointer_size); + pointer = le64_to_cpu(pointer); + pointer += (unsigned long)src_file->data; + pointer = cpu_to_le64(pointer); + memcpy(dest_file->data + offset, &pointer, entry->pointer_size); + + return; +err: + warn_internalerror(); +} + +static void linker_loader_add_checksum(struct linker_loader_entry_s *entry) +{ + struct romfile_s *file = linker_loader_find(entry->cksum_file); + unsigned offset = le32_to_cpu(entry->cksum_offset); + unsigned start = le32_to_cpu(entry->cksum_start); + unsigned len = le32_to_cpu(entry->cksum_length); + u8 *data; + if (!file || !file->data || offset >= file->size || + start + len < start || start + len > file->size) + goto err; + + data = file->data + offset; + *data -= checksum(file->data + start, len); + + return; +err: + warn_internalerror(); +} + +void linker_loader_execute(const char *name) +{ + struct linker_loader_entry_s *entry; + int size, offset = 0; + void *data = romfile_loadfile(name, &size); + if (!data) + return; + + for (offset = 0; offset < size; offset += sizeof *entry) { + entry = data + offset; + /* Check that entry fits in buffer. */ + if (offset + sizeof *entry > size) { + warn_internalerror(); + break; + } + switch (le32_to_cpu(entry->command)) { + case LINKER_LOADER_COMMAND_ALLOCATE: + linker_loader_allocate(entry); + break; + case LINKER_LOADER_COMMAND_ADD_POINTER: + linker_loader_add_pointer(entry); + break; + case LINKER_LOADER_COMMAND_ADD_CHECKSUM: + linker_loader_add_checksum(entry); + default: + /* Skip commands that we don't recognize. */ + break; + } + } + + free(data); +} diff --git a/src/linker.h b/src/linker.h new file mode 100644 index 0000000..0c374e5 --- /dev/null +++ b/src/linker.h @@ -0,0 +1,72 @@ +#ifndef __LINKER_H +#define __LINKER_H + +#include "types.h" // u8 +#include "util.h" // romfile_s + +#define LINKER_LOADER_FILESZ 56 + +/* ROM file linker/loader interface. Linker uses little endian format */ +struct linker_loader_entry_s { + u32 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[LINKER_LOADER_FILESZ]; + u32 alloc_align; + u8 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[LINKER_LOADER_FILESZ]; + char pointer_src_file[LINKER_LOADER_FILESZ]; + u32 pointer_offset; + u8 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[LINKER_LOADER_FILESZ]; + u32 cksum_offset; + u32 cksum_start; + u32 cksum_length; + }; + + /* padding */ + char pad[124]; + }; +}; + +enum { + LINKER_LOADER_COMMAND_ALLOCATE = 0x1, + LINKER_LOADER_COMMAND_ADD_POINTER = 0x2, + LINKER_LOADER_COMMAND_ADD_CHECKSUM = 0x3, +}; + +enum { + LINKER_LOADER_ALLOC_ZONE_HIGH = 0x1, + LINKER_LOADER_ALLOC_ZONE_FSEG = 0x2, +}; + +void linker_loader_execute(const char *name); + +#endif diff --git a/src/paravirt.c b/src/paravirt.c index d1a5d3e..9dd229d 100644 --- a/src/paravirt.c +++ b/src/paravirt.c @@ -327,6 +327,8 @@ void qemu_cfg_init(void) for (e = 0; e < count; e++) { struct QemuCfgFile qfile; qemu_cfg_read(&qfile, sizeof(qfile)); + if (!*qfile.name) + return; qemu_romfile_add(qfile.name, be16_to_cpu(qfile.select) , 0, be32_to_cpu(qfile.size)); } diff --git a/src/util.h b/src/util.h index 996c29a..7b50c38 100644 --- a/src/util.h +++ b/src/util.h @@ -436,6 +436,7 @@ struct romfile_s { char name[128]; u32 size; int (*copy)(struct romfile_s *file, void *dest, u32 maxlen); + void *data; }; void romfile_add(struct romfile_s *file); struct romfile_s *romfile_findprefix(const char *prefix, struct romfile_s *prev);
Add ability for a ROM file to point to it's image in memory. When file is in memory, add utility that can patch it, storing pointers to one file within another file. This is not a lot of code: together with the follow-up patch to load ACPI tables from ROM, we get: Before: Total size: 127880 Fixed: 59060 Free: 3192 (used 97.6% of 128KiB rom) After: Total size: 128776 Fixed: 59100 Free: 2296 (used 98.2% of 128KiB rom) Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Makefile | 2 +- src/linker.c | 132 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/linker.h | 72 +++++++++++++++++++++++++++++++ src/paravirt.c | 2 + src/util.h | 1 + 5 files changed, 208 insertions(+), 1 deletion(-) create mode 100644 src/linker.c create mode 100644 src/linker.h