It allows to extract the beginning of a Package object content.
Signed-off-by: Gleb Natapov gleb@redhat.com --- tools/acpi_extract.py | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/tools/acpi_extract.py b/tools/acpi_extract.py index 5f613e4..14c1dfe 100755 --- a/tools/acpi_extract.py +++ b/tools/acpi_extract.py @@ -29,6 +29,7 @@ # ACPI_EXTRACT_PROCESSOR_START - start of Processor() block # ACPI_EXTRACT_PROCESSOR_STRING - extract a NameString from Processor() # ACPI_EXTRACT_PROCESSOR_END - offset at last byte of Processor() + 1 +# ACPI_EXTRACT_PKG_START - start of Package block # # ACPI_EXTRACT_ALL_CODE - create an array storing the generated AML bytecode # @@ -181,6 +182,15 @@ def aml_processor_end(offset): pkglen = aml_pkglen(offset) return offset + pkglen
+def aml_package_start(offset): + offset = aml_name_string(offset) + 4 + # 0x12 PkgLength NumElements PackageElementList + if (aml[offset] != 0x12): + die( "Name offset 0x%x: expected 0x12 actual 0x%x" % + (offset, aml[offset])); + offset += 1 + return offset + aml_pkglen_bytes(offset) + 1 + lineno = 0 for line in fileinput.input(): # Strip trailing newline @@ -263,6 +273,8 @@ for i in range(len(asl)): offset = aml_processor_string(offset) elif (directive == "ACPI_EXTRACT_PROCESSOR_END"): offset = aml_processor_end(offset) + elif (directive == "ACPI_EXTRACT_PKG_START"): + offset = aml_package_start(offset) else: die("Unsupported directive %s" % directive)
QEMU may want to disable guest's S3/S4 support and it wants to distinguish between regular powerdown and S4 powerdown. To support that new fw_cfg option was added that passes supported system states and what value should guest use to enter each state. States are passed in 6 byte array. Each byte represents one system state. If byte at offset X has its MSB set it means that system state X is supported and to enter it guest should use the value from lowest 7 bits. Patch also detects old QEMU and uses values that work in backwards compatible way there.
Signed-off-by: Gleb Natapov gleb@redhat.com --- src/acpi-dsdt.dsl | 7 +++++-- src/acpi-dsdt.hex | 21 ++++++++++++++++----- src/acpi.c | 12 +++++++++++- src/paravirt.c | 16 ++++++++++++++++ src/paravirt.h | 2 ++ 5 files changed, 50 insertions(+), 8 deletions(-)
diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl index 4bdc268..cd4ce52 100644 --- a/src/acpi-dsdt.dsl +++ b/src/acpi-dsdt.dsl @@ -613,6 +613,7 @@ DefinitionBlock ( * S3 (suspend-to-ram), S4 (suspend-to-disk) and S5 (power-off) type codes: * must match piix4 emulation. */ + ACPI_EXTRACT_NAME_STRING acpi_s3_name Name (_S3, Package (0x04) { 0x01, /* PM1a_CNT.SLP_TYP */ @@ -620,10 +621,12 @@ DefinitionBlock ( Zero, /* reserved */ Zero /* reserved */ }) + ACPI_EXTRACT_NAME_STRING acpi_s4_name + ACPI_EXTRACT_PKG_START acpi_s4_pkg Name (_S4, Package (0x04) { - Zero, /* PM1a_CNT.SLP_TYP */ - Zero, /* PM1b_CNT.SLP_TYP */ + 0x2, /* PM1a_CNT.SLP_TYP */ + 0x2, /* PM1b_CNT.SLP_TYP */ Zero, /* reserved */ Zero /* reserved */ }) diff --git a/src/acpi-dsdt.hex b/src/acpi-dsdt.hex index a4af597..8a50f55 100644 --- a/src/acpi-dsdt.hex +++ b/src/acpi-dsdt.hex @@ -1,14 +1,20 @@ +static unsigned short acpi_s3_name[] = { +0xf57 +}; +static unsigned short acpi_s4_name[] = { +0xf63 +}; static unsigned char AmlCode[] = { 0x44, 0x53, 0x44, 0x54, -0x21, +0x23, 0x11, 0x0, 0x0, 0x1, -0xe8, +0xcc, 0x42, 0x58, 0x50, @@ -3943,10 +3949,12 @@ static unsigned char AmlCode[] = { 0x34, 0x5f, 0x12, -0x6, +0x8, 0x4, -0x0, -0x0, +0xa, +0x2, +0xa, +0x2, 0x0, 0x0, 0x8, @@ -4385,3 +4393,6 @@ static unsigned char AmlCode[] = { 0xa4, 0x1 }; +static unsigned short acpi_s4_pkg[] = { +0xf6a +}; diff --git a/src/acpi.c b/src/acpi.c index 30888b9..1e7d466 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -734,13 +734,23 @@ acpi_bios_init(void) } if (fadt && !fadt->dsdt) { /* default DSDT */ - void *dsdt = malloc_high(sizeof(AmlCode)); + char *dsdt = malloc_high(sizeof(AmlCode)); + char sys_states[6]; if (!dsdt) { warn_noalloc(); return; } memcpy(dsdt, AmlCode, sizeof(AmlCode)); fill_dsdt(fadt, dsdt); + qemu_cfg_system_states(sys_states); + if (!(sys_states[3] & 128)) + dsdt[acpi_s3_name[0]] = 'X'; + if (!(sys_states[4] & 128)) + dsdt[acpi_s4_name[0]] = 'X'; + else + dsdt[acpi_s4_pkg[0] + 1] = dsdt[acpi_s4_pkg[0] + 3] = sys_states[4] & 127; + ((struct acpi_table_header*)dsdt)->checksum = 0; + ((struct acpi_table_header*)dsdt)->checksum -= checksum(dsdt, sizeof(AmlCode)); }
// Build final rsdt table diff --git a/src/paravirt.c b/src/paravirt.c index 9cf77de..201bbc3 100644 --- a/src/paravirt.c +++ b/src/paravirt.c @@ -92,6 +92,22 @@ int qemu_cfg_irq0_override(void) return v; }
+int qemu_cfg_system_states(char *states) +{ + char s[6] = {128, 0, 0, 129, 128, 128}; + + if (qemu_cfg_present) { + qemu_cfg_read_entry(states, QEMU_CFG_SYSTEM_STATES, 6); + if (states[0]) + return 1; + } + + /* use defaults matching old QEMU */ + memcpy(states, s, sizeof(s)); + + return 0; +} + u16 qemu_cfg_acpi_additional_tables(void) { u16 cnt; diff --git a/src/paravirt.h b/src/paravirt.h index f39e226..b69646c 100644 --- a/src/paravirt.h +++ b/src/paravirt.h @@ -40,6 +40,7 @@ static inline int kvm_para_available(void) #define QEMU_CFG_SMBIOS_ENTRIES (QEMU_CFG_ARCH_LOCAL + 1) #define QEMU_CFG_IRQ0_OVERRIDE (QEMU_CFG_ARCH_LOCAL + 2) #define QEMU_CFG_E820_TABLE (QEMU_CFG_ARCH_LOCAL + 3) +#define QEMU_CFG_SYSTEM_STATES (QEMU_CFG_ARCH_LOCAL + 5)
extern int qemu_cfg_present;
@@ -109,4 +110,5 @@ u64 romfile_loadint(const char *name, u64 defval); u32 qemu_cfg_e820_entries(void); void* qemu_cfg_e820_load_next(void *addr);
+int qemu_cfg_system_states(char *states); #endif
On Mon, May 14, 2012 at 03:35:23PM +0300, Gleb Natapov wrote:
QEMU may want to disable guest's S3/S4 support and it wants to distinguish between regular powerdown and S4 powerdown. To support that new fw_cfg option was added that passes supported system states and what value should guest use to enter each state. States are passed in 6 byte array. Each byte represents one system state. If byte at offset X has its MSB set it means that system state X is supported and to enter it guest should use the value from lowest 7 bits. Patch also detects old QEMU and uses values that work in backwards compatible way there.
A couple of comments - see below.
[...]
--- a/src/acpi-dsdt.dsl +++ b/src/acpi-dsdt.dsl @@ -613,6 +613,7 @@ DefinitionBlock ( * S3 (suspend-to-ram), S4 (suspend-to-disk) and S5 (power-off) type codes: * must match piix4 emulation. */
- ACPI_EXTRACT_NAME_STRING acpi_s3_name Name (_S3, Package (0x04) { 0x01, /* PM1a_CNT.SLP_TYP */
@@ -620,10 +621,12 @@ DefinitionBlock ( Zero, /* reserved */ Zero /* reserved */ })
- ACPI_EXTRACT_NAME_STRING acpi_s4_name
- ACPI_EXTRACT_PKG_START acpi_s4_pkg
The DSDT is quite complex and has a diverse usage. I'd feel more comfortable leaving it as static and doing any dynamic work in an SSDT. In this particular case, can't the objects be turned into methods which calculate the associated values and return the correct results?
[...]
--- a/src/paravirt.c +++ b/src/paravirt.c @@ -92,6 +92,22 @@ int qemu_cfg_irq0_override(void) return v; }
+int qemu_cfg_system_states(char *states) +{
I'd prefer to see any new fw_cfg entries use the QEMU_CFG_FILE_DIR mechanism so that seabios can use romfile_loadfile (or similar).
-Kevin
On Mon, May 14, 2012 at 09:43:19PM -0400, Kevin O'Connor wrote:
On Mon, May 14, 2012 at 03:35:23PM +0300, Gleb Natapov wrote:
QEMU may want to disable guest's S3/S4 support and it wants to distinguish between regular powerdown and S4 powerdown. To support that new fw_cfg option was added that passes supported system states and what value should guest use to enter each state. States are passed in 6 byte array. Each byte represents one system state. If byte at offset X has its MSB set it means that system state X is supported and to enter it guest should use the value from lowest 7 bits. Patch also detects old QEMU and uses values that work in backwards compatible way there.
A couple of comments - see below.
[...]
--- a/src/acpi-dsdt.dsl +++ b/src/acpi-dsdt.dsl @@ -613,6 +613,7 @@ DefinitionBlock ( * S3 (suspend-to-ram), S4 (suspend-to-disk) and S5 (power-off) type codes: * must match piix4 emulation. */
- ACPI_EXTRACT_NAME_STRING acpi_s3_name Name (_S3, Package (0x04) { 0x01, /* PM1a_CNT.SLP_TYP */
@@ -620,10 +621,12 @@ DefinitionBlock ( Zero, /* reserved */ Zero /* reserved */ })
- ACPI_EXTRACT_NAME_STRING acpi_s4_name
- ACPI_EXTRACT_PKG_START acpi_s4_pkg
The DSDT is quite complex and has a diverse usage. I'd feel more comfortable leaving it as static and doing any dynamic work in an SSDT. In this particular case, can't the objects be turned into methods which calculate the associated values and return the correct results?
Checked with WindowsXP and Linux and they work if I make _S3 to be a method that returns package, so we can drop ACPI_EXTRACT_PKG_START and do runtime calculation, but what this calculation will be based on? We will have to pass QEMU S4 value to AML somehow and this will involve patching of something eventually. And of course we will still have to patch out _S3/_S4 names in case qemu want to disable them. I do not see how we can disable them in any other way.
I think the use of patching will only increase now after we let that genie out of the bottle, so moving each part that we want to patch in separate SSDT will not scale. Here the patching is minimal, moving only _Sx to a separate SSDT feels unnecessary. Of course we can do it later if thing will become more complex. We are not creating any ABIs here that we cannot redo, just small implementation detail.
[...]
--- a/src/paravirt.c +++ b/src/paravirt.c @@ -92,6 +92,22 @@ int qemu_cfg_irq0_override(void) return v; }
+int qemu_cfg_system_states(char *states) +{
I'd prefer to see any new fw_cfg entries use the QEMU_CFG_FILE_DIR mechanism so that seabios can use romfile_loadfile (or similar).
The number of files you can pass over fw_cfg interface is limited due to implementation details. I think we should continue using regular fw_cfg entries for code that is QEMU specific and files for code that is shared with coreboot.
-- Gleb.
On Tue, May 15, 2012 at 11:06:05AM +0300, Gleb Natapov wrote:
On Mon, May 14, 2012 at 09:43:19PM -0400, Kevin O'Connor wrote:
On Mon, May 14, 2012 at 03:35:23PM +0300, Gleb Natapov wrote:
QEMU may want to disable guest's S3/S4 support and it wants to distinguish between regular powerdown and S4 powerdown. To support that new fw_cfg option was added that passes supported system states and what value should guest use to enter each state. States are passed in 6 byte array. Each byte represents one system state. If byte at offset X has its MSB set it means that system state X is supported and to enter it guest should use the value from lowest 7 bits. Patch also detects old QEMU and uses values that work in backwards compatible way there.
A couple of comments - see below.
[...]
--- a/src/acpi-dsdt.dsl +++ b/src/acpi-dsdt.dsl @@ -613,6 +613,7 @@ DefinitionBlock ( * S3 (suspend-to-ram), S4 (suspend-to-disk) and S5 (power-off) type codes: * must match piix4 emulation. */
- ACPI_EXTRACT_NAME_STRING acpi_s3_name Name (_S3, Package (0x04) { 0x01, /* PM1a_CNT.SLP_TYP */
@@ -620,10 +621,12 @@ DefinitionBlock ( Zero, /* reserved */ Zero /* reserved */ })
- ACPI_EXTRACT_NAME_STRING acpi_s4_name
- ACPI_EXTRACT_PKG_START acpi_s4_pkg
The DSDT is quite complex and has a diverse usage. I'd feel more comfortable leaving it as static and doing any dynamic work in an SSDT. In this particular case, can't the objects be turned into methods which calculate the associated values and return the correct results?
Checked with WindowsXP and Linux and they work if I make _S3 to be a method that returns package, so we can drop ACPI_EXTRACT_PKG_START and do runtime calculation, but what this calculation will be based on? We will have to pass QEMU S4 value to AML somehow and this will involve patching of something eventually.
As in the other recent discussion, a struct can be built by the BIOS and a pointer passed in via a dynamic SSDT (eg, BDAT). Whatever data is needed can then be passed in via that struct.
And of course we will still have to patch out _S3/_S4 names in case qemu want to disable them. I do not see how we can disable them in any other way.
If the mere existence of _S3 tells the OS that S3 is supported, then it will have to be patched in.
I think the use of patching will only increase now after we let that genie out of the bottle, so moving each part that we want to patch in separate SSDT will not scale.
Why? Just put the definitions in ssdp_pcihp.dsl instead of acpi-dsdt.dsl - there's no real difference.
+int qemu_cfg_system_states(char *states) +{
I'd prefer to see any new fw_cfg entries use the QEMU_CFG_FILE_DIR mechanism so that seabios can use romfile_loadfile (or similar).
The number of files you can pass over fw_cfg interface is limited due to implementation details. I think we should continue using regular fw_cfg entries for code that is QEMU specific and files for code that is shared with coreboot.
The QEMU_CFG_FILE_DIR is just a list of "names" and "sizes" for each "port". There's no fundamental limitation to the interface. If QEMU has a limit, we should just fix that.
-Kevin
On Tue, May 15, 2012 at 07:18:10PM -0400, Kevin O'Connor wrote:
On Tue, May 15, 2012 at 11:06:05AM +0300, Gleb Natapov wrote:
On Mon, May 14, 2012 at 09:43:19PM -0400, Kevin O'Connor wrote:
On Mon, May 14, 2012 at 03:35:23PM +0300, Gleb Natapov wrote:
QEMU may want to disable guest's S3/S4 support and it wants to distinguish between regular powerdown and S4 powerdown. To support that new fw_cfg option was added that passes supported system states and what value should guest use to enter each state. States are passed in 6 byte array. Each byte represents one system state. If byte at offset X has its MSB set it means that system state X is supported and to enter it guest should use the value from lowest 7 bits. Patch also detects old QEMU and uses values that work in backwards compatible way there.
A couple of comments - see below.
[...]
--- a/src/acpi-dsdt.dsl +++ b/src/acpi-dsdt.dsl @@ -613,6 +613,7 @@ DefinitionBlock ( * S3 (suspend-to-ram), S4 (suspend-to-disk) and S5 (power-off) type codes: * must match piix4 emulation. */
- ACPI_EXTRACT_NAME_STRING acpi_s3_name Name (_S3, Package (0x04) { 0x01, /* PM1a_CNT.SLP_TYP */
@@ -620,10 +621,12 @@ DefinitionBlock ( Zero, /* reserved */ Zero /* reserved */ })
- ACPI_EXTRACT_NAME_STRING acpi_s4_name
- ACPI_EXTRACT_PKG_START acpi_s4_pkg
The DSDT is quite complex and has a diverse usage. I'd feel more comfortable leaving it as static and doing any dynamic work in an SSDT. In this particular case, can't the objects be turned into methods which calculate the associated values and return the correct results?
Checked with WindowsXP and Linux and they work if I make _S3 to be a method that returns package, so we can drop ACPI_EXTRACT_PKG_START and do runtime calculation, but what this calculation will be based on? We will have to pass QEMU S4 value to AML somehow and this will involve patching of something eventually.
As in the other recent discussion, a struct can be built by the BIOS and a pointer passed in via a dynamic SSDT (eg, BDAT). Whatever data is needed can then be passed in via that struct.
I saw that, but I don't get why doing it this way instead of defining the object in AML and patching it? I can define Name(S4VL, 0x2) and path 0x2 to whatever QEMU wants me to use, or I can patch Package directly like I did.
And of course we will still have to patch out _S3/_S4 names in case qemu want to disable them. I do not see how we can disable them in any other way.
If the mere existence of _S3 tells the OS that S3 is supported, then it will have to be patched in.
Seems to be so.
I think the use of patching will only increase now after we let that genie out of the bottle, so moving each part that we want to patch in separate SSDT will not scale.
Why? Just put the definitions in ssdp_pcihp.dsl instead of acpi-dsdt.dsl - there's no real difference.
Fine by me. I verified and Windows/Linux can cope with _Sx definitions being in SSDT. If we a going to move all the code that needs patching to this file may we should rename it to something like ssdp_dynamic.dsl?
+int qemu_cfg_system_states(char *states) +{
I'd prefer to see any new fw_cfg entries use the QEMU_CFG_FILE_DIR mechanism so that seabios can use romfile_loadfile (or similar).
The number of files you can pass over fw_cfg interface is limited due to implementation details. I think we should continue using regular fw_cfg entries for code that is QEMU specific and files for code that is shared with coreboot.
The QEMU_CFG_FILE_DIR is just a list of "names" and "sizes" for each "port". There's no fundamental limitation to the interface. If QEMU has a limit, we should just fix that.
Each time Seabios wants to read a file it need to iterate over all/most existing files. I can understand advantage of using files for code that is shared between coreboot and qemu since files is what real HW uses, but for QEMU internal code it is just overhead for the sake of it. I do not have strong fillings about this issue. If you think that files is the only way forward may be you should communicate this to QEMU and put a comment in hw/fw_cfg.h explaining that and increasing FW_CFG_FILE_SLOTS to some ridiculously large value.
-- Gleb.
Il 16/05/2012 15:46, Gleb Natapov ha scritto:
I saw that, but I don't get why doing it this way instead of defining the object in AML and patching it? I can define Name(S4VL, 0x2) and path 0x2 to whatever QEMU wants me to use, or I can patch Package directly like I did.
Can we build an SSDT that includes the contents of fw_cfg (e.g. FW_CFG_SIGNATURE at offset 0, FW_CFG_UUID at offset 4, FW_CFG_NOGRAPHIC at offset 16... the entry <-> offset mapping and the defaults would be part of SeaBIOS), and then read that data from normal DSDT methods?
That would be similar to Gerd's patch, but without letting the OSPM use the real fw_cfg device.
Paolo
On Wed, May 16, 2012 at 05:50:31PM +0200, Paolo Bonzini wrote:
Il 16/05/2012 15:46, Gleb Natapov ha scritto:
I saw that, but I don't get why doing it this way instead of defining the object in AML and patching it? I can define Name(S4VL, 0x2) and path 0x2 to whatever QEMU wants me to use, or I can patch Package directly like I did.
Can we build an SSDT that includes the contents of fw_cfg (e.g. FW_CFG_SIGNATURE at offset 0, FW_CFG_UUID at offset 4, FW_CFG_NOGRAPHIC at offset 16... the entry <-> offset mapping and the defaults would be part of SeaBIOS), and then read that data from normal DSDT methods?
Kevin does not want to use offsets any more :) He wants to use files, so this will not work for new entries.
That would be similar to Gerd's patch, but without letting the OSPM use the real fw_cfg device.
Latest Gerd's patch does not use fw_cfg device. It creates AML code manually, I am saying we can do the same by patching, but the end result will be the same.
-- Gleb.
Il 16/05/2012 18:40, Gleb Natapov ha scritto:
On Wed, May 16, 2012 at 05:50:31PM +0200, Paolo Bonzini wrote:
Il 16/05/2012 15:46, Gleb Natapov ha scritto:
I saw that, but I don't get why doing it this way instead of defining the object in AML and patching it? I can define Name(S4VL, 0x2) and path 0x2 to whatever QEMU wants me to use, or I can patch Package directly like I did.
Can we build an SSDT that includes the contents of fw_cfg (e.g. FW_CFG_SIGNATURE at offset 0, FW_CFG_UUID at offset 4, FW_CFG_NOGRAPHIC at offset 16... the entry <-> offset mapping and the defaults would be part of SeaBIOS), and then read that data from normal DSDT methods?
Kevin does not want to use offsets any more :) He wants to use files, so this will not work for new entries.
Then we can have: - a table in SeaBIOS with (filenames, expected length) pairs - an ACPI table with a list of offsets for each file (-1 if file not found or length < expected length), with the same indices as the previous table - and another blob with all the files concatenated
The idea is the same, just pass the fw_cfg data to the DSDT and read it from there. As long as Windows and Linux can cope with the more complex AML, there is no need to do complicated patching IMO...
That would be similar to Gerd's patch, but without letting the OSPM use the real fw_cfg device.
Latest Gerd's patch does not use fw_cfg device.
Yes, I meant the same as his first patch, not really the machanics of creating the BDAT.
Paolo
On Wed, May 16, 2012 at 06:47:55PM +0200, Paolo Bonzini wrote:
Il 16/05/2012 18:40, Gleb Natapov ha scritto:
On Wed, May 16, 2012 at 05:50:31PM +0200, Paolo Bonzini wrote:
Il 16/05/2012 15:46, Gleb Natapov ha scritto:
I saw that, but I don't get why doing it this way instead of defining the object in AML and patching it? I can define Name(S4VL, 0x2) and path 0x2 to whatever QEMU wants me to use, or I can patch Package directly like I did.
Can we build an SSDT that includes the contents of fw_cfg (e.g. FW_CFG_SIGNATURE at offset 0, FW_CFG_UUID at offset 4, FW_CFG_NOGRAPHIC at offset 16... the entry <-> offset mapping and the defaults would be part of SeaBIOS), and then read that data from normal DSDT methods?
Kevin does not want to use offsets any more :) He wants to use files, so this will not work for new entries.
Then we can have:
- a table in SeaBIOS with (filenames, expected length) pairs
- an ACPI table with a list of offsets for each file (-1 if file not
found or length < expected length), with the same indices as the previous table
- and another blob with all the files concatenated
The idea is the same, just pass the fw_cfg data to the DSDT and read it from there. As long as Windows and Linux can cope with the more complex AML, there is no need to do complicated patching IMO...
Why would we want to do that? We do not need 99% present of fw_cfg data in AML. There are exactly two cases, that appeared recently, where we need to patch AML according to fw_cfg and your proposal does not eliminate patching in one of those cases anyway. Namely we need to patch out _S3/_S4 from AML. You can't do that in AML itself.
That would be similar to Gerd's patch, but without letting the OSPM use the real fw_cfg device.
Latest Gerd's patch does not use fw_cfg device.
Yes, I meant the same as his first patch, not really the machanics of creating the BDAT.
Paolo
-- Gleb.
On Wed, May 16, 2012 at 05:50:31PM +0200, Paolo Bonzini wrote:
Il 16/05/2012 15:46, Gleb Natapov ha scritto:
I saw that, but I don't get why doing it this way instead of defining the object in AML and patching it? I can define Name(S4VL, 0x2) and path 0x2 to whatever QEMU wants me to use, or I can patch Package directly like I did.
Can we build an SSDT that includes the contents of fw_cfg (e.g. FW_CFG_SIGNATURE at offset 0, FW_CFG_UUID at offset 4, FW_CFG_NOGRAPHIC at offset 16... the entry <-> offset mapping and the defaults would be part of SeaBIOS), and then read that data from normal DSDT methods?
That would be similar to Gerd's patch, but without letting the OSPM use the real fw_cfg device.
I'm not sure I understand your proposal. Are you suggesting reading every fw_cfg "port" into memory and then passing that memory into an SSDT? If so, that wouldn't be easy (we don't necessarily know the size of each "port") and could potentially waste a lot of memory (think a vmlinuz stored in fw_cfg).
-Kevin
Il 17/05/2012 02:24, Kevin O'Connor ha scritto:
Can we build an SSDT that includes the contents of fw_cfg (e.g. FW_CFG_SIGNATURE at offset 0, FW_CFG_UUID at offset 4, FW_CFG_NOGRAPHIC at offset 16... the entry <-> offset mapping and the defaults would be part of SeaBIOS), and then read that data from normal DSDT methods?
That would be similar to Gerd's patch, but without letting the OSPM use the real fw_cfg device.
I'm not sure I understand your proposal. Are you suggesting reading every fw_cfg "port" into memory and then passing that memory into an SSDT? If so, that wouldn't be easy (we don't necessarily know the size of each "port") and could potentially waste a lot of memory (think a vmlinuz stored in fw_cfg).
No, only entries and/or files that are needed by the DSDT (PCI region and sleep data).
Paolo
On Wed, May 16, 2012 at 04:46:57PM +0300, Gleb Natapov wrote:
On Tue, May 15, 2012 at 07:18:10PM -0400, Kevin O'Connor wrote:
As in the other recent discussion, a struct can be built by the BIOS and a pointer passed in via a dynamic SSDT (eg, BDAT). Whatever data is needed can then be passed in via that struct.
I saw that, but I don't get why doing it this way instead of defining the object in AML and patching it? I can define Name(S4VL, 0x2) and path 0x2 to whatever QEMU wants me to use, or I can patch Package directly like I did.
If it has to be patched anyway (eg, to remove _S3 definition) then, yes, might as well do the whole thing via patching.
Why? Just put the definitions in ssdp_pcihp.dsl instead of acpi-dsdt.dsl - there's no real difference.
Fine by me. I verified and Windows/Linux can cope with _Sx definitions being in SSDT. If we a going to move all the code that needs patching to this file may we should rename it to something like ssdp_dynamic.dsl?
Agreed.
BTW, what's the background to the requirement to configure the Sx sleep levels? As we've discussed some time back, I'm leery of building custom QEMU->SeaBIOS interfaces just so SeaBIOS can then reencode into ACPI for the OS.
The QEMU_CFG_FILE_DIR is just a list of "names" and "sizes" for each "port". There's no fundamental limitation to the interface. If QEMU has a limit, we should just fix that.
Each time Seabios wants to read a file it need to iterate over all/most existing files.
Yes. I'd like to change this in SeaBIOS by caching the "romfile" entries. It would actually simplify the code.
I can understand advantage of using files for code that is shared between coreboot and qemu since files is what real HW uses, but for QEMU internal code it is just overhead for the sake of it.
The real benefit to using QEMU_CFG_FILE_DIR is that we can get at the size of the data in a standard way. Without that, each new data item has to have its own fw_cfg reading code which is just a waste.
I do not have strong fillings about this issue. If you think that files is the only way forward may be you should communicate this to QEMU and put a comment in hw/fw_cfg.h explaining that and increasing FW_CFG_FILE_SLOTS to some ridiculously large value.
I've been meaning to build a qemu patch that puts all fw_cfg entries in QEMU_CFG_FILE_DIR. (There's no harm in making names for the existing hard-coded fw_cfg "ports".) Unfortunately, I haven't gotten around to it. :-(
-Kevin
-
Gleb Natapov -
Kevin O'Connor -
Paolo Bonzini