[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
On Thu, 07 Mar 2024 12:43, Michal Orzel <michal.orzel@xxxxxxx> wrote: On 07/03/2024 11:02, Manos Pitsidianakis wrote:Hi Michal, Alex, I'm responding to Michel but also giving my own review comments here. On Thu, 07 Mar 2024 10:40, Michal Orzel <michal.orzel@xxxxxxx> wrote: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 EncodingThis line is not neededI think it is needed for context (it answers the question "what is C4.1?" in the following line.+ * + * 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.Style manuals use either space (like here), a period (.) or colon (:), never a comma.Since it's a NIT, I'm not going to object. I just care about readability, we do not need to adhere to any "style manuals". I agree about readability :) the manuals mention was not an appeal to authority, just a sign of what is more common out there hence readable for more people. It is a nitpicking and subjective of course, so I'm not arguing for/against it, just sharing my 2 cents. + * + * 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 reservedI'm not sure how to read it. It is reserved provided op1 is also 0.Yes, it should say something like:Decode field values op0 = 0, op1 = 0 are reserved. Values op0 = 1, op1 = x1x0 correspond to Loads and Stores+ * op1 = x1x0 for Loads and Stores + * + * Section C4.1.88 Loads and StoresMissing colon at the end?It's a heading so a colon would not be appropriate.In this case why was it added before in: "C4.1 A64 instruction set encoding:" It should be removed from that, good point. Or at least put a colon here and in all headers for consistency. + * + * 31 28 27 26 25 24 23 22 21 16 15 12 11 10 9 0 + * ___________________________________________________________________ + * | op0 | 1 | op1 | 0 | op2 | | op3 | | op4 | | + * |________|___|_____|___|_____|__|__________|______|_____|__________| + *Maybe we should add the op{0,1,2,3,4} values for consistency?Values op0=xx11, op1=0, op2=0x, op3=0xxxxx, op4=01 correspond to Load/store register (immediate post-indexed)I think this should stay neutral in case we add a new emulation in a future. Do you mean for future Arm versions? decode.{c,h} should definitely be more future-proof... I think it's okay in this case only because the comment block starts with the source's name "ARMv8 ARM (DDI 0487J.a)". I don't object however to what you're saying, either is fine for me! + * 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.It is a reserved bit which can never be 0.Where did you take this information from? As I wrote above, I don't think we should bind this union to a single use case we currently have.The struct top_level should represent the generic encoding of A64 instruction. C4.1, page C4-400. op0 is only zero in the reserved (unallocated) case, for the generic encoding. ~Michal
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |