Re: [RFC PATCH v2] xen/arm: improve handling of load/store instruction decoding

From: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
To: "Michal Orzel" <michal.orzel@amd.com>,
	"Alex Benné e" <alex.bennee@linaro.org>,
	xen-devel@lists.xenproject.org
Cc: julien@xen.org, sstabellini@kernel.org, bertrand.marquis@arm.com,
	Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
Subject: Re: [RFC PATCH v2] xen/arm: improve handling of load/store instruction decoding
Date: Thu, 07 Mar 2024 12:02:25 +0200	[thread overview]
Message-ID: <9z3mh.tiey2z2itr9a@linaro.org> (raw)
In-Reply-To: <3cb1b056-59a7-4ffe-856d-e45aac1936a9@amd.com>

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
>> 
>> 
>