Re: [Xen-devel] [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 24 Jun 2014, Julien Grall wrote:
> On 06/24/2014 07:04 PM, Stefano Stabellini wrote:
> > On Tue, 24 Jun 2014, Julien Grall wrote:
> >> On 24/06/14 12:38, Stefano Stabellini wrote:
> >>>> Sorry for not having catch this earlier. I don't really like the idea to
> >>>> extend the rank lock to vgic_{enable/disable}_irqs. The 2 functions
> >>>> can be long to execute as it may touch the GIC distributor.
> >>>>
> >>>> In another way, the rank lock is only taken in the distributor emulation.
> >>>>
> >>>> Assuming we consider that distributor access may be slow:
> >>>
> >>> We could end up enabling or disabling the wrong set of interrupts
> >>> without this change. I think it is necessary.
> >>
> >> AFAIU, this lock only protects the rank when we retrieve the target VCPU, 
> >> the
> >> other part of the function will still work without it.
> >>
> >> What I meant is to call vgic_get_target_vcpu, so the lock will protect only
> >> what is necessary and not more.
> > 
> > I don't think so (unless I misunderstood your suggestion): setting
> > ienable and enabling the interrupts need to be atomic.  Otherwise this
> > can happen:
> > 
> >     VCPU0                                       VCPU1
> > - rank_lock
> > - write icenabler
> > - get target vcpus
> > - rank_unlock
> >                                          - rank_lock
> >                                          - write icenable
> >                                          - get target vcpus (some are the 
> > same)
> >                                          - rank_unlock
> > 
> >                                          - vgic_enable_irqs
> > 
> > - vgic_enable_irqs
> > 
> > 
> > we now have an inconsistent state: we enabled the irqs written from
> > vcpu0 but icenable reflects what was written from vcpu1.
> 
> In your example it's not possible because we save the value of ienable
> in a temporary variable. So calling to vgic_enable_irqs on 2 different
> VCPU with the same range of IRQ won't be a problem.
> 
> But... there is a possible race condition between enable and disable. We
> need to serialize the access otherwise we may enable/disable by mistake
> an IRQ and it's not synced anymore with the register state.
> 
> This is issue is also on Xen 4.4. Can you send a single patch which move
> the unlock for this branch?

Sure



> Thanks,
> 
> -- 
> Julien Grall
> 

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