From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web09.8100.1608212341569908436 for ; Thu, 17 Dec 2020 05:39:01 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=AFwt9Vi4; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1608212340; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eaEJAfQjWhWnvqfdyRFFjDr7Q/tPEnUG3BPz2idjjkI=; b=AFwt9Vi4P8PxDI73h1Zj0UUMFf7g6zde/airPvn1lYG4BWi8jlvn/533tMtnfkmR+vEZfY XggM4hM6svP3i5sT/TRmBshgfnqp/H4w9+RZC1k+WCrB6RIZd5mTRQVrxdOslkfcTLXUto Z96BJa5w1NXmAAibBENDQAVg3g/+n8A= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-242-IN93rWuDMPG1VrYyByey7g-1; Thu, 17 Dec 2020 08:38:59 -0500 X-MC-Unique: IN93rWuDMPG1VrYyByey7g-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 06A57107ACE4; Thu, 17 Dec 2020 13:38:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-40.ams2.redhat.com [10.36.115.40]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0000D100164C; Thu, 17 Dec 2020 13:38:56 +0000 (UTC) Subject: Re: [PATCH v4 04/10] ArmPkg: Add helper to read the Memory Model Features Register 2 To: Leif Lindholm , Rebecca Cran Cc: devel@edk2.groups.io, Ard Biesheuvel References: <20201207175427.28712-1-rebecca@nuviainc.com> <20201207175427.28712-5-rebecca@nuviainc.com> <20201215191114.GD1664@vanye> From: "Laszlo Ersek" Message-ID: <76d90616-dc08-0c65-0ccf-d9c99259e99c@redhat.com> Date: Thu, 17 Dec 2020 14:38:55 +0100 MIME-Version: 1.0 In-Reply-To: <20201215191114.GD1664@vanye> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 12/15/20 20:11, Leif Lindholm wrote: > +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 I can follow until here... (and yes, using the concrete register names in the function names makes sense) > > 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. I lost you here, sorry. > > The AArch64 prototypes should then only be made available to AARCH64 > code, and the AArch32 ones only to ARM. But this again makes sense to me. I guess what confuses me is your interpretation of "implicit" vs. "explicit". I'm missing what the "AArch32 aspect" means, probably. Thanks laszlo