From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) by mx.groups.io with SMTP id smtpd.web08.271.1608227881947836291 for ; Thu, 17 Dec 2020 09:58:02 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=fSJp5c+i; spf=pass (domain: nuviainc.com, ip: 209.85.128.45, mailfrom: leif@nuviainc.com) Received: by mail-wm1-f45.google.com with SMTP id a6so6312291wmc.2 for ; Thu, 17 Dec 2020 09:58:01 -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=Woew5r+6MjC2krcTAXeat5euJM4QhCG9uBf+/2IidCE=; b=fSJp5c+iUPa7QsFLT/vQ4D/wIEj83/tw87juVcYdNVkS7jfXNNOyJ133AnQfVEfQSf i+3EddEaMEGJ+qkzBt/k52qgWuGwzcnZkBopbv5eS95BkVoKuQBPeoIU66u6wpQezDBI ixd5mc8gclkDtXM+kGpK0hcJOw9iqf5FuAuKUSQI5UkYNug+Pcp9L7AF8FCiNs7ed+N/ rsMEyctEIHATUvl+WCfMMib4mbkMZqCLeLuKvRwL3x+lKTXNlwdW2GPUo+4bcxrdTs14 gZtW93DH03sx41NXjmW4q1zIoABpn7c8j9avpuq11T0XFJBoaROwQetAOx2PwHg8n1bC 5rwA== 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=Woew5r+6MjC2krcTAXeat5euJM4QhCG9uBf+/2IidCE=; b=kQ0n/tI2YeJF4zEevWip43zE5eEVd750q4ch4ZmeaLn2TDd2t1NyIFQdBeRAXT/urR FCmg8oVpIj4NbM00xZxiYoCZbiiyVJ5ZuxWC6eT7ghOi4FmV5G9NMR/2i4/CvrscTBOT LuQ71AmF1dRNv78Ytw2hL2TQAzwvyTEAocN8YTpVcZl3W3ghxsrhwFP+xC9Y3AHEMr7c v/FE+2BDH2ZggExx3uxDPbOHofosw3zOHVoO5rxoJ/cT/YwrVPpT8eu+KUFpWDqWG3zC +6yAM9DoFJM42dZ7uXkFuBLngLVleszeYdR40JYZw54Wqe5t/c8fKBvhsBAT+oJVg2Ro efkg== X-Gm-Message-State: AOAM533b2VaOPL9mIoyjGnj0zMgQC3898aevFL2WtrcKaa8NEZmkXogg eE9wx1Rgje8ofLVyIxfCSp3eqA== X-Google-Smtp-Source: ABdhPJz/W03I2xWq9xlhEJearxt4960dzIPLO4KXPNZJSY71Tge5iYBjrm5NfSiZyP+qoHcZnKDyFQ== X-Received: by 2002:a1c:2b46:: with SMTP id r67mr504643wmr.162.1608227880551; Thu, 17 Dec 2020 09:58:00 -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 h14sm10174995wrx.37.2020.12.17.09.57.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 17 Dec 2020 09:58:00 -0800 (PST) Date: Thu, 17 Dec 2020 17:57:58 +0000 From: "Leif Lindholm" To: Laszlo Ersek Cc: Rebecca Cran , devel@edk2.groups.io, Ard Biesheuvel Subject: Re: [PATCH v4 04/10] ArmPkg: Add helper to read the Memory Model Features Register 2 Message-ID: <20201217175758.GR1664@vanye> References: <20201207175427.28712-1-rebecca@nuviainc.com> <20201207175427.28712-5-rebecca@nuviainc.com> <20201215191114.GD1664@vanye> <76d90616-dc08-0c65-0ccf-d9c99259e99c@redhat.com> MIME-Version: 1.0 In-Reply-To: <76d90616-dc08-0c65-0ccf-d9c99259e99c@redhat.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Dec 17, 2020 at 14:38:55 +0100, Laszlo Ersek wrote: > 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. I simply meant we probably don't want to call the AArch32 accesssors ArmReadIdAArch32Pfr0, since ID_PFR0 is the architectural name for the AArch32 register when executing in AArch32 state. As Sami pointed out, we could similarily just stick with the CamelCased architectural names for AArch64 accessors rather than spelling out AArch64. / Leif > > 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 >