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 <lviv...@redhat.com> Reviewed-by:
> >> Thomas Huth <th...@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
pgpl6hkKj28e2.pgp
Description: PGP signature
