* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
From: Vineet Gupta @ 2021-04-20 21:14 UTC (permalink / raw)
To: Arnd Bergmann, Matthew Wilcox
Cc: grygorii.strashko@ti.com, linux-snps-arc@lists.infradead.org,
netdev@vger.kernel.org, Vineet Gupta, ilias.apalodimas@linaro.org,
linux-kernel@vger.kernel.org, linux-mips@vger.kernel.org,
linux-mm@kvack.org, mgorman@suse.de, brouer@redhat.com,
mcroce@linux.microsoft.com, mhocko@kernel.org,
linuxppc-dev@lists.ozlabs.org, hch@lst.de,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAK8P3a0KUwf1Z0bHiUaHC2nHztevkxg5_FBSzHddNeSsBayWUA@mail.gmail.com>
On 4/20/21 12:07 AM, Arnd Bergmann wrote:
> On Tue, Apr 20, 2021 at 5:10 AM Matthew Wilcox <willy@infradead.org> wrote:
>> On Tue, Apr 20, 2021 at 02:48:17AM +0000, Vineet Gupta wrote:
>>>> 32-bit architectures which expect 8-byte alignment for 8-byte integers
>>>> and need 64-bit DMA addresses (arc, arm, mips, ppc) had their struct
>>>> page inadvertently expanded in 2019.
>>> FWIW, ARC doesn't require 8 byte alignment for 8 byte integers. This is
>>> only needed for 8-byte atomics due to the requirements of LLOCKD/SCOND
>>> instructions.
>> Ah, like x86? OK, great, I'll drop your arch from the list of
>> affected. Thanks!
> I mistakenly assumed that i386 and m68k were the only supported
> architectures with 32-bit alignment on u64. I checked it now and found
>
> $ for i in /home/arnd/cross/x86_64/gcc-10.1.0-nolibc/*/bin/*-gcc ; do
> echo `echo 'int a = __alignof__(long long);' | $i -xc - -Wall -S -o- |
> grep -A1 a: | tail -n 1 | cut -f 3 -d\ `
> ${i#/home/arnd/cross/x86_64/gcc-10.1.0-nolibc/*/bin/} ; done
> 8 aarch64-linux-gcc
> 8 alpha-linux-gcc
> 4 arc-linux-gcc
> 8 arm-linux-gnueabi-gcc
> 8 c6x-elf-gcc
> 4 csky-linux-gcc
> 4 h8300-linux-gcc
> 8 hppa-linux-gcc
> 8 hppa64-linux-gcc
> 8 i386-linux-gcc
> 8 ia64-linux-gcc
> 2 m68k-linux-gcc
> 4 microblaze-linux-gcc
> 8 mips-linux-gcc
> 8 mips64-linux-gcc
> 8 nds32le-linux-gcc
> 4 nios2-linux-gcc
> 4 or1k-linux-gcc
> 8 powerpc-linux-gcc
> 8 powerpc64-linux-gcc
> 8 riscv32-linux-gcc
> 8 riscv64-linux-gcc
> 8 s390-linux-gcc
> 4 sh2-linux-gcc
> 4 sh4-linux-gcc
> 8 sparc-linux-gcc
> 8 sparc64-linux-gcc
> 8 x86_64-linux-gcc
> 8 xtensa-linux-gcc
>
> which means that half the 32-bit architectures do this. This may
> cause more problems when arc and/or microblaze want to support
> 64-bit kernels and compat mode in the future on their latest hardware,
> as that means duplicating the x86 specific hacks we have for compat.
>
> What is alignof(u64) on 64-bit arc?
$ echo 'int a = __alignof__(long long);' | arc64-linux-gnu-gcc -xc -
-Wall -S -o - | grep -A1 a: | tail -n 1 | cut -f 3
8
Yeah ARCv2 alignment of 4 for 64-bit data was a bit of surprise finding
for me as well. When 64-bit load/stores were initially targeted by the
internal Metaware compiler (llvm based) they decided to keep alignment
to 4 still (granted hardware allowed this) and then gcc guys decided to
follow the same ABI. I only found this by accident :-)
Can you point me to some specifics on the compat issue. For better of
worse, arc64 does''t have a compat 32-bit mode, so everything is
64-on-64 or 32-on-32 (ARC32 flavor of ARCv3)
Thx,
-Vineet
^ permalink raw reply
* Re: [PATCH 1/2] mm: Fix struct page layout on 32-bit systems
From: Arnd Bergmann @ 2021-04-20 21:20 UTC (permalink / raw)
To: Vineet Gupta
Cc: grygorii.strashko@ti.com, linux-snps-arc@lists.infradead.org,
netdev@vger.kernel.org, ilias.apalodimas@linaro.org,
linux-kernel@vger.kernel.org, Matthew Wilcox,
linux-mips@vger.kernel.org, linux-mm@kvack.org, mgorman@suse.de,
brouer@redhat.com, mcroce@linux.microsoft.com, mhocko@kernel.org,
linuxppc-dev@lists.ozlabs.org, hch@lst.de,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <8d0fce1c-be7c-1c9b-bf5c-0c531db496ac@synopsys.com>
On Tue, Apr 20, 2021 at 11:14 PM Vineet Gupta
<Vineet.Gupta1@synopsys.com> wrote:
> On 4/20/21 12:07 AM, Arnd Bergmann wrote:
> >
> > which means that half the 32-bit architectures do this. This may
> > cause more problems when arc and/or microblaze want to support
> > 64-bit kernels and compat mode in the future on their latest hardware,
> > as that means duplicating the x86 specific hacks we have for compat.
> >
> > What is alignof(u64) on 64-bit arc?
>
> $ echo 'int a = __alignof__(long long);' | arc64-linux-gnu-gcc -xc -
> -Wall -S -o - | grep -A1 a: | tail -n 1 | cut -f 3
> 8
Ok, good.
> Yeah ARCv2 alignment of 4 for 64-bit data was a bit of surprise finding
> for me as well. When 64-bit load/stores were initially targeted by the
> internal Metaware compiler (llvm based) they decided to keep alignment
> to 4 still (granted hardware allowed this) and then gcc guys decided to
> follow the same ABI. I only found this by accident :-)
>
> Can you point me to some specifics on the compat issue. For better of
> worse, arc64 does''t have a compat 32-bit mode, so everything is
> 64-on-64 or 32-on-32 (ARC32 flavor of ARCv3)
In that case, there should be no problem for you.
The main issue is with system calls and ioctls that contain a misaligned
struct member like
struct s {
u32 a;
u64 b;
};
Passing this structure by reference from a 32-bit user space application
to a 64-bit kernel with different alignment constraints means that the
kernel has to convert the structure layout. See
compat_ioctl_preallocate() in fs/ioctl.c for one such example.
Arnd
^ permalink raw reply
* Re: [PATCH v2 2/2] powerpc/legacy_serial: Use early_ioremap()
From: Chris Packham @ 2021-04-20 21:15 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <103ed8ee9e5973c958ec1da2d0b0764f69395d01.1618925560.git.christophe.leroy@csgroup.eu>
Hi Christophe,
On 21/04/21 1:32 am, Christophe Leroy wrote:
> From: Christophe Leroy <christophe.leroy@c-s.fr>
>
> [ 0.000000] ioremap() called early from find_legacy_serial_ports+0x3cc/0x474. Use early_ioremap() instead
>
> find_legacy_serial_ports() is called early from setup_arch(), before
> paging_init(). vmalloc is not available yet, ioremap shouldn't be
> used that early.
>
> Use early_ioremap() and switch to a regular ioremap() later.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
I gave it a spin on my T2080RDB. The error message is gone and I still
get console output.
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> arch/powerpc/kernel/legacy_serial.c | 33 +++++++++++++++++++++++++----
> 1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
> index f061e06e9f51..8b2c1a8553a0 100644
> --- a/arch/powerpc/kernel/legacy_serial.c
> +++ b/arch/powerpc/kernel/legacy_serial.c
> @@ -15,6 +15,7 @@
> #include <asm/udbg.h>
> #include <asm/pci-bridge.h>
> #include <asm/ppc-pci.h>
> +#include <asm/early_ioremap.h>
>
> #undef DEBUG
>
> @@ -34,6 +35,7 @@ static struct legacy_serial_info {
> unsigned int clock;
> int irq_check_parent;
> phys_addr_t taddr;
> + void __iomem *early_addr;
> } legacy_serial_infos[MAX_LEGACY_SERIAL_PORTS];
>
> static const struct of_device_id legacy_serial_parents[] __initconst = {
> @@ -325,17 +327,16 @@ static void __init setup_legacy_serial_console(int console)
> {
> struct legacy_serial_info *info = &legacy_serial_infos[console];
> struct plat_serial8250_port *port = &legacy_serial_ports[console];
> - void __iomem *addr;
> unsigned int stride;
>
> stride = 1 << port->regshift;
>
> /* Check if a translated MMIO address has been found */
> if (info->taddr) {
> - addr = ioremap(info->taddr, 0x1000);
> - if (addr == NULL)
> + info->early_addr = early_ioremap(info->taddr, 0x1000);
> + if (info->early_addr == NULL)
> return;
> - udbg_uart_init_mmio(addr, stride);
> + udbg_uart_init_mmio(info->early_addr, stride);
> } else {
> /* Check if it's PIO and we support untranslated PIO */
> if (port->iotype == UPIO_PORT && isa_io_special)
> @@ -353,6 +354,30 @@ static void __init setup_legacy_serial_console(int console)
> udbg_uart_setup(info->speed, info->clock);
> }
>
> +static int __init ioremap_legacy_serial_console(void)
> +{
> + struct legacy_serial_info *info = &legacy_serial_infos[legacy_serial_console];
> + struct plat_serial8250_port *port = &legacy_serial_ports[legacy_serial_console];
> + void __iomem *vaddr;
> +
> + if (legacy_serial_console < 0)
> + return 0;
> +
> + if (!info->early_addr)
> + return 0;
> +
> + vaddr = ioremap(info->taddr, 0x1000);
> + if (WARN_ON(!vaddr))
> + return -ENOMEM;
> +
> + udbg_uart_init_mmio(vaddr, 1 << port->regshift);
> + early_iounmap(info->early_addr, 0x1000);
> + info->early_addr = NULL;
> +
> + return 0;
> +}
> +early_initcall(ioremap_legacy_serial_console);
> +
> /*
> * This is called very early, as part of setup_system() or eventually
> * setup_arch(), basically before anything else in this file. This function
^ permalink raw reply
* [PATCH 2/2] powerpc: If kexec_build_elf_info() fails return immediately from elf64_load()
From: Lakshmi Ramasubramanian @ 2021-04-20 19:03 UTC (permalink / raw)
To: robh, dan.carpenter, mpe
Cc: devicetree, kbuild-all, lkp, nramas, linuxppc-dev, bauerman, dja
In-Reply-To: <20210420190355.10059-1-nramas@linux.microsoft.com>
Uninitialized local variable "elf_info" would be passed to
kexec_free_elf_info() if kexec_build_elf_info() returns an error
in elf64_load().
If kexec_build_elf_info() returns an error, return the error
immediately.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---
arch/powerpc/kexec/elf_64.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 02662e72c53d..eeb258002d1e 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -45,7 +45,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
ret = kexec_build_elf_info(kernel_buf, kernel_len, &ehdr, &elf_info);
if (ret)
- goto out;
+ return ERR_PTR(ret);
if (image->type == KEXEC_TYPE_CRASH) {
/* min & max buffer values for kdump case */
--
2.31.0
^ permalink raw reply related
* [PATCH 1/2] powerpc: Free fdt on error in elf64_load()
From: Lakshmi Ramasubramanian @ 2021-04-20 19:03 UTC (permalink / raw)
To: robh, dan.carpenter, mpe
Cc: devicetree, kbuild-all, lkp, nramas, linuxppc-dev, bauerman, dja
There are a few "goto out;" statements before the local variable "fdt"
is initialized through the call to of_kexec_alloc_and_setup_fdt() in
elf64_load(). This will result in an uninitialized "fdt" being passed
to kvfree() in this function if there is an error before the call to
of_kexec_alloc_and_setup_fdt().
If there is any error after fdt is allocated, but before it is
saved in the arch specific kimage struct, free the fdt.
Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/kexec/elf_64.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 5a569bb51349..02662e72c53d 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
initrd_len, cmdline);
if (ret)
- goto out;
+ goto out_free_fdt;
fdt_pack(fdt);
@@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
ret = kexec_add_buffer(&kbuf);
if (ret)
- goto out;
+ goto out_free_fdt;
/* FDT will be freed in arch_kimage_file_post_load_cleanup */
image->arch.fdt = fdt;
@@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
if (ret)
pr_err("Error setting up the purgatory.\n");
+ goto out;
+
+out_free_fdt:
+ kvfree(fdt);
out:
kfree(modified_cmdline);
kexec_free_elf_info(&elf_info);
- /*
- * Once FDT buffer has been successfully passed to kexec_add_buffer(),
- * the FDT buffer address is saved in image->arch.fdt. In that case,
- * the memory cannot be freed here in case of any other error.
- */
- if (ret && !image->arch.fdt)
- kvfree(fdt);
-
return ret ? ERR_PTR(ret) : NULL;
}
--
2.31.0
^ permalink raw reply related
* Re: PPC_FPU, ALTIVEC: enable_kernel_fp, put_vr, get_vr
From: Randy Dunlap @ 2021-04-20 18:25 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, Segher Boessenkool; +Cc: PowerPC, LKML
In-Reply-To: <871rb5cd25.fsf@mpe.ellerman.id.au>
On 4/20/21 6:15 AM, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 19/04/2021 à 23:39, Randy Dunlap a écrit :
>>> On 4/19/21 6:16 AM, Michael Ellerman wrote:
>>>> Randy Dunlap <rdunlap@infradead.org> writes:
>>>
>>>>> Sure. I'll post them later today.
>>>>> They keep FPU and ALTIVEC as independent (build) features.
>>>>
>>>> Those patches look OK.
>>>>
>>>> But I don't think it makes sense to support that configuration, FPU=n
>>>> ALTVEC=y. No one is ever going to make a CPU like that. We have enough
>>>> testing surface due to configuration options, without adding artificial
>>>> combinations that no one is ever going to use.
>>>>
>>>> IMHO :)
>>>>
>>>> So I'd rather we just make ALTIVEC depend on FPU.
>>>
>>> That's rather simple. See below.
>>> I'm doing a bunch of randconfig builds with it now.
>>>
>>> ---
>>> From: Randy Dunlap <rdunlap@infradead.org>
>>> Subject: [PATCH] powerpc: make ALTIVEC depend PPC_FPU
>>>
>>> On a kernel config with ALTIVEC=y and PPC_FPU not set/enabled,
>>> there are build errors:
>>>
>>> drivers/cpufreq/pmac32-cpufreq.c:262:2: error: implicit declaration of function 'enable_kernel_fp' [-Werror,-Wimplicit-function-declaration]
>>> enable_kernel_fp();
>>> ../arch/powerpc/lib/sstep.c: In function 'do_vec_load':
>>> ../arch/powerpc/lib/sstep.c:637:3: error: implicit declaration of function 'put_vr' [-Werror=implicit-function-declaration]
>>> 637 | put_vr(rn, &u.v);
>>> | ^~~~~~
>>> ../arch/powerpc/lib/sstep.c: In function 'do_vec_store':
>>> ../arch/powerpc/lib/sstep.c:660:3: error: implicit declaration of function 'get_vr'; did you mean 'get_oc'? [-Werror=implicit-function-declaration]
>>> 660 | get_vr(rn, &u.v);
>>> | ^~~~~~
>>>
>>> In theory ALTIVEC is independent of PPC_FPU but in practice nobody
>>> is going to build such a machine, so make ALTIVEC require PPC_FPU
>>> by depending on PPC_FPU.
>>>
>>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>> Cc: linuxppc-dev@lists.ozlabs.org
>>> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> Cc: Segher Boessenkool <segher@kernel.crashing.org>
>>> Cc: lkp@intel.com
>>> ---
>>> arch/powerpc/platforms/86xx/Kconfig | 1 +
>>> arch/powerpc/platforms/Kconfig.cputype | 2 ++
>>> 2 files changed, 3 insertions(+)
>>>
>>> --- linux-next-20210416.orig/arch/powerpc/platforms/86xx/Kconfig
>>> +++ linux-next-20210416/arch/powerpc/platforms/86xx/Kconfig
>>> @@ -4,6 +4,7 @@ menuconfig PPC_86xx
>>> bool "86xx-based boards"
>>> depends on PPC_BOOK3S_32
>>> select FSL_SOC
>>> + select PPC_FPU
>>> select ALTIVEC
>>> help
>>> The Freescale E600 SoCs have 74xx cores.
>>> --- linux-next-20210416.orig/arch/powerpc/platforms/Kconfig.cputype
>>> +++ linux-next-20210416/arch/powerpc/platforms/Kconfig.cputype
>>> @@ -186,6 +186,7 @@ config E300C3_CPU
>>> config G4_CPU
>>> bool "G4 (74xx)"
>>> depends on PPC_BOOK3S_32
>>> + select PPC_FPU
>>> select ALTIVEC
>>>
>>> endchoice
>>> @@ -309,6 +310,7 @@ config PHYS_64BIT
>>>
>>> config ALTIVEC
>>> bool "AltiVec Support"
>>> + depends on PPC_FPU
>>
>> Shouldn't we do it the other way round ? In extenso make ALTIVEC select PPC_FPU and avoid the two
>> selects that are above ?
>
> Yes, ALTIVEC should select PPC_FPU.
>
> The latter is (generally) not user selectable, so there's no issue with
> selecting it, whereas the reverse is not true.
>
> For 64-bit Book3S I think we could just always enable ALTIVEC these
> days. It's only Power5 that doesn't have it, and essentially no one is
> running mainline on those AFAIK. But that can be done separately.
OK, I'll run that thru some tests today.
thanks.
--
~Randy
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/sstep: Add tests for setb instruction
From: Naveen N. Rao @ 2021-04-20 17:37 UTC (permalink / raw)
To: linuxppc-dev, Sathvika Vasireddy
In-Reply-To: <ab1756406a5c39c32b809a0699a22c10425030ae.1618469454.git.sathvika@linux.vnet.ibm.com>
Sathvika Vasireddy wrote:
> This adds selftests for setb instruction.
>
> Signed-off-by: Sathvika Vasireddy <sathvika@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/ppc-opcode.h | 1 +
> arch/powerpc/lib/test_emulate_step.c | 28 +++++++++++++++++++++++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> index ed161ef2b3ca..32bf53260737 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -245,6 +245,7 @@
> #define PPC_INST_STRING 0x7c00042a
> #define PPC_INST_STRING_MASK 0xfc0007fe
> #define PPC_INST_STRING_GEN_MASK 0xfc00067e
> +#define PPC_INST_SETB 0x7c000100
> #define PPC_INST_STSWI 0x7c0005aa
> #define PPC_INST_STSWX 0x7c00052a
> #define PPC_INST_TRECHKPT 0x7c0007dd
> diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
> index 783d1b85ecfe..c338e35b627c 100644
> --- a/arch/powerpc/lib/test_emulate_step.c
> +++ b/arch/powerpc/lib/test_emulate_step.c
> @@ -53,6 +53,8 @@
> ppc_inst_prefix(PPC_PREFIX_MLS | __PPC_PRFX_R(pr) | IMM_H(i), \
> PPC_RAW_ADDI(t, a, i))
>
> +#define TEST_SETB(t, bfa) ppc_inst(PPC_INST_SETB | ___PPC_RT(t) | ___PPC_RA((bfa & 0x7) << 2))
> +
>
> static void __init init_pt_regs(struct pt_regs *regs)
> {
> @@ -929,6 +931,67 @@ static struct compute_test compute_tests[] = {
> }
> }
> },
> + {
> + .mnemonic = "setb",
> + .subtests = {
Since this is ISA v3.0, you also need to restrict these tests. You can
do that by setting cpu_feature.
- Naveen
^ permalink raw reply
* Re: [PATCH v4] pseries: prevent free CPU ids to be reused on another node
From: Laurent Dufour @ 2021-04-20 16:34 UTC (permalink / raw)
To: mpe, benh, paulus, nathanl, Nick Piggin
Cc: cheloha, linuxppc-dev, linux-kernel
In-Reply-To: <20210407153808.59993-1-ldufour@linux.ibm.com>
Le 07/04/2021 à 17:38, Laurent Dufour a écrit :
> When a CPU is hot added, the CPU ids are taken from the available mask from
> the lower possible set. If that set of values was previously used for CPU
> attached to a different node, this seems to application like if these CPUs
> have migrated from a node to another one which is not expected in real
> life.
>
> To prevent this, it is needed to record the CPU ids used for each node and
> to not reuse them on another node. However, to prevent CPU hot plug to
> fail, in the case the CPU ids is starved on a node, the capability to reuse
> other nodes’ free CPU ids is kept. A warning is displayed in such a case
> to warn the user.
>
> A new CPU bit mask (node_recorded_ids_map) is introduced for each possible
> node. It is populated with the CPU onlined at boot time, and then when a
> CPU is hot plug to a node. The bits in that mask remain when the CPU is hot
> unplugged, to remind this CPU ids have been used for this node.
>
> The effect of this patch can be seen by removing and adding CPUs using the
> Qemu monitor. In the following case, the first CPU from the node 2 is
> removed, then the first one from the node 1 is removed too. Later, the
> first CPU of the node 2 is added back. Without that patch, the kernel will
> numbered these CPUs using the first CPU ids available which are the ones
> freed when removing the second CPU of the node 0. This leads to the CPU ids
> 16-23 to move from the node 1 to the node 2. With the patch applied, the
> CPU ids 32-39 are used since they are the lowest free ones which have not
> been used on another node.
>
> At boot time:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
>
> Vanilla kernel, after the CPU hot unplug/plug operations:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 24 25 26 27 28 29 30 31
> node 2 cpus: 16 17 18 19 20 21 22 23 40 41 42 43 44 45 46 47
>
> Patched kernel, after the CPU hot unplug/plug operations:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 24 25 26 27 28 29 30 31
> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
>
> Changes since V3, addressing Nathan's comment:
> - Rename the local variable named 'nid' into 'assigned_node'
> Changes since V2, addressing Nathan's comments:
> - Remove the retry feature
> - Reduce the number of local variables (removing 'i')
> - Add comment about the cpu_add_remove_lock protecting the added CPU mask.
> Changes since V1 (no functional changes):
> - update the test's output in the commit's description
> - node_recorded_ids_map should be static
>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
I did further LPM tests with this patch applied and not allowing fall back
reusing free ids of another node is too strong.
This is easy to hit that limitation when a LPAR is running at the maximum number
of CPU it is configured for and when a LPAR migration leads to new node activation.
For instance, consider a dedicated LPAR configured with a max of 32 CPUs (4
cores SMT 8). At boot time, cpu_possible_mask is filled with CPU ids 0-31 in
smp_setup_cpu_maps() by reading the DT property "/rtas/ibm,lrdr-capacity", so
the higher CPU id for this LPAR is 31.
Departure box:
node 0 : CPU 0-31
Arrival box:
node 0 : CPU 0-15
node 1 : CPU 16-31 << need to reuse ids from node 0
Visualizing the CPU ids would have a big impact as it is used in several places
in the kernel as to index linear table.
But in the case the LPAR is migratable (DT property "ibm,migratable-partition"
is present), we may set the higher CPU ids to NR_CPUS (usually 2048), to limit
the case where CPU id has to be reused on a different node. Doing this will have
impact on some data allocation done in the kernel when the size is based on
num_possible_cpus.
Any better idea?
Thanks,
Laurent.
^ permalink raw reply
* Re: [PATCH v4 19/20] mips: Convert to GENERIC_CMDLINE
From: Christophe Leroy @ 2021-04-20 16:05 UTC (permalink / raw)
To: Daniel Walker, Rob Herring
Cc: linux-arch, arnd, microblaze, daniel, devicetree, linux-sh,
linuxppc-dev, linux-xtensa, x86, linux-kernel, nios2, linux-mips,
linux-mm, openrisc, linux-hexagon, sparclinux, akpm, will,
linux-riscv, linux-arm-kernel
In-Reply-To: <20210409012349.GG3981976@zorba>
Le 09/04/2021 à 03:23, Daniel Walker a écrit :
> On Thu, Apr 08, 2021 at 02:04:08PM -0500, Rob Herring wrote:
>> On Tue, Apr 06, 2021 at 10:38:36AM -0700, Daniel Walker wrote:
>>> On Fri, Apr 02, 2021 at 03:18:21PM +0000, Christophe Leroy wrote:
>>>> -config CMDLINE_BOOL
>>>> - bool "Built-in kernel command line"
>>>> - help
>>>> - For most systems, it is firmware or second stage bootloader that
>>>> - by default specifies the kernel command line options. However,
>>>> - it might be necessary or advantageous to either override the
>>>> - default kernel command line or add a few extra options to it.
>>>> - For such cases, this option allows you to hardcode your own
>>>> - command line options directly into the kernel. For that, you
>>>> - should choose 'Y' here, and fill in the extra boot arguments
>>>> - in CONFIG_CMDLINE.
>>>> -
>>>> - The built-in options will be concatenated to the default command
>>>> - line if CMDLINE_OVERRIDE is set to 'N'. Otherwise, the default
>>>> - command line will be ignored and replaced by the built-in string.
>>>> -
>>>> - Most MIPS systems will normally expect 'N' here and rely upon
>>>> - the command line from the firmware or the second-stage bootloader.
>>>> -
>>>
>>>
>>> See how you complained that I have CMDLINE_BOOL in my changed, and you think it
>>> shouldn't exist.
>>>
>>> Yet here mips has it, and you just deleted it with no feature parity in your
>>> changes for this.
>>
>> AFAICT, CMDLINE_BOOL equates to a non-empty or empty CONFIG_CMDLINE. You
>> seem to need it just because you have CMDLINE_PREPEND and
>> CMDLINE_APPEND. If that's not it, what feature is missing? CMDLINE_BOOL
>> is not a feature, but an implementation detail.
>
> Not true.
>
> It makes it easier to turn it all off inside the Kconfig , so it's for usability
> and multiple architecture have it even with just CMDLINE as I was commenting
> here.
>
Among the 13 architectures having CONFIG_CMDLINE, todayb only 6 have a CONFIG_CMDLINE_BOOL in addition:
arch/arm/Kconfig:config CMDLINE
arch/arm64/Kconfig:config CMDLINE
arch/hexagon/Kconfig:config CMDLINE
arch/microblaze/Kconfig:config CMDLINE
arch/mips/Kconfig.debug:config CMDLINE
arch/nios2/Kconfig:config CMDLINE
arch/openrisc/Kconfig:config CMDLINE
arch/powerpc/Kconfig:config CMDLINE
arch/riscv/Kconfig:config CMDLINE
arch/sh/Kconfig:config CMDLINE
arch/sparc/Kconfig:config CMDLINE
arch/x86/Kconfig:config CMDLINE
arch/xtensa/Kconfig:config CMDLINE
arch/microblaze/Kconfig:config CMDLINE_BOOL
arch/mips/Kconfig.debug:config CMDLINE_BOOL
arch/nios2/Kconfig:config CMDLINE_BOOL
arch/sparc/Kconfig:config CMDLINE_BOOL
arch/x86/Kconfig:config CMDLINE_BOOL
arch/xtensa/Kconfig:config CMDLINE_BOOL
In the begining I hesitated about the CMDLINE_BOOL, at the end I decided to go the same way as what
is done today in the kernel for initramfs with CONFIG_INITRAMFS_SOURCE.
The problem I see within adding CONFIG_CMDLINE_BOOL for every architecture which don't have it today
is that when doing a "make oldconfig" on their custom configs, thousands of users will loose their
CMDLINE without notice.
When we do the other way round, removing CONFIG_CMDLINE_BOOL on the 6 architectures that have it
today will have no impact on existing config.
Also, in order to avoid tons of #ifdefs in the code as mandated by Kernel Codying Style §21, we have
to have CONFIG_CMDLINE defined at all time, so at the end CONFIG_CMDLINE_BOOL is really redundant
with an empty CONFIG_CMDLINE.
Unlike you, the approach I took for my series is to minimise the impact on existing implementation
and existing configurations as much as possible.
I know you have a different approach where you break every existing config anyway.
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation
Christophe
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Lakshmi Ramasubramanian @ 2021-04-20 15:55 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, kbuild-all, kbuild test robot, Thiago Jung Bauermann,
linuxppc-dev, Dan Carpenter, Daniel Axtens
In-Reply-To: <CAL_JsqJ5QWiQKiw+hYC6554U+tzKboMM4qc_vpfSz0QTm7+_Lw@mail.gmail.com>
On 4/20/21 8:47 AM, Rob Herring wrote:
> On Tue, Apr 20, 2021 at 10:04 AM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
>>
>> On 4/20/21 7:42 AM, Lakshmi Ramasubramanian wrote:
>>> On 4/20/21 6:06 AM, Rob Herring wrote:
>>>> On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian
>>>> <nramas@linux.microsoft.com> wrote:
>>>>>
>>>>> On 4/19/21 10:00 PM, Dan Carpenter wrote:
>>>>>> On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
>>>>>>> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
>>>>>>>> On 4/16/21 2:05 AM, Michael Ellerman wrote:
>>>>>>>>
>>>>>>>>> Daniel Axtens <dja@axtens.net> writes:
>>>>>>>>>>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
>>>>>>>>>>>
>>>>>>>>>>> Sorry - missed copying device-tree and powerpc mailing lists.
>>>>>>>>>>>
>>>>>>>>>>>> There are a few "goto out;" statements before the local
>>>>>>>>>>>> variable "fdt"
>>>>>>>>>>>> is initialized through the call to
>>>>>>>>>>>> of_kexec_alloc_and_setup_fdt() in
>>>>>>>>>>>> elf64_load(). This will result in an uninitialized "fdt" being
>>>>>>>>>>>> passed
>>>>>>>>>>>> to kvfree() in this function if there is an error before the
>>>>>>>>>>>> call to
>>>>>>>>>>>> of_kexec_alloc_and_setup_fdt().
>>>>>>>>>>>>
>>>>>>>>>>>> Initialize the local variable "fdt" to NULL.
>>>>>>>>>>>>
>>>>>>>>>> I'm a huge fan of initialising local variables! But I'm
>>>>>>>>>> struggling to
>>>>>>>>>> find the code path that will lead to an uninit fdt being
>>>>>>>>>> returned...
>>>>>>>>>>
>>>>>>>>>> The out label reads in part:
>>>>>>>>>>
>>>>>>>>>> /* Make kimage_file_post_load_cleanup free the fdt buffer for
>>>>>>>>>> us. */
>>>>>>>>>> return ret ? ERR_PTR(ret) : fdt;
>>>>>>>>>>
>>>>>>>>>> As far as I can tell, any time we get a non-zero ret, we're
>>>>>>>>>> going to
>>>>>>>>>> return an error pointer rather than the uninitialised value...
>>>>>>>>
>>>>>>>> As Dan pointed out, the new code is in linux-next.
>>>>>>>>
>>>>>>>> I have copied the new one below - the function doesn't return fdt,
>>>>>>>> but
>>>>>>>> instead sets it in the arch specific field (please see the link to
>>>>>>>> the
>>>>>>>> updated elf_64.c below).
>>>>>>>>
>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
>>>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> (btw, it does look like we might leak fdt if we have an error
>>>>>>>>>> after we
>>>>>>>>>> successfully kmalloc it.)
>>>>>>>>>>
>>>>>>>>>> Am I missing something? Can you link to the report for the
>>>>>>>>>> kernel test
>>>>>>>>>> robot or from Dan?
>>>>>>>>
>>>>>>>> /*
>>>>>>>> * Once FDT buffer has been successfully passed to
>>>>>>>> kexec_add_buffer(),
>>>>>>>> * the FDT buffer address is saved in image->arch.fdt.
>>>>>>>> In that
>>>>>>>> case,
>>>>>>>> * the memory cannot be freed here in case of any other
>>>>>>>> error.
>>>>>>>> */
>>>>>>>> if (ret && !image->arch.fdt)
>>>>>>>> kvfree(fdt);
>>>>>>>>
>>>>>>>> return ret ? ERR_PTR(ret) : NULL;
>>>>>>>>
>>>>>>>> In case of an error, the memory allocated for fdt is freed unless
>>>>>>>> it has
>>>>>>>> already been passed to kexec_add_buffer().
>>>>>>>
>>>>>>> It feels like the root of the problem is that the kvfree of fdt is in
>>>>>>> the wrong place. It's only allocated later in the function, so the
>>>>>>> error
>>>>>>> path should reflect that. Something like the patch below.
>>>>>>>
>>>>>>> cheers
>>>>>>>
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>>>>>>> index 5a569bb51349..02662e72c53d 100644
>>>>>>> --- a/arch/powerpc/kexec/elf_64.c
>>>>>>> +++ b/arch/powerpc/kexec/elf_64.c
>>>>>>> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image,
>>>>>>> char *kernel_buf,
>>>>>>> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
>>>>>>> initrd_len, cmdline);
>>>>>>> if (ret)
>>>>>>> - goto out;
>>>>>>> + goto out_free_fdt;
>>>>>>>
>>>>>>> fdt_pack(fdt);
>>>>>>>
>>>>>>> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image,
>>>>>>> char *kernel_buf,
>>>>>>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>>>>>>> ret = kexec_add_buffer(&kbuf);
>>>>>>> if (ret)
>>>>>>> - goto out;
>>>>>>> + goto out_free_fdt;
>>>>>>>
>>>>>>> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
>>>>>>> image->arch.fdt = fdt;
>>>>>>> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image,
>>>>>>> char *kernel_buf,
>>>>>>> if (ret)
>>>>>>> pr_err("Error setting up the purgatory.\n");
>>>>>>>
>>>>>>> + goto out;
>>>>>>
>>>>>> This will leak. It would need to be something like:
>>>>>>
>>>>>> if (ret) {
>>>>>> pr_err("Error setting up the purgatory.\n");
>>>>>> goto out_free_fdt;
>>>>>> }
>>>>> Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
>>>>> be freed here - it will be freed when the kexec cleanup function is
>>>>> called.
>>>>
>>>> That may be the case currently, but really if a function returns an
>>>> error it should have undone anything it did like memory allocations. I
>>>> don't think you should do that to fix this issue, but it would be a
>>>> good clean-up.
>>>>
>>>
>>> I agree - in case of an error the function should do a proper clean-up.
>>> Just to be clear - for now, I will leave this as is. Correct?
>
> Yes.
okay.
>
>>> In my patch, I will do the following changes:
>>>
>>> => Free "fdt" when possible (as Michael had suggested in his patch)
>>> => Zero out "elf_info" struct at the start of the function.
>>>
>>
>> Instead of zeroing out "elf_info", I think it would be better to return
>> an error immediately, instead of the "goto out;", if
>> kexec_build_elf_info() fails.
>>
>> ret = kexec_build_elf_info(kernel_buf, kernel_len, &ehdr, &elf_info);
>> if (ret)
>> return ERR_PTR(ret);
>
> I thought kexec_build_elf_info() can return an error and allocated
> memory, so that would leak memory.
>
I looked at kexec_build_elf_info() more - it does free elf_info, if it
allocated it but encountered an error after the allocation. So it does a
proper clean-up in case of an error.
-lakshmi
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Rob Herring @ 2021-04-20 15:47 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: devicetree, kbuild-all, kbuild test robot, Thiago Jung Bauermann,
linuxppc-dev, Dan Carpenter, Daniel Axtens
In-Reply-To: <433b4518-0a83-5a97-ac07-da8748dcc90e@linux.microsoft.com>
On Tue, Apr 20, 2021 at 10:04 AM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> On 4/20/21 7:42 AM, Lakshmi Ramasubramanian wrote:
> > On 4/20/21 6:06 AM, Rob Herring wrote:
> >> On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian
> >> <nramas@linux.microsoft.com> wrote:
> >>>
> >>> On 4/19/21 10:00 PM, Dan Carpenter wrote:
> >>>> On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
> >>>>> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> >>>>>> On 4/16/21 2:05 AM, Michael Ellerman wrote:
> >>>>>>
> >>>>>>> Daniel Axtens <dja@axtens.net> writes:
> >>>>>>>>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> >>>>>>>>>
> >>>>>>>>> Sorry - missed copying device-tree and powerpc mailing lists.
> >>>>>>>>>
> >>>>>>>>>> There are a few "goto out;" statements before the local
> >>>>>>>>>> variable "fdt"
> >>>>>>>>>> is initialized through the call to
> >>>>>>>>>> of_kexec_alloc_and_setup_fdt() in
> >>>>>>>>>> elf64_load(). This will result in an uninitialized "fdt" being
> >>>>>>>>>> passed
> >>>>>>>>>> to kvfree() in this function if there is an error before the
> >>>>>>>>>> call to
> >>>>>>>>>> of_kexec_alloc_and_setup_fdt().
> >>>>>>>>>>
> >>>>>>>>>> Initialize the local variable "fdt" to NULL.
> >>>>>>>>>>
> >>>>>>>> I'm a huge fan of initialising local variables! But I'm
> >>>>>>>> struggling to
> >>>>>>>> find the code path that will lead to an uninit fdt being
> >>>>>>>> returned...
> >>>>>>>>
> >>>>>>>> The out label reads in part:
> >>>>>>>>
> >>>>>>>> /* Make kimage_file_post_load_cleanup free the fdt buffer for
> >>>>>>>> us. */
> >>>>>>>> return ret ? ERR_PTR(ret) : fdt;
> >>>>>>>>
> >>>>>>>> As far as I can tell, any time we get a non-zero ret, we're
> >>>>>>>> going to
> >>>>>>>> return an error pointer rather than the uninitialised value...
> >>>>>>
> >>>>>> As Dan pointed out, the new code is in linux-next.
> >>>>>>
> >>>>>> I have copied the new one below - the function doesn't return fdt,
> >>>>>> but
> >>>>>> instead sets it in the arch specific field (please see the link to
> >>>>>> the
> >>>>>> updated elf_64.c below).
> >>>>>>
> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
> >>>>>>
> >>>>>>
> >>>>>>>>
> >>>>>>>> (btw, it does look like we might leak fdt if we have an error
> >>>>>>>> after we
> >>>>>>>> successfully kmalloc it.)
> >>>>>>>>
> >>>>>>>> Am I missing something? Can you link to the report for the
> >>>>>>>> kernel test
> >>>>>>>> robot or from Dan?
> >>>>>>
> >>>>>> /*
> >>>>>> * Once FDT buffer has been successfully passed to
> >>>>>> kexec_add_buffer(),
> >>>>>> * the FDT buffer address is saved in image->arch.fdt.
> >>>>>> In that
> >>>>>> case,
> >>>>>> * the memory cannot be freed here in case of any other
> >>>>>> error.
> >>>>>> */
> >>>>>> if (ret && !image->arch.fdt)
> >>>>>> kvfree(fdt);
> >>>>>>
> >>>>>> return ret ? ERR_PTR(ret) : NULL;
> >>>>>>
> >>>>>> In case of an error, the memory allocated for fdt is freed unless
> >>>>>> it has
> >>>>>> already been passed to kexec_add_buffer().
> >>>>>
> >>>>> It feels like the root of the problem is that the kvfree of fdt is in
> >>>>> the wrong place. It's only allocated later in the function, so the
> >>>>> error
> >>>>> path should reflect that. Something like the patch below.
> >>>>>
> >>>>> cheers
> >>>>>
> >>>>>
> >>>>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> >>>>> index 5a569bb51349..02662e72c53d 100644
> >>>>> --- a/arch/powerpc/kexec/elf_64.c
> >>>>> +++ b/arch/powerpc/kexec/elf_64.c
> >>>>> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image,
> >>>>> char *kernel_buf,
> >>>>> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
> >>>>> initrd_len, cmdline);
> >>>>> if (ret)
> >>>>> - goto out;
> >>>>> + goto out_free_fdt;
> >>>>>
> >>>>> fdt_pack(fdt);
> >>>>>
> >>>>> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image,
> >>>>> char *kernel_buf,
> >>>>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> >>>>> ret = kexec_add_buffer(&kbuf);
> >>>>> if (ret)
> >>>>> - goto out;
> >>>>> + goto out_free_fdt;
> >>>>>
> >>>>> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
> >>>>> image->arch.fdt = fdt;
> >>>>> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image,
> >>>>> char *kernel_buf,
> >>>>> if (ret)
> >>>>> pr_err("Error setting up the purgatory.\n");
> >>>>>
> >>>>> + goto out;
> >>>>
> >>>> This will leak. It would need to be something like:
> >>>>
> >>>> if (ret) {
> >>>> pr_err("Error setting up the purgatory.\n");
> >>>> goto out_free_fdt;
> >>>> }
> >>> Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
> >>> be freed here - it will be freed when the kexec cleanup function is
> >>> called.
> >>
> >> That may be the case currently, but really if a function returns an
> >> error it should have undone anything it did like memory allocations. I
> >> don't think you should do that to fix this issue, but it would be a
> >> good clean-up.
> >>
> >
> > I agree - in case of an error the function should do a proper clean-up.
> > Just to be clear - for now, I will leave this as is. Correct?
Yes.
> > In my patch, I will do the following changes:
> >
> > => Free "fdt" when possible (as Michael had suggested in his patch)
> > => Zero out "elf_info" struct at the start of the function.
> >
>
> Instead of zeroing out "elf_info", I think it would be better to return
> an error immediately, instead of the "goto out;", if
> kexec_build_elf_info() fails.
>
> ret = kexec_build_elf_info(kernel_buf, kernel_len, &ehdr, &elf_info);
> if (ret)
> return ERR_PTR(ret);
I thought kexec_build_elf_info() can return an error and allocated
memory, so that would leak memory.
Rob
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Lakshmi Ramasubramanian @ 2021-04-20 15:04 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, kbuild-all, kbuild test robot, Thiago Jung Bauermann,
linuxppc-dev, Dan Carpenter, Daniel Axtens
In-Reply-To: <2e8dd39b-0372-4874-340e-6f87185091cc@linux.microsoft.com>
On 4/20/21 7:42 AM, Lakshmi Ramasubramanian wrote:
> On 4/20/21 6:06 AM, Rob Herring wrote:
>> On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian
>> <nramas@linux.microsoft.com> wrote:
>>>
>>> On 4/19/21 10:00 PM, Dan Carpenter wrote:
>>>> On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
>>>>> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
>>>>>> On 4/16/21 2:05 AM, Michael Ellerman wrote:
>>>>>>
>>>>>>> Daniel Axtens <dja@axtens.net> writes:
>>>>>>>>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
>>>>>>>>>
>>>>>>>>> Sorry - missed copying device-tree and powerpc mailing lists.
>>>>>>>>>
>>>>>>>>>> There are a few "goto out;" statements before the local
>>>>>>>>>> variable "fdt"
>>>>>>>>>> is initialized through the call to
>>>>>>>>>> of_kexec_alloc_and_setup_fdt() in
>>>>>>>>>> elf64_load(). This will result in an uninitialized "fdt" being
>>>>>>>>>> passed
>>>>>>>>>> to kvfree() in this function if there is an error before the
>>>>>>>>>> call to
>>>>>>>>>> of_kexec_alloc_and_setup_fdt().
>>>>>>>>>>
>>>>>>>>>> Initialize the local variable "fdt" to NULL.
>>>>>>>>>>
>>>>>>>> I'm a huge fan of initialising local variables! But I'm
>>>>>>>> struggling to
>>>>>>>> find the code path that will lead to an uninit fdt being
>>>>>>>> returned...
>>>>>>>>
>>>>>>>> The out label reads in part:
>>>>>>>>
>>>>>>>> /* Make kimage_file_post_load_cleanup free the fdt buffer for
>>>>>>>> us. */
>>>>>>>> return ret ? ERR_PTR(ret) : fdt;
>>>>>>>>
>>>>>>>> As far as I can tell, any time we get a non-zero ret, we're
>>>>>>>> going to
>>>>>>>> return an error pointer rather than the uninitialised value...
>>>>>>
>>>>>> As Dan pointed out, the new code is in linux-next.
>>>>>>
>>>>>> I have copied the new one below - the function doesn't return fdt,
>>>>>> but
>>>>>> instead sets it in the arch specific field (please see the link to
>>>>>> the
>>>>>> updated elf_64.c below).
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
>>>>>>
>>>>>>
>>>>>>>>
>>>>>>>> (btw, it does look like we might leak fdt if we have an error
>>>>>>>> after we
>>>>>>>> successfully kmalloc it.)
>>>>>>>>
>>>>>>>> Am I missing something? Can you link to the report for the
>>>>>>>> kernel test
>>>>>>>> robot or from Dan?
>>>>>>
>>>>>> /*
>>>>>> * Once FDT buffer has been successfully passed to
>>>>>> kexec_add_buffer(),
>>>>>> * the FDT buffer address is saved in image->arch.fdt.
>>>>>> In that
>>>>>> case,
>>>>>> * the memory cannot be freed here in case of any other
>>>>>> error.
>>>>>> */
>>>>>> if (ret && !image->arch.fdt)
>>>>>> kvfree(fdt);
>>>>>>
>>>>>> return ret ? ERR_PTR(ret) : NULL;
>>>>>>
>>>>>> In case of an error, the memory allocated for fdt is freed unless
>>>>>> it has
>>>>>> already been passed to kexec_add_buffer().
>>>>>
>>>>> It feels like the root of the problem is that the kvfree of fdt is in
>>>>> the wrong place. It's only allocated later in the function, so the
>>>>> error
>>>>> path should reflect that. Something like the patch below.
>>>>>
>>>>> cheers
>>>>>
>>>>>
>>>>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>>>>> index 5a569bb51349..02662e72c53d 100644
>>>>> --- a/arch/powerpc/kexec/elf_64.c
>>>>> +++ b/arch/powerpc/kexec/elf_64.c
>>>>> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image,
>>>>> char *kernel_buf,
>>>>> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
>>>>> initrd_len, cmdline);
>>>>> if (ret)
>>>>> - goto out;
>>>>> + goto out_free_fdt;
>>>>>
>>>>> fdt_pack(fdt);
>>>>>
>>>>> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image,
>>>>> char *kernel_buf,
>>>>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>>>>> ret = kexec_add_buffer(&kbuf);
>>>>> if (ret)
>>>>> - goto out;
>>>>> + goto out_free_fdt;
>>>>>
>>>>> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
>>>>> image->arch.fdt = fdt;
>>>>> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image,
>>>>> char *kernel_buf,
>>>>> if (ret)
>>>>> pr_err("Error setting up the purgatory.\n");
>>>>>
>>>>> + goto out;
>>>>
>>>> This will leak. It would need to be something like:
>>>>
>>>> if (ret) {
>>>> pr_err("Error setting up the purgatory.\n");
>>>> goto out_free_fdt;
>>>> }
>>> Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
>>> be freed here - it will be freed when the kexec cleanup function is
>>> called.
>>
>> That may be the case currently, but really if a function returns an
>> error it should have undone anything it did like memory allocations. I
>> don't think you should do that to fix this issue, but it would be a
>> good clean-up.
>>
>
> I agree - in case of an error the function should do a proper clean-up.
> Just to be clear - for now, I will leave this as is. Correct?
>
> In my patch, I will do the following changes:
>
> => Free "fdt" when possible (as Michael had suggested in his patch)
> => Zero out "elf_info" struct at the start of the function.
>
Instead of zeroing out "elf_info", I think it would be better to return
an error immediately, instead of the "goto out;", if
kexec_build_elf_info() fails.
ret = kexec_build_elf_info(kernel_buf, kernel_len, &ehdr, &elf_info);
if (ret)
return ERR_PTR(ret);
-lakshmi
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Lakshmi Ramasubramanian @ 2021-04-20 14:42 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, kbuild-all, kbuild test robot, Thiago Jung Bauermann,
linuxppc-dev, Dan Carpenter, Daniel Axtens
In-Reply-To: <CAL_JsqLS9Wu_+_S-2wwMb3Chd_8RYAtFe_uLh5tjj_sAkTgRJA@mail.gmail.com>
On 4/20/21 6:06 AM, Rob Herring wrote:
> On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian
> <nramas@linux.microsoft.com> wrote:
>>
>> On 4/19/21 10:00 PM, Dan Carpenter wrote:
>>> On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
>>>> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
>>>>> On 4/16/21 2:05 AM, Michael Ellerman wrote:
>>>>>
>>>>>> Daniel Axtens <dja@axtens.net> writes:
>>>>>>>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
>>>>>>>>
>>>>>>>> Sorry - missed copying device-tree and powerpc mailing lists.
>>>>>>>>
>>>>>>>>> There are a few "goto out;" statements before the local variable "fdt"
>>>>>>>>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
>>>>>>>>> elf64_load(). This will result in an uninitialized "fdt" being passed
>>>>>>>>> to kvfree() in this function if there is an error before the call to
>>>>>>>>> of_kexec_alloc_and_setup_fdt().
>>>>>>>>>
>>>>>>>>> Initialize the local variable "fdt" to NULL.
>>>>>>>>>
>>>>>>> I'm a huge fan of initialising local variables! But I'm struggling to
>>>>>>> find the code path that will lead to an uninit fdt being returned...
>>>>>>>
>>>>>>> The out label reads in part:
>>>>>>>
>>>>>>> /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
>>>>>>> return ret ? ERR_PTR(ret) : fdt;
>>>>>>>
>>>>>>> As far as I can tell, any time we get a non-zero ret, we're going to
>>>>>>> return an error pointer rather than the uninitialised value...
>>>>>
>>>>> As Dan pointed out, the new code is in linux-next.
>>>>>
>>>>> I have copied the new one below - the function doesn't return fdt, but
>>>>> instead sets it in the arch specific field (please see the link to the
>>>>> updated elf_64.c below).
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
>>>>>
>>>>>>>
>>>>>>> (btw, it does look like we might leak fdt if we have an error after we
>>>>>>> successfully kmalloc it.)
>>>>>>>
>>>>>>> Am I missing something? Can you link to the report for the kernel test
>>>>>>> robot or from Dan?
>>>>>
>>>>> /*
>>>>> * Once FDT buffer has been successfully passed to
>>>>> kexec_add_buffer(),
>>>>> * the FDT buffer address is saved in image->arch.fdt. In that
>>>>> case,
>>>>> * the memory cannot be freed here in case of any other error.
>>>>> */
>>>>> if (ret && !image->arch.fdt)
>>>>> kvfree(fdt);
>>>>>
>>>>> return ret ? ERR_PTR(ret) : NULL;
>>>>>
>>>>> In case of an error, the memory allocated for fdt is freed unless it has
>>>>> already been passed to kexec_add_buffer().
>>>>
>>>> It feels like the root of the problem is that the kvfree of fdt is in
>>>> the wrong place. It's only allocated later in the function, so the error
>>>> path should reflect that. Something like the patch below.
>>>>
>>>> cheers
>>>>
>>>>
>>>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
>>>> index 5a569bb51349..02662e72c53d 100644
>>>> --- a/arch/powerpc/kexec/elf_64.c
>>>> +++ b/arch/powerpc/kexec/elf_64.c
>>>> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>>>> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
>>>> initrd_len, cmdline);
>>>> if (ret)
>>>> - goto out;
>>>> + goto out_free_fdt;
>>>>
>>>> fdt_pack(fdt);
>>>>
>>>> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>>>> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
>>>> ret = kexec_add_buffer(&kbuf);
>>>> if (ret)
>>>> - goto out;
>>>> + goto out_free_fdt;
>>>>
>>>> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
>>>> image->arch.fdt = fdt;
>>>> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>>>> if (ret)
>>>> pr_err("Error setting up the purgatory.\n");
>>>>
>>>> + goto out;
>>>
>>> This will leak. It would need to be something like:
>>>
>>> if (ret) {
>>> pr_err("Error setting up the purgatory.\n");
>>> goto out_free_fdt;
>>> }
>> Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
>> be freed here - it will be freed when the kexec cleanup function is called.
>
> That may be the case currently, but really if a function returns an
> error it should have undone anything it did like memory allocations. I
> don't think you should do that to fix this issue, but it would be a
> good clean-up.
>
I agree - in case of an error the function should do a proper clean-up.
Just to be clear - for now, I will leave this as is. Correct?
In my patch, I will do the following changes:
=> Free "fdt" when possible (as Michael had suggested in his patch)
=> Zero out "elf_info" struct at the start of the function.
thanks,
-lakshmi
^ permalink raw reply
* [PATCH v2 2/2] powerpc: Enable OPTPROBES on PPC32
From: Christophe Leroy @ 2021-04-20 14:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
naveen.n.rao
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <22cadf29620664b600b82026d2a72b8b23351777.1618927318.git.christophe.leroy@csgroup.eu>
For that, create a 32 bits version of patch_imm64_load_insns()
and create a patch_imm_load_insns() which calls
patch_imm32_load_insns() on PPC32 and patch_imm64_load_insns()
on PPC64.
Adapt optprobes_head.S for PPC32. Use PPC_LL/PPC_STL macros instead
of raw ld/std, opt out things linked to paca and use stmw/lmw to
save/restore registers.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: Comments from Naveen.
---
arch/powerpc/Kconfig | 2 +-
arch/powerpc/kernel/optprobes.c | 24 ++++++++--
arch/powerpc/kernel/optprobes_head.S | 65 +++++++++++++++-------------
3 files changed, 56 insertions(+), 35 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 475d77a6ebbe..d2e31a578e26 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -229,7 +229,7 @@ config PPC
select HAVE_MOD_ARCH_SPECIFIC
select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
select HAVE_HARDLOCKUP_DETECTOR_ARCH if PPC64 && PPC_BOOK3S && SMP
- select HAVE_OPTPROBES if PPC64
+ select HAVE_OPTPROBES
select HAVE_PERF_EVENTS
select HAVE_PERF_EVENTS_NMI if PPC64
select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI && !HAVE_HARDLOCKUP_DETECTOR_ARCH
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 58fdb9f66e0f..cdf87086fa33 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -141,11 +141,21 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
}
}
+static void patch_imm32_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr)
+{
+ patch_instruction((struct ppc_inst *)addr,
+ ppc_inst(PPC_RAW_LIS(reg, IMM_H(val))));
+ addr++;
+
+ patch_instruction((struct ppc_inst *)addr,
+ ppc_inst(PPC_RAW_ORI(reg, reg, IMM_L(val))));
+}
+
/*
* Generate instructions to load provided immediate 64-bit value
* to register 'reg' and patch these instructions at 'addr'.
*/
-static void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr)
+static void patch_imm64_load_insns(unsigned long long val, int reg, kprobe_opcode_t *addr)
{
/* lis reg,(op)@highest */
patch_instruction((struct ppc_inst *)addr,
@@ -177,6 +187,14 @@ static void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t *
___PPC_RS(reg) | (val & 0xffff)));
}
+static void patch_imm_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr)
+{
+ if (IS_ENABLED(CONFIG_PPC64))
+ patch_imm64_load_insns(val, reg, addr);
+ else
+ patch_imm32_load_insns(val, reg, addr);
+}
+
int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
{
struct ppc_inst branch_op_callback, branch_emulate_step, temp;
@@ -230,7 +248,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
* Fixup the template with instructions to:
* 1. load the address of the actual probepoint
*/
- patch_imm64_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX);
+ patch_imm_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX);
/*
* 2. branch to optimized_callback() and emulate_step()
@@ -264,7 +282,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
* 3. load instruction to be emulated into relevant register, and
*/
temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
- patch_imm64_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
+ patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
/*
* 4. branch back from trampoline
diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S
index ff8ba4d3824d..19ea3312403c 100644
--- a/arch/powerpc/kernel/optprobes_head.S
+++ b/arch/powerpc/kernel/optprobes_head.S
@@ -9,6 +9,16 @@
#include <asm/ptrace.h>
#include <asm/asm-offsets.h>
+#ifdef CONFIG_PPC64
+#define SAVE_30GPRS(base) SAVE_10GPRS(2,base); SAVE_10GPRS(12,base); SAVE_10GPRS(22,base)
+#define REST_30GPRS(base) REST_10GPRS(2,base); REST_10GPRS(12,base); REST_10GPRS(22,base)
+#define TEMPLATE_FOR_IMM_LOAD_INSNS nop; nop; nop; nop; nop
+#else
+#define SAVE_30GPRS(base) stmw r2, GPR2(base)
+#define REST_30GPRS(base) lmw r2, GPR2(base)
+#define TEMPLATE_FOR_IMM_LOAD_INSNS nop; nop; nop
+#endif
+
#define OPT_SLOT_SIZE 65536
.balign 4
@@ -30,39 +40,41 @@ optinsn_slot:
.global optprobe_template_entry
optprobe_template_entry:
/* Create an in-memory pt_regs */
- stdu r1,-INT_FRAME_SIZE(r1)
+ PPC_STLU r1,-INT_FRAME_SIZE(r1)
SAVE_GPR(0,r1)
/* Save the previous SP into stack */
addi r0,r1,INT_FRAME_SIZE
- std r0,GPR1(r1)
- SAVE_10GPRS(2,r1)
- SAVE_10GPRS(12,r1)
- SAVE_10GPRS(22,r1)
+ PPC_STL r0,GPR1(r1)
+ SAVE_30GPRS(r1)
/* Save SPRS */
mfmsr r5
- std r5,_MSR(r1)
+ PPC_STL r5,_MSR(r1)
li r5,0x700
- std r5,_TRAP(r1)
+ PPC_STL r5,_TRAP(r1)
li r5,0
- std r5,ORIG_GPR3(r1)
- std r5,RESULT(r1)
+ PPC_STL r5,ORIG_GPR3(r1)
+ PPC_STL r5,RESULT(r1)
mfctr r5
- std r5,_CTR(r1)
+ PPC_STL r5,_CTR(r1)
mflr r5
- std r5,_LINK(r1)
+ PPC_STL r5,_LINK(r1)
mfspr r5,SPRN_XER
- std r5,_XER(r1)
+ PPC_STL r5,_XER(r1)
mfcr r5
- std r5,_CCR(r1)
+ PPC_STL r5,_CCR(r1)
+#ifdef CONFIG_PPC64
lbz r5,PACAIRQSOFTMASK(r13)
std r5,SOFTE(r1)
+#endif
/*
* We may get here from a module, so load the kernel TOC in r2.
* The original TOC gets restored when pt_regs is restored
* further below.
*/
+#ifdef CONFIG_PPC64
ld r2,PACATOC(r13)
+#endif
.global optprobe_template_op_address
optprobe_template_op_address:
@@ -70,11 +82,8 @@ optprobe_template_op_address:
* Parameters to optimized_callback():
* 1. optimized_kprobe structure in r3
*/
- nop
- nop
- nop
- nop
- nop
+ TEMPLATE_FOR_IMM_LOAD_INSNS
+
/* 2. pt_regs pointer in r4 */
addi r4,r1,STACK_FRAME_OVERHEAD
@@ -92,11 +101,7 @@ optprobe_template_call_handler:
.global optprobe_template_insn
optprobe_template_insn:
/* 2, Pass instruction to be emulated in r4 */
- nop
- nop
- nop
- nop
- nop
+ TEMPLATE_FOR_IMM_LOAD_INSNS
.global optprobe_template_call_emulate
optprobe_template_call_emulate:
@@ -107,20 +112,18 @@ optprobe_template_call_emulate:
* All done.
* Now, restore the registers...
*/
- ld r5,_MSR(r1)
+ PPC_LL r5,_MSR(r1)
mtmsr r5
- ld r5,_CTR(r1)
+ PPC_LL r5,_CTR(r1)
mtctr r5
- ld r5,_LINK(r1)
+ PPC_LL r5,_LINK(r1)
mtlr r5
- ld r5,_XER(r1)
+ PPC_LL r5,_XER(r1)
mtxer r5
- ld r5,_CCR(r1)
+ PPC_LL r5,_CCR(r1)
mtcr r5
REST_GPR(0,r1)
- REST_10GPRS(2,r1)
- REST_10GPRS(12,r1)
- REST_10GPRS(22,r1)
+ REST_30GPRS(r1)
/* Restore the previous SP */
addi r1,r1,INT_FRAME_SIZE
--
2.25.0
^ permalink raw reply related
* [PATCH v2 1/2] powerpc/inst: ppc_inst_as_u64() becomes ppc_inst_as_ulong()
From: Christophe Leroy @ 2021-04-20 14:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
naveen.n.rao
Cc: linuxppc-dev, linux-kernel
In order to simplify use on PPC32, change ppc_inst_as_u64()
into ppc_inst_as_ulong() that returns the 32 bits instruction
on PPC32.
Will be used when porting OPTPROBES to PPC32.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/inst.h | 13 +++++++------
arch/powerpc/kernel/optprobes.c | 2 +-
arch/powerpc/lib/code-patching.c | 2 +-
arch/powerpc/xmon/xmon.c | 2 +-
4 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index 19e18af2fac9..9646c63f7420 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -147,13 +147,14 @@ static inline struct ppc_inst *ppc_inst_next(void *location, struct ppc_inst *va
return location + ppc_inst_len(tmp);
}
-static inline u64 ppc_inst_as_u64(struct ppc_inst x)
+static inline unsigned long ppc_inst_as_ulong(struct ppc_inst x)
{
-#ifdef CONFIG_CPU_LITTLE_ENDIAN
- return (u64)ppc_inst_suffix(x) << 32 | ppc_inst_val(x);
-#else
- return (u64)ppc_inst_val(x) << 32 | ppc_inst_suffix(x);
-#endif
+ if (IS_ENABLED(CONFIG_PPC32))
+ return ppc_inst_val(x);
+ else if (IS_ENABLED(CONFIG_CPU_LITTLE_ENDIAN))
+ return (u64)ppc_inst_suffix(x) << 32 | ppc_inst_val(x);
+ else
+ return (u64)ppc_inst_val(x) << 32 | ppc_inst_suffix(x);
}
#define PPC_INST_STR_LEN sizeof("00000000 00000000")
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 7f7cdbeacd1a..58fdb9f66e0f 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -264,7 +264,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
* 3. load instruction to be emulated into relevant register, and
*/
temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
- patch_imm64_load_insns(ppc_inst_as_u64(temp), 4, buff + TMPL_INSN_IDX);
+ patch_imm64_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
/*
* 4. branch back from trampoline
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 65aec4d6d9ba..870b30d9be2f 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -26,7 +26,7 @@ static int __patch_instruction(struct ppc_inst *exec_addr, struct ppc_inst instr
__put_kernel_nofault(patch_addr, &val, u32, failed);
} else {
- u64 val = ppc_inst_as_u64(instr);
+ u64 val = ppc_inst_as_ulong(instr);
__put_kernel_nofault(patch_addr, &val, u64, failed);
}
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index a619b9ed8458..ff2b92bfeedc 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -2953,7 +2953,7 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
if (!ppc_inst_prefixed(inst))
dump_func(ppc_inst_val(inst), adr);
else
- dump_func(ppc_inst_as_u64(inst), adr);
+ dump_func(ppc_inst_as_ulong(inst), adr);
printf("\n");
}
return adr - first_adr;
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
From: Lucas Stach @ 2021-04-20 14:00 UTC (permalink / raw)
To: Robin Gong, Shengjiu Wang
Cc: sumit.semwal@linaro.org, linaro-mm-sig@lists.linaro.org,
Linux-ALSA, linuxppc-dev@lists.ozlabs.org, Timur Tabi, Xiubo Li,
Fabio Estevam, s.hauer@pengutronix.de, Takashi Iwai,
Liam Girdwood, dri-devel@lists.freedesktop.org, linux-kernel,
Nicolin Chen, Mark Brown, dl-linux-imx, kernel@pengutronix.de,
perex@perex.cz, shawnguo@kernel.org, S.j. Wang,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <VE1PR04MB6638659EC8557D01861042B189489@VE1PR04MB6638.eurprd04.prod.outlook.com>
Am Dienstag, dem 20.04.2021 um 13:47 +0000 schrieb Robin Gong:
> On 2021/04/19 17:46 Lucas Stach <l.stach@pengutronix.de> wrote:
> > Am Montag, dem 19.04.2021 um 07:17 +0000 schrieb Robin Gong:
> > > Hi Lucas,
> > >
> > > On 2021/04/14 Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > Hi Robin,
> > > >
> > > > Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong:
> > > > > On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang:
> > > > > > > Hi
> > > > > > >
> > > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach
> > > > > > > <l.stach@pengutronix.de>
> > > > > > wrote:
> > > > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang:
> > > > > > > > > There are two requirements that we need to move the
> > > > > > > > > request of dma channel from probe to open.
> > > > > > > >
> > > > > > > > How do you handle -EPROBE_DEFER return code from the channel
> > > > > > > > request if you don't do it in probe?
> > > > > > >
> > > > > > > I use the dma_request_slave_channel or dma_request_channel
> > > > > > > instead of dmaengine_pcm_request_chan_of. so there should be
> > > > > > > not -EPROBE_DEFER return code.
> > > > > >
> > > > > > This is a pretty weak argument. The dmaengine device might probe
> > > > > > after you try to get the channel. Using a function to request
> > > > > > the channel that doesn't allow you to handle probe deferral is
> > > > > > IMHO a bug and should be fixed, instead of building even more
> > > > > > assumptions on top
> > > > of it.
> > > > > >
> > > > > > > > > - When dma device binds with power-domains, the power will
> > > > > > > > > be enabled when we request dma channel. If the request of
> > > > > > > > > dma channel happen on probe, then the power-domains will
> > > > > > > > > be always enabled after kernel boot up, which is not good
> > > > > > > > > for power saving, so we need to move the request of dma
> > > > > > > > > channel to .open();
> > > > > > > >
> > > > > > > > This is certainly something which could be fixed in the
> > > > > > > > dmaengine driver.
> > > > > > >
> > > > > > > Dma driver always call the pm_runtime_get_sync in
> > > > > > > device_alloc_chan_resources, the device_alloc_chan_resources
> > > > > > > is called when channel is requested. so power is enabled on
> > > > > > > channel
> > > > request.
> > > > > >
> > > > > > So why can't you fix the dmaengine driver to do that RPM call at
> > > > > > a later time when the channel is actually going to be used? This
> > > > > > will allow further power savings with other slave devices than the audio
> > PCM.
> > > > > Hi Lucas,
> > > > > Thanks for your suggestion. I have tried to implement runtime
> > > > > autosuspend in fsl-edma driver on i.mx8qm/qxp with delay time (2
> > > > > sec) for this feature as below (or you can refer to
> > > > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/
> > > > > pm_runtime_put_autosuspend in all dmaengine driver interface like
> > > > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_c
> > > > > ycli
> > > > > c/
> > > > > device_tx_status...
> > > > >
> > > > >
> > > > > pm_runtime_use_autosuspend(fsl_chan->dev);
> > > > > pm_runtime_set_autosuspend_delay(fsl_chan->
> > dev,
> > > > 2000);
> > > > >
> > > > > That could resolve this audio case since the autosuspend could
> > > > > suspend runtime after
> > > > > 2 seconds if there is no further dma transfer but only channel
> > > > request(device_alloc_chan_resources).
> > > > > But unfortunately, it cause another issue. As you know, on our
> > > > > i.mx8qm/qxp, power domain done by scfw
> > > > > (drivers/firmware/imx/scu-pd.c)
> > > > over mailbox:
> > > > > imx_sc_pd_power()->imx_scu_call_rpc()->
> > > > > imx_scu_ipc_write()->mbox_send_message()
> > > > > which means have to 'waits for completion', meanwhile, some driver
> > > > > like tty will call dmaengine interfaces in non-atomic case as
> > > > > below,
> > > > >
> > > > > static int uart_write(struct tty_struct *tty, const unsigned char
> > > > > *buf, int count) {
> > > > > .......
> > > > > port = uart_port_lock(state, flags);
> > > > > ......
> > > > > __uart_start(tty); //call
> > start_tx()->dmaengine_prep_slave_sg...
> > > > > uart_port_unlock(port, flags);
> > > > > return ret;
> > > > > }
> > > > >
> > > > > Thus dma runtime resume may happen in that timing window and cause
> > > > kernel alarm.
> > > > > I'm not sure whether there are similar limitations on other driver
> > > > > subsystem. But for me, It looks like the only way to resolve the
> > > > > contradiction between tty and scu-pd (hardware limitation on
> > > > > i.mx8qm/qxp) is to give up autosuspend and keep
> > > > > pm_runtime_get_sync
> > > > only in device_alloc_chan_resources because request channel is a
> > > > safe non-atomic phase.
> > > > > Do you have any idea? Thanks in advance.
> > > >
> > > > If you look closely at the driver you used as an example (hidma.c)
> > > > it looks like there is already something in there, which looks very
> > > > much like what you need
> > > > here:
> > > >
> > > > In hidma_issue_pending() the driver tries to get the device to runtime
> > resume.
> > > > If this doesn't work, maybe due to the power domain code not being
> > > > able to be called in atomic context, the actual work of waking up
> > > > the dma hardware and issuing the descriptor is shunted to a tasklet.
> > > >
> > > > If I'm reading this right, this is exactly what you need here to be
> > > > able to call the dmaengine code from atomic context: try the rpm get
> > > > and issue immediately when possible, otherwise shunt the work to a
> > > > non- atomic context where you can deal with the requirements of scu-pd.
> > > Yes, I can schedule_work to worker to runtime resume edma channel by
> > calling scu-pd.
> > > But that means all dmaengine interfaces should be taken care, not only
> > > issue_pending() but also
> > > dmaengine_terminate_all()/dmaengine_pause()/dmaengine_resume()/
> > > dmaengine_tx_status(). Not sure why hidma only take care
> > > issue_pending. Maybe their user case is just for memcpy/memset so that
> > > no further complicate case as ALSA or TTY.
> > > Besides, for autosuspend in cyclic, we have to add pm_runtime_get_sync
> > > into interrupt handler as qcom/bam_dma.c. but how could resolve the
> > > scu-pd's non-atmoic limitation in interrupt handler?
> >
> > Sure, this all needs some careful analysis on how those functions are called
> > and what to do about atomic callers, but it should be doable. I don't see any
> > fundamental issues here.
> >
> > I don't see why you would ever need to wake the hardware in an interrupt
> > handler. Surely the hardware is already awake, as it wouldn't signal an
> > interrupt otherwise. And for the issue with scu-pd you only care about the
> > state transition of suspended->running. If the hardware is already
> > running/awake, the runtime pm state handling is nothing more than bumping
> > a refcount, which is atomic safe. Putting the HW in suspend is already handled
> > asynchronously in a worker, so this is also atomic safe.
> But with autosuspend used, in corner case, may runtime suspended before falling
> Into edma interrupt handler if timeout happen with the delay value of
> pm_runtime_set_autosuspend_delay(). Thus, can't touch any edma interrupt
> status register unless runtime resume edma in interrupt handler while runtime
> resume function based on scu-pd's power domain may block or sleep.
> I have a simple workaround that disable runtime suspend in issue_pending worker
> by calling pm_runtime_forbid() and then enable runtime auto suspend in
> dmaengine_terminate_all so that we could easily regard that edma channel is always
> in runtime resume between issue_pending and channel terminated and ignore the above
> interrupt handler/scu-pd limitation.
The IRQ handler is the point where you are informed by the hardware
that a specific operation is complete. I don't see any use-case where
it would be valid to drop the rpm refcount to 0 before the IRQ is
handled. Surely the hardware needs to stay awake until the currently
queued operations are complete and if the IRQ handler is the completion
point the IRQ handler is the first point in time where your autosuspend
timer should start to run. There should never be a situation where the
timer expiry can get between IRQ signaling and the handler code
running.
Regards,
Lucas
^ permalink raw reply
* Re: swiotlb cleanups v3
From: Tom Lendacky @ 2021-04-20 13:48 UTC (permalink / raw)
To: Christoph Hellwig
Cc: xen-devel, konrad.wilk, dongli.zhang, iommu, tientzu,
linuxppc-dev
In-Reply-To: <20210420092312.GB26092@lst.de>
On 4/20/21 4:23 AM, Christoph Hellwig wrote:
> On Sat, Apr 17, 2021 at 11:39:22AM -0500, Tom Lendacky wrote:
>> Somewhere between the 1st and 2nd patch, specifying a specific swiotlb
>> for an SEV guest is no longer honored. For example, if I start an SEV
>> guest with 16GB of memory and specify swiotlb=131072 I used to get a
>> 256MB SWIOTLB. However, after the 2nd patch, the swiotlb=131072 is no
>> longer honored and I get a 982MB SWIOTLB (as set via sev_setup_arch() in
>> arch/x86/mm/mem_encrypt.c).
>>
>> I can't be sure which patch caused the issue since an SEV guest fails to
>> boot with the 1st patch but can boot with the 2nd patch, at which point
>> the SWIOTLB comes in at 982MB (I haven't had a chance to debug it and so
>> I'm hoping you might be able to quickly spot what's going on).
>
> Can you try this patch?
Thanks, Christoph. This works for honoring the command line value with SEV
guests.
There was still a reference to default_nslabs in setup_io_tlb_npages()
that I'm not sure how you want to handle. I just commented it out for now
to let the code compile to test the intent of the patch.
Thanks,
Tom
>
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index 0a5b6f7e75bce6..ac81ef97df32f5 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -71,15 +71,17 @@ struct io_tlb_mem *io_tlb_default_mem;
> */
> static unsigned int max_segment;
>
> -static unsigned long default_nslabs = IO_TLB_DEFAULT_SIZE >> IO_TLB_SHIFT;
> +static unsigned long swiotlb_cmdline_size;
>
> static int __init
> setup_io_tlb_npages(char *str)
> {
> if (isdigit(*str)) {
> /* avoid tail segment of size < IO_TLB_SEGSIZE */
> - default_nslabs =
> - ALIGN(simple_strtoul(str, &str, 0), IO_TLB_SEGSIZE);
> + unsigned long nslabs = simple_strtoul(str, &str, 0);
> +
> + swiotlb_cmdline_size =
> + ALIGN(nslabs, IO_TLB_SEGSIZE) << IO_TLB_SHIFT;
> }
> if (*str == ',')
> ++str;
> @@ -108,7 +110,9 @@ void swiotlb_set_max_segment(unsigned int val)
>
> unsigned long swiotlb_size_or_default(void)
> {
> - return default_nslabs << IO_TLB_SHIFT;
> + if (swiotlb_cmdline_size)
> + return swiotlb_cmdline_size;
> + return IO_TLB_DEFAULT_SIZE;
> }
>
> void __init swiotlb_adjust_size(unsigned long size)
> @@ -118,9 +122,10 @@ void __init swiotlb_adjust_size(unsigned long size)
> * architectures such as those supporting memory encryption to
> * adjust/expand SWIOTLB size for their use.
> */
> - size = ALIGN(size, IO_TLB_SIZE);
> - default_nslabs = ALIGN(size >> IO_TLB_SHIFT, IO_TLB_SEGSIZE);
> - pr_info("SWIOTLB bounce buffer size adjusted to %luMB", size >> 20);
> + if (!swiotlb_cmdline_size)
> + swiotlb_cmdline_size = ALIGN(size, IO_TLB_SIZE);
> + pr_info("SWIOTLB bounce buffer size adjusted to %luMB",
> + swiotlb_cmdline_size >> 20);
> }
>
> void swiotlb_print_info(void)
> @@ -209,7 +214,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
> void __init
> swiotlb_init(int verbose)
> {
> - size_t bytes = PAGE_ALIGN(default_nslabs << IO_TLB_SHIFT);
> + size_t bytes = PAGE_ALIGN(swiotlb_size_or_default());
> void *tlb;
>
> if (swiotlb_force == SWIOTLB_NO_FORCE)
> @@ -219,7 +224,7 @@ swiotlb_init(int verbose)
> tlb = memblock_alloc_low(bytes, PAGE_SIZE);
> if (!tlb)
> goto fail;
> - if (swiotlb_init_with_tbl(tlb, default_nslabs, verbose))
> + if (swiotlb_init_with_tbl(tlb, bytes >> IO_TLB_SHIFT, verbose))
> goto fail_free_mem;
> return;
>
>
^ permalink raw reply
* RE: [PATCH] ASoC: fsl: imx-pcm-dma: Don't request dma channel in probe
From: Robin Gong @ 2021-04-20 13:47 UTC (permalink / raw)
To: Lucas Stach, Shengjiu Wang
Cc: Nicolin Chen, Linux-ALSA, Liam Girdwood, s.hauer@pengutronix.de,
Timur Tabi, Xiubo Li, shawnguo@kernel.org, S.j. Wang,
linux-kernel, dri-devel@lists.freedesktop.org, Takashi Iwai,
linaro-mm-sig@lists.linaro.org, Mark Brown, dl-linux-imx,
kernel@pengutronix.de, Fabio Estevam, perex@perex.cz,
linuxppc-dev@lists.ozlabs.org, sumit.semwal@linaro.org,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <97262466d537402ad4032098ef277d6d47734f1f.camel@pengutronix.de>
On 2021/04/19 17:46 Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Montag, dem 19.04.2021 um 07:17 +0000 schrieb Robin Gong:
> > Hi Lucas,
> >
> > On 2021/04/14 Lucas Stach <l.stach@pengutronix.de> wrote:
> > > Hi Robin,
> > >
> > > Am Mittwoch, dem 14.04.2021 um 14:33 +0000 schrieb Robin Gong:
> > > > On 2020/05/20 17:43 Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > > Am Mittwoch, den 20.05.2020, 16:20 +0800 schrieb Shengjiu Wang:
> > > > > > Hi
> > > > > >
> > > > > > On Tue, May 19, 2020 at 6:04 PM Lucas Stach
> > > > > > <l.stach@pengutronix.de>
> > > > > wrote:
> > > > > > > Am Dienstag, den 19.05.2020, 17:41 +0800 schrieb Shengjiu Wang:
> > > > > > > > There are two requirements that we need to move the
> > > > > > > > request of dma channel from probe to open.
> > > > > > >
> > > > > > > How do you handle -EPROBE_DEFER return code from the channel
> > > > > > > request if you don't do it in probe?
> > > > > >
> > > > > > I use the dma_request_slave_channel or dma_request_channel
> > > > > > instead of dmaengine_pcm_request_chan_of. so there should be
> > > > > > not -EPROBE_DEFER return code.
> > > > >
> > > > > This is a pretty weak argument. The dmaengine device might probe
> > > > > after you try to get the channel. Using a function to request
> > > > > the channel that doesn't allow you to handle probe deferral is
> > > > > IMHO a bug and should be fixed, instead of building even more
> > > > > assumptions on top
> > > of it.
> > > > >
> > > > > > > > - When dma device binds with power-domains, the power will
> > > > > > > > be enabled when we request dma channel. If the request of
> > > > > > > > dma channel happen on probe, then the power-domains will
> > > > > > > > be always enabled after kernel boot up, which is not good
> > > > > > > > for power saving, so we need to move the request of dma
> > > > > > > > channel to .open();
> > > > > > >
> > > > > > > This is certainly something which could be fixed in the
> > > > > > > dmaengine driver.
> > > > > >
> > > > > > Dma driver always call the pm_runtime_get_sync in
> > > > > > device_alloc_chan_resources, the device_alloc_chan_resources
> > > > > > is called when channel is requested. so power is enabled on
> > > > > > channel
> > > request.
> > > > >
> > > > > So why can't you fix the dmaengine driver to do that RPM call at
> > > > > a later time when the channel is actually going to be used? This
> > > > > will allow further power savings with other slave devices than the audio
> PCM.
> > > > Hi Lucas,
> > > > Thanks for your suggestion. I have tried to implement runtime
> > > > autosuspend in fsl-edma driver on i.mx8qm/qxp with delay time (2
> > > > sec) for this feature as below (or you can refer to
> > > > drivers/dma/qcom/hidma.c), and pm_runtime_get_sync/
> > > > pm_runtime_put_autosuspend in all dmaengine driver interface like
> > > > device_alloc_chan_resources/device_prep_slave_sg/device_prep_dma_c
> > > > ycli
> > > > c/
> > > > device_tx_status...
> > > >
> > > >
> > > > pm_runtime_use_autosuspend(fsl_chan->dev);
> > > > pm_runtime_set_autosuspend_delay(fsl_chan->
> dev,
> > > 2000);
> > > >
> > > > That could resolve this audio case since the autosuspend could
> > > > suspend runtime after
> > > > 2 seconds if there is no further dma transfer but only channel
> > > request(device_alloc_chan_resources).
> > > > But unfortunately, it cause another issue. As you know, on our
> > > > i.mx8qm/qxp, power domain done by scfw
> > > > (drivers/firmware/imx/scu-pd.c)
> > > over mailbox:
> > > > imx_sc_pd_power()->imx_scu_call_rpc()->
> > > > imx_scu_ipc_write()->mbox_send_message()
> > > > which means have to 'waits for completion', meanwhile, some driver
> > > > like tty will call dmaengine interfaces in non-atomic case as
> > > > below,
> > > >
> > > > static int uart_write(struct tty_struct *tty, const unsigned char
> > > > *buf, int count) {
> > > > .......
> > > > port = uart_port_lock(state, flags);
> > > > ......
> > > > __uart_start(tty); //call
> start_tx()->dmaengine_prep_slave_sg...
> > > > uart_port_unlock(port, flags);
> > > > return ret;
> > > > }
> > > >
> > > > Thus dma runtime resume may happen in that timing window and cause
> > > kernel alarm.
> > > > I'm not sure whether there are similar limitations on other driver
> > > > subsystem. But for me, It looks like the only way to resolve the
> > > > contradiction between tty and scu-pd (hardware limitation on
> > > > i.mx8qm/qxp) is to give up autosuspend and keep
> > > > pm_runtime_get_sync
> > > only in device_alloc_chan_resources because request channel is a
> > > safe non-atomic phase.
> > > > Do you have any idea? Thanks in advance.
> > >
> > > If you look closely at the driver you used as an example (hidma.c)
> > > it looks like there is already something in there, which looks very
> > > much like what you need
> > > here:
> > >
> > > In hidma_issue_pending() the driver tries to get the device to runtime
> resume.
> > > If this doesn't work, maybe due to the power domain code not being
> > > able to be called in atomic context, the actual work of waking up
> > > the dma hardware and issuing the descriptor is shunted to a tasklet.
> > >
> > > If I'm reading this right, this is exactly what you need here to be
> > > able to call the dmaengine code from atomic context: try the rpm get
> > > and issue immediately when possible, otherwise shunt the work to a
> > > non- atomic context where you can deal with the requirements of scu-pd.
> > Yes, I can schedule_work to worker to runtime resume edma channel by
> calling scu-pd.
> > But that means all dmaengine interfaces should be taken care, not only
> > issue_pending() but also
> > dmaengine_terminate_all()/dmaengine_pause()/dmaengine_resume()/
> > dmaengine_tx_status(). Not sure why hidma only take care
> > issue_pending. Maybe their user case is just for memcpy/memset so that
> > no further complicate case as ALSA or TTY.
> > Besides, for autosuspend in cyclic, we have to add pm_runtime_get_sync
> > into interrupt handler as qcom/bam_dma.c. but how could resolve the
> > scu-pd's non-atmoic limitation in interrupt handler?
>
> Sure, this all needs some careful analysis on how those functions are called
> and what to do about atomic callers, but it should be doable. I don't see any
> fundamental issues here.
>
> I don't see why you would ever need to wake the hardware in an interrupt
> handler. Surely the hardware is already awake, as it wouldn't signal an
> interrupt otherwise. And for the issue with scu-pd you only care about the
> state transition of suspended->running. If the hardware is already
> running/awake, the runtime pm state handling is nothing more than bumping
> a refcount, which is atomic safe. Putting the HW in suspend is already handled
> asynchronously in a worker, so this is also atomic safe.
But with autosuspend used, in corner case, may runtime suspended before falling
Into edma interrupt handler if timeout happen with the delay value of
pm_runtime_set_autosuspend_delay(). Thus, can't touch any edma interrupt
status register unless runtime resume edma in interrupt handler while runtime
resume function based on scu-pd's power domain may block or sleep.
I have a simple workaround that disable runtime suspend in issue_pending worker
by calling pm_runtime_forbid() and then enable runtime auto suspend in
dmaengine_terminate_all so that we could easily regard that edma channel is always
in runtime resume between issue_pending and channel terminated and ignore the above
interrupt handler/scu-pd limitation.
^ permalink raw reply
* Re: [PATCH v2 2/2] powerpc/legacy_serial: Use early_ioremap()
From: Christophe Leroy @ 2021-04-20 13:39 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
chris.packham
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <103ed8ee9e5973c958ec1da2d0b0764f69395d01.1618925560.git.christophe.leroy@csgroup.eu>
Le 20/04/2021 à 15:32, Christophe Leroy a écrit :
> From: Christophe Leroy <christophe.leroy@c-s.fr>
Oops, I forgot to reset the Author. Michael if you apply this patch please update the author and
remove the old Signed-off-by
Thanks
>
> [ 0.000000] ioremap() called early from find_legacy_serial_ports+0x3cc/0x474. Use early_ioremap() instead
>
> find_legacy_serial_ports() is called early from setup_arch(), before
> paging_init(). vmalloc is not available yet, ioremap shouldn't be
> used that early.
>
> Use early_ioremap() and switch to a regular ioremap() later.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/legacy_serial.c | 33 +++++++++++++++++++++++++----
> 1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
> index f061e06e9f51..8b2c1a8553a0 100644
> --- a/arch/powerpc/kernel/legacy_serial.c
> +++ b/arch/powerpc/kernel/legacy_serial.c
> @@ -15,6 +15,7 @@
> #include <asm/udbg.h>
> #include <asm/pci-bridge.h>
> #include <asm/ppc-pci.h>
> +#include <asm/early_ioremap.h>
>
> #undef DEBUG
>
> @@ -34,6 +35,7 @@ static struct legacy_serial_info {
> unsigned int clock;
> int irq_check_parent;
> phys_addr_t taddr;
> + void __iomem *early_addr;
> } legacy_serial_infos[MAX_LEGACY_SERIAL_PORTS];
>
> static const struct of_device_id legacy_serial_parents[] __initconst = {
> @@ -325,17 +327,16 @@ static void __init setup_legacy_serial_console(int console)
> {
> struct legacy_serial_info *info = &legacy_serial_infos[console];
> struct plat_serial8250_port *port = &legacy_serial_ports[console];
> - void __iomem *addr;
> unsigned int stride;
>
> stride = 1 << port->regshift;
>
> /* Check if a translated MMIO address has been found */
> if (info->taddr) {
> - addr = ioremap(info->taddr, 0x1000);
> - if (addr == NULL)
> + info->early_addr = early_ioremap(info->taddr, 0x1000);
> + if (info->early_addr == NULL)
> return;
> - udbg_uart_init_mmio(addr, stride);
> + udbg_uart_init_mmio(info->early_addr, stride);
> } else {
> /* Check if it's PIO and we support untranslated PIO */
> if (port->iotype == UPIO_PORT && isa_io_special)
> @@ -353,6 +354,30 @@ static void __init setup_legacy_serial_console(int console)
> udbg_uart_setup(info->speed, info->clock);
> }
>
> +static int __init ioremap_legacy_serial_console(void)
> +{
> + struct legacy_serial_info *info = &legacy_serial_infos[legacy_serial_console];
> + struct plat_serial8250_port *port = &legacy_serial_ports[legacy_serial_console];
> + void __iomem *vaddr;
> +
> + if (legacy_serial_console < 0)
> + return 0;
> +
> + if (!info->early_addr)
> + return 0;
> +
> + vaddr = ioremap(info->taddr, 0x1000);
> + if (WARN_ON(!vaddr))
> + return -ENOMEM;
> +
> + udbg_uart_init_mmio(vaddr, 1 << port->regshift);
> + early_iounmap(info->early_addr, 0x1000);
> + info->early_addr = NULL;
> +
> + return 0;
> +}
> +early_initcall(ioremap_legacy_serial_console);
> +
> /*
> * This is called very early, as part of setup_system() or eventually
> * setup_arch(), basically before anything else in this file. This function
>
^ permalink raw reply
* Re: [PATCH v1 2/2] powerpc: Enable OPTPROBES on PPC32
From: Christophe Leroy @ 2021-04-20 13:37 UTC (permalink / raw)
To: Naveen N. Rao, Benjamin Herrenschmidt, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <1618900760.son2fwciv1.naveen@linux.ibm.com>
Le 20/04/2021 à 08:51, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> For that, create a 32 bits version of patch_imm64_load_insns()
>> and create a patch_imm_load_insns() which calls
>> patch_imm32_load_insns() on PPC32 and patch_imm64_load_insns()
>> on PPC64.
>>
>> Adapt optprobes_head.S for PPC32. Use PPC_LL/PPC_STL macros instead
>> of raw ld/std, opt out things linked to paca and use stmw/lmw to
>> save/restore registers.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/Kconfig | 2 +-
>> arch/powerpc/kernel/optprobes.c | 24 +++++++++++++--
>> arch/powerpc/kernel/optprobes_head.S | 46 +++++++++++++++++++---------
>> 3 files changed, 53 insertions(+), 19 deletions(-)
>
> Thanks for adding support for ppc32. It is good to see that it works without too many changes.
>
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index c1344c05226c..49b538e54efb 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -227,7 +227,7 @@ config PPC
>> select HAVE_MOD_ARCH_SPECIFIC
>> select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
>> select HAVE_HARDLOCKUP_DETECTOR_ARCH if PPC64 && PPC_BOOK3S && SMP
>> - select HAVE_OPTPROBES if PPC64
>> + select HAVE_OPTPROBES
>> select HAVE_PERF_EVENTS
>> select HAVE_PERF_EVENTS_NMI if PPC64
>> select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI &&
>> !HAVE_HARDLOCKUP_DETECTOR_ARCH
>> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
>> index 58fdb9f66e0f..cdf87086fa33 100644
>> --- a/arch/powerpc/kernel/optprobes.c
>> +++ b/arch/powerpc/kernel/optprobes.c
>> @@ -141,11 +141,21 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
>> }
>> }
>>
>> +static void patch_imm32_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr)
>> +{
>> + patch_instruction((struct ppc_inst *)addr,
>> + ppc_inst(PPC_RAW_LIS(reg, IMM_H(val))));
>> + addr++;
>> +
>> + patch_instruction((struct ppc_inst *)addr,
>> + ppc_inst(PPC_RAW_ORI(reg, reg, IMM_L(val))));
>> +}
>> +
>> /*
>> * Generate instructions to load provided immediate 64-bit value
>> * to register 'reg' and patch these instructions at 'addr'.
>> */
>> -static void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr)
>> +static void patch_imm64_load_insns(unsigned long long val, int reg, kprobe_opcode_t *addr)
>
> Do you really need this?
Without it I get:
from arch/powerpc/kernel/optprobes.c:8:
arch/powerpc/kernel/optprobes.c: In function 'patch_imm64_load_insns':
arch/powerpc/kernel/optprobes.c:163:14: error: right shift count >= width of type
[-Werror=shift-count-overflow]
163 | ((val >> 48) & 0xffff)));
| ^~
./arch/powerpc/include/asm/inst.h:69:48: note: in definition of macro 'ppc_inst'
69 | #define ppc_inst(x) ((struct ppc_inst){ .val = x })
| ^
arch/powerpc/kernel/optprobes.c:169:31: error: right shift count >= width of type
[-Werror=shift-count-overflow]
169 | ___PPC_RS(reg) | ((val >> 32) & 0xffff)));
| ^~
./arch/powerpc/include/asm/inst.h:69:48: note: in definition of macro 'ppc_inst'
69 | #define ppc_inst(x) ((struct ppc_inst){ .val = x })
| ^
>
>> {
>> /* lis reg,(op)@highest */
>> patch_instruction((struct ppc_inst *)addr,
>> @@ -177,6 +187,14 @@ static void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t *
>> ___PPC_RS(reg) | (val & 0xffff)));
>> }
>>
>> +static void patch_imm_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr)
>> +{
>> + if (IS_ENABLED(CONFIG_PPC64))
>> + patch_imm64_load_insns(val, reg, addr);
>> + else
>> + patch_imm32_load_insns(val, reg, addr);
>> +}
>> +
>> int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
>> {
>> struct ppc_inst branch_op_callback, branch_emulate_step, temp;
>> @@ -230,7 +248,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
>> * Fixup the template with instructions to:
>> * 1. load the address of the actual probepoint
>> */
>> - patch_imm64_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX);
>> + patch_imm_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX);
>>
>> /*
>> * 2. branch to optimized_callback() and emulate_step()
>> @@ -264,7 +282,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
>> * 3. load instruction to be emulated into relevant register, and
>> */
>> temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
>> - patch_imm64_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
>> + patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
>>
>> /*
>> * 4. branch back from trampoline
>> diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S
>> index ff8ba4d3824d..49f31e554573 100644
>> --- a/arch/powerpc/kernel/optprobes_head.S
>> +++ b/arch/powerpc/kernel/optprobes_head.S
>> @@ -30,39 +30,47 @@ optinsn_slot:
>> .global optprobe_template_entry
>> optprobe_template_entry:
>> /* Create an in-memory pt_regs */
>> - stdu r1,-INT_FRAME_SIZE(r1)
>> + PPC_STLU r1,-INT_FRAME_SIZE(r1)
>> SAVE_GPR(0,r1)
>> /* Save the previous SP into stack */
>> addi r0,r1,INT_FRAME_SIZE
>> - std r0,GPR1(r1)
>> + PPC_STL r0,GPR1(r1)
>> +#ifdef CONFIG_PPC64
>> SAVE_10GPRS(2,r1)
>> SAVE_10GPRS(12,r1)
>> SAVE_10GPRS(22,r1)
>> +#else
>> + stmw r2, GPR2(r1)
>> +#endif
>> /* Save SPRS */
>> mfmsr r5
>> - std r5,_MSR(r1)
>> + PPC_STL r5,_MSR(r1)
>> li r5,0x700
>> - std r5,_TRAP(r1)
>> + PPC_STL r5,_TRAP(r1)
>> li r5,0
>> - std r5,ORIG_GPR3(r1)
>> - std r5,RESULT(r1)
>> + PPC_STL r5,ORIG_GPR3(r1)
>> + PPC_STL r5,RESULT(r1)
>> mfctr r5
>> - std r5,_CTR(r1)
>> + PPC_STL r5,_CTR(r1)
>> mflr r5
>> - std r5,_LINK(r1)
>> + PPC_STL r5,_LINK(r1)
>> mfspr r5,SPRN_XER
>> - std r5,_XER(r1)
>> + PPC_STL r5,_XER(r1)
>> mfcr r5
>> - std r5,_CCR(r1)
>> + PPC_STL r5,_CCR(r1)
>> +#ifdef CONFIG_PPC64
>> lbz r5,PACAIRQSOFTMASK(r13)
>> std r5,SOFTE(r1)
>> +#endif
>>
>> /*
>> * We may get here from a module, so load the kernel TOC in r2.
>> * The original TOC gets restored when pt_regs is restored
>> * further below.
>> */
>> +#ifdef CONFIG_PPC64
>> ld r2,PACATOC(r13)
>> +#endif
>
> Are the ISA and ABI documents for ppc32 available publicly?
ABI: https://wiki.raptorcs.com/w/images/0/03/Power-Arch-32-bit-ABI-supp-1.0-Unified.pdf
ISA: https://www.nxp.com/files-static/product/doc/MPCFPE32B.pdf
> I would have thought that the TOC issues
> apply to ppc32 as well, but want to understand why this isn't a problem there.
r2 is the pointer to 'current' on PPC32.
No TOC.
>
>>
>> .global optprobe_template_op_address
>> optprobe_template_op_address:
>> @@ -72,9 +80,11 @@ optprobe_template_op_address:
>> */
>> nop
>> nop
>> +#ifdef CONFIG_PPC64
>> nop
>> nop
>> nop
>> +#endif
>> /* 2. pt_regs pointer in r4 */
>> addi r4,r1,STACK_FRAME_OVERHEAD
>>
>> @@ -94,9 +104,11 @@ optprobe_template_insn:
>> /* 2, Pass instruction to be emulated in r4 */
>> nop
>> nop
>> +#ifdef CONFIG_PPC64
>> nop
>> nop
>> nop
>> +#endif
>
> It would be nice to put these behind a macro so as to avoid these #ifdef blocks here, as well as
> with the register save/restore sequence.
>
Will see what I can do
Christophe
^ permalink raw reply
* [PATCH v2 1/2] powerpc/64: Fix the definition of the fixmap area
From: Christophe Leroy @ 2021-04-20 13:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
chris.packham
Cc: linuxppc-dev, linux-kernel
At the time being, the fixmap area is defined at the top of
the address space or just below KASAN.
This definition is not valid for PPC64.
For PPC64, use the top of the I/O space.
Because of circular dependencies, it is not possible to include
asm/fixmap.h in asm/book3s/64/pgtable.h , so define a fixed size
AREA at the top of the I/O space for fixmap and ensure during
build that the size is big enough.
Fixes: 265c3491c4bc ("powerpc: Add support for GENERIC_EARLY_IOREMAP")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/book3s/64/pgtable.h | 4 +++-
arch/powerpc/include/asm/fixmap.h | 9 +++++++++
arch/powerpc/include/asm/nohash/64/pgtable.h | 5 ++++-
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 0c89977ec10b..a666d561b44d 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -7,6 +7,7 @@
#ifndef __ASSEMBLY__
#include <linux/mmdebug.h>
#include <linux/bug.h>
+#include <linux/sizes.h>
#endif
/*
@@ -324,7 +325,8 @@ extern unsigned long pci_io_base;
#define PHB_IO_END (KERN_IO_START + FULL_IO_SIZE)
#define IOREMAP_BASE (PHB_IO_END)
#define IOREMAP_START (ioremap_bot)
-#define IOREMAP_END (KERN_IO_END)
+#define IOREMAP_END (KERN_IO_END - FIXADDR_SIZE)
+#define FIXADDR_SIZE SZ_32M
/* Advertise special mapping type for AGP */
#define HAVE_PAGE_AGP
diff --git a/arch/powerpc/include/asm/fixmap.h b/arch/powerpc/include/asm/fixmap.h
index 8d03c16a3663..947b5b9c4424 100644
--- a/arch/powerpc/include/asm/fixmap.h
+++ b/arch/powerpc/include/asm/fixmap.h
@@ -23,12 +23,17 @@
#include <asm/kmap_size.h>
#endif
+#ifdef CONFIG_PPC64
+#define FIXADDR_TOP (IOREMAP_END + FIXADDR_SIZE)
+#else
+#define FIXADDR_SIZE 0
#ifdef CONFIG_KASAN
#include <asm/kasan.h>
#define FIXADDR_TOP (KASAN_SHADOW_START - PAGE_SIZE)
#else
#define FIXADDR_TOP ((unsigned long)(-PAGE_SIZE))
#endif
+#endif
/*
* Here we define all the compile-time 'special' virtual
@@ -50,6 +55,7 @@
*/
enum fixed_addresses {
FIX_HOLE,
+#ifdef CONFIG_PPC32
/* reserve the top 128K for early debugging purposes */
FIX_EARLY_DEBUG_TOP = FIX_HOLE,
FIX_EARLY_DEBUG_BASE = FIX_EARLY_DEBUG_TOP+(ALIGN(SZ_128K, PAGE_SIZE)/PAGE_SIZE)-1,
@@ -72,6 +78,7 @@ enum fixed_addresses {
FIX_IMMR_SIZE,
#endif
/* FIX_PCIE_MCFG, */
+#endif /* CONFIG_PPC32 */
__end_of_permanent_fixed_addresses,
#define NR_FIX_BTMAPS (SZ_256K / PAGE_SIZE)
@@ -98,6 +105,8 @@ enum fixed_addresses {
static inline void __set_fixmap(enum fixed_addresses idx,
phys_addr_t phys, pgprot_t flags)
{
+ BUILD_BUG_ON(IS_ENABLED(CONFIG_PPC64) && __FIXADDR_SIZE > FIXADDR_SIZE);
+
if (__builtin_constant_p(idx))
BUILD_BUG_ON(idx >= __end_of_fixed_addresses);
else if (WARN_ON(idx >= __end_of_fixed_addresses))
diff --git a/arch/powerpc/include/asm/nohash/64/pgtable.h b/arch/powerpc/include/asm/nohash/64/pgtable.h
index 6cb8aa357191..57cd3892bfe0 100644
--- a/arch/powerpc/include/asm/nohash/64/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/64/pgtable.h
@@ -6,6 +6,8 @@
* the ppc64 non-hashed page table.
*/
+#include <linux/sizes.h>
+
#include <asm/nohash/64/pgtable-4k.h>
#include <asm/barrier.h>
#include <asm/asm-const.h>
@@ -54,7 +56,8 @@
#define PHB_IO_END (KERN_IO_START + FULL_IO_SIZE)
#define IOREMAP_BASE (PHB_IO_END)
#define IOREMAP_START (ioremap_bot)
-#define IOREMAP_END (KERN_VIRT_START + KERN_VIRT_SIZE)
+#define IOREMAP_END (KERN_VIRT_START + KERN_VIRT_SIZE - FIXADDR_SIZE)
+#define FIXADDR_SIZE SZ_32M
/*
--
2.25.0
^ permalink raw reply related
* [PATCH v2 2/2] powerpc/legacy_serial: Use early_ioremap()
From: Christophe Leroy @ 2021-04-20 13:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
chris.packham
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <0d51620eacf036d683d1a3c41328f69adb601dc0.1618925560.git.christophe.leroy@csgroup.eu>
From: Christophe Leroy <christophe.leroy@c-s.fr>
[ 0.000000] ioremap() called early from find_legacy_serial_ports+0x3cc/0x474. Use early_ioremap() instead
find_legacy_serial_ports() is called early from setup_arch(), before
paging_init(). vmalloc is not available yet, ioremap shouldn't be
used that early.
Use early_ioremap() and switch to a regular ioremap() later.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/legacy_serial.c | 33 +++++++++++++++++++++++++----
1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/legacy_serial.c b/arch/powerpc/kernel/legacy_serial.c
index f061e06e9f51..8b2c1a8553a0 100644
--- a/arch/powerpc/kernel/legacy_serial.c
+++ b/arch/powerpc/kernel/legacy_serial.c
@@ -15,6 +15,7 @@
#include <asm/udbg.h>
#include <asm/pci-bridge.h>
#include <asm/ppc-pci.h>
+#include <asm/early_ioremap.h>
#undef DEBUG
@@ -34,6 +35,7 @@ static struct legacy_serial_info {
unsigned int clock;
int irq_check_parent;
phys_addr_t taddr;
+ void __iomem *early_addr;
} legacy_serial_infos[MAX_LEGACY_SERIAL_PORTS];
static const struct of_device_id legacy_serial_parents[] __initconst = {
@@ -325,17 +327,16 @@ static void __init setup_legacy_serial_console(int console)
{
struct legacy_serial_info *info = &legacy_serial_infos[console];
struct plat_serial8250_port *port = &legacy_serial_ports[console];
- void __iomem *addr;
unsigned int stride;
stride = 1 << port->regshift;
/* Check if a translated MMIO address has been found */
if (info->taddr) {
- addr = ioremap(info->taddr, 0x1000);
- if (addr == NULL)
+ info->early_addr = early_ioremap(info->taddr, 0x1000);
+ if (info->early_addr == NULL)
return;
- udbg_uart_init_mmio(addr, stride);
+ udbg_uart_init_mmio(info->early_addr, stride);
} else {
/* Check if it's PIO and we support untranslated PIO */
if (port->iotype == UPIO_PORT && isa_io_special)
@@ -353,6 +354,30 @@ static void __init setup_legacy_serial_console(int console)
udbg_uart_setup(info->speed, info->clock);
}
+static int __init ioremap_legacy_serial_console(void)
+{
+ struct legacy_serial_info *info = &legacy_serial_infos[legacy_serial_console];
+ struct plat_serial8250_port *port = &legacy_serial_ports[legacy_serial_console];
+ void __iomem *vaddr;
+
+ if (legacy_serial_console < 0)
+ return 0;
+
+ if (!info->early_addr)
+ return 0;
+
+ vaddr = ioremap(info->taddr, 0x1000);
+ if (WARN_ON(!vaddr))
+ return -ENOMEM;
+
+ udbg_uart_init_mmio(vaddr, 1 << port->regshift);
+ early_iounmap(info->early_addr, 0x1000);
+ info->early_addr = NULL;
+
+ return 0;
+}
+early_initcall(ioremap_legacy_serial_console);
+
/*
* This is called very early, as part of setup_system() or eventually
* setup_arch(), basically before anything else in this file. This function
--
2.25.0
^ permalink raw reply related
* Re: PPC_FPU, ALTIVEC: enable_kernel_fp, put_vr, get_vr
From: Michael Ellerman @ 2021-04-20 13:15 UTC (permalink / raw)
To: Christophe Leroy, Randy Dunlap, Segher Boessenkool; +Cc: PowerPC, LKML
In-Reply-To: <1f337b4c-940e-110c-d0a2-2ad95cfb2dc8@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 19/04/2021 à 23:39, Randy Dunlap a écrit :
>> On 4/19/21 6:16 AM, Michael Ellerman wrote:
>>> Randy Dunlap <rdunlap@infradead.org> writes:
>>
>>>> Sure. I'll post them later today.
>>>> They keep FPU and ALTIVEC as independent (build) features.
>>>
>>> Those patches look OK.
>>>
>>> But I don't think it makes sense to support that configuration, FPU=n
>>> ALTVEC=y. No one is ever going to make a CPU like that. We have enough
>>> testing surface due to configuration options, without adding artificial
>>> combinations that no one is ever going to use.
>>>
>>> IMHO :)
>>>
>>> So I'd rather we just make ALTIVEC depend on FPU.
>>
>> That's rather simple. See below.
>> I'm doing a bunch of randconfig builds with it now.
>>
>> ---
>> From: Randy Dunlap <rdunlap@infradead.org>
>> Subject: [PATCH] powerpc: make ALTIVEC depend PPC_FPU
>>
>> On a kernel config with ALTIVEC=y and PPC_FPU not set/enabled,
>> there are build errors:
>>
>> drivers/cpufreq/pmac32-cpufreq.c:262:2: error: implicit declaration of function 'enable_kernel_fp' [-Werror,-Wimplicit-function-declaration]
>> enable_kernel_fp();
>> ../arch/powerpc/lib/sstep.c: In function 'do_vec_load':
>> ../arch/powerpc/lib/sstep.c:637:3: error: implicit declaration of function 'put_vr' [-Werror=implicit-function-declaration]
>> 637 | put_vr(rn, &u.v);
>> | ^~~~~~
>> ../arch/powerpc/lib/sstep.c: In function 'do_vec_store':
>> ../arch/powerpc/lib/sstep.c:660:3: error: implicit declaration of function 'get_vr'; did you mean 'get_oc'? [-Werror=implicit-function-declaration]
>> 660 | get_vr(rn, &u.v);
>> | ^~~~~~
>>
>> In theory ALTIVEC is independent of PPC_FPU but in practice nobody
>> is going to build such a machine, so make ALTIVEC require PPC_FPU
>> by depending on PPC_FPU.
>>
>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Cc: Segher Boessenkool <segher@kernel.crashing.org>
>> Cc: lkp@intel.com
>> ---
>> arch/powerpc/platforms/86xx/Kconfig | 1 +
>> arch/powerpc/platforms/Kconfig.cputype | 2 ++
>> 2 files changed, 3 insertions(+)
>>
>> --- linux-next-20210416.orig/arch/powerpc/platforms/86xx/Kconfig
>> +++ linux-next-20210416/arch/powerpc/platforms/86xx/Kconfig
>> @@ -4,6 +4,7 @@ menuconfig PPC_86xx
>> bool "86xx-based boards"
>> depends on PPC_BOOK3S_32
>> select FSL_SOC
>> + select PPC_FPU
>> select ALTIVEC
>> help
>> The Freescale E600 SoCs have 74xx cores.
>> --- linux-next-20210416.orig/arch/powerpc/platforms/Kconfig.cputype
>> +++ linux-next-20210416/arch/powerpc/platforms/Kconfig.cputype
>> @@ -186,6 +186,7 @@ config E300C3_CPU
>> config G4_CPU
>> bool "G4 (74xx)"
>> depends on PPC_BOOK3S_32
>> + select PPC_FPU
>> select ALTIVEC
>>
>> endchoice
>> @@ -309,6 +310,7 @@ config PHYS_64BIT
>>
>> config ALTIVEC
>> bool "AltiVec Support"
>> + depends on PPC_FPU
>
> Shouldn't we do it the other way round ? In extenso make ALTIVEC select PPC_FPU and avoid the two
> selects that are above ?
Yes, ALTIVEC should select PPC_FPU.
The latter is (generally) not user selectable, so there's no issue with
selecting it, whereas the reverse is not true.
For 64-bit Book3S I think we could just always enable ALTIVEC these
days. It's only Power5 that doesn't have it, and essentially no one is
running mainline on those AFAIK. But that can be done separately.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: Initialize local variable fdt to NULL in elf64_load()
From: Rob Herring @ 2021-04-20 13:06 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: devicetree, kbuild-all, kbuild test robot, Thiago Jung Bauermann,
linuxppc-dev, Dan Carpenter, Daniel Axtens
In-Reply-To: <b84c76d6-2be8-77a4-3c0f-ad8657c0e508@linux.microsoft.com>
On Tue, Apr 20, 2021 at 12:20 AM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> On 4/19/21 10:00 PM, Dan Carpenter wrote:
> > On Tue, Apr 20, 2021 at 09:30:16AM +1000, Michael Ellerman wrote:
> >> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> >>> On 4/16/21 2:05 AM, Michael Ellerman wrote:
> >>>
> >>>> Daniel Axtens <dja@axtens.net> writes:
> >>>>>> On 4/15/21 12:14 PM, Lakshmi Ramasubramanian wrote:
> >>>>>>
> >>>>>> Sorry - missed copying device-tree and powerpc mailing lists.
> >>>>>>
> >>>>>>> There are a few "goto out;" statements before the local variable "fdt"
> >>>>>>> is initialized through the call to of_kexec_alloc_and_setup_fdt() in
> >>>>>>> elf64_load(). This will result in an uninitialized "fdt" being passed
> >>>>>>> to kvfree() in this function if there is an error before the call to
> >>>>>>> of_kexec_alloc_and_setup_fdt().
> >>>>>>>
> >>>>>>> Initialize the local variable "fdt" to NULL.
> >>>>>>>
> >>>>> I'm a huge fan of initialising local variables! But I'm struggling to
> >>>>> find the code path that will lead to an uninit fdt being returned...
> >>>>>
> >>>>> The out label reads in part:
> >>>>>
> >>>>> /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */
> >>>>> return ret ? ERR_PTR(ret) : fdt;
> >>>>>
> >>>>> As far as I can tell, any time we get a non-zero ret, we're going to
> >>>>> return an error pointer rather than the uninitialised value...
> >>>
> >>> As Dan pointed out, the new code is in linux-next.
> >>>
> >>> I have copied the new one below - the function doesn't return fdt, but
> >>> instead sets it in the arch specific field (please see the link to the
> >>> updated elf_64.c below).
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/tree/arch/powerpc/kexec/elf_64.c?h=for-next
> >>>
> >>>>>
> >>>>> (btw, it does look like we might leak fdt if we have an error after we
> >>>>> successfully kmalloc it.)
> >>>>>
> >>>>> Am I missing something? Can you link to the report for the kernel test
> >>>>> robot or from Dan?
> >>>
> >>> /*
> >>> * Once FDT buffer has been successfully passed to
> >>> kexec_add_buffer(),
> >>> * the FDT buffer address is saved in image->arch.fdt. In that
> >>> case,
> >>> * the memory cannot be freed here in case of any other error.
> >>> */
> >>> if (ret && !image->arch.fdt)
> >>> kvfree(fdt);
> >>>
> >>> return ret ? ERR_PTR(ret) : NULL;
> >>>
> >>> In case of an error, the memory allocated for fdt is freed unless it has
> >>> already been passed to kexec_add_buffer().
> >>
> >> It feels like the root of the problem is that the kvfree of fdt is in
> >> the wrong place. It's only allocated later in the function, so the error
> >> path should reflect that. Something like the patch below.
> >>
> >> cheers
> >>
> >>
> >> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> >> index 5a569bb51349..02662e72c53d 100644
> >> --- a/arch/powerpc/kexec/elf_64.c
> >> +++ b/arch/powerpc/kexec/elf_64.c
> >> @@ -114,7 +114,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> >> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr,
> >> initrd_len, cmdline);
> >> if (ret)
> >> - goto out;
> >> + goto out_free_fdt;
> >>
> >> fdt_pack(fdt);
> >>
> >> @@ -125,7 +125,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> >> kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;
> >> ret = kexec_add_buffer(&kbuf);
> >> if (ret)
> >> - goto out;
> >> + goto out_free_fdt;
> >>
> >> /* FDT will be freed in arch_kimage_file_post_load_cleanup */
> >> image->arch.fdt = fdt;
> >> @@ -140,18 +140,14 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
> >> if (ret)
> >> pr_err("Error setting up the purgatory.\n");
> >>
> >> + goto out;
> >
> > This will leak. It would need to be something like:
> >
> > if (ret) {
> > pr_err("Error setting up the purgatory.\n");
> > goto out_free_fdt;
> > }
> Once "fdt" buffer is successfully passed to kexec_add_buffer() it cannot
> be freed here - it will be freed when the kexec cleanup function is called.
That may be the case currently, but really if a function returns an
error it should have undone anything it did like memory allocations. I
don't think you should do that to fix this issue, but it would be a
good clean-up.
Rob
^ permalink raw reply
* Re: [PATCH] powerpc/mce: save ignore_event flag unconditionally for UE
From: Santosh Sivaraj @ 2021-04-20 11:35 UTC (permalink / raw)
To: Ganesh, linuxppc-dev, mpe; +Cc: mahesh, npiggin
In-Reply-To: <9b8c7347-47fe-822d-7fae-9365bb7cde7c@linux.ibm.com>
Ganesh <ganeshgr@linux.ibm.com> writes:
> On 4/20/21 12:54 PM, Santosh Sivaraj wrote:
>
>> Hi Ganesh,
>>
>> Ganesh Goudar <ganeshgr@linux.ibm.com> writes:
>>
>>> When we hit an UE while using machine check safe copy routines,
>>> ignore_event flag is set and the event is ignored by mce handler,
>>> And the flag is also saved for defered handling and printing of
>>> mce event information, But as of now saving of this flag is done
>>> on checking if the effective address is provided or physical address
>>> is calculated, which is not right.
>>>
>>> Save ignore_event flag regardless of whether the effective address is
>>> provided or physical address is calculated.
>>>
>>> Without this change following log is seen, when the event is to be
>>> ignored.
>>>
>>> [ 512.971365] MCE: CPU1: machine check (Severe) UE Load/Store [Recovered]
>>> [ 512.971509] MCE: CPU1: NIP: [c0000000000b67c0] memcpy+0x40/0x90
>>> [ 512.971655] MCE: CPU1: Initiator CPU
>>> [ 512.971739] MCE: CPU1: Unknown
>>> [ 512.972209] MCE: CPU1: machine check (Severe) UE Load/Store [Recovered]
>>> [ 512.972334] MCE: CPU1: NIP: [c0000000000b6808] memcpy+0x88/0x90
>>> [ 512.972456] MCE: CPU1: Initiator CPU
>>> [ 512.972534] MCE: CPU1: Unknown
>>>
>>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>>> ---
>>> arch/powerpc/kernel/mce.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
>>> index 11f0cae086ed..db9363e131ce 100644
>>> --- a/arch/powerpc/kernel/mce.c
>>> +++ b/arch/powerpc/kernel/mce.c
>>> @@ -131,6 +131,8 @@ void save_mce_event(struct pt_regs *regs, long handled,
>>> * Populate the mce error_type and type-specific error_type.
>>> */
>>> mce_set_error_info(mce, mce_err);
>>> + if (mce->error_type == MCE_ERROR_TYPE_UE)
>>> + mce->u.ue_error.ignore_event = mce_err->ignore_event;
>>>
>>> if (!addr)
>>> return;
>>> @@ -159,7 +161,6 @@ void save_mce_event(struct pt_regs *regs, long handled,
>>> if (phys_addr != ULONG_MAX) {
>>> mce->u.ue_error.physical_address_provided = true;
>>> mce->u.ue_error.physical_address = phys_addr;
>>> - mce->u.ue_error.ignore_event = mce_err->ignore_event;
>>> machine_check_ue_event(mce);
>>> }
>>> }
>> Small nit:
>> Setting ignore event can happen before the phys_addr check, under the existing
>> check for MCE_ERROR_TYPE_UE, instead of repeating the same condition again.
>
> In some cases we may not get effective address also, so it is placed before
> effective address check.
Yes, I forgot the last two lines in the changelog after I applied the patch :-)
Thanks,
Santosh
>
>>
>> Except for the above nit
>>
>> Reviewed-by: Santosh Sivaraj <santosh@fossix.org>
>>
>> Thanks,
>> Santosh
>>> --
>>> 2.26.2
^ 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