Re: [Xen-devel] [RFC 06/22] xen/arm: traps: Check the P2M before injecting a data/instruction abort

[Julien Grall <julien.grall@xxxxxxx>]

Hi Stefano,

On 23/08/16 02:05, Stefano Stabellini wrote:
> On Thu, 28 Jul 2016, Julien Grall wrote:
> > A data/instruction abort may have occurred if another CPU was playing
> > with the stage-2 page table when following the break-before-make
> > sequence (see D4.7.1 in ARM DDI 0487A.j). Rather than injecting directly
> > the fault to the guest, we need to check whether the mapping exists. If
> > it exists, return to the guest to replay the instruction.
> >
> > Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
> > ---
> >  xen/arch/arm/traps.c | 40 ++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 38 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index b46284c..da56cc0 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -2404,6 +2404,7 @@ static void do_trap_instr_abort_guest(struct 
> > cpu_user_regs *regs,
> >      register_t gva = READ_SYSREG(FAR_EL2);
> >      uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
> >      paddr_t gpa;
> > +    mfn_t mfn;
> >
> >      if ( hpfar_is_valid(hsr.iabt.s1ptw, fsc) )
> >          gpa = get_faulting_ipa(gva);
> > @@ -2417,6 +2418,11 @@ static void do_trap_instr_abort_guest(struct 
> > cpu_user_regs *regs,
> >           */
> >          flush_tlb_local();
> >
> > +        /*
> > +         * We may not be able to translate because someone is
> > +         * playing with the Stage-2 page table of the domain.
> > +         * Return to the guest.
> > +         */
> >          rc = gva_to_ipa(gva, &gpa, GV2M_READ);
> >          if ( rc == -EFAULT )
> >              return; /* Try again */
> > @@ -2437,8 +2443,17 @@ static void do_trap_instr_abort_guest(struct 
> > cpu_user_regs *regs,
> >          /* Trap was triggered by mem_access, work here is done */
> >          if ( !rc )
> >              return;
> > +        break;
> >      }
> > -    break;
> > +    case FSC_FLT_TRANS:
> Please add brackets under the case statement for code style
This is not part of the coding style. We only use the brackets when 
local variable is defined for a specific case. See vgic-{v2,v3}.c for 
instance.
Note that I am a bit surprised you complain here about the missing 
brackets but you did not on commit def4273 "xen/arm: traps: MMIO should 
only be emulated for fault translation" at the beginning of august.
> > +        /*
> > +         * The PT walk may have failed because someone was playing
> > +         * with the Stage-2 page table. Walk the Stage-2 PT to check
> > +         * if the entry exists. If it's the case, return to the guest
> > +         */
> > +        mfn = p2m_lookup(current->domain, _gfn(paddr_to_pfn(gpa)), NULL);
> > +        if ( !mfn_eq(mfn, INVALID_MFN) )
> > +            return;
> Just checking, but isn't it possible to get a genuine translation abort
> with an mfn != invalid_mfn?
A translation fault means the entry is not present in the page table at 
the time the processor did the page table walk.
This may happen because the hypervisor is modifying the stage-2 page 
table on another processor (for instance with the break-before-make 
sequence).
p2m_lookup will do a software page walk to get entry. As the function is 
using a read lock, it will wait until the other processor finishes to 
update the page tables before doing the lookup. So if the mfn returned 
is valid, then we know that the translation fault is spurious and should 
return to the guest to retry the execution of the faulting instruction.
> >      }
> >
> >      inject_iabt_exception(regs, gva, hsr.len);
> > @@ -2455,7 +2470,7 @@ static bool_t try_handle_mmio(struct cpu_user_regs *regs,
> >          return 0;
> >
> >      /* All the instructions used on emulated MMIO region should be valid */
> > -    if ( !dabt.valid )
> > +    if ( !info->dabt.valid )
> >          return 0;
> Spurious change?
Yes. I will drop it.

Regards,

--
Julien Grall

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