| Message ID | 1386951736-929-10-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > .. and report range used by it to OSPM via _CRS. > PRST is needed in SSDT since its base will depend on > chipset and will be dynamically set by QEMU. > Also move PRSC() method along with PRST since cross > table reference to PRST doesn't work. Could you clarify this last sentence? I don't mind where it is but I'd like to know where does the limitation come from. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +---------------------- > hw/i386/acpi-dsdt.dsl | 2 +- > hw/i386/q35-acpi-dsdt.dsl | 2 +- > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++ > 4 files changed, 68 insertions(+), 40 deletions(-) > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > index 995b415..f26f81b 100644 > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > @@ -20,6 +20,7 @@ > Scope(\_SB) { > /* Objects filled in by run-time generated SSDT */ > External(NTFY, MethodObj) > + External(\_SB.CPHD.PRSC, MethodObj) > External(CPON, PkgObj) > > /* Methods called by run-time generated SSDT Processor objects */ > @@ -51,42 +52,4 @@ Scope(\_SB) { > // _EJ0 method - eject callback > Sleep(200) > } > - > - OperationRegion(PRST, SystemIO, 0xaf00, 32) > - Field(PRST, ByteAcc, NoLock, Preserve) { > - PRS, 256 > - } > - Method(PRSC, 0) { > - // Local5 = active cpu bitmap > - Store(PRS, Local5) > - // Local2 = last read byte from bitmap > - Store(Zero, Local2) > - // Local0 = Processor ID / APIC ID iterator > - Store(Zero, Local0) > - While (LLess(Local0, SizeOf(CPON))) { > - // Local1 = CPON flag for this cpu > - Store(DerefOf(Index(CPON, Local0)), Local1) > - If (And(Local0, 0x07)) { > - // Shift down previously read bitmap byte > - ShiftRight(Local2, 1, Local2) > - } Else { > - // Read next byte from cpu bitmap > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > - } > - // Local3 = active state for this cpu > - Store(And(Local2, 1), Local3) > - > - If (LNotEqual(Local1, Local3)) { > - // State change - update CPON with new state > - Store(Local3, Index(CPON, Local0)) > - // Do CPU notify > - If (LEqual(Local3, 1)) { > - NTFY(Local0, 1) > - } Else { > - NTFY(Local0, 3) > - } > - } > - Increment(Local0) > - } > - } > } > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > index 90efce0..fa9f2d4 100644 > --- a/hw/i386/acpi-dsdt.dsl > +++ b/hw/i386/acpi-dsdt.dsl > @@ -311,7 +311,7 @@ DefinitionBlock ( > } > Method(_E02) { > // CPU hotplug event > - \_SB.PRSC() > + \_SB.CPHD.PRSC() > } > Method(_L03) { > } > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > index 22baa58..9ccc543 100644 > --- a/hw/i386/q35-acpi-dsdt.dsl > +++ b/hw/i386/q35-acpi-dsdt.dsl > @@ -420,7 +420,7 @@ DefinitionBlock ( > } > Method(_E02) { > // CPU hotplug event > - \_SB.PRSC() > + \_SB.CPHD.PRSC() > } > Method(_L03) { > } > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl > index a4484b8..ec8893c 100644 > --- a/hw/i386/ssdt-misc.dsl > +++ b/hw/i386/ssdt-misc.dsl > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) > } > } > } > + Scope(\_SB) { > + External(NTFY, MethodObj) > + External(CPON, PkgObj) > + > + Device(CPHD) { > + Name(_HID, EISAID("PNP0C08")) > + Name(CPPL, 32) // cpu-gpe length > + Name(CPHP, 0xaf00) > + > + OperationRegion(PRST, SystemIO, CPHP, CPPL) > + Field(PRST, ByteAcc, NoLock, Preserve) { > + PRS, 256 > + } > + > + Method(PRSC, 0) { > + // Local5 = active cpu bitmap > + Store(PRS, Local5) > + // Local2 = last read byte from bitmap > + Store(Zero, Local2) > + // Local0 = Processor ID / APIC ID iterator > + Store(Zero, Local0) > + While (LLess(Local0, SizeOf(CPON))) { > + // Local1 = CPON flag for this cpu > + Store(DerefOf(Index(CPON, Local0)), Local1) > + If (And(Local0, 0x07)) { > + // Shift down previously read bitmap byte > + ShiftRight(Local2, 1, Local2) > + } Else { > + // Read next byte from cpu bitmap > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > + } > + // Local3 = active state for this cpu > + Store(And(Local2, 1), Local3) > + > + If (LNotEqual(Local1, Local3)) { > + // State change - update CPON with new state > + Store(Local3, Index(CPON, Local0)) > + // Do CPU notify > + If (LEqual(Local3, 1)) { > + NTFY(Local0, 1) > + } Else { > + NTFY(Local0, 3) > + } > + } > + Increment(Local0) > + } > + } > + > + /* Leave bit 0 cleared to avoid Windows BSOD */ > + Name(_STA, 0xA) > + > + Method(_CRS, 0) { > + Store(ResourceTemplate() { > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO) > + }, Local0) > + > + CreateWordField(Local0, IO._MIN, IOMN) > + CreateWordField(Local0, IO._MAX, IOMX) > + > + Store(CPHP, IOMN) > + Subtract(Add(CPHP, CPPL), 1, IOMX) > + Return(Local0) > + } > + } // Device(CPHD) > + } // Scope(\_SB) > } > -- > 1.8.3.1
On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > .. and report range used by it to OSPM via _CRS. > PRST is needed in SSDT since its base will depend on > chipset and will be dynamically set by QEMU. > Also move PRSC() method along with PRST since cross > table reference to PRST doesn't work. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +---------------------- > hw/i386/acpi-dsdt.dsl | 2 +- > hw/i386/q35-acpi-dsdt.dsl | 2 +- > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++ > 4 files changed, 68 insertions(+), 40 deletions(-) > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > index 995b415..f26f81b 100644 > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > @@ -20,6 +20,7 @@ > Scope(\_SB) { > /* Objects filled in by run-time generated SSDT */ > External(NTFY, MethodObj) > + External(\_SB.CPHD.PRSC, MethodObj) > External(CPON, PkgObj) > > /* Methods called by run-time generated SSDT Processor objects */ > @@ -51,42 +52,4 @@ Scope(\_SB) { > // _EJ0 method - eject callback > Sleep(200) > } > - > - OperationRegion(PRST, SystemIO, 0xaf00, 32) > - Field(PRST, ByteAcc, NoLock, Preserve) { > - PRS, 256 > - } > - Method(PRSC, 0) { > - // Local5 = active cpu bitmap > - Store(PRS, Local5) > - // Local2 = last read byte from bitmap > - Store(Zero, Local2) > - // Local0 = Processor ID / APIC ID iterator > - Store(Zero, Local0) > - While (LLess(Local0, SizeOf(CPON))) { > - // Local1 = CPON flag for this cpu > - Store(DerefOf(Index(CPON, Local0)), Local1) > - If (And(Local0, 0x07)) { > - // Shift down previously read bitmap byte > - ShiftRight(Local2, 1, Local2) > - } Else { > - // Read next byte from cpu bitmap > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > - } > - // Local3 = active state for this cpu > - Store(And(Local2, 1), Local3) > - > - If (LNotEqual(Local1, Local3)) { > - // State change - update CPON with new state > - Store(Local3, Index(CPON, Local0)) > - // Do CPU notify > - If (LEqual(Local3, 1)) { > - NTFY(Local0, 1) > - } Else { > - NTFY(Local0, 3) > - } > - } > - Increment(Local0) > - } > - } > } > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > index 90efce0..fa9f2d4 100644 > --- a/hw/i386/acpi-dsdt.dsl > +++ b/hw/i386/acpi-dsdt.dsl > @@ -311,7 +311,7 @@ DefinitionBlock ( > } > Method(_E02) { > // CPU hotplug event > - \_SB.PRSC() > + \_SB.CPHD.PRSC() > } > Method(_L03) { > } > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > index 22baa58..9ccc543 100644 > --- a/hw/i386/q35-acpi-dsdt.dsl > +++ b/hw/i386/q35-acpi-dsdt.dsl > @@ -420,7 +420,7 @@ DefinitionBlock ( > } > Method(_E02) { > // CPU hotplug event > - \_SB.PRSC() > + \_SB.CPHD.PRSC() > } > Method(_L03) { > } > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl > index a4484b8..ec8893c 100644 > --- a/hw/i386/ssdt-misc.dsl > +++ b/hw/i386/ssdt-misc.dsl > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) > } > } > } > + Scope(\_SB) { > + External(NTFY, MethodObj) > + External(CPON, PkgObj) > + > + Device(CPHD) { > + Name(_HID, EISAID("PNP0C08")) > + Name(CPPL, 32) // cpu-gpe length > + Name(CPHP, 0xaf00) > + > + OperationRegion(PRST, SystemIO, CPHP, CPPL) > + Field(PRST, ByteAcc, NoLock, Preserve) { > + PRS, 256 > + } > + > + Method(PRSC, 0) { > + // Local5 = active cpu bitmap > + Store(PRS, Local5) > + // Local2 = last read byte from bitmap > + Store(Zero, Local2) > + // Local0 = Processor ID / APIC ID iterator > + Store(Zero, Local0) > + While (LLess(Local0, SizeOf(CPON))) { > + // Local1 = CPON flag for this cpu > + Store(DerefOf(Index(CPON, Local0)), Local1) > + If (And(Local0, 0x07)) { > + // Shift down previously read bitmap byte > + ShiftRight(Local2, 1, Local2) > + } Else { > + // Read next byte from cpu bitmap > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > + } > + // Local3 = active state for this cpu > + Store(And(Local2, 1), Local3) > + > + If (LNotEqual(Local1, Local3)) { > + // State change - update CPON with new state > + Store(Local3, Index(CPON, Local0)) > + // Do CPU notify > + If (LEqual(Local3, 1)) { > + NTFY(Local0, 1) > + } Else { > + NTFY(Local0, 3) > + } > + } > + Increment(Local0) > + } > + } > + > + /* Leave bit 0 cleared to avoid Windows BSOD */ > + Name(_STA, 0xA) This shared the problem you yourself pointed out with patch 'pc: ACPI BIOS: implement memory hotplug': if we make device non present ospm can ignore our _CRS. Can't we get a better handle on why windows crashes? > + > + Method(_CRS, 0) { > + Store(ResourceTemplate() { > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO) > + }, Local0) > + > + CreateWordField(Local0, IO._MIN, IOMN) > + CreateWordField(Local0, IO._MAX, IOMX) > + > + Store(CPHP, IOMN) > + Subtract(Add(CPHP, CPPL), 1, IOMX) > + Return(Local0) > + } > + } // Device(CPHD) > + } // Scope(\_SB) > } > -- > 1.8.3.1
On Mon, 16 Dec 2013 21:30:14 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > > .. and report range used by it to OSPM via _CRS. > > PRST is needed in SSDT since its base will depend on > > chipset and will be dynamically set by QEMU. > > Also move PRSC() method along with PRST since cross > > table reference to PRST doesn't work. > > Could you clarify this last sentence? > I don't mind where it is but I'd like to know > where does the limitation come from. It's empiric deduction so far I haven't found such limitation in spec yet. iasl builds tables just fine but neither linux nor windows were able to find Operation region from SSDT when loading DSDT, failing whole table loading process. Decompiling DSDT/SSDT tables in guest shows that region is in expected scope but OSPM refuses to see it when referenced outside SSDT. Maybe there is some AML magic to make it work, I'm not aware of. The same thing I had to do for memory hotplug as well. So I've tried to play nicely 2 times and I have ended up with this solution both times. > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +---------------------- > > hw/i386/acpi-dsdt.dsl | 2 +- > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 68 insertions(+), 40 deletions(-) > > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > index 995b415..f26f81b 100644 > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > @@ -20,6 +20,7 @@ > > Scope(\_SB) { > > /* Objects filled in by run-time generated SSDT */ > > External(NTFY, MethodObj) > > + External(\_SB.CPHD.PRSC, MethodObj) > > External(CPON, PkgObj) > > > > /* Methods called by run-time generated SSDT Processor objects */ > > @@ -51,42 +52,4 @@ Scope(\_SB) { > > // _EJ0 method - eject callback > > Sleep(200) > > } > > - > > - OperationRegion(PRST, SystemIO, 0xaf00, 32) > > - Field(PRST, ByteAcc, NoLock, Preserve) { > > - PRS, 256 > > - } > > - Method(PRSC, 0) { > > - // Local5 = active cpu bitmap > > - Store(PRS, Local5) > > - // Local2 = last read byte from bitmap > > - Store(Zero, Local2) > > - // Local0 = Processor ID / APIC ID iterator > > - Store(Zero, Local0) > > - While (LLess(Local0, SizeOf(CPON))) { > > - // Local1 = CPON flag for this cpu > > - Store(DerefOf(Index(CPON, Local0)), Local1) > > - If (And(Local0, 0x07)) { > > - // Shift down previously read bitmap byte > > - ShiftRight(Local2, 1, Local2) > > - } Else { > > - // Read next byte from cpu bitmap > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > - } > > - // Local3 = active state for this cpu > > - Store(And(Local2, 1), Local3) > > - > > - If (LNotEqual(Local1, Local3)) { > > - // State change - update CPON with new state > > - Store(Local3, Index(CPON, Local0)) > > - // Do CPU notify > > - If (LEqual(Local3, 1)) { > > - NTFY(Local0, 1) > > - } Else { > > - NTFY(Local0, 3) > > - } > > - } > > - Increment(Local0) > > - } > > - } > > } > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > index 90efce0..fa9f2d4 100644 > > --- a/hw/i386/acpi-dsdt.dsl > > +++ b/hw/i386/acpi-dsdt.dsl > > @@ -311,7 +311,7 @@ DefinitionBlock ( > > } > > Method(_E02) { > > // CPU hotplug event > > - \_SB.PRSC() > > + \_SB.CPHD.PRSC() > > } > > Method(_L03) { > > } > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > index 22baa58..9ccc543 100644 > > --- a/hw/i386/q35-acpi-dsdt.dsl > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > @@ -420,7 +420,7 @@ DefinitionBlock ( > > } > > Method(_E02) { > > // CPU hotplug event > > - \_SB.PRSC() > > + \_SB.CPHD.PRSC() > > } > > Method(_L03) { > > } > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl > > index a4484b8..ec8893c 100644 > > --- a/hw/i386/ssdt-misc.dsl > > +++ b/hw/i386/ssdt-misc.dsl > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) > > } > > } > > } > > + Scope(\_SB) { > > + External(NTFY, MethodObj) > > + External(CPON, PkgObj) > > + > > + Device(CPHD) { > > + Name(_HID, EISAID("PNP0C08")) > > + Name(CPPL, 32) // cpu-gpe length > > + Name(CPHP, 0xaf00) > > + > > + OperationRegion(PRST, SystemIO, CPHP, CPPL) > > + Field(PRST, ByteAcc, NoLock, Preserve) { > > + PRS, 256 > > + } > > + > > + Method(PRSC, 0) { > > + // Local5 = active cpu bitmap > > + Store(PRS, Local5) > > + // Local2 = last read byte from bitmap > > + Store(Zero, Local2) > > + // Local0 = Processor ID / APIC ID iterator > > + Store(Zero, Local0) > > + While (LLess(Local0, SizeOf(CPON))) { > > + // Local1 = CPON flag for this cpu > > + Store(DerefOf(Index(CPON, Local0)), Local1) > > + If (And(Local0, 0x07)) { > > + // Shift down previously read bitmap byte > > + ShiftRight(Local2, 1, Local2) > > + } Else { > > + // Read next byte from cpu bitmap > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > + } > > + // Local3 = active state for this cpu > > + Store(And(Local2, 1), Local3) > > + > > + If (LNotEqual(Local1, Local3)) { > > + // State change - update CPON with new state > > + Store(Local3, Index(CPON, Local0)) > > + // Do CPU notify > > + If (LEqual(Local3, 1)) { > > + NTFY(Local0, 1) > > + } Else { > > + NTFY(Local0, 3) > > + } > > + } > > + Increment(Local0) > > + } > > + } > > + > > + /* Leave bit 0 cleared to avoid Windows BSOD */ > > + Name(_STA, 0xA) > > + > > + Method(_CRS, 0) { > > + Store(ResourceTemplate() { > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO) > > + }, Local0) > > + > > + CreateWordField(Local0, IO._MIN, IOMN) > > + CreateWordField(Local0, IO._MAX, IOMX) > > + > > + Store(CPHP, IOMN) > > + Subtract(Add(CPHP, CPPL), 1, IOMX) > > + Return(Local0) > > + } > > + } // Device(CPHD) > > + } // Scope(\_SB) > > } > > -- > > 1.8.3.1
On 12/16/13 21:38, Igor Mammedov wrote: > On Mon, 16 Dec 2013 21:30:14 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: >>> .. and report range used by it to OSPM via _CRS. >>> PRST is needed in SSDT since its base will depend on >>> chipset and will be dynamically set by QEMU. >>> Also move PRSC() method along with PRST since cross >>> table reference to PRST doesn't work. >> >> Could you clarify this last sentence? >> I don't mind where it is but I'd like to know >> where does the limitation come from. > It's empiric deduction so far I haven't found such limitation in spec yet. > iasl builds tables just fine but neither linux nor windows were able to find > Operation region from SSDT when loading DSDT, failing whole table loading > process. Decompiling DSDT/SSDT tables in guest shows that region is in > expected scope but OSPM refuses to see it when referenced outside SSDT. There seem to be four things here: - the OperationRegion definition, - its external declaration, - the Field() declaration, - use of fields. I think referencing an OperationRegion defined in another table should work (by way of External). I suspect the tricky part is with Field(): The fields are parts of the object named by RegionName, but their names appear in the same scope as the Field term. So, - maybe moving PRST only, and leaving the definition of PRS (as part of Field()) together with PRSC would suffice, - or, after moving the definition of PRS (as part of Field()) together with PRST to another table, all references to PRS (in the PRSC method) would have to be qualified. (But I guess this is what you tried.) Laszlo
On 12/16/13 22:13, Laszlo Ersek wrote: > On 12/16/13 21:38, Igor Mammedov wrote: >> On Mon, 16 Dec 2013 21:30:14 +0200 >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> >>> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: >>>> .. and report range used by it to OSPM via _CRS. >>>> PRST is needed in SSDT since its base will depend on >>>> chipset and will be dynamically set by QEMU. >>>> Also move PRSC() method along with PRST since cross >>>> table reference to PRST doesn't work. >>> >>> Could you clarify this last sentence? >>> I don't mind where it is but I'd like to know >>> where does the limitation come from. >> It's empiric deduction so far I haven't found such limitation in spec yet. >> iasl builds tables just fine but neither linux nor windows were able to find >> Operation region from SSDT when loading DSDT, failing whole table loading >> process. Decompiling DSDT/SSDT tables in guest shows that region is in >> expected scope but OSPM refuses to see it when referenced outside SSDT. > > There seem to be four things here: > - the OperationRegion definition, > - its external declaration, > - the Field() declaration, > - use of fields. > > I think referencing an OperationRegion defined in another table should > work (by way of External). I suspect the tricky part is with Field(): > > The fields are parts of the object named by RegionName, but their > names appear in the same scope as the Field term. > > So, > - maybe moving PRST only, and leaving the definition of PRS (as part of > Field()) together with PRSC would suffice, I think this might be exactly what we don in OVMF's builtin tables. (I could of course fully misinterpret the situation.) In the DSDT we have a _CRS method that includes: - External (FWDT, OpRegionObj) - Field(FWDT, QWordAcc, NoLock, Preserve) { ... fields ... } - accesses to these fields In the SSDT we have: - OperationRegion(FWDT, ...) So I guess: - you could put the PRST in the SSDT (this is the goal), - you could keep the PRSC method in DSDT, - but an External reference and the PRS field declaration would also have to remain in DSDT. Laszlo
On Mon, 16 Dec 2013 22:13:30 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 12/16/13 21:38, Igor Mammedov wrote: > > On Mon, 16 Dec 2013 21:30:14 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > >> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > >>> .. and report range used by it to OSPM via _CRS. > >>> PRST is needed in SSDT since its base will depend on > >>> chipset and will be dynamically set by QEMU. > >>> Also move PRSC() method along with PRST since cross > >>> table reference to PRST doesn't work. > >> > >> Could you clarify this last sentence? > >> I don't mind where it is but I'd like to know > >> where does the limitation come from. > > It's empiric deduction so far I haven't found such limitation in spec yet. > > iasl builds tables just fine but neither linux nor windows were able to find > > Operation region from SSDT when loading DSDT, failing whole table loading > > process. Decompiling DSDT/SSDT tables in guest shows that region is in > > expected scope but OSPM refuses to see it when referenced outside SSDT. > > There seem to be four things here: > - the OperationRegion definition, > - its external declaration, > - the Field() declaration, > - use of fields. > > I think referencing an OperationRegion defined in another table should > work (by way of External). I suspect the tricky part is with Field(): ^^^ it looks like it should work and decompiled tables look fine as well but it unfortunately doesn't. > > The fields are parts of the object named by RegionName, but their > names appear in the same scope as the Field term. > > So, > - maybe moving PRST only, and leaving the definition of PRS (as part of > Field()) together with PRSC would suffice, That was the first thing I've tried. > - or, after moving the definition of PRS (as part of Field()) together > with PRST to another table, all references to PRS (in the PRSC method) > would have to be qualified. (But I guess this is what you tried.) yep, that didn't work too. I'm not fun of moving a bunch of code around, but looks like there is no other way. I'd be happy to try if there are any other suggestions. > > Laszlo >
On Mon, 16 Dec 2013 21:53:07 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > > .. and report range used by it to OSPM via _CRS. > > PRST is needed in SSDT since its base will depend on > > chipset and will be dynamically set by QEMU. > > Also move PRSC() method along with PRST since cross > > table reference to PRST doesn't work. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +---------------------- > > hw/i386/acpi-dsdt.dsl | 2 +- > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 68 insertions(+), 40 deletions(-) > > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > index 995b415..f26f81b 100644 > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > @@ -20,6 +20,7 @@ > > Scope(\_SB) { > > /* Objects filled in by run-time generated SSDT */ > > External(NTFY, MethodObj) > > + External(\_SB.CPHD.PRSC, MethodObj) > > External(CPON, PkgObj) > > > > /* Methods called by run-time generated SSDT Processor objects */ > > @@ -51,42 +52,4 @@ Scope(\_SB) { > > // _EJ0 method - eject callback > > Sleep(200) > > } > > - > > - OperationRegion(PRST, SystemIO, 0xaf00, 32) > > - Field(PRST, ByteAcc, NoLock, Preserve) { > > - PRS, 256 > > - } > > - Method(PRSC, 0) { > > - // Local5 = active cpu bitmap > > - Store(PRS, Local5) > > - // Local2 = last read byte from bitmap > > - Store(Zero, Local2) > > - // Local0 = Processor ID / APIC ID iterator > > - Store(Zero, Local0) > > - While (LLess(Local0, SizeOf(CPON))) { > > - // Local1 = CPON flag for this cpu > > - Store(DerefOf(Index(CPON, Local0)), Local1) > > - If (And(Local0, 0x07)) { > > - // Shift down previously read bitmap byte > > - ShiftRight(Local2, 1, Local2) > > - } Else { > > - // Read next byte from cpu bitmap > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > - } > > - // Local3 = active state for this cpu > > - Store(And(Local2, 1), Local3) > > - > > - If (LNotEqual(Local1, Local3)) { > > - // State change - update CPON with new state > > - Store(Local3, Index(CPON, Local0)) > > - // Do CPU notify > > - If (LEqual(Local3, 1)) { > > - NTFY(Local0, 1) > > - } Else { > > - NTFY(Local0, 3) > > - } > > - } > > - Increment(Local0) > > - } > > - } > > } > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > index 90efce0..fa9f2d4 100644 > > --- a/hw/i386/acpi-dsdt.dsl > > +++ b/hw/i386/acpi-dsdt.dsl > > @@ -311,7 +311,7 @@ DefinitionBlock ( > > } > > Method(_E02) { > > // CPU hotplug event > > - \_SB.PRSC() > > + \_SB.CPHD.PRSC() > > } > > Method(_L03) { > > } > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > index 22baa58..9ccc543 100644 > > --- a/hw/i386/q35-acpi-dsdt.dsl > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > @@ -420,7 +420,7 @@ DefinitionBlock ( > > } > > Method(_E02) { > > // CPU hotplug event > > - \_SB.PRSC() > > + \_SB.CPHD.PRSC() > > } > > Method(_L03) { > > } > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl > > index a4484b8..ec8893c 100644 > > --- a/hw/i386/ssdt-misc.dsl > > +++ b/hw/i386/ssdt-misc.dsl > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) > > } > > } > > } > > + Scope(\_SB) { > > + External(NTFY, MethodObj) > > + External(CPON, PkgObj) > > + > > + Device(CPHD) { > > + Name(_HID, EISAID("PNP0C08")) > > + Name(CPPL, 32) // cpu-gpe length > > + Name(CPHP, 0xaf00) > > + > > + OperationRegion(PRST, SystemIO, CPHP, CPPL) > > + Field(PRST, ByteAcc, NoLock, Preserve) { > > + PRS, 256 > > + } > > + > > + Method(PRSC, 0) { > > + // Local5 = active cpu bitmap > > + Store(PRS, Local5) > > + // Local2 = last read byte from bitmap > > + Store(Zero, Local2) > > + // Local0 = Processor ID / APIC ID iterator > > + Store(Zero, Local0) > > + While (LLess(Local0, SizeOf(CPON))) { > > + // Local1 = CPON flag for this cpu > > + Store(DerefOf(Index(CPON, Local0)), Local1) > > + If (And(Local0, 0x07)) { > > + // Shift down previously read bitmap byte > > + ShiftRight(Local2, 1, Local2) > > + } Else { > > + // Read next byte from cpu bitmap > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > + } > > + // Local3 = active state for this cpu > > + Store(And(Local2, 1), Local3) > > + > > + If (LNotEqual(Local1, Local3)) { > > + // State change - update CPON with new state > > + Store(Local3, Index(CPON, Local0)) > > + // Do CPU notify > > + If (LEqual(Local3, 1)) { > > + NTFY(Local0, 1) > > + } Else { > > + NTFY(Local0, 3) > > + } > > + } > > + Increment(Local0) > > + } > > + } > > + > > + /* Leave bit 0 cleared to avoid Windows BSOD */ > > + Name(_STA, 0xA) > > This shared the problem you yourself pointed out with > patch 'pc: ACPI BIOS: implement memory hotplug': > if we make device non present ospm can ignore our _CRS. I repeat my question is there need to expose it OperationRegion as _CRS? > > Can't we get a better handle on why windows crashes? Short of installing debug build and looking at crash dump and/or reverse engineering acpi.sys, might give a clue, I don't have any other idea. Are there any simpler suggestions? > > > > + > > + Method(_CRS, 0) { > > + Store(ResourceTemplate() { > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO) > > + }, Local0) > > + > > + CreateWordField(Local0, IO._MIN, IOMN) > > + CreateWordField(Local0, IO._MAX, IOMX) > > + > > + Store(CPHP, IOMN) > > + Subtract(Add(CPHP, CPPL), 1, IOMX) > > + Return(Local0) > > + } > > + } // Device(CPHD) > > + } // Scope(\_SB) > > } > > -- > > 1.8.3.1 >
On Mon, Dec 16, 2013 at 10:53:24PM +0100, Igor Mammedov wrote: > On Mon, 16 Dec 2013 22:13:30 +0100 > Laszlo Ersek <lersek@redhat.com> wrote: > > > On 12/16/13 21:38, Igor Mammedov wrote: > > > On Mon, 16 Dec 2013 21:30:14 +0200 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > >> On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > > >>> .. and report range used by it to OSPM via _CRS. > > >>> PRST is needed in SSDT since its base will depend on > > >>> chipset and will be dynamically set by QEMU. > > >>> Also move PRSC() method along with PRST since cross > > >>> table reference to PRST doesn't work. > > >> > > >> Could you clarify this last sentence? > > >> I don't mind where it is but I'd like to know > > >> where does the limitation come from. > > > It's empiric deduction so far I haven't found such limitation in spec yet. > > > iasl builds tables just fine but neither linux nor windows were able to find > > > Operation region from SSDT when loading DSDT, failing whole table loading > > > process. Decompiling DSDT/SSDT tables in guest shows that region is in > > > expected scope but OSPM refuses to see it when referenced outside SSDT. > > > > There seem to be four things here: > > - the OperationRegion definition, > > - its external declaration, > > - the Field() declaration, > > - use of fields. > > > > I think referencing an OperationRegion defined in another table should > > work (by way of External). I suspect the tricky part is with Field(): > ^^^ it looks like it should work and decompiled tables > look fine as well but it unfortunately doesn't. > > > > > The fields are parts of the object named by RegionName, but their > > names appear in the same scope as the Field term. > > > > So, > > - maybe moving PRST only, and leaving the definition of PRS (as part of > > Field()) together with PRSC would suffice, > That was the first thing I've tried. > > > - or, after moving the definition of PRS (as part of Field()) together > > with PRST to another table, all references to PRS (in the PRSC method) > > would have to be qualified. (But I guess this is what you tried.) > yep, that didn't work too. > > I'm not fun of moving a bunch of code around, but looks like there is > no other way. I'd be happy to try if there are any other suggestions. I'm not sure we need to spend too much time on it. Just clarifying that 'doesn't work' means 'builds but makes OSPM fail to resolve the OperationRegion, even though the spec makes it look like this should work' in the commit log should be enough. > > > > Laszlo > > > > > -- > Regards, > Igor
On Mon, 16 Dec 2013 21:53:07 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > > .. and report range used by it to OSPM via _CRS. > > PRST is needed in SSDT since its base will depend on > > chipset and will be dynamically set by QEMU. > > Also move PRSC() method along with PRST since cross > > table reference to PRST doesn't work. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +---------------------- > > hw/i386/acpi-dsdt.dsl | 2 +- > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 68 insertions(+), 40 deletions(-) > > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > index 995b415..f26f81b 100644 > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > @@ -20,6 +20,7 @@ > > Scope(\_SB) { > > /* Objects filled in by run-time generated SSDT */ > > External(NTFY, MethodObj) > > + External(\_SB.CPHD.PRSC, MethodObj) > > External(CPON, PkgObj) > > > > /* Methods called by run-time generated SSDT Processor objects */ > > @@ -51,42 +52,4 @@ Scope(\_SB) { > > // _EJ0 method - eject callback > > Sleep(200) > > } > > - > > - OperationRegion(PRST, SystemIO, 0xaf00, 32) > > - Field(PRST, ByteAcc, NoLock, Preserve) { > > - PRS, 256 > > - } > > - Method(PRSC, 0) { > > - // Local5 = active cpu bitmap > > - Store(PRS, Local5) > > - // Local2 = last read byte from bitmap > > - Store(Zero, Local2) > > - // Local0 = Processor ID / APIC ID iterator > > - Store(Zero, Local0) > > - While (LLess(Local0, SizeOf(CPON))) { > > - // Local1 = CPON flag for this cpu > > - Store(DerefOf(Index(CPON, Local0)), Local1) > > - If (And(Local0, 0x07)) { > > - // Shift down previously read bitmap byte > > - ShiftRight(Local2, 1, Local2) > > - } Else { > > - // Read next byte from cpu bitmap > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > - } > > - // Local3 = active state for this cpu > > - Store(And(Local2, 1), Local3) > > - > > - If (LNotEqual(Local1, Local3)) { > > - // State change - update CPON with new state > > - Store(Local3, Index(CPON, Local0)) > > - // Do CPU notify > > - If (LEqual(Local3, 1)) { > > - NTFY(Local0, 1) > > - } Else { > > - NTFY(Local0, 3) > > - } > > - } > > - Increment(Local0) > > - } > > - } > > } > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > index 90efce0..fa9f2d4 100644 > > --- a/hw/i386/acpi-dsdt.dsl > > +++ b/hw/i386/acpi-dsdt.dsl > > @@ -311,7 +311,7 @@ DefinitionBlock ( > > } > > Method(_E02) { > > // CPU hotplug event > > - \_SB.PRSC() > > + \_SB.CPHD.PRSC() > > } > > Method(_L03) { > > } > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > index 22baa58..9ccc543 100644 > > --- a/hw/i386/q35-acpi-dsdt.dsl > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > @@ -420,7 +420,7 @@ DefinitionBlock ( > > } > > Method(_E02) { > > // CPU hotplug event > > - \_SB.PRSC() > > + \_SB.CPHD.PRSC() > > } > > Method(_L03) { > > } > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl > > index a4484b8..ec8893c 100644 > > --- a/hw/i386/ssdt-misc.dsl > > +++ b/hw/i386/ssdt-misc.dsl > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) > > } > > } > > } > > + Scope(\_SB) { > > + External(NTFY, MethodObj) > > + External(CPON, PkgObj) > > + > > + Device(CPHD) { > > + Name(_HID, EISAID("PNP0C08")) > > + Name(CPPL, 32) // cpu-gpe length > > + Name(CPHP, 0xaf00) > > + > > + OperationRegion(PRST, SystemIO, CPHP, CPPL) > > + Field(PRST, ByteAcc, NoLock, Preserve) { > > + PRS, 256 > > + } > > + > > + Method(PRSC, 0) { > > + // Local5 = active cpu bitmap > > + Store(PRS, Local5) > > + // Local2 = last read byte from bitmap > > + Store(Zero, Local2) > > + // Local0 = Processor ID / APIC ID iterator > > + Store(Zero, Local0) > > + While (LLess(Local0, SizeOf(CPON))) { > > + // Local1 = CPON flag for this cpu > > + Store(DerefOf(Index(CPON, Local0)), Local1) > > + If (And(Local0, 0x07)) { > > + // Shift down previously read bitmap byte > > + ShiftRight(Local2, 1, Local2) > > + } Else { > > + // Read next byte from cpu bitmap > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > + } > > + // Local3 = active state for this cpu > > + Store(And(Local2, 1), Local3) > > + > > + If (LNotEqual(Local1, Local3)) { > > + // State change - update CPON with new state > > + Store(Local3, Index(CPON, Local0)) > > + // Do CPU notify > > + If (LEqual(Local3, 1)) { > > + NTFY(Local0, 1) > > + } Else { > > + NTFY(Local0, 3) > > + } > > + } > > + Increment(Local0) > > + } > > + } > > + > > + /* Leave bit 0 cleared to avoid Windows BSOD */ > > + Name(_STA, 0xA) > > This shared the problem you yourself pointed out with > patch 'pc: ACPI BIOS: implement memory hotplug': > if we make device non present ospm can ignore our _CRS. > > Can't we get a better handle on why windows crashes? Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack: : nt!PnpBootDeviceWait+0x181 : nt!IopInitializeBootDrivers+0xbd3 : nt!IoInitSystem+0xbb6 : nt!Phase1InitializationDiscard+0x16f6 : nt!Phase1Initialization+0x13 : nt!PspSystemThreadStartup+0x23d : nt!KiStartSystemThread+0x16 However it works fine if containing device is PNP0C02 "Motherboard registers", so we probably should use it instead. > > > > + > > + Method(_CRS, 0) { > > + Store(ResourceTemplate() { > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO) > > + }, Local0) > > + > > + CreateWordField(Local0, IO._MIN, IOMN) > > + CreateWordField(Local0, IO._MAX, IOMX) > > + > > + Store(CPHP, IOMN) > > + Subtract(Add(CPHP, CPPL), 1, IOMX) > > + Return(Local0) > > + } > > + } // Device(CPHD) > > + } // Scope(\_SB) > > } > > -- > > 1.8.3.1 >
On Sun, Dec 22, 2013 at 03:51:28PM +0100, Igor Mammedov wrote: > On Mon, 16 Dec 2013 21:53:07 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > > > .. and report range used by it to OSPM via _CRS. > > > PRST is needed in SSDT since its base will depend on > > > chipset and will be dynamically set by QEMU. > > > Also move PRSC() method along with PRST since cross > > > table reference to PRST doesn't work. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +---------------------- > > > hw/i386/acpi-dsdt.dsl | 2 +- > > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 68 insertions(+), 40 deletions(-) > > > > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > index 995b415..f26f81b 100644 > > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > @@ -20,6 +20,7 @@ > > > Scope(\_SB) { > > > /* Objects filled in by run-time generated SSDT */ > > > External(NTFY, MethodObj) > > > + External(\_SB.CPHD.PRSC, MethodObj) > > > External(CPON, PkgObj) > > > > > > /* Methods called by run-time generated SSDT Processor objects */ > > > @@ -51,42 +52,4 @@ Scope(\_SB) { > > > // _EJ0 method - eject callback > > > Sleep(200) > > > } > > > - > > > - OperationRegion(PRST, SystemIO, 0xaf00, 32) > > > - Field(PRST, ByteAcc, NoLock, Preserve) { > > > - PRS, 256 > > > - } > > > - Method(PRSC, 0) { > > > - // Local5 = active cpu bitmap > > > - Store(PRS, Local5) > > > - // Local2 = last read byte from bitmap > > > - Store(Zero, Local2) > > > - // Local0 = Processor ID / APIC ID iterator > > > - Store(Zero, Local0) > > > - While (LLess(Local0, SizeOf(CPON))) { > > > - // Local1 = CPON flag for this cpu > > > - Store(DerefOf(Index(CPON, Local0)), Local1) > > > - If (And(Local0, 0x07)) { > > > - // Shift down previously read bitmap byte > > > - ShiftRight(Local2, 1, Local2) > > > - } Else { > > > - // Read next byte from cpu bitmap > > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > > - } > > > - // Local3 = active state for this cpu > > > - Store(And(Local2, 1), Local3) > > > - > > > - If (LNotEqual(Local1, Local3)) { > > > - // State change - update CPON with new state > > > - Store(Local3, Index(CPON, Local0)) > > > - // Do CPU notify > > > - If (LEqual(Local3, 1)) { > > > - NTFY(Local0, 1) > > > - } Else { > > > - NTFY(Local0, 3) > > > - } > > > - } > > > - Increment(Local0) > > > - } > > > - } > > > } > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > > index 90efce0..fa9f2d4 100644 > > > --- a/hw/i386/acpi-dsdt.dsl > > > +++ b/hw/i386/acpi-dsdt.dsl > > > @@ -311,7 +311,7 @@ DefinitionBlock ( > > > } > > > Method(_E02) { > > > // CPU hotplug event > > > - \_SB.PRSC() > > > + \_SB.CPHD.PRSC() > > > } > > > Method(_L03) { > > > } > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > > index 22baa58..9ccc543 100644 > > > --- a/hw/i386/q35-acpi-dsdt.dsl > > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > > @@ -420,7 +420,7 @@ DefinitionBlock ( > > > } > > > Method(_E02) { > > > // CPU hotplug event > > > - \_SB.PRSC() > > > + \_SB.CPHD.PRSC() > > > } > > > Method(_L03) { > > > } > > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl > > > index a4484b8..ec8893c 100644 > > > --- a/hw/i386/ssdt-misc.dsl > > > +++ b/hw/i386/ssdt-misc.dsl > > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) > > > } > > > } > > > } > > > + Scope(\_SB) { > > > + External(NTFY, MethodObj) > > > + External(CPON, PkgObj) > > > + > > > + Device(CPHD) { > > > + Name(_HID, EISAID("PNP0C08")) > > > + Name(CPPL, 32) // cpu-gpe length > > > + Name(CPHP, 0xaf00) > > > + > > > + OperationRegion(PRST, SystemIO, CPHP, CPPL) > > > + Field(PRST, ByteAcc, NoLock, Preserve) { > > > + PRS, 256 > > > + } > > > + > > > + Method(PRSC, 0) { > > > + // Local5 = active cpu bitmap > > > + Store(PRS, Local5) > > > + // Local2 = last read byte from bitmap > > > + Store(Zero, Local2) > > > + // Local0 = Processor ID / APIC ID iterator > > > + Store(Zero, Local0) > > > + While (LLess(Local0, SizeOf(CPON))) { > > > + // Local1 = CPON flag for this cpu > > > + Store(DerefOf(Index(CPON, Local0)), Local1) > > > + If (And(Local0, 0x07)) { > > > + // Shift down previously read bitmap byte > > > + ShiftRight(Local2, 1, Local2) > > > + } Else { > > > + // Read next byte from cpu bitmap > > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > > + } > > > + // Local3 = active state for this cpu > > > + Store(And(Local2, 1), Local3) > > > + > > > + If (LNotEqual(Local1, Local3)) { > > > + // State change - update CPON with new state > > > + Store(Local3, Index(CPON, Local0)) > > > + // Do CPU notify > > > + If (LEqual(Local3, 1)) { > > > + NTFY(Local0, 1) > > > + } Else { > > > + NTFY(Local0, 3) > > > + } > > > + } > > > + Increment(Local0) > > > + } > > > + } > > > + > > > + /* Leave bit 0 cleared to avoid Windows BSOD */ > > > + Name(_STA, 0xA) > > > > This shared the problem you yourself pointed out with > > patch 'pc: ACPI BIOS: implement memory hotplug': > > if we make device non present ospm can ignore our _CRS. > > > > Can't we get a better handle on why windows crashes? > Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or > ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack: > : nt!PnpBootDeviceWait+0x181 > : nt!IopInitializeBootDrivers+0xbd3 > : nt!IoInitSystem+0xbb6 > : nt!Phase1InitializationDiscard+0x16f6 > : nt!Phase1Initialization+0x13 > : nt!PspSystemThreadStartup+0x23d > : nt!KiStartSystemThread+0x16 Interesting. This seems to imply that it can't access IO port for the disk. Which boot disk type are you using? Is the _CRS resource overlapping any other resource by any chance? > > However it works fine if containing device is PNP0C02 "Motherboard registers", > so we probably should use it instead. > > > > > > > > > + > > > + Method(_CRS, 0) { > > > + Store(ResourceTemplate() { > > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO) > > > + }, Local0) > > > + > > > + CreateWordField(Local0, IO._MIN, IOMN) > > > + CreateWordField(Local0, IO._MAX, IOMX) > > > + > > > + Store(CPHP, IOMN) > > > + Subtract(Add(CPHP, CPPL), 1, IOMX) > > > + Return(Local0) > > > + } > > > + } // Device(CPHD) > > > + } // Scope(\_SB) > > > } > > > -- > > > 1.8.3.1 > > > > > -- > Regards, > Igor
On Mon, 23 Dec 2013 13:26:37 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Sun, Dec 22, 2013 at 03:51:28PM +0100, Igor Mammedov wrote: > > On Mon, 16 Dec 2013 21:53:07 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > > > > .. and report range used by it to OSPM via _CRS. > > > > PRST is needed in SSDT since its base will depend on > > > > chipset and will be dynamically set by QEMU. > > > > Also move PRSC() method along with PRST since cross > > > > table reference to PRST doesn't work. > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > --- > > > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +---------------------- > > > > hw/i386/acpi-dsdt.dsl | 2 +- > > > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 68 insertions(+), 40 deletions(-) > > > > > > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > index 995b415..f26f81b 100644 > > > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > @@ -20,6 +20,7 @@ > > > > Scope(\_SB) { > > > > /* Objects filled in by run-time generated SSDT */ > > > > External(NTFY, MethodObj) > > > > + External(\_SB.CPHD.PRSC, MethodObj) > > > > External(CPON, PkgObj) > > > > > > > > /* Methods called by run-time generated SSDT Processor objects */ > > > > @@ -51,42 +52,4 @@ Scope(\_SB) { > > > > // _EJ0 method - eject callback > > > > Sleep(200) > > > > } > > > > - > > > > - OperationRegion(PRST, SystemIO, 0xaf00, 32) > > > > - Field(PRST, ByteAcc, NoLock, Preserve) { > > > > - PRS, 256 > > > > - } > > > > - Method(PRSC, 0) { > > > > - // Local5 = active cpu bitmap > > > > - Store(PRS, Local5) > > > > - // Local2 = last read byte from bitmap > > > > - Store(Zero, Local2) > > > > - // Local0 = Processor ID / APIC ID iterator > > > > - Store(Zero, Local0) > > > > - While (LLess(Local0, SizeOf(CPON))) { > > > > - // Local1 = CPON flag for this cpu > > > > - Store(DerefOf(Index(CPON, Local0)), Local1) > > > > - If (And(Local0, 0x07)) { > > > > - // Shift down previously read bitmap byte > > > > - ShiftRight(Local2, 1, Local2) > > > > - } Else { > > > > - // Read next byte from cpu bitmap > > > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > > > - } > > > > - // Local3 = active state for this cpu > > > > - Store(And(Local2, 1), Local3) > > > > - > > > > - If (LNotEqual(Local1, Local3)) { > > > > - // State change - update CPON with new state > > > > - Store(Local3, Index(CPON, Local0)) > > > > - // Do CPU notify > > > > - If (LEqual(Local3, 1)) { > > > > - NTFY(Local0, 1) > > > > - } Else { > > > > - NTFY(Local0, 3) > > > > - } > > > > - } > > > > - Increment(Local0) > > > > - } > > > > - } > > > > } > > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > > > index 90efce0..fa9f2d4 100644 > > > > --- a/hw/i386/acpi-dsdt.dsl > > > > +++ b/hw/i386/acpi-dsdt.dsl > > > > @@ -311,7 +311,7 @@ DefinitionBlock ( > > > > } > > > > Method(_E02) { > > > > // CPU hotplug event > > > > - \_SB.PRSC() > > > > + \_SB.CPHD.PRSC() > > > > } > > > > Method(_L03) { > > > > } > > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > > > index 22baa58..9ccc543 100644 > > > > --- a/hw/i386/q35-acpi-dsdt.dsl > > > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > > > @@ -420,7 +420,7 @@ DefinitionBlock ( > > > > } > > > > Method(_E02) { > > > > // CPU hotplug event > > > > - \_SB.PRSC() > > > > + \_SB.CPHD.PRSC() > > > > } > > > > Method(_L03) { > > > > } > > > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl > > > > index a4484b8..ec8893c 100644 > > > > --- a/hw/i386/ssdt-misc.dsl > > > > +++ b/hw/i386/ssdt-misc.dsl > > > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) > > > > } > > > > } > > > > } > > > > + Scope(\_SB) { > > > > + External(NTFY, MethodObj) > > > > + External(CPON, PkgObj) > > > > + > > > > + Device(CPHD) { > > > > + Name(_HID, EISAID("PNP0C08")) > > > > + Name(CPPL, 32) // cpu-gpe length > > > > + Name(CPHP, 0xaf00) > > > > + > > > > + OperationRegion(PRST, SystemIO, CPHP, CPPL) > > > > + Field(PRST, ByteAcc, NoLock, Preserve) { > > > > + PRS, 256 > > > > + } > > > > + > > > > + Method(PRSC, 0) { > > > > + // Local5 = active cpu bitmap > > > > + Store(PRS, Local5) > > > > + // Local2 = last read byte from bitmap > > > > + Store(Zero, Local2) > > > > + // Local0 = Processor ID / APIC ID iterator > > > > + Store(Zero, Local0) > > > > + While (LLess(Local0, SizeOf(CPON))) { > > > > + // Local1 = CPON flag for this cpu > > > > + Store(DerefOf(Index(CPON, Local0)), Local1) > > > > + If (And(Local0, 0x07)) { > > > > + // Shift down previously read bitmap byte > > > > + ShiftRight(Local2, 1, Local2) > > > > + } Else { > > > > + // Read next byte from cpu bitmap > > > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > > > + } > > > > + // Local3 = active state for this cpu > > > > + Store(And(Local2, 1), Local3) > > > > + > > > > + If (LNotEqual(Local1, Local3)) { > > > > + // State change - update CPON with new state > > > > + Store(Local3, Index(CPON, Local0)) > > > > + // Do CPU notify > > > > + If (LEqual(Local3, 1)) { > > > > + NTFY(Local0, 1) > > > > + } Else { > > > > + NTFY(Local0, 3) > > > > + } > > > > + } > > > > + Increment(Local0) > > > > + } > > > > + } > > > > + > > > > + /* Leave bit 0 cleared to avoid Windows BSOD */ > > > > + Name(_STA, 0xA) > > > > > > This shared the problem you yourself pointed out with > > > patch 'pc: ACPI BIOS: implement memory hotplug': > > > if we make device non present ospm can ignore our _CRS. > > > > > > Can't we get a better handle on why windows crashes? > > Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or > > ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack: > > : nt!PnpBootDeviceWait+0x181 > > : nt!IopInitializeBootDrivers+0xbd3 > > : nt!IoInitSystem+0xbb6 > > : nt!Phase1InitializationDiscard+0x16f6 > > : nt!Phase1Initialization+0x13 > > : nt!PspSystemThreadStartup+0x23d > > : nt!KiStartSystemThread+0x16 > > > Interesting. This seems to imply that it can't access > IO port for the disk. Which boot disk type are you using? > Is the _CRS resource overlapping any other resource > by any chance? Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus. PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and basically take all io ports except of 0xcf8-0xcff hole. Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already covered by bus CRS, the same applies to PCI hotplug as well. So I was doing it wrong trying advertise bus resources out of bus scope. What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO ports CRS to PCI bus. Question is what PNP IDs they should use? It looks pretty much out of scope of cpu_hotplug and should be done for pci hotplug as well. So I'm ditching ACPI device and CRS parts from this series as not directly related. Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably for all resources consumed by it to make it right. > > > > > However it works fine if containing device is PNP0C02 "Motherboard registers", > > so we probably should use it instead. > > > > > > > > > > > > > > + > > > > + Method(_CRS, 0) { > > > > + Store(ResourceTemplate() { > > > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO) > > > > + }, Local0) > > > > + > > > > + CreateWordField(Local0, IO._MIN, IOMN) > > > > + CreateWordField(Local0, IO._MAX, IOMX) > > > > + > > > > + Store(CPHP, IOMN) > > > > + Subtract(Add(CPHP, CPPL), 1, IOMX) > > > > + Return(Local0) > > > > + } > > > > + } // Device(CPHD) > > > > + } // Scope(\_SB) > > > > } > > > > -- > > > > 1.8.3.1 > > > > > > > > > -- > > Regards, > > Igor
On Mon, Dec 23, 2013 at 02:06:27PM +0100, Igor Mammedov wrote: > On Mon, 23 Dec 2013 13:26:37 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Sun, Dec 22, 2013 at 03:51:28PM +0100, Igor Mammedov wrote: > > > On Mon, 16 Dec 2013 21:53:07 +0200 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > > > > > .. and report range used by it to OSPM via _CRS. > > > > > PRST is needed in SSDT since its base will depend on > > > > > chipset and will be dynamically set by QEMU. > > > > > Also move PRSC() method along with PRST since cross > > > > > table reference to PRST doesn't work. > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > --- > > > > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +---------------------- > > > > > hw/i386/acpi-dsdt.dsl | 2 +- > > > > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > > > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++ > > > > > 4 files changed, 68 insertions(+), 40 deletions(-) > > > > > > > > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > > index 995b415..f26f81b 100644 > > > > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > > @@ -20,6 +20,7 @@ > > > > > Scope(\_SB) { > > > > > /* Objects filled in by run-time generated SSDT */ > > > > > External(NTFY, MethodObj) > > > > > + External(\_SB.CPHD.PRSC, MethodObj) > > > > > External(CPON, PkgObj) > > > > > > > > > > /* Methods called by run-time generated SSDT Processor objects */ > > > > > @@ -51,42 +52,4 @@ Scope(\_SB) { > > > > > // _EJ0 method - eject callback > > > > > Sleep(200) > > > > > } > > > > > - > > > > > - OperationRegion(PRST, SystemIO, 0xaf00, 32) > > > > > - Field(PRST, ByteAcc, NoLock, Preserve) { > > > > > - PRS, 256 > > > > > - } > > > > > - Method(PRSC, 0) { > > > > > - // Local5 = active cpu bitmap > > > > > - Store(PRS, Local5) > > > > > - // Local2 = last read byte from bitmap > > > > > - Store(Zero, Local2) > > > > > - // Local0 = Processor ID / APIC ID iterator > > > > > - Store(Zero, Local0) > > > > > - While (LLess(Local0, SizeOf(CPON))) { > > > > > - // Local1 = CPON flag for this cpu > > > > > - Store(DerefOf(Index(CPON, Local0)), Local1) > > > > > - If (And(Local0, 0x07)) { > > > > > - // Shift down previously read bitmap byte > > > > > - ShiftRight(Local2, 1, Local2) > > > > > - } Else { > > > > > - // Read next byte from cpu bitmap > > > > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > > > > - } > > > > > - // Local3 = active state for this cpu > > > > > - Store(And(Local2, 1), Local3) > > > > > - > > > > > - If (LNotEqual(Local1, Local3)) { > > > > > - // State change - update CPON with new state > > > > > - Store(Local3, Index(CPON, Local0)) > > > > > - // Do CPU notify > > > > > - If (LEqual(Local3, 1)) { > > > > > - NTFY(Local0, 1) > > > > > - } Else { > > > > > - NTFY(Local0, 3) > > > > > - } > > > > > - } > > > > > - Increment(Local0) > > > > > - } > > > > > - } > > > > > } > > > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > > > > index 90efce0..fa9f2d4 100644 > > > > > --- a/hw/i386/acpi-dsdt.dsl > > > > > +++ b/hw/i386/acpi-dsdt.dsl > > > > > @@ -311,7 +311,7 @@ DefinitionBlock ( > > > > > } > > > > > Method(_E02) { > > > > > // CPU hotplug event > > > > > - \_SB.PRSC() > > > > > + \_SB.CPHD.PRSC() > > > > > } > > > > > Method(_L03) { > > > > > } > > > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > > > > index 22baa58..9ccc543 100644 > > > > > --- a/hw/i386/q35-acpi-dsdt.dsl > > > > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > > > > @@ -420,7 +420,7 @@ DefinitionBlock ( > > > > > } > > > > > Method(_E02) { > > > > > // CPU hotplug event > > > > > - \_SB.PRSC() > > > > > + \_SB.CPHD.PRSC() > > > > > } > > > > > Method(_L03) { > > > > > } > > > > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl > > > > > index a4484b8..ec8893c 100644 > > > > > --- a/hw/i386/ssdt-misc.dsl > > > > > +++ b/hw/i386/ssdt-misc.dsl > > > > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) > > > > > } > > > > > } > > > > > } > > > > > + Scope(\_SB) { > > > > > + External(NTFY, MethodObj) > > > > > + External(CPON, PkgObj) > > > > > + > > > > > + Device(CPHD) { > > > > > + Name(_HID, EISAID("PNP0C08")) > > > > > + Name(CPPL, 32) // cpu-gpe length > > > > > + Name(CPHP, 0xaf00) > > > > > + > > > > > + OperationRegion(PRST, SystemIO, CPHP, CPPL) > > > > > + Field(PRST, ByteAcc, NoLock, Preserve) { > > > > > + PRS, 256 > > > > > + } > > > > > + > > > > > + Method(PRSC, 0) { > > > > > + // Local5 = active cpu bitmap > > > > > + Store(PRS, Local5) > > > > > + // Local2 = last read byte from bitmap > > > > > + Store(Zero, Local2) > > > > > + // Local0 = Processor ID / APIC ID iterator > > > > > + Store(Zero, Local0) > > > > > + While (LLess(Local0, SizeOf(CPON))) { > > > > > + // Local1 = CPON flag for this cpu > > > > > + Store(DerefOf(Index(CPON, Local0)), Local1) > > > > > + If (And(Local0, 0x07)) { > > > > > + // Shift down previously read bitmap byte > > > > > + ShiftRight(Local2, 1, Local2) > > > > > + } Else { > > > > > + // Read next byte from cpu bitmap > > > > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > > > > + } > > > > > + // Local3 = active state for this cpu > > > > > + Store(And(Local2, 1), Local3) > > > > > + > > > > > + If (LNotEqual(Local1, Local3)) { > > > > > + // State change - update CPON with new state > > > > > + Store(Local3, Index(CPON, Local0)) > > > > > + // Do CPU notify > > > > > + If (LEqual(Local3, 1)) { > > > > > + NTFY(Local0, 1) > > > > > + } Else { > > > > > + NTFY(Local0, 3) > > > > > + } > > > > > + } > > > > > + Increment(Local0) > > > > > + } > > > > > + } > > > > > + > > > > > + /* Leave bit 0 cleared to avoid Windows BSOD */ > > > > > + Name(_STA, 0xA) > > > > > > > > This shared the problem you yourself pointed out with > > > > patch 'pc: ACPI BIOS: implement memory hotplug': > > > > if we make device non present ospm can ignore our _CRS. > > > > > > > > Can't we get a better handle on why windows crashes? > > > Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or > > > ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack: > > > : nt!PnpBootDeviceWait+0x181 > > > : nt!IopInitializeBootDrivers+0xbd3 > > > : nt!IoInitSystem+0xbb6 > > > : nt!Phase1InitializationDiscard+0x16f6 > > > : nt!Phase1Initialization+0x13 > > > : nt!PspSystemThreadStartup+0x23d > > > : nt!KiStartSystemThread+0x16 > > > > > > Interesting. This seems to imply that it can't access > > IO port for the disk. Which boot disk type are you using? > > Is the _CRS resource overlapping any other resource > > by any chance? > Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus. > PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and > basically take all io ports except of 0xcf8-0xcff hole. > Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already > covered by bus CRS, the same applies to PCI hotplug as well. > So I was doing it wrong trying advertise bus resources out of bus scope. Yes, that's explicitly prohibited by the firmware specification. > What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO > ports CRS to PCI bus. Question is what PNP IDs they should use? > > It looks pretty much out of scope of cpu_hotplug and should be done for > pci hotplug as well. So I'm ditching ACPI device and CRS parts from this > series as not directly related. > Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably > for all resources consumed by it to make it right. Could be ok but are we using any new ports? If yes then I think before doing that we should make sure _CRS for the host bridge does not include them, or consume them in some child device. Otherwise we increase the chance of a conflict in case some guest uses that port. > > > > > > > > > However it works fine if containing device is PNP0C02 "Motherboard registers", > > > so we probably should use it instead. > > > > > > > > > > > > > > > > > > > + > > > > > + Method(_CRS, 0) { > > > > > + Store(ResourceTemplate() { > > > > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO) > > > > > + }, Local0) > > > > > + > > > > > + CreateWordField(Local0, IO._MIN, IOMN) > > > > > + CreateWordField(Local0, IO._MAX, IOMX) > > > > > + > > > > > + Store(CPHP, IOMN) > > > > > + Subtract(Add(CPHP, CPPL), 1, IOMX) > > > > > + Return(Local0) > > > > > + } > > > > > + } // Device(CPHD) > > > > > + } // Scope(\_SB) > > > > > } > > > > > -- > > > > > 1.8.3.1 > > > > > > > > > > > > > -- > > > Regards, > > > Igor > > > -- > Regards, > Igor
On Mon, 23 Dec 2013 16:48:49 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Mon, Dec 23, 2013 at 02:06:27PM +0100, Igor Mammedov wrote: > > On Mon, 23 Dec 2013 13:26:37 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Sun, Dec 22, 2013 at 03:51:28PM +0100, Igor Mammedov wrote: > > > > On Mon, 16 Dec 2013 21:53:07 +0200 > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > > > > > > .. and report range used by it to OSPM via _CRS. > > > > > > PRST is needed in SSDT since its base will depend on > > > > > > chipset and will be dynamically set by QEMU. > > > > > > Also move PRSC() method along with PRST since cross > > > > > > table reference to PRST doesn't work. > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > --- > > > > > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +---------------------- > > > > > > hw/i386/acpi-dsdt.dsl | 2 +- > > > > > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > > > > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++ > > > > > > 4 files changed, 68 insertions(+), 40 deletions(-) > > > > > > > > > > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > > > index 995b415..f26f81b 100644 > > > > > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > > > @@ -20,6 +20,7 @@ > > > > > > Scope(\_SB) { > > > > > > /* Objects filled in by run-time generated SSDT */ > > > > > > External(NTFY, MethodObj) > > > > > > + External(\_SB.CPHD.PRSC, MethodObj) > > > > > > External(CPON, PkgObj) > > > > > > > > > > > > /* Methods called by run-time generated SSDT Processor objects */ > > > > > > @@ -51,42 +52,4 @@ Scope(\_SB) { > > > > > > // _EJ0 method - eject callback > > > > > > Sleep(200) > > > > > > } > > > > > > - > > > > > > - OperationRegion(PRST, SystemIO, 0xaf00, 32) > > > > > > - Field(PRST, ByteAcc, NoLock, Preserve) { > > > > > > - PRS, 256 > > > > > > - } > > > > > > - Method(PRSC, 0) { > > > > > > - // Local5 = active cpu bitmap > > > > > > - Store(PRS, Local5) > > > > > > - // Local2 = last read byte from bitmap > > > > > > - Store(Zero, Local2) > > > > > > - // Local0 = Processor ID / APIC ID iterator > > > > > > - Store(Zero, Local0) > > > > > > - While (LLess(Local0, SizeOf(CPON))) { > > > > > > - // Local1 = CPON flag for this cpu > > > > > > - Store(DerefOf(Index(CPON, Local0)), Local1) > > > > > > - If (And(Local0, 0x07)) { > > > > > > - // Shift down previously read bitmap byte > > > > > > - ShiftRight(Local2, 1, Local2) > > > > > > - } Else { > > > > > > - // Read next byte from cpu bitmap > > > > > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > > > > > - } > > > > > > - // Local3 = active state for this cpu > > > > > > - Store(And(Local2, 1), Local3) > > > > > > - > > > > > > - If (LNotEqual(Local1, Local3)) { > > > > > > - // State change - update CPON with new state > > > > > > - Store(Local3, Index(CPON, Local0)) > > > > > > - // Do CPU notify > > > > > > - If (LEqual(Local3, 1)) { > > > > > > - NTFY(Local0, 1) > > > > > > - } Else { > > > > > > - NTFY(Local0, 3) > > > > > > - } > > > > > > - } > > > > > > - Increment(Local0) > > > > > > - } > > > > > > - } > > > > > > } > > > > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > > > > > index 90efce0..fa9f2d4 100644 > > > > > > --- a/hw/i386/acpi-dsdt.dsl > > > > > > +++ b/hw/i386/acpi-dsdt.dsl > > > > > > @@ -311,7 +311,7 @@ DefinitionBlock ( > > > > > > } > > > > > > Method(_E02) { > > > > > > // CPU hotplug event > > > > > > - \_SB.PRSC() > > > > > > + \_SB.CPHD.PRSC() > > > > > > } > > > > > > Method(_L03) { > > > > > > } > > > > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > > > > > index 22baa58..9ccc543 100644 > > > > > > --- a/hw/i386/q35-acpi-dsdt.dsl > > > > > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > > > > > @@ -420,7 +420,7 @@ DefinitionBlock ( > > > > > > } > > > > > > Method(_E02) { > > > > > > // CPU hotplug event > > > > > > - \_SB.PRSC() > > > > > > + \_SB.CPHD.PRSC() > > > > > > } > > > > > > Method(_L03) { > > > > > > } > > > > > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl > > > > > > index a4484b8..ec8893c 100644 > > > > > > --- a/hw/i386/ssdt-misc.dsl > > > > > > +++ b/hw/i386/ssdt-misc.dsl > > > > > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) > > > > > > } > > > > > > } > > > > > > } > > > > > > + Scope(\_SB) { > > > > > > + External(NTFY, MethodObj) > > > > > > + External(CPON, PkgObj) > > > > > > + > > > > > > + Device(CPHD) { > > > > > > + Name(_HID, EISAID("PNP0C08")) > > > > > > + Name(CPPL, 32) // cpu-gpe length > > > > > > + Name(CPHP, 0xaf00) > > > > > > + > > > > > > + OperationRegion(PRST, SystemIO, CPHP, CPPL) > > > > > > + Field(PRST, ByteAcc, NoLock, Preserve) { > > > > > > + PRS, 256 > > > > > > + } > > > > > > + > > > > > > + Method(PRSC, 0) { > > > > > > + // Local5 = active cpu bitmap > > > > > > + Store(PRS, Local5) > > > > > > + // Local2 = last read byte from bitmap > > > > > > + Store(Zero, Local2) > > > > > > + // Local0 = Processor ID / APIC ID iterator > > > > > > + Store(Zero, Local0) > > > > > > + While (LLess(Local0, SizeOf(CPON))) { > > > > > > + // Local1 = CPON flag for this cpu > > > > > > + Store(DerefOf(Index(CPON, Local0)), Local1) > > > > > > + If (And(Local0, 0x07)) { > > > > > > + // Shift down previously read bitmap byte > > > > > > + ShiftRight(Local2, 1, Local2) > > > > > > + } Else { > > > > > > + // Read next byte from cpu bitmap > > > > > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > > > > > + } > > > > > > + // Local3 = active state for this cpu > > > > > > + Store(And(Local2, 1), Local3) > > > > > > + > > > > > > + If (LNotEqual(Local1, Local3)) { > > > > > > + // State change - update CPON with new state > > > > > > + Store(Local3, Index(CPON, Local0)) > > > > > > + // Do CPU notify > > > > > > + If (LEqual(Local3, 1)) { > > > > > > + NTFY(Local0, 1) > > > > > > + } Else { > > > > > > + NTFY(Local0, 3) > > > > > > + } > > > > > > + } > > > > > > + Increment(Local0) > > > > > > + } > > > > > > + } > > > > > > + > > > > > > + /* Leave bit 0 cleared to avoid Windows BSOD */ > > > > > > + Name(_STA, 0xA) > > > > > > > > > > This shared the problem you yourself pointed out with > > > > > patch 'pc: ACPI BIOS: implement memory hotplug': > > > > > if we make device non present ospm can ignore our _CRS. > > > > > > > > > > Can't we get a better handle on why windows crashes? > > > > Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or > > > > ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack: > > > > : nt!PnpBootDeviceWait+0x181 > > > > : nt!IopInitializeBootDrivers+0xbd3 > > > > : nt!IoInitSystem+0xbb6 > > > > : nt!Phase1InitializationDiscard+0x16f6 > > > > : nt!Phase1Initialization+0x13 > > > > : nt!PspSystemThreadStartup+0x23d > > > > : nt!KiStartSystemThread+0x16 > > > > > > > > > Interesting. This seems to imply that it can't access > > > IO port for the disk. Which boot disk type are you using? > > > Is the _CRS resource overlapping any other resource > > > by any chance? > > Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus. > > PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and > > basically take all io ports except of 0xcf8-0xcff hole. > > Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already > > covered by bus CRS, the same applies to PCI hotplug as well. > > So I was doing it wrong trying advertise bus resources out of bus scope. > > Yes, that's explicitly prohibited by the firmware specification. > > > What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO > > ports CRS to PCI bus. Question is what PNP IDs they should use? > > > > It looks pretty much out of scope of cpu_hotplug and should be done for > > pci hotplug as well. So I'm ditching ACPI device and CRS parts from this > > series as not directly related. > > Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably > > for all resources consumed by it to make it right. > > Could be ok but are we using any new ports? yes, for q35. series adds 0xa18-0xa37 IO ports, it was requested by Gerd not to use 0xaf00-0xaf1f. > If yes then I think before doing that we should make sure _CRS for > the host bridge does not include them, or consume them I'm fine with making holes in PCI bus CRES template, I can do it for pci-hotplug as well while at it. > in some child device. that would be cleanest, but is there any suggestion what device(s) it would be for piix and ich9-lpc, i.e. which PNP IDs should we use? > Otherwise we increase the chance of a conflict in case some > guest uses that port. > > > > > > > > > > > > > > However it works fine if containing device is PNP0C02 "Motherboard registers", > > > > so we probably should use it instead. > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > + Method(_CRS, 0) { > > > > > > + Store(ResourceTemplate() { > > > > > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO) > > > > > > + }, Local0) > > > > > > + > > > > > > + CreateWordField(Local0, IO._MIN, IOMN) > > > > > > + CreateWordField(Local0, IO._MAX, IOMX) > > > > > > + > > > > > > + Store(CPHP, IOMN) > > > > > > + Subtract(Add(CPHP, CPPL), 1, IOMX) > > > > > > + Return(Local0) > > > > > > + } > > > > > > + } // Device(CPHD) > > > > > > + } // Scope(\_SB) > > > > > > } > > > > > > -- > > > > > > 1.8.3.1 > > > > > > > > > > > > > > > > > -- > > > > Regards, > > > > Igor > > > > > > -- > > Regards, > > Igor
On 12/23/13 17:24, Igor Mammedov wrote: > On Mon, 23 Dec 2013 16:48:49 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> On Mon, Dec 23, 2013 at 02:06:27PM +0100, Igor Mammedov wrote: >>> On Mon, 23 Dec 2013 13:26:37 +0200 >>> "Michael S. Tsirkin" <mst@redhat.com> wrote: >>>> Interesting. This seems to imply that it can't access >>>> IO port for the disk. Which boot disk type are you using? >>>> Is the _CRS resource overlapping any other resource >>>> by any chance? >>> Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus. >>> PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and >>> basically take all io ports except of 0xcf8-0xcff hole. >>> Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already >>> covered by bus CRS, the same applies to PCI hotplug as well. >>> So I was doing it wrong trying advertise bus resources out of bus scope. >> >> Yes, that's explicitly prohibited by the firmware specification. >> >>> What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO >>> ports CRS to PCI bus. Question is what PNP IDs they should use? >>> >>> It looks pretty much out of scope of cpu_hotplug and should be done for >>> pci hotplug as well. So I'm ditching ACPI device and CRS parts from this >>> series as not directly related. >>> Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably >>> for all resources consumed by it to make it right. >> >> Could be ok but are we using any new ports? > yes, for q35. series adds 0xa18-0xa37 IO ports, it was requested by Gerd not > to use 0xaf00-0xaf1f. > >> If yes then I think before doing that we should make sure _CRS for >> the host bridge does not include them, or consume them > I'm fine with making holes in PCI bus CRES template, I can do it for > pci-hotplug as well while at it. Can you guys please summarize the problem? (Just so I can understand.) \_SB.PCI0 consumes 0x0CF8..0x0CFF, and is a resource producer for all other IO ports (ie. it passes them on to child devices). Did we try to consume such a passed-on port in a device that is *not* a child of \_SB.PCI0? And if so, what's the suggested solution? Make the new consumer a child of \_SB.PCI0, or punch out the new (non-child) consumer's port range from \_SB.PCI0's forwarding? >> in some child device. > that would be cleanest, but is there any suggestion what device(s) it would be > for piix and ich9-lpc, i.e. which PNP IDs should we use? You could browse <http://www.plasma-online.de/english/identify/serial/pnp_id_pnp.html>. One suggestion could be: PNP0C02 -- General ID for reserving resources required by PnP motherboard registers. (Not device specific.) (AFAICS this PNP ID has been mentioned earlier in the thread.) PNP0C08 -- ACPI system board hardware (Also used already, apparently.) Laszlo
On Mon, Dec 23, 2013 at 05:24:30PM +0100, Igor Mammedov wrote: > On Mon, 23 Dec 2013 16:48:49 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Mon, Dec 23, 2013 at 02:06:27PM +0100, Igor Mammedov wrote: > > > On Mon, 23 Dec 2013 13:26:37 +0200 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Sun, Dec 22, 2013 at 03:51:28PM +0100, Igor Mammedov wrote: > > > > > On Mon, 16 Dec 2013 21:53:07 +0200 > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > > > > > On Fri, Dec 13, 2013 at 05:22:14PM +0100, Igor Mammedov wrote: > > > > > > > .. and report range used by it to OSPM via _CRS. > > > > > > > PRST is needed in SSDT since its base will depend on > > > > > > > chipset and will be dynamically set by QEMU. > > > > > > > Also move PRSC() method along with PRST since cross > > > > > > > table reference to PRST doesn't work. > > > > > > > > > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > > > > > --- > > > > > > > hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +---------------------- > > > > > > > hw/i386/acpi-dsdt.dsl | 2 +- > > > > > > > hw/i386/q35-acpi-dsdt.dsl | 2 +- > > > > > > > hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++ > > > > > > > 4 files changed, 68 insertions(+), 40 deletions(-) > > > > > > > > > > > > > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > > > > index 995b415..f26f81b 100644 > > > > > > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > > > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl > > > > > > > @@ -20,6 +20,7 @@ > > > > > > > Scope(\_SB) { > > > > > > > /* Objects filled in by run-time generated SSDT */ > > > > > > > External(NTFY, MethodObj) > > > > > > > + External(\_SB.CPHD.PRSC, MethodObj) > > > > > > > External(CPON, PkgObj) > > > > > > > > > > > > > > /* Methods called by run-time generated SSDT Processor objects */ > > > > > > > @@ -51,42 +52,4 @@ Scope(\_SB) { > > > > > > > // _EJ0 method - eject callback > > > > > > > Sleep(200) > > > > > > > } > > > > > > > - > > > > > > > - OperationRegion(PRST, SystemIO, 0xaf00, 32) > > > > > > > - Field(PRST, ByteAcc, NoLock, Preserve) { > > > > > > > - PRS, 256 > > > > > > > - } > > > > > > > - Method(PRSC, 0) { > > > > > > > - // Local5 = active cpu bitmap > > > > > > > - Store(PRS, Local5) > > > > > > > - // Local2 = last read byte from bitmap > > > > > > > - Store(Zero, Local2) > > > > > > > - // Local0 = Processor ID / APIC ID iterator > > > > > > > - Store(Zero, Local0) > > > > > > > - While (LLess(Local0, SizeOf(CPON))) { > > > > > > > - // Local1 = CPON flag for this cpu > > > > > > > - Store(DerefOf(Index(CPON, Local0)), Local1) > > > > > > > - If (And(Local0, 0x07)) { > > > > > > > - // Shift down previously read bitmap byte > > > > > > > - ShiftRight(Local2, 1, Local2) > > > > > > > - } Else { > > > > > > > - // Read next byte from cpu bitmap > > > > > > > - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > > > > > > - } > > > > > > > - // Local3 = active state for this cpu > > > > > > > - Store(And(Local2, 1), Local3) > > > > > > > - > > > > > > > - If (LNotEqual(Local1, Local3)) { > > > > > > > - // State change - update CPON with new state > > > > > > > - Store(Local3, Index(CPON, Local0)) > > > > > > > - // Do CPU notify > > > > > > > - If (LEqual(Local3, 1)) { > > > > > > > - NTFY(Local0, 1) > > > > > > > - } Else { > > > > > > > - NTFY(Local0, 3) > > > > > > > - } > > > > > > > - } > > > > > > > - Increment(Local0) > > > > > > > - } > > > > > > > - } > > > > > > > } > > > > > > > diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl > > > > > > > index 90efce0..fa9f2d4 100644 > > > > > > > --- a/hw/i386/acpi-dsdt.dsl > > > > > > > +++ b/hw/i386/acpi-dsdt.dsl > > > > > > > @@ -311,7 +311,7 @@ DefinitionBlock ( > > > > > > > } > > > > > > > Method(_E02) { > > > > > > > // CPU hotplug event > > > > > > > - \_SB.PRSC() > > > > > > > + \_SB.CPHD.PRSC() > > > > > > > } > > > > > > > Method(_L03) { > > > > > > > } > > > > > > > diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl > > > > > > > index 22baa58..9ccc543 100644 > > > > > > > --- a/hw/i386/q35-acpi-dsdt.dsl > > > > > > > +++ b/hw/i386/q35-acpi-dsdt.dsl > > > > > > > @@ -420,7 +420,7 @@ DefinitionBlock ( > > > > > > > } > > > > > > > Method(_E02) { > > > > > > > // CPU hotplug event > > > > > > > - \_SB.PRSC() > > > > > > > + \_SB.CPHD.PRSC() > > > > > > > } > > > > > > > Method(_L03) { > > > > > > > } > > > > > > > diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl > > > > > > > index a4484b8..ec8893c 100644 > > > > > > > --- a/hw/i386/ssdt-misc.dsl > > > > > > > +++ b/hw/i386/ssdt-misc.dsl > > > > > > > @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) > > > > > > > } > > > > > > > } > > > > > > > } > > > > > > > + Scope(\_SB) { > > > > > > > + External(NTFY, MethodObj) > > > > > > > + External(CPON, PkgObj) > > > > > > > + > > > > > > > + Device(CPHD) { > > > > > > > + Name(_HID, EISAID("PNP0C08")) > > > > > > > + Name(CPPL, 32) // cpu-gpe length > > > > > > > + Name(CPHP, 0xaf00) > > > > > > > + > > > > > > > + OperationRegion(PRST, SystemIO, CPHP, CPPL) > > > > > > > + Field(PRST, ByteAcc, NoLock, Preserve) { > > > > > > > + PRS, 256 > > > > > > > + } > > > > > > > + > > > > > > > + Method(PRSC, 0) { > > > > > > > + // Local5 = active cpu bitmap > > > > > > > + Store(PRS, Local5) > > > > > > > + // Local2 = last read byte from bitmap > > > > > > > + Store(Zero, Local2) > > > > > > > + // Local0 = Processor ID / APIC ID iterator > > > > > > > + Store(Zero, Local0) > > > > > > > + While (LLess(Local0, SizeOf(CPON))) { > > > > > > > + // Local1 = CPON flag for this cpu > > > > > > > + Store(DerefOf(Index(CPON, Local0)), Local1) > > > > > > > + If (And(Local0, 0x07)) { > > > > > > > + // Shift down previously read bitmap byte > > > > > > > + ShiftRight(Local2, 1, Local2) > > > > > > > + } Else { > > > > > > > + // Read next byte from cpu bitmap > > > > > > > + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) > > > > > > > + } > > > > > > > + // Local3 = active state for this cpu > > > > > > > + Store(And(Local2, 1), Local3) > > > > > > > + > > > > > > > + If (LNotEqual(Local1, Local3)) { > > > > > > > + // State change - update CPON with new state > > > > > > > + Store(Local3, Index(CPON, Local0)) > > > > > > > + // Do CPU notify > > > > > > > + If (LEqual(Local3, 1)) { > > > > > > > + NTFY(Local0, 1) > > > > > > > + } Else { > > > > > > > + NTFY(Local0, 3) > > > > > > > + } > > > > > > > + } > > > > > > > + Increment(Local0) > > > > > > > + } > > > > > > > + } > > > > > > > + > > > > > > > + /* Leave bit 0 cleared to avoid Windows BSOD */ > > > > > > > + Name(_STA, 0xA) > > > > > > > > > > > > This shared the problem you yourself pointed out with > > > > > > patch 'pc: ACPI BIOS: implement memory hotplug': > > > > > > if we make device non present ospm can ignore our _CRS. > > > > > > > > > > > > Can't we get a better handle on why windows crashes? > > > > > Whenever _CRS with PRST IO range is exposed as part of PNP0A05/PNP0A06 or > > > > > ACPI0004 device, it BSODs with INACCESSIBLE_BOOT_DEVICE with following stack: > > > > > : nt!PnpBootDeviceWait+0x181 > > > > > : nt!IopInitializeBootDrivers+0xbd3 > > > > > : nt!IoInitSystem+0xbb6 > > > > > : nt!Phase1InitializationDiscard+0x16f6 > > > > > : nt!Phase1Initialization+0x13 > > > > > : nt!PspSystemThreadStartup+0x23d > > > > > : nt!KiStartSystemThread+0x16 > > > > > > > > > > > > Interesting. This seems to imply that it can't access > > > > IO port for the disk. Which boot disk type are you using? > > > > Is the _CRS resource overlapping any other resource > > > > by any chance? > > > Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus. > > > PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and > > > basically take all io ports except of 0xcf8-0xcff hole. > > > Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already > > > covered by bus CRS, the same applies to PCI hotplug as well. > > > So I was doing it wrong trying advertise bus resources out of bus scope. > > > > Yes, that's explicitly prohibited by the firmware specification. > > > > > What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO > > > ports CRS to PCI bus. Question is what PNP IDs they should use? > > > > > > It looks pretty much out of scope of cpu_hotplug and should be done for > > > pci hotplug as well. So I'm ditching ACPI device and CRS parts from this > > > series as not directly related. > > > Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably > > > for all resources consumed by it to make it right. > > > > Could be ok but are we using any new ports? > yes, for q35. series adds 0xa18-0xa37 IO ports, it was requested by Gerd not > to use 0xaf00-0xaf1f. > > > If yes then I think before doing that we should make sure _CRS for > > the host bridge does not include them, or consume them > I'm fine with making holes in PCI bus CRES template, I can do it for > pci-hotplug as well while at it. Cool. We might need to make this conditional on using a new machine type. > > in some child device. > that would be cleanest, but is there any suggestion what device(s) it would be > for piix and ich9-lpc, i.e. which PNP IDs should we use? AFAIK if they are pci devices they don't need PNP IDs, they are matched by PCI bus address. > > > Otherwise we increase the chance of a conflict in case some > > guest uses that port. > > > > > > > > > > > > > > > > > > > However it works fine if containing device is PNP0C02 "Motherboard registers", > > > > > so we probably should use it instead. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > + Method(_CRS, 0) { > > > > > > > + Store(ResourceTemplate() { > > > > > > > + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO) > > > > > > > + }, Local0) > > > > > > > + > > > > > > > + CreateWordField(Local0, IO._MIN, IOMN) > > > > > > > + CreateWordField(Local0, IO._MAX, IOMX) > > > > > > > + > > > > > > > + Store(CPHP, IOMN) > > > > > > > + Subtract(Add(CPHP, CPPL), 1, IOMX) > > > > > > > + Return(Local0) > > > > > > > + } > > > > > > > + } // Device(CPHD) > > > > > > > + } // Scope(\_SB) > > > > > > > } > > > > > > > -- > > > > > > > 1.8.3.1 > > > > > > > > > > > > > > > > > > > > > -- > > > > > Regards, > > > > > Igor > > > > > > > > > -- > > > Regards, > > > Igor > > > -- > Regards, > Igor
On Mon, 23 Dec 2013 17:52:03 +0100 Laszlo Ersek <lersek@redhat.com> wrote: > On 12/23/13 17:24, Igor Mammedov wrote: > > On Mon, 23 Dec 2013 16:48:49 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > >> On Mon, Dec 23, 2013 at 02:06:27PM +0100, Igor Mammedov wrote: > >>> On Mon, 23 Dec 2013 13:26:37 +0200 > >>> "Michael S. Tsirkin" <mst@redhat.com> wrote: > > >>>> Interesting. This seems to imply that it can't access > >>>> IO port for the disk. Which boot disk type are you using? > >>>> Is the _CRS resource overlapping any other resource > >>>> by any chance? > >>> Yes, I've dug in the issue more and it is indeed _CRS overlapping with PCI bus. > >>> PCI bus's IO ports are statically defined in acpi-dsdt-pci-crs.dsl and > >>> basically take all io ports except of 0xcf8-0xcff hole. > >>> Since PIIX4_PM and ICH9 LPC are PCI devices, it appears that PRST already > >>> covered by bus CRS, the same applies to PCI hotplug as well. > >>> So I was doing it wrong trying advertise bus resources out of bus scope. > >> > >> Yes, that's explicitly prohibited by the firmware specification. > >> > >>> What we need is to add PIIX4_PM/ICH9 LPC device definition with consumed IO > >>> ports CRS to PCI bus. Question is what PNP IDs they should use? > >>> > >>> It looks pretty much out of scope of cpu_hotplug and should be done for > >>> pci hotplug as well. So I'm ditching ACPI device and CRS parts from this > >>> series as not directly related. > >>> Adding PIIX4_PM/ICH9 LPC ACPI device could be done later and preferably > >>> for all resources consumed by it to make it right. > >> > >> Could be ok but are we using any new ports? > > yes, for q35. series adds 0xa18-0xa37 IO ports, it was requested by Gerd not > > to use 0xaf00-0xaf1f. > > > >> If yes then I think before doing that we should make sure _CRS for > >> the host bridge does not include them, or consume them > > I'm fine with making holes in PCI bus CRES template, I can do it for > > pci-hotplug as well while at it. > > Can you guys please summarize the problem? (Just so I can understand.) > > \_SB.PCI0 consumes 0x0CF8..0x0CFF, and is a resource producer for all > other IO ports (ie. it passes them on to child devices). Did we try to > consume such a passed-on port in a device that is *not* a child of > \_SB.PCI0? yep, and that was an error. > > And if so, what's the suggested solution? Make the new consumer a child > of \_SB.PCI0, or punch out the new (non-child) consumer's port range > from \_SB.PCI0's forwarding? I've tried to add bridge that uses hotplug ports as Device under PCI bus with respective _CRS so it would consume ports, unfortunately for PIIX bridge isn't visible under Windows and any other ways to confirm that _CRS was consumed failed. The same for linux. For Q35, Windows' device manager shows LPC bridge however there is no "Resources" tab with consumed ports. Due to lack of ways to confirm that _CRS was really consumed, I've switched to punching holes, and exposing hotplug IO region as \_SB.Device(ACPI0004)._CRS. It works rather stable with new and old versions Windows. > > >> in some child device. > > that would be cleanest, but is there any suggestion what device(s) it would be > > for piix and ich9-lpc, i.e. which PNP IDs should we use? > > You could browse > <http://www.plasma-online.de/english/identify/serial/pnp_id_pnp.html>. > > One suggestion could be: > > PNP0C02 -- General ID for reserving resources required by PnP > motherboard registers. (Not device specific.) I've chosen ACPI0004 instead, as the later we might wish to group CPU devices under it (when extending support for 1024 and more CPUs) > > (AFAICS this PNP ID has been mentioned earlier in the thread.) > > PNP0C08 -- ACPI system board hardware it doesn't work. > > (Also used already, apparently.) > > Laszlo
diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl index 995b415..f26f81b 100644 --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl @@ -20,6 +20,7 @@ Scope(\_SB) { /* Objects filled in by run-time generated SSDT */ External(NTFY, MethodObj) + External(\_SB.CPHD.PRSC, MethodObj) External(CPON, PkgObj) /* Methods called by run-time generated SSDT Processor objects */ @@ -51,42 +52,4 @@ Scope(\_SB) { // _EJ0 method - eject callback Sleep(200) } - - OperationRegion(PRST, SystemIO, 0xaf00, 32) - Field(PRST, ByteAcc, NoLock, Preserve) { - PRS, 256 - } - Method(PRSC, 0) { - // Local5 = active cpu bitmap - Store(PRS, Local5) - // Local2 = last read byte from bitmap - Store(Zero, Local2) - // Local0 = Processor ID / APIC ID iterator - Store(Zero, Local0) - While (LLess(Local0, SizeOf(CPON))) { - // Local1 = CPON flag for this cpu - Store(DerefOf(Index(CPON, Local0)), Local1) - If (And(Local0, 0x07)) { - // Shift down previously read bitmap byte - ShiftRight(Local2, 1, Local2) - } Else { - // Read next byte from cpu bitmap - Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) - } - // Local3 = active state for this cpu - Store(And(Local2, 1), Local3) - - If (LNotEqual(Local1, Local3)) { - // State change - update CPON with new state - Store(Local3, Index(CPON, Local0)) - // Do CPU notify - If (LEqual(Local3, 1)) { - NTFY(Local0, 1) - } Else { - NTFY(Local0, 3) - } - } - Increment(Local0) - } - } } diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl index 90efce0..fa9f2d4 100644 --- a/hw/i386/acpi-dsdt.dsl +++ b/hw/i386/acpi-dsdt.dsl @@ -311,7 +311,7 @@ DefinitionBlock ( } Method(_E02) { // CPU hotplug event - \_SB.PRSC() + \_SB.CPHD.PRSC() } Method(_L03) { } diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl index 22baa58..9ccc543 100644 --- a/hw/i386/q35-acpi-dsdt.dsl +++ b/hw/i386/q35-acpi-dsdt.dsl @@ -420,7 +420,7 @@ DefinitionBlock ( } Method(_E02) { // CPU hotplug event - \_SB.PRSC() + \_SB.CPHD.PRSC() } Method(_L03) { } diff --git a/hw/i386/ssdt-misc.dsl b/hw/i386/ssdt-misc.dsl index a4484b8..ec8893c 100644 --- a/hw/i386/ssdt-misc.dsl +++ b/hw/i386/ssdt-misc.dsl @@ -116,4 +116,69 @@ DefinitionBlock ("ssdt-misc.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) } } } + Scope(\_SB) { + External(NTFY, MethodObj) + External(CPON, PkgObj) + + Device(CPHD) { + Name(_HID, EISAID("PNP0C08")) + Name(CPPL, 32) // cpu-gpe length + Name(CPHP, 0xaf00) + + OperationRegion(PRST, SystemIO, CPHP, CPPL) + Field(PRST, ByteAcc, NoLock, Preserve) { + PRS, 256 + } + + Method(PRSC, 0) { + // Local5 = active cpu bitmap + Store(PRS, Local5) + // Local2 = last read byte from bitmap + Store(Zero, Local2) + // Local0 = Processor ID / APIC ID iterator + Store(Zero, Local0) + While (LLess(Local0, SizeOf(CPON))) { + // Local1 = CPON flag for this cpu + Store(DerefOf(Index(CPON, Local0)), Local1) + If (And(Local0, 0x07)) { + // Shift down previously read bitmap byte + ShiftRight(Local2, 1, Local2) + } Else { + // Read next byte from cpu bitmap + Store(DerefOf(Index(Local5, ShiftRight(Local0, 3))), Local2) + } + // Local3 = active state for this cpu + Store(And(Local2, 1), Local3) + + If (LNotEqual(Local1, Local3)) { + // State change - update CPON with new state + Store(Local3, Index(CPON, Local0)) + // Do CPU notify + If (LEqual(Local3, 1)) { + NTFY(Local0, 1) + } Else { + NTFY(Local0, 3) + } + } + Increment(Local0) + } + } + + /* Leave bit 0 cleared to avoid Windows BSOD */ + Name(_STA, 0xA) + + Method(_CRS, 0) { + Store(ResourceTemplate() { + IO(Decode16, 0x00, 0x00, 0x01, 0x15, IO) + }, Local0) + + CreateWordField(Local0, IO._MIN, IOMN) + CreateWordField(Local0, IO._MAX, IOMX) + + Store(CPHP, IOMN) + Subtract(Add(CPHP, CPPL), 1, IOMX) + Return(Local0) + } + } // Device(CPHD) + } // Scope(\_SB) }
.. and report range used by it to OSPM via _CRS. PRST is needed in SSDT since its base will depend on chipset and will be dynamically set by QEMU. Also move PRSC() method along with PRST since cross table reference to PRST doesn't work. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/i386/acpi-dsdt-cpu-hotplug.dsl | 39 +---------------------- hw/i386/acpi-dsdt.dsl | 2 +- hw/i386/q35-acpi-dsdt.dsl | 2 +- hw/i386/ssdt-misc.dsl | 65 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 40 deletions(-)