From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f65.google.com (mail-wm1-f65.google.com [209.85.128.65]) by mx.groups.io with SMTP id smtpd.web11.748.1608059478827405845 for ; Tue, 15 Dec 2020 11:11:19 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=Pf5TKGJD; spf=pass (domain: nuviainc.com, ip: 209.85.128.65, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f65.google.com with SMTP id v14so268832wml.1 for ; Tue, 15 Dec 2020 11:11:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=R595qfAWGDYaWaJuyV16mmvjevyWjFBSxR6fELc/Qhs=; b=Pf5TKGJDIAvySN9lRFHO0bGr8K9YrudkRstAerCLbvczCSxRdpzRD/qb/jisFhhOhj aSPwc92PhLa0A4wErLMp8xnDqSyeNh9CNvxMoahi/0CSUas1yig1kmnwRdCTnUmwCFNQ hnwqBYxaYmYB6uEgyU9wBPhK20pvtUUM7aFBfW5g1qeAfBOM48BDcfI7vTTPKuM4z03m kelH0HtxZ+KDibTkaXXMfNF7jfjGnuADE93EfaJ97t27JapqRz3KW7fb++6HCFoL9q0o 3+dfLsrRuteaVoBBmDpD9xUl+psnFpQ0DZM5Y8/fQDMWgkgaI8ra0qJwYNCcjTIIXDI0 6Arg== 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:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=R595qfAWGDYaWaJuyV16mmvjevyWjFBSxR6fELc/Qhs=; b=oZHpRJowN1Uu/G90qOLYXq6kIUrFeuHSzCvbfEJvn54Hg9wX02ok/a7xditz4AnwGc UxKC4IxrQnA5K8lB/XJ4b13zwjcTkzvQv91sMz04nLcq/3SwdUJ5N8YnTAjW7r2Q1Cuh Ck+EB1pzH4x1cCCayEaj74OnaF8cBCDYIBBZ1V/lix3bpYMXnFh71a/uyi1eN52zFwZc 9GYoR4C8CH+yrWyMedLtq+9WNp+Rg4TBhKW5GHrVDA1qLYDDmQ/fEmJeqr2jMYoeOts3 3dm2oS3PuIzEklInXm4PIz0aJA/NYW2u8neY21VZzP8GHh3lhWKnuokx7vCYINN7gJqB QNdA== X-Gm-Message-State: AOAM530ZqJyGPGggyHpV92ObM9CmwhbBueF9IdiCpGkPZmbuZQwL1zOQ ZbE0OGFCcuecSxWYV2bqVb3GEw== X-Google-Smtp-Source: ABdhPJwqhYEjjmAtsMe76D3s+MD1/SKDAAm3u/fxiC5HOBo7cnVY1HIAOYnNhXDnK0Q2iWYvUtuptA== X-Received: by 2002:a05:600c:2188:: with SMTP id e8mr322647wme.99.1608059477244; Tue, 15 Dec 2020 11:11:17 -0800 (PST) Return-Path: Received: from vanye (cpc1-cmbg19-2-0-cust915.5-4.cable.virginm.net. [82.27.183.148]) by smtp.gmail.com with ESMTPSA id c190sm38369175wme.19.2020.12.15.11.11.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 15 Dec 2020 11:11:16 -0800 (PST) Date: Tue, 15 Dec 2020 19:11:14 +0000 From: "Leif Lindholm" To: Rebecca Cran Cc: devel@edk2.groups.io, Ard Biesheuvel , Laszlo Ersek Subject: Re: [PATCH v4 04/10] ArmPkg: Add helper to read the Memory Model Features Register 2 Message-ID: <20201215191114.GD1664@vanye> References: <20201207175427.28712-1-rebecca@nuviainc.com> <20201207175427.28712-5-rebecca@nuviainc.com> MIME-Version: 1.0 In-Reply-To: <20201207175427.28712-5-rebecca@nuviainc.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline +Laszlo Ard, I could use your input on the below, and Laszlo might also have an opinion: On Mon, Dec 07, 2020 at 10:54:21 -0700, Rebecca Cran wrote: > Add helper function to read the MMFR2 register. We will need this to > determine CCIDX support. > > Signed-off-by: Rebecca Cran > --- > ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h | 6 ++++++ > ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 3 +++ > 2 files changed, 9 insertions(+) > > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h > index b2c8a8ea0b84..d6bcfc3b82ae 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Lib.h > @@ -35,5 +35,11 @@ ArmCleanInvalidateDataCacheEntryBySetWay ( > IN UINTN SetWayFormat > ); > > +UINTN > +EFIAPI > +ArmReadIdMmfr2 ( > + VOID > + ); > + First of all, I think this prototype belongs in Include/Library/ArmLib.h ... but! So, there are a lot of system registers, many of which share at least the view of the bottom 32 bits between aarch64/aarch32 versions. This isn't true for the ID registers - which are always 64-bit for aarch64 state, and always 32-bit for aarch32, where aarch64 have access to both. So this helper function isn't generic - in this particular case, we're adding this accessor because we want to determine CCIDX support. For aarch64 this means ID_AA64MMFR2_EL1, but for aarch32 this means ID_MMFR4 (also accessible from aarch64 as ID_MMFR4_EL1). We already have ArmReadIdPfr0 and ArmReadIdPfr1 in ArmLib.h, already being made use of, helping to demonstrate the problem: ArmPkg/Library/ArmGicArchLib/AArch64/ArmGicArchLib.c: if (ArmReadIdPfr0 () & AARCH64_PFR0_GIC) { ArmPkg/Library/ArmGicArchLib/Arm/ArmGicArchLib.c: if (ArmReadIdPfr1 () & ARM_PFR1_GIC) { I would propose that since the high-level abstraction serve only to confuse things, we change existing (and new) accessors to ID registers to be explicit: - ArmReadIdAArch64Mmfr0 - ArmReadIdAArch64Pfr0 - ArmReadIdAArch64Pfr1 The question is whether we should make the AArch32 aspect explicit or implicit? My instinctive reaction is the latter. This matches the native naming scheme used in the ARM ARM, and we don't support mixing instruction set widths in UEFI. The AArch64 prototypes should then only be made available to AARCH64 code, and the AArch32 ones only to ARM. Does the above makes sense to everyone? Best Regards, Leif > #endif // __AARCH64_LIB_H__ > > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > index 199374ff59e3..874bc2866ac3 100644 > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S > @@ -424,6 +424,9 @@ ASM_FUNC(ArmCallWFI) > wfi > ret > > +ASM_FUNC(ArmReadIdMmfr2) > + mrs x0, ID_AA64MMFR2_EL1 // read EL1 MMFR2 > + ret > > ASM_FUNC(ArmReadMpidr) > mrs x0, mpidr_el1 // read EL1 MPIDR > -- > 2.26.2 >