[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [SeaBIOS] [PATCH v2 1/5] linker: utility to patch in-me
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [SeaBIOS] [PATCH v2 1/5] linker: utility to patch in-memory ROM files |
Date: |
Sun, 22 Sep 2013 14:18:45 +0300 |
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;
}