[PATCH 0/2] vgic emulation and GICD_ITARGETSR

[PATCH 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: add a warning if the guest asks for SPI delivery to vcpu != 0.
      xen/arm: observe itarget setting in vgic_enable_irqs and vgic_disable_irqs

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

[PATCH 1/2] xen/arm: add a warning if the guest asks for SPI delivery to vcpu != 0.

[Stefano Stabellini]

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

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 4869b87..e4f38a0 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -581,6 +581,11 @@ 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;
+        if ( *r )
+        {
+            gdprintk(XENLOG_DEBUG, "SPI delivery to seconday cpus is unimplemented\n");
+            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

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

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

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index e4f38a0..0f0ba1d 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -376,12 +376,25 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
     unsigned int irq;
     unsigned long flags;
     int i = 0;
+    int target;
+    struct vcpu *v_target;
+    struct vgic_irq_rank *rank;
 
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
-        p = irq_to_pending(v, irq);
+        rank = vgic_irq_rank(v, 1, irq/32);
+        vgic_lock_rank(v, rank);
+        if ( irq >= 32 )
+        {
+            target = rank->itargets[(irq%32)/4] >> (8*(irq % 4));
+            target &= 0xff;
+            v_target = v->domain->vcpu[target];
+        } else
+            v_target = v;
+        vgic_unlock_rank(v, rank);
+        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,21 +412,34 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
     unsigned int irq;
     unsigned long flags;
     int i = 0;
+    int target;
+    struct vcpu *v_target;
+    struct vgic_irq_rank *rank;
 
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
-        p = irq_to_pending(v, irq);
+        rank = vgic_irq_rank(v, 1, irq/32);
+        vgic_lock_rank(v, rank);
+        if ( irq >= 32 )
+        {
+            target = rank->itargets[(irq%32)/4] >> (8*(irq % 4));
+            target &= 0xff;
+            v_target = v->domain->vcpu[target];
+        } else
+            v_target = v;
+        vgic_unlock_rank(v, rank);
+        p = irq_to_pending(v_target, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
-        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 1/2] xen/arm: add a warning if the guest asks for SPI delivery to vcpu != 0.

[Julien Grall]

Hi Stefano,

On 25/05/14 19:06, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>   xen/arch/arm/vgic.c |    5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 4869b87..e4f38a0 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -581,6 +581,11 @@ 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;
> +        if ( *r )

This is wrong, ITARGETSR store a  4 bitmask of CPUs, one per interrupt. 
Each bit of the mask correspond to a CPU (see Table 4-17 in the GICv2 
manual).

Furthermore, I think it's safe to just ignore write. The manual says:
"It is IMPLEMENTATION DEFINED which, if any, SPIs are statically 
configured in hardware. The CPU targets field for such an SPI is 
read-only, and returns a value that indicates the CPU targets for the 
interrupt."

With the former comment, your patch #2 is also wrong.

> +        {
> +            gdprintk(XENLOG_DEBUG, "SPI delivery to seconday cpus is unimplemented\n");

s/seconday/secondary/

Regards,

-- 
Julien Grall

Re: [PATCH 1/2] xen/arm: add a warning if the guest asks for SPI delivery to vcpu != 0.

[Stefano Stabellini]

On Sun, 25 May 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 25/05/14 19:06, Stefano Stabellini wrote:
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >   xen/arch/arm/vgic.c |    5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 4869b87..e4f38a0 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -581,6 +581,11 @@ 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;
> > +        if ( *r )
> 
> This is wrong, ITARGETSR store a  4 bitmask of CPUs, one per interrupt. Each
> bit of the mask correspond to a CPU (see Table 4-17 in the GICv2 manual).

you are right


> Furthermore, I think it's safe to just ignore write. The manual says:
> "It is IMPLEMENTATION DEFINED which, if any, SPIs are statically configured in
> hardware. The CPU targets field for such an SPI is read-only, and returns a
> value that indicates the CPU targets for the interrupt."

Good point


> With the former comment, your patch #2 is also wrong.

Patch #2 is valid regardless of the specific itarget setting


> > +        {
> > +            gdprintk(XENLOG_DEBUG, "SPI delivery to seconday cpus is
> > unimplemented\n");
> 
> s/seconday/secondary/
> 
> Regards,
> 
> -- 
> Julien Grall
> 

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

[Julien Grall]

Hi Stefano,

On 05/25/2014 07:06 PM, Stefano Stabellini wrote:
>      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
> -        p = irq_to_pending(v, irq);
> +        rank = vgic_irq_rank(v, 1, irq/32);
> +        vgic_lock_rank(v, rank);
> +        if ( irq >= 32 )
> +        {
> +            target = rank->itargets[(irq%32)/4] >> (8*(irq % 4));
> +            target &= 0xff;
> +            v_target = v->domain->vcpu[target];

Without looking to the target stuff (see comment on patch #1), I don't
need to do a specific case for SPIs.

It will avoid diverging following the IRQ type.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Tue, 27 May 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 05/25/2014 07:06 PM, Stefano Stabellini wrote:
> >      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> >          irq = i + (32 * n);
> > -        p = irq_to_pending(v, irq);
> > +        rank = vgic_irq_rank(v, 1, irq/32);
> > +        vgic_lock_rank(v, rank);
> > +        if ( irq >= 32 )
> > +        {
> > +            target = rank->itargets[(irq%32)/4] >> (8*(irq % 4));
> > +            target &= 0xff;
> > +            v_target = v->domain->vcpu[target];
> 
> Without looking to the target stuff (see comment on patch #1), I don't
> need to do a specific case for SPIs.
> 
> It will avoid diverging following the IRQ type.

Sooner or later we'll implement SPI delivery to vcpu != 0. When we do
we'll actually need this patch, that is correct even without SPI
delivery to vcpu != 0.

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

[Julien Grall]

On 05/27/2014 06:02 PM, Stefano Stabellini wrote:
> On Tue, 27 May 2014, Julien Grall wrote:
>> On 05/25/2014 07:06 PM, Stefano Stabellini wrote:
>>>      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>>>          irq = i + (32 * n);
>>> -        p = irq_to_pending(v, irq);
>>> +        rank = vgic_irq_rank(v, 1, irq/32);
>>> +        vgic_lock_rank(v, rank);
>>> +        if ( irq >= 32 )
>>> +        {
>>> +            target = rank->itargets[(irq%32)/4] >> (8*(irq % 4));
>>> +            target &= 0xff;
>>> +            v_target = v->domain->vcpu[target];
>>
>> Without looking to the target stuff (see comment on patch #1), I don't
>> need to do a specific case for SPIs.
>>
>> It will avoid diverging following the IRQ type.
> 
> Sooner or later we'll implement SPI delivery to vcpu != 0. When we do
> we'll actually need this patch, that is correct even without SPI
> delivery to vcpu != 0.
> 

Hrrrm .... right I forgot this case. If so, the vgic_irq_rank_lock
doesn't need to be taken for non-SPIs.

Regards,

-- 
Julien Grall

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

[Julien Grall]

On 05/27/2014 06:02 PM, Stefano Stabellini wrote:
> On Tue, 27 May 2014, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 05/25/2014 07:06 PM, Stefano Stabellini wrote:
>>>      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>>>          irq = i + (32 * n);
>>> -        p = irq_to_pending(v, irq);
>>> +        rank = vgic_irq_rank(v, 1, irq/32);
>>> +        vgic_lock_rank(v, rank);
>>> +        if ( irq >= 32 )
>>> +        {
>>> +            target = rank->itargets[(irq%32)/4] >> (8*(irq % 4));
>>> +            target &= 0xff;
>>> +            v_target = v->domain->vcpu[target];
>>
>> Without looking to the target stuff (see comment on patch #1), I don't
>> need to do a specific case for SPIs.
>>
>> It will avoid diverging following the IRQ type.
> 
> Sooner or later we'll implement SPI delivery to vcpu != 0. When we do
> we'll actually need this patch, that is correct even without SPI
> delivery to vcpu != 0.
> 

To be clear, I didn't say this patch was not useful. I just say we can
merge the code and do use the same path for non-SPIs.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Sun, 2014-05-25 at 19:06 +0100, Stefano Stabellini wrote:
> 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.

How hard would this be to support? I think all interrupts already all
arrive at pCPU==0, so we must be capable of forwarding them to whichever
pCPU might be running vCPU==0? Is extending that to select wihch vcpu to
send to hard?

> 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: add a warning if the guest asks for SPI delivery to vcpu != 0.
>       xen/arm: observe itarget setting in vgic_enable_irqs and vgic_disable_irqs
> 
>  xen/arch/arm/vgic.c |   47 +++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 39 insertions(+), 8 deletions(-)

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

[Ian Campbell]

On Sun, 2014-05-25 at 19:06 +0100, Stefano Stabellini wrote:
> 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.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/vgic.c |   42 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index e4f38a0..0f0ba1d 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -376,12 +376,25 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>      unsigned int irq;
>      unsigned long flags;
>      int i = 0;
> +    int target;
> +    struct vcpu *v_target;
> +    struct vgic_irq_rank *rank;
>  
>      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
> -        p = irq_to_pending(v, irq);
> +        rank = vgic_irq_rank(v, 1, irq/32);
> +        vgic_lock_rank(v, rank);
> +        if ( irq >= 32 )
> +        {
> +            target = rank->itargets[(irq%32)/4] >> (8*(irq % 4));
> +            target &= 0xff;

This is byte_read(), isn't it?

> +            v_target = v->domain->vcpu[target];

There needs to be some sort of range check here I think. Else you are
setting a trap for whoever implements itargets properly.

> +        } else
> +            v_target = v;
> +        vgic_unlock_rank(v, rank);
> +        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,21 +412,34 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>      unsigned int irq;
>      unsigned long flags;
>      int i = 0;
> +    int target;
> +    struct vcpu *v_target;
> +    struct vgic_irq_rank *rank;
>  
>      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
>          irq = i + (32 * n);
> -        p = irq_to_pending(v, irq);
> +        rank = vgic_irq_rank(v, 1, irq/32);
> +        vgic_lock_rank(v, rank);
> +        if ( irq >= 32 )
> +        {
> +            target = rank->itargets[(irq%32)/4] >> (8*(irq % 4));
> +            target &= 0xff;
> +            v_target = v->domain->vcpu[target];
> +        } else
> +            v_target = v;

This is the same code as above -- there should be a helper
(get_target_vcpu or some such).

> +        vgic_unlock_rank(v, rank);
> +        p = irq_to_pending(v_target, irq);
>          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> -        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 )
>          {

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

[Stefano Stabellini]

On Wed, 28 May 2014, Ian Campbell wrote:
> On Sun, 2014-05-25 at 19:06 +0100, Stefano Stabellini wrote:
> > 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.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  xen/arch/arm/vgic.c |   42 ++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 34 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index e4f38a0..0f0ba1d 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -376,12 +376,25 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
> >      unsigned int irq;
> >      unsigned long flags;
> >      int i = 0;
> > +    int target;
> > +    struct vcpu *v_target;
> > +    struct vgic_irq_rank *rank;
> >  
> >      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> >          irq = i + (32 * n);
> > -        p = irq_to_pending(v, irq);
> > +        rank = vgic_irq_rank(v, 1, irq/32);
> > +        vgic_lock_rank(v, rank);
> > +        if ( irq >= 32 )
> > +        {
> > +            target = rank->itargets[(irq%32)/4] >> (8*(irq % 4));
> > +            target &= 0xff;
> 
> This is byte_read(), isn't it?

yes, I'll use it


> > +            v_target = v->domain->vcpu[target];
> 
> There needs to be some sort of range check here I think. Else you are
> setting a trap for whoever implements itargets properly.

The check should be at the point where we write itargets, not here.
The previous patch already introduces a check for itargets to always be
zero.


> > +        } else
> > +            v_target = v;
> > +        vgic_unlock_rank(v, rank);
> > +        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,21 +412,34 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> >      unsigned int irq;
> >      unsigned long flags;
> >      int i = 0;
> > +    int target;
> > +    struct vcpu *v_target;
> > +    struct vgic_irq_rank *rank;
> >  
> >      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> >          irq = i + (32 * n);
> > -        p = irq_to_pending(v, irq);
> > +        rank = vgic_irq_rank(v, 1, irq/32);
> > +        vgic_lock_rank(v, rank);
> > +        if ( irq >= 32 )
> > +        {
> > +            target = rank->itargets[(irq%32)/4] >> (8*(irq % 4));
> > +            target &= 0xff;
> > +            v_target = v->domain->vcpu[target];
> > +        } else
> > +            v_target = v;
> 
> This is the same code as above -- there should be a helper
> (get_target_vcpu or some such).

Good idea


> > +        vgic_unlock_rank(v, rank);
> > +        p = irq_to_pending(v_target, irq);
> >          set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> > -        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 )
> >          {
> 
> 

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

[Stefano Stabellini]

On Tue, 27 May 2014, Julien Grall wrote:
> On 05/27/2014 06:02 PM, Stefano Stabellini wrote:
> > On Tue, 27 May 2014, Julien Grall wrote:
> >> Hi Stefano,
> >>
> >> On 05/25/2014 07:06 PM, Stefano Stabellini wrote:
> >>>      while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> >>>          irq = i + (32 * n);
> >>> -        p = irq_to_pending(v, irq);
> >>> +        rank = vgic_irq_rank(v, 1, irq/32);
> >>> +        vgic_lock_rank(v, rank);
> >>> +        if ( irq >= 32 )
> >>> +        {
> >>> +            target = rank->itargets[(irq%32)/4] >> (8*(irq % 4));
> >>> +            target &= 0xff;
> >>> +            v_target = v->domain->vcpu[target];
> >>
> >> Without looking to the target stuff (see comment on patch #1), I don't
> >> need to do a specific case for SPIs.
> >>
> >> It will avoid diverging following the IRQ type.
> > 
> > Sooner or later we'll implement SPI delivery to vcpu != 0. When we do
> > we'll actually need this patch, that is correct even without SPI
> > delivery to vcpu != 0.
> > 
> 
> To be clear, I didn't say this patch was not useful. I just say we can
> merge the code and do use the same path for non-SPIs.

Ah OK