| Message ID | 20240306165621.3819343-1-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v2] xen/arm: improve handling of load/store instruction decoding | expand |
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@linaro.org> > Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> 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
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@amd.com> 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@linaro.org> >> Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> >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 I 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. > >> + * >> + * 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. 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 Stores >Missing colon at the end? It's a heading so a colon would not be appropriate. > >> + * >> + * 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) >> + * 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. > >> + } top_level; >> + struct { >> + unsigned int ign1:10; >here in ascending. Let's be consistent (fixed fields are in ascending order). Agreed, otherwise names like ign2, fixed1 are not helpful. >> + 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 >> >> >
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@amd.com> 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@linaro.org> >>> Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> >> 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 > > I 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". > >> >>> + * >>> + * 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. > > 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 Stores >> Missing 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:" > >> >>> + * >>> + * 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. > >>> + * 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. ~Michal
On Thu, 07 Mar 2024 12:43, Michal Orzel <michal.orzel@amd.com> 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@amd.com> 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@linaro.org> >>>> Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> >>> 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 >> >> I 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 reserved >>> I'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 Stores >>> Missing 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
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", + 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 + * + * C4.1 A64 instruction set encoding: + * + * 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 + * op1 = x1x0 for Loads and Stores + * + * Section C4.1.88 Loads and Stores + * + * 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; + unsigned int op1:4; /* instruction class */ + unsigned int ign1:2; + unsigned int op0:1; /* value = 1b */ + } top_level; + struct { + unsigned int ign1:10; + 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
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@linaro.org> Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> --- 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(-)