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 <th...@redhat.com>"
Thomas
