From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=mzyix5Pa; spf=pass (domain: linaro.org, ip: 209.85.128.66, mailfrom: leif.lindholm@linaro.org) Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by groups.io with SMTP; Thu, 26 Sep 2019 17:20:04 -0700 Received: by mail-wm1-f66.google.com with SMTP id 5so4640302wmg.0 for ; Thu, 26 Sep 2019 17:20:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=kU3kWAY5aVghKqYvNpr43ywg4Q9YaUlyUcmm5hfTfSg=; b=mzyix5PaDz8ykrZ/J/H3vwLTa8DDlqLjObExT6ooLxQI2RjELgW/Iq81tl1m+R87qA ihz7z+uW58FhlacO4mpbcBkeFL3qFZ3yODWy5aG7DhofA4zyrFKcrqwWIos4YPAJ7+W7 /0qUYg75PrVEGZPF66T/iBGj3j1y9T5WrXhZ625bnr+mly3gBJtLklSEAWbmvpWjD/x6 ZvUAnIAgclTa6HIveRPmvO5gI1zKycd2ZPDYqJpiMYI68AUX2hqttQFAeZI8z+/3GAt5 nVFykpWBa6Goajg1ZZzILz6anWr2VL06CyEPudQL+cLybN7dLeh+Zzhy0gY/GnriwqAR LdQQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=kU3kWAY5aVghKqYvNpr43ywg4Q9YaUlyUcmm5hfTfSg=; b=nEXpiI84KC8A2QZmqI/DUvfXzEUyZ5Y8sXycUFXMeG/n6qVc759HBVqtAqvpo62kYI R9uepM0iLiHY3rebhofDEYA0EbueH3T5ZtljkeRXg84dFpDJrn5wESv+HAe1vI25GqrN ekLqz9oOFyeshjp9eZ3fDQhAH0sts9SMucEx6dfYqwiFJ678aHBTQYyF812N6ChLVVEA Ujp3yZd3N5a9dgVoi2GwmbTzgd0Srz+TfryXZGnwiOryDQsIdmVH+dQFFo4dIjt9irgk mN6/nl//pZBG7m1V2CD7W+fqtAxoHxrKbFo+IAPIDIOIA2BK8Zub2tlaLFnn8ByBmG1U B0VA== X-Gm-Message-State: APjAAAUJMIa4ixAZUTEjGHs5JCYJYHJHQJIzRhEFhvoN2zfqXTIiLpr3 fWy+dDXrXkIvjG3GyoP633ZLtGE1SxZbMg== X-Google-Smtp-Source: APXvYqxpmHyX4p917jG1YszWvdPYDCh2VZVD0bM6jYSrIW75BaaXhmjQoXT0UBECFkM8KKTidlF63w== X-Received: by 2002:a1c:3bd6:: with SMTP id i205mr4783173wma.135.1569543601956; Thu, 26 Sep 2019 17:20:01 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id n1sm2232320wrg.67.2019.09.26.17.20.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Sep 2019 17:20:01 -0700 (PDT) Date: Fri, 27 Sep 2019 01:19:59 +0100 From: "Leif Lindholm" To: devel@edk2.groups.io, abner.chang@hpe.com Subject: Re: [edk2-devel] [edk2-staging/RISC-V-V2 PATCH v2 12/29] MdePkg/BaseSynchronizationLib: RISC-V cache related code. Message-ID: <20190927001959.GO25504@bivouac.eciton.net> References: <1569198715-31552-1-git-send-email-abner.chang@hpe.com> <1569198715-31552-14-git-send-email-abner.chang@hpe.com> MIME-Version: 1.0 In-Reply-To: <1569198715-31552-14-git-send-email-abner.chang@hpe.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Sep 23, 2019 at 08:31:38AM +0800, Abner Chang wrote: > Support RISC-V cache related functions. > > Signed-off-by: Abner Chang What is the purpose of the .c file? Currently all I see it doing it printing some messages before calling into assembly, and forcing Hungarian notation on the filenames. There is no value in providing runtime notifications that synchronization is not required - this is a library, we wil learn at build time if our code is impacted by lack of some primitive. Can you drop the .c file, rename the .S file Synchronization.S, and renaming the functions in the .S: InternalSyncCompareExchange32 InternalSyncCompareExchange64 InternalSyncIncrement InternalSyncDecrement U500 still builds fine after this change. / Leif > --- > .../BaseSynchronizationLib.inf | 6 + > .../RiscV64/Synchronization.c | 183 +++++++++++++++++++++ > .../RiscV64/SynchronizationAsm.S | 78 +++++++++ > 3 files changed, 267 insertions(+) > create mode 100644 MdePkg/Library/BaseSynchronizationLib/RiscV64/Synchronization.c > create mode 100644 MdePkg/Library/BaseSynchronizationLib/RiscV64/SynchronizationAsm.S > > diff --git a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > index 446bc19..c16ef9d 100755 > --- a/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > +++ b/MdePkg/Library/BaseSynchronizationLib/BaseSynchronizationLib.inf > @@ -3,6 +3,7 @@ > # > # Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
> # Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> +# Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development LP. All rights reserved.
> # > # SPDX-License-Identifier: BSD-2-Clause-Patent > # > @@ -78,6 +79,11 @@ > AArch64/Synchronization.S | GCC > AArch64/Synchronization.asm | MSFT > > +[Sources.RISCV64] > + Synchronization.c > + RiscV64/Synchronization.c | GCC > + RiscV64/SynchronizationAsm.S > + > [Packages] > MdePkg/MdePkg.dec > > diff --git a/MdePkg/Library/BaseSynchronizationLib/RiscV64/Synchronization.c b/MdePkg/Library/BaseSynchronizationLib/RiscV64/Synchronization.c > new file mode 100644 > index 0000000..e210b74 > --- /dev/null > +++ b/MdePkg/Library/BaseSynchronizationLib/RiscV64/Synchronization.c > @@ -0,0 +1,183 @@ > +/** @file > + Implementation of synchronization functions on RISC-V > + > + Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development LP. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > +**/ > + > +#include > + > +UINT32 > +SyncCompareExchange32 ( > + IN volatile UINT32 *Value, > + IN UINT32 CompareValue, > + IN UINT32 ExchangeValue > +); > + > +UINT64 > +SyncCompareExchange64 ( > + IN volatile UINT64 *Value, > + IN UINT64 CompareValue, > + IN UINT64 ExchangeValue > +); > + > +UINT32 > +SyncSyncIncrement32 ( > + IN volatile UINT32 *Value > + ); > + > +UINT32 > +SyncSyncDecrement32 ( > + IN volatile UINT32 *Value > + ); > + > +/** > + 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. > + > +**/ > +UINT16 > +EFIAPI > +InternalSyncCompareExchange16 ( > + IN volatile UINT16 *Value, > + IN UINT16 CompareValue, > + IN UINT16 ExchangeValue > + ) > +{ > + DEBUG((DEBUG_ERROR, "%a:RISC-V does not support 16-bit AMO operation\n", __FUNCTION__)); > + ASSERT (FALSE); > + return 0; > +} > + > +/** > + Performs an atomic compare exchange operation on a 32-bit > + unsigned integer. > + > + Performs an atomic compare exchange operation on the 32-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 32-bit value for the > + compare exchange operation. > + @param CompareValue 32-bit value used in compare operation. > + @param ExchangeValue 32-bit value used in exchange operation. > + > + @return The original *Value before exchange. > + > +**/ > +UINT32 > +EFIAPI > +InternalSyncCompareExchange32 ( > + IN volatile UINT32 *Value, > + IN UINT32 CompareValue, > + IN UINT32 ExchangeValue > + ) > +{ > + > + if (((UINTN)Value % sizeof (UINT32)) != 0) { > + DEBUG((DEBUG_ERROR, "%a:Value pointer must aligned at natural address.\n", __FUNCTION__)); > + ASSERT (FALSE); > + } > + return SyncCompareExchange32(Value, CompareValue, ExchangeValue); > +} > + > +/** > + Performs an atomic compare exchange operation on a 64-bit unsigned integer. > + > + Performs an atomic compare exchange operation on the 64-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 64-bit value for the compare exchange > + operation. > + @param CompareValue 64-bit value used in compare operation. > + @param ExchangeValue 64-bit value used in exchange operation. > + > + @return The original *Value before exchange. > + > +**/ > +UINT64 > +EFIAPI > +InternalSyncCompareExchange64 ( > + IN volatile UINT64 *Value, > + IN UINT64 CompareValue, > + IN UINT64 ExchangeValue > + ) > +{ > + if (((UINTN)Value % sizeof (UINT64)) != 0) { > + DEBUG((DEBUG_ERROR, "%a:Value pointer must aligned at natural address.\n", __FUNCTION__)); > + ASSERT (FALSE); > + } > + return SyncCompareExchange64 (Value, CompareValue, ExchangeValue); > +} > + > +/** > + Performs an atomic increment of an 32-bit unsigned integer. > + > + Performs an atomic increment of the 32-bit unsigned integer specified by > + Value and returns the incremented value. The increment operation must be > + performed using MP safe mechanisms. The state of the return value is not > + guaranteed to be MP safe. > + > + @param Value A pointer to the 32-bit value to increment. > + > + @return The incremented value. > + > +**/ > +UINT32 > +EFIAPI > +InternalSyncIncrement ( > + IN volatile UINT32 *Value > + ) > +{ > + if (((UINTN)Value % sizeof (UINT32)) != 0) { > + DEBUG((DEBUG_ERROR, "%a:Value pointer must aligned at natural address.\n", __FUNCTION__)); > + ASSERT (FALSE); > + } > + return SyncSyncIncrement32 (Value); > +} > + > +/** > + Performs an atomic decrement of an 32-bit unsigned integer. > + > + Performs an atomic decrement of the 32-bit unsigned integer specified by > + Value and returns the decrement value. The decrement operation must be > + performed using MP safe mechanisms. The state of the return value is not > + guaranteed to be MP safe. > + > + @param Value A pointer to the 32-bit value to decrement. > + > + @return The decrement value. > + > +**/ > +UINT32 > +EFIAPI > +InternalSyncDecrement ( > + IN volatile UINT32 *Value > + ) > +{ > + if (((UINTN)Value % sizeof (UINT32)) != 0) { > + DEBUG((DEBUG_ERROR, "%a:Value pointer must aligned at natural address.\n", __FUNCTION__)); > + ASSERT (FALSE); > + } > + return SyncSyncDecrement32 (Value); > +} > diff --git a/MdePkg/Library/BaseSynchronizationLib/RiscV64/SynchronizationAsm.S b/MdePkg/Library/BaseSynchronizationLib/RiscV64/SynchronizationAsm.S > new file mode 100644 > index 0000000..943e274 > --- /dev/null > +++ b/MdePkg/Library/BaseSynchronizationLib/RiscV64/SynchronizationAsm.S > @@ -0,0 +1,78 @@ > +//------------------------------------------------------------------------------ > +// > +// RISC-V synchronization functions. > +// > +// Copyright (c) 2016 - 2019, Hewlett Packard Enterprise Development LP. All rights reserved.
> +// > +// SPDX-License-Identifier: BSD-2-Clause-Patent > +// > +//------------------------------------------------------------------------------ > +#include > + > +.data > + > +.text > +.align 3 > + > +.global ASM_PFX(SyncCompareExchange32) > +.global ASM_PFX(SyncCompareExchange64) > +.global ASM_PFX(SyncSyncIncrement32) > +.global ASM_PFX(SyncSyncDecrement32) > + > +// > +// ompare and xchange a 32-bit value. > +// > +// @param a0 : Pointer to 32-bit value. > +// @param a1 : Compare value. > +// @param a2 : Exchange value. > +// > +ASM_PFX (SyncCompareExchange32): > + lr.w a3, (a0) // Load the value from a0 and make > + // the reservation of address. > + bne a3, a1, exit > + sc.w a3, a2, (a0) // Write the value back to the address. > + mv a3, a1 > +exit: > + mv a0, a3 > + ret > + > +.global ASM_PFX(SyncCompareExchange64) > + > +// > +// Compare and xchange a 64-bit value. > +// > +// @param a0 : Pointer to 64-bit value. > +// @param a1 : Compare value. > +// @param a2 : Exchange value. > +// > +ASM_PFX (SyncCompareExchange64): > + lr.d a3, (a0) // Load the value from a0 and make > + // the reservation of address. > + bne a3, a1, exit > + sc.d a3, a2, (a0) // Write the value back to the address. > + mv a3, a1 > +exit2: > + mv a0, a3 > + ret > + > +// > +// Performs an atomic increment of an 32-bit unsigned integer. > +// > +// @param a0 : Pointer to 32-bit value. > +// > +ASM_PFX (SyncSyncIncrement32): > + li a1, 1 > + amoadd.w a2, a1, (a0) > + mv a0, a2 > + ret > + > +// > +// Performs an atomic decrement of an 32-bit unsigned integer. > +// > +// @param a0 : Pointer to 32-bit value. > +// > +ASM_PFX (SyncSyncDecrement32): > + li a1, -1 > + amoadd.w a2, a1, (a0) > + mv a0, a2 > + ret > -- > 2.7.4 > > > >