Re: [Qemu-devel] [PATCH v2] pci: allow 0 address for PCI IO/MEM regions

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jul 23, 2015 at 08:24:18PM +0200, Laurent Vivier wrote:
> From: Michael Roth <mdroth@linux.vnet.ibm.com>

I think at this point it's your patches, I'd write
"based on patch by Michael Roth" instead.

> 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 <mdroth@linux.vnet.ibm.com>
> [lvivier: add pci_allow_0_addr in MachineClass to conditionally
> allow addr 0 for pseries, as this can break other architectures]

Please spell the detailed explanation out in the commit log, not here.

> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

I don't know whether this is the right thing to do for spapr,
for pci.c I'm fine with this as a temporary hack:

Acked-by: Michael S. Tsirkin <mst@redhat.com>

but please look (in 2.5 timeframe) at fixing priorities for more machine
types, esp the PC.

> ---
> v2: move flag from PCIBus to MachineClass, rename it pci_allow_0_address
>  hw/pci/pci.c        | 12 +++++++++---
>  hw/ppc/spapr.c      |  1 +
>  include/hw/boards.h |  3 ++-
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a017614..9f57aea 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -38,6 +38,7 @@
>  #include "hw/pci/msix.h"
>  #include "exec/address-spaces.h"
>  #include "hw/hotplug.h"
> +#include "hw/boards.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> @@ -1065,6 +1066,10 @@ 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);
> +    Object *machine = qdev_get_machine();
> +    ObjectClass *oc = object_get_class(machine);
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    bool allow_0_address = mc->pci_allow_0_address;
>  
>      if (type & PCI_BASE_ADDRESS_SPACE_IO) {
>          if (!(cmd & PCI_COMMAND_IO)) {
> @@ -1075,7 +1080,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 ||
> +            (!allow_0_address && new_addr == 0)) {
>              return PCI_BAR_UNMAPPED;
>          }
>          return new_addr;
> @@ -1099,8 +1105,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 ||
> +        (!allow_0_address && new_addr == 0)) {
>          return PCI_BAR_UNMAPPED;
>      }
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a6f1947..bf0c64f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1835,6 +1835,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
>      mc->default_ram_size = 512 * M_BYTE;
>      mc->kvm_type = spapr_kvm_type;
>      mc->has_dynamic_sysbus = true;
> +    mc->pci_allow_0_address = true;
>  
>      fwc->get_dev_path = spapr_get_fw_dev_path;
>      nc->nmi_monitor_handler = spapr_nmi;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2aec9cb..3f84afd 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -100,7 +100,8 @@ struct MachineClass {
>          no_cdrom:1,
>          no_sdcard:1,
>          has_dynamic_sysbus:1,
> -        no_tco:1;
> +        no_tco:1,
> +        pci_allow_0_address:1;
>      int is_default;
>      const char *default_machine_opts;
>      const char *default_boot_order;
> -- 
> 2.1.0