Re: [Xen-devel] [PATCH v4 04/12] xen/arm: support for guest SGI

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-04-26 at 16:28 +0100, Stefano Stabellini wrote:
> Trap writes to GICD_SGIR, parse the requests, inject SGIs into the right
> guest vcpu.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> Changes in v4:
> - move the code to a separate function;
> - use gdprintk for debugging output;
> - make use of PRIregister;
> - replace the cpumask with a bitmask;
> - move the virtual_irq check outside the loop;
> - ignore non-existent target vcpus.
> 
> Changes in v3:
> - make use of cpumask_from_bitmap.
> ---
>  xen/arch/arm/vgic.c       |   63 
> ++++++++++++++++++++++++++++++++++++++++++---
>  xen/include/asm-arm/gic.h |    9 ++++--
>  2 files changed, 65 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index b30da78..8d87609 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -17,6 +17,7 @@
>   * GNU General Public License for more details.
>   */
>  
> +#include <xen/bitops.h>
>  #include <xen/config.h>
>  #include <xen/lib.h>
>  #include <xen/init.h>
> @@ -368,6 +369,61 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, 
> int n)
>      }
>  }
>  
> +static int vgic_to_sgi(struct vcpu *v, register_t sgir)
> +{
> +    struct domain *d = v->domain;
> +    int virtual_irq;
> +    int filter;
> +    int vcpuid;
> +    struct vcpu *vt;
> +    int i;
> +    unsigned long vcpu_mask = 0;
> +
> +    ASSERT(d->max_vcpus < 8*sizeof(vcpu_mask));
> +
> +    filter = (sgir & GICD_SGI_TARGET_LIST_MASK);
> +    virtual_irq = (sgir & GICD_SGI_INTID_MASK);
> +
> +    switch ( filter )
> +    {
> +        case GICD_SGI_TARGET_LIST:
> +            vcpu_mask = (sgir & GICD_SGI_TARGET_MASK) >> 
> GICD_SGI_TARGET_SHIFT;
> +            break;
> +        case GICD_SGI_TARGET_OTHERS:
> +            for ( i = 0; i < d->max_vcpus; i++ )
> +            {
> +                if ( i != current->vcpu_id && d->vcpu[i] != NULL )
> +                    set_bit(i, &vcpu_mask);
> +            }
> +        case GICD_SGI_TARGET_SELF:
> +            set_bit(current->vcpu_id, &vcpu_mask);
> +            break;
> +        default:
> +            gdprintk(XENLOG_WARNING, "vGICD: unhandled GICD_SGIR write 
> %"PRIregister" with wrong TargetListFilter field\n",
> +                     sgir);
> +            return 0;
> +    }
> +    if ( virtual_irq >= 16 )
> +    {
> +        gdprintk(XENLOG_WARNING, "vGICD: try to send SGI %d that is >= 16\n",
> +                 virtual_irq);
> +        return 0;
> +    }

It occurs to me that the only way this can happen is if we have decoded
the instruction incorrectly, because it's only 4 bits.

It can probably be an ASSERT/BUG_ON.

> +
> +
> +    for_each_set_bit( vcpuid, &vcpu_mask, d->max_vcpus )
> +    {
> +        if ( vcpuid >= d->max_vcpus || (vt = d->vcpu[vcpuid]) == NULL )

Is d->vcpu[vcpuid] == NULL sufficient here? A vcpu which exists but has
not been PSCI'd up will pass this -- what do we do to them? Hopefully we
don't wake them up? Do we not want to check something like _VPF_down
too?

The same likely goes for the TARGET_OTHERS code too which builds the
mask, I'm not sure how this isn't racy against a cpu going down though.

(personally I dislike assignments inside if()s, but they are not
uncommon in the hypervisor code)

> +        {
> +            gdprintk(XENLOG_WARNING, "vGICD: GICD_SGIR write 
> r=%"PRIregister" vcpu_mask=%lx, wrong CPUTargetList\n",
> +                     sgir, vcpu_mask);
> +            continue;
> +        }
> +        vgic_vcpu_inject_irq(vt, virtual_irq, 1);
> +    }
> +    return 1;
> +}
> +
>  static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>  {
>      struct hsr_dabt dabt = info->dabt;
> @@ -498,10 +554,9 @@ static int vgic_distr_mmio_write(struct vcpu *v, 
> mmio_info_t *info)
>          goto write_ignore;
>  
>      case GICD_SGIR:
> -        if ( dabt.size != 2 ) goto bad_width;
> -        printk("vGICD: unhandled write %#"PRIregister" to ICFGR%d\n",
> -               *r, gicd_reg - GICD_ICFGR);
> -        return 0;
> +        if ( dabt.size != 2 )
> +            goto bad_width;
> +        return vgic_to_sgi(v, *r);
>  
>      case GICD_CPENDSGIR ... GICD_CPENDSGIRN:
>          if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 92711d5..061ebf4 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -51,12 +51,15 @@
>  #define GICD_SPENDSGIRN (0xF2C/4)
>  #define GICD_ICPIDR2    (0xFE8/4)
>  
> -#define GICD_SGI_TARGET_LIST   (0UL<<24)
> -#define GICD_SGI_TARGET_OTHERS (1UL<<24)
> -#define GICD_SGI_TARGET_SELF   (2UL<<24)
> +#define GICD_SGI_TARGET_LIST_SHIFT   (24)
> +#define GICD_SGI_TARGET_LIST_MASK    (0x3UL << GICD_SGI_TARGET_LIST_SHIFT)

Can you line the others up with these please, you are touching most of
them anyway.

> +#define GICD_SGI_TARGET_LIST   (0UL<<GICD_SGI_TARGET_LIST_SHIFT)
> +#define GICD_SGI_TARGET_OTHERS (1UL<<GICD_SGI_TARGET_LIST_SHIFT)
> +#define GICD_SGI_TARGET_SELF   (2UL<<GICD_SGI_TARGET_LIST_SHIFT)
>  #define GICD_SGI_TARGET_SHIFT  (16)
>  #define GICD_SGI_TARGET_MASK   (0xFFUL<<GICD_SGI_TARGET_SHIFT)
>  #define GICD_SGI_GROUP1        (1UL<<15)
> +#define GICD_SGI_INTID_MASK    (0xFUL)
>  
>  #define GICC_CTLR       (0x0000/4)
>  #define GICC_PMR        (0x0004/4)



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel