Re: [Xen-devel] [PATCH 05/12] Placeholder for handling Group1 register traps

[Julien Grall <julien.grall@xxxxxxx>]

Hi Manish,

On 12/03/18 12:42, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> From: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
>
> Since this is a SoC errata and trapping of certain group1 registers
> should not affect the normal flow. A new file vsysreg_errata.c is added.
>
> Function vgic_v3_handle_cpuif_access is called from do_trap_guest_sync
> if ARM64_WORKAROUND_CAVIUM_30115 capability is found.
>
> A flag skip_hyp_tail is introduced in struct cpu_info. This flag specifies
> that leave_hypervisor_tail not to be called when handling group1 traps
> under this errata.
Please give some rationale in the commit message why 
leave_hypervisor_tail is skipped on the errata.
> Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> ---
>   xen/arch/arm/arm64/Makefile         |  1 +
>   xen/arch/arm/arm64/vsysreg_errata.c | 28 ++++++++++++++++++++++++++++
The name of the file does not make sense, the errata is not about 
sysreg. It is about vGIC. Please rename it to vgic-v3-sr.c.
>   xen/arch/arm/traps.c                | 20 ++++++++++++++++++++
>   xen/include/asm-arm/arm64/traps.h   |  3 ++-
>   xen/include/asm-arm/current.h       |  1 +
>   5 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile
> index 718fe44455..19440c3d8c 100644
> --- a/xen/arch/arm/arm64/Makefile
> +++ b/xen/arch/arm/arm64/Makefile
> @@ -11,3 +11,4 @@ obj-y += smpboot.o
>   obj-y += traps.o
>   obj-y += vfp.o
>   obj-y += vsysreg.o
> +obj-$(CONFIG_CAVIUM_ERRATUM_30115) += vsysreg_errata.o
> diff --git a/xen/arch/arm/arm64/vsysreg_errata.c 
> b/xen/arch/arm/arm64/vsysreg_errata.c
> new file mode 100644
> index 0000000000..6af162bdf7
> --- /dev/null
> +++ b/xen/arch/arm/arm64/vsysreg_errata.c
> @@ -0,0 +1,28 @@
Missing copyright header.

> +#include <asm/current.h>
> +#include <asm/regs.h>
> +#include <asm/traps.h>
> +#include <asm/system.h>
> +
> +bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs, const union hsr 
> hsr)
> +{
> +    bool ret = 0;
You should use false/true when using bool. No plain integer.

> +
> +    local_irq_disable();
Please add a comment explain why IRQs are disabled.

> +    if ( hsr.ec != HSR_EC_SYSREG )
> +    {
> +        ret = 1;
> +        goto end;
> +    }
> +
> +    switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
> +    {
> +    default:
> +        ret = 1;
> +        break;
> +    }
> +end:
> +    local_irq_enable();
> +
> +    return ret;
> +}
> +
Missing emacs magic.

> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index f6f6de3691..9d08cd6ad3 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -40,6 +40,7 @@
>   #include <asm/acpi.h>
>   #include <asm/cpuerrata.h>
>   #include <asm/cpufeature.h>
> +#include <asm/cpuerrata.h>
>   #include <asm/debugger.h>
>   #include <asm/event.h>
>   #include <asm/flushtlb.h>
> @@ -2103,6 +2104,21 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>   {
>       const union hsr hsr = { .bits = regs->hsr };
>   
> +#ifdef CONFIG_CAVIUM_ERRATUM_30115
I am not a big fan of #ifdef in the code. I think it would be better to 
stub the vgic_v3_handle_cpuif_access.
> +    if ( cpus_have_cap(ARM64_WORKAROUND_CAVIUM_30115) )
cpus_have_cap is expensive to use in hot path. This is because the 
function is going to check is the bits is set on every exit. You want to 
use check_workaround_* for that purpose as this will be replaced by an 
alternative.
> +    {
> +        int ret;
vgic_v3_handle_cpuif_access is returning a bool.

> +        get_cpu_info()->skip_hyp_tail = 0;
Same remark as above about bool and integer. But I think this one is not 
necessary. skip_hyp_tail is going to be false by default. So if you 
reset to false in the return path when it is true, you avoid
> +        ret  = vgic_v3_handle_cpuif_access(regs, hsr);
> +        if ( !ret )
> +        {
> +            advance_pc(regs, hsr);
> +            get_cpu_info()->skip_hyp_tail = 1;
true.

> +            return;
> +        }
> +    }
> +#endif
> +
>       enter_hypervisor_head(regs);
>   
>       switch (hsr.ec) {
> @@ -2295,6 +2311,10 @@ void do_trap_fiq(struct cpu_user_regs *regs)
>   
>   void leave_hypervisor_tail(void)
>   {
> +#ifdef CONFIG_CAVIUM_ERRATUM_30115
I don't think the #ifdef is necessary here. Supporting to skip the 
hypervisor tail is a nice feature to have.
> +    if ( get_cpu_info()->skip_hyp_tail )
You want this to be an unlikely(...).

> +        return;
> +#endif
>       while (1)
>       {
>           local_irq_disable();
> diff --git a/xen/include/asm-arm/arm64/traps.h 
> b/xen/include/asm-arm/arm64/traps.h
> index 2379b578cb..a5ae93ec11 100644
> --- a/xen/include/asm-arm/arm64/traps.h
> +++ b/xen/include/asm-arm/arm64/traps.h
> @@ -2,7 +2,8 @@
>   #define __ASM_ARM64_TRAPS__
>   
>   void inject_undef64_exception(struct cpu_user_regs *regs, int instr_len);
> -
Why removing the newline?

> +bool vgic_v3_handle_cpuif_access(struct cpu_user_regs *regs,
> +                                const union hsr hsr);
>   void do_sysreg(struct cpu_user_regs *regs,
>                  const union hsr hsr);
>   
> diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h
> index 7a0971fdea..dacf3adc85 100644
> --- a/xen/include/asm-arm/current.h
> +++ b/xen/include/asm-arm/current.h
> @@ -22,6 +22,7 @@ struct cpu_info {
>       struct cpu_user_regs guest_cpu_user_regs;
>       unsigned long elr;
>       unsigned int pad;
> +    bool skip_hyp_tail;
You should just reuse some bits of the padding (i.e 'pad' field) here.

bool skip_hyp_tail:1;
unsigned int pad:31;

Also, some documentation of the code would be highly appreciated.

>   };
>   
>   static inline struct cpu_info *get_cpu_info(void)
Cheers,

--
Julien Grall

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