From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51024) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zh9BP-0003gN-KP for qemu-devel@nongnu.org; Wed, 30 Sep 2015 00:40:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zh9BO-0007NO-0x for qemu-devel@nongnu.org; Wed, 30 Sep 2015 00:39:59 -0400 Date: Wed, 30 Sep 2015 14:33:19 +1000 From: David Gibson Message-ID: <20150930043319.GA23574@voom> References: <1443090459-9281-1-git-send-email-lvivier@redhat.com> <1443090459-9281-3-git-send-email-lvivier@redhat.com> <20150929051844.GM19428@voom.redhat.com> <560A4DCB.1080109@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="8t9RHnE3ZwKMSgU+" Content-Disposition: inline In-Reply-To: <560A4DCB.1080109@redhat.com> Subject: Re: [Qemu-devel] [PATCH v4 2/2] spapr: generate DT node names List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laurent Vivier Cc: thuth@redhat.com, "Michael S. Tsirkin" , qemu-ppc@nongnu.org, Alexander Graf , qemu-devel@nongnu.org --8t9RHnE3ZwKMSgU+ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Sep 29, 2015 at 10:37:31AM +0200, Laurent Vivier wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA256 >=20 >=20 >=20 > 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). > >>=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. > >>=20 > >> Example: > >>=20 > >> Hotplugging some PCI cards with QEMU monitor: > >>=20 > >> device_add virtio-tablet-pci device_add virtio-serial-pci=20 > >> device_add virtio-mouse-pci device_add virtio-scsi-pci device_add > >> virtio-gpu-pci device_add ne2k_pci device_add nec-usb-xhci=20 > >> device_add intel-hda > >>=20 > >> What we can see in linux device tree: > >>=20 > >> for dir in /proc/device-tree/pci@800000020000000/*@*/; do echo > >> $dir cat $dir/name echo done > >>=20 > >> WITHOUT this patch: > >>=20 > >> /proc/device-tree/pci@800000020000000/pci@0/ pci=20 > >> /proc/device-tree/pci@800000020000000/pci@1/ pci=20 > >> /proc/device-tree/pci@800000020000000/pci@2/ pci=20 > >> /proc/device-tree/pci@800000020000000/pci@3/ pci=20 > >> /proc/device-tree/pci@800000020000000/pci@4/ pci=20 > >> /proc/device-tree/pci@800000020000000/pci@5/ pci=20 > >> /proc/device-tree/pci@800000020000000/pci@6/ pci=20 > >> /proc/device-tree/pci@800000020000000/pci@7/ pci > >>=20 > >> WITH this patch: > >>=20 > >> /proc/device-tree/pci@800000020000000/communication-controller@1/ > >> > >>=20 > communication-controller > >> /proc/device-tree/pci@800000020000000/display@4/ display=20 > >> /proc/device-tree/pci@800000020000000/ethernet@5/ ethernet=20 > >> /proc/device-tree/pci@800000020000000/input-controller@0/=20 > >> input-controller /proc/device-tree/pci@800000020000000/mouse@2/=20 > >> mouse /proc/device-tree/pci@800000020000000/multimedia-device@7/=20 > >> multimedia-device /proc/device-tree/pci@800000020000000/scsi@3/=20 > >> scsi /proc/device-tree/pci@800000020000000/usb-xhci@6/ usb-xhci > >>=20 > >> Signed-off-by: Laurent Vivier Reviewed-by: > >> Thomas Huth > >=20 > > Concept and approach seem good to me. > >=20 > >=20 > >> --- hw/ppc/spapr_pci.c | 292 > >> ++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file > >> changed, 278 insertions(+), 14 deletions(-) > >>=20 > >> 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 @@ > >>=20 > >> #include "hw/pci/pci_bridge.h" #include "hw/pci/pci_bus.h"=20 > >> +#include "hw/pci/pci_ids.h" #include "hw/ppc/spapr_drc.h"=20 > >> #include "sysemu/device_tree.h" > >>=20 > >> @@ -944,6 +945,276 @@ static void > >> populate_resource_props(PCIDevice *d, ResourceProps *rp)=20 > >> 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; +}; +=20 > >> +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; +}; +=20 > >> +static const PCISubClass undef_subclass[] =3D { + { > >> IFACE(PCI_CLASS_NOT_DEFINED_VGA), "display", NULL }, > >=20 > > These IFACE and SUBCLASS macro calls seem ugly to me. What's the=20 > > purpose of them? >=20 > 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. >=20 > And on this line, it's wrong, we should use SUBCLASS(): >=20 > #define PCI_CLASS_NOT_DEFINED 0x0000 >=20 > So class is "0x00" >=20 > #define PCI_CLASS_NOT_DEFINED_VGA 0x0001 >=20 > subclass is "0x01". >=20 > For the case of iface: >=20 > #define PCI_BASE_CLASS_SERIAL 0x0c >=20 > class is "serial", 0x0c >=20 > #define PCI_CLASS_SERIAL_USB 0x0c03 >=20 > subclass is "usb", 0x03 >=20 > #define PCI_CLASS_SERIAL_USB_XHCI 0x0c0330 >=20 > iface is "xhci", 0x30 >=20 > 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. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --8t9RHnE3ZwKMSgU+ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWC2YPAAoJEGw4ysog2bOSRNEQAJZXXj9ILYpSOwko7OPjZOhm EynpdsTwE7OkrnoSjzMu+AfRuAW/TkG4st60W5n6MPix1NP2Hx1Gc/Z2auPMlIrr BrW+tJw99VU5L9xQt0McOYMAYH522ZftC9yIVjkAvmEW8/i9dJJldH2C66QtN4zs kTdxlTIQiVIgQW+WqRB7WF8tADdWx1FTI5s0YKOYTAZios3F3gA+C5yPZ8IluRIS GZn2dGEiGLToF38vuGq8p8cQfmLsgIwuLoqJZigjMsdWuCMG2dVyEfD/JBJm26qT px2CRKJ83aip4GCj+HqwSbG0YiW5XruZboaSlKuQf+ZIckcWmjoAOvkd/VPDWyAb OJAGH2dr8dud8Bk/c1R75PXvq+sCB4yCbmFR2U0U5mtsn/YaliMBXTqxX3/5I0fv 8Zxpn2bHlWcyqzl4g2f4T29euSr4k3naRN6qhs9ovvpxcz0wZZI8oWhURY3/6k7h RBnXhDerzLfN52Zl3IwSPeUOeC4r1KrybATy5s97T/mDmpWAqQ+qwpk+iPm3zap2 OJ/y5V/2bDuUE8KFkNWCK7PNu/+cOP+r4evUF3u+z3qw3smuDF1/OGLyCqRUQ4in TjwkPz6n+e2YlyuK6a0o8WDEhEV29symfYXo/8blA42ZOR8qI26bSslApBkDe27f SMUKe6Ri/jIaKn12mIwk =gfac -----END PGP SIGNATURE----- --8t9RHnE3ZwKMSgU+--