Re: [Xen-devel] [RFC PATCH 02/10] arm64: Add hook to handle guest GICv3 sysreg accesses

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Manish,

On 16/01/18 15:42, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> From: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
>
> In order to start handling guest access to GICv3 system registers,
> let's add a hook that will get called when we trap a system register
> access.
> This handling code is kept independent of other traps.
> Set CONFIG_VGIC_ERRATA to enable this code.
The commit message should explain the rationale behind calling 
do_fixup_vgic_errata from do_trap_guest_sync.

> Signed-off-by: Manish Jaggi <manish.jaggi@xxxxxxxxxx>
> ---
>   xen/arch/arm/arm64/vsysreg.c      | 17 +++++++++++++++++
>   xen/arch/arm/traps.c              | 11 +++++++++++
>   xen/include/asm-arm/arm64/traps.h |  2 ++
>   3 files changed, 30 insertions(+)
>
> diff --git a/xen/arch/arm/arm64/vsysreg.c b/xen/arch/arm/arm64/vsysreg.c
> index c57ac12503..a5c17e460f 100644
> --- a/xen/arch/arm/arm64/vsysreg.c
> +++ b/xen/arch/arm/arm64/vsysreg.c
> @@ -219,6 +219,23 @@ void do_sysreg(struct cpu_user_regs *regs,
>       regs->pc += 4;
>   }
>   
> +#ifdef CONFIG_VGIC_ERRATA
> +int do_fixup_vgic_errata(struct cpu_user_regs *regs, const union hsr hsr)
Can we have a separate file to emulate vgic system registers? We might 
want to re-use some of the code for 32-bit guests in the future. Also as 
far as I can tell, this will only cover GICv3 system registers. To be 
more specific it will only be cpu interface register.
So I think you should name vgic_v3_handle_cpuif_access(...).

> +{
> +    int ret = 0;
You are returning either -1 or 0. Can we please use bool in that case?

> +
> +    local_irq_disable();
> +    switch ( hsr.bits & HSR_SYSREG_REGS_MASK )
You will enter in do_trap_guest_sync for various reasons. So you should 
check that exception class correspond to a trapped of system 
instructions (e.g HSR_EC_SYSREG). Otherwise you will emulate sysreg on 
other encoding by mistake.
> +    {
> +    default:
> +        ret = -1;
> +        break;
> +    }
> +    local_irq_enable();
> +
> +    return ret;
> +}
> +#endif
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index f6f6de3691..d4f0581d33 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2103,6 +2103,17 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>   {
>       const union hsr hsr = { .bits = regs->hsr };
>   
> +#ifdef CONFIG_VGIC_ERRATA
> +    int ret;
> +
> +    ret  = do_fixup_vgic_errata(regs,hsr);
Most likely distros will have the errata built in Xen because they want 
the hypervisor to run on many platforms. And we don't want to affect the 
 on platform that are not affected by the errata.
So we want to skip the call. Have a look at CHECK_WORKAROUND_HELPER.

> +    if ( !ret )
> +    {
> +        advance_pc(regs, hsr);
> +        return;
> +    }
> +#endif
> +
>       enter_hypervisor_head(regs);
>   
>       switch (hsr.ec) {
> diff --git a/xen/include/asm-arm/arm64/traps.h 
> b/xen/include/asm-arm/arm64/traps.h
> index 2379b578cb..7378a1b022 100644
> --- a/xen/include/asm-arm/arm64/traps.h
> +++ b/xen/include/asm-arm/arm64/traps.h
> @@ -5,6 +5,8 @@ void inject_undef64_exception(struct cpu_user_regs *regs, int 
> instr_len);
>   
>   void do_sysreg(struct cpu_user_regs *regs,
>                  const union hsr hsr);
> +int do_fixup_vgic_errata(struct cpu_user_regs *regs,
> +               const union hsr hsr);
>   
>   #endif /* __ASM_ARM64_TRAPS__ */
>   /*
Cheers,

--
Julien Grall

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