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 3D954C27C79 for ; Thu, 20 Jun 2024 09:59:18 +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:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=yTsx1prYul/dMDt/NF8GA3uriH6uayCYZdNRZA8BZuI=; b=gPF81ZHuilMByC59g4Vuay0cZN Xc6d7gfWW/nm4rqcd32XSzNATcB9MUuRIgVC1bb1XZzmxJI9rpZ0/8bf1YSzkMIBqGSgk0ofp6WuC geO5OV7GHeFFh7rZnmfTG8ZcjWAF6vSekbQPHtLm//bwpHTtrrpkHUBwfXTrKPpbJ3a2t334s7dUU 1PPGTfg28lPIp2iaCcXeHO1CBtB0j8H4d0oH/92l39WQxZTQXuIThRyrwFld2CvDtBXUDv76Z6GVJ hwQ6NxHRbHcQbuvGo1Rt43XjodM/O4Xq4/AQIZLdp3+aXvPRiLwkLse3H6BjUfMBRhTKpMKS3+UOv yvzgCIIQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sKEZN-00000004Tn3-3Op9; Thu, 20 Jun 2024 09:59:05 +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 1sKEZK-00000004TkP-0YI4 for linux-arm-kernel@lists.infradead.org; Thu, 20 Jun 2024 09:59:03 +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 6D39FDA7; Thu, 20 Jun 2024 02:59:23 -0700 (PDT) Received: from J2N7QTR9R3 (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7E10D3F73B; Thu, 20 Jun 2024 02:58:57 -0700 (PDT) Date: Thu, 20 Jun 2024 10:58:52 +0100 From: Mark Rutland To: Ryan Roberts Cc: Anshuman Khandual , linux-arm-kernel@lists.infradead.org, maz@kernel.org, Catalin Marinas , Will Deacon , linux-kernel@vger.kernel.org Subject: Re: [PATCH V2] arm64/mm: Stop using ESR_ELx_FSC_TYPE during fault Message-ID: References: <20240618034703.3622510-1-anshuman.khandual@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240620_025902_312851_0B480780 X-CRM114-Status: GOOD ( 35.03 ) 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 Thu, Jun 20, 2024 at 09:45:38AM +0100, Ryan Roberts wrote: > On 18/06/2024 04:47, 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, where in > > 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 stop using ESR_ELx_FSC_TYPE, which is > > losing its value as a common mask. > > > > Cc: Catalin Marinas > > Cc: Will Deacon > > Cc: Marc Zyngier > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: Anshuman Khandual > > --- > > This applies on v6.10-rc4 and still leaves behind ESR_ELx_FSC_TYPE for now. > > > > Changes in V2: > > > > - Defined ESR_ELx_FSC_[ACCESS|FAULT_PERM]_L() macros > > - Changed fault helpers using the above macros instead > > - Dropped each page table level fault status discrete values > > - Dropped set_thread_esr() changes in arch/arm64/mm/fault.c > > - Updated the commit message > > > > Changes in V1: > > > > https://lore.kernel.org/linux-arm-kernel/20240613094538.3263536-1-anshuman.khandual@arm.com/ > > > > arch/arm64/include/asm/esr.h | 33 +++++++++++++++++++++++++++------ > > 1 file changed, 27 insertions(+), 6 deletions(-) > > > > diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h > > index 7abf09df7033..3f482500f71f 100644 > > --- a/arch/arm64/include/asm/esr.h > > +++ b/arch/arm64/include/asm/esr.h > > @@ -121,6 +121,14 @@ > > #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_PERM_L(n) (ESR_ELx_FSC_PERM + 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)) > > I think the only real argument for parameterizing it like this is so we can > write "-1" as a parameter rather than "N1" as part of the macro name? Other than > that (marginal) benefit, personally I don't think this approach is very > extensible because we are devining a pattern from the encoding that doesn't > really exist. If we ever needed a level 4 or -3 the encoding would have to be > discontiguous and we would need to rework this again to accomodate. Perhaps the > chances of that ever happening are small enough that the problem can be ignored. FWIW, I agree, I had preferred the spearate definitions because that matched what was in the ARM ARM and didn't infer a pattern that could be broken later. > TBH, I didn't really follow Marc's argument for keeping the "type" macros either > since ESR_ELx_FSC_FAULT does not help to identify the type of a level -1 or -2 > translation fault - the encoding is completely different. But I'll take it on > faith that Marc is correct and I just don't understand ;-) > > Regardless, the implementation looks correct, so: > > Reviewed-by: Ryan Roberts Likewise, either way: Acked-by: Mark Rutland Mark. > > > + > > /* ISS field definitions for Data Aborts */ > > #define ESR_ELx_ISV_SHIFT (24) > > #define ESR_ELx_ISV (UL(1) << ESR_ELx_ISV_SHIFT) > > @@ -388,20 +396,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 */ >