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 <nf...@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
pgpIJr6FPDQ5U.pgp
Description: PGP signature
