[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v4] xen/arm64: io: Decode ldr/str post-indexing instructions
On Tue, 25 Jan 2022, Ayan Kumar Halder wrote: > At the moment, Xen is only handling data abort with valid syndrome (i.e. > ISV=0). Unfortunately, this doesn't cover all the instructions a domain > could use to access MMIO regions. > > For instance, a baremetal OS can use any of the following instructions, where > x1 contains the address of the MMIO region: > > 1. ldr x2, [x1], #8 > 2. ldr w2, [x1], #-4 > 3. ldr x2, [x1], #-8 > 4. ldr w2, [x1], #4 > 5. ldrh w2, [x1], #2 > 6. ldrb w2, [x1], #1 > 7. str x2, [x1], #8 > 8. str w2, [x1], #-4 > 9. strh w2, [x1], #2 > 10. strb w2, [x1], #1 > > In the following two instructions, Rn could theoretically be stack pointer > which > might contain the address of the MMIO region:- > 11. ldrb w2, [Rn], #1 > 12. ldrb wzr, [Rn], #1 > > In order to handle post-indexing store/load instructions (like those mentioned > above), Xen will need to fetch and decode the instruction. > > This patch only cover post-index store/load instructions from AArch64 mode. > For now, this is left unimplemented for trap from AArch32 mode. NIT: "For now, AArch32 mode is left unimplemented." > Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxxxxx> > --- > > Changelog :- > v2 - 1. Updated the rn register after reading from it. (Pointed by Julien, > Stefano) > 2. Used a union to represent the instruction opcode (Suggestd by > Bertrand) > 3. Fixed coding style issues (Pointed by Julien) > 4. In the previous patch, I was updating dabt->sign based on the > signedness > of imm9. This was incorrect. As mentioned in ARMv8 ARM DDI 0487G.b, > Page 3221, SSE indicates the signedness of the data item loaded. In > our > case, the data item loaded is always unsigned. > > v3- 1. Handled all the variants of ldr/str (ie 64, 32, 16, 8 bit variants). > Thus, I have removed the check for "instr->code.opc == 0" (Suggested by > Andre) > 2. Handled the scenario when rn = SP, rt = XZR (Suggested by Jan, Andre) > 3. Added restriction for "rt != rn" (Suggested by Andre) > 4. Moved union ldr_str_instr_class {} to decode.h. This is the header > included > by io.c and decode.c (where the union is referred). (Suggested by Jan) > 5. Indentation and typo fixes (Suggested by Jan) > > v4- 1. Fixed the patch as per Stefano's comments on v3. They are as follows :- > 1.1 Use macros to determine the fixed values in the instruction opcode > 1.2 Checked if instr != NULL > 1.3 Changed some data types and added #define ARM_64 for AArch64 > specific > code > 1.4 Moved post_increment_register() to decode.c so that the decoding > logic is confined to a single file. > 1.5 Moved some checks from post_increment_register() to > decode_loadstore_postindexing() > 1.6 Removed a duplicate check > 2. Updated the commit message as per Andre's comments. > 3. Changed the names of a label and some comments. *32bit* was erroneously > mentioned in a label and comments in decode_loadstore_postindexing() > although the function handled all variants of ldr/str post indexing. > > xen/arch/arm/decode.c | 124 +++++++++++++++++++++++++++++++++++++++++- > xen/arch/arm/decode.h | 41 +++++++++++++- > xen/arch/arm/io.c | 41 +++++++++++--- > 3 files changed, 195 insertions(+), 11 deletions(-) > > diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c > index 792c2e92a7..0c12af7afa 100644 > --- a/xen/arch/arm/decode.c > +++ b/xen/arch/arm/decode.c > @@ -84,6 +84,101 @@ bad_thumb2: > return 1; > } > > +static int decode_loadstore_postindexing(register_t pc, > + struct hsr_dabt *dabt, > + union ldr_str_instr_class *instr) > +{ > + struct cpu_user_regs *regs = guest_cpu_user_regs(); > + > + if ( instr == NULL ) > + { > + gprintk(XENLOG_ERR, "instr should not be NULL\n"); > + return -EINVAL; > + } > + > + if ( raw_copy_from_guest(&instr->value, (void * __user)pc, sizeof > (instr)) ) > + { > + gprintk(XENLOG_ERR, "Could not copy the instruction from PC\n"); > + return -EFAULT; > + } > + > + /* > + * Rn -ne Rt for ldr/str instruction. > + * Check https://developer.arm.com/documentation/dui0802/a/CIHGJHED > + * (Register restrictions) > + * > + * The only exception for this is when rn = 31. It denotes SP ("Use of > SP") > + * > + * And when rt = 31, it denotes wzr/xzr. (Refer > + * > https://developer.arm.com/documentation/den0024/a/ARMv8-Registers/AArch64-special-registers > + * "There is no register called X31 or W31. Many instructions are encoded > + * such that the number 31 represents the zero register, ZR (WZR/XZR)." > + */ > + if ( (instr->code.rn == instr->code.rt) && (instr->code.rn != 31) ) > + { > + gprintk(XENLOG_ERR, "Rn should not be equal to Rt except for r31\n"); > + return -EINVAL; > + } > + > + /* First, let's check for the fixed values */ > + if ( (instr->value & POST_INDEX_FIXED_MASK) != POST_INDEX_FIXED_VALUE ) > + { > + gprintk(XENLOG_ERR, "Cannot decode instruction 0x%x",instr->value); > + gprintk(XENLOG_ERR, "Decoding not supported for instructions other > than" > + " ldr/str post indexing\n"); > + goto bad_loadstore; > + } > + > + /* > + * Handle when rn = SP > + * Refer ArmV8 ARM DDI 0487G.b, Page - D1-2463 "Stack pointer register > selection" > + * As we are interested in handling exceptions only from EL1 in AArch64 > state, > + * thus M[3:0] == EL1h (Page - C5-480 "When exception taken from AArch64 > state:") > + */ > + if ( (instr->code.rn == 31) && ((regs->cpsr & PSR_MODE_MASK) != > PSR_MODE_EL1h) ) > + { > + gprintk(XENLOG_ERR, "SP is valid only for EL1h\n"); > + goto bad_loadstore; > + } > + > + if ( instr->code.v != 0 ) > + { > + gprintk(XENLOG_ERR, > + "ldr/str post indexing for vector types are not supported\n"); > + goto bad_loadstore; > + } > + > + /* Check for STR (immediate) */ > + if ( instr->code.opc == 0 ) > + { > + dabt->write = 1; > + } > + /* Check for LDR (immediate) */ > + else if ( instr->code.opc == 1 ) > + { > + dabt->write = 0; > + } > + else > + { > + gprintk(XENLOG_ERR, > + "Decoding ldr/str post indexing is not supported for this > variant\n"); > + goto bad_loadstore; > + } > + > + gprintk(XENLOG_INFO, > + "instr->code.rt = 0x%x, instr->code.size = 0x%x, instr->code.imm9 = > %d\n", > + instr->code.rt, instr->code.size, instr->code.imm9); > + > + update_dabt(dabt, instr->code.rt, instr->code.size, false); > + dabt->valid = 1; > + > + return 0; > + > + bad_loadstore: > + gprintk(XENLOG_ERR, "unhandled Arm instruction 0x%x\n", instr->value); > + return 1; > +} > + > static int decode_thumb(register_t pc, struct hsr_dabt *dabt) > { > uint16_t instr; > @@ -150,17 +245,44 @@ bad_thumb: > return 1; > } > > -int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt > *dabt) > +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt > *dabt, > + union ldr_str_instr_class *instr) > { > if ( is_32bit_domain(current->domain) && regs->cpsr & PSR_THUMB ) > return decode_thumb(regs->pc, dabt); > > + if ( (is_64bit_domain(current->domain) && !psr_mode_is_32bit(regs)) ) > + { > + return decode_loadstore_postindexing(regs->pc, dabt, instr); > + } > + > /* TODO: Handle ARM instruction */ > gprintk(XENLOG_ERR, "unhandled ARM instruction\n"); > > return 1; > } > > +#if CONFIG_ARM_64 > +void post_increment_register(union ldr_str_instr_class *instr) > +{ > + struct cpu_user_regs *regs = guest_cpu_user_regs(); > + register_t val; > + > + /* handle when rn = SP */ > + if ( instr->code.rn == 31 ) > + val = regs->sp_el1; > + else > + val = get_user_reg(regs, instr->code.rn); > + > + val += instr->code.imm9; > + > + if ( instr->code.rn == 31 ) > + regs->sp_el1 = val; > + else > + set_user_reg(regs, instr->code.rn, val); > +} > +#endif > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h > index 4613763bdb..511cd4a05f 100644 > --- a/xen/arch/arm/decode.h > +++ b/xen/arch/arm/decode.h > @@ -23,6 +23,35 @@ > #include <asm/regs.h> > #include <asm/processor.h> > > +/* > + * Refer to the ARMv8 ARM (DDI 0487G.b), Section C4.1.4 Loads and Stores > + * Page 318 specifies the following bit pattern for > + * "load/store register (immediate post-indexed)". > + * > + * 31 30 29 27 26 25 23 21 20 11 9 4 0 > + * ___________________________________________________________________ > + * |size|1 1 1 |V |0 0 |opc |0 | imm9 |0 1 | Rn | Rt | > + * |____|______|__|____|____|__|_______________|____|_________|_______| > + */ > +union ldr_str_instr_class { > + uint32_t value; > + struct ldr_str { > + unsigned int rt:5; /* Rt register */ > + unsigned int rn:5; /* Rn register */ > + unsigned int fixed1:2; /* value == 01b */ > + signed int imm9:9; /* imm9 */ > + unsigned int fixed2:1; /* value == 0b */ > + unsigned int opc:2; /* opc */ > + unsigned int fixed3:2; /* value == 00b */ > + unsigned int v:1; /* vector */ > + unsigned int fixed4:3; /* value == 111b */ > + unsigned int size:2; /* size */ > + } code; > +}; > + > +#define POST_INDEX_FIXED_MASK 0x3B200C00 > +#define POST_INDEX_FIXED_VALUE 0x38000400 > + > /** > * Decode an instruction from pc > * /!\ This function is not intended to fully decode an instruction. It > @@ -35,8 +64,18 @@ > */ > > int decode_instruction(const struct cpu_user_regs *regs, > - struct hsr_dabt *dabt); > + struct hsr_dabt *dabt, > + union ldr_str_instr_class *instr); > > +/** NIT: the Xen coding style only has /*, not /** In any case aside from these two minor NITs that can be fixed on commit: Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx> > + * Update the register value for Rn > + * /!\ This function is used to update the register value for Rn when a > + * post indexing ldr/str instruction is decoded. > + * > + * This function will get: > + * - The post indexing ldr/str instruction opcode > + */ > +void post_increment_register(union ldr_str_instr_class *instr); > #endif /* __ARCH_ARM_DECODE_H_ */ > > /* > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c > index 729287e37c..b9c15e1fe7 100644 > --- a/xen/arch/arm/io.c > +++ b/xen/arch/arm/io.c > @@ -106,14 +106,29 @@ enum io_state try_handle_mmio(struct cpu_user_regs > *regs, > .gpa = gpa, > .dabt = dabt > }; > + int rc; > + union ldr_str_instr_class instr = {0}; > > ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL); > > + /* > + * Armv8 processor does not provide a valid syndrome for post-indexing > + * ldr/str instructions. So in order to process these instructions, > + * Xen must decode them. > + */ > + if ( !info.dabt.valid ) > + { > + rc = decode_instruction(regs, &info.dabt, &instr); > + if ( rc ) > + { > + gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); > + return IO_ABORT; > + } > + } > + > 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,10 +136,6 @@ 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 ) > - return IO_ABORT; > - > /* > * Erratum 766422: Thumb store translation fault to Hypervisor may > * not have correct HSR Rt value. > @@ -134,7 +145,7 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs, > { > int rc; > > - rc = decode_instruction(regs, &info.dabt); > + rc = decode_instruction(regs, &info.dabt, NULL); > if ( rc ) > { > gprintk(XENLOG_DEBUG, "Unable to decode instruction\n"); > @@ -143,9 +154,21 @@ enum io_state try_handle_mmio(struct cpu_user_regs *regs, > } > > if ( info.dabt.write ) > - return handle_write(handler, v, &info); > + rc = handle_write(handler, v, &info); > else > - return handle_read(handler, v, &info); > + rc = handle_read(handler, v, &info); > + > +#if CONFIG_ARM_64 > + if ( (is_64bit_domain(current->domain) && !psr_mode_is_32bit(regs)) ) > + { > + if ( instr.value != 0 ) > + { > + post_increment_register(&instr); > + } > + } > +#endif > + > + return rc; > } > > void register_mmio_handler(struct domain *d, > -- > 2.17.1 >
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |