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

[Volker Rümelin]

> 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.

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