Re: [PATCH 10/10] Enable Trapping of Group1 registers which is controlled by command line

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

Hi Manish,

On 16/01/18 15:43, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <manish.jaggi@cavium.com>
> 
> In order to be able to trap Group-1 GICv3 system registers, we need to
> set ICH_HCR_EL2.TALL1 before entering the guest. This is controlled by
> the command line parameter group1_trap.

I was expecting a patch to enable group1_trap by default on affected 
platform.

> 
> Singed-off-by: Manish Jaggi <manish.jaggi@cavium.com>
> ---
>   xen/arch/arm/gic-v3.c     | 11 ++++++++++-
>   xen/include/asm-arm/gic.h |  1 +
>   2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 5dba8bc932..f22877c468 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -833,9 +833,12 @@ static void gicv3_cpu_disable(void)
>       isb();
>   }
>   
> +static unsigned int group1_trap = 0;
> +integer_param("group1_trap", group1_trap);

New parameter should be describe in docs/misc/xen-command-line.markdown.

Also, you likely want to use a boolean_param here.

> +
>   static void gicv3_hyp_init(void)
>   {
> -    uint32_t vtr;
> +    uint32_t vtr, reg32;
>   
>       vtr = READ_SYSREG32(ICH_VTR_EL2);
>       gicv3_info.nr_lrs  = (vtr & GICH_VTR_NRLRGS) + 1;
> @@ -847,6 +850,12 @@ static void gicv3_hyp_init(void)
>   
>       WRITE_SYSREG32(GICH_VMCR_EOI | GICH_VMCR_VENG1, ICH_VMCR_EL2);
>       WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
> +
> +    reg32 = READ_SYSREG32(ICH_HCR_EL2);

There are no point to read ICH_HCR_EL2. You know the value (see the line 
above).

So this code could simplified as:

reg32 = GICH_HCR_EN;
reg32 |= (group1_trap) ? GICH_HCR_TALL1 : 0;

WRITE_SYSREG32(reg32, ICH_HCR_EL2);

> +    if ( group1_trap )
> +        reg32 |= GICH_HCR_TALL1;
> +
> +    WRITE_SYSREG32(reg32, ICH_HCR_EL2);
>   }
>   
>   /* Set up the per-CPU parts of the GIC for a secondary CPU */
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index d3d7bda50d..e4c77fefd6 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -117,6 +117,7 @@
>   #define GICH_HCR_VGRP0DIE (1 << 5)
>   #define GICH_HCR_VGRP1EIE (1 << 6)
>   #define GICH_HCR_VGRP1DIE (1 << 7)
> +#define GICH_HCR_TALL1    (1 << 12)
>   
>   #define GICH_MISR_EOI     (1 << 0)
>   #define GICH_MISR_U       (1 << 1) >

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel