Re: [Qemu-devel] [PATCH v3] spapr: generate DT node names

[Thomas Huth]

On 23/09/15 14:14, 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.
[...]
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index a2feb4c..c521d31 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,280 @@ 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 },
> +    { 0xFF, NULL, NULL, NULL },
> +};
> +
> +static const PCISubClass mass_subclass[] = {
> +    { SUBCLASS(PCI_CLASS_STORAGE_SCSI), "scsi", NULL },
> +    { SUBCLASS(PCI_CLASS_STORAGE_IDE), "ide", NULL },
> +    { SUBCLASS(PCI_CLASS_STORAGE_FLOPPY), "fdc", NULL },
> +    { SUBCLASS(PCI_CLASS_STORAGE_IPI), "ipi", NULL },
> +    { SUBCLASS(PCI_CLASS_STORAGE_RAID), "raid", NULL },
> +    { SUBCLASS(PCI_CLASS_STORAGE_ATA), "ata", NULL },
> +    { SUBCLASS(PCI_CLASS_STORAGE_SATA), "sata", NULL },
> +    { SUBCLASS(PCI_CLASS_STORAGE_SAS), "sas", NULL },
> +    { 0xFF, NULL, NULL },
> +};
> +
> +static const PCISubClass net_subclass[] = {
> +    { SUBCLASS(PCI_CLASS_NETWORK_ETHERNET), "ethernet", NULL },
> +    { SUBCLASS(PCI_CLASS_NETWORK_TOKEN_RING), "token-ring", NULL },
> +    { SUBCLASS(PCI_CLASS_NETWORK_FDDI), "fddi", NULL },
> +    { SUBCLASS(PCI_CLASS_NETWORK_ATM), "atm", NULL },
> +    { SUBCLASS(PCI_CLASS_NETWORK_ISDN), "isdn", NULL },
> +    { SUBCLASS(PCI_CLASS_NETWORK_WORDFIP), "worldfip", NULL },
> +    { SUBCLASS(PCI_CLASS_NETWORK_PICMG214), "picmg", NULL },
> +    { 0xFF, NULL, NULL },
> +};
> +
> +static const PCISubClass displ_subclass[] = {
> +    { SUBCLASS(PCI_CLASS_DISPLAY_VGA), "vga", NULL },
> +    { SUBCLASS(PCI_CLASS_DISPLAY_XGA), "xga", NULL },
> +    { SUBCLASS(PCI_CLASS_DISPLAY_3D), "3d-controller", NULL },
> +    { 0xFF, NULL, NULL },
> +};
> +
> +static const PCISubClass media_subclass[] = {
> +    { SUBCLASS(PCI_CLASS_MULTIMEDIA_VIDEO), "video", NULL },
> +    { SUBCLASS(PCI_CLASS_MULTIMEDIA_AUDIO), "sound", NULL },
> +    { SUBCLASS(PCI_CLASS_MULTIMEDIA_PHONE), "telephony", NULL },
> +    { 0xFF, NULL, NULL },
> +};
> +
> +static const PCISubClass mem_subclass[] = {
> +    { SUBCLASS(PCI_CLASS_MEMORY_RAM), "memory", NULL },
> +    { SUBCLASS(PCI_CLASS_MEMORY_FLASH), "flash", NULL },
> +    { 0xFF, NULL, NULL },
> +};
> +
> +

One new-line should be sufficient.

[...]
> +static const PCISubClass cpu_subclass[] = {
> +    { SUBCLASS(PCI_CLASS_PROCESSOR_386), "386", NULL },
> +    { SUBCLASS(PCI_CLASS_PROCESSOR_486), "486", NULL },
> +    { SUBCLASS(PCI_CLASS_PROCESSOR_PENTIUM), "pentium", NULL },
> +    { SUBCLASS(PCI_CLASS_PROCESSOR_ALPHA), "alpha", NULL },
> +    { SUBCLASS(PCI_CLASS_PROCESSOR_POWERPC), "powerpc", NULL },
> +    { SUBCLASS(PCI_CLASS_PROCESSOR_MIPS), "mips", NULL },
> +    { SUBCLASS(PCI_CLASS_PROCESSOR_CO), "co-processor", NULL },
> +    { 0xFF, NULL, NULL },
> +};

I really really doubt that we'll ever see such a device on the spapr PCI
bus ... could you at least omit entries like "386", "486" and "alpha" ?

[...]
> +
> +static const PCISubClass spc_subclass[] = {
> +    { SUBCLASS(PCI_CLASS_SP_DPIO), "dpio", NULL },
> +    { SUBCLASS(PCI_CLASS_SP_PERF), "counter", NULL },
> +    { SUBCLASS(PCI_CLASS_SP_SYNCH), "measurement", NULL },
> +    { SUBCLASS(PCI_CLASS_SP_MANAGEMENT), "management-card", NULL },
> +    { 0xFF, NULL, NULL },
> +};
> +
> +static const PCIClass pci_classes[] = {
> +    { "unknown-legacy-device", undef_subclass },

Maybe just "legacy-device" instead of "unknown-legacy-device" ?

> +    { "mass-storage",  mass_subclass },
> +    { "network", net_subclass },
> +    { "display", displ_subclass, },
> +    { "multimedia-device", media_subclass },
> +    { "memory-controller", mem_subclass },
> +    { "unknown-bridge", bridg_subclass },
> +    { "communication-controller", comm_subclass},
> +    { "system-peripheral", sys_subclass },
> +    { "input-controller", inp_subclass },
> +    { "docking-station", dock_subclass },
> +    { "cpu", cpu_subclass },
> +    { "serial-bus", ser_subclass },
> +    { "wireless-controller", wrl_subclass },
> +    { "intelligent-io", NULL },
> +    { "satellite-device", sat_subclass },
> +    { "encryption", crypt_subclass },
> +    { "data-processing-controller", spc_subclass },
> +};
> +
> +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 > (sizeof(pci_classes) / sizeof(PCIClass))) {

I think you could also use the ARRAY_SIZE macro here.

And shouldn't that rather be ">=" instead of ">" ?

> +        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;
> +}
[...]
> diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
> index d98e6c9..42c86df 100644
> --- a/include/hw/pci/pci_ids.h
> +++ b/include/hw/pci/pci_ids.h
> @@ -12,41 +12,84 @@
>  
>  /* Device classes and subclasses */
>  
> -#define PCI_BASE_CLASS_STORAGE           0x01
> -#define PCI_BASE_CLASS_NETWORK           0x02
> +#define PCI_CLASS_NOT_DEFINED            0x0000
> +#define PCI_CLASS_NOT_DEFINED_VGA        0x0001
>  
> +#define PCI_BASE_CLASS_STORAGE           0x01
>  #define PCI_CLASS_STORAGE_SCSI           0x0100
>  #define PCI_CLASS_STORAGE_IDE            0x0101
> +#define PCI_CLASS_STORAGE_FLOPPY         0x0102
> +#define PCI_CLASS_STORAGE_IPI            0x0103
>  #define PCI_CLASS_STORAGE_RAID           0x0104
> +#define PCI_CLASS_STORAGE_ATA            0x0105
>  #define PCI_CLASS_STORAGE_SATA           0x0106
> +#define PCI_CLASS_STORAGE_SAS            0x0107
>  #define PCI_CLASS_STORAGE_EXPRESS        0x0108
>  #define PCI_CLASS_STORAGE_OTHER          0x0180
>  
> +#define PCI_BASE_CLASS_NETWORK           0x02
>  #define PCI_CLASS_NETWORK_ETHERNET       0x0200
> +#define PCI_CLASS_NETWORK_TOKEN_RING     0x0201
> +#define PCI_CLASS_NETWORK_FDDI           0x0202
> +#define PCI_CLASS_NETWORK_ATM            0x0203
> +#define PCI_CLASS_NETWORK_ISDN           0x0204
> +#define PCI_CLASS_NETWORK_WORDFIP        0x0205

I think it's WORLDFIP, not WORDFIP.

Also you should maybe put the updates to pci_ids.h into a separate
patch, so that the PCI maintainer can ACK it separately?

... apart from that, the patch looks very fine to me now, so if you've
at least address the ">=" vs. ">" issue, feel free to add my
"Reviewed-by: Thomas Huth <address@hidden>"

 Thomas