Re: [Qemu-devel] [Qemu-ppc] [PATCH v8 15/16] spapr_pci: enable basic hotplug operations

[Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>]

Hi Michael,

Couple of issues I found during my testing with pci enumerations patches
yesterday:

Michael Roth <mdroth@linux.vnet.ibm.com> writes:

> This enables hotplug of PCI devices to a PHB. Upon hotplug we
> generate the OF-nodes required by PAPR specification and
> IEEE 1275-1994 "PCI Bus Binding to Open Firmware" for the
> device.
>
> We associate the corresponding FDT for these nodes with the DRC
> corresponding to the slot, which will be fetched via
> ibm,configure-connector RTAS calls by the guest as described by PAPR
> specification.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c | 388 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 380 insertions(+), 8 deletions(-)
>
[SNIP]
> +
> +/* fill in the 'reg'/'assigned-resources' OF properties for
> + * a PCI device. 'reg' describes resource requirements for a
> + * device's IO/MEM regions, 'assigned-addresses' describes the
> + * actual resource assignments.
> + *
> + * the properties are arrays 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
> + * RESOURCE_CELLS_SIZE-tuple corresponding to (size.hi, size.lo).
> + *
> + * phys.hi = 0xYYXXXXZZ, where:
> + *   0xYY = npt000ss
> + *          |||   |
> + *          |||   +-- space code: 1 if IO region, 2 if MEM region

The above is missing 64-bit memory space

00  configuration space 
01  I/O space
10  memory space 32-bit address
11  memory space 64-bit address

> + *          ||+------ for non-relocatable IO: 1 if aliased
> + *          ||        for relocatable IO: 1 if below 64KB
> + *          ||        for MEM: 1 if below 1MB
> + *          |+------- 1 if region is prefetchable
> + *          +-------- 1 if region is non-relocatable
> + *   0xXXXX = bbbbbbbb dddddfff, encoding bus, slot, and function
> + *            bits respectively
> + *   0xZZ = rrrrrrrr, the register number of the BAR corresponding
> + *          to the region
> + *

[SNIP]

> +static void populate_resource_props(PCIDevice *d, ResourceProps *rp)
> +{
> +    int bus_num = pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(d))));
> +    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;
> +
> +    /* config space region */
> +    reg = &rp->reg[reg_idx++];
> +    reg->phys_hi = cpu_to_be32(dev_id);
> +    reg->phys_mid = 0;
> +    reg->phys_lo = 0;
> +    reg->size_hi = 0;
> +    reg->size_lo = 0;
> +
> +    for (i = 0; i < PCI_NUM_REGIONS; i++) {
> +        if (!d->io_regions[i].size) {
> +            continue;
> +        }
> +
> +        reg = &rp->reg[reg_idx++];
> +
> +        reg->phys_hi = cpu_to_be32(dev_id | b_rrrrrrrr(pci_bar(d, i)));
> +        if (d->io_regions[i].type & PCI_BASE_ADDRESS_SPACE_IO) {
> +            reg->phys_hi |= cpu_to_be32(b_ss(1));
> +        } else {
> +            reg->phys_hi |= cpu_to_be32(b_ss(2));
> +        }

We need to identify 64-bit memory space here.


> +    _FDT(fdt_setprop_cell(fdt, offset, "class-code",
> +                          pci_default_read_config(dev, PCI_CLASS_DEVICE, 2)
> +                            << 8));

Complete class code needs to be encoded in the "class-code" property:

+                         pci_default_read_config(dev, PCI_CLASS_PROG, 3)));


Regards,
Nikunj