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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

I forgot to mention one thing about the placement of do_fixup_vgic_errata.

On 16/01/18 15:42, mjaggi@xxxxxxxxxxxxxxxxxx wrote:
> 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);
> +    if ( !ret )
> +    {
> +        advance_pc(regs, hsr);
> +        return;
I am fully aware that I suggested this solution and still support that 
the vGIC errata should be fully separated. After all, it deals with 
hardware bug and the errata will just update the LRs as the hardware 
would do.
enter_hypervisor_head() will sync the LRs state to the internal vGIC 
state. leave_hypervisor_head() will process pending softirq and 
write/update the LRs based on the internal vGIC state.
As you rightfully did, the do_fixup_vgic_errata should be called before 
syncing the LRs. However, even if you return early here, you will still 
execute leave_hypervisor_tail(). This mean that pending softirqs will be 
processed and potentially the vCPU rescheduled. Because the LRs were not 
synced (enter_hypervisor_head()) was not called, then the vGIC state 
will not out-of-date and would lead to all sort of potential issues.
As the vGIC errata implies trapping the register such as IAR1 (reading 
interrupt), we want to get a fastpath for it (e.g not trying to execute 
softirq...). So I think we should bypass leave_hypervisor_tail(). I am 
not entirely sure how to do it nicely thought.
Cheers,

> +    }
> +#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__ */
>   /*
--
Julien Grall

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