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

[Laszlo Ersek <lersek@redhat.com>]

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

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

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

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?"

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

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

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.

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

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