[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v8 2/2] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO/ioreq handler
Hi, On 12/02/2022 23:34, Ayan Kumar Halder wrote: xen/arch/arm/arm32/traps.c | 7 +++ xen/arch/arm/arm64/traps.c | 47 +++++++++++++++ xen/arch/arm/decode.c | 1 + xen/arch/arm/include/asm/domain.h | 4 ++ xen/arch/arm/include/asm/mmio.h | 15 ++++- xen/arch/arm/include/asm/traps.h | 2 + xen/arch/arm/io.c | 98 ++++++++++++++++++++----------- xen/arch/arm/ioreq.c | 7 ++- xen/arch/arm/traps.c | 80 +++++++++++++++++++++---- xen/arch/x86/include/asm/ioreq.h | 3 + This change technically needs an ack from the x86 maintainers. And... xen/include/xen/sched.h | 2 + this one for anyone from THE REST (Stefano and I are part of it). Please use scripts/add_maintainers.pl to automatically add the relevant maintainers in CC. 11 files changed, 217 insertions(+), 49 deletions(-) diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c index 9c9790a6d1..70c6238196 100644 --- a/xen/arch/arm/arm32/traps.c +++ b/xen/arch/arm/arm32/traps.c @@ -18,9 +18,11 @@#include <xen/lib.h>#include <xen/kernel.h> +#include <xen/sched.h>#include <public/xen.h> +#include <asm/mmio.h>#include <asm/processor.h> #include <asm/traps.h>@@ -82,6 +84,11 @@ void do_trap_data_abort(struct cpu_user_regs *regs)do_unexpected_trap("Data Abort", regs); }+void post_increment_register(const struct instr_details *instr)+{ + domain_crash(current->domain); Please add a comment explaning why this is resulting to a domain crash. AFAICT, this is because this should not be reachable (yet) for 32-bit. +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c index 9113a15c7a..a6766689b3 100644 --- a/xen/arch/arm/arm64/traps.c +++ b/xen/arch/arm/arm64/traps.c @@ -23,6 +23,7 @@ #include <asm/processor.h>#include <public/xen.h>+#include <xen/sched.h> The headers should ordered so <xen/*.h> are first, then <asm/*.h>, then <public/*.h>. They should then be ordered alphabetically within each of the category. So, this new header should be included right after <xen/lib.h> [...] diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h index 3354d9c635..745130b7fe 100644 --- a/xen/arch/arm/include/asm/mmio.h +++ b/xen/arch/arm/include/asm/mmio.h @@ -26,12 +26,22 @@#define MAX_IO_HANDLER 16 +enum instr_decode_state+{ + INSTR_ERROR, /* Error encountered while decoding instr */ + INSTR_VALID, /* ISS is valid, so no need to decode */ + INSTR_LDR_STR_POSTINDEXING, /* Instruction is decoded successfully. + It is ldr/str post indexing */ Coding style: multiple-line comments for Xen should be: /* * ... * ... */ In this case, I would simply move the comment on top. [...] diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index a289d393f9..203466b869 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -95,57 +95,87 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d, return handler; }+void try_decode_instruction(const struct cpu_user_regs *regs,+ mmio_info_t *info) +{ + int rc; + + /* + * Erratum 766422: Thumb store translation fault to Hypervisor may + * not have correct HSR Rt value. + */ + if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && + info->dabt.write ) + { + rc = decode_instruction(regs, info); + if ( rc ) + { + gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); + info->dabt_instr.state = INSTR_ERROR; + return; + } + } At the moment, the errata would only be handled when the ISS is valid. Now, you are moving it before we know if it is valid. Can you explain why? [...] enum io_state try_handle_mmio(struct cpu_user_regs *regs, - const union hsr hsr, - paddr_t gpa) + mmio_info_t *info) { struct vcpu *v = current; const struct mmio_handler *handler = NULL; - const struct hsr_dabt dabt = hsr.dabt; - mmio_info_t info = { - .gpa = gpa, - .dabt = dabt - }; + int rc;- ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);+ ASSERT(info->dabt.ec == HSR_EC_DATA_ABORT_LOWER_EL);- handler = find_mmio_handler(v->domain, info.gpa);+ handler = find_mmio_handler(v->domain, info->gpa); if ( !handler ) { - int rc; - - rc = try_fwd_ioserv(regs, v, &info); + rc = try_fwd_ioserv(regs, v, info); if ( rc == IO_HANDLED ) return handle_ioserv(regs, v);return rc;}- /* All the instructions used on emulated MMIO region should be valid */- if ( !dabt.valid ) - return IO_ABORT; - AFAIU, the assumption is now try_handle_mmio() and try_fwd_ioserv() will always be called when dabt.valid == 1. I think it would still be good to check that assumption. So I would move the check at the beginning of try_handle_mmio() and add an ASSERT_UNREACHABLE in the if(). Something like: if ( !dabt.valid ) { ASSERT_UNREACHABLE(); return IO_ABORT; } /* - * Erratum 766422: Thumb store translation fault to Hypervisor may - * not have correct HSR Rt value. + * At this point, we know that the instruction is either valid or has been + * decoded successfully. Thus, Xen should be allowed to execute the + * instruction on the emulated MMIO region. */ - if ( check_workaround_766422() && (regs->cpsr & PSR_THUMB) && - dabt.write ) - { - int rc; - - rc = decode_instruction(regs, &info); - if ( rc ) - { - gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); - return IO_ABORT; - } - } - - if ( info.dabt.write ) - return handle_write(handler, v, &info); + if ( info->dabt.write ) + rc = handle_write(handler, v, info); else - return handle_read(handler, v, &info); + rc = handle_read(handler, v, info); + + return rc; It looks like there are some left-over of the previous approach. It is fine to return directly from each branch. }void register_mmio_handler(struct domain *d,diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c index 308650b400..3c0a935ccf 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -47,6 +47,7 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, struct vcpu *v, mmio_info_t *info) { struct vcpu_io *vio = &v->io; + struct dabt_instr instr = info->dabt_instr; ioreq_t p = { .type = IOREQ_TYPE_COPY, .addr = info->gpa, @@ -76,10 +77,8 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, if ( !s ) return IO_UNHANDLED;- if ( !info->dabt.valid )- return IO_ABORT; - For this one, I would switch to ASSERT(dabt.valid); vio->req = p; + vio->info.dabt_instr = instr;rc = ioreq_send(s, &p, 0);if ( rc != IO_RETRY || v->domain->is_shutting_down ) @@ -95,6 +94,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 };@@ -106,6 +106,7 @@ bool arch_ioreq_complete_mmio(void) if ( handle_ioserv(regs, v) == IO_HANDLED ){ + post_increment_register(&dabt_instr); advance_pc(regs, hsr); return true; } diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 9339d12f58..455e51cdbe 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1893,6 +1893,21 @@ static bool try_map_mmio(gfn_t gfn) return !map_regions_p2mt(d, gfn, 1, mfn, p2m_mmio_direct_c); }+static inline bool check_p2m(bool is_data, paddr_t gpa)+{ + /* + * 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)) ) Coding style: missing space before and after the comma. + return true; + + if ( is_data && try_map_mmio(gaddr_to_gfn(gpa)) ) + return true; + + return false; +} + static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, const union hsr hsr) { @@ -1906,6 +1921,7 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, paddr_t gpa; uint8_t fsc = xabt.fsc & ~FSC_LL_MASK; bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL); + mmio_info_t info;/** If this bit has been set, it means that this stage-2 abort is caused @@ -1959,6 +1975,25 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, return; } case FSC_FLT_TRANS: + { + info.gpa = gpa; + info.dabt = hsr.dabt; + + /* Check that the ISS is invalid and it is not data abort. */ This comment looks a bit pointless. You are writing literally what the check is doing. But you don't really explain why. I think you want to move some of the commint with the if here. However,... + if ( !hsr.dabt.valid && !is_data ) ... this code can be reached by Instruction Abort and Data Abort. So you can't use hsr.dabt. Instead, you should use xabt (or check is_data first). If you use xabt, you will notice that the 'valid' bit is not existent because the instruction syndrome only exists for data abort.But then, I don't understand why this is only restricted to instruction abort. As I wrote in the previous versions and on IRC, there are valid use cases to trap a data abort with invalid syndrome. Below... + { + + /* + * Assumption :- Most of the times when we get a translation fault + * and the ISS is invalid, the underlying cause is that the page + * tables have not been set up correctly. + */ + if ( check_p2m(is_data, gpa) ) + return; + else + goto inject_abt; + } + /* * Attempt first to emulate the MMIO as the data abort will * likely happen in an emulated region. @@ -1967,13 +2002,45 @@ static void do_trap_stage2_abort_guest(struct cpu_user_regs *regs, */ if ( is_data ) { - enum io_state state = try_handle_mmio(regs, hsr, gpa); + enum io_state state; + + try_decode_instruction(regs, &info); + + /* + * If Xen could not decode the instruction for any reason, then it + * should ask the caller to abort the guest. + */ + if ( info.dabt_instr.state == INSTR_ERROR ) + goto inject_abt; ... this will inject a data abort to the guest when we can't decode. This is not what we want. We should check whether this is a P2M translation fault or we need to map an MMIO region. In pseudo-code, this would look like: if ( !is_data || hsr.dabt.valid ) { if ( check_p2m() ) return; if ( !is_data ) goto inject_dabt; decode_instruction(); if ( !dabt.invalid ) goto inject_dabt; } try_handle_mmio(); if ( instruction was not decoded ) check_p2m(); Cheers, -- Julien Grall
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |