On 07.01.20 21:59, Logan Gunthorpe wrote:
> devm_memremap_pages() is currently used by the PCI P2PDMA code to create
> struct page mappings for IO memory. At present, these mappings are created
> with PAGE_KERNEL which implies setting the PAT bits to be WB. However, on
> x86, an mtrr register will typically override this and force the cache
> type to be UC-. In the case firmware doesn't set this register it is
> effectively WB and will typically result in a machine check exception
> when it's accessed.
>
> Other arches are not currently likely to function correctly seeing they
> don't have any MTRR registers to fall back on.
>
> To solve this, add an argument to arch_add_memory() to explicitly
> set the pgprot value to a specific value.
You're adding a parameter indirectly by adding it to the structure.
Maybe "provide a way to specify the pgprot value explicitly to
arch_add_memory()"
>
> Of the arches that support MEMORY_HOTPLUG: x86_64, s390 and arm64 is a
s/is/need/
> simple change to pass the pgprot_t down to their respective functions
> which set up the page tables. For x86_32, set the page tables explicitly
"page table protection" ?
> using _set_memory_prot() (seeing they are already mapped). For sh, reject
> anything but PAGE_KERNEL settings -- this should be fine, for now, seeing
> sh doesn't support ZONE_DEVICE anyway.
>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Signed-off-by: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> ---
> arch/arm64/mm/mmu.c | 3 ++-
> arch/ia64/mm/init.c | 4 ++++
> arch/powerpc/mm/mem.c | 3 ++-
> arch/s390/mm/init.c | 2 +-
> arch/sh/mm/init.c | 3 +++
> arch/x86/mm/init_32.c | 5 +++++
> arch/x86/mm/init_64.c | 2 +-
> include/linux/memory_hotplug.h | 2 ++
> mm/memory_hotplug.c | 2 +-
> mm/memremap.c | 6 +++---
> 10 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 3320406579c3..9b214b0d268f 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1058,7 +1058,8 @@ int arch_add_memory(int nid, u64 start, u64 size,
> flags = NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
>
> __create_pgd_mapping(swapper_pg_dir, start, __phys_to_virt(start),
> - size, PAGE_KERNEL, __pgd_pgtable_alloc, flags);
> + size, modifiers->pgprot, __pgd_pgtable_alloc,
> + flags);
>
> memblock_clear_nomap(start, size);
>
> diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
> index daf438e08b96..5fd6ae4929c9 100644
> --- a/arch/ia64/mm/init.c
> +++ b/arch/ia64/mm/init.c
> @@ -677,6 +677,10 @@ int arch_add_memory(int nid, u64 start, u64 size,
> int ret;
>
> ret = __add_pages(nid, start_pfn, nr_pages, modifiers);
> + if (modifiers->pgprot != PAGE_KERNEL)
> + return -EINVAL;
... maybe better "if (WARN_ON_ONCE(...))"
[...]
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -56,9 +56,11 @@ enum {
> /*
> * Restrictions for the memory hotplug:
> * altmap: alternative allocator for memmap array
> + * pgprot: page protection flags to apply to newly added page tables
> */
> struct mhp_modifiers {
> struct vmem_altmap *altmap;
> + pgprot_t pgprot;
> };
>
> /*
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 1bb3f92e087d..0888f821af06 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1027,7 +1027,7 @@ static int online_memory_block(struct memory_block *mem, void *arg)
> */
> int __ref add_memory_resource(int nid, struct resource *res)
> {
> - struct mhp_modifiers modifiers = {};
> + struct mhp_modifiers modifiers = {.pgprot = PAGE_KERNEL};
I think we usually use spaces like
= { .pgprot = PAGE_KERNEL };
t480s: ~/git/linux virtio-mem-v1 $ git grep "= {\." | wc -l
978
t480s: ~/git/linux virtio-mem-v1 $ git grep "= { " | wc -l
35447
> u64 start, size;
> bool new_node = false;
> int ret;
> diff --git a/mm/memremap.c b/mm/memremap.c
> index e30be8ba706b..45ab4ef0643d 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -163,8 +163,8 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
> * We do not want any optional features only our own memmap
> */
> .altmap = pgmap_altmap(pgmap),
> + .pgprot = PAGE_KERNEL,
> };
> - pgprot_t pgprot = PAGE_KERNEL;
> int error, is_ram;
> bool need_devmap_managed = true;
>
> @@ -252,8 +252,8 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
> if (nid < 0)
> nid = numa_mem_id();
>
> - error = track_pfn_remap(NULL, &pgprot, PHYS_PFN(res->start), 0,
> - resource_size(res));
> + error = track_pfn_remap(NULL, &modifiers.pgprot, PHYS_PFN(res->start),
> + 0, resource_size(res));
> if (error)
> goto err_pfn_remap;
>
>
The !arch code looks good to me (besides I would prefer "params" instead
of "modifiers").
Acked-by: David Hildenbrand <david@xxxxxxxxxx>
--
Thanks,
David / dhildenb
