Re: [Qemu-devel] [SeaBIOS] [PATCH v2 1/5] linker: utility to patch in-memory ROM files

["Michael S. Tsirkin" <mst@redhat.com>]

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.


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