Re: [Xen-devel] [PATCH 2/2] xen/arm: take the rank lock before accessing ipriority

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 2 Jul 2014, Ian Campbell wrote:
> On Tue, 2014-06-24 at 19:11 +0100, Stefano Stabellini wrote:
> > Currently we read ipriority from vgic_vcpu_inject_irq without taking the
> > rank lock. Fix that by taking the rank lock and reading ipriority at the
> > beginning of the function.
> 
> Since it is a byte read we'll always get either the before or after
> value of any racing write, won't we?

I don't think so: a byte_write is a 2 steps operation:

    *reg &= ~(0xff << (8*byte));
    *reg |= var;

so a byte_read could get mixed up with it. Am I missing something?



> The real danger would be the compiler deciding to read the value twice
> for some reason, which it is entitled to do (e.g. under register
> pressure).
> 
> The unlock has enough barriers in to prevent that I think (hope!). But I
> think you could probably get away with an ACCESS_ONCE() type thing only.
> 
> > As vgic_vcpu_inject_irq is called from the irq.c upon receiving an
> > interrupt, we need to change the implementation of vgic_lock/unlock_rank
> > to spin_lock_irqsave to make it safe in irq context.
> > 
> > Also add a warning to point out that if the irq is already inflight with
> > a different priority, we are not changing the irq priority for the
> > second injection.
> 
> I think this matches the defined hardware behaviour, doesn't it? No need
> for a warning in that case IMHO.

You are right, I'll fix it.

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