Re: [PATCH 7/7] xen/arm: phys_timer fixes

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Mon, 18 Feb 2013, Ian Campbell wrote:
> On Mon, 2013-02-18 at 15:31 +0000, Stefano Stabellini wrote:
> > On Mon, 18 Feb 2013, Ian Campbell wrote:
> > > On Mon, 2013-02-18 at 14:44 +0000, Stefano Stabellini wrote:
> > > > On Fri, 15 Feb 2013, Ian Campbell wrote:
> > > > > > @@ -33,8 +34,8 @@ static void phys_timer_expired(void *data)
> > > > > >  {
> > > > > >      struct vtimer *t = data;
> > > > > >      t->ctl |= CNTx_CTL_PENDING;
> > > > > > -    t->ctl &= ~CNTx_CTL_MASK;
> > > > > > -    vgic_vcpu_inject_irq(t->v, 30, 1);
> > > > > > +    if ( !(t->ctl & CNTx_CTL_MASK) )
> > > > > > +        vgic_vcpu_inject_irq(t->v, 30, 1);
> > > > > 
> > > > > Thinking about the previous discussion about exposing the change of the
> > > > > mask bit to the guest, it just occurred to me that t->ctl.MASK need not
> > > > > actually represent the state of the underlying physical mask bit, since
> > > > > we do emulate accesses after all.
> > > > 
> > > > Sure. However this change still makes sense: if the guest masked the
> > > > timer (no matter how we internally represent it), we should not inject
> > > > an interrupt.
> > > 
> > > Yes. Actually, I think the setting of CNTx_CTL_PENDING here is wrong
> > > also, it should be set only when the guest masks the interrupt.
> > 
> > I don't think so. It is a fact that the pending bit is set when an
> > interrupt is raised,
> 
> Perhaps it is the case that:
> 	MASKED == 0 => PENDING represents the "live" state of the 
> 			interrupt
> 	MASKED == 1 => PENDING is latched to what ever it was when the 
> 			mask bit was set
> ?
> 
> That is consistent with the docs as I read them, but not very explicit
> (since they only really specify the MASKED==1 case). This would be worth
> clarifying with ARM I think.
> 
> >  in fact both Xen and Linux check for the pending
> > bit in the physical interrupt handler.
> 
> Right, if the behaviour is as I described then they are not buggy to do
> so without masking the interrupt.
> 
> However even with the behaviour above this patch is not implementing the
> pending behaviour correctly, since it is cleared unconditionally in
> phys_timer_gic_callback(). Also it should be r/o from the guest point of
> view.

You are right about being RO. See below regarding the "latch" case.


> > So the question is: should the pending bit also be set when an interrupt
> > should have been raised but it wasn't because the timer was masked?
> > As a matter of fact the answer is yes from my experiments on the
> > Versatile Express Cortex A15 machine I have.
> 
> The answer may well be yes *on the Cortex A15*. That is why I would like
> to see this clarified with ARM. Especially given that this is not
> consistent with what the docs say, since they preclude PENDING changing
> while MASK is set:
>         A register write that sets IMASK to 1 latches this bit to
>         reflect the state of the interrupt immediately before that
>         write.
> 
> The key word is "latches" here -- although it's not clear until when I
> think it is likely to be until the mask bit is cleared (another thing to
> ask about I think).
> 
> It is certainly possible for the timer to fire but be masked and for it
> to trigger the interrupt immediately when the mask bit is subsequently
> cleared, but that does not imply that the pending bit changes at the
> point where it would have fired while it was masked.

I think they mean "latch" in electronic sense: they use an SR latch to
keep the pending bit high even if the guest EOIs the interrupt, as long
as the mask bit is 1.
In other words the pending bit cannot be reset if the mask bit is 1.

So I should prevent the guest from resetting this bit directly in all
cases and also I should not reset the bit on EOI if the mask bit is 1.

I agree that is confusing though.