[PATCH] xen/arm: introduce platform_need_explicit_eoi

[PATCH] xen/arm: introduce platform_need_explicit_eoi

[Stefano Stabellini]

GICH_LR_HW doesn't work as expected on X-Gene: request maintenance
interrupts and perform EOIs in the hypervisor as a workaround.
Trigger this behaviour with a per platform option.

No need to find the pcpu that needs to write to GICC_DIR, because after
"physical irq follow virtual irq" we always inject the virtual irq on
the vcpu that is running on the pcpu that received the interrupt.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: psawargaonkar@apm.com
CC: apatel@apm.com
---
 xen/arch/arm/gic.c                   |   13 ++++++++++++-
 xen/arch/arm/platforms/xgene-storm.c |    2 +-
 xen/include/asm-arm/platform.h       |    5 +++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 58233cc..8695078 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -577,7 +577,9 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
 
     lr_val = state | (GIC_PRI_TO_GUEST(p->priority) << GICH_LR_PRIORITY_SHIFT) |
         ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
-    if ( p->desc != NULL )
+    if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) )
+        lr_val |= GICH_LR_MAINTENANCE_IRQ;
+    else if ( p->desc != NULL )
         lr_val |= GICH_LR_HW | (p->desc->irq << GICH_LR_PHYSICAL_SHIFT);
 
     GICH[GICH_LR + lr] = lr_val;
@@ -656,6 +658,11 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
     gic_add_to_lr_pending(v, irq_to_pending(v, virtual_irq));
 }
 
+static void gic_irq_eoi(int irq)
+{
+    GICC[GICC_DIR] = irq;
+}
+
 static void gic_update_one_lr(struct vcpu *v, int i)
 {
     struct pending_irq *p;
@@ -692,7 +699,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         clear_bit(i, &this_cpu(lr_mask));
 
         if ( p->desc != NULL )
+        {
             p->desc->status &= ~IRQ_INPROGRESS;
+            if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) )
+                gic_irq_eoi(p->irq);
+        }
         clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
         clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
         p->lr = GIC_INVALID_LR;
diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index c9dd63c..5396c46 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -37,7 +37,7 @@ static bool reset_vals_valid = false;
 
 static uint32_t xgene_storm_quirks(void)
 {
-    return PLATFORM_QUIRK_GIC_64K_STRIDE;
+    return PLATFORM_QUIRK_GIC_64K_STRIDE|PLATFORM_QUIRK_NEED_EOI;
 }
 
 static int map_one_mmio(struct domain *d, const char *what,
diff --git a/xen/include/asm-arm/platform.h b/xen/include/asm-arm/platform.h
index bcd2097..572f5b1 100644
--- a/xen/include/asm-arm/platform.h
+++ b/xen/include/asm-arm/platform.h
@@ -55,6 +55,11 @@ struct platform_desc {
  */
 #define PLATFORM_QUIRK_GIC_64K_STRIDE (1 << 0)
 
+/*
+ * Quirk for platforms where GICH_LR_HW does not work as expected.
+ */
+#define PLATFORM_QUIRK_NEED_EOI       (1 << 1)
+
 void __init platform_init(void);
 int __init platform_init_time(void);
 int __init platform_specific_mapping(struct domain *d);
-- 
1.7.10.4

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Julien Grall]

Hi Stefano,

On 20/06/14 14:35, Stefano Stabellini wrote:
> GICH_LR_HW doesn't work as expected on X-Gene: request maintenance
> interrupts and perform EOIs in the hypervisor as a workaround.
> Trigger this behaviour with a per platform option.
>
> No need to find the pcpu that needs to write to GICC_DIR, because after
> "physical irq follow virtual irq" we always inject the virtual irq on
> the vcpu that is running on the pcpu that received the interrupt.

Even without the patch "physical irq follow virtual irq" we can EOI an 
SPIs on any physical CPUs.

Otherwise, even with this patch, you will have to find pcpu when the 
VCPU has been migrated by EOI the IRQ :).

I would rework this last paragraph to explain this is how the GIC behave.

>   static void gic_update_one_lr(struct vcpu *v, int i)
>   {
>       struct pending_irq *p;
> @@ -692,7 +699,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>           clear_bit(i, &this_cpu(lr_mask));
>
>           if ( p->desc != NULL )
> +        {
>               p->desc->status &= ~IRQ_INPROGRESS;
> +            if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) )
> +                gic_irq_eoi(p->irq);
> +        }
>           clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
>           clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
>           p->lr = GIC_INVALID_LR;

With this change, shouldn't we clear the bit and the LR before EOI the IRQ?

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Stefano Stabellini]

On Fri, 20 Jun 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 20/06/14 14:35, Stefano Stabellini wrote:
> > GICH_LR_HW doesn't work as expected on X-Gene: request maintenance
> > interrupts and perform EOIs in the hypervisor as a workaround.
> > Trigger this behaviour with a per platform option.
> > 
> > No need to find the pcpu that needs to write to GICC_DIR, because after
> > "physical irq follow virtual irq" we always inject the virtual irq on
> > the vcpu that is running on the pcpu that received the interrupt.
> 
> Even without the patch "physical irq follow virtual irq" we can EOI an SPIs on
> any physical CPUs.

Can you point me to the paragraph of the GIC spec that says that?


> Otherwise, even with this patch, you will have to find pcpu when the VCPU has
> been migrated by EOI the IRQ :).
> I would rework this last paragraph to explain this is how the GIC behave.
> 
> >   static void gic_update_one_lr(struct vcpu *v, int i)
> >   {
> >       struct pending_irq *p;
> > @@ -692,7 +699,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> >           clear_bit(i, &this_cpu(lr_mask));
> > 
> >           if ( p->desc != NULL )
> > +        {
> >               p->desc->status &= ~IRQ_INPROGRESS;
> > +            if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) )
> > +                gic_irq_eoi(p->irq);
> > +        }
> >           clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> >           clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> >           p->lr = GIC_INVALID_LR;
> 
> With this change, shouldn't we clear the bit and the LR before EOI the IRQ?

Conceptually you might be right but it is all happening with the  vgic
lock taken, so it shouldn't make any difference.

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Julien Grall]

On 20/06/14 15:27, Stefano Stabellini wrote:
> On Fri, 20 Jun 2014, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 20/06/14 14:35, Stefano Stabellini wrote:
>>> GICH_LR_HW doesn't work as expected on X-Gene: request maintenance
>>> interrupts and perform EOIs in the hypervisor as a workaround.
>>> Trigger this behaviour with a per platform option.
>>>
>>> No need to find the pcpu that needs to write to GICC_DIR, because after
>>> "physical irq follow virtual irq" we always inject the virtual irq on
>>> the vcpu that is running on the pcpu that received the interrupt.
>>
>> Even without the patch "physical irq follow virtual irq" we can EOI an SPIs on
>> any physical CPUs.
>
> Can you point me to the paragraph of the GIC spec that says that?

While it's clearly specified in the spec that the GICC_EOIR must be done 
in the same physical CPU. Nothing has been specified for GICC_DIR.

I'm assuming this is the case, because even with the HW bit the physical 
interrupt may be EOIed on a different CPU. Think about the VCPU has been 
migrated when the guest is still handling the interrupt...

Your patch "physical irq follow virtual irq" won't even solve this 
problem if the maintenance interrupt is request to EOI the IRQ.

>
>> Otherwise, even with this patch, you will have to find pcpu when the VCPU has
>> been migrated by EOI the IRQ :).
>> I would rework this last paragraph to explain this is how the GIC behave.
>>
>>>    static void gic_update_one_lr(struct vcpu *v, int i)
>>>    {
>>>        struct pending_irq *p;
>>> @@ -692,7 +699,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>>>            clear_bit(i, &this_cpu(lr_mask));
>>>
>>>            if ( p->desc != NULL )
>>> +        {
>>>                p->desc->status &= ~IRQ_INPROGRESS;
>>> +            if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) )
>>> +                gic_irq_eoi(p->irq);
>>> +        }
>>>            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
>>>            clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
>>>            p->lr = GIC_INVALID_LR;
>>
>> With this change, shouldn't we clear the bit and the LR before EOI the IRQ?
>
> Conceptually you might be right but it is all happening with the  vgic
> lock taken, so it shouldn't make any difference.

The VGIC lock is per-vcpu.

If this is happening while we migrate, nothing protect p->lr and the 
different bit anymore.
Indeed, the vgic_inject_irq will use the new VCPU. This can happen as 
soon as we EOI the IRQ.

I'm not sure why you didn't decide to have a dist lock for SPIs. It 
would have make interrupt handling easier.

-- 
Julien Grall

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Stefano Stabellini]

On Fri, 20 Jun 2014, Julien Grall wrote:
> On 20/06/14 15:27, Stefano Stabellini wrote:
> > On Fri, 20 Jun 2014, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 20/06/14 14:35, Stefano Stabellini wrote:
> > > > GICH_LR_HW doesn't work as expected on X-Gene: request maintenance
> > > > interrupts and perform EOIs in the hypervisor as a workaround.
> > > > Trigger this behaviour with a per platform option.
> > > > 
> > > > No need to find the pcpu that needs to write to GICC_DIR, because after
> > > > "physical irq follow virtual irq" we always inject the virtual irq on
> > > > the vcpu that is running on the pcpu that received the interrupt.
> > > 
> > > Even without the patch "physical irq follow virtual irq" we can EOI an
> > > SPIs on
> > > any physical CPUs.
> > 
> > Can you point me to the paragraph of the GIC spec that says that?
> 
> While it's clearly specified in the spec that the GICC_EOIR must be done in
> the same physical CPU. Nothing has been specified for GICC_DIR.

That's a pity.
I suspect that you are right but I don't like to make an assumption like
that without the spec clearly supporting it.


> I'm assuming this is the case, because even with the HW bit the physical
> interrupt may be EOIed on a different CPU. Think about the VCPU has been
> migrated when the guest is still handling the interrupt...

I also think it should be OK, but can we be sure that all the SOC
vendors are going to use a GICv2 that supports this behaviour?


> 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.



> > > Otherwise, even with this patch, you will have to find pcpu when the VCPU
> > > has
> > > been migrated by EOI the IRQ :).
> > > I would rework this last paragraph to explain this is how the GIC behave.
> > > 
> > > >    static void gic_update_one_lr(struct vcpu *v, int i)
> > > >    {
> > > >        struct pending_irq *p;
> > > > @@ -692,7 +699,11 @@ static void gic_update_one_lr(struct vcpu *v, int
> > > > i)
> > > >            clear_bit(i, &this_cpu(lr_mask));
> > > > 
> > > >            if ( p->desc != NULL )
> > > > +        {
> > > >                p->desc->status &= ~IRQ_INPROGRESS;
> > > > +            if ( platform_has_quirk(PLATFORM_QUIRK_NEED_EOI) )
> > > > +                gic_irq_eoi(p->irq);
> > > > +        }
> > > >            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > > >            clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> > > >            p->lr = GIC_INVALID_LR;
> > > 
> > > With this change, shouldn't we clear the bit and the LR before EOI the
> > > IRQ?
> > 
> > Conceptually you might be right but it is all happening with the  vgic
> > lock taken, so it shouldn't make any difference.
> 
> 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@kaball.uk.xensource.com 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.

However looking back again at my series, if the guest kernel keeps
writing to GICD_ITARGETSR while the interrupt is firing, there is a
small window of time when it could be possible to read GICC_IAR in Xen
on a different pcpu than the one we are injecting the virtual irq.
It involves migrating an interrupt more than once before EOIing the one
that is currently INPROGRESS.

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Julien Grall]

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.

>> 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@kaball.uk.xensource.com 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.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Stefano Stabellini]

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@kaball.uk.xensource.com 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?

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Julien Grall]

[-- Attachment #1.1: Type: text/plain, Size: 2120 bytes --]

On 20 Jun 2014 18:41, "Stefano Stabellini" <stefano.stabellini@eu.citrix.com>
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@kaball.uk.xensource.com 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.

-- 
Julien Grall

[-- Attachment #1.2: Type: text/html, Size: 3025 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Stefano Stabellini]

On Fri, 20 Jun 2014, Julien Grall wrote:
> On 20 Jun 2014 18:41, "Stefano Stabellini" <stefano.stabellini@eu.citrix.com> 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@kaball.uk.xensource.com 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.

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Julien Grall]

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@eu.citrix.com> 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@kaball.uk.xensource.com 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.

-- 
Julien Grall

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Stefano Stabellini]

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@eu.citrix.com> 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@kaball.uk.xensource.com 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.

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Julien Grall]

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@eu.citrix.com> 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@kaball.uk.xensource.com 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

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Stefano Stabellini]

On Sat, 21 Jun 2014, Julien Grall wrote:
> 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@eu.citrix.com> 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@kaball.uk.xensource.com
> > > > > > > > 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.

This is a valid concern.
I thought about this concurrency problem and I think I managed to
solve it correctly. The solution I used was introducing a new flag
called "GIC_IRQ_GUEST_MIGRATING". See:

1402504032-13267-4-git-send-email-stefano.stabellini@eu.citrix.com

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Ian Campbell]

On Mon, 2014-06-23 at 11:43 +0100, Stefano Stabellini wrote:
> I thought about this concurrency problem and I think I managed to
> solve it correctly. The solution I used was introducing a new flag
> called "GIC_IRQ_GUEST_MIGRATING". See:
> 
> 1402504032-13267-4-git-send-email-stefano.stabellini@eu.citrix.com

So, I'm not sure whether I should apply this patch now or wait for that
series to go in first. What should I do?

Ian.

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Julien Grall]

On 06/27/2014 04:51 PM, Ian Campbell wrote:
> On Mon, 2014-06-23 at 11:43 +0100, Stefano Stabellini wrote:
>> I thought about this concurrency problem and I think I managed to
>> solve it correctly. The solution I used was introducing a new flag
>> called "GIC_IRQ_GUEST_MIGRATING". See:
>>
>> 1402504032-13267-4-git-send-email-stefano.stabellini@eu.citrix.com
> 
> So, I'm not sure whether I should apply this patch now or wait for that
> series to go in first. What should I do?

IIRC, I've asked Stefano to rewrote a bit the following paragraph.

"
> No need to find the pcpu that needs to write to GICC_DIR, because after
> "physical irq follow virtual irq" we always inject the virtual irq on
> the vcpu that is running on the pcpu that received the interrupt.
"

The "physical irq follow virtual irq" is part of the migration series
but this is not the reason that make GICC_DIR working. This is GIC
specific (even though, I can't find a clearly define behavior in the
specification).

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Stefano Stabellini]

On Mon, 30 Jun 2014, Julien Grall wrote:
> On 06/27/2014 04:51 PM, Ian Campbell wrote:
> > On Mon, 2014-06-23 at 11:43 +0100, Stefano Stabellini wrote:
> >> I thought about this concurrency problem and I think I managed to
> >> solve it correctly. The solution I used was introducing a new flag
> >> called "GIC_IRQ_GUEST_MIGRATING". See:
> >>
> >> 1402504032-13267-4-git-send-email-stefano.stabellini@eu.citrix.com
> > 
> > So, I'm not sure whether I should apply this patch now or wait for that
> > series to go in first. What should I do?
> 
> IIRC, I've asked Stefano to rewrote a bit the following paragraph.
> 
> "
> > No need to find the pcpu that needs to write to GICC_DIR, because after
> > "physical irq follow virtual irq" we always inject the virtual irq on
> > the vcpu that is running on the pcpu that received the interrupt.
> "
> 
> The "physical irq follow virtual irq" is part of the migration series
> but this is not the reason that make GICC_DIR working. This is GIC
> specific (even though, I can't find a clearly define behavior in the
> specification).

Yes, I'll rewrite and resend.

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Stefano Stabellini]

On Fri, 27 Jun 2014, Ian Campbell wrote:
> On Mon, 2014-06-23 at 11:43 +0100, Stefano Stabellini wrote:
> > I thought about this concurrency problem and I think I managed to
> > solve it correctly. The solution I used was introducing a new flag
> > called "GIC_IRQ_GUEST_MIGRATING". See:
> > 
> > 1402504032-13267-4-git-send-email-stefano.stabellini@eu.citrix.com
> 
> So, I'm not sure whether I should apply this patch now or wait for that
> series to go in first. What should I do?

You can go ahead and apply it.
I have just sent v2 with a better commit message:

<1404315229-31047-1-git-send-email-stefano.stabellini@eu.citrix.com>

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Julien Grall]

Hi Stefano,

The commit title doesn't reflect the patch. You don't add a new
platform_need_explicit_eoi but a new quirk.

On 06/20/2014 02:35 PM, Stefano Stabellini wrote:
> +/*
> + * Quirk for platforms where GICH_LR_HW does not work as expected.
> + */
> +#define PLATFORM_QUIRK_NEED_EOI       (1 << 1)
> +

I would rename to PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI. So people will
directly know from the name that it's only for guest pirq.

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: introduce platform_need_explicit_eoi

[Stefano Stabellini]

On Wed, 2 Jul 2014, Julien Grall wrote:
> Hi Stefano,
> 
> The commit title doesn't reflect the patch. You don't add a new
> platform_need_explicit_eoi but a new quirk.

OK

> On 06/20/2014 02:35 PM, Stefano Stabellini wrote:
> > +/*
> > + * Quirk for platforms where GICH_LR_HW does not work as expected.
> > + */
> > +#define PLATFORM_QUIRK_NEED_EOI       (1 << 1)
> > +
> 
> I would rename to PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI. So people will
> directly know from the name that it's only for guest pirq.

OK