[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v1] xen/arm: io: Check ESR_EL2.ISV != 0 before searching for a MMIO handler
On Fri, 28 Jan 2022, Julien Grall wrote: > 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. That's a good point. I wonder if it could be as simple as making sure that iomem_access_permitted returns false for all emulated regions? Looking at the code, it looks like it is already the case today. Is that right? > > 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. Yeah... we can find a way to avoid it.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |