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

[Kevin O'Connor]

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 
> Signed-off-by: Volker Rümelin
>
> 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