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

[Marcelo Tosatti <mtosatti@redhat.com>]

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