Re: [Qemu-ppc] [PATCH v4 2/2] spapr: generate DT node names

[David Gibson]

On Thu, Sep 24, 2015 at 12:27:39PM +0200, Laurent Vivier wrote:
> When DT node names for PCI devices are generated by SLOF,
> they are generated according to the type of the device
> (for instance, ethernet for virtio-net-pci device).
> 
> Node name for hotplugged devices is generated by QEMU.
> This patch adds the mechanic to QEMU to create the node
> name according to the device type too.
> 
> The data structure has been roughly copied from OpenBIOS/OpenHackware,
> node names from SLOF.
> 
> Example:
> 
> Hotplugging some PCI cards with QEMU monitor:
> 
> device_add virtio-tablet-pci
> device_add virtio-serial-pci
> device_add virtio-mouse-pci
> device_add virtio-scsi-pci
> device_add virtio-gpu-pci
> device_add ne2k_pci
> device_add nec-usb-xhci
> device_add intel-hda
> 
> What we can see in linux device tree:
> 
> for dir in /proc/device-tree/address@hidden/address@hidden/; do
>     echo $dir
>     cat $dir/name
>     echo
> done
> 
> WITHOUT this patch:
> 
> /proc/device-tree/address@hidden/address@hidden/
> pci
> /proc/device-tree/address@hidden/address@hidden/
> pci
> /proc/device-tree/address@hidden/address@hidden/
> pci
> /proc/device-tree/address@hidden/address@hidden/
> pci
> /proc/device-tree/address@hidden/address@hidden/
> pci
> /proc/device-tree/address@hidden/address@hidden/
> pci
> /proc/device-tree/address@hidden/address@hidden/
> pci
> /proc/device-tree/address@hidden/address@hidden/
> pci
> 
> WITH this patch:
> 
> /proc/device-tree/address@hidden/address@hidden/
> communication-controller
> /proc/device-tree/address@hidden/address@hidden/
> display
> /proc/device-tree/address@hidden/address@hidden/
> ethernet
> /proc/device-tree/address@hidden/address@hidden/
> input-controller
> /proc/device-tree/address@hidden/address@hidden/
> mouse
> /proc/device-tree/address@hidden/address@hidden/
> multimedia-device
> /proc/device-tree/address@hidden/address@hidden/
> scsi
> /proc/device-tree/address@hidden/address@hidden/
> usb-xhci
> 
> Signed-off-by: Laurent Vivier <address@hidden>
> Reviewed-by: Thomas Huth <address@hidden>

Concept and approach seem good to me.


> ---
>  hw/ppc/spapr_pci.c | 292 
> ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 278 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index a2feb4c..63eb28c 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -38,6 +38,7 @@
>  
>  #include "hw/pci/pci_bridge.h"
>  #include "hw/pci/pci_bus.h"
> +#include "hw/pci/pci_ids.h"
>  #include "hw/ppc/spapr_drc.h"
>  #include "sysemu/device_tree.h"
>  
> @@ -944,6 +945,276 @@ static void populate_resource_props(PCIDevice *d, 
> ResourceProps *rp)
>      rp->assigned_len = assigned_idx * sizeof(ResourceFields);
>  }
>  
> +typedef struct PCIClass PCIClass;
> +typedef struct PCISubClass PCISubClass;
> +typedef struct PCIIFace PCIIFace;
> +
> +struct PCIIFace {
> +    uint8_t iface;
> +    const char *name;
> +};
> +
> +struct PCISubClass {
> +    uint8_t subclass;
> +    const char *name;
> +    const PCIIFace *iface;
> +};
> +#define SUBCLASS(a) ((uint8_t)a)
> +#define IFACE(a)    ((uint8_t)a)
> +
> +struct PCIClass {
> +    const char *name;
> +    const PCISubClass *subc;
> +};
> +
> +static const PCISubClass undef_subclass[] = {
> +    { IFACE(PCI_CLASS_NOT_DEFINED_VGA), "display", NULL },

These IFACE and SUBCLASS macro calls seem ugly to me.  What's the
purpose of them?

[snip]
> +static const char *pci_find_device_name(uint8_t class, uint8_t subclass,
> +                                        uint8_t iface)
> +{
> +    const PCIClass *pclass;
> +    const PCISubClass *psubclass;
> +    const PCIIFace *piface;
> +    const char *name;
> +
> +    if (class >= ARRAY_SIZE(pci_classes)) {
> +        return "pci";
> +    }
> +
> +    pclass = pci_classes + class;
> +    name = pclass->name;
> +
> +    if (pclass->subc == NULL) {
> +        return name;
> +    }
> +
> +    psubclass = pclass->subc;
> +    while (psubclass->subclass != 0xff) {
> +        if (psubclass->subclass == subclass) {
> +            name = psubclass->name;
> +            break;
> +        }
> +        psubclass++;
> +    }
> +
> +    piface = psubclass->iface;
> +    if (piface == NULL) {
> +        return name;
> +    }
> +    while (piface->iface != 0xff) {
> +        if (piface->iface == iface) {
> +            name = piface->name;
> +            break;
> +        }
> +        piface++;
> +    }
> +
> +    return name;
> +}
> +
> +static void pci_get_node_name(char *nodename, int len, PCIDevice *dev)
> +{
> +    int slot = PCI_SLOT(dev->devfn);
> +    int func = PCI_FUNC(dev->devfn);
> +    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> +    const char *name;
> +
> +    name = pci_find_device_name((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> +                                ccode & 0xff);
> +
> +    if (func != 0) {
> +        snprintf(nodename, len, "address@hidden,%x", name, slot, func);
> +    } else {
> +        snprintf(nodename, len, "address@hidden", name, slot);
> +    }
> +}
> +
>  static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb,
>                                              PCIDevice *pdev);
>  
> @@ -955,6 +1226,7 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, 
> void *fdt, int offset,
>      int pci_status, err;
>      char *buf = NULL;
>      uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
> +    uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
>  
>      if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
>          PCI_HEADER_TYPE_BRIDGE) {
> @@ -968,8 +1240,7 @@ static int spapr_populate_pci_child_dt(PCIDevice *dev, 
> void *fdt, int offset,
>                            pci_default_read_config(dev, PCI_DEVICE_ID, 2)));
>      _FDT(fdt_setprop_cell(fdt, offset, "revision-id",
>                            pci_default_read_config(dev, PCI_REVISION_ID, 1)));
> -    _FDT(fdt_setprop_cell(fdt, offset, "class-code",
> -                          pci_default_read_config(dev, PCI_CLASS_PROG, 3)));
> +    _FDT(fdt_setprop_cell(fdt, offset, "class-code", ccode));
>      if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) {
>          _FDT(fdt_setprop_cell(fdt, offset, "interrupts",
>                   pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)));
> @@ -1010,11 +1281,10 @@ static int spapr_populate_pci_child_dt(PCIDevice 
> *dev, void *fdt, int offset,
>          _FDT(fdt_setprop(fdt, offset, "udf-supported", NULL, 0));
>      }
>  
> -    /* NOTE: this is normally generated by firmware via path/unit name,
> -     * but in our case we must set it manually since it does not get
> -     * processed by OF beforehand
> -     */
> -    _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
> +    _FDT(fdt_setprop_string(fdt, offset, "name",
> +                            pci_find_device_name((ccode >> 16) & 0xff,
> +                                                 (ccode >> 8) & 0xff,
> +                                                 ccode & 0xff)));

Heh.  This isn't wrong, but there's a non-obvious wrinkle as to why.
Generally flattened trees shouldn't require the 'name' property (the
consumer is supposed to infer that from the node name).  However,
since this may also be used to generate tree fragments which get
passed through the PAPR dynamic reconfiguration interface, and that
*does* expect the name property to be passed through.

>      buf = spapr_phb_get_loc_code(sphb, dev);
>      if (!buf) {
>          error_report("Failed setting the ibm,loc-code");
> @@ -1051,15 +1321,9 @@ static int spapr_create_pci_child_dt(sPAPRPHBState 
> *phb, PCIDevice *dev,
>                                       void *fdt, int node_offset)
>  {
>      int offset, ret;
> -    int slot = PCI_SLOT(dev->devfn);
> -    int func = PCI_FUNC(dev->devfn);
>      char nodename[FDT_NAME_MAX];
>  
> -    if (func != 0) {
> -        snprintf(nodename, FDT_NAME_MAX, "address@hidden,%x", slot, func);
> -    } else {
> -        snprintf(nodename, FDT_NAME_MAX, "address@hidden", slot);
> -    }
> +    pci_get_node_name(nodename, FDT_NAME_MAX, dev);
>      offset = fdt_add_subnode(fdt, node_offset, nodename);
>      ret = spapr_populate_pci_child_dt(dev, fdt, offset, phb);
>  

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

pgpD1aSsirrj8.pgp
Description: PGP signature