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
pgpJLHjXV3cTa.pgp
Description: PGP signature
