[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT |
Date: |
Mon, 23 Dec 2013 18:55:39 +0200 |
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" <address@hidden> 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" <address@hidden> 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" <address@hidden> 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 <address@hidden>
> > > > > > > ---
> > > > > > > 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
- Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT, (continued)
Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT, Michael S. Tsirkin, 2013/12/16
- Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT, Igor Mammedov, 2013/12/16
- Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT, Igor Mammedov, 2013/12/22
- Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT, Michael S. Tsirkin, 2013/12/23
- Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT, Igor Mammedov, 2013/12/23
- Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT, Michael S. Tsirkin, 2013/12/23
- Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT, Igor Mammedov, 2013/12/23
- Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT,
Michael S. Tsirkin <=
- Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT, Laszlo Ersek, 2013/12/23
- Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT, Igor Mammedov, 2013/12/27
[Qemu-devel] [PATCH 07/11] ACPI: Q35 DSDT: fix CPU hotplug GPE0.2 handler, Igor Mammedov, 2013/12/13
[Qemu-devel] [PATCH 11/11] ACPI: update ssdt-misc.hex.generated acpi-dsdt.hex.generated q35-acpi-dsdt.hex.generated, Igor Mammedov, 2013/12/13
[Qemu-devel] [PATCH 03/11] acpi: factor out common cpu hotplug code for PIIX4/Q35, Igor Mammedov, 2013/12/13
[Qemu-devel] [PATCH 02/11] acpi: factor out common pm_update_sci() into acpi core, Igor Mammedov, 2013/12/13
[Qemu-devel] [PATCH 05/11] acpi: ich9: allow guest to clear SCI rised by GPE, Igor Mammedov, 2013/12/13