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 A96AACDB465 for ; Thu, 19 Oct 2023 05:59:26 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id: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=aK9v6RNJn6q6x3kToC42f+jwEQieKFGDLKAsT6ES1xs=; b=R7I8/P2gD71+CX LXYcAEOIiF+ojzPHdxQUdY+/y9SzrRSxU10Pn3gd65r3wdP/6VtZ3gQeFA8/r14M0uyjYhLCa7YN8 B0IVLKSBnDxe4kvbUzGgpbFFyL4dvf/Dz2P3J86EAkWEM3U0amNiKQoAeb+cGVJDFzLUMSfaHVqlA A4rF2PASIr77Rtgvcpb2eIoWd+0c6qF5nXNXlLTiD62JhtBwVjkG5JwG44zt9v8L9rjwOTF3p7AtB RCdNabFiLMrTjXWudoLFXqdcczHkSME6t7dLij+OoOIWv2x51UGv1iz79VGoCaUNYJaxAzFwH09v2 G/IGsa9ACDflCguvihrw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qtM3g-00GRSz-1s; Thu, 19 Oct 2023 05:59:00 +0000 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qtM3c-00GRPj-1p for linux-arm-kernel@lists.infradead.org; Thu, 19 Oct 2023 05:58:58 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1697695135; 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=OYiNcQwLkcJ0JWlC42a8NTarycDAR71hXESfUG+RdkM=; b=Uj1hyyJ9F8Mj7Xd63x8CDTaxPCh1qOo6iJx37QzQ87FQCQqiWXnux4e14pm42835Fwp3Y4 4LcU7uUZT6P2mfYJbI27zLLft6E3mikG+TR9eX2H39Mq1LrmJqlsaTfMVgWLqvzr0iroJi df494pnE4gb/EX0SCJ042wjSF9w5vZE= Received: from mail-ua1-f72.google.com (mail-ua1-f72.google.com [209.85.222.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-99-tEYRkxmnMAeksZCeovIFjQ-1; Thu, 19 Oct 2023 01:58:54 -0400 X-MC-Unique: tEYRkxmnMAeksZCeovIFjQ-1 Received: by mail-ua1-f72.google.com with SMTP id a1e0cc1a2514c-7b0de3829c0so2479401241.3 for ; Wed, 18 Oct 2023 22:58:54 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697695133; x=1698299933; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=OYiNcQwLkcJ0JWlC42a8NTarycDAR71hXESfUG+RdkM=; b=HDnGM6Qdp8m9jGc3OHQqNFXx1D3rbEZH7Dv3Alw2E+L8fNNoogZJGR35UOr4Nek/HO BRcT0veXs2BHwJhOlW90/ZB+8BnQxz+kaxtotjU6wkuyxYOvyG5a5sXzD7BSe/G5mc82 fNST8eGZ4SySJACR6Zc9bdGG+gGtCXXxPDQVB56WkijTYIBvL6LfbUFmaSghdKTQEiB6 5dgensbSH2H3YyfQlwhBBYhpM5Pp9MvzmGBXmQpA9BMfD66Ols4DDOyYSgOnrQNgdYft OyjSf2hGERDElPn6uZoXP9d4RkENpyrr/Ts+qFhCj1Qpdd1iKau/Sq6oWXMLD5vUTVhL PPgg== X-Gm-Message-State: AOJu0YyCdFECNzvluDeVxLsUqwQXa6opp6/G/TBH81VciW9eBp7C/q67 xCz+4XuL6N37Wl2qSWO2xbTwOOgVB8cv62K+s3Q8jeV9qnsW65KI/Eyt99BGeDkAAKRGYqgNyt7 tvyAkbVtwaQqkujQW3lD4FDEhNt5/7+yOs7o= X-Received: by 2002:a67:ab4f:0:b0:457:adc8:b163 with SMTP id k15-20020a67ab4f000000b00457adc8b163mr1118434vsh.27.1697695133548; Wed, 18 Oct 2023 22:58:53 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHiyLmygww0ENS5AhUS3kBqt+gwGtLmYIbLmwqlO+j4nePp4S7H35hIlNeSEhfWNn8yMh+KNg== X-Received: by 2002:a67:ab4f:0:b0:457:adc8:b163 with SMTP id k15-20020a67ab4f000000b00457adc8b163mr1118421vsh.27.1697695133222; Wed, 18 Oct 2023 22:58:53 -0700 (PDT) Received: from ?IPV6:2001:8003:e5b0:9f00:b890:3e54:96bb:2a15? ([2001:8003:e5b0:9f00:b890:3e54:96bb:2a15]) by smtp.gmail.com with ESMTPSA id o9-20020a655209000000b005b46e691108sm2226270pgp.68.2023.10.18.22.58.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 18 Oct 2023 22:58:52 -0700 (PDT) Message-ID: <2ce4b984-b3d6-4c35-96f3-d71d0a7c8ef2@redhat.com> Date: Thu, 19 Oct 2023 15:58:47 +1000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] arm64: mm: Validate CONFIG_PGTABLE_LEVELS conditionally To: Mark Rutland Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, will@kernel.org, ryan.roberts@arm.com, anshuman.khandual@arm.com, shan.gavin@gmail.com References: <20231017005300.334140-1-gshan@redhat.com> From: Gavin Shan In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231018_225856_698993_AC37B271 X-CRM114-Status: GOOD ( 35.54 ) 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: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 10/18/23 19:42, Mark Rutland wrote: > On Wed, Oct 18, 2023 at 04:33:09PM +1000, Gavin Shan wrote: >> On 10/17/23 21:05, Mark Rutland wrote: >>> On Tue, Oct 17, 2023 at 10:53:00AM +1000, Gavin Shan wrote: >>>> It's allowed for the fixmap virtual address space to span multiple >>>> PMD entries. Instead, the address space isn't allowed to span multiple >>>> PUD entries. However, PMD entries are folded to PUD and PGD entries >>>> in the following combination. In this particular case, the validation >>>> on NR_BM_PMD_TABLES should be avoided. >>>> >>>> CONFIG_ARM64_PAGE_SHIFT = 14 >>>> CONFIG_ARM64_VA_BITS_36 = y >>>> CONFIG_PGTABLE_LEVELS = 2 >>> >>> Is this something you found by inspection, or are you hitting a real issue on a >>> particular config? >>> >>> I built a kernel with: >>> >>> defconfig + CONFIG_ARM64_16K_PAGES=y + CONFIG_ARM64_VA_BITS_36=y >>> >>> ... which gives the CONFIG_* configuration you list above, and that works just >>> fine. >>> >>> For 2-level 16K pages we'd need to reserve more than 32M of fixmap slots for >>> the assertion to fire, and we only reserve ~6M of slots in total today, so I >>> can't see how this would be a problem unless you have 26M+ of local additions >>> to the fixmap? >>> >>> Regardless of that, I don't think it's right to elide the check entirely. >>> >> >> It's all about code inspection. When CONFIG_PGTABLE_LEVELS == 2, PGD/PUD/PMD >> are equivalent. The following two macros are interchangeable. The forthcoming >> static_assert() enforces that the fixmap virtual space can't span multiple >> PMD entries, meaning the space is limited to 32MB with above configuration. >> >> #define NR_BM_PMD_TABLES \ >> SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PUD_SHIFT) >> #define NR_BM_PMD_TABLES \ >> SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PMD_SHIFT) >> >> static_assert(NR_BM_PMD_TABLES == 1); >> >> However, multiple PTE tables are allowed. It means the fixmap virtual space >> can span multiple PMD entries, which is controversial to the above enforcement >> from the code level. Hopefully, I understood everything correctly. >> >> #define NR_BM_PTE_TABLES \ >> SPAN_NR_ENTRIES(FIXADDR_TOT_START, FIXADDR_TOP, PMD_SHIFT) >> static pte_t bm_pte[NR_BM_PTE_TABLES][PTRS_PER_PTE] __page_aligned_bss; >> > > The intent is that the fixmap can span multiple PTE tables, but has to fall > within a single PMD table (and within a single PGD entry). See the next couple > of lines where we only allocate one PMD table and one PUD table: > > static pte_t bm_pte[NR_BM_PTE_TABLES][PTRS_PER_PTE] __page_aligned_bss; > static pmd_t bm_pmd[PTRS_PER_PMD] __page_aligned_bss __maybe_unused; > static pud_t bm_pud[PTRS_PER_PUD] __page_aligned_bss __maybe_unused; > > The NR_BM_PMD_TABLES definition is only there for the static_assert(). > Ok, thanks for the hints. >> You're correct that the following edition is needed to trigger the assert. >> The point is to have the fixmap virtual space larger than 32MB. > > It's not intended to be more than 32M. > > If we want to support 32M and larger, we'd need to rework the rest of the code, > allocating more intermediate tables and manipulating multiple PGD entries. As > we have no need for that, it's simpler to leave it as-is, with the > static_assert() ther to catch if/when someone tries to expand it beyond what is supported. > Yeah, it's a small space anyway. >> >> enum fixed_addresses { >> FIX_HOLE, >> : >> FIX_PTE, >> FIX_PMD, >> FIX_PUD, >> FIX_PGD, >> FIX_DUMMY = FIX_PGD + 2048, >> >> __end_of_fixed_addresses >> }; >> >> >>> The point of the check is to make sure that the fixmap VA range doesn't span >>> across multiple PMD/PUD/P4D/PGD entries, as the early_fixmap_init() and >>> fixmap_copy() code don't handle that in general. When using 2-level 16K pages, >>> we still want to ensure the fixmap is contained within a single PGD, and >>> checking that it falls within a single folded PMD will check that. >>> >>> See the message for commit: >>> >>> 414c109bdf496195 ("arm64: mm: always map fixmap at page granularity") >>> >>> ... and the bits that deleted from early_fixmap_init(). >>> >>> AFAICT this is fine as-is. >>> >> >> As I can see, multiple PMD entries can be handled well in early_fixmap_init(). >> However, multiple PMD entries aren't handled in fixmap_copy(), as you said. >> >> early_fixmap_init >> early_fixmap_init_pud >> early_fixmap_init_pmd // multiple PMD entries handled in the loop > > If you remove the restriction of a single PMD entry, you also permit multiple > PUD/P4D/PGD entries, and the early_fixmap_init() code cannot handle that. > Consider how early_fixmap_init_pud() and early_fixmap_init_pmd() use bm_pud and > bm_pmd respectively. > > As above, this code doesn't need to change: > > * It works today, there is no configuration where the statis_assert() fires > spuriously. > > * If the static_assert() were to fire, we'd need to alter some portion of the > code to handle that case (e.g. expanding bm_pmd and/or bm_pud, altering > fixmap_copy()). > > * It's simpler and better to have the assertion today rather than making the > code handle the currently-impossible cases. That avoids wasting memory on > unusable tables, and avoids having code which is never tested. > Agree. Please ignore my patch and lets keep it as-is. Again, it's a small space and I don't see it needs to be enlarged to hit the limit. Thanks for explaining everything in a clear way. Thanks, Gavin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel