[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [RFC PATCH] Added the logic to decode 32 bit ldr/str post-indexing instructions
Hi Ayan, > -----Original Message----- > From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Ayan > Kumar Halder > Sent: 2021年11月20日 0:52 > To: xen-devel@xxxxxxxxxxxxxxxxxxxx > Cc: sstabellini@xxxxxxxxxx; stefano.stabellini@xxxxxxxxxx; julien@xxxxxxx; > Volodymyr_Babchuk@xxxxxxxx; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; > Rahul Singh <Rahul.Singh@xxxxxxx>; ayankuma@xxxxxxxxxx > Subject: [RFC PATCH] Added the logic to decode 32 bit ldr/str post- > indexing instructions > > At present, post indexing instructions are not emulated by Xen. > When Xen gets the exception, EL2_ESR.ISV bit not set. Thus as a > result, data abort is triggered. > > Added the logic to decode ldr/str post indexing instructions. > With this, Xen can decode instructions like these:- > ldr w2, [x1], #4 > Thus, domU can read ioreg with post indexing instructions. > > Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxxxxx> > --- > Note to reviewer:- > This patch is based on an issue discussed in > https://lists.xenproject.org/archives/html/xen-devel/2021-11/msg00969.html > "Xen/ARM - Query about a data abort seen while reading GICD registers" > > > xen/arch/arm/decode.c | 77 +++++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/io.c | 14 ++++++-- > 2 files changed, 88 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c > index 792c2e92a7..7b60bedbc5 100644 > --- a/xen/arch/arm/decode.c > +++ b/xen/arch/arm/decode.c > @@ -84,6 +84,80 @@ bad_thumb2: > return 1; > } > > +static inline int32_t extract32(uint32_t value, int start, int length) > +{ > + int32_t ret; > + > + if ( !(start >= 0 && length > 0 && length <= 32 - start) ) > + return -EINVAL; > + > + ret = (value >> start) & (~0U >> (32 - length)); > + > + return ret; > +} > + This function's behavior is very similar to the helps vreg_regx_extra in vreg.h. If we will introduce more reg bit operation functions like extract32. Can we consider to reuse them? > +static int decode_64bit_loadstore_postindexing(register_t pc, struct > hsr_dabt *dabt) > +{ > + uint32_t instr; > + int size; > + int v; > + int opc; > + int rt; > + int imm9; > + > + /* For details on decoding, refer to Armv8 Architecture reference > manual > + * Section - "Load/store register (immediate post-indexed)", Pg 318 > + */ I have seen two styles of multiple line comments in this patch. I checked the original file and it has no special comment. So I think you may need to follow the multiple line comments you have done for arm/io.c in this patch. And the same for some comments below. > + if ( raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr)) ) > + return -EFAULT; > + > + /* First, let's check for the fixed values */ > + > + /* As per the "Encoding table for the Loads and Stores group", Pg > 299 > + * op4 = 1 - Load/store register (immediate post-indexed) > + */ > + if ( extract32(instr, 10, 2) != 1 ) > + goto bad_64bit_loadstore; > + > + /* For the following, refer to "Load/store register (immediate post- > indexed)" > + * to get the fixed values at various bit positions. > + */ > + if ( extract32(instr, 21, 1) != 0 ) > + goto bad_64bit_loadstore; > + > + if ( extract32(instr, 24, 2) != 0 ) > + goto bad_64bit_loadstore; > + > + if ( extract32(instr, 27, 3) != 7 ) > + goto bad_64bit_loadstore; > + If the check is fixed, why we don't define a VALID_MASK to check it, something like: if ( instr & MASK_for_21_24_27 == VALID_FOR_21_24_27 ) > + size = extract32(instr, 30, 2); > + v = extract32(instr, 26, 1); > + opc = extract32(instr, 22, 1); > + > + /* At the moment, we support STR(immediate) - 32 bit variant and > + * LDR(immediate) - 32 bit variant only. > + */ > + if (!((size==2) && (v==0) && ((opc==0) || (opc==1)))) > + goto bad_64bit_loadstore; > + > + rt = extract32(instr, 0, 5); > + imm9 = extract32(instr, 12, 9); > + > + if ( imm9 < 0 ) > + update_dabt(dabt, rt, size, true); > + else > + update_dabt(dabt, rt, size, false); > + > + dabt->valid = 1; > + > + > + return 0; > +bad_64bit_loadstore: > + gprintk(XENLOG_ERR, "unhandled 64bit instruction 0x%x\n", instr); > + return 1; > +} > + > static int decode_thumb(register_t pc, struct hsr_dabt *dabt) > { > uint16_t instr; > @@ -155,6 +229,9 @@ int decode_instruction(const struct cpu_user_regs > *regs, struct hsr_dabt *dabt) > if ( is_32bit_domain(current->domain) && regs->cpsr & PSR_THUMB ) > return decode_thumb(regs->pc, dabt); > > + if ( is_64bit_domain(current->domain) ) > + return decode_64bit_loadstore_postindexing(regs->pc, dabt); > + > /* TODO: Handle ARM instruction */ > gprintk(XENLOG_ERR, "unhandled ARM instruction\n"); > > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c > index 729287e37c..49e80358c0 100644 > --- a/xen/arch/arm/io.c > +++ b/xen/arch/arm/io.c > @@ -106,14 +106,13 @@ enum io_state try_handle_mmio(struct cpu_user_regs > *regs, > .gpa = gpa, > .dabt = dabt > }; > + int rc; > > 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); > @@ -123,7 +122,16 @@ enum io_state try_handle_mmio(struct cpu_user_regs > *regs, > > /* All the instructions used on emulated MMIO region should be valid > */ > if ( !dabt.valid ) > - return IO_ABORT; > + { > + /* > + * Post indexing ldr/str instructions are not emulated by Xen. So, > the > + * ISS is invalid. In such a scenario, we try to manually decode > the > + * instruction from the program counter. > + */ > + rc = decode_instruction(regs, &info.dabt); > + if ( rc ) > + return IO_ABORT; > + } > > /* > * Erratum 766422: Thumb store translation fault to Hypervisor may > -- > 2.17.1 >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |