From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 0D438AC111A for ; Tue, 31 Oct 2023 10:42:02 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=IJ4ozBf2/JEdVskRZIHZP1ZIAwXaYpYvVhQvpsfxac8=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1698748921; v=1; b=wdKqArskJ2htZQxdFVJ5B9Xvc98Gh0ymx6zb3Mx0iQfdPnsMiN4sGs/fX4Du+ywKcxBQy55l YrlqdeblihV4m9z9b9WDb9Ji0qAR2BG3cBWhhlFk9AR29oK/vLf6pUgwxCOcVHDM6aZrj78N/5c fY0a5n+vgoE7mayPzmTz/1fM= X-Received: by 127.0.0.2 with SMTP id f3A3YY7687511xmzqIR15ZWW; Tue, 31 Oct 2023 03:42:01 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.183350.1698748921229481098 for ; Tue, 31 Oct 2023 03:42:01 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-626-1GHdfeS0OFCJOiNuAkAQLw-1; Tue, 31 Oct 2023 06:41:57 -0400 X-MC-Unique: 1GHdfeS0OFCJOiNuAkAQLw-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D2DD7881F0D; Tue, 31 Oct 2023 10:41:56 +0000 (UTC) X-Received: from [10.39.195.34] (unknown [10.39.195.34]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D2B912166B26; Tue, 31 Oct 2023 10:41:54 +0000 (UTC) Message-ID: <757acfa3-ed57-ed6e-5d43-a87398d6c5de@redhat.com> Date: Tue, 31 Oct 2023 11:41:49 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v7 4/5] MdePkg: Utilize Cache Management Operations Implementation For RISC-V To: Sunil V L , devel@edk2.groups.io, dhaval@rivosinc.com Cc: Michael D Kinney , Liming Gao , Zhiguang Liu References: <20231029144613.150580-1-dhaval@rivosinc.com> <20231029144613.150580-5-dhaval@rivosinc.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: P9W1vyVFwurAjCiUuQ5r083Cx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=wdKqArsk; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 10/30/23 12:18, Sunil V L wrote: > On Sun, Oct 29, 2023 at 08:16:12PM +0530, Dhaval Sharma wrote: >> Use newly defined cache management operations for RISC-V where possible >> It builds up on the support added for RISC-V cache management >> instructions in BaseLib. >> Cc: Michael D Kinney >> Cc: Liming Gao >> Cc: Zhiguang Liu >> Cc: Laszlo Ersek >> >> Signed-off-by: Dhaval Sharma >> Acked-by: Laszlo Ersek >> --- >> >> Notes: >> V7: >> - Added PcdLib >> - Restructure DEBUG message based on feedback on V6 >> - Make naming consistent to CMO, remove all CBO references >> - Add ASSERT for not supported functions instead of plain debug mess= age >> - Added RB tag >> V6: >> - Utilize cache management instructions if HW supports it >> This patch is part of restructuring on top of v5 >> >> MdePkg/MdePkg.dec | = 8 + >> MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf | = 5 + >> MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | 16= 8 +++++++++++++++++--- >> MdePkg/MdePkg.uni | = 4 + >> 4 files changed, 165 insertions(+), 20 deletions(-) >> >> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec >> index ac54338089e8..fa92673ff633 100644 >> --- a/MdePkg/MdePkg.dec >> +++ b/MdePkg/MdePkg.dec >> @@ -2399,6 +2399,14 @@ [PcdsFixedAtBuild.AARCH64, PcdsPatchableInModule.= AARCH64] >> # @Prompt CPU Rng algorithm's GUID. >> gEfiMdePkgTokenSpaceGuid.PcdCpuRngSupportedAlgorithm|{0x00,0x00,0x00,= 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00}|VOID*|0x0= 0000037 >> =20 >> +[PcdsFixedAtBuild.RISCV64, PcdsPatchableInModule.RISCV64] >> + # >> + # Configurability to override RISC-V CPU Features >> + # BIT 0 =3D Cache Management Operations. This bit is relevant only if >> + # previous stage has feature enabled and user wants to disable it. > NIT: I am wondering whether PcdRiscVCpuFeatureDisable is better so that > it is explicit. >=20 >> + # >> + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride|0xFFFFFFFFFFFFFFFF|U= INT64|0x69 >> + > Instead of this, can default value match only those features which are > enabled by default for qemu virt machine? That way, I think we can avoid > having this PCD defined again in RiscVVirt. I proposed the all-bits-one value. First, PcdRiscVFeatureOverride is in MdePkg, which is much more general than QEMU. Second, the all-bits-one pattern means that the MdePkg.dec default does not try to disable any features enabled by earlier components in the boot process, *even such features* that it does not know about specifically (i.e., those for which edk2 does not define a specific feature bit). IMO, for any "feature negotiation" bitmask, there are two choices: - clear all bits except those that we know about (and know how to use), - use all-bits-one. The first option is good if unknown (future) features are expected to *break* us. The second option is good if we are unaffected by extra (future) features, and we want to keep everything enabled for the runtime OS that comes after us. I understood that we had case #2 here. If we have case#1 instead, then we should indeed limit the bitmask to those features that *core edk2* knows about. But even in that case, I think MdePkg.dec should be as permissive as possible, and any QEMU-specific limitation should go into the OVMF DSC file. >=20 >> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] >> ## This value is used to set the base address of PCI express hierarch= y. >> # @Prompt PCI Express Base Address. >> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenance= Lib.inf b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.in= f >> index 6fd9cbe5f6c9..601a38d6c109 100644 >> --- a/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf >> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/BaseCacheMaintenanceLib.inf >> @@ -56,3 +56,8 @@ [LibraryClasses] >> BaseLib >> DebugLib >> =20 >> +[LibraryClasses.RISCV64] >> + PcdLib >> + >> +[Pcd.RISCV64] >> + gEfiMdePkgTokenSpaceGuid.PcdRiscVFeatureOverride ## CONSUMES >> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/MdePk= g/Library/BaseCacheMaintenanceLib/RiscVCache.c >> index 4eb18edb9aa7..5b3104afb67e 100644 >> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c >> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c >> @@ -2,6 +2,7 @@ >> RISC-V specific functionality for cache. >> =20 >> Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All ri= ghts reserved.
>> + Copyright (c) 2023, Rivos Inc. All rights reserved.
>> =20 >> SPDX-License-Identifier: BSD-2-Clause-Patent >> **/ >> @@ -9,10 +10,115 @@ >> #include >> #include >> #include >> +#include >> + >> +// >> +// TODO: Grab cache block size and make Cache Management Operation >> +// enabling decision based on RISC-V CPU HOB in >> +// future when it is available. >> +// >> +#define RISCV_CACHE_BLOCK_SIZE 64 >> +#define RISCV_CPU_FEATURE_CMO_BITMASK 0x1 >> + > Can we define these bits in the header file so that the definitions can > be used by multiple modules? I noticed this too, but didn't call it out, because the DEC file sufficiently explained the bit meaning(s). Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110397): https://edk2.groups.io/g/devel/message/110397 Mute This Topic: https://groups.io/mt/102256468/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-