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

[Qemu-devel] [PATCH 1/2] 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 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)
 
-- 
1.7.7.3

[Qemu-devel] [PATCH 2/2] Get system state configuration from QEMU and patcth 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 |    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
-- 
1.7.7.3

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

[Kevin O'Connor]

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

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

[Gleb Natapov]

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.

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

[Kevin O'Connor]

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

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

[Gleb Natapov]

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.

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

[Paolo Bonzini]

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

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

[Gleb Natapov]

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.

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

[Paolo Bonzini]

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

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

[Gleb Natapov]

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.

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

[Kevin O'Connor]

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

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

[Paolo Bonzini]

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

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

[Kevin O'Connor]

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