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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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.


> > > > > diff --git a/xen/include/asm-arm/domain.h 
> > > > > b/xen/include/asm-arm/domain.h
> > > > > index 90ab9c3..dd50e2c 100644
> > > > > --- a/xen/include/asm-arm/domain.h
> > > > > +++ b/xen/include/asm-arm/domain.h
> > > > > @@ -34,12 +34,23 @@ enum domain_type {
> > > > >  extern int dom0_11_mapping;
> > > > >  #define is_domain_direct_mapped(d) ((d) == hardware_domain && 
> > > > > dom0_11_mapping)
> > > > >  
> > > > > +struct hwppi_state {
> > > > > +    /* h/w state */
> > > > > +    unsigned long enabled:1;
> > > > > +    unsigned long pending:1;
> > > > > +    unsigned long active:1;
> > > > > +
> > > > > +    /* 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.



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