Re: [Qemu-devel] [PATCH v4 13/15] spapr_drc: Allow FDT fragment to be added later

[Greg Kurz <groug@kaod.org>]

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

On Wed, 13 Feb 2019 15:05:24 +1100
David Gibson <david@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 <groug@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 {
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]