Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler

[Julien Grall <julien@xxxxxxx>]

On 28/01/2022 01:20, Stefano Stabellini wrote:
> On Thu, 27 Jan 2022, Julien Grall wrote:
> > On Thu, 27 Jan 2022 at 23:05, Julien Grall <julien.grall.oss@xxxxxxxxx> wrote:
> > > On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> > > > I am with you on both points.
> > > >
> > > > One thing I noticed is that the code today is not able to deal with
> > > > IO_UNHANDLED for MMIO regions handled by IOREQ servers or Xen MMIO
> > > > emulator handlers. p2m_resolve_translation_fault and try_map_mmio are
> > > > called after try_handle_mmio returns IO_UNHANDLED but try_handle_mmio is
> > > > not called a second time (or am I mistaken?)
> > > Why would you need it? If try_mmio_fault() doesn't work the first time, then
> > Sorry I meant try_handle_mmio().
> >
> > > it will not work the second it.
> I think I explained myself badly, I'll try again below.
>
>
> > > > Another thing I noticed is that currently find_mmio_handler and
> > > > try_fwd_ioserv expect dabt to be already populated and valid so it would
> > > > be better if we could get there only when dabt.valid.
> > > >
> > > > With these two things in mind, I think maybe the best thing to do is to
> > > > change the code in do_trap_stage2_abort_guest slightly so that
> > > > p2m_resolve_translation_fault and try_map_mmio are called first when
> > > > !dabt.valid.
> > > An abort will mostly likely happen because of emulated I/O. If we call
> > > p2m_resolve_translation_fault() and try_map_mmio() first, then it means
> > > the processing will take longer than necessary for the common case.
> > >
> > > So I think we want to keep the order as it is. I.e first trying the MMIO
> > > and then falling back to the less likely reason for a trap.
> Yeah I thought about it as well. The idea would be that if dabt.valid is
> set then we leave things as they are (we call try_handle_mmio first) but
> if dabt.valid is not set (it is not valid) then we skip the
> try_handle_mmio() call because it wouldn't succeed anyway and go
> directly to p2m_resolve_translation_fault() and try_map_mmio().
>
> If either of them work (also reading what you wrote about it) then we
> return immediately.
Ok. So the assumption is data abort with invalid syndrome would mostly 
likely be because of a fault handled by p2m_resolve_translation_fault().
I think this makes sense. However, I am not convinced we can currently 
safely call try_map_mmio() before try_handle_mmio(). This is because the 
logic in try_map_mmio() is quite fragile and we may mistakenly map an 
emulated region.
Similarly, we can't call try_map_mmio() before 
p2m_resolve_translation_fault() because a transient fault may be
misinterpreted.

I think we may be able to harden try_map_mmio() by checking if the I/O 
region is emulated. But this will need to be fully thought through first.
> If not, then we call decode_instruction from do_trap_stage2_abort_guest
> and try again. The second time dabt.valid is set so we end up calling
> try_handle_mmio() as usual.
With the approach below, you will also end up to call 
p2m_resolve_translation_fault() and try_map_mmio() a second time if 
try_handle_mmio() fails.
> Just for clarity let me copy/paste the relevant code, apologies if it
> was already obvious to you -- I got the impression my suggestion wasn't
> very clear.
>
>
>
> +again:
> +        if ( is_data && hsr.dabt.valid )
>          {
>              enum io_state state = try_handle_mmio(regs, hsr, gpa);
>
>              switch ( state )
>              {
>              case IO_ABORT:
>                  goto inject_abt;
>              case IO_HANDLED:
>                  advance_pc(regs, hsr);
>                  return;
>              case IO_RETRY:
>                  /* finish later */
>                  return;
>              case IO_UNHANDLED:
>                  /* IO unhandled, try another way to handle it. */
>                  break;
>              }
>          }
>
>          /*
>           * First check if the translation fault can be resolved by the
>           * P2M subsystem. If that's the case nothing else to do.
>           */
>          if ( p2m_resolve_translation_fault(current->domain,
>                                             gaddr_to_gfn(gpa)) )
>              return;
>
>          if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
>              return;
>
> +        if ( !hsr.dabt.valid )
One more thing I noticed, a stage 2 fault can also happen on an access 
made for a stage 1 translation walk. In this case, I think we don't want 
to decode the instruction.
So this would need to be !hsr.dabt.valid && !hsr.dabt.s1ptw. Depending 
on which patch we go with, this would also need to be adjusted in the 
other one as well.
Cheers,

--
Julien Grall