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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

On 01/25/2018 11:37 PM, Julien Grall wrote:
> 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.
How about adding a check for group1_trap enable in leave_hypervisor_tail().

void leave_hypervisor_tail(void)
{
+if (group1_trap)
+   return;
    while (1)


> 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__ */
> >   /*

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