From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47770) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vta3T-0006WP-PS for qemu-devel@nongnu.org; Thu, 19 Dec 2013 04:38:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vta3N-0000lE-OE for qemu-devel@nongnu.org; Thu, 19 Dec 2013 04:38:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:23403) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vta3N-0000l3-G0 for qemu-devel@nongnu.org; Thu, 19 Dec 2013 04:38:01 -0500 Message-ID: <1387445872.28892.5.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Thu, 19 Dec 2013 11:37:52 +0200 In-Reply-To: <20131218172025.GD21916@redhat.com> References: <1386753670-11238-1-git-send-email-ghammer@redhat.com> <52B1AFC3.1050809@redhat.com> <20131218152227.GA21186@redhat.com> <1387384032.2585.7.camel@localhost.localdomain> <20131218163306.GB21916@redhat.com> <1387385364.2585.11.camel@localhost.localdomain> <20131218172025.GD21916@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] piix: do not reset APIC base address (0x80) on piix4_reset. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Gal Hammer , Paolo Bonzini , "seabios@seabios.org" , qemu-devel@nongnu.org On Wed, 2013-12-18 at 19:20 +0200, Michael S. Tsirkin wrote: > On Wed, Dec 18, 2013 at 06:49:24PM +0200, Marcel Apfelbaum wrote: > > On Wed, 2013-12-18 at 18:33 +0200, Michael S. Tsirkin wrote: > > > On Wed, Dec 18, 2013 at 06:27:12PM +0200, Marcel Apfelbaum wrote: > > > > On Wed, 2013-12-18 at 17:22 +0200, Michael S. Tsirkin wrote: > > > > > On Wed, Dec 18, 2013 at 03:22:59PM +0100, Paolo Bonzini wrote: > > > > > > Il 11/12/2013 10:21, Gal Hammer ha scritto: > > > > > > > Fix a bug that was introduced in commit c046e8c4. QEMU fail= s to > > > > > > > resume from suspend mode (S3). > > > > > > >=20 > > > > > > > Signed-off-by: Gal Hammer > > > > > > > --- > > > > > > > hw/acpi/piix4.c | 1 - > > > > > > > 1 file changed, 1 deletion(-) > > > > > > >=20 > > > > > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > > > > > > > index 93849c8..5c736a4 100644 > > > > > > > --- a/hw/acpi/piix4.c > > > > > > > +++ b/hw/acpi/piix4.c > > > > > > > @@ -376,7 +376,6 @@ static void piix4_reset(void *opaque) > > > > > > > pci_conf[0x5b] =3D 0; > > > > > > > =20 > > > > > > > pci_conf[0x40] =3D 0x01; /* PM io base read only bit *= / > > > > > > > - pci_conf[0x80] =3D 0; > > > > > > > =20 > > > > > > > if (s->kvm_enabled) { > > > > > > > /* Mark SMM as already inited (until KVM supports = SMM). */ > > > > > >=20 > > > > > > Note this is not the APIC base address, that one is 80h on th= e ISA > > > > > > bridge (function 0). You're changing the behavior for 80h on= the power > > > > > > management function, which is function 3. The register is "P= MBA=E2=80=94POWER > > > > > > MANAGEMENT BASE ADDRESS" and it is indeed initialized by SeaB= IOS in > > > > > > piix4_pm_setup (src/fw/pciinit.c). > > > > > >=20 > > > > > > Michael, perhaps a part of pci_setup (same file) should run o= n S3 resume? > > > > > >=20 > > > > > > Paolo > > > > >=20 > > > > > Seems reasonable: either seabios or guest OS must do it, and > > > > > guest does not seem to. > > > > I was looking into this today, but it seems that we have a proble= m. > > > > We cannot run pci_setup() in init section: > > > > .data.varinit.seabios/src/hw/pci.h.66 is VARVERIFY32INIT but used= from ['.text.runtime.seabios/src/resume.c.150', '.text.pci_setup'] > > > >=20 > > > > Any thoughts how to get around this? > > > > Thanks, > > > > Marcel > > >=20 > > > We defintely don't want to do full pci enumeration. > > > Just pci_bios_init_platform or even less. > > The problem still remains, we have to use pci_bios_init_device that > > in turn uses the PCIDevices list. > >=20 > > Thanks, > > Marcel >=20 > It does but it does not have to. We can use a chunk out of > pci_probe_devices to initialize struct pci_device on stack. >=20 > Basically something like the below (warning: completely untested, > sorry). I tested and it works fine for both windows and linux guests! Also I agree this is the right thing to do after s3. Do you want to submit the patch? (or I can do it, no problem) Thanks, Marcel >=20 > Seems much easier and more robust than building up > the list of devices in memory. >=20 > Signed-off-by: Michael S. Tsirkin >=20 >=20 > diff --git a/src/hw/pci.h b/src/hw/pci.h > index 9c7351d..a64f7c5 100644 > --- a/src/hw/pci.h > +++ b/src/hw/pci.h > @@ -66,6 +66,7 @@ extern u64 pcimem64_start, pcimem64_end; > extern struct hlist_head PCIDevices; > extern int MaxPCIBus; > int pci_probe_host(void); > +void pci_probe_device(int bdf, struct pci_device *dev); > void pci_probe_devices(void); > static inline u32 pci_classprog(struct pci_device *pci) { > return (pci->class << 8) | pci->prog_if; > diff --git a/src/util.h b/src/util.h > index e6a6cb5..a8c71a8 100644 > --- a/src/util.h > +++ b/src/util.h > @@ -28,6 +28,7 @@ void boot_add_cbfs(void *data, const char *desc, int = prio); > void interactive_bootmenu(void); > void bcv_prepboot(void); > struct pci_device; > +void pci_bios_init_device(struct pci_device *pci); > int bootprio_find_pci_device(struct pci_device *pci); > int bootprio_find_scsi_device(struct pci_device *pci, int target, int = lun); > int bootprio_find_ata_device(struct pci_device *pci, int chanid, int s= lave); > diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c > index 34279a4..a35b58d 100644 > --- a/src/fw/pciinit.c > +++ b/src/fw/pciinit.c > @@ -293,7 +293,7 @@ static const struct pci_device_id pci_device_tbl[] = =3D { > PCI_DEVICE_END, > }; > =20 > -static void pci_bios_init_device(struct pci_device *pci) > +void pci_bios_init_device(struct pci_device *pci) > { > u16 bdf =3D pci->bdf; > dprintf(1, "PCI: init bdf=3D%02x:%02x.%x id=3D%04x:%04x\n" > diff --git a/src/hw/pci.c b/src/hw/pci.c > index 6c9aa81..d22804f 100644 > --- a/src/hw/pci.c > +++ b/src/hw/pci.c > @@ -105,6 +105,24 @@ pci_probe_host(void) > return 0; > } > =20 > +void > +pci_probe_device(int bdf, struct pci_device *dev) > +{ > + dev->bdf =3D bdf; > + u32 vendev =3D pci_config_readl(bdf, PCI_VENDOR_ID); > + dev->vendor =3D vendev & 0xffff; > + dev->device =3D vendev >> 16; > + u32 classrev =3D pci_config_readl(bdf, PCI_CLASS_REVISION); > + dev->class =3D classrev >> 16; > + dev->prog_if =3D classrev >> 8; > + dev->revision =3D classrev & 0xff; > + dev->header_type =3D pci_config_readb(bdf, PCI_HEADER_TYPE); > + u8 v =3D dev->header_type & 0x7f; > + if (v =3D=3D PCI_HEADER_TYPE_BRIDGE || v =3D=3D PCI_HEADER_TYPE_CA= RDBUS) { > + u8 secbus =3D pci_config_readb(bdf, PCI_SECONDARY_BUS); > + dev->secondary_bus =3D secbus; > + } > +} > // Find all PCI devices and populate PCIDevices linked list. > void > pci_probe_devices(void) > @@ -145,21 +163,12 @@ pci_probe_devices(void) > } > =20 > // Populate pci_device info. > - dev->bdf =3D bdf; > + pci_probe_device(bdf, dev); > dev->parent =3D parent; > dev->rootbus =3D rootbus; > - u32 vendev =3D pci_config_readl(bdf, PCI_VENDOR_ID); > - dev->vendor =3D vendev & 0xffff; > - dev->device =3D vendev >> 16; > - u32 classrev =3D pci_config_readl(bdf, PCI_CLASS_REVISION)= ; > - dev->class =3D classrev >> 16; > - dev->prog_if =3D classrev >> 8; > - dev->revision =3D classrev & 0xff; > - dev->header_type =3D pci_config_readb(bdf, PCI_HEADER_TYPE= ); > u8 v =3D dev->header_type & 0x7f; > if (v =3D=3D PCI_HEADER_TYPE_BRIDGE || v =3D=3D PCI_HEADER= _TYPE_CARDBUS) { > - u8 secbus =3D pci_config_readb(bdf, PCI_SECONDARY_BUS)= ; > - dev->secondary_bus =3D secbus; > + u8 secbus =3D dev->secondary_bus; > if (secbus > bus && !busdevs[secbus]) > busdevs[secbus] =3D dev; > if (secbus > MaxPCIBus) > diff --git a/src/resume.c b/src/resume.c > index fc2fee9..f6c8b3b 100644 > --- a/src/resume.c > +++ b/src/resume.c > @@ -101,6 +101,14 @@ s3_resume(void) > pic_setup(); > smm_setup(); > =20 > + int bdf; > + foreachbdf(bdf, 0) { > + struct pci_device pci; > + > + pci_probe_device(bdf, &pci); > + pci_bios_init_device(&pci); > + } > + > s3_resume_vga(); > =20 > make_bios_readonly();