[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,
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.

Cheers,

--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.