[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [XEN v2] xen/arm64: io: Decode 32-bit ldr/str post-indexing instructions
On 29.11.2021 20:16, 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, Xilinx baremetal OS will use: > > volatile u32 *LocalAddr = (volatile u32 *)Addr; > *LocalAddr = Value; > > This leave the compiler to decide which store instructions to use. This > may be a post-index store instruction where the HW will not provide a > valid syndrome. > > In order to handle post-indexing store/load instructions, 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. > > Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxxxxx> Just a couple of general remarks, with no judgement towards its use in the codebase, and leaving out several obvious style issues. > 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. > > This has been tested for the following cases :- > ldr x2, [x1], #4 DYM "ldr w2, [x1], #4" or "ldr x2, [x1], #8" here? > ldr w2, [x1], #-4 > > str x2, [x1], #4 Similar aspect here. > str w2, [x1], #-4 > > The reason being I am testing on 32bit MMIO registers only. I don't see a > 8bit > or 16bit MMIO register. As per this, perhaps the former of the two. > --- a/xen/arch/arm/decode.c > +++ b/xen/arch/arm/decode.c > @@ -84,6 +84,66 @@ bad_thumb2: > return 1; > } > > +static int decode_32bit_loadstore_postindexing(register_t pc, > + struct hsr_dabt *dabt, > + union ldr_str_instr_class > *instr) > +{ > + if ( raw_copy_from_guest(&instr->value, (void * __user)pc, sizeof > (instr)) ) > + return -EFAULT; > + > + /* First, let's check for the fixed values */ > + if ( !((instr->code.fixed1 == 1) && (instr->code.fixed2 == 0) && > + (instr->code.fixed3 == 0) && (instr->code.fixed4 == 7)) ) > + { > + gprintk(XENLOG_ERR, "Decoding not supported for instructions other > than" > + " ldr/str post indexing\n"); > + goto bad_32bit_loadstore; > + } > + > + if ( instr->code.size != 2 ) > + { > + gprintk(XENLOG_ERR, > + "ldr/str post indexing is supported for 32 bit variant only\n"); > + goto bad_32bit_loadstore; > + } > + > + if ( instr->code.v != 0 ) > + { > + gprintk(XENLOG_ERR, > + "ldr/str post indexing for vector types are not supported\n"); > + goto bad_32bit_loadstore; > + } > + > + /* Check for STR (immediate) - 32 bit variant */ > + if ( instr->code.opc == 0 ) > + { > + dabt->write = 1; > + } > + /* Check for LDR (immediate) - 32 bit variant */ > + 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_32bit_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_32bit_loadstore: Please indent labels by at least a blank. I also think this would benefit from a preceding blank line. > --- a/xen/arch/arm/io.c > +++ b/xen/arch/arm/io.c > @@ -65,6 +65,16 @@ static enum io_state handle_write(const struct > mmio_handler *handler, > return ret ? IO_HANDLED : IO_ABORT; > } > > +static void post_incremenet_register(union ldr_str_instr_class *instr) I think you mean post_increment_register()? > +{ > + struct cpu_user_regs *regs = guest_cpu_user_regs(); > + unsigned int val; > + > + val = get_user_reg(regs, instr->code.rn); > + val += instr->code.imm9; > + set_user_reg(regs, instr->code.rn, val); I don't think this handles the SP case correctly, and I also don't see that case getting rejected elsewhere. > --- a/xen/include/asm-arm/hsr.h > +++ b/xen/include/asm-arm/hsr.h > @@ -15,6 +15,32 @@ enum dabt_size { > DABT_DOUBLE_WORD = 3, > }; > > +/* > + * 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 */ > + int imm9:9; /* imm9 */ Plain int bitfields are, iirc, signed or unsigned at the compiler's discretion. Hence I think you mean explicitly "signed int" here. > + 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; > +}; I'd recommend types needed in just one CU to live there, rather than getting exposed to every source file including this header (even more so when - aiui - this is entirely unrelated to HSR). When used in just a single function, it might even want to live here (i.e. as close as possible to the [only] use). Jan
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |