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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 11 Jan 2016 14:24:23 +0200
Marcel Apfelbaum <marcel@redhat.com> 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 <marcel@redhat.com>
> ---
> 
> 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.

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

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