Re: [Xen-devel] [PATCH 04/12] Enable Group1 Traps by default for Cavium ThunderX1

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

title: "arm64: ...".

On 12/03/18 12:42, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> From: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
>
> Enable trapping for Group1 register access when
> CONFIG_CAVIUM_ERRATUM_30115 is enabled.
This is really odd to enable group1 trapping before the sysreg are 
actually emulated. This will be impossible to bisect that series on your 
platform. Please re-order the series to first add sysreg emulation and 
then hook it.
> Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> ---
>   xen/arch/arm/gic-v3.c     | 16 ++++++++++++++--
>   xen/include/asm-arm/gic.h |  1 +
>   2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> index 473e26111f..53a772a313 100644
> --- a/xen/arch/arm/gic-v3.c
> +++ b/xen/arch/arm/gic-v3.c
> @@ -44,6 +44,7 @@
>   #include <asm/gic_v3_defs.h>
>   #include <asm/gic_v3_its.h>
>   #include <asm/cpufeature.h>
> +#include <asm/cpuerrata.h>
Please order them alphabetically.

>   #include <asm/acpi.h>
>   
>   /* Global state */
> @@ -825,7 +826,7 @@ static void gicv3_cpu_disable(void)
>   
>   static void gicv3_hyp_init(void)
>   {
> -    uint32_t vtr;
> +    uint32_t vtr, reg32 = GICH_HCR_EN;
The name reg32 is not obvious. Please rename it to hcr.

>   
>       vtr = READ_SYSREG32(ICH_VTR_EL2);
>       gicv3_info.nr_lrs  = (vtr & GICH_VTR_NRLRGS) + 1;
> @@ -836,7 +837,18 @@ static void gicv3_hyp_init(void)
>           panic("GICv3: Invalid number of priority bits\n");
>   
>       WRITE_SYSREG32(GICH_VMCR_EOI | GICH_VMCR_VENG1, ICH_VMCR_EL2);
> -    WRITE_SYSREG32(GICH_HCR_EN, ICH_HCR_EL2);
> +
> +#ifdef CONFIG_CAVIUM_ERRATUM_30115
I would rather avoid to spread the #ifdef everywhere. In that particular 
case, it is not necessary.
> +    if ( cpus_have_cap(ARM64_WORKAROUND_CAVIUM_30115) )
> +    {
> +        reg32 |= GICH_HCR_TALL1;
> +        printk("%s: 30115 Workaround enabled \r\n", __func__);
The cpuerrata framework will already print a message when the errata is 
enabled. So no need for this message.
> +    }
> +    else
> +        printk("%s: 30115 Workaround not enabled \r\n", __func__);
> +#endif
> +
> +     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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel