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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 71D99EB64DD for ; Wed, 5 Jul 2023 13:13:42 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231969AbjGENNl (ORCPT ); Wed, 5 Jul 2023 09:13:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33292 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229576AbjGENNj (ORCPT ); Wed, 5 Jul 2023 09:13:39 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id CD2D01700 for ; Wed, 5 Jul 2023 06:13:36 -0700 (PDT) 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 95C3F16A3; Wed, 5 Jul 2023 06:14:18 -0700 (PDT) Received: from [10.57.76.116] (unknown [10.57.76.116]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 205843F73F; Wed, 5 Jul 2023 06:13:33 -0700 (PDT) Message-ID: <7aa05d5e-c4ea-cef1-f20e-ead77be1a027@arm.com> Date: Wed, 5 Jul 2023 14:13:30 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH v1 11/14] arm64/mm: Wire up PTE_CONT for user mappings From: Ryan Roberts To: Catalin Marinas Cc: Will Deacon , Ard Biesheuvel , Marc Zyngier , Oliver Upton , James Morse , Suzuki K Poulose , Zenghui Yu , Andrey Ryabinin , Alexander Potapenko , Andrey Konovalov , Dmitry Vyukov , Vincenzo Frascino , Andrew Morton , Anshuman Khandual , Matthew Wilcox , Yu Zhao , Mark Rutland , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org References: <20230622144210.2623299-1-ryan.roberts@arm.com> <20230622144210.2623299-12-ryan.roberts@arm.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/07/2023 12:09, Ryan Roberts wrote: > On 03/07/2023 16:17, Catalin Marinas wrote: >> Hi Ryan, >> ... >>> + >>> +int contpte_ptep_set_access_flags(struct vm_area_struct *vma, >>> + unsigned long addr, pte_t *ptep, >>> + pte_t entry, int dirty) >>> +{ >>> + pte_t orig_pte; >>> + int i; >>> + >>> + /* >>> + * Gather the access/dirty bits for the contiguous range. If nothing has >>> + * changed, its a noop. >>> + */ >>> + orig_pte = ptep_get(ptep); >>> + if (pte_val(orig_pte) == pte_val(entry)) >>> + return 0; >>> + >>> + /* >>> + * We can fix up access/dirty bits without having to unfold/fold the >>> + * contig range. But if the write bit is changing, we need to go through >>> + * the full unfold/fold cycle. >>> + */ >>> + if (pte_write(orig_pte) == pte_write(entry)) { >> >> Depending on the architecture version, pte_write() either checks a >> software only bit or it checks the DBM one. >> >>> + /* >>> + * No need to flush here; This is always "more permissive" so we >>> + * can only be _adding_ the access or dirty bit. And since the >>> + * tlb can't cache an entry without the AF set and the dirty bit >>> + * is a SW bit, there can be no confusion. For HW access >>> + * management, we technically only need to update the flag on a >>> + * single pte in the range. But for SW access management, we >>> + * need to update all the ptes to prevent extra faults. >>> + */ >> >> On pre-DBM hardware, a PTE_RDONLY entry (writable from the kernel >> perspective but clean) may be cached in the TLB and we do need flushing. > > I don't follow; The Arm ARM says: > > IPNQBP When an Access flag fault is generated, the translation table entry > causing the fault is not cached in a TLB. > > So the entry can only be in the TLB if AF is already 1. And given the dirty bit > is SW, it shouldn't affect the TLB state. And this function promises to only > change the bits so they are more permissive (so AF=0 -> AF=1, D=0 -> D=1). > > So I'm not sure what case you are describing here? Ahh sorry, I get your point now - on pre-DBM hardware, the HW sees a read-only PTE when the kernel considers it clean and this can be in the TLB. Then when making it dirty (from kernel's perspective), we are removing the read-only protection from the HW perspective, so we need to flush the TLB entry. > >> >>> + ptep = contpte_align_down(ptep); >>> + addr = ALIGN_DOWN(addr, CONT_PTE_SIZE); >>> + >>> + for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE) >>> + __ptep_set_access_flags(vma, addr, ptep, entry, 0); Fixed by adding this after iterating though the ptes, intent it to avoid the per-page tlb flash and instead flush the whole range at the end: if (dirty) __flush_tlb_range(vma, start_addr, addr, PAGE_SIZE, true, 3); >>> + } else { >>> + /* >>> + * No need to flush in __ptep_set_access_flags() because we just >>> + * flushed the whole range in __contpte_try_unfold(). >>> + */ >>> + __contpte_try_unfold(vma->vm_mm, addr, ptep, orig_pte); >>> + __ptep_set_access_flags(vma, addr, ptep, entry, 0); I also think this is wrong; we must pass `dirty` as the last parameter so that __ptep_set_access_flags will flush if neccessary. My comment about having just done the flush is incorrect - we have just done a flush, but the ptes are still valid with their old value so the HW could pull this into the TLB before we modify the value. >>> + contpte_try_fold(vma->vm_mm, addr, ptep, entry); >>> + } >>> + >>> + return 1; >>> +} >> > > Thanks, > Ryan >