seabios: load acpi tables from qemu

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.

src/linker.c new file +0 -0

Quoted 4 lines · 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.

Thanks. See my comments below.

[...]

Quoted 2 lines

src/linker.c new file +0 -0

[...]

src/linker.c line 109

Quoted 9 lines

src/linker.c new file +9 -0

+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) ).

src/linker.c line 118

Quoted 9 lines

src/linker.c new file +9 -0

+        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.

[...]

src/util.h struct romfile_s { · line 440

Quoted 8 lines

src/util.h +1 -0

@@ -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

Quoted 38 lines · Kevin O'Connor

Quoted 4 lines · 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.

Thanks. See my comments below.

[...]

Quoted 2 lines

src/linker.c new file +0 -0

[...]

src/linker.c line 109

Quoted 9 lines

src/linker.c new file +9 -0

+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) ).

src/linker.c line 118

Quoted 9 lines

src/linker.c new file +9 -0

+        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.

[...]

Quoted 11 lines

src/util.h struct romfile_s { · line 440

Quoted 8 lines

src/util.h +1 -0

@@ -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?

Quoted 5 lines

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?

Signature · 1 line

MST

Quoted 8 lines · Michael S. Tsirkin

Quoted 2 lines · Kevin O'Connor

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

Quoted 7 lines

struct linker {
	char name[56]
	void *data;
	struct hlist_node node;
}

is this preferable?

Quoted 6 lines

Quoted 4 lines

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?

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Michael S. Tsirkin

Quoted 2 lines · Kevin O'Connor

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

Quoted 7 lines

Quoted 7 lines

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.

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

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

Quoted 31 lines · Kevin O'Connor

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Michael S. Tsirkin

Quoted 2 lines · Kevin O'Connor

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

Quoted 7 lines

Quoted 7 lines

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

Quoted 1 line

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.

Quoted 10 lines

Quoted 6 lines

Quoted 6 lines

Quoted 4 lines

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

Quoted 42 lines · Michael S. Tsirkin

Quoted 31 lines · Kevin O'Connor

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Michael S. Tsirkin

Quoted 2 lines · Kevin O'Connor

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

Quoted 7 lines

Quoted 7 lines

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

Quoted 1 line

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.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

Quoted 1 line

romfile_loader?

Shrug. How about "tabledeploy"?

-Kevin

So I tried this out

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 <[email protected]>

---
 src/acpi.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

src/acpi.c +29 -0

index a81f0cb..07d311a 100644
@@ -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)

Quoted 61 lines · 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 <[email protected]>

---
 src/acpi.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

src/acpi.c +29 -0

index a81f0cb..07d311a 100644
@@ -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");
 

src/acpi.c acpi_setup(void) · line 646

Quoted 7 lines

src/acpi.c +7 -0

+    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

Quoted 62 lines · Kevin O'Connor

Quoted 61 lines · 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 <[email protected]>

---
 src/acpi.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

src/acpi.c +29 -0

index a81f0cb..07d311a 100644
@@ -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");
 

Quoted 10 lines

src/acpi.c acpi_setup(void) · line 646

Quoted 7 lines

src/acpi.c +7 -0

+    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?

Quoted 4 lines

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"?

Signature · 1 line

MST