Re: [Xen-devel] [PATCH v3 33/38] arm/p2m: Add altp2m paging mechanism

[Julien Grall <julien.grall@xxxxxxx>]

On 14/09/16 08:53, Sergej Proskurin wrote:
> Hi Julien
>
>
> [...]
>
> > > >  static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
> > > >                                        const union hsr hsr)
> > > >  {
> > > > @@ -2445,6 +2465,14 @@ static void do_trap_instr_abort_guest(struct
> > > > cpu_user_regs *regs,
> > > >          break;
> > > >      case FSC_FLT_TRANS:
> > > >          /*
> > > > +         * The guest shall retry accessing the page if the altp2m
> > > > handler
> > > > +         * succeeds. Otherwise, we continue injecting an instruction
> > > > abort
> > > > +         * exception.
> > > > +         */
> > > > +        if ( try_handle_altp2m(current, gpa, npfec) )
> > > > +            return;
> > > I would have expected that altp2m is the last thing we want to check
> > > in the abort handler. I.e after p2m_lookup.
> > Alright, I will reorder both calls, thank you.
> Actually, reordering of these calls (try_handle_altp2m/altp2m_lazy_copy
> and p2m_lookup) would make the system stall if altp2m is active. This is
> because the p2m_lookup routine considers only the host's p2m, which will
> most likely return a mfn != INVALID_MFN and thus entirely skip the call
> to altp2m_lazy_copy. Thus, both calls should rather be conditional as
> shown in the following:
> if ( likely(!altp2m_active(current->domain)) )
> {
>     /*
>      * 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;
> }
> else
>     /*
>      * The guest shall retry accessing the page if the altp2m handler
>      * succeeds. Otherwise, we continue injecting an instruction abort
>      * exception.
>      */
>     if ( altp2m_lazy_copy(current, _gfn(paddr_to_pfn(gpa))) )
>         return;
I think this solution is racy because altp2m_active could be changed by 
another physical CPU whilst we are in the abort handler.
So the best here is to keep the previous solution:

if ( altp2m_lazy_copy(current, _gfn(paddr_to_pfn(gpa))) )
  return;

if ( p2m_lookup(current->domain, _gfn(paddr_to_pfn(gpa)), NULL )
  return;

Regards,

--
Julien Grall

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