Re: [edk2-devel] [staging/LoongArch RESEND PATCH v1 26/33] MdePkg/BaseSynchronizationLib: LoongArch cache related code.
Hi Abner, All of the feedback that you pointed out I will fix it in the next version, thank you! -- Thanks, Chao On 4ζ 8 2022, at 7:02 ζδΈ, "Chang, Abner (HPS SW/FW Technologist)" wrote: > > > > -Original Message- > > From: devel@edk2.groups.io On Behalf Of Chao Li > > Sent: Wednesday, February 9, 2022 2:56 PM > > To: devel@edk2.groups.io > > Cc: Michael D Kinney ; Liming Gao > > ; Zhiguang Liu ; Baoqi > > Zhang > > Subject: [edk2-devel] [staging/LoongArch RESEND PATCH v1 26/33] > > MdePkg/BaseSynchronizationLib: LoongArch cache related code. > > > > Support LoongArch cache related functions. > > > > Cc: Michael D Kinney > > Cc: Liming Gao > > Cc: Zhiguang Liu > > > > Signed-off-by: Chao Li > > Co-authored-by: Baoqi Zhang > > --- > > .../BaseSynchronizationLib.inf | 5 + > > .../LoongArch64/Synchronization.c | 239 ++ > > 2 files changed, 244 insertions(+) > > create mode 100644 > > MdePkg/Library/BaseSynchronizationLib/LoongArch64/Synchronization.c > > > > diff --git > > a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > > b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > > index 83d5b8ed7c..3cf5b6d4b1 100755 > > --- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > > +++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > > @@ -4,6 +4,7 @@ > > # Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved. > > # Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved. > > # Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights > > reserved. > > +# Copyright (c) 2022, Loongson Technology Corporation Limited. All rights > > reserved. > > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > # > > @@ -83,6 +84,10 @@ > > Synchronization.c > > RiscV64/Synchronization.S > > > > +[Sources.LOONGARCH64] > > + Synchronization.c > > + LoongArch64/Synchronization.c > > + > > [Packages] > > MdePkg/MdePkg.dec > > > > diff --git > > a/MdePkg/Library/BaseSynchronizationLib/LoongArch64/Synchronization.c > > b/MdePkg/Library/BaseSynchronizationLib/LoongArch64/Synchronization.c > > new file mode 100644 > > index 00..a191a50c81 > > --- /dev/null > > +++ > > b/MdePkg/Library/BaseSynchronizationLib/LoongArch64/Synchronization.c > > @@ -0,0 +1,239 @@ > > +/** @file > > + LoongArch synchronization functions. > > + > > + Copyright (c) 2022, Loongson Technology Corporation Limited. All rights > > reserved. > > + > > + SPDX-License-Identifier: BSD-2-Clause-Patent > > + > > +**/ > > + > > +#include > > + > > +/** > > + Performs an atomic compare exchange operation on a 16-bit > > + unsigned integer. > > + > > + Performs an atomic compare exchange operation on the 16-bit > > + unsigned integer specified by Value. If Value is equal to > > + CompareValue, then Value is set to ExchangeValue and > > + CompareValue is returned. If Value is not equal to > > + CompareValue, then Value is returned. The compare exchange > > + operation must be performed using MP safe mechanisms. > > + > > + @param Value A pointer to the 16-bit value for the > > + compare exchange operation. > > + @param CompareValue 16-bit value used in compare operation. > > + @param ExchangeValue 16-bit value used in exchange operation. > > + > > + @return The original *Value before exchange. > I see the modifiers (e.g., IN and/or OUT) are not assigned to @param of > arguments in the function header across the source files in this patch set. I > know some old source files don't have modifiers neither, however, we should > have those in the new source file according to the coding standard. > > > + > > +**/ > > +UINT16 > > +EFIAPI > > +InternalSyncCompareExchange16 ( > > + IN volatile UINT16 *Value, > > + IN UINT16 CompareValue, > > + IN UINT16 ExchangeValue > > + ) > > +{ > > + UINT32 RetValue, Temp, Shift; > > + UINT64 Mask, LocalCompareValue, LocalExchangeValue; > > + volatile UINT32 *Ptr32; > I can't find the statement in the edk2 C coding standard spec, however as I > can remember each local variable should start at a new line. > > + > > + /* Check that ptr is naturally aligned */ > > + ASSERT(!((UINT64)Value & (sizeof(Value) - 1))); > Please use double back slash for the comment. > > + > > + /* Mask inputs to the correct size. */ > > + Mask = (((~0UL) - (1UL << (0)) + 1) & (~0UL >> (64 - 1 - ((sizeof(UINT16) > > * 8) > > - 1; > > + LocalCompareValue = ((UINT64)CompareValue) & Mask; > > + LocalExchangeValue = ((UINT64)ExchangeValue) & Mask; > > + > > + /* > > + * Calculate a shift & mask that correspond to the value we wish to > > + * compare & exchange within the naturally aligned 4 byte integer > > + * that includes it. > > + */ > > + Shift = (UINT64)Value & 0x3; > > + Shift *= 8; /* BITS_PER_BYTE */ > > + LocalCompareValue <<= Shift; > > + LocalExchangeValue <<= Shift; > > + Mask <<= Shift; > > + > > + /* > > + * Calculate a pointer to the naturally aligned 4 byte integer that > >
Re: [edk2-devel] [staging/LoongArch RESEND PATCH v1 26/33] MdePkg/BaseSynchronizationLib: LoongArch cache related code.
> -Original Message- > From: devel@edk2.groups.io On Behalf Of Chao Li > Sent: Wednesday, February 9, 2022 2:56 PM > To: devel@edk2.groups.io > Cc: Michael D Kinney ; Liming Gao > ; Zhiguang Liu ; Baoqi > Zhang > Subject: [edk2-devel] [staging/LoongArch RESEND PATCH v1 26/33] > MdePkg/BaseSynchronizationLib: LoongArch cache related code. > > Support LoongArch cache related functions. > > Cc: Michael D Kinney > Cc: Liming Gao > Cc: Zhiguang Liu > > Signed-off-by: Chao Li > Co-authored-by: Baoqi Zhang > --- > .../BaseSynchronizationLib.inf| 5 + > .../LoongArch64/Synchronization.c | 239 ++ > 2 files changed, 244 insertions(+) > create mode 100644 > MdePkg/Library/BaseSynchronizationLib/LoongArch64/Synchronization.c > > diff --git > a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > index 83d5b8ed7c..3cf5b6d4b1 100755 > --- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > +++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > @@ -4,6 +4,7 @@ > # Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved. > # Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved. > # Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All rights > reserved. > +# Copyright (c) 2022, Loongson Technology Corporation Limited. All rights > reserved. > # > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > @@ -83,6 +84,10 @@ >Synchronization.c >RiscV64/Synchronization.S > > +[Sources.LOONGARCH64] > + Synchronization.c > + LoongArch64/Synchronization.c > + > [Packages] >MdePkg/MdePkg.dec > > diff --git > a/MdePkg/Library/BaseSynchronizationLib/LoongArch64/Synchronization.c > b/MdePkg/Library/BaseSynchronizationLib/LoongArch64/Synchronization.c > new file mode 100644 > index 00..a191a50c81 > --- /dev/null > +++ > b/MdePkg/Library/BaseSynchronizationLib/LoongArch64/Synchronization.c > @@ -0,0 +1,239 @@ > +/** @file > + LoongArch synchronization functions. > + > + Copyright (c) 2022, Loongson Technology Corporation Limited. All rights > reserved. > + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > + > +/** > + Performs an atomic compare exchange operation on a 16-bit > + unsigned integer. > + > + Performs an atomic compare exchange operation on the 16-bit > + unsigned integer specified by Value. If Value is equal to > + CompareValue, then Value is set to ExchangeValue and > + CompareValue is returned. If Value is not equal to > + CompareValue, then Value is returned. The compare exchange > + operation must be performed using MP safe mechanisms. > + > + @param Value A pointer to the 16-bit value for the > +compare exchange operation. > + @param CompareValue 16-bit value used in compare operation. > + @param ExchangeValue 16-bit value used in exchange operation. > + > + @return The original *Value before exchange. I see the modifiers (e.g., IN and/or OUT) are not assigned to @param of arguments in the function header across the source files in this patch set. I know some old source files don't have modifiers neither, however, we should have those in the new source file according to the coding standard. > + > +**/ > +UINT16 > +EFIAPI > +InternalSyncCompareExchange16 ( > + IN volatile UINT16 *Value, > + IN UINT16CompareValue, > + IN UINT16ExchangeValue > + ) > +{ > + UINT32 RetValue, Temp, Shift; > + UINT64 Mask, LocalCompareValue, LocalExchangeValue; > + volatile UINT32 *Ptr32; I can't find the statement in the edk2 C coding standard spec, however as I can remember each local variable should start at a new line. > + > + /* Check that ptr is naturally aligned */ > + ASSERT(!((UINT64)Value & (sizeof(Value) - 1))); Please use double back slash for the comment. > + > + /* Mask inputs to the correct size. */ > + Mask = (((~0UL) - (1UL << (0)) + 1) & (~0UL >> (64 - 1 - ((sizeof(UINT16) > * 8) > - 1; > + LocalCompareValue = ((UINT64)CompareValue) & Mask; > + LocalExchangeValue = ((UINT64)ExchangeValue) & Mask; > + > + /* > + * Calculate a shift & mask that correspond to the value we wish to > + * compare & exchange within the naturally aligned 4 byte integer > + * that includes it. > + */ > + Shift = (UINT64)Value & 0x3; > + Shift *= 8; /* BITS_PER_BYTE */ > + LocalCompareValue <<= Shift; > + LocalExchangeValue <<= Shift; > + Mask <<= Shift; > + > + /* > + * Calculate a pointer to the naturally aligned 4 byte integer that > + * includes our byte of interest, and load its value. > + */ > + Ptr32 = (UINT32 *)((UINT64)Value & ~0x3); > + > + __asm__ __volatile__ ( > +"1: \n" > +"ll.w %0, %3 \n" > +"and %1, %0, %4 \n" > +"bne %1, %5, 2f \n" > +"or%1,