[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH v5 08/16] spapr_events: re-use EPOW event infrastr
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH v5 08/16] spapr_events: re-use EPOW event infrastructure for hotplug events |
Date: |
Wed, 25 Feb 2015 11:54:18 +1100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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 <address@hidden>
[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
pgpFqhKCgGveC.pgp
Description: PGP signature