[PATCH 1/3] Fix aml_name_string() to recognize block name modifiers.
Signed-off-by: Gleb Natapov <gleb@redhat.com> --- tools/acpi_extract.py | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/tools/acpi_extract.py b/tools/acpi_extract.py index 5f613e4..8038269 100755 --- a/tools/acpi_extract.py +++ b/tools/acpi_extract.py @@ -121,7 +121,11 @@ def aml_name_string(offset): if (aml[offset] != 0x08): die( "Name offset 0x%x: expected 0x08 actual 0x%x" % (offset, aml[offset])); - return offset + 1; + offset += 1 + # Block Name Modifier. Skip it. + if (aml[offset] == 0x5c or aml[offset] == 0x5e): + offset += 1 + return offset; # Given data offset, find dword const offset def aml_data_dword_const(offset): -- 1.7.7.3
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 8038269..167a322 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 # @@ -185,6 +186,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 @@ -267,6 +277,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) -- 1.7.7.3
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 | 32 --------- src/acpi-dsdt.hex | 42 +----------- src/acpi.c | 15 ++++ src/ssdt-pcihp.dsl | 36 ++++++++++ src/ssdt-pcihp.hex | 185 +++++++++++++++++++++++++++++++++------------------- 5 files changed, 172 insertions(+), 138 deletions(-) diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl index 4bdc268..37899fc 100644 --- a/src/acpi-dsdt.dsl +++ b/src/acpi-dsdt.dsl @@ -604,38 +604,6 @@ DefinitionBlock ( } } - -/**************************************************************** - * Suspend - ****************************************************************/ - - /* - * S3 (suspend-to-ram), S4 (suspend-to-disk) and S5 (power-off) type codes: - * must match piix4 emulation. - */ - Name (\_S3, Package (0x04) - { - 0x01, /* PM1a_CNT.SLP_TYP */ - 0x01, /* PM1b_CNT.SLP_TYP */ - Zero, /* reserved */ - Zero /* reserved */ - }) - Name (\_S4, Package (0x04) - { - Zero, /* PM1a_CNT.SLP_TYP */ - Zero, /* PM1b_CNT.SLP_TYP */ - Zero, /* reserved */ - Zero /* reserved */ - }) - Name (\_S5, Package (0x04) - { - Zero, /* PM1a_CNT.SLP_TYP */ - Zero, /* PM1b_CNT.SLP_TYP */ - Zero, /* reserved */ - Zero /* reserved */ - }) - - /**************************************************************** * CPU hotplug ****************************************************************/ diff --git a/src/acpi-dsdt.hex b/src/acpi-dsdt.hex index a4af597..8678fbf 100644 --- a/src/acpi-dsdt.hex +++ b/src/acpi-dsdt.hex @@ -3,12 +3,12 @@ static unsigned char AmlCode[] = { 0x53, 0x44, 0x54, -0x21, -0x11, +0xfd, +0x10, 0x0, 0x0, 0x1, -0xe8, +0x4a, 0x42, 0x58, 0x50, @@ -3925,42 +3925,6 @@ static unsigned char AmlCode[] = { 0x52, 0x51, 0x30, -0x8, -0x5f, -0x53, -0x33, -0x5f, -0x12, -0x6, -0x4, -0x1, -0x1, -0x0, -0x0, -0x8, -0x5f, -0x53, -0x34, -0x5f, -0x12, -0x6, -0x4, -0x0, -0x0, -0x0, -0x0, -0x8, -0x5f, -0x53, -0x35, -0x5f, -0x12, -0x6, -0x4, -0x0, -0x0, -0x0, -0x0, 0x10, 0x49, 0xe, diff --git a/src/acpi.c b/src/acpi.c index 30888b9..06ffe0a 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -492,6 +492,8 @@ extern void link_time_assertion(void); static void* build_pcihp(void) { + char *sys_states; + int sys_state_size; u32 rmvc_pcrm; int i; @@ -523,6 +525,19 @@ static void* build_pcihp(void) } } + sys_states = romfile_loadfile("etc/system-states", &sys_state_size); + if (!sys_states || sys_state_size != 6) + sys_states = (char[]){128, 0, 0, 129, 128, 128}; + + if (!(sys_states[3] & 128)) + ssdt[acpi_s3_name[0]] = 'X'; + if (!(sys_states[4] & 128)) + ssdt[acpi_s4_name[0]] = 'X'; + else + ssdt[acpi_s4_pkg[0] + 1] = ssdt[acpi_s4_pkg[0] + 3] = sys_states[4] & 127; + ((struct acpi_table_header*)ssdt)->checksum = 0; + ((struct acpi_table_header*)ssdt)->checksum -= checksum(ssdt, sizeof(ssdp_pcihp_aml)); + return ssdt; } diff --git a/src/ssdt-pcihp.dsl b/src/ssdt-pcihp.dsl index 4b435b8..12555e2 100644 --- a/src/ssdt-pcihp.dsl +++ b/src/ssdt-pcihp.dsl @@ -95,4 +95,40 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1) gen_pci_hotplug(1f) } } + + Scope(\) { +/**************************************************************** + * Suspend + ****************************************************************/ + + /* + * 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) + { + One, /* PM1a_CNT.SLP_TYP */ + One, /* PM1b_CNT.SLP_TYP */ + Zero, /* reserved */ + Zero /* reserved */ + }) + ACPI_EXTRACT_NAME_STRING acpi_s4_name + ACPI_EXTRACT_PKG_START acpi_s4_pkg + Name (_S4, Package (0x04) + { + 0x2, /* PM1a_CNT.SLP_TYP */ + 0x2, /* PM1b_CNT.SLP_TYP */ + Zero, /* reserved */ + Zero /* reserved */ + }) + Name (_S5, Package (0x04) + { + Zero, /* PM1a_CNT.SLP_TYP */ + Zero, /* PM1b_CNT.SLP_TYP */ + Zero, /* reserved */ + Zero /* reserved */ + }) + } } diff --git a/src/ssdt-pcihp.hex b/src/ssdt-pcihp.hex index b15ad5a..f3bb3c6 100644 --- a/src/ssdt-pcihp.hex +++ b/src/ssdt-pcihp.hex @@ -1,80 +1,20 @@ -static unsigned short aml_adr_dword[] = { -0x3e, -0x62, -0x88, -0xae, -0xd4, -0xfa, -0x120, -0x146, -0x16c, -0x192, -0x1b8, -0x1de, -0x204, -0x22a, -0x250, -0x276, -0x29c, -0x2c2, -0x2e8, -0x30e, -0x334, -0x35a, -0x380, -0x3a6, -0x3cc, -0x3f2, -0x418, -0x43e, -0x464, -0x48a, -0x4b0 +static unsigned short acpi_s4_pkg[] = { +0x65c }; -static unsigned short aml_ej0_name[] = { -0x44, -0x68, -0x8e, -0xb4, -0xda, -0x100, -0x126, -0x14c, -0x172, -0x198, -0x1be, -0x1e4, -0x20a, -0x230, -0x256, -0x27c, -0x2a2, -0x2c8, -0x2ee, -0x314, -0x33a, -0x360, -0x386, -0x3ac, -0x3d2, -0x3f8, -0x41e, -0x444, -0x46a, -0x490, -0x4b6 +static unsigned short acpi_s3_name[] = { +0x649 }; static unsigned char ssdp_pcihp_aml[] = { 0x53, 0x53, 0x44, 0x54, -0x44, +0x6e, 0x6, 0x0, 0x0, 0x1, -0x94, +0x7e, 0x42, 0x58, 0x50, @@ -1668,5 +1608,116 @@ static unsigned char ssdp_pcihp_aml[] = { 0x31, 0x46, 0x5f, -0x69 +0x69, +0x10, +0x29, +0x5c, +0x0, +0x8, +0x5f, +0x53, +0x33, +0x5f, +0x12, +0x6, +0x4, +0x1, +0x1, +0x0, +0x0, +0x8, +0x5f, +0x53, +0x34, +0x5f, +0x12, +0x8, +0x4, +0xa, +0x2, +0xa, +0x2, +0x0, +0x0, +0x8, +0x5f, +0x53, +0x35, +0x5f, +0x12, +0x6, +0x4, +0x0, +0x0, +0x0, +0x0 +}; +static unsigned short aml_adr_dword[] = { +0x3e, +0x62, +0x88, +0xae, +0xd4, +0xfa, +0x120, +0x146, +0x16c, +0x192, +0x1b8, +0x1de, +0x204, +0x22a, +0x250, +0x276, +0x29c, +0x2c2, +0x2e8, +0x30e, +0x334, +0x35a, +0x380, +0x3a6, +0x3cc, +0x3f2, +0x418, +0x43e, +0x464, +0x48a, +0x4b0 +}; +static unsigned short acpi_s4_name[] = { +0x655 +}; +static unsigned short aml_ej0_name[] = { +0x44, +0x68, +0x8e, +0xb4, +0xda, +0x100, +0x126, +0x14c, +0x172, +0x198, +0x1be, +0x1e4, +0x20a, +0x230, +0x256, +0x27c, +0x2a2, +0x2c8, +0x2ee, +0x314, +0x33a, +0x360, +0x386, +0x3ac, +0x3d2, +0x3f8, +0x41e, +0x444, +0x46a, +0x490, +0x4b6 }; -- 1.7.7.3
On 05/20/2012 12:03 PM, 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.
Do we actually have to patch the DSDT? Or can _S3 etc be made into functions instead? (and talk to the bios, or even to fwcfg directly?) -- error compiling committee.c: too many arguments to function
On Sun, May 20, 2012 at 02:44:51PM +0300, Avi Kivity wrote:
On 05/20/2012 12:03 PM, 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.
Do we actually have to patch the DSDT? Or can _S3 etc be made into functions instead? (and talk to the bios, or even to fwcfg directly?)
We better not talk to fwcfg after OSPM is started since this is firmware confing interface. Regardless, presence of _S3 name or method is all that needed for OS enabling S3 option. If _S3 is defined as a method it has to return Package() otherwise iasl refuses to compile it. -- Gleb.
On 05/20/2012 03:15 PM, Gleb Natapov wrote:
On Sun, May 20, 2012 at 02:44:51PM +0300, Avi Kivity wrote:
On 05/20/2012 12:03 PM, 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.
Do we actually have to patch the DSDT? Or can _S3 etc be made into functions instead? (and talk to the bios, or even to fwcfg directly?)
We better not talk to fwcfg after OSPM is started since this is firmware confing interface.
Why not? The OS isn't going to talk to it, so we can have a driver in ACPI.
Regardless, presence of _S3 name or method is all that needed for OS enabling S3 option. If _S3 is defined as a method it has to return Package() otherwise iasl refuses to compile it.
Can't we Return (Package (...) { ... }) or equivalent? -- error compiling committee.c: too many arguments to function
On Sun, May 20, 2012 at 03:30:50PM +0300, Avi Kivity wrote:
On 05/20/2012 03:15 PM, Gleb Natapov wrote:
On Sun, May 20, 2012 at 02:44:51PM +0300, Avi Kivity wrote:
On 05/20/2012 12:03 PM, 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.
Do we actually have to patch the DSDT? Or can _S3 etc be made into functions instead? (and talk to the bios, or even to fwcfg directly?)
We better not talk to fwcfg after OSPM is started since this is firmware confing interface.
Why not? The OS isn't going to talk to it, so we can have a driver in ACPI.
The OS is going to talk to it since the OS is the one who interprets AML. We may want to disable fwcfg after OS bootup at all in the feature. Who knows what kind of sensitive information we may want to pass by it in the feature? May be something TPM related? And I do not see any advantage of using fwcfg from AML.
Regardless, presence of _S3 name or method is all that needed for OS enabling S3 option. If _S3 is defined as a method it has to return Package() otherwise iasl refuses to compile it.
Can't we Return (Package (...) { ... }) or equivalent?
We can, how does it help? -- Gleb.
On 05/20/2012 03:36 PM, Gleb Natapov wrote:
On Sun, May 20, 2012 at 03:30:50PM +0300, Avi Kivity wrote:
On 05/20/2012 03:15 PM, Gleb Natapov wrote:
On Sun, May 20, 2012 at 02:44:51PM +0300, Avi Kivity wrote:
On 05/20/2012 12:03 PM, 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.
Do we actually have to patch the DSDT? Or can _S3 etc be made into functions instead? (and talk to the bios, or even to fwcfg directly?)
We better not talk to fwcfg after OSPM is started since this is firmware confing interface.
Why not? The OS isn't going to talk to it, so we can have a driver in ACPI.
The OS is going to talk to it since the OS is the one who interprets AML.
I meant, not directly. So the driver in ACPI has exclusive access.
We may want to disable fwcfg after OS bootup at all in the feature. Who knows what kind of sensitive information we may want to pass by it in the feature? May be something TPM related?
fwcfg is for passing information to the guest. If you want to hide something from the guest, just don't put it in fwcfg.
And I do not see any advantage of using fwcfg from AML.
It's an alternative to patching AML. Sure it takes some effort to write the driver, but afterwards we can modify the guest behaviour more easily. One possible client is -M old, so you can revert to previous behaviour depending on fwcfg data. (we don't need a driver in AML to avoid patching, we can have AML talk to the bios and the bios drive fwcfg; but I think we'll find uses for a driver).
Regardless, presence of _S3 name or method is all that needed for OS enabling S3 option. If _S3 is defined as a method it has to return Package() otherwise iasl refuses to compile it.
Can't we Return (Package (...) { ... }) or equivalent?
We can, how does it help?
The contents of the package can be determined at runtime. -- error compiling committee.c: too many arguments to function
On Sun, May 20, 2012 at 03:47:02PM +0300, Avi Kivity wrote:
On 05/20/2012 03:36 PM, Gleb Natapov wrote:
On Sun, May 20, 2012 at 03:30:50PM +0300, Avi Kivity wrote:
On 05/20/2012 03:15 PM, Gleb Natapov wrote:
On Sun, May 20, 2012 at 02:44:51PM +0300, Avi Kivity wrote:
On 05/20/2012 12:03 PM, 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.
Do we actually have to patch the DSDT? Or can _S3 etc be made into functions instead? (and talk to the bios, or even to fwcfg directly?)
We better not talk to fwcfg after OSPM is started since this is firmware confing interface.
Why not? The OS isn't going to talk to it, so we can have a driver in ACPI.
The OS is going to talk to it since the OS is the one who interprets AML.
I meant, not directly. So the driver in ACPI has exclusive access.
What's the difference?
We may want to disable fwcfg after OS bootup at all in the feature. Who knows what kind of sensitive information we may want to pass by it in the feature? May be something TPM related?
fwcfg is for passing information to the guest. If you want to hide something from the guest, just don't put it in fwcfg.
Where to put it if we want to pass it to a firmware, but not an OS. That was the point of fwcfg. If you want to pass something to a guest OS use virtio-serial.
And I do not see any advantage of using fwcfg from AML.
It's an alternative to patching AML. Sure it takes some effort to write the driver, but afterwards we can modify the guest behaviour more easily. One possible client is -M old, so you can revert to previous behaviour depending on fwcfg data. -M old is easy to support with the current patch. You just set new properties to compatibility values. The code is written with this in mind. And this is not an alternative to patching AML as I am trying to explain to you below. You can eliminate patching of s4 value, but that's it, you still need to patch out _S3/_S4 names.
(we don't need a driver in AML to avoid patching, we can have AML talk to the bios and the bios drive fwcfg; but I think we'll find uses for a driver).
I am not sure what you mean. AML can't talk to the bios. It can read values that bios put somewhere. I do not see advantage of this method and it requires patching still.
Regardless, presence of _S3 name or method is all that needed for OS enabling S3 option. If _S3 is defined as a method it has to return Package() otherwise iasl refuses to compile it.
Can't we Return (Package (...) { ... }) or equivalent?
We can, how does it help?
The contents of the package can be determined at runtime.
And? _S3 name should not exists at all in order to disable S3, not return something different. -- Gleb.
On 05/20/2012 03:59 PM, Gleb Natapov wrote:
Do we actually have to patch the DSDT? Or can _S3 etc be made into functions instead? (and talk to the bios, or even to fwcfg directly?)
We better not talk to fwcfg after OSPM is started since this is firmware confing interface.
Why not? The OS isn't going to talk to it, so we can have a driver in ACPI.
The OS is going to talk to it since the OS is the one who interprets AML.
I meant, not directly. So the driver in ACPI has exclusive access.
What's the difference?
ACPI is firmware, not OS.
We may want to disable fwcfg after OS bootup at all in the feature. Who knows what kind of sensitive information we may want to pass by it in the feature? May be something TPM related?
fwcfg is for passing information to the guest. If you want to hide something from the guest, just don't put it in fwcfg.
Where to put it if we want to pass it to a firmware, but not an OS. That was the point of fwcfg. If you want to pass something to a guest OS use virtio-serial.
See above.
And I do not see any advantage of using fwcfg from AML.
It's an alternative to patching AML. Sure it takes some effort to write the driver, but afterwards we can modify the guest behaviour more easily. One possible client is -M old, so you can revert to previous behaviour depending on fwcfg data. -M old is easy to support with the current patch. You just set new properties to compatibility values. The code is written with this in mind. And this is not an alternative to patching AML as I am trying to explain to you below. You can eliminate patching of s4 value, but that's it, you still need to patch out _S3/_S4 names.
What about If (Fcfg(...)) { Method()... } ? (i.e.. define the method conditionally at runtime)
(we don't need a driver in AML to avoid patching, we can have AML talk to the bios and the bios drive fwcfg; but I think we'll find uses for a driver).
I am not sure what you mean. AML can't talk to the bios. It can read values that bios put somewhere.
That's what I meant - communicate through memory.
I do not see advantage of this method and it requires patching still.
For the existence of the names? Yes, if we can't avoid it it's a problem. But if we can avoid patching, we should.
Regardless, presence of _S3 name or method is all that needed for OS enabling S3 option. If _S3 is defined as a method it has to return Package() otherwise iasl refuses to compile it.
Can't we Return (Package (...) { ... }) or equivalent?
We can, how does it help?
The contents of the package can be determined at runtime.
And? _S3 name should not exists at all in order to disable S3, not return something different.
See above. -- error compiling committee.c: too many arguments to function
On Sun, May 20, 2012 at 04:39:01PM +0300, Avi Kivity wrote:
On 05/20/2012 03:59 PM, Gleb Natapov wrote:
> > Do we actually have to patch the DSDT? Or can _S3 etc be made into > functions instead? (and talk to the bios, or even to fwcfg directly?) > We better not talk to fwcfg after OSPM is started since this is firmware confing interface.
Why not? The OS isn't going to talk to it, so we can have a driver in ACPI.
The OS is going to talk to it since the OS is the one who interprets AML.
I meant, not directly. So the driver in ACPI has exclusive access.
What's the difference?
ACPI is firmware, not OS. AML is a data provided by firmware. AML's runtime is different from firmware's.
We may want to disable fwcfg after OS bootup at all in the feature. Who knows what kind of sensitive information we may want to pass by it in the feature? May be something TPM related?
fwcfg is for passing information to the guest. If you want to hide something from the guest, just don't put it in fwcfg.
Where to put it if we want to pass it to a firmware, but not an OS. That was the point of fwcfg. If you want to pass something to a guest OS use virtio-serial.
See above.
And I do not see any advantage of using fwcfg from AML.
It's an alternative to patching AML. Sure it takes some effort to write the driver, but afterwards we can modify the guest behaviour more easily. One possible client is -M old, so you can revert to previous behaviour depending on fwcfg data. -M old is easy to support with the current patch. You just set new properties to compatibility values. The code is written with this in mind. And this is not an alternative to patching AML as I am trying to explain to you below. You can eliminate patching of s4 value, but that's it, you still need to patch out _S3/_S4 names.
What about
If (Fcfg(...)) { Method()... }
?
syntax error, unexpected PARSEOP_IF
(i.e.. define the method conditionally at runtime)
(we don't need a driver in AML to avoid patching, we can have AML talk to the bios and the bios drive fwcfg; but I think we'll find uses for a driver).
I am not sure what you mean. AML can't talk to the bios. It can read values that bios put somewhere.
That's what I meant - communicate through memory.
What's the benefit? The patching is still needed. You need to pass address of OperationRegion() to AML. You can do it either by patching or by creating OperationRegion() code dynamically.
I do not see advantage of this method and it requires patching still.
For the existence of the names? Yes, if we can't avoid it it's a problem. But if we can avoid patching, we should.
If we can, we should, but we can't as far as I see. The patching was here long before this patch.
Regardless, presence of _S3 name or method is all that needed for OS enabling S3 option. If _S3 is defined as a method it has to return Package() otherwise iasl refuses to compile it.
Can't we Return (Package (...) { ... }) or equivalent?
We can, how does it help?
The contents of the package can be determined at runtime.
And? _S3 name should not exists at all in order to disable S3, not return something different.
See above.
Does not work for me, can you send me a patch that works? -- Gleb.
On 05/20/2012 04:57 PM, Gleb Natapov wrote:
On Sun, May 20, 2012 at 04:39:01PM +0300, Avi Kivity wrote:
On 05/20/2012 03:59 PM, Gleb Natapov wrote:
> > > > Do we actually have to patch the DSDT? Or can _S3 etc be made into > > functions instead? (and talk to the bios, or even to fwcfg directly?) > > > We better not talk to fwcfg after OSPM is started since this is firmware > confing interface.
Why not? The OS isn't going to talk to it, so we can have a driver in ACPI.
The OS is going to talk to it since the OS is the one who interprets AML.
I meant, not directly. So the driver in ACPI has exclusive access.
What's the difference?
ACPI is firmware, not OS. AML is a data provided by firmware. AML's runtime is different from firmware's.
It's still firmware.
It's an alternative to patching AML. Sure it takes some effort to write the driver, but afterwards we can modify the guest behaviour more easily. One possible client is -M old, so you can revert to previous behaviour depending on fwcfg data.
-M old is easy to support with the current patch. You just set new properties to compatibility values. The code is written with this in mind. And this is not an alternative to patching AML as I am trying to explain to you below. You can eliminate patching of s4 value, but that's it, you still need to patch out _S3/_S4 names.
What about
If (Fcfg(...)) { Method()... }
? syntax error, unexpected PARSEOP_IF
Unfortunately the ACPI spec forbids this construct, so either patching or double complication is necessary.
(i.e.. define the method conditionally at runtime)
(we don't need a driver in AML to avoid patching, we can have AML talk to the bios and the bios drive fwcfg; but I think we'll find uses for a driver).
I am not sure what you mean. AML can't talk to the bios. It can read values that bios put somewhere.
That's what I meant - communicate through memory.
What's the benefit? The patching is still needed. You need to pass address of OperationRegion() to AML. You can do it either by patching or by creating OperationRegion() code dynamically.
Or it can be a fixed address in low memory, or a scratch register in hardware.
I do not see advantage of this method and it requires patching still.
For the existence of the names? Yes, if we can't avoid it it's a problem. But if we can avoid patching, we should.
If we can, we should, but we can't as far as I see. The patching was here long before this patch.
I agree we probably can't. -- error compiling committee.c: too many arguments to function
On Sun, May 20, 2012 at 05:34:56PM +0300, Avi Kivity wrote:
On 05/20/2012 04:57 PM, Gleb Natapov wrote:
On Sun, May 20, 2012 at 04:39:01PM +0300, Avi Kivity wrote:
On 05/20/2012 03:59 PM, Gleb Natapov wrote:
> > > > > > Do we actually have to patch the DSDT? Or can _S3 etc be made into > > > functions instead? (and talk to the bios, or even to fwcfg directly?) > > > > > We better not talk to fwcfg after OSPM is started since this is firmware > > confing interface. > > Why not? The OS isn't going to talk to it, so we can have a driver in ACPI. > The OS is going to talk to it since the OS is the one who interprets AML.
I meant, not directly. So the driver in ACPI has exclusive access.
What's the difference?
ACPI is firmware, not OS. AML is a data provided by firmware. AML's runtime is different from firmware's.
It's still firmware.
We have to agree to disagree here :) It's just a data for OS to use as far as I am concern.
It's an alternative to patching AML. Sure it takes some effort to write the driver, but afterwards we can modify the guest behaviour more easily. One possible client is -M old, so you can revert to previous behaviour depending on fwcfg data.
-M old is easy to support with the current patch. You just set new properties to compatibility values. The code is written with this in mind. And this is not an alternative to patching AML as I am trying to explain to you below. You can eliminate patching of s4 value, but that's it, you still need to patch out _S3/_S4 names.
What about
If (Fcfg(...)) { Method()... }
? syntax error, unexpected PARSEOP_IF
Unfortunately the ACPI spec forbids this construct, so either patching or double complication is necessary.
It's not double if we will take all possible combinations into account.
(i.e.. define the method conditionally at runtime)
(we don't need a driver in AML to avoid patching, we can have AML talk to the bios and the bios drive fwcfg; but I think we'll find uses for a driver).
I am not sure what you mean. AML can't talk to the bios. It can read values that bios put somewhere.
That's what I meant - communicate through memory.
What's the benefit? The patching is still needed. You need to pass address of OperationRegion() to AML. You can do it either by patching or by creating OperationRegion() code dynamically.
Or it can be a fixed address in low memory, or a scratch register in hardware.
Both will work (fixed addresses are better be avoided and who needs another PV device), but I do not see how either of them is better then patching. What is your concern?
I do not see advantage of this method and it requires patching still.
For the existence of the names? Yes, if we can't avoid it it's a problem. But if we can avoid patching, we should.
If we can, we should, but we can't as far as I see. The patching was here long before this patch.
I agree we probably can't.
-- error compiling committee.c: too many arguments to function
-- Gleb.
On 05/20/2012 05:43 PM, Gleb Natapov wrote:
Or it can be a fixed address in low memory, or a scratch register in hardware.
Both will work (fixed addresses are better be avoided and who needs another PV device), but I do not see how either of them is better then patching. What is your concern?
Patching is harder to maintain. Unfortunately it's unavoidable. -- error compiling committee.c: too many arguments to function
On Sun, May 20, 2012 at 05:46:46PM +0300, Avi Kivity wrote:
On 05/20/2012 05:43 PM, Gleb Natapov wrote:
Or it can be a fixed address in low memory, or a scratch register in hardware.
Both will work (fixed addresses are better be avoided and who needs another PV device), but I do not see how either of them is better then patching. What is your concern?
Patching is harder to maintain. Unfortunately it's unavoidable.
Here we in agreement, and I was against patching till it was unavoidable, but than pci hotplug started using it, and afterwards processor definitions, so no point in avoiding it now by using inferior methods. -- Gleb.
On Sun, May 20, 2012 at 06:15:44PM +0300, Gleb Natapov wrote:
On Sun, May 20, 2012 at 05:46:46PM +0300, Avi Kivity wrote:
On 05/20/2012 05:43 PM, Gleb Natapov wrote:
Or it can be a fixed address in low memory, or a scratch register in hardware.
Both will work (fixed addresses are better be avoided and who needs another PV device), but I do not see how either of them is better then patching. What is your concern?
Patching is harder to maintain. Unfortunately it's unavoidable.
Here we in agreement, and I was against patching till it was unavoidable, but than pci hotplug started using it, and afterwards processor definitions, so no point in avoiding it now by using inferior methods.
I agree as well. What's the background to needing to have dynamic S3/S4 definitions? (Why will some qemu instances be able to sleep and not others?) -Kevin
On 05/20/2012 07:16 PM, Kevin O'Connor wrote:
Here we in agreement, and I was against patching till it was unavoidable, but than pci hotplug started using it, and afterwards processor definitions, so no point in avoiding it now by using inferior methods.
I agree as well.
What's the background to needing to have dynamic S3/S4 definitions? (Why will some qemu instances be able to sleep and not others?)
Backwards compatibility. qemu has a -M machine-type option that expose an old qemu's guest-visible attributes. If an old qemu didn't support S3, then -M old shouldn't either. -- error compiling committee.c: too many arguments to function
On Sun, May 20, 2012 at 07:25:40PM +0300, Avi Kivity wrote:
On 05/20/2012 07:16 PM, Kevin O'Connor wrote:
Here we in agreement, and I was against patching till it was unavoidable, but than pci hotplug started using it, and afterwards processor definitions, so no point in avoiding it now by using inferior methods.
I agree as well.
What's the background to needing to have dynamic S3/S4 definitions? (Why will some qemu instances be able to sleep and not others?)
Backwards compatibility. qemu has a -M machine-type option that expose an old qemu's guest-visible attributes. If an old qemu didn't support S3, then -M old shouldn't either.
The DSDT has claimed S3, S4, and S5 support since SeaBIOS has supported ACPI. What's the background to the requirement to stop claiming support for it? -Kevin
On Sun, May 20, 2012 at 12:39:13PM -0400, Kevin O'Connor wrote:
On Sun, May 20, 2012 at 07:25:40PM +0300, Avi Kivity wrote:
On 05/20/2012 07:16 PM, Kevin O'Connor wrote:
Here we in agreement, and I was against patching till it was unavoidable, but than pci hotplug started using it, and afterwards processor definitions, so no point in avoiding it now by using inferior methods.
I agree as well.
What's the background to needing to have dynamic S3/S4 definitions? (Why will some qemu instances be able to sleep and not others?)
Backwards compatibility. qemu has a -M machine-type option that expose an old qemu's guest-visible attributes. If an old qemu didn't support S3, then -M old shouldn't either.
The DSDT has claimed S3, S4, and S5 support since SeaBIOS has supported ACPI. What's the background to the requirement to stop claiming support for it?
Not all guests have working S3/S4 implementation. We want management to be able to disable S3/S4 for such guests. S4 value changes since we want to distinguish between S4 and S5 in qemu. -- Gleb.
On Sun, May 20, 2012 at 04:39:01PM +0300, Avi Kivity wrote:
What about
If (Fcfg(...)) { Method()... }
?
(i.e.. define the method conditionally at runtime)
As Gleb points out, this wont work. AML defines a static device/method/variable tree heirarchy. Only the return values of methods is programmable. This completely confused me when I first started looking at AML. ACPI is truly a bizarre spec. -Kevin
On Sun, May 20, 2012 at 12:03:38PM +0300, Gleb Natapov wrote:
Signed-off-by: Gleb Natapov <gleb@redhat.com> --- tools/acpi_extract.py | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/tools/acpi_extract.py b/tools/acpi_extract.py index 5f613e4..8038269 100755 --- a/tools/acpi_extract.py +++ b/tools/acpi_extract.py @@ -121,7 +121,11 @@ def aml_name_string(offset): if (aml[offset] != 0x08): die( "Name offset 0x%x: expected 0x08 actual 0x%x" % (offset, aml[offset])); - return offset + 1; + offset += 1 + # Block Name Modifier. Skip it. + if (aml[offset] == 0x5c or aml[offset] == 0x5e):
You don't need parenthesis around the whole conditional.
+ offset += 1 + return offset;
# Given data offset, find dword const offset def aml_data_dword_const(offset): -- 1.7.7.3
_______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
On Sun, May 20, 2012 at 01:32:18PM +0300, Alon Levy wrote:
On Sun, May 20, 2012 at 12:03:38PM +0300, Gleb Natapov wrote:
Signed-off-by: Gleb Natapov <gleb@redhat.com> --- tools/acpi_extract.py | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/tools/acpi_extract.py b/tools/acpi_extract.py index 5f613e4..8038269 100755 --- a/tools/acpi_extract.py +++ b/tools/acpi_extract.py @@ -121,7 +121,11 @@ def aml_name_string(offset): if (aml[offset] != 0x08): die( "Name offset 0x%x: expected 0x08 actual 0x%x" % (offset, aml[offset])); - return offset + 1; + offset += 1 + # Block Name Modifier. Skip it. + if (aml[offset] == 0x5c or aml[offset] == 0x5e):
You don't need parenthesis around the whole conditional. Rest of the code has it. Better to keep same style :)
+ offset += 1 + return offset;
# Given data offset, find dword const offset def aml_data_dword_const(offset): -- 1.7.7.3
_______________________________________________ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
-- Gleb.
On Sun, May 20, 2012 at 12:03:38PM +0300, Gleb Natapov wrote:
Signed-off-by: Gleb Natapov <gleb@redhat.com>
The patch series looks okay to me. Let me know when the corresponding qemu patches are committed. -Kevin
On Tue, May 22, 2012 at 09:23:03PM -0400, Kevin O'Connor wrote:
On Sun, May 20, 2012 at 12:03:38PM +0300, Gleb Natapov wrote:
Signed-off-by: Gleb Natapov <gleb@redhat.com>
The patch series looks okay to me. Let me know when the corresponding qemu patches are committed.
It is committed now: 459ae5ea5ad682c2b3220beb244d4102c1a4e332 -- Gleb.
On Wed, Jun 20, 2012 at 04:08:41PM +0300, Gleb Natapov wrote:
On Tue, May 22, 2012 at 09:23:03PM -0400, Kevin O'Connor wrote:
On Sun, May 20, 2012 at 12:03:38PM +0300, Gleb Natapov wrote:
Signed-off-by: Gleb Natapov <gleb@redhat.com>
The patch series looks okay to me. Let me know when the corresponding qemu patches are committed.
It is committed now: 459ae5ea5ad682c2b3220beb244d4102c1a4e332
Thanks. I committed your seabios patches. -Kevin
participants (4)
-
Alon Levy -
Avi Kivity -
Gleb Natapov -
Kevin O'Connor