From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43572) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bv7bF-0001bG-RO for qemu-devel@nongnu.org; Fri, 14 Oct 2016 14:53:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bv7bA-0001mY-My for qemu-devel@nongnu.org; Fri, 14 Oct 2016 14:52:56 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:53821 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bv7bA-0001mM-ID for qemu-devel@nongnu.org; Fri, 14 Oct 2016 14:52:52 -0400 Received: from pps.filterd (m0098417.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id u9EInSnt040790 for ; Fri, 14 Oct 2016 14:52:52 -0400 Received: from e36.co.us.ibm.com (e36.co.us.ibm.com [32.97.110.154]) by mx0a-001b2d01.pphosted.com with ESMTP id 2630kakg3d-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 14 Oct 2016 14:52:51 -0400 Received: from localhost by e36.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 14 Oct 2016 12:52:51 -0600 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Michael Roth In-Reply-To: <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> <20161014045643.GJ28562@umbus> Date: Fri, 14 Oct 2016 13:44:17 -0500 Message-Id: <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: David Gibson Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, bharata@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com, jallen@linux.vnet.ibm.com 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 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. > > = > > 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. > > = > > 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? 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 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. > = > > --- > > hw/ppc/spapr.c | 10 ++-- > > hw/ppc/spapr_events.c | 148 ++++++++++++++++++++++++++++++++++++++---= -------- > > include/hw/ppc/spapr.h | 3 +- > > 3 files changed, 120 insertions(+), 41 deletions(-) > > = > > 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_ba= se, > > 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_ba= se, > > _FDT((fdt_end_node(fdt))); > > = > > /* event-sources */ > > - spapr_events_fdt_skel(fdt, epow_irq); > > + spapr_events_fdt_skel(fdt); > > = > > /* /hypervisor node */ > > if (kvm_enabled()) { > > @@ -1944,7 +1943,7 @@ static void ppc_spapr_init(MachineState *machine) > > } > > g_free(filename); > > = > > - /* Set up EPOW events infrastructure */ > > + /* Set up RTAS event infrastructure */ > > spapr_events_init(spapr); > > = > > /* 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); > > = > > /* 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 > > = > > struct rtas_error_log { > > @@ -206,28 +207,104 @@ struct hp_log_full { > > struct rtas_event_log_v6_hp hp; > > } QEMU_PACKED; > > = > > -#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-erro= rs", }, > > + [EVENT_CLASS_EPOW] =3D { .name =3D "epow-events",= }, > > + [EVENT_CLASS_HOT_PLUG] =3D { .name =3D "hot-plug-even= ts", }, > > + [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. I think I started off going that route. Not sure what steered me in this direction, but will take another look at that approach. > = > > -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_t= o_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; > > = > > _FDT((fdt_begin_node(fdt, "event-sources"))); > > = > > + 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(inter= rupts)))); > > + _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)))); > > = > > - _FDT((fdt_begin_node(fdt, "epow-events"))); > > - _FDT((fdt_property(fdt, "interrupts", interrupts, sizeof(interrupt= s)))); > > _FDT((fdt_end_node(fdt))); > > +} > > = > > - _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; > > } > > = > > static void rtas_event_log_queue(int log_type, void *data, bool except= ion) > > @@ -248,19 +325,14 @@ static sPAPREventLogEntry *rtas_event_log_dequeue= (uint32_t event_mask, > > sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > > sPAPREventLogEntry *entry =3D NULL; > > = > > - /* 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; > > } > > = > > - /* 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 even= t_mask, bool exception) > > sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); > > sPAPREventLogEntry *entry =3D NULL; > > = > > - /* 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; > > } > > = > > - /* 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) > > = > > rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, new_epow, true); > > = > > - qemu_irq_pulse(xics_get_qirq(spapr->xics, spapr->check_exception_i= rq)); > > + qemu_irq_pulse(xics_get_qirq(spapr->xics, > > + rtas_event_log_to_irq(RTAS_LOG_TYPE_E= POW))); > > } > > = > > 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, > > = > > rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true); > > = > > - qemu_irq_pulse(xics_get_qirq(spapr->xics, spapr->check_exception_i= rq)); > > + qemu_irq_pulse(xics_get_qirq(spapr->xics, > > + rtas_event_log_to_irq(RTAS_LOG_TYPE_H= OTPLUG))); > > } > > = > > void spapr_hotplug_req_add_by_index(sPAPRDRConnector *drc) > > @@ -505,6 +574,7 @@ static void check_exception(PowerPCCPU *cpu, sPAPRM= achineState *spapr, > > uint64_t xinfo; > > sPAPREventLogEntry *event; > > struct rtas_error_log *hdr; > > + int i; > > = > > 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, sPAPR= MachineState *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_excepti= on_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)); > > + } > > } > > = > > 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, fal= se, > > + &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; > > = > > - uint32_t check_exception_irq; > > Notifier epow_notifier; > > QTAILQ_HEAD(, sPAPREventLogEntry) pending_events; > > bool use_hotplug_event_source; > > @@ -579,7 +578,7 @@ struct sPAPREventLogEntry { > > }; > > = > > 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, > = > -- = > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _othe= r_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson