[Qemu-devel] [PATCH 1/3] Fix aml_name_string() to recognize block name modifiers.

[Qemu-devel] [PATCH 1/3] Fix aml_name_string() to recognize block name modifiers.

[Gleb Natapov]

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

[Qemu-devel] [PATCH 2/3] Add ACPI_EXTRACT_PKG_START macro parsing

[Gleb Natapov]

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-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Gleb Natapov]

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

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Avi Kivity]

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

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Avi Kivity]

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

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Avi Kivity]

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

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Avi Kivity]

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

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Avi Kivity]

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

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Avi Kivity]

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

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Kevin O'Connor]

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

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Avi Kivity]

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

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Kevin O'Connor]

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

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 3/3] Get system state configuration from QEMU and patch DSDT with it.

[Kevin O'Connor]

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

Re: [Qemu-devel] [SeaBIOS] [PATCH 1/3] Fix aml_name_string() to recognize block name modifiers.

[Alon Levy]

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

Re: [Qemu-devel] [SeaBIOS] [PATCH 1/3] Fix aml_name_string() to recognize block name modifiers.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 1/3] Fix aml_name_string() to recognize block name modifiers.

[Kevin O'Connor]

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

Re: [Qemu-devel] [PATCH 1/3] Fix aml_name_string() to recognize block name modifiers.

[Gleb Natapov]

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.

Re: [Qemu-devel] [PATCH 1/3] Fix aml_name_string() to recognize block name modifiers.

[Kevin O'Connor]

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