diff mbox

[SeaBIOS,v2,1/5] linker: utility to patch in-memory ROM files

Message ID 20130922111845.GA7951@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Sept. 22, 2013, 11:18 a.m. UTC 
On Sun, Sep 22, 2013 at 01:49:58PM +0300, Michael S. Tsirkin wrote:
> 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


So I tried this out, and I don't like this much:
see below.
Lots of code, a single data pointer in file struct seems much easier.

Further, without a pointer from file to data,
there's no way to find the tables in memory,
so I will have to make the loader interface ACPI-specific,
make it look into tables loaded for the RSDP signature
and return the address of RSDP.

Comments

Michael S. Tsirkin Sept. 22, 2013, 1:44 p.m. UTC | #1 
On Sun, Sep 22, 2013 at 02:18:45PM +0300, Michael S. Tsirkin wrote:
> On Sun, Sep 22, 2013 at 01:49:58PM +0300, Michael S. Tsirkin wrote:
> > 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
> 
> 
> So I tried this out


Latest version that I posted uses the approach suggested by Kevin here.
It adds about 200 bytes to code size.
If we want to cut that out and go back to data pointer,
we can do this as a patch on top.

Pls let me know.
diff mbox

Patch

diff --git a/src/util.h b/src/util.h
index 1e883f2..d777521 100644
--- a/src/util.h
+++ b/src/util.h
@@ -441,7 +441,6 @@  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);
diff --git a/src/acpi.c b/src/acpi.c
index 24cb1fa..c3a3c16 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -611,14 +611,14 @@  acpi_find_rsdp_rom(void)
         if (!file)
             break;
 
-        if (!file->data || !pmm_test_fseg(file->data) ||
+        if (/*!file->data ||*/ !pmm_test_fseg(NULL /*file->data*/) ||
             file->size < sizeof(rsdp->signature))
             continue;
 
         void *data;
 
-        for (data = file->data;
-             data + sizeof(*rsdp) <= file->data + file->size;
+        for (data = NULL /*file->data */;
+             data + sizeof(*rsdp) <= /* file->data */ NULL + file->size;
              data++) {
             rsdp = data;
             if (rsdp->signature == cpu_to_le64(RSDP_SIGNATURE))
diff --git a/src/romfile_loader.c b/src/romfile_loader.c
index 6ba03ed..5e98810 100644
--- a/src/romfile_loader.c
+++ b/src/romfile_loader.c
@@ -2,17 +2,33 @@ 
 #include "byteorder.h" // leXX_to_cpu/cpu_to_leXX
 #include "util.h" // checksum
 
-static struct romfile_s *romfile_loader_find(const char *name)
+struct romfile_loader_file {
+    struct romfile_s *file;
+    void *data;
+};
+struct romfile_loader_files {
+    int nfiles;
+    struct romfile_loader_file files[];
+};
+
+static struct romfile_loader_file *
+romfile_loader_find(const char *name,
+                    struct romfile_loader_files *files)
 {
+    int i;
     if (name[ROMFILE_LOADER_FILESZ - 1])
         return NULL;
-    return romfile_find(name);
+    for (i = 0; i < files->nfiles; ++i)
+        if (!strcmp(files->files[i].file->name, name))
+            return &files->files[i];
+    return NULL;
 }
 
-static void romfile_loader_allocate(struct romfile_loader_entry_s *entry)
+static void romfile_loader_allocate(struct romfile_loader_entry_s *entry,
+                                    struct romfile_loader_files *files)
 {
     struct zone_s *zone;
-    struct romfile_s *file;
+    struct romfile_loader_file *file = &files->files[files->nfiles];
     void *data;
     int ret;
     unsigned alloc_align = le32_to_cpu(entry->alloc_align);
@@ -32,20 +48,21 @@  static void romfile_loader_allocate(struct romfile_loader_entry_s *entry)
     }
     if (alloc_align < MALLOC_MIN_ALIGN)
         alloc_align = MALLOC_MIN_ALIGN;
-    file = romfile_loader_find(entry->alloc_file);
-    if (!file || file->data)
+    if (entry->alloc_file[ROMFILE_LOADER_FILESZ - 1])
         goto err;
-    if (!file->size)
+    file->file = romfile_find(entry->alloc_file);
+    if (!file->file || !file->file->size)
         return;
-    data = pmm_malloc(zone, PMM_DEFAULT_HANDLE, file->size, alloc_align);
+    data = pmm_malloc(zone, PMM_DEFAULT_HANDLE, file->file->size, alloc_align);
     if (!data) {
         warn_noalloc();
         return;
     }
-    ret = file->copy(file, data, file->size);
-    if (ret != file->size)
+    ret = file->file->copy(file->file, data, file->file->size);
+    if (ret != file->file->size)
         goto file_err;
     file->data = data;
+    files->nfiles++;
     return;
 
 file_err:
@@ -54,16 +71,20 @@  err:
     warn_internalerror();
 }
 
-static void romfile_loader_add_pointer(struct romfile_loader_entry_s *entry)
+static void romfile_loader_add_pointer(struct romfile_loader_entry_s *entry,
+                                       struct romfile_loader_files *files)
 {
-    struct romfile_s *dest_file = romfile_loader_find(entry->pointer_dest_file);
-    struct romfile_s *src_file = romfile_loader_find(entry->pointer_src_file);
+    struct romfile_loader_file *dest_file;
+    struct romfile_loader_file *src_file;
     unsigned offset = le32_to_cpu(entry->pointer_offset);
     u64 pointer = 0;
 
+    dest_file = romfile_loader_find(entry->pointer_dest_file, files);
+    src_file = romfile_loader_find(entry->pointer_src_file, files);
+
     if (!dest_file || !src_file || !dest_file->data || !src_file->data ||
         offset + entry->pointer_size < offset ||
-        offset + entry->pointer_size > dest_file->size ||
+        offset + entry->pointer_size > dest_file->file->size ||
         entry->pointer_size < 1 || entry->pointer_size > 8 ||
         entry->pointer_size & (entry->pointer_size - 1))
         goto err;
@@ -79,15 +100,19 @@  err:
     warn_internalerror();
 }
 
-static void romfile_loader_add_checksum(struct romfile_loader_entry_s *entry)
+static void romfile_loader_add_checksum(struct romfile_loader_entry_s *entry,
+                                        struct romfile_loader_files *files)
 {
-    struct romfile_s *file = romfile_loader_find(entry->cksum_file);
+    struct romfile_loader_file *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)
+
+    file = romfile_loader_find(entry->cksum_file, files);
+
+    if (!file || !file->data || offset >= file->file->size ||
+        start + len < start || start + len > file->file->size)
         goto err;
 
     data = file->data + offset;
@@ -101,33 +126,47 @@  err:
 int romfile_loader_execute(const char *name)
 {
     struct romfile_loader_entry_s *entry;
-    int size, offset = 0;
+    int size, offset = 0, nfiles = 0;
+    struct romfile_loader_files *files;
     void *data = romfile_loadfile(name, &size);
     if (!data)
         return -1;
 
+    /* Validate and count files */
     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;
+            return -1;
         }
+
+        if (le32_to_cpu(entry->command) == ROMFILE_LOADER_COMMAND_ALLOCATE) {
+            nfiles++;
+	}
+    }
+
+    files = malloc_tmp(sizeof(*files) + nfiles * sizeof(files->files[0]));
+    files->nfiles = 0;
+
+    for (offset = 0; offset < size; offset += sizeof(*entry)) {
+        entry = data + offset;
 	switch (le32_to_cpu(entry->command)) {
 		case ROMFILE_LOADER_COMMAND_ALLOCATE:
-			romfile_loader_allocate(entry);
+			romfile_loader_allocate(entry, files);
 			break;
 		case ROMFILE_LOADER_COMMAND_ADD_POINTER:
-			romfile_loader_add_pointer(entry);
+			romfile_loader_add_pointer(entry, files);
 			break;
 		case ROMFILE_LOADER_COMMAND_ADD_CHECKSUM:
-			romfile_loader_add_checksum(entry);
+			romfile_loader_add_checksum(entry, files);
 		default:
 			/* Skip commands that we don't recognize. */
 			break;
 	}
     }
 
+    free(files);
     free(data);
     return 0;
 }