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

["Michael S. Tsirkin" <mst@redhat.com>]

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