Re: [Qemu-devel] [PATCH v4 16/17] spapr_pci: enable basic hotplug operations

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

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

On Mon, Jan 26, 2015 at 03:17:31PM -0600, Michael Roth wrote:
> Quoting David Gibson (2015-01-18 23:58:28)
> > On Tue, Dec 23, 2014 at 06:30:30AM -0600, Michael Roth wrote:
[snip]
> > > +/* create OF node for pci device and required OF DT properties */
> > > +static void *spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice *dev,
> > > +                                       int drc_index, int *dt_offset)
> > > +{
> > > +    void *fdt_orig, *fdt;
> > > +    int offset, ret;
> > > +    int slot = PCI_SLOT(dev->devfn);
> > > +    char nodename[512];
> > > +
> > > +    fdt_orig = g_malloc0(FDT_MAX_SIZE);
> > > +    offset = fdt_create(fdt_orig, FDT_MAX_SIZE);
> > > +    fdt_begin_node(fdt_orig, "");
> > > +    fdt_end_node(fdt_orig);
> > > +    fdt_finish(fdt_orig);
> > 
> > Recent versions of libfdt have an fdt_create_empty_tree() function to
> > simplify that standard idiom.
> 
> Hmm, it doesn't seem to be in the source that qemu.git/dtc points to, so I'm
> hesitant to rely on it. Would it be viable to get the QEMU submodule
> updated to v1.4.0?

Ah, right.  Yes, we should probably update the qemu submodule, but I
don't think your patches should have to wait on that.


> > > +    fdt = g_malloc0(FDT_MAX_SIZE);
> > > +    fdt_open_into(fdt_orig, fdt, FDT_MAX_SIZE);
> > 
> > There's no need for a second malloc here - fdt_open_into() may be used
> > in place.
> > 
> > > +    sprintf(nodename, "pci@%d", slot);
> > > +    offset = fdt_add_subnode(fdt, 0, nodename);
> > > +    ret = spapr_populate_pci_child_dt(dev, fdt, offset, phb->index, drc_index);
> > > +    g_assert(!ret);
> > > +    g_free(fdt_orig);
> > > +
> > > +    *dt_offset = offset;
> > > +    return fdt;
> > > +}
> > > +
> > > +static void spapr_device_hotplug_add(sPAPRDRConnector *drc,
> > > +                                     sPAPRPHBState *phb,
> > > +                                     PCIDevice *pdev)
> > > +{
> > > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +    DeviceState *dev = DEVICE(pdev);
> > > +    int drc_index = drck->get_index(drc);
> > > +    void *fdt = NULL;
> > > +    int fdt_start_offset = 0;
> > > +
> > > +    /* boot-time devices get their device tree node created by SLOF, but for
> > > +     * hotplugged devices we need QEMU to generate it so the guest can fetch
> > > +     * it via RTAS
> > 
> > Now that we have to have this code in qemu for the hotplug case we may
> > want to consider using it for boot-time devices as well, and removing
> > the corresponding code from SLOF, but that's a problem for another day.
> 
> Makes sense, since we do this for PHBs already. Can look into it as
> a follow-up.

Ok, great.

> > > +     */
> > > +    if (dev->hotplugged) {
> > > +        fdt = spapr_create_pci_child_dt(phb, pdev, drc_index,
> > > +                                        &fdt_start_offset);
> > > +    }
> > > +    drck->attach(drc, DEVICE(pdev), fdt, fdt_start_offset, !dev->hotplugged);
> > > +}
> > > +
> > > +static void spapr_device_hotplug_remove_cb(DeviceState *dev, void *opaque)
> > > +{
> > > +    object_unparent(OBJECT(dev));
> > > +}
> > > +
> > > +static void spapr_device_hotplug_remove(sPAPRDRConnector *drc,
> > > +                                        sPAPRPHBState *phb,
> > > +                                        PCIDevice *pdev)
> > > +{
> > > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +
> > > +    drck->detach(drc, DEVICE(pdev), spapr_device_hotplug_remove_cb, phb);
> > > +}
> > > +
> > > +static void spapr_phb_hot_plug(HotplugHandler *plug_handler,
> > > +                               DeviceState *plugged_dev, Error **errp)
> > 
> > So, this function is hotplugging a PCI device into an existing PHB,
> > rather than hotplugging a PHB itself.  Since the DR protocol does
> > support both operations, I could see this name becoming confusing.
> > 
> > > +{
> > > +    sPAPRPHBState *phb = SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler));
> > > +    PCIDevice *pdev = PCI_DEVICE(plugged_dev);
> > > +    sPAPRDRConnector *drc =
> > > +        spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI, pdev->devfn);
> > 
> > Is it safe to call this before checking phb->dr_enabled?
> 
> It will be NULL if the DRC wasn't created, so the assertion below the check
> should catch any misuse before it happens.

Ok.

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