Re: [Qemu-devel] [PATCH v4 08/17] spapr_events: re-use EPOW event infrastructure for hotplug events

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6042 bytes --]

On Mon, Jan 26, 2015 at 10:56:51AM -0600, Michael Roth wrote:
> Quoting David Gibson (2015-01-18 22:31:23)
> > On Tue, Dec 23, 2014 at 06:30:22AM -0600, Michael Roth wrote:
> > > From: Nathan Fontenot <nfont@linux.vnet.ibm.com>
[snip]
> > > +static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t hp_action)
> > > +{
> > > +    struct hp_log_full *new_hp;
> > > +    struct rtas_error_log *hdr;
> > > +    struct rtas_event_log_v6 *v6hdr;
> > > +    struct rtas_event_log_v6_maina *maina;
> > > +    struct rtas_event_log_v6_mainb *mainb;
> > > +    struct rtas_event_log_v6_hp *hp;
> > > +    sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > > +    sPAPRDRConnectorType drc_type = drck->get_type(drc);
> > > +
> > > +    new_hp = g_malloc0(sizeof(struct hp_log_full));
> > > +    hdr = &new_hp->hdr;
> > > +    v6hdr = &new_hp->v6hdr;
> > > +    maina = &new_hp->maina;
> > > +    mainb = &new_hp->mainb;
> > > +    hp = &new_hp->hp;
> > > +
> > > +    hdr->summary = cpu_to_be32(RTAS_LOG_VERSION_6
> > > +                               | RTAS_LOG_SEVERITY_EVENT
> > > +                               | RTAS_LOG_DISPOSITION_NOT_RECOVERED
> > > +                               | RTAS_LOG_OPTIONAL_PART_PRESENT
> > > +                               | RTAS_LOG_INITIATOR_HOTPLUG
> > > +                               | RTAS_LOG_TYPE_HOTPLUG);
> > > +    hdr->extended_length = cpu_to_be32(sizeof(*new_hp)
> > > +                                       - sizeof(new_hp->hdr));
> > > +
> > > +    spapr_init_v6hdr(v6hdr);
> > > +    spapr_init_maina(maina, 3 /* Main-A, Main-B, HP */);
> > > +
> > > +    mainb->hdr.section_id = cpu_to_be16(RTAS_LOG_V6_SECTION_ID_MAINB);
> > > +    mainb->hdr.section_length = cpu_to_be16(sizeof(*mainb));
> > > +    mainb->subsystem_id = 0x80; /* External environment */
> > > +    mainb->event_severity = 0x00; /* Informational / non-error */
> > > +    mainb->event_subtype = 0x00; /* Normal shutdown */
> > > +
> > > +    hp->hdr.section_id = cpu_to_be16(RTAS_LOG_V6_SECTION_ID_HOTPLUG);
> > > +    hp->hdr.section_length = cpu_to_be16(sizeof(*hp));
> > > +    hp->hdr.section_version = 1; /* includes extended modifier */
> > > +    hp->hotplug_action = hp_action;
> > > +
> > > +
> > > +    switch (drc_type) {
> > > +    case SPAPR_DR_CONNECTOR_TYPE_PCI:
> > > +        hp->drc.index = cpu_to_be32(drck->get_index(drc));
> > > +        hp->hotplug_identifier = RTAS_LOG_V6_HP_ID_DRC_INDEX;
> > > +        hp->hotplug_type = RTAS_LOG_V6_HP_TYPE_PCI;
> > > +        break;
> > > +    default:
> > > +        /* skip notification for unknown connector types */
> > > +        g_free(new_hp);
> > > +        return;
> > > +    }
> > > +
> > > +    if (pending_hp) {
> > > +        /* Just toss any pending hotplug events for now, this will
> > > +         * need to be fixed later on.
> > > +         */
> > 
> > So, we can get away with a 1-element queue for EPOW, because they're
> > just triggering a shutdown - so once the first one's processed, any
> > others aren't going to matter.  For hotplug you really do need a
> > proper queue.
> 
> Yah, this was discussed in the past, but until now I didn't notice how
> easy it would be to trigger this when hotplugging multiple devices from
> a script or management harness of some sort. Should be simple enough to
> fix for v5 though.

Ok, good.

> > > +        g_free(pending_hp);
> > > +    }
> > > +    pending_hp = new_hp;
> > > +
> > > +    qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq));
> > > +}
> > > +
> > > +void spapr_hotplug_req_add_event(sPAPRDRConnector *drc)
> > > +{
> > > +    spapr_hotplug_req_event(drc, RTAS_LOG_V6_HP_ACTION_ADD);
> > > +}
> > > +
> > > +void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc)
> > > +{
> > > +    spapr_hotplug_req_event(drc, RTAS_LOG_V6_HP_ACTION_REMOVE);
> > >  }
> > >  
> > >  static void check_exception(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> > > @@ -298,15 +420,26 @@ static void check_exception(PowerPCCPU *cpu, sPAPREnvironment *spapr,
> > >          xinfo |= (uint64_t)rtas_ld(args, 6) << 32;
> > >      }
> > >  
> > > -    if ((mask & EVENT_MASK_EPOW) && pending_epow) {
> > > -        if (sizeof(*pending_epow) < len) {
> > > -            len = sizeof(*pending_epow);
> > > -        }
> > > +    if (mask & EVENT_MASK_EPOW) {
> > > +        if (pending_epow) {
> > > +            if (sizeof(*pending_epow) < len) {
> > > +                len = sizeof(*pending_epow);
> > > +            }
> > >  
> > > -        cpu_physical_memory_write(buf, pending_epow, len);
> > > -        g_free(pending_epow);
> > > -        pending_epow = NULL;
> > > -        rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> > > +            cpu_physical_memory_write(buf, pending_epow, len);
> > > +            g_free(pending_epow);
> > > +            pending_epow = NULL;
> > > +            rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> > > +        } else if (pending_hp) {
> > 
> > So.. the hotplug messages are a different type from EPOW, but are
> > still selected by EVENT_MASK_EPOW?  Seems a bit odd.
> 
> It's a little odd, but it's mainly just due to the way we surface the
> hotplug event. Rather than requiring patched guest kernels, we opted
> to re-use and generalize the EPOW IRQ to also surface hotplug-related
> RTAS events, which is why we still expect the EVENT_MASK_EPOW when
> returning an HP event via check-exception.
> 
> EPOW events have well-defined behavior in how they're exposed to
> userspace via rtas_errd, which allows us to add hotplug support for
> older guests via patched userspace tools.

Ok.  Kind of a hack, but for good reasons.  I kind of think it needs a
comment, but I'm not sure where it would belong.

-- 
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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]