Re: [Xen-devel] [PATCH RFC] xen: arm: context switch vtimer PPI state.

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 3 Mar 2015, Ian Campbell wrote:
> On Tue, 2015-03-03 at 12:00 +0000, Stefano Stabellini wrote:
> > > > > > +        /* An edge triggered interrupt should now be pending. */
> > > > > > +        t->ppi_state.pending = true;
> > > > > > +        vcpu_unblock(t->v);
> > > > 
> > > > I was going to say that this is trouble because
> > > > local_events_need_delivery wouldn't recognize that we actually have an
> > > > event pending. But it doesn't matter because local_events_need_delivery
> > > > only works with the current vcpu and if this code trigger then the vcpu
> > > > that needs to receive the event cannot be current. So I think that is OK
> > > > but for clarity it might be better to add a check or a comment in
> > > > local_events_need_delivery_nomask anyway.
> > > 
> > > i.e. a BUG_ON(t->v == current) + a comment to the affect that the timer
> > > must never expire for the current vcpu?
> > 
> > I think it would be best just to add a comment in
> > local_events_need_delivery to remember these events are not listed
> > there.
> 
> So something like "events generated by XXX are not accounted for here".
> But what should XXX be?
> 
> "Events which are queued to be delivered when the vcpu is next
> scheduled" perhaps?
> 
> BTW: note that setting t->ppi_state.pending here means we will take the
> IRQ on context restore, at which point local_events_need_delivery (via
> gic_events_need_delivery)will do the right thing.

Yeah, you are right. Simply expand a bit more the comment you are
already adding to virt_timer_expired.


> > > > > > > +    /* Xen s/w state */
> > > > > > > +    unsigned long inprogress:1;
> > > > > > > +};
> > > > > 
> > > > > Using a uint32_t bitmask would be more in line the rest of the code
> > > > > style, but it is just a matter of taste.
> > > > 
> > > > You mean "uint32_t inprogress:1" for each? Or
> > > > #define ENAVBLED 1
> > > > #define PENDING 2
> > > > etc
> > > > and test_set_bit stuff?
> > >  
> > > Something like status in xen/include/asm-arm/vgic.h:struct pending_irq
> > 
> > OK.
> 
> Actually, what is the benefit? I think pending_irq.status is done this
> way because it is used lock free from multiple contexts and this allows
> for test_and_set_bit etc to be used.
> 
> The hwppi state isn't accessed like that, so I think using the normal
> appropriate types like we do in stuct arch_{domain,vcpu} is the
> appropriate style. struct hwppi_state is even defined in domain.h.

OK, feel free to keep it as is.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel