Hi
On 08.08.20 01:19, Oleksandr wrote:
On 08.08.20 00:50, Stefano Stabellini wrote:
Hi Stefano
On Fri, 7 Aug 2020, Oleksandr wrote:
On 06.08.20 03:37, Stefano Stabellini wrote:
Hi Stefano
Trying to simulate IO_RETRY handling mechanism (according to model
below) I
continuously get IO_RETRY from try_fwd_ioserv() ...
OK, thanks for the details. My interpretation seems to be correct.
In which case, it looks like xen/arch/arm/io.c:try_fwd_ioserv should
return IO_RETRY. Then, xen/arch/arm/traps.c:do_trap_stage2_abort_guest
also needs to handle try_handle_mmio returning IO_RETRY the first
around, and IO_HANDLED when after QEMU does its job.
What should do_trap_stage2_abort_guest do on IO_RETRY? Simply return
early and let the scheduler do its job? Something like:
enum io_state state = try_handle_mmio(regs, hsr, gpa);
switch ( state )
{
case IO_ABORT:
goto inject_abt;
case IO_HANDLED:
advance_pc(regs, hsr);
return;
case IO_RETRY:
/* finish later */
return;
case IO_UNHANDLED:
/* IO unhandled, try another way to handle it. */
break;
default:
ASSERT_UNREACHABLE();
}
Then, xen/arch/arm/ioreq.c:handle_mmio() gets called by
handle_hvm_io_completion() after QEMU completes the emulation. Today,
handle_mmio just sets the user register with the read value.
But it would be better if it called again the original function
do_trap_stage2_abort_guest to actually retry the original operation.
This time do_trap_stage2_abort_guest calls try_handle_mmio() and gets
IO_HANDLED instead of IO_RETRY,
I may miss some important point, but I failed to see why
try_handle_mmio
(try_fwd_ioserv) will return IO_HANDLED instead of IO_RETRY at this
stage.
Or current try_fwd_ioserv() logic needs rework?
I think you should check the ioreq->state in try_fwd_ioserv(), if the
result is ready, then ioreq->state should be STATE_IORESP_READY, and you
can return IO_HANDLED.
I optimized test patch a bit (now it looks much simpler). I didn't face
any issues during a quick test.
Julien, Stefano, what do you think it does proper things for addressing
TODO? Or I missed something?
---
xen/arch/arm/io.c | 4 ----
xen/arch/arm/ioreq.c | 7 ++++++-
xen/arch/arm/traps.c | 4 +++-
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 436f669..3063577 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -156,10 +156,6 @@ static enum io_state try_fwd_ioserv(struct
cpu_user_regs *regs,
else
vio->io_completion = HVMIO_mmio_completion;
- /* XXX: Decide what to do */
- if ( rc == IO_RETRY )
- rc = IO_HANDLED;
-
return rc;
}
#endif
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index 8f60c41..e5235c6 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -33,6 +33,8 @@
#include <public/hvm/dm_op.h>
#include <public/hvm/ioreq.h>
+#include <asm/traps.h>
+
bool handle_mmio(void)
{
struct vcpu *v = current;
@@ -52,7 +54,7 @@ bool handle_mmio(void)
/* XXX: Do we need to take care of write here ? */
if ( dabt.write )
- return true;
+ goto done;
/*
* Sign extend if required.
@@ -72,6 +74,9 @@ bool handle_mmio(void)
set_user_reg(regs, dabt.reg, r);
+done:
+ advance_pc(regs, hsr);
+
return true;
}
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ea472d1..974c744 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1965,11 +1965,13 @@ static void do_trap_stage2_abort_guest(struct
cpu_user_regs *regs,
case IO_HANDLED:
advance_pc(regs, hsr);
return;
+ case IO_RETRY:
+ /* finish later */
+ return;
case IO_UNHANDLED:
/* IO unhandled, try another way to handle it. */
break;
default:
- /* XXX: Handle IO_RETRY */
ASSERT_UNREACHABLE();
}
}
--
2.7.4
--
Regards,
Oleksandr Tyshchenko
