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 04:07 PM, Igor Mammedov wrote:
> On Mon, 11 Jan 2016 14:24:23 +0200
> Marcel Apfelbaum <address@hidden> 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.

Hi Igor,
Thanks for the review.

> What about migration case?
> Shouldn't mappings be updated to match source even if bit isn't set?

I don't know much about migration, but I think it would be just OK
because the regions itself are migrated (I think...) and the migration
target can't use the IO or MEM (until the bit will be set), so the mappings 
don't matter.
Once set, the mappings would be updated on the target.

Another thing, in the source the mappings are not updated either until the bit 
is is set.

Thanks,
Marcel


> > 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 */