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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id CB3F6C54EBC for ; Tue, 10 Jan 2023 18:08:24 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4A7198E0002; Tue, 10 Jan 2023 13:08:24 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 457028E0001; Tue, 10 Jan 2023 13:08:24 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2F7748E0002; Tue, 10 Jan 2023 13:08:24 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 192EE8E0001 for ; Tue, 10 Jan 2023 13:08:24 -0500 (EST) Received: from smtpin16.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id D2522C032F for ; Tue, 10 Jan 2023 18:08:23 +0000 (UTC) X-FDA: 80339674086.16.0CC2B9A Received: from mail-ej1-f44.google.com (mail-ej1-f44.google.com [209.85.218.44]) by imf22.hostedemail.com (Postfix) with ESMTP id 255DDC0018 for ; Tue, 10 Jan 2023 18:08:21 +0000 (UTC) Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=LHEQpZxg; spf=pass (imf22.hostedemail.com: domain of fvdl@google.com designates 209.85.218.44 as permitted sender) smtp.mailfrom=fvdl@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1673374102; h=from:from:sender: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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=fySU2GMXZP1CrgiuWT68v+2u1BBVFbbU1U8DLH69jCk=; b=YdSCk94yXWM0WG3kYVuhUOLUePi24UG12HkUyXbVHJjwmUinfGZTJr5U3GjSO/6sf2cXAs z5nN7Ja8XcqOhb4yHSlE3jKRaapvwYMLxiQ3xinaCIqvpaGkB5ljQHZizUwm6fziXKbXUD wDsHEa1q003JqdLQjC5NASHQsjfRJlk= ARC-Authentication-Results: i=1; imf22.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=LHEQpZxg; spf=pass (imf22.hostedemail.com: domain of fvdl@google.com designates 209.85.218.44 as permitted sender) smtp.mailfrom=fvdl@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1673374102; a=rsa-sha256; cv=none; b=DjroZGZ39kh/uQBQXUUaIoeEeeQD56Bkqg1XwUUJUf3mlz8ivzV9CxYVoCpY7D0F2JTORj 0OQ9RdHfngcF7CENJU7Wl6OW9aWsAodJzciu6WW30VzC9MAmfrTv2ayB92yTq8E7++mPXC sNBcbDOK+iLE9Lnk2YLi80UhUK/V+yA= Received: by mail-ej1-f44.google.com with SMTP id gh17so30864950ejb.6 for ; Tue, 10 Jan 2023 10:08:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=fySU2GMXZP1CrgiuWT68v+2u1BBVFbbU1U8DLH69jCk=; b=LHEQpZxgMqhdXjl9JlA3oYDX8XwFkBj1sDlSsSTYFDYgwR2PV9YGFswNRiN8bMNPlS oSQsUlMQ+pxpgWTWdAwXAkns3mjbeMqRiDiFNt0MvEQkdZLtUad8ZZraUBpTXpuWksu6 rZEyPM8V+dePKekLm0b5xgieyyDc6rMv41oG+CThRWoffKUapyY5sddH8pk1DLwCGMEP uwGSW3IE9ElGoZcl16JeE/oXw5W8Vh35QAKXiF4HuXhxz20xaV4k+ERp12SKBjDyjaBE AaY8tvC35L2TWBm2wci6IPbm+m+kDOaNFfB34aSaDfp846zo694spo48m5uePpqQrcIy 2IHA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=fySU2GMXZP1CrgiuWT68v+2u1BBVFbbU1U8DLH69jCk=; b=uzcc2EOo8g/NB+DKGeutjdoUquMtkiBAgKx3m6srg+359uwN2CL0efkVVXN16/rBnp FM7XLVia5icLh+Dh+5PdAP3dzIBBlH+rkTqsboso1Uqapj8zI8Sr5Lmm6o4dRhTwEntQ R27Vao0lSVHd7nS0ofz1AzbIF6iSFTjWJ+k5/+m1GQkfvI9Pfd4nw5S/vQvL4r07d3HV p3tdMOJLSkpwXOtAGqzBMiGWbj9hm0FZXlyIHFnvUj3tpPgJCSTEg1ne8xJBZhTllFFP Kaycwq7AjwUQBhgizTfESDSDlJpYR4YGibHAs2eNRSQp8VTBoLYNpT4tk6D6CcinzNSE /aCw== X-Gm-Message-State: AFqh2kqxT4cuXeAUzstOn8zETAyUQHrVmhrJb0B9tT5FAm17bVMucQOI snURvz2pNKoiESxgCcbMHzacCqY3xXrSD4IWQLlCfw== X-Google-Smtp-Source: AMrXdXv/69EUFfvPWoo3i4CpdEGo5C5Mhz/FNsC3amIEyY06dWQaxDz1hzDA1QnpPHruHch95ngzdeTrgf1NYtr/KmA= X-Received: by 2002:a17:906:b217:b0:84d:300a:cbfe with SMTP id p23-20020a170906b21700b0084d300acbfemr1910104ejz.332.1673374100502; Tue, 10 Jan 2023 10:08:20 -0800 (PST) MIME-Version: 1.0 References: <20230109174332.329366-1-fvdl@google.com> <026509bc-aefa-1cf1-5f05-0e258fbc0bc2@arm.com> In-Reply-To: <026509bc-aefa-1cf1-5f05-0e258fbc0bc2@arm.com> From: Frank van der Linden Date: Tue, 10 Jan 2023 10:08:09 -0800 Message-ID: Subject: Re: [PATCH v2] mm/debug: use valid physical memory for pmd/pud tests To: Anshuman Khandual Cc: Andrew Morton , linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 255DDC0018 X-Stat-Signature: wo4a1rocbbmiorc9xd74ipscgk1g1k6q X-Rspam-User: X-HE-Tag: 1673374101-899815 X-HE-Meta: U2FsdGVkX18sbsxG+MIp7phSHZWiz+Itu8Cj9uxwVuxb+gQk98OM38NARiQ+UYFRIMCisgobm2V8Jp+sxZRxLkoGD6byvjdpkVsoWOua7sBw0w6RgipaXcT4NP0FOVzncsZ2oOpHK9/FKY/ZzghDMTij+LTFJoucB2LtN2Fa6WCD00dpmd4FVY4ReDRk279C3Wna81iVRaBxFmRjuZHt1/EFyncB67VsBJyNngIl+MmTYDZip1BQoKLDSwABNv8FGNqP7X5qOrFjh4796lRtBC+DAVZimQNpwA4yUUrjmCIuboWG2bH6An7A8vv8pK9uJHulNm1vPi56Rh7qlU8XUJ/uupO8mGIjbwqJ6xZpejqCxVTWM8NRcKGZIB8fmX8C5RMr1RBCSEmcuoW/VWTzWqb4NIkuwp1mE72AC1igs+tMrH0j0oXxovrsZ3GB+WmHUrjmV7JL3cteszIwf1KzEQdDwurx4F93eMOLKHSFYWwlIaj30R0ZO4AWShL+RlzWDGchwQDnDVlolDxHaMSq8sv6RdA6IBvFl+stm4t5zHB99TiQuSGtT/vb49fhj/gGoaWa1YtasRwezAgDSv7K+Wr3RHjvYKN1ygUq9xqpgSANi60wgKTTXdhpnzmSXCV8Kt7t8hr90y/k4frh4F6gXwHATSV0vYOm+EH4kpDtshao5hIfry4wpKfpdjk0S2QHox06ySR/jECoarz71cerniAmooHEY3E0Gnj7pq4DmovKERDh+A9c1T0YdpZ9ORVBHKD2KYsZ2PUMcOKBcWTxuZiG9jWwVZCf7VNwBoPoa/lWBgpVHckxo1T0XCfeUfhqCzUqcRyME3+xpsAbJWEx2abkSlh6/MibcyIlYOclaOLCE7fEVpogVyHRTFd/IwLCGAcnYVUj6+HiMKUem2bAgQ8O35Ho/8FS5UU1hwlGHT6+XJlnRFqy1ptIs2E3cV4VSxmpR5zn2pQgXRkVlyG NF4p08+G IGk96ObEIb89ooSQ= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hi Anshuman, Sure, I'll send out v3 in a bit addressing your comments: - change ' if (args->fixed_alignment >= PUD_SIZE)' to '== PUD_SIZE' - consistently use align-with-function-open-parenthesis for header and calls - remove redundant if() brackets Thanks, - Frank On Mon, Jan 9, 2023 at 8:40 PM Anshuman Khandual wrote: > > > On 1/9/23 23:13, Frank van der Linden wrote: > > The page table debug tests need a physical address to validate > > low-level page table manipulation with. The memory at this address > > is not actually touched, it just encoded in the page table entries > > at various levels during the tests only. > > > > Since the memory is not used, the code just picks the physical > > address of the start_kernel symbol. This value is then truncated > > to get a properly aligned address that is to be used for various > > tests. Because of the truncation, the address might not actually > > exist, or might not describe a complete huge page. That's not a > > problem for most tests, but the arch-specific code may check > > for attribute validity and consistency. The x86 version of > > {pud,pmd}_set_huge actually validates the MTRRs for the PMD/PUD > > range. This may fail with an address derived from start_kernel, > > depending on where the kernel was loaded and what the physical > > memory layout of the system is. This then leads to false negatives > > for the {pud,pmd}_set_huge tests. > > > > Avoid this by finding a properly aligned memory range that exists > > and is usable. If such a range is not found, skip the tests that > > needed it. > > > > Fixes: 399145f9eb6c ("mm/debug: add tests validating architecture page table helpers") > > Cc: Anshuman Khandual > > Signed-off-by: Frank van der Linden > > --- > > mm/debug_vm_pgtable.c | 103 ++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 84 insertions(+), 19 deletions(-) > > > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > > index c631ade3f1d2..503358332c92 100644 > > --- a/mm/debug_vm_pgtable.c > > +++ b/mm/debug_vm_pgtable.c > > @@ -15,6 +15,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -80,6 +81,7 @@ struct pgtable_debug_args { > > unsigned long pmd_pfn; > > unsigned long pte_pfn; > > > > + unsigned long fixed_alignment; > > unsigned long fixed_pgd_pfn; > > unsigned long fixed_p4d_pfn; > > unsigned long fixed_pud_pfn; > > @@ -430,7 +432,8 @@ static void __init pmd_huge_tests(struct pgtable_debug_args *args) > > { > > pmd_t pmd; > > > > - if (!arch_vmap_pmd_supported(args->page_prot)) > > + if (!arch_vmap_pmd_supported(args->page_prot) || > > + args->fixed_alignment < PMD_SIZE) > > return; > > > > pr_debug("Validating PMD huge\n"); > > @@ -449,7 +452,8 @@ static void __init pud_huge_tests(struct pgtable_debug_args *args) > > { > > pud_t pud; > > > > - if (!arch_vmap_pud_supported(args->page_prot)) > > + if (!arch_vmap_pud_supported(args->page_prot) || > > + args->fixed_alignment < PUD_SIZE) > > return; > > > > pr_debug("Validating PUD huge\n"); > > @@ -1077,10 +1081,86 @@ debug_vm_pgtable_alloc_huge_page(struct pgtable_debug_args *args, int order) > > return page; > > } > > > > +/* > > + * Check if a physical memory range described by contains > > + * an area that is of size psize, and aligned to psize. > > + * > > + * Don't use address 0, an all-zeroes physical address might mask bugs, and > > + * it's not used on x86. > > + */ > > +static void __init phys_align_check(phys_addr_t pstart, > > + phys_addr_t pend, unsigned long psize, phys_addr_t *physp, > > + unsigned long *alignp) > > Small nit. The arguments here need to be aligned properly. > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > index 503358332c92..7bdc6f41a6a6 100644 > --- a/mm/debug_vm_pgtable.c > +++ b/mm/debug_vm_pgtable.c > @@ -1088,9 +1088,9 @@ debug_vm_pgtable_alloc_huge_page(struct pgtable_debug_args *args, int order) > * Don't use address 0, an all-zeroes physical address might mask bugs, and > * it's not used on x86. > */ > -static void __init phys_align_check(phys_addr_t pstart, > - phys_addr_t pend, unsigned long psize, phys_addr_t *physp, > - unsigned long *alignp) > +static void __init phys_align_check(phys_addr_t pstart, phys_addr_t pend, > + unsigned long psize, phys_addr_t *physp, > + unsigned long *alignp) > { > phys_addr_t aligned_start, aligned_end; > > > > +{ > > + phys_addr_t aligned_start, aligned_end; > > + > > + if (pstart == 0) > > + pstart = PAGE_SIZE; > > + > > + aligned_start = ALIGN(pstart, psize); > > + aligned_end = aligned_start + psize; > > + > > + if (aligned_end > aligned_start && aligned_end <= pend) { > > + *alignp = psize; > > + *physp = aligned_start; > > + } > > +} > > + > > +static void __init init_fixed_pfns(struct pgtable_debug_args *args) > > +{ > > + u64 idx; > > + phys_addr_t phys, pstart, pend; > > + > > + /* > > + * Initialize the fixed pfns. To do this, try to find a > > + * valid physical range, preferably aligned to PUD_SIZE, > > + * but settling for aligned to PMD_SIZE as a fallback. If > > + * neither of those is found, use the physical address of > > + * the start_kernel symbol. > > + * > > + * The memory doesn't need to be allocated, it just needs to exist > > + * as usable memory. It won't be touched. > > + * > > + * The alignment is recorded, and can be checked to see if we > > + * can run the tests that require an actual valid physical > > + * address range on some architectures ({pmd,pud}_huge_test > > + * on x86). > > + */ > > + > > + phys = __pa_symbol(&start_kernel); > > + args->fixed_alignment = PAGE_SIZE; > > + > > + for_each_mem_range(idx, &pstart, &pend) { > > + /* First check for a PUD-aligned area */ > > + phys_align_check(pstart, pend, PUD_SIZE, &phys, > > + &args->fixed_alignment); > > Please align the second line properly as above or better just move it in a > single line exceeding 80. > > > + > > + /* If a PUD-aligned area is found, we're done */ > > + if (args->fixed_alignment >= PUD_SIZE) > > + break; > > This check should be "args->fixed_alignment == PUD_SIZE" because the previous > call into phys_align_check() was with PUD_SIZE size and alignment request. So > it could not have achieved args->fixed_alignment larger than PUD_SIZE ? > > > + > > + /* > > + * If no PMD-aligned area found yet, check for one, > > + * but continue the loop to look for a PUD-aligned area. > > + */ > > + if (args->fixed_alignment < PMD_SIZE) { > > + phys_align_check(pstart, pend, PMD_SIZE, &phys, > > + &args->fixed_alignment); > > Moving this into previous line will fix the alignment problem and also will > help drop these additional redundant braces here. In case these lines are > decided to be folded back, please also follow the same for the conditional > statements in pmd_huge_tests/pud_huge_tests as mentioned earlier. > > > + } > > + } > > + > > + args->fixed_pgd_pfn = __phys_to_pfn(phys & PGDIR_MASK); > > + args->fixed_p4d_pfn = __phys_to_pfn(phys & P4D_MASK); > > + args->fixed_pud_pfn = __phys_to_pfn(phys & PUD_MASK); > > + args->fixed_pmd_pfn = __phys_to_pfn(phys & PMD_MASK); > > + args->fixed_pte_pfn = __phys_to_pfn(phys & PAGE_MASK); > > + WARN_ON(!pfn_valid(args->fixed_pte_pfn)); > > +} > > + > > + > > static int __init init_args(struct pgtable_debug_args *args) > > { > > struct page *page = NULL; > > - phys_addr_t phys; > > int ret = 0; > > > > /* > > @@ -1160,22 +1240,7 @@ static int __init init_args(struct pgtable_debug_args *args) > > args->start_ptep = pmd_pgtable(READ_ONCE(*args->pmdp)); > > WARN_ON(!args->start_ptep); > > > > - /* > > - * PFN for mapping at PTE level is determined from a standard kernel > > - * text symbol. But pfns for higher page table levels are derived by > > - * masking lower bits of this real pfn. These derived pfns might not > > - * exist on the platform but that does not really matter as pfn_pxx() > > - * helpers will still create appropriate entries for the test. This > > - * helps avoid large memory block allocations to be used for mapping > > - * at higher page table levels in some of the tests. > > - */ > > - phys = __pa_symbol(&start_kernel); > > - args->fixed_pgd_pfn = __phys_to_pfn(phys & PGDIR_MASK); > > - args->fixed_p4d_pfn = __phys_to_pfn(phys & P4D_MASK); > > - args->fixed_pud_pfn = __phys_to_pfn(phys & PUD_MASK); > > - args->fixed_pmd_pfn = __phys_to_pfn(phys & PMD_MASK); > > - args->fixed_pte_pfn = __phys_to_pfn(phys & PAGE_MASK); > > - WARN_ON(!pfn_valid(args->fixed_pte_pfn)); > > + init_fixed_pfns(args); > > > > /* > > * Allocate (huge) pages because some of the tests need to access > > With the suggested changes accommodated > > Reviewed-by: Anshuman Khandual