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

[Ayan Kumar Halder <ayan.kumar.halder@xxxxxxxxxx>]

Hi Stefano/Julien,

On 29/01/2022 17:40, Julien Grall wrote:
> 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,
> > > > > > 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?
> 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.
I have sent out a "[XEN v5] xen/arm64: io: Decode ldr/str post-indexing 
instructions" addressing some of the comments based on the discussion. 
As for this patch, we agreed that this is incorrect. Thus, there will be 
no v2 patch for this.
- Ayan

> Cheers,