[SeaBIOS] [PATCH v2 2/2] reset: force standard PCI configuration access

Volker Rümelin posted 2 patches 2 years, 1 month ago
There is a newer version of this series
[SeaBIOS] [PATCH v2 2/2] reset: force standard PCI configuration access
Posted by Volker Rümelin 2 years, 1 month ago
After a reset of a QEMU -machine q35 guest, the PCI Express
Enhanced Configuration Mechanism is disabled and the variable
mmconfig no longer matches the configuration register PCIEXBAR
of the Q35 chipset. Until the variable mmconfig is reset to 0,
all pci_config_*() functions no longer work.

The variable mmconfig is located in the read-only F-segment.
To reset it the pci_config_*() functions are needed, but they
do not work.

Replace all pci_config_*() calls with Standard PCI Configuration
Mechanism pci_ioconfig_*() calls until mmconfig is reset to 0.

This fixes

In resume (status=0)
In 32bit resume
Attempting a hard reboot
Unable to unlock ram - bridge not found

and a reset loop with QEMU -accel tcg.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
---
 src/fw/shadow.c | 16 +++++++++-------
 src/hw/pci.c    | 32 ++++++++++++++++++++++++++++++++
 src/hw/pci.h    |  7 +++++++
 3 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/src/fw/shadow.c b/src/fw/shadow.c
index 4c627a8..0722df2 100644
--- a/src/fw/shadow.c
+++ b/src/fw/shadow.c
@@ -32,8 +32,8 @@ __make_bios_writable_intel(u16 bdf, u32 pam0)
 {
     // Read in current PAM settings from pci config space
     union pamdata_u pamdata;
-    pamdata.data32[0] = pci_config_readl(bdf, ALIGN_DOWN(pam0, 4));
-    pamdata.data32[1] = pci_config_readl(bdf, ALIGN_DOWN(pam0, 4) + 4);
+    pamdata.data32[0] = pci_ioconfig_readl(bdf, ALIGN_DOWN(pam0, 4));
+    pamdata.data32[1] = pci_ioconfig_readl(bdf, ALIGN_DOWN(pam0, 4) + 4);
     u8 *pam = &pamdata.data8[pam0 & 0x03];
 
     // Make ram from 0xc0000-0xf0000 writable
@@ -46,8 +46,8 @@ __make_bios_writable_intel(u16 bdf, u32 pam0)
     pam[0] = 0x30;
 
     // Write PAM settings back to pci config space
-    pci_config_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
-    pci_config_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);
+    pci_ioconfig_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
+    pci_ioconfig_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);
 
     if (!ram_present)
         // Copy bios.
@@ -59,7 +59,7 @@ __make_bios_writable_intel(u16 bdf, u32 pam0)
 static void
 make_bios_writable_intel(u16 bdf, u32 pam0)
 {
-    int reg = pci_config_readb(bdf, pam0);
+    int reg = pci_ioconfig_readb(bdf, pam0);
     if (!(reg & 0x10)) {
         // QEMU doesn't fully implement the piix shadow capabilities -
         // if ram isn't backing the bios segment when shadowing is
@@ -125,8 +125,8 @@ make_bios_writable(void)
     // At this point, statically allocated variables can't be written,
     // so do this search manually.
     int bdf;
-    foreachbdf(bdf, 0) {
-        u32 vendev = pci_config_readl(bdf, PCI_VENDOR_ID);
+    pci_ioconfig_foreachbdf(bdf, 0) {
+        u32 vendev = pci_ioconfig_readl(bdf, PCI_VENDOR_ID);
         u16 vendor = vendev & 0xffff, device = vendev >> 16;
         if (vendor == PCI_VENDOR_ID_INTEL
             && device == PCI_DEVICE_ID_INTEL_82441) {
@@ -183,10 +183,12 @@ qemu_reboot(void)
             apm_shutdown();
         }
         make_bios_writable();
+        pci_disable_mmconfig();
         HaveRunPost = 3;
     } else {
         // Copy the BIOS making sure to only reset HaveRunPost at end
         make_bios_writable();
+        pci_disable_mmconfig();
         u32 cstart = SYMBOL(code32flat_start), cend = SYMBOL(code32flat_end);
         memcpy((void*)cstart, flash + cstart, hrp - cstart);
         memcpy((void*)hrp + 4, flash + hrp + 4, cend - (hrp + 4));
diff --git a/src/hw/pci.c b/src/hw/pci.c
index f13cbde..ccf208a 100644
--- a/src/hw/pci.c
+++ b/src/hw/pci.c
@@ -133,6 +133,11 @@ pci_enable_mmconfig(u64 addr, const char *name)
     mmconfig = addr;
 }
 
+void pci_disable_mmconfig(void)
+{
+    mmconfig = 0;
+}
+
 u8 pci_find_capability(u16 bdf, u8 cap_id, u8 cap)
 {
     int i;
@@ -157,6 +162,33 @@ u8 pci_find_capability(u16 bdf, u8 cap_id, u8 cap)
     return 0;
 }
 
+// Helper function for pci_ioconfig_foreachbdf() macro - return next device
+int pci_ioconfig_next(int bdf, int bus)
+{
+    if (pci_bdf_to_fn(bdf) == 0
+        && (pci_ioconfig_readb(bdf, PCI_HEADER_TYPE) & 0x80) == 0)
+        // Last found device wasn't a multi-function device - skip to
+        // the next device.
+        bdf += 8;
+    else
+        bdf += 1;
+
+    for (;;) {
+        if (pci_bdf_to_bus(bdf) != bus)
+            return -1;
+
+        u16 v = pci_ioconfig_readw(bdf, PCI_VENDOR_ID);
+        if (v != 0x0000 && v != 0xffff)
+            // Device is present.
+            return bdf;
+
+        if (pci_bdf_to_fn(bdf) == 0)
+            bdf += 8;
+        else
+            bdf += 1;
+    }
+}
+
 // Helper function for foreachbdf() macro - return next device
 int
 pci_next(int bdf, int bus)
diff --git a/src/hw/pci.h b/src/hw/pci.h
index ee6acaf..ec45fca 100644
--- a/src/hw/pci.h
+++ b/src/hw/pci.h
@@ -27,6 +27,11 @@ static inline u16 pci_bus_devfn_to_bdf(int bus, u16 devfn) {
     return (bus << 8) | devfn;
 }
 
+#define pci_ioconfig_foreachbdf(BDF, BUS)                               \
+    for (BDF=pci_ioconfig_next(pci_bus_devfn_to_bdf((BUS), 0)-1, (BUS)) \
+         ; BDF >= 0                                                     \
+         ; BDF=pci_ioconfig_next(BDF, (BUS)))
+
 #define foreachbdf(BDF, BUS)                                    \
     for (BDF=pci_next(pci_bus_devfn_to_bdf((BUS), 0)-1, (BUS))  \
          ; BDF >= 0                                             \
@@ -39,6 +44,7 @@ void pci_ioconfig_writeb(u16 bdf, u32 addr, u8 val);
 u32 pci_ioconfig_readl(u16 bdf, u32 addr);
 u16 pci_ioconfig_readw(u16 bdf, u32 addr);
 u8 pci_ioconfig_readb(u16 bdf, u32 addr);
+int pci_ioconfig_next(int bdf, int bus);
 
 // PCI configuration access using either PCI CAM or PCIe ECAM
 void pci_config_writel(u16 bdf, u32 addr, u32 val);
@@ -52,6 +58,7 @@ u8 pci_find_capability(u16 bdf, u8 cap_id, u8 cap);
 int pci_next(int bdf, int bus);
 
 void pci_enable_mmconfig(u64 addr, const char *name);
+void pci_disable_mmconfig(void);
 int pci_probe_host(void);
 void pci_reboot(void);
 
-- 
2.34.1

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH v2 2/2] reset: force standard PCI configuration access
Posted by Kevin O'Connor 2 years, 1 month ago
On Sat, Mar 26, 2022 at 10:33:19AM +0100, Volker Rümelin wrote:
> After a reset of a QEMU -machine q35 guest, the PCI Express
> Enhanced Configuration Mechanism is disabled and the variable
> mmconfig no longer matches the configuration register PCIEXBAR
> of the Q35 chipset. Until the variable mmconfig is reset to 0,
> all pci_config_*() functions no longer work.
> 
> The variable mmconfig is located in the read-only F-segment.
> To reset it the pci_config_*() functions are needed, but they
> do not work.
> 
> Replace all pci_config_*() calls with Standard PCI Configuration
> Mechanism pci_ioconfig_*() calls until mmconfig is reset to 0.
> 
> This fixes
> 
> In resume (status=0)
> In 32bit resume
> Attempting a hard reboot
> Unable to unlock ram - bridge not found
> 
> and a reset loop with QEMU -accel tcg.
> 
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
> ---
>  src/fw/shadow.c | 16 +++++++++-------
>  src/hw/pci.c    | 32 ++++++++++++++++++++++++++++++++
>  src/hw/pci.h    |  7 +++++++
>  3 files changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/src/fw/shadow.c b/src/fw/shadow.c
> index 4c627a8..0722df2 100644
> --- a/src/fw/shadow.c
> +++ b/src/fw/shadow.c
> @@ -32,8 +32,8 @@ __make_bios_writable_intel(u16 bdf, u32 pam0)
>  {
>      // Read in current PAM settings from pci config space
>      union pamdata_u pamdata;
> -    pamdata.data32[0] = pci_config_readl(bdf, ALIGN_DOWN(pam0, 4));
> -    pamdata.data32[1] = pci_config_readl(bdf, ALIGN_DOWN(pam0, 4) + 4);
> +    pamdata.data32[0] = pci_ioconfig_readl(bdf, ALIGN_DOWN(pam0, 4));
> +    pamdata.data32[1] = pci_ioconfig_readl(bdf, ALIGN_DOWN(pam0, 4) + 4);
>      u8 *pam = &pamdata.data8[pam0 & 0x03];
>  
>      // Make ram from 0xc0000-0xf0000 writable
> @@ -46,8 +46,8 @@ __make_bios_writable_intel(u16 bdf, u32 pam0)
>      pam[0] = 0x30;
>  
>      // Write PAM settings back to pci config space
> -    pci_config_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
> -    pci_config_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);
> +    pci_ioconfig_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
> +    pci_ioconfig_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);
>  
>      if (!ram_present)
>          // Copy bios.
> @@ -59,7 +59,7 @@ __make_bios_writable_intel(u16 bdf, u32 pam0)
>  static void
>  make_bios_writable_intel(u16 bdf, u32 pam0)
>  {
> -    int reg = pci_config_readb(bdf, pam0);
> +    int reg = pci_ioconfig_readb(bdf, pam0);
>      if (!(reg & 0x10)) {
>          // QEMU doesn't fully implement the piix shadow capabilities -
>          // if ram isn't backing the bios segment when shadowing is
> @@ -125,8 +125,8 @@ make_bios_writable(void)
>      // At this point, statically allocated variables can't be written,
>      // so do this search manually.
>      int bdf;
> -    foreachbdf(bdf, 0) {
> -        u32 vendev = pci_config_readl(bdf, PCI_VENDOR_ID);
> +    pci_ioconfig_foreachbdf(bdf, 0) {
> +        u32 vendev = pci_ioconfig_readl(bdf, PCI_VENDOR_ID);
>          u16 vendor = vendev & 0xffff, device = vendev >> 16;
>          if (vendor == PCI_VENDOR_ID_INTEL
>              && device == PCI_DEVICE_ID_INTEL_82441) {
> @@ -183,10 +183,12 @@ qemu_reboot(void)
>              apm_shutdown();
>          }
>          make_bios_writable();
> +        pci_disable_mmconfig();
>          HaveRunPost = 3;
>      } else {
>          // Copy the BIOS making sure to only reset HaveRunPost at end
>          make_bios_writable();
> +        pci_disable_mmconfig();

I don't understand these two calls to pci_disable_mmconfig() as the
f-segment is about to be overwritten and the machine is about to be
turned off.

Othwerwise, looks fine to me.

Thanks,
-Kevin
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org
[SeaBIOS] Re: [PATCH v2 2/2] reset: force standard PCI configuration access
Posted by Volker Rümelin 2 years, 1 month ago
> On Sat, Mar 26, 2022 at 10:33:19AM +0100, Volker Rümelin wrote:
>> After a reset of a QEMU -machine q35 guest, the PCI Express
>> Enhanced Configuration Mechanism is disabled and the variable
>> mmconfig no longer matches the configuration register PCIEXBAR
>> of the Q35 chipset. Until the variable mmconfig is reset to 0,
>> all pci_config_*() functions no longer work.
>>
>> The variable mmconfig is located in the read-only F-segment.
>> To reset it the pci_config_*() functions are needed, but they
>> do not work.
>>
>> Replace all pci_config_*() calls with Standard PCI Configuration
>> Mechanism pci_ioconfig_*() calls until mmconfig is reset to 0.
>>
>> This fixes
>>
>> In resume (status=0)
>> In 32bit resume
>> Attempting a hard reboot
>> Unable to unlock ram - bridge not found
>>
>> and a reset loop with QEMU -accel tcg.
>>
>> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
>> Signed-off-by: Volker Rümelin <vr_qemu@t-online.de>
>> ---
>>   src/fw/shadow.c | 16 +++++++++-------
>>   src/hw/pci.c    | 32 ++++++++++++++++++++++++++++++++
>>   src/hw/pci.h    |  7 +++++++
>>   3 files changed, 48 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/fw/shadow.c b/src/fw/shadow.c
>> index 4c627a8..0722df2 100644
>> --- a/src/fw/shadow.c
>> +++ b/src/fw/shadow.c
>> @@ -32,8 +32,8 @@ __make_bios_writable_intel(u16 bdf, u32 pam0)
>>   {
>>       // Read in current PAM settings from pci config space
>>       union pamdata_u pamdata;
>> -    pamdata.data32[0] = pci_config_readl(bdf, ALIGN_DOWN(pam0, 4));
>> -    pamdata.data32[1] = pci_config_readl(bdf, ALIGN_DOWN(pam0, 4) + 4);
>> +    pamdata.data32[0] = pci_ioconfig_readl(bdf, ALIGN_DOWN(pam0, 4));
>> +    pamdata.data32[1] = pci_ioconfig_readl(bdf, ALIGN_DOWN(pam0, 4) + 4);
>>       u8 *pam = &pamdata.data8[pam0 & 0x03];
>>   
>>       // Make ram from 0xc0000-0xf0000 writable
>> @@ -46,8 +46,8 @@ __make_bios_writable_intel(u16 bdf, u32 pam0)
>>       pam[0] = 0x30;
>>   
>>       // Write PAM settings back to pci config space
>> -    pci_config_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
>> -    pci_config_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);
>> +    pci_ioconfig_writel(bdf, ALIGN_DOWN(pam0, 4), pamdata.data32[0]);
>> +    pci_ioconfig_writel(bdf, ALIGN_DOWN(pam0, 4) + 4, pamdata.data32[1]);
>>   
>>       if (!ram_present)
>>           // Copy bios.
>> @@ -59,7 +59,7 @@ __make_bios_writable_intel(u16 bdf, u32 pam0)
>>   static void
>>   make_bios_writable_intel(u16 bdf, u32 pam0)
>>   {
>> -    int reg = pci_config_readb(bdf, pam0);
>> +    int reg = pci_ioconfig_readb(bdf, pam0);
>>       if (!(reg & 0x10)) {
>>           // QEMU doesn't fully implement the piix shadow capabilities -
>>           // if ram isn't backing the bios segment when shadowing is
>> @@ -125,8 +125,8 @@ make_bios_writable(void)
>>       // At this point, statically allocated variables can't be written,
>>       // so do this search manually.
>>       int bdf;
>> -    foreachbdf(bdf, 0) {
>> -        u32 vendev = pci_config_readl(bdf, PCI_VENDOR_ID);
>> +    pci_ioconfig_foreachbdf(bdf, 0) {
>> +        u32 vendev = pci_ioconfig_readl(bdf, PCI_VENDOR_ID);
>>           u16 vendor = vendev & 0xffff, device = vendev >> 16;
>>           if (vendor == PCI_VENDOR_ID_INTEL
>>               && device == PCI_DEVICE_ID_INTEL_82441) {
>> @@ -183,10 +183,12 @@ qemu_reboot(void)
>>               apm_shutdown();
>>           }
>>           make_bios_writable();
>> +        pci_disable_mmconfig();
>>           HaveRunPost = 3;
>>       } else {
>>           // Copy the BIOS making sure to only reset HaveRunPost at end
>>           make_bios_writable();
>> +        pci_disable_mmconfig();
> I don't understand these two calls to pci_disable_mmconfig() as the
> f-segment is about to be overwritten and the machine is about to be
> turned off.

You're right. Both calls to pci_disable_mmconfig() are unnecessary. I'll 
send a version 3 series.

With best regards,
Volker

>
> Othwerwise, looks fine to me.
>
> Thanks,
> -Kevin

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org