Re: [Xen-devel] [PATCH v3 16/16] xen/arm: add SGI handling for GICv3

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-04-15 at 16:47 +0530, vijay.kilari@xxxxxxxxx wrote:

> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index e3d773a..2bef977 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -774,6 +774,85 @@ write_ignore:
>      return 1;
>  }
>  
> +static int vgic_to_sgi(struct vcpu *v, register_t sgir)
> +{
> +    struct domain *d = v->domain;
> +    int virq;
> +    int irqmode;
> +    int vcpuid;
> +    int i;
> +    unsigned long vcpu_mask = 0;
> +
> +    ASSERT(d->max_vcpus < 8*sizeof(vcpu_mask));

We get away with an unsigned long and ASSERT on v2 because it is limited
to 8 cpus, but gic v3 supports many more. You didn't change
MAX_VIRT_CPUS in this series, did you? In which case I suppose this is
safe enough for now, but will need to be cleverer at some point in the
future.

> +    irqmode = (sgir >> ICH_SGI_IRQMODE_SHIFT) & ICH_SGI_IRQMODE_MASK;
> +    virq = (sgir >> ICH_SGI_IRQ_SHIFT ) & ICH_SGI_IRQ_MASK;
> +
> +    ASSERT( virq < 16 );

The rest of this function is identical to vgic_to_sgi, I think. Please
can you make both call a common function after decoding the register.
        vgic_cpu_inject_sgi(..., irqmode, virq);

> +static int vgic_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
> +{
> +    struct vcpu *v = current;
> +    struct hsr_sysreg sysreg = hsr.sysreg;
> +    register_t *r = select_user_reg(regs, sysreg.reg);
> +    switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
> +    {
> +    case HSR_SYSREG_ICC_SGI1R_EL1:
> +        /* WO */
> +        if ( !sysreg.read )
> +            return vgic_to_sgi(v, *r);
> +        else
> +        {
> +            gdprintk(XENLOG_WARNING, "Reading SGI1R_EL1 - WO register\n");
> +            return 0;
> +        }
> +    default:
> +        return 0;
> +    }
> +}
> +
> +int vgic_emulate(struct cpu_user_regs *regs, union hsr hsr)
> +{
> +    switch ( hsr.ec )
> +    {
> +    case HSR_EC_SYSREG:

This is only called from do_sysreg, which is already in the
HSR_EC_SYSREG branch, so this whole function is redundant, unless you
expect there to be a v7 version of the sysreg gic interface?

I think you can move the body of vgic_emulate_sysreg into vgic_emulate
and simply ASSERT(hsr.ec == HSR_EC_SYSREG).

> diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
> index 0cee0e9..18e5a45 100644
> --- a/xen/include/asm-arm/sysregs.h
> +++ b/xen/include/asm-arm/sysregs.h
> @@ -56,7 +56,7 @@
>  #define HSR_SYSREG_CNTP_CTL_EL0   HSR_SYSREG(3,3,c14,c2,1)
>  #define HSR_SYSREG_CNTP_TVAL_EL0  HSR_SYSREG(3,3,c14,c2,0)
>  
> -
> +#define HSR_SYSREG_ICC_SGI1R_EL1  HSR_SYSREG(3,0,c12,c11,5)

Please sort into the correct order (which is by the order of the
arguments)

Ian.


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