[edk2-discuss] Need memory barriers in IoLib for AARCH64

[edk2-discuss] Need memory barriers in IoLib for AARCH64

[Wasim Khan]

Hello,

MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf:
IoLib library uses IoLibArm.c for AARCH64/ARM architecture and IoLib.c for other architectures. 

While IoLib.c already has memory barriers in MmioWrite functions, there barriers are missing in IoLibArm.c
Is there any reason for **not** adding these memory barriers in IoLibArm.c to guarantee that all MMIO operations
are serialized ?

I am facing some issues and I need to add memory barriers in IoLibArm.c for AARCH64 also .


Regards,
Wasim

Re: [edk2-discuss] Need memory barriers in IoLib for AARCH64

[Wasim Khan]

Hello 

Any comments ?

> -----Original Message-----
> From: Wasim Khan
> Sent: Friday, July 10, 2020 6:20 PM
> To: michael.d.kinney@intel.com; liming.gao@intel.com; devel@edk2.groups.io
> Subject: [edk2-discuss] Need memory barriers in IoLib for AARCH64
> 
> Hello,
> 
> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf:
> IoLib library uses IoLibArm.c for AARCH64/ARM architecture and IoLib.c for
> other architectures.
> 
> While IoLib.c already has memory barriers in MmioWrite functions, there
> barriers are missing in IoLibArm.c Is there any reason for **not** adding these
> memory barriers in IoLibArm.c to guarantee that all MMIO operations are
> serialized ?
> 
> I am facing some issues and I need to add memory barriers in IoLibArm.c for
> AARCH64 also .
> 
> 
> Regards,
> Wasim

Re: [edk2-devel] [edk2-discuss] Need memory barriers in IoLib for AARCH64

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]



> On Jul 11, 2020, at 10:17 PM, Wasim Khan <wasim.khan@nxp.com> wrote:
> 
> Hello 
> 
> Any comments ?
> 

I don’t see IoLibArm.c in master? I see IoLibNoIo.c. 

The MMIO function look like ARM assembler with the correct barrier instructions. The IO operations in this lib are the x86 in/out instructions, so they just ASSERT on ARM. 

On the X86 MemoryFence() is just a serializing intrinsic for the compiler to prevent optimizations from breaking the code, kind of like how you need to make MMIO as volatile in C. 

Thanks,

Andrew Fish

>> -----Original Message-----
>> From: Wasim Khan
>> Sent: Friday, July 10, 2020 6:20 PM
>> To: michael.d.kinney@intel.com; liming.gao@intel.com; devel@edk2.groups.io
>> Subject: [edk2-discuss] Need memory barriers in IoLib for AARCH64
>> 
>> Hello,
>> 
>> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf:
>> IoLib library uses IoLibArm.c for AARCH64/ARM architecture and IoLib.c for
>> other architectures.
>> 
>> While IoLib.c already has memory barriers in MmioWrite functions, there
>> barriers are missing in IoLibArm.c Is there any reason for **not** adding these
>> memory barriers in IoLibArm.c to guarantee that all MMIO operations are
>> serialized ?
>> 
>> I am facing some issues and I need to add memory barriers in IoLibArm.c for
>> AARCH64 also .
>> 
>> 
>> Regards,
>> Wasim
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 6012 bytes --]

Re: [edk2-devel] [edk2-discuss] Need memory barriers in IoLib for AARCH64

[Laszlo Ersek]

On 07/12/20 18:54, Andrew Fish via groups.io wrote:
> 
> 
>> On Jul 11, 2020, at 10:17 PM, Wasim Khan <wasim.khan@nxp.com> wrote:
>>
>> Hello 
>>
>> Any comments ?
>>
> 
> I don’t see IoLibArm.c in master? I see IoLibNoIo.c. 

That's due to the rename in commit 089e9c19a8c1
("MdePkg/BaseIoLibIntrinsic: Rename IoLibArm.c=>IoLibNoIo.c",
2020-05-07), which has been first included in edk2-stable202005.

I think Ard is away at the moment, so I'm adding Leif to the CC list.

Thanks
Laszlo

> The MMIO function look like ARM assembler with the correct barrier instructions. The IO operations in this lib are the x86 in/out instructions, so they just ASSERT on ARM. 
> 
> On the X86 MemoryFence() is just a serializing intrinsic for the compiler to prevent optimizations from breaking the code, kind of like how you need to make MMIO as volatile in C. 
> 
> Thanks,
> 
> Andrew Fish
> 
>>> -----Original Message-----
>>> From: Wasim Khan
>>> Sent: Friday, July 10, 2020 6:20 PM
>>> To: michael.d.kinney@intel.com; liming.gao@intel.com; devel@edk2.groups.io
>>> Subject: [edk2-discuss] Need memory barriers in IoLib for AARCH64
>>>
>>> Hello,
>>>
>>> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf:
>>> IoLib library uses IoLibArm.c for AARCH64/ARM architecture and IoLib.c for
>>> other architectures.
>>>
>>> While IoLib.c already has memory barriers in MmioWrite functions, there
>>> barriers are missing in IoLibArm.c Is there any reason for **not** adding these
>>> memory barriers in IoLibArm.c to guarantee that all MMIO operations are
>>> serialized ?
>>>
>>> I am facing some issues and I need to add memory barriers in IoLibArm.c for
>>> AARCH64 also .
>>>
>>>
>>> Regards,
>>> Wasim
>>
>>
>>
> 
> 
> 
> 
> 

Re: [edk2-devel] [edk2-discuss] Need memory barriers in IoLib for AARCH64

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 2204 bytes --]

Wasim,

Was there a specific function you had concerns about? Or was it confusion about the names? Hopefully the name confusion would have been fixed by 089e9c19a8c?

Thanks,

Andrew Fish

> On Jul 13, 2020, at 8:31 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 07/12/20 18:54, Andrew Fish via groups.io <http://groups.io/> wrote:
>> 
>> 
>>> On Jul 11, 2020, at 10:17 PM, Wasim Khan <wasim.khan@nxp.com <mailto:wasim.khan@nxp.com>> wrote:
>>> 
>>> Hello 
>>> 
>>> Any comments ?
>>> 
>> 
>> I don’t see IoLibArm.c in master? I see IoLibNoIo.c. 
> 
> That's due to the rename in commit 089e9c19a8c1
> ("MdePkg/BaseIoLibIntrinsic: Rename IoLibArm.c=>IoLibNoIo.c",
> 2020-05-07), which has been first included in edk2-stable202005.
> 
> I think Ard is away at the moment, so I'm adding Leif to the CC list.
> 
> Thanks
> Laszlo
> 
>> The MMIO function look like ARM assembler with the correct barrier instructions. The IO operations in this lib are the x86 in/out instructions, so they just ASSERT on ARM. 
>> 
>> On the X86 MemoryFence() is just a serializing intrinsic for the compiler to prevent optimizations from breaking the code, kind of like how you need to make MMIO as volatile in C. 
>> 
>> Thanks,
>> 
>> Andrew Fish
>> 
>>>> -----Original Message-----
>>>> From: Wasim Khan
>>>> Sent: Friday, July 10, 2020 6:20 PM
>>>> To: michael.d.kinney@intel.com; liming.gao@intel.com; devel@edk2.groups.io
>>>> Subject: [edk2-discuss] Need memory barriers in IoLib for AARCH64
>>>> 
>>>> Hello,
>>>> 
>>>> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf:
>>>> IoLib library uses IoLibArm.c for AARCH64/ARM architecture and IoLib.c for
>>>> other architectures.
>>>> 
>>>> While IoLib.c already has memory barriers in MmioWrite functions, there
>>>> barriers are missing in IoLibArm.c Is there any reason for **not** adding these
>>>> memory barriers in IoLibArm.c to guarantee that all MMIO operations are
>>>> serialized ?
>>>> 
>>>> I am facing some issues and I need to add memory barriers in IoLibArm.c for
>>>> AARCH64 also .
>>>> 
>>>> 
>>>> Regards,
>>>> Wasim
>>> 
>>> 
>>> 
>> 
>> 
>> 
>> 
>> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 12664 bytes --]

Re: [edk2-devel] [edk2-discuss] Need memory barriers in IoLib for AARCH64

[Wasim Khan]

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Monday, July 13, 2020 9:02 PM
> To: devel@edk2.groups.io; afish@apple.com; Wasim Khan
> <wasim.khan@nxp.com>
> Cc: Mike Kinney <michael.d.kinney@intel.com>; liming.gao@intel.com; Leif
> Lindholm (Nuvia address) <leif@nuviainc.com>
> Subject: Re: [edk2-devel] [edk2-discuss] Need memory barriers in IoLib for
> AARCH64
> 
> On 07/12/20 18:54, Andrew Fish via groups.io wrote:
> >
> >
> >> On Jul 11, 2020, at 10:17 PM, Wasim Khan <wasim.khan@nxp.com> wrote:
> >>
> >> Hello
> >>
> >> Any comments ?
> >>
> >
> > I don’t see IoLibArm.c in master? I see IoLibNoIo.c.
> 
> That's due to the rename in commit 089e9c19a8c1
> ("MdePkg/BaseIoLibIntrinsic: Rename IoLibArm.c=>IoLibNoIo.c", 2020-05-07),
> which has been first included in edk2-stable202005.
> 
> I think Ard is away at the moment, so I'm adding Leif to the CC list.
> 
> Thanks
> Laszlo
> 
> > The MMIO function look like ARM assembler with the correct barrier
> instructions. The IO operations in this lib are the x86 in/out instructions, so they
> just ASSERT on ARM.
> >
> > On the X86 MemoryFence() is just a serializing intrinsic for the compiler to
> prevent optimizations from breaking the code, kind of like how you need to
> make MMIO as volatile in C.
> >
> > Thanks,
> >
> > Andrew Fish

Thank you Andrew and Laszlo for you response. 
My problem was that because there is no MemoryFence() in MmioRead/Write functions in IoLibNoIo.c, I was facing some serializing problem with AARCH64.
I have two options:
Either put MemoryFence() in my code under edl2-platforms before and after MmioRead/Write calls
OR
Add the MemoryFence() in MmioRead/Write itself for IoLibNoIo.c, like it is done for IoLib.c . Because I don’t see any harm to make sure that MmioRead/Write operation are serialized by using MemoryFence() in it. 

I prefer later option, if there is no specific reason for not adding MemoryFence() for IoLibNoIo.c.

> >
> >>> -----Original Message-----
> >>> From: Wasim Khan
> >>> Sent: Friday, July 10, 2020 6:20 PM
> >>> To: michael.d.kinney@intel.com; liming.gao@intel.com;
> >>> devel@edk2.groups.io
> >>> Subject: [edk2-discuss] Need memory barriers in IoLib for AARCH64
> >>>
> >>> Hello,
> >>>
> >>> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf:
> >>> IoLib library uses IoLibArm.c for AARCH64/ARM architecture and
> >>> IoLib.c for other architectures.
> >>>
> >>> While IoLib.c already has memory barriers in MmioWrite functions,
> >>> there barriers are missing in IoLibArm.c Is there any reason for
> >>> **not** adding these memory barriers in IoLibArm.c to guarantee that
> >>> all MMIO operations are serialized ?
> >>>
> >>> I am facing some issues and I need to add memory barriers in
> >>> IoLibArm.c for
> >>> AARCH64 also .
> >>>
> >>>
> >>> Regards,
> >>> Wasim
> >>
> >>
> >>
> >
> >
> > 
> >
> >

Re: [edk2-devel] [edk2-discuss] Need memory barriers in IoLib for AARCH64

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 5860 bytes --]



> On Jul 16, 2020, at 11:43 PM, Wasim Khan <wasim.khan@nxp.com> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com <mailto:lersek@redhat.com>>
>> Sent: Monday, July 13, 2020 9:02 PM
>> To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; afish@apple.com <mailto:afish@apple.com>; Wasim Khan
>> <wasim.khan@nxp.com <mailto:wasim.khan@nxp.com>>
>> Cc: Mike Kinney <michael.d.kinney@intel.com <mailto:michael.d.kinney@intel.com>>; liming.gao@intel.com <mailto:liming.gao@intel.com>; Leif
>> Lindholm (Nuvia address) <leif@nuviainc.com <mailto:leif@nuviainc.com>>
>> Subject: Re: [edk2-devel] [edk2-discuss] Need memory barriers in IoLib for
>> AARCH64
>> 
>> On 07/12/20 18:54, Andrew Fish via groups.io wrote:
>>> 
>>> 
>>>> On Jul 11, 2020, at 10:17 PM, Wasim Khan <wasim.khan@nxp.com> wrote:
>>>> 
>>>> Hello
>>>> 
>>>> Any comments ?
>>>> 
>>> 
>>> I don’t see IoLibArm.c in master? I see IoLibNoIo.c.
>> 
>> That's due to the rename in commit 089e9c19a8c1
>> ("MdePkg/BaseIoLibIntrinsic: Rename IoLibArm.c=>IoLibNoIo.c", 2020-05-07),
>> which has been first included in edk2-stable202005.
>> 
>> I think Ard is away at the moment, so I'm adding Leif to the CC list.
>> 
>> Thanks
>> Laszlo
>> 
>>> The MMIO function look like ARM assembler with the correct barrier
>> instructions. The IO operations in this lib are the x86 in/out instructions, so they
>> just ASSERT on ARM.
>>> 
>>> On the X86 MemoryFence() is just a serializing intrinsic for the compiler to
>> prevent optimizations from breaking the code, kind of like how you need to
>> make MMIO as volatile in C.
>>> 
>>> Thanks,
>>> 
>>> Andrew Fish
> 
> Thank you Andrew and Laszlo for you response. 
> My problem was that because there is no MemoryFence() in MmioRead/Write functions in IoLibNoIo.c, I was facing some serializing problem with AARCH64.
> I have two options:
> Either put MemoryFence() in my code under edl2-platforms before and after MmioRead/Write calls
> OR
> Add the MemoryFence() in MmioRead/Write itself for IoLibNoIo.c, like it is done for IoLib.c . Because I don’t see any harm to make sure that MmioRead/Write operation are serialized by using MemoryFence() in it. 
> 
> I prefer later option, if there is no specific reason for not adding MemoryFence() for IoLibNoIo.c.
> 

Look at BaseIoLibIntrinsicArmVirt.inf [1] I think it does what you want. 

I don’t know the history of the original ARM BaseIoLibIntrinsic port, but it does look like there is an assumption that with volatile the compiler would “do the right thing”. The ArmVirt flavor seems to have the serializing instructions you are looking for? 

[1] https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicArmVirt.inf

[2] git log MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicArmVirt.inf 
...
commit 4134f2bddcb68d2e20ed000cdf54abf3f1140904
Author: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date:   Thu Jun 7 12:44:12 2018 +0200

    MdePkg/BaseIoLibIntrinsic: make BaseIoLibIntrinsic safe for ArmVirt/KVM

    KVM on ARM refuses to decode load/store instructions used to perform
    I/O to emulated devices, and instead relies on the exception syndrome
    information to describe the operand register, access size, etc.
    This is only possible for instructions that have a single input/output
    register (as opposed to ones that increment the offset register, or
    load/store pair instructions, etc). Otherwise, QEMU crashes with the
    following error

      error: kvm run failed Function not implemented
      R00=01010101 R01=00000008 R02=00000048 R03=08000820
      R04=00000120 R05=7faaa0e0 R06=7faaa0dc R07=7faaa0e8
      R08=7faaa0ec R09=7faaa088 R10=000000ff R11=00000080
      R12=ff000000 R13=7fccfe08 R14=7faa835f R15=7faa887c
      PSR=800001f3 N--- T svc32
      QEMU: Terminated

    and KVM produces a warning such as the following in the kernel log

      kvm [17646]: load/store instruction decoding not implemented

    The IoLib implementation provided by MdePkg/Library/BaseIoLibIntrinsic
    is based on C code, and when LTO is in effect, the MMIO accesses could
    be merged with, e.g., manipulations of the loop counter, producing
    opcodes that KVM does not support for emulated MMIO.

    So let's add a special ArmVirt flavor of this library that implements
    that actual load/store operations in assembler, ensuring that the
    instructions involved can be emulated by KVM.

    Contributed-under: TianoCore Contribution Agreement 1.1
    Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
    Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
    Acked-by: Laszlo Ersek <lersek@redhat.com>
    Reviewed-by: Liming Gao <liming.gao@intel.com>

Thanks,

Andrew Fish

>>> 
>>>>> -----Original Message-----
>>>>> From: Wasim Khan
>>>>> Sent: Friday, July 10, 2020 6:20 PM
>>>>> To: michael.d.kinney@intel.com; liming.gao@intel.com;
>>>>> devel@edk2.groups.io
>>>>> Subject: [edk2-discuss] Need memory barriers in IoLib for AARCH64
>>>>> 
>>>>> Hello,
>>>>> 
>>>>> MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf:
>>>>> IoLib library uses IoLibArm.c for AARCH64/ARM architecture and
>>>>> IoLib.c for other architectures.
>>>>> 
>>>>> While IoLib.c already has memory barriers in MmioWrite functions,
>>>>> there barriers are missing in IoLibArm.c Is there any reason for
>>>>> **not** adding these memory barriers in IoLibArm.c to guarantee that
>>>>> all MMIO operations are serialized ?
>>>>> 
>>>>> I am facing some issues and I need to add memory barriers in
>>>>> IoLibArm.c for
>>>>> AARCH64 also .
>>>>> 
>>>>> 
>>>>> Regards,
>>>>> Wasim
>>>> 
>>>> 
>>>> 
>>> 
>>> 
>>> 
>>> 
>>> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 25951 bytes --]