| Message ID | 20220205225816.5952-3-ayankuma@xilinx.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm64: io: Decode ldr/str post-indexing instruction | expand |
Hi All, I had a discussion with Julien on IRC and this patch need a correction (based on my understanding):- On 05/02/2022 22:58, Ayan Kumar Halder wrote: > If the instruction was related to cache maintenance, Xen > will not decode the instruction or do any MMIO operation. Rather it simply > increments the PC and returns to the guest. This is not entirely correct :- If the instruction is cache maintenance and the address is emulated, then Xen needs to ignore the ignore the instruction ie increment the PC and return to the guest. However, if the address is not emulated, then Xen will get invoked only if the translation table entry corresponding to the address is set invalid. In this case, Xen will need to iterate through the translation tables and mark the entry as valid and return to the guest to retry the instruction. As of today, we do not handle cache condition (ie dabt.cache == 1) in Xen. Thus, I will drop this logic in my patch and will resend the current series (unless there are any objections). I will send out a separate patch to handle the cache condition. > If the instruction was trapped due > to stage1 page translation table walk, Xen will update the page tables and will > return to the guest so that it can retry the instruction. To handle all these > different states, we have introduced 'enum instr_decode_state'. > > Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com> > --- <snip> > diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h > index 3354d9c635..fb7ff72cdc 100644 > --- a/xen/arch/arm/include/asm/mmio.h > +++ b/xen/arch/arm/include/asm/mmio.h > @@ -26,12 +26,23 @@ > > #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 */ > + INSTR_IGNORE, /* Instruction is to be ignored (ie NOP) */ Drop this entry. > + INSTR_RETRY /* Instruction is to be retried */ > +}; > + > <snip> > + /* > + * If the fault occurred due to cache maintenance or address translation > + * instructions, then Xen needs to ignore these instructions. > + */ > + if ( info->dabt.cache ) > + { > + info->dabt_instr.state = INSTR_IGNORE; > + return; > + } I need to drop this. As stated before, this needs to be done in a separate patch. <snip> > + > + try_decode_instruction(regs, &info); > + > + /* > + * If the instruction was to be ignored by Xen, then it should return > + * to the caller which will increment the PC, so that the guest can > + * execute the next instruction. > + */ > + if ( info.dabt_instr.state == INSTR_IGNORE ) > + { > + advance_pc(regs, hsr); > + return; > + } Drop this. <snip> - Ayan
On Sat, 5 Feb 2022, Ayan Kumar Halder wrote: > When an instruction is trapped in Xen due to translation fault, Xen checks if > the ISS is valid. If not, Xen tries to resolve the translation fault using > p2m page tables. In case if it is a data abort, Xen will try to map the mmio > region to the guest (ie tries to emulate the mmio region). > > If the ISS is not valid, then Xen tries to decode the instruction. In case of > ioreq, Xen saves the decoding state, rn and imm9 to vcpu_io. Whenever the > vcpu handles the ioreq successfully, it will read the decoding state to > determine if the instruction decoded was a ldr/str post indexing (ie > INSTR_LDR_STR_POSTINDEXING). If so, it uses these details to post increment rn. > > In case of mmio handler, if the mmio operation was successful, then Xen retrives > the decoding state, rn and imm9. For state == INSTR_LDR_STR_POSTINDEXING, Xen > will update rn. > > If there is an error encountered while decoding the instruction, Xen will issue > an abort to the guest. If the instruction was related to cache maintenance, Xen > will not decode the instruction or do any MMIO operation. Rather it simply > increments the PC and returns to the guest. If the instruction was trapped due > to stage1 page translation table walk, Xen will update the page tables and will > return to the guest so that it can retry the instruction. To handle all these > different states, we have introduced 'enum instr_decode_state'. > > Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com> > --- > > Changelog :- > > v2..v5 - Mentioned in the cover letter. > > v6 - 1. Mantained the decoding state of the instruction. This is used by the > caller to either abort the guest or retry or ignore or perform read/write on > the mmio region. > > 2. try_decode() invokes decoding for both aarch64 and thumb state. (Previously > it used to invoke decoding only for aarch64 state). Thus, it handles all the > checking of the registers before invoking any decoding of instruction. > try_decode_instruction_invalid_iss() has thus been removed. > > 3. Introduced a new field('enum instr_decode_state state') inside > 'struct instr_details'. This holds the decoding state of the instruction. > This is later read by the post_increment_register() to determine if rn needs to > be incremented. Also, this is read by the callers of try_decode_instruction() > to determine if the instruction was valid or ignored or to be retried or > error or decoded successfully. > > 4. Also stored 'instr_details' inside 'struct ioreq'. This enables > arch_ioreq_complete_mmio() to invoke post_increment_register() without decoding > the instruction again. > > 5. Check hsr.dabt.valid in do_trap_stage2_abort_guest(). If it is not valid, > then decode the instruction. This ensures that try_handle_mmio() is invoked only > when the instruction is either valid or decoded successfully. > > 6. Inside do_trap_stage2_abort_guest(), if hsr.dabt.valid is not set, then > resolve the translation fault before trying to decode the instruction. If > translation fault is resolved, then return to the guest to execute the instruction > again. > > > v7 - 1. Moved the decoding instruction details ie instr_details from 'struct ioreq' > to 'struct vcpu_io'. > > 2. The instruction is decoded only when we get a data abort. > > 3. Replaced ASSERT_UNREACHABLE() with domain_crash(). The reason being asserts > can be disabled in some builds. In this scenario when the guest's cpsr is in an > erroneous state, Xen should crash the guest. > > 4. Introduced check_p2m() which invokes p2m_resolve_translation_fault() and > try_map_mmio() to resolve translation fault by configuring the page tables. This > gets invoked first if ISS is invalid and it is an instruction abort. If it is > a data abort and hsr.dabt.s1ptw is set or try_handle_mmio() returns IO_UNHANDLED, > then check_p2m() gets invoked again. > > xen/arch/arm/arm32/traps.c | 7 ++ > xen/arch/arm/arm64/traps.c | 47 +++++++++++++ > xen/arch/arm/decode.c | 1 + > xen/arch/arm/decode.h | 3 +- > xen/arch/arm/include/asm/domain.h | 4 ++ > xen/arch/arm/include/asm/ioreq.h | 1 + > xen/arch/arm/include/asm/mmio.h | 16 ++++- > xen/arch/arm/include/asm/traps.h | 2 + > xen/arch/arm/io.c | 108 ++++++++++++++++++++---------- > xen/arch/arm/ioreq.c | 12 ++-- > xen/arch/arm/traps.c | 85 ++++++++++++++++++++--- > xen/arch/x86/include/asm/ioreq.h | 3 + > xen/include/xen/sched.h | 2 + > 13 files changed, 240 insertions(+), 51 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); > +} > + > /* > * 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> > > static const char *handler[]= { > "Synchronous Abort", > @@ -44,6 +45,52 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason) > panic("bad mode\n"); > } > > +void post_increment_register(const struct instr_details *instr) > +{ > + struct cpu_user_regs *regs = guest_cpu_user_regs(); > + register_t val = 0; > + > + /* Currently, we handle only ldr/str post indexing instructions */ > + if ( instr->state != INSTR_LDR_STR_POSTINDEXING ) > + return; > + > + /* > + * Handle when rn = SP > + * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer register > + * selection" > + * t = SP_EL0 > + * h = SP_ELx > + * and M[3:0] (Page - C5-474 "When exception taken from AArch64 state:") > + */ > + if (instr->rn == 31 ) > + { > + if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h ) > + val = regs->sp_el1; > + else if ( ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1t) || > + ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t) ) > + val = regs->sp_el0; > + else > + { > + domain_crash(current->domain); > + return; > + } > + } > + else > + val = get_user_reg(regs, instr->rn); > + > + val += instr->imm9; > + > + if ( instr->rn == 31 ) > + { > + if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h ) > + regs->sp_el1 = val; > + else > + regs->sp_el0 = val; > + } > + else > + set_user_reg(regs, instr->rn, val); > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c > index 3f2d2a3f62..0a4d9d2772 100644 > --- a/xen/arch/arm/decode.c > +++ b/xen/arch/arm/decode.c > @@ -147,6 +147,7 @@ static int decode_arm64(register_t pc, mmio_info_t *info) > > update_dabt(dabt, opcode.ldr_str.rt, opcode.ldr_str.size, false); > > + dabt_instr->state = INSTR_LDR_STR_POSTINDEXING; > dabt_instr->rn = opcode.ldr_str.rn; > dabt_instr->imm9 = opcode.ldr_str.imm9; > > diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h > index fe7512a053..6a09b07b46 100644 > --- a/xen/arch/arm/decode.h > +++ b/xen/arch/arm/decode.h > @@ -52,7 +52,8 @@ union instr { > #define POST_INDEX_FIXED_MASK 0x3B200C00 > #define POST_INDEX_FIXED_VALUE 0x38000400 > > -/* Decode an instruction from pc > +/* > + * Decode an instruction from pc > * /!\ This function is intended to decode an instruction. It considers that the > * instruction is valid. > * Ah! It should have been in the previous patch > diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h > index 9b3647587a..29efbbd334 100644 > --- a/xen/arch/arm/include/asm/domain.h > +++ b/xen/arch/arm/include/asm/domain.h > @@ -266,6 +266,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {} > /* vPCI is not available on Arm */ > #define has_vpci(d) ({ (void)(d); false; }) > > +struct arch_vcpu_io { > + struct instr_details *dabt_instr; /* when the instruction is decoded */ > +}; > + > #endif /* __ASM_DOMAIN_H__ */ > > /* > diff --git a/xen/arch/arm/include/asm/ioreq.h b/xen/arch/arm/include/asm/ioreq.h > index 50185978d5..a2f2cf76c7 100644 > --- a/xen/arch/arm/include/asm/ioreq.h > +++ b/xen/arch/arm/include/asm/ioreq.h > @@ -52,6 +52,7 @@ static inline void msix_write_completion(struct vcpu *v) > { > } > > + Spurious change > /* This correlation must not be altered */ > #define IOREQ_STATUS_HANDLED IO_HANDLED > #define IOREQ_STATUS_UNHANDLED IO_UNHANDLED > diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h > index 3354d9c635..fb7ff72cdc 100644 > --- a/xen/arch/arm/include/asm/mmio.h > +++ b/xen/arch/arm/include/asm/mmio.h > @@ -26,12 +26,23 @@ > > #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 */ > + INSTR_IGNORE, /* Instruction is to be ignored (ie NOP) */ > + INSTR_RETRY /* Instruction is to be retried */ > +}; > + > typedef struct > { > struct hsr_dabt dabt; > struct instr_details { > unsigned long rn:5; > signed int imm9:9; > + enum instr_decode_state state; > } dabt_instr; > paddr_t gpa; > } mmio_info_t; > @@ -69,14 +80,15 @@ struct vmmio { > }; > > enum io_state try_handle_mmio(struct cpu_user_regs *regs, > - const union hsr hsr, > - paddr_t gpa); > + mmio_info_t *info); > void register_mmio_handler(struct domain *d, > const struct mmio_handler_ops *ops, > paddr_t addr, paddr_t size, void *priv); > int domain_io_init(struct domain *d, int max_count); > void domain_io_free(struct domain *d); > > +void try_decode_instruction(const struct cpu_user_regs *regs, > + mmio_info_t *info); > > #endif /* __ASM_ARM_MMIO_H__ */ > > diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h > index 2ed2b85c6f..95c46ad391 100644 > --- a/xen/arch/arm/include/asm/traps.h > +++ b/xen/arch/arm/include/asm/traps.h > @@ -109,6 +109,8 @@ static inline register_t sign_extend(const struct hsr_dabt dabt, register_t r) > return r; > } > > +void post_increment_register(const struct instr_details *instr); > + > #endif /* __ASM_ARM_TRAPS__ */ > /* > * Local variables: > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c > index a289d393f9..25bc7e481a 100644 > --- a/xen/arch/arm/io.c > +++ b/xen/arch/arm/io.c > @@ -95,57 +95,97 @@ 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; > + } > + } > + > + /* If ISS is valid, then no need to decode the instruction any further */ > + if ( info->dabt.valid ) > + { > + info->dabt_instr.state = INSTR_VALID; > + return; > + } > + > + /* > + * Xen should not decode the instruction when it was trapped due to > + * translation fault. > + */ > + if ( info->dabt.s1ptw ) > + { > + info->dabt_instr.state = INSTR_RETRY; > + return; > + } > + > + /* > + * If the fault occurred due to cache maintenance or address translation > + * instructions, then Xen needs to ignore these instructions. > + */ > + if ( info->dabt.cache ) > + { > + info->dabt_instr.state = INSTR_IGNORE; > + return; > + } > + > + /* > + * Armv8 processor does not provide a valid syndrome for decoding some > + * instructions. So in order to process these instructions, Xen must > + * decode them. > + */ > + rc = decode_instruction(regs, info); > + if ( rc ) > + { > + gprintk(XENLOG_ERR, "Unable to decode instruction\n"); > + info->dabt_instr.state = INSTR_ERROR; > + } > +} > + > 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; > - > /* > - * 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; > } > > void register_mmio_handler(struct domain *d, > diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c > index 308650b400..ec830a7a4a 100644 > --- a/xen/arch/arm/ioreq.c > +++ b/xen/arch/arm/ioreq.c > @@ -26,7 +26,8 @@ > enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) > { > const union hsr hsr = { .bits = regs->hsr }; > - const struct hsr_dabt dabt = hsr.dabt; > + struct hsr_dabt dabt = hsr.dabt; This change is not needed, right? > /* Code is similar to handle_read */ > register_t r = v->io.req.data; > > @@ -47,6 +48,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, > @@ -65,6 +67,8 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > }; > struct ioreq_server *s = NULL; > enum io_state rc; > + bool instr_decoded = false; > + const union hsr hsr = { .bits = regs->hsr }; It doesn't look like these two local variables are actually used anywhere > if ( vio->req.state != STATE_IOREQ_NONE ) > { > @@ -76,10 +80,8 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, > if ( !s ) > return IO_UNHANDLED; > > - if ( !info->dabt.valid ) > - return IO_ABORT; > - > 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 +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). > @@ -106,6 +109,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..fb5a2f9634 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)) ) > + 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. */ > + if ( !hsr.dabt.valid && !is_data ) > + { > + > + /* > + * 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,49 @@ 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 the instruction was to be ignored by Xen, then it should return > + * to the caller which will increment the PC, so that the guest can > + * execute the next instruction. > + */ > + if ( info.dabt_instr.state == INSTR_IGNORE ) > + { > + advance_pc(regs, hsr); > + return; > + } > + > + /* > + * If Xen could not decode the instruction for any reason, then it > + * should ask the caller to abort the guest. > + */ > + else if ( info.dabt_instr.state == INSTR_ERROR ) > + goto inject_abt; > + > + /* > + * 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; > + > + 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 >
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 >
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); +} + /* * 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> static const char *handler[]= { "Synchronous Abort", @@ -44,6 +45,52 @@ void do_bad_mode(struct cpu_user_regs *regs, int reason) panic("bad mode\n"); } +void post_increment_register(const struct instr_details *instr) +{ + struct cpu_user_regs *regs = guest_cpu_user_regs(); + register_t val = 0; + + /* Currently, we handle only ldr/str post indexing instructions */ + if ( instr->state != INSTR_LDR_STR_POSTINDEXING ) + return; + + /* + * Handle when rn = SP + * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer register + * selection" + * t = SP_EL0 + * h = SP_ELx + * and M[3:0] (Page - C5-474 "When exception taken from AArch64 state:") + */ + if (instr->rn == 31 ) + { + if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h ) + val = regs->sp_el1; + else if ( ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1t) || + ((regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL0t) ) + val = regs->sp_el0; + else + { + domain_crash(current->domain); + return; + } + } + else + val = get_user_reg(regs, instr->rn); + + val += instr->imm9; + + if ( instr->rn == 31 ) + { + if ( (regs->cpsr & PSR_MODE_MASK) == PSR_MODE_EL1h ) + regs->sp_el1 = val; + else + regs->sp_el0 = val; + } + else + set_user_reg(regs, instr->rn, val); +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c index 3f2d2a3f62..0a4d9d2772 100644 --- a/xen/arch/arm/decode.c +++ b/xen/arch/arm/decode.c @@ -147,6 +147,7 @@ static int decode_arm64(register_t pc, mmio_info_t *info) update_dabt(dabt, opcode.ldr_str.rt, opcode.ldr_str.size, false); + dabt_instr->state = INSTR_LDR_STR_POSTINDEXING; dabt_instr->rn = opcode.ldr_str.rn; dabt_instr->imm9 = opcode.ldr_str.imm9; diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h index fe7512a053..6a09b07b46 100644 --- a/xen/arch/arm/decode.h +++ b/xen/arch/arm/decode.h @@ -52,7 +52,8 @@ union instr { #define POST_INDEX_FIXED_MASK 0x3B200C00 #define POST_INDEX_FIXED_VALUE 0x38000400 -/* Decode an instruction from pc +/* + * Decode an instruction from pc * /!\ This function is intended to decode an instruction. It considers that the * instruction is valid. * diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h index 9b3647587a..29efbbd334 100644 --- a/xen/arch/arm/include/asm/domain.h +++ b/xen/arch/arm/include/asm/domain.h @@ -266,6 +266,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {} /* vPCI is not available on Arm */ #define has_vpci(d) ({ (void)(d); false; }) +struct arch_vcpu_io { + struct instr_details *dabt_instr; /* when the instruction is decoded */ +}; + #endif /* __ASM_DOMAIN_H__ */ /* diff --git a/xen/arch/arm/include/asm/ioreq.h b/xen/arch/arm/include/asm/ioreq.h index 50185978d5..a2f2cf76c7 100644 --- a/xen/arch/arm/include/asm/ioreq.h +++ b/xen/arch/arm/include/asm/ioreq.h @@ -52,6 +52,7 @@ static inline void msix_write_completion(struct vcpu *v) { } + /* This correlation must not be altered */ #define IOREQ_STATUS_HANDLED IO_HANDLED #define IOREQ_STATUS_UNHANDLED IO_UNHANDLED diff --git a/xen/arch/arm/include/asm/mmio.h b/xen/arch/arm/include/asm/mmio.h index 3354d9c635..fb7ff72cdc 100644 --- a/xen/arch/arm/include/asm/mmio.h +++ b/xen/arch/arm/include/asm/mmio.h @@ -26,12 +26,23 @@ #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 */ + INSTR_IGNORE, /* Instruction is to be ignored (ie NOP) */ + INSTR_RETRY /* Instruction is to be retried */ +}; + typedef struct { struct hsr_dabt dabt; struct instr_details { unsigned long rn:5; signed int imm9:9; + enum instr_decode_state state; } dabt_instr; paddr_t gpa; } mmio_info_t; @@ -69,14 +80,15 @@ struct vmmio { }; enum io_state try_handle_mmio(struct cpu_user_regs *regs, - const union hsr hsr, - paddr_t gpa); + mmio_info_t *info); void register_mmio_handler(struct domain *d, const struct mmio_handler_ops *ops, paddr_t addr, paddr_t size, void *priv); int domain_io_init(struct domain *d, int max_count); void domain_io_free(struct domain *d); +void try_decode_instruction(const struct cpu_user_regs *regs, + mmio_info_t *info); #endif /* __ASM_ARM_MMIO_H__ */ diff --git a/xen/arch/arm/include/asm/traps.h b/xen/arch/arm/include/asm/traps.h index 2ed2b85c6f..95c46ad391 100644 --- a/xen/arch/arm/include/asm/traps.h +++ b/xen/arch/arm/include/asm/traps.h @@ -109,6 +109,8 @@ static inline register_t sign_extend(const struct hsr_dabt dabt, register_t r) return r; } +void post_increment_register(const struct instr_details *instr); + #endif /* __ASM_ARM_TRAPS__ */ /* * Local variables: diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c index a289d393f9..25bc7e481a 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -95,57 +95,97 @@ 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; + } + } + + /* If ISS is valid, then no need to decode the instruction any further */ + if ( info->dabt.valid ) + { + info->dabt_instr.state = INSTR_VALID; + return; + } + + /* + * Xen should not decode the instruction when it was trapped due to + * translation fault. + */ + if ( info->dabt.s1ptw ) + { + info->dabt_instr.state = INSTR_RETRY; + return; + } + + /* + * If the fault occurred due to cache maintenance or address translation + * instructions, then Xen needs to ignore these instructions. + */ + if ( info->dabt.cache ) + { + info->dabt_instr.state = INSTR_IGNORE; + return; + } + + /* + * Armv8 processor does not provide a valid syndrome for decoding some + * instructions. So in order to process these instructions, Xen must + * decode them. + */ + rc = decode_instruction(regs, info); + if ( rc ) + { + gprintk(XENLOG_ERR, "Unable to decode instruction\n"); + info->dabt_instr.state = INSTR_ERROR; + } +} + 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; - /* - * 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; } void register_mmio_handler(struct domain *d, diff --git a/xen/arch/arm/ioreq.c b/xen/arch/arm/ioreq.c index 308650b400..ec830a7a4a 100644 --- a/xen/arch/arm/ioreq.c +++ b/xen/arch/arm/ioreq.c @@ -26,7 +26,8 @@ enum io_state handle_ioserv(struct cpu_user_regs *regs, struct vcpu *v) { const union hsr hsr = { .bits = regs->hsr }; - const struct hsr_dabt dabt = hsr.dabt; + struct hsr_dabt dabt = hsr.dabt; + /* Code is similar to handle_read */ register_t r = v->io.req.data; @@ -47,6 +48,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, @@ -65,6 +67,8 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, }; struct ioreq_server *s = NULL; enum io_state rc; + bool instr_decoded = false; + const union hsr hsr = { .bits = regs->hsr }; if ( vio->req.state != STATE_IOREQ_NONE ) { @@ -76,10 +80,8 @@ enum io_state try_fwd_ioserv(struct cpu_user_regs *regs, if ( !s ) return IO_UNHANDLED; - if ( !info->dabt.valid ) - return IO_ABORT; - 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 +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 }; @@ -106,6 +109,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..fb5a2f9634 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)) ) + 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. */ + if ( !hsr.dabt.valid && !is_data ) + { + + /* + * 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,49 @@ 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 the instruction was to be ignored by Xen, then it should return + * to the caller which will increment the PC, so that the guest can + * execute the next instruction. + */ + if ( info.dabt_instr.state == INSTR_IGNORE ) + { + advance_pc(regs, hsr); + return; + } + + /* + * If Xen could not decode the instruction for any reason, then it + * should ask the caller to abort the guest. + */ + else if ( info.dabt_instr.state == INSTR_ERROR ) + goto inject_abt; + + /* + * 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; + + 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
When an instruction is trapped in Xen due to translation fault, Xen checks if the ISS is valid. If not, Xen tries to resolve the translation fault using p2m page tables. In case if it is a data abort, Xen will try to map the mmio region to the guest (ie tries to emulate the mmio region). If the ISS is not valid, then Xen tries to decode the instruction. In case of ioreq, Xen saves the decoding state, rn and imm9 to vcpu_io. Whenever the vcpu handles the ioreq successfully, it will read the decoding state to determine if the instruction decoded was a ldr/str post indexing (ie INSTR_LDR_STR_POSTINDEXING). If so, it uses these details to post increment rn. In case of mmio handler, if the mmio operation was successful, then Xen retrives the decoding state, rn and imm9. For state == INSTR_LDR_STR_POSTINDEXING, Xen will update rn. If there is an error encountered while decoding the instruction, Xen will issue an abort to the guest. If the instruction was related to cache maintenance, Xen will not decode the instruction or do any MMIO operation. Rather it simply increments the PC and returns to the guest. If the instruction was trapped due to stage1 page translation table walk, Xen will update the page tables and will return to the guest so that it can retry the instruction. To handle all these different states, we have introduced 'enum instr_decode_state'. Signed-off-by: Ayan Kumar Halder <ayankuma@xilinx.com> --- Changelog :- v2..v5 - Mentioned in the cover letter. v6 - 1. Mantained the decoding state of the instruction. This is used by the caller to either abort the guest or retry or ignore or perform read/write on the mmio region. 2. try_decode() invokes decoding for both aarch64 and thumb state. (Previously it used to invoke decoding only for aarch64 state). Thus, it handles all the checking of the registers before invoking any decoding of instruction. try_decode_instruction_invalid_iss() has thus been removed. 3. Introduced a new field('enum instr_decode_state state') inside 'struct instr_details'. This holds the decoding state of the instruction. This is later read by the post_increment_register() to determine if rn needs to be incremented. Also, this is read by the callers of try_decode_instruction() to determine if the instruction was valid or ignored or to be retried or error or decoded successfully. 4. Also stored 'instr_details' inside 'struct ioreq'. This enables arch_ioreq_complete_mmio() to invoke post_increment_register() without decoding the instruction again. 5. Check hsr.dabt.valid in do_trap_stage2_abort_guest(). If it is not valid, then decode the instruction. This ensures that try_handle_mmio() is invoked only when the instruction is either valid or decoded successfully. 6. Inside do_trap_stage2_abort_guest(), if hsr.dabt.valid is not set, then resolve the translation fault before trying to decode the instruction. If translation fault is resolved, then return to the guest to execute the instruction again. v7 - 1. Moved the decoding instruction details ie instr_details from 'struct ioreq' to 'struct vcpu_io'. 2. The instruction is decoded only when we get a data abort. 3. Replaced ASSERT_UNREACHABLE() with domain_crash(). The reason being asserts can be disabled in some builds. In this scenario when the guest's cpsr is in an erroneous state, Xen should crash the guest. 4. Introduced check_p2m() which invokes p2m_resolve_translation_fault() and try_map_mmio() to resolve translation fault by configuring the page tables. This gets invoked first if ISS is invalid and it is an instruction abort. If it is a data abort and hsr.dabt.s1ptw is set or try_handle_mmio() returns IO_UNHANDLED, then check_p2m() gets invoked again. xen/arch/arm/arm32/traps.c | 7 ++ xen/arch/arm/arm64/traps.c | 47 +++++++++++++ xen/arch/arm/decode.c | 1 + xen/arch/arm/decode.h | 3 +- xen/arch/arm/include/asm/domain.h | 4 ++ xen/arch/arm/include/asm/ioreq.h | 1 + xen/arch/arm/include/asm/mmio.h | 16 ++++- xen/arch/arm/include/asm/traps.h | 2 + xen/arch/arm/io.c | 108 ++++++++++++++++++++---------- xen/arch/arm/ioreq.c | 12 ++-- xen/arch/arm/traps.c | 85 ++++++++++++++++++++--- xen/arch/x86/include/asm/ioreq.h | 3 + xen/include/xen/sched.h | 2 + 13 files changed, 240 insertions(+), 51 deletions(-)