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
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; +}
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 +
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));
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 */
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
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?
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
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
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/...
Thanks Rudolf
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,
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:
- 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
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
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
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
-
Kevin O'Connor -
Marcelo Tosatti -
Michael S. Tsirkin -
Paolo Bonzini -
Rudolf Marek