Hi Stefano,
Thanks for your feedback.
On 09/02/2022 01:50, Stefano Stabellini wrote:
On Sat, 5 Feb 2022, Ayan Kumar Halder wrote:
<snip>
@@ -95,6 +97,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs,
bool arch_ioreq_complete_mmio(void)
{
struct vcpu *v = current;
+ struct instr_details *dabt_instr = v->io.info.dabt_instr;
struct cpu_user_regs *regs = guest_cpu_user_regs();
const union hsr hsr = { .bits = regs->hsr };
I don't think that v->io.info.dabt_instr should be a pointer because at
this point the original structure is long gone. The original field was a
local variable mmio_info_t info in do_trap_stage2_abort_guest. So by the
time arch_ioreq_complete_mmio is called, the original variable has been
deallocated.
Instead, v->io.info.dabt_instr should be a "struct instr_details" (no
pointer).
I see what you mean.
arch_ioreq_complete_mmio() is called from leave_hypervisor_to_guest().
That is after do_trap_stage2_abort_guest
() has been invoked. So the original variable is no longer valid.
<snip>
+
+ /*
+ * When the instruction needs to be retried by the guest after
+ * resolving the translation fault.
+ */
+ else if ( info.dabt_instr.state == INSTR_RETRY )
+ goto set_page_tables;
As discussed with Julien on IRC, when hsr_dabt.s1ptw == 1, Xen should
only invoke p2m_resolve_translation_fault(). If it returns an error, it
should send an abort to the guest. The reason being there is no need to
invoke try_map_mmio() as the gpa is not a mmio address. Also, Xen does
not support the use case when the guest places the page tables in the
MMIO region.
I will wait for Julien's comments before sending out a v8 patch.
- Ayan
+
+ state = try_handle_mmio(regs, &info);
switch ( state )
{
case IO_ABORT:
goto inject_abt;
case IO_HANDLED:
+ /*
+ * If the instruction was decoded and has executed successfully
+ * on the MMIO region, then Xen should execute the next part of
+ * the instruction. (for eg increment the rn if it is a
+ * post-indexing instruction.
+ */
+ post_increment_register(&info.dabt_instr);
advance_pc(regs, hsr);
return;
case IO_RETRY:
@@ -1985,18 +2056,12 @@ static void do_trap_stage2_abort_guest(struct
cpu_user_regs *regs,
}
}
- /*
- * First check if the translation fault can be resolved by the
- * P2M subsystem. If that's the case nothing else to do.
- */
- if ( p2m_resolve_translation_fault(current->domain,
- gaddr_to_gfn(gpa)) )
- return;
-
- if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) )
+ set_page_tables:
+ if ( check_p2m(is_data, gpa) )
return;
break;
+ }
default:
gprintk(XENLOG_WARNING,
"Unsupported FSC: HSR=%#"PRIregister" DFSC=%#x\n",
diff --git a/xen/arch/x86/include/asm/ioreq.h b/xen/arch/x86/include/asm/ioreq.h
index d06ce9a6ea..ecfe7f9fdb 100644
--- a/xen/arch/x86/include/asm/ioreq.h
+++ b/xen/arch/x86/include/asm/ioreq.h
@@ -26,6 +26,9 @@
#include <asm/hvm/ioreq.h>
#endif
+struct arch_vcpu_io {
+};
+
#endif /* __ASM_X86_IOREQ_H__ */
/*
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 37f78cc4c4..afe5508be8 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -160,6 +160,8 @@ struct vcpu_io {
/* I/O request in flight to device model. */
enum vio_completion completion;
ioreq_t req;
+ /* Arch specific info pertaining to the io request */
+ struct arch_vcpu_io info;
};
struct vcpu
--
2.17.1
