On Wed, 13 Feb 2019 15:05:24 +1100 David Gibson <da...@gibson.dropbear.id.au> wrote:
> On Tue, Feb 12, 2019 at 07:25:19PM +0100, Greg Kurz wrote:
> > The current logic is to provide the FDT fragment when attaching a device
> > to a DRC. This works perfectly fine for our current hotplug support, but
> > soon we will add support for PHB hotplug which has some constraints, that
> > CPU, PCI and LMB devices don't seem to have.
> >
> > The first constraint is that the "ibm,dma-window" property of the PHB
> > node requires the IOMMU to be configured, ie, spapr_tce_table_enable()
> > has been called, which happens during PHB reset. It is okay in the case
> > of hotplug since the device is reset before the hotplug handler is
> > called. On the contrary with coldplug, the hotplug handler is called
> > first and device is only reset during the initial system reset. Trying
> > to create the FDT fragment on the hotplug path in this case, would
> > result in somthing like this:
> >
> > ibm,dma-window = < 0x80000000 0x00 0x00 0x00 0x00 >;
> >
> > This will cause linux in the guest to panic, by simply removing and
> > re-adding the PHB using the drmgr command:
> >
> > page = alloc_pages_node(nid, GFP_KERNEL, get_order(sz));
> > if (!page)
> > panic("iommu_init_table: Can't allocate %ld bytes\n", sz);
> >
> > The second and maybe more problematic constraint is that the
> > "interrupt-map" property needs to reference the interrupt controller
> > node using the very same phandle that SLOF has already exposed to the
> > guest. QEMU requires SLOF to call the private KVMPPC_H_UPDATE_DT hcall
> > at some point to know about this phandle. With the latest QEMU and SLOF,
> > this happens when SLOF gets quiesced. This means that if the PHB gets
> > hotplugged after CAS but before SLOF quiesce, then we're sure that the
> > phandle is not known when the hotplug handler is called.
> >
> > The FDT is only needed when the guest first invokes RTAS to configure
> > the connector actually, long after SLOF quiesce. Let's postpone the
> > creation of FDT fragments for PHBs to rtas_ibm_configure_connector().
> >
> > Since we only need this for PHBs, introduce a new method in the base
> > DRC class for that. It will implemented for "spapr-drc-phb" DRCs in
> > a subsequent patch.
> >
> > Allow spapr_drc_attach() to be passed a NULL fdt argument if the method
> > is available.
> >
> > Signed-off-by: Greg Kurz <gr...@kaod.org>
>
> The basic solution looks fine. However I don't much like the fact
> that this leaves us with two ways to handle the fdt fragment - either
> at connect time or at configure connector time via a callback. qemu
> already has way to many places where there are confusingly multiple
> ways to do things.
>
> I know it's a detour, but I'd really prefer to convert the existing
> DRC handling to this new callback scheme, rather than have two
> different approaches.
>
Ok. I'll introduce the new callback scheme and convert the existing code
in a separate series.
> > ---
> > hw/ppc/spapr_drc.c | 34 +++++++++++++++++++++++++++++-----
> > include/hw/ppc/spapr_drc.h | 6 ++++++
> > 2 files changed, 35 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 189ee681062a..c5a281915665 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -22,6 +22,7 @@
> > #include "qemu/error-report.h"
> > #include "hw/ppc/spapr.h" /* for RTAS return codes */
> > #include "hw/pci-host/spapr.h" /* spapr_phb_remove_pci_device_cb callback
> > */
> > +#include "sysemu/device_tree.h"
> > #include "trace.h"
> >
> > #define DRC_CONTAINER_PATH "/dr-connector"
> > @@ -376,6 +377,8 @@ static void prop_get_fdt(Object *obj, Visitor *v, const
> > char *name,
> > void spapr_drc_attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt,
> > int fdt_start_offset, Error **errp)
> > {
> > + sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +
> > trace_spapr_drc_attach(spapr_drc_index(drc));
> >
> > if (drc->dev) {
> > @@ -384,11 +387,14 @@ void spapr_drc_attach(sPAPRDRConnector *drc,
> > DeviceState *d, void *fdt,
> > }
> > g_assert((drc->state == SPAPR_DRC_STATE_LOGICAL_UNUSABLE)
> > || (drc->state == SPAPR_DRC_STATE_PHYSICAL_POWERON));
> > - g_assert(fdt);
> > + g_assert(fdt || drck->populate_dt);
> >
> > drc->dev = d;
> > - drc->fdt = fdt;
> > - drc->fdt_start_offset = fdt_start_offset;
> > +
> > + if (fdt) {
> > + drc->fdt = fdt;
> > + drc->fdt_start_offset = fdt_start_offset;
> > + }
> >
> > object_property_add_link(OBJECT(drc), "device",
> > object_get_typename(OBJECT(drc->dev)),
> > @@ -1118,10 +1124,28 @@ static void rtas_ibm_configure_connector(PowerPCCPU
> > *cpu,
> > goto out;
> > }
> >
> > - g_assert(drc->fdt);
> > -
> > drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> >
> > + g_assert(drc->fdt || drck->populate_dt);
> > +
> > + if (!drc->fdt) {
> > + Error *local_err = NULL;
> > + void *fdt;
> > + int fdt_size;
> > +
> > + fdt = create_device_tree(&fdt_size);
> > +
> > + if (drck->populate_dt(drc->dev, spapr, fdt, &drc->fdt_start_offset,
> > + &local_err)) {
> > + g_free(fdt);
> > + error_free(local_err);
> > + rc = SPAPR_DR_CC_RESPONSE_ERROR;
> > + goto out;
> > + }
> > +
> > + drc->fdt = fdt;
> > + }
> > +
> > do {
> > uint32_t tag;
> > const char *name;
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 56bba36ad4da..e947d6987bf2 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -18,6 +18,7 @@
> > #include "qom/object.h"
> > #include "sysemu/sysemu.h"
> > #include "hw/qdev.h"
> > +#include "qapi/error.h"
> >
> > #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> > #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> > @@ -221,6 +222,8 @@ typedef struct sPAPRDRConnector {
> > int fdt_start_offset;
> > } sPAPRDRConnector;
> >
> > +struct sPAPRMachineState;
> > +
> > typedef struct sPAPRDRConnectorClass {
> > /*< private >*/
> > DeviceClass parent;
> > @@ -236,6 +239,9 @@ typedef struct sPAPRDRConnectorClass {
> > uint32_t (*isolate)(sPAPRDRConnector *drc);
> > uint32_t (*unisolate)(sPAPRDRConnector *drc);
> > void (*release)(DeviceState *dev);
> > +
> > + int (*populate_dt)(DeviceState *dev, struct sPAPRMachineState *spapr,
> > + void *fdt, int *fdt_start_offset, Error **errp);
> > } sPAPRDRConnectorClass;
> >
> > typedef struct sPAPRDRCPhysical {
> >
>
pgpdsWPQwiy_m.pgp
Description: OpenPGP digital signature
