Re: [Qemu-devel] [PATCH] hw/pci: do not update the PCI mappings while Decode (I/O or memory) bit is not set in the Command register

[Marcel Apfelbaum]

On 01/11/2016 06:11 PM, Laszlo Ersek wrote:
> On 01/11/16 13:24, Marcel Apfelbaum wrote:
> > Two reasons:
> >   - PCI Spec indicates that while the bit is not set
> >     the memory sizing is not finished.
> >   - pci_bar_address will return PCI_BAR_UNMAPPED
> >     and a previous value can be accidentally overridden
> >     if the command register is modified (and not the BAR).
> >
> > Signed-off-by: Marcel Apfelbaum <address@hidden>
> > ---
> >
> > Hi,
> >
> > I found this when trying to use multiple root complexes with OVMF.
> >
> > When trying to attach a device to the pxb-pcie device as Integrated
> > Device it did not receive the IO/MEM resources.
> >
> > The reason is that OVMF is working like that:
> >   1. It disables the Decode (I/O or memory) bit in the Command register
> >   2. It configures the device BARS
> >   3. Makes some tests on the Command register
> >   4. ...
> >   5. Enables the Decode (I/O or memory) at some point.
> >
> > On step 3 all the BARS are overridden to 0xffffffff by QEMU.
> >
> > Since QEMU uses the device BARs to compute the new host bridge resources
> > it now gets garbage.
> >
> > Laszlo, this also solves the SHPC problem for the pci-2-pci bridge inside the 
> > pxb.
> > Now we can enable the SHPC for it too.
>
> I encountered the exact same problem months ago. I posted patches for
> it; you were CC'd. :)
>
> http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342209
> http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342210
>
> As you can see under the second link above, I made the same analysis &
> observations as you do now. (It took me quite long to track down the
> "inexplicable" behavior of edk2's generic PCI bus driver / enumerator
> that is built into OVMF.)

Wow, I just re-worked this issue again from 0! I wish I have remembered those 
threads :(
This was another symptom of the exact problem! And I remembered something about
SHPC, I should have looked at those mail threads again...

> I proposed to change pci_bar_address() so that it could return, to
> distinguished callers, the BAR values "under programming", even if the
> command bits were clear. Then the ACPI generator would utilize this
> special exception.
>
> Michael disagreed; in
>
> http://thread.gmane.org/gmane.comp.emulators.qemu/342206/focus=342242
>
> he wrote "[t]his is problematic - disabled BAR values have no meaning
> according to the PCI spec".

Yes... because it looked like a hook for our case only,
the good news is that this patch is based exactly on the fact that
the BARs have no meaning if the bit is not set.

> The current  solution to the problem (= we disable the SHPC) was
> recommended by Michael in that message: "It might be best to add a
> property to just disable shpc in the bridge so no devices reside
> directly behind the pxb?"

I confess I don't exactly understand what the SHPC of the pci-2-pci bridge
has to do with sibling devices on the pxb's root bus (SHPC is the hot-plug 
controller
for the devices behind the pci-2-pci bridge).

The second part I do understand, the pxb design was to not have devices 
directly behind
the pxb, so maybe he meant that SHPC is the part of the pci-bridge that behaves 
like
a device in the sense it requires IO/MEM resources.

Bottom line, your solution for the PXB was just fine :)


> In comparison, your patch doesn't change pci_bar_address(). Instead, it
> modifies pci_update_mappings() *not to call* pci_bar_address(), if the
> respective command bits are clear.
>
> I guess that could have about the same effect.
>
> If, unlike my patch, yours actually improves QEMU's compliance with the
> PCI specs, then it's likely a good patch. (And apparently more general
> than the SHPC-specific solution we have now.)


Exactly! Why should a pci write to the command  register *delete*
previously set resources? I am looking at it as a bug.

And also updating the mappings while the Decoding bit is not enables
is at least not necessary.

> I just don't know if it's a good idea to leave any old mappings active
> while the BARs are being reprogrammed (with the command bits clear).

First, because the OS can't use the IO/MEM anyway, secondly the guest 
OS/firmware
is the one that disabled the bit... (in order to program resources)

> In other words, what guarantees that this change will not regress
> anything? (I'm not doubting -- I'm asking; I honestly don't know.)
>
> So I guess I'll defer to Michael on this one.

Michael, do you agree with the above?

> In any case, I fully agree with your analysis of OVMF's behavior.

Thanks! I looked for this bug in OVMF for some time now :)
Marcel

> Thanks!
> Laszlo
>
> > Thanks,
> > Marcel
> >
> >   hw/pci/pci.c | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> >
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index 168b9cc..f9127dc 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -1148,6 +1148,7 @@ static void pci_update_mappings(PCIDevice *d)
> >       PCIIORegion *r;
> >       int i;
> >       pcibus_t new_addr;
> > +    uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> >
> >       for(i = 0; i < PCI_NUM_REGIONS; i++) {
> >           r = &d->io_regions[i];
> > @@ -1156,6 +1157,22 @@ static void pci_update_mappings(PCIDevice *d)
> >           if (!r->size)
> >               continue;
> >
> > +        /*
> > +         * Do not update the mappings until the command register's
> > +         * Decode (I/O or memory) bit is not set. Two reasons:
> > +         * - PCI Spec indicates that while the bit is not set
> > +         *   the memory sizing is not finished.
> > +         * - pci_bar_address will return PCI_BAR_UNMAPPED
> > +         *   and a previous value can be accidentally overridden
> > +         *   if the command register is modified (and not the BAR).
> > +         * */
> > +        if (((r->type & PCI_BASE_ADDRESS_SPACE_IO) &&
> > +             !(cmd & PCI_COMMAND_IO)) ||
> > +            ((r->type != PCI_BASE_ADDRESS_SPACE_IO) &&
> > +             !(cmd & PCI_COMMAND_MEMORY))) {
> > +            continue;
> > +        }
> > +
> >           new_addr = pci_bar_address(d, i, r->type, r->size);
> >
> >           /* This bar isn't changed */