Re: [Qemu-devel] [PATCH 07/10] spapr_events: add support for dedicated hotplug event source

[Michael Roth]

Quoting David Gibson (2016-10-25 19:42:18)
> On Mon, Oct 24, 2016 at 11:47:33PM -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 <address@hidden>
> > ---
> >  hw/ppc/spapr.c         |   4 +-
> >  hw/ppc/spapr_events.c  | 202 
> > ++++++++++++++++++++++++++++++++++++++++---------
> >  include/hw/ppc/spapr.h |   5 +-
> >  3 files changed, 170 insertions(+), 41 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index a3ea140..dc4224b 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -973,7 +973,7 @@ static void *spapr_build_fdt(sPAPRMachineState *spapr,
> >      }
> >  
> >      /* /event-sources */
> > -    spapr_dt_events(fdt, spapr->check_exception_irq);
> > +    spapr_dt_events(spapr, fdt);
> >  
> >      /* /rtas */
> >      spapr_dt_rtas(spapr, fdt);
> > @@ -1917,7 +1917,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 */
> > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> > index 89aa5a7..b6b3511 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 <libfdt.h>
> >  
> >  struct rtas_error_log {
> > @@ -206,27 +207,140 @@ 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 EventClass {
> > +    EVENT_CLASS_INTERNAL_ERRORS     = 0,
> > +    EVENT_CLASS_EPOW                = 1,
> > +    EVENT_CLASS_RESERVED            = 2,
> > +    EVENT_CLASS_HOT_PLUG            = 3,
> > +    EVENT_CLASS_IO                  = 4,
> > +    EVENT_CLASS_MAX
> > +} EventClassIndex;
> > +#define EVENT_CLASS_MASK(index) (1 << (31 - index))
> > +
> > +static const char *event_names[EVENT_CLASS_MAX] = {
> > +    [EVENT_CLASS_INTERNAL_ERRORS]       = "internal-errors",
> > +    [EVENT_CLASS_EPOW]                  = "epow-events",
> > +    [EVENT_CLASS_HOT_PLUG]              = "hot-plug-events",
> > +    [EVENT_CLASS_IO]                    = "ibm,io-events",
> > +};
> > +
> > +struct sPAPREventSource {
> > +    const char *name;
> > +    int irq;
> > +    uint32_t mask;
> > +    bool enabled;
> > +};
> > +
> > +static sPAPREventSource *spapr_event_sources_new(void)
> > +{
> > +    sPAPREventSource *event_sources = g_new0(sPAPREventSource,
> > +                                             EVENT_CLASS_MAX);
> > +    int i;
> > +
> > +    for (i = 0; i < EVENT_CLASS_MAX; i++) {
> > +        event_sources[i].name = event_names[i];
> 
> You don't really need to have the pointer to the name in
> sPAPREventSource.  You only need it for building the DT, and you can
> look up event_names in parallel just as easily there.
> 
> > +    }
> >  
> > -void spapr_dt_events(void *fdt, uint32_t check_exception_irq)
> > +    return event_sources;
> > +}
> > +
> > +static void spapr_event_sources_register(sPAPREventSource *event_sources,
> > +                                        EventClassIndex index, int irq)
> >  {
> > -    int event_sources, epow_events;
> > -    uint32_t irq_ranges[] = {cpu_to_be32(check_exception_irq), 
> > cpu_to_be32(1)};
> > -    uint32_t interrupts[] = {cpu_to_be32(check_exception_irq), 0};
> > +    /* we only support 1 irq per event class at the moment */
> > +    g_assert(event_sources);
> > +    g_assert(!event_sources[index].enabled);
> > +    event_sources[index].irq = irq;
> > +    event_sources[index].mask = EVENT_CLASS_MASK(index);
> > +    event_sources[index].enabled = true;
> > +}
> > +
> > +static const sPAPREventSource
> > +*spapr_event_sources_get_source(sPAPREventSource *event_sources,
> > +                                EventClassIndex index)
> 
> function return type on previous line or same line as the function
> name is fine by me.  But please don't split it across lines as you
> have here (with the '*' on the second line).
> 
> > +{
> > +    g_assert(index < EVENT_CLASS_MAX);
> > +    g_assert(event_sources);
> > +
> > +    return &event_sources[index];
> > +}
> > +
> > +void spapr_dt_events(sPAPRMachineState *spapr, void *fdt)
> > +{
> > +    uint32_t irq_ranges[EVENT_CLASS_MAX * 2];
> > +    int i, count = 0, event_sources;
> > +    sPAPREventSource *events = spapr->event_sources;
> > +
> > +    g_assert(events);
> >  
> >      _FDT(event_sources = fdt_add_subnode(fdt, 0, "event-sources"));
> >  
> > -    _FDT(fdt_setprop(fdt, event_sources, "interrupt-controller", NULL, 0));
> > -    _FDT(fdt_setprop_cell(fdt, event_sources, "#interrupt-cells", 2));
> > -    _FDT(fdt_setprop(fdt, event_sources, "interrupt-ranges",
> > -                     irq_ranges, sizeof(irq_ranges)));
> > +    for (i = 0, count = 0; i < EVENT_CLASS_MAX; i++) {
> > +        int node_offset;
> > +        uint32_t interrupts[2];
> > +        const sPAPREventSource *source =
> > +            spapr_event_sources_get_source(events, i);
> > +
> > +        if (!source->enabled) {
> > +            continue;
> > +        }
> > +
> > +        interrupts[0] = cpu_to_be32(source->irq);
> > +        interrupts[1] = 0;
> > +
> > +        _FDT(node_offset = fdt_add_subnode(fdt, event_sources, 
> > source->name));
> > +        _FDT(fdt_setprop(fdt, node_offset, "interrupts", interrupts,
> > +                         sizeof(interrupts)));
> > +
> > +        irq_ranges[count++] = interrupts[0];
> > +        irq_ranges[count++] = cpu_to_be32(1);
> > +    }
> > +
> > +    irq_ranges[count] = cpu_to_be32(count);
> > +    count++;
> > +
> > +    _FDT((fdt_setprop(fdt, event_sources, "interrupt-controller", NULL, 
> > 0)));
> > +    _FDT((fdt_setprop_cell(fdt, event_sources, "#interrupt-cells", 2)));
> > +    _FDT((fdt_setprop(fdt, event_sources, "interrupt-ranges",
> > +                      irq_ranges, count * sizeof(uint32_t))));
> > +}
> > +
> > +static const sPAPREventSource
> > +*rtas_event_log_to_source(sPAPRMachineState *spapr, int log_type)
> 
> And here.
> 
> > +{
> > +    const sPAPREventSource *source;
> > +
> > +    g_assert(spapr->event_sources);
> > +
> > +    switch (log_type) {
> > +    case RTAS_LOG_TYPE_HOTPLUG:
> > +        source = spapr_event_sources_get_source(spapr->event_sources,
> > +                                                EVENT_CLASS_HOT_PLUG);
> > +        if (spapr_ovec_test(spapr->ov5_cas, OV5_HP_EVT)) {
> > +            g_assert(source->enabled);
> > +            break;
> > +        }
> > +        /* fall back to epow for legacy hotplug interrupt source */
> > +    case RTAS_LOG_TYPE_EPOW:
> > +        source = spapr_event_sources_get_source(spapr->event_sources,
> > +                                                EVENT_CLASS_EPOW);
> > +        break;
> > +    default:
> > +        source = NULL;
> > +    }
> >  
> > -    _FDT(epow_events = fdt_add_subnode(fdt, event_sources, "epow-events"));
> > -    _FDT(fdt_setprop(fdt, epow_events, "interrupts",
> > -                     interrupts, sizeof(interrupts)));
> > +    return source;
> > +}
> > +
> > +static int rtas_event_log_to_irq(sPAPRMachineState *spapr, int log_type)
> > +{
> > +    const sPAPREventSource *source;
> > +
> > +    source = rtas_event_log_to_source(spapr, log_type);
> > +    g_assert(source);
> > +    g_assert(source->enabled);
> > +
> > +    return source->irq;
> >  }
> >  
> >  static void rtas_event_log_queue(int log_type, void *data, bool exception)
> > @@ -247,19 +361,15 @@ static sPAPREventLogEntry 
> > *rtas_event_log_dequeue(uint32_t event_mask,
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      sPAPREventLogEntry *entry = NULL;
> >  
> > -    /* we only queue EPOW events atm. */
> > -    if ((event_mask & EVENT_MASK_EPOW) == 0) {
> > -        return NULL;
> > -    }
> > -
> >      QTAILQ_FOREACH(entry, &spapr->pending_events, next) {
> > +        const sPAPREventSource *source =
> > +            rtas_event_log_to_source(spapr, entry->log_type);
> > +
> >          if (entry->exception != exception) {
> >              continue;
> >          }
> >  
> > -        /* EPOW and hotplug events are surfaced in the same manner */
> > -        if (entry->log_type == RTAS_LOG_TYPE_EPOW ||
> > -            entry->log_type == RTAS_LOG_TYPE_HOTPLUG) {
> > +        if (source->mask & event_mask) {
> >              break;
> >          }
> >      }
> > @@ -276,19 +386,15 @@ static bool rtas_event_log_contains(uint32_t 
> > event_mask, bool exception)
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      sPAPREventLogEntry *entry = NULL;
> >  
> > -    /* we only queue EPOW events atm. */
> > -    if ((event_mask & EVENT_MASK_EPOW) == 0) {
> > -        return false;
> > -    }
> > -
> >      QTAILQ_FOREACH(entry, &spapr->pending_events, next) {
> > +        const sPAPREventSource *source =
> > +            rtas_event_log_to_source(spapr, entry->log_type);
> > +
> >          if (entry->exception != exception) {
> >              continue;
> >          }
> >  
> > -        /* EPOW and hotplug events are surfaced in the same manner */
> > -        if (entry->log_type == RTAS_LOG_TYPE_EPOW ||
> > -            entry->log_type == RTAS_LOG_TYPE_HOTPLUG) {
> > +        if (source->mask & event_mask) {
> >              return true;
> >          }
> >      }
> > @@ -376,7 +482,9 @@ 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_irq));
> > +    qemu_irq_pulse(xics_get_qirq(spapr->xics,
> > +                                 rtas_event_log_to_irq(spapr,
> > +                                                       
> > RTAS_LOG_TYPE_EPOW)));
> >  }
> >  
> >  static void spapr_hotplug_set_signalled(uint32_t drc_index)
> > @@ -458,7 +566,9 @@ 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_irq));
> > +    qemu_irq_pulse(xics_get_qirq(spapr->xics,
> > +                                 rtas_event_log_to_irq(spapr,
> > +                                                       
> > RTAS_LOG_TYPE_HOTPLUG)));
> >  }
> >  
> >  void spapr_hotplug_req_add_by_index(sPAPRDRConnector *drc)
> > @@ -504,6 +614,7 @@ static void check_exception(PowerPCCPU *cpu, 
> > sPAPRMachineState *spapr,
> >      uint64_t xinfo;
> >      sPAPREventLogEntry *event;
> >      struct rtas_error_log *hdr;
> > +    int i;
> >  
> >      if ((nargs < 6) || (nargs > 7) || nret != 1) {
> >          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > @@ -540,8 +651,14 @@ static void check_exception(PowerPCCPU *cpu, 
> > sPAPRMachineState *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 = 0; i < EVENT_CLASS_MAX; i++) {
> > +        if (rtas_event_log_contains(EVENT_CLASS_MASK(i), true)) {
> > +            const sPAPREventSource *source =
> > +                spapr_event_sources_get_source(spapr->event_sources, i);
> > +
> > +            g_assert(source->enabled);
> > +            qemu_irq_pulse(xics_get_qirq(spapr->xics, source->irq));
> > +        }
> >      }
> >  
> >      return;
> > @@ -593,8 +710,19 @@ out_no_events:
> >  void spapr_events_init(sPAPRMachineState *spapr)
> >  {
> >      QTAILQ_INIT(&spapr->pending_events);
> > -    spapr->check_exception_irq = xics_spapr_alloc(spapr->xics, 0, false,
> > -                                            &error_fatal);
> > +
> > +    spapr->event_sources = spapr_event_sources_new();
> > +
> > +    spapr_event_sources_register(spapr->event_sources, EVENT_CLASS_EPOW,
> > +                                 xics_spapr_alloc(spapr->xics, 0, false,
> > +                                                  &error_fatal));
> > +
> > +    if (spapr->use_hotplug_event_source) {
> > +        spapr_event_sources_register(spapr->event_sources, 
> > EVENT_CLASS_HOT_PLUG,
> > +                                     xics_spapr_alloc(spapr->xics, 0, 
> > false,
> > +                                                      &error_fatal));
> > +    }
> 
> If I'm following this correctly, this means that if modern events are
> enabled on the qemu side, then the event source will be registered and
> thus advertised in the device tree, regardless of whether the guest
> supports the new-style events.
> 
> However, if the guest didn't advertise modern events support, the
> hotplug events sources would be unused, and hotplug events would be
> surfaced through epow.

Yes, exactly.

> 
> That seems like it should work, just verifying that having the
> new-but-unused event source show up in the guest device tree is
> intended behaviour.

Yes, that's the intent. It's up to the OS to set up an interrupt handler
for the event source in cases where it intends to handle modern hotplug
events, but whether or not we opt to include it in the device tree
before vs. after negotiation (via CAS) is arbitrary.

There was actually a bug in the RFC due to me forgetting this source was,
as a result of the above, always enabled for pseries-2.8+. I'll add a
small comment here to help clarify.

> 
> >      spapr->epow_notifier.notify = 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 851f536..21af971 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -13,6 +13,7 @@ struct sPAPRPHBState;
> >  struct sPAPRNVRAM;
> >  typedef struct sPAPRConfigureConnectorState sPAPRConfigureConnectorState;
> >  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
> > +typedef struct sPAPREventSource sPAPREventSource;
> >  
> >  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
> >  #define SPAPR_ENTRY_POINT       0x100
> > @@ -77,10 +78,10 @@ struct sPAPRMachineState {
> >      sPAPROptionVector *ov5_cas;     /* negotiated (via CAS) option vectors 
> > */
> >      bool cas_reboot;
> >  
> > -    uint32_t check_exception_irq;
> >      Notifier epow_notifier;
> >      QTAILQ_HEAD(, sPAPREventLogEntry) pending_events;
> >      bool use_hotplug_event_source;
> > +    sPAPREventSource *event_sources;
> >  
> >      /* Migration state */
> >      int htab_save_index;
> > @@ -585,7 +586,7 @@ struct sPAPREventLogEntry {
> >  };
> >  
> >  void spapr_events_init(sPAPRMachineState *sm);
> > -void spapr_dt_events(void *fdt, uint32_t check_exception_irq);
> > +void spapr_dt_events(sPAPRMachineState *sm, 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_ _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson