[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 07/16] spapr_rtas: add ibm, configure-connect
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH v5 07/16] spapr_rtas: add ibm, configure-connector RTAS interface |
Date: |
Fri, 27 Feb 2015 16:31:20 +1100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Thu, Feb 26, 2015 at 04:21:55PM -0600, Michael Roth wrote:
> Quoting David Gibson (2015-02-24 18:48:23)
> > On Tue, Feb 24, 2015 at 02:43:45PM -0600, Michael Roth wrote:
> > > Quoting David Gibson (2015-02-24 00:40:32)
> > > > On Mon, Feb 16, 2015 at 08:27:43AM -0600, Michael Roth wrote:
[snip]
> > > > > + uint64_t wa_offset;
> > > > > + uint32_t drc_index;
> > > > > + sPAPRDRConnector *drc;
> > > > > + sPAPRDRConnectorClass *drck;
> > > > > + sPAPRDRCCResponse resp;
> > > > > + const struct fdt_property *prop = NULL;
> > > > > + char *prop_name = NULL;
> > > > > + int prop_len, rc;
> > > > > +
> > > > > + drc_index = rtas_ld(wa_addr, 0);
> > > > > + drc = spapr_dr_connector_by_index(drc_index);
> > > > > + if (!drc) {
> > > > > + DPRINTF("rtas_ibm_configure_connector: invalid sensor/DRC
> > > > > index: %xh\n",
> > > > > + drc_index);
> > > > > + rc = RTAS_OUT_PARAM_ERROR;
> > > > > + goto out;
> > > > > + }
> > > > > + drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > > > + resp = drck->configure_connector(drc, &prop_name, &prop,
> > > > > &prop_len);
> > > >
> > > > You may have answered this last time round, but if so I forgot the
> > > > reason.
> > > >
> > > > Why does the awkward iteration need to go down to the drck callback?
> > > > Coudln't the drck callback part just supply the fdt fragment blob,
> > > > then have generic code which streams it out via iteration?
> > > >
> > > > Obviously we have to support the horrid PAPR interface, but it would
> > > > be nice to confine the PAPR derived horridness to as small an area as
> > > > we can.
> > >
> > > That horrid interface percolates all the way up the QEMU stack,
> > > unfortunately :)
> > >
> > > Upon successfully having it's device tree node received, a DRC transitions
> > > to a 'configured' state that's defined in the DR state machine (PAPR+
> > > 13.4).
> > >
> > > We need to track that state, since it's used to differentiate between
> > > a case where a device is set to 'isolated' as part of entity-sense/device
> > > configuration, as opposed to 'isolated' as part the unplug path. The
> > > overlap between the 2 can occur if we do device_add followed by an
> > > immediate
> > > device_del, but since the 'configured' transition must occur before the
> > > latter,
> > > it becomes unambiguous.
> > >
> > > It's also possible that a guest might be reset in the middle of a series
> > > of
> > > calls to configure-connector, in which case that state needs to be reset.
> > > This
> > > is currently handled by sPAPRDRConnector's reset hook, so if we moved that
> > > aspect out I think we'd need to wire up a reset hook for the
> > > configure-connector
> > > state, which is kinda messy. We'd also need a list of some sort, keyed by
> > > the DRC indexes, to handle the subsequent call-per-iteration's (no
> > > guarantee
> > > only one device configuration is 'in-flight' at a time), so we end up
> > > duplicating a lot of tracking/functionality.
> >
> > Hmm. You should still be able to handle that with just 2 hooks and 1
> > bit of state right? Say "start_configuration" and "end_configuration"
> > or similar. start_configuration gets the blob from the backend, then
> > end_configuration is called once RTAS has finished streaming it out to
> > the guest and updates to the cnofigured state.
>
> {start,end}_configuration callbacks would work for handling
> the normal 'configured' state transitions induced by the call, but we'd also
> need hooks in the "other direction" for a couple cases:
>
> This scenario for instance:
>
> qemu: guest:
> add device0 to drc0
> drmgr0: rtas-configure-connector drc0
> drc0->start_configuration...
> drmgr0: rtas-configure-connector drc0
> drmgr0: rtas-configure-connector drc0
> ...
> drmgr0: rtas-configure-connector drc0
> del device0 <device0 removal pending>
> drmgr0: rtas-configure-connector drc0
> system_reset
> device0 removed by drc0
> reset hook
> add device1 to drc0 drmgr0: rtas-configure-connector drc0
> <begins fetching stale FDT>
>
> So I think we'd need at least a reset hook wired up to the RTAS state.
>
> It's also possible for the guest to force a transition out of the
> configured state by ISOLATE'ing the device. This can happen in the
> middle of the guests configure-connector calls if there's an error. If
> rtas-configure-connector is the one generating the error, it can anticipate
> this and automatically reset the state, but in some cases the error is
> guest internal: the get_node() in src/drmgr/rtas_calls.c can fail for
> memory allocation errors, or unexpected workarea structure, after which
> point it simply stops calling rtas-configure-connector and ISOLATEs the
> device. If we don't track that, a subsequent unplug/plug could also
> result in stale FDT fragments being sent to the guest.
>
> So we'd the RTAS state tracking code to provide an interface of some
> sort for the DRC code to call into, which results in a lot of
> duplicated state-tracking.
>
> It's a bit unintuitive, but those FDT bits are closely coupled to the DRC
> state, so in the end I think that ends up being the most straight-forward
> place to manage them.
Hrm. I'm still not following. I mean, I see that the back-end needs
to be aware of the three-valued state: CC-not-called /
CC-calls-in-progress / CC-calls-completed, but I'm not seeing why it
needs to be aware of the individual CC calls.
Could we have instead a set_state() callback of some sort, that
handles both some of the existing architected states like ISOLATED,
and also special states like CC-in-progress?
--
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
pgpp07gvV_Flp.pgp
Description: PGP signature
- Re: [Qemu-devel] [PATCH v5 02/16] spapr_drc: initial implementation of sPAPRDRConnector device, (continued)
- [Qemu-devel] [PATCH v5 03/16] spapr_rtas: add get/set-power-level RTAS interfaces, Michael Roth, 2015/02/16
- [Qemu-devel] [PATCH v5 05/16] spapr_rtas: add get-sensor-state RTAS interface, Michael Roth, 2015/02/16
- [Qemu-devel] [PATCH v5 07/16] spapr_rtas: add ibm, configure-connector RTAS interface, Michael Roth, 2015/02/16
- Re: [Qemu-devel] [PATCH v5 07/16] spapr_rtas: add ibm, configure-connector RTAS interface, David Gibson, 2015/02/24
- Re: [Qemu-devel] [PATCH v5 07/16] spapr_rtas: add ibm, configure-connector RTAS interface, Michael Roth, 2015/02/24
- Re: [Qemu-devel] [PATCH v5 07/16] spapr_rtas: add ibm, configure-connector RTAS interface, David Gibson, 2015/02/24
- Re: [Qemu-devel] [PATCH v5 07/16] spapr_rtas: add ibm, configure-connector RTAS interface, Michael Roth, 2015/02/26
- Re: [Qemu-devel] [PATCH v5 07/16] spapr_rtas: add ibm, configure-connector RTAS interface,
David Gibson <=
- Re: [Qemu-devel] [PATCH v5 07/16] spapr_rtas: add ibm, configure-connector RTAS interface, Michael Roth, 2015/02/27
[Qemu-devel] [PATCH v5 08/16] spapr_events: re-use EPOW event infrastructure for hotplug events, Michael Roth, 2015/02/16