From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35574) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bvvnd-0004Rb-U0 for qemu-devel@nongnu.org; Sun, 16 Oct 2016 20:29:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bvvna-0006hj-0K for qemu-devel@nongnu.org; Sun, 16 Oct 2016 20:29:05 -0400 Date: Mon, 17 Oct 2016 10:39:44 +1100 From: David Gibson Message-ID: <20161016233944.GE25390@umbus.fritz.box> References: <1476314039-9520-1-git-send-email-mdroth@linux.vnet.ibm.com> <1476314039-9520-9-git-send-email-mdroth@linux.vnet.ibm.com> <20161014045643.GJ28562@umbus> <20161014184417.29726.34906@loki> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="FFoLq8A0u+X9iRU8" Content-Disposition: inline In-Reply-To: <20161014184417.29726.34906@loki> Subject: Re: [Qemu-devel] [PATCH 08/11] spapr_events: add support for dedicated hotplug event source List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, bharata@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com, jallen@linux.vnet.ibm.com --FFoLq8A0u+X9iRU8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Oct 14, 2016 at 01:44:17PM -0500, Michael Roth wrote: > Quoting David Gibson (2016-10-13 23:56:43) > > On Wed, Oct 12, 2016 at 06:13:56PM -0500, Michael Roth wrote: > > > Hotplug events were previously delivered using an EPOW interrupt > > > and were queued by linux guests into a circular buffer. For tradition= al > > > EPOW events like shutdown/resets, this isn't an issue, but for hotplug > > > events there are cases where this buffer can be exhausted, resulting > > > in the loss of hotplug events, resets, etc. > > >=20 > > > Newer-style hotplug event are delivered using a dedicated event sourc= e. > > > We enable this in supported guests by adding standard an additional > > > event source in the guest device-tree via /event-sources, and, if > > > the guest advertises support for the newer-style hotplug events, > > > using the corresponding interrupt to signal the available of > > > hotplug/unplug events. > > >=20 > > > Signed-off-by: Michael Roth > >=20 > > So.. are you saying that as well as allowing new event types, the new > > special hotplug event souce effectively allows for a bigger queue? > >=20 > > Does that mean that we didn't even necessarily need the base+length > > unplug events, because we could now have sent the many single-LMB > > unplug requests that were necessary? Or does it not increase the > > effective queue enough for that? >=20 > I assume there are still some internal limits, but the events > are now processed using a workqueue which should provide a bit more > headroom than the RTAS event buffer used for EPOW events. Maybe > John (on cc:) can provide more insight into what the actual limits >=20 > In either case, the possibility for huge memory hotplug/unplug > situations still warrant the use of count+index I think, and since > support for the new event delivery mechanism is negotiated using the > same option bit as count+index, there's not really any reason not to > use it when we can. For situations where we can't (CPU/PCI), it > might give us a bit of breathing room there as well. Ok, makes sense. Thanks for the extra information. >=20 > >=20 > > > --- > > > hw/ppc/spapr.c | 10 ++-- > > > hw/ppc/spapr_events.c | 148 ++++++++++++++++++++++++++++++++++++++-= ---------- > > > include/hw/ppc/spapr.h | 3 +- > > > 3 files changed, 120 insertions(+), 41 deletions(-) > > >=20 > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > index d80a6fa..2037222 100644 > > > --- a/hw/ppc/spapr.c > > > +++ b/hw/ppc/spapr.c > > > @@ -275,8 +275,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_= base, > > > hwaddr initrd_size, > > > hwaddr kernel_size, > > > bool little_endian, > > > - const char *kernel_cmdline, > > > - uint32_t epow_irq) > > > + const char *kernel_cmdline) > > > { > > > void *fdt; > > > uint32_t start_prop =3D cpu_to_be32(initrd_base); > > > @@ -437,7 +436,7 @@ static void *spapr_create_fdt_skel(hwaddr initrd_= base, > > > _FDT((fdt_end_node(fdt))); > > > =20 > > > /* event-sources */ > > > - spapr_events_fdt_skel(fdt, epow_irq); > > > + spapr_events_fdt_skel(fdt); > > > =20 > > > /* /hypervisor node */ > > > if (kvm_enabled()) { > > > @@ -1944,7 +1943,7 @@ static void ppc_spapr_init(MachineState *machin= e) > > > } > > > g_free(filename); > > > =20 > > > - /* Set up EPOW events infrastructure */ > > > + /* Set up RTAS event infrastructure */ > > > spapr_events_init(spapr); > > > =20 > > > /* Set up the RTC RTAS interfaces */ > > > @@ -2076,8 +2075,7 @@ static void ppc_spapr_init(MachineState *machin= e) > > > /* Prepare the device tree */ > > > spapr->fdt_skel =3D spapr_create_fdt_skel(initrd_base, initrd_si= ze, > > > kernel_size, kernel_le, > > > - kernel_cmdline, > > > - spapr->check_exception_i= rq); > > > + kernel_cmdline); > > > assert(spapr->fdt_skel !=3D NULL); > > > =20 > > > /* used by RTAS */ > > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > > > index 4c7b6ae..f8bbec6 100644 > > > --- a/hw/ppc/spapr_events.c > > > +++ b/hw/ppc/spapr_events.c > > > @@ -40,6 +40,7 @@ > > > #include "hw/ppc/spapr_drc.h" > > > #include "qemu/help_option.h" > > > #include "qemu/bcd.h" > > > +#include "hw/ppc/spapr_ovec.h" > > > #include > > > =20 > > > struct rtas_error_log { > > > @@ -206,28 +207,104 @@ struct hp_log_full { > > > struct rtas_event_log_v6_hp hp; > > > } QEMU_PACKED; > > > =20 > > > -#define EVENT_MASK_INTERNAL_ERRORS 0x80000000 > > > -#define EVENT_MASK_EPOW 0x40000000 > > > -#define EVENT_MASK_HOTPLUG 0x10000000 > > > -#define EVENT_MASK_IO 0x08000000 > > > +typedef enum EventClassIndex { > > > + EVENT_CLASS_INTERNAL_ERRORS =3D 0, > > > + EVENT_CLASS_EPOW =3D 1, > > > + EVENT_CLASS_RESERVED =3D 2, > > > + EVENT_CLASS_HOT_PLUG =3D 3, > > > + EVENT_CLASS_IO =3D 4, > > > + EVENT_CLASS_MAX > > > +} EventClassIndex; > > > + > > > +#define EVENT_CLASS_MASK(index) (1 << (31 - index)) > > > + > > > +typedef struct EventSource { > > > + const char *name; > > > + int irq; > > > + uint32_t mask; > > > + bool enabled; > > > +} EventSource; > > > + > > > +static EventSource event_source[EVENT_CLASS_MAX] =3D { > > > + [EVENT_CLASS_INTERNAL_ERRORS] =3D { .name =3D "internal-er= rors", }, > > > + [EVENT_CLASS_EPOW] =3D { .name =3D "epow-events= ", }, > > > + [EVENT_CLASS_HOT_PLUG] =3D { .name =3D "hot-plug-ev= ents", }, > > > + [EVENT_CLASS_IO] =3D { .name =3D "ibm,io-even= ts", }, > > > +}; > > > + > > > +static void rtas_event_source_register(EventClassIndex index, int ir= q) > > > +{ > > > + /* we only support 1 irq per event class at the moment */ > > > + g_assert(!event_source[index].enabled); > > > + event_source[index].irq =3D irq; > > > + event_source[index].mask =3D EVENT_CLASS_MASK(index); > > > + event_source[index].enabled =3D true; > > > +} > >=20 > > I really don't like adding a mutable global table. This should > > probably be under the sPAPRMachineState. >=20 > I think I started off going that route. Not sure what steered me in this > direction, but will take another look at that approach. >=20 > >=20 > > > -void spapr_events_fdt_skel(void *fdt, uint32_t check_exception_irq) > > > +void spapr_events_fdt_skel(void *fdt) > > > { > > > - uint32_t irq_ranges[] =3D {cpu_to_be32(check_exception_irq), cpu= _to_be32(1)}; > > > - uint32_t interrupts[] =3D {cpu_to_be32(check_exception_irq), 0}; > > > + uint32_t irq_ranges[EVENT_CLASS_MAX * 2]; > > > + int i, count =3D 0; > > > =20 > > > _FDT((fdt_begin_node(fdt, "event-sources"))); > > > =20 > > > + for (i =3D 0, count =3D 0; i < EVENT_CLASS_MAX; i++) { > > > + /* TODO: what does 0 entail? */ > >=20 > > It's just part of the interrupt specifier format expected by the > > event-sources binding. It's not really important. > >=20 > > > + uint32_t interrupts[] =3D { cpu_to_be32(event_source[i].irq)= , 0 }; > > > + > > > + if (!event_source[i].enabled) { > > > + continue; > > > + } > > > + > > > + _FDT((fdt_begin_node(fdt, event_source[i].name))); > > > + _FDT((fdt_property(fdt, "interrupts", interrupts, sizeof(int= errupts)))); > > > + _FDT((fdt_end_node(fdt))); > > > + > > > + irq_ranges[count++] =3D interrupts[0]; > > > + irq_ranges[count++] =3D cpu_to_be32(1); > > > + } > > > + > > > + /* TODO: confirm the count is the last expected element */ > > > + irq_ranges[count] =3D cpu_to_be32(count); > > > + count++; > > > + > > > _FDT((fdt_property(fdt, "interrupt-controller", NULL, 0))); > > > _FDT((fdt_property_cell(fdt, "#interrupt-cells", 2))); > > > _FDT((fdt_property(fdt, "interrupt-ranges", > > > - irq_ranges, sizeof(irq_ranges)))); > > > + irq_ranges, count * sizeof(uint32_t)))); > > > =20 > > > - _FDT((fdt_begin_node(fdt, "epow-events"))); > > > - _FDT((fdt_property(fdt, "interrupts", interrupts, sizeof(interru= pts)))); > > > _FDT((fdt_end_node(fdt))); > > > +} > > > =20 > > > - _FDT((fdt_end_node(fdt))); > > > +static const EventSource *rtas_event_log_to_source(int log_type) > > > +{ > > > + const EventSource *source; > > > + > > > + switch (log_type) { > > > + case RTAS_LOG_TYPE_HOTPLUG: > > > + source =3D &event_source[EVENT_CLASS_HOT_PLUG]; > > > + if (event_source[EVENT_CLASS_HOT_PLUG].enabled) { > > > + break; > > > + } > >=20 > > This should probably be using the flag you already have in the > > MachineState, rather than a global. > >=20 > > > + /* fall back to epow for legacy hotplug interrupt source= */ > > > + case RTAS_LOG_TYPE_EPOW: > > > + source =3D &event_source[EVENT_CLASS_EPOW]; > > > + break; > > > + default: > > > + source =3D NULL; > > > + } > > > + > > > + return source; > > > +} > > > + > > > +static int rtas_event_log_to_irq(int log_type) > > > +{ > > > + const EventSource *source =3D rtas_event_log_to_source(log_type); > > > + > > > + g_assert(source); > > > + g_assert(source->enabled); > > > + > > > + return source->irq; > > > } > > > =20 > > > static void rtas_event_log_queue(int log_type, void *data, bool exce= ption) > > > @@ -248,19 +325,14 @@ static sPAPREventLogEntry *rtas_event_log_deque= ue(uint32_t event_mask, > > > sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > > > sPAPREventLogEntry *entry =3D NULL; > > > =20 > > > - /* we only queue EPOW events atm. */ > > > - if ((event_mask & EVENT_MASK_EPOW) =3D=3D 0) { > > > - return NULL; > > > - } > > > - > > > QTAILQ_FOREACH(entry, &spapr->pending_events, next) { > > > + const EventSource *source =3D rtas_event_log_to_source(entry= ->log_type); > > > + > > > if (entry->exception !=3D exception) { > > > continue; > > > } > > > =20 > > > - /* EPOW and hotplug events are surfaced in the same manner */ > > > - if (entry->log_type =3D=3D RTAS_LOG_TYPE_EPOW || > > > - entry->log_type =3D=3D RTAS_LOG_TYPE_HOTPLUG) { > > > + if (source->mask & event_mask) { > > > break; > > > } > > > } > > > @@ -277,19 +349,14 @@ static bool rtas_event_log_contains(uint32_t ev= ent_mask, bool exception) > > > sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > > > sPAPREventLogEntry *entry =3D NULL; > > > =20 > > > - /* we only queue EPOW events atm. */ > > > - if ((event_mask & EVENT_MASK_EPOW) =3D=3D 0) { > > > - return false; > > > - } > > > - > > > QTAILQ_FOREACH(entry, &spapr->pending_events, next) { > > > + const EventSource *source =3D rtas_event_log_to_source(entry= ->log_type); > > > + > > > if (entry->exception !=3D exception) { > > > continue; > > > } > > > =20 > > > - /* EPOW and hotplug events are surfaced in the same manner */ > > > - if (entry->log_type =3D=3D RTAS_LOG_TYPE_EPOW || > > > - entry->log_type =3D=3D RTAS_LOG_TYPE_HOTPLUG) { > > > + if (source->mask & event_mask) { > > > return true; > > > } > > > } > > > @@ -377,7 +444,8 @@ static void spapr_powerdown_req(Notifier *n, void= *opaque) > > > =20 > > > rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true); > > > =20 > > > - qemu_irq_pulse(xics_get_qirq(spapr->xics, spapr->check_exception= _irq)); > > > + qemu_irq_pulse(xics_get_qirq(spapr->xics, > > > + rtas_event_log_to_irq(RTAS_LOG_TYPE= _EPOW))); > > > } > > > =20 > > > static void spapr_hotplug_set_signalled(uint32_t drc_index) > > > @@ -459,7 +527,8 @@ static void spapr_hotplug_req_event(uint8_t hp_id= , uint8_t hp_action, > > > =20 > > > rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true); > > > =20 > > > - qemu_irq_pulse(xics_get_qirq(spapr->xics, spapr->check_exception= _irq)); > > > + qemu_irq_pulse(xics_get_qirq(spapr->xics, > > > + rtas_event_log_to_irq(RTAS_LOG_TYPE= _HOTPLUG))); > > > } > > > =20 > > > void spapr_hotplug_req_add_by_index(sPAPRDRConnector *drc) > > > @@ -505,6 +574,7 @@ static void check_exception(PowerPCCPU *cpu, sPAP= RMachineState *spapr, > > > uint64_t xinfo; > > > sPAPREventLogEntry *event; > > > struct rtas_error_log *hdr; > > > + int i; > > > =20 > > > if ((nargs < 6) || (nargs > 7) || nret !=3D 1) { > > > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); > > > @@ -541,8 +611,11 @@ static void check_exception(PowerPCCPU *cpu, sPA= PRMachineState *spapr, > > > * do the latter here, since our code relies on edge-triggered > > > * interrupts. > > > */ > > > - if (rtas_event_log_contains(mask, true)) { > > > - qemu_irq_pulse(xics_get_qirq(spapr->xics, spapr->check_excep= tion_irq)); > > > + for (i =3D 0; i < EVENT_CLASS_MAX; i++) { > > > + if (rtas_event_log_contains(EVENT_CLASS_MASK(i), true)) { > > > + g_assert(event_source[i].enabled); > > > + qemu_irq_pulse(xics_get_qirq(spapr->xics, event_source[i= ].irq)); > > > + } > > > } > > > =20 > > > return; > > > @@ -594,8 +667,17 @@ out_no_events: > > > void spapr_events_init(sPAPRMachineState *spapr) > > > { > > > QTAILQ_INIT(&spapr->pending_events); > > > - spapr->check_exception_irq =3D xics_spapr_alloc(spapr->xics, 0, = 0, false, > > > - &error_fatal); > > > + > > > + rtas_event_source_register(EVENT_CLASS_EPOW, > > > + xics_spapr_alloc(spapr->xics, 0, 0, f= alse, > > > + &error_fatal)); > > > + > > > + if (spapr->use_hotplug_event_source) { > > > + rtas_event_source_register(EVENT_CLASS_HOT_PLUG, > > > + xics_spapr_alloc(spapr->xics, 0, = 0, false, > > > + &error_fatal)); > > > + } > > > + > > > spapr->epow_notifier.notify =3D spapr_powerdown_req; > > > qemu_register_powerdown_notifier(&spapr->epow_notifier); > > > spapr_rtas_register(RTAS_CHECK_EXCEPTION, "check-exception", > > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > > > index d1a4a14..2295ac6 100644 > > > --- a/include/hw/ppc/spapr.h > > > +++ b/include/hw/ppc/spapr.h > > > @@ -71,7 +71,6 @@ struct sPAPRMachineState { > > > sPAPROptionVector *ov5_cas; > > > bool cas_reboot; > > > =20 > > > - uint32_t check_exception_irq; > > > Notifier epow_notifier; > > > QTAILQ_HEAD(, sPAPREventLogEntry) pending_events; > > > bool use_hotplug_event_source; > > > @@ -579,7 +578,7 @@ struct sPAPREventLogEntry { > > > }; > > > =20 > > > void spapr_events_init(sPAPRMachineState *sm); > > > -void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq); > > > +void spapr_events_fdt_skel(void *fdt); > > > int spapr_h_cas_compose_response(sPAPRMachineState *sm, > > > target_ulong addr, target_ulong siz= e, > > > bool cpu_update, > >=20 >=20 --=20 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 --FFoLq8A0u+X9iRU8 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYBA+9AAoJEGw4ysog2bOSTvwP/3XSQhs1ePXKH56qfJDTWMOo pEsSCGgmifv7K77o4tIyd5RZru+jbg9maLW4OGRooD1b9I5wN6+mhtxo/8JwZlCS gVtcQcLPbzeiZ7+Echq/4fnFb1PR6+FPMU9LpkEP8Xf4NcyLmVWRMeFxLrX19PLr h2Jfy8PMZKrcdNxRF2Ny25OoEowPSm5bOiKoVAMsUlJl08MQPmywwses98pSoUPp EnT1VpqdURnAPmHGJught2reNxsfinO408Jq/FqlEuwuhCqebFfirHn3V6Y/azEe sa9UlgLSMGDyn2o7WV+N6xuCmswNmiYM1X7kWnPsEHrFqizb9ag35Tq7d/3d5nSU i1uq/WHDhM3Vly+MaaIQmTPLxwppib0pg1dIiIqh24vktnUsslh9ZkHxhfbBXzJL f5uS6hVVMgyRvi6sVNLqXo0qlQDRbTgpecewE5KEMjWFp9FHCF2ukyYoAVSXEJhj 9D7MraFGNsbeZvqBTNFnhSpMl87vAi53sipo7bjsOO9r1q2vCXbIQ/Sw+23vVxd4 rHpliU4Un5+D9bgFpU37pr2NEEXkHlg324Yv1QFaKlAg8H7ceEQUlq8mqV5aRbTT Hac5B7pAnlk4lqC76pQmLx88lDiRyAkwkt2j3LHO2DBg7toEUloOvJrdzEpvVUUU f3o9aXlAuw7CYHMlNEIq =xhTi -----END PGP SIGNATURE----- --FFoLq8A0u+X9iRU8--