Re: [Xen-devel] [RFC 01/22] xen/arm: do_trap_instr_abort_guest: Move the IPA computation out of the switch

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 16/08/16 17:20, Julien Grall wrote:
> On 16/08/2016 01:21, Stefano Stabellini wrote:
> > On Thu, 28 Jul 2016, Julien Grall wrote:
> > > A follow-up patch will add more case to the switch that will require the
> > > IPA. So move the computation out of the switch.
> > >
> > > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > > ---
> > >  xen/arch/arm/traps.c | 36 ++++++++++++++++++------------------
> > >  1 file changed, 18 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > index 683bcb2..46e0663 100644
> > > --- a/xen/arch/arm/traps.c
> > > +++ b/xen/arch/arm/traps.c
> > > @@ -2403,35 +2403,35 @@ static void do_trap_instr_abort_guest(struct
> > > cpu_user_regs *regs,
> > >      int rc;
> > >      register_t gva = READ_SYSREG(FAR_EL2);
> > >      uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
> > > +    paddr_t gpa;
> > > +
> > > +    if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
> > > +        gpa = get_faulting_ipa(gva);
> > > +    else
> > > +    {
> > > +        /*
> > > +         * Flush the TLB to make sure the DTLB is clear before
> > > +         * doing GVA->IPA translation. If we got here because of
> > > +         * an entry only present in the ITLB, this translation may
> > > +         * still be inaccurate.
> > > +         */
> > > +        flush_tlb_local();
> > > +
> > > +        rc = gva_to_ipa(gva, &gpa, GV2M_READ);
> > > +        if ( rc == -EFAULT )
> > > +            return; /* Try again */
> > The issue with this is that now for any cases that don't require a gpa
> > if gva_to_ipa fails we wrongly return -EFAULT.
> Well, stage-1 fault is prioritized over stage-2 fault (see B3.12.3 in
> ARM DDI 0406C.b), so gva_to_ipa should never fail unless someone is
> playing with the stage-1 page table at the same time or because of an
> erratum (see 834220). In both case, we should replay the instruction to
> let the processor injecting the correct fault.
>
> FWIW, this is already what we do for the data abort handler.
>
> > I suggest having two switches or falling through from the first case to
> > the second.
> I am not sure to understand your suggestion. Could you detail it?
Do you have any thoughts here or are you fine with the current approach?

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel