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 70BB1C3DA7A for ; Fri, 6 Jan 2023 04:25:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 027C18E0002; Thu, 5 Jan 2023 23:25:00 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id F19CF8E0001; Thu, 5 Jan 2023 23:24:59 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E08BB8E0002; Thu, 5 Jan 2023 23:24:59 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id D50318E0001 for ; Thu, 5 Jan 2023 23:24:59 -0500 (EST) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 909B212028F for ; Fri, 6 Jan 2023 04:24:59 +0000 (UTC) X-FDA: 80323083918.05.4E34696 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf19.hostedemail.com (Postfix) with ESMTP id 836751A0007 for ; Fri, 6 Jan 2023 04:24:57 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf19.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1672979098; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YHVLZiW6cKEPcDigsnYVuL8vXG/eNTxqvSg4kyR0Hlk=; b=DnTjwws5jYomcmftc3xojyK2/s3L17q3c0MhI/a4xBh+pNYZHFjB5JxHokqXun7YUa7R9e fQg50tFKgVBrq978u8TlOxL0anwOGHjwlz36ocd3bhRKAZ287IiZIyqRgGW7BHHxPPh3xs 7EQg0+M7AyGrwJNtOo6hab1qTgBaE/E= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf19.hostedemail.com: domain of anshuman.khandual@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=anshuman.khandual@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1672979098; a=rsa-sha256; cv=none; b=huxTNFdCmNN2HZIFPfwi3EPe1XXTCv91VJCtKQ75AmEi2GWNHMAxbfySL1VKZWEP2YbiYu E9mtgs/gflf9dNFDexvOJQq16FqGeWH0zQ9IcCtjeJS/o6jv/Y4co4T9MmjvRKmhs3Yj61 6cNRD3W0aYlBfBf4JzGeuGtCaaGs3XM= 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 38CD711FB; Thu, 5 Jan 2023 20:25:38 -0800 (PST) Received: from [10.162.42.6] (unknown [10.162.42.6]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 4C90E3F663; Thu, 5 Jan 2023 20:24:54 -0800 (PST) Message-ID: <083aa5a9-9209-7e06-a00f-dc9657acf1e6@arm.com> Date: Fri, 6 Jan 2023 09:54:52 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.2.2 Subject: Re: [PATCH] mm/debug: use valid physical memory for pmd/pud tests To: Frank van der Linden , Andrew Morton , linux-mm@kvack.org References: <20230105215025.422635-1-fvdl@google.com> Content-Language: en-US From: Anshuman Khandual In-Reply-To: <20230105215025.422635-1-fvdl@google.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: 836751A0007 X-Stat-Signature: nfkegtb4ryearxtu9qq4h37ujda3d1ce X-HE-Tag: 1672979097-82111 X-HE-Meta: U2FsdGVkX184uhcV2lKEZaUicOaSAIGvadLTgpL5edI8vWCg6x6sZMIf6gFphuppoxGaFpujhmOg8NEzkMfMwfjV9ADUkuiC7CIOuJb9uM45aiCFh13/QXEN/8U2tQ7V2GbJ5iTFAiYa3TYi+3muTv7vrbwwDU8tjAxbmTy5miXVjCjz6E8hCqerZtqJiOzZnmMZYv31TmHVWKcmI0iKstKh/mYkz4ev3sqsZohAlbtXHk9Al3KbL/0jwuvt9/jkwF5whra67+Cy90tClX8WUAKdOsWQWsrAxUwsD0FRcKPryBkkTXFoVCd+IEHclhtVIoUy8Fozw7rIMA+hMATubsFNL+ue3FfrxV83mP+8CwhTtpIaCc3KjfkhXUaZVohOMzbQY93v2MRifu6XYrCvlovrey4TPwSjjLqHX8I8KVV6cIuT5vPg9lYo88+uj5Q4P6SMePY3q8hyWeKXGJPeMAE+AgjQJeeUcXpPRRQDWLY/4duaHqL+2A7uyAxLtQfjdQv7kBYn4VX7nLz7fq7uzu9DkzYmCZA2FATDZvH4Z6b0rVPeUZWHwFtGsVBi/RTzH5hx5oxyzy81m/QFL+FRWkAKmDU9ieMG+iI5zdQvKpww8veDxE7t/NOxC0euuLsZERkbDaDTvrAPSbbeBV8M3QUpoxJuE3WqdmXR0s+ZteBKLyySZyGeav7Ums8ABBqJL20OqWMWjVVm2IorRNPvO6T3Q3oDSd08s3ewuuyGvQhukR46Yb1MU+v1RFvgfCTQobEx/ArB4rN3SygEe7jK4BRI3YPjH6fqfC1KlJfCWP/52Blm9NzGPoY/mJvYIZGoYh1sJpIIUTc= 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 Frank, Thanks for the patch, in principle this LGTM. Did a quick run on arm64, did not find anything problematic. Although I have some comments below. On 1/6/23 03:20, 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 | 70 +++++++++++++++++++++++++++++++++++++------ > 1 file changed, 61 insertions(+), 9 deletions(-) > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > index c631ade3f1d2..e9b52600904a 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,8 @@ struct pgtable_debug_args { > unsigned long pmd_pfn; > unsigned long pte_pfn; > > + phys_addr_t fixed_alignment; > + This should not be a 'phys_addr_t', as it does not really contain a physical address. Alignment value can be captured in 'unsigned long' like other elements. > unsigned long fixed_pgd_pfn; > unsigned long fixed_p4d_pfn; > unsigned long fixed_pud_pfn; > @@ -430,7 +433,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; Small nit. Additional line not need for the conditional statement. > > pr_debug("Validating PMD huge\n"); > @@ -449,7 +453,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; Small nit. Additional line not needed for the conditional statement. > > pr_debug("Validating PUD huge\n"); > @@ -1077,11 +1082,41 @@ 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 the same. > + * > + * Don't use address 0, and check for overflow. > + */ > +static int __init phys_align_check(phys_addr_t pstart, > + phys_addr_t pend, phys_addr_t psize, phys_addr_t *physp, > + phys_addr_t *alignp) > +{ > + phys_addr_t aligned_start, aligned_end; > + > + if (pstart == 0) > + pstart = PAGE_SIZE; Why ? > + > + aligned_start = ALIGN(pstart, psize); > + aligned_end = aligned_start + psize; > + > + if (aligned_end > aligned_start && aligned_end <= pend) { > + *alignp = psize; > + *physp = aligned_start; > + return 1; > + } > + > + return 0; > +} To be more clear, this function should return a 'bool' instead > + > + > static int __init init_args(struct pgtable_debug_args *args) > { > struct page *page = NULL; > phys_addr_t phys; > int ret = 0; > + u64 idx; > + phys_addr_t pstart, pend; This declaration can be merged into the previous line containing 'phys'. > > /* > * Initialize the debugging data. > @@ -1161,15 +1196,32 @@ static int __init init_args(struct pgtable_debug_args *args) > 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. > + * Find a valid physical range, preferably aligned to PUD_SIZE. > + * Return the address and the alignment. It doesn't need to be > + * allocated, it just needs to exist as usable memory. The memory > + * won't be touched. > + * > + * The alignment is recorded, and can be checked to see if we > + * can run the tests that require and actual valid physical s/and/an ? > + * address range on some architectures ({pmd,pud}_huge_test > + * on x86). > */ > + > phys = __pa_symbol(&start_kernel); This original 'phys' will still be used as fallback, in case the below attempt does not find a physical address with required alignments i.e [PUD|PMD]_SIZE ? > + args->fixed_alignment = PAGE_SIZE; > + > + for_each_mem_range(idx, &pstart, &pend) { > + if (phys_align_check(pstart, pend, PUD_SIZE, &phys, > + &args->fixed_alignment)) > + break; > + > + if (args->fixed_alignment >= PMD_SIZE) > + continue; > + > + (void)phys_align_check(pstart, pend, PMD_SIZE, &phys, > + &args->fixed_alignment); (void) ? Why not check the return value here ? > + } > + > 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); This loops attempts to find a PUD_SIZE aligned address but breaks out in case it atleast finds a PMD_SIZE aligned address, while looping through available memory ranges. The entire process of finding 'phys' and 'args->fixed_alignment' should be encapsulated inside a helper that also updates 'args->fixed_pxx_pfn' elements. - Anshuman