[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [RFC PATCH v2] xen/arm: improve handling of load/store instruction decoding
Hi Alex, NIT: RFC tag is no longer needed. On 06/03/2024 17:56, Alex Bennée wrote: > > > While debugging VirtIO on Arm we ran into a warning due to memory > being memcpy'd across MMIO space. While the bug was in the mappings > the warning was a little confusing: > > (XEN) d47v2 Rn should not be equal to Rt except for r31 > (XEN) d47v2 unhandled Arm instruction 0x3d800000 > (XEN) d47v2 Unable to decode instruction > > The Rn == Rt warning is only applicable to single register load/stores > so add some verification steps before to weed out unexpected accesses. > > While at it update the Arm ARM references to the latest version of the > documentation. > > Signed-off-by: Alex Bennée <alex.bennee@xxxxxxxxxx> > Cc: Manos Pitsidianakis <manos.pitsidianakis@xxxxxxxxxx> Move the CC line after --- so that it's not included in the final commit msg. > > --- > v2 > - use single line comments where applicable > - update Arm ARM references > - use #defines for magic numbers > --- > xen/arch/arm/decode.c | 35 ++++++++++++++++++++------ > xen/arch/arm/decode.h | 57 ++++++++++++++++++++++++++++++++++++++----- > 2 files changed, 79 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c > index 2537dbebc1..73a88e4701 100644 > --- a/xen/arch/arm/decode.c > +++ b/xen/arch/arm/decode.c > @@ -87,15 +87,36 @@ static int decode_arm64(register_t pc, mmio_info_t *info) > return 1; > } > > + /* Check this is a load/store of some sort */ > + if ( (opcode.top_level.op1 & TL_LDST_OP1_MASK) != TL_LDST_OP1_VALUE ) > + { > + gprintk(XENLOG_ERR, "Not a load/store instruction op1=%u\n", > + opcode.top_level.op1); > + goto bad_loadstore; > + } > + > + /* We are only expecting single register load/stores */ > + if ( (opcode.ld_st.op0 & LS_SREG_OP0_MASK) != LS_SREG_OP0_VALUE ) > + { > + gprintk(XENLOG_ERR, "Not single register load/store op0=%u\n", NIT: missing 'a' between Not and single > + opcode.ld_st.op0); > + goto bad_loadstore; > + } > + > /* > - * Refer Arm v8 ARM DDI 0487G.b, Page - C6-1107 > - * "Shared decode for all encodings" (under ldr immediate) > - * If n == t && n != 31, then the return value is implementation defined > - * (can be WBSUPPRESS, UNKNOWN, UNDEFINED or NOP). Thus, we do not > support > - * this. This holds true for ldrb/ldrh immediate as well. > + * Refer Arm v8 ARM DDI 0487J.a, Page - K1-12586 > + * > + * STR (immediate) CONSTRAINED UNPREDICTABLE behaviour > + * > + * "If the instruction encoding specifies pre-indexed addressing or > + * post-indexed addressing, and n == t && n != 31, then one of the > + * following behaviors must occur:" UNDEFINED, NOP or UNKNOWN > + * > + * Execution @ EL0/EL1 when HCR_EL2.TIDCP is 1 traps to EL2 with > + * EC = 0. > * > - * Also refer, Page - C6-1384, the above described behaviour is same for > - * str immediate. This holds true for strb/strh immediate as well > + * This also hold true for LDR (immediate), Page K1-12581 and > + * the RB/RH variants of both. > */ > if ( (opcode.ldr_str.rn == opcode.ldr_str.rt) && (opcode.ldr_str.rn != > 31) ) > { > diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h > index 13db8ac968..188114a71e 100644 > --- a/xen/arch/arm/decode.h > +++ b/xen/arch/arm/decode.h > @@ -24,17 +24,54 @@ > #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)". > + * Refer to the ARMv8 ARM (DDI 0487J.a) > * > - * 31 30 29 27 26 25 23 21 20 11 9 4 0 > + * Section C A64 Instruct Set Encoding This line is not needed > + * > + * C4.1 A64 instruction set encoding: NIT: I would put a comma after section number i.e. C4.1, A64 ... The same would apply in other places. > + * > + * 31 30 29 28 25 24 0 > * ___________________________________________________________________ > - * |size|1 1 1 |V |0 0 |opc |0 | imm9 |0 1 | Rn | Rt | > - * |____|______|__|____|____|__|_______________|____|_________|_______| > + * |op0 | x x | op1 | | > + * |____|______|______|_______________________________________________| > + * > + * op0 = 0 is reserved I'm not sure how to read it. It is reserved provided op1 is also 0. > + * op1 = x1x0 for Loads and Stores > + * > + * Section C4.1.88 Loads and Stores Missing colon at the end? > + * > + * 31 28 27 26 25 24 23 22 21 16 15 12 11 10 9 0 > + * ___________________________________________________________________ > + * | op0 | 1 | op1 | 0 | op2 | | op3 | | op4 | | > + * |________|___|_____|___|_____|__|__________|______|_____|__________| > + * > + * Page C4-653 Load/store register (immediate post-indexed) > + * > + * 31 30 29 27 26 25 24 23 22 21 20 12 11 10 9 5 4 0 > + * ___________________________________________________________________ > + * |size|1 1 1 |V | 0 0 | opc |0 | imm9 | 0 1 | Rn | Rt | > + * |____|______|__|_____|_____|__|_______________|_____|______|_______| > */ > union instr { > uint32_t value; > + struct { > + unsigned int ign2:25; Here, your numeration of ignore fields is in descending order (starting from lsb) but .., > + unsigned int op1:4; /* instruction class */ > + unsigned int ign1:2; > + unsigned int op0:1; /* value = 1b */ Why op0 = 0b1 ? This structure represents the generic bit layout (the emulation deals with single ldr/str). I would drop this comment. > + } top_level; > + struct { > + unsigned int ign1:10; here in ascending. Let's be consistent (fixed fields are in ascending order). > + unsigned int op4:2; > + unsigned int ign2:4; > + unsigned int op3:6; > + unsigned int ign3:1; > + unsigned int op2:2; > + unsigned int fixed1:1; /* value = 0b */ > + unsigned int op1:1; > + unsigned int fixed2:1; /* value = 1b */ > + unsigned int op0:4; > + } ld_st; > struct { > unsigned int rt:5; /* Rt register */ > unsigned int rn:5; /* Rn register */ > @@ -49,6 +86,14 @@ union instr { > } ldr_str; > }; > > +/* Top level load/store encoding */ > +#define TL_LDST_OP1_MASK 0b0101 > +#define TL_LDST_OP1_VALUE 0b0100 > + > +/* Load/store single reg encoding */ > +#define LS_SREG_OP0_MASK 0b0011 > +#define LS_SREG_OP0_VALUE 0b0011 > + > #define POST_INDEX_FIXED_MASK 0x3B200C00 > #define POST_INDEX_FIXED_VALUE 0x38000400 > > -- > 2.39.2 > > ~Michal
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |