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

[Julien Grall <julien.grall@linaro.org>]

Hello Vijaya,

On 15/04/14 12:17, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>
> In ARMv8, write to ICC_SGI1R_EL1 register raises trap to EL2.
> Handle the trap and inject SGI to vcpu.
>
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>   xen/arch/arm/traps.c              |    9 +++++
>   xen/arch/arm/vgic-v3.c            |   79 +++++++++++++++++++++++++++++++++++++
>   xen/include/asm-arm/gic_v3_defs.h |   14 +++++++
>   xen/include/asm-arm/sysregs.h     |    2 +-
>   xen/include/asm-arm/vgic.h        |    1 +
>   5 files changed, 104 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index e0184b1..dc1f202 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -40,6 +40,7 @@
>   #include "decode.h"
>   #include "vtimer.h"
>   #include <asm/gic.h>
> +#include <asm/vgic.h>
>
>   /* The base of the stack must always be double-word aligned, which means
>    * that both the kernel half of struct cpu_user_regs (which is pushed in
> @@ -1406,6 +1407,14 @@ static void do_sysreg(struct cpu_user_regs *regs,
>               domain_crash_synchronous();
>           }
>           break;
> +    case HSR_SYSREG_ICC_SGI1R_EL1:

Any reason to not trap ICC_SGI0R_EL1 and ICC_ASGI1R_EL1?

> +        if ( !vgic_emulate(regs, hsr) )
> +        {
> +            dprintk(XENLOG_ERR,
> +                    "failed emulation of 64-bit vgic sysreg access\n");
> +            domain_crash_synchronous();

So, you crash the domain if the SGI is not handled??? The GICv3 spec 
requests a specific behavior.

> +        }
> +        break;
>       default:
>           printk("%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
>                  sysreg.read ? "mrs" : "msr",

> +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);

Missing newline.

> +    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)

You should extend the gic ops rather than by pass the GIC common code.

> +{
> +    switch ( hsr.ec )
> +    {
> +    case HSR_EC_SYSREG:
> +        return vgic_emulate_sysreg(regs, hsr);
> +    default:
> +        return 0;
> +    }
> +}
> +

[..]

>   #endif /* __ASM_ARM_GIC_V3_DEFS_H__ */
>
>   /*
> 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)

Newline here please.

>   #endif
>
>   #endif

Regards,

-- 
Julien Grall