acpi: fix up EJ0 in DSDT

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 <[email protected]>

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 <[email protected]>

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

src/find_ej0.pl new file +136 -0

new file mode 100755
index 0000000..37e8a8c
@@ -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
+

Signature · 1 line

1.7.5.53.gc233e

Quoted 4 lines · 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.

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).

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.

Quoted 4 lines · Michael S. Tsirkin

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?)

Quoted 3 lines

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

Quoted 8 lines · Kevin O'Connor

Quoted 3 lines · Michael S. Tsirkin

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?

Quoted 3 lines · Kevin O'Connor

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).

Quoted 5 lines · Michael S. Tsirkin

Quoted 3 lines · Kevin O'Connor

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?

Quoted 5 lines

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.

Quoted 3 lines

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

Quoted 6 lines · Kevin O'Connor

Quoted 5 lines · Michael S. Tsirkin

Quoted 3 lines · Kevin O'Connor

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?

Quoted 7 lines

Quoted 5 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

(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.

Quoted 4 lines

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 <[email protected]>

-----

src/acpi-dsdt.dsl +4 -7

index 055202b..305d2e5 100644
@@ -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,

Signature · 2 lines

-- 
MST

Quoted 2 lines

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.

Quoted 66 lines

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.

Quoted 4 lines

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 <[email protected]>

-----

src/acpi-dsdt.dsl +4 -7

index 055202b..305d2e5 100644
@@ -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,

Signature · 2 lines

-- 
MST

Quoted 12 lines · Kevin O'Connor

Quoted 10 lines · Michael S. Tsirkin

Quoted 3 lines · Kevin O'Connor

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.

Quoted 1 line

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.

Quoted 15 lines · Michael S. Tsirkin

Quoted 13 lines · Kevin O'Connor

Quoted 10 lines · Michael S. Tsirkin

Quoted 3 lines · Kevin O'Connor

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.

Quoted 6 lines

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

Quoted 3 lines

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 ...

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