From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33920) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zf2Pa-0006ID-B0 for qemu-devel@nongnu.org; Thu, 24 Sep 2015 05:01:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zf2PV-0007mm-6a for qemu-devel@nongnu.org; Thu, 24 Sep 2015 05:01:54 -0400 References: <1443010443-4432-1-git-send-email-lvivier@redhat.com> From: Thomas Huth Message-ID: <5603BBF8.9070303@redhat.com> Date: Thu, 24 Sep 2015 11:01:44 +0200 MIME-Version: 1.0 In-Reply-To: <1443010443-4432-1-git-send-email-lvivier@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3] spapr: generate DT node names List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier , qemu-ppc@nongnu.org Cc: qemu-devel@nongnu.org, "Michael S. Tsirkin" , Alexander Graf , David Gibson On 23/09/15 14:14, Laurent Vivier wrote: > When DT node names for PCI devices are generated by SLOF, > they are generated according to the type of the device > (for instance, ethernet for virtio-net-pci device). >=20 > 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. >=20 > The data structure has been roughly copied from OpenBIOS/OpenHackware, > node names from SLOF. [...] > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index a2feb4c..c521d31 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -38,6 +38,7 @@ > =20 > #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" > =20 > @@ -944,6 +945,280 @@ static void populate_resource_props(PCIDevice *d,= ResourceProps *rp) > rp->assigned_len =3D assigned_idx * sizeof(ResourceFields); > } > =20 > +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[] =3D { > + { IFACE(PCI_CLASS_NOT_DEFINED_VGA), "display", NULL }, > + { 0xFF, NULL, NULL, NULL }, > +}; > + > +static const PCISubClass mass_subclass[] =3D { > + { 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[] =3D { > + { SUBCLASS(PCI_CLASS_NETWORK_ETHERNET), "ethernet", NULL }, > + { SUBCLASS(PCI_CLASS_NETWORK_TOKEN_RING), "token-ring", NULL }, > + { SUBCLASS(PCI_CLASS_NETWORK_FDDI), "fddi", NULL }, > + { SUBCLASS(PCI_CLASS_NETWORK_ATM), "atm", NULL }, > + { SUBCLASS(PCI_CLASS_NETWORK_ISDN), "isdn", NULL }, > + { SUBCLASS(PCI_CLASS_NETWORK_WORDFIP), "worldfip", NULL }, > + { SUBCLASS(PCI_CLASS_NETWORK_PICMG214), "picmg", NULL }, > + { 0xFF, NULL, NULL }, > +}; > + > +static const PCISubClass displ_subclass[] =3D { > + { 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[] =3D { > + { 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[] =3D { > + { SUBCLASS(PCI_CLASS_MEMORY_RAM), "memory", NULL }, > + { SUBCLASS(PCI_CLASS_MEMORY_FLASH), "flash", NULL }, > + { 0xFF, NULL, NULL }, > +}; > + > + One new-line should be sufficient. [...] > +static const PCISubClass cpu_subclass[] =3D { > + { SUBCLASS(PCI_CLASS_PROCESSOR_386), "386", NULL }, > + { SUBCLASS(PCI_CLASS_PROCESSOR_486), "486", NULL }, > + { SUBCLASS(PCI_CLASS_PROCESSOR_PENTIUM), "pentium", NULL }, > + { SUBCLASS(PCI_CLASS_PROCESSOR_ALPHA), "alpha", NULL }, > + { SUBCLASS(PCI_CLASS_PROCESSOR_POWERPC), "powerpc", NULL }, > + { SUBCLASS(PCI_CLASS_PROCESSOR_MIPS), "mips", NULL }, > + { SUBCLASS(PCI_CLASS_PROCESSOR_CO), "co-processor", NULL }, > + { 0xFF, NULL, NULL }, > +}; I really really doubt that we'll ever see such a device on the spapr PCI bus ... could you at least omit entries like "386", "486" and "alpha" ? [...] > + > +static const PCISubClass spc_subclass[] =3D { > + { 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[] =3D { > + { "unknown-legacy-device", undef_subclass }, Maybe just "legacy-device" instead of "unknown-legacy-device" ? > + { "mass-storage", mass_subclass }, > + { "network", net_subclass }, > + { "display", displ_subclass, }, > + { "multimedia-device", media_subclass }, > + { "memory-controller", mem_subclass }, > + { "unknown-bridge", bridg_subclass }, > + { "communication-controller", comm_subclass}, > + { "system-peripheral", sys_subclass }, > + { "input-controller", inp_subclass }, > + { "docking-station", dock_subclass }, > + { "cpu", cpu_subclass }, > + { "serial-bus", ser_subclass }, > + { "wireless-controller", wrl_subclass }, > + { "intelligent-io", NULL }, > + { "satellite-device", sat_subclass }, > + { "encryption", crypt_subclass }, > + { "data-processing-controller", spc_subclass }, > +}; > + > +static const char *pci_find_device_name(uint8_t class, uint8_t subclas= s, > + uint8_t iface) > +{ > + const PCIClass *pclass; > + const PCISubClass *psubclass; > + const PCIIFace *piface; > + const char *name; > + > + if (class > (sizeof(pci_classes) / sizeof(PCIClass))) { I think you could also use the ARRAY_SIZE macro here. And shouldn't that rather be ">=3D" instead of ">" ? > + return "pci"; > + } > + > + pclass =3D pci_classes + class; > + name =3D pclass->name; > + > + if (pclass->subc =3D=3D NULL) { > + return name; > + } > + > + psubclass =3D pclass->subc; > + while (psubclass->subclass !=3D 0xff) { > + if (psubclass->subclass =3D=3D subclass) { > + name =3D psubclass->name; > + break; > + } > + psubclass++; > + } > + > + piface =3D psubclass->iface; > + if (piface =3D=3D NULL) { > + return name; > + } > + while (piface->iface !=3D 0xff) { > + if (piface->iface =3D=3D iface) { > + name =3D piface->name; > + break; > + } > + piface++; > + } > + > + return name; > +} [...] > diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h > index d98e6c9..42c86df 100644 > --- a/include/hw/pci/pci_ids.h > +++ b/include/hw/pci/pci_ids.h > @@ -12,41 +12,84 @@ > =20 > /* Device classes and subclasses */ > =20 > -#define PCI_BASE_CLASS_STORAGE 0x01 > -#define PCI_BASE_CLASS_NETWORK 0x02 > +#define PCI_CLASS_NOT_DEFINED 0x0000 > +#define PCI_CLASS_NOT_DEFINED_VGA 0x0001 > =20 > +#define PCI_BASE_CLASS_STORAGE 0x01 > #define PCI_CLASS_STORAGE_SCSI 0x0100 > #define PCI_CLASS_STORAGE_IDE 0x0101 > +#define PCI_CLASS_STORAGE_FLOPPY 0x0102 > +#define PCI_CLASS_STORAGE_IPI 0x0103 > #define PCI_CLASS_STORAGE_RAID 0x0104 > +#define PCI_CLASS_STORAGE_ATA 0x0105 > #define PCI_CLASS_STORAGE_SATA 0x0106 > +#define PCI_CLASS_STORAGE_SAS 0x0107 > #define PCI_CLASS_STORAGE_EXPRESS 0x0108 > #define PCI_CLASS_STORAGE_OTHER 0x0180 > =20 > +#define PCI_BASE_CLASS_NETWORK 0x02 > #define PCI_CLASS_NETWORK_ETHERNET 0x0200 > +#define PCI_CLASS_NETWORK_TOKEN_RING 0x0201 > +#define PCI_CLASS_NETWORK_FDDI 0x0202 > +#define PCI_CLASS_NETWORK_ATM 0x0203 > +#define PCI_CLASS_NETWORK_ISDN 0x0204 > +#define PCI_CLASS_NETWORK_WORDFIP 0x0205 I think it's WORLDFIP, not WORDFIP. Also you should maybe put the updates to pci_ids.h into a separate patch, so that the PCI maintainer can ACK it separately? ... apart from that, the patch looks very fine to me now, so if you've at least address the ">=3D" vs. ">" issue, feel free to add my "Reviewed-by: Thomas Huth " Thomas