Re: Xen/ARM - Query about a data abort seen while reading GICD registers

[Ayan Kumar Halder <ayan.kumar.halder@xxxxxxxxxx>]

Hi Stefano/Julien/Bertrand,

Thanks a lot for your inputs.

On 18/11/2021 01:11, Stefano Stabellini wrote:
> On Wed, 17 Nov 2021, Julien Grall wrote:
> > I will combine my answers for this thread in one single e-mail.
> >
> > On 17/11/2021 16:35, Bertrand Marquis wrote:
> > > > On 17 Nov 2021, at 16:21, Ayan Kumar Halder <ayan.kumar.halder@xxxxxxxxxx>
> > > > wrote:
> > > >
> > > > Hi Bertrand,
> > > >
> > > > Many thanks for your response.
> > > >
> > > > On 17/11/2021 15:48, Bertrand Marquis wrote:
> > > > > Hi Ayan,
> > > > > > On 16 Nov 2021, at 16:24, Ayan Kumar Halder
> > > > > > <ayan.kumar.halder@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > Hi Bertrand,
> > > > > >
> > > > > > Thanks for looking into it.
> > > > > >
> > > > > > On 16/11/2021 15:36, Bertrand Marquis wrote:
> > > > > > > Hi Ayan,
> > > > > > > > On 16 Nov 2021, at 15:27, Ayan Kumar Halder
> > > > > > > > <ayan.kumar.halder@xxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > Hi Xen/Arm experts,
> > > > > > > >
> > > > > > > > I am facing a very strange issue while running a baremetal
> > > > > > > > application as a DomU guest on arm64 platform.
> > > > > > > >
> > > > > > > > The baremetal app tries to read the GICD register with post
> > > > > > > > indexing as follows :-
> > > > > > > > ldr x1, =0x3001000
> > > > > > > > ldr w2, [x1], #4 <<<------ PC = 0x40000ca8
> > > > > > > Increment on on load is not supported by the emulation layer.
> > > > > > That is surprising. The reason being if I try to read the GICC
> > > > > > register (0x3002000) with post indexing then it works fine.
> > > > > When the ISV bit is not set, Xen would have to decode the instruction to
> > > > > actually emulate the access and properly modify the registers values as
> > > > > long as providing the “emulated” access value.
> > > > This is very interesting. Is this being done for any of the other
> > > > instructions in Xen ?
> > > No Xen is not trying to decode any instructions.
> > We actually decode some instructions (see arch/arm/decode.c). This was
> > necessary because early revision of Cortex-A15 would not properly fill
> > syndrome for Thumb instructions.
> >
> > decode_instruction() could be extended to handle the specific instruction if
> > needed.
> >
> > > If you look at Linux source code, this is the kind of stuff that the kernel
> > > is delegating to user application (qemu) to do as it can be quite complex.
> > >
> > > > > There is not such a decoder in Xen right now which means those kind of
> > > > > accesses are not supported for emulated mmio accesses.
> > > > I am actually trying to understand where I will need to make the changes
> > > > if I have to add support for the decoder. The reason being this issue is
> > > > being faced by one of our customer application.
> > > > Besides changing the instruction to prevent post increment, is there any
> > > > other mitigation ?
> > > Not that I know of.
> > Decoding the instruction is the only solution if you don't want to update the
> > baremetal app.
> >
> > > > The reason being I don't see a way to instruct the compiler to not
> > > > generate the post indexing instructions.
> > In general, it is not safe to let the compiler decide for you how to access
> > the MMIO registers as it can do all sort of optimization behind your back.
> > That's why...
> >
> > > You can define io access functions instead of letting the compiler generate
> > > the read/write functions.
> > > Look at arch/arm64/include/asm-arm/io.h in linux for example.
> > ... Linux provides io{read, write} helpers.
> >
> > @Ayan, is the code written in assembly or C? If the latter, how is it written?
> I chatted with Ayan this morning and he will try to contact the original
> author and get a proper answer, but I am pretty sure that it is written
> in C. What makes it worse is that I believe the issue only started to
> appear recently due to updating compiler (hence new compiler
> optimizations.) Which means that this issue might become more common in
> the future in other environments too :-(
>
> I believe this is the code (source [1]):
>
>      XScuGic_WriteReg(BaseAddress, XSCUGIC_DIST_EN_OFFSET, 0UL);
>
> which expands to:
>
>      #define XScuGic_WriteReg(BaseAddress, RegOffset, Data) \
>         (Xil_Out32(((BaseAddress) + (RegOffset)), ((u32)(Data))))
>
> Which is:
>
>      static INLINE void Xil_Out32(UINTPTR Addr, u32 Value)
>      {
>         /* write 32 bit value to specified address */
>      #ifndef ENABLE_SAFETY
>         volatile u32 *LocalAddr = (volatile u32 *)Addr;
>         *LocalAddr = Value;
>      #else
>         XStl_RegUpdate(Addr, Value);
>      #endif
>      }
>
> [1] 
> https://github.com/Xilinx/embeddedsw/tree/master/XilinxProcessorIPLib/drivers/scugic/examples/xscugic_low_level_example.c
>
>
> So it seems to be a pretty standard volatile write in a static inline
> function.
>
>
> > That said, the ldr writeback instructions should be safe to use. It happens
> > that we never had OS using them before.
> Yeah I agree... My two cents is that if we can find a way to decode the
> instruction without a huge amount of code then it would be worth doing
> it.
I have submitted a patch to decode the ldr/str post-indexing instructions.
"[RFC PATCH] Added the logic to decode 32 bit ldr/str post-indexing 
instructions"
Please have a look and let me know your thoughts.
- Ayan