* [PATCH v2 2/2] powerpc/mm/radix: Use the right page size for vmemmap mapping
From: Aneesh Kumar K.V @ 2019-07-01 14:34 UTC (permalink / raw)
To: npiggin, paulus, mpe; +Cc: Aneesh Kumar K.V, linuxppc-dev
In-Reply-To: <20190701143442.17174-1-aneesh.kumar@linux.ibm.com>
We use mmu_vmemmap_psize to find the page size for mapping the vmmemap area.
With radix translation, we are suboptimally setting this value to PAGE_SIZE.
We do check for 2M page size support and update mmu_vmemap_psize to use
hugepage size but we suboptimally reset the value to PAGE_SIZE in
radix__early_init_mmu(). This resulted in always mapping vmemmap area with
64K page size.
Fixes: 2bfd65e45e87 ("powerpc/mm/radix: Add radix callbacks for early init routines")
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/mm/book3s64/radix_pgtable.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c
index 273ae66a9a45..8deb432c2975 100644
--- a/arch/powerpc/mm/book3s64/radix_pgtable.c
+++ b/arch/powerpc/mm/book3s64/radix_pgtable.c
@@ -515,14 +515,6 @@ void __init radix__early_init_devtree(void)
mmu_psize_defs[MMU_PAGE_64K].shift = 16;
mmu_psize_defs[MMU_PAGE_64K].ap = 0x5;
found:
-#ifdef CONFIG_SPARSEMEM_VMEMMAP
- if (mmu_psize_defs[MMU_PAGE_2M].shift) {
- /*
- * map vmemmap using 2M if available
- */
- mmu_vmemmap_psize = MMU_PAGE_2M;
- }
-#endif /* CONFIG_SPARSEMEM_VMEMMAP */
return;
}
@@ -587,7 +579,13 @@ void __init radix__early_init_mmu(void)
#ifdef CONFIG_SPARSEMEM_VMEMMAP
/* vmemmap mapping */
- mmu_vmemmap_psize = mmu_virtual_psize;
+ if (mmu_psize_defs[MMU_PAGE_2M].shift) {
+ /*
+ * map vmemmap using 2M if available
+ */
+ mmu_vmemmap_psize = MMU_PAGE_2M;
+ } else
+ mmu_vmemmap_psize = mmu_virtual_psize;
#endif
/*
* initialize page table size
--
2.21.0
^ permalink raw reply related
* [PATCH v2 1/2] powerpc/mm/hash/4k: Don't use 64K page size for vmemmap with 4K pagesize
From: Aneesh Kumar K.V @ 2019-07-01 14:34 UTC (permalink / raw)
To: npiggin, paulus, mpe; +Cc: Aneesh Kumar K.V, linuxppc-dev
With hash translation and 4K PAGE_SIZE config, we need to make sure we don't
use 64K page size for vmemmap.
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/mm/book3s64/hash_utils.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
index 28ced26f2a00..a8cc40d2a9ed 100644
--- a/arch/powerpc/mm/book3s64/hash_utils.c
+++ b/arch/powerpc/mm/book3s64/hash_utils.c
@@ -684,10 +684,8 @@ static void __init htab_init_page_sizes(void)
if (mmu_psize_defs[MMU_PAGE_16M].shift &&
memblock_phys_mem_size() >= 0x40000000)
mmu_vmemmap_psize = MMU_PAGE_16M;
- else if (mmu_psize_defs[MMU_PAGE_64K].shift)
- mmu_vmemmap_psize = MMU_PAGE_64K;
else
- mmu_vmemmap_psize = MMU_PAGE_4K;
+ mmu_vmemmap_psize = mmu_virtual_psize;
#endif /* CONFIG_SPARSEMEM_VMEMMAP */
printk(KERN_DEBUG "Page orders: linear mapping = %d, "
--
2.21.0
^ permalink raw reply related
* [PATCH v2] powerpc/mm/nvdimm: Add an informative message if we fail to allocate altmap block
From: Aneesh Kumar K.V @ 2019-07-01 14:33 UTC (permalink / raw)
To: npiggin, paulus, mpe
Cc: Oliver O'Halloran, linuxppc-dev, Aneesh Kumar K.V
Allocation from altmap area can fail based on vmemmap page size used. Add kernel
info message to indicate the failure. That allows the user to identify whether they
are really using persistent memory reserved space for per-page metadata.
The message looks like:
[ 136.587212] altmap block allocation failed, falling back to system memory
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/mm/init_64.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
index a4e17a979e45..f3b64f49082b 100644
--- a/arch/powerpc/mm/init_64.c
+++ b/arch/powerpc/mm/init_64.c
@@ -194,8 +194,12 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
* fail due to alignment issues when using 16MB hugepages, so
* fall back to system memory if the altmap allocation fail.
*/
- if (altmap)
+ if (altmap) {
p = altmap_alloc_block_buf(page_size, altmap);
+ if (!p)
+ pr_debug("altmap block allocation failed, " \
+ "falling back to system memory");
+ }
if (!p)
p = vmemmap_alloc_block_buf(page_size, node);
if (!p)
--
2.21.0
^ permalink raw reply related
* Re: [RFC PATCH] virtio_ring: Use DMA API if guest memory is encrypted
From: Michael S. Tsirkin @ 2019-07-01 14:17 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: Mike Anderson, Michael Roth, Jean-Philippe Brucker, Jason Wang,
Alexey Kardashevskiy, Ram Pai, linux-kernel, virtualization,
iommu, linuxppc-dev, Christoph Hellwig, David Gibson
In-Reply-To: <877e96qxm7.fsf@morokweng.localdomain>
On Thu, Jun 27, 2019 at 10:58:40PM -0300, Thiago Jung Bauermann wrote:
>
> Michael S. Tsirkin <mst@redhat.com> writes:
>
> > On Mon, Jun 03, 2019 at 10:13:59PM -0300, Thiago Jung Bauermann wrote:
> >>
> >>
> >> Michael S. Tsirkin <mst@redhat.com> writes:
> >>
> >> > On Wed, Apr 17, 2019 at 06:42:00PM -0300, Thiago Jung Bauermann wrote:
> >> >> I rephrased it in terms of address translation. What do you think of
> >> >> this version? The flag name is slightly different too:
> >> >>
> >> >>
> >> >> VIRTIO_F_ACCESS_PLATFORM_NO_TRANSLATION This feature has the same
> >> >> meaning as VIRTIO_F_ACCESS_PLATFORM both when set and when not set,
> >> >> with the exception that address translation is guaranteed to be
> >> >> unnecessary when accessing memory addresses supplied to the device
> >> >> by the driver. Which is to say, the device will always use physical
> >> >> addresses matching addresses used by the driver (typically meaning
> >> >> physical addresses used by the CPU) and not translated further. This
> >> >> flag should be set by the guest if offered, but to allow for
> >> >> backward-compatibility device implementations allow for it to be
> >> >> left unset by the guest. It is an error to set both this flag and
> >> >> VIRTIO_F_ACCESS_PLATFORM.
> >> >
> >> >
> >> >
> >> >
> >> > OK so VIRTIO_F_ACCESS_PLATFORM is designed to allow unpriveledged
> >> > drivers. This is why devices fail when it's not negotiated.
> >>
> >> Just to clarify, what do you mean by unprivileged drivers? Is it drivers
> >> implemented in guest userspace such as with VFIO? Or unprivileged in
> >> some other sense such as needing to use bounce buffers for some reason?
> >
> > I had drivers in guest userspace in mind.
>
> Great. Thanks for clarifying.
>
> I don't think this flag would work for guest userspace drivers. Should I
> add a note about that in the flag definition?
I think you need to clarify access protection rules. Is it only
translation that is bypassed or is any platform-specific
protection mechanism bypassed too?
> >> > This confuses me.
> >> > If driver is unpriveledged then what happens with this flag?
> >> > It can supply any address it wants. Will that corrupt kernel
> >> > memory?
> >>
> >> Not needing address translation doesn't necessarily mean that there's no
> >> IOMMU. On powerpc we don't use VIRTIO_F_ACCESS_PLATFORM but there's
> >> always an IOMMU present. And we also support VFIO drivers. The VFIO API
> >> for pseries (sPAPR section in Documentation/vfio.txt) has extra ioctls
> >> to program the IOMMU.
> >>
> >> For our use case, we don't need address translation because we set up an
> >> identity mapping in the IOMMU so that the device can use guest physical
> >> addresses.
OK so I think I am beginning to see it in a different light. Right now the specific
platform creates an identity mapping. That in turn means DMA API can be
fast - it does not need to do anything. What you are looking for is a
way to tell host it's an identity mapping - just as an optimization.
Is that right? So this is what I would call this option:
VIRTIO_F_ACCESS_PLATFORM_IDENTITY_ADDRESS
and the explanation should state that all device
addresses are translated by the platform to identical
addresses.
In fact this option then becomes more, not less restrictive
than VIRTIO_F_ACCESS_PLATFORM - it's a promise
by guest to only create identity mappings,
and only before driver_ok is set.
This option then would always be negotiated together with
VIRTIO_F_ACCESS_PLATFORM.
Host then must verify that
1. full 1:1 mappings are created before driver_ok
or can we make sure this happens before features_ok?
that would be ideal as we could require that features_ok fails
2. mappings are not modified between driver_ok and reset
i guess attempts to change them will fail -
possibly by causing a guest crash
or some other kind of platform-specific error
So far so good, but now a question:
how are we handling guest address width limitations?
Is VIRTIO_F_ACCESS_PLATFORM_IDENTITY_ADDRESS subject to
guest address width limitations?
I am guessing we can make them so ...
This needs to be documented.
> >
> > And can it access any guest physical address?
>
> Sorry, I was mistaken. We do support VFIO in guests but not for virtio
> devices, only for regular PCI devices. In which case they will use
> address translation.
Not sure how this answers the question.
> >> If the guest kernel is concerned that an unprivileged driver could
> >> jeopardize its integrity it should not negotiate this feature flag.
> >
> > Unfortunately flag negotiation is done through config space
> > and so can be overwritten by the driver.
>
> Ok, so the guest kernel has to forbid VFIO access on devices where this
> flag is advertised.
That's possible in theory but in practice we did not yet teach VFIO not
to attach to legacy devices without VIRTIO_F_ACCESS_PLATFORM. So all
security relies on host denying driver_ok without
VIRTIO_F_ACCESS_PLATFORM. New options that bypass guest security are
thus tricky as they can create security holes for existing guests.
I'm open to ideas about how to do this in a safe way,
> >> Perhaps there should be a note about this in the flag definition? This
> >> concern is platform-dependant though. I don't believe it's an issue in
> >> pseries.
> >
> > Again ACCESS_PLATFORM has a pretty open definition. It does actually
> > say it's all up to the platform.
> >
> > Specifically how will VIRTIO_F_ACCESS_PLATFORM_NO_TRANSLATION be
> > implemented portably? virtio has no portable way to know
> > whether DMA API bypasses translation.
>
> The fact that VIRTIO_F_ACCESS_PLATFORM_NO_TRANSLATION is set
> communicates that knowledge to virtio. There is a shared understanding
> between the guest and the host about what this flag being set means.
Right but I wonder how are you going to *actually* implement it on Linux?
Are you adding a new set of DMA APIs that do everything except
translation?
> --
> Thiago Jung Bauermann
> IBM Linux Technology Center
^ permalink raw reply
* Re: [PATCH v3 3/9] powerpc: Introduce FW_FEATURE_ULTRAVISOR
From: Claudio Carvalho @ 2019-07-01 14:12 UTC (permalink / raw)
To: Paul Mackerras
Cc: Madhavan Srinivasan, Michael Anderson, Ram Pai, kvm-ppc,
Bharata B Rao, linuxppc-dev, Sukadev Bhattiprolu,
Anshuman Khandual
In-Reply-To: <20190615073600.GA24709@blackberry>
On 6/15/19 4:36 AM, Paul Mackerras wrote:
> On Thu, Jun 06, 2019 at 02:36:08PM -0300, Claudio Carvalho wrote:
>> This feature tells if the ultravisor firmware is available to handle
>> ucalls.
> Everything in this patch that depends on CONFIG_PPC_UV should just
> depend on CONFIG_PPC_POWERNV instead. The reason is that every host
> kernel needs to be able to do the ultracall to set partition table
> entry 0, in case it ends up being run on a machine with an ultravisor.
> Otherwise we will have the situation where a host kernel may crash
> early in boot just because the machine it's booted on happens to have
> an ultravisor running. The crash will be a particularly nasty one
> because it will happen before we have probed the machine type and
> initialized the console; therefore it will just look like the machine
> hangs for no discernable reason.
> We also need to think about how to provide a way for petitboot to know
> whether the kernel it is booting knows how to do a ucall to set its
> partition table entry. One suggestion would be to modify
> vmlinux.lds.S to add a new PT_NOTE entry in the program header of the
> binary with (say) a 64-bit doubleword which is a bitmap indicating
> capabilities of the binary. We would define the first bit as
> indicating that the kernel knows how to run under an ultravisor.
> When running under an ultravisor, petitboot could then look for the
> PT_NOTE and the ultravisor-capable bit in it, and if the PT_NOTE is
> not there or the bit is zero, put up a dialog warning the user that
> the kernel will probably crash early in boot, and asking for explicit
> confirmation that the user wants to proceed.
I just posted a separated RFC patch for the ELF note.
Thanks, Claudio.
>
> Paul.
>
^ permalink raw reply
* [RFC PATCH] powerpc: Add the ppc_capabilities ELF note
From: Claudio Carvalho @ 2019-07-01 14:09 UTC (permalink / raw)
To: linuxppc-dev
Cc: Michael Anderson, Ram Pai, Claudio Carvalho, kvm-ppc,
Thiago Bauermann
Add the ppc_capabilities ELF note to the powerpc kernel binary. It is a
bitmap that can be used to advertise kernel capabilities to userland.
This patch also defines PPCCAP_ULTRAVISOR_BIT as being the bit zero.
Suggested-by: Paul Mackerras <paulus@ozlabs.org>
Signed-off-by: Claudio Carvalho <cclaudio@linux.ibm.com>
---
arch/powerpc/kernel/Makefile | 2 +-
arch/powerpc/kernel/note.S | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 37 insertions(+), 1 deletion(-)
create mode 100644 arch/powerpc/kernel/note.S
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 0ea6c4aa3a20..4ec36fe4325b 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -49,7 +49,7 @@ obj-y := cputable.o ptrace.o syscalls.o \
signal.o sysfs.o cacheinfo.o time.o \
prom.o traps.o setup-common.o \
udbg.o misc.o io.o misc_$(BITS).o \
- of_platform.o prom_parse.o
+ of_platform.o prom_parse.o note.o
obj-$(CONFIG_PPC64) += setup_64.o sys_ppc32.o \
signal_64.o ptrace32.o \
paca.o nvram_64.o firmware.o
diff --git a/arch/powerpc/kernel/note.S b/arch/powerpc/kernel/note.S
new file mode 100644
index 000000000000..721bf8ce9eb7
--- /dev/null
+++ b/arch/powerpc/kernel/note.S
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PowerPC ELF notes.
+ *
+ * Copyright 2019, IBM Corporation
+ */
+#include <linux/elfnote.h>
+
+/*
+ * Ultravisor-capable bit (PowerNV only).
+ *
+ * Indicate that the powerpc kernel binary knows how to run in an
+ * ultravisor-enabled system.
+ *
+ * In an ultravisor-enabled system, some machine resources are now controlled
+ * by the ultravisor. If the kernel is not ultravisor-capable, but it ends up
+ * being run on a machine with ultravisor, the kernel will probably crash
+ * trying to access ultravisor resources. For instance, it may crash in early
+ * boot trying to set the partition table entry 0.
+ *
+ * In an ultravisor-enabled system, petitboot can warn the user or prevent the
+ * kernel from being run if the ppc_capabilities doesn't exist or the
+ * Ultravisor-capable bit is not set.
+ */
+#if defined(CONFIG_PPC_POWERNV)
+#define PPCCAP_ULTRAVISOR_BIT (1 << 0)
+#else
+#define PPCCAP_ULTRAVISOR_BIT 0
+#endif
+
+/*
+ * Add the ppc_capabilities ELF note to the powerpc kernel binary. It is a
+ * bitmap that can be used to advertise kernel capabilities to userland.
+ */
+ELFNOTE(ppc_capabilities, 3,
+ .long PPCCAP_ULTRAVISOR_BIT)
--
2.20.1
^ permalink raw reply related
* [PATCH] mm/nvdimm: Add is_ioremap_addr and use that to check ioremap address
From: Aneesh Kumar K.V @ 2019-07-01 13:40 UTC (permalink / raw)
To: dan.j.williams, akpm
Cc: linux-mm, linuxppc-dev, Aneesh Kumar K.V, linux-nvdimm
Architectures like powerpc use different address range to map ioremap
and vmalloc range. The memunmap() check used by the nvdimm layer was
wrongly using is_vmalloc_addr() to check for ioremap range which fails for
ppc64. This result in ppc64 not freeing the ioremap mapping. The side effect
of this is an unbind failure during module unload with papr_scm nvdimm driver
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
---
arch/powerpc/include/asm/pgtable.h | 14 ++++++++++++++
include/linux/mm.h | 5 +++++
kernel/iomem.c | 2 +-
3 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index 3f53be60fb01..64145751b2fd 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -140,6 +140,20 @@ static inline void pte_frag_set(mm_context_t *ctx, void *p)
}
#endif
+#ifdef CONFIG_PPC64
+#define is_ioremap_addr is_ioremap_addr
+static inline bool is_ioremap_addr(const void *x)
+{
+#ifdef CONFIG_MMU
+ unsigned long addr = (unsigned long)x;
+
+ return addr >= IOREMAP_BASE && addr < IOREMAP_END;
+#else
+ return false;
+#endif
+}
+#endif /* CONFIG_PPC64 */
+
#endif /* __ASSEMBLY__ */
#endif /* _ASM_POWERPC_PGTABLE_H */
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 973ebf71f7b6..65b2eb6c9f0a 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -633,6 +633,11 @@ static inline bool is_vmalloc_addr(const void *x)
return false;
#endif
}
+
+#ifndef is_ioremap_addr
+#define is_ioremap_addr(x) is_vmalloc_addr(x)
+#endif
+
#ifdef CONFIG_MMU
extern int is_vmalloc_or_module_addr(const void *x);
#else
diff --git a/kernel/iomem.c b/kernel/iomem.c
index 93c264444510..62c92e43aa0d 100644
--- a/kernel/iomem.c
+++ b/kernel/iomem.c
@@ -121,7 +121,7 @@ EXPORT_SYMBOL(memremap);
void memunmap(void *addr)
{
- if (is_vmalloc_addr(addr))
+ if (is_ioremap_addr(addr))
iounmap((void __iomem *) addr);
}
EXPORT_SYMBOL(memunmap);
--
2.21.0
^ permalink raw reply related
* Can I compile Linux Kernel 5.2-rc7 for PowerPC on Intel/AMD x86 hardware?
From: Turritopsis Dohrnii Teo En Ming @ 2019-07-01 13:39 UTC (permalink / raw)
To: linuxppc-dev@lists.ozlabs.org; +Cc: Turritopsis Dohrnii Teo En Ming
Good evening from Singapore,
Can I compile Linux Kernel 5.2-rc7 for PowerPC on Intel/AMD x86 hardware, for example, AMD Ryzen 9 3950X, with 16 CPU cores and 32 threads?
Is it called cross-compiling?
Thank you.
-----BEGIN EMAIL SIGNATURE-----
The Gospel for all Targeted Individuals (TIs):
[The New York Times] Microwave Weapons Are Prime Suspect in Ills of
U.S. Embassy Workers
Link: https://www.nytimes.com/2018/09/01/science/sonic-attack-cuba-microwave.html
********************************************************************************************
Singaporean Mr. Turritopsis Dohrnii Teo En Ming's Academic
Qualifications as at 14 Feb 2019
[1] https://tdtemcerts.wordpress.com/
[2] https://tdtemcerts.blogspot.sg/
[3] https://www.scribd.com/user/270125049/Teo-En-Ming
-----END EMAIL SIGNATURE-----
^ permalink raw reply
* Re: [PATCH RFC] generic ELF support for kexec
From: Philipp Rudo @ 2019-07-01 12:31 UTC (permalink / raw)
To: Sven Schnelle; +Cc: linux-s390, deller, kexec, linuxppc-dev
In-Reply-To: <20190625185433.GA10934@t470p.stackframe.org>
Hi Sven,
On Tue, 25 Jun 2019 20:54:34 +0200
Sven Schnelle <svens@stackframe.org> wrote:
> Hi List,
>
> i recently started working on kexec for PA-RISC. While doing so, i figured
> that powerpc already has support for reading ELF images inside of the Kernel.
> My first attempt was to steal the source code and modify it for PA-RISC, but
> it turned out that i didn't had to change much. Only ARM specific stuff like
> fdt blob fetching had to be removed.
>
> So instead of duplicating the code, i thought about moving the ELF stuff to
> the core kexec code, and exposing several function to use that code from the
> arch specific code.
That's always the right approach. Well done.
> I'm attaching the patch to this Mail. What do you think about that change?
> s390 also uses ELF files, and (maybe?) could also switch to this implementation.
> But i don't know anything about S/390 and don't have one in my basement. So
> i'll leave s390 to the IBM folks.
I'm afraid there isn't much code here s390 can reuse. I see multiple problems
in kexec_elf_load:
* while loading the phdrs we also need to setup some data structures to pass
to the next kernel
* the s390 kernel needs to be loaded to a fixed address
* there is no support to load a crash kernel
Of course that could all be fixed/worked around by introducing some arch hooks.
But when you take into account that the whole elf loader on s390 is ~100 lines
of code, I don't think it is worth it.
More comments below.
[...]
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index b9b1bc5f9669..49b23b425f84 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -216,6 +216,41 @@ extern int crash_prepare_elf64_headers(struct crash_mem *mem, int kernel_map,
> void **addr, unsigned long *sz);
> #endif /* CONFIG_KEXEC_FILE */
>
> +#ifdef CONFIG_KEXEC_FILE_ELF
> +
> +struct kexec_elf_info {
> + /*
> + * Where the ELF binary contents are kept.
> + * Memory managed by the user of the struct.
> + */
> + const char *buffer;
> +
> + const struct elfhdr *ehdr;
> + const struct elf_phdr *proghdrs;
> + struct elf_shdr *sechdrs;
> +};
Do i understand this right? elf_info->buffer contains the full elf file and
elf_info->ehdr is a (endianness translated) copy of the files ehdr?
If so ...
> +void kexec_free_elf_info(struct kexec_elf_info *elf_info);
> +
> +int kexec_build_elf_info(const char *buf, size_t len, struct elfhdr *ehdr,
> + struct kexec_elf_info *elf_info);
> +
> +int kexec_elf_kernel_load(struct kimage *image, struct kexec_buf *kbuf,
> + char *kernel_buf, unsigned long kernel_len,
> + unsigned long *kernel_load_addr);
> +
> +int kexec_elf_probe(const char *buf, unsigned long len);
> +
> +int kexec_elf_load(struct kimage *image, struct elfhdr *ehdr,
> + struct kexec_elf_info *elf_info,
> + struct kexec_buf *kbuf,
> + unsigned long *lowest_load_addr);
> +
> +int kexec_elf_load(struct kimage *image, struct elfhdr *ehdr,
> + struct kexec_elf_info *elf_info,
> + struct kexec_buf *kbuf,
> + unsigned long *lowest_load_addr);
... you could simplify the arguments by dropping the ehdr argument. The
elf_info should contain all the information needed. Furthermore the kexec_buf
also contains a pointer to its kimage. So you can drop that argument as well.
An other thing is that you kzalloc the memory needed for proghdrs and sechdrs
but expect the user of those functions to provide the memory needed for ehdr.
Wouldn't it be more consistent to also kzalloc the ehdr?
[...]
> diff --git a/kernel/kexec_file_elf.c b/kernel/kexec_file_elf.c
> new file mode 100644
> index 000000000000..bb966c93492c
> --- /dev/null
> +++ b/kernel/kexec_file_elf.c
> @@ -0,0 +1,574 @@
[...]
> +static uint64_t elf64_to_cpu(const struct elfhdr *ehdr, uint64_t value)
> +{
> + if (ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
> + value = le64_to_cpu(value);
> + else if (ehdr->e_ident[EI_DATA] == ELFDATA2MSB)
> + value = be64_to_cpu(value);
> +
> + return value;
> +}
> +
> +static uint16_t elf16_to_cpu(const struct elfhdr *ehdr, uint16_t value)
> +{
> + if (ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
> + value = le16_to_cpu(value);
> + else if (ehdr->e_ident[EI_DATA] == ELFDATA2MSB)
> + value = be16_to_cpu(value);
> +
> + return value;
> +}
> +
> +static uint32_t elf32_to_cpu(const struct elfhdr *ehdr, uint32_t value)
> +{
> + if (ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
> + value = le32_to_cpu(value);
> + else if (ehdr->e_ident[EI_DATA] == ELFDATA2MSB)
> + value = be32_to_cpu(value);
> +
> + return value;
> +}
What are the elf*_to_cpu good for? In general I'd assume that kexec loads a
kernel for the same architecture it is running on. So the new kernel should
also have the same endianness like the one which loads it. Is this a
ppcle/ppcbe issue?
Furthermore the current order is 64->16->32, which my OCPD absolutely hates :)
[...]
> +/**
> + * elf_is_shdr_sane - check that it is safe to use the section header
> + * @buf_len: size of the buffer in which the ELF file is loaded.
> + */
> +static bool elf_is_shdr_sane(const struct elf_shdr *shdr, size_t buf_len)
> +{
> + bool size_ok;
> +
> + /* SHT_NULL headers have undefined values, so we can't check them. */
> + if (shdr->sh_type == SHT_NULL)
> + return true;
> +
> + /* Now verify sh_entsize */
> + switch (shdr->sh_type) {
> + case SHT_SYMTAB:
> + size_ok = shdr->sh_entsize == sizeof(Elf_Sym);
> + break;
> + case SHT_RELA:
> + size_ok = shdr->sh_entsize == sizeof(Elf_Rela);
> + break;
> + case SHT_DYNAMIC:
> + size_ok = shdr->sh_entsize == sizeof(Elf_Dyn);
> + break;
> + case SHT_REL:
> + size_ok = shdr->sh_entsize == sizeof(Elf_Rel);
> + break;
> + case SHT_NOTE:
> + case SHT_PROGBITS:
> + case SHT_HASH:
> + case SHT_NOBITS:
> + default:
> + /*
> + * This is a section whose entsize requirements
> + * I don't care about. If I don't know about
> + * the section I can't care about it's entsize
> + * requirements.
> + */
> + size_ok = true;
> + break;
> + }
> +
> + if (!size_ok) {
> + pr_debug("ELF section with wrong entry size.\n");
> + return false;
> + } else if (shdr->sh_addr + shdr->sh_size < shdr->sh_addr) {
> + pr_debug("ELF section address wraps around.\n");
> + return false;
> + }
> +
> + if (shdr->sh_type != SHT_NOBITS) {
> + if (shdr->sh_offset + shdr->sh_size < shdr->sh_offset) {
> + pr_debug("ELF section location wraps around.\n");
> + return false;
> + } else if (shdr->sh_offset + shdr->sh_size > buf_len) {
> + pr_debug("ELF section not in file.\n");
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> +static int elf_read_shdr(const char *buf, size_t len, struct kexec_elf_info *elf_info,
> + int idx)
> +{
> + struct elf_shdr *shdr = &elf_info->sechdrs[idx];
> + const struct elfhdr *ehdr = elf_info->ehdr;
> + const char *sbuf;
> + struct elf_shdr *buf_shdr;
> +
> + sbuf = buf + ehdr->e_shoff + idx * sizeof(*buf_shdr);
> + buf_shdr = (struct elf_shdr *) sbuf;
> +
> + shdr->sh_name = elf32_to_cpu(ehdr, buf_shdr->sh_name);
> + shdr->sh_type = elf32_to_cpu(ehdr, buf_shdr->sh_type);
> + shdr->sh_addr = elf_addr_to_cpu(ehdr, buf_shdr->sh_addr);
> + shdr->sh_offset = elf_addr_to_cpu(ehdr, buf_shdr->sh_offset);
> + shdr->sh_link = elf32_to_cpu(ehdr, buf_shdr->sh_link);
> + shdr->sh_info = elf32_to_cpu(ehdr, buf_shdr->sh_info);
> +
> + /*
> + * The following fields have a type equivalent to Elf_Addr
> + * both in 32 bit and 64 bit ELF.
> + */
> + shdr->sh_flags = elf_addr_to_cpu(ehdr, buf_shdr->sh_flags);
> + shdr->sh_size = elf_addr_to_cpu(ehdr, buf_shdr->sh_size);
> + shdr->sh_addralign = elf_addr_to_cpu(ehdr, buf_shdr->sh_addralign);
> + shdr->sh_entsize = elf_addr_to_cpu(ehdr, buf_shdr->sh_entsize);
> +
> + return elf_is_shdr_sane(shdr, len) ? 0 : -ENOEXEC;
> +}
> +
> +/**
> + * elf_read_shdrs - read the section headers from the buffer
> + *
> + * This function assumes that the section header table was checked for sanity.
> + * Use elf_is_ehdr_sane() if it wasn't.
> + */
> +static int elf_read_shdrs(const char *buf, size_t len,
> + struct kexec_elf_info *elf_info)
> +{
> + size_t shdr_size, i;
> +
> + /*
> + * e_shnum is at most 65536 so calculating
> + * the size of the section header cannot overflow.
> + */
> + shdr_size = sizeof(struct elf_shdr) * elf_info->ehdr->e_shnum;
> +
> + elf_info->sechdrs = kzalloc(shdr_size, GFP_KERNEL);
> + if (!elf_info->sechdrs)
> + return -ENOMEM;
> +
> + for (i = 0; i < elf_info->ehdr->e_shnum; i++) {
> + int ret;
> +
> + ret = elf_read_shdr(buf, len, elf_info, i);
> + if (ret) {
> + kfree(elf_info->sechdrs);
> + elf_info->sechdrs = NULL;
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
In the end you only use the phdrs. So in theory you can drop everything shdr
related. Although keeping it would be 'formally more correct'.
[...]
> +/**
> + * kexec_free_elf_info - free memory allocated by elf_read_from_buffer
> + */
> +void kexec_free_elf_info(struct kexec_elf_info *elf_info)
> +{
> + kfree(elf_info->proghdrs);
> + kfree(elf_info->sechdrs);
> + memset(elf_info, 0, sizeof(*elf_info));
> +}
> +EXPORT_SYMBOL(kexec_free_elf_info);
Why are you exporting these functions? Is there any kexec implementation out
there which is put into a module? Do you even want that to be possible?
> +/**
> + * kexec_build_elf_info - read ELF executable and check that we can use it
> + */
> +int kexec_build_elf_info(const char *buf, size_t len, struct elfhdr *ehdr,
> + struct kexec_elf_info *elf_info)
> +{
> + int i;
> + int ret;
> +
> + ret = elf_read_from_buffer(buf, len, ehdr, elf_info);
> + if (ret)
> + return ret;
> +
> + /* Big endian vmlinux has type ET_DYN. */
> + if (ehdr->e_type != ET_EXEC && ehdr->e_type != ET_DYN) {
s390 is big endian and it's vmlinux has type ET_EXEC. So I assume that this is
a ppc issue?
> + pr_err("Not an ELF executable.\n");
> + goto error;
> + } else if (!elf_info->proghdrs) {
> + pr_err("No ELF program header.\n");
> + goto error;
> + }
> +
> + for (i = 0; i < ehdr->e_phnum; i++) {
> + /*
> + * Kexec does not support loading interpreters.
> + * In addition this check keeps us from attempting
> + * to kexec ordinay executables.
> + */
> + if (elf_info->proghdrs[i].p_type == PT_INTERP) {
> + pr_err("Requires an ELF interpreter.\n");
> + goto error;
> + }
> + }
> +
> + return 0;
> +error:
> + kexec_free_elf_info(elf_info);
> + return -ENOEXEC;
> +}
> +EXPORT_SYMBOL(kexec_build_elf_info);
[...]
> +int kexec_elf_probe(const char *buf, unsigned long len)
> +{
> + struct elfhdr ehdr;
> + struct kexec_elf_info elf_info;
> + int ret;
> +
> + ret = kexec_build_elf_info(buf, len, &ehdr, &elf_info);
On s390 I only check the elf magic when probing. That's because the image
loader cannot reliably check the image and thus accepts everything that is
given to it. That also means that any elf file the elf probe rejects (e.g.
because it has a phdr with type PT_INTERP) is passed on to the image loader,
which happily takes it.
If you plan to also add an image loader you should keep that in mind.
Thanks
Philipp
^ permalink raw reply
* Re: [PATCH v3 06/11] mm/memory_hotplug: Allow arch_remove_pages() without CONFIG_MEMORY_HOTREMOVE
From: Michal Hocko @ 2019-07-01 12:51 UTC (permalink / raw)
To: David Hildenbrand
Cc: Oscar Salvador, Rich Felker, linux-ia64, Anshuman Khandual,
linux-sh, Peter Zijlstra, Dave Hansen, Heiko Carstens, Wei Yang,
linux-mm, Arun KS, Paul Mackerras, H. Peter Anvin,
Thomas Gleixner, Qian Cai, linux-s390, Yoshinori Sato,
Rafael J. Wysocki, Mike Rapoport, Ingo Molnar, Fenghua Yu,
Pavel Tatashin, Vasily Gorbik, Rob Herring, mike.travis@hpe.com,
Nicholas Piggin, Alex Deucher, Mark Brown, Borislav Petkov,
Andy Lutomirski, Dan Williams, Chris Wilson, linux-arm-kernel,
Tony Luck, Baoquan He, Masahiro Yamada, Mathieu Malaterre,
Greg Kroah-Hartman, Andrew Banman, linux-kernel, Logan Gunthorpe,
Wei Yang, Martin Schwidefsky, Igor Mammedov, akpm, linuxppc-dev,
David S. Miller, Kirill A. Shutemov
In-Reply-To: <20190701080141.GF6376@dhcp22.suse.cz>
On Mon 01-07-19 10:01:41, Michal Hocko wrote:
> On Mon 27-05-19 13:11:47, David Hildenbrand wrote:
> > We want to improve error handling while adding memory by allowing
> > to use arch_remove_memory() and __remove_pages() even if
> > CONFIG_MEMORY_HOTREMOVE is not set to e.g., implement something like:
> >
> > arch_add_memory()
> > rc = do_something();
> > if (rc) {
> > arch_remove_memory();
> > }
> >
> > We won't get rid of CONFIG_MEMORY_HOTREMOVE for now, as it will require
> > quite some dependencies for memory offlining.
>
> If we cannot really remove CONFIG_MEMORY_HOTREMOVE altogether then why
> not simply add an empty placeholder for arch_remove_memory when the
> config is disabled?
In other words, can we replace this by something as simple as:
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index ae892eef8b82..0329027fe740 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -128,6 +128,20 @@ extern void arch_remove_memory(int nid, u64 start, u64 size,
struct vmem_altmap *altmap);
extern void __remove_pages(struct zone *zone, unsigned long start_pfn,
unsigned long nr_pages, struct vmem_altmap *altmap);
+#else
+/*
+ * Allow code using
+ * arch_add_memory();
+ * rc = do_something();
+ * if (rc)
+ * arch_remove_memory();
+ *
+ * without ifdefery.
+ */
+static inline void arch_remove_memory(int nid, u64 start, u64 size,
+ struct vmem_altmap *altmap)
+{
+}
#endif /* CONFIG_MEMORY_HOTREMOVE */
/*
--
Michal Hocko
SUSE Labs
^ permalink raw reply related
* Re: [PATCH v3 04/11] arm64/mm: Add temporary arch_remove_memory() implementation
From: Michal Hocko @ 2019-07-01 12:48 UTC (permalink / raw)
To: David Hildenbrand
Cc: Mark Rutland, linux-s390, linux-ia64, Yu Zhao, Anshuman Khandual,
linux-sh, Catalin Marinas, Ard Biesheuvel, Will Deacon,
linux-kernel, Wei Yang, Jun Yao, linux-mm, Chintan Pandya,
Igor Mammedov, akpm, Mike Rapoport, linuxppc-dev, Dan Williams,
linux-arm-kernel, Robin Murphy
In-Reply-To: <20190527111152.16324-5-david@redhat.com>
On Mon 27-05-19 13:11:45, David Hildenbrand wrote:
> A proper arch_remove_memory() implementation is on its way, which also
> cleanly removes page tables in arch_add_memory() in case something goes
> wrong.
>
> As we want to use arch_remove_memory() in case something goes wrong
> during memory hotplug after arch_add_memory() finished, let's add
> a temporary hack that is sufficient enough until we get a proper
> implementation that cleans up page table entries.
>
> We will remove CONFIG_MEMORY_HOTREMOVE around this code in follow up
> patches.
I would drop this one as well (like s390 counterpart).
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Chintan Pandya <cpandya@codeaurora.org>
> Cc: Mike Rapoport <rppt@linux.ibm.com>
> Cc: Jun Yao <yaojun8558363@gmail.com>
> Cc: Yu Zhao <yuzhao@google.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Anshuman Khandual <anshuman.khandual@arm.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> arch/arm64/mm/mmu.c | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
>
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index a1bfc4413982..e569a543c384 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1084,4 +1084,23 @@ int arch_add_memory(int nid, u64 start, u64 size,
> return __add_pages(nid, start >> PAGE_SHIFT, size >> PAGE_SHIFT,
> restrictions);
> }
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +void arch_remove_memory(int nid, u64 start, u64 size,
> + struct vmem_altmap *altmap)
> +{
> + unsigned long start_pfn = start >> PAGE_SHIFT;
> + unsigned long nr_pages = size >> PAGE_SHIFT;
> + struct zone *zone;
> +
> + /*
> + * FIXME: Cleanup page tables (also in arch_add_memory() in case
> + * adding fails). Until then, this function should only be used
> + * during memory hotplug (adding memory), not for memory
> + * unplug. ARCH_ENABLE_MEMORY_HOTREMOVE must not be
> + * unlocked yet.
> + */
> + zone = page_zone(pfn_to_page(start_pfn));
> + __remove_pages(zone, start_pfn, nr_pages, altmap);
> +}
> +#endif
> #endif
> --
> 2.20.1
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v3 03/11] s390x/mm: Implement arch_remove_memory()
From: Michal Hocko @ 2019-07-01 12:47 UTC (permalink / raw)
To: David Hildenbrand
Cc: Oscar Salvador, linux-s390, linux-ia64, Vasily Gorbik, linux-sh,
Heiko Carstens, linux-kernel, Wei Yang, linux-mm, Mike Rapoport,
Martin Schwidefsky, Igor Mammedov, akpm, linuxppc-dev,
Dan Williams, linux-arm-kernel
In-Reply-To: <20190701074503.GD6376@dhcp22.suse.cz>
On Mon 01-07-19 09:45:03, Michal Hocko wrote:
> On Mon 27-05-19 13:11:44, David Hildenbrand wrote:
> > Will come in handy when wanting to handle errors after
> > arch_add_memory().
>
> I do not understand this. Why do you add a code for something that is
> not possible on this HW (based on the comment - is it still valid btw?)
Same as the previous patch (drop it).
> > Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> > Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Mike Rapoport <rppt@linux.vnet.ibm.com>
> > Cc: David Hildenbrand <david@redhat.com>
> > Cc: Vasily Gorbik <gor@linux.ibm.com>
> > Cc: Oscar Salvador <osalvador@suse.com>
> > Signed-off-by: David Hildenbrand <david@redhat.com>
> > ---
> > arch/s390/mm/init.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
> > index d552e330fbcc..14955e0a9fcf 100644
> > --- a/arch/s390/mm/init.c
> > +++ b/arch/s390/mm/init.c
> > @@ -243,12 +243,13 @@ int arch_add_memory(int nid, u64 start, u64 size,
> > void arch_remove_memory(int nid, u64 start, u64 size,
> > struct vmem_altmap *altmap)
> > {
> > - /*
> > - * There is no hardware or firmware interface which could trigger a
> > - * hot memory remove on s390. So there is nothing that needs to be
> > - * implemented.
> > - */
> > - BUG();
> > + unsigned long start_pfn = start >> PAGE_SHIFT;
> > + unsigned long nr_pages = size >> PAGE_SHIFT;
> > + struct zone *zone;
> > +
> > + zone = page_zone(pfn_to_page(start_pfn));
> > + __remove_pages(zone, start_pfn, nr_pages, altmap);
> > + vmem_remove_mapping(start, size);
> > }
> > #endif
> > #endif /* CONFIG_MEMORY_HOTPLUG */
> > --
> > 2.20.1
> >
>
> --
> Michal Hocko
> SUSE Labs
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v3 02/11] s390x/mm: Fail when an altmap is used for arch_add_memory()
From: Michal Hocko @ 2019-07-01 12:46 UTC (permalink / raw)
To: David Hildenbrand
Cc: Oscar Salvador, linux-s390, linux-ia64, Vasily Gorbik, linux-sh,
Heiko Carstens, linux-kernel, Wei Yang, linux-mm, Mike Rapoport,
Martin Schwidefsky, Igor Mammedov, akpm, linuxppc-dev,
Dan Williams, linux-arm-kernel
In-Reply-To: <20190701074306.GC6376@dhcp22.suse.cz>
On Mon 01-07-19 09:43:06, Michal Hocko wrote:
> On Mon 27-05-19 13:11:43, David Hildenbrand wrote:
> > ZONE_DEVICE is not yet supported, fail if an altmap is passed, so we
> > don't forget arch_add_memory()/arch_remove_memory() when unlocking
> > support.
>
> Why do we need this? Sure ZONE_DEVICE is not supported for s390 and so
> might be the case for other arches which support hotplug. I do not see
> much point in adding warning to each of them.
I would drop this one. If there is a strong reason to have something
like that it should come with a better explanation and it can be done on
top.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] powerpc/mm/nvdimm: Add an informative message if we fail to allocate altmap block
From: Aneesh Kumar K.V @ 2019-07-01 10:57 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev, Paul Mackerras, Nicholas Piggin
In-Reply-To: <CAOSf1CH_X7TG7Cato4hBt+U5=5HHHwR3hXQYC-z_GiBQiWnB1w@mail.gmail.com>
"Oliver O'Halloran" <oohall@gmail.com> writes:
> On Sat, Jun 29, 2019 at 5:39 PM Aneesh Kumar K.V
> <aneesh.kumar@linux.ibm.com> wrote:
>>
>> Allocation from altmap area can fail based on vmemmap page size used. Add kernel
>> info message to indicate the failure. That allows the user to identify whether they
>> are really using persistent memory reserved space for per-page metadata.
>>
>> The message looks like:
>> [ 136.587212] altmap block allocation failed, falling back to system memory
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>> arch/powerpc/mm/init_64.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
>> index a4e17a979e45..57c0573650dc 100644
>> --- a/arch/powerpc/mm/init_64.c
>> +++ b/arch/powerpc/mm/init_64.c
>> @@ -194,8 +194,12 @@ int __meminit vmemmap_populate(unsigned long start, unsigned long end, int node,
>> * fail due to alignment issues when using 16MB hugepages, so
>> * fall back to system memory if the altmap allocation fail.
>> */
>> - if (altmap)
>> + if (altmap) {
>> p = altmap_alloc_block_buf(page_size, altmap);
>> + if (!p)
>
>> + pr_info("altmap block allocation failed, " \
>> + "falling back to system memory");
>
> I think this is kind of misleading. If you're mapping a large amount
> of memory you can have most of the vmemmap backing allocated from the
> altmap and one extra block allocated from normal memory. E.g. If you
> have 32MB of altmap space, one 16MB block will be allocated from the
> altmap, but the 2nd 16MB block is probably unusable due to the
> reserved pages at the start of the altmap. Maybe this should be a
> pr_debug() so it's only printed along with the "vmemmap_populate ..."
> message above?
Will switch to pr_debug. What I really wanted was an indication of which
pfn device failed to allocate per page meata data in the device. But
we really don't have device details here and we don't end up calling
this function if there is already a 16MB mapping in DRAM for this area.
>
> Also, isn't kernel style to keep printf()s, even long ones, on one line?
I was not sure. It do print to kernel log in one line.
-aneesh
^ permalink raw reply
* Re: Re: [PATCH 1/3] arm64: mm: Add p?d_large() definitions
From: Will Deacon @ 2019-07-01 10:15 UTC (permalink / raw)
To: Steven Price
Cc: Mark Rutland, Anshuman Khandual, Catalin Marinas, Ard Biesheuvel,
Will Deacon, Nicholas Piggin, linux-mm, Andrew Morton,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <3d002af8-d8cd-f750-132e-12109e1e3039@arm.com>
On Mon, Jul 01, 2019 at 11:03:51AM +0100, Steven Price wrote:
> On 01/07/2019 10:27, Will Deacon wrote:
> > On Sun, Jun 23, 2019 at 07:44:44PM +1000, Nicholas Piggin wrote:
> >> walk_page_range() is going to be allowed to walk page tables other than
> >> those of user space. For this it needs to know when it has reached a
> >> 'leaf' entry in the page tables. This information will be provided by the
> >> p?d_large() functions/macros.
> >
> > I can't remember whether or not I asked this before, but why not call
> > this macro p?d_leaf() if that's what it's identifying? "Large" and "huge"
> > are usually synonymous, so I find this naming needlessly confusing based
> > on this patch in isolation.
>
> You replied to my posting of this patch before[1], to which you said:
>
> > I've have thought p?d_leaf() might match better with your description
> > above, but I'm not going to quibble on naming.
That explains the sense of deja vu.
> Have you changed your mind about quibbling? ;)
Ha, I suppose I have! If it's not loads of effort to use p?d_leaf() instead
of p?d_large, then I'd certainly prefer that.
Will
^ permalink raw reply
* Re: [PATCH 1/3] arm64: mm: Add p?d_large() definitions
From: Will Deacon @ 2019-07-01 9:27 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Mark Rutland, Ard Biesheuvel, Catalin Marinas, Anshuman Khandual,
Will Deacon, Steven Price, linux-mm, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <20190623094446.28722-2-npiggin@gmail.com>
Hi Nick,
On Sun, Jun 23, 2019 at 07:44:44PM +1000, Nicholas Piggin wrote:
> walk_page_range() is going to be allowed to walk page tables other than
> those of user space. For this it needs to know when it has reached a
> 'leaf' entry in the page tables. This information will be provided by the
> p?d_large() functions/macros.
I can't remember whether or not I asked this before, but why not call
this macro p?d_leaf() if that's what it's identifying? "Large" and "huge"
are usually synonymous, so I find this naming needlessly confusing based
on this patch in isolation.
Will
^ permalink raw reply
* Re: [PATCH v3 10/11] mm/memory_hotplug: Make unregister_memory_block_under_nodes() never fail
From: Michal Hocko @ 2019-07-01 10:27 UTC (permalink / raw)
To: Oscar Salvador
Cc: linux-s390, Alex Deucher, linux-ia64, David Hildenbrand,
Greg Kroah-Hartman, Mark Brown, linux-sh, linux-kernel, Wei Yang,
linux-mm, David S. Miller, Jonathan Cameron, Rafael J. Wysocki,
Igor Mammedov, akpm, Chris Wilson, linuxppc-dev, Dan Williams,
linux-arm-kernel
In-Reply-To: <20190701093640.GA17349@linux>
On Mon 01-07-19 11:36:44, Oscar Salvador wrote:
> On Mon, Jul 01, 2019 at 10:51:44AM +0200, Michal Hocko wrote:
> > Yeah, we do not allow to offline multi zone (node) ranges so the current
> > code seems to be over engineered.
> >
> > Anyway, I am wondering why do we have to strictly check for already
> > removed nodes links. Is the sysfs code going to complain we we try to
> > remove again?
>
> No, sysfs will silently "fail" if the symlink has already been removed.
> At least that is what I saw last time I played with it.
>
> I guess the question is what if sysfs handling changes in the future
> and starts dropping warnings when trying to remove a symlink is not there.
> Maybe that is unlikely to happen?
And maybe we handle it then rather than have a static allocation that
everybody with hotremove configured has to pay for.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: Re: [PATCH 1/3] arm64: mm: Add p?d_large() definitions
From: Steven Price @ 2019-07-01 10:03 UTC (permalink / raw)
To: Will Deacon, Nicholas Piggin
Cc: Mark Rutland, Ard Biesheuvel, Catalin Marinas, Anshuman Khandual,
Will Deacon, linux-mm, Andrew Morton, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <20190701092756.s4u5rdjr7gazvu66@willie-the-truck>
On 01/07/2019 10:27, Will Deacon wrote:
> Hi Nick,
>
> On Sun, Jun 23, 2019 at 07:44:44PM +1000, Nicholas Piggin wrote:
>> walk_page_range() is going to be allowed to walk page tables other than
>> those of user space. For this it needs to know when it has reached a
>> 'leaf' entry in the page tables. This information will be provided by the
>> p?d_large() functions/macros.
>
> I can't remember whether or not I asked this before, but why not call
> this macro p?d_leaf() if that's what it's identifying? "Large" and "huge"
> are usually synonymous, so I find this naming needlessly confusing based
> on this patch in isolation.
Hi Will,
You replied to my posting of this patch before[1], to which you said:
> I've have thought p?d_leaf() might match better with your description
> above, but I'm not going to quibble on naming.
Have you changed your mind about quibbling? ;)
Steve
[1]
https://lore.kernel.org/lkml/20190611153650.GB4324@fuggles.cambridge.arm.com/
^ permalink raw reply
* Re: [PATCH v2 3/3] mm/vmalloc: fix vmalloc_to_page for huge vmap mappings
From: Anshuman Khandual @ 2019-07-01 9:58 UTC (permalink / raw)
To: Nicholas Piggin, linux-mm @ kvack . org
Cc: Mark Rutland, Ard Biesheuvel, Andrew Morton,
linuxppc-dev @ lists . ozlabs . org,
linux-arm-kernel @ lists . infradead . org
In-Reply-To: <20190701064026.970-4-npiggin@gmail.com>
On 07/01/2019 12:10 PM, Nicholas Piggin wrote:
> vmalloc_to_page returns NULL for addresses mapped by larger pages[*].
> Whether or not a vmap is huge depends on the architecture details,
> alignments, boot options, etc., which the caller can not be expected
> to know. Therefore HUGE_VMAP is a regression for vmalloc_to_page.
>
> This change teaches vmalloc_to_page about larger pages, and returns
> the struct page that corresponds to the offset within the large page.
> This makes the API agnostic to mapping implementation details.
>
> [*] As explained by commit 029c54b095995 ("mm/vmalloc.c: huge-vmap:
> fail gracefully on unexpected huge vmap mappings")
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
^ permalink raw reply
* Re: [PATCH v2 1/3] arm64: mm: Add p?d_large() definitions
From: Steven Price @ 2019-07-01 9:57 UTC (permalink / raw)
To: Nicholas Piggin, linux-mm @ kvack . org
Cc: Mark Rutland, Ard Biesheuvel, Catalin Marinas, Anshuman Khandual,
Will Deacon, Andrew Morton, linuxppc-dev @ lists . ozlabs . org,
linux-arm-kernel @ lists . infradead . org
In-Reply-To: <20190701064026.970-2-npiggin@gmail.com>
On 01/07/2019 07:40, Nicholas Piggin wrote:
> walk_page_range() is going to be allowed to walk page tables other than
> those of user space. For this it needs to know when it has reached a
> 'leaf' entry in the page tables. This information will be provided by the
> p?d_large() functions/macros.
>
> For arm64, we already have p?d_sect() macros which we can reuse for
> p?d_large().
>
> pud_sect() is defined as a dummy function when CONFIG_PGTABLE_LEVELS < 3
> or CONFIG_ARM64_64K_PAGES is defined. However when the kernel is
> configured this way then architecturally it isn't allowed to have a
> large page that this level, and any code using these page walking macros
> is implicitly relying on the page size/number of levels being the same as
> the kernel. So it is safe to reuse this for p?d_large() as it is an
> architectural restriction.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
Hi Nicolas,
This appears to my patch which I originally posted as part of converting
x86/arm64 to use a generic page walk code[1]. I'm not sure that this
patch makes much sense on its own, in particular it was working up to
having a generic macro[2] which means the _large() macros could be used
across all architectures.
Also as a matter of courtesy please can you ensure the authorship
information is preserved when posting other people's patches (there
should be a From: line with my name on). You should also include your
own Signed-off-by: line (see submitting-patches[3]) after mine.
Apologies to anyone that has been following my patch series, I've been
on holiday so not actively working on it. My aim is to combine the
series with a generic ptdump implementation[4] which should improve the
diff state. I should be able to post that in the next few weeks.
Thanks,
Steve
[1] Series: https://patchwork.kernel.org/cover/10883885/
Last posting of this patch:
https://patchwork.kernel.org/patch/10883899/
[2] https://patchwork.kernel.org/patch/10883965/
[3]
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1
[4] RFC version here:
https://lore.kernel.org/lkml/20190417143423.26665-1-steven.price@arm.com/
> ---
> arch/arm64/include/asm/pgtable.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index fca26759081a..0e973201bc16 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -417,6 +417,7 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
> PMD_TYPE_TABLE)
> #define pmd_sect(pmd) ((pmd_val(pmd) & PMD_TYPE_MASK) == \
> PMD_TYPE_SECT)
> +#define pmd_large(pmd) pmd_sect(pmd)
>
> #if defined(CONFIG_ARM64_64K_PAGES) || CONFIG_PGTABLE_LEVELS < 3
> #define pud_sect(pud) (0)
> @@ -499,6 +500,7 @@ static inline void pte_unmap(pte_t *pte) { }
> #define pud_none(pud) (!pud_val(pud))
> #define pud_bad(pud) (!(pud_val(pud) & PUD_TABLE_BIT))
> #define pud_present(pud) pte_present(pud_pte(pud))
> +#define pud_large(pud) pud_sect(pud)
> #define pud_valid(pud) pte_valid(pud_pte(pud))
>
> static inline void set_pud(pud_t *pudp, pud_t pud)
>
^ permalink raw reply
* Re: [PATCH v2 1/3] arm64: mm: Add p?d_large() definitions
From: Catalin Marinas @ 2019-07-01 9:44 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Mark Rutland, Anshuman Khandual, Ard Biesheuvel, Will Deacon,
Steven Price, linux-mm @ kvack . org, Andrew Morton,
linuxppc-dev @ lists . ozlabs . org,
linux-arm-kernel @ lists . infradead . org
In-Reply-To: <20190701064026.970-2-npiggin@gmail.com>
On Mon, Jul 01, 2019 at 04:40:24PM +1000, Nicholas Piggin wrote:
> walk_page_range() is going to be allowed to walk page tables other than
> those of user space. For this it needs to know when it has reached a
> 'leaf' entry in the page tables. This information will be provided by the
> p?d_large() functions/macros.
>
> For arm64, we already have p?d_sect() macros which we can reuse for
> p?d_large().
>
> pud_sect() is defined as a dummy function when CONFIG_PGTABLE_LEVELS < 3
> or CONFIG_ARM64_64K_PAGES is defined. However when the kernel is
> configured this way then architecturally it isn't allowed to have a
> large page that this level, and any code using these page walking macros
> is implicitly relying on the page size/number of levels being the same as
> the kernel. So it is safe to reuse this for p?d_large() as it is an
> architectural restriction.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Steven Price <steven.price@arm.com>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
^ permalink raw reply
* Re: [PATCH v3 10/11] mm/memory_hotplug: Make unregister_memory_block_under_nodes() never fail
From: Oscar Salvador @ 2019-07-01 9:36 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-s390, Alex Deucher, linux-ia64, David Hildenbrand,
Greg Kroah-Hartman, Mark Brown, linux-sh, linux-kernel, Wei Yang,
linux-mm, David S. Miller, Jonathan Cameron, Rafael J. Wysocki,
Igor Mammedov, akpm, Chris Wilson, linuxppc-dev, Dan Williams,
linux-arm-kernel
In-Reply-To: <20190701085144.GJ6376@dhcp22.suse.cz>
On Mon, Jul 01, 2019 at 10:51:44AM +0200, Michal Hocko wrote:
> Yeah, we do not allow to offline multi zone (node) ranges so the current
> code seems to be over engineered.
>
> Anyway, I am wondering why do we have to strictly check for already
> removed nodes links. Is the sysfs code going to complain we we try to
> remove again?
No, sysfs will silently "fail" if the symlink has already been removed.
At least that is what I saw last time I played with it.
I guess the question is what if sysfs handling changes in the future
and starts dropping warnings when trying to remove a symlink is not there.
Maybe that is unlikely to happen?
--
Oscar Salvador
SUSE L3
^ permalink raw reply
* Re: [PATCH v3 11/11] mm/memory_hotplug: Remove "zone" parameter from sparse_remove_one_section
From: Michal Hocko @ 2019-07-01 8:52 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-s390, linux-ia64, linux-sh, linux-kernel, Wei Yang,
linux-mm, Igor Mammedov, akpm, linuxppc-dev, Dan Williams,
linux-arm-kernel
In-Reply-To: <20190527111152.16324-12-david@redhat.com>
On Mon 27-05-19 13:11:52, David Hildenbrand wrote:
> The parameter is unused, so let's drop it. Memory removal paths should
> never care about zones. This is the job of memory offlining and will
> require more refactorings.
>
> Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> include/linux/memory_hotplug.h | 2 +-
> mm/memory_hotplug.c | 2 +-
> mm/sparse.c | 4 ++--
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 2f1f87e13baa..1a4257c5f74c 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -346,7 +346,7 @@ extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
> extern bool is_memblock_offlined(struct memory_block *mem);
> extern int sparse_add_one_section(int nid, unsigned long start_pfn,
> struct vmem_altmap *altmap);
> -extern void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
> +extern void sparse_remove_one_section(struct mem_section *ms,
> unsigned long map_offset, struct vmem_altmap *altmap);
> extern struct page *sparse_decode_mem_map(unsigned long coded_mem_map,
> unsigned long pnum);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 82136c5b4c5f..e48ec7b9dee2 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -524,7 +524,7 @@ static void __remove_section(struct zone *zone, struct mem_section *ms,
> start_pfn = section_nr_to_pfn((unsigned long)scn_nr);
> __remove_zone(zone, start_pfn);
>
> - sparse_remove_one_section(zone, ms, map_offset, altmap);
> + sparse_remove_one_section(ms, map_offset, altmap);
> }
>
> /**
> diff --git a/mm/sparse.c b/mm/sparse.c
> index d1d5e05f5b8d..1552c855d62a 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -800,8 +800,8 @@ static void free_section_usemap(struct page *memmap, unsigned long *usemap,
> free_map_bootmem(memmap);
> }
>
> -void sparse_remove_one_section(struct zone *zone, struct mem_section *ms,
> - unsigned long map_offset, struct vmem_altmap *altmap)
> +void sparse_remove_one_section(struct mem_section *ms, unsigned long map_offset,
> + struct vmem_altmap *altmap)
> {
> struct page *memmap = NULL;
> unsigned long *usemap = NULL;
> --
> 2.20.1
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] powerpc: remove device_to_mask
From: Michael Ellerman @ 2019-07-01 8:52 UTC (permalink / raw)
To: Alexey Kardashevskiy, Christoph Hellwig, paulus; +Cc: linuxppc-dev
In-Reply-To: <7ace6732-d1cd-3783-8ead-c9f262a78a3a@ozlabs.ru>
Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> On 29/06/2019 18:03, Christoph Hellwig wrote:
>> Use the dma_get_mask helper from dma-mapping.h instead.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
I'll add to the change log "because they are functionally identical."
cheers
^ permalink raw reply
* Re: [PATCH v3 10/11] mm/memory_hotplug: Make unregister_memory_block_under_nodes() never fail
From: Michal Hocko @ 2019-07-01 8:51 UTC (permalink / raw)
To: David Hildenbrand
Cc: linux-s390, linux-ia64, linux-sh, Greg Kroah-Hartman, Mark Brown,
Rafael J. Wysocki, linux-kernel, Wei Yang, linux-mm,
David S. Miller, Jonathan Cameron, Alex Deucher, Igor Mammedov,
akpm, Chris Wilson, linuxppc-dev, Dan Williams, linux-arm-kernel,
Oscar Salvador
In-Reply-To: <20190527111152.16324-11-david@redhat.com>
On Mon 27-05-19 13:11:51, David Hildenbrand wrote:
> We really don't want anything during memory hotunplug to fail.
> We always pass a valid memory block device, that check can go. Avoid
> allocating memory and eventually failing. As we are always called under
> lock, we can use a static piece of memory. This avoids having to put
> the structure onto the stack, having to guess about the stack size
> of callers.
>
> Patch inspired by a patch from Oscar Salvador.
>
> In the future, there might be no need to iterate over nodes at all.
> mem->nid should tell us exactly what to remove. Memory block devices
> with mixed nodes (added during boot) should properly fenced off and never
> removed.
Yeah, we do not allow to offline multi zone (node) ranges so the current
code seems to be over engineered.
Anyway, I am wondering why do we have to strictly check for already
removed nodes links. Is the sysfs code going to complain we we try to
remove again?
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
Anyway
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> drivers/base/node.c | 18 +++++-------------
> include/linux/node.h | 5 ++---
> 2 files changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/base/node.c b/drivers/base/node.c
> index 04fdfa99b8bc..9be88fd05147 100644
> --- a/drivers/base/node.c
> +++ b/drivers/base/node.c
> @@ -803,20 +803,14 @@ int register_mem_sect_under_node(struct memory_block *mem_blk, void *arg)
>
> /*
> * Unregister memory block device under all nodes that it spans.
> + * Has to be called with mem_sysfs_mutex held (due to unlinked_nodes).
> */
> -int unregister_memory_block_under_nodes(struct memory_block *mem_blk)
> +void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
> {
> - NODEMASK_ALLOC(nodemask_t, unlinked_nodes, GFP_KERNEL);
> unsigned long pfn, sect_start_pfn, sect_end_pfn;
> + static nodemask_t unlinked_nodes;
>
> - if (!mem_blk) {
> - NODEMASK_FREE(unlinked_nodes);
> - return -EFAULT;
> - }
> - if (!unlinked_nodes)
> - return -ENOMEM;
> - nodes_clear(*unlinked_nodes);
> -
> + nodes_clear(unlinked_nodes);
> sect_start_pfn = section_nr_to_pfn(mem_blk->start_section_nr);
> sect_end_pfn = section_nr_to_pfn(mem_blk->end_section_nr);
> for (pfn = sect_start_pfn; pfn <= sect_end_pfn; pfn++) {
> @@ -827,15 +821,13 @@ int unregister_memory_block_under_nodes(struct memory_block *mem_blk)
> continue;
> if (!node_online(nid))
> continue;
> - if (node_test_and_set(nid, *unlinked_nodes))
> + if (node_test_and_set(nid, unlinked_nodes))
> continue;
> sysfs_remove_link(&node_devices[nid]->dev.kobj,
> kobject_name(&mem_blk->dev.kobj));
> sysfs_remove_link(&mem_blk->dev.kobj,
> kobject_name(&node_devices[nid]->dev.kobj));
> }
> - NODEMASK_FREE(unlinked_nodes);
> - return 0;
> }
>
> int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn)
> diff --git a/include/linux/node.h b/include/linux/node.h
> index 02a29e71b175..548c226966a2 100644
> --- a/include/linux/node.h
> +++ b/include/linux/node.h
> @@ -139,7 +139,7 @@ extern int register_cpu_under_node(unsigned int cpu, unsigned int nid);
> extern int unregister_cpu_under_node(unsigned int cpu, unsigned int nid);
> extern int register_mem_sect_under_node(struct memory_block *mem_blk,
> void *arg);
> -extern int unregister_memory_block_under_nodes(struct memory_block *mem_blk);
> +extern void unregister_memory_block_under_nodes(struct memory_block *mem_blk);
>
> extern int register_memory_node_under_compute_node(unsigned int mem_nid,
> unsigned int cpu_nid,
> @@ -175,9 +175,8 @@ static inline int register_mem_sect_under_node(struct memory_block *mem_blk,
> {
> return 0;
> }
> -static inline int unregister_memory_block_under_nodes(struct memory_block *mem_blk)
> +static inline void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
> {
> - return 0;
> }
>
> static inline void register_hugetlbfs_with_node(node_registration_func_t reg,
> --
> 2.20.1
--
Michal Hocko
SUSE Labs
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox