From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-002e3701.pphosted.com (mx0a-002e3701.pphosted.com [148.163.147.86]) by mx.groups.io with SMTP id smtpd.web10.36010.1590763302302841301 for ; Fri, 29 May 2020 07:41:42 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: hpe.com, ip: 148.163.147.86, mailfrom: prvs=041805dddd=abner.chang@hpe.com) Received: from pps.filterd (m0150241.ppops.net [127.0.0.1]) by mx0a-002e3701.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 04TEbr0d001014; Fri, 29 May 2020 14:41:42 GMT Received: from g4t3425.houston.hpe.com (g4t3425.houston.hpe.com [15.241.140.78]) by mx0a-002e3701.pphosted.com with ESMTP id 31b0njt2k5-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 29 May 2020 14:41:41 +0000 Received: from G1W8107.americas.hpqcorp.net (g1w8107.austin.hp.com [16.193.72.59]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by g4t3425.houston.hpe.com (Postfix) with ESMTPS id C8DDC8D; Fri, 29 May 2020 14:41:40 +0000 (UTC) Received: from G4W9121.americas.hpqcorp.net (2002:10d2:1510::10d2:1510) by G1W8107.americas.hpqcorp.net (2002:10c1:483b::10c1:483b) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Fri, 29 May 2020 14:41:40 +0000 Received: from NAM04-CO1-obe.outbound.protection.outlook.com (15.241.52.10) by G4W9121.americas.hpqcorp.net (16.210.21.16) with Microsoft SMTP Server (TLS) id 15.0.1497.2 via Frontend Transport; Fri, 29 May 2020 14:41:40 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ZWnz/fLV5MwlfTPty0M1i6FaNcfUONh6nmBPbHb5bg4mDgzXQz2JQr8waYxqTeaguABs60ioTGTWqyF7XadEPy7U/WIQ8ZjQbul9yCl8UxQ08T5hsD1GXHPSD9rGcADjt1H5/u/37sxrE4oUN6R2q7ucOYSA6DIi0B3r8BHYDNnOTYkJHQpiiKNrC8lBvjD96cO0apmPGos7nM+h97j/OJ2OzvpjtnVE1zPBfmzXh10ekWUkHbuFzqdly290YLGQAvT0T4C+WKHKfntGhKcE8SIdUSF2yEDw9vecPtVScrVwSK620m/Uq1xQ6QuYYsTz49pY1DiWndYaRRHkUnExXw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=VFciw14NKdONFoL1vaTFzCum9vBia7o3s8SiBij2IRg=; b=HXb9ohgTwZocYRquqvMJHDM92NOXd2O2HAdSanuZBfpGX2W25GvHI7KQmeG/DSX6/YSQmvTpG0EZaZ02Cxhjr1B7cW4PqL034VoDMBczN9EtJFtAbWiEjkOlUHfGgS37gcTt/4EdXvWAiK4gsjbZbSihN4O3+OG/qhj20iEG6H3mTuNrWGmEjvrAJmj0ESEZcPJwhjFj7qmsIItLF3r5/vI48QdKXcsLmQfnJO8YFK/NzGROMzv2AqHxqaQ6TsWgViF6u3HgRbyuS8AsT9GxIMNRCwsNmBW1QlXoOaOFxat1zVGWGwEJxn/nTWrzbeDjqMWoRG/qJlFZzLzDn36y/A== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=hpe.com; dmarc=pass action=none header.from=hpe.com; dkim=pass header.d=hpe.com; arc=none Received: from TU4PR8401MB1182.NAMPRD84.PROD.OUTLOOK.COM (2a01:111:e400:7714::14) by TU4PR8401MB0880.NAMPRD84.PROD.OUTLOOK.COM (2a01:111:e400:770f::7) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3045.18; Fri, 29 May 2020 14:41:37 +0000 Received: from TU4PR8401MB1182.NAMPRD84.PROD.OUTLOOK.COM ([fe80::29a3:bff2:a760:1f07]) by TU4PR8401MB1182.NAMPRD84.PROD.OUTLOOK.COM ([fe80::29a3:bff2:a760:1f07%9]) with mapi id 15.20.3045.018; Fri, 29 May 2020 14:41:37 +0000 From: "Abner Chang" To: Leif Lindholm CC: "Schaefer, Daniel (DualStudy)" , "devel@edk2.groups.io" , "Chen, Gilbert" , "Kinney, Michael D" , "Bret.Barkelew@microsoft.com" , "sean.brogan@microsoft.com" Subject: Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms Thread-Topic: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2-platforms Thread-Index: AQHWLsAz7/HbRnILxkeVlbDXgiK4p6ixI+cAgAADDYCAAPJgcIALVr+AgAHALgA= Date: Fri, 29 May 2020 14:41:37 +0000 Message-ID: References: <20200515133937.29909-1-daniel.schaefer@hpe.com> <20200520114336.GK1923@vanye> <6f0d755e-4e69-5080-ef69-caf7259ce9ee@hpe.com> <27d3bf55-eb2e-75e1-e5fa-17af59e105aa@hpe.com> <20200528115449.GE1923@vanye> In-Reply-To: <20200528115449.GE1923@vanye> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: nuviainc.com; dkim=none (message not signed) header.d=none;nuviainc.com; dmarc=none action=none header.from=hpe.com; x-originating-ip: [1.34.113.40] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: eecd73a8-8089-46d8-c352-08d803de648d x-ms-traffictypediagnostic: TU4PR8401MB0880: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:6108; x-forefront-prvs: 04180B6720 x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: NZXH53rbCsP/V0uhv4iDtB9pCNHODbPQwq+PmmjSwtFn9HQnHYOTwixYAPTniO5zFu/JMa7U4FaEmNnVxEUpYyHBQS/PrfXSeVla7daQpUtSTux+YiieiLOfSsJKs/mEy/yW1x27uZlGJjoOaDwlZUT28x+8SifqepGE96g7ceVDsLbizUs3Olbrh6Gyy+eEiaDaE/rY9YIfuKynr5Rk6Vx3DKj9q2bEcAtsdzP1qv0k/Hfxv5trPljRYqFdo/dEBTER6/foYesEhg4nazhgWRs/MoajDykH1/rxIhuMJlwtWRSsGs46zCK6DMDG8aosi2iKaAD/qffRiUDfYHJzGnSucpwaRx0E0y3W4BhlMMV89v/xB5m5+LAJ/dfjl29/PmqQi+LyTEFuK5J7RLt/CQ== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:TU4PR8401MB1182.NAMPRD84.PROD.OUTLOOK.COM;PTR:;CAT:NONE;SFTY:;SFS:(346002)(366004)(136003)(376002)(39860400002)(396003)(33656002)(5660300002)(4326008)(186003)(6916009)(6506007)(53546011)(26005)(54906003)(71200400001)(83380400001)(316002)(7696005)(30864003)(8676002)(8936002)(66446008)(19627235002)(9686003)(64756008)(66556008)(66476007)(66946007)(76116006)(45080400002)(55016002)(478600001)(2906002)(86362001)(52536014)(966005)(579004);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: BlbEgI6ApPupqsKF9eWq9vqOLD/6MGU6NcwSeskyUFbVMKsf6KnVKqNStWEH6qBhGhBi/D5h2/gIMn/YOtO7HodrkZsYsnMqBC6+gwBoQyLFOxOjdu3MrNJwP0HKVOmUf/4NbB4Tm7zibVxQxAFaaqUuTLvetrVwxe3fNNKRltImpA5ki59aKXNGaB3LBGxAB1fwiIM5B00FGToon7hxyABjTLG/9LrqLXqWvlv4yGUEDU+vr55zTj6+YVKqEVwrNL5aFqmejltuXemnjqDPd8p+AozA/VRyIxSIqhiLFzxX6Oo6xRh5T4+wCCZeKcTKZbuq2a5KqbBUDdjdzY1j5JiZGBOg8/Qm37emjAMW9hxkUWckKaljWaoIlRK+F2dT3c1ra7aOKtM5h3p8/m1K9vdGDEExyJvK2ZlArCeUIpQS29+8m+ffd5iuJ8WraM27vSd3YdHQyRLkeT5pJpv2InTguQvc6rI86ou4qDgbhpo= X-MS-Exchange-CrossTenant-Network-Message-Id: eecd73a8-8089-46d8-c352-08d803de648d X-MS-Exchange-CrossTenant-originalarrivaltime: 29 May 2020 14:41:37.5721 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 105b2061-b669-4b31-92ac-24d304d195dc X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: mhoachKKtCkVdJXNgZ0imHeDoE6C1AOtDIomJr9CIajJ/xVksauLhHQ6gyCwJNsp3mU3kTTzxbMe5JED+brymg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: TU4PR8401MB0880 X-OriginatorOrg: hpe.com X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-HPE-SCL: -1 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216,18.0.687 definitions=2020-05-29_07:2020-05-28,2020-05-29 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 adultscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 malwarescore=0 spamscore=0 cotscore=-2147483648 suspectscore=0 bulkscore=0 mlxscore=0 mlxlogscore=999 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2004280000 definitions=main-2005290116 Content-Language: en-US Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Leif Lindholm [mailto:leif@nuviainc.com] > Sent: Thursday, May 28, 2020 7:55 PM > To: Chang, Abner (HPS SW/FW Technologist) > Cc: Schaefer, Daniel (DualStudy) ; > devel@edk2.groups.io; Chen, Gilbert ; Kinney, > Michael D ; Bret.Barkelew@microsoft.com; > sean.brogan@microsoft.com > Subject: Re: [edk2-devel] [PATCH v2 0/3] New RISC-V Patches - Why in edk2- > platforms >=20 > Hi Abner, >=20 > Sorry, I should have followed up on this sooner. >=20 > On Thu, May 21, 2020 at 06:59:20 +0000, Chang, Abner (HPS SW/FW > Technologist) wrote: > > > On 5/20/20 6:07 PM, Daniel Schaefer wrote: > > > > please reply here, fixed Mike's email address, sorry... > > > > > > > > On 5/20/20 6:03 PM, Daniel Schaefer wrote: > > > >> On 5/20/20 1:43 PM, Leif Lindholm wrote: > > > >>> On Fri, May 15, 2020 at 15:39:34 +0200, Daniel Schaefer wrote: > > > >>>> Previously we had two packages just for RISC-V on our edk2 branc= h: > > > >>>> =A0=A0 RiscVPkg and RiscVPlatformPkg They are now under > > > >>>> =A0=A0 Platform/RISC-V/PlatformPkg and Silicon/RISC-V/ProcessorP= kg > > > >>>> in edk2-platforms. > > > >>> > > > >>> Understood. I took my eye off the ball there for a while, but > > > >>> I'm a bit confused as to why RiscVPkg isn't going into EDK2. > > > >>> That is very counterintuitive. And clearly it will need > > > >>> revisiting if we are to add first-class CI checks like those we d= o with > OvmfPkg/ArmVirtPkg. > > > >> > > > >> Yes, I understand your concern. Personally I'd like it also to be > > > >> in > > > >> EDK2 straight away, however Mike, Bret and Sean have raised valid > > > >> concerns: >=20 > Can you point me to the conversation I have missed? >=20 > > > >> 1. RISC-V is very new and potentially unstable - it's quicker to > > > >> make changes in edk2-platforms. >=20 > I don't see this as a valid argument. > It's not edk2-unstable, it is edk2-platforms. >=20 > edk2-platforms exists because there used to be strong feelings against > holding *real* platforms in edk2, with edk2 being originally intended onl= y as > a code library for IBV/ISV to cherry-pick from. >=20 > But fundamentally, if code is too immature to go into the master branch of > edk2, it is too immature to go into the master branch of edk2-platforms. = If > we want edk2-might-be-a-bit-shaky-but-who-cares, > then someone will have to create it. >=20 > > > >> 2. If we define new interfaces and libraries in edk2, we can't > > > >> remove them easily because it would be a backwards-incompatible > change. > > > >> edk2-platforms isn't quite as strict. >=20 > Yes it is. > The only thing making it less strict is its contents - platform ports and= device > drivers. The changes tend to be self-contained. Where they are not, they > need to be carefully managed. >=20 > > > >> 3. Long-term, I think many agree, we should aim to move much of > > > >> the RISC-V code into UefiCpuPkg and OvmfPkg. Mike mentioned that > > > >> would need coordination with ARM maintainers because it might > > > >> make sense to move their code there as well. >=20 > I don't think there is any need to tie the two together. > Yes, UefiCpuPkg should be a generic place where not only x86 support can = be > contained, but the paths for ARM* and RISC-V into there do not have any > interdependencies. >=20 > > > >>> I *did* have some outstanding comments specifically with regards > > > >>> to large amounts of code duplication between the SMBIOS > > > >>> implementation of some closely related RISC-V platforms. That now > needs to be revisited. > > > >> > > > >> The SMBIOS code hasn't changed. It has moved to > > > >> > > > >> Silicon/SiFive/{E51,U54,U54MCCoreplex}/Library/PeiCoreInfoHobLib > > > >> You're referring to this library, right? > > > >> > > > >> They build the SMBIOS entries for a particular processor. Yes, > > > >> the values do have a lot of overlap but these files are like > > > >> configuration files. They don't do much, they only set the values > > > >> of the > > > properties. > > > >> > > > >> Currently it is not possible to let the UEFI firmware get this > > > >> information from the hardware at runtime, especially now, since > > > >> we're running in S-Mode. > > > >> To allow that, we created a RISC-V working group to be able to > > > >> retrieve all of this information dynamically from the processor > > > >> (among other goals). Then the vendor will not have to modify > > > >> these files and hardcode the values anymore. Which enables us to > > > >> create a single library for all processors. > > > >> See: https://github.com/riscv/configuration-structure > > > >> > > > >> I hope I described everything properly, please correct me > > > >> otherwise, Abner. > > [Abner] Yes almost. Thanks Daniel. > > One thing I would like to add, > > If you take a look on SiFive Core IP > > > >> INVALID URI REMOVED > > > >> om_risc-2Dv-2Dcore- > 2Dip&d=3DDwIDAw&c=3DC5b8zRQO1miGmBeVZ2LFWg&r=3D_SN6F > > > >> > ZBN4Vgi4Ulkskz6qU3NYRO03nHp9P7Z5q59A3E&m=3DCYgLjPyxDUeKYpXV_283 > 3iYF > > > >> > 0_kR2PGdGoEVD7Qare0&s=3DOQvDbn7xhejlUpQrBMNReCLl1WGUoPKdIp5Y3 > 6-6e8E > > > >> &e=3D you can see there are different SKUs of RISC-V core. Just > > > >> take some as exampl,e > > S51 - Single core > > U54 - Single core > > S76 - Single core > > U74- single core > > U54-MC - Multicore which is 4*U54 cores +1*S51 core U74-MC - Multicore > > which is 4*U74 core + 1*S7 core > > > > Those are the combinations of core IP. Silicon vendor can get those > > core IPs and combine them to the RISC-V processor. To have > > CoreInfoHobLib libraries for each different core (not multicore) to > > build up the core capability is reasonable and makes sense. For the > > multicore, it just pulling the single core CoreInfoHobLib to build up > > the SMBIOS table for the multicore processor. Those libraries look > > duplicate in logically, however only one instance of CoreInfoHobLib is > > built in for multiple identical cores in physically view. Maybe we > > still can move some identical core into the core-specific library but > > it is not worthwhile. >=20 > OK, lets start with the *full* diff of E51 and U54 from the (admittedly s= lightly > dated) devel-riscvplatforms branch: >=20 > <<< > --- ./E51/Library/PeiCoreInfoHobLib/CoreInfoHob.c 2020-05-28 > 12:12:11.211028141 +0100 > +++ ./U54/Library/PeiCoreInfoHobLib/CoreInfoHob.c 2020-05-28 > 12:12:11.211028141 +0100 > @@ -7,6 +7,8 @@ >=20 > **/ >=20 > +#include > + >=20 > [LL] > U54 inserts this file in a different location, this is not actual diverge= nce. >=20 > // > // The package level header files this module uses // @@ -19,21 +21,15 = @@ > #include #include > > #include > -#include >=20 > [LL] > Interestingly, only E51 actually includes this, but both files *use* it -= this is a > bug in U54 caused by the separation. >=20 > -#include >=20 > [LL] > This is the other end of the files placing this inlude in a different loc= ation. >=20 > -#include > - >=20 > [LL] > This header is irrelevant and unused. Present only in E51. >=20 > #include > -#include > -#include >=20 > [LL] > Included in different location for E51/U54. >=20 > #include > -#include > #include > +#include >=20 > [LL] > sbi_scratch.h included in different order between platforms. >=20 > +#include >=20 > [LL] > Included in different location for E51/U54 (other end of). >=20 > #include >=20 > /** > - Function to build core specific information HOB. RISC-V SMBIOS DXE dri= ver > collect > - this information and build SMBIOS Type44. > + Function to build core specific information HOB. >=20 > [LL] > Different documentation description for the otherwise identical functions. >=20 >=20 > @param ParentProcessorGuid Parent processor od this core. > ParentProcessorGuid > could be the same as CoreGuid if one pr= ocessor has @@ - > 41,19 +37,19 @@ > @param ParentProcessorUid Unique ID of pysical processor which ow= ns > this core. > @param HartId Hart ID of this core. > @param IsBootHart TRUE means this is the boot HART. > - @param GuidHobData Pointer to receive EFI_HOB_GUID_TYPE. > + @param GuidHobdata Pointer to > RISC_V_PROCESSOR_SPECIFIC_HOB_DATA. >=20 > [LL] > Different capitalisation of input variable and different documentation fo= r the > same parameter in the identical functions. E51 gets the former correct, U= 54 > the latter. >=20 >=20 > @return EFI_SUCCESS The PEIM initialized successfully. >=20 > **/ > EFI_STATUS > EFIAPI > -CreateE51CoreProcessorSpecificDataHob ( > +CreateU54CoreProcessorSpecificDataHob ( >=20 > [LL] > We reach the first *real* difference between the two - the name of the > function. > This could have been addressed with different .inf files with different -D > cflags. >=20 > IN EFI_GUID *ParentProcessorGuid, > IN UINTN ParentProcessorUid, > IN UINTN HartId, > IN BOOLEAN IsBootHart, > - OUT RISC_V_PROCESSOR_SPECIFIC_HOB_DATA **GuidHobData > + OUT RISC_V_PROCESSOR_SPECIFIC_HOB_DATA **GuidHobdata >=20 > [LL] > Again, difference only in capitalisation. >=20 > ) > { > RISC_V_PROCESSOR_SPECIFIC_HOB_DATA *CoreGuidHob; @@ -64,7 +60,7 > @@ >=20 > DEBUG ((DEBUG_INFO, "%a: Entry.\n", __FUNCTION__)); >=20 > - if (GuidHobData =3D=3D NULL) { > + if (GuidHobdata =3D=3D NULL) { >=20 > [LL] > Again, difference only in capitalisation. >=20 > return EFI_INVALID_PARAMETER; > } >=20 > @@ -80,7 +76,7 @@ > FirmwareContextHartSpecific, > ParentProcessorGuid, > ParentProcessorUid, > - (EFI_GUID *)PcdGetPtr (PcdSiFiveE51CoreGuid), > + (EFI_GUID *)PcdGetPtr (PcdSiFiveU54CoreGuid), >=20 > [LL] > Different Pcd names. >=20 > HartId, > IsBootHart, > &ProcessorSpecDataHob > @@ -109,7 +105,7 @@ > DEBUG ((DEBUG_INFO, " *MachineImplId =3D 0x%x\n", > ProcessorSpecDataHob.ProcessorSpecificData.MachineImplId.Value64_L)); >=20 > // > - // Build GUID HOB for E51 core, this is for SMBIOS type 44 > + // Build GUID HOB for U54 core. >=20 > [LL] > Different comments for identical code. >=20 > // > ProcessorSpecDataHobGuid =3D PcdGetPtr > (PcdProcessorSpecificDataGuidHobGuid); > CoreGuidHob =3D (RISC_V_PROCESSOR_SPECIFIC_HOB_DATA > *)BuildGuidDataHob (ProcessorSpecDataHobGuid, (VOID > *)&ProcessorSpecDataHob, sizeof > (RISC_V_PROCESSOR_SPECIFIC_HOB_DATA)); > @@ -117,7 +113,7 @@ > DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core.\n"= )); > ASSERT (FALSE); > } > - *GuidHobData =3D CoreGuidHob; > + *GuidHobdata =3D CoreGuidHob; >=20 > [LL] > Again, difference only in capitalisation. >=20 > return EFI_SUCCESS; > } >=20 > @@ -135,17 +131,21 @@ > **/ > EFI_STATUS > EFIAPI > -CreateE51ProcessorSmbiosDataHob ( > +CreateU54ProcessorSmbiosDataHob ( >=20 > [LL] > Difference in name only. >=20 > IN UINTN ProcessorUid, > - OUT RISC_V_PROCESSOR_SMBIOS_HOB_DATA **SmbiosHobPtr > + IN RISC_V_PROCESSOR_SMBIOS_HOB_DATA **SmbiosHobPtr >=20 > [LL] > I am going to go out on a limb here and suggest one of the above is incor= rect, > and once that is corrected, these two lines would be identical. >=20 > ) > { > EFI_GUID *GuidPtr; > RISC_V_PROCESSOR_TYPE4_HOB_DATA ProcessorDataHob; > RISC_V_PROCESSOR_TYPE7_HOB_DATA L1InstCacheDataHob; > + RISC_V_PROCESSOR_TYPE7_HOB_DATA L1DataCacheDataHob; > + RISC_V_PROCESSOR_TYPE7_HOB_DATA L2CacheDataHob; >=20 > [LL] > Here is the first functional difference. >=20 > RISC_V_PROCESSOR_SMBIOS_HOB_DATA SmbiosDataHob; > RISC_V_PROCESSOR_TYPE4_HOB_DATA *ProcessorDataHobPtr; > RISC_V_PROCESSOR_TYPE7_HOB_DATA *L1InstCacheDataHobPtr; > + RISC_V_PROCESSOR_TYPE7_HOB_DATA *L1DataCacheDataHobPtr; > + RISC_V_PROCESSOR_TYPE7_HOB_DATA *L2CacheDataHobPtr; >=20 > [LL] > Which could be merged with this inside an ifdef. >=20 > RISC_V_PROCESSOR_SMBIOS_HOB_DATA *SmbiosDataHobPtr; >=20 > if (SmbiosHobPtr =3D=3D NULL) { > @@ -155,7 +155,7 @@ > // Build up SMBIOS type 7 L1 instruction cache record. > // > ZeroMem((VOID *)&L1InstCacheDataHob, sizeof > (RISC_V_PROCESSOR_TYPE7_HOB_DATA)); > - CopyGuid (&L1InstCacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr > (PcdSiFiveE51CoreGuid)); > + CopyGuid (&L1InstCacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr > + (PcdSiFiveU54CoreGuid)); >=20 > [LL] > Difference in name only. >=20 > L1InstCacheDataHob.ProcessorUid =3D ProcessorUid; > L1InstCacheDataHob.SmbiosType7Cache.SocketDesignation =3D > TO_BE_FILLED_BY_VENDOR; > L1InstCacheDataHob.SmbiosType7Cache.CacheConfiguration =3D > RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_1 | \ @@ -173,7 +173,59 > @@ > GuidPtr =3D (EFI_GUID *)PcdGetPtr > (PcdProcessorSmbiosType7GuidHobGuid); > L1InstCacheDataHobPtr =3D (RISC_V_PROCESSOR_TYPE7_HOB_DATA > *)BuildGuidDataHob (GuidPtr, (VOID *)&L1InstCacheDataHob, sizeof > (RISC_V_PROCESSOR_TYPE7_HOB_DATA)); > if (L1InstCacheDataHobPtr =3D=3D NULL) { > - DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core L1 > instruction cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n")); > + DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core L1 > + instruction cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n")); >=20 > [LL] > Difference in name only. >=20 > + ASSERT (FALSE); > + } > + >=20 > [LL] > Below starts the fundamental difference between the two: >=20 > + // > + // Build up SMBIOS type 7 L1 data cache record. > + // > + ZeroMem((VOID *)&L1DataCacheDataHob, sizeof > + (RISC_V_PROCESSOR_TYPE7_HOB_DATA)); > + CopyGuid (&L1DataCacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr > + (PcdSiFiveU54CoreGuid)); L1DataCacheDataHob.ProcessorUid =3D > + ProcessorUid; > L1DataCacheDataHob.SmbiosType7Cache.SocketDesignation =3D > + TO_BE_FILLED_BY_VENDOR; > L1DataCacheDataHob.SmbiosType7Cache.CacheConfiguration =3D > RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_1 | \ > + RISC_V_CACHE_CONFIGURATION_LOCATION_INTERNAL | \ > + RISC_V_CACHE_CONFIGURATION_ENABLED | \ > + RISC_V_CACHE_CONFIGURATION_MODE_UNKNOWN; > + L1DataCacheDataHob.SmbiosType7Cache.MaximumCacheSize =3D > + TO_BE_FILLED_BY_VENDOR; > + L1DataCacheDataHob.SmbiosType7Cache.InstalledSize =3D > + TO_BE_FILLED_BY_VENDOR; > + L1DataCacheDataHob.SmbiosType7Cache.SupportedSRAMType.Unknown > =3D 1; > + L1DataCacheDataHob.SmbiosType7Cache.CurrentSRAMType.Unknown =3D 1; > + L1DataCacheDataHob.SmbiosType7Cache.CacheSpeed =3D > + TO_BE_FILLED_BY_VENDOR; > + L1DataCacheDataHob.SmbiosType7Cache.ErrorCorrectionType =3D > + TO_BE_FILLED_BY_VENDOR; > + L1DataCacheDataHob.SmbiosType7Cache.SystemCacheType =3D > CacheTypeData; > + L1DataCacheDataHob.SmbiosType7Cache.Associativity =3D > + TO_BE_FILLED_BY_VENDOR; GuidPtr =3D (EFI_GUID *)PcdGetPtr > + (PcdProcessorSmbiosType7GuidHobGuid); > + L1DataCacheDataHobPtr =3D (RISC_V_PROCESSOR_TYPE7_HOB_DATA > + *)BuildGuidDataHob (GuidPtr, (VOID *)&L1DataCacheDataHob, sizeof > + (RISC_V_PROCESSOR_TYPE7_HOB_DATA)); > + if (L1DataCacheDataHobPtr =3D=3D NULL) { > + DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core L1 > data cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n")); > + ASSERT (FALSE); > + } > + > + // > + // Build up SMBIOS type 7 L2 cache record. > + // > + ZeroMem((VOID *)&L2CacheDataHob, sizeof > + (RISC_V_PROCESSOR_TYPE7_HOB_DATA)); > + CopyGuid (&L2CacheDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr > + (PcdSiFiveU54CoreGuid)); L2CacheDataHob.ProcessorUid =3D ProcessorUid; > + L2CacheDataHob.SmbiosType7Cache.SocketDesignation =3D > + TO_BE_FILLED_BY_VENDOR; > L2CacheDataHob.SmbiosType7Cache.CacheConfiguration =3D > RISC_V_CACHE_CONFIGURATION_CACHE_LEVEL_2 | \ > + RISC_V_CACHE_CONFIGURATION_LOCATION_EXTERNAL | \ > + RISC_V_CACHE_CONFIGURATION_ENABLED | \ > + RISC_V_CACHE_CONFIGURATION_MODE_UNKNOWN; > + L2CacheDataHob.SmbiosType7Cache.MaximumCacheSize =3D > + TO_BE_FILLED_BY_VENDOR; > L2CacheDataHob.SmbiosType7Cache.InstalledSize > + =3D TO_BE_FILLED_BY_VENDOR; > + L2CacheDataHob.SmbiosType7Cache.SupportedSRAMType.Unknown =3D 1; > + L2CacheDataHob.SmbiosType7Cache.CurrentSRAMType.Unknown =3D 1; > + L2CacheDataHob.SmbiosType7Cache.CacheSpeed =3D > TO_BE_FILLED_BY_VENDOR; > + L2CacheDataHob.SmbiosType7Cache.ErrorCorrectionType =3D > + TO_BE_FILLED_BY_VENDOR; > + L2CacheDataHob.SmbiosType7Cache.SystemCacheType =3D > CacheTypeUnified; > + L2CacheDataHob.SmbiosType7Cache.Associativity =3D > + TO_BE_FILLED_BY_VENDOR; GuidPtr =3D (EFI_GUID *)PcdGetPtr > + (PcdProcessorSmbiosType7GuidHobGuid); > + L2CacheDataHobPtr =3D (RISC_V_PROCESSOR_TYPE7_HOB_DATA > + *)BuildGuidDataHob (GuidPtr, (VOID *)&L2CacheDataHob, sizeof > + (RISC_V_PROCESSOR_TYPE7_HOB_DATA)); > + if (L2CacheDataHobPtr =3D=3D NULL) { > + DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core L2 > + cache RISC_V_PROCESSOR_TYPE7_HOB_DATA.\n")); > ASSERT (FALSE); > } >=20 >=20 > [LL] > And the funamental difference ends here. >=20 > @@ -181,7 +233,7 @@ > // Build up SMBIOS type 4 record. > // > ZeroMem((VOID *)&ProcessorDataHob, sizeof > (RISC_V_PROCESSOR_TYPE4_HOB_DATA)); > - CopyGuid (&ProcessorDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr > (PcdSiFiveE51CoreGuid)); > + CopyGuid (&ProcessorDataHob.PrcessorGuid, (EFI_GUID *)PcdGetPtr > + (PcdSiFiveU54CoreGuid)); >=20 > [LL] > Differ in name only. >=20 > ProcessorDataHob.ProcessorUid =3D ProcessorUid; > ProcessorDataHob.SmbiosType4Processor.Socket =3D > TO_BE_FILLED_BY_VENDOR; > ProcessorDataHob.SmbiosType4Processor.ProcessorType =3D > CentralProcessor; @@ -196,7 +248,7 @@ > ProcessorDataHob.SmbiosType4Processor.Status =3D > TO_BE_FILLED_BY_CODE; > ProcessorDataHob.SmbiosType4Processor.ProcessorUpgrade =3D > TO_BE_FILLED_BY_VENDOR; > ProcessorDataHob.SmbiosType4Processor.L1CacheHandle =3D > TO_BE_FILLED_BY_RISC_V_SMBIOS_DXE_DRIVER; > - ProcessorDataHob.SmbiosType4Processor.L2CacheHandle =3D 0xffff; > + ProcessorDataHob.SmbiosType4Processor.L2CacheHandle =3D > + TO_BE_FILLED_BY_RISC_V_SMBIOS_DXE_DRIVER; >=20 > [LL] > Real diff. >=20 > ProcessorDataHob.SmbiosType4Processor.L3CacheHandle =3D 0xffff; > ProcessorDataHob.SmbiosType4Processor.SerialNumber =3D > TO_BE_FILLED_BY_CODE; > ProcessorDataHob.SmbiosType4Processor.AssetTag =3D > TO_BE_FILLED_BY_VENDOR; @@ -212,24 +264,23 @@ > GuidPtr =3D (EFI_GUID *)PcdGetPtr > (PcdProcessorSmbiosType4GuidHobGuid); > ProcessorDataHobPtr =3D (RISC_V_PROCESSOR_TYPE4_HOB_DATA > *)BuildGuidDataHob (GuidPtr, (VOID *)&ProcessorDataHob, sizeof > (RISC_V_PROCESSOR_TYPE4_HOB_DATA)); > if (ProcessorDataHobPtr =3D=3D NULL) { > - DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core > RISC_V_PROCESSOR_TYPE4_HOB_DATA.\n")); > + DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core > + RISC_V_PROCESSOR_TYPE4_HOB_DATA.\n")); >=20 > [LL] > Difference in name only. >=20 > ASSERT (FALSE); > } >=20 > ZeroMem((VOID *)&SmbiosDataHob, sizeof > (RISC_V_PROCESSOR_SMBIOS_HOB_DATA)); > SmbiosDataHob.Processor =3D ProcessorDataHobPtr; > SmbiosDataHob.L1InstCache =3D L1InstCacheDataHobPtr; > - SmbiosDataHob.L1DataCache =3D NULL; > - SmbiosDataHob.L2Cache =3D NULL; > + SmbiosDataHob.L1DataCache =3D L1DataCacheDataHobPtr; > + SmbiosDataHob.L2Cache =3D L2CacheDataHobPtr; >=20 > [LL] > Real diff. >=20 > SmbiosDataHob.L3Cache =3D NULL; > GuidPtr =3D (EFI_GUID *)PcdGetPtr (PcdProcessorSmbiosGuidHobGuid); > SmbiosDataHobPtr =3D (RISC_V_PROCESSOR_SMBIOS_HOB_DATA > *)BuildGuidDataHob (GuidPtr, (VOID *)&SmbiosDataHob, sizeof > (RISC_V_PROCESSOR_SMBIOS_HOB_DATA)); > if (SmbiosDataHobPtr =3D=3D NULL) { > - DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive E51 core > RISC_V_PROCESSOR_SMBIOS_HOB_DATA.\n")); > + DEBUG ((DEBUG_ERROR, "Fail to create GUID HOB of SiFive U54 core > + RISC_V_PROCESSOR_SMBIOS_HOB_DATA.\n")); >=20 > [LL] > Difference in name only. >=20 > ASSERT (FALSE); > } > *SmbiosHobPtr =3D SmbiosDataHobPtr; > return EFI_SUCCESS; > } >=20 > - >=20 > >>> >=20 > The meat of the difference between these two is less than 20% of the lines > of code in each file - and it is mutually exclusive, not some horrific ta= ngle of > interdependencies. >=20 > The story with the difference between U54 and U54MCCoreplex isn't much > better, only works along a different axis. >=20 > "It isn't worthwhile" in an open source project isn't a question of "how > quickly can I create *one* new platform by copying instead of > refactoring/reusing", but a judgement call between: > - How many mistakes do I risk inserting while editing a new file as > opposed to being directly able to see the differences I have caused > while editing an existing file. > - How much do I increase reviewing effort by doing this? > - How much do I increase ongoing maintainership (or affect quality) by > requiring bugs to be fixed in multiple places instead of one. >=20 > Not to mention: > - How many common pattern that could be broken out into common helper > libraries do we miss when we need to compare every SoC/platform > combination ever created, as opposed to being able to look at least > at implementations covering families. >=20 > If it isn't important enough to take that into consideration, it isn't im= portant > enough to upstream the SMBIOS support. Ok Leif, Daniel will evaluate which code could be leveraged and put it in t= he library under either Silicon vendor's folder or under RiscVPlatformPkg = base on the functionality. Many thanks for this. Abner >=20 > / > Leif