[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, 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@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; 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 > > >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |