The first patch cleans up some field assignments in build_madt(). The second one handles the case when MADT is provided by qemu over fw_cfg.
I'll soon post the qemu-side series as well.
I cross-tested {patched, unpatched} qemu with {patched, unpatched} seabios. Inside a RHEL-6 guest I dumped the RSDT, DSDT, and APIC (MADT) in all four cases, and compared the results. Patched qemu with unpatched seabios don't play nicely together (two APIC (MADT) tables present). Otherwise everything seemed fine to me.
The CPU topology was set with
-smp 3,maxcpus=16,sockets=2,cores=4,threads=2
The table dumps / disassemblies are too big to include here, please find them at http://people.redhat.com/~lersek/acpi_move/madt_tests.tar.gz.
Laszlo Ersek (2): build_madt(): "fix" intsrcovr->{gsi,flags} and local_nmi->flags byte order accept MADT over fw_cfg
src/acpi.c | 31 +++++++++++++++++++++++-------- 1 files changed, 23 insertions(+), 8 deletions(-)
These fields are wider than a single byte; stick to cpu_to_leXX() for consistency with other field settings in this function.
Signed-off-by: Laszlo Ersek lersek@redhat.com --- src/acpi.c | 12 +++++++----- 1 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/acpi.c b/src/acpi.c index 119d1c1..8bbc92b 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -367,8 +367,9 @@ build_madt(void) intsrcovr->type = APIC_XRUPT_OVERRIDE; intsrcovr->length = sizeof(*intsrcovr); intsrcovr->source = 0; - intsrcovr->gsi = 2; - intsrcovr->flags = 0; /* conforms to bus specifications */ + intsrcovr->gsi = cpu_to_le32(2); + intsrcovr->flags = cpu_to_le16(0); + /* conforms to bus specifications */ intsrcovr++; } for (i = 1; i < 16; i++) { @@ -379,8 +380,9 @@ build_madt(void) intsrcovr->type = APIC_XRUPT_OVERRIDE; intsrcovr->length = sizeof(*intsrcovr); intsrcovr->source = i; - intsrcovr->gsi = i; - intsrcovr->flags = 0xd; /* active high, level triggered */ + intsrcovr->gsi = cpu_to_le32(i); + intsrcovr->flags = cpu_to_le16(0xd); + /* active high, level triggered */ intsrcovr++; }
@@ -388,7 +390,7 @@ build_madt(void) local_nmi->type = APIC_LOCAL_NMI; local_nmi->length = sizeof(*local_nmi); local_nmi->processor_id = 0xff; /* all processors */ - local_nmi->flags = 0; + local_nmi->flags = cpu_to_le16(0); local_nmi->lint = 1; /* LINT1 */ local_nmi++;
Signed-off-by: Laszlo Ersek lersek@redhat.com --- src/acpi.c | 19 ++++++++++++++++--- 1 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/src/acpi.c b/src/acpi.c index 8bbc92b..611553e 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -797,13 +797,13 @@ acpi_setup(void) struct fadt_descriptor_rev1 *fadt = build_fadt(pci); ACPI_INIT_TABLE(fadt); ACPI_INIT_TABLE(build_ssdt()); - ACPI_INIT_TABLE(build_madt()); ACPI_INIT_TABLE(build_hpet()); ACPI_INIT_TABLE(build_srat()); if (pci->device == PCI_DEVICE_ID_INTEL_ICH9_LPC) ACPI_INIT_TABLE(build_mcfg_q35());
struct romfile_s *file = NULL; + int madt_found = 0; for (;;) { file = romfile_findprefix("acpi/", file); if (!file) @@ -816,13 +816,19 @@ acpi_setup(void) int ret = file->copy(file, table, file->size); if (ret <= sizeof(*table)) continue; - if (table->signature == DSDT_SIGNATURE) { + switch (table->signature) { + case DSDT_SIGNATURE: if (fadt) { fill_dsdt(fadt, table); } - } else { + break; + case APIC_SIGNATURE: + madt_found = 1; + /* fall through */ + default: ACPI_INIT_TABLE(table); } + if (tbl_idx == MAX_ACPI_TABLES) { warn_noalloc(); break; @@ -838,6 +844,13 @@ acpi_setup(void) memcpy(dsdt, AmlCode, sizeof(AmlCode)); fill_dsdt(fadt, dsdt); } + if (!madt_found) { + if (tbl_idx == MAX_ACPI_TABLES) { + warn_noalloc(); + return; + } + ACPI_INIT_TABLE(build_madt()); + }
// Build final rsdt table struct rsdt_descriptor_rev1 *rsdt;
On Wed, Mar 20, 2013 at 10:53:04PM +0100, Laszlo Ersek wrote:
These fields are wider than a single byte; stick to cpu_to_leXX() for consistency with other field settings in this function.
Thanks. We can do this to improve documentation. Please ack that you are okay with licensing under GPLv2+.
-Kevin
On 03/21/13 00:52, Kevin O'Connor wrote:
On Wed, Mar 20, 2013 at 10:53:04PM +0100, Laszlo Ersek wrote:
These fields are wider than a single byte; stick to cpu_to_leXX() for consistency with other field settings in this function.
Thanks. We can do this to improve documentation. Please ack that you are okay with licensing under GPLv2+.
Surprisingly, my patch has just moved me into the target audience of Michael's patch. Will ACK.
Thanks Laszlo
On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
Signed-off-by: Laszlo Ersek lersek@redhat.com
I think we need to figure out what the final fw_cfg interface for ACPI, SMBIOS, mptable, and PIR will be.
We can use the current fw_cfg ACPI table passing mechanism for ACPI, but there are a couple of things that need to be worked out. For each table, there must be a way to determine if SeaBIOS should build it, or if the table should not be present at all. For example, if MADT isn't present in the fw_cfg, is that because SeaBIOS is expected to build it or because it is not expected to be present at all? This also needs to be resolved for SSDT tables, which can have zero, one, or more instances. Finally, the mechanism should define how the RSDP signatures is set as well as support both RSDT and XSDT tables (with signatures defined by QEMU for these as well).
For SMBIOS, I don't think we should use the existing fw_cfg mechanism. It's too complicated for what is needed. Instead, one fw_cfg "file" entry with the whole smbios table should suffice. For mptable and PIR, there is no current mechanism, so we can add new fw_cfg "files" for them. However, for all of these SeaBIOS needs to be able to determine when it needs to create the table and when no table should be published at all.
One possible way to accomplish the above would be to add an "all_tables_from_qemu" fw_cfg entry that turned off all of the existing SeaBIOS code. There are probably other ways.
Thoughts?
-Kevin
On Wed, Mar 20, 2013 at 08:22:30PM -0400, Kevin O'Connor wrote:
On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
Signed-off-by: Laszlo Ersek lersek@redhat.com
I think we need to figure out what the final fw_cfg interface for ACPI, SMBIOS, mptable, and PIR will be.
We can use the current fw_cfg ACPI table passing mechanism for ACPI, but there are a couple of things that need to be worked out. For each table, there must be a way to determine if SeaBIOS should build it, or if the table should not be present at all. For example, if MADT isn't present in the fw_cfg, is that because SeaBIOS is expected to build it or because it is not expected to be present at all? This also needs to be resolved for SSDT tables, which can have zero, one, or more instances.
How about we don't bother to determine this at runtime at all? Long term all tables will be in qemu so why bother with a runtime mechanism just for an intermediate stage? Just build BIOS with the correct flags.
Finally, the mechanism should define how the RSDP signatures is set as well as support both RSDT and XSDT tables (with signatures defined by QEMU for these as well).
For SMBIOS, I don't think we should use the existing fw_cfg mechanism. It's too complicated for what is needed. Instead, one fw_cfg "file" entry with the whole smbios table should suffice. For mptable and PIR, there is no current mechanism, so we can add new fw_cfg "files" for them. However, for all of these SeaBIOS needs to be able to determine when it needs to create the table and when no table should be published at all.
One possible way to accomplish the above would be to add an "all_tables_from_qemu" fw_cfg entry that turned off all of the existing SeaBIOS code. There are probably other ways.
Thoughts?
-Kevin
Yes I agree these tables need some thought. But first things first, we can move quite a lot of code out to qemu with just fw_cfg.
On 03/21/13 07:23, Michael S. Tsirkin wrote:
On Wed, Mar 20, 2013 at 08:22:30PM -0400, Kevin O'Connor wrote:
On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
Signed-off-by: Laszlo Ersek lersek@redhat.com
I think we need to figure out what the final fw_cfg interface for ACPI, SMBIOS, mptable, and PIR will be.
We can use the current fw_cfg ACPI table passing mechanism for ACPI, but there are a couple of things that need to be worked out. For each table, there must be a way to determine if SeaBIOS should build it, or if the table should not be present at all. For example, if MADT isn't present in the fw_cfg, is that because SeaBIOS is expected to build it or because it is not expected to be present at all?
I think we always have a MADT, don't we? So why worry about the case that we might have no MADT? I think the logic is just fine here.
Having a debug printk saying which table came from there would be nice for trouble shooting though.
This also needs to be resolved for SSDT tables, which can have zero, one, or more instances.
Same argument as for the MADT.
How about we don't bother to determine this at runtime at all?
Because it will be a PITA for testers + developers to figure the correct .config switches of the day during the transition phase?
For SMBIOS, I don't think we should use the existing fw_cfg mechanism. It's too complicated for what is needed. Instead, one fw_cfg "file" entry with the whole smbios table should suffice.
Agree.
For mptable and PIR, there is no current mechanism, so we can add new fw_cfg "files" for them. However, for all of these SeaBIOS needs to be able to determine when it needs to create the table and when no table should be published at all.
Same argument as for the MADT.
One possible way to accomplish the above would be to add an "all_tables_from_qemu" fw_cfg entry that turned off all of the existing SeaBIOS code. There are probably other ways.
As this is quite a bunch of work I would tend to avoid a flag day like this so we can switch over tables one by one without building up big patch queues.
Once all is done switched over we can add a .config option for the seabios acpi table generation code, so it can be turned off altogether for qemu versions new enougth.
cheers, Gerd
On Thu, Mar 21, 2013 at 09:18:50AM +0100, Gerd Hoffmann wrote:
On 03/21/13 07:23, Michael S. Tsirkin wrote:
On Wed, Mar 20, 2013 at 08:22:30PM -0400, Kevin O'Connor wrote:
On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
Signed-off-by: Laszlo Ersek lersek@redhat.com
I think we need to figure out what the final fw_cfg interface for ACPI, SMBIOS, mptable, and PIR will be.
We can use the current fw_cfg ACPI table passing mechanism for ACPI, but there are a couple of things that need to be worked out. For each table, there must be a way to determine if SeaBIOS should build it, or if the table should not be present at all. For example, if MADT isn't present in the fw_cfg, is that because SeaBIOS is expected to build it or because it is not expected to be present at all?
I think we always have a MADT, don't we? So why worry about the case that we might have no MADT? I think the logic is just fine here.
Having a debug printk saying which table came from there would be nice for trouble shooting though.
This also needs to be resolved for SSDT tables, which can have zero, one, or more instances.
Same argument as for the MADT.
How about we don't bother to determine this at runtime at all?
Because it will be a PITA for testers + developers to figure the correct .config switches of the day during the transition phase?
Why is it a PITA? Are you developing QEMU? Just use the makefile from roms/config.seabios Are you using QEMU binary? Just use the defaults.
For SMBIOS, I don't think we should use the existing fw_cfg mechanism. It's too complicated for what is needed. Instead, one fw_cfg "file" entry with the whole smbios table should suffice.
Agree.
For mptable and PIR, there is no current mechanism, so we can add new fw_cfg "files" for them. However, for all of these SeaBIOS needs to be able to determine when it needs to create the table and when no table should be published at all.
Same argument as for the MADT.
One possible way to accomplish the above would be to add an "all_tables_from_qemu" fw_cfg entry that turned off all of the existing SeaBIOS code. There are probably other ways.
As this is quite a bunch of work I would tend to avoid a flag day like this so we can switch over tables one by one without building up big patch queues.
Once all is done switched over we can add a .config option for the seabios acpi table generation code, so it can be turned off altogether for qemu versions new enougth.
cheers, Gerd
Agree. But no need for a runtime hack. People building seabios for qemu should use the makefile from roms/config.seabios we can flip switches there atomically with adding tables to QEMU.
On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
Signed-off-by: Laszlo Ersek lersek@redhat.com
I think this is a bit too aggressive. Let's do what I did for DSDT, add a config option and default to yes. In QEMU, override it to remove MADT from bios.
src/acpi.c | 19 ++++++++++++++++--- 1 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/src/acpi.c b/src/acpi.c index 8bbc92b..611553e 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -797,13 +797,13 @@ acpi_setup(void) struct fadt_descriptor_rev1 *fadt = build_fadt(pci); ACPI_INIT_TABLE(fadt); ACPI_INIT_TABLE(build_ssdt());
ACPI_INIT_TABLE(build_madt()); ACPI_INIT_TABLE(build_hpet()); ACPI_INIT_TABLE(build_srat()); if (pci->device == PCI_DEVICE_ID_INTEL_ICH9_LPC) ACPI_INIT_TABLE(build_mcfg_q35());
struct romfile_s *file = NULL;
- int madt_found = 0; for (;;) { file = romfile_findprefix("acpi/", file); if (!file)
@@ -816,13 +816,19 @@ acpi_setup(void) int ret = file->copy(file, table, file->size); if (ret <= sizeof(*table)) continue;
if (table->signature == DSDT_SIGNATURE) {
switch (table->signature) {case DSDT_SIGNATURE: if (fadt) { fill_dsdt(fadt, table); }
} else {
break;case APIC_SIGNATURE:madt_found = 1;/* fall through */default: ACPI_INIT_TABLE(table); }if (tbl_idx == MAX_ACPI_TABLES) { warn_noalloc(); break;@@ -838,6 +844,13 @@ acpi_setup(void) memcpy(dsdt, AmlCode, sizeof(AmlCode)); fill_dsdt(fadt, dsdt); }
if (!madt_found) {
if (tbl_idx == MAX_ACPI_TABLES) {warn_noalloc();return;}ACPI_INIT_TABLE(build_madt());}
// Build final rsdt table struct rsdt_descriptor_rev1 *rsdt;
-- 1.7.1
On Wed, 2013-03-20 at 20:22 -0400, Kevin O'Connor wrote:
On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
Signed-off-by: Laszlo Ersek lersek@redhat.com
I think we need to figure out what the final fw_cfg interface for ACPI, SMBIOS, mptable, and PIR will be.
Once we have consensus, we can implement this on the OVMF side too. Is anyone (Laszlo?) looking at that, or should I?
For SMBIOS, I don't think we should use the existing fw_cfg mechanism. It's too complicated for what is needed. Instead, one fw_cfg "file" entry with the whole smbios table should suffice.
Agreed. I'd already looked at doing this in OVMF, and run away screaming.
For mptable and PIR, there is no current mechanism, so we can add new fw_cfg "files" for them. However, for all of these SeaBIOS needs to be able to determine when it needs to create the table and when no table should be published at all.
I'd make it all-or-nothing. Except for a few historical qemu commits if you're bisecting, why would qemu ever provide a *partial* set of tables?
On Thu, Mar 21, 2013 at 12:23:45PM +0000, David Woodhouse wrote:
On Wed, 2013-03-20 at 20:22 -0400, Kevin O'Connor wrote:
On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
Signed-off-by: Laszlo Ersek lersek@redhat.com
I think we need to figure out what the final fw_cfg interface for ACPI, SMBIOS, mptable, and PIR will be.
Once we have consensus, we can implement this on the OVMF side too. Is anyone (Laszlo?) looking at that, or should I?
For SMBIOS, I don't think we should use the existing fw_cfg mechanism. It's too complicated for what is needed. Instead, one fw_cfg "file" entry with the whole smbios table should suffice.
Agreed. I'd already looked at doing this in OVMF, and run away screaming.
For mptable and PIR, there is no current mechanism, so we can add new fw_cfg "files" for them. However, for all of these SeaBIOS needs to be able to determine when it needs to create the table and when no table should be published at all.
I'd make it all-or-nothing. Except for a few historical qemu commits if you're bisecting, why would qemu ever provide a *partial* set of tables?
I think we should just get the config from qemu. Pls take a look at the README patch I posted. It will also help future extensions.
-- Sent with MeeGo's ActiveSync support.
David Woodhouse Open Source Technology Centre David.Woodhouse@intel.com Intel Corporation
Hi,
How about we don't bother to determine this at runtime at all?
Because it will be a PITA for testers + developers to figure the correct .config switches of the day during the transition phase?
Why is it a PITA? Are you developing QEMU? Just use the makefile from roms/config.seabios Are you using QEMU binary? Just use the defaults.
SeaBIOS binaries are running on a wide range of qemu versions today. Changing that is a big deal. People are not used to it, and even when writing it to the README people will stumble over it. It also is pretty inconvenient in a number of cases. For starters the usual way to package seabios and qemu in distros is to have separate packages ...
cheers, Gerd
On Thu, 2013-03-21 at 13:49 +0100, Gerd Hoffmann wrote:
How about we don't bother to determine this at runtime at all?
Because it will be a PITA for testers + developers to figure the correct .config switches of the day during the transition phase?
Why is it a PITA? Are you developing QEMU? Just use the makefile from roms/config.seabios Are you using QEMU binary? Just use the defaults.
SeaBIOS binaries are running on a wide range of qemu versions today. Changing that is a big deal. People are not used to it, and even when writing it to the README people will stumble over it. It also is pretty inconvenient in a number of cases. For starters the usual way to package seabios and qemu in distros is to have separate packages ...
I agree that for the foreseeable future, we should be able to build SeaBIOS such that it can cope with old versions of Qemu that *don't* provide ACPI tables.
And of course we should make it cope with new versions of Qemu that *do*.
But I'm not sure I see any point in doing it table-by-table. Surely it can be all or nothing?
On 03/21/13 13:23, David Woodhouse wrote:
On Wed, 2013-03-20 at 20:22 -0400, Kevin O'Connor wrote:
On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
Signed-off-by: Laszlo Ersek lersek@redhat.com
I think we need to figure out what the final fw_cfg interface for ACPI, SMBIOS, mptable, and PIR will be.
Once we have consensus, we can implement this on the OVMF side too. Is anyone (Laszlo?) looking at that, or should I?
I figured we should design and implement the thing first between qemu and seabios (seabios being the more widely used firmware ATM I reckon), soak it in user testing, and then implement a "ready plan" in OVMF. I expect a lot of back&forth in this, so let's not double it :)
As long as qemu is exporting the current fw_cfg keys, present code in OVMF shouldn't break even if we export some more stuff.
For SMBIOS, I don't think we should use the existing fw_cfg mechanism. It's too complicated for what is needed. Instead, one fw_cfg "file" entry with the whole smbios table should suffice.
Agreed. I'd already looked at doing this in OVMF, and run away screaming.
In light of this, your offer above is even more appreciated :)
I wouldn't mind doing the OVMF side from an emotional standpoint, I just feel overloaded / overwhelmed already by what I'm looking at. So certainly don't wait for me (but consider that only work that is not done is not lost). If you pick it up I'm thankful, if you don't, I can still tack it to my todo list.
For mptable and PIR, there is no current mechanism, so we can add new fw_cfg "files" for them. However, for all of these SeaBIOS needs to be able to determine when it needs to create the table and when no table should be published at all.
I'd make it all-or-nothing. Except for a few historical qemu commits if you're bisecting, why would qemu ever provide a *partial* set of tables?
I can visualize myself implementing Michael's build time option suggestion: if the option is set, SeaBIOS installs its own MADT (and any MADT from qemu will create a duplicate); if the option is clear, SeaBIOS won't install its own MADT (and if qemu doesn't provide one either, no MADT will be present). So, - for an earlier qemu, the option must be set, - for a later qemu the option must be clear && (no -acpitable switch must be specified on the qemu cmldine || one -acpitable switch must load a MADT)
Thanks Laszlo
On Thu, Mar 21, 2013 at 01:49:36PM +0100, Gerd Hoffmann wrote:
Hi,
How about we don't bother to determine this at runtime at all?
Because it will be a PITA for testers + developers to figure the correct .config switches of the day during the transition phase?
Why is it a PITA? Are you developing QEMU? Just use the makefile from roms/config.seabios Are you using QEMU binary? Just use the defaults.
SeaBIOS binaries are running on a wide range of qemu versions today. Changing that is a big deal.
Chaqnging what? Legacy QEMU will keep working if you build in ACPI tables.
People are not used to it, and even when writing it to the README people will stumble over it. It also is pretty inconvenient in a number of cases.
Could you give me one example?
I don't get it. If you are working on both QEMU and seabios, you better build seabios in the way that qemu build process uses. Anything else is just wrong on so many levels...
For starters the usual way to package seabios and qemu in distros is to have separate packages ...
cheers, Gerd
Which ones? That's just crazy. Fedora packages them together: qemu-system-x86-1.2.2-6.fc18.i686 bios is a firmware flashed on the motherboard. You want to change motherboard but use the same firmware?
On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
- for an earlier qemu, the option must be set,
- for a later qemu the option must be clear && (no -acpitable switch must be specified on the qemu cmldine || one -acpitable switch must load a MADT)
Hm, that sounds like it won't be possible to build one version of SeaBIOS that works for *both* old and new qemu. That doesn't seem like a great idea. I'd prefer something like:
- If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then SeaBIOS uses them. - Otherwise, it makes its own.
On 03/21/13 13:52, David Woodhouse wrote:
On Thu, 2013-03-21 at 13:49 +0100, Gerd Hoffmann wrote:
How about we don't bother to determine this at runtime at all?
Because it will be a PITA for testers + developers to figure the correct .config switches of the day during the transition phase?
Why is it a PITA? Are you developing QEMU? Just use the makefile from roms/config.seabios Are you using QEMU binary? Just use the defaults.
SeaBIOS binaries are running on a wide range of qemu versions today. Changing that is a big deal. People are not used to it, and even when writing it to the README people will stumble over it. It also is pretty inconvenient in a number of cases. For starters the usual way to package seabios and qemu in distros is to have separate packages ...
I agree that for the foreseeable future, we should be able to build SeaBIOS such that it can cope with old versions of Qemu that *don't* provide ACPI tables.
And of course we should make it cope with new versions of Qemu that *do*.
I'm fine with that as well.
I can't say anything about tables "in general". Regarding the MADT, we always must have one (at least under the "APIC interrupt model", if I remember the ACPI spec correctly), and for that this 2/2 should be OK. The debug message regarding the table's origin I do agree with, I can repost if that's the way to go.
Wrt. the interface. I think the current "mass dump of tables in one fw_cfg file" is a bit inconvenient but I didn't want to change it. If we had a separate fw_cfg *file* (not key of course) for each table, we could define its format like: - file not present: qemu is old, seabios should install the table, - file is present with a special first byte: seabios should *not* install the table, - file is present with another first byte and a trailing blob: seabios should install the blob as the table.
(Very coarse idea of admittedly, maybe some more headers will be necessary.)
Combining the current "big dump of tables's contents" with the more flexible "Y/N file per table" approach seems ugly.
But I'm not sure I see any point in doing it table-by-table. Surely it can be all or nothing?
Not while we're implementing those "few historical commits" :)
Thanks Laszlo
On Thu, Mar 21, 2013 at 12:52:17PM +0000, David Woodhouse wrote:
On Thu, 2013-03-21 at 13:49 +0100, Gerd Hoffmann wrote:
How about we don't bother to determine this at runtime at all?
Because it will be a PITA for testers + developers to figure the correct .config switches of the day during the transition phase?
Why is it a PITA? Are you developing QEMU? Just use the makefile from roms/config.seabios Are you using QEMU binary? Just use the defaults.
SeaBIOS binaries are running on a wide range of qemu versions today. Changing that is a big deal. People are not used to it, and even when writing it to the README people will stumble over it. It also is pretty inconvenient in a number of cases. For starters the usual way to package seabios and qemu in distros is to have separate packages ...
I agree that for the foreseeable future, we should be able to build SeaBIOS such that it can cope with old versions of Qemu that *don't* provide ACPI tables.
And of course we should make it cope with new versions of Qemu that *do*.
But I'm not sure I see any point in doing it table-by-table. Surely it can be all or nothing?
-- dwmw2
If someone wants to add a feature where same bios works with old and new qemu, I don't see a problem here. I also don't see why we should not allow building a minimal bios that only works with the new specific qemu. This is the only option that distros will actually ship, so runtime detection is a developer's tool really, seems quite sane to allow saving some resources by removing it.
No?
On Thu, Mar 21, 2013 at 01:04:35PM +0000, David Woodhouse wrote:
On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
- for an earlier qemu, the option must be set,
- for a later qemu the option must be clear && (no -acpitable switch must be specified on the qemu cmldine || one -acpitable switch must load a MADT)
Hm, that sounds like it won't be possible to build one version of SeaBIOS that works for *both* old and new qemu. That doesn't seem like a great idea. I'd prefer something like:
- If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then SeaBIOS uses them.
- Otherwise, it makes its own.
-- dwmw2
It might simplify life for someone bisecting qemu as you don't need to rebuild seabios after each bisect but is this really a common workflow?
Anyway, I am not against such runtime flags.
If we add to this an option to build a minimal BIOS that only works with the new QEMU, do we have a deal?
Then the plan is to make progress and apply patches step by step without deciding on the detection interface first. Before QEMU is switched to the new configuration, we'll add the runtime thing for the benefit of developers that bisect.
Makes sense?
On Thu, 2013-03-21 at 15:12 +0200, Michael S. Tsirkin wrote:
On Thu, Mar 21, 2013 at 01:04:35PM +0000, David Woodhouse wrote:
On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
- for an earlier qemu, the option must be set,
- for a later qemu the option must be clear && (no -acpitable switch must be specified on the qemu cmldine || one -acpitable switch must load a MADT)
Hm, that sounds like it won't be possible to build one version of SeaBIOS that works for *both* old and new qemu. That doesn't seem like a great idea. I'd prefer something like:
- If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then SeaBIOS uses them.
- Otherwise, it makes its own.
-- dwmw2
It might simplify life for someone bisecting qemu as you don't need to rebuild seabios after each bisect but is this really a common workflow?
Not bisection, but perhaps switching between an old and a new version of qemu.
Anyway, I am not against such runtime flags.
If we add to this an option to build a minimal BIOS that only works with the new QEMU, do we have a deal?
Yeah, definitely. The code for SeaBIOS to build its own should certainly be optional. It should be possible to build a minimal SeaBIOS which *can't*.
On Thu, Mar 21, 2013 at 01:14:38PM +0000, David Woodhouse wrote:
On Thu, 2013-03-21 at 15:12 +0200, Michael S. Tsirkin wrote:
On Thu, Mar 21, 2013 at 01:04:35PM +0000, David Woodhouse wrote:
On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
- for an earlier qemu, the option must be set,
- for a later qemu the option must be clear && (no -acpitable switch must be specified on the qemu cmldine || one -acpitable switch must load a MADT)
Hm, that sounds like it won't be possible to build one version of SeaBIOS that works for *both* old and new qemu. That doesn't seem like a great idea. I'd prefer something like:
- If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then SeaBIOS uses them.
- Otherwise, it makes its own.
-- dwmw2
It might simplify life for someone bisecting qemu as you don't need to rebuild seabios after each bisect but is this really a common workflow?
Not bisection, but perhaps switching between an old and a new version of qemu.
Anyway, I am not against such runtime flags.
If we add to this an option to build a minimal BIOS that only works with the new QEMU, do we have a deal?
Yeah, definitely. The code for SeaBIOS to build its own should certainly be optional. It should be possible to build a minimal SeaBIOS which *can't*.
Okay that's exactly what me and Laszlo are working on. We want to add a special way to build qemu and seabios such that they work with tables in qemu, then add runtime detection on top.
The advantage is that we can make progress without keeping lots of patches out of tree. Unless of course Kevin nacks this all ...
-- dwmw2
On 03/21/13 14:01, Michael S. Tsirkin wrote:
On Thu, Mar 21, 2013 at 01:49:36PM +0100, Gerd Hoffmann wrote:
Hi,
How about we don't bother to determine this at runtime at all?
Because it will be a PITA for testers + developers to figure the correct .config switches of the day during the transition phase?
Why is it a PITA? Are you developing QEMU? Just use the makefile from roms/config.seabios Are you using QEMU binary? Just use the defaults.
SeaBIOS binaries are running on a wide range of qemu versions today. Changing that is a big deal.
Chaqnging what? Legacy QEMU will keep working if you build in ACPI tables.
Today you can clone upstream seabios, build it, and the resulting image will work on pretty much any qemu version since 0.12 or so. You don't have to pick the correct config switches for your particular qemu version, it just works. I think it should stay that way by default.
Having config options to turn off support for older qemu versions is fine, so we can strip the bios binaries bundled with qemu by leaving out code which would not be used anyway.
For starters the usual way to package seabios and qemu in distros is to have separate packages ...
Which ones? That's just crazy. Fedora packages them together:
No:
[root@fedora ~]# ll /usr/share/qemu/bios.bin lrwxrwxrwx. 1 root root 19 Feb 13 15:49 /usr/share/qemu/bios.bin -> ../seabios/bios.bin [root@fedora ~]# rpm -qf /usr/share/*/bios.bin qemu-system-x86-1.2.2-6.fc18.x86_64 seabios-bin-1.7.1-4.fc18.noarch
cheers, Gerd
Hi,
But I'm not sure I see any point in doing it table-by-table. Surely it can be all or nothing?
It allows to merge changes piecewise and avoids piling up long patch queues. It makes bisecting regressions easier. Also the logic "if table $foo is provided by qemu, just use it, otherwise generate it / use compiled-in" seems to be simple enougth.
But at the end of the day it is the call of the one actually doing the work. Maybe I'm over-estimating the work needed to do the switch.
cheers, Gerd
This config option, when set to N, allows any qemu-built MADT to take precedence.
Signed-off-by: Laszlo Ersek lersek@redhat.com --- in v2: - replaced primitive dynamic detection code with static config option - 352 bytes saved in 32bit flat init when set to N (RHEL-6 gcc-4.4.7)
v2 depends on Michael's [PATCHv4] acpi: make default DSDT optional
src/acpi.c | 3 +++ src/Kconfig | 14 ++++++++++++++ 2 files changed, 17 insertions(+), 0 deletions(-)
diff --git a/src/acpi.c b/src/acpi.c index 66509b4..6ef7106 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -327,6 +327,9 @@ build_fadt(struct pci_device *pci) static void* build_madt(void) { + if (!CONFIG_ACPI_MADT) + return NULL; + int madt_size = (sizeof(struct multiple_apic_table) + sizeof(struct madt_processor_apic) * MaxCountCPUs + sizeof(struct madt_io_apic) diff --git a/src/Kconfig b/src/Kconfig index 3c80132..a847e71 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -397,6 +397,20 @@ menu "BIOS Tables" This option can be disabled for QEMU 1.4 and newer to save some space in the ROM file. If unsure, say Y. + config ACPI_MADT + bool "Build ACPI MADT (APIC) in SeaBIOS" + default y + depends on ACPI + help + Dynamically build the ACPI MADT (APIC) table in SeaBIOS, based on + individual values exported by QEMU and online status of VCPUs. + Required for QEMU 1.4 and older. + This option can (and should) be disabled for QEMU 1.5 and newer as + QEMU prepares and exports the MADT for SeaBIOS to install. + (Although presence of more than one MADT (APIC) tables counts as a + BIOS bug, guest OSes should stick with the first instance, built by + SeaBIOS.) + If unsure, say Y. endmenu
source vgasrc/Kconfig
On Thu, Mar 21, 2013 at 03:04:37PM +0100, Gerd Hoffmann wrote:
Today you can clone upstream seabios, build it, and the resulting image will work on pretty much any qemu version since 0.12 or so. You don't have to pick the correct config switches for your particular qemu version, it just works. I think it should stay that way by default.
Having config options to turn off support for older qemu versions is fine, so we can strip the bios binaries bundled with qemu by leaving out code which would not be used anyway.
Yes. Exactly.
-Kevin
On Thu, Mar 21, 2013 at 09:18:50AM +0100, Gerd Hoffmann wrote:
On 03/21/13 07:23, Michael S. Tsirkin wrote:
On Wed, Mar 20, 2013 at 08:22:30PM -0400, Kevin O'Connor wrote:
On Wed, Mar 20, 2013 at 10:53:05PM +0100, Laszlo Ersek wrote:
Signed-off-by: Laszlo Ersek lersek@redhat.com
I think we need to figure out what the final fw_cfg interface for ACPI, SMBIOS, mptable, and PIR will be.
We can use the current fw_cfg ACPI table passing mechanism for ACPI, but there are a couple of things that need to be worked out. For each table, there must be a way to determine if SeaBIOS should build it, or if the table should not be present at all. For example, if MADT isn't present in the fw_cfg, is that because SeaBIOS is expected to build it or because it is not expected to be present at all?
I think we always have a MADT, don't we? So why worry about the case that we might have no MADT? I think the logic is just fine here.
It's not always the case that we have an HPET or an SRAT though. (And potentially one may wish to test cases where the MADT is not present.)
This also needs to be resolved for SSDT tables, which can have zero, one, or more instances.
Same argument as for the MADT.
The issue with the SSDT is that there can be many of them. QEMU may wish to pass in 2 or more. If QEMU does pass in one or more SSDTs, how will SeaBIOS know if it should also generate it's own SSDT?
For mptable and PIR, there is no current mechanism, so we can add new fw_cfg "files" for them. However, for all of these SeaBIOS needs to be able to determine when it needs to create the table and when no table should be published at all.
Same argument as for the MADT.
I'm not sure about that. Right now the PIR table on q35 is totally bogus - it would assuredly be better to not produce a PIR table then to produce an incorrect one. Maybe the right solution here is to fix the PIR on q35, but I would not be surprised if at some point it became impossible to define a valid PIR for new hardware.
One possible way to accomplish the above would be to add an "all_tables_from_qemu" fw_cfg entry that turned off all of the existing SeaBIOS code. There are probably other ways.
As this is quite a bunch of work I would tend to avoid a flag day like this so we can switch over tables one by one without building up big patch queues.
True. I'm certainly open to ideas.
In practice, one never wants to mix QEMU generated ACPI tables with SeaBIOS generated ACPI tables. To do so would be chaos - the signatures and versions would likely not match, various code assumptions could conflict, and basic things like knowing where to report a bug would be overly complex.
So, I wouldn't want to have to merge all patches in one big "flag day", but maybe, for example, the new QEMU tables should be sent via new fw_cfg entries and SeaBIOS should only be updated to use these new entries once everything is produced by QEMU.
As an aside, there is some mixing of tables already because of the qemu "-acpitable" option. However, this is relatively limited and done solely to import external system tables to appease certain guests - it will always be an external mixing of tables because of this. (The DSDT import isn't mixing tables - the DSDT is generated in SeaBIOS to be used with other SeaBIOS tables - it's just a means of reducing the size of the SeaBIOS binary.)
-Kevin
On Thu, Mar 21, 2013 at 03:26:26PM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 21, 2013 at 01:14:38PM +0000, David Woodhouse wrote:
On Thu, 2013-03-21 at 15:12 +0200, Michael S. Tsirkin wrote:
Anyway, I am not against such runtime flags.
If we add to this an option to build a minimal BIOS that only works with the new QEMU, do we have a deal?
Yeah, definitely. The code for SeaBIOS to build its own should certainly be optional. It should be possible to build a minimal SeaBIOS which *can't*.
Agreed.
Okay that's exactly what me and Laszlo are working on. We want to add a special way to build qemu and seabios such that they work with tables in qemu, then add runtime detection on top.
The advantage is that we can make progress without keeping lots of patches out of tree. Unless of course Kevin nacks this all ...
I think we need to have a plan for what the final interface will look like.
Right now, we can't change QEMU to pass tables via the existing fw_cfg entries unless we upgrade SeaBIOS in QEMU first (otherwise things like duplicate MADT entries occur). If we're going to upgrade SeaBIOS in QEMU, we really want that upgrade to be the final version. We don't want to have to bump the seabios rev in qemu multiple times in lock-step with the changes.
So, I see two ways to do this:
1 - update SeaBIOS with a patch series that makes it capable of handling all tables via existing and new fw_cfg entries (including flags to disable all internal tables), update QEMU to use that SeaBIOS rev, and then apply a patch series to QEMU that has it create all the tables (with the final patch flagging to seabios that it should no longer create any internal tables).
or, 2 - apply a patch series to QEMU that has it create all the tables via new fw_cfg entries, then apply a patch series to seabios that updates it to use the new fw_cfg entries instead of its existing internal tables, and then apply that new rev of seabios to qemu.
Were you proposing one of the above paths, or did you have something else in mind?
-Kevin
As this is quite a bunch of work I would tend to avoid a flag day like this so we can switch over tables one by one without building up big patch queues.
True. I'm certainly open to ideas.
In practice, one never wants to mix QEMU generated ACPI tables with SeaBIOS generated ACPI tables.
Yea, that would only be the case for the transition phase.
But maybe it isn't the best idea to attempt doing the switch without breaking anything along the way. We could use the existing logic vs. "all tables from qemu" depending on a fw_cfg file being present or not as you've suggested. Make that runtime-switchable in qemu for easy testing.
Then start the qemu-provided tables with the minimal feature set needed to boot up (i.e. no cpu hotplug, no pci hotplug, no ssdt patching, static smbios, ...). That could be easy enougth to be added in a single patch series.
ovmf + coreboot can be updated to also use the qemu tables at this point.
Once we are there start generating/patching tables on the qemu side to get back to feature parity with current seabios-provided tables. Can easily be done incrementally, even without having to patch seabios / ovmf / coreboot for each step (well, in theory, in practice there are probably cases where we have to fixup something overseen earlier ...).
Sounds sane?
cheers, Gerd
On 03/22/13 01:04, Kevin O'Connor wrote:
On Thu, Mar 21, 2013 at 03:26:26PM +0200, Michael S. Tsirkin wrote:
The advantage is that we can make progress without keeping lots of patches out of tree. Unless of course Kevin nacks this all ...
I think we need to have a plan for what the final interface will look like.
Right now, we can't change QEMU to pass tables via the existing fw_cfg entries unless we upgrade SeaBIOS in QEMU first (otherwise things like duplicate MADT entries occur). If we're going to upgrade SeaBIOS in QEMU, we really want that upgrade to be the final version. We don't want to have to bump the seabios rev in qemu multiple times in lock-step with the changes.
So, I see two ways to do this:
1 - update SeaBIOS with a patch series that makes it capable of handling all tables via existing and new fw_cfg entries (including flags to disable all internal tables), update QEMU to use that SeaBIOS rev, and then apply a patch series to QEMU that has it create all the tables (with the final patch flagging to seabios that it should no longer create any internal tables).
or, 2 - apply a patch series to QEMU that has it create all the tables via new fw_cfg entries, then apply a patch series to seabios that updates it to use the new fw_cfg entries instead of its existing internal tables, and then apply that new rev of seabios to qemu.
Were you proposing one of the above paths, or did you have something else in mind?
Both of these say "apply a patch series to QEMU that has it create all the tables". I think it would be preferable (for whoever develops the tables in qemu) to submit a standalone series per table, testable in isolation. A huge series covering all tables would likely go up to v5+, and waste a lot of developer & reviewer effort.
In http://thread.gmane.org/gmane.comp.bios.coreboot.seabios/5960/focus=6008,
On 03/22/13 00:22, Kevin O'Connor wrote:
In practice, one never wants to mix QEMU generated ACPI tables with SeaBIOS generated ACPI tables.
AIUI, this is exactly what we'd like to do during development.
But also in general I see no problem with this; *what* should be in any given ACPI table is independent from the table's producer's identity. (The producer only needs access to the dependencies, and that access is producer-specific, like global variables or functions in qemu, fw_cfg in SeaBIOS & OVMF).
The reason for the move is just that we don't want to duplicate work between SeaBIOS and OVMF; a table being produced in qemu versus boot firmware is not inherently right or wrong. (At least it wasn't bothering/intriguing anyone until OVMF started to care about ACPI tables in earnest).
Laszlo
Il 22/03/2013 00:22, Kevin O'Connor ha scritto:
This also needs
> to be resolved for SSDT tables, which can have zero, one, or more > instances.
Same argument as for the MADT.
The issue with the SSDT is that there can be many of them. QEMU may wish to pass in 2 or more. If QEMU does pass in one or more SSDTs, how will SeaBIOS know if it should also generate it's own SSDT?
I think it's a fair assumption that having one SSDT in fw_cfg disables generation of _all_ SSDTs in SeaBIOS.
Paolo
Il 21/03/2013 14:12, Michael S. Tsirkin ha scritto:
On Thu, Mar 21, 2013 at 01:04:35PM +0000, David Woodhouse wrote:
On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
- for an earlier qemu, the option must be set,
- for a later qemu the option must be clear && (no -acpitable switch must be specified on the qemu cmldine || one -acpitable switch must load a MADT)
Hm, that sounds like it won't be possible to build one version of SeaBIOS that works for *both* old and new qemu. That doesn't seem like a great idea. I'd prefer something like:
- If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then SeaBIOS uses them.
- Otherwise, it makes its own.
-- dwmw2
It might simplify life for someone bisecting qemu as you don't need to rebuild seabios after each bisect but is this really a common workflow?
It would help me working on RHEL's qemu-kvm, which I often do on a Fedora box.
The bios.bin in the RHEL qemu-kvm tree is obsolete (like Gerd showed for Fedora, RHEL's bios.bin comes from a separate package), and using whatever happens to be in /usr or in my local SeaBIOS clone is the simplest thing to do. I'd like this not to break.
Paolo
Anyway, I am not against such runtime flags.
If we add to this an option to build a minimal BIOS that only works with the new QEMU, do we have a deal?
Then the plan is to make progress and apply patches step by step without deciding on the detection interface first. Before QEMU is switched to the new configuration, we'll add the runtime thing for the benefit of developers that bisect.
Makes sense?
Il 21/03/2013 16:54, Laszlo Ersek ha scritto:
Dynamically build the ACPI MADT (APIC) table in SeaBIOS, based onindividual values exported by QEMU and online status of VCPUs.Required for QEMU 1.4 and older.This option can (and should) be disabled for QEMU 1.5 and newer asQEMU prepares and exports the MADT for SeaBIOS to install.
No, please...
Paolo
On Fri, Mar 22, 2013 at 08:02:21PM +0100, Paolo Bonzini wrote:
Il 21/03/2013 14:12, Michael S. Tsirkin ha scritto:
On Thu, Mar 21, 2013 at 01:04:35PM +0000, David Woodhouse wrote:
On Thu, 2013-03-21 at 13:56 +0100, Laszlo Ersek wrote:
- for an earlier qemu, the option must be set,
- for a later qemu the option must be clear && (no -acpitable switch must be specified on the qemu cmldine || one -acpitable switch must load a MADT)
Hm, that sounds like it won't be possible to build one version of SeaBIOS that works for *both* old and new qemu. That doesn't seem like a great idea. I'd prefer something like:
- If Qemu provides the 'core' ACPI tables (i.e. not just SSDT) then SeaBIOS uses them.
- Otherwise, it makes its own.
-- dwmw2
It might simplify life for someone bisecting qemu as you don't need to rebuild seabios after each bisect but is this really a common workflow?
It would help me working on RHEL's qemu-kvm, which I often do on a Fedora box.
The bios.bin in the RHEL qemu-kvm tree is obsolete (like Gerd showed for Fedora, RHEL's bios.bin comes from a separate package), and using whatever happens to be in /usr or in my local SeaBIOS clone is the simplest thing to do. I'd like this not to break.
Paolo
Sure, but be warned that the seabios copy of ACPI tables will bitrot with time, it will only be used for old qemu binaries. We might not be able to keep it alive for the full multi-year RHEL lifecycle.
Anyway, I am not against such runtime flags.
If we add to this an option to build a minimal BIOS that only works with the new QEMU, do we have a deal?
Then the plan is to make progress and apply patches step by step without deciding on the detection interface first. Before QEMU is switched to the new configuration, we'll add the runtime thing for the benefit of developers that bisect.
Makes sense?
On Thu, Mar 21, 2013 at 08:04:38PM -0400, Kevin O'Connor wrote:
On Thu, Mar 21, 2013 at 03:26:26PM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 21, 2013 at 01:14:38PM +0000, David Woodhouse wrote:
On Thu, 2013-03-21 at 15:12 +0200, Michael S. Tsirkin wrote:
Anyway, I am not against such runtime flags.
If we add to this an option to build a minimal BIOS that only works with the new QEMU, do we have a deal?
Yeah, definitely. The code for SeaBIOS to build its own should certainly be optional. It should be possible to build a minimal SeaBIOS which *can't*.
Agreed.
Okay that's exactly what me and Laszlo are working on. We want to add a special way to build qemu and seabios such that they work with tables in qemu, then add runtime detection on top.
The advantage is that we can make progress without keeping lots of patches out of tree. Unless of course Kevin nacks this all ...
I think we need to have a plan for what the final interface will look like.
Right now, we can't change QEMU to pass tables via the existing fw_cfg entries unless we upgrade SeaBIOS in QEMU first (otherwise things like duplicate MADT entries occur). If we're going to upgrade SeaBIOS in QEMU, we really want that upgrade to be the final version. We don't want to have to bump the seabios rev in qemu multiple times in lock-step with the changes.
So, I see two ways to do this:
1 - update SeaBIOS with a patch series that makes it capable of handling all tables via existing and new fw_cfg entries (including flags to disable all internal tables), update QEMU to use that SeaBIOS rev, and then apply a patch series to QEMU that has it create all the tables (with the final patch flagging to seabios that it should no longer create any internal tables).
or, 2 - apply a patch series to QEMU that has it create all the tables via new fw_cfg entries, then apply a patch series to seabios that updates it to use the new fw_cfg entries instead of its existing internal tables, and then apply that new rev of seabios to qemu.
Were you proposing one of the above paths, or did you have something else in mind?
-Kevin
Exactly, I'm proposing 1, on top of this, a set of compile-time flags to optionally stip unused tables from the bios binary.
On Sun, Mar 24, 2013 at 03:07:40PM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 21, 2013 at 08:04:38PM -0400, Kevin O'Connor wrote:
So, I see two ways to do this:
1 - update SeaBIOS with a patch series that makes it capable of handling all tables via existing and new fw_cfg entries (including flags to disable all internal tables), update QEMU to use that SeaBIOS rev, and then apply a patch series to QEMU that has it create all the tables (with the final patch flagging to seabios that it should no longer create any internal tables).
or, 2 - apply a patch series to QEMU that has it create all the tables via new fw_cfg entries, then apply a patch series to seabios that updates it to use the new fw_cfg entries instead of its existing internal tables, and then apply that new rev of seabios to qemu.
Were you proposing one of the above paths, or did you have something else in mind?
Exactly, I'm proposing 1, on top of this, a set of compile-time flags to optionally stip unused tables from the bios binary.
What do you think about using approach 2 as I outline at: http://www.seabios.org/pipermail/seabios/2013-March/006020.html ?
The existing fw_cfg acpi table passing mechanism is pretty hokey, and new fw_cfg entries will be needed for smbios, pir, and mptable anyway. Converting to all new fw_cfg tables may make the transition simpler and provide both forward and backwards compatibility.
-Kevin
On Sun, Mar 24, 2013 at 01:17:38PM -0400, Kevin O'Connor wrote:
On Sun, Mar 24, 2013 at 03:07:40PM +0200, Michael S. Tsirkin wrote:
On Thu, Mar 21, 2013 at 08:04:38PM -0400, Kevin O'Connor wrote:
So, I see two ways to do this:
1 - update SeaBIOS with a patch series that makes it capable of handling all tables via existing and new fw_cfg entries (including flags to disable all internal tables), update QEMU to use that SeaBIOS rev, and then apply a patch series to QEMU that has it create all the tables (with the final patch flagging to seabios that it should no longer create any internal tables).
or, 2 - apply a patch series to QEMU that has it create all the tables via new fw_cfg entries, then apply a patch series to seabios that updates it to use the new fw_cfg entries instead of its existing internal tables, and then apply that new rev of seabios to qemu.
Were you proposing one of the above paths, or did you have something else in mind?
Exactly, I'm proposing 1, on top of this, a set of compile-time flags to optionally stip unused tables from the bios binary.
What do you think about using approach 2 as I outline at: http://www.seabios.org/pipermail/seabios/2013-March/006020.html ?
The existing fw_cfg acpi table passing mechanism is pretty hokey, and new fw_cfg entries will be needed for smbios, pir, and mptable anyway. Converting to all new fw_cfg tables may make the transition simpler and provide both forward and backwards compatibility.
-Kevin
I don't exactly understand what do you mean by "file".
On Sun, Mar 24, 2013 at 09:14:47PM +0200, Michael S. Tsirkin wrote:
I don't exactly understand what do you mean by "file".
A fw_cfg entry added using QEMU's fw_cfg_add_file() instead of fw_cfg_add_bytes().
-Kevin
On Sun, Mar 24, 2013 at 03:21:08PM -0400, Kevin O'Connor wrote:
On Sun, Mar 24, 2013 at 09:14:47PM +0200, Michael S. Tsirkin wrote:
I don't exactly understand what do you mean by "file".
A fw_cfg entry added using QEMU's fw_cfg_add_file() instead of fw_cfg_add_bytes().
-Kevin
Looks good overall. Some further proposals:
1. I think we need runtime patching so we'll need another one that adds stuff from memory but yes it can look like a file for seabios. 2. paths exposed to seabios are relative at the moment. So "acpi/*" ?
3. add option to disable builtin tables at compile time
4. to keep the ability to develop seabios code in-tree, make it depend on compile option disabled by default.
5. similarly in QEMU, put code there disabled first, so developers can play with it.
6. when all is ready, flip the switch to enable in both places.
On Sun, Mar 24, 2013 at 09:45:54PM +0200, Michael S. Tsirkin wrote:
On Sun, Mar 24, 2013 at 03:21:08PM -0400, Kevin O'Connor wrote:
On Sun, Mar 24, 2013 at 09:14:47PM +0200, Michael S. Tsirkin wrote:
I don't exactly understand what do you mean by "file".
A fw_cfg entry added using QEMU's fw_cfg_add_file() instead of fw_cfg_add_bytes().
Looks good overall. Some further proposals:
- I think we need runtime patching so we'll need another one that adds stuff from memory but yes it can look like a file for seabios.
- paths exposed to seabios are relative at the moment. So "acpi/*" ?
fw_cfg_add_file() does read from memory. It's not tied to the host filesystem or guest filesystem.
- add option to disable builtin tables at compile time
Agreed.
to keep the ability to develop seabios code in-tree, make it depend on compile option disabled by default.
similarly in QEMU, put code there disabled first, so developers can play with it.
when all is ready, flip the switch to enable in both places.
Sure - we can do that if needed.
-Kevin
On 03/24/13 20:14, Michael S. Tsirkin wrote:
On Sun, Mar 24, 2013 at 01:17:38PM -0400, Kevin O'Connor wrote:
What do you think about using approach 2 as I outline at: http://www.seabios.org/pipermail/seabios/2013-March/006020.html ?
The existing fw_cfg acpi table passing mechanism is pretty hokey, and new fw_cfg entries will be needed for smbios, pir, and mptable anyway. Converting to all new fw_cfg tables may make the transition simpler and provide both forward and backwards compatibility.
I don't exactly understand what do you mean by "file".
Probably a key in the [FW_CFG_FILE_FIRST, FW_CFG_FILE_FIRST + FW_CFG_FILE_SLOTS - 1] range, with an associated entry (name) in the FWCfgFiles array (FW_CFG_FILE_DIR).
Laszlo
On Sun, Mar 24, 2013 at 05:55:22PM -0400, Kevin O'Connor wrote:
On Sun, Mar 24, 2013 at 09:45:54PM +0200, Michael S. Tsirkin wrote:
On Sun, Mar 24, 2013 at 03:21:08PM -0400, Kevin O'Connor wrote:
On Sun, Mar 24, 2013 at 09:14:47PM +0200, Michael S. Tsirkin wrote:
I don't exactly understand what do you mean by "file".
A fw_cfg entry added using QEMU's fw_cfg_add_file() instead of fw_cfg_add_bytes().
Looks good overall. Some further proposals:
- I think we need runtime patching so we'll need another one that adds stuff from memory but yes it can look like a file for seabios.
- paths exposed to seabios are relative at the moment. So "acpi/*" ?
fw_cfg_add_file() does read from memory. It's not tied to the host filesystem or guest filesystem.
- add option to disable builtin tables at compile time
Agreed.
to keep the ability to develop seabios code in-tree, make it depend on compile option disabled by default.
similarly in QEMU, put code there disabled first, so developers can play with it.
when all is ready, flip the switch to enable in both places.
Sure - we can do that if needed.
-Kevin
Okay I'm pretty close to posting some patches that advance this project further, but wanted to check something beforehand: there are several tables that point to other tables (for example: FADT points to DSDT). What I did is provide a list of fixups such that bios can patch in pointers without any need to understand what's what. Thoughts?
On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
Okay I'm pretty close to posting some patches that advance this project further, but wanted to check something beforehand: there are several tables that point to other tables (for example: FADT points to DSDT). What I did is provide a list of fixups such that bios can patch in pointers without any need to understand what's what. Thoughts?
Great!
For the RSDP, RSDT, and FADT I think SeaBIOS should just update those tables to set the pointers within them and then recalculate the checksum. I don't think anything complex is needed - it's easy for SeaBIOS to recognize those special tables and modify them.
-Kevin
On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
Okay I'm pretty close to posting some patches that advance this project further, but wanted to check something beforehand: there are several tables that point to other tables (for example: FADT points to DSDT). What I did is provide a list of fixups such that bios can patch in pointers without any need to understand what's what. Thoughts?
Great!
For the RSDP, RSDT, and FADT I think SeaBIOS should just update those tables to set the pointers within them and then recalculate the checksum. I don't think anything complex is needed - it's easy for SeaBIOS to recognize those special tables and modify them.
-Kevin
True, that's simple enough. My worry is we can add more such tables. For example, we can decide to switch to XSDT in the future. If you look at the latest ACPI spec, there are tons of pointer tables not implemented in seabios. If we need to add support in seabios as well as qemu for each of them, this will kind of defeat the purpose.
On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
Okay I'm pretty close to posting some patches that advance this project further, but wanted to check something beforehand: there are several tables that point to other tables (for example: FADT points to DSDT). What I did is provide a list of fixups such that bios can patch in pointers without any need to understand what's what. Thoughts?
Great!
For the RSDP, RSDT, and FADT I think SeaBIOS should just update those tables to set the pointers within them and then recalculate the checksum. I don't think anything complex is needed - it's easy for SeaBIOS to recognize those special tables and modify them.
-Kevin
True, that's simple enough. My worry is we can add more such tables. For example, we can decide to switch to XSDT in the future. If you look at the latest ACPI spec, there are tons of pointer tables not implemented in seabios. If we need to add support in seabios as well as qemu for each of them, this will kind of defeat the purpose.
And just to show how simple it can be, here's that patch in the series: basically ACPI tables will be in-memory, so all we need to do to link them together is keep a pointer from file to the table. Basically all linker needs is ability to add or subtract. What I like with this approach is host is in complete control of table layout. All BIOS does is allocate memory for them, it never changes the tables, including the checksum. Note that it's coded very defensively, which makes it a bit bigger than absolutely necessary, but hopefully more future proof.
---> linker: utility to patch in-memory ROM files
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.
Signed-off-by: Michael S. Tsirkin mst@redhat.com
---
Makefile | 2 - src/linker.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/linker.h | 28 +++++++++++++++++++++ src/util.h | 1 4 files changed, 108 insertions(+), 1 deletion(-)
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..5009f88 --- /dev/null +++ b/src/linker.c @@ -0,0 +1,78 @@ +#include "linker.h" +#include "byteorder.h" // le64_to_cpu + +void linker_link(const char *name) +{ + struct linker_entry_s *entry; + int offset = 0; + int size, lsrc; + void *data = romfile_loadfile(name, &size); + struct romfile_s *src, *dst; + u32 src_offset, dst_offset; + u64 val, buf; + if (!data) + return; + + for (offset = 0; offset < size; offset += entry->size) { + entry = data + offset; + /* Entry must have some data. If not - skip it. */ + if (entry->size <= sizeof *entry) + continue; + /* Check that entry fits in buffer. */ + if (offset + entry->size > size) { + warn_internalerror(); + break; + } + /* Skip any command that we don't recognize. */ + if (entry->reserved) + continue; + if (entry->bytes != 1 && + entry->bytes != 2 && + entry->bytes != 4 && + entry->bytes != 8) + continue; + if (entry->type != LINKER_ENTRY_TYPE_ADD && + entry->type != LINKER_ENTRY_TYPE_SUB) + continue; + if (entry->format != LINKER_ENTRY_FORMAT_LE) + continue; + /* Last byte must be 0 */ + if (entry->src_dst[entry->size - 1]) { + warn_internalerror(); + continue; + } + lsrc = strlen(entry->src_dst); + if (!lsrc || lsrc + 1 + sizeof *entry >= entry->size) { + warn_internalerror(); + continue; + } + src = romfile_find(entry->src_dst); + dst = romfile_find(entry->src_dst + lsrc + 1); + if (!src || !dst) { + warn_internalerror(); + continue; + } + if (!src->data || !dst->data) { + warn_internalerror(); + continue; + } + src_offset = le32_to_cpu(entry->src_offset); + dst_offset = le32_to_cpu(entry->dst_offset); + if (dst->size < dst_offset + entry->bytes) { + warn_internalerror(); + continue; + } + buf = 0; + memcpy(&buf, dst->data + dst_offset, entry->bytes); + val = ((unsigned long)src->data) + src_offset; + buf = le64_to_cpu(buf); + if (entry->type == LINKER_ENTRY_TYPE_ADD) + buf += val; + else + buf -= val; + buf = cpu_to_le64(val); + memcpy(dst->data + dst_offset, &buf, entry->bytes); + } + + free(data); +} diff --git a/src/linker.h b/src/linker.h new file mode 100644 index 0000000..e1d55ea --- /dev/null +++ b/src/linker.h @@ -0,0 +1,28 @@ +#include "types.h" // u8 +#include "util.h" // romfile_s + +/* ROM file linker interface. Linker uses little endian format */ +struct linker_entry_s { + u8 size; /* size in bytes including the header */ + u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */ + u8 type; + u8 format; + u32 reserved; + u64 src_offset; /* Offset in source. Little endian format. */ + u64 dst_offset; /* Offset in destination. Little endian format. */ + /* Followed by source and destination, optionally padded by + * 0, up to the total of entry_len - 4 bytes. + * Strings are 0-terminated. */ + char src_dst[]; +} PACKED; + +enum linker_entry_type { + LINKER_ENTRY_TYPE_ADD = 0x0, + LINKER_ENTRY_TYPE_SUB = 0x1, +}; + +enum linker_entry_format { + LINKER_ENTRY_FORMAT_LE = 0x0, +}; + +void linker_link(const char *name); 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);
On Mon, Apr 22, 2013 at 06:37:32PM +0300, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
Okay I'm pretty close to posting some patches that advance this project further, but wanted to check something beforehand: there are several tables that point to other tables (for example: FADT points to DSDT). What I did is provide a list of fixups such that bios can patch in pointers without any need to understand what's what. Thoughts?
Great!
For the RSDP, RSDT, and FADT I think SeaBIOS should just update those tables to set the pointers within them and then recalculate the checksum. I don't think anything complex is needed - it's easy for SeaBIOS to recognize those special tables and modify them.
-Kevin
True, that's simple enough. My worry is we can add more such tables. For example, we can decide to switch to XSDT in the future. If you look at the latest ACPI spec, there are tons of pointer tables not implemented in seabios. If we need to add support in seabios as well as qemu for each of them, this will kind of defeat the purpose.
And just to show how simple it can be, here's that patch in the series: basically ACPI tables will be in-memory, so all we need to do to link them together is keep a pointer from file to the table. Basically all linker needs is ability to add or subtract. What I like with this approach is host is in complete control of table layout. All BIOS does is allocate memory for them, it never changes the tables, including the checksum. Note that it's coded very defensively, which makes it a bit bigger than absolutely necessary, but hopefully more future proof.
Addressing some comments by Laszlo (sent off list)/.
---> linker: utility to patch in-memory ROM files
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.
Signed-off-by: Michael S. Tsirkin mst@redhat.com
Makefile | 2 - src/linker.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/linker.h | 28 +++++++++++++++++++++ src/util.h | 1 4 files changed, 108 insertions(+), 1 deletion(-)
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..5009f88 --- /dev/null +++ b/src/linker.c @@ -0,0 +1,78 @@ +#include "linker.h" +#include "byteorder.h" // le64_to_cpu
+void linker_link(const char *name) +{
- struct linker_entry_s *entry;
- int offset = 0;
- int size, lsrc;
- void *data = romfile_loadfile(name, &size);
- struct romfile_s *src, *dst;
- u32 src_offset, dst_offset;
- u64 val, buf;
- if (!data)
return;- for (offset = 0; offset < size; offset += entry->size) {
entry = data + offset;/* Entry must have some data. If not - skip it. */if (entry->size <= sizeof *entry)continue;/* Check that entry fits in buffer. */if (offset + entry->size > size) {warn_internalerror();break;}/* Skip any command that we don't recognize. */if (entry->reserved)continue;if (entry->bytes != 1 &&entry->bytes != 2 &&entry->bytes != 4 &&entry->bytes != 8)continue;if (entry->type != LINKER_ENTRY_TYPE_ADD &&entry->type != LINKER_ENTRY_TYPE_SUB)continue;if (entry->format != LINKER_ENTRY_FORMAT_LE)continue;/* Last byte must be 0 */if (entry->src_dst[entry->size - 1]) {warn_internalerror();continue;}lsrc = strlen(entry->src_dst);if (!lsrc || lsrc + 1 + sizeof *entry >= entry->size) {warn_internalerror();continue;}src = romfile_find(entry->src_dst);dst = romfile_find(entry->src_dst + lsrc + 1);if (!src || !dst) {warn_internalerror();continue;}if (!src->data || !dst->data) {warn_internalerror();continue;}src_offset = le32_to_cpu(entry->src_offset);dst_offset = le32_to_cpu(entry->dst_offset);if (dst->size < dst_offset + entry->bytes) {warn_internalerror();continue;}- buf = 0;
- memcpy(&buf, dst->data + dst_offset, entry->bytes);
val = ((unsigned long)src->data) + src_offset;- buf = le64_to_cpu(buf);
if (entry->type == LINKER_ENTRY_TYPE_ADD)buf += val;elsebuf -= val;- buf = cpu_to_le64(val);
- memcpy(dst->data + dst_offset, &buf, entry->bytes);
- }
- free(data);
+} diff --git a/src/linker.h b/src/linker.h new file mode 100644 index 0000000..e1d55ea --- /dev/null +++ b/src/linker.h @@ -0,0 +1,28 @@ +#include "types.h" // u8 +#include "util.h" // romfile_s
+/* ROM file linker interface. Linker uses little endian format */ +struct linker_entry_s {
- u8 size; /* size in bytes including the header */
- u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
- u8 type;
- u8 format;
- u32 reserved;
- u64 src_offset; /* Offset in source. Little endian format. */
src_offset is not needed: host can pre-fill destination table with the offset value. Will probably drop it.
- u64 dst_offset; /* Offset in destination. Little endian format. */
- /* Followed by source and destination
Add "file names"
, optionally padded by
* 0, up to the total of entry_len - 4 bytes.* Strings are 0-terminated. */- char src_dst[];
+} PACKED;
+enum linker_entry_type {
Documentation:
ADD: increment value in DST by the address of SRC useful e.g. to fill in pointer values in ACPI tables SUB: decrement value in DST by the address of SRC useful e.g. to fix up checksum values in ACPI tables
- LINKER_ENTRY_TYPE_ADD = 0x0,
- LINKER_ENTRY_TYPE_SUB = 0x1,
+};
+enum linker_entry_format {
- LINKER_ENTRY_FORMAT_LE = 0x0,
+};
Documentation:
/* * Load linker script and execute it. * All linked files referenced by the script * must already be in memory. */
+void linker_link(const char *name); 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);
On 04/22/13 17:37, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
Okay I'm pretty close to posting some patches that advance this project further, but wanted to check something beforehand: there are several tables that point to other tables (for example: FADT points to DSDT). What I did is provide a list of fixups such that bios can patch in pointers without any need to understand what's what. Thoughts?
Great!
For the RSDP, RSDT, and FADT I think SeaBIOS should just update those tables to set the pointers within them and then recalculate the checksum. I don't think anything complex is needed - it's easy for SeaBIOS to recognize those special tables and modify them.
-Kevin
True, that's simple enough. My worry is we can add more such tables. For example, we can decide to switch to XSDT in the future. If you look at the latest ACPI spec, there are tons of pointer tables not implemented in seabios. If we need to add support in seabios as well as qemu for each of them, this will kind of defeat the purpose.
And just to show how simple it can be, here's that patch in the series: basically ACPI tables will be in-memory, so all we need to do to link them together is keep a pointer from file to the table. Basically all linker needs is ability to add or subtract. What I like with this approach is host is in complete control of table layout. All BIOS does is allocate memory for them, it never changes the tables, including the checksum. Note that it's coded very defensively, which makes it a bit bigger than absolutely necessary, but hopefully more future proof.
---> linker: utility to patch in-memory ROM files
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.
Signed-off-by: Michael S. Tsirkin mst@redhat.com
Makefile | 2 - src/linker.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/linker.h | 28 +++++++++++++++++++++ src/util.h | 1 4 files changed, 108 insertions(+), 1 deletion(-)
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..5009f88 --- /dev/null +++ b/src/linker.c @@ -0,0 +1,78 @@ +#include "linker.h" +#include "byteorder.h" // le64_to_cpu
+void linker_link(const char *name) +{
- struct linker_entry_s *entry;
- int offset = 0;
- int size, lsrc;
- void *data = romfile_loadfile(name, &size);
- struct romfile_s *src, *dst;
- u32 src_offset, dst_offset;
- u64 val, buf;
- if (!data)
return;- for (offset = 0; offset < size; offset += entry->size) {
entry = data + offset;/* Entry must have some data. If not - skip it. */if (entry->size <= sizeof *entry)continue;/* Check that entry fits in buffer. */if (offset + entry->size > size) {warn_internalerror();break;}/* Skip any command that we don't recognize. */if (entry->reserved)continue;if (entry->bytes != 1 &&entry->bytes != 2 &&entry->bytes != 4 &&entry->bytes != 8)continue;if (entry->type != LINKER_ENTRY_TYPE_ADD &&entry->type != LINKER_ENTRY_TYPE_SUB)continue;if (entry->format != LINKER_ENTRY_FORMAT_LE)continue;/* Last byte must be 0 */if (entry->src_dst[entry->size - 1]) {warn_internalerror();continue;}lsrc = strlen(entry->src_dst);if (!lsrc || lsrc + 1 + sizeof *entry >= entry->size) {warn_internalerror();continue;}src = romfile_find(entry->src_dst);dst = romfile_find(entry->src_dst + lsrc + 1);if (!src || !dst) {warn_internalerror();continue;}if (!src->data || !dst->data) {warn_internalerror();continue;}src_offset = le32_to_cpu(entry->src_offset);dst_offset = le32_to_cpu(entry->dst_offset);if (dst->size < dst_offset + entry->bytes) {warn_internalerror();continue;}- buf = 0;
- memcpy(&buf, dst->data + dst_offset, entry->bytes);
val = ((unsigned long)src->data) + src_offset;- buf = le64_to_cpu(buf);
if (entry->type == LINKER_ENTRY_TYPE_ADD)buf += val;elsebuf -= val;- buf = cpu_to_le64(val);
- memcpy(dst->data + dst_offset, &buf, entry->bytes);
- }
- free(data);
+} diff --git a/src/linker.h b/src/linker.h new file mode 100644 index 0000000..e1d55ea --- /dev/null +++ b/src/linker.h @@ -0,0 +1,28 @@ +#include "types.h" // u8 +#include "util.h" // romfile_s
+/* ROM file linker interface. Linker uses little endian format */ +struct linker_entry_s {
- u8 size; /* size in bytes including the header */
- u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
- u8 type;
- u8 format;
- u32 reserved;
- u64 src_offset; /* Offset in source. Little endian format. */
- u64 dst_offset; /* Offset in destination. Little endian format. */
- /* Followed by source and destination, optionally padded by
* 0, up to the total of entry_len - 4 bytes.* Strings are 0-terminated. */- char src_dst[];
+} PACKED;
+enum linker_entry_type {
- LINKER_ENTRY_TYPE_ADD = 0x0,
- LINKER_ENTRY_TYPE_SUB = 0x1,
+};
+enum linker_entry_format {
- LINKER_ENTRY_FORMAT_LE = 0x0,
+};
+void linker_link(const char *name); 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);
After discussing this with Michael on IRC: I like the general idea, but more comments / handholding / intended use case would helpful for the reader.
Hopefully I'll find the time to "formally" review this (or its next version) sometime.
Thanks Laszlo
On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
Okay I'm pretty close to posting some patches that advance this project further, but wanted to check something beforehand: there are several tables that point to other tables (for example: FADT points to DSDT). What I did is provide a list of fixups such that bios can patch in pointers without any need to understand what's what. Thoughts?
For the RSDP, RSDT, and FADT I think SeaBIOS should just update those tables to set the pointers within them and then recalculate the checksum. I don't think anything complex is needed - it's easy for SeaBIOS to recognize those special tables and modify them.
True, that's simple enough. My worry is we can add more such tables. For example, we can decide to switch to XSDT in the future.
I know of the following quirks that would have to be handled:
1 - the RSDP must be in the f-segment (where as all other tables can go into "high" memory).
2 - the RSDP has a checksum in a different location from the other tables and (with an XSDT) it can have two checksums.
3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
4 - the RSDT (and XSDT if present) has pointers to all the other tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be first in the list.
5 - the FADT table has pointers to DSDT and FACS.
6 - the FACS table must be 64 byte aligned.
So, will a generic scheme really be able to handle all of the above quirks, or will we just be mixing some hardcoded quirks with some generic quirks? And, will the code to handle the above quirks in a generic fashion be of a higher complexity than simply hard-coding it?
-Kevin
On Mon, Apr 22, 2013 at 08:23:39PM +0300, Michael S. Tsirkin wrote:
@@ -0,0 +1,28 @@ +#include "types.h" // u8 +#include "util.h" // romfile_s
+/* ROM file linker interface. Linker uses little endian format */ +struct linker_entry_s {
- u8 size; /* size in bytes including the header */
- u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
- u8 type;
- u8 format;
- u32 reserved;
- u64 src_offset; /* Offset in source. Little endian format. */
src_offset is not needed: host can pre-fill destination table with the offset value. Will probably drop it.
- u64 dst_offset; /* Offset in destination. Little endian format. */
- /* Followed by source and destination
Add "file names"
, optionally padded by
* 0, up to the total of entry_len - 4 bytes.* Strings are 0-terminated. */- char src_dst[];
+} PACKED;
+enum linker_entry_type {
Documentation:
ADD: increment value in DST by the address of SRC useful e.g. to fill in pointer values in ACPI tables SUB: decrement value in DST by the address of SRC useful e.g. to fix up checksum values in ACPI tables
I don't see how one could implement a checksum with just a subtraction. If the table is at 0x12345678 the checksum isn't (oldcksum-0x12345678), it's (oldcksum-0x12-0x34-0x56-0x78).
-Kevin
On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
Okay I'm pretty close to posting some patches that advance this project further, but wanted to check something beforehand: there are several tables that point to other tables (for example: FADT points to DSDT). What I did is provide a list of fixups such that bios can patch in pointers without any need to understand what's what. Thoughts?
For the RSDP, RSDT, and FADT I think SeaBIOS should just update those tables to set the pointers within them and then recalculate the checksum. I don't think anything complex is needed - it's easy for SeaBIOS to recognize those special tables and modify them.
True, that's simple enough. My worry is we can add more such tables. For example, we can decide to switch to XSDT in the future.
I know of the following quirks that would have to be handled:
1 - the RSDP must be in the f-segment (where as all other tables can go into "high" memory).
2 - the RSDP has a checksum in a different location from the other tables and (with an XSDT) it can have two checksums.
3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
4 - the RSDT (and XSDT if present) has pointers to all the other tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be first in the list.
5 - the FADT table has pointers to DSDT and FACS.
6 - the FACS table must be 64 byte aligned.
And of course newer ACPI has lots of other pointer quirks, I assume you are aware of this.
So, will a generic scheme really be able to handle all of the above quirks, or will we just be mixing some hardcoded quirks with some generic quirks? And, will the code to handle the above quirks in a generic fashion be of a higher complexity than simply hard-coding it?
-Kevin
I wanted to handle checksums and pointers in a generic fashion, and allocation rules in a table specific version (since I only found two such examples in all of the spec: FACS and RSDP). It's not hard to add generic handlers for these two, and it's not much more code. You think it's preferable then?
On Mon, Apr 22, 2013 at 08:42:18PM -0400, Kevin O'Connor wrote:
On Mon, Apr 22, 2013 at 08:23:39PM +0300, Michael S. Tsirkin wrote:
@@ -0,0 +1,28 @@ +#include "types.h" // u8 +#include "util.h" // romfile_s
+/* ROM file linker interface. Linker uses little endian format */ +struct linker_entry_s {
- u8 size; /* size in bytes including the header */
- u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
- u8 type;
- u8 format;
- u32 reserved;
- u64 src_offset; /* Offset in source. Little endian format. */
src_offset is not needed: host can pre-fill destination table with the offset value. Will probably drop it.
- u64 dst_offset; /* Offset in destination. Little endian format. */
- /* Followed by source and destination
Add "file names"
, optionally padded by
* 0, up to the total of entry_len - 4 bytes.* Strings are 0-terminated. */- char src_dst[];
+} PACKED;
+enum linker_entry_type {
Documentation:
ADD: increment value in DST by the address of SRC useful e.g. to fill in pointer values in ACPI tables SUB: decrement value in DST by the address of SRC useful e.g. to fix up checksum values in ACPI tables
I don't see how one could implement a checksum with just a subtraction. If the table is at 0x12345678 the checksum isn't (oldcksum-0x12345678), it's (oldcksum-0x12-0x34-0x56-0x78).
-Kevin
True, it's a bug. I'll add a shift option to fix it.
On Mon, Apr 22, 2013 at 08:42:18PM -0400, Kevin O'Connor wrote:
On Mon, Apr 22, 2013 at 08:23:39PM +0300, Michael S. Tsirkin wrote:
@@ -0,0 +1,28 @@ +#include "types.h" // u8 +#include "util.h" // romfile_s
+/* ROM file linker interface. Linker uses little endian format */ +struct linker_entry_s {
- u8 size; /* size in bytes including the header */
- u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
- u8 type;
- u8 format;
- u32 reserved;
- u64 src_offset; /* Offset in source. Little endian format. */
src_offset is not needed: host can pre-fill destination table with the offset value. Will probably drop it.
- u64 dst_offset; /* Offset in destination. Little endian format. */
- /* Followed by source and destination
Add "file names"
, optionally padded by
* 0, up to the total of entry_len - 4 bytes.* Strings are 0-terminated. */- char src_dst[];
+} PACKED;
+enum linker_entry_type {
Documentation:
ADD: increment value in DST by the address of SRC useful e.g. to fill in pointer values in ACPI tables SUB: decrement value in DST by the address of SRC useful e.g. to fix up checksum values in ACPI tables
I don't see how one could implement a checksum with just a subtraction. If the table is at 0x12345678 the checksum isn't (oldcksum-0x12345678), it's (oldcksum-0x12-0x34-0x56-0x78).
-Kevin
So here's the version with shift, that should do it correctly. Add mere 8 lines of code. I'll look into alignment and fseg now.
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..a1f473d --- /dev/null +++ b/src/linker.c @@ -0,0 +1,79 @@ +#include "linker.h" +#include "byteorder.h" // le64_to_cpu + +void linker_link(const char *name) +{ + struct linker_entry_s *entry; + int offset = 0; + int size, lsrc; + void *data = romfile_loadfile(name, &size); + struct romfile_s *src, *dst; + u32 dst_offset; + u64 val, buf; + if (!data) + return; + + for (offset = 0; offset < size; offset += entry->size) { + entry = data + offset; + /* Entry must have some data. If not - skip it. */ + if (entry->size <= sizeof *entry) + continue; + /* Check that entry fits in buffer. */ + if (offset + entry->size > size) { + warn_internalerror(); + break; + } + /* Skip any command that we don't recognize. */ + if (entry->reserved1 || entry->reserved2) + continue; + if (entry->bytes != 1 && + entry->bytes != 2 && + entry->bytes != 4 && + entry->bytes != 8) + continue; + if (entry->shift > 63) + continue; + if (entry->type != LINKER_ENTRY_TYPE_ADD && + entry->type != LINKER_ENTRY_TYPE_SUB) + continue; + if (entry->format != LINKER_ENTRY_FORMAT_LE) + continue; + /* Last byte must be 0 */ + if (entry->src_dst[entry->size - 1]) { + warn_internalerror(); + continue; + } + lsrc = strlen(entry->src_dst); + if (!lsrc || lsrc + 1 + sizeof *entry >= entry->size) { + warn_internalerror(); + continue; + } + src = romfile_find(entry->src_dst); + dst = romfile_find(entry->src_dst + lsrc + 1); + if (!src || !dst) { + warn_internalerror(); + continue; + } + if (!src->data || !dst->data) { + warn_internalerror(); + continue; + } + dst_offset = le32_to_cpu(entry->dst_offset); + if (dst->size < dst_offset + entry->bytes) { + warn_internalerror(); + continue; + } + buf = 0; + memcpy(&buf, dst->data + dst_offset, entry->bytes); + val = ((u64)(unsigned long)src->data) >> entry->shift; + buf = le64_to_cpu(buf); + if (entry->type == LINKER_ENTRY_TYPE_ADD) + buf += val; + else + buf -= val; + buf = cpu_to_le64(val); + memcpy(dst->data + dst_offset, &buf, entry->bytes); + } + + free(data); +} diff --git a/src/linker.h b/src/linker.h new file mode 100644 index 0000000..2bb376d --- /dev/null +++ b/src/linker.h @@ -0,0 +1,29 @@ +#include "types.h" // u8 +#include "util.h" // romfile_s + +/* ROM file linker interface. Linker uses little endian format */ +struct linker_entry_s { + u8 size; /* size in bytes including the header */ + u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */ + u8 shift; /* Shift source address right by this many bits. 0-63. */ + u8 type; + u8 format; + u8 reserved1; + u16 reserved2; + u64 dst_offset; /* Offset in destination. Little endian format. */ + /* Followed by source and destination, optionally padded by + * 0, up to the total of entry_len - 4 bytes. + * Strings are 0-terminated. */ + char src_dst[]; +} PACKED; + +enum linker_entry_type { + LINKER_ENTRY_TYPE_ADD = 0x0, + LINKER_ENTRY_TYPE_SUB = 0x1, +}; + +enum linker_entry_format { + LINKER_ENTRY_FORMAT_LE = 0x0, +}; + +void linker_link(const char *name); 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);
On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
Okay I'm pretty close to posting some patches that advance this project further, but wanted to check something beforehand: there are several tables that point to other tables (for example: FADT points to DSDT). What I did is provide a list of fixups such that bios can patch in pointers without any need to understand what's what. Thoughts?
For the RSDP, RSDT, and FADT I think SeaBIOS should just update those tables to set the pointers within them and then recalculate the checksum. I don't think anything complex is needed - it's easy for SeaBIOS to recognize those special tables and modify them.
True, that's simple enough. My worry is we can add more such tables. For example, we can decide to switch to XSDT in the future.
I know of the following quirks that would have to be handled:
1 - the RSDP must be in the f-segment (where as all other tables can go into "high" memory).
2 - the RSDP has a checksum in a different location from the other tables and (with an XSDT) it can have two checksums.
3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
4 - the RSDT (and XSDT if present) has pointers to all the other tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be first in the list.
5 - the FADT table has pointers to DSDT and FACS.
6 - the FACS table must be 64 byte aligned.
So, will a generic scheme really be able to handle all of the above quirks, or will we just be mixing some hardcoded quirks with some generic quirks? And, will the code to handle the above quirks in a generic fashion be of a higher complexity than simply hard-coding it?
-Kevin
-->
So here's an implementation for align and FSEG. Not a big deal as you see.
I really have doubts about it however: BIOS still must be able to parse get the resume vector in FACS in order to support wakeup, right? So this means that we need to be able to parse RSDP and FACT. These happen to be the only things that need anything not addressed by ADD and SUB so ... maybe a couple of hardcoded quirks just to allocate these correctly is cleaner.
Signed-off-by: Michael S. Tsirkin mst@redhat.com
---
diff --git a/src/linker.c b/src/linker.c index a1f473d..22a0dff 100644 --- a/src/linker.c +++ b/src/linker.c @@ -34,7 +34,9 @@ void linker_link(const char *name) if (entry->shift > 63) continue; if (entry->type != LINKER_ENTRY_TYPE_ADD && - entry->type != LINKER_ENTRY_TYPE_SUB) + entry->type != LINKER_ENTRY_TYPE_SUB && + entry->type != LINKER_ENTRY_TYPE_ALIGN && + entry->type != LINKER_ENTRY_TYPE_FSEG) continue; if (entry->format != LINKER_ENTRY_FORMAT_LE) continue; @@ -44,17 +46,55 @@ void linker_link(const char *name) continue; } lsrc = strlen(entry->src_dst); - if (!lsrc || lsrc + 1 + sizeof *entry >= entry->size) { + if (!lsrc) { warn_internalerror(); continue; } src = romfile_find(entry->src_dst); + if (!src) { + warn_internalerror(); + continue; + } + if (!src->data) { + warn_internalerror(); + continue; + } + if (entry->type == LINKER_ENTRY_TYPE_ALIGN || + entry->type == LINKER_ENTRY_TYPE_FSEG) { + void *data; + u32 align; + struct zone_s *zone; + + if (entry->shift > 31) { + warn_internalerror(); + continue; + } + align = 0x1 << entry->shift; + if (align < MALLOC_MIN_ALIGN) + align = MALLOC_MIN_ALIGN; + zone = entry->type == LINKER_ENTRY_TYPE_FSEG ? + &ZoneFSeg : &ZoneHigh; + data = pmm_malloc(zone, PMM_DEFAULT_HANDLE, src->size, align); + if (!data) { + warn_internalerror(); + continue; + } + memcpy(data, src->data, src->size); + free(src->data); + src->data = data; + continue; + } + + if (lsrc + 1 + sizeof *entry >= entry->size) { + warn_internalerror(); + continue; + } dst = romfile_find(entry->src_dst + lsrc + 1); - if (!src || !dst) { + if (!dst) { warn_internalerror(); continue; } - if (!src->data || !dst->data) { + if (!dst->data) { warn_internalerror(); continue; } diff --git a/src/linker.h b/src/linker.h index 2bb376d..b0cc536 100644 --- a/src/linker.h +++ b/src/linker.h @@ -17,9 +17,12 @@ struct linker_entry_s { char src_dst[]; } PACKED;
+/* Note: align types must appear before all other types. */ enum linker_entry_type { LINKER_ENTRY_TYPE_ADD = 0x0, LINKER_ENTRY_TYPE_SUB = 0x1, + LINKER_ENTRY_TYPE_ALIGN = 0x2, /* Align source to shift bits. Must come first. */ + LINKER_ENTRY_TYPE_FSEG = 0x3, /* Align source to shift bits in FSEG memory. */ };
enum linker_entry_format {
On Tue, Apr 23, 2013 at 11:49:57AM +0300, Michael S. Tsirkin wrote:
I know of the following quirks that would have to be handled:
1 - the RSDP must be in the f-segment (where as all other tables can go into "high" memory).
2 - the RSDP has a checksum in a different location from the other tables and (with an XSDT) it can have two checksums.
3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
4 - the RSDT (and XSDT if present) has pointers to all the other tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be first in the list.
5 - the FADT table has pointers to DSDT and FACS.
6 - the FACS table must be 64 byte aligned.
And of course newer ACPI has lots of other pointer quirks, I assume you are aware of this.
I haven't had a chance to look through the newer acpi specs, but it's a shame more of the pointer quirks have been added.
So, will a generic scheme really be able to handle all of the above quirks, or will we just be mixing some hardcoded quirks with some generic quirks? And, will the code to handle the above quirks in a generic fashion be of a higher complexity than simply hard-coding it?
I wanted to handle checksums and pointers in a generic fashion, and allocation rules in a table specific version (since I only found two such examples in all of the spec: FACS and RSDP). It's not hard to add generic handlers for these two, and it's not much more code. You think it's preferable then?
I don't know if it's perferable or not. I guess one advantage of it is that the same mechanism could then also be used for smbios, mptable, and pir.
I think the seabios part of this will be straight forward no matter which direction is taken. The real work will be on getting the tables created in qemu.
-Kevin
On Tue, Apr 23, 2013 at 06:22:12PM +0300, Michael S. Tsirkin wrote:
So here's the version with shift, that should do it correctly. Add mere 8 lines of code. I'll look into alignment and fseg now.
[...]
--- /dev/null +++ b/src/linker.h @@ -0,0 +1,29 @@ +#include "types.h" // u8 +#include "util.h" // romfile_s
+/* ROM file linker interface. Linker uses little endian format */ +struct linker_entry_s {
- u8 size; /* size in bytes including the header */
- u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
- u8 shift; /* Shift source address right by this many bits. 0-63. */
- u8 type;
- u8 format;
- u8 reserved1;
- u16 reserved2;
- u64 dst_offset; /* Offset in destination. Little endian format. */
- /* Followed by source and destination, optionally padded by
* 0, up to the total of entry_len - 4 bytes.* Strings are 0-terminated. */- char src_dst[];
+} PACKED;
As in my previous email, I think the main emphasis should be on getting the content into QEMU, so I don't want to nitpick on the details of the seabios side. However, if I understand your proposal, an RSDT with 10 table entries would require 50 "linker" entries (10 to add the pointers and 40 to update the checksum) - that seems excessive. If the goal is to do a checksum - might as well just define a checksum action. Also, the variable length filenames aren't needed - the QemuCfgFile interface already limits the filename to a max of 56 bytes.
If you really feel QEMU should direct the pointer and checksum updates, you might want to consider something like:
struct tabledeploy_s { u32 command; union { // COMMAND_ALLOC - allocate a table from the given "file" struct { char alloc_file[FILESZ]; u32 alloc_align; u8 alloc_zone; };
// COMMAND_PATCH - patch the table (originating from // "dest_file") to a pointer to the table originating from // "src_file". struct { char patch_dest_file[FILESZ]; char patch_src_file[FILESZ]; u32 patch_offset; u8 patch_size; };
// COMMAND_CHECKSUM - Update a checksum in a table struct { char cksum_file[FILESZ]; u32 cksum_offset; u32 cksum_start; u32 cksum_length; };
// PAD char pad[124]; }; } PACKED;
-Kevin
On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
Okay I'm pretty close to posting some patches that advance this project further, but wanted to check something beforehand: there are several tables that point to other tables (for example: FADT points to DSDT). What I did is provide a list of fixups such that bios can patch in pointers without any need to understand what's what. Thoughts?
For the RSDP, RSDT, and FADT I think SeaBIOS should just update those tables to set the pointers within them and then recalculate the checksum. I don't think anything complex is needed - it's easy for SeaBIOS to recognize those special tables and modify them.
True, that's simple enough. My worry is we can add more such tables. For example, we can decide to switch to XSDT in the future.
I know of the following quirks that would have to be handled:
1 - the RSDP must be in the f-segment (where as all other tables can go into "high" memory).
2 - the RSDP has a checksum in a different location from the other tables and (with an XSDT) it can have two checksums.
3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
4 - the RSDT (and XSDT if present) has pointers to all the other tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be first in the list.
5 - the FADT table has pointers to DSDT and FACS.
6 - the FACS table must be 64 byte aligned.
So, will a generic scheme really be able to handle all of the above quirks, or will we just be mixing some hardcoded quirks with some generic quirks? And, will the code to handle the above quirks in a generic fashion be of a higher complexity than simply hard-coding it?
-Kevin
-->
So here's an implementation for align and FSEG. Not a big deal as you see.
I really have doubts about it however: BIOS still must be able to parse get the resume vector in FACS in order to support wakeup, right? So this means that we need to be able to parse RSDP and FACT. These happen to be the only things that need anything not addressed by ADD and SUB so ... maybe a couple of hardcoded quirks just to allocate these correctly is cleaner.
Heh, it's actually pretty easy: let's just ask qemu to give us the address of the resume vector in a file with a pre-defined name. Linker can patch table offset there in the regular way.
Signed-off-by: Michael S. Tsirkin mst@redhat.com
diff --git a/src/linker.c b/src/linker.c index a1f473d..22a0dff 100644 --- a/src/linker.c +++ b/src/linker.c @@ -34,7 +34,9 @@ void linker_link(const char *name) if (entry->shift > 63) continue; if (entry->type != LINKER_ENTRY_TYPE_ADD &&
entry->type != LINKER_ENTRY_TYPE_SUB)
entry->type != LINKER_ENTRY_TYPE_SUB &&entry->type != LINKER_ENTRY_TYPE_ALIGN &&entry->type != LINKER_ENTRY_TYPE_FSEG) continue; if (entry->format != LINKER_ENTRY_FORMAT_LE) continue;@@ -44,17 +46,55 @@ void linker_link(const char *name) continue; } lsrc = strlen(entry->src_dst);
if (!lsrc || lsrc + 1 + sizeof *entry >= entry->size) {
if (!lsrc) { warn_internalerror(); continue; } src = romfile_find(entry->src_dst);if (!src) {warn_internalerror();continue;}if (!src->data) {warn_internalerror();continue;}- if (entry->type == LINKER_ENTRY_TYPE_ALIGN ||
entry->type == LINKER_ENTRY_TYPE_FSEG) {void *data;u32 align;struct zone_s *zone;if (entry->shift > 31) {warn_internalerror();continue;}align = 0x1 << entry->shift;if (align < MALLOC_MIN_ALIGN)align = MALLOC_MIN_ALIGN;zone = entry->type == LINKER_ENTRY_TYPE_FSEG ?&ZoneFSeg : &ZoneHigh;data = pmm_malloc(zone, PMM_DEFAULT_HANDLE, src->size, align);if (!data) {warn_internalerror();continue;}memcpy(data, src->data, src->size);free(src->data);src->data = data;continue;- }
if (lsrc + 1 + sizeof *entry >= entry->size) {warn_internalerror();continue;} dst = romfile_find(entry->src_dst + lsrc + 1);
if (!src || !dst) {
if (!dst) { warn_internalerror(); continue; }
if (!src->data || !dst->data) {
if (!dst->data) { warn_internalerror(); continue; }diff --git a/src/linker.h b/src/linker.h index 2bb376d..b0cc536 100644 --- a/src/linker.h +++ b/src/linker.h @@ -17,9 +17,12 @@ struct linker_entry_s { char src_dst[]; } PACKED;
+/* Note: align types must appear before all other types. */ enum linker_entry_type { LINKER_ENTRY_TYPE_ADD = 0x0, LINKER_ENTRY_TYPE_SUB = 0x1,
- LINKER_ENTRY_TYPE_ALIGN = 0x2, /* Align source to shift bits. Must come first. */
- LINKER_ENTRY_TYPE_FSEG = 0x3, /* Align source to shift bits in FSEG memory. */
};
enum linker_entry_format {
On Tue, Apr 23, 2013 at 08:54:17PM -0400, Kevin O'Connor wrote:
On Tue, Apr 23, 2013 at 06:22:12PM +0300, Michael S. Tsirkin wrote:
So here's the version with shift, that should do it correctly. Add mere 8 lines of code. I'll look into alignment and fseg now.
[...]
--- /dev/null +++ b/src/linker.h @@ -0,0 +1,29 @@ +#include "types.h" // u8 +#include "util.h" // romfile_s
+/* ROM file linker interface. Linker uses little endian format */ +struct linker_entry_s {
- u8 size; /* size in bytes including the header */
- u8 bytes; /* How many bytes to change. Can be 1,2,4 or 8. */
- u8 shift; /* Shift source address right by this many bits. 0-63. */
- u8 type;
- u8 format;
- u8 reserved1;
- u16 reserved2;
- u64 dst_offset; /* Offset in destination. Little endian format. */
- /* Followed by source and destination, optionally padded by
* 0, up to the total of entry_len - 4 bytes.* Strings are 0-terminated. */- char src_dst[];
+} PACKED;
As in my previous email, I think the main emphasis should be on getting the content into QEMU, so I don't want to nitpick on the details of the seabios side. However, if I understand your proposal, an RSDT with 10 table entries would require 50 "linker" entries (10 to add the pointers and 40 to update the checksum) - that seems excessive.
Well that's not a lot of memory, is it? And it's freed immediately after we link.
If the goal is to do a checksum - might as well just define a checksum action.
I agree it makes the interface more more straight-forward to use.
Also, the variable length filenames aren't needed - the QemuCfgFile interface already limits the filename to a max of 56 bytes.
True. Two reasons I did not do it this way: - this makes the code generic. Maybe others will want to reuse this? - a bit less memory - we dont have long file names in practice. Valid concerns?
If you really feel QEMU should direct the pointer and checksum updates, you might want to consider something like:
struct tabledeploy_s { u32 command; union { // COMMAND_ALLOC - allocate a table from the given "file" struct { char alloc_file[FILESZ]; u32 alloc_align; u8 alloc_zone; };
// COMMAND_PATCH - patch the table (originating from // "dest_file") to a pointer to the table originating from // "src_file". struct { char patch_dest_file[FILESZ]; char patch_src_file[FILESZ]; u32 patch_offset; u8 patch_size; };
I think ADD is better since this way we can use not the start of table, but something in the middle. For example, a single fw_cfg can give us multiple tables, it should be up to QEMU.
It's equivalent to PATCH if the original data is 0.
// COMMAND_CHECKSUM - Update a checksum in a table struct { char cksum_file[FILESZ]; u32 cksum_offset; u32 cksum_start; u32 cksum_length; };
Same here. So I would do:
COMMAND_ADD_POINTER
COMMAND_ADD_CHECKSUM
// PAD char pad[124]; };} PACKED;
-Kevin
On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote:
Okay I'm pretty close to posting some patches that advance this project further, but wanted to check something beforehand: there are several tables that point to other tables (for example: FADT points to DSDT). What I did is provide a list of fixups such that bios can patch in pointers without any need to understand what's what. Thoughts?
For the RSDP, RSDT, and FADT I think SeaBIOS should just update those tables to set the pointers within them and then recalculate the checksum. I don't think anything complex is needed - it's easy for SeaBIOS to recognize those special tables and modify them.
True, that's simple enough. My worry is we can add more such tables. For example, we can decide to switch to XSDT in the future.
I know of the following quirks that would have to be handled:
1 - the RSDP must be in the f-segment (where as all other tables can go into "high" memory).
2 - the RSDP has a checksum in a different location from the other tables and (with an XSDT) it can have two checksums.
3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
4 - the RSDT (and XSDT if present) has pointers to all the other tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be first in the list.
5 - the FADT table has pointers to DSDT and FACS.
6 - the FACS table must be 64 byte aligned.
So, will a generic scheme really be able to handle all of the above quirks, or will we just be mixing some hardcoded quirks with some generic quirks? And, will the code to handle the above quirks in a generic fashion be of a higher complexity than simply hard-coding it?
-Kevin
-->
So here's an implementation for align and FSEG. Not a big deal as you see.
I really have doubts about it however: BIOS still must be able to parse get the resume vector in FACS in order to support wakeup, right? So this means that we need to be able to parse RSDP and FACT. These happen to be the only things that need anything not addressed by ADD and SUB so ... maybe a couple of hardcoded quirks just to allocate these correctly is cleaner.
Heh, it's actually pretty easy: let's just ask qemu to give us the address of the resume vector in a file with a pre-defined name. Linker can patch table offset there in the regular way.
Seabios can find resume vector address the same way OSPM does: by following pointers in memory. What QEMU has to do with it?
-- Gleb.
On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote:
On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote: > Okay I'm pretty close to posting some patches > that advance this project further, but wanted to > check something beforehand: there are several tables > that point to other tables (for example: FADT points > to DSDT). What I did is provide a list of fixups > such that bios can patch in pointers without > any need to understand what's what. > Thoughts?
For the RSDP, RSDT, and FADT I think SeaBIOS should just update those tables to set the pointers within them and then recalculate the checksum. I don't think anything complex is needed - it's easy for SeaBIOS to recognize those special tables and modify them.
True, that's simple enough. My worry is we can add more such tables. For example, we can decide to switch to XSDT in the future.
I know of the following quirks that would have to be handled:
1 - the RSDP must be in the f-segment (where as all other tables can go into "high" memory).
2 - the RSDP has a checksum in a different location from the other tables and (with an XSDT) it can have two checksums.
3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
4 - the RSDT (and XSDT if present) has pointers to all the other tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be first in the list.
5 - the FADT table has pointers to DSDT and FACS.
6 - the FACS table must be 64 byte aligned.
So, will a generic scheme really be able to handle all of the above quirks, or will we just be mixing some hardcoded quirks with some generic quirks? And, will the code to handle the above quirks in a generic fashion be of a higher complexity than simply hard-coding it?
-Kevin
-->
So here's an implementation for align and FSEG. Not a big deal as you see.
I really have doubts about it however: BIOS still must be able to parse get the resume vector in FACS in order to support wakeup, right? So this means that we need to be able to parse RSDP and FACT. These happen to be the only things that need anything not addressed by ADD and SUB so ... maybe a couple of hardcoded quirks just to allocate these correctly is cleaner.
Heh, it's actually pretty easy: let's just ask qemu to give us the address of the resume vector in a file with a pre-defined name. Linker can patch table offset there in the regular way.
Seabios can find resume vector address the same way OSPM does: by following pointers in memory.
Yes, that's what we do now.
What QEMU has to do with it?
The paragraphs above explain the connection.
-- Gleb.
On Tue, May 07, 2013 at 09:54:33PM +0300, Michael S. Tsirkin wrote:
On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote:
On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote: > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote: > > Okay I'm pretty close to posting some patches > > that advance this project further, but wanted to > > check something beforehand: there are several tables > > that point to other tables (for example: FADT points > > to DSDT). What I did is provide a list of fixups > > such that bios can patch in pointers without > > any need to understand what's what. > > Thoughts? > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those > tables to set the pointers within them and then recalculate the > checksum. I don't think anything complex is needed - it's easy for > SeaBIOS to recognize those special tables and modify them.
True, that's simple enough. My worry is we can add more such tables. For example, we can decide to switch to XSDT in the future.
I know of the following quirks that would have to be handled:
1 - the RSDP must be in the f-segment (where as all other tables can go into "high" memory).
2 - the RSDP has a checksum in a different location from the other tables and (with an XSDT) it can have two checksums.
3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
4 - the RSDT (and XSDT if present) has pointers to all the other tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be first in the list.
5 - the FADT table has pointers to DSDT and FACS.
6 - the FACS table must be 64 byte aligned.
So, will a generic scheme really be able to handle all of the above quirks, or will we just be mixing some hardcoded quirks with some generic quirks? And, will the code to handle the above quirks in a generic fashion be of a higher complexity than simply hard-coding it?
-Kevin
-->
So here's an implementation for align and FSEG. Not a big deal as you see.
I really have doubts about it however: BIOS still must be able to parse get the resume vector in FACS in order to support wakeup, right? So this means that we need to be able to parse RSDP and FACT. These happen to be the only things that need anything not addressed by ADD and SUB so ... maybe a couple of hardcoded quirks just to allocate these correctly is cleaner.
Heh, it's actually pretty easy: let's just ask qemu to give us the address of the resume vector in a file with a pre-defined name. Linker can patch table offset there in the regular way.
Seabios can find resume vector address the same way OSPM does: by following pointers in memory.
Yes, that's what we do now.
Good.
What QEMU has to do with it?
The paragraphs above explain the connection.
Do not see any explanation anywhere.
-- Gleb.
On Tue, May 07, 2013 at 10:26:34PM +0300, Gleb Natapov wrote:
On Tue, May 07, 2013 at 09:54:33PM +0300, Michael S. Tsirkin wrote:
On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote:
On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote: > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote: > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote: > > > Okay I'm pretty close to posting some patches > > > that advance this project further, but wanted to > > > check something beforehand: there are several tables > > > that point to other tables (for example: FADT points > > > to DSDT). What I did is provide a list of fixups > > > such that bios can patch in pointers without > > > any need to understand what's what. > > > Thoughts? > > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those > > tables to set the pointers within them and then recalculate the > > checksum. I don't think anything complex is needed - it's easy for > > SeaBIOS to recognize those special tables and modify them. > > True, that's simple enough. My worry is we can add more such tables. > For example, we can decide to switch to XSDT in the future.
I know of the following quirks that would have to be handled:
1 - the RSDP must be in the f-segment (where as all other tables can go into "high" memory).
2 - the RSDP has a checksum in a different location from the other tables and (with an XSDT) it can have two checksums.
3 - the RSDP has a pointer to the RSDT (and to the XSDT if present).
4 - the RSDT (and XSDT if present) has pointers to all the other tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be first in the list.
5 - the FADT table has pointers to DSDT and FACS.
6 - the FACS table must be 64 byte aligned.
So, will a generic scheme really be able to handle all of the above quirks, or will we just be mixing some hardcoded quirks with some generic quirks? And, will the code to handle the above quirks in a generic fashion be of a higher complexity than simply hard-coding it?
-Kevin
-->
So here's an implementation for align and FSEG. Not a big deal as you see.
I really have doubts about it however: BIOS still must be able to parse get the resume vector in FACS in order to support wakeup, right? So this means that we need to be able to parse RSDP and FACT. These happen to be the only things that need anything not addressed by ADD and SUB so ... maybe a couple of hardcoded quirks just to allocate these correctly is cleaner.
Heh, it's actually pretty easy: let's just ask qemu to give us the address of the resume vector in a file with a pre-defined name. Linker can patch table offset there in the regular way.
Seabios can find resume vector address the same way OSPM does: by following pointers in memory.
Yes, that's what we do now.
Good.
What QEMU has to do with it?
The paragraphs above explain the connection.
Do not see any explanation anywhere.
Maybe I don't understand your question then. What exactly would you like to know?
-- Gleb.
On Tue, May 07, 2013 at 10:33:17PM +0300, Michael S. Tsirkin wrote:
On Tue, May 07, 2013 at 10:26:34PM +0300, Gleb Natapov wrote:
On Tue, May 07, 2013 at 09:54:33PM +0300, Michael S. Tsirkin wrote:
On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote:
On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote: > On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote: > > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote: > > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote: > > > > Okay I'm pretty close to posting some patches > > > > that advance this project further, but wanted to > > > > check something beforehand: there are several tables > > > > that point to other tables (for example: FADT points > > > > to DSDT). What I did is provide a list of fixups > > > > such that bios can patch in pointers without > > > > any need to understand what's what. > > > > Thoughts? > > > > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those > > > tables to set the pointers within them and then recalculate the > > > checksum. I don't think anything complex is needed - it's easy for > > > SeaBIOS to recognize those special tables and modify them. > > > > True, that's simple enough. My worry is we can add more such tables. > > For example, we can decide to switch to XSDT in the future. > > I know of the following quirks that would have to be handled: > > 1 - the RSDP must be in the f-segment (where as all other tables can > go into "high" memory). > > 2 - the RSDP has a checksum in a different location from the other > tables and (with an XSDT) it can have two checksums. > > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if present). > > 4 - the RSDT (and XSDT if present) has pointers to all the other > tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be > first in the list. > > 5 - the FADT table has pointers to DSDT and FACS. > > 6 - the FACS table must be 64 byte aligned. > > So, will a generic scheme really be able to handle all of the above > quirks, or will we just be mixing some hardcoded quirks with some > generic quirks? And, will the code to handle the above quirks in a > generic fashion be of a higher complexity than simply hard-coding it? > > -Kevin
-->
So here's an implementation for align and FSEG. Not a big deal as you see.
I really have doubts about it however: BIOS still must be able to parse get the resume vector in FACS in order to support wakeup, right? So this means that we need to be able to parse RSDP and FACT. These happen to be the only things that need anything not addressed by ADD and SUB so ... maybe a couple of hardcoded quirks just to allocate these correctly is cleaner.
Heh, it's actually pretty easy: let's just ask qemu to give us the address of the resume vector in a file with a pre-defined name. Linker can patch table offset there in the regular way.
Seabios can find resume vector address the same way OSPM does: by following pointers in memory.
Yes, that's what we do now.
Good.
What QEMU has to do with it?
The paragraphs above explain the connection.
Do not see any explanation anywhere.
Maybe I don't understand your question then. What exactly would you like to know?
My question is why would you "ask qemu to give us the address of the resume vector in a file with a pre-defined name". But since you do not do that in your patches I think we are in agreement that this is not needed.
-- Gleb.
On Tue, May 07, 2013 at 10:37:28PM +0300, Gleb Natapov wrote:
On Tue, May 07, 2013 at 10:33:17PM +0300, Michael S. Tsirkin wrote:
On Tue, May 07, 2013 at 10:26:34PM +0300, Gleb Natapov wrote:
On Tue, May 07, 2013 at 09:54:33PM +0300, Michael S. Tsirkin wrote:
On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote:
On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote: > On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote: > > On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote: > > > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote: > > > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote: > > > > > Okay I'm pretty close to posting some patches > > > > > that advance this project further, but wanted to > > > > > check something beforehand: there are several tables > > > > > that point to other tables (for example: FADT points > > > > > to DSDT). What I did is provide a list of fixups > > > > > such that bios can patch in pointers without > > > > > any need to understand what's what. > > > > > Thoughts? > > > > > > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those > > > > tables to set the pointers within them and then recalculate the > > > > checksum. I don't think anything complex is needed - it's easy for > > > > SeaBIOS to recognize those special tables and modify them. > > > > > > True, that's simple enough. My worry is we can add more such tables. > > > For example, we can decide to switch to XSDT in the future. > > > > I know of the following quirks that would have to be handled: > > > > 1 - the RSDP must be in the f-segment (where as all other tables can > > go into "high" memory). > > > > 2 - the RSDP has a checksum in a different location from the other > > tables and (with an XSDT) it can have two checksums. > > > > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if present). > > > > 4 - the RSDT (and XSDT if present) has pointers to all the other > > tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be > > first in the list. > > > > 5 - the FADT table has pointers to DSDT and FACS. > > > > 6 - the FACS table must be 64 byte aligned. > > > > So, will a generic scheme really be able to handle all of the above > > quirks, or will we just be mixing some hardcoded quirks with some > > generic quirks? And, will the code to handle the above quirks in a > > generic fashion be of a higher complexity than simply hard-coding it? > > > > -Kevin > > --> > > So here's an implementation for align and FSEG. > Not a big deal as you see. > > I really have doubts about it however: BIOS still must be able to parse > get the resume vector in FACS in order to support wakeup, right? So this > means that we need to be able to parse RSDP and FACT. These happen to > be the only things that need anything not addressed by ADD and SUB so > ... maybe a couple of hardcoded quirks just to allocate these correctly > is cleaner.
Heh, it's actually pretty easy: let's just ask qemu to give us the address of the resume vector in a file with a pre-defined name. Linker can patch table offset there in the regular way.
Seabios can find resume vector address the same way OSPM does: by following pointers in memory.
Yes, that's what we do now.
Good.
What QEMU has to do with it?
The paragraphs above explain the connection.
Do not see any explanation anywhere.
Maybe I don't understand your question then. What exactly would you like to know?
My question is why would you "ask qemu to give us the address of the resume vector in a file with a pre-defined name". But since you do not do that in your patches I think we are in agreement that this is not needed.
-- Gleb.
Generally I'd rather not write summaries of old discussions. But just this once.
Not strictly. The thread discussed what should we do with FACS which has alignment requirements: let bios detect FACS and align, or have qemu tell bios "align this table at X". If we do later, it appeared we still need knowledge of the FACS layout in bios, for resume, so why try to be generic? Unless, we add another channel to tell bios where resume vector within FACS is.
For now, I plan to keep it simple and parse FACS in bios.
Now let's stop this thread, ok? It's from Apr 24, let it pass.
On Tue, May 07, 2013 at 10:49:14PM +0300, Michael S. Tsirkin wrote:
On Tue, May 07, 2013 at 10:37:28PM +0300, Gleb Natapov wrote:
On Tue, May 07, 2013 at 10:33:17PM +0300, Michael S. Tsirkin wrote:
On Tue, May 07, 2013 at 10:26:34PM +0300, Gleb Natapov wrote:
On Tue, May 07, 2013 at 09:54:33PM +0300, Michael S. Tsirkin wrote:
On Tue, May 07, 2013 at 09:07:04PM +0300, Gleb Natapov wrote:
On Wed, Apr 24, 2013 at 11:09:16AM +0300, Michael S. Tsirkin wrote: > On Tue, Apr 23, 2013 at 08:23:44PM +0300, Michael S. Tsirkin wrote: > > On Mon, Apr 22, 2013 at 08:38:58PM -0400, Kevin O'Connor wrote: > > > On Mon, Apr 22, 2013 at 10:03:01AM +0300, Michael S. Tsirkin wrote: > > > > On Sun, Apr 21, 2013 at 08:39:41PM -0400, Kevin O'Connor wrote: > > > > > On Sun, Apr 21, 2013 at 11:41:48PM +0300, Michael S. Tsirkin wrote: > > > > > > Okay I'm pretty close to posting some patches > > > > > > that advance this project further, but wanted to > > > > > > check something beforehand: there are several tables > > > > > > that point to other tables (for example: FADT points > > > > > > to DSDT). What I did is provide a list of fixups > > > > > > such that bios can patch in pointers without > > > > > > any need to understand what's what. > > > > > > Thoughts? > > > > > > > > > > For the RSDP, RSDT, and FADT I think SeaBIOS should just update those > > > > > tables to set the pointers within them and then recalculate the > > > > > checksum. I don't think anything complex is needed - it's easy for > > > > > SeaBIOS to recognize those special tables and modify them. > > > > > > > > True, that's simple enough. My worry is we can add more such tables. > > > > For example, we can decide to switch to XSDT in the future. > > > > > > I know of the following quirks that would have to be handled: > > > > > > 1 - the RSDP must be in the f-segment (where as all other tables can > > > go into "high" memory). > > > > > > 2 - the RSDP has a checksum in a different location from the other > > > tables and (with an XSDT) it can have two checksums. > > > > > > 3 - the RSDP has a pointer to the RSDT (and to the XSDT if present). > > > > > > 4 - the RSDT (and XSDT if present) has pointers to all the other > > > tables (except RSDP, RSDT, DSDT, and FACS). The FADT pointer must be > > > first in the list. > > > > > > 5 - the FADT table has pointers to DSDT and FACS. > > > > > > 6 - the FACS table must be 64 byte aligned. > > > > > > So, will a generic scheme really be able to handle all of the above > > > quirks, or will we just be mixing some hardcoded quirks with some > > > generic quirks? And, will the code to handle the above quirks in a > > > generic fashion be of a higher complexity than simply hard-coding it? > > > > > > -Kevin > > > > --> > > > > So here's an implementation for align and FSEG. > > Not a big deal as you see. > > > > I really have doubts about it however: BIOS still must be able to parse > > get the resume vector in FACS in order to support wakeup, right? So this > > means that we need to be able to parse RSDP and FACT. These happen to > > be the only things that need anything not addressed by ADD and SUB so > > ... maybe a couple of hardcoded quirks just to allocate these correctly > > is cleaner. > > Heh, it's actually pretty easy: let's just ask qemu > to give us the address of the resume vector > in a file with a pre-defined name. > Linker can patch table offset there in the > regular way. > Seabios can find resume vector address the same way OSPM does: by following pointers in memory.
Yes, that's what we do now.
Good.
What QEMU has to do with it?
The paragraphs above explain the connection.
Do not see any explanation anywhere.
Maybe I don't understand your question then. What exactly would you like to know?
My question is why would you "ask qemu to give us the address of the resume vector in a file with a pre-defined name". But since you do not do that in your patches I think we are in agreement that this is not needed.
-- Gleb.
Generally I'd rather not write summaries of old discussions. But just this once.
Not strictly. The thread discussed what should we do with FACS which has alignment requirements: let bios detect FACS and align, or have qemu tell bios "align this table at X". If we do later, it appeared we still need knowledge of the FACS layout in bios, for resume, so why try to be generic? Unless, we add another channel to tell bios where resume vector within FACS is.
What is wrong with BIOS knowing (some) ACPI table internals?! Why invent crazy qemu/bios interfaces instead of implementing small part of well defined spec in the seabios (the part is there already actually, just do not remove it).
For now, I plan to keep it simple and parse FACS in bios.
Now let's stop this thread, ok? It's from Apr 24, let it pass.
You referred me to this ACPI discussions and now you want me to stop looking at them?
-- Gleb.
On Tue, May 07, 2013 at 07:33:13PM +0300, Michael S. Tsirkin wrote:
If you really feel QEMU should direct the pointer and checksum updates, you might want to consider something like:
struct tabledeploy_s { u32 command; union { // COMMAND_ALLOC - allocate a table from the given "file" struct { char alloc_file[FILESZ]; u32 alloc_align; u8 alloc_zone; };
// COMMAND_PATCH - patch the table (originating from // "dest_file") to a pointer to the table originating from // "src_file". struct { char patch_dest_file[FILESZ]; char patch_src_file[FILESZ]; u32 patch_offset; u8 patch_size; };I think ADD is better since this way we can use not the start of table, but something in the middle. For example, a single fw_cfg can give us multiple tables, it should be up to QEMU.
It's equivalent to PATCH if the original data is 0.
// COMMAND_CHECKSUM - Update a checksum in a table struct { char cksum_file[FILESZ]; u32 cksum_offset; u32 cksum_start; u32 cksum_length; };Same here. So I would do:
COMMAND_ADD_POINTER
COMMAND_ADD_CHECKSUM
That's fine.
-Kevin
-
David Woodhouse -
Gerd Hoffmann -
Gleb Natapov -
Kevin O'Connor -
Laszlo Ersek -
Michael S. Tsirkin -
Paolo Bonzini