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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Ayan

> On 22 Nov 2021, at 14:19, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxxxxx> 
> wrote:
> 
> Hi Julien/Stefano/Wei/Jan,
> 
> Many thanks for your review.
> 
> As some of the comments were inter-related, I have consolidated my response 
> to all the comments below.
> 
> On 19/11/2021 17:26, Julien Grall wrote:
>> Hi Ayan,
>> On 19/11/2021 16:52, Ayan Kumar Halder wrote:
>>> At present, post indexing instructions are not emulated by Xen.
>> Can you explain in the commit message why this is a problem?
> 
> Yes, I will update the message as below :-
> Armv8 hardware does not provide the correct syndrome for decoding post 
> indexing ldr/str instructions. Thus any post indexing ldr/str instruction at 
> EL1 results in a data abort with ISV=0. As a result, Xen needs to decode the 
> instruction.
> 
> Thus, DomU will be able to read/write to ioreg space with post indexing 
> instructions for 32 bit.
> 
>>> When Xen gets the exception, EL2_ESR.ISV bit not set. Thus as a
>>> result, data abort is triggered.
>>> 
>>> Added the logic to decode ldr/str post indexing instructions.
>>> With this, Xen can decode instructions like these:-
>>> ldr w2, [x1], #4
>>> Thus, domU can read ioreg with post indexing instructions.
>> Hmmm.... Don't you also need to update the register x1 after the instruction 
>> was emulated?
> 
> Yes, this is a mistake. X1 needs to be incremented by 4 after W2 is written.
>>> 
>>> Signed-off-by: Ayan Kumar Halder <ayankuma@xxxxxxxxxx>
>>> ---
>>> Note to reviewer:-
>>> This patch is based on an issue discussed in
>>> https://lists.xenproject.org/archives/html/xen-devel/2021-11/msg00969.html 
>>> "Xen/ARM - Query about a data abort seen while reading GICD registers"
>>> 
>>> 
>>>   xen/arch/arm/decode.c | 77 +++++++++++++++++++++++++++++++++++++++++++
>>>   xen/arch/arm/io.c     | 14 ++++++--
>>>   2 files changed, 88 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
>>> index 792c2e92a7..7b60bedbc5 100644
>>> --- a/xen/arch/arm/decode.c
>>> +++ b/xen/arch/arm/decode.c
>>> @@ -84,6 +84,80 @@ bad_thumb2:
>>>       return 1;
>>>   }
>>> +static inline int32_t extract32(uint32_t value, int start, int length)
>> Any reason to have start and length signed?
> 
> Again a mistake. There is no reason to use signed types here or in the other 
> places.
> As Jan Beulich has pointed out, I should be using unsigned int as per the 
> CODING_STYLE.
>>> +{
>>> +    int32_t ret;
>>> +
>>> +    if ( !(start >= 0 && length > 0 && length <= 32 - start) )
>>> +        return -EINVAL;
>>> +
>>> +    ret = (value >> start) & (~0U >> (32 - length));
>> This would be easier to read if you use GENMASK().
> 
> I see that GENMASK returns a register mask. In my scenario, I wish to compare 
> the offsets 10, 21, 24 and 27 to some fixed value.
> 
> So, instead of using GENMASK four times, I can try the following
> instr & MASK_for_10_21_24_27 == VALID_for_10_21_24_27 (Wei Chen's suggestion)
> 
> 
> Also for size, v and opc, I can defined another bitmask to compare with 
> VALID_for_32bit_LDR | VALID_for_32bit_STR.
> 
> Wei Chen, You had suggested using vreg_regx_extract(). However, I see that it 
> is used to extract the complete u64 register value. In this case, I wish to 
> compare certain offsets within a 32 bit register to some expected values. So, 
> vreg_regx_extract() might not be appropriate and we can use the method 
> mentioned before.
> 
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +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;
>> Should all those variables need to be signed?
> 
> A mistake. I will change them to unsigned int.
>>> +
>>> +    /* For details on decoding, refer to Armv8 Architecture reference 
>>> manual
>>> +     * Section - "Load/store register (immediate post-indexed)", Pg 318
>> The page number varies between revision of the Armv8 spec. So can you 
>> specify which version you used?
> 
> Ack. I will mention the version.
>>> +    */
>> The coding style for comment in Xen is:
>> /*
>>  * Foo
>>  * Bar
>>  */
> Ack
>>> +    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
>> Same question here about the version.
> Ack, I will mention the version.
> 
>>> +     * op4 = 1 - Load/store register (immediate post-indexed)
>>> +     */
>> Coding style.
> Ack
> 
>>> +    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);
> 
> Stefano:- Thanks for catching my mistake. opc is 2 bits (bits 22, 23). I will 
> fix this.
> 
>>> +
>>> +    /* At the moment, we support STR(immediate) - 32 bit variant and
>>> +     * LDR(immediate) - 32 bit variant only.
>>> +     */
>> Coding style.
> Ack
> 
>>> +    if (!((size==2) && (v==0) && ((opc==0) || (opc==1))))
>>> 
>> The coding style for this should be:
>> if ( !(( size == 2 ) && ( ... ) ... ) )
> Ack
> 
>>  +        goto bad_64bit_loadstore;
>>> +
>>> +    rt = extract32(instr, 0, 5);
>>> +    imm9 = extract32(instr, 12, 9);
>>> +
>>> +    if ( imm9 < 0 )
>>> +        update_dabt(dabt, rt, size, true);
>>> +    else
>>> +        update_dabt(dabt, rt, size, false);
>> This could be simplified with:
>> update_dabt(dabt, rt, size, imm9 < 0);
> Ack
> 
>>> +
>>> +    dabt->valid = 1;
>>> +
>>> +
>>> +    return 0;
>>> +bad_64bit_loadstore:
>>> +    gprintk(XENLOG_ERR, "unhandled 64bit instruction 0x%x\n", instr);
>>> +    return 1;
>>> +}
>>> +
>>>   static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
>>>   {
>>>       uint16_t instr;
>>> @@ -155,6 +229,9 @@ int decode_instruction(const struct cpu_user_regs 
>>> *regs, struct hsr_dabt *dabt)
>>>       if ( is_32bit_domain(current->domain) && regs->cpsr & PSR_THUMB )
>>>           return decode_thumb(regs->pc, dabt);
>>> +    if ( is_64bit_domain(current->domain) )
>> You can still run 32-bit code in 64-bit domain. So I think you want to check 
>> the SPSR mode.
> 
> Do you mean the following check :-
> if ( (is_64bit_domain(current->domain) && (!(regs->spsr & PSR_MODE_BIT)) )
> 
>>> +        return decode_64bit_loadstore_postindexing(regs->pc, dabt);
>>> +
>>>       /* TODO: Handle ARM instruction */
>>>       gprintk(XENLOG_ERR, "unhandled ARM instruction\n");
>> I think this comment should now be updated to "unhandled 32-bit ...".
> 
> Ack
>>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>>> index 729287e37c..49e80358c0 100644
>>> --- a/xen/arch/arm/io.c
>>> +++ b/xen/arch/arm/io.c
>>> @@ -106,14 +106,13 @@ enum io_state try_handle_mmio(struct cpu_user_regs 
>>> *regs,
>>>           .gpa = gpa,
>>>           .dabt = dabt
>>>       };
>>> +    int rc;
>>>       ASSERT(hsr.ec == HSR_EC_DATA_ABORT_LOWER_EL);
>>>       handler = find_mmio_handler(v->domain, info.gpa);
>>>       if ( !handler )
>>>       {
>>> -        int rc;
>>> -
>>>           rc = try_fwd_ioserv(regs, v, &info);
>>>           if ( rc == IO_HANDLED )
>>>               return handle_ioserv(regs, v);
>>> @@ -123,7 +122,16 @@ enum io_state try_handle_mmio(struct cpu_user_regs 
>>> *regs,
>>>       /* All the instructions used on emulated MMIO region should be valid 
>>> */
>>>       if ( !dabt.valid )
>>> -        return IO_ABORT;
>>> +    {
>>> +        /*
>>> +         * Post indexing ldr/str instructions are not emulated by Xen. So, 
>>> the
>>> +         * ISS is invalid. In such a scenario, we try to manually decode 
>>> the
>>> +         * instruction from the program counter.
>> I am afraid this is wrong. The problem here is the processor didn't provide 
>> a valid syndrom for post-indexing ldr/str instructions. So in order to 
>> support them, Xen must decode.
> 
> Ack
>>> +         */
>>> +        rc = decode_instruction(regs, &info.dabt);
>> I actually expect this to also be useful when forwarding the I/O to 
>> device-model. So I would move the decoding earlier in the code and the check 
>> of dabt.valid earlier.
> 
> You mean I will move decode_instruction() before find_mmio_handler() ?
> 
> Stefano > It doesn't look like we are setting dabt->write anywhere.
> 
> Ayan > Yes, this is a miss. Depending on the opc, this should be upadeted 
> accordingly in decode_64bit_loadstore_postindexing().
> 
> Stefano > Also, is info.gpa in try_handle_mmio already updated in the 
> pre-index
> case? If not, do we need to apply the offset manually here?
> 
> Ayan > Sorry, I did not understand you. This patch is only related to the 
> post indexing ldr/str issue. Why do we need to check for pre-indexing ?
> 
> Stefano > In the post-index case, we need to update the base address in the Rn
> register?
> 
> Ayan > Yes this is a mistake, As Julien pointed out before, the register x1 
> ie Rn needs to the incremented.
> 
> Jan > In addition to Julien's comment regarding the function parameters - why
> is the return type int32_t and not uint32_t? Plus as per ./CODING_STYLE
> it really shouldn't be a fixed width type anyway, but e.g. unsigned int.
> 
> Ayan > Yes, this is a mistake. I will update int32_t to unsigned int in all 
> the places.
> However for extract32(), I don't think we need this function. Rather Wei's 
> suggestion (ie instr & MASK_for_10_21_24_27 == VALID_for_10_21_24_27 ) makes 
> the code simpler and shorter.

In fact you could also use a union and define in a structure what the bits are.

Union instr {
        uint32_t value
        struct {
            ….
            Unsigned int size:2;
        }
}

This could simplify some of your code.

Cheers
Bertrand


> 
> - Ayan
> 
>>> +        if ( rc )
>>> +            return IO_ABORT;
>>> +    }
>>>       /*
>>>        * Erratum 766422: Thumb store translation fault to Hypervisor may
>>> 
>> Cheers,