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.or...@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.ben...@linaro.org>
>>> Cc: Manos Pitsidianakis <manos.pitsidiana...@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
