| Message ID | 20131218172025.GD21916@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 fails to > > > > > > > resume from suspend mode (S3). > > > > > > > > > > > > > > Signed-off-by: Gal Hammer <ghammer@redhat.com> > > > > > > > --- > > > > > > > hw/acpi/piix4.c | 1 - > > > > > > > 1 file changed, 1 deletion(-) > > > > > > > > > > > > > > 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] = 0; > > > > > > > > > > > > > > pci_conf[0x40] = 0x01; /* PM io base read only bit */ > > > > > > > - pci_conf[0x80] = 0; > > > > > > > > > > > > > > if (s->kvm_enabled) { > > > > > > > /* Mark SMM as already inited (until KVM supports SMM). */ > > > > > > > > > > > > Note this is not the APIC base address, that one is 80h on the ISA > > > > > > bridge (function 0). You're changing the behavior for 80h on the power > > > > > > management function, which is function 3. The register is "PMBA—POWER > > > > > > MANAGEMENT BASE ADDRESS" and it is indeed initialized by SeaBIOS in > > > > > > piix4_pm_setup (src/fw/pciinit.c). > > > > > > > > > > > > Michael, perhaps a part of pci_setup (same file) should run on S3 resume? > > > > > > > > > > > > Paolo > > > > > > > > > > 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 problem. > > > > 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'] > > > > > > > > Any thoughts how to get around this? > > > > Thanks, > > > > Marcel > > > > > > 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. > > > > Thanks, > > Marcel > > 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. > > 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 > > Seems much easier and more robust than building up > the list of devices in memory. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > 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 slave); > 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[] = { > PCI_DEVICE_END, > }; > > -static void pci_bios_init_device(struct pci_device *pci) > +void pci_bios_init_device(struct pci_device *pci) > { > u16 bdf = pci->bdf; > dprintf(1, "PCI: init bdf=%02x:%02x.%x id=%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; > } > > +void > +pci_probe_device(int bdf, struct pci_device *dev) > +{ > + dev->bdf = bdf; > + u32 vendev = pci_config_readl(bdf, PCI_VENDOR_ID); > + dev->vendor = vendev & 0xffff; > + dev->device = vendev >> 16; > + u32 classrev = pci_config_readl(bdf, PCI_CLASS_REVISION); > + dev->class = classrev >> 16; > + dev->prog_if = classrev >> 8; > + dev->revision = classrev & 0xff; > + dev->header_type = pci_config_readb(bdf, PCI_HEADER_TYPE); > + u8 v = dev->header_type & 0x7f; > + if (v == PCI_HEADER_TYPE_BRIDGE || v == PCI_HEADER_TYPE_CARDBUS) { > + u8 secbus = pci_config_readb(bdf, PCI_SECONDARY_BUS); > + dev->secondary_bus = secbus; > + } > +} > // Find all PCI devices and populate PCIDevices linked list. > void > pci_probe_devices(void) > @@ -145,21 +163,12 @@ pci_probe_devices(void) > } > > // Populate pci_device info. > - dev->bdf = bdf; > + pci_probe_device(bdf, dev); > dev->parent = parent; > dev->rootbus = rootbus; > - u32 vendev = pci_config_readl(bdf, PCI_VENDOR_ID); > - dev->vendor = vendev & 0xffff; > - dev->device = vendev >> 16; > - u32 classrev = pci_config_readl(bdf, PCI_CLASS_REVISION); > - dev->class = classrev >> 16; > - dev->prog_if = classrev >> 8; > - dev->revision = classrev & 0xff; > - dev->header_type = pci_config_readb(bdf, PCI_HEADER_TYPE); > u8 v = dev->header_type & 0x7f; > if (v == PCI_HEADER_TYPE_BRIDGE || v == PCI_HEADER_TYPE_CARDBUS) { > - u8 secbus = pci_config_readb(bdf, PCI_SECONDARY_BUS); > - dev->secondary_bus = secbus; > + u8 secbus = dev->secondary_bus; > if (secbus > bus && !busdevs[secbus]) > busdevs[secbus] = 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(); > > + int bdf; > + foreachbdf(bdf, 0) { > + struct pci_device pci; > + > + pci_probe_device(bdf, &pci); > + pci_bios_init_device(&pci); > + } > + > s3_resume_vga(); > > make_bios_readonly();
On Thu, 2013-12-19 at 11:37 +0200, Marcel Apfelbaum wrote: > 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 fails to > > > > > > > > resume from suspend mode (S3). > > > > > > > > > > > > > > > > Signed-off-by: Gal Hammer <ghammer@redhat.com> > > > > > > > > --- > > > > > > > > hw/acpi/piix4.c | 1 - > > > > > > > > 1 file changed, 1 deletion(-) > > > > > > > > > > > > > > > > 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] = 0; > > > > > > > > > > > > > > > > pci_conf[0x40] = 0x01; /* PM io base read only bit */ > > > > > > > > - pci_conf[0x80] = 0; > > > > > > > > > > > > > > > > if (s->kvm_enabled) { > > > > > > > > /* Mark SMM as already inited (until KVM supports SMM). */ > > > > > > > > > > > > > > Note this is not the APIC base address, that one is 80h on the ISA > > > > > > > bridge (function 0). You're changing the behavior for 80h on the power > > > > > > > management function, which is function 3. The register is "PMBA—POWER > > > > > > > MANAGEMENT BASE ADDRESS" and it is indeed initialized by SeaBIOS in > > > > > > > piix4_pm_setup (src/fw/pciinit.c). > > > > > > > > > > > > > > Michael, perhaps a part of pci_setup (same file) should run on S3 resume? > > > > > > > > > > > > > > Paolo > > > > > > > > > > > > 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 problem. > > > > > 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'] > > > > > > > > > > Any thoughts how to get around this? > > > > > Thanks, > > > > > Marcel > > > > > > > > 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. > > > > > > Thanks, > > > Marcel > > > > 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. > > > > Basically something like the below (warning: completely untested, > > sorry). > I tested and it works fine for both windows and linux guests! Actually it doesn't work :( I was testing a "working hack", not the master branch. I still think it is the right direction. Thanks, Marcel > 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 > > > > > Seems much easier and more robust than building up > > the list of devices in memory. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > 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 slave); > > 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[] = { > > PCI_DEVICE_END, > > }; > > > > -static void pci_bios_init_device(struct pci_device *pci) > > +void pci_bios_init_device(struct pci_device *pci) > > { > > u16 bdf = pci->bdf; > > dprintf(1, "PCI: init bdf=%02x:%02x.%x id=%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; > > } > > > > +void > > +pci_probe_device(int bdf, struct pci_device *dev) > > +{ > > + dev->bdf = bdf; > > + u32 vendev = pci_config_readl(bdf, PCI_VENDOR_ID); > > + dev->vendor = vendev & 0xffff; > > + dev->device = vendev >> 16; > > + u32 classrev = pci_config_readl(bdf, PCI_CLASS_REVISION); > > + dev->class = classrev >> 16; > > + dev->prog_if = classrev >> 8; > > + dev->revision = classrev & 0xff; > > + dev->header_type = pci_config_readb(bdf, PCI_HEADER_TYPE); > > + u8 v = dev->header_type & 0x7f; > > + if (v == PCI_HEADER_TYPE_BRIDGE || v == PCI_HEADER_TYPE_CARDBUS) { > > + u8 secbus = pci_config_readb(bdf, PCI_SECONDARY_BUS); > > + dev->secondary_bus = secbus; > > + } > > +} > > // Find all PCI devices and populate PCIDevices linked list. > > void > > pci_probe_devices(void) > > @@ -145,21 +163,12 @@ pci_probe_devices(void) > > } > > > > // Populate pci_device info. > > - dev->bdf = bdf; > > + pci_probe_device(bdf, dev); > > dev->parent = parent; > > dev->rootbus = rootbus; > > - u32 vendev = pci_config_readl(bdf, PCI_VENDOR_ID); > > - dev->vendor = vendev & 0xffff; > > - dev->device = vendev >> 16; > > - u32 classrev = pci_config_readl(bdf, PCI_CLASS_REVISION); > > - dev->class = classrev >> 16; > > - dev->prog_if = classrev >> 8; > > - dev->revision = classrev & 0xff; > > - dev->header_type = pci_config_readb(bdf, PCI_HEADER_TYPE); > > u8 v = dev->header_type & 0x7f; > > if (v == PCI_HEADER_TYPE_BRIDGE || v == PCI_HEADER_TYPE_CARDBUS) { > > - u8 secbus = pci_config_readb(bdf, PCI_SECONDARY_BUS); > > - dev->secondary_bus = secbus; > > + u8 secbus = dev->secondary_bus; > > if (secbus > bus && !busdevs[secbus]) > > busdevs[secbus] = 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(); > > > > + int bdf; > > + foreachbdf(bdf, 0) { > > + struct pci_device pci; > > + > > + pci_probe_device(bdf, &pci); > > + pci_bios_init_device(&pci); > > + } > > + > > s3_resume_vga(); > > > > make_bios_readonly(); > > > >
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 slave); 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[] = { PCI_DEVICE_END, }; -static void pci_bios_init_device(struct pci_device *pci) +void pci_bios_init_device(struct pci_device *pci) { u16 bdf = pci->bdf; dprintf(1, "PCI: init bdf=%02x:%02x.%x id=%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; } +void +pci_probe_device(int bdf, struct pci_device *dev) +{ + dev->bdf = bdf; + u32 vendev = pci_config_readl(bdf, PCI_VENDOR_ID); + dev->vendor = vendev & 0xffff; + dev->device = vendev >> 16; + u32 classrev = pci_config_readl(bdf, PCI_CLASS_REVISION); + dev->class = classrev >> 16; + dev->prog_if = classrev >> 8; + dev->revision = classrev & 0xff; + dev->header_type = pci_config_readb(bdf, PCI_HEADER_TYPE); + u8 v = dev->header_type & 0x7f; + if (v == PCI_HEADER_TYPE_BRIDGE || v == PCI_HEADER_TYPE_CARDBUS) { + u8 secbus = pci_config_readb(bdf, PCI_SECONDARY_BUS); + dev->secondary_bus = secbus; + } +} // Find all PCI devices and populate PCIDevices linked list. void pci_probe_devices(void) @@ -145,21 +163,12 @@ pci_probe_devices(void) } // Populate pci_device info. - dev->bdf = bdf; + pci_probe_device(bdf, dev); dev->parent = parent; dev->rootbus = rootbus; - u32 vendev = pci_config_readl(bdf, PCI_VENDOR_ID); - dev->vendor = vendev & 0xffff; - dev->device = vendev >> 16; - u32 classrev = pci_config_readl(bdf, PCI_CLASS_REVISION); - dev->class = classrev >> 16; - dev->prog_if = classrev >> 8; - dev->revision = classrev & 0xff; - dev->header_type = pci_config_readb(bdf, PCI_HEADER_TYPE); u8 v = dev->header_type & 0x7f; if (v == PCI_HEADER_TYPE_BRIDGE || v == PCI_HEADER_TYPE_CARDBUS) { - u8 secbus = pci_config_readb(bdf, PCI_SECONDARY_BUS); - dev->secondary_bus = secbus; + u8 secbus = dev->secondary_bus; if (secbus > bus && !busdevs[secbus]) busdevs[secbus] = 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(); + int bdf; + foreachbdf(bdf, 0) { + struct pci_device pci; + + pci_probe_device(bdf, &pci); + pci_bios_init_device(&pci); + } + s3_resume_vga(); make_bios_readonly();