Re: [PATCH v5 3/6] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Ian Campbell <Ian.Campbell@citrix.com>]

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
>