QEMU does not allocate PCI resources (BARs) in any case - coldplug devices
are configured by the firmware and hotplug devices rely on the guest
system to do the assignment via the PCI rescan mechanism. Also in order
to create non empty "assigned-addresses", the device has to be enabled
(i.e. PCI_COMMAND needs the MMIO bit set) first as otherwise
io_regions[i].addr are -1, and devices are not enabled at this point.

This removes "assigned-addresses" and leaves it to those who actually
do resource allocation.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---

This replaces:
[PATCH qemu] spapr-pci: Provide either correct assigned-addresses or none

---
 hw/ppc/spapr_pci.c | 42 +++++++-----------------------------------
 1 file changed, 7 insertions(+), 35 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 6934d506a7e9..01ff41d4c43f 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -836,7 +836,7 @@ static char *spapr_phb_get_loc_code(SpaprPhbState *sphb, PCIDevice *pdev)
 #define b_fff(x)        b_x((x), 8, 3)  /* function number */
 #define b_rrrrrrrr(x)   b_x((x), 0, 8)  /* register number */
 
-/* for 'reg'/'assigned-addresses' OF properties */
+/* for 'reg' OF properties */
 #define RESOURCE_CELLS_SIZE 2
 #define RESOURCE_CELLS_ADDRESS 3
 
@@ -850,17 +850,14 @@ typedef struct ResourceFields {
 
 typedef struct ResourceProps {
     ResourceFields reg[8];
-    ResourceFields assigned[7];
     uint32_t reg_len;
-    uint32_t assigned_len;
 } ResourceProps;
 
-/* fill in the 'reg'/'assigned-resources' OF properties for
+/* fill in the 'reg' OF properties for
  * a PCI device. 'reg' describes resource requirements for a
- * device's IO/MEM regions, 'assigned-addresses' describes the
- * actual resource assignments.
+ * device's IO/MEM regions.
  *
- * the properties are arrays of ('phys-addr', 'size') pairs describing
+ * the property is an array of ('phys-addr', 'size') pairs describing
  * the addressable regions of the PCI device, where 'phys-addr' is a
  * RESOURCE_CELLS_ADDRESS-tuple of 32-bit integers corresponding to
  * (phys.hi, phys.mid, phys.lo), and 'size' is a
@@ -889,18 +886,7 @@ typedef struct ResourceProps {
  * phys.mid and phys.lo correspond respectively to the hi/lo portions
  * of the actual address of the region.
  *
- * how the phys-addr/size values are used differ slightly between
- * 'reg' and 'assigned-addresses' properties. namely, 'reg' has
- * an additional description for the config space region of the
- * device, and in the case of QEMU has n=0 and phys.mid=phys.lo=0
- * to describe the region as relocatable, with an address-mapping
- * that corresponds directly to the PHB's address space for the
- * resource. 'assigned-addresses' always has n=1 set with an absolute
- * address assigned for the resource. in general, 'assigned-addresses'
- * won't be populated, since addresses for PCI devices are generally
- * unmapped initially and left to the guest to assign.
- *
- * note also that addresses defined in these properties are, at least
+ * note also that addresses defined in this property are, at least
  * for PAPR guests, relative to the PHBs IO/MEM windows, and
  * correspond directly to the addresses in the BARs.
  *
@@ -914,8 +900,8 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
     uint32_t dev_id = (b_bbbbbbbb(bus_num) |
                        b_ddddd(PCI_SLOT(d->devfn)) |
                        b_fff(PCI_FUNC(d->devfn)));
-    ResourceFields *reg, *assigned;
-    int i, reg_idx = 0, assigned_idx = 0;
+    ResourceFields *reg;
+    int i, reg_idx = 0;
 
     /* config space region */
     reg = &rp->reg[reg_idx++];
@@ -944,21 +930,9 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
         reg->phys_lo = 0;
         reg->size_hi = cpu_to_be32(d->io_regions[i].size >> 32);
         reg->size_lo = cpu_to_be32(d->io_regions[i].size);
-
-        if (d->io_regions[i].addr == PCI_BAR_UNMAPPED) {
-            continue;
-        }
-
-        assigned = &rp->assigned[assigned_idx++];
-        assigned->phys_hi = cpu_to_be32(be32_to_cpu(reg->phys_hi) | b_n(1));
-        assigned->phys_mid = cpu_to_be32(d->io_regions[i].addr >> 32);
-        assigned->phys_lo = cpu_to_be32(d->io_regions[i].addr);
-        assigned->size_hi = reg->size_hi;
-        assigned->size_lo = reg->size_lo;
     }
 
     rp->reg_len = reg_idx * sizeof(ResourceFields);
-    rp->assigned_len = assigned_idx * sizeof(ResourceFields);
 }
 
 typedef struct PCIClass PCIClass;
@@ -1472,8 +1446,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
 
     populate_resource_props(dev, &rp);
     _FDT(fdt_setprop(fdt, offset, "reg", (uint8_t *)rp.reg, rp.reg_len));
-    _FDT(fdt_setprop(fdt, offset, "assigned-addresses",
-                     (uint8_t *)rp.assigned, rp.assigned_len));
 
     if (sphb->pcie_ecs && pci_is_express(dev)) {
         _FDT(fdt_setprop_cell(fdt, offset, "ibm,pci-config-space-type", 0x1));
-- 
2.17.1

On Fri, Sep 27, 2019 at 12:26:51PM +1000, Alexey Kardashevskiy wrote:
> QEMU does not allocate PCI resources (BARs) in any case - coldplug devices
> are configured by the firmware and hotplug devices rely on the guest
> system to do the assignment via the PCI rescan mechanism. Also in order
> to create non empty "assigned-addresses", the device has to be enabled
> (i.e. PCI_COMMAND needs the MMIO bit set) first as otherwise
> io_regions[i].addr are -1, and devices are not enabled at this point.
> 
> This removes "assigned-addresses" and leaves it to those who actually
> do resource allocation.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> 
> This replaces:
> [PATCH qemu] spapr-pci: Provide either correct assigned-addresses or
> none

Applied to ppc-for-4.2, along with your earlier full fdt render patch,
which this now unbreaks.

> 
> ---
>  hw/ppc/spapr_pci.c | 42 +++++++-----------------------------------
>  1 file changed, 7 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 6934d506a7e9..01ff41d4c43f 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -836,7 +836,7 @@ static char *spapr_phb_get_loc_code(SpaprPhbState *sphb, PCIDevice *pdev)
>  #define b_fff(x)        b_x((x), 8, 3)  /* function number */
>  #define b_rrrrrrrr(x)   b_x((x), 0, 8)  /* register number */
>  
> -/* for 'reg'/'assigned-addresses' OF properties */
> +/* for 'reg' OF properties */
>  #define RESOURCE_CELLS_SIZE 2
>  #define RESOURCE_CELLS_ADDRESS 3
>  
> @@ -850,17 +850,14 @@ typedef struct ResourceFields {
>  
>  typedef struct ResourceProps {
>      ResourceFields reg[8];
> -    ResourceFields assigned[7];
>      uint32_t reg_len;
> -    uint32_t assigned_len;
>  } ResourceProps;
>  
> -/* fill in the 'reg'/'assigned-resources' OF properties for
> +/* fill in the 'reg' OF properties for
>   * a PCI device. 'reg' describes resource requirements for a
> - * device's IO/MEM regions, 'assigned-addresses' describes the
> - * actual resource assignments.
> + * device's IO/MEM regions.
>   *
> - * the properties are arrays of ('phys-addr', 'size') pairs describing
> + * the property is an array of ('phys-addr', 'size') pairs describing
>   * the addressable regions of the PCI device, where 'phys-addr' is a
>   * RESOURCE_CELLS_ADDRESS-tuple of 32-bit integers corresponding to
>   * (phys.hi, phys.mid, phys.lo), and 'size' is a
> @@ -889,18 +886,7 @@ typedef struct ResourceProps {
>   * phys.mid and phys.lo correspond respectively to the hi/lo portions
>   * of the actual address of the region.
>   *
> - * how the phys-addr/size values are used differ slightly between
> - * 'reg' and 'assigned-addresses' properties. namely, 'reg' has
> - * an additional description for the config space region of the
> - * device, and in the case of QEMU has n=0 and phys.mid=phys.lo=0
> - * to describe the region as relocatable, with an address-mapping
> - * that corresponds directly to the PHB's address space for the
> - * resource. 'assigned-addresses' always has n=1 set with an absolute
> - * address assigned for the resource. in general, 'assigned-addresses'
> - * won't be populated, since addresses for PCI devices are generally
> - * unmapped initially and left to the guest to assign.
> - *
> - * note also that addresses defined in these properties are, at least
> + * note also that addresses defined in this property are, at least
>   * for PAPR guests, relative to the PHBs IO/MEM windows, and
>   * correspond directly to the addresses in the BARs.
>   *
> @@ -914,8 +900,8 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
>      uint32_t dev_id = (b_bbbbbbbb(bus_num) |
>                         b_ddddd(PCI_SLOT(d->devfn)) |
>                         b_fff(PCI_FUNC(d->devfn)));
> -    ResourceFields *reg, *assigned;
> -    int i, reg_idx = 0, assigned_idx = 0;
> +    ResourceFields *reg;
> +    int i, reg_idx = 0;
>  
>      /* config space region */
>      reg = &rp->reg[reg_idx++];
> @@ -944,21 +930,9 @@ static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
>          reg->phys_lo = 0;
>          reg->size_hi = cpu_to_be32(d->io_regions[i].size >> 32);
>          reg->size_lo = cpu_to_be32(d->io_regions[i].size);
> -
> -        if (d->io_regions[i].addr == PCI_BAR_UNMAPPED) {
> -            continue;
> -        }
> -
> -        assigned = &rp->assigned[assigned_idx++];
> -        assigned->phys_hi = cpu_to_be32(be32_to_cpu(reg->phys_hi) | b_n(1));
> -        assigned->phys_mid = cpu_to_be32(d->io_regions[i].addr >> 32);
> -        assigned->phys_lo = cpu_to_be32(d->io_regions[i].addr);
> -        assigned->size_hi = reg->size_hi;
> -        assigned->size_lo = reg->size_lo;
>      }
>  
>      rp->reg_len = reg_idx * sizeof(ResourceFields);
> -    rp->assigned_len = assigned_idx * sizeof(ResourceFields);
>  }
>  
>  typedef struct PCIClass PCIClass;
> @@ -1472,8 +1446,6 @@ static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
>  
>      populate_resource_props(dev, &rp);
>      _FDT(fdt_setprop(fdt, offset, "reg", (uint8_t *)rp.reg, rp.reg_len));
> -    _FDT(fdt_setprop(fdt, offset, "assigned-addresses",
> -                     (uint8_t *)rp.assigned, rp.assigned_len));
>  
>      if (sphb->pcie_ecs && pci_is_express(dev)) {
>          _FDT(fdt_setprop_cell(fdt, offset, "ibm,pci-config-space-type", 0x1));

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson