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

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

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

On Tue, Feb 24, 2015 at 02:04:29PM -0600, Michael Roth wrote:
> Quoting David Gibson (2015-02-24 00:49:42)
> > On Mon, Feb 16, 2015 at 08:27:44AM -0600, Michael Roth wrote:
> > > From: Nathan Fontenot <nfont@linux.vnet.ibm.com>
[snip]
> > > +    hdr = &pending_epow->hdr;
> > > +    v6hdr = &pending_epow->v6hdr;
> > > +    maina = &pending_epow->maina;
> > > +    mainb = &pending_epow->mainb;
> > > +    epow = &pending_epow->epow;
> > > +
> > > +    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_TYPE_EPOW);
> > > +    hdr->extended_length = cpu_to_be32(sizeof(*pending_epow)
> > > +                                       - sizeof(pending_epow->hdr));
> > > +
> > > +    spapr_init_v6hdr(v6hdr);
> > > +    spapr_init_maina(maina, 3 /* Main-A, Main-B and EPOW */);
> > >  
> > >      mainb->hdr.section_id = cpu_to_be16(RTAS_LOG_V6_SECTION_ID_MAINB);
> > >      mainb->hdr.section_length = cpu_to_be16(sizeof(*mainb));
> > > @@ -274,7 +356,78 @@ static void spapr_powerdown_req(Notifier *n, void *opaque)
> > >      epow->event_modifier = RTAS_LOG_V6_EPOW_MODIFIER_NORMAL;
> > >      epow->extended_modifier = RTAS_LOG_V6_EPOW_XMODIFIER_PARTITION_SPECIFIC;
> > >  
> > > -    qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->epow_irq));
> > > +    rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, pending_epow);
> > > +
> > > +    qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq));
> > 
> > Shouldn't the irq pulse go into rtas_event_log_queue()?
> 
> It seems maybe it should...
> 
> "If a platform chooses to report multiple event or error sources through a
>  single interrupt, it must ensure that the interrupt remains asserted
>  or is re-asserted until check-exception has been used to process all
>  out-standing errors or events for that interrupt."
> 
> But another alternative would be to have rtas-check-exception re-pulse if there
> are still events present for that particular IRQ. I think I'd prefer this
> approach, since it makes it easier to share the queueing functionality with
> rtas-event-scan (which relies on polling instead of interrupts)

Actually I wasn't thinking that deeply, just looking at the fact that
both callerss to rtas_event_log_queue() pulse the irq immediately
afterwards.

Hrm.  Now I'm a bit confused - the use of qemu_irq_pulse() suggests an
edge/message interrupt, but the reference to "remains asserted"
suggests level interrupt semantics.  I no longer remember enough of
PAPR to know why I used qemu_irq_pulse() in the first place.

[snip]
> > > -        cpu_physical_memory_write(buf, pending_epow, len);
> > > -        g_free(pending_epow);
> > > -        pending_epow = NULL;
> > > -        rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> > > -    } else {
> > > -        rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND);
> > > +    switch (event->log_type) {
> > > +        case RTAS_LOG_TYPE_EPOW:
> > > +            event_len = sizeof(struct epow_log_full);
> > > +            break;
> > > +        case RTAS_LOG_TYPE_HOTPLUG:
> > > +            event_len = sizeof(struct hp_log_full);
> > > +            break;
> > > +        default:
> > > +            goto out_no_events;
> > 
> > Doesn't one of the headers include size information you could use
> > here, avoiding the switch?
> 
> Looks like hdr->extended_length + sizeof(hdr) should do the
> trick. Nice!

Excellent. A switch on the same selector at multiple levels of the
same callstack always seems to be a warning sign of layering
violations.

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