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

[Thomas Huth]

On 19/09/15 00:06, 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.

Note that many names are mandated by the "PCI Bus Binding to:
IEEE Std 1275-1994" specification, e.g. available here:

 http://www.o3one.org/hwdocs/openfirmware/pci_supplement_2_1.pdf

See "Table 1: Generic Names based upon Class Code".

AFAIK the SLOF names are based on that spec, so most names should
hopefully be fine already.

[...]
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index a2feb4c..ca3a0ba 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -944,6 +944,487 @@ static void populate_resource_props(PCIDevice *d, 
> ResourceProps *rp)
>      rp->assigned_len = assigned_idx * sizeof(ResourceFields);
[...]
> +
> +typedef struct pci_class_t pci_class_t;
> +typedef struct pci_subclass_t pci_subclass_t;
> +typedef struct pci_iface_t pci_iface_t;
> +
> +struct pci_iface_t {
> +    uint8_t iface;
> +    const char *name;
> +};
> +
> +struct pci_subclass_t {
> +    uint8_t subclass;
> +    const char *name;
> +    const pci_iface_t *iface;
> +};
> +
> +struct pci_class_t {
> +    const char *name;
> +    const pci_subclass_t *subc;
> +};

According to the QEMU CODING_STYLEs, "Structured
type names are in CamelCase", and AFAIK not using a "_t" suffix.

> +static const pci_subclass_t undef_subclass[] = {
> +    { 0x01, "display", NULL },
> +    { 0xFF, NULL, NULL, NULL },
> +};
> +
> +static const pci_subclass_t mass_subclass[] = {
> +    { PCI_SUBCLASS_STORAGE_SCSI, "scsi", NULL },
> +    { PCI_SUBCLASS_STORAGE_IDE, "ide", NULL },
> +    { PCI_SUBCLASS_STORAGE_FLOPPY, "fdc", NULL },
> +    { PCI_SUBCLASS_STORAGE_IPI, "ipi", NULL },
> +    { PCI_SUBCLASS_STORAGE_RAID, "raid", NULL },
> +    { PCI_SUBCLASS_STORAGE_ATA, "ata", NULL },
> +    { PCI_SUBCLASS_STORAGE_SATA, "sata", NULL },
> +    { PCI_SUBCLASS_STORAGE_SAS, "sas", NULL },
> +    { PCI_SUBCLASS_STORAGE_OTHER, "mass-storage", NULL },
> +    { 0xFF, NULL, NULL },
> +};
> +
> +static const pci_subclass_t net_subclass[] = {
> +    { PCI_SUBCLASS_NETWORK_ETHERNET, "ethernet", NULL },
> +    { PCI_SUBCLASS_NETWORK_TOKEN_RING, "token-ring", NULL },
> +    { PCI_SUBCLASS_NETWORK_FDDI, "fddi", NULL },
> +    { PCI_SUBCLASS_NETWORK_ATM, "atm", NULL },
> +    { PCI_SUBCLASS_NETWORK_ISDN, "isdn", NULL },
> +    { PCI_SUBCLASS_NETWORK_WORDFIP, "worldfip", NULL },
> +    { PCI_SUBCLASS_NETWORK_PICMG214, "picmg", NULL },
> +    { PCI_SUBCLASS_NETWORK_OTHER, "network", NULL },
> +    { 0xFF, NULL, NULL },
> +};
> +
> +static const struct pci_iface_t vga_iface[] = {
> +    { 0x00, "VGA", },
> +    { 0x01, "8514-compatible", },
> +    { 0xFF, NULL, },
> +};
> +
> +static const pci_subclass_t displ_subclass[] = {
> +    { PCI_SUBCLASS_DISPLAY_VGA, "vga", vga_iface },
> +    { PCI_SUBCLASS_DISPLAY_XGA, "xga", NULL },
> +    { PCI_SUBCLASS_DISPLAY_3D, "3d-controller", NULL },
> +    { PCI_SUBCLASS_DISPLAY_OTHER, "misc-display-controller", NULL },
> +    { 0xFF, NULL, NULL },
> +};

I somewhat dislike such long device node names like
"misc-display-controller" (since I sometimes have to type in them
manually at the SLOF prompt) ... and the PCI bus binding spec says that
the default name should be "display" here instead, so simply use
"display" instead of "misc-display-controller" ?

> +static const pci_subclass_t media_subclass[] = {
> +    { PCI_SUBCLASS_MULTIMEDIA_VIDEO, "video", NULL },
> +    { PCI_SUBCLASS_MULTIMEDIA_AUDIO, "sound", NULL },
> +    { PCI_SUBCLASS_MULTIMEDIA_PHONE, "telephony", NULL },
> +    { PCI_SUBCLASS_MULTIMEDIA_OTHER, "misc-multimedia-device", NULL },
> +    { 0xFF, NULL, NULL },
> +};

s/misc-multimedia-device/multimedia/ ?

"misc-" prefix and "-device" suffix IMHO do not make too much sense for
a device tree node.

> +static const pci_subclass_t mem_subclass[] = {
> +    { PCI_SUBCLASS_MEMORY_RAM, "memory", NULL },
> +    { PCI_SUBCLASS_MEMORY_FLASH, "flash", NULL },
> +    { 0xFF, NULL, NULL },
> +};
> +
> +
> +static const pci_subclass_t bridg_subclass[] = {
> +    { PCI_SUBCLASS_BRIDGE_HOST, "host", NULL },
> +    { PCI_SUBCLASS_BRIDGE_ISA, "isa", NULL },
> +    { PCI_SUBCLASS_BRIDGE_EISA, "eisa", NULL },
> +    { PCI_SUBCLASS_BRIDGE_MC, "mca", NULL },
> +    { PCI_SUBCLASS_BRIDGE_PCI, "pci", NULL },
> +    { PCI_SUBCLASS_BRIDGE_PCMCIA, "pcmcia", NULL },
> +    { PCI_SUBCLASS_BRIDGE_NUBUS, "nubus", NULL },
> +    { PCI_SUBCLASS_BRIDGE_CARDBUS, "cardbus", NULL },
> +    { PCI_SUBCLASS_BRIDGE_RACEWAY, "raceway", NULL },
> +    { PCI_SUBCLASS_BRIDGE_PCI_SEMITP, "semi-transparent-pci", NULL },
> +    { PCI_SUBCLASS_BRIDGE_IB_PCI, "infiniband", NULL },
> +    { PCI_SUBCLASS_BRIDGE_OTHER, "misc-brige", NULL },
> +    { 0xFF, NULL, NULL },
> +};
> +
> +static const pci_iface_t serial_iface[] = {
> +    { 0x00, "serial" },
> +    { 0x01, "16450-serial" },
> +    { 0x02, "16550-serial" },
> +    { 0x03, "16650-serial" },
> +    { 0x04, "16750-serial" },
> +    { 0x05, "16850-serial" },
> +    { 0x06, "16950-serial" },
> +    { 0xFF, NULL },
> +};
> +
> +static const pci_iface_t par_iface[] = {
> +    { 0x00, "parallel" },
> +    { 0x01, "bi-directional-parallel" },
> +    { 0x02, "ecp-1.x-parallel" },
> +    { 0x03, "ieee1284-controller" },
> +    { 0xFE, "ieee1284-device" },
> +    { 0xFF, NULL },
> +};
> +
> +static const pci_iface_t modem_iface[] = {
> +    { 0x00, "modem" },
> +    { 0x01, "16450-modem" },
> +    { 0x02, "16550-modem" },
> +    { 0x03, "16650-modem" },
> +    { 0x04, "16750-modem" },
> +    { 0xFF, NULL },
> +};
> +
> +static const pci_subclass_t comm_subclass[] = {
> +    { PCI_SUBCLASS_COMMUNICATION_SERIAL, "serial", serial_iface },
> +    { PCI_SUBCLASS_COMMUNICATION_PARALLEL, "parallel", par_iface },
> +    { PCI_SUBCLASS_COMMUNICATION_MULTISERIAL, "multiport-serial", NULL },
> +    { PCI_SUBCLASS_COMMUNICATION_MODEM, "modem", modem_iface },
> +    { PCI_SUBCLASS_COMMUNICATION_GPIB, "gpib", NULL },
> +    { PCI_SUBCLASS_COMMUNICATION_SC, "smart-card", NULL },
> +    { PCI_SUBCLASS_COMMUNICATION_OTHER, "misc-communication-controller",
> +      NULL },
> +    { 0xFF, NULL, NULL, NULL },
> +};

Maybe s/misc-communication-controller/communication/ ?

I'd also drop the other "misc-" prefixes in this patch.

> +static const pci_subclass_t cpu_subclass[] = {
> +    { PCI_SUBCLASS_PROCESSOR_386, "386", NULL },
> +    { PCI_SUBCLASS_PROCESSOR_486, "486", NULL },
> +    { PCI_SUBCLASS_PROCESSOR_PENTIUM, "pentium", NULL },
> +    { PCI_SUBCLASS_PROCESSOR_ALPHA, "alpha", NULL },
> +    { PCI_SUBCLASS_PROCESSOR_POWERPC, "powerpc", NULL },
> +    { PCI_SUBCLASS_PROCESSOR_MIPS, "mips", NULL },
> +    { PCI_SUBCLASS_PROCESSOR_CO, "co-processor", NULL },
> +    { 0xFF, NULL, NULL },
> +};

I wonder whether there should really still be entries for 386 and other
antique hardware in this patch? Feels somewhat weird... maybe you could
omit them?

> +static const pci_iface_t usb_iface[] = {
> +    { 0x00, "usb-uhci" },
> +    { 0x10, "usb-ohci", },
> +    { 0x20, "usb-ehci" },
> +    { 0x30, "usb-xhci" },
> +    { 0x80, "misc-usb-controller" },
> +    { 0xFE, "usb-device" },
> +    { 0xFF, NULL },
> +};
> +
> +static const pci_iface_t ipmi_iface[] = {
> +    { 0x00, "ipmi-smic" },
> +    { 0x01, "ipmi-kbrd" },
> +    { 0x02, "IPMI-bltr" },
> +    { 0xFF, NULL },
> +};

Why is IPMI-bltr partially in upper case letters?


... apart from the naming nits, the patch looks fine to me.

 Thomas