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 E3F42C001DC for ; Wed, 26 Jul 2023 06:20:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4925D6B0071; Wed, 26 Jul 2023 02:20:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 442EA6B0074; Wed, 26 Jul 2023 02:20:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 309CB8D0001; Wed, 26 Jul 2023 02:20:39 -0400 (EDT) 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 217396B0071 for ; Wed, 26 Jul 2023 02:20:39 -0400 (EDT) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id DA3F44018C for ; Wed, 26 Jul 2023 06:20:38 +0000 (UTC) X-FDA: 81052764156.05.7429705 Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by imf30.hostedemail.com (Postfix) with ESMTP id 442A980012 for ; Wed, 26 Jul 2023 06:20:34 +0000 (UTC) Authentication-Results: imf30.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf30.hostedemail.com: domain of mawupeng1@huawei.com designates 45.249.212.188 as permitted sender) smtp.mailfrom=mawupeng1@huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1690352436; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=t46h3fL+LqMoqsV92CwwQruDKEGOswF7KlsS9Zkb0OY=; b=ho9GllxH6COCOxiwUwofD1uON5PBoo8ILgpuE3SpcUzQIvFq78Ugpx2EW0qIFe5St7RCqF MnE/sQSEsRfGHXF0Mkq0UvR4OtSg4D6lr6lZ4muN4UDtCNV64aaZ+26F3FRQs/aGr7oUY3 aRDeLpvtr6N9THbNHjpw119AnCQpsVo= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=none; dmarc=pass (policy=quarantine) header.from=huawei.com; spf=pass (imf30.hostedemail.com: domain of mawupeng1@huawei.com designates 45.249.212.188 as permitted sender) smtp.mailfrom=mawupeng1@huawei.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1690352436; a=rsa-sha256; cv=none; b=EQ0LaDsctCV0FUoROcwGXO6kPORlI71BmfTYDSr3p483vMRzBNqFYiCn5K/PxQMePd0AZt lLNeZRQMOfvml8D4Cymnq5W6O836Fsu/BHrdqf1qpQD1vUY9fmKs7hjwiGPKEUPHvv1meT XI1XwUw3FTAaGL0At4z0rRwZ39fcp5c= Received: from dggpemm500014.china.huawei.com (unknown [172.30.72.57]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4R9kFV4SFLzCrVn; Wed, 26 Jul 2023 14:17:02 +0800 (CST) Received: from [10.174.178.120] (10.174.178.120) by dggpemm500014.china.huawei.com (7.185.36.153) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27; Wed, 26 Jul 2023 14:20:24 +0800 Message-ID: <3149f5f8-7878-dfe1-5745-870fddcc1108@huawei.com> Date: Wed, 26 Jul 2023 14:20:24 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird CC: , , , , , , , , Subject: Re: [RFC PATCH] arm64: mm: Fix kernel page tables incorrectly deleted during memory removal Content-Language: en-US To: , , References: <20230717115150.1806954-1-mawupeng1@huawei.com> <20230721103628.GA12601@willie-the-truck> <35a0dad6-4f3b-f2c3-f835-b13c1e899f8d@huawei.com> <732e0db0-eb41-6c58-85b7-46257b4ba0b7@redhat.com> From: mawupeng In-Reply-To: <732e0db0-eb41-6c58-85b7-46257b4ba0b7@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.174.178.120] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To dggpemm500014.china.huawei.com (7.185.36.153) X-CFilter-Loop: Reflected X-Rspamd-Queue-Id: 442A980012 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: knz373pkt7xh7uso6q7amytcpw7zu3q3 X-HE-Tag: 1690352434-508841 X-HE-Meta: U2FsdGVkX1/Q7CMa/ZmkydpTf0ba7shss2os/Q+zYZtHD/jb0V7lkxnKR+pv3xwGo618FZab0sEFzHy0GCdGO+5N65vaILAKiunV2Blb3VgRfh/YgrHdMIWE6izKlXUwZb3QDgLbEc5ufH0Hnaz2c9kjDNr+zLzNyEHJBkxmtGfyLg5WlkD7c/CGCdWmhLl4cQa03b7AN7t+nSWwkeOLhjzr2hznYMYByY/3T2EfmjB3i40lQjsd4YQEE7pfnSL9KerBibhYDz76iZW8ZWhAQpCmkgXCtT2tW0l3m/hpdzKcDtn7encoPzSkeSLWNc+foe0QO91GzOke64HT2zn6awkMvWh9OZ1hh9+6oM48JGcZAmcGPSNfOzqUi60jrdiufem6ho7Du1huuizSCp5m1Dioe9kBWf8awmhmnbfWlETbdnvJCDVlTpU7C4DfO4Fc7eXmZ209Zg3KYR4+Rg3948ZHqxXe9d7KnmGkR94vPJA+10znl3i5it+iWPVidXB3CPHxn5J4ocg9rtToMaQLiVYjX5Uo7heqFK52ZV9QS/7FrljFrgHThyoSOuDxHPEYmnNNuCisHLQ6kdcKN+c1f2UAsQxvLfRxmyIEwTSK7odjHlPJrIRkjS/SaBeFE02yC+okHhots95KxjNJ6z19yTo8myI9TmT9nJ3ZxQpxhO9Vmt9zOA40en2IGggOqCZ8s+gpc4qe6fH8PhVxeOE1K/lmE5fIMMRfPmiKLU1o8HdXmL4Hm1DODowMKdxM8f5jbV0CAQ1IEGUuF5maycpunE2diuxBh/tB7yeJY/QoFtxDc9H0tuaOzrBR43Gz7O8VSYBwpJqAN7mwIJ/lkVR2BR8v6FnUYb1iLp01hq6QZEhZ4CpWiD891QHxWoJKPD+KHcW6J6TIfZ+eWZMGg/SSeibPX3d5lM8ZW0Zg1s9O0rbiwgDA8d5AkmgwHfDW7dBH/hMtuqVKtLsZU2X4CtL qWsUp1DH 7Y4lFlWfTIxjAykwjXvIICkX/aHnOJu4+XKZLZrRofKZfrujRAU+kEo/ooFXnT5IImR1Svhlw+3G2UtL7jXbMhj4ZcJc6pyYXkKbWZRHG+SKK+w+o8VA5oogAMz4JFyhkdJMsBpnOQJp7xf/O8J6PF2QcqV7qCaiwuWPZQBwRzsQxfe4kC5iwpU9sCVwKcamCmZhxxVsZ7LCqLyvyAZxfPIc46oFzjtWnYEUF3Lpx02DsNFxwWRAlbCxxBYS5iKcrP4cHLH+Oz4WTu6H/L6dv1xY7zg== 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: On 2023/7/24 14:11, David Hildenbrand wrote: > On 24.07.23 07:54, Anshuman Khandual wrote: >> >> >> On 7/24/23 06:55, mawupeng wrote: >>> >>> On 2023/7/21 18:36, Will Deacon wrote: >>>> On Mon, Jul 17, 2023 at 07:51:50PM +0800, Wupeng Ma wrote: >>>>> From: Ma Wupeng >>>>> >>>>> During our test, we found that kernel page table may be unexpectedly >>>>> cleared with rodata off. The root cause is that the kernel page is >>>>> initialized with pud size(1G block mapping) while offline is memory >>>>> block size(MIN_MEMORY_BLOCK_SIZE 128M), eg, if 2G memory is hot-added, >>>>> when offline a memory block, the call trace is shown below, > > Is someone adding memory in 2 GiB granularity and then removing parts of it in 128 MiB granularity? That would be against what we support using the add_memory() / offline_and_remove_memory() API and that driver should be fixed instead. Yes, this kind of situation. The problem occurs in the following scenarios: 1. use mem=xxG to reserve memory. 2. add_momory to online memory. 3. offline part of the memroy via offline_and_remove_memory. During my research, ACPI memory removal use memory_subsys_offline to offline memory section and this will not delete page table entry which do not trigger this kind of problem. So I understand what you are talking about. 1. 3rd-party driver shouldn't use add_memory/offline_and_remove_memory to online/offline memory. If it have to use, this can be achieved by driver. 2. memory_subsys_offline is perfered to do such thing. Should we update the doc to describe this kind of limitation? > > Or does this trigger only when a hotplugged memory block falls into the same 2 GiB area as boot memor> >>>>> >>>>>   offline_and_remove_memory >>>>>      try_remove_memory >>>>>        arch_remove_memory >>>>>         __remove_pgd_mapping >>>>>           unmap_hotplug_range >>>>>             unmap_hotplug_p4d_range >>>>>               unmap_hotplug_pud_range >>>>>                 if (pud_sect(pud)) >>>>>                   pud_clear(pudp); > > Which drivers triggers that? In-tree is only virtio-mem and dax/kmem. Both add and remove memory in the same granularity. It is 3rd-party driver. which use try to offline part of(128M) movable memory and this lead to the problem. > > For example, virtio-mem will only call add_memory(memory_block_size()) to then offline_and_remove_memory(memory_block_size()). > > Could that trigger it as well? > >>>> Sorry, but I'm struggling to understand the problem here. If we're adding >>>> and removing a 2G memory region, why _wouldn't_ we want to use large 1GiB >>>> mappings? >>> >>>> Or are you saying that only a subset of the memory is removed, >>>> but we then accidentally unmap the whole thing? >>> Yes, umap a subset but the whole thing page table entry is removed. >>> > > Can we have some more details about the user and how to trigger it? > >>>>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >>>>> index 95d360805f8a..44c724ce4f70 100644 >>>>> --- a/arch/arm64/mm/mmu.c >>>>> +++ b/arch/arm64/mm/mmu.c >>>>> @@ -44,6 +44,7 @@ >>>>>   #define NO_BLOCK_MAPPINGS    BIT(0) >>>>>   #define NO_CONT_MAPPINGS    BIT(1) >>>>>   #define NO_EXEC_MAPPINGS    BIT(2)    /* assumes FEAT_HPDS is not used */ >>>>> +#define NO_PUD_MAPPINGS        BIT(3) >>>>>     int idmap_t0sz __ro_after_init; >>>>>   @@ -344,7 +345,7 @@ static void alloc_init_pud(pgd_t *pgdp, unsigned long addr, unsigned long end, >>>>>            */ >>>>>           if (pud_sect_supported() && >>>>>              ((addr | next | phys) & ~PUD_MASK) == 0 && >>>>> -            (flags & NO_BLOCK_MAPPINGS) == 0) { >>>>> +            (flags & (NO_BLOCK_MAPPINGS | NO_PUD_MAPPINGS)) == 0) { >>>>>               pud_set_huge(pudp, phys, prot); >>>>>                 /* >>>>> @@ -1305,7 +1306,7 @@ struct range arch_get_mappable_range(void) >>>>>   int arch_add_memory(int nid, u64 start, u64 size, >>>>>               struct mhp_params *params) >>>>>   { >>>>> -    int ret, flags = NO_EXEC_MAPPINGS; >>>>> +    int ret, flags = NO_EXEC_MAPPINGS | NO_PUD_MAPPINGS; >>>> I think we should allow large mappings here and instead prevent partial >>>> removal of the block, if that's what is causing the issue. >>> This could solve this problem. >>> Or we can prevent  partial removal? Or rebulid page table entry which is not removed? >> >> + David Hildenbrand >> >> Splitting the block mapping and rebuilding page table entry to reflect non-removed >> areas will require additional information such as flags and pgtable alloc function >> as in __create_pgd_mapping(), which need to be passed along, depending on whether >> it's tearing down vmemmap (would not have PUD block map) or linear mapping. But I >> am just wondering if we have to go in that direction at all or just prevent partial >> memory block removal as suggested by Will. >> >> - arch_remove_memory() does not have return type, core MM hotremove would not fail >>    because arch_remove_memory() failed or warned >> >> - core MM hotremove does check_hotplug_memory_range() which ensures the range and >>    start address are memory_block_size_bytes() aligned >> >> - Default memory_block_size_bytes() is dependent on SECTION_SIZE_BITS which on arm64 >>    now can be less than PUD_SIZE triggering this problem. >> >>     #define MIN_MEMORY_BLOCK_SIZE     (1UL << SECTION_SIZE_BITS) >> >>     unsigned long __weak memory_block_size_bytes(void) >>     { >>              return MIN_MEMORY_BLOCK_SIZE; >>     } >>     EXPORT_SYMBOL_GPL(memory_block_size_bytes); >> >> - We would need to override memory_block_size_bytes() on arm64 to accommodate such >>    scenarios here >> >> Something like this might work (built but not tested) >> >> commit 2eb8dc0d08dfe0b2a3bb71df93b12f7bf74a2ca6 (HEAD) >> Author: Anshuman Khandual >> Date:   Mon Jul 24 06:45:34 2023 +0100 >> >>      arm64/mm: Define memory_block_size_bytes() >>           Define memory_block_size_bytes() on arm64 platforms to set minimum hot plug >>      and remove granularity as PUD_SIZE in case where MIN_MEMORY_BLOCK_SIZE just >>      falls below PUD_SIZE. Otherwise a complete PUD block mapping will be teared >>      down while unmapping MIN_MEMORY_BLOCK_SIZE range. >>           Signed-off-by: Anshuman Khandual >> >> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c >> index 95d360805f8a..1918459b3460 100644 >> --- a/arch/arm64/mm/mmu.c >> +++ b/arch/arm64/mm/mmu.c >> @@ -1157,6 +1157,17 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node, >>   } >>     #ifdef CONFIG_MEMORY_HOTPLUG >> +unsigned long memory_block_size_bytes(void) >> +{ >> +       /* >> +        * Linear mappings might include PUD based block mappings which >> +        * cannot be teared down in part during memory hotremove. Hence >> +        * PUD_SIZE needs to be the minimum granularity, for memory hot >> +        * removal in case MIN_MEMORY_BLOCK_SIZE falls below. >> +        */ >> +       return max_t(unsigned long, MIN_MEMORY_BLOCK_SIZE, PUD_SIZE); >> +} >> + >>   void vmemmap_free(unsigned long start, unsigned long end, >>                  struct vmem_altmap *altmap) >>   { >> > > OH god no. That would seriously degrade memory hotplug capabilities in virtual environments (especially, virtio-mem and DIMMS). > > If someone adds memory in 128 MiB chunks and removes memory in 128 MiB chunks, that has to be working. > > Removing boot memory is blocked via register_memory_notifier(&prevent_bootmem_remove_nb); >