From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53095) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vv6ko-0005oC-Ko for qemu-devel@nongnu.org; Mon, 23 Dec 2013 09:45:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vv6ki-0008L0-KA for qemu-devel@nongnu.org; Mon, 23 Dec 2013 09:45:10 -0500 Received: from mx1.redhat.com ([209.132.183.28]:48968) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vv6ki-0008Hr-CM for qemu-devel@nongnu.org; Mon, 23 Dec 2013 09:45:04 -0500 Date: Mon, 23 Dec 2013 16:48:49 +0200 From: "Michael S. Tsirkin" Message-ID: <20131223144849.GE21800@redhat.com> References: <1386951736-929-1-git-send-email-imammedo@redhat.com> <1386951736-929-10-git-send-email-imammedo@redhat.com> <20131216195307.GA23942@redhat.com> <20131222155128.6f76675d@thinkpad> <20131223112637.GA5619@redhat.com> <20131223140627.49ba07be@thinkpad> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20131223140627.49ba07be@thinkpad> Subject: Re: [Qemu-devel] [PATCH 09/11] ACPI: move PRST OperationRegion into SSDT List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: aliguori@amazon.com, hutao@cn.fujitsu.com, jjherne@us.ibm.com, qemu-devel@nongnu.org, kraxel@redhat.com, stefanha@redhat.com, kaneshige.kenji@jp.fujitsu.com, chen.fan.fnst@cn.fujitsu.com, pbonzini@redhat.com, brogers@suse.com, lersek@redhat.com 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" 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" 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 > > > > > --- > > > > > 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