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
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.
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
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.
----- Messaggio originale -----
Da: "Michael S. Tsirkin" mst@redhat.com A: "marcel a" marcel.a@redhat.com Cc: "Paolo Bonzini" pbonzini@redhat.com, "Gal Hammer" ghammer@redhat.com, seabios@seabios.org, qemu-devel@nongnu.org Inviato: Mercoledì, 18 dicembre 2013 17:33:06 Oggetto: Re: [Qemu-devel] [PATCH] piix: do not reset APIC base address (0x80) on piix4_reset.
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.
Or put an array of (bdf, offset, size, value) tuples somewhere in low memory, fill it at startup, and reproduce it blindly at S3 resume time. This is similar to what UEFI does.
Paolo
On Wed, 2013-12-18 at 11:34 -0500, Paolo Bonzini wrote:
----- Messaggio originale -----
Da: "Michael S. Tsirkin" mst@redhat.com A: "marcel a" marcel.a@redhat.com Cc: "Paolo Bonzini" pbonzini@redhat.com, "Gal Hammer" ghammer@redhat.com, seabios@seabios.org, qemu-devel@nongnu.org Inviato: Mercoledì, 18 dicembre 2013 17:33:06 Oggetto: Re: [Qemu-devel] [PATCH] piix: do not reset APIC base address (0x80) on piix4_reset.
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.
Or put an array of (bdf, offset, size, value) tuples somewhere in low memory, fill it at startup, and reproduce it blindly at S3 resume time. This is similar to what UEFI does.
Could you please elaborate a little more? Let me see first if I understand the problem: PciDevices list is a list of pointers that cannot be used inside init code which is 16 bit code, right?
If I got it right, the solution is to find another way to iterate over pci devices? If yes, *when* would this data be put in memory and "when"? A pointer to the right direction would be appreciated,
Thanks! Marcel
Paolo
On Wed, Dec 18, 2013 at 06:55:24PM +0200, Marcel Apfelbaum wrote:
On Wed, 2013-12-18 at 11:34 -0500, Paolo Bonzini wrote:
----- Messaggio originale -----
Da: "Michael S. Tsirkin" mst@redhat.com A: "marcel a" marcel.a@redhat.com Cc: "Paolo Bonzini" pbonzini@redhat.com, "Gal Hammer" ghammer@redhat.com, seabios@seabios.org, qemu-devel@nongnu.org Inviato: Mercoledì, 18 dicembre 2013 17:33:06 Oggetto: Re: [Qemu-devel] [PATCH] piix: do not reset APIC base address (0x80) on piix4_reset.
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.
Or put an array of (bdf, offset, size, value) tuples somewhere in low memory, fill it at startup, and reproduce it blindly at S3 resume time. This is similar to what UEFI does.
Could you please elaborate a little more? Let me see first if I understand the problem: PciDevices list is a list of pointers that cannot be used inside init code which is 16 bit code, right?
If I got it right, the solution is to find another way to iterate over pci devices?
Yes, but I think foreachbdf does this in already.
If yes, *when* would this data be put in memory and "when"? A pointer to the right direction would be appreciated,
Thanks! Marcel
Paolo
On Wed, Dec 18, 2013 at 06:55:24PM +0200, Marcel Apfelbaum wrote:
On Wed, 2013-12-18 at 11:34 -0500, Paolo Bonzini wrote:
Or put an array of (bdf, offset, size, value) tuples somewhere in low memory, fill it at startup, and reproduce it blindly at S3 resume time. This is similar to what UEFI does.
Could you please elaborate a little more? Let me see first if I understand the problem: PciDevices list is a list of pointers that cannot be used inside init code which is 16 bit code, right?
FYI, all the code at this point is 32bit code. Both the SeaBIOS init code (aka POST) and the SeaBIOS resume code run in 32bit mode.
The problem is that SeaBIOS has ownership of all ram during its initialization phase, but it must release ownership during its runtime phase. (During the runtime phase, the OS has ownership of the bulk of ram and SeaBIOS only has a tiny fraction that it reserves.) The PCI device cache that SeaBIOS builds is not placed in reserved memory, and that's why it is marked as VARVERIFY32INIT. It's to try and prevent pointers that no longer point to valid memory from being accessed after the init phase has completed.
The error it produces is correct - one must not access the pci_device structs from the resume code in the current code.
If I got it right, the solution is to find another way to iterate over pci devices? If yes, *when* would this data be put in memory and "when"? A pointer to the right direction would be appreciated,
A good question. I'll take a look at Michael's patch.
-Kevin
On Thu, 2013-12-19 at 11:06 -0500, Kevin O'Connor wrote:
On Wed, Dec 18, 2013 at 06:55:24PM +0200, Marcel Apfelbaum wrote:
On Wed, 2013-12-18 at 11:34 -0500, Paolo Bonzini wrote:
Or put an array of (bdf, offset, size, value) tuples somewhere in low memory, fill it at startup, and reproduce it blindly at S3 resume time. This is similar to what UEFI does.
Could you please elaborate a little more? Let me see first if I understand the problem: PciDevices list is a list of pointers that cannot be used inside init code which is 16 bit code, right?
FYI, all the code at this point is 32bit code. Both the SeaBIOS init code (aka POST) and the SeaBIOS resume code run in 32bit mode.
The problem is that SeaBIOS has ownership of all ram during its initialization phase, but it must release ownership during its runtime phase. (During the runtime phase, the OS has ownership of the bulk of ram and SeaBIOS only has a tiny fraction that it reserves.) The PCI device cache that SeaBIOS builds is not placed in reserved memory, and that's why it is marked as VARVERIFY32INIT. It's to try and prevent pointers that no longer point to valid memory from being accessed after the init phase has completed.
The error it produces is correct - one must not access the pci_device structs from the resume code in the current code.
Thank you Kevin for the detailed explanation! By the way, do you know how this fraction is allocated by Seabios and how can one "decide" to move the device cache to this region reserved by the BIOS ? (not that I want to, but to understand how Seabios does this)
Thanks again! Marcel
If I got it right, the solution is to find another way to iterate over pci devices? If yes, *when* would this data be put in memory and "when"? A pointer to the right direction would be appreciated,
A good question. I'll take a look at Michael's patch.
-Kevin
On Thu, Dec 19, 2013 at 08:03:15PM +0200, Marcel Apfelbaum wrote:
On Thu, 2013-12-19 at 11:06 -0500, Kevin O'Connor wrote:
On Wed, Dec 18, 2013 at 06:55:24PM +0200, Marcel Apfelbaum wrote:
On Wed, 2013-12-18 at 11:34 -0500, Paolo Bonzini wrote:
Or put an array of (bdf, offset, size, value) tuples somewhere in low memory, fill it at startup, and reproduce it blindly at S3 resume time. This is similar to what UEFI does.
Could you please elaborate a little more? Let me see first if I understand the problem: PciDevices list is a list of pointers that cannot be used inside init code which is 16 bit code, right?
FYI, all the code at this point is 32bit code. Both the SeaBIOS init code (aka POST) and the SeaBIOS resume code run in 32bit mode.
The problem is that SeaBIOS has ownership of all ram during its initialization phase, but it must release ownership during its runtime phase. (During the runtime phase, the OS has ownership of the bulk of ram and SeaBIOS only has a tiny fraction that it reserves.) The PCI device cache that SeaBIOS builds is not placed in reserved memory, and that's why it is marked as VARVERIFY32INIT. It's to try and prevent pointers that no longer point to valid memory from being accessed after the init phase has completed.
The error it produces is correct - one must not access the pci_device structs from the resume code in the current code.
Thank you Kevin for the detailed explanation! By the way, do you know how this fraction is allocated by Seabios and how can one "decide" to move the device cache to this region reserved by the BIOS ? (not that I want to, but to understand how Seabios does this)
In pci.c:pci_probe_devices(), you'll see that it calls malloc_tmp() to allocate the struct pci_device. That allocation function takes memory from ram that will eventually be given back to the OS. To make it not do that, one would need to choose one of the reserved zones (ie, malloc_fseg, malloc_low, or malloc_high). There is some freedom in the choice of zones - malloc_fseg would probably be the simplest to use.
However, as you suspected, I don't think just allocating in a reserved zone is the right thing to do. Caching all the PCI devices after init could lead to confusion as the devices cached may not be present later on or have different parameters (eg, due to hotplug).
-Kevin
On Wed, Dec 18, 2013 at 11:34:14AM -0500, Paolo Bonzini wrote:
----- Messaggio originale -----
Da: "Michael S. Tsirkin" mst@redhat.com A: "marcel a" marcel.a@redhat.com Cc: "Paolo Bonzini" pbonzini@redhat.com, "Gal Hammer" ghammer@redhat.com, seabios@seabios.org, qemu-devel@nongnu.org Inviato: Mercoledì, 18 dicembre 2013 17:33:06 Oggetto: Re: [Qemu-devel] [PATCH] piix: do not reset APIC base address (0x80) on piix4_reset.
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.
Or put an array of (bdf, offset, size, value) tuples somewhere in low memory, fill it at startup, and reproduce it blindly at S3 resume time. This is similar to what UEFI does.
Paolo
Yes, it's an option. Though reworking pci_bios_init_devices so that it can work from low memory seems easier.
On 12/18/13 17:34, Paolo Bonzini wrote:
----- Messaggio originale -----
Da: "Michael S. Tsirkin" mst@redhat.com A: "marcel a" marcel.a@redhat.com Cc: "Paolo Bonzini" pbonzini@redhat.com, "Gal Hammer" ghammer@redhat.com, seabios@seabios.org, qemu-devel@nongnu.org Inviato: Mercoledì, 18 dicembre 2013 17:33:06 Oggetto: Re: [Qemu-devel] [PATCH] piix: do not reset APIC base address (0x80) on piix4_reset.
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.
Or put an array of (bdf, offset, size, value) tuples somewhere in low memory, fill it at startup, and reproduce it blindly at S3 resume time. This is similar to what UEFI does.
What UEFI does is kind of a mess :)
PEI runs both during cold boot and S3 resume.
DXE runs only after cold boot, it is not reached during S3 resume. (The DXE initial program loader is the last PEI module, and dependent on the boot mode, it either loads the DXE core, or invokes the S3 resume vector.)
So, PEI itself can reinitialize whatever it wants. The S3 boot script that you refer to above is a way for DXE drivers to stash a bunch of IO / PCI etc writes for the *next* PEI phase, ie. the one that runs when resuming from S3. QemuVideoDxe (the driver in OVMF that configures produces the GOP on top of Cirrus) is such a DXE driver.
In a nutshell,
1. SEC after cold boot 2. PEI after cold boot 2.5 DXE IPL PEIM loads DXE core 3. DXE after cold boot 4. BDS after cold boot 5. runtime (OSPM), normal entry 6. PEI after S3 resume 6.5 DXE IPL PEIM branches to S3 resume PEIM 7. runtime (OSPM), entry via S3 resume vector
Steps 2 and 6 are implemented by the same PEI code (it can branch internally based on boot mode of course), and this PEI code contains the excerpt that I posted a minute ago.
The S3 boot script is prepared during step 3 by various DXE drivers, and it is saved into reserved / AcpiNVS RAM no later than entering 5. This script is then replayed / interpreted by step 6.
So, if PEI must do something after S3 resume that is independent of any DXE drivers, it can simply do it. The boot script is only necessary when the S3 resume PEI actions (in step 6) need to depend on earlier actions during DXE (step 3).
Thanks, Laszlo
On 12/18/13 23:10, Laszlo Ersek wrote:
- SEC after cold boot
- PEI after cold boot
2.5 DXE IPL PEIM loads DXE core 3. DXE after cold boot 4. BDS after cold boot 5. runtime (OSPM), normal entry 6. PEI after S3 resume 6.5 DXE IPL PEIM branches to S3 resume PEIM 7. runtime (OSPM), entry via S3 resume vector
(Sorry I left out SEC from between 5 and 6, but it's not relevant now.)
Laszlo
Il 18/12/2013 23:10, Laszlo Ersek ha scritto:
So, if PEI must do something after S3 resume that is independent of any DXE drivers, it can simply do it. The boot script is only necessary when the S3 resume PEI actions (in step 6) need to depend on earlier actions during DXE (step 3).
In SeaBIOS, PEI is really almost nothing (it's just the contents of src/resume.c), so all the setup needs to be placed in the "boot script".
For example I suspect that using INT 13h for disk I/O does not work after S3.
Paolo
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
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).
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 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();
On Thu, Dec 19, 2013 at 11:49:21AM +0200, Marcel Apfelbaum wrote:
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
Actually in hindsight there's no chance this can work: it assumes device memory is mapped already. I really meant just calling pci_init_device. Maybe something like the below, or maybe we even need a separate table for resume callbacks.
---
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..3a24dba 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); diff --git a/src/fw/pciinit.c b/src/fw/pciinit.c index 34279a4..a4cedfb 100644 --- a/src/fw/pciinit.c +++ b/src/fw/pciinit.c @@ -293,6 +293,11 @@ static const struct pci_device_id pci_device_tbl[] = { PCI_DEVICE_END, };
+void pci_bios_resume_device(struct pci_device *pci) +{ + pci_init_device(pci_device_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/resume.c b/src/resume.c index fc2fee9..9aac853 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();
On 18/12/2013 16:22, 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).
I think we both made a mistake and the right name is "PMREGMISC—MISCELLANEOUS POWER MANAGEMENT (FUNCTION 3)" :-).
Michael, perhaps a part of pci_setup (same file) should run on S3 resume?
Paolo
Gal.
Il 18/12/2013 16:59, Gal Hammer ha scritto:
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).
I think we both made a mistake and the right name is "PMREGMISC—MISCELLANEOUS POWER MANAGEMENT (FUNCTION 3)" :-).
Yeah. :) Still, both PMBA and PMREGMISC need to be initialized by SeaBIOS.
Paolo
-
Gal Hammer -
Kevin O'Connor -
Laszlo Ersek -
Marcel Apfelbaum -
Marcel Apfelbaum -
Michael S. Tsirkin -
Paolo Bonzini