On Thu, 27 Jan 2022, Julien Grall wrote: > On Thu, 27 Jan 2022 at 23:05, Julien Grall <julien.grall....@gmail.com> wrote: > > > > On Thu, 27 Jan 2022 at 22:40, Stefano Stabellini <sstabell...@kernel.org> > > 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); }