[v4,2/2] spapr: generate DT node names

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/pci@800000020000000/*@*/; do
    echo $dir
    cat $dir/name
    echo
done

WITHOUT this patch:

/proc/device-tree/pci@800000020000000/pci@0/
pci
/proc/device-tree/pci@800000020000000/pci@1/
pci
/proc/device-tree/pci@800000020000000/pci@2/
pci
/proc/device-tree/pci@800000020000000/pci@3/
pci
/proc/device-tree/pci@800000020000000/pci@4/
pci
/proc/device-tree/pci@800000020000000/pci@5/
pci
/proc/device-tree/pci@800000020000000/pci@6/
pci
/proc/device-tree/pci@800000020000000/pci@7/
pci

WITH this patch:

/proc/device-tree/pci@800000020000000/communication-controller@1/
communication-controller
/proc/device-tree/pci@800000020000000/display@4/
display
/proc/device-tree/pci@800000020000000/ethernet@5/
ethernet
/proc/device-tree/pci@800000020000000/input-controller@0/
input-controller
/proc/device-tree/pci@800000020000000/mouse@2/
mouse
/proc/device-tree/pci@800000020000000/multimedia-device@7/
multimedia-device
/proc/device-tree/pci@800000020000000/scsi@3/
scsi
/proc/device-tree/pci@800000020000000/usb-xhci@6/
usb-xhci

Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 hw/ppc/spapr_pci.c | 292 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 278 insertions(+), 14 deletions(-)

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/pci@800000020000000/*@*/; do
>    echo $dir
>    cat $dir/name
>    echo
>done
>
>WITHOUT this patch:
>
>/proc/device-tree/pci@800000020000000/pci@0/
>pci
>/proc/device-tree/pci@800000020000000/pci@1/
>pci
>/proc/device-tree/pci@800000020000000/pci@2/
>pci
>/proc/device-tree/pci@800000020000000/pci@3/
>pci
>/proc/device-tree/pci@800000020000000/pci@4/
>pci
>/proc/device-tree/pci@800000020000000/pci@5/
>pci
>/proc/device-tree/pci@800000020000000/pci@6/
>pci
>/proc/device-tree/pci@800000020000000/pci@7/
>pci
>
>WITH this patch:
>
>/proc/device-tree/pci@800000020000000/communication-controller@1/
>communication-controller
>/proc/device-tree/pci@800000020000000/display@4/
>display
>/proc/device-tree/pci@800000020000000/ethernet@5/
>ethernet
>/proc/device-tree/pci@800000020000000/input-controller@0/
>input-controller
>/proc/device-tree/pci@800000020000000/mouse@2/
>mouse
>/proc/device-tree/pci@800000020000000/multimedia-device@7/
>multimedia-device
>/proc/device-tree/pci@800000020000000/scsi@3/
>scsi
>/proc/device-tree/pci@800000020000000/usb-xhci@6/
>usb-xhci
>
>Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>Reviewed-by: Thomas Huth <thuth@redhat.com>
>---
> 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);
> }
>

One question would be: is there one reason why the logic, converting
class/subclass/iface code to tring, isn't put into generic PCI module?
If the code is put there, all platforms can reuse it.

Thanks,
Gavin

>+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_WORLDFIP), "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 },
>+};
>+
>+static const PCISubClass bridg_subclass[] = {
>+    { SUBCLASS(PCI_CLASS_BRIDGE_HOST), "host", NULL },
>+    { SUBCLASS(PCI_CLASS_BRIDGE_ISA), "isa", NULL },
>+    { SUBCLASS(PCI_CLASS_BRIDGE_EISA), "eisa", NULL },
>+    { SUBCLASS(PCI_CLASS_BRIDGE_MC), "mca", NULL },
>+    { SUBCLASS(PCI_CLASS_BRIDGE_PCI), "pci", NULL },
>+    { SUBCLASS(PCI_CLASS_BRIDGE_PCMCIA), "pcmcia", NULL },
>+    { SUBCLASS(PCI_CLASS_BRIDGE_NUBUS), "nubus", NULL },
>+    { SUBCLASS(PCI_CLASS_BRIDGE_CARDBUS), "cardbus", NULL },
>+    { SUBCLASS(PCI_CLASS_BRIDGE_RACEWAY), "raceway", NULL },
>+    { SUBCLASS(PCI_CLASS_BRIDGE_PCI_SEMITP), "semi-transparent-pci", NULL },
>+    { SUBCLASS(PCI_CLASS_BRIDGE_IB_PCI), "infiniband", NULL },
>+    { 0xFF, NULL, NULL },
>+};
>+
>+static const PCISubClass comm_subclass[] = {
>+    { SUBCLASS(PCI_CLASS_COMMUNICATION_SERIAL), "serial", NULL },
>+    { SUBCLASS(PCI_CLASS_COMMUNICATION_PARALLEL), "parallel", NULL },
>+    { SUBCLASS(PCI_CLASS_COMMUNICATION_MULTISERIAL), "multiport-serial", NULL },
>+    { SUBCLASS(PCI_CLASS_COMMUNICATION_MODEM), "modem", NULL },
>+    { SUBCLASS(PCI_CLASS_COMMUNICATION_GPIB), "gpib", NULL },
>+    { SUBCLASS(PCI_CLASS_COMMUNICATION_SC), "smart-card", NULL },
>+    { 0xFF, NULL, NULL, NULL },
>+};
>+
>+static const PCIIFace pic_iface[] = {
>+    { IFACE(PCI_CLASS_SYSTEM_PIC_IOAPIC), "io-apic" },
>+    { IFACE(PCI_CLASS_SYSTEM_PIC_IOXAPIC), "io-xapic" },
>+    { 0xFF, NULL },
>+};
>+
>+static const PCISubClass sys_subclass[] = {
>+    { SUBCLASS(PCI_CLASS_SYSTEM_PIC), "interrupt-controller", pic_iface },
>+    { SUBCLASS(PCI_CLASS_SYSTEM_DMA), "dma-controller", NULL },
>+    { SUBCLASS(PCI_CLASS_SYSTEM_TIMER), "timer", NULL },
>+    { SUBCLASS(PCI_CLASS_SYSTEM_RTC), "rtc", NULL },
>+    { SUBCLASS(PCI_CLASS_SYSTEM_PCI_HOTPLUG), "hot-plug-controller", NULL },
>+    { SUBCLASS(PCI_CLASS_SYSTEM_SDHCI), "sd-host-controller", NULL },
>+    { 0xFF, NULL, NULL },
>+};
>+
>+static const PCISubClass inp_subclass[] = {
>+    { SUBCLASS(PCI_CLASS_INPUT_KEYBOARD), "keyboard", NULL },
>+    { SUBCLASS(PCI_CLASS_INPUT_PEN), "pen", NULL },
>+    { SUBCLASS(PCI_CLASS_INPUT_MOUSE), "mouse", NULL },
>+    { SUBCLASS(PCI_CLASS_INPUT_SCANNER), "scanner", NULL },
>+    { SUBCLASS(PCI_CLASS_INPUT_GAMEPORT), "gameport", NULL },
>+    { 0xFF, NULL, NULL },
>+};
>+
>+static const PCISubClass dock_subclass[] = {
>+    { SUBCLASS(PCI_CLASS_DOCKING_GENERIC), "dock", NULL },
>+    { 0xFF, NULL, NULL },
>+};
>+
>+static const PCISubClass cpu_subclass[] = {
>+    { SUBCLASS(PCI_CLASS_PROCESSOR_PENTIUM), "pentium", 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 },
>+};
>+
>+static const PCIIFace usb_iface[] = {
>+    { IFACE(PCI_CLASS_SERIAL_USB_UHCI), "usb-uhci" },
>+    { IFACE(PCI_CLASS_SERIAL_USB_OHCI), "usb-ohci", },
>+    { IFACE(PCI_CLASS_SERIAL_USB_EHCI), "usb-ehci" },
>+    { IFACE(PCI_CLASS_SERIAL_USB_XHCI), "usb-xhci" },
>+    { IFACE(PCI_CLASS_SERIAL_USB_UNKNOWN), "usb-unknown" },
>+    { IFACE(PCI_CLASS_SERIAL_USB_DEVICE), "usb-device" },
>+    { 0xFF, NULL },
>+};
>+
>+static const PCISubClass ser_subclass[] = {
>+    { SUBCLASS(PCI_CLASS_SERIAL_FIREWIRE), "firewire", NULL },
>+    { SUBCLASS(PCI_CLASS_SERIAL_ACCESS), "access-bus", NULL },
>+    { SUBCLASS(PCI_CLASS_SERIAL_SSA), "ssa", NULL },
>+    { SUBCLASS(PCI_CLASS_SERIAL_USB), "usb", usb_iface },
>+    { SUBCLASS(PCI_CLASS_SERIAL_FIBER), "fibre-channel", NULL },
>+    { SUBCLASS(PCI_CLASS_SERIAL_SMBUS), "smb", NULL },
>+    { SUBCLASS(PCI_CLASS_SERIAL_IB), "infiniband", NULL },
>+    { SUBCLASS(PCI_CLASS_SERIAL_IPMI), "ipmi", NULL },
>+    { SUBCLASS(PCI_CLASS_SERIAL_SERCOS), "sercos", NULL },
>+    { SUBCLASS(PCI_CLASS_SERIAL_CANBUS), "canbus", NULL },
>+    { 0xFF, NULL, NULL },
>+};
>+
>+static const PCISubClass wrl_subclass[] = {
>+    { SUBCLASS(PCI_CLASS_WIRELESS_IRDA), "irda", NULL },
>+    { SUBCLASS(PCI_CLASS_WIRELESS_CIR), "consumer-ir", NULL },
>+    { SUBCLASS(PCI_CLASS_WIRELESS_RF_CONTROLLER), "rf-controller", NULL },
>+    { SUBCLASS(PCI_CLASS_WIRELESS_BLUETOOTH), "bluetooth", NULL },
>+    { SUBCLASS(PCI_CLASS_WIRELESS_BROADBAND), "broadband", NULL },
>+    { 0xFF, NULL, NULL },
>+};
>+
>+static const PCISubClass sat_subclass[] = {
>+    { SUBCLASS(PCI_CLASS_SATELLITE_TV), "satellite-tv", NULL },
>+    { SUBCLASS(PCI_CLASS_SATELLITE_AUDIO), "satellite-audio", NULL },
>+    { SUBCLASS(PCI_CLASS_SATELLITE_VOICE), "satellite-voice", NULL },
>+    { SUBCLASS(PCI_CLASS_SATELLITE_DATA), "satellite-data", NULL },
>+    { 0xFF, NULL, NULL },
>+};
>+
>+static const PCISubClass crypt_subclass[] = {
>+    { SUBCLASS(PCI_CLASS_CRYPT_NETWORK), "network-encryption", NULL },
>+    { SUBCLASS(PCI_CLASS_CRYPT_ENTERTAINMENT),
>+      "entertainment-encryption", NULL },
>+    { 0xFF, NULL, NULL },
>+};
>+
>+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[] = {
>+    { "legacy-device", undef_subclass },
>+    { "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 >= 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, "%s@%x,%x", name, slot, func);
>+    } else {
>+        snprintf(nodename, len, "%s@%x", 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)));
>     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, "pci@%x,%x", slot, func);
>-    } else {
>-        snprintf(nodename, FDT_NAME_MAX, "pci@%x", 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);
>
>-- 
>2.4.3
>
>

On 25/09/2015 01:29, Gavin Shan wrote:
> 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/pci@800000020000000/*@*/; do
>>    echo $dir
>>    cat $dir/name
>>    echo
>> done
>>
>> WITHOUT this patch:
>>
>> /proc/device-tree/pci@800000020000000/pci@0/
>> pci
>> /proc/device-tree/pci@800000020000000/pci@1/
>> pci
>> /proc/device-tree/pci@800000020000000/pci@2/
>> pci
>> /proc/device-tree/pci@800000020000000/pci@3/
>> pci
>> /proc/device-tree/pci@800000020000000/pci@4/
>> pci
>> /proc/device-tree/pci@800000020000000/pci@5/
>> pci
>> /proc/device-tree/pci@800000020000000/pci@6/
>> pci
>> /proc/device-tree/pci@800000020000000/pci@7/
>> pci
>>
>> WITH this patch:
>>
>> /proc/device-tree/pci@800000020000000/communication-controller@1/
>> communication-controller
>> /proc/device-tree/pci@800000020000000/display@4/
>> display
>> /proc/device-tree/pci@800000020000000/ethernet@5/
>> ethernet
>> /proc/device-tree/pci@800000020000000/input-controller@0/
>> input-controller
>> /proc/device-tree/pci@800000020000000/mouse@2/
>> mouse
>> /proc/device-tree/pci@800000020000000/multimedia-device@7/
>> multimedia-device
>> /proc/device-tree/pci@800000020000000/scsi@3/
>> scsi
>> /proc/device-tree/pci@800000020000000/usb-xhci@6/
>> usb-xhci
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> ---
>> 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);
>> }
>>
> 
> One question would be: is there one reason why the logic, converting
> class/subclass/iface code to tring, isn't put into generic PCI module?
> If the code is put there, all platforms can reuse it.

For the moment, it is only used by the device tree generation for spapr,
and moreover the names are ones from the openfirmware specification, so
except if openbios on sparc/macintosh takes its device tree from QEMU I
see no reason to put this in a generic PCI module.

Laurent
> 
> Thanks,
> Gavin
> 
>> +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_WORLDFIP), "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 },
>> +};
>> +
>> +static const PCISubClass bridg_subclass[] = {
>> +    { SUBCLASS(PCI_CLASS_BRIDGE_HOST), "host", NULL },
>> +    { SUBCLASS(PCI_CLASS_BRIDGE_ISA), "isa", NULL },
>> +    { SUBCLASS(PCI_CLASS_BRIDGE_EISA), "eisa", NULL },
>> +    { SUBCLASS(PCI_CLASS_BRIDGE_MC), "mca", NULL },
>> +    { SUBCLASS(PCI_CLASS_BRIDGE_PCI), "pci", NULL },
>> +    { SUBCLASS(PCI_CLASS_BRIDGE_PCMCIA), "pcmcia", NULL },
>> +    { SUBCLASS(PCI_CLASS_BRIDGE_NUBUS), "nubus", NULL },
>> +    { SUBCLASS(PCI_CLASS_BRIDGE_CARDBUS), "cardbus", NULL },
>> +    { SUBCLASS(PCI_CLASS_BRIDGE_RACEWAY), "raceway", NULL },
>> +    { SUBCLASS(PCI_CLASS_BRIDGE_PCI_SEMITP), "semi-transparent-pci", NULL },
>> +    { SUBCLASS(PCI_CLASS_BRIDGE_IB_PCI), "infiniband", NULL },
>> +    { 0xFF, NULL, NULL },
>> +};
>> +
>> +static const PCISubClass comm_subclass[] = {
>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_SERIAL), "serial", NULL },
>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_PARALLEL), "parallel", NULL },
>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_MULTISERIAL), "multiport-serial", NULL },
>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_MODEM), "modem", NULL },
>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_GPIB), "gpib", NULL },
>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_SC), "smart-card", NULL },
>> +    { 0xFF, NULL, NULL, NULL },
>> +};
>> +
>> +static const PCIIFace pic_iface[] = {
>> +    { IFACE(PCI_CLASS_SYSTEM_PIC_IOAPIC), "io-apic" },
>> +    { IFACE(PCI_CLASS_SYSTEM_PIC_IOXAPIC), "io-xapic" },
>> +    { 0xFF, NULL },
>> +};
>> +
>> +static const PCISubClass sys_subclass[] = {
>> +    { SUBCLASS(PCI_CLASS_SYSTEM_PIC), "interrupt-controller", pic_iface },
>> +    { SUBCLASS(PCI_CLASS_SYSTEM_DMA), "dma-controller", NULL },
>> +    { SUBCLASS(PCI_CLASS_SYSTEM_TIMER), "timer", NULL },
>> +    { SUBCLASS(PCI_CLASS_SYSTEM_RTC), "rtc", NULL },
>> +    { SUBCLASS(PCI_CLASS_SYSTEM_PCI_HOTPLUG), "hot-plug-controller", NULL },
>> +    { SUBCLASS(PCI_CLASS_SYSTEM_SDHCI), "sd-host-controller", NULL },
>> +    { 0xFF, NULL, NULL },
>> +};
>> +
>> +static const PCISubClass inp_subclass[] = {
>> +    { SUBCLASS(PCI_CLASS_INPUT_KEYBOARD), "keyboard", NULL },
>> +    { SUBCLASS(PCI_CLASS_INPUT_PEN), "pen", NULL },
>> +    { SUBCLASS(PCI_CLASS_INPUT_MOUSE), "mouse", NULL },
>> +    { SUBCLASS(PCI_CLASS_INPUT_SCANNER), "scanner", NULL },
>> +    { SUBCLASS(PCI_CLASS_INPUT_GAMEPORT), "gameport", NULL },
>> +    { 0xFF, NULL, NULL },
>> +};
>> +
>> +static const PCISubClass dock_subclass[] = {
>> +    { SUBCLASS(PCI_CLASS_DOCKING_GENERIC), "dock", NULL },
>> +    { 0xFF, NULL, NULL },
>> +};
>> +
>> +static const PCISubClass cpu_subclass[] = {
>> +    { SUBCLASS(PCI_CLASS_PROCESSOR_PENTIUM), "pentium", 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 },
>> +};
>> +
>> +static const PCIIFace usb_iface[] = {
>> +    { IFACE(PCI_CLASS_SERIAL_USB_UHCI), "usb-uhci" },
>> +    { IFACE(PCI_CLASS_SERIAL_USB_OHCI), "usb-ohci", },
>> +    { IFACE(PCI_CLASS_SERIAL_USB_EHCI), "usb-ehci" },
>> +    { IFACE(PCI_CLASS_SERIAL_USB_XHCI), "usb-xhci" },
>> +    { IFACE(PCI_CLASS_SERIAL_USB_UNKNOWN), "usb-unknown" },
>> +    { IFACE(PCI_CLASS_SERIAL_USB_DEVICE), "usb-device" },
>> +    { 0xFF, NULL },
>> +};
>> +
>> +static const PCISubClass ser_subclass[] = {
>> +    { SUBCLASS(PCI_CLASS_SERIAL_FIREWIRE), "firewire", NULL },
>> +    { SUBCLASS(PCI_CLASS_SERIAL_ACCESS), "access-bus", NULL },
>> +    { SUBCLASS(PCI_CLASS_SERIAL_SSA), "ssa", NULL },
>> +    { SUBCLASS(PCI_CLASS_SERIAL_USB), "usb", usb_iface },
>> +    { SUBCLASS(PCI_CLASS_SERIAL_FIBER), "fibre-channel", NULL },
>> +    { SUBCLASS(PCI_CLASS_SERIAL_SMBUS), "smb", NULL },
>> +    { SUBCLASS(PCI_CLASS_SERIAL_IB), "infiniband", NULL },
>> +    { SUBCLASS(PCI_CLASS_SERIAL_IPMI), "ipmi", NULL },
>> +    { SUBCLASS(PCI_CLASS_SERIAL_SERCOS), "sercos", NULL },
>> +    { SUBCLASS(PCI_CLASS_SERIAL_CANBUS), "canbus", NULL },
>> +    { 0xFF, NULL, NULL },
>> +};
>> +
>> +static const PCISubClass wrl_subclass[] = {
>> +    { SUBCLASS(PCI_CLASS_WIRELESS_IRDA), "irda", NULL },
>> +    { SUBCLASS(PCI_CLASS_WIRELESS_CIR), "consumer-ir", NULL },
>> +    { SUBCLASS(PCI_CLASS_WIRELESS_RF_CONTROLLER), "rf-controller", NULL },
>> +    { SUBCLASS(PCI_CLASS_WIRELESS_BLUETOOTH), "bluetooth", NULL },
>> +    { SUBCLASS(PCI_CLASS_WIRELESS_BROADBAND), "broadband", NULL },
>> +    { 0xFF, NULL, NULL },
>> +};
>> +
>> +static const PCISubClass sat_subclass[] = {
>> +    { SUBCLASS(PCI_CLASS_SATELLITE_TV), "satellite-tv", NULL },
>> +    { SUBCLASS(PCI_CLASS_SATELLITE_AUDIO), "satellite-audio", NULL },
>> +    { SUBCLASS(PCI_CLASS_SATELLITE_VOICE), "satellite-voice", NULL },
>> +    { SUBCLASS(PCI_CLASS_SATELLITE_DATA), "satellite-data", NULL },
>> +    { 0xFF, NULL, NULL },
>> +};
>> +
>> +static const PCISubClass crypt_subclass[] = {
>> +    { SUBCLASS(PCI_CLASS_CRYPT_NETWORK), "network-encryption", NULL },
>> +    { SUBCLASS(PCI_CLASS_CRYPT_ENTERTAINMENT),
>> +      "entertainment-encryption", NULL },
>> +    { 0xFF, NULL, NULL },
>> +};
>> +
>> +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[] = {
>> +    { "legacy-device", undef_subclass },
>> +    { "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 >= 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, "%s@%x,%x", name, slot, func);
>> +    } else {
>> +        snprintf(nodename, len, "%s@%x", 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)));
>>     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, "pci@%x,%x", slot, func);
>> -    } else {
>> -        snprintf(nodename, FDT_NAME_MAX, "pci@%x", 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);
>>
>> -- 
>> 2.4.3
>>
>>
>

On Fri, Sep 25, 2015 at 10:29:41AM +0200, Laurent Vivier wrote:
> 
> 
> On 25/09/2015 01:29, Gavin Shan wrote:
> > 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/pci@800000020000000/*@*/; do
> >>    echo $dir
> >>    cat $dir/name
> >>    echo
> >> done
> >>
> >> WITHOUT this patch:
> >>
> >> /proc/device-tree/pci@800000020000000/pci@0/
> >> pci
> >> /proc/device-tree/pci@800000020000000/pci@1/
> >> pci
> >> /proc/device-tree/pci@800000020000000/pci@2/
> >> pci
> >> /proc/device-tree/pci@800000020000000/pci@3/
> >> pci
> >> /proc/device-tree/pci@800000020000000/pci@4/
> >> pci
> >> /proc/device-tree/pci@800000020000000/pci@5/
> >> pci
> >> /proc/device-tree/pci@800000020000000/pci@6/
> >> pci
> >> /proc/device-tree/pci@800000020000000/pci@7/
> >> pci
> >>
> >> WITH this patch:
> >>
> >> /proc/device-tree/pci@800000020000000/communication-controller@1/
> >> communication-controller
> >> /proc/device-tree/pci@800000020000000/display@4/
> >> display
> >> /proc/device-tree/pci@800000020000000/ethernet@5/
> >> ethernet
> >> /proc/device-tree/pci@800000020000000/input-controller@0/
> >> input-controller
> >> /proc/device-tree/pci@800000020000000/mouse@2/
> >> mouse
> >> /proc/device-tree/pci@800000020000000/multimedia-device@7/
> >> multimedia-device
> >> /proc/device-tree/pci@800000020000000/scsi@3/
> >> scsi
> >> /proc/device-tree/pci@800000020000000/usb-xhci@6/
> >> usb-xhci
> >>
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> >> Reviewed-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >> 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);
> >> }
> >>
> > 
> > One question would be: is there one reason why the logic, converting
> > class/subclass/iface code to tring, isn't put into generic PCI module?
> > If the code is put there, all platforms can reuse it.
> 
> For the moment, it is only used by the device tree generation for spapr,
> and moreover the names are ones from the openfirmware specification, so
> except if openbios on sparc/macintosh takes its device tree from QEMU I
> see no reason to put this in a generic PCI module.
> 
> Laurent

Our boot paths are supposed to use OF compatible names.
I note they look different for some reason.

> > 
> > Thanks,
> > Gavin
> > 
> >> +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_WORLDFIP), "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 },
> >> +};
> >> +
> >> +static const PCISubClass bridg_subclass[] = {
> >> +    { SUBCLASS(PCI_CLASS_BRIDGE_HOST), "host", NULL },
> >> +    { SUBCLASS(PCI_CLASS_BRIDGE_ISA), "isa", NULL },
> >> +    { SUBCLASS(PCI_CLASS_BRIDGE_EISA), "eisa", NULL },
> >> +    { SUBCLASS(PCI_CLASS_BRIDGE_MC), "mca", NULL },
> >> +    { SUBCLASS(PCI_CLASS_BRIDGE_PCI), "pci", NULL },
> >> +    { SUBCLASS(PCI_CLASS_BRIDGE_PCMCIA), "pcmcia", NULL },
> >> +    { SUBCLASS(PCI_CLASS_BRIDGE_NUBUS), "nubus", NULL },
> >> +    { SUBCLASS(PCI_CLASS_BRIDGE_CARDBUS), "cardbus", NULL },
> >> +    { SUBCLASS(PCI_CLASS_BRIDGE_RACEWAY), "raceway", NULL },
> >> +    { SUBCLASS(PCI_CLASS_BRIDGE_PCI_SEMITP), "semi-transparent-pci", NULL },
> >> +    { SUBCLASS(PCI_CLASS_BRIDGE_IB_PCI), "infiniband", NULL },
> >> +    { 0xFF, NULL, NULL },
> >> +};
> >> +
> >> +static const PCISubClass comm_subclass[] = {
> >> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_SERIAL), "serial", NULL },
> >> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_PARALLEL), "parallel", NULL },
> >> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_MULTISERIAL), "multiport-serial", NULL },
> >> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_MODEM), "modem", NULL },
> >> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_GPIB), "gpib", NULL },
> >> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_SC), "smart-card", NULL },
> >> +    { 0xFF, NULL, NULL, NULL },
> >> +};
> >> +
> >> +static const PCIIFace pic_iface[] = {
> >> +    { IFACE(PCI_CLASS_SYSTEM_PIC_IOAPIC), "io-apic" },
> >> +    { IFACE(PCI_CLASS_SYSTEM_PIC_IOXAPIC), "io-xapic" },
> >> +    { 0xFF, NULL },
> >> +};
> >> +
> >> +static const PCISubClass sys_subclass[] = {
> >> +    { SUBCLASS(PCI_CLASS_SYSTEM_PIC), "interrupt-controller", pic_iface },
> >> +    { SUBCLASS(PCI_CLASS_SYSTEM_DMA), "dma-controller", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SYSTEM_TIMER), "timer", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SYSTEM_RTC), "rtc", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SYSTEM_PCI_HOTPLUG), "hot-plug-controller", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SYSTEM_SDHCI), "sd-host-controller", NULL },
> >> +    { 0xFF, NULL, NULL },
> >> +};
> >> +
> >> +static const PCISubClass inp_subclass[] = {
> >> +    { SUBCLASS(PCI_CLASS_INPUT_KEYBOARD), "keyboard", NULL },
> >> +    { SUBCLASS(PCI_CLASS_INPUT_PEN), "pen", NULL },
> >> +    { SUBCLASS(PCI_CLASS_INPUT_MOUSE), "mouse", NULL },
> >> +    { SUBCLASS(PCI_CLASS_INPUT_SCANNER), "scanner", NULL },
> >> +    { SUBCLASS(PCI_CLASS_INPUT_GAMEPORT), "gameport", NULL },
> >> +    { 0xFF, NULL, NULL },
> >> +};
> >> +
> >> +static const PCISubClass dock_subclass[] = {
> >> +    { SUBCLASS(PCI_CLASS_DOCKING_GENERIC), "dock", NULL },
> >> +    { 0xFF, NULL, NULL },
> >> +};
> >> +
> >> +static const PCISubClass cpu_subclass[] = {
> >> +    { SUBCLASS(PCI_CLASS_PROCESSOR_PENTIUM), "pentium", 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 },
> >> +};
> >> +
> >> +static const PCIIFace usb_iface[] = {
> >> +    { IFACE(PCI_CLASS_SERIAL_USB_UHCI), "usb-uhci" },
> >> +    { IFACE(PCI_CLASS_SERIAL_USB_OHCI), "usb-ohci", },
> >> +    { IFACE(PCI_CLASS_SERIAL_USB_EHCI), "usb-ehci" },
> >> +    { IFACE(PCI_CLASS_SERIAL_USB_XHCI), "usb-xhci" },
> >> +    { IFACE(PCI_CLASS_SERIAL_USB_UNKNOWN), "usb-unknown" },
> >> +    { IFACE(PCI_CLASS_SERIAL_USB_DEVICE), "usb-device" },
> >> +    { 0xFF, NULL },
> >> +};
> >> +
> >> +static const PCISubClass ser_subclass[] = {
> >> +    { SUBCLASS(PCI_CLASS_SERIAL_FIREWIRE), "firewire", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SERIAL_ACCESS), "access-bus", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SERIAL_SSA), "ssa", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SERIAL_USB), "usb", usb_iface },
> >> +    { SUBCLASS(PCI_CLASS_SERIAL_FIBER), "fibre-channel", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SERIAL_SMBUS), "smb", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SERIAL_IB), "infiniband", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SERIAL_IPMI), "ipmi", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SERIAL_SERCOS), "sercos", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SERIAL_CANBUS), "canbus", NULL },
> >> +    { 0xFF, NULL, NULL },
> >> +};
> >> +
> >> +static const PCISubClass wrl_subclass[] = {
> >> +    { SUBCLASS(PCI_CLASS_WIRELESS_IRDA), "irda", NULL },
> >> +    { SUBCLASS(PCI_CLASS_WIRELESS_CIR), "consumer-ir", NULL },
> >> +    { SUBCLASS(PCI_CLASS_WIRELESS_RF_CONTROLLER), "rf-controller", NULL },
> >> +    { SUBCLASS(PCI_CLASS_WIRELESS_BLUETOOTH), "bluetooth", NULL },
> >> +    { SUBCLASS(PCI_CLASS_WIRELESS_BROADBAND), "broadband", NULL },
> >> +    { 0xFF, NULL, NULL },
> >> +};
> >> +
> >> +static const PCISubClass sat_subclass[] = {
> >> +    { SUBCLASS(PCI_CLASS_SATELLITE_TV), "satellite-tv", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SATELLITE_AUDIO), "satellite-audio", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SATELLITE_VOICE), "satellite-voice", NULL },
> >> +    { SUBCLASS(PCI_CLASS_SATELLITE_DATA), "satellite-data", NULL },
> >> +    { 0xFF, NULL, NULL },
> >> +};
> >> +
> >> +static const PCISubClass crypt_subclass[] = {
> >> +    { SUBCLASS(PCI_CLASS_CRYPT_NETWORK), "network-encryption", NULL },
> >> +    { SUBCLASS(PCI_CLASS_CRYPT_ENTERTAINMENT),
> >> +      "entertainment-encryption", NULL },
> >> +    { 0xFF, NULL, NULL },
> >> +};
> >> +
> >> +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[] = {
> >> +    { "legacy-device", undef_subclass },
> >> +    { "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 >= 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, "%s@%x,%x", name, slot, func);
> >> +    } else {
> >> +        snprintf(nodename, len, "%s@%x", 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)));
> >>     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, "pci@%x,%x", slot, func);
> >> -    } else {
> >> -        snprintf(nodename, FDT_NAME_MAX, "pci@%x", 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);
> >>
> >> -- 
> >> 2.4.3
> >>
> >>
> >

On 25/09/2015 11:12, Michael S. Tsirkin wrote:
> On Fri, Sep 25, 2015 at 10:29:41AM +0200, Laurent Vivier wrote:
>>
>>
>> On 25/09/2015 01:29, Gavin Shan wrote:
>>> 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/pci@800000020000000/*@*/; do
>>>>    echo $dir
>>>>    cat $dir/name
>>>>    echo
>>>> done
>>>>
>>>> WITHOUT this patch:
>>>>
>>>> /proc/device-tree/pci@800000020000000/pci@0/
>>>> pci
>>>> /proc/device-tree/pci@800000020000000/pci@1/
>>>> pci
>>>> /proc/device-tree/pci@800000020000000/pci@2/
>>>> pci
>>>> /proc/device-tree/pci@800000020000000/pci@3/
>>>> pci
>>>> /proc/device-tree/pci@800000020000000/pci@4/
>>>> pci
>>>> /proc/device-tree/pci@800000020000000/pci@5/
>>>> pci
>>>> /proc/device-tree/pci@800000020000000/pci@6/
>>>> pci
>>>> /proc/device-tree/pci@800000020000000/pci@7/
>>>> pci
>>>>
>>>> WITH this patch:
>>>>
>>>> /proc/device-tree/pci@800000020000000/communication-controller@1/
>>>> communication-controller
>>>> /proc/device-tree/pci@800000020000000/display@4/
>>>> display
>>>> /proc/device-tree/pci@800000020000000/ethernet@5/
>>>> ethernet
>>>> /proc/device-tree/pci@800000020000000/input-controller@0/
>>>> input-controller
>>>> /proc/device-tree/pci@800000020000000/mouse@2/
>>>> mouse
>>>> /proc/device-tree/pci@800000020000000/multimedia-device@7/
>>>> multimedia-device
>>>> /proc/device-tree/pci@800000020000000/scsi@3/
>>>> scsi
>>>> /proc/device-tree/pci@800000020000000/usb-xhci@6/
>>>> usb-xhci
>>>>
>>>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>> 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);
>>>> }
>>>>
>>>
>>> One question would be: is there one reason why the logic, converting
>>> class/subclass/iface code to tring, isn't put into generic PCI module?
>>> If the code is put there, all platforms can reuse it.
>>
>> For the moment, it is only used by the device tree generation for spapr,
>> and moreover the names are ones from the openfirmware specification, so
>> except if openbios on sparc/macintosh takes its device tree from QEMU I
>> see no reason to put this in a generic PCI module.
>>
>> Laurent
> 
> Our boot paths are supposed to use OF compatible names.
> I note they look different for some reason.

Do you want I move this code to the PCI directory (pci_database.c, for
instance) ? It's up to you.

>>>
>>> Thanks,
>>> Gavin
>>>
>>>> +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_WORLDFIP), "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 },
>>>> +};
>>>> +
>>>> +static const PCISubClass bridg_subclass[] = {
>>>> +    { SUBCLASS(PCI_CLASS_BRIDGE_HOST), "host", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_BRIDGE_ISA), "isa", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_BRIDGE_EISA), "eisa", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_BRIDGE_MC), "mca", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_BRIDGE_PCI), "pci", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_BRIDGE_PCMCIA), "pcmcia", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_BRIDGE_NUBUS), "nubus", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_BRIDGE_CARDBUS), "cardbus", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_BRIDGE_RACEWAY), "raceway", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_BRIDGE_PCI_SEMITP), "semi-transparent-pci", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_BRIDGE_IB_PCI), "infiniband", NULL },
>>>> +    { 0xFF, NULL, NULL },
>>>> +};
>>>> +
>>>> +static const PCISubClass comm_subclass[] = {
>>>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_SERIAL), "serial", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_PARALLEL), "parallel", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_MULTISERIAL), "multiport-serial", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_MODEM), "modem", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_GPIB), "gpib", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_COMMUNICATION_SC), "smart-card", NULL },
>>>> +    { 0xFF, NULL, NULL, NULL },
>>>> +};
>>>> +
>>>> +static const PCIIFace pic_iface[] = {
>>>> +    { IFACE(PCI_CLASS_SYSTEM_PIC_IOAPIC), "io-apic" },
>>>> +    { IFACE(PCI_CLASS_SYSTEM_PIC_IOXAPIC), "io-xapic" },
>>>> +    { 0xFF, NULL },
>>>> +};
>>>> +
>>>> +static const PCISubClass sys_subclass[] = {
>>>> +    { SUBCLASS(PCI_CLASS_SYSTEM_PIC), "interrupt-controller", pic_iface },
>>>> +    { SUBCLASS(PCI_CLASS_SYSTEM_DMA), "dma-controller", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SYSTEM_TIMER), "timer", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SYSTEM_RTC), "rtc", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SYSTEM_PCI_HOTPLUG), "hot-plug-controller", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SYSTEM_SDHCI), "sd-host-controller", NULL },
>>>> +    { 0xFF, NULL, NULL },
>>>> +};
>>>> +
>>>> +static const PCISubClass inp_subclass[] = {
>>>> +    { SUBCLASS(PCI_CLASS_INPUT_KEYBOARD), "keyboard", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_INPUT_PEN), "pen", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_INPUT_MOUSE), "mouse", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_INPUT_SCANNER), "scanner", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_INPUT_GAMEPORT), "gameport", NULL },
>>>> +    { 0xFF, NULL, NULL },
>>>> +};
>>>> +
>>>> +static const PCISubClass dock_subclass[] = {
>>>> +    { SUBCLASS(PCI_CLASS_DOCKING_GENERIC), "dock", NULL },
>>>> +    { 0xFF, NULL, NULL },
>>>> +};
>>>> +
>>>> +static const PCISubClass cpu_subclass[] = {
>>>> +    { SUBCLASS(PCI_CLASS_PROCESSOR_PENTIUM), "pentium", 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 },
>>>> +};
>>>> +
>>>> +static const PCIIFace usb_iface[] = {
>>>> +    { IFACE(PCI_CLASS_SERIAL_USB_UHCI), "usb-uhci" },
>>>> +    { IFACE(PCI_CLASS_SERIAL_USB_OHCI), "usb-ohci", },
>>>> +    { IFACE(PCI_CLASS_SERIAL_USB_EHCI), "usb-ehci" },
>>>> +    { IFACE(PCI_CLASS_SERIAL_USB_XHCI), "usb-xhci" },
>>>> +    { IFACE(PCI_CLASS_SERIAL_USB_UNKNOWN), "usb-unknown" },
>>>> +    { IFACE(PCI_CLASS_SERIAL_USB_DEVICE), "usb-device" },
>>>> +    { 0xFF, NULL },
>>>> +};
>>>> +
>>>> +static const PCISubClass ser_subclass[] = {
>>>> +    { SUBCLASS(PCI_CLASS_SERIAL_FIREWIRE), "firewire", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SERIAL_ACCESS), "access-bus", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SERIAL_SSA), "ssa", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SERIAL_USB), "usb", usb_iface },
>>>> +    { SUBCLASS(PCI_CLASS_SERIAL_FIBER), "fibre-channel", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SERIAL_SMBUS), "smb", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SERIAL_IB), "infiniband", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SERIAL_IPMI), "ipmi", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SERIAL_SERCOS), "sercos", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SERIAL_CANBUS), "canbus", NULL },
>>>> +    { 0xFF, NULL, NULL },
>>>> +};
>>>> +
>>>> +static const PCISubClass wrl_subclass[] = {
>>>> +    { SUBCLASS(PCI_CLASS_WIRELESS_IRDA), "irda", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_WIRELESS_CIR), "consumer-ir", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_WIRELESS_RF_CONTROLLER), "rf-controller", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_WIRELESS_BLUETOOTH), "bluetooth", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_WIRELESS_BROADBAND), "broadband", NULL },
>>>> +    { 0xFF, NULL, NULL },
>>>> +};
>>>> +
>>>> +static const PCISubClass sat_subclass[] = {
>>>> +    { SUBCLASS(PCI_CLASS_SATELLITE_TV), "satellite-tv", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SATELLITE_AUDIO), "satellite-audio", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SATELLITE_VOICE), "satellite-voice", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_SATELLITE_DATA), "satellite-data", NULL },
>>>> +    { 0xFF, NULL, NULL },
>>>> +};
>>>> +
>>>> +static const PCISubClass crypt_subclass[] = {
>>>> +    { SUBCLASS(PCI_CLASS_CRYPT_NETWORK), "network-encryption", NULL },
>>>> +    { SUBCLASS(PCI_CLASS_CRYPT_ENTERTAINMENT),
>>>> +      "entertainment-encryption", NULL },
>>>> +    { 0xFF, NULL, NULL },
>>>> +};
>>>> +
>>>> +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[] = {
>>>> +    { "legacy-device", undef_subclass },
>>>> +    { "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 >= 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, "%s@%x,%x", name, slot, func);
>>>> +    } else {
>>>> +        snprintf(nodename, len, "%s@%x", 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)));
>>>>     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, "pci@%x,%x", slot, func);
>>>> -    } else {
>>>> -        snprintf(nodename, FDT_NAME_MAX, "pci@%x", 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);
>>>>
>>>> -- 
>>>> 2.4.3
>>>>
>>>>
>>>

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/pci@800000020000000/*@*/; do
>     echo $dir
>     cat $dir/name
>     echo
> done
> 
> WITHOUT this patch:
> 
> /proc/device-tree/pci@800000020000000/pci@0/
> pci
> /proc/device-tree/pci@800000020000000/pci@1/
> pci
> /proc/device-tree/pci@800000020000000/pci@2/
> pci
> /proc/device-tree/pci@800000020000000/pci@3/
> pci
> /proc/device-tree/pci@800000020000000/pci@4/
> pci
> /proc/device-tree/pci@800000020000000/pci@5/
> pci
> /proc/device-tree/pci@800000020000000/pci@6/
> pci
> /proc/device-tree/pci@800000020000000/pci@7/
> pci
> 
> WITH this patch:
> 
> /proc/device-tree/pci@800000020000000/communication-controller@1/
> communication-controller
> /proc/device-tree/pci@800000020000000/display@4/
> display
> /proc/device-tree/pci@800000020000000/ethernet@5/
> ethernet
> /proc/device-tree/pci@800000020000000/input-controller@0/
> input-controller
> /proc/device-tree/pci@800000020000000/mouse@2/
> mouse
> /proc/device-tree/pci@800000020000000/multimedia-device@7/
> multimedia-device
> /proc/device-tree/pci@800000020000000/scsi@3/
> scsi
> /proc/device-tree/pci@800000020000000/usb-xhci@6/
> usb-xhci
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>

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, "%s@%x,%x", name, slot, func);
> +    } else {
> +        snprintf(nodename, len, "%s@%x", 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, "pci@%x,%x", slot, func);
> -    } else {
> -        snprintf(nodename, FDT_NAME_MAX, "pci@%x", 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);
>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256



On 29/09/2015 07:18, David Gibson wrote:
> 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/pci@800000020000000/*@*/; do echo
>> $dir cat $dir/name echo done
>> 
>> WITHOUT this patch:
>> 
>> /proc/device-tree/pci@800000020000000/pci@0/ pci 
>> /proc/device-tree/pci@800000020000000/pci@1/ pci 
>> /proc/device-tree/pci@800000020000000/pci@2/ pci 
>> /proc/device-tree/pci@800000020000000/pci@3/ pci 
>> /proc/device-tree/pci@800000020000000/pci@4/ pci 
>> /proc/device-tree/pci@800000020000000/pci@5/ pci 
>> /proc/device-tree/pci@800000020000000/pci@6/ pci 
>> /proc/device-tree/pci@800000020000000/pci@7/ pci
>> 
>> WITH this patch:
>> 
>> /proc/device-tree/pci@800000020000000/communication-controller@1/
>>
>> 
communication-controller
>> /proc/device-tree/pci@800000020000000/display@4/ display 
>> /proc/device-tree/pci@800000020000000/ethernet@5/ ethernet 
>> /proc/device-tree/pci@800000020000000/input-controller@0/ 
>> input-controller /proc/device-tree/pci@800000020000000/mouse@2/ 
>> mouse /proc/device-tree/pci@800000020000000/multimedia-device@7/ 
>> multimedia-device /proc/device-tree/pci@800000020000000/scsi@3/ 
>> scsi /proc/device-tree/pci@800000020000000/usb-xhci@6/ usb-xhci
>> 
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com> Reviewed-by:
>> Thomas Huth <thuth@redhat.com>
> 
> 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?

In fact, in case of subclass, the value is ((CLASS << 8) + SUBCLASS)
and in case of iface is ((CLASS << 16) + (SUBCLASS << 8) + IFACE).
To build the array we need either only the CLASS or the ICLASS of the
value. These macros (which are indentical) take the low order byte to
have only the subclass or the iface to put it in the array.

And on this line, it's wrong, we should use SUBCLASS():

#define PCI_CLASS_NOT_DEFINED            0x0000

So class is "0x00"

#define PCI_CLASS_NOT_DEFINED_VGA        0x0001

subclass is "0x01".

For the case of iface:

#define PCI_BASE_CLASS_SERIAL            0x0c

class is "serial", 0x0c

#define PCI_CLASS_SERIAL_USB             0x0c03

subclass is "usb", 0x03

#define PCI_CLASS_SERIAL_USB_XHCI        0x0c0330

iface is "xhci", 0x30

So of course I can remove macros SUBCLASS() and IFACE() and simply
replace them by a cast "(uint8_t *)".

> 
> [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,
>> "%s@%x,%x", name, slot, func); +    } else { +
>> snprintf(nodename, len, "%s@%x", 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.

In fact, this is also the behavior of SLOF.

> 
>> 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,
>> "pci@%x,%x", slot, func); -    } else { -
>> snprintf(nodename, FDT_NAME_MAX, "pci@%x", 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);
>> 
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJWCk3LAAoJEPMMOL0/L748PKsP/2pFORLvqnU6oOB/RFQsAygW
ux/EgoBE1eHS6XHus2hT73x5J8AGQMjmK7TtE1DRpEbNqWvcHAagsTvvgYj+swIY
hydvsNj9tFvMNLsrZ7pX2xVBivjeydlKrEs0+cvO+RmBXV8O0npBlKvbRLDNZQl5
k//qBtKVNSzlej6GU3a9zSx/rR9dMqt2aQudw6xwbTx/olzC+tQU4Ddgz/iIQchj
8WyiZ+eyc744OEilRMMDa/jkS+1Bq4e3IP1V9BITtOkEtAlhmO2lilTCMVaSTBr8
Kdi5YsFY88g2nh3UkvHpc8Glrois7SaxpP7Z6pb2yj/KDuew3rf/Gjag5bkRTq6g
NqgeBmnjyvJWqNM01I2jDD23c4luD/YChtGHF2lW8FET3Jca5D9/U4diXlITiFNd
njrhK2SYq/2SfaUe2zeqRpHJE7kKOeUjhMo/n2SvEhA9qjnRwbfKZhAHAs+qZ2IH
wDQ1SR19mWYpyN2CStOY6T+sCKYg4ZfuBnSkr/b5KQL/CjhXTV9ECoCrjEhvO7m2
5bpzX5gIgehSarHRLDdRrkUoZ+MHa116qjLa6x7pBhYGj7FY3L+ED1Gl5YJz7cQe
J8MOqsr2uA1Yw5ZwwI+lwIJxI1/A/dRkD2nXDivu1/sFAVdVat80oJJ5jL4yIJ9E
Piwez/HY+TsVtCsZCUDL
=c7gD
-----END PGP SIGNATURE-----

On 29/09/15 10:37, Laurent Vivier wrote:
> 
> 
> On 29/09/2015 07:18, David Gibson wrote:
>> On Thu, Sep 24, 2015 at 12:27:39PM +0200, Laurent Vivier wrote:
...
>>> -    /* 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.
> 
> In fact, this is also the behavior of SLOF.

Well, SLOF builds an Open Firmware device tree (not a flattened device
tree), thus the "name" property is always required there.

 Thomas

On Tue, Sep 29, 2015 at 10:37:31AM +0200, Laurent Vivier wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> 
> 
> On 29/09/2015 07:18, David Gibson wrote:
> > 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/pci@800000020000000/*@*/; do echo
> >> $dir cat $dir/name echo done
> >> 
> >> WITHOUT this patch:
> >> 
> >> /proc/device-tree/pci@800000020000000/pci@0/ pci 
> >> /proc/device-tree/pci@800000020000000/pci@1/ pci 
> >> /proc/device-tree/pci@800000020000000/pci@2/ pci 
> >> /proc/device-tree/pci@800000020000000/pci@3/ pci 
> >> /proc/device-tree/pci@800000020000000/pci@4/ pci 
> >> /proc/device-tree/pci@800000020000000/pci@5/ pci 
> >> /proc/device-tree/pci@800000020000000/pci@6/ pci 
> >> /proc/device-tree/pci@800000020000000/pci@7/ pci
> >> 
> >> WITH this patch:
> >> 
> >> /proc/device-tree/pci@800000020000000/communication-controller@1/
> >>
> >> 
> communication-controller
> >> /proc/device-tree/pci@800000020000000/display@4/ display 
> >> /proc/device-tree/pci@800000020000000/ethernet@5/ ethernet 
> >> /proc/device-tree/pci@800000020000000/input-controller@0/ 
> >> input-controller /proc/device-tree/pci@800000020000000/mouse@2/ 
> >> mouse /proc/device-tree/pci@800000020000000/multimedia-device@7/ 
> >> multimedia-device /proc/device-tree/pci@800000020000000/scsi@3/ 
> >> scsi /proc/device-tree/pci@800000020000000/usb-xhci@6/ usb-xhci
> >> 
> >> Signed-off-by: Laurent Vivier <lvivier@redhat.com> Reviewed-by:
> >> Thomas Huth <thuth@redhat.com>
> > 
> > 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?
> 
> In fact, in case of subclass, the value is ((CLASS << 8) + SUBCLASS)
> and in case of iface is ((CLASS << 16) + (SUBCLASS << 8) + IFACE).
> To build the array we need either only the CLASS or the ICLASS of the
> value. These macros (which are indentical) take the low order byte to
> have only the subclass or the iface to put it in the array.
> 
> And on this line, it's wrong, we should use SUBCLASS():
> 
> #define PCI_CLASS_NOT_DEFINED            0x0000
> 
> So class is "0x00"
> 
> #define PCI_CLASS_NOT_DEFINED_VGA        0x0001
> 
> subclass is "0x01".
> 
> For the case of iface:
> 
> #define PCI_BASE_CLASS_SERIAL            0x0c
> 
> class is "serial", 0x0c
> 
> #define PCI_CLASS_SERIAL_USB             0x0c03
> 
> subclass is "usb", 0x03
> 
> #define PCI_CLASS_SERIAL_USB_XHCI        0x0c0330
> 
> iface is "xhci", 0x30
> 
> So of course I can remove macros SUBCLASS() and IFACE() and simply
> replace them by a cast "(uint8_t *)".

Hm, ok.  If the point of the macro is to mask out the relevant bits,
I'd prefer to see an explicit (x & 0xff), rather than using the cast.
The effect is the same, but it's more obvious what it's for.

But.. I wonder if it might be simpler still to just store the whole
value in the table, and only mask in the lookup function.