From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48988) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1buuby-0008Vl-IC for qemu-devel@nongnu.org; Fri, 14 Oct 2016 01:00:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1buubv-0003A1-Ck for qemu-devel@nongnu.org; Fri, 14 Oct 2016 01:00:49 -0400 Date: Fri, 14 Oct 2016 15:56:43 +1100 From: David Gibson Message-ID: <20161014045643.GJ28562@umbus> References: <1476314039-9520-1-git-send-email-mdroth@linux.vnet.ibm.com> <1476314039-9520-9-git-send-email-mdroth@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="g3RkK9jYN81zD2N+" Content-Disposition: inline In-Reply-To: <1476314039-9520-9-git-send-email-mdroth@linux.vnet.ibm.com> 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 --g3RkK9jYN81zD2N+ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 traditional > 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 source. > 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 So.. are you saying that as well as allowing new event types, the new special hotplug event souce effectively allows for a bigger queue? 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? > --- > 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 *machine) > } > 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 *machine) > /* Prepare the device tree */ > spapr->fdt_skel =3D spapr_create_fdt_skel(initrd_base, initrd_size, > kernel_size, kernel_le, > - kernel_cmdline, > - spapr->check_exception_irq); > + 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-errors= ", }, > + [EVENT_CLASS_EPOW] =3D { .name =3D "epow-events", }, > + [EVENT_CLASS_HOT_PLUG] =3D { .name =3D "hot-plug-events= ", }, > + [EVENT_CLASS_IO] =3D { .name =3D "ibm,io-events",= }, > +}; > + > +static void rtas_event_source_register(EventClassIndex index, int irq) > +{ > + /* 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; > +} I really don't like adding a mutable global table. This should probably be under the sPAPRMachineState. > -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? */ It's just part of the interrupt specifier format expected by the event-sources binding. It's not really important. > + 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(interru= pts)))); > + _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(interrupts)= ))); > _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; > + } This should probably be using the flag you already have in the MachineState, rather than a global. > + /* 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 exceptio= n) > @@ -248,19 +325,14 @@ static sPAPREventLogEntry *rtas_event_log_dequeue(u= int32_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->lo= g_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 event_= 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->lo= g_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 *op= aque) > =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_EPO= W))); > } > =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, ui= nt8_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_HOT= PLUG))); > } > =20 > void spapr_hotplug_req_add_by_index(sPAPRDRConnector *drc) > @@ -505,6 +574,7 @@ static void check_exception(PowerPCCPU *cpu, sPAPRMac= hineState *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, sPAPRMa= chineState *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_exception= _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].ir= q)); > + } > } > =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, f= alse, > - &error_fatal); > + > + rtas_event_source_register(EVENT_CLASS_EPOW, > + xics_spapr_alloc(spapr->xics, 0, 0, false, > + &error_fatal)); > + > + if (spapr->use_hotplug_event_source) { > + rtas_event_source_register(EVENT_CLASS_HOT_PLUG, > + xics_spapr_alloc(spapr->xics, 0, 0, f= alse, > + &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 size, > bool cpu_update, --=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 --g3RkK9jYN81zD2N+ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYAGWHAAoJEGw4ysog2bOSvtsP+wQjN1sL//eLoEtCwvKAudNV Z1MFfF8BwtbZmDOJSAsV38ACCU8O8ttOGeF0dWQBdDDgROOwjaovO7sLadphOEH/ itSc2nibC/gMVBzDZoJG9n4/S+rjzcqoPpSIMYVW8r+mUQSqQNoiELOdtvQkMe0v fbBRi8bCJRW3nsgIX5uFSleSxyheHG6VfYtAqe/TYHoB9zf1FU4osaInCuyI6hA9 7Nbo7sqqaqbNP+DE46wkEUFuJKAeUEetwTKgFHrkIQV2lEaNlJxM/Z5O2V2clLer Hei2G7ygvV/SJA+K4MfQFSJ3D2vS1tBNmwWl9y0caDfIhlmFPedcMessTuXQdFZ4 17wYsAW6uqTTO0Hb+jczWuX9mN8brMr/Tjk0F4mcVevhD1ai7kuFlVoEfrMPYAjy xuxUU3uUTSIdkqJkxzsPIOr3t53Ex4s8+j3Cxj93dOdbqb4v2KpifwmvEd+uZ+Hj DfB0hxkZ6H1uoPNJG9hBd5XS9HF7+HhFcOpJDCHNKJ3Rib904tpnLe3nYDBQJrwj H9Kfo2BxiyYQ4xdPt/uVzSRb9iQ62qmRkxUpGK2THtUTsF7xtkXTzV5/yUZIG7wY astzpQ+V9xhHUSJaP6NVPb7h6Io2cIbA8HPPQaGqEhLKA2YJdlIE2Jxn4XUAMKWs eGeRFkBfvaiDL+3lCOL4 =3jf3 -----END PGP SIGNATURE----- --g3RkK9jYN81zD2N+--