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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 16 Aug 2016, Julien Grall wrote:
> Hi Stefano,
> 
> 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?

I was merely suggesting to add another switch like:

  +   switch ( fsc )
  +   {
  +   case FSC_FLT_PERM:
  +   case BLA:
  +       find gpa;
  +   default:
  +   }

     switch ( fsc )
     {

but given that we are already relying on the gva_to_ipa translation in
the data abort handler, your patch is fine too.

Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

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