Re: [Xen-devel] [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 21/06/14 16:26, Stefano Stabellini wrote:
> On Sat, 21 Jun 2014, Julien Grall wrote:
> > On 21/06/14 15:43, Stefano Stabellini wrote:
> > > On Fri, 20 Jun 2014, Julien Grall wrote:
> > > > On 20 Jun 2014 18:41, "Stefano Stabellini"
> > > > <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > > > > On Fri, 20 Jun 2014, Julien Grall wrote:
> > > > > > On 20/06/14 17:48, Stefano Stabellini wrote:
> > > > > > > > Your patch "physical irq follow virtual irq" won't even solve this
> > > > > > > > problem
> > > > > > > > if
> > > > > > > > the maintenance interrupt is request to EOI the IRQ.
> > > > > > > I don't understand this statement.
> > > > > > > The maintenance interrupt is requested to make sure that the vcpu is
> > > > > > > interrupted as soon as it EOIs the virtual irq, so that
> > > > > > > gic_update_one_lr can run.
> > > > > > I agree with that but ... the following step can happen
> > > > > >
> > > > > > 1) Xen receives the interrupt
> > > > > > 2) Xen writes EOIR
> > > > > > 3) Xen inject the IRQ to the guest
> > > > > > 4) The guest will receive the IRQ (i.e read IAR)
> > > > > > 5) Xen migrates the VCPU to another physical CPU
> > > > > > 6) The guest writes into GIC_DIR and a maintenance IRQ is fired.
> > > > > >
> > > > > > In a such case, the IRQ will be EOIed to another physical CPU. If we
> > > > > > assume
> > > > > > that we should always write to GICC_DIR of the physical CPU that
> > > > > > receive the
> > > > > > interrupt, then even your patch "physcial irq follow virtual irq"
> > > > > > won't solve
> > > > > > the problem.
> > > > > That's true.
> > > > >
> > > > >
> > > > >
> > > > > > > > The VGIC lock is per-vcpu.
> > > > > > > >
> > > > > > > > If this is happening while we migrate, nothing protect p->lr and
> > > > > > > > the
> > > > > > > > different
> > > > > > > > bit anymore.
> > > > > > > The patch series
> > > > > > > alpine.DEB.2.02.1406111607450.13771@xxxxxxxxxxxxxxxxxxxxxxx takes
> > > > > > > care of vcpu migration and locking.
> > > > > > >
> > > > > > >
> > > > > > > > Indeed, the vgic_inject_irq will use the new VCPU. This can happen
> > > > > > > > as soon
> > > > > > > > as
> > > > > > > > we EOI the IRQ.
> > > > > > > Yes, but at that point we have also already migrated the
> > > > > > > corresponding
> > > > > > > physical irq to the new pcpu.
> > > > > > Even tho we the migration has been done, the 2 clear bit are not
> > > > > > atomic. Let's
> > > > > > imagine that the IRQ has fired between the two clear bit. In this case
> > > > > > we may
> > > > > > clear the ACTIVE bit by mistake.
> > > > > >
> > > > > > I don't see anything in this code which prevents a such configuration.
> > > > > Which 2 clear bit?
> > > > GUEST_VISIBLE and GUEST_ACTIVE see the code after eoi_irq.
> > > The code is protected by the vgic lock.
> > If the IRQ has been migrating while it's injected, the maintenance interrupt
> > will use the previous VCPU and vgic_inject_irq will use the new one. 2
> > different lock protecting the same thing we but not mutually exclusive.
> >
> > As soon as we write in GICC_DIR, this case can happen.
> I don't think this case can happen: the maintenance interrupt is
> synchronous, so it can happen either before the migration, or after the
> vcpu has been fully migrated and its execution has been resumed.
> GICC_DIR is written in response to a maintenance interrupt being fired,
> so GICC_DIR will be written either before the vcpu has been migrated or
> after.
I agree that the maintenance interrupt synchronous... but for this part 
I was talking talking about VIRQ migration (i.e the guest is writing in 
ITARGETSR).
1) The guest is receiving the IRQ
2) The guest is writing in ITARGETSR to move the IRQ to another VCPU
3) The guest is EOIing the IRQ and Xen receive a maintenance interrupt.

You set the affinity of the physical IRQ directly in the VGIC ITARGETSR 
emulation. So as soon as we write in GICC_DIR, this IRQ may fired again 
on another physical CPU (assuming the new VCPU is currently running). In 
this case, vgic_inject_irq will use the new VGIC lock and nothing will 
prevent concurrent access to clear/set the bit GUEST_VISIBLE and 
GUEST_ACTIVE.
--
Julien Grall

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