[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
Hi Stefano, On 28/01/2022 20:23, Stefano Stabellini wrote: 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, thenSorry 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? I have replied to that in the other thread. The short answer is no and... Looking at the code, it looks like it is already the case today. Is that right? not 100%. The thing is iomem_access_permitted() is telling you which *host* physical address is accessible. Not which *guest* physical address is emulated. We could possibly take some short cut at the risk of bitting back in the future if we end up to emulate non-existing region in the host physical address. Cheers, -- Julien Grall
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |