[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v6 2/3] xen/arm64: io: Support instructions (for which ISS is not valid) on emulated MMIO region using MMIO handler
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@xxxxxxxxxx> > --- > > 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 >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |