[PATCH 0/4] acpi: fix up EJ0 in DSDT

[PATCH 0/4] acpi: fix up EJ0 in DSDT

[Michael S. Tsirkin]

Here's a bug: guest thinks it can eject VGA device and ISA bridge.

[root@dhcp74-172 ~]#lspci
00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02)
00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II]
00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II]
00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03)
00:02.0 VGA compatible controller: Cirrus Logic GD 5446
00:03.0 PCI bridge: Red Hat, Inc. Device 0001
00:04.0 Ethernet controller: Qumranet, Inc. Virtio network device
00:05.0 SCSI storage controller: Qumranet, Inc. Virtio block device
01:00.0 Ethernet controller: Intel Corporation 82540EM Gigabit Ethernet Controller (rev 03)

[root@dhcp74-172 ~]# ls /sys/bus/pci/slots/1/
adapter  address  attention  latch  module  power
[root@dhcp74-172 ~]# ls /sys/bus/pci/slots/2/
adapter  address  attention  latch  module  power

[root@dhcp74-172 ~]# echo 0 > /sys/bus/pci/slots/2/power 
[root@dhcp74-172 ~]# lspci
00:00.0 Host bridge: Intel Corporation 440FX - 82441FX PMC [Natoma] (rev 02)
00:01.0 ISA bridge: Intel Corporation 82371SB PIIX3 ISA [Natoma/Triton II]
00:01.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton II]
00:01.3 Bridge: Intel Corporation 82371AB/EB/MB PIIX4 ACPI (rev 03)
00:03.0 PCI bridge: Red Hat, Inc. Device 0001
00:04.0 Ethernet controller: Qumranet, Inc. Virtio network device
00:05.0 SCSI storage controller: Qumranet, Inc. Virtio block device
01:00.0 Ethernet controller: Intel Corporation 82540EM Gigabit Ethernet Controller (rev 03)

This is wrong because slots 1 and 2 are marked as not hotpluggable
in qemu.

The reason is that our acpi tables declare both _RMV with value 0,
and _EJ0 method for these slots. What happens in this case
is undocumented by ACPI spec, so linux ignores _RMV,
and windows seems to ignore _EJ0.

The correct way to suppress hotplug is not to have _EJ0,
so this is what this patch does: it probes PIIX and
modifies DSDT to match.

With these patches applied, we get:

[root@dhcp74-172 ~]# ls /sys/bus/pci/slots/1/
address
[root@dhcp74-172 ~]# ls /sys/bus/pci/slots/2/
address

I had to add a bit of compile-time infrastructure to handle the
runtime patching in a simple way. I expect that
it will be possible to reuse it if we need to
patch other methods in the future.

Michael S. Tsirkin (4):
  acpi: generate mixed asl/aml listing
  acpi: add aml/asl parsing script
  acpi: EJ0 method name patching
  acpi: remove _RMV

 Makefile          |   10 ++--
 src/acpi-dsdt.dsl |   58 +++--------------------
 src/acpi.c        |   11 ++++
 src/find_ej0.pl   |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/splitdsl.pl   |   16 ++++++
 5 files changed, 176 insertions(+), 55 deletions(-)
 create mode 100755 src/find_ej0.pl
 create mode 100755 src/splitdsl.pl

-- 
1.7.5.53.gc233e

[PATCH 1/4] acpi: generate mixed asl/aml listing

[Michael S. Tsirkin]

Use iasl -l flag to produce a mixed listing, where a
source line is followed by matching AML.
Since we use a preprocessed input, this generates long lines with
multiple commands per line. To make it possible to match
AML to source exactly, split the input that we supply iasl,
with each command on a separate line.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 Makefile        |    7 ++++---
 src/splitdsl.pl |   16 ++++++++++++++++
 2 files changed, 20 insertions(+), 3 deletions(-)
 create mode 100755 src/splitdsl.pl

diff --git a/Makefile b/Makefile
index 109091b..5c011bb 100644
--- a/Makefile
+++ b/Makefile
@@ -192,10 +192,11 @@ $(OUT)vgabios.bin: $(OUT)vgabios.bin.raw tools/buildrom.py
 	$(Q)./tools/buildrom.py $< $@
 
 ####### dsdt build rules
-src/%.hex: src/%.dsl
+src/%.hex: src/%.dsl src/splitdsl.pl src/find_ej0.pl
 	@echo "Compiling DSDT"
-	$(Q)cpp -P $< > $(OUT)$*.dsl.i
-	$(Q)iasl -tc -p $(OUT)$* $(OUT)$*.dsl.i
+	$(Q)cpp -P $< > $(OUT)$*.dsl.i.orig
+	$(Q)./src/splitdsl.pl $(OUT)$*.dsl.i.orig > $(OUT)$*.dsl.i
+	$(Q)iasl -l -tc -p $(OUT)$* $(OUT)$*.dsl.i
 	$(Q)cp $(OUT)$*.hex $@
 
 $(OUT)ccode32flat.o: src/acpi-dsdt.hex
diff --git a/src/splitdsl.pl b/src/splitdsl.pl
new file mode 100755
index 0000000..1caccba
--- /dev/null
+++ b/src/splitdsl.pl
@@ -0,0 +1,16 @@
+#!/usr/bin/perl
+
+while (<>)
+{
+	#Remove // comments if any as we can not split lines after them
+	s#//.*##;
+	#Code after { starts a new line
+	s/(\{)(\s*\S)/$1\n$2/g;
+	#Operator after ) starts a new line
+	s/(\))(\s*[_A-Za-z])/$1\n$2/g;
+	#Code after } starts a new line
+	s/(})(\s*\S)/$1\n$2/g;
+	#} starts a new line
+	s/(\S\s*)(\})/$1\n$2/g;
+	print;
+}
-- 
1.7.5.53.gc233e

[PATCH 2/4] acpi: add aml/asl parsing script

[Michael S. Tsirkin]

script ./src/find_ej0.pl finds all instances of
method named EJ0_ and the matching _ADR information,
and outputs the AML offset and slot mask of each.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 src/find_ej0.pl |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 136 insertions(+), 0 deletions(-)
 create mode 100755 src/find_ej0.pl

diff --git a/src/find_ej0.pl b/src/find_ej0.pl
new file mode 100755
index 0000000..37e8a8c
--- /dev/null
+++ b/src/find_ej0.pl
@@ -0,0 +1,136 @@
+#!/usr/bin/perl
+
+# Process mixed ASL/AML listing (.lst file) produced by iasl -l
+# Locate all occurences of Name _ADR followed by Method EJ0_
+# Output slot info from _ADR and offset of method name in AML
+
+use strict;
+
+my @aml = ();
+my @asl = ();
+my @asl_lineno = ();
+my @asl_to_aml_offset = ();
+my @output = ();
+
+#Store an ASL command, matching AML offset, and input line (for debugging)
+sub add_asl {
+    my $srcline = shift(@_);
+    my $line = shift(@_);
+    push @asl, $line;
+    push @asl_lineno, $.;
+    push @asl_to_aml_offset, $#aml + 1;
+}
+
+#Store an AML byte sequence
+#Verify that offset output by iasl matches # of bytes so far
+sub add_aml {
+    my $offset = shift(@_);
+    my $line = shift(@_);
+    my $o = hex($offset);
+    # Sanity check: offset must match
+    die "Offset $o != " .($#aml + 1) if ($o != $#aml + 1);
+    # Strip any traling dots and ASCII dump after "
+    $line =~ s/\s*\.*\s*".*//;
+
+    my @code = split(' ', $line);
+    foreach my $c (@code) {
+        if (not($c =~ m/^[0-9A-Fa-f][0-9A-Fa-f]$/)) {
+            die "Unexpected octet $c";
+        }
+        push @aml, hex($c);
+    }
+}
+
+# Process aml bytecode array, decoding AML
+# Given method offset, find its name offset
+sub aml_method_name {
+    my $lineno = shift(@_);
+    my $offset = shift(@_);
+    #0x14 MethodOp PkgLength NameString MethodFlags TermList
+    if ($aml[$offset] != 0x14) {
+        die "Method after input line $lineno offset $offset: " .
+            " expected 0x14 actual ". sprintf("0x%x", $aml[$offset]);
+    }
+    $offset += 1;
+    # PkgLength can be multibyte. Bits 8-7 give the # of extra bytes.
+    my $pkglenbytes = $aml[$offset] >> 6;
+    $offset += 1 + $pkglenbytes;
+    return $offset;
+}
+
+while (<>) {
+        #Strip trailing newline
+        chomp;
+	#ASL listing: space, then line#, then ...., then code
+	if (s#^\s+([0-9]+)\.\.\.\.\s*##) {
+            add_asl($1, $_);
+	}
+        # AML listing: offset in hex, then ...., then code
+	if (s#^([0-9A-F]+)\.\.\.\.\s*##) {
+            add_aml($1, $_);
+        }
+	# Ignore everything else
+}
+
+# Now go over code, look for EJ0_ methods
+# For each such method, output slot mask from the
+# preceding _ADR line, as well as the method name offset.
+for (my $i = 0; $i <= $#asl; $i++) {
+    my $l = $asl[$i];
+    # match: Method (EJ0_,1)
+    if (not $l =~ m#^Method\s*\(\s*EJ0_\s*[,)]#) {
+        # Make sure we do not miss any EJ0_:
+        # die if EJ0_ is found anywhere else in source code
+        if ($l =~ m#EJ0_#) {
+            die "Stray EJ0_ detected at input line $asl_lineno[$i]: $asl[$i]";
+        }
+        next;
+    }
+    # EJ0_ found. Previous line must be _ADR
+    my $p = $i > 0 ? $asl[$i - 1] : "";
+    # match: Name (_ADR, 0x<address>)
+    if (not ($p =~ m#Name\s*\(\s*_ADR\s*,\s*0x([0-9A-Fa-f]+)\s*\)#)) {
+        die "_ADR not found before EJ0_ ".
+            "at input line $asl_lineno[$i]: $asl[$i]";
+    }
+    my $adr = hex($1);
+    my $slot = $adr >> 16;
+    if ($slot > 31) {
+        die "_ADR device out of range: actual $slot " .
+            "expected 0 to 31 at input line $asl_lineno[$i]: $asl[$i]";
+    }
+
+    # We have offset of EJ0_ method in code
+    # Now find EJ0_ itself
+    my $offset = $asl_to_aml_offset[$i];
+    my $ej0 = aml_method_name($asl_lineno[$i], $offset);
+    # Verify AML: name must be EJ0_:
+    if (($aml[$ej0 + 0] != ord('E')) or
+        ($aml[$ej0 + 1] != ord('J')) or
+        ($aml[$ej0 + 2] != ord('0')) or
+        ($aml[$ej0 + 3] != ord('_'))) {
+        die "AML after input line $asl_lineno[$i] offset $ej0 " .
+            "does not match EJ0_";
+    }
+
+    # OK we are done. Output slot mask and offset
+    push @output, sprintf("    {.slot_mask = 0x%x, .offset = 0x%x}",
+                          0x1 << $slot, $ej0);
+}
+
+# Pretty print output
+if ($#output < 0) {
+    die "No EJ0_ Method found!"
+}
+print <<EOF;
+static struct aml_ej0_data {
+    unsigned slot_mask;
+    unsigned offset;
+} aml_ej0_data[] = {
+EOF
+print join(",\n", @output);
+print <<EOF;
+
+};
+EOF
+
-- 
1.7.5.53.gc233e

[PATCH 3/4] acpi: EJ0 method name patching

[Michael S. Tsirkin]

Modify ACPI to only supply _EJ0 methods for PCI
slots that support hotplug.

This is done by runtime patching:
- Rename _EJ0 methods for PCI slots in DSDT to EJ0_:
  note that this has the same checksum, but
  is ignored by OSPM.
- At compile time, look for these methods in ASL source,
  find the matching AML,  and store the offsets of these methods
  in a table named aml_ej0_data.
  Note that we are looking for EJ0_ in source code,
  so we'll be able to write EJ0 if we want to and the script
  will not match it.
- At run time, go over aml_ej0_data, check which slots
  support hotplug and patch the ACPI table, replacing EJ0_ with _EJ0.

Note: the method used is robust in that we don't need
to change any offsets manually in case of ASL code changes.
As all parsing is done at compile time, any unexpected input causes
build failure, not a runtime failure.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 Makefile          |    3 ++-
 src/acpi-dsdt.dsl |    9 +++++++--
 src/acpi.c        |   11 +++++++++++
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 5c011bb..dee93d6 100644
--- a/Makefile
+++ b/Makefile
@@ -197,7 +197,8 @@ src/%.hex: src/%.dsl src/splitdsl.pl src/find_ej0.pl
 	$(Q)cpp -P $< > $(OUT)$*.dsl.i.orig
 	$(Q)./src/splitdsl.pl $(OUT)$*.dsl.i.orig > $(OUT)$*.dsl.i
 	$(Q)iasl -l -tc -p $(OUT)$* $(OUT)$*.dsl.i
-	$(Q)cp $(OUT)$*.hex $@
+	$(Q)./src/find_ej0.pl $(OUT)$*.lst > $(OUT)$*.off
+	$(Q)cat $(OUT)$*.hex $(OUT)$*.off > $@
 
 $(OUT)ccode32flat.o: src/acpi-dsdt.hex
 
diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index 08412e2..3d43e4b 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -127,11 +127,16 @@ DefinitionBlock (
             {
                 PCRM, 32,
             }
-
+            // Method EJ0_ will be patched by BIOS to _EJ0
+            // at runtime, if the slot is detected to support hotplug.
+            // Must be immediately preceded by _ADR for this to work.
+            // EJ0_ is not allowed anywhere else in this file,
+            // if you really want to use it, write EJ0 which
+            // creates the same AML but isn't patched.
 #define hotplug_slot(name, nr) \
             Device (S##name) {                    \
                Name (_ADR, nr##0000)              \
-               Method (_EJ0,1) {                  \
+               Method (EJ0_,1) {                  \
                     Store(ShiftLeft(1, nr), B0EJ) \
                     Return (0x0)                  \
                }                                  \
diff --git a/src/acpi.c b/src/acpi.c
index 6bb6ff6..cbb5143 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -198,6 +198,8 @@ struct srat_memory_affinity
     u32    reserved3[2];
 } PACKED;
 
+#define PCI_RMV_BASE 0xae0c
+
 #include "acpi-dsdt.hex"
 
 static void
@@ -243,6 +245,8 @@ build_fadt(struct pci_device *pci)
     struct fadt_descriptor_rev1 *fadt = malloc_high(sizeof(*fadt));
     struct facs_descriptor_rev1 *facs = memalign_high(64, sizeof(*facs));
     void *dsdt = malloc_high(sizeof(AmlCode));
+    u32 rmvc_pcrm;
+    int i;
 
     if (!fadt || !facs || !dsdt) {
         warn_noalloc();
@@ -255,7 +259,13 @@ build_fadt(struct pci_device *pci)
     facs->length = cpu_to_le32(sizeof(*facs));
 
     /* DSDT */
     memcpy(dsdt, AmlCode, sizeof(AmlCode));
+    rmvc_pcrm = inl(PCI_RMV_BASE);
+    for (i = 0; i < sizeof(aml_ej0_data) / sizeof(*aml_ej0_data); ++i) {
+        if (rmvc_pcrm & aml_ej0_data[i].slot_mask) {
+            memcpy(dsdt + aml_ej0_data[i].offset, "_EJ0", 4);
+        }
+    }
 
     /* FADT */
     memset(fadt, 0, sizeof(*fadt));
-- 
1.7.5.53.gc233e

[PATCH 4/4] acpi: remove _RMV

[Michael S. Tsirkin]

The macro gen_pci_device is used to add _RMV
method to a slot device so it is no longer needed:
presence of _EJ0 now indicates that the slot is ejectable.
It is also placing two devices with the same _ADR
on the same bus, which isn't defined by the ACPI spec.
So let's remove it.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 src/acpi-dsdt.dsl |   49 -------------------------------------------------
 1 files changed, 0 insertions(+), 49 deletions(-)

diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index 3d43e4b..44a27e4 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -121,12 +121,6 @@ DefinitionBlock (
             {
                 B0EJ, 32,
             }
-
-            OperationRegion(RMVC, SystemIO, 0xae0c, 0x04)
-            Field(RMVC, DWordAcc, NoLock, WriteAsZeros)
-            {
-                PCRM, 32,
-            }
             // Method EJ0_ will be patched by BIOS to _EJ0
             // at runtime, if the slot is detected to support hotplug.
             // Must be immediately preceded by _ADR for this to work.
@@ -464,49 +458,6 @@ DefinitionBlock (
 		DRSJ, 32
 	    }
 	}
-
-#define gen_pci_device(name, nr)                                \
-        Device(SL##name) {                                      \
-            Name (_ADR, nr##0000)                               \
-            Method (_RMV) {                                     \
-                If (And(\_SB.PCI0.PCRM, ShiftLeft(1, nr))) {    \
-                    Return (0x1)                                \
-                }                                               \
-                Return (0x0)                                    \
-            }                                                   \
-            Name (_SUN, name)                                   \
-        }
-
-        /* VGA (slot 1) and ISA bus (slot 2) defined above */
-	gen_pci_device(3, 0x0003)
-	gen_pci_device(4, 0x0004)
-	gen_pci_device(5, 0x0005)
-	gen_pci_device(6, 0x0006)
-	gen_pci_device(7, 0x0007)
-	gen_pci_device(8, 0x0008)
-	gen_pci_device(9, 0x0009)
-	gen_pci_device(10, 0x000a)
-	gen_pci_device(11, 0x000b)
-	gen_pci_device(12, 0x000c)
-	gen_pci_device(13, 0x000d)
-	gen_pci_device(14, 0x000e)
-	gen_pci_device(15, 0x000f)
-	gen_pci_device(16, 0x0010)
-	gen_pci_device(17, 0x0011)
-	gen_pci_device(18, 0x0012)
-	gen_pci_device(19, 0x0013)
-	gen_pci_device(20, 0x0014)
-	gen_pci_device(21, 0x0015)
-	gen_pci_device(22, 0x0016)
-	gen_pci_device(23, 0x0017)
-	gen_pci_device(24, 0x0018)
-	gen_pci_device(25, 0x0019)
-	gen_pci_device(26, 0x001a)
-	gen_pci_device(27, 0x001b)
-	gen_pci_device(28, 0x001c)
-	gen_pci_device(29, 0x001d)
-	gen_pci_device(30, 0x001e)
-	gen_pci_device(31, 0x001f)
     }
 
     /* PCI IRQs */
-- 
1.7.5.53.gc233e

Re: [PATCH 1/4] acpi: generate mixed asl/aml listing

[Michael S. Tsirkin]

On Wed, Sep 21, 2011 at 03:44:21PM +0300, Michael S. Tsirkin wrote:
> Use iasl -l flag to produce a mixed listing, where a
> source line is followed by matching AML.
> Since we use a preprocessed input, this generates long lines with
> multiple commands per line. To make it possible to match
> AML to source exactly, split the input that we supply iasl,
> with each command on a separate line.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

I thought I'd add an example listing here:
     467....     Device (S1) {

0000080B....5B 82 25 53 31 5F 5F ...    "[.%S1__"

     468.... Name (_ADR, 0x00010000)

00000812....08 5F 41 44 52 0C 00 00     "._ADR..."
0000081A....01 00 ..................    ".."

     469.... Method (EJ0_,1) {

0000081C....14 0F 45 4A 30 5F 01 ...    "..EJ0_."

     470.... Store(ShiftLeft(1, 0x0001), B0EJ)

-- 
MST

Re: [PATCH 2/4] acpi: add aml/asl parsing script

[Gleb Natapov]

On Wed, Sep 21, 2011 at 03:44:29PM +0300, Michael S. Tsirkin wrote:
> script ./src/find_ej0.pl finds all instances of
> method named EJ0_ and the matching _ADR information,
> and outputs the AML offset and slot mask of each.
> 
There is tools/ directory for such kind of scripts. Most (if not all) of
scripts there are in python though. Perl should die painful death.

This approach delivers nice result, but since the script does not really
decodes AML, but tries to match ASL source code with regular expressions,
it introduces some assumptions to the code that make DSDT code less
hackable. I'll hate to be the one who will have to change PCI device
definitions in DSDT next time.

Generally speaking finding an offset of some scope in AML is useful not
only for PCI hotplug. For instance we want to make S3/S4 capability
configurable by a command line switch, but this also requires DSDT
patching and having automatic way to find _S3_/_S4_ offset is required
for that too (we do not what to find it by hand each time DSDT is
recompiled).

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  src/find_ej0.pl |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 136 insertions(+), 0 deletions(-)
>  create mode 100755 src/find_ej0.pl
> 
> diff --git a/src/find_ej0.pl b/src/find_ej0.pl
> new file mode 100755
> index 0000000..37e8a8c
> --- /dev/null
> +++ b/src/find_ej0.pl
> @@ -0,0 +1,136 @@
> +#!/usr/bin/perl
> +
> +# Process mixed ASL/AML listing (.lst file) produced by iasl -l
> +# Locate all occurences of Name _ADR followed by Method EJ0_
> +# Output slot info from _ADR and offset of method name in AML
> +
> +use strict;
> +
> +my @aml = ();
> +my @asl = ();
> +my @asl_lineno = ();
> +my @asl_to_aml_offset = ();
> +my @output = ();
> +
> +#Store an ASL command, matching AML offset, and input line (for debugging)
> +sub add_asl {
> +    my $srcline = shift(@_);
> +    my $line = shift(@_);
> +    push @asl, $line;
> +    push @asl_lineno, $.;
> +    push @asl_to_aml_offset, $#aml + 1;
> +}
> +
> +#Store an AML byte sequence
> +#Verify that offset output by iasl matches # of bytes so far
> +sub add_aml {
> +    my $offset = shift(@_);
> +    my $line = shift(@_);
> +    my $o = hex($offset);
> +    # Sanity check: offset must match
> +    die "Offset $o != " .($#aml + 1) if ($o != $#aml + 1);
> +    # Strip any traling dots and ASCII dump after "
> +    $line =~ s/\s*\.*\s*".*//;
> +
> +    my @code = split(' ', $line);
> +    foreach my $c (@code) {
> +        if (not($c =~ m/^[0-9A-Fa-f][0-9A-Fa-f]$/)) {
> +            die "Unexpected octet $c";
> +        }
> +        push @aml, hex($c);
> +    }
> +}
> +
> +# Process aml bytecode array, decoding AML
> +# Given method offset, find its name offset
> +sub aml_method_name {
> +    my $lineno = shift(@_);
> +    my $offset = shift(@_);
> +    #0x14 MethodOp PkgLength NameString MethodFlags TermList
> +    if ($aml[$offset] != 0x14) {
> +        die "Method after input line $lineno offset $offset: " .
> +            " expected 0x14 actual ". sprintf("0x%x", $aml[$offset]);
> +    }
> +    $offset += 1;
> +    # PkgLength can be multibyte. Bits 8-7 give the # of extra bytes.
> +    my $pkglenbytes = $aml[$offset] >> 6;
> +    $offset += 1 + $pkglenbytes;
> +    return $offset;
> +}
> +
> +while (<>) {
> +        #Strip trailing newline
> +        chomp;
> +	#ASL listing: space, then line#, then ...., then code
> +	if (s#^\s+([0-9]+)\.\.\.\.\s*##) {
> +            add_asl($1, $_);
> +	}
> +        # AML listing: offset in hex, then ...., then code
> +	if (s#^([0-9A-F]+)\.\.\.\.\s*##) {
> +            add_aml($1, $_);
> +        }
> +	# Ignore everything else
> +}
> +
> +# Now go over code, look for EJ0_ methods
> +# For each such method, output slot mask from the
> +# preceding _ADR line, as well as the method name offset.
> +for (my $i = 0; $i <= $#asl; $i++) {
> +    my $l = $asl[$i];
> +    # match: Method (EJ0_,1)
> +    if (not $l =~ m#^Method\s*\(\s*EJ0_\s*[,)]#) {
> +        # Make sure we do not miss any EJ0_:
> +        # die if EJ0_ is found anywhere else in source code
> +        if ($l =~ m#EJ0_#) {
> +            die "Stray EJ0_ detected at input line $asl_lineno[$i]: $asl[$i]";
> +        }
> +        next;
> +    }
> +    # EJ0_ found. Previous line must be _ADR
> +    my $p = $i > 0 ? $asl[$i - 1] : "";
> +    # match: Name (_ADR, 0x<address>)
> +    if (not ($p =~ m#Name\s*\(\s*_ADR\s*,\s*0x([0-9A-Fa-f]+)\s*\)#)) {
> +        die "_ADR not found before EJ0_ ".
> +            "at input line $asl_lineno[$i]: $asl[$i]";
> +    }
> +    my $adr = hex($1);
> +    my $slot = $adr >> 16;
> +    if ($slot > 31) {
> +        die "_ADR device out of range: actual $slot " .
> +            "expected 0 to 31 at input line $asl_lineno[$i]: $asl[$i]";
> +    }
> +
> +    # We have offset of EJ0_ method in code
> +    # Now find EJ0_ itself
> +    my $offset = $asl_to_aml_offset[$i];
> +    my $ej0 = aml_method_name($asl_lineno[$i], $offset);
> +    # Verify AML: name must be EJ0_:
> +    if (($aml[$ej0 + 0] != ord('E')) or
> +        ($aml[$ej0 + 1] != ord('J')) or
> +        ($aml[$ej0 + 2] != ord('0')) or
> +        ($aml[$ej0 + 3] != ord('_'))) {
> +        die "AML after input line $asl_lineno[$i] offset $ej0 " .
> +            "does not match EJ0_";
> +    }
> +
> +    # OK we are done. Output slot mask and offset
> +    push @output, sprintf("    {.slot_mask = 0x%x, .offset = 0x%x}",
> +                          0x1 << $slot, $ej0);
> +}
> +
> +# Pretty print output
> +if ($#output < 0) {
> +    die "No EJ0_ Method found!"
> +}
> +print <<EOF;
> +static struct aml_ej0_data {
> +    unsigned slot_mask;
> +    unsigned offset;
> +} aml_ej0_data[] = {
> +EOF
> +print join(",\n", @output);
> +print <<EOF;
> +
> +};
> +EOF
> +
> -- 
> 1.7.5.53.gc233e

--
			Gleb.

Re: [PATCH 2/4] acpi: add aml/asl parsing script

[Michael S. Tsirkin]

On Wed, Sep 21, 2011 at 05:27:32PM +0300, Gleb Natapov wrote:
> On Wed, Sep 21, 2011 at 03:44:29PM +0300, Michael S. Tsirkin wrote:
> > script ./src/find_ej0.pl finds all instances of
> > method named EJ0_ and the matching _ADR information,
> > and outputs the AML offset and slot mask of each.
> > 
> There is tools/ directory for such kind of scripts. Most (if not all) of
> scripts there are in python though.

OK, rewriting that in python should be easy.
I'll wait a bit for more comments on the design though.

> Perl should die painful death.
> 
> This approach delivers nice result, but since the script does not really
> decodes AML, but tries to match ASL source code with regular expressions,
> it introduces some assumptions to the code that make DSDT code less
> hackable. I'll hate to be the one who will have to change PCI device
> definitions in DSDT next time.

There are three requirements now:
1. don't use the name EJ0_ anywhere if you don't want it patches
2. _ADR must be an integer constant
3. put _ADR name immediately before EJ0_ method
I tried to make it easy to obey these rules
by adding comments in source code.

I don't believe a generic mechanism that does not place
any restrictions on language use is possible without adding an
AML interpreter in bios.

> Generally speaking finding an offset of some scope in AML is useful not
> only for PCI hotplug. For instance we want to make S3/S4 capability
> configurable by a command line switch, but this also requires DSDT
> patching and having automatic way to find _S3_/_S4_ offset is required
> for that too (we do not what to find it by hand each time DSDT is
> recompiled).

Right. So that would be an easy extension.
We could also add some directives for the tool
(e.g. in C comments) so that you can e.g. find more names
just by adding such a directive in dsl.

I'll be happy to work on that preferably
after we merge a simple version of the tool first.

Re: [PATCH 2/4] acpi: add aml/asl parsing script

[Michael S. Tsirkin, Kevin O'Connor]

I've rewritten the script in python. Seems to work but
I didn't have time to test - only compiled for now -
and needs to move to tools - but I hope this makes
review easier.

Thanks,

---
 src/find_ej0.py |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 140 insertions(+), 0 deletions(-)
 create mode 100755 src/find_ej0.py

diff --git a/src/find_ej0.py b/src/find_ej0.py
new file mode 100755
index 0000000..75e5491
--- /dev/null
+++ b/src/find_ej0.py
@@ -0,0 +1,140 @@
+#!/usr/bin/python
+
+# Process mixed ASL/AML listing (.lst file) produced by iasl -l
+# Locate all occurences of Name _ADR followed by Method EJ0_
+# Output slot info from _ADR and offset of method name in AML
+
+import re;
+import sys;
+import fileinput;
+
+aml = []
+asl = []
+output = []
+lineno = 0
+debug = ""
+
+class asl_line:
+    line = None
+    lineno = None
+    aml_offset = None
+
+def die(diag):
+    sys.stderr.write("Error: %s; %s\n" % (diag, debug))
+    sys.exit(1)
+    
+#Store an ASL command, matching AML offset, and input line (for debugging)
+def add_asl(line):
+    l = asl_line()
+    l.line = line
+    l.lineno = lineno
+    l.aml_offset = len(aml)
+    asl.append(l)
+
+#Store an AML byte sequence
+#Verify that offset output by iasl matches # of bytes so far
+def add_aml(offset, line):
+    o = int(offset, 16);
+    # Sanity check: offset must match size of code so far
+    if (o != len(aml)):
+        die("Offset 0x%x != 0x%x" % (o, len(aml)))
+    # Strip any trailing dots and ASCII dump after "
+    line = re.sub(r'\s*\.*\s*".*$',"", line)
+    # Strip traling whitespace
+    line = re.sub(r'\s+$',"", line)
+    # Strip leading whitespace
+    line = re.sub(r'^\s+',"", line)
+    # Split on whitespace
+    code = re.split(r'\s+', line)
+    for c in code:
+        # Require a legal hex number, two digits
+        if (not(re.search(r'^[0-9A-Fa-f][0-9A-Fa-f]$', c))):
+            die("Unexpected octet %s" % c);
+        aml.append(int(c, 16));
+
+# Process aml bytecode array, decoding AML
+# Given method offset, find its name offset
+def aml_method_name(lineno, offset):
+    #0x14 MethodOp PkgLength NameString MethodFlags TermList
+    if (aml[offset] != 0x14):
+        die( "Method after input line $lineno offset $offset: "
+            " expected 0x14 actual 0x%x" % aml[offset]);
+    offset += 1;
+    # PkgLength can be multibyte. Bits 8-7 give the # of extra bytes.
+    pkglenbytes = aml[offset] >> 6;
+    offset += 1 + pkglenbytes;
+    return offset;
+
+for line in fileinput.input():
+    # Strip trailing newline
+    line.rstrip();
+    # line number and debug string to output in case of errors
+    lineno = lineno + 1
+    debug = "input line %d: %s" % (lineno, line)
+    #ASL listing: space, then line#, then ...., then code
+    pasl = re.compile('^\s+([0-9]+)\.\.\.\.\s*')
+    m = pasl.search(line)
+    if (m):
+        add_asl(pasl.sub("", line));
+    # AML listing: offset in hex, then ...., then code
+    paml = re.compile('^([0-9A-Fa-f]+)\.\.\.\.\s*')
+    m = paml.search(line)
+    if (m):
+        add_aml(m.group(1), paml.sub("", line))
+
+# Now go over code, look for EJ0_ methods
+# For each such method, output slot mask from the
+# preceding _ADR line, as well as the method name offset.
+
+for i in range(len(asl)):
+    l = asl[i].line
+    debug = "input line %d: %s" % (asl[i].lineno, asl[i].line)
+    # match: Method (EJ0_,1)
+    if (not(re.search(r'^Method\s*\(\s*EJ0_\s*[,)]', l))):
+        # Make sure we do not miss any EJ0_:
+        # die if EJ0_ is found anywhere else in source code
+        if (re.search(r'EJ0_', l)):
+            die("Stray EJ0_ detected");
+        continue
+    # EJ0_ found. Previous line must be _ADR
+    p = asl[i - 1].line
+    # match: Name (_ADR, 0x<address>)
+    padr = re.compile('Name\s*\(\s*_ADR\s*,\s*0x([0-9A-Fa-f]+)\s*\)')
+    m = padr.search(p);
+    if (not m):
+        die("_ADR not found before EJ0_ ")
+
+    adr = int(m.group(1), 16)
+    slot = adr >> 16;
+    if (slot > 31):
+        die("_ADR device out of range: actual %d expected 0 to 31" % slot)
+
+    # We have offset of EJ0_ method in code
+    # Now find EJ0_ itself
+    ej0 = aml_method_name(asl[i].lineno, asl[i].aml_offset)
+    # Verify AML: name must be EJ0_:
+    if ((aml[ej0 + 0] != ord('E')) or
+        (aml[ej0 + 1] != ord('J')) or
+        (aml[ej0 + 2] != ord('0')) or
+        (aml[ej0 + 3] != ord('_'))):
+        die("AML offset 0x%x does not match EJ0_" % ej0)
+
+    # OK we are done. Output slot mask and offset
+    output.append("    {.slot_mask = 0x%x, .offset = 0x%x}" %
+                  (0x1 << slot, ej0))
+
+debug = "at end of file"
+
+# Pretty print output
+if (not len(output)):
+    die("No EJ0_ Method found!")
+
+print '''
+static struct aml_ej0_data {
+    unsigned slot_mask;
+    unsigned offset;
+} aml_ej0_data[] = {'''
+
+print ",\n".join(output)
+print '};\n'
+
-- 
1.7.5.53.gc233e

Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT

[Kevin O'Connor]

On Wed, Sep 21, 2011 at 03:44:13PM +0300, Michael S. Tsirkin wrote:
> The reason is that our acpi tables declare both _RMV with value 0,
> and _EJ0 method for these slots. What happens in this case
> is undocumented by ACPI spec, so linux ignores _RMV,
> and windows seems to ignore _EJ0.

Could the DSDT just not define _EJ0 for device 1 & 2 instead of
dynamically patching them?  (Would there ever be a case where we
wouldn't know at compile time which devices need _EJ0?)

> The correct way to suppress hotplug is not to have _EJ0,
> so this is what this patch does: it probes PIIX and
> modifies DSDT to match.

The code to generate basic SSDT code isn't that difficult (see
build_ssdt and src/ssdt-proc.dsl).  Is there a compelling reason to
patch the DSDT versus just generating the necessary blocks in an SSDT?

-Kevin

Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT

[Michael S. Tsirkin]

On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote:
> On Wed, Sep 21, 2011 at 03:44:13PM +0300, Michael S. Tsirkin wrote:
> > The reason is that our acpi tables declare both _RMV with value 0,
> > and _EJ0 method for these slots. What happens in this case
> > is undocumented by ACPI spec, so linux ignores _RMV,
> > and windows seems to ignore _EJ0.
> 
> Could the DSDT just not define _EJ0 for device 1 & 2 instead of
> dynamically patching them?  (Would there ever be a case where we
> wouldn't know at compile time which devices need _EJ0?)

Yes. in qemu we can make any slot non hotpluggable on command
line by requesting a non hotpluggable device be put there.

> > The correct way to suppress hotplug is not to have _EJ0,
> > so this is what this patch does: it probes PIIX and
> > modifies DSDT to match.
> 
> The code to generate basic SSDT code isn't that difficult (see
> build_ssdt and src/ssdt-proc.dsl).  Is there a compelling reason to
> patch the DSDT versus just generating the necessary blocks in an SSDT?
> 
> -Kevin

I don't really care whether the code is in DSDT or SSDT,
IMO there isn't much difference between build_ssdt and patching:
main reason is build_ssdt uses offsets hardcoded to a specific binary
(ssdt_proc and SD_OFFSET_* ) while I used
a script to extract offsets.

I think we should avoid relying on copy-pasted binary 
because I see the related ASL code changing in the near future
(with multifunction and bridge support among others).

I can generalize the approach though, so that
it can work for finding arbitrary names
without writing more scripts, hopefully with the
potential to address the hard-coded offsets in acpi.c
as well. Does that sound interesting?

-- 
MST

Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT

[Kevin O'Connor]

On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote:
> > On Wed, Sep 21, 2011 at 03:44:13PM +0300, Michael S. Tsirkin wrote:
> > > The correct way to suppress hotplug is not to have _EJ0,
> > > so this is what this patch does: it probes PIIX and
> > > modifies DSDT to match.
> > 
> > The code to generate basic SSDT code isn't that difficult (see
> > build_ssdt and src/ssdt-proc.dsl).  Is there a compelling reason to
> > patch the DSDT versus just generating the necessary blocks in an SSDT?
> 
> I don't really care whether the code is in DSDT or SSDT,
> IMO there isn't much difference between build_ssdt and patching:
> main reason is build_ssdt uses offsets hardcoded to a specific binary
> (ssdt_proc and SD_OFFSET_* ) while I used
> a script to extract offsets.
> 
> I think we should avoid relying on copy-pasted binary 
> because I see the related ASL code changing in the near future
> (with multifunction and bridge support among others).
> 
> I can generalize the approach though, so that
> it can work for finding arbitrary names
> without writing more scripts, hopefully with the
> potential to address the hard-coded offsets in acpi.c
> as well. Does that sound interesting?

Replacing the hardcoding of offsets in src/ssdt-proc.dsl would be
nice.

I'll take a look at your new patches tonight.

-Kevin

Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT

[Kevin O'Connor]

On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote:
> > The code to generate basic SSDT code isn't that difficult (see
> > build_ssdt and src/ssdt-proc.dsl).  Is there a compelling reason to
> > patch the DSDT versus just generating the necessary blocks in an SSDT?
> 
> I don't really care whether the code is in DSDT or SSDT,
> IMO there isn't much difference between build_ssdt and patching:
> main reason is build_ssdt uses offsets hardcoded to a specific binary
> (ssdt_proc and SD_OFFSET_* ) while I used
> a script to extract offsets.

Yes - your script to extract the offsets is nice.

> I think we should avoid relying on copy-pasted binary 
> because I see the related ASL code changing in the near future
> (with multifunction and bridge support among others).

Can you expand on this?  Would multi-function and bridge support make
patching easier than dynamic SSDT generation?

I'm a little leary of patching the DSDT - doubly so when the DSDT is
creating dummy devices that are then dynamically patched out.  (For
example, even with your patch series there are still two devices
defined with _ADR 1.)  It seems more straight-forward to just create
the devices that are needed.

-Kevin

Re: [SeaBIOS] [PATCH 0/4] acpi: fix up EJ0 in DSDT

[Rudolf Marek]

Hi all,

> defined with _ADR 1.)  It seems more straight-forward to just create
> the devices that are needed.

Yes something like this:

http://review.coreboot.org/gitweb?p=coreboot.git;a=blob;f=src/arch/x86/boot/acpigen.c;hb=HEAD



Thanks
Rudolf

Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT

[Michael S. Tsirkin]

On Mon, Sep 26, 2011 at 12:40:18AM -0400, Kevin O'Connor wrote:
> On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote:
> > On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote:
> > > The code to generate basic SSDT code isn't that difficult (see
> > > build_ssdt and src/ssdt-proc.dsl).  Is there a compelling reason to
> > > patch the DSDT versus just generating the necessary blocks in an SSDT?
> > 
> > I don't really care whether the code is in DSDT or SSDT,
> > IMO there isn't much difference between build_ssdt and patching:
> > main reason is build_ssdt uses offsets hardcoded to a specific binary
> > (ssdt_proc and SD_OFFSET_* ) while I used
> > a script to extract offsets.
> 
> Yes - your script to extract the offsets is nice.

If you still have doubts,
it might make sense to merge just patch 1 -
acpi: generate and parse mixed asl/aml listing
- as the first step.
With the infrastructure in place it will be
easier to discuss the best way to use it.

> > I think we should avoid relying on copy-pasted binary 
> > because I see the related ASL code changing in the near future
> > (with multifunction and bridge support among others).
> 
> Can you expand on this?  Would multi-function and bridge support make
> patching easier than dynamic SSDT generation?

Just generic concerns:
1. We are going to have to modify DSL, so binary bytecode
would be hard to maintain. Specifically IMO the more
work can be done compile-time, the better, both iasl
and my script do compile-time checks to keep runtime simple.
2. There are going to be a lot of devices so one new table
with all of them would be ok, a table per device would be
expensive.

> I'm a little leary of patching the DSDT - doubly so when the DSDT is
> creating dummy devices that are then dynamically patched out.

A small correction, my specific code only patches out methods, not whole
devices.


>  (For
> example, even with your patch series there are still two devices
> defined with _ADR 1.)

So this is a bug, I think.
I am not sure whether we just want ISA and VGA always
non-removable - is there some reason that
we have hotplug_slot macros for these slots?
If no we should just remove hotplug
macros for these two slots. Gleb, Marcelo, any preferences?

If we want to keep the option of making these slots hotpluggable
from qemu, it is still easy to fix the duplication. For example,
I could put ACPI_EXTRACT tags in VGA/ISA devices without
renaming them. But the easiest way is probably by using Scope.
See patch below.


>  It seems more straight-forward to just create
> the devices that are needed.
> 
> -Kevin

FWIW the acpi spec does mention that it's ok to describe
unoccupied slots.

---->

Multiple device with the same _ADR relies on unspecified
behavior of ACPI. Fix DSDT so we always have a single device per slot.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

-----

diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index 055202b..305d2e5 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -260,8 +260,8 @@ DefinitionBlock (
     }
 
     Scope(\_SB.PCI0) {
-        Device (VGA) {
-                 Name (_ADR, 0x00020000)
+	/* VGA device */
+        Scope (S2) {
                  OperationRegion(PCIC, PCI_Config, Zero, 0x4)
                  Field(PCIC, DWordAcc, NoLock, Preserve) {
                          VEND, 32
@@ -282,13 +282,10 @@ DefinitionBlock (
                                  Return (0x00)
                          }
                  }
-                 Method(_RMV) { Return (0x00) }
         }
 
 	/* PIIX3 ISA bridge */
-        Device (ISA) {
-            Name (_ADR, 0x00010000)
-            Method(_RMV) { Return (0x00) }
+        Scope (S1) {
 
 
             /* PIIX PCI to ISA irq remapping */
@@ -486,7 +483,7 @@ DefinitionBlock (
 
     /* PCI IRQs */
     Scope(\_SB) {
-         Field (\_SB.PCI0.ISA.P40C, ByteAcc, NoLock, Preserve)
+         Field (\_SB.PCI0.S1.P40C, ByteAcc, NoLock, Preserve)
          {
              PRQ0,   8,
              PRQ1,   8,

-- 
MST

Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT

[Marcelo Tosatti]

On Mon, Sep 26, 2011 at 10:04:13AM +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 26, 2011 at 12:40:18AM -0400, Kevin O'Connor wrote:
> > On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote:
> > > > The code to generate basic SSDT code isn't that difficult (see
> > > > build_ssdt and src/ssdt-proc.dsl).  Is there a compelling reason to
> > > > patch the DSDT versus just generating the necessary blocks in an SSDT?
> > > 
> > > I don't really care whether the code is in DSDT or SSDT,
> > > IMO there isn't much difference between build_ssdt and patching:
> > > main reason is build_ssdt uses offsets hardcoded to a specific binary
> > > (ssdt_proc and SD_OFFSET_* ) while I used
> > > a script to extract offsets.
> > 
> > Yes - your script to extract the offsets is nice.
> 
> If you still have doubts,
> it might make sense to merge just patch 1 -
> acpi: generate and parse mixed asl/aml listing
> - as the first step.
> With the infrastructure in place it will be
> easier to discuss the best way to use it.
> 
> > > I think we should avoid relying on copy-pasted binary 
> > > because I see the related ASL code changing in the near future
> > > (with multifunction and bridge support among others).
> > 
> > Can you expand on this?  Would multi-function and bridge support make
> > patching easier than dynamic SSDT generation?
> 
> Just generic concerns:
> 1. We are going to have to modify DSL, so binary bytecode
> would be hard to maintain. Specifically IMO the more
> work can be done compile-time, the better, both iasl
> and my script do compile-time checks to keep runtime simple.
> 2. There are going to be a lot of devices so one new table
> with all of them would be ok, a table per device would be
> expensive.
> 
> > I'm a little leary of patching the DSDT - doubly so when the DSDT is
> > creating dummy devices that are then dynamically patched out.
> 
> A small correction, my specific code only patches out methods, not whole
> devices.

And note only the name of the method is changed to something which the
guests do not identify, its not as if the entire method is added or
removed (although IMO it would be interesting to patch out entirely with
NOPs).

> >  (For
> > example, even with your patch series there are still two devices
> > defined with _ADR 1.)
> 
> So this is a bug, I think.
> I am not sure whether we just want ISA and VGA always
> non-removable - is there some reason that
> we have hotplug_slot macros for these slots?

Just so its a nice linear range.

> If no we should just remove hotplug
> macros for these two slots. Gleb, Marcelo, any preferences?

Currently they are hardcoded as not hotpluggable (as can be noted by
the RMV macros), but its nice to retrieve that information from QEMU
instead.

> If we want to keep the option of making these slots hotpluggable
> from qemu, it is still easy to fix the duplication. For example,
> I could put ACPI_EXTRACT tags in VGA/ISA devices without
> renaming them. But the easiest way is probably by using Scope.
> See patch below.
> 
> 
> >  It seems more straight-forward to just create
> > the devices that are needed.
> > 
> > -Kevin
> 
> FWIW the acpi spec does mention that it's ok to describe
> unoccupied slots.
> 
> ---->
> 
> Multiple device with the same _ADR relies on unspecified
> behavior of ACPI. Fix DSDT so we always have a single device per slot.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> -----
> 
> diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
> index 055202b..305d2e5 100644
> --- a/src/acpi-dsdt.dsl
> +++ b/src/acpi-dsdt.dsl
> @@ -260,8 +260,8 @@ DefinitionBlock (
>      }
>  
>      Scope(\_SB.PCI0) {
> -        Device (VGA) {
> -                 Name (_ADR, 0x00020000)
> +	/* VGA device */
> +        Scope (S2) {
>                   OperationRegion(PCIC, PCI_Config, Zero, 0x4)
>                   Field(PCIC, DWordAcc, NoLock, Preserve) {
>                           VEND, 32
> @@ -282,13 +282,10 @@ DefinitionBlock (
>                                   Return (0x00)
>                           }
>                   }
> -                 Method(_RMV) { Return (0x00) }
>          }
>  
>  	/* PIIX3 ISA bridge */
> -        Device (ISA) {
> -            Name (_ADR, 0x00010000)
> -            Method(_RMV) { Return (0x00) }
> +        Scope (S1) {
>  
>  
>              /* PIIX PCI to ISA irq remapping */
> @@ -486,7 +483,7 @@ DefinitionBlock (
>  
>      /* PCI IRQs */
>      Scope(\_SB) {
> -         Field (\_SB.PCI0.ISA.P40C, ByteAcc, NoLock, Preserve)
> +         Field (\_SB.PCI0.S1.P40C, ByteAcc, NoLock, Preserve)
>           {
>               PRQ0,   8,
>               PRQ1,   8,
> 
> -- 
> MST

Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT

[Michael S. Tsirkin]

On Mon, Sep 26, 2011 at 08:36:41AM -0300, Marcelo Tosatti wrote:
> > If no we should just remove hotplug
> > macros for these two slots. Gleb, Marcelo, any preferences?
> 
> Currently they are hardcoded as not hotpluggable (as can be noted by
> the RMV macros), but its nice to retrieve that information from QEMU
> instead.

OK, so you ACK the patch below?

> > If we want to keep the option of making these slots hotpluggable
> > from qemu, it is still easy to fix the duplication. For example,
> > I could put ACPI_EXTRACT tags in VGA/ISA devices without
> > renaming them. But the easiest way is probably by using Scope.
> > See patch below.
> > 
> > 
> > >  It seems more straight-forward to just create
> > > the devices that are needed.
> > > 
> > > -Kevin
> > 
> > FWIW the acpi spec does mention that it's ok to describe
> > unoccupied slots.
> > 
> > ---->
> > 
> > Multiple device with the same _ADR relies on unspecified
> > behavior of ACPI. Fix DSDT so we always have a single device per slot.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > -----
> > 
> > diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
> > index 055202b..305d2e5 100644
> > --- a/src/acpi-dsdt.dsl
> > +++ b/src/acpi-dsdt.dsl
> > @@ -260,8 +260,8 @@ DefinitionBlock (
> >      }
> >  
> >      Scope(\_SB.PCI0) {
> > -        Device (VGA) {
> > -                 Name (_ADR, 0x00020000)
> > +	/* VGA device */
> > +        Scope (S2) {
> >                   OperationRegion(PCIC, PCI_Config, Zero, 0x4)
> >                   Field(PCIC, DWordAcc, NoLock, Preserve) {
> >                           VEND, 32
> > @@ -282,13 +282,10 @@ DefinitionBlock (
> >                                   Return (0x00)
> >                           }
> >                   }
> > -                 Method(_RMV) { Return (0x00) }
> >          }
> >  
> >  	/* PIIX3 ISA bridge */
> > -        Device (ISA) {
> > -            Name (_ADR, 0x00010000)
> > -            Method(_RMV) { Return (0x00) }
> > +        Scope (S1) {
> >  
> >  
> >              /* PIIX PCI to ISA irq remapping */
> > @@ -486,7 +483,7 @@ DefinitionBlock (
> >  
> >      /* PCI IRQs */
> >      Scope(\_SB) {
> > -         Field (\_SB.PCI0.ISA.P40C, ByteAcc, NoLock, Preserve)
> > +         Field (\_SB.PCI0.S1.P40C, ByteAcc, NoLock, Preserve)
> >           {
> >               PRQ0,   8,
> >               PRQ1,   8,
> > 
> > -- 
> > MST

Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT

[Kevin O'Connor]

On Mon, Sep 26, 2011 at 10:04:13AM +0300, Michael S. Tsirkin wrote:
> On Mon, Sep 26, 2011 at 12:40:18AM -0400, Kevin O'Connor wrote:
> > On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote:
> > > On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote:
> > > > The code to generate basic SSDT code isn't that difficult (see
> > > > build_ssdt and src/ssdt-proc.dsl).  Is there a compelling reason to
> > > > patch the DSDT versus just generating the necessary blocks in an SSDT?
> > > 
> > > I don't really care whether the code is in DSDT or SSDT,
> > > IMO there isn't much difference between build_ssdt and patching:
> > > main reason is build_ssdt uses offsets hardcoded to a specific binary
> > > (ssdt_proc and SD_OFFSET_* ) while I used
> > > a script to extract offsets.
> > 
> > Yes - your script to extract the offsets is nice.
> 
> If you still have doubts,
> it might make sense to merge just patch 1 -
> acpi: generate and parse mixed asl/aml listing
> - as the first step.
> With the infrastructure in place it will be
> easier to discuss the best way to use it.

I'm okay with your first patch.  However, I wish to tag a release
before committing ACPI changes.  There was a concern raised with
two-pass PCI initialization that I need to follow up on before
tagging.

-Kevin

Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT

[Michael S. Tsirkin]

On Mon, Sep 26, 2011 at 08:04:24PM -0400, Kevin O'Connor wrote:
> On Mon, Sep 26, 2011 at 10:04:13AM +0300, Michael S. Tsirkin wrote:
> > On Mon, Sep 26, 2011 at 12:40:18AM -0400, Kevin O'Connor wrote:
> > > On Thu, Sep 22, 2011 at 09:09:49AM +0300, Michael S. Tsirkin wrote:
> > > > On Thu, Sep 22, 2011 at 12:35:13AM -0400, Kevin O'Connor wrote:
> > > > > The code to generate basic SSDT code isn't that difficult (see
> > > > > build_ssdt and src/ssdt-proc.dsl).  Is there a compelling reason to
> > > > > patch the DSDT versus just generating the necessary blocks in an SSDT?
> > > > 
> > > > I don't really care whether the code is in DSDT or SSDT,
> > > > IMO there isn't much difference between build_ssdt and patching:
> > > > main reason is build_ssdt uses offsets hardcoded to a specific binary
> > > > (ssdt_proc and SD_OFFSET_* ) while I used
> > > > a script to extract offsets.
> > > 
> > > Yes - your script to extract the offsets is nice.
> > 
> > If you still have doubts,
> > it might make sense to merge just patch 1 -
> > acpi: generate and parse mixed asl/aml listing
> > - as the first step.
> > With the infrastructure in place it will be
> > easier to discuss the best way to use it.
> 
> I'm okay with your first patch.

BTW, any more comments with the rest of the patchset?
If you just need to think about it, I understand.

> However, I wish to tag a release
> before committing ACPI changes.

Sure. So you'll take this patchset from here or want me to
ping you later?

>  There was a concern raised with
> two-pass PCI initialization that I need to follow up on before
> tagging.

The isa bridge? I thought that got fixed ...

> 
> -Kevin

-- 
MST

Re: [PATCH 0/4] acpi: fix up EJ0 in DSDT

[Paolo Bonzini]

On 09/27/2011 03:04 PM, Michael S. Tsirkin wrote:
> >    There was a concern raised with
> >  two-pass PCI initialization that I need to follow up on before
> >  tagging.
>
> The isa bridge? I thought that got fixed ...

Daniel Berrange reported a regression with virtio-9p.

Paolo