xen/arch/arm/decode.c | 35 ++++++++++++++++++++------ xen/arch/arm/decode.h | 57 ++++++++++++++++++++++++++++++++++++++----- 2 files changed, 79 insertions(+), 13 deletions(-)
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(-)
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
--
2.39.2
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
© 2016 - 2026 Red Hat, Inc.