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

[Manish Jaggi <mjaggi@xxxxxxxxxxxxxxxxxx>]

On 02/01/2018 04:24 PM, Julien Grall wrote:
> Hi Manish,
>
> On 01/02/18 08:51, Manish Jaggi wrote:
> > 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;
> I guess you mean the variable you introduced in patch #10. In that 
> case, this would be totally wrong. You only want to skip 
> leave_hypervisor_tail() when you are handling a ICV_* System registers.
> What you want is adding a bool in the structure cpu_info to tell 
> whether leave_hypervisor_tail() should be skipped.
Why would it be wrong? I don't follow our point.
Also adding a flag in cpu_info would be overkill, use of a global 
variable is simpler.
I am planning to remove group1_trap as a command line, and use it as

static void gicv3_hyp_init(void)
{
...
#ifdef CONFIG_CAVIUM_ERRATUM_30115
    if (cpus_have_const_cap(ARM64_WORKAROUND_CAVIUM_30115))
        group1_trap = 1;
#endif
...
}

> Cheers,

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