[PATCH 0/2] more vgic fixes

[PATCH 0/2] more vgic fixes

[Stefano Stabellini]

Hi all,
this patch series has a couple of fixes for the vgic emulation.

The first patch removes the workaround to inject the evtchn_irq when the
irq gets enabled. It is necessary to avoid recursive lock issues with
the following patch.

The second patch takes the rank lock to read ipriority from
vgic_vcpu_inject_irq.


Stefano Stabellini (2):
      xen/arm: remove workaround to inject evtchn_irq on irq enable
      xen/arm: take the rank lock before accessing ipriority

 xen/arch/arm/domain.c       |    4 +++-
 xen/arch/arm/domain_build.c |    2 ++
 xen/arch/arm/vgic.c         |   33 +++++++++++++++------------------
 3 files changed, 20 insertions(+), 19 deletions(-)

[PATCH 1/2] xen/arm: remove workaround to inject evtchn_irq on irq enable

[Stefano Stabellini]

evtchn_upcall_pending is already set by common code at vcpu creation,
therefore on ARM we also need to call vgic_vcpu_inject_irq for it.
Currently we do that from vgic_enable_irqs as a workaround.

Do this properly by calling vgic_vcpu_inject_irq in the appropriate
places at vcpu creation time, making sure to call it after the vcpu is
up (_VPF_down has been cleared).

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/domain.c       |    4 +++-
 xen/arch/arm/domain_build.c |    2 ++
 xen/arch/arm/vgic.c         |   18 ++++--------------
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e20ba0b..c29b063 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -651,8 +651,10 @@ int arch_set_info_guest(
     v->is_initialised = 1;
 
     if ( ctxt->flags & VGCF_online )
+    {
         clear_bit(_VPF_down, &v->pause_flags);
-    else
+        vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
+    } else
         set_bit(_VPF_down, &v->pause_flags);
 
     return 0;
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 9d9cba9..f7cf80d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1134,6 +1134,8 @@ int construct_dom0(struct domain *d)
     }
 #endif
 
+    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
+
     for ( i = 1, cpu = 0; i < d->max_vcpus; i++ )
     {
         cpu = cpumask_cycle(cpu, &cpu_online_map);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 7f1e263..1806b72 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -504,20 +504,10 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         v_target = _vgic_get_target_vcpu(v, irq);
         p = irq_to_pending(v_target, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
-        /* We need to force the first injection of evtchn_irq because
-         * evtchn_upcall_pending is already set by common code on vcpu
-         * creation. */
-        if ( irq == v_target->domain->arch.evtchn_irq &&
-             vcpu_info(current, evtchn_upcall_pending) &&
-             list_empty(&p->inflight) )
-            vgic_vcpu_inject_irq(v_target, irq);
-        else {
-            unsigned long flags;
-            spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
-            if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
-                gic_raise_guest_irq(v_target, irq, p->priority);
-            spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
-        }
+        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
+        if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
+            gic_raise_guest_irq(v_target, irq, p->priority);
+        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
         if ( p->desc != NULL )
         {
             irq_set_affinity(p->desc, cpumask_of(v_target->processor));
-- 
1.7.10.4

Re: [PATCH 1/2] xen/arm: remove workaround to inject evtchn_irq on irq enable

[Julien Grall]

Hi Stefano,

On 06/24/2014 07:11 PM, Stefano Stabellini wrote:
> evtchn_upcall_pending is already set by common code at vcpu creation,
> therefore on ARM we also need to call vgic_vcpu_inject_irq for it.
> Currently we do that from vgic_enable_irqs as a workaround.
> 
> Do this properly by calling vgic_vcpu_inject_irq in the appropriate
> places at vcpu creation time, making sure to call it after the vcpu is
> up (_VPF_down has been cleared).

While it's works perfectly on common case, as the toolstack is always
setting VGCF_online.

It would be possible to call the hypercall DOMCTL_vcpusetcontext without
this flags enable. If so, the new VCPU will never receive event channel
interrupt.

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/domain.c       |    4 +++-
>  xen/arch/arm/domain_build.c |    2 ++
>  xen/arch/arm/vgic.c         |   18 ++++--------------
>  3 files changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index e20ba0b..c29b063 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -651,8 +651,10 @@ int arch_set_info_guest(
>      v->is_initialised = 1;
>  
>      if ( ctxt->flags & VGCF_online )
> +    {
>          clear_bit(_VPF_down, &v->pause_flags);
> -    else
> +        vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);

I'd like a comment above each vgic_vcpu_inject(v, evtchn_irq) to explain
why we need them.

So in the future we won't need to spend hours to search in log because
someone has moved the line.

> +    } else

Coding style:

else
{

Regards,

-- 
Julien Grall

Re: [PATCH 1/2] xen/arm: remove workaround to inject evtchn_irq on irq enable

[Ian Campbell]

On Wed, 2014-06-25 at 16:03 +0100, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/24/2014 07:11 PM, Stefano Stabellini wrote:
> > evtchn_upcall_pending is already set by common code at vcpu creation,
> > therefore on ARM we also need to call vgic_vcpu_inject_irq for it.
> > Currently we do that from vgic_enable_irqs as a workaround.
> > 
> > Do this properly by calling vgic_vcpu_inject_irq in the appropriate
> > places at vcpu creation time, making sure to call it after the vcpu is
> > up (_VPF_down has been cleared).
> 
> While it's works perfectly on common case, as the toolstack is always
> setting VGCF_online.
> 
> It would be possible to call the hypercall DOMCTL_vcpusetcontext without
> this flags enable. If so, the new VCPU will never receive event channel
> interrupt.

I think the only other way to clear _VPF_down is the psci code, so I
suppose it needs a kick too.

The other option is VCPUOP_up whch we do not support (or wire up) on
ARM.

Re: [PATCH 1/2] xen/arm: remove workaround to inject evtchn_irq on irq enable

[Julien Grall]

On 07/02/2014 04:38 PM, Ian Campbell wrote:
> On Wed, 2014-06-25 at 16:03 +0100, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 06/24/2014 07:11 PM, Stefano Stabellini wrote:
>>> evtchn_upcall_pending is already set by common code at vcpu creation,
>>> therefore on ARM we also need to call vgic_vcpu_inject_irq for it.
>>> Currently we do that from vgic_enable_irqs as a workaround.
>>>
>>> Do this properly by calling vgic_vcpu_inject_irq in the appropriate
>>> places at vcpu creation time, making sure to call it after the vcpu is
>>> up (_VPF_down has been cleared).
>>
>> While it's works perfectly on common case, as the toolstack is always
>> setting VGCF_online.
>>
>> It would be possible to call the hypercall DOMCTL_vcpusetcontext without
>> this flags enable. If so, the new VCPU will never receive event channel
>> interrupt.
> 
> I think the only other way to clear _VPF_down is the psci code, so I
> suppose it needs a kick too.

The function PSCI on will call arch_set_info_guest directly with
VGCF_online.

> The other option is VCPUOP_up whch we do not support (or wire up) on
> ARM.

Hmmm... right I was looking directly to do_vcpu_op, but we have an extra
layer on ARM (do_arm_vcpu_op).

In this case, I would return an error if VGCF_online is not set. I will
allow us to catch easily an error later if we decide to implement VCPUOP_up.

Regards,

-- 
Julien Grall

Re: [PATCH 1/2] xen/arm: remove workaround to inject evtchn_irq on irq enable

[Stefano Stabellini]

On Wed, 2 Jul 2014, Julien Grall wrote:
> On 07/02/2014 04:38 PM, Ian Campbell wrote:
> > On Wed, 2014-06-25 at 16:03 +0100, Julien Grall wrote:
> >> Hi Stefano,
> >>
> >> On 06/24/2014 07:11 PM, Stefano Stabellini wrote:
> >>> evtchn_upcall_pending is already set by common code at vcpu creation,
> >>> therefore on ARM we also need to call vgic_vcpu_inject_irq for it.
> >>> Currently we do that from vgic_enable_irqs as a workaround.
> >>>
> >>> Do this properly by calling vgic_vcpu_inject_irq in the appropriate
> >>> places at vcpu creation time, making sure to call it after the vcpu is
> >>> up (_VPF_down has been cleared).
> >>
> >> While it's works perfectly on common case, as the toolstack is always
> >> setting VGCF_online.
> >>
> >> It would be possible to call the hypercall DOMCTL_vcpusetcontext without
> >> this flags enable. If so, the new VCPU will never receive event channel
> >> interrupt.
> > 
> > I think the only other way to clear _VPF_down is the psci code, so I
> > suppose it needs a kick too.
> 
> The function PSCI on will call arch_set_info_guest directly with
> VGCF_online.
> 
> > The other option is VCPUOP_up whch we do not support (or wire up) on
> > ARM.
> 
> Hmmm... right I was looking directly to do_vcpu_op, but we have an extra
> layer on ARM (do_arm_vcpu_op).
> 
> In this case, I would return an error if VGCF_online is not set. I will
> allow us to catch easily an error later if we decide to implement VCPUOP_up.

I could print a message and return an error, seems reasonable.

[PATCH 2/2] xen/arm: take the rank lock before accessing ipriority

[Stefano Stabellini]

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.

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.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/vgic.c |   15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 1806b72..1f55fcf 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -158,8 +158,8 @@ int vcpu_vgic_init(struct vcpu *v)
 #define vgic_lock(v)   spin_lock_irq(&(v)->domain->arch.vgic.lock)
 #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
 
-#define vgic_lock_rank(v, r) spin_lock(&(r)->lock)
-#define vgic_unlock_rank(v, r) spin_unlock(&(r)->lock)
+#define vgic_lock_rank(v, r) spin_lock_irqsave(&(r)->lock, flags)
+#define vgic_unlock_rank(v, r) spin_unlock_irqrestore(&(r)->lock, flags)
 
 static uint32_t byte_read(uint32_t val, int sign, int offset)
 {
@@ -191,6 +191,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
     struct vgic_irq_rank *rank;
     int offset = (int)(info->gpa - v->domain->arch.vgic.dbase);
     int gicd_reg = REG(offset);
+    unsigned long flags;
 
     switch ( gicd_reg )
     {
@@ -403,6 +404,7 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
 {
     struct vcpu *v_target;
     struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+    unsigned long flags;
 
     vgic_lock_rank(v, rank);
     v_target = _vgic_get_target_vcpu(v, irq);
@@ -595,6 +597,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     uint32_t tr;
     unsigned long trl;
     int i;
+    unsigned long flags;
 
     switch ( gicd_reg )
     {
@@ -856,6 +859,10 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
     unsigned long flags;
     bool_t running;
 
+    vgic_lock_rank(v, rank);
+    priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
+    vgic_unlock_rank(v, rank);
+
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
     /* vcpu offline */
@@ -873,12 +880,12 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     if ( !list_empty(&n->inflight) )
     {
+        if ( priority != n->priority )
+            gdprintk(XENLOG_WARNING, "IRQ is already inflight with a different priority\n");
         gic_raise_inflight_irq(v, irq);
         goto out;
     }
 
-    priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
-
     n->irq = irq;
     n->priority = priority;
 
-- 
1.7.10.4

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

[Ian Campbell]

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?

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.

Ian.

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

[Stefano Stabellini]

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.