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

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

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

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.

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

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]