[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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.

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.