On resume, the OS queries the power management event that caused it. In order to complete this task, it executes some reads to the piix pm io space. This all happens before the OS has a chance to restore the PCI config space for devices, so it is bios's responsibility to make sure the pm IO space is configured correctly. (During suspend, the piix pm configuration space is lost).
Note: For 'ordinary' pci devices the config space is saved by the OS on sleep and restored on resume.
Signed-off-by: Marcel Apfelbaum marcel.a@redhat.com --- This patch is based on Michael S. Tsirkin's patch: [SeaBIOS] [PATCH] seabios: call pci_init_device on resume If Michael agrees with it, I'll add him to signed-off section.
Notes: - Without this patch the OS gets stuck because it tries repeatedly to read/write to pm io space, but the memory region is not enabled, so -1 is returned. - After resume the OS does not actually use the pm base address configured by the bios to get the IO ports, but uses the value from the ACPI FADT table actually. However, as a side effect of the configuration, the pm-io space is enabled by Qemu and the OS can continue the the boot sequence. - Bioses used for hardware like coreboot have the same init sequence for piix, see enable_pm from src/southbridge/intel/i82371eb/early_pm.c.
src/fw/pciinit.c | 22 ++++++++++++++++++++-- src/hw/pci.c | 31 ++++++++++++++++++++----------- src/hw/pci.h | 1 + src/resume.c | 8 ++++++++ src/util.h | 1 + 5 files changed, 50 insertions(+), 13 deletions(-)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index a24b8ff..252328f 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -230,8 +230,7 @@ static void apple_macio_setup(struct pci_device *pci, void *arg) pci_set_io_region_addr(pci, 0, 0x80800000, 0); }
-/* PIIX4 Power Management device (for ACPI) */ -static void piix4_pm_setup(struct pci_device *pci, void *arg) +static void piix4_pm_config_setup(struct pci_device *pci, void *arg) { u16 bdf = pci->bdf; // acpi sci is hardwired to 9 @@ -241,6 +240,12 @@ static void piix4_pm_setup(struct pci_device *pci, void *arg) pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */ pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1); pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */ +} + +/* PIIX4 Power Management device (for ACPI) */ +static void piix4_pm_setup(struct pci_device *pci, void *arg) +{ + piix4_pm_config_setup(pci, arg);
acpi_pm1a_cnt = PORT_ACPI_PM_BASE + 0x04; pmtimer_setup(PORT_ACPI_PM_BASE + 0x08); @@ -295,6 +300,19 @@ static const struct pci_device_id pci_device_tbl[] = { PCI_DEVICE_END, };
+static const struct pci_device_id pci_device_resume_tbl[] = { + /* PIIX4 Power Management device (for ACPI) */ + PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3, + piix4_pm_config_setup), + + PCI_DEVICE_END, +}; + +void pci_bios_resume_device(struct pci_device *pci) +{ + pci_init_device(pci_device_resume_tbl, pci, NULL); +} + static void pci_bios_init_device(struct pci_device *pci) { u16 bdf = pci->bdf; diff --git a/src/hw/pci.c b/src/hw/pci.c index 6c9aa81..c2873c3 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/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/resume.c b/src/resume.c index d69429c..9626f9a 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) { + /* Create new pci_device struct and add to list. */ + struct pci_device pci; + pci_probe_device(bdf, &pci); + pci_bios_resume_device(&pci); + } + s3_resume_vga();
make_bios_readonly(); diff --git a/src/util.h b/src/util.h index 1b7d525..a4d4ee3 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_resume_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);
On Thu, 2014-01-02 at 19:00 +0200, Marcel Apfelbaum wrote:
On resume, the OS queries the power management event that caused it. In order to complete this task, it executes some reads to the piix pm io space. This all happens before the OS has a chance to restore the PCI config space for devices, so it is bios's responsibility to make sure the pm IO space is configured correctly. (During suspend, the piix pm configuration space is lost).
Note: For 'ordinary' pci devices the config space is saved by the OS on sleep and restored on resume.
Signed-off-by: Marcel Apfelbaum marcel.a@redhat.com
Ping
Thanks, Marcel
This patch is based on Michael S. Tsirkin's patch: [SeaBIOS] [PATCH] seabios: call pci_init_device on resume If Michael agrees with it, I'll add him to signed-off section.
Notes:
- Without this patch the OS gets stuck because it tries repeatedly to read/write to pm io space, but the memory region is not enabled, so -1 is returned.
- After resume the OS does not actually use the pm base address configured by the bios to get the IO ports, but uses the value from the ACPI FADT table actually. However, as a side effect of the configuration, the pm-io space is enabled by Qemu and the OS can continue the the boot sequence.
- Bioses used for hardware like coreboot have the same init sequence for piix, see enable_pm from src/southbridge/intel/i82371eb/early_pm.c.
src/fw/pciinit.c | 22 ++++++++++++++++++++-- src/hw/pci.c | 31 ++++++++++++++++++++----------- src/hw/pci.h | 1 + src/resume.c | 8 ++++++++ src/util.h | 1 + 5 files changed, 50 insertions(+), 13 deletions(-)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index a24b8ff..252328f 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -230,8 +230,7 @@ static void apple_macio_setup(struct pci_device *pci, void *arg) pci_set_io_region_addr(pci, 0, 0x80800000, 0); }
-/* PIIX4 Power Management device (for ACPI) */ -static void piix4_pm_setup(struct pci_device *pci, void *arg) +static void piix4_pm_config_setup(struct pci_device *pci, void *arg) { u16 bdf = pci->bdf; // acpi sci is hardwired to 9 @@ -241,6 +240,12 @@ static void piix4_pm_setup(struct pci_device *pci, void *arg) pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */ pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1); pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */ +}
+/* PIIX4 Power Management device (for ACPI) */ +static void piix4_pm_setup(struct pci_device *pci, void *arg) +{
piix4_pm_config_setup(pci, arg);
acpi_pm1a_cnt = PORT_ACPI_PM_BASE + 0x04; pmtimer_setup(PORT_ACPI_PM_BASE + 0x08);
@@ -295,6 +300,19 @@ static const struct pci_device_id pci_device_tbl[] = { PCI_DEVICE_END, };
+static const struct pci_device_id pci_device_resume_tbl[] = {
- /* PIIX4 Power Management device (for ACPI) */
- PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3,
piix4_pm_config_setup),
- PCI_DEVICE_END,
+};
+void pci_bios_resume_device(struct pci_device *pci) +{
- pci_init_device(pci_device_resume_tbl, pci, NULL);
+}
static void pci_bios_init_device(struct pci_device *pci) { u16 bdf = pci->bdf; diff --git a/src/hw/pci.c b/src/hw/pci.c index 6c9aa81..c2873c3 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/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/resume.c b/src/resume.c index d69429c..9626f9a 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) {
/* Create new pci_device struct and add to list. */
struct pci_device pci;
pci_probe_device(bdf, &pci);
pci_bios_resume_device(&pci);
}
s3_resume_vga();
make_bios_readonly();
diff --git a/src/util.h b/src/util.h index 1b7d525..a4d4ee3 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_resume_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);
On Do, 2014-01-02 at 19:00 +0200, Marcel Apfelbaum wrote:
On resume, the OS queries the power management event that caused it. In order to complete this task, it executes some reads to the piix pm io space. This all happens before the OS has a chance to restore the PCI config space for devices, so it is bios's responsibility to make sure the pm IO space is configured correctly. (During suspend, the piix pm configuration space is lost).
Note: For 'ordinary' pci devices the config space is saved by the OS on sleep and restored on resume.
Signed-off-by: Marcel Apfelbaum marcel.a@redhat.com
Patch looks good to me.
cheers, Gerd
On Thu, Jan 02, 2014 at 07:00:45PM +0200, Marcel Apfelbaum wrote:
On resume, the OS queries the power management event that caused it. In order to complete this task, it executes some reads to the piix pm io space. This all happens before the OS has a chance to restore the PCI config space for devices, so it is bios's responsibility to make sure the pm IO space is configured correctly. (During suspend, the piix pm configuration space is lost).
Note: For 'ordinary' pci devices the config space is saved by the OS on sleep and restored on resume.
Signed-off-by: Marcel Apfelbaum marcel.a@redhat.com
Acked-by: Michael S. Tsirkin mst@redhat.com
This patch is based on Michael S. Tsirkin's patch: [SeaBIOS] [PATCH] seabios: call pci_init_device on resume If Michael agrees with it, I'll add him to signed-off section.
you can do this too.
Notes:
- Without this patch the OS gets stuck because it tries repeatedly to read/write to pm io space, but the memory region is not enabled, so -1 is returned.
- After resume the OS does not actually use the pm base address configured by the bios to get the IO ports, but uses the value from the ACPI FADT table actually. However, as a side effect of the configuration, the pm-io space is enabled by Qemu and the OS can continue the the boot sequence.
- Bioses used for hardware like coreboot have the same init sequence for piix, see enable_pm from src/southbridge/intel/i82371eb/early_pm.c.
src/fw/pciinit.c | 22 ++++++++++++++++++++-- src/hw/pci.c | 31 ++++++++++++++++++++----------- src/hw/pci.h | 1 + src/resume.c | 8 ++++++++ src/util.h | 1 + 5 files changed, 50 insertions(+), 13 deletions(-)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index a24b8ff..252328f 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -230,8 +230,7 @@ static void apple_macio_setup(struct pci_device *pci, void *arg) pci_set_io_region_addr(pci, 0, 0x80800000, 0); }
-/* PIIX4 Power Management device (for ACPI) */ -static void piix4_pm_setup(struct pci_device *pci, void *arg) +static void piix4_pm_config_setup(struct pci_device *pci, void *arg) { u16 bdf = pci->bdf; // acpi sci is hardwired to 9 @@ -241,6 +240,12 @@ static void piix4_pm_setup(struct pci_device *pci, void *arg) pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */ pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1); pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */ +}
+/* PIIX4 Power Management device (for ACPI) */ +static void piix4_pm_setup(struct pci_device *pci, void *arg) +{
piix4_pm_config_setup(pci, arg);
acpi_pm1a_cnt = PORT_ACPI_PM_BASE + 0x04; pmtimer_setup(PORT_ACPI_PM_BASE + 0x08);
@@ -295,6 +300,19 @@ static const struct pci_device_id pci_device_tbl[] = { PCI_DEVICE_END, };
+static const struct pci_device_id pci_device_resume_tbl[] = {
- /* PIIX4 Power Management device (for ACPI) */
- PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3,
piix4_pm_config_setup),
Any idea what needs to be done for q35?
- PCI_DEVICE_END,
+};
+void pci_bios_resume_device(struct pci_device *pci) +{
- pci_init_device(pci_device_resume_tbl, pci, NULL);
+}
static void pci_bios_init_device(struct pci_device *pci) { u16 bdf = pci->bdf; diff --git a/src/hw/pci.c b/src/hw/pci.c index 6c9aa81..c2873c3 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/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/resume.c b/src/resume.c index d69429c..9626f9a 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) {
/* Create new pci_device struct and add to list. */
struct pci_device pci;
pci_probe_device(bdf, &pci);
pci_bios_resume_device(&pci);
}
s3_resume_vga();
make_bios_readonly();
diff --git a/src/util.h b/src/util.h index 1b7d525..a4d4ee3 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_resume_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); -- 1.8.3.1
On Mon, 2014-01-13 at 15:17 +0200, Michael S. Tsirkin wrote:
On Thu, Jan 02, 2014 at 07:00:45PM +0200, Marcel Apfelbaum wrote:
On resume, the OS queries the power management event that caused it. In order to complete this task, it executes some reads to the piix pm io space. This all happens before the OS has a chance to restore the PCI config space for devices, so it is bios's responsibility to make sure the pm IO space is configured correctly. (During suspend, the piix pm configuration space is lost).
Note: For 'ordinary' pci devices the config space is saved by the OS on sleep and restored on resume.
Signed-off-by: Marcel Apfelbaum marcel.a@redhat.com
Acked-by: Michael S. Tsirkin mst@redhat.com
This patch is based on Michael S. Tsirkin's patch: [SeaBIOS] [PATCH] seabios: call pci_init_device on resume If Michael agrees with it, I'll add him to signed-off section.
you can do this too.
Kevin, can you add Signed-off-by: Michael S. Tsirkin mst@redhat.com or do you prefer me to send a V2?
Thanks, Marcel
Notes:
- Without this patch the OS gets stuck because it tries repeatedly to read/write to pm io space, but the memory region is not enabled, so -1 is returned.
- After resume the OS does not actually use the pm base address configured by the bios to get the IO ports, but uses the value from the ACPI FADT table actually. However, as a side effect of the configuration, the pm-io space is enabled by Qemu and the OS can continue the the boot sequence.
- Bioses used for hardware like coreboot have the same init sequence for piix, see enable_pm from src/southbridge/intel/i82371eb/early_pm.c.
src/fw/pciinit.c | 22 ++++++++++++++++++++-- src/hw/pci.c | 31 ++++++++++++++++++++----------- src/hw/pci.h | 1 + src/resume.c | 8 ++++++++ src/util.h | 1 + 5 files changed, 50 insertions(+), 13 deletions(-)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index a24b8ff..252328f 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -230,8 +230,7 @@ static void apple_macio_setup(struct pci_device *pci, void *arg) pci_set_io_region_addr(pci, 0, 0x80800000, 0); }
-/* PIIX4 Power Management device (for ACPI) */ -static void piix4_pm_setup(struct pci_device *pci, void *arg) +static void piix4_pm_config_setup(struct pci_device *pci, void *arg) { u16 bdf = pci->bdf; // acpi sci is hardwired to 9 @@ -241,6 +240,12 @@ static void piix4_pm_setup(struct pci_device *pci, void *arg) pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */ pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1); pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */ +}
+/* PIIX4 Power Management device (for ACPI) */ +static void piix4_pm_setup(struct pci_device *pci, void *arg) +{
piix4_pm_config_setup(pci, arg);
acpi_pm1a_cnt = PORT_ACPI_PM_BASE + 0x04; pmtimer_setup(PORT_ACPI_PM_BASE + 0x08);
@@ -295,6 +300,19 @@ static const struct pci_device_id pci_device_tbl[] = { PCI_DEVICE_END, };
+static const struct pci_device_id pci_device_resume_tbl[] = {
- /* PIIX4 Power Management device (for ACPI) */
- PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3,
piix4_pm_config_setup),
Any idea what needs to be done for q35?
Probably the same logic for q35 pm device . I was going to look into it after I get OK for this patch. :) So now is the time for it, I'll start looking into it and come with something soon.
Thanks, Marcel
- PCI_DEVICE_END,
+};
+void pci_bios_resume_device(struct pci_device *pci) +{
- pci_init_device(pci_device_resume_tbl, pci, NULL);
+}
static void pci_bios_init_device(struct pci_device *pci) { u16 bdf = pci->bdf; diff --git a/src/hw/pci.c b/src/hw/pci.c index 6c9aa81..c2873c3 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/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/resume.c b/src/resume.c index d69429c..9626f9a 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) {
/* Create new pci_device struct and add to list. */
struct pci_device pci;
pci_probe_device(bdf, &pci);
pci_bios_resume_device(&pci);
}
s3_resume_vga();
make_bios_readonly();
diff --git a/src/util.h b/src/util.h index 1b7d525..a4d4ee3 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_resume_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); -- 1.8.3.1
On Mon, 2014-01-13 at 15:50 +0200, Marcel Apfelbaum wrote:
On Mon, 2014-01-13 at 15:17 +0200, Michael S. Tsirkin wrote:
On Thu, Jan 02, 2014 at 07:00:45PM +0200, Marcel Apfelbaum wrote:
On resume, the OS queries the power management event that caused it. In order to complete this task, it executes some reads to the piix pm io space. This all happens before the OS has a chance to restore the PCI config space for devices, so it is bios's responsibility to make sure the pm IO space is configured correctly. (During suspend, the piix pm configuration space is lost).
Note: For 'ordinary' pci devices the config space is saved by the OS on sleep and restored on resume.
Signed-off-by: Marcel Apfelbaum marcel.a@redhat.com
Acked-by: Michael S. Tsirkin mst@redhat.com
This patch is based on Michael S. Tsirkin's patch: [SeaBIOS] [PATCH] seabios: call pci_init_device on resume If Michael agrees with it, I'll add him to signed-off section.
you can do this too.
Kevin, can you add Signed-off-by: Michael S. Tsirkin mst@redhat.com or do you prefer me to send a V2?
Added Kevin to CC
Thanks, Marcel
Notes:
- Without this patch the OS gets stuck because it tries repeatedly to read/write to pm io space, but the memory region is not enabled, so -1 is returned.
- After resume the OS does not actually use the pm base address configured by the bios to get the IO ports, but uses the value from the ACPI FADT table actually. However, as a side effect of the configuration, the pm-io space is enabled by Qemu and the OS can continue the the boot sequence.
- Bioses used for hardware like coreboot have the same init sequence for piix, see enable_pm from src/southbridge/intel/i82371eb/early_pm.c.
src/fw/pciinit.c | 22 ++++++++++++++++++++-- src/hw/pci.c | 31 ++++++++++++++++++++----------- src/hw/pci.h | 1 + src/resume.c | 8 ++++++++ src/util.h | 1 + 5 files changed, 50 insertions(+), 13 deletions(-)
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index a24b8ff..252328f 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -230,8 +230,7 @@ static void apple_macio_setup(struct pci_device *pci, void *arg) pci_set_io_region_addr(pci, 0, 0x80800000, 0); }
-/* PIIX4 Power Management device (for ACPI) */ -static void piix4_pm_setup(struct pci_device *pci, void *arg) +static void piix4_pm_config_setup(struct pci_device *pci, void *arg) { u16 bdf = pci->bdf; // acpi sci is hardwired to 9 @@ -241,6 +240,12 @@ static void piix4_pm_setup(struct pci_device *pci, void *arg) pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */ pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1); pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */ +}
+/* PIIX4 Power Management device (for ACPI) */ +static void piix4_pm_setup(struct pci_device *pci, void *arg) +{
piix4_pm_config_setup(pci, arg);
acpi_pm1a_cnt = PORT_ACPI_PM_BASE + 0x04; pmtimer_setup(PORT_ACPI_PM_BASE + 0x08);
@@ -295,6 +300,19 @@ static const struct pci_device_id pci_device_tbl[] = { PCI_DEVICE_END, };
+static const struct pci_device_id pci_device_resume_tbl[] = {
- /* PIIX4 Power Management device (for ACPI) */
- PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3,
piix4_pm_config_setup),
Any idea what needs to be done for q35?
Probably the same logic for q35 pm device . I was going to look into it after I get OK for this patch. :) So now is the time for it, I'll start looking into it and come with something soon. Thanks, Marcel
- PCI_DEVICE_END,
+};
+void pci_bios_resume_device(struct pci_device *pci) +{
- pci_init_device(pci_device_resume_tbl, pci, NULL);
+}
static void pci_bios_init_device(struct pci_device *pci) { u16 bdf = pci->bdf; diff --git a/src/hw/pci.c b/src/hw/pci.c index 6c9aa81..c2873c3 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/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/resume.c b/src/resume.c index d69429c..9626f9a 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) {
/* Create new pci_device struct and add to list. */
struct pci_device pci;
pci_probe_device(bdf, &pci);
pci_bios_resume_device(&pci);
}
s3_resume_vga();
make_bios_readonly();
diff --git a/src/util.h b/src/util.h index 1b7d525..a4d4ee3 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_resume_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); -- 1.8.3.1
SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
On Mon, Jan 13, 2014 at 03:50:45PM +0200, Marcel Apfelbaum wrote:
On Mon, 2014-01-13 at 15:17 +0200, Michael S. Tsirkin wrote:
On Thu, Jan 02, 2014 at 07:00:45PM +0200, Marcel Apfelbaum wrote:
This patch is based on Michael S. Tsirkin's patch: [SeaBIOS] [PATCH] seabios: call pci_init_device on resume If Michael agrees with it, I'll add him to signed-off section.
you can do this too.
Kevin, can you add Signed-off-by: Michael S. Tsirkin mst@redhat.com or do you prefer me to send a V2?
Off topic, but is there an easy way to add a third-party signed-off-by signature to a patch with git?
-Kevin
On Mon, Jan 13, 2014 at 11:18:04AM -0500, Kevin O'Connor wrote:
On Mon, Jan 13, 2014 at 03:50:45PM +0200, Marcel Apfelbaum wrote:
On Mon, 2014-01-13 at 15:17 +0200, Michael S. Tsirkin wrote:
On Thu, Jan 02, 2014 at 07:00:45PM +0200, Marcel Apfelbaum wrote:
This patch is based on Michael S. Tsirkin's patch: [SeaBIOS] [PATCH] seabios: call pci_init_device on resume If Michael agrees with it, I'll add him to signed-off section.
you can do this too.
Kevin, can you add Signed-off-by: Michael S. Tsirkin mst@redhat.com or do you prefer me to send a V2?
Off topic, but is there an easy way to add a third-party signed-off-by signature to a patch with git?
-Kevin
The way I do this, is I have a script to add a stub commit with squash! subject prefix and the signature in the commit log. I later do git rebase --autosquash and it gets listed in the correct place automatically: all that remains is to tweak "# pick" to "squash" and drop the extra lines from log.
I keep meaning to automate the rebase bit or add an option to git, didn't do this yet.
On Thu, Jan 02, 2014 at 07:00:45PM +0200, Marcel Apfelbaum wrote:
On resume, the OS queries the power management event that caused it. In order to complete this task, it executes some reads to the piix pm io space. This all happens before the OS has a chance to restore the PCI config space for devices, so it is bios's responsibility to make sure the pm IO space is configured correctly. (During suspend, the piix pm configuration space is lost).
Note: For 'ordinary' pci devices the config space is saved by the OS on sleep and restored on resume.
Signed-off-by: Marcel Apfelbaum marcel.a@redhat.com
This patch is based on Michael S. Tsirkin's patch: [SeaBIOS] [PATCH] seabios: call pci_init_device on resume If Michael agrees with it, I'll add him to signed-off section.
Notes:
- Without this patch the OS gets stuck because it tries repeatedly to read/write to pm io space, but the memory region is not enabled, so -1 is returned.
- After resume the OS does not actually use the pm base address configured by the bios to get the IO ports, but uses the value from the ACPI FADT table actually. However, as a side effect of the configuration, the pm-io space is enabled by Qemu and the OS can continue the the boot sequence.
- Bioses used for hardware like coreboot have the same init sequence for piix, see enable_pm from src/southbridge/intel/i82371eb/early_pm.c.
Thanks. SeaBIOS isn't responsible for PCI setup on CSM/coreboot, so the patch must check for CONFIG_QEMU.
Also, I think we can simplify this a bit - how about the patch below (untested)?
-Kevin
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index a24b8ff..1c90059 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -230,10 +230,8 @@ static void apple_macio_setup(struct pci_device *pci, void *arg) pci_set_io_region_addr(pci, 0, 0x80800000, 0); }
-/* PIIX4 Power Management device (for ACPI) */ -static void piix4_pm_setup(struct pci_device *pci, void *arg) +static void piix4_pm_config_setup(u16 bdf) { - u16 bdf = pci->bdf; // acpi sci is hardwired to 9 pci_config_writeb(bdf, PCI_INTERRUPT_LINE, 9);
@@ -241,6 +239,15 @@ static void piix4_pm_setup(struct pci_device *pci, void *arg) pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */ pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1); pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */ +} + +static int PiixPMBDF = -1; + +/* PIIX4 Power Management device (for ACPI) */ +static void piix4_pm_setup(struct pci_device *pci, void *arg) +{ + PiixPMBDF = pci->bdf; + piix4_pm_config_setup(pci->bdf);
acpi_pm1a_cnt = PORT_ACPI_PM_BASE + 0x04; pmtimer_setup(PORT_ACPI_PM_BASE + 0x08); @@ -858,3 +865,12 @@ pci_setup(void)
pci_enable_default_vga(); } + +void +pci_resume(void) +{ + if (!CONFIG_QEMU) + return; + if (PiixPMBDF >= 0) + piix4_pm_config_setup(PiixPMBDF); +} diff --git a/src/resume.c b/src/resume.c index d69429c..0c0e60e 100644 --- a/src/resume.c +++ b/src/resume.c @@ -100,6 +100,7 @@ s3_resume(void)
pic_setup(); smm_setup(); + pci_resume();
s3_resume_vga();
diff --git a/src/util.h b/src/util.h index 1b7d525..b8acbfa 100644 --- a/src/util.h +++ b/src/util.h @@ -99,6 +99,7 @@ void mtrr_setup(void); // fw/pciinit.c extern const u8 pci_irqs[4]; void pci_setup(void); +void pci_resume(void);
// fw/pirtable.c extern struct pir_header *PirAddr;
On Mon, Jan 13, 2014 at 11:31:20AM -0500, Kevin O'Connor wrote:
On Thu, Jan 02, 2014 at 07:00:45PM +0200, Marcel Apfelbaum wrote:
On resume, the OS queries the power management event that caused it. In order to complete this task, it executes some reads to the piix pm io space. This all happens before the OS has a chance to restore the PCI config space for devices, so it is bios's responsibility to make sure the pm IO space is configured correctly. (During suspend, the piix pm configuration space is lost).
Note: For 'ordinary' pci devices the config space is saved by the OS on sleep and restored on resume.
Signed-off-by: Marcel Apfelbaum marcel.a@redhat.com
This patch is based on Michael S. Tsirkin's patch: [SeaBIOS] [PATCH] seabios: call pci_init_device on resume If Michael agrees with it, I'll add him to signed-off section.
Notes:
- Without this patch the OS gets stuck because it tries repeatedly to read/write to pm io space, but the memory region is not enabled, so -1 is returned.
- After resume the OS does not actually use the pm base address configured by the bios to get the IO ports, but uses the value from the ACPI FADT table actually. However, as a side effect of the configuration, the pm-io space is enabled by Qemu and the OS can continue the the boot sequence.
- Bioses used for hardware like coreboot have the same init sequence for piix, see enable_pm from src/southbridge/intel/i82371eb/early_pm.c.
Thanks. SeaBIOS isn't responsible for PCI setup on CSM/coreboot, so the patch must check for CONFIG_QEMU.
Also, I think we can simplify this a bit - how about the patch below (untested)?
-Kevin
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index a24b8ff..1c90059 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -230,10 +230,8 @@ static void apple_macio_setup(struct pci_device *pci, void *arg) pci_set_io_region_addr(pci, 0, 0x80800000, 0); }
-/* PIIX4 Power Management device (for ACPI) */ -static void piix4_pm_setup(struct pci_device *pci, void *arg) +static void piix4_pm_config_setup(u16 bdf) {
- u16 bdf = pci->bdf; // acpi sci is hardwired to 9 pci_config_writeb(bdf, PCI_INTERRUPT_LINE, 9);
@@ -241,6 +239,15 @@ static void piix4_pm_setup(struct pci_device *pci, void *arg) pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */ pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1); pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */ +}
+static int PiixPMBDF = -1;
+/* PIIX4 Power Management device (for ACPI) */ +static void piix4_pm_setup(struct pci_device *pci, void *arg) +{
PiixPMBDF = pci->bdf;
piix4_pm_config_setup(pci->bdf);
acpi_pm1a_cnt = PORT_ACPI_PM_BASE + 0x04; pmtimer_setup(PORT_ACPI_PM_BASE + 0x08);
@@ -858,3 +865,12 @@ pci_setup(void)
pci_enable_default_vga();
}
+void +pci_resume(void) +{
- if (!CONFIG_QEMU)
return;
- if (PiixPMBDF >= 0)
so this does nothing unless piix4_pm_setup run but if it did why do we need to resume anything?
piix4_pm_config_setup(PiixPMBDF);
+} diff --git a/src/resume.c b/src/resume.c index d69429c..0c0e60e 100644 --- a/src/resume.c +++ b/src/resume.c @@ -100,6 +100,7 @@ s3_resume(void)
pic_setup(); smm_setup();
pci_resume();
s3_resume_vga();
diff --git a/src/util.h b/src/util.h index 1b7d525..b8acbfa 100644 --- a/src/util.h +++ b/src/util.h @@ -99,6 +99,7 @@ void mtrr_setup(void); // fw/pciinit.c extern const u8 pci_irqs[4]; void pci_setup(void); +void pci_resume(void);
// fw/pirtable.c extern struct pir_header *PirAddr;
On Mon, Jan 13, 2014 at 06:36:56PM +0200, Michael S. Tsirkin wrote:
On Mon, Jan 13, 2014 at 11:31:20AM -0500, Kevin O'Connor wrote:
+void +pci_resume(void) +{
- if (!CONFIG_QEMU)
return;
- if (PiixPMBDF >= 0)
so this does nothing unless piix4_pm_setup run but if it did why do we need to resume anything?
PiixPMBDF is set during the SeaBIOS init phase. The proposed pci_resume() function is called during the resume phase. The goal of the proposed patch is to store the BDF during init so that it's available during resume without requiring a bus scan.
-Kevin
On Mon, Jan 13, 2014 at 11:45:40AM -0500, Kevin O'Connor wrote:
On Mon, Jan 13, 2014 at 06:36:56PM +0200, Michael S. Tsirkin wrote:
On Mon, Jan 13, 2014 at 11:31:20AM -0500, Kevin O'Connor wrote:
+void +pci_resume(void) +{
- if (!CONFIG_QEMU)
return;
- if (PiixPMBDF >= 0)
so this does nothing unless piix4_pm_setup run but if it did why do we need to resume anything?
PiixPMBDF is set during the SeaBIOS init phase. The proposed pci_resume() function is called during the resume phase. The goal of the proposed patch is to store the BDF during init so that it's available during resume without requiring a bus scan.
-Kevin
Confused. I thought pci_init_device is the one calling piix4_pm_setup.
What am I missing?
On Mon, Jan 13, 2014 at 06:55:50PM +0200, Michael S. Tsirkin wrote:
On Mon, Jan 13, 2014 at 11:45:40AM -0500, Kevin O'Connor wrote:
On Mon, Jan 13, 2014 at 06:36:56PM +0200, Michael S. Tsirkin wrote:
On Mon, Jan 13, 2014 at 11:31:20AM -0500, Kevin O'Connor wrote:
+void +pci_resume(void) +{
- if (!CONFIG_QEMU)
return;
- if (PiixPMBDF >= 0)
so this does nothing unless piix4_pm_setup run but if it did why do we need to resume anything?
PiixPMBDF is set during the SeaBIOS init phase. The proposed pci_resume() function is called during the resume phase. The goal of the proposed patch is to store the BDF during init so that it's available during resume without requiring a bus scan.
-Kevin
Confused. I thought pci_init_device is the one calling piix4_pm_setup.
It is. Call chain on init is:
handle_post()->...->qemu_platform_setup()->pci_setup()->pci_bios_init_devices()->pci_bios_init_device()->pci_init_device()->piix4_pm_setup()
Call chain on proposed resume is:
handle_resume()->handle_resume32()->s3_resume()->pci_resume().
What am I missing?
You have me confused now. Perhaps I've misunderstood your question or misunderstood the intent of your/Marcel's original patch.
-Kevin
On Mon, 2014-01-13 at 11:31 -0500, Kevin O'Connor wrote:
On Thu, Jan 02, 2014 at 07:00:45PM +0200, Marcel Apfelbaum wrote:
On resume, the OS queries the power management event that caused it. In order to complete this task, it executes some reads to the piix pm io space. This all happens before the OS has a chance to restore the PCI config space for devices, so it is bios's responsibility to make sure the pm IO space is configured correctly. (During suspend, the piix pm configuration space is lost).
Note: For 'ordinary' pci devices the config space is saved by the OS on sleep and restored on resume.
Signed-off-by: Marcel Apfelbaum marcel.a@redhat.com
This patch is based on Michael S. Tsirkin's patch: [SeaBIOS] [PATCH] seabios: call pci_init_device on resume If Michael agrees with it, I'll add him to signed-off section.
Notes:
- Without this patch the OS gets stuck because it tries repeatedly to read/write to pm io space, but the memory region is not enabled, so -1 is returned.
- After resume the OS does not actually use the pm base address configured by the bios to get the IO ports, but uses the value from the ACPI FADT table actually. However, as a side effect of the configuration, the pm-io space is enabled by Qemu and the OS can continue the the boot sequence.
- Bioses used for hardware like coreboot have the same init sequence for piix, see enable_pm from src/southbridge/intel/i82371eb/early_pm.c.
Thanks. SeaBIOS isn't responsible for PCI setup on CSM/coreboot, so the patch must check for CONFIG_QEMU.
Sure thing, Sorry I missed that, I'll add it to V2.
Also, I think we can simplify this a bit - how about the patch below (untested)?
Hi Kevin, I followed the patch and indeed it is smaller and it does the job, but - We also have a q35 chipset and we'll need a global variable also for it. - If we will have other devices that need special attention on resume we will be ready for them (low chance, but you never know). - Finally, the pci_resume will look a little strange and unclear with the new "if" statements (that means, this is the board and we saved the value)
What do you think? Thanks, Marcel
-Kevin
diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index a24b8ff..1c90059 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -230,10 +230,8 @@ static void apple_macio_setup(struct pci_device *pci, void *arg) pci_set_io_region_addr(pci, 0, 0x80800000, 0); }
-/* PIIX4 Power Management device (for ACPI) */ -static void piix4_pm_setup(struct pci_device *pci, void *arg) +static void piix4_pm_config_setup(u16 bdf) {
- u16 bdf = pci->bdf; // acpi sci is hardwired to 9 pci_config_writeb(bdf, PCI_INTERRUPT_LINE, 9);
@@ -241,6 +239,15 @@ static void piix4_pm_setup(struct pci_device *pci, void *arg) pci_config_writeb(bdf, 0x80, 0x01); /* enable PM io space */ pci_config_writel(bdf, 0x90, PORT_SMB_BASE | 1); pci_config_writeb(bdf, 0xd2, 0x09); /* enable SMBus io space */ +}
+static int PiixPMBDF = -1;
+/* PIIX4 Power Management device (for ACPI) */ +static void piix4_pm_setup(struct pci_device *pci, void *arg) +{
PiixPMBDF = pci->bdf;
piix4_pm_config_setup(pci->bdf);
acpi_pm1a_cnt = PORT_ACPI_PM_BASE + 0x04; pmtimer_setup(PORT_ACPI_PM_BASE + 0x08);
@@ -858,3 +865,12 @@ pci_setup(void)
pci_enable_default_vga();
}
+void +pci_resume(void) +{
- if (!CONFIG_QEMU)
return;
- if (PiixPMBDF >= 0)
piix4_pm_config_setup(PiixPMBDF);
+} diff --git a/src/resume.c b/src/resume.c index d69429c..0c0e60e 100644 --- a/src/resume.c +++ b/src/resume.c @@ -100,6 +100,7 @@ s3_resume(void)
pic_setup(); smm_setup();
pci_resume();
s3_resume_vga();
diff --git a/src/util.h b/src/util.h index 1b7d525..b8acbfa 100644 --- a/src/util.h +++ b/src/util.h @@ -99,6 +99,7 @@ void mtrr_setup(void); // fw/pciinit.c extern const u8 pci_irqs[4]; void pci_setup(void); +void pci_resume(void);
// fw/pirtable.c extern struct pir_header *PirAddr;
On Mon, Jan 13, 2014 at 07:46:33PM +0200, Marcel Apfelbaum wrote:
On Mon, 2014-01-13 at 11:31 -0500, Kevin O'Connor wrote:
Thanks. SeaBIOS isn't responsible for PCI setup on CSM/coreboot, so the patch must check for CONFIG_QEMU.
Sure thing, Sorry I missed that, I'll add it to V2.
Also, I think we can simplify this a bit - how about the patch below (untested)?
Hi Kevin, I followed the patch and indeed it is smaller and it does the job, but
- We also have a q35 chipset and we'll need a global variable also for it.
- If we will have other devices that need special attention on resume we will be ready for them (low chance, but you never know).
- Finally, the pci_resume will look a little strange and unclear with the new "if" statements (that means, this is the board and we saved the value)
Is this needed for q35? In general, the firmware is not responsible for restoring hardware state, so I don't think resume fixups will be common. The storing of PCI BDFs for use in resume is already done elsewhere in the code (see shadow.c and smm.c).
What do you think?
I'm open to alternatives. However, the code needs to be run only when CONFIG_QEMU is set, and I would ask that resume.c not be made more complicated - so lets have it just call a function (eg, pci_resume()) and put the rest of the logic in the fw/ directory.
-Kevin
On Mon, 2014-01-13 at 16:39 -0500, Kevin O'Connor wrote:
On Mon, Jan 13, 2014 at 07:46:33PM +0200, Marcel Apfelbaum wrote:
On Mon, 2014-01-13 at 11:31 -0500, Kevin O'Connor wrote:
Thanks. SeaBIOS isn't responsible for PCI setup on CSM/coreboot, so the patch must check for CONFIG_QEMU.
Sure thing, Sorry I missed that, I'll add it to V2.
Also, I think we can simplify this a bit - how about the patch below (untested)?
Hi Kevin, I followed the patch and indeed it is smaller and it does the job, but
- We also have a q35 chipset and we'll need a global variable also for it.
- If we will have other devices that need special attention on resume we will be ready for them (low chance, but you never know).
- Finally, the pci_resume will look a little strange and unclear with the new "if" statements (that means, this is the board and we saved the value)
Is this needed for q35? In general, the firmware is not responsible for restoring hardware state, so I don't think resume fixups will be common. The storing of PCI BDFs for use in resume is already done elsewhere in the code (see shadow.c and smm.c).
I checked and Q35 does not need this kind of hack, but it has other problems :( (Can't suspend/resume 2 times). I am going to find some time to look into it.
What do you think?
I'm open to alternatives. However, the code needs to be run only when CONFIG_QEMU is set, and I would ask that resume.c not be made more complicated - so lets have it just call a function (eg, pci_resume()) and put the rest of the logic in the fw/ directory.
Thank you for the review! I sent a V2 following your review.
Thanks, Marcel
-Kevin