Hi,
Asking here as well (might not have been clear on irc).
On 27/01/2022 00:10, Julien Grall wrote:
Hi,
On Wed, 26 Jan 2022, 17:56 Ayan Kumar Halder,
<ayan.kumar.hal...@xilinx.com> wrote:
Hi Julien,
On 26/01/2022 17:22, Julien Grall wrote:
Hi,
On Wed, 26 Jan 2022, 16:58 Ayan Kumar Halder,
<ayan.kumar.hal...@xilinx.com> wrote:
Refer to Armv8 ARM DDI 0487G.b, Page D13-3219 "ISS encoding
for an exception
from a Data Abort" :-
ISV - ISS[23:14] holds a valid instruction syndrome
When the ISV is 0, the instruction could not be decoded by
the hardware (ie ISS
is invalid). One should immediately return an error to the
caller with an
appropriate error message.
That's going to be very spammy because we have use-case where it
could trap without a valid ISV (e.g. when break-before-make
happens). So please don't had a message.
There is already a logging statement in traps.c :-
inject_abt:
gdprintk(XENLOG_DEBUG,
"HSR=%#"PRIregister" pc=%#"PRIregister"
gva=%#"PRIvaddr" gpa=%#"PRIpaddr"\n",
hsr.bits, regs->pc, gva, gpa);
The reason for the error is to give the user some hint that an IO
abort is triggered by Xen.
The main difference here is inject_dabt should only be reached when we
exhausted all the possibility in I/O handling.
In your case, if we can't handle as an MMIO then we should fallthrough
the other method (see do_trap_stage2_abort_guest()).
In fact, moving the check early and doing the decoding before
find_mmio() or try_fwd_io() is actually wrong. Sorry I should realized
that earlier.
Why should we care about MMIO when ISS is invalid ? Should we not check
first if the ISS is valid or not as it will trigger IO_abort regardless
of the MMIO.
I am assuming that once Xen resolves the MMIO
(p2m_resolve_translation_fault()), the guest will again try to run the
same instruction on MMIO region. This will be trapped in Xen which will
return IO abort as the post-indexing instruction could not be decoded.
In this scenario, translation fault resolved by Xen was of no use.
Please help to clear my doubts.
- Ayan
So we want to provide an helper that will do the dabt.vid check and do
the decoding. The helper will be called in 2 places.
With that in place, then I agree the gprintk could be kept in place.
Can we keep the error message ?
Also, I think this is a duplicate check
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=xen/arch/arm/ioreq.c;h=308650b40051825fdea78bb33bfbcc87ef5deaff;hb=HEAD#l79
, I will remove this in v2 if it makes sense.
- Ayan
That makes me think that the same will be valid for your other patch.
There is no use of the MMIO handler. This is the
reason why one should check for ISV before attempting to find
a MMIO handler.
Signed-off-by: Ayan Kumar Halder <ayank...@xilinx.com>
---
Suggested by Julien Grall in
https://lists.xenproject.org/archives/html/xen-devel/2022-01/msg01245.html
xen/arch/arm/io.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 729287e37c..14d39222f2 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -109,6 +109,13 @@ enum io_state try_handle_mmio(struct
cpu_user_regs *regs,
ASSERT(hsr.ec <http://hsr.ec> ==
HSR_EC_DATA_ABORT_LOWER_EL);
+ /* All the instructions used on emulated MMIO region
should be valid */
+ if ( !dabt.valid )
+ {
+ gprintk(XENLOG_DEBUG, "No valid instruction syndrome
for data abort\n");
+ return IO_ABORT;
+ }
+
handler = find_mmio_handler(v->domain, info.gpa);
if ( !handler )
{
@@ -121,10 +128,6 @@ enum io_state try_handle_mmio(struct
cpu_user_regs *regs,
return rc;
}
- /* All the instructions used on emulated MMIO region
should be valid */
- if ( !dabt.valid )
- return IO_ABORT;
-
/*
* Erratum 766422: Thumb store translation fault to
Hypervisor may
* not have correct HSR Rt value.
--
2.17
