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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

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.

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 )
+        {
+            if ( !decode_instruction(regs, &hsr.dabt) )
+                goto again;
+        }

        break;
    default:
        gprintk(XENLOG_WARNING,
                "Unsupported FSC: HSR=%#"PRIregister" DFSC=%#x\n",
                hsr.bits, xabt.fsc);
    }