Re: [RFC PATCH] Added the logic to decode 32 bit ldr/str post-indexing instructions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2021 02:41, Stefano Stabellini wrote:
> On Fri, 19 Nov 2021, Ayan Kumar Halder wrote:
>> +static int decode_64bit_loadstore_postindexing(register_t pc, struct 
>> hsr_dabt *dabt)
>> +{
>> +    uint32_t instr;
>> +    int size;
>> +    int v;
>> +    int opc;
>> +    int rt;
>> +    int imm9;
>> +
>> +    /* For details on decoding, refer to Armv8 Architecture reference manual
>> +     * Section - "Load/store register (immediate post-indexed)", Pg 318
>> +    */
>> +    if ( raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr)) )
>> +        return -EFAULT;
>> +
>> +    /* First, let's check for the fixed values */
>> +
>> +    /*  As per the "Encoding table for the Loads and Stores group", Pg 299
>> +     * op4 = 1 - Load/store register (immediate post-indexed)
>> +     */
>> +    if ( extract32(instr, 10, 2) != 1 )
>> +        goto bad_64bit_loadstore;
>> +
>> +    /* For the following, refer to "Load/store register (immediate 
>> post-indexed)"
>> +     * to get the fixed values at various bit positions.
>> +     */
>> +    if ( extract32(instr, 21, 1) != 0 )
>> +        goto bad_64bit_loadstore;
>> +
>> +    if ( extract32(instr, 24, 2) != 0 )
>> +        goto bad_64bit_loadstore;
>> +
>> +    if ( extract32(instr, 27, 3) != 7 )
>> +        goto bad_64bit_loadstore;
>> +
>> +    size = extract32(instr, 30, 2);
>> +    v = extract32(instr, 26, 1);
>> +    opc = extract32(instr, 22, 1);
>> +
>> +    /* At the moment, we support STR(immediate) - 32 bit variant and
>> +     * LDR(immediate) - 32 bit variant only.
>> +     */
>> +    if (!((size==2) && (v==0) && ((opc==0) || (opc==1))))
>> +        goto bad_64bit_loadstore;
> 
> The opc field is actually 2 bits, not 1. I think we should get both
> bits for opc even if some of the configurations are not interesting.

Even more so that checking the value extracted from a 1-bit field
against both 0 and 1 is pointless.

Jan