Re: [Qemu-ppc] [PATCH|RFC 1/1] pci: allow 0 address for PCI IO/MEM regions

[Michael S. Tsirkin]

On Wed, Jul 22, 2015 at 01:54:59PM +0200, Laurent Vivier wrote:
> From: Michael Roth <address@hidden>
> 
> Some kernels program a 0 address for io regions. PCI 3.0 spec
> section 6.2.5.1 doesn't seem to disallow this.
> 
> Signed-off-by: Michael Roth <address@hidden>
> [lvivier: add accept_addr_0 in PCIBus to conditionally allow addr 0,
>  as this can break other architectures]
> Signed-off-by: Laurent Vivier <address@hidden>

I guess it's a solution - though I'd rather fix up priorities
for PC and others.

> ---
>  hw/pci/pci.c             | 9 ++++++---
>  hw/ppc/spapr_pci.c       | 1 +
>  include/hw/pci/pci_bus.h | 5 +++++
>  3 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a017614..b5a3658 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -338,6 +338,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>      bus->devfn_min = devfn_min;
>      bus->address_space_mem = address_space_mem;
>      bus->address_space_io = address_space_io;
> +    bus->accept_addr_0 = false;
>  
>      /* host bridge */
>      QLIST_INIT(&bus->child);
> @@ -1065,6 +1066,7 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>      pcibus_t new_addr, last_addr;
>      int bar = pci_bar(d, reg);
>      uint16_t cmd = pci_get_word(d->config + PCI_COMMAND);
> +    bool accept_addr_0 = d->bus->accept_addr_0;
>  
>      if (type & PCI_BASE_ADDRESS_SPACE_IO) {
>          if (!(cmd & PCI_COMMAND_IO)) {
> @@ -1075,7 +1077,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>          /* Check if 32 bit BAR wraps around explicitly.
>           * TODO: make priorities correct and remove this work around.
>           */
> -        if (last_addr <= new_addr || new_addr == 0 || last_addr >= 
> UINT32_MAX) {
> +        if (last_addr <= new_addr || last_addr >= UINT32_MAX ||
> +            (!accept_addr_0 && new_addr == 0)) {
>              return PCI_BAR_UNMAPPED;
>          }
>          return new_addr;
> @@ -1099,8 +1102,8 @@ static pcibus_t pci_bar_address(PCIDevice *d,
>      /* XXX: as we cannot support really dynamic
>         mappings, we handle specific values as invalid
>         mappings. */
> -    if (last_addr <= new_addr || new_addr == 0 ||
> -        last_addr == PCI_BAR_UNMAPPED) {
> +    if (last_addr <= new_addr || last_addr == PCI_BAR_UNMAPPED ||
> +        (!accept_addr_0 && new_addr == 0)) {
>          return PCI_BAR_UNMAPPED;
>      }
>  
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index a8f79d8..29374ea 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1302,6 +1302,7 @@ static void spapr_phb_realize(DeviceState *dev, Error 
> **errp)
>                             pci_spapr_set_irq, pci_spapr_map_irq, sphb,
>                             &sphb->memspace, &sphb->iospace,
>                             PCI_DEVFN(0, 0), PCI_NUM_PINS, TYPE_PCI_BUS);
> +    bus->accept_addr_0 = true;
>      phb->bus = bus;
>      qbus_set_hotplug_handler(BUS(phb->bus), DEVICE(sphb), NULL);
>  
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 403fec6..7eeeaa9 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -39,6 +39,11 @@ struct PCIBus {
>         Keep a count of the number of devices with raised IRQs.  */
>      int nirq;
>      int *irq_count;
> +    /* XXX: pseries kernels can configure BARs at address 0
> +     * it is allowed by specs but not managed correctly
> +     * by other architectures...
> +     */
> +    bool accept_addr_0;
>  };
>  
>  typedef struct PCIBridgeWindows PCIBridgeWindows;
> -- 
> 2.1.0