From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-qt1-f175.google.com (mail-qt1-f175.google.com [209.85.160.175]) by mx.groups.io with SMTP id smtpd.web11.43271.1679939959784379541 for ; Mon, 27 Mar 2023 10:59:20 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@rivosinc-com.20210112.gappssmtp.com header.s=20210112 header.b=pwuCgPcN; spf=pass (domain: rivosinc.com, ip: 209.85.160.175, mailfrom: dhaval@rivosinc.com) Received: by mail-qt1-f175.google.com with SMTP id n14so9400667qta.10 for ; Mon, 27 Mar 2023 10:59:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20210112.gappssmtp.com; s=20210112; t=1679939959; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=co/ndKNvaCVP4KWef/I6hKo9KyZP7TfK95SGNk1Di1M=; b=pwuCgPcNi0dSxaroJIT1rsup4GgS7U4R6qYNVrehHwirqzjVgM93A72N/5nMI4lRQy ntjmY8zyVWYWCrLgWUfJCtdj3c8ubEy2NK6QtiW1WRHfzfVU+3zlsqlYeRpP5XDMo5g5 LkMW/10LZVXpW9uzhDaYVeK/p/z0GMjJgvGDLqw4gtxJl2aiG+cPAIR6VZDc7XhACZP+ mtUpIn+VWrBnTzmrp4FYt7u4uivrHdlPZD1G6W5e0JYfC6dPQb2lhynlRzejYenZ6jYy mW/i4UwTKOHesH06aiXGgHE7qarL/9wAU0BFQdidyOtPhKjVDuoNb7rTUe1XwSQeL8NA llvw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679939959; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=co/ndKNvaCVP4KWef/I6hKo9KyZP7TfK95SGNk1Di1M=; b=zRpVQkYTQV038Nuwu2K+Qe7U1Pscp3c01Qy4MW9+yFJbEvD2DNF6Nsjhi8Jew3q3QR uaAu25N67yCpW74GKNZ6xxfKfPn6Sx/ZoRXZvfCnCRs0dS6eZVvbPafVN+IMEIP/D/IJ I+HDRwTh5pUiIFHNl4f++v/U9gaKu4XIKmWoTxcyEx2rvR4i6LI96GZQQcMSwAvscAns DHFXeDpurPPsZ18TZmiy1vR3rJKMqhbOZs93a9TUMhqw0OBNlrfpD44KoI7I0jFyMy4h crMlNC1znwJ8dCWvR28tp4FsONNdgoOftotRFHcjt2oGrJ5vmdLtmcbyVGzqg9y2JQ5f 5ElA== X-Gm-Message-State: AO0yUKUSil/dlBNsmprEpL7VJVHbmmt7sOaBSFoTdkDiysMB4w/JEfkj M0Pp6Q6273zN5c1z3JtGTdcHDcVPP0tNz3mnmLv9SFF3LxSB6Tzy1ac= X-Google-Smtp-Source: AK7set9jur5+n9JqFROd/arkGFUKrrzsXV7foVG9Bg5PaRpnluLEFCcoTGKZzgZXuFiHX1E8ISdGEh+zNuzHVAzoIvs= X-Received: by 2002:a05:622a:1a0d:b0:3d7:9d03:75ae with SMTP id f13-20020a05622a1a0d00b003d79d0375aemr4771233qtb.10.1679939958752; Mon, 27 Mar 2023 10:59:18 -0700 (PDT) MIME-Version: 1.0 References: <20230324154342.180062-1-dhaval@rivosinc.com> <20230324154342.180062-2-dhaval@rivosinc.com> In-Reply-To: From: "Dhaval Sharma" Date: Mon, 27 Mar 2023 23:29:07 +0530 Message-ID: Subject: Re: [PATCH v1 1/2] MdePkg/BaseCacheMaintenanceLib: Enable RISCV CMO To: Sunil V L Cc: devel@edk2.groups.io, Andrei Warkentin , Daniel Schaefer Content-Type: multipart/alternative; boundary="0000000000007c625f05f7e5809b" --0000000000007c625f05f7e5809b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable My comments inline: On Mon, Mar 27, 2023 at 9:12=E2=80=AFPM Sunil V L wrote: > Hi Dhaval, > > Thank you for looking at CMO support! > > General comments first: > 1) Please have a cover letter patch and move some part of the commit message to cover letter. Please CC all maintainers in the cover letter > also. > https://edk2.groups.io/g/devel/message/101795?p=3D%2C%2C%2C20%2C0%2C0%2C0%3= A%3Arecentpostdate%2Fsticky%2C%2Ccmo%2C20%2C2%2C0%2C97826395. Is this the one you are looking for? > 2) Please run BaseTools/Scripts/GetMaintainer.py and CC all maintainers. > Sure. > 3) Follow > > https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-= Process > > Have you run CI tests? > I actually did run it but I believe the current edk2 CI is using a GCC5 based compiler. Hence it failed as it did not recognize cmo instructions as expected. So I submitted this as WIP patch to sort that out first. Do let me know if I can follow any other better process here. > On Fri, Mar 24, 2023 at 09:13:41PM +0530, Dhaval Sharma wrote: > > Adding code to support Cache Management Operations > > (CMO) defined by RV spec https://github.com/riscv/riscv-CMOs > > Notes: > > 1. CMO only supports block based Operations. Meaning complete > > cache flush/invd/clean Operations are not available > > 2. Current implementation uses ifence instructions but it > > maybe platform specific. Many platforms may not support cache > > Operations based on ifence. > fence.i? Ack. > > > IMO, it is better to add a new library such as BaseRiscV64CMOLib and > included conditionally in the DSC for the platforms which support CMO. > BaseCacheMaintenanceLib will continue to have default fence.i > implementation. Is there an issue with this? > There are 2 libraries involved here. 1. BaseCacheMaintenanceLib. It is a generic Lib for multiple archs. So yes it is possible to create another Lib, but I was thinking if it is possible somehow to create a RV specific Lib. 2. BaseLib which contains required .S files. For CBO I have added a separate .S. Again this is generic Baselib for all Arch. So we need to be able to differentiate in DSC now for both these libs. I am not sure if this is the best way to address this. I could try to do inline assembly within CMOCachelib to address #2. > > 3. For now adding CMO on top of ifence as it is not considered > > harmful. > > 4. This requires support for GCC12.2 onwards. > > > Yeah, this is another challenge like zifencei_zicsr which we could > workaround and support both older and newer tool chain. But for CMO, > I don't see any option but to support only GCC12.2+. > How do we support this in CI? > Thanks, > Sunil > --=20 Thanks! =3DD --0000000000007c625f05f7e5809b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
My comments inline:

On Mon, Mar 27, 2023 at 9:12=E2=80= =AFPM Sunil V L <sunilvl@ven= tanamicro.com> wrote:
Hi Dhaval,

Thank you for looking at CMO support!

General comments first:
1) Please have a cover letter patch and move some part of the commit=C2=A0<= /blockquote>
message to cover letter. Please CC all maintainers in the cover letter
also.

=C2=A0
2) Please run BaseTools/Scripts/GetMaintainer.py and CC all maintainers.

Sure.
3) Follow
https://github.com/t= ianocore/tianocore.github.io/wiki/EDK-II-Development-Process

Have you run CI tests?
I actually did run it but I bel= ieve the current edk2 CI is using a GCC5 based compiler. Hence it failed as= it did not recognize cmo instructions as expected. So I submitted this as = WIP patch to sort that out first.
Do let me know if I can follow = any other better process here.


On Fri, Mar 24, 2023 at 09:13:41PM +0530, Dhaval Sharma wrote:
> Adding code to support Cache Management Operations
> (CMO) defined by RV spec https://github.com/riscv/riscv-CMOs=
> Notes:
> 1. CMO only supports block based Operations. Meaning complete
> cache flush/invd/clean Operations are not available
> 2. Current implementation uses ifence instructions but it
> maybe platform specific. Many platforms may not support cache
> Operations based on ifence.
fence.i?

Ack.=C2=A0


IMO, it is better to add a new library such as BaseRiscV64CMOLib and
included conditionally in the DSC for the platforms which support CMO.
BaseCacheMaintenanceLib will continue to have default fence.i
implementation. Is there an issue with this?
There are= 2 libraries involved here. 1. BaseCacheMaintenanceLib. It is a generic Lib= for multiple archs. So yes it is possible to create another Lib, but I was= thinking if it is possible somehow to create a RV specific Lib.
= 2. BaseLib which contains required .S files. For CBO I have added a separat= e .S. Again this is generic Baselib for all Arch. So we need to be able to = differentiate in DSC now for both these libs. I am not sure if this is the<= /div>
best way to address this. I could try to do inline assembly withi= n CMOCachelib to address #2.
=C2=A0
> 3. For now adding CMO on top of ifence as it is not considered
> harmful.
> 4. This requires support for GCC12.2 onwards.
>
Yeah, this is another challenge like zifencei_zicsr which we could
workaround and support both older and newer tool chain. But for CMO,
I don't see any option but to support only GCC12.2+.

How do we support this in CI?


Thanks,
Sunil


--
Thanks!
=3DD
--0000000000007c625f05f7e5809b--