diff mbox

[Xen-devel,v5,3/6] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

Message ID 1402504032-13267-3-git-send-email-stefano.stabellini@eu.citrix.com
State New
Headers show

Commit Message

Stefano Stabellini June 11, 2014, 4:27 p.m. UTC
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.

itargets can be set to a mask but vgic_get_target_vcpu always returns
the lower vcpu in the mask.

Correctly initialize itargets for SPIs.

Ignore bits in GICD_ITARGETSR corresponding to invalid vcpus.

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

---

Changes in v5:
- improve in-code comments;
- use vgic_rank_irq;
- use bit masks to write-ignore GICD_ITARGETSR;
- introduce an version of vgic_get_target_vcpu that doesn't take the
rank lock;
- keep the rank lock while enabling/disabling irqs;
- use find_first_bit instead of find_next_bit.

Changes in v4:
- remove assert that could allow a guest to crash Xen;
- add itargets validation to vgic_distr_mmio_write;
- export vgic_get_target_vcpu.

Changes in v3:
- add assert in get_target_vcpu;
- rename get_target_vcpu to vgic_get_target_vcpu.

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       |   71 +++++++++++++++++++++++++++++++++++++--------
 xen/include/asm-arm/gic.h |    2 ++
 2 files changed, 61 insertions(+), 12 deletions(-)

Comments

Julien Grall June 12, 2014, 9:45 a.m. UTC | #1
Hi Stefano,

On 11/06/14 17:27, Stefano Stabellini wrote:
> +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)

[..]

> +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)

Can you add comment explaining what are the differences between these 2 
functions?

AFAIU, the first one is assuming the rank lock is taken. If so, I would 
add an ASSERT in it. I would avoid people misuse the 2 functions.

>       case GICD_ISPENDR ... GICD_ISPENDRN:
> @@ -589,12 +625,23 @@ 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_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
>           if ( rank == NULL) goto write_ignore;
> +        /* 8-bit vcpu mask for this domain */

BUG_ON(v->domain->max_vcpus > 8)? Just for enforcement.

> +        tr = (1 << v->domain->max_vcpus) - 1;
> +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> +        tr &= *r;
> +        /* ignore zero writes */
> +        if ( !tr )
> +            goto write_ignore;
> +        if ( dabt.size == 2 &&
> +            !((tr & 0xff) && (tr & (0xff << 8)) &&
> +             (tr & (0xff << 16)) && (tr & (0xff << 24))))
> +            goto write_ignore;

I quite difficult to understand this check. Does this check is only for 
word-access?

AFAIU, with byte-access it's possible to write 0 in itargets. For this 
case, the register may contain some 1 out of the byte offset.

Regards,
Stefano Stabellini June 12, 2014, 2:42 p.m. UTC | #2
On Thu, 12 Jun 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/06/14 17:27, Stefano Stabellini wrote:
> > +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> 
> [..]
> 
> > +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> 
> Can you add comment explaining what are the differences between these 2
> functions?
> 
> AFAIU, the first one is assuming the rank lock is taken. If so, I would add an
> ASSERT in it. I would avoid people misuse the 2 functions.

OK


> >       case GICD_ISPENDR ... GICD_ISPENDRN:
> > @@ -589,12 +625,23 @@ 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_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
> >           if ( rank == NULL) goto write_ignore;
> > +        /* 8-bit vcpu mask for this domain */
> 
> BUG_ON(v->domain->max_vcpus > 8)? Just for enforcement.

OK


> > +        tr = (1 << v->domain->max_vcpus) - 1;
> > +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> > +        tr &= *r;
> > +        /* ignore zero writes */
> > +        if ( !tr )
> > +            goto write_ignore;
> > +        if ( dabt.size == 2 &&
> > +            !((tr & 0xff) && (tr & (0xff << 8)) &&
> > +             (tr & (0xff << 16)) && (tr & (0xff << 24))))
> > +            goto write_ignore;
> 
> I quite difficult to understand this check. Does this check is only for
> word-access?

The previous test covers byte-access after the change of the following
patch. I should move it to this patch to make it clearer.


> AFAIU, with byte-access it's possible to write 0 in itargets. For this case,
> the register may contain some 1 out of the byte offset.
Julien Grall June 15, 2014, 3:57 p.m. UTC | #3
On 12/06/14 15:42, Stefano Stabellini wrote:
>>> +        tr = (1 << v->domain->max_vcpus) - 1;
>>> +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
>>> +        tr &= *r;
>>> +        /* ignore zero writes */
>>> +        if ( !tr )
>>> +            goto write_ignore;
>>> +        if ( dabt.size == 2 &&
>>> +            !((tr & 0xff) && (tr & (0xff << 8)) &&
>>> +             (tr & (0xff << 16)) && (tr & (0xff << 24))))
>>> +            goto write_ignore;
>>
>> I quite difficult to understand this check. Does this check is only for
>> word-access?
>
> The previous test covers byte-access after the change of the following
> patch. I should move it to this patch to make it clearer.

Yes please.

Regards,
Ian Campbell June 18, 2014, 10:48 a.m. UTC | #4
On Wed, 2014-06-11 at 17:27 +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.
> 
> itargets can be set to a mask but vgic_get_target_vcpu always returns
> the lower vcpu in the mask.
> 
> Correctly initialize itargets for SPIs.
> 
> Ignore bits in GICD_ITARGETSR corresponding to invalid vcpus.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> Changes in v5:
> - improve in-code comments;
> - use vgic_rank_irq;
> - use bit masks to write-ignore GICD_ITARGETSR;
> - introduce an version of vgic_get_target_vcpu that doesn't take the
> rank lock;
> - keep the rank lock while enabling/disabling irqs;
> - use find_first_bit instead of find_next_bit.
> 
> Changes in v4:
> - remove assert that could allow a guest to crash Xen;
> - add itargets validation to vgic_distr_mmio_write;
> - export vgic_get_target_vcpu.
> 
> Changes in v3:
> - add assert in get_target_vcpu;
> - rename get_target_vcpu to vgic_get_target_vcpu.
> 
> 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       |   71 +++++++++++++++++++++++++++++++++++++--------
>  xen/include/asm-arm/gic.h |    2 ++
>  2 files changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 757707e..4d655af 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -111,7 +111,13 @@ 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++)
> +    {
>          spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> +        /* By default deliver to CPU0 */
> +        memset(d->arch.vgic.shared_irqs[i].itargets,
> +               0x1,
> +               8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]));

8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]) ==
sizeof(d->arch.vgic.shared_irqs[i].itargets) doesn't it?

> +    }
>      return 0;
>  }
>  
> @@ -374,6 +380,32 @@ read_as_zero:
>      return 1;
>  }
>  
> +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> +{
> +    unsigned long target;
> +    struct vcpu *v_target;
> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
> +
> +    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
> +    /* 1-N SPI should be delivered as pending to all the vcpus in the
> +     * mask, but here we just return the first vcpu for simplicity and
> +     * because it would be too slow to do otherwise. */
> +    target = find_first_bit((const unsigned long *) &target, 8);

Is this definitely aligned ok?

Also the cast isn't necessary, the type is already unsigned long * and
if find_first_bit takes a const that'll happen automatically.

> @@ -589,12 +625,23 @@ 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_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
>          if ( rank == NULL) goto write_ignore;
> +        /* 8-bit vcpu mask for this domain */
> +        tr = (1 << v->domain->max_vcpus) - 1;
> +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> +        tr &= *r;
> +        /* ignore zero writes */
> +        if ( !tr )
> +            goto write_ignore;

Is this per the spec? Can you provide a reference. If not then I think
writing target==0 is the guest's problem.

> +        if ( dabt.size == 2 &&
> +            !((tr & 0xff) && (tr & (0xff << 8)) &&
> +             (tr & (0xff << 16)) && (tr & (0xff << 24))))
> +            goto write_ignore;

Isn't this just !(tr & 0xffffffff) ? Even then I'm not sure what it is
actually for.

>          vgic_lock_rank(v, rank);
>          if ( dabt.size == 2 )
> -            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = *r;
> +            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = tr;
>          else
>              byte_write(&rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)],
> -                       *r, offset);
> +                       tr, offset);
>          vgic_unlock_rank(v, rank);
>          return 1;
>  
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index bf6fb1e..bd40628 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -227,6 +227,8 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
>                    unsigned int *out_hwirq, unsigned int *out_type);
>  void gic_clear_lrs(struct vcpu *v);
>  
> +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
> +
>  #endif /* __ASSEMBLY__ */
>  #endif
>
Stefano Stabellini June 19, 2014, 6:07 p.m. UTC | #5
On Wed, 18 Jun 2014, Ian Campbell wrote:
> On Wed, 2014-06-11 at 17:27 +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.
> > 
> > itargets can be set to a mask but vgic_get_target_vcpu always returns
> > the lower vcpu in the mask.
> > 
> > Correctly initialize itargets for SPIs.
> > 
> > Ignore bits in GICD_ITARGETSR corresponding to invalid vcpus.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > 
> > Changes in v5:
> > - improve in-code comments;
> > - use vgic_rank_irq;
> > - use bit masks to write-ignore GICD_ITARGETSR;
> > - introduce an version of vgic_get_target_vcpu that doesn't take the
> > rank lock;
> > - keep the rank lock while enabling/disabling irqs;
> > - use find_first_bit instead of find_next_bit.
> > 
> > Changes in v4:
> > - remove assert that could allow a guest to crash Xen;
> > - add itargets validation to vgic_distr_mmio_write;
> > - export vgic_get_target_vcpu.
> > 
> > Changes in v3:
> > - add assert in get_target_vcpu;
> > - rename get_target_vcpu to vgic_get_target_vcpu.
> > 
> > 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       |   71 +++++++++++++++++++++++++++++++++++++--------
> >  xen/include/asm-arm/gic.h |    2 ++
> >  2 files changed, 61 insertions(+), 12 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 757707e..4d655af 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -111,7 +111,13 @@ 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++)
> > +    {
> >          spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> > +        /* By default deliver to CPU0 */
> > +        memset(d->arch.vgic.shared_irqs[i].itargets,
> > +               0x1,
> > +               8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]));
> 
> 8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]) ==
> sizeof(d->arch.vgic.shared_irqs[i].itargets) doesn't it?

Yeah, I'll make the change.


> > +    }
> >      return 0;
> >  }
> >  
> > @@ -374,6 +380,32 @@ read_as_zero:
> >      return 1;
> >  }
> >  
> > +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> > +{
> > +    unsigned long target;
> > +    struct vcpu *v_target;
> > +    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
> > +
> > +    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
> > +    /* 1-N SPI should be delivered as pending to all the vcpus in the
> > +     * mask, but here we just return the first vcpu for simplicity and
> > +     * because it would be too slow to do otherwise. */
> > +    target = find_first_bit((const unsigned long *) &target, 8);
> 
> Is this definitely aligned ok?

I think so: target is unsigned long and find_first_bit expects an
unsigned long. The problem is when target is a uint32_t or an unsigned
int and we cast it to unsigned long on armv8.


> Also the cast isn't necessary, the type is already unsigned long * and
> if find_first_bit takes a const that'll happen automatically.

OK


> > @@ -589,12 +625,23 @@ 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_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
> >          if ( rank == NULL) goto write_ignore;
> > +        /* 8-bit vcpu mask for this domain */
> > +        tr = (1 << v->domain->max_vcpus) - 1;
> > +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> > +        tr &= *r;
> > +        /* ignore zero writes */
> > +        if ( !tr )
> > +            goto write_ignore;
> 
> Is this per the spec? Can you provide a reference. If not then I think
> writing target==0 is the guest's problem.

The reference is 4.3.12 of the GIC architecture specification but
unfortunately it is not clearly written how the gic should behave in
case of 0 writes to GICD_ITARGETSR.

I wrote the patch thinking that a 0 write is invalid and therefore
should be ignored. Now I am not sure anymore. You could effectively
disable an interrupt by writing 0 to GICD_ITARGETSR. Why do you even
need GICD_ICENABLER in that case?

That said, the current code cannot deal with itargets being 0, so this
check is needed for security. I could change the code to be able to deal
with invalid values or 0 values to itargets, then we could remove the
check. I am not sure what is the best course of action.


> > +        if ( dabt.size == 2 &&
> > +            !((tr & 0xff) && (tr & (0xff << 8)) &&
> > +             (tr & (0xff << 16)) && (tr & (0xff << 24))))
> > +            goto write_ignore;
> 
> Isn't this just !(tr & 0xffffffff) ? Even then I'm not sure what it is
> actually for.

tr & 0xffffffff

is non-zero if any of the bytes in tr is non-zero.

((tr & 0xff) && (tr & (0xff << 8)) && (tr & (0xff << 16)) && (tr & (0xff << 24)))

is non-zero if all the bytes in tr are non-zero.


> >          vgic_lock_rank(v, rank);
> >          if ( dabt.size == 2 )
> > -            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = *r;
> > +            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = tr;
> >          else
> >              byte_write(&rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)],
> > -                       *r, offset);
> > +                       tr, offset);
> >          vgic_unlock_rank(v, rank);
> >          return 1;
> >  
> > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> > index bf6fb1e..bd40628 100644
> > --- a/xen/include/asm-arm/gic.h
> > +++ b/xen/include/asm-arm/gic.h
> > @@ -227,6 +227,8 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
> >                    unsigned int *out_hwirq, unsigned int *out_type);
> >  void gic_clear_lrs(struct vcpu *v);
> >  
> > +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
> > +
> >  #endif /* __ASSEMBLY__ */
> >  #endif
> >  
> 
>
Stefano Stabellini June 21, 2014, 4:19 p.m. UTC | #6
On Thu, 19 Jun 2014, Stefano Stabellini wrote:
> On Wed, 18 Jun 2014, Ian Campbell wrote:
> > On Wed, 2014-06-11 at 17:27 +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.
> > > 
> > > itargets can be set to a mask but vgic_get_target_vcpu always returns
> > > the lower vcpu in the mask.
> > > 
> > > Correctly initialize itargets for SPIs.
> > > 
> > > Ignore bits in GICD_ITARGETSR corresponding to invalid vcpus.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > 
> > > ---
> > > 
> > > Changes in v5:
> > > - improve in-code comments;
> > > - use vgic_rank_irq;
> > > - use bit masks to write-ignore GICD_ITARGETSR;
> > > - introduce an version of vgic_get_target_vcpu that doesn't take the
> > > rank lock;
> > > - keep the rank lock while enabling/disabling irqs;
> > > - use find_first_bit instead of find_next_bit.
> > > 
> > > Changes in v4:
> > > - remove assert that could allow a guest to crash Xen;
> > > - add itargets validation to vgic_distr_mmio_write;
> > > - export vgic_get_target_vcpu.
> > > 
> > > Changes in v3:
> > > - add assert in get_target_vcpu;
> > > - rename get_target_vcpu to vgic_get_target_vcpu.
> > > 
> > > 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       |   71 +++++++++++++++++++++++++++++++++++++--------
> > >  xen/include/asm-arm/gic.h |    2 ++
> > >  2 files changed, 61 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > index 757707e..4d655af 100644
> > > --- a/xen/arch/arm/vgic.c
> > > +++ b/xen/arch/arm/vgic.c
> > > @@ -111,7 +111,13 @@ 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++)
> > > +    {
> > >          spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> > > +        /* By default deliver to CPU0 */
> > > +        memset(d->arch.vgic.shared_irqs[i].itargets,
> > > +               0x1,
> > > +               8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]));
> > 
> > 8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]) ==
> > sizeof(d->arch.vgic.shared_irqs[i].itargets) doesn't it?
> 
> Yeah, I'll make the change.
> 
> 
> > > +    }
> > >      return 0;
> > >  }
> > >  
> > > @@ -374,6 +380,32 @@ read_as_zero:
> > >      return 1;
> > >  }
> > >  
> > > +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> > > +{
> > > +    unsigned long target;
> > > +    struct vcpu *v_target;
> > > +    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
> > > +
> > > +    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
> > > +    /* 1-N SPI should be delivered as pending to all the vcpus in the
> > > +     * mask, but here we just return the first vcpu for simplicity and
> > > +     * because it would be too slow to do otherwise. */
> > > +    target = find_first_bit((const unsigned long *) &target, 8);
> > 
> > Is this definitely aligned ok?
> 
> I think so: target is unsigned long and find_first_bit expects an
> unsigned long. The problem is when target is a uint32_t or an unsigned
> int and we cast it to unsigned long on armv8.
> 
> 
> > Also the cast isn't necessary, the type is already unsigned long * and
> > if find_first_bit takes a const that'll happen automatically.
> 
> OK
> 
> 
> > > @@ -589,12 +625,23 @@ 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_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
> > >          if ( rank == NULL) goto write_ignore;
> > > +        /* 8-bit vcpu mask for this domain */
> > > +        tr = (1 << v->domain->max_vcpus) - 1;
> > > +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> > > +        tr &= *r;
> > > +        /* ignore zero writes */
> > > +        if ( !tr )
> > > +            goto write_ignore;
> > 
> > Is this per the spec? Can you provide a reference. If not then I think
> > writing target==0 is the guest's problem.
> 
> The reference is 4.3.12 of the GIC architecture specification but
> unfortunately it is not clearly written how the gic should behave in
> case of 0 writes to GICD_ITARGETSR.
> 
> I wrote the patch thinking that a 0 write is invalid and therefore
> should be ignored. Now I am not sure anymore. You could effectively
> disable an interrupt by writing 0 to GICD_ITARGETSR. Why do you even
> need GICD_ICENABLER in that case?
> 
> That said, the current code cannot deal with itargets being 0, so this
> check is needed for security. I could change the code to be able to deal
> with invalid values or 0 values to itargets, then we could remove the
> check. I am not sure what is the best course of action.


I confirm that a real GICv2 (the one on Midway) ignores 0 writes. I
strongly suggest we do the same.
diff mbox

Patch

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 757707e..4d655af 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -111,7 +111,13 @@  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++)
+    {
         spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
+        /* By default deliver to CPU0 */
+        memset(d->arch.vgic.shared_irqs[i].itargets,
+               0x1,
+               8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]));
+    }
     return 0;
 }
 
@@ -374,6 +380,32 @@  read_as_zero:
     return 1;
 }
 
+static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
+{
+    unsigned long target;
+    struct vcpu *v_target;
+    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+
+    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
+    /* 1-N SPI should be delivered as pending to all the vcpus in the
+     * mask, but here we just return the first vcpu for simplicity and
+     * because it would be too slow to do otherwise. */
+    target = find_first_bit((const unsigned long *) &target, 8);
+    v_target = v->domain->vcpu[target];
+    return v_target;
+}
+
+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);
+
+    vgic_lock_rank(v, rank);
+    v_target = _vgic_get_target_vcpu(v, irq);
+    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;
@@ -381,12 +413,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 = _vgic_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);
@@ -404,24 +438,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 = _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->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 )
         {
@@ -536,8 +572,8 @@  static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         vgic_lock_rank(v, rank);
         tr = rank->ienable;
         rank->ienable |= *r;
-        vgic_unlock_rank(v, rank);
         vgic_enable_irqs(v, (*r) & (~tr), gicd_reg - GICD_ISENABLER);
+        vgic_unlock_rank(v, rank);
         return 1;
 
     case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -547,8 +583,8 @@  static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         vgic_lock_rank(v, rank);
         tr = rank->ienable;
         rank->ienable &= ~*r;
-        vgic_unlock_rank(v, rank);
         vgic_disable_irqs(v, (*r) & tr, gicd_reg - GICD_ICENABLER);
+        vgic_unlock_rank(v, rank);
         return 1;
 
     case GICD_ISPENDR ... GICD_ISPENDRN:
@@ -589,12 +625,23 @@  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_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
         if ( rank == NULL) goto write_ignore;
+        /* 8-bit vcpu mask for this domain */
+        tr = (1 << v->domain->max_vcpus) - 1;
+        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
+        tr &= *r;
+        /* ignore zero writes */
+        if ( !tr )
+            goto write_ignore;
+        if ( dabt.size == 2 &&
+            !((tr & 0xff) && (tr & (0xff << 8)) &&
+             (tr & (0xff << 16)) && (tr & (0xff << 24))))
+            goto write_ignore;
         vgic_lock_rank(v, rank);
         if ( dabt.size == 2 )
-            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = *r;
+            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = tr;
         else
             byte_write(&rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)],
-                       *r, offset);
+                       tr, offset);
         vgic_unlock_rank(v, rank);
         return 1;
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index bf6fb1e..bd40628 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -227,6 +227,8 @@  int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
                   unsigned int *out_hwirq, unsigned int *out_type);
 void gic_clear_lrs(struct vcpu *v);
 
+struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
+
 #endif /* __ASSEMBLY__ */
 #endif