| Message ID | 20220202173017.48463-3-ayankuma@xilinx.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm64: io: Decode ldr/str post-indexing instruction | expand |
On Wed, 2 Feb 2022, Ayan Kumar Halder wrote: > For instructions on MMIO regions emulated by Xen, Xen reads the > remaining bits of the HSR. It determines if the instruction is to be > ignored, retried or decoded. If it gets an error while decoding the > instruction, then it sends an abort to the guest. > > If the instruction is valid or successfully decoded, Xen tries to > execute the instruction for the emulated MMIO region. If the instruction > was successfully executed, then Xen determines if the instruction needs > further processing. For eg:- In case of ldr/str post indexing on arm64, > the rn register needs to be updated. > > 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. > > xen/arch/arm/arm32/traps.c | 6 ++ > xen/arch/arm/arm64/traps.c | 41 ++++++++++++ > xen/arch/arm/decode.h | 12 +++- > xen/arch/arm/include/asm/traps.h | 2 + > xen/arch/arm/io.c | 108 +++++++++++++++++++++++++------ > 5 files changed, 148 insertions(+), 21 deletions(-) > > diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c > index 9c9790a6d1..6ad9a31499 100644 > --- a/xen/arch/arm/arm32/traps.c > +++ b/xen/arch/arm/arm32/traps.c > @@ -21,6 +21,7 @@ > > #include <public/xen.h> > > +#include <asm/mmio.h> > #include <asm/processor.h> > #include <asm/traps.h> > > @@ -82,6 +83,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) > +{ > + ASSERT_UNREACHABLE(); > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c > index 9113a15c7a..4de2206801 100644 > --- a/xen/arch/arm/arm64/traps.c > +++ b/xen/arch/arm/arm64/traps.c > @@ -18,9 +18,12 @@ > > #include <xen/lib.h> > > +#include <asm/current.h> > #include <asm/hsr.h> > +#include <asm/mmio.h> > #include <asm/system.h> > #include <asm/processor.h> > +#include <asm/regs.h> > > #include <public/xen.h> > > @@ -44,6 +47,44 @@ 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; > + > + /* > + * 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 > + ASSERT_UNREACHABLE(); > + } > + 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.h b/xen/arch/arm/decode.h > index fe7512a053..5efd72405e 100644 > --- a/xen/arch/arm/decode.h > +++ b/xen/arch/arm/decode.h > @@ -52,7 +52,17 @@ union instr { > #define POST_INDEX_FIXED_MASK 0x3B200C00 > #define POST_INDEX_FIXED_VALUE 0x38000400 > > -/* Decode an instruction from pc > +enum instr_decode_state > +{ > + INSTR_ERROR, /* Error encountered while decoding the instruction */ > + INSTR_VALID, /* ISS is valid, so there is no need to decode */ > + INSTR_SUCCESS, /* Instruction is decoded successfully */ > + INSTR_IGNORE, /* Instruction is to be ignored (similar to NOP) */ > + INSTR_RETRY /* Instruction is to be retried */ > +}; > + > +/* > + * 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/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..1011327058 100644 > --- a/xen/arch/arm/io.c > +++ b/xen/arch/arm/io.c > @@ -95,6 +95,59 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d, > return handler; > } > > +enum instr_decode_state 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"); > + return INSTR_ERROR; > + } It looks like we want a "return" here? But it should work either way because it should return with the if ( info->dabt.valid ) check right after anyway. > + } > + > + /* If ISS is valid, then no need to decode the instruction any further */ > + if (info->dabt.valid) > + return INSTR_VALID; code style > + /* > + * Xen should not decode the instruction when it was trapped due to > + * translation fault. > + */ > + if ( info->dabt.s1ptw ) > + return INSTR_RETRY; > + > + /* > + * If the fault occurred due to cache maintenance or address translation > + * instructions, then Xen needs to ignore these instructions. > + */ > + if ( info->dabt.cache ) > + return INSTR_IGNORE; > + > + /* > + * 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"); > + return INSTR_ERROR; > + } > + else > + return INSTR_SUCCESS; > +} > + > enum io_state try_handle_mmio(struct cpu_user_regs *regs, > const union hsr hsr, > paddr_t gpa) > @@ -106,14 +159,14 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs, > .gpa = gpa, > .dabt = dabt > }; > + int rc; > + enum instr_decode_state state; > > ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL); > > handler = find_mmio_handler(v->domain, info.gpa); > if ( !handler ) > { > - int rc; > - > rc = try_fwd_ioserv(regs, v, &info); > if ( rc == IO_HANDLED ) > return handle_ioserv(regs, v); > @@ -121,31 +174,46 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs, > return rc; > } > > - /* All the instructions used on emulated MMIO region should be valid */ > - if ( !dabt.valid ) > + state = try_decode_instruction(regs, &info); We still have the issue that try_fwd_ioserv (called above) doesn't work properly if !dabt.valid. I think we need to call try_decode_instruction and do the "state" checks before find_mmio_handler/try_fwd_ioserv. > + /* > + * 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 ( state == INSTR_IGNORE ) > + return IO_HANDLED; > + /* > + * If Xen could not decode the instruction for any reason, then it should > + * ask the caller to abort the guest. > + */ > + else if ( state == INSTR_ERROR ) > return IO_ABORT; > + /* When the instruction needs to be retried by the guest */ > + else if ( state == INSTR_RETRY ) > + return IO_UNHANDLED; > > /* > - * 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; > + if ( info.dabt.write ) > + rc = handle_write(handler, v, &info); > + else > + rc = handle_read(handler, v, &info); > > - rc = decode_instruction(regs, &info); > - if ( rc ) > - { > - gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); > - return IO_ABORT; > - } > + /* > + * 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. > + */ > + if ( (rc == IO_HANDLED) && (state == INSTR_SUCCESS) ) > + { > + post_increment_register(&info.dabt_instr); > } We need to call post_increment_register also from arch_ioreq_complete_mmio for the IOREQ case. > - if ( info.dabt.write ) > - return handle_write(handler, v, &info); > - else > - return handle_read(handler, v, &info); > + return rc; > } > > void register_mmio_handler(struct domain *d, > -- > 2.17.1 >
On Wed, 2 Feb 2022, Stefano Stabellini wrote: > On Wed, 2 Feb 2022, Ayan Kumar Halder wrote: > > For instructions on MMIO regions emulated by Xen, Xen reads the > > remaining bits of the HSR. It determines if the instruction is to be > > ignored, retried or decoded. If it gets an error while decoding the > > instruction, then it sends an abort to the guest. > > > > If the instruction is valid or successfully decoded, Xen tries to > > execute the instruction for the emulated MMIO region. If the instruction > > was successfully executed, then Xen determines if the instruction needs > > further processing. For eg:- In case of ldr/str post indexing on arm64, > > the rn register needs to be updated. > > > > 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. > > > > xen/arch/arm/arm32/traps.c | 6 ++ > > xen/arch/arm/arm64/traps.c | 41 ++++++++++++ > > xen/arch/arm/decode.h | 12 +++- > > xen/arch/arm/include/asm/traps.h | 2 + > > xen/arch/arm/io.c | 108 +++++++++++++++++++++++++------ > > 5 files changed, 148 insertions(+), 21 deletions(-) > > > > diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c > > index 9c9790a6d1..6ad9a31499 100644 > > --- a/xen/arch/arm/arm32/traps.c > > +++ b/xen/arch/arm/arm32/traps.c > > @@ -21,6 +21,7 @@ > > > > #include <public/xen.h> > > > > +#include <asm/mmio.h> > > #include <asm/processor.h> > > #include <asm/traps.h> > > > > @@ -82,6 +83,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) > > +{ > > + ASSERT_UNREACHABLE(); > > +} > > + > > /* > > * Local variables: > > * mode: C > > diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c > > index 9113a15c7a..4de2206801 100644 > > --- a/xen/arch/arm/arm64/traps.c > > +++ b/xen/arch/arm/arm64/traps.c > > @@ -18,9 +18,12 @@ > > > > #include <xen/lib.h> > > > > +#include <asm/current.h> > > #include <asm/hsr.h> > > +#include <asm/mmio.h> > > #include <asm/system.h> > > #include <asm/processor.h> > > +#include <asm/regs.h> > > > > #include <public/xen.h> > > > > @@ -44,6 +47,44 @@ 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; val needs to be initialized: traps.c: In function ‘post_increment_register’: traps.c:79:9: error: ‘val’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 79 | val += instr->imm9; | ~~~~^~~~~~~~~~~~~~ > > + /* > > + * 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 > > + ASSERT_UNREACHABLE(); > > + } > > + 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.h b/xen/arch/arm/decode.h > > index fe7512a053..5efd72405e 100644 > > --- a/xen/arch/arm/decode.h > > +++ b/xen/arch/arm/decode.h > > @@ -52,7 +52,17 @@ union instr { > > #define POST_INDEX_FIXED_MASK 0x3B200C00 > > #define POST_INDEX_FIXED_VALUE 0x38000400 > > > > -/* Decode an instruction from pc > > +enum instr_decode_state > > +{ > > + INSTR_ERROR, /* Error encountered while decoding the instruction */ > > + INSTR_VALID, /* ISS is valid, so there is no need to decode */ > > + INSTR_SUCCESS, /* Instruction is decoded successfully */ > > + INSTR_IGNORE, /* Instruction is to be ignored (similar to NOP) */ > > + INSTR_RETRY /* Instruction is to be retried */ > > +}; > > + > > +/* > > + * 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/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..1011327058 100644 > > --- a/xen/arch/arm/io.c > > +++ b/xen/arch/arm/io.c > > @@ -95,6 +95,59 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d, > > return handler; > > } > > > > +enum instr_decode_state 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"); > > + return INSTR_ERROR; > > + } > > It looks like we want a "return" here? But it should work either way > because it should return with the if ( info->dabt.valid ) check right > after anyway. > > > > + } > > + > > + /* If ISS is valid, then no need to decode the instruction any further */ > > + if (info->dabt.valid) > > + return INSTR_VALID; > > code style > > > > + /* > > + * Xen should not decode the instruction when it was trapped due to > > + * translation fault. > > + */ > > + if ( info->dabt.s1ptw ) > > + return INSTR_RETRY; > > + > > + /* > > + * If the fault occurred due to cache maintenance or address translation > > + * instructions, then Xen needs to ignore these instructions. > > + */ > > + if ( info->dabt.cache ) > > + return INSTR_IGNORE; > > + > > + /* > > + * 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"); > > + return INSTR_ERROR; > > + } > > + else > > + return INSTR_SUCCESS; > > +} > > + > > enum io_state try_handle_mmio(struct cpu_user_regs *regs, > > const union hsr hsr, > > paddr_t gpa) > > @@ -106,14 +159,14 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs, > > .gpa = gpa, > > .dabt = dabt > > }; > > + int rc; > > + enum instr_decode_state state; > > > > ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL); > > > > handler = find_mmio_handler(v->domain, info.gpa); > > if ( !handler ) > > { > > - int rc; > > - > > rc = try_fwd_ioserv(regs, v, &info); > > if ( rc == IO_HANDLED ) > > return handle_ioserv(regs, v); > > @@ -121,31 +174,46 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs, > > return rc; > > } > > > > - /* All the instructions used on emulated MMIO region should be valid */ > > - if ( !dabt.valid ) > > + state = try_decode_instruction(regs, &info); > > We still have the issue that try_fwd_ioserv (called above) doesn't work > properly if !dabt.valid. I think we need to call try_decode_instruction > and do the "state" checks before find_mmio_handler/try_fwd_ioserv. > > > > + /* > > + * 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 ( state == INSTR_IGNORE ) > > + return IO_HANDLED; > > + /* > > + * If Xen could not decode the instruction for any reason, then it should > > + * ask the caller to abort the guest. > > + */ > > + else if ( state == INSTR_ERROR ) > > return IO_ABORT; > > + /* When the instruction needs to be retried by the guest */ > > + else if ( state == INSTR_RETRY ) > > + return IO_UNHANDLED; > > > > /* > > - * 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; > > + if ( info.dabt.write ) > > + rc = handle_write(handler, v, &info); > > + else > > + rc = handle_read(handler, v, &info); > > > > - rc = decode_instruction(regs, &info); > > - if ( rc ) > > - { > > - gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); > > - return IO_ABORT; > > - } > > + /* > > + * 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. > > + */ > > + if ( (rc == IO_HANDLED) && (state == INSTR_SUCCESS) ) > > + { > > + post_increment_register(&info.dabt_instr); > > } > > We need to call post_increment_register also from arch_ioreq_complete_mmio for the IOREQ case. > > > > - if ( info.dabt.write ) > > - return handle_write(handler, v, &info); > > - else > > - return handle_read(handler, v, &info); > > + return rc; > > } > > > > void register_mmio_handler(struct domain *d, > > -- > > 2.17.1 > > >
diff --git a/xen/arch/arm/arm32/traps.c b/xen/arch/arm/arm32/traps.c index 9c9790a6d1..6ad9a31499 100644 --- a/xen/arch/arm/arm32/traps.c +++ b/xen/arch/arm/arm32/traps.c @@ -21,6 +21,7 @@ #include <public/xen.h> +#include <asm/mmio.h> #include <asm/processor.h> #include <asm/traps.h> @@ -82,6 +83,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) +{ + ASSERT_UNREACHABLE(); +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/arm64/traps.c b/xen/arch/arm/arm64/traps.c index 9113a15c7a..4de2206801 100644 --- a/xen/arch/arm/arm64/traps.c +++ b/xen/arch/arm/arm64/traps.c @@ -18,9 +18,12 @@ #include <xen/lib.h> +#include <asm/current.h> #include <asm/hsr.h> +#include <asm/mmio.h> #include <asm/system.h> #include <asm/processor.h> +#include <asm/regs.h> #include <public/xen.h> @@ -44,6 +47,44 @@ 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; + + /* + * 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 + ASSERT_UNREACHABLE(); + } + 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.h b/xen/arch/arm/decode.h index fe7512a053..5efd72405e 100644 --- a/xen/arch/arm/decode.h +++ b/xen/arch/arm/decode.h @@ -52,7 +52,17 @@ union instr { #define POST_INDEX_FIXED_MASK 0x3B200C00 #define POST_INDEX_FIXED_VALUE 0x38000400 -/* Decode an instruction from pc +enum instr_decode_state +{ + INSTR_ERROR, /* Error encountered while decoding the instruction */ + INSTR_VALID, /* ISS is valid, so there is no need to decode */ + INSTR_SUCCESS, /* Instruction is decoded successfully */ + INSTR_IGNORE, /* Instruction is to be ignored (similar to NOP) */ + INSTR_RETRY /* Instruction is to be retried */ +}; + +/* + * 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/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..1011327058 100644 --- a/xen/arch/arm/io.c +++ b/xen/arch/arm/io.c @@ -95,6 +95,59 @@ static const struct mmio_handler *find_mmio_handler(struct domain *d, return handler; } +enum instr_decode_state 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"); + return INSTR_ERROR; + } + } + + /* If ISS is valid, then no need to decode the instruction any further */ + if (info->dabt.valid) + return INSTR_VALID; + + /* + * Xen should not decode the instruction when it was trapped due to + * translation fault. + */ + if ( info->dabt.s1ptw ) + return INSTR_RETRY; + + /* + * If the fault occurred due to cache maintenance or address translation + * instructions, then Xen needs to ignore these instructions. + */ + if ( info->dabt.cache ) + return INSTR_IGNORE; + + /* + * 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"); + return INSTR_ERROR; + } + else + return INSTR_SUCCESS; +} + enum io_state try_handle_mmio(struct cpu_user_regs *regs, const union hsr hsr, paddr_t gpa) @@ -106,14 +159,14 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs, .gpa = gpa, .dabt = dabt }; + int rc; + enum instr_decode_state state; ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL); handler = find_mmio_handler(v->domain, info.gpa); if ( !handler ) { - int rc; - rc = try_fwd_ioserv(regs, v, &info); if ( rc == IO_HANDLED ) return handle_ioserv(regs, v); @@ -121,31 +174,46 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs, return rc; } - /* All the instructions used on emulated MMIO region should be valid */ - if ( !dabt.valid ) + 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 ( state == INSTR_IGNORE ) + return IO_HANDLED; + /* + * If Xen could not decode the instruction for any reason, then it should + * ask the caller to abort the guest. + */ + else if ( state == INSTR_ERROR ) return IO_ABORT; + /* When the instruction needs to be retried by the guest */ + else if ( state == INSTR_RETRY ) + return IO_UNHANDLED; /* - * 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; + if ( info.dabt.write ) + rc = handle_write(handler, v, &info); + else + rc = handle_read(handler, v, &info); - rc = decode_instruction(regs, &info); - if ( rc ) - { - gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); - return IO_ABORT; - } + /* + * 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. + */ + if ( (rc == IO_HANDLED) && (state == INSTR_SUCCESS) ) + { + post_increment_register(&info.dabt_instr); } - if ( info.dabt.write ) - return handle_write(handler, v, &info); - else - return handle_read(handler, v, &info); + return rc; } void register_mmio_handler(struct domain *d,
For instructions on MMIO regions emulated by Xen, Xen reads the remaining bits of the HSR. It determines if the instruction is to be ignored, retried or decoded. If it gets an error while decoding the instruction, then it sends an abort to the guest. If the instruction is valid or successfully decoded, Xen tries to execute the instruction for the emulated MMIO region. If the instruction was successfully executed, then Xen determines if the instruction needs further processing. For eg:- In case of ldr/str post indexing on arm64, the rn register needs to be updated. 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. xen/arch/arm/arm32/traps.c | 6 ++ xen/arch/arm/arm64/traps.c | 41 ++++++++++++ xen/arch/arm/decode.h | 12 +++- xen/arch/arm/include/asm/traps.h | 2 + xen/arch/arm/io.c | 108 +++++++++++++++++++++++++------ 5 files changed, 148 insertions(+), 21 deletions(-)