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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5379 bytes --]

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.

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]