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