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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

> On 23 Nov 2021, at 08:53, Bertrand Marquis <bertrand.marquis@xxxxxxx> wrote:
> 
> Hi Stefano,
> 
>> On 23 Nov 2021, at 04:13, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
>> 
>> On Mon, 22 Nov 2021, Julien Grall wrote:
>>> On Mon, 22 Nov 2021 at 19:59, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
>>> wrote:
>>>> 
>>>> On Mon, 22 Nov 2021, Ayan Kumar Halder wrote:
>>>>> 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 ?
>>>> 
>>>> I thought you were trying to handle both post-indexing and pre-indexing.
>>>> It is OK if you intend to only handle post-indexing but considering that
>>>> most of the code is shared between the two, we might as well also make
>>>> pre-indexing work (unless it turns out it is more difficult).
>>> 
>>> Wouldn't this effectively be dead code?
> 
> I agree this would be dead code. Pre-indexing is handled by the hardware, 
> only post are not.
> 
>>> 
>>>> 
>>>> In the pre-indexing case, I would imagine we need to update the base
>>>> address before taking any other actions.
>>> 
>>>> From my understanding, this would have already been performed by the
>>> HW when the syndrome is valid. This may also be the case for
>>> the non-valid case, but I haven't checked the Arm Arm.
>> 
>> It is not clear to me either, that's why I wrote "I would imagine"... I
>> was guessing that it is not done by the HW in the non-valid case but I
>> don't know.
> 
> This should be handled by the hardware here, so only post actions should
> be handled here.
> 
>> 
>> Of course, if it is already done by the HW, that's all the better: no
>> need for us to do anything.
> 
> I am wondering though if other types of accesses could not be handled here
> without major modification of the code like other sizes then 32bit.

I did some checks and I think the following cases could be handled:
    ldr x2, [x1], #4
    nop
    ldr w2, [x1], #-4
    nop
    ldrh w2, [x1], #8
    nop
    ldrb w2, [x1], #16
    nop
    str x2, [x1], #4
    nop
    str w2, [x1], #-4
    nop
    strh w2, [x1], #8
    nop
    strb w2, [x1], #16
    nop

Anything that I could have missed ?

> 
> There are also post instructions with shifting but to be honest I do not 
> think this is really needed.

Please ignore this, there is no post shifting.

Once this is done I can test and add a test to XTF on arm (on our side, 
upstreaming of this is in progress) to make sure this is maintained.

Regards
Bertrand