[PATCH v2 0/2] vgic emulation and GICD_ITARGETSR

[PATCH v2 0/2] vgic emulation and GICD_ITARGETSR

[Stefano Stabellini]

Hi all,
this small patch series improves vgic emulation in relation to
GICD_ITARGETSR and irq delivery.

At the moment we don't support irq delivery to vcpu != 0, so prevent the
guest from setting itarget to something != 0.
vgic_enable_irqs and vgic_disable_irqs currently ignore the itarget
settings and just enable/disable irqs on the current vcpu. Fix their
behaviour to enable/disable irqs on the vcpu set by itarget, that is
always vcpu0 for irq >= 32.


Stefano Stabellini (2):
      xen/arm: ignore guest writes to GICD_ITARGETSR for SPIs
      xen/arm: observe itarget setting in vgic_enable_irqs and vgic_disable_irqs

 xen/arch/arm/vgic.c |   50 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 42 insertions(+), 8 deletions(-)

[PATCH v2 1/2] xen/arm: ignore guest writes to GICD_ITARGETSR for SPIs

[Stefano Stabellini]

Ignore guest writes to GICD_ITARGETSR that set the target cpu to a cpu
other than cpu0 for SPIs.

Also ignore guest writes to GICD_ITARGETSR for PPIs and SGIs as they can
only be delivered to the same cpu and that has already been configured
at initialization time.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v2:
- ignore writes to rank 0;
- don't print a warning for ignoring writes to GICD_ITARGETSR;
- add a comment in the code to remember that we don't implement writes
to GICD_ITARGETSR.
---
 xen/arch/arm/vgic.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index cb8df3a..1304b5e 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -584,6 +584,13 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
         rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
         if ( rank == NULL) goto write_ignore;
+        /* only same vcpu delivery can be allowed for PPIs and SGIs */
+        if ( REG_RANK_NR(8, gicd_reg - GICD_ITARGETSR) == 0 ) 
+            return 1;
+        /* SPI delivery to secondary vcpus is unimplemented */
+        if ( REG_RANK_NR(8, gicd_reg - GICD_ITARGETSR) > 0 &&
+             *r != (1|1<<8|1<<16|1<<24) )
+            return 1;
         vgic_lock_rank(v, rank);
         if ( dabt.size == 2 )
             rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = *r;
-- 
1.7.10.4

Re: [PATCH v2 1/2] xen/arm: ignore guest writes to GICD_ITARGETSR for SPIs

[Ian Campbell]

On Tue, 2014-06-03 at 17:07 +0100, Stefano Stabellini wrote:
> Ignore guest writes to GICD_ITARGETSR that set the target cpu to a cpu
> other than cpu0 for SPIs.
> 
> Also ignore guest writes to GICD_ITARGETSR for PPIs and SGIs as they can
> only be delivered to the same cpu and that has already been configured
> at initialization time.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> Changes in v2:
> - ignore writes to rank 0;
> - don't print a warning for ignoring writes to GICD_ITARGETSR;
> - add a comment in the code to remember that we don't implement writes
> to GICD_ITARGETSR.
> ---
>  xen/arch/arm/vgic.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index cb8df3a..1304b5e 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -584,6 +584,13 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>          if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
>          rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
>          if ( rank == NULL) goto write_ignore;
> +        /* only same vcpu delivery can be allowed for PPIs and SGIs */
> +        if ( REG_RANK_NR(8, gicd_reg - GICD_ITARGETSR) == 0 ) 
> +            return 1;

This is handled by a different case of the switch statement.

> +        /* SPI delivery to secondary vcpus is unimplemented */
> +        if ( REG_RANK_NR(8, gicd_reg - GICD_ITARGETSR) > 0 &&
> +             *r != (1|1<<8|1<<16|1<<24) )
> +            return 1;

Given that itargets is (about to be, in the next patch) initialised to
the same value isn't this entire hunk just a complicated way of saying
"goto write_ignore"?

(and if not please use goto write_ignore with the condition, since it is
self documenting)

And the REG_RANK_NR check here is unnecessary, because you checked for
== 0 already (and as I say it's handled further up anyway).

But -- doesn't your second patch actually make writing to ITARGETSR
work?

>          vgic_lock_rank(v, rank);
>          if ( dabt.size == 2 )
>              rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = *r;

Re: [PATCH v2 1/2] xen/arm: ignore guest writes to GICD_ITARGETSR for SPIs

[Julien Grall]

Hi Stefano,

On 06/03/2014 05:07 PM, Stefano Stabellini wrote:
> Ignore guest writes to GICD_ITARGETSR that set the target cpu to a cpu
> other than cpu0 for SPIs.
> 
> Also ignore guest writes to GICD_ITARGETSR for PPIs and SGIs as they can
> only be delivered to the same cpu and that has already been configured
> at initialization time.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> Changes in v2:
> - ignore writes to rank 0;
> - don't print a warning for ignoring writes to GICD_ITARGETSR;
> - add a comment in the code to remember that we don't implement writes
> to GICD_ITARGETSR.
> ---
>  xen/arch/arm/vgic.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index cb8df3a..1304b5e 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -584,6 +584,13 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>          if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
>          rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
>          if ( rank == NULL) goto write_ignore;
> +        /* only same vcpu delivery can be allowed for PPIs and SGIs */
> +        if ( REG_RANK_NR(8, gicd_reg - GICD_ITARGETSR) == 0 ) 
> +            return 1;

This test is wrong. PPIs and SGIs are already a preset value (see
vcpu_vgic_init).

Futhermore, there is already a specific case for PPIs and SGIs (see few
lines above).

> +        /* SPI delivery to secondary vcpus is unimplemented */
> +        if ( REG_RANK_NR(8, gicd_reg - GICD_ITARGETSR) > 0 &&
> +             *r != (1|1<<8|1<<16|1<<24) )
> +            return 1;

You don't handle byte-access here. Rather than testing the value, I
think it's fine to return unconditionally.

>          vgic_lock_rank(v, rank);
>          if ( dabt.size == 2 )
>              rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = *r;
> 

Regards,

-- 
Julien Grall

[PATCH v2 2/2] xen/arm: observe itarget setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini]

vgic_enable_irqs should enable irq delivery to the vcpu specified by
GICD_ITARGETSR, rather than the vcpu that wrote to GICD_ISENABLER.
Similarly vgic_disable_irqs should use the target vcpu specified by
itarget to disable irqs.

Correctly initialize itargets for SPIs.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v2:
- refactor the common code in get_target_vcpu;
- unify PPI and SPI paths;
- correctly initialize itargets for SPI;
- use byte_read.
---
 xen/arch/arm/vgic.c |   43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 1304b5e..55ed27d 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -106,7 +106,15 @@ int domain_vgic_init(struct domain *d)
         INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
     }
     for (i=0; i<DOMAIN_NR_RANKS(d); i++)
+    {
+        int j;
+
         spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
+        /* Only delivery to CPU0 */
+        for ( j = 0 ; j < 8 ; j++ )
+            d->arch.vgic.shared_irqs[i].itargets[j] =
+                (1<<0) | (1<<8) | (1<<16) | (1<<24);
+    }
     return 0;
 }
 
@@ -369,6 +377,21 @@ read_as_zero:
     return 1;
 }
 
+static struct vcpu *get_target_vcpu(struct vcpu *v, unsigned int irq)
+{
+    int target;
+    struct vgic_irq_rank *rank;
+    struct vcpu *v_target;
+
+    rank = vgic_irq_rank(v, 1, irq/32);
+    vgic_lock_rank(v, rank);
+    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
+    target = find_next_bit((const unsigned long *) &target, 8, 0);
+    v_target = v->domain->vcpu[target];
+    vgic_unlock_rank(v, rank);
+    return v_target;
+}
+
 static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
 {
     const unsigned long mask = r;
@@ -376,12 +399,14 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
     unsigned int irq;
     unsigned long flags;
     int i = 0;
+    struct vcpu *v_target;
 
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
-        p = irq_to_pending(v, irq);
+        v_target = get_target_vcpu(v, irq);
+        p = irq_to_pending(v_target, irq);
         clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
-        gic_remove_from_queues(v, irq);
+        gic_remove_from_queues(v_target, irq);
         if ( p->desc != NULL )
         {
             spin_lock_irqsave(&p->desc->lock, flags);
@@ -399,24 +424,26 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
     unsigned int irq;
     unsigned long flags;
     int i = 0;
+    struct vcpu *v_target;
 
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
-        p = irq_to_pending(v, irq);
+        v_target = 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->domain->arch.evtchn_irq &&
+        if ( irq == v_target->domain->arch.evtchn_irq &&
              vcpu_info(current, evtchn_upcall_pending) &&
              list_empty(&p->inflight) )
-            vgic_vcpu_inject_irq(v, irq);
+            vgic_vcpu_inject_irq(v_target, irq);
         else {
             unsigned long flags;
-            spin_lock_irqsave(&v->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, irq, p->priority);
-            spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+                gic_raise_guest_irq(v_target, irq, p->priority);
+            spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
         }
         if ( p->desc != NULL )
         {
-- 
1.7.10.4

Re: [PATCH v2 2/2] xen/arm: observe itarget setting in vgic_enable_irqs and vgic_disable_irqs

[Julien Grall]

Hi Stefano,

On 06/03/2014 05:07 PM, Stefano Stabellini wrote:
> +static struct vcpu *get_target_vcpu(struct vcpu *v, unsigned int irq)
> +{
> +    int target;
> +    struct vgic_irq_rank *rank;
> +    struct vcpu *v_target;
> +
> +    rank = vgic_irq_rank(v, 1, irq/32);
> +    vgic_lock_rank(v, rank);
> +    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
> +    target = find_next_bit((const unsigned long *) &target, 8, 0);

It might be interesting to add an ASSERT(target < d->max_vcpus) here.

Other than that this patch looks good to me:

Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

Re: [PATCH v2 0/2] vgic emulation and GICD_ITARGETSR

[Ian Campbell]

On Tue, 2014-06-03 at 17:04 +0100, Stefano Stabellini wrote:
> Hi all,
> this small patch series improves vgic emulation in relation to
> GICD_ITARGETSR and irq delivery.

I think this somehow relates to the little discussion on "[PATCH v8
13/13] gic_remove_from_queues: take a lock on the right vcpu" is that
correct?

Do these two patches need to go in instead of that one? Or as precursors
to it? Or maybe they are nothing to do with each other?

Ian.

> 
> At the moment we don't support irq delivery to vcpu != 0, so prevent the
> guest from setting itarget to something != 0.
> vgic_enable_irqs and vgic_disable_irqs currently ignore the itarget
> settings and just enable/disable irqs on the current vcpu. Fix their
> behaviour to enable/disable irqs on the vcpu set by itarget, that is
> always vcpu0 for irq >= 32.
> 
> 
> Stefano Stabellini (2):
>       xen/arm: ignore guest writes to GICD_ITARGETSR for SPIs
>       xen/arm: observe itarget setting in vgic_enable_irqs and vgic_disable_irqs
> 
>  xen/arch/arm/vgic.c |   50 ++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 42 insertions(+), 8 deletions(-)

Re: [PATCH v2 0/2] vgic emulation and GICD_ITARGETSR

[Ian Campbell]

On Fri, 2014-06-06 at 17:29 +0100, Ian Campbell wrote:

I meant to reply to v3 of this series, sorry!

> On Tue, 2014-06-03 at 17:04 +0100, Stefano Stabellini wrote:
> > Hi all,
> > this small patch series improves vgic emulation in relation to
> > GICD_ITARGETSR and irq delivery.
> 
> I think this somehow relates to the little discussion on "[PATCH v8
> 13/13] gic_remove_from_queues: take a lock on the right vcpu" is that
> correct?
> 
> Do these two patches need to go in instead of that one? Or as precursors
> to it? Or maybe they are nothing to do with each other?
> 
> Ian.
> 
> > 
> > At the moment we don't support irq delivery to vcpu != 0, so prevent the
> > guest from setting itarget to something != 0.
> > vgic_enable_irqs and vgic_disable_irqs currently ignore the itarget
> > settings and just enable/disable irqs on the current vcpu. Fix their
> > behaviour to enable/disable irqs on the vcpu set by itarget, that is
> > always vcpu0 for irq >= 32.
> > 
> > 
> > Stefano Stabellini (2):
> >       xen/arm: ignore guest writes to GICD_ITARGETSR for SPIs
> >       xen/arm: observe itarget setting in vgic_enable_irqs and vgic_disable_irqs
> > 
> >  xen/arch/arm/vgic.c |   50 ++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 42 insertions(+), 8 deletions(-)
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v2 0/2] vgic emulation and GICD_ITARGETSR

[Stefano Stabellini]

On Fri, 6 Jun 2014, Ian Campbell wrote:
> On Tue, 2014-06-03 at 17:04 +0100, Stefano Stabellini wrote:
> > Hi all,
> > this small patch series improves vgic emulation in relation to
> > GICD_ITARGETSR and irq delivery.
> 
> I think this somehow relates to the little discussion on "[PATCH v8
> 13/13] gic_remove_from_queues: take a lock on the right vcpu" is that
> correct?
> 
> Do these two patches need to go in instead of that one? Or as precursors
> to it? Or maybe they are nothing to do with each other?
> 

Patch 13/13 identified the problem but didn't fix it properly, this
series does.
I kept it separately because it doesn't have anything to do with
the removal of the maintenance interrupt.

The only relation between the two series is that I wrote this one on
top of the other.

 
> > 
> > At the moment we don't support irq delivery to vcpu != 0, so prevent the
> > guest from setting itarget to something != 0.
> > vgic_enable_irqs and vgic_disable_irqs currently ignore the itarget
> > settings and just enable/disable irqs on the current vcpu. Fix their
> > behaviour to enable/disable irqs on the vcpu set by itarget, that is
> > always vcpu0 for irq >= 32.
> > 
> > 
> > Stefano Stabellini (2):
> >       xen/arm: ignore guest writes to GICD_ITARGETSR for SPIs
> >       xen/arm: observe itarget setting in vgic_enable_irqs and vgic_disable_irqs
> > 
> >  xen/arch/arm/vgic.c |   50 ++++++++++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 42 insertions(+), 8 deletions(-)
> 
> 

Re: [PATCH v2 0/2] vgic emulation and GICD_ITARGETSR

[Ian Campbell]

On Mon, 2014-06-09 at 11:41 +0100, Stefano Stabellini wrote:
> On Fri, 6 Jun 2014, Ian Campbell wrote:
> > On Tue, 2014-06-03 at 17:04 +0100, Stefano Stabellini wrote:
> > > Hi all,
> > > this small patch series improves vgic emulation in relation to
> > > GICD_ITARGETSR and irq delivery.
> > 
> > I think this somehow relates to the little discussion on "[PATCH v8
> > 13/13] gic_remove_from_queues: take a lock on the right vcpu" is that
> > correct?
> > 
> > Do these two patches need to go in instead of that one? Or as precursors
> > to it? Or maybe they are nothing to do with each other?
> > 
> 
> Patch 13/13 identified the problem but didn't fix it properly, this
> series does.
> I kept it separately because it doesn't have anything to do with
> the removal of the maintenance interrupt.
> 
> The only relation between the two series is that I wrote this one on
> top of the other.

OK, so I can apply the series in either order and it'll be fine? Or
should I apply the 13-patch maintenance interrupt series first?

Ian.

Re: [PATCH v2 0/2] vgic emulation and GICD_ITARGETSR

[Stefano Stabellini]

On Tue, 10 Jun 2014, Ian Campbell wrote:
> On Mon, 2014-06-09 at 11:41 +0100, Stefano Stabellini wrote:
> > On Fri, 6 Jun 2014, Ian Campbell wrote:
> > > On Tue, 2014-06-03 at 17:04 +0100, Stefano Stabellini wrote:
> > > > Hi all,
> > > > this small patch series improves vgic emulation in relation to
> > > > GICD_ITARGETSR and irq delivery.
> > > 
> > > I think this somehow relates to the little discussion on "[PATCH v8
> > > 13/13] gic_remove_from_queues: take a lock on the right vcpu" is that
> > > correct?
> > > 
> > > Do these two patches need to go in instead of that one? Or as precursors
> > > to it? Or maybe they are nothing to do with each other?
> > > 
> > 
> > Patch 13/13 identified the problem but didn't fix it properly, this
> > series does.
> > I kept it separately because it doesn't have anything to do with
> > the removal of the maintenance interrupt.
> > 
> > The only relation between the two series is that I wrote this one on
> > top of the other.
> 
> OK, so I can apply the series in either order and it'll be fine? Or
> should I apply the 13-patch maintenance interrupt series first?

You need to apply the series after the now 12-patch maintenance
interrupt series otherwise you'll get conflicts.