[Qemu-devel] [PATCH v2 0/5] seabios: load acpi tables from qemu

[Qemu-devel] [PATCH v2 0/5] seabios: load acpi tables from qemu

[Michael S. Tsirkin]

This is the seabios code that adds support for loading
acpi tables from QEMU: it's working fine here and
in fact, I am able to get exactly same tables with builtin
tables and with tables coming from qemu.

Changes from v1:
	- simplified linker interfaces along the lines suggested
	  by Kevin
	- fixed lots of bugs

Michael S. Tsirkin (5):
  linker: utility to patch in-memory ROM files
  pmm: add a way to test whether memory is in FSEG
  acpi: pack rsdp
  acpi: load and link tables from /etc/acpi/
  acpi: add an option to disable builtin tables

 Makefile       |   2 +-
 src/Kconfig    |  12 +++++-
 src/acpi.c     |  39 +++++++++++++++++
 src/acpi.h     |   2 +-
 src/linker.c   | 132 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/linker.h   |  72 +++++++++++++++++++++++++++++++
 src/paravirt.c |   2 +
 src/pmm.c      |  24 ++++++++---
 src/util.h     |   2 +
 9 files changed, 278 insertions(+), 9 deletions(-)
 create mode 100644 src/linker.c
 create mode 100644 src/linker.h

-- 
MST

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

[Michael S. Tsirkin]

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

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);
-- 
MST

[Qemu-devel] [PATCH v2 2/5] pmm: add a way to test whether memory is in FSEG

[Michael S. Tsirkin]

Will be handy for looking for RSDP.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 src/pmm.c  | 24 ++++++++++++++++++------
 src/util.h |  1 +
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/src/pmm.c b/src/pmm.c
index 8f993fd..8bd8983 100644
--- a/src/pmm.c
+++ b/src/pmm.c
@@ -120,15 +120,23 @@ addSpace(struct zone_s *zone, void *start, void *end)
 
 // Search all zones for an allocation obtained from allocSpace()
 static struct allocinfo_s *
+findAllocInZone(struct zone_s *zone, void *data)
+{
+    struct allocinfo_s *info;
+    hlist_for_each_entry(info, &zone->head, node)
+        if (info->data == data)
+            return info;
+    return NULL;
+}
+
+static struct allocinfo_s *
 findAlloc(void *data)
 {
     int i;
-    for (i=0; i<ARRAY_SIZE(Zones); i++) {
-        struct allocinfo_s *info;
-        hlist_for_each_entry(info, &Zones[i]->head, node) {
-            if (info->data == data)
-                return info;
-        }
+    for (i = 0; i < ARRAY_SIZE(Zones); i++) {
+        struct allocinfo_s *info = findAllocInZone(Zones[i], data);
+        if (info)
+            return info;
     }
     return NULL;
 }
@@ -241,6 +249,10 @@ pmm_find(u32 handle)
     return NULL;
 }
 
+int pmm_test_fseg(void *data)
+{
+    return !!findAllocInZone(&ZoneFSeg, data);
+}
 
 /****************************************************************
  * 0xc0000-0xf0000 management
diff --git a/src/util.h b/src/util.h
index 7b50c38..44e1c1a 100644
--- a/src/util.h
+++ b/src/util.h
@@ -378,6 +378,7 @@ void malloc_init(void);
 void malloc_prepboot(void);
 void *pmm_malloc(struct zone_s *zone, u32 handle, u32 size, u32 align);
 int pmm_free(void *data);
+int pmm_test_fseg(void *data);
 void pmm_init(void);
 void pmm_prepboot(void);
 #define PMM_DEFAULT_HANDLE 0xFFFFFFFF
-- 
MST

[Qemu-devel] [PATCH v2 3/5] acpi: pack rsdp

[Michael S. Tsirkin]

rsdp might not be aligned, so mark it packed.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 src/acpi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/acpi.h b/src/acpi.h
index f872b39..eab982a 100644
--- a/src/acpi.h
+++ b/src/acpi.h
@@ -47,7 +47,7 @@ struct rsdp_descriptor {        /* Root System Descriptor Pointer */
     u64 xsdt_physical_address;  /* 64-bit physical address of XSDT */
     u8  extended_checksum;      /* Checksum of entire table */
     u8  reserved [3];           /* Reserved field must be 0 */
-};
+} PACKED;
 
 extern struct rsdp_descriptor *RsdpAddr;
 
-- 
MST

[Qemu-devel] [PATCH v2 4/5] acpi: load and link tables from /etc/acpi/

[Michael S. Tsirkin]

Load files in /etc/acpi/, link them using
a linker script and use for acpi tables, including the RSDP.
Presense of RSDP in this directory completely disables
generating and loading legacy acpi tables.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 src/acpi.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/src/acpi.c b/src/acpi.c
index a81f0cb..07d311a 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -27,6 +27,7 @@
 #include "config.h" // CONFIG_*
 #include "paravirt.h" // RamSize
 #include "dev-q35.h"
+#include "linker.h"
 
 #include "acpi-dsdt.hex"
 
@@ -599,6 +600,34 @@ static const struct pci_device_id acpi_find_tbl[] = {
 
 struct rsdp_descriptor *RsdpAddr;
 
+/* Look for RSDP signature in files loaded in FSEG */
+struct rsdp_descriptor *
+acpi_find_rsdp_rom(void)
+{
+    struct romfile_s *file = NULL;
+    struct rsdp_descriptor *rsdp = NULL;
+    for (;;) {
+        file = romfile_findprefix("", file);
+        if (!file)
+            break;
+
+        if (!file->data || !pmm_test_fseg(file->data) ||
+            file->size < sizeof(rsdp->signature))
+            continue;
+
+        void *data;
+
+        for (data = file->data;
+             data + sizeof(*rsdp) <= file->data + file->size;
+             data++) {
+            rsdp = data;
+            if (rsdp->signature == cpu_to_le64(RSDP_SIGNATURE))
+                return rsdp;
+        }
+    }
+    return NULL;
+}
+
 #define MAX_ACPI_TABLES 20
 void
 acpi_setup(void)
@@ -608,6 +637,16 @@ acpi_setup(void)
 
     dprintf(3, "init ACPI tables\n");
 
+    linker_loader_execute("/etc/linker-script");
+
+    RsdpAddr = acpi_find_rsdp_rom();
+
+    if (RsdpAddr) {
+        return;
+    }
+
+    dprintf(3, "generate ACPI tables\n");
+
     // This code is hardcoded for PIIX4 Power Management device.
     struct pci_device *pci = pci_find_init_device(acpi_find_tbl, NULL);
     if (!pci)
-- 
MST

[Qemu-devel] [PATCH v2 5/5] acpi: add an option to disable builtin tables

[Michael S. Tsirkin]

Serves to save a bit of memory, and is helpful
for debugging (making sure tables come from qemu).

Memory stats:
Enabled:
Total size: 128776  Fixed: 59100  Free: 2296 (used 98.2% of 128KiB rom)
Disabled:
Total size: 119836  Fixed: 58996  Free: 11236 (used 91.4% of 128KiB rom)

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 src/Kconfig | 12 +++++++++++-
 src/acpi.c  |  2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/src/Kconfig b/src/Kconfig
index 5882d11..ff4c9d1 100644
--- a/src/Kconfig
+++ b/src/Kconfig
@@ -387,10 +387,20 @@ menu "BIOS Tables"
         default y
         help
             Support generation of ACPI tables.
+    config ACPI_BUILTIN
+        bool "Include built-in ACPI tables"
+        default y
+        depends on ACPI
+        help
+            Include built-in ACPI tables in BIOS.
+            Required for QEMU 1.5 and older.
+            This option can be disabled for QEMU 1.6 and newer
+            to save some space in the ROM file.
+            If unsure, say Y.
     config ACPI_DSDT
         bool "Include default ACPI DSDT"
         default y
-        depends on ACPI
+        depends on ACPI && ACPI_BUILTIN
         help
             Include default DSDT ACPI table in BIOS.
             Required for QEMU 1.3 and older.
diff --git a/src/acpi.c b/src/acpi.c
index 07d311a..2c51fd1 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -641,7 +641,7 @@ acpi_setup(void)
 
     RsdpAddr = acpi_find_rsdp_rom();
 
-    if (RsdpAddr) {
+    if (!CONFIG_ACPI_BUILTIN || RsdpAddr) {
         return;
     }
 
-- 
MST

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

[Kevin O'Connor]

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

Re: [Qemu-devel] [SeaBIOS] [PATCH v2 4/5] acpi: load and link tables from /etc/acpi/

[Kevin O'Connor]

On Sun, Jul 07, 2013 at 06:42:43PM +0300, Michael S. Tsirkin wrote:
> Load files in /etc/acpi/, link them using
> a linker script and use for acpi tables, including the RSDP.
> Presense of RSDP in this directory completely disables
> generating and loading legacy acpi tables.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  src/acpi.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/src/acpi.c b/src/acpi.c
> index a81f0cb..07d311a 100644
> --- a/src/acpi.c
> +++ b/src/acpi.c
> @@ -27,6 +27,7 @@
>  #include "config.h" // CONFIG_*
>  #include "paravirt.h" // RamSize
>  #include "dev-q35.h"
> +#include "linker.h"
>  
>  #include "acpi-dsdt.hex"
>  
> @@ -599,6 +600,34 @@ static const struct pci_device_id acpi_find_tbl[] = {
>  
>  struct rsdp_descriptor *RsdpAddr;
>  
> +/* Look for RSDP signature in files loaded in FSEG */
> +struct rsdp_descriptor *
> +acpi_find_rsdp_rom(void)
> +{
> +    struct romfile_s *file = NULL;
> +    struct rsdp_descriptor *rsdp = NULL;
> +    for (;;) {
> +        file = romfile_findprefix("", file);
> +        if (!file)
> +            break;
> +
> +        if (!file->data || !pmm_test_fseg(file->data) ||
> +            file->size < sizeof(rsdp->signature))
> +            continue;
> +
> +        void *data;
> +
> +        for (data = file->data;
> +             data + sizeof(*rsdp) <= file->data + file->size;
> +             data++) {
> +            rsdp = data;
> +            if (rsdp->signature == cpu_to_le64(RSDP_SIGNATURE))
> +                return rsdp;
> +        }
> +    }
> +    return NULL;
> +}
> +
>  #define MAX_ACPI_TABLES 20
>  void
>  acpi_setup(void)
> @@ -608,6 +637,16 @@ acpi_setup(void)
>  
>      dprintf(3, "init ACPI tables\n");
>  
> +    linker_loader_execute("/etc/linker-script");
> +
> +    RsdpAddr = acpi_find_rsdp_rom();
> +
> +    if (RsdpAddr) {
> +        return;
> +    }

I don't understand this.  Why not use the presence of
"/etc/linker-script" to determine if ACPI should be produced.

Also, /etc/linker-script is not very informative - how about something
like "/etc/biostables".

-Kevin

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

[Michael S. Tsirkin]

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?

-- 
MST

Re: [Qemu-devel] [SeaBIOS] [PATCH v2 4/5] acpi: load and link tables from /etc/acpi/

[Michael S. Tsirkin]

On Sun, Jul 14, 2013 at 02:27:14PM -0400, Kevin O'Connor wrote:
> On Sun, Jul 07, 2013 at 06:42:43PM +0300, Michael S. Tsirkin wrote:
> > Load files in /etc/acpi/, link them using
> > a linker script and use for acpi tables, including the RSDP.
> > Presense of RSDP in this directory completely disables
> > generating and loading legacy acpi tables.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  src/acpi.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/src/acpi.c b/src/acpi.c
> > index a81f0cb..07d311a 100644
> > --- a/src/acpi.c
> > +++ b/src/acpi.c
> > @@ -27,6 +27,7 @@
> >  #include "config.h" // CONFIG_*
> >  #include "paravirt.h" // RamSize
> >  #include "dev-q35.h"
> > +#include "linker.h"
> >  
> >  #include "acpi-dsdt.hex"
> >  
> > @@ -599,6 +600,34 @@ static const struct pci_device_id acpi_find_tbl[] = {
> >  
> >  struct rsdp_descriptor *RsdpAddr;
> >  
> > +/* Look for RSDP signature in files loaded in FSEG */
> > +struct rsdp_descriptor *
> > +acpi_find_rsdp_rom(void)
> > +{
> > +    struct romfile_s *file = NULL;
> > +    struct rsdp_descriptor *rsdp = NULL;
> > +    for (;;) {
> > +        file = romfile_findprefix("", file);
> > +        if (!file)
> > +            break;
> > +
> > +        if (!file->data || !pmm_test_fseg(file->data) ||
> > +            file->size < sizeof(rsdp->signature))
> > +            continue;
> > +
> > +        void *data;
> > +
> > +        for (data = file->data;
> > +             data + sizeof(*rsdp) <= file->data + file->size;
> > +             data++) {
> > +            rsdp = data;
> > +            if (rsdp->signature == cpu_to_le64(RSDP_SIGNATURE))
> > +                return rsdp;
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> >  #define MAX_ACPI_TABLES 20
> >  void
> >  acpi_setup(void)
> > @@ -608,6 +637,16 @@ acpi_setup(void)
> >  
> >      dprintf(3, "init ACPI tables\n");
> >  
> > +    linker_loader_execute("/etc/linker-script");
> > +
> > +    RsdpAddr = acpi_find_rsdp_rom();
> > +
> > +    if (RsdpAddr) {
> > +        return;
> > +    }
> 
> I don't understand this.  Why not use the presence of
> "/etc/linker-script" to determine if ACPI should be produced.

We can do this but what do we do if linker script is there
but no rsdp?
Maybe just add
	test -e /etc/linker-script != RsdpAddr
	        dprintf(1, "linker script found but no rsdp. Falling back on builtin tables\n");

Would this address your comment?


> Also, /etc/linker-script is not very informative - how about something
> like "/etc/biostables".
> 
> -Kevin

Hmm bios seems redundant, and tables makes one think it's the tables
themselves.

"/etc/acpi-linker"
"/etc/acpi-loader"?


-- 
MST

Re: [Qemu-devel] [SeaBIOS] [PATCH v2 4/5] acpi: load and link tables from /etc/acpi/

[Michael S. Tsirkin]

On Mon, Jul 15, 2013 at 11:11:59AM +0300, Michael S. Tsirkin wrote:
> On Sun, Jul 14, 2013 at 02:27:14PM -0400, Kevin O'Connor wrote:
> > On Sun, Jul 07, 2013 at 06:42:43PM +0300, Michael S. Tsirkin wrote:
> > > Load files in /etc/acpi/, link them using
> > > a linker script and use for acpi tables, including the RSDP.
> > > Presense of RSDP in this directory completely disables
> > > generating and loading legacy acpi tables.
> > > 
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  src/acpi.c | 39 +++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 39 insertions(+)
> > > 
> > > diff --git a/src/acpi.c b/src/acpi.c
> > > index a81f0cb..07d311a 100644
> > > --- a/src/acpi.c
> > > +++ b/src/acpi.c
> > > @@ -27,6 +27,7 @@
> > >  #include "config.h" // CONFIG_*
> > >  #include "paravirt.h" // RamSize
> > >  #include "dev-q35.h"
> > > +#include "linker.h"
> > >  
> > >  #include "acpi-dsdt.hex"
> > >  
> > > @@ -599,6 +600,34 @@ static const struct pci_device_id acpi_find_tbl[] = {
> > >  
> > >  struct rsdp_descriptor *RsdpAddr;
> > >  
> > > +/* Look for RSDP signature in files loaded in FSEG */
> > > +struct rsdp_descriptor *
> > > +acpi_find_rsdp_rom(void)
> > > +{
> > > +    struct romfile_s *file = NULL;
> > > +    struct rsdp_descriptor *rsdp = NULL;
> > > +    for (;;) {
> > > +        file = romfile_findprefix("", file);
> > > +        if (!file)
> > > +            break;
> > > +
> > > +        if (!file->data || !pmm_test_fseg(file->data) ||
> > > +            file->size < sizeof(rsdp->signature))
> > > +            continue;
> > > +
> > > +        void *data;
> > > +
> > > +        for (data = file->data;
> > > +             data + sizeof(*rsdp) <= file->data + file->size;
> > > +             data++) {
> > > +            rsdp = data;
> > > +            if (rsdp->signature == cpu_to_le64(RSDP_SIGNATURE))
> > > +                return rsdp;
> > > +        }
> > > +    }
> > > +    return NULL;
> > > +}
> > > +
> > >  #define MAX_ACPI_TABLES 20
> > >  void
> > >  acpi_setup(void)
> > > @@ -608,6 +637,16 @@ acpi_setup(void)
> > >  
> > >      dprintf(3, "init ACPI tables\n");
> > >  
> > > +    linker_loader_execute("/etc/linker-script");
> > > +
> > > +    RsdpAddr = acpi_find_rsdp_rom();
> > > +
> > > +    if (RsdpAddr) {
> > > +        return;
> > > +    }
> > 
> > I don't understand this.  Why not use the presence of
> > "/etc/linker-script" to determine if ACPI should be produced.
> 
> We can do this but what do we do if linker script is there
> but no rsdp?
> Maybe just add
> 	test -e /etc/linker-script != RsdpAddr
> 	        dprintf(1, "linker script found but no rsdp. Falling back on builtin tables\n");
> 
> Would this address your comment?
> 

And just to clarify, in my eyes it's preferable to use
acpi_find_rsdp_rom which is based on ACPI standard than
look for a specific PV interface.

For example, one can imagine a hypervisor that maps
ACPI tables directly into guest memory. Looking for
rsdp in fseg memory will work, relying on the
linker won't.

> > Also, /etc/linker-script is not very informative - how about something
> > like "/etc/biostables".
> > 
> > -Kevin
> 
> Hmm bios seems redundant, and tables makes one think it's the tables
> themselves.
> 
> "/etc/acpi-linker"
> "/etc/acpi-loader"?
> 
> 
> -- 
> MST

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

[Michael S. Tsirkin]

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

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

[Kevin O'Connor]

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

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

[Michael S. Tsirkin]

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

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

[Michael S. Tsirkin]

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

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

[Michael S. Tsirkin]

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.

-- 
MST