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.
Indeed! Just coming to this opinion I saw your answer)
This is a dirty test patch:
---
xen/arch/arm/io.c | 12 ++++++++++++
xen/arch/arm/ioreq.c | 12 ++++++++++++
xen/arch/arm/traps.c | 6 ++++--
xen/include/asm-arm/hvm/ioreq.h | 2 ++
xen/include/asm-arm/traps.h | 3 +++
5 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 436f669..65a08f8 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -130,6 +130,10 @@ static enum io_state try_fwd_ioserv(struct
cpu_user_regs *regs,
{
case STATE_IOREQ_NONE:
break;
+
+ case STATE_IORESP_READY:
+ return IO_HANDLED;
+
default:
printk("d%u wrong state %u\n", v->domain->domain_id,
vio->io_req.state);
@@ -156,9 +160,11 @@ static enum io_state try_fwd_ioserv(struct
cpu_user_regs *regs,
else
vio->io_completion = HVMIO_mmio_completion;
+#if 0
/* XXX: Decide what to do */
if ( rc == IO_RETRY )
rc = IO_HANDLED;
+#endif
return rc;
}
@@ -185,6 +191,12 @@ enum io_state try_handle_mmio(struct cpu_user_regs
*regs,
#ifdef CONFIG_IOREQ_SERVER
rc = try_fwd_ioserv(regs, v, &info);
+ if ( rc == IO_HANDLED )
+ {
+ printk("HANDLED %s[%d]\n", __func__, __LINE__);
+ handle_mmio_finish();
+ } else
+ printk("RETRY %s[%d]\n", __func__, __LINE__);
#endif
return rc;
diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c
index 8f60c41..c8ed454 100644
--- a/xen/arch/arm/ioreq.c
+++ b/xen/arch/arm/ioreq.c
@@ -33,8 +33,20 @@
#include <public/hvm/dm_op.h>
#include <public/hvm/ioreq.h>
+#include <asm/traps.h>
+
bool handle_mmio(void)
{
+ struct cpu_user_regs *regs = guest_cpu_user_regs();
+ const union hsr hsr = { .bits = regs->hsr };
+
+ do_trap_stage2_abort_guest(regs, hsr);
+
+ return true;
+}
+
+bool handle_mmio_finish(void)
+{
struct vcpu *v = current;
struct cpu_user_regs *regs = guest_cpu_user_regs();
const union hsr hsr = { .bits = regs->hsr };
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ea472d1..3493d77 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1882,7 +1882,7 @@ static bool try_map_mmio(gfn_t gfn)
return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c);
}
-static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
+void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
const union hsr hsr)
{
/*
@@ -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();
}
}
diff --git a/xen/include/asm-arm/hvm/ioreq.h
b/xen/include/asm-arm/hvm/ioreq.h
index 392ce64..fb4684d 100644
--- a/xen/include/asm-arm/hvm/ioreq.h
+++ b/xen/include/asm-arm/hvm/ioreq.h
@@ -27,6 +27,8 @@
bool handle_mmio(void);
+bool handle_mmio_finish(void);
+
static inline bool handle_pio(uint16_t port, unsigned int size, int dir)
{
/* XXX */
diff --git a/xen/include/asm-arm/traps.h b/xen/include/asm-arm/traps.h
index 997c378..392fdb1 100644
--- a/xen/include/asm-arm/traps.h
+++ b/xen/include/asm-arm/traps.h
@@ -40,6 +40,9 @@ void advance_pc(struct cpu_user_regs *regs, const
union hsr hsr);
void inject_undef_exception(struct cpu_user_regs *regs, const union
hsr hsr);
+void do_trap_stage2_abort_guest(struct cpu_user_regs *regs,
+ const union hsr hsr);
+
/* read as zero and write ignore */
void handle_raz_wi(struct cpu_user_regs *regs, int regidx, bool read,
const union hsr hsr, int min_el);
--
2.7.4
That is assuming that you are looking at the live version of the ioreq
shared with QEMU instead of a private copy of it, which I am not sure.
Looking at try_fwd_ioserv() it would seem that vio->io_req is just a
copy? The live version is returned by get_ioreq() ?
Even in handle_hvm_io_completion, instead of setting vio->io_req.state
to STATE_IORESP_READY by hand, it would be better to look at the live
version of the ioreq because QEMU will have already set ioreq->state
to STATE_IORESP_READY (hw/i386/xen/xen-hvm.c:cpu_handle_ioreq).
I need to recheck that.
Thank you.
--
Regards,
Oleksandr Tyshchenko
