From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 05C49C27C53 for ; Mon, 17 Jun 2024 03:16:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=s7BSJXtNnqC/SZR21T+WyZ3LmLXNxNEsSV/ZuUoLb2Y=; b=dc2wu5o423/XXASJWWseb0aDan YBDjZyAcSS27jud+plMW9YFuKO5z03qCTSYf72iHoYHro6G+rH/dIP2K6+85Ro47BFe6PaeiIkjoP 6i4hFKsqfJU3sYSfRKqiTzI9eTH/xa7zY1g/yarIdXt9/bFdapRMrX07kMWls1s5ECT43Epc+ZcVc M05ELEwb1eMBvIvVT4APrydTKmSKGS2250ZaUxkylBIwZdDJXQ8slVDE9EsEwvL1ozyceSKm6h+A/ 0Qyk+qq5nIGg0Ny2tEDWbVApefCDfJq7zT+Q4Lb30qzYBmCrnTHqjoruL4x92b8pDA3k1XUGyKwon ArCuH+KQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sJ2qW-00000008zEU-0Web; Mon, 17 Jun 2024 03:15:52 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sJ2qS-00000008zDr-3oAK for linux-arm-kernel@lists.infradead.org; Mon, 17 Jun 2024 03:15:50 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 02283DA7; Sun, 16 Jun 2024 20:16:10 -0700 (PDT) Received: from [10.162.16.42] (a077893.blr.arm.com [10.162.16.42]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 47D673F73B; Sun, 16 Jun 2024 20:15:43 -0700 (PDT) Message-ID: <45ace175-9b59-4ba2-91b8-b96151c03c05@arm.com> Date: Mon, 17 Jun 2024 08:45:40 +0530 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] arm64/mm: Drop ESR_ELx_FSC_TYPE Content-Language: en-US To: Marc Zyngier Cc: linux-arm-kernel@lists.infradead.org, mark.rutland@arm.com, ryan.roberts@arm.com, Catalin Marinas , Will Deacon , linux-kernel@vger.kernel.org References: <20240613094538.3263536-1-anshuman.khandual@arm.com> <86y179jdbx.wl-maz@kernel.org> <87cyoj3j44.wl-maz@kernel.org> From: Anshuman Khandual In-Reply-To: <87cyoj3j44.wl-maz@kernel.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240616_201549_077571_C4678713 X-CRM114-Status: GOOD ( 28.59 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 6/14/24 16:07, Marc Zyngier wrote: > On Fri, 14 Jun 2024 03:24:53 +0100, > Anshuman Khandual wrote: >> On 6/13/24 16:53, Marc Zyngier wrote: >>> On Thu, 13 Jun 2024 10:45:38 +0100, >>> Anshuman Khandual wrote: >>>> >>>> Fault status codes at page table level 0, 1, 2 and 3 for access, permission >>>> and translation faults are architecturally organized in a way, that masking >>>> out ESR_ELx_FSC_TYPE, fetches Level 0 status code for the respective fault. >>>> >>>> Helpers like esr_fsc_is_[translation|permission|access_flag]_fault() mask >>>> out ESR_ELx_FSC_TYPE before comparing against corresponding Level 0 status >>>> code as the kernel does not yet care about the page table level, the fault >>>> really occurred previously. >>>> >>>> This scheme is starting to crumble after FEAT_LPA2 when level -1 got added. >>>> Fault status code for translation fault at level -1 is 0x2B which does not >>>> follow ESR_ELx_FSC_TYPE, requiring esr_fsc_is_translation_fault() changes. >>>> >>>> This changes above helpers to compare against individual fault status code >>>> values for each page table level and drop ESR_ELx_FSC_TYPE which is losing >>>> its value as a common mask. >>> >>> I'd rather we do not drop the existing #defines, for a very >>> self-serving reason: >>> >>> NV requires an implementation to synthesise fault syndromes, and these >>> definition are extensively used to compose the syndrome information >>> (see the NV MMU series at [1]). This is also heavily use to emulate >>> the AT instructions (fault reporting in PAR_EL1.FST). >>> >>> Having additional helpers is fine. Dropping the base definitions >>> isn't, and I'd like to avoid reintroducing them. >> >> You would like to just leave behind all the existing level 0 syndrome macro >> definitions in place ? > > They are not level 0. They are values for the type of the fault. They > are *abused* as level 0, but that's not what they are here for. After thinking through the above statement multiple times, just realized that individual page table level fault type value is *derived* from given fault type arithmetically, which is also reflected in the macros you have proposed later. > >> >> #define ESR_ELx_FSC_ACCESS (0x08) >> #define ESR_ELx_FSC_FAULT (0x04) >> #define ESR_ELx_FSC_PERM (0x0C) > > + ESR_ELx_FSC_{TYPE,LEVEL}, because they are convenient macros to > extract the type/level of a fault. NV further adds ESR_ELx_FSC_ADDRSZ > which has been missing. Understood, this extracts both the fault type and its level as well. > >> >> Or which are rather >> >> #define ESR_ELx_FSC_ACCESS ESR_ELx_FSC_ACCESS_L0 >> #define ESR_ELx_FSC_FAULT ESR_ELx_FSC_FAULT_L0 >> #define ESR_ELx_FSC_PERM ESR_ELx_FSC_PERM_L0 > > I definitely prefer the former. Okay. > >> But just wondering why cannot ESR_ELx_FSC_[ACCESS|FAULT|PERM]_L0 definitions >> be used directly in new use cases ? > > Because that is semantically wrong to add/or a level on something that > *already* describes a level. Specially for the level -1 case. Fair enough, as the value already have both the parameters. > > On top of that, what I dislike the most about this patch is that it > defines discrete values for something that could be parametric at zero > cost, just like ESR_ELx_FSC_SEA_TTW(). Yes, there is some additional > complexity, but nothing that the compiler can't elide. Understood. > > For example, something like this: > > diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h > index 7abf09df7033..c320aeb1bb9a 100644 > --- a/arch/arm64/include/asm/esr.h > +++ b/arch/arm64/include/asm/esr.h > @@ -121,6 +121,10 @@ > #define ESR_ELx_FSC_SECC (0x18) > #define ESR_ELx_FSC_SECC_TTW(n) (0x1c + (n)) > > +#define ESR_ELx_FSC_FAULT_nL (0x2C) > +#define ESR_ELx_FSC_FAULT_L(n) (((n) < 0 ? ESR_ELx_FSC_FAULT_nL : \ > + ESR_ELx_FSC_FAULT) + (n)) > + > /* ISS field definitions for Data Aborts */ > #define ESR_ELx_ISV_SHIFT (24) > #define ESR_ELx_ISV (UL(1) << ESR_ELx_ISV_SHIFT) > > Importantly, it avoids the ESR_ELx_FSC_FAULT_LN1 horror, and allows > ESR_ELx_FSC_FAULT_L(-1) to be written. > > M. > Does the following re-worked patch looks okay ? Earlier set_thread_esr() changes can be dropped from arch/arm64/mm/fault.c and also the original commit message still makes sense. diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h index 7abf09df7033..6cd13ac61005 100644 --- a/arch/arm64/include/asm/esr.h +++ b/arch/arm64/include/asm/esr.h @@ -121,6 +121,13 @@ #define ESR_ELx_FSC_SECC (0x18) #define ESR_ELx_FSC_SECC_TTW(n) (0x1c + (n)) +/* Status codes for individual page table levels */ +#define ESR_ELx_FSC_ACCESS_L(n) (ESR_ELx_FSC_ACCESS + n) +#define ESR_ELx_FSC_FAULT_nL (0x2C) +#define ESR_ELx_FSC_FAULT_L(n) (((n) < 0 ? ESR_ELx_FSC_FAULT_nL : \ + ESR_ELx_FSC_FAULT) + (n)) +#define ESR_ELx_FSC_PERM_L(n) (ESR_ELx_FSC_PERM + n) + /* ISS field definitions for Data Aborts */ #define ESR_ELx_ISV_SHIFT (24) #define ESR_ELx_ISV (UL(1) << ESR_ELx_ISV_SHIFT) @@ -388,20 +395,33 @@ static inline bool esr_is_data_abort(unsigned long esr) static inline bool esr_fsc_is_translation_fault(unsigned long esr) { - /* Translation fault, level -1 */ - if ((esr & ESR_ELx_FSC) == 0b101011) - return true; - return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_FAULT; + esr = esr & ESR_ELx_FSC; + + return (esr == ESR_ELx_FSC_FAULT_L(3)) || + (esr == ESR_ELx_FSC_FAULT_L(2)) || + (esr == ESR_ELx_FSC_FAULT_L(1)) || + (esr == ESR_ELx_FSC_FAULT_L(0)) || + (esr == ESR_ELx_FSC_FAULT_L(-1)); } static inline bool esr_fsc_is_permission_fault(unsigned long esr) { - return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_PERM; + esr = esr & ESR_ELx_FSC; + + return (esr == ESR_ELx_FSC_PERM_L(3)) || + (esr == ESR_ELx_FSC_PERM_L(2)) || + (esr == ESR_ELx_FSC_PERM_L(1)) || + (esr == ESR_ELx_FSC_PERM_L(0)); } static inline bool esr_fsc_is_access_flag_fault(unsigned long esr) { - return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_ACCESS; + esr = esr & ESR_ELx_FSC; + + return (esr == ESR_ELx_FSC_ACCESS_L(3)) || + (esr == ESR_ELx_FSC_ACCESS_L(2)) || + (esr == ESR_ELx_FSC_ACCESS_L(1)) || + (esr == ESR_ELx_FSC_ACCESS_L(0)); } /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */