* Re: [PATCH v2 2/3] powerpc/fsl: set fsl, i2c-erratum-a004447 flag for P2041 i2c controllers
From: Joakim Tjernlund @ 2021-05-07 8:04 UTC (permalink / raw)
To: andy.shevchenko@gmail.com, andriy.shevchenko@linux.intel.com,
mpe@ellerman.id.au, chris.packham@alliedtelesis.co.nz,
wsa@kernel.org, robh+dt@kernel.org
Cc: devicetree@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, paulus@samba.org,
linux-i2c@vger.kernel.org
In-Reply-To: <20210507004047.4454-3-chris.packham@alliedtelesis.co.nz>
On Fri, 2021-05-07 at 12:40 +1200, Chris Packham wrote:
> The i2c controllers on the P2040/P2041 have an erratum where the
> documented scheme for i2c bus recovery will not work (A-004447). A
> different mechanism is needed which is documented in the P2040 Chip
> Errata Rev Q (latest available at the time of writing).
From what I can tell this Erratum also applies to P1010
Jocke
^ permalink raw reply
* Re: [PATCH v2 2/3] powerpc/fsl: set fsl, i2c-erratum-a004447 flag for P2041 i2c controllers
From: Joakim Tjernlund @ 2021-05-07 8:24 UTC (permalink / raw)
To: andy.shevchenko@gmail.com, andriy.shevchenko@linux.intel.com,
mpe@ellerman.id.au, chris.packham@alliedtelesis.co.nz,
wsa@kernel.org, robh+dt@kernel.org
Cc: devicetree@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, paulus@samba.org,
linux-i2c@vger.kernel.org
In-Reply-To: <5e625652e3f980b7cd9fb86fe3d66f19a2f4201d.camel@infinera.com>
On Fri, 2021-05-07 at 10:04 +0200, Joakim Tjernlund wrote:
> On Fri, 2021-05-07 at 12:40 +1200, Chris Packham wrote:
> > The i2c controllers on the P2040/P2041 have an erratum where the
> > documented scheme for i2c bus recovery will not work (A-004447). A
> > different mechanism is needed which is documented in the P2040 Chip
> > Errata Rev Q (latest available at the time of writing).
>
> From what I can tell this Erratum also applies to P1010
>
> Jocke
Reference: https://media.digikey.com/pdf/PCNs/Freescale/P1010CE_RevL.pdf
Also, I think this series should go to stable.
Jocke
^ permalink raw reply
* Re: UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:359:56
From: Christophe Leroy @ 2021-05-07 8:31 UTC (permalink / raw)
To: Paul Menzel, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras
Cc: linuxppc-dev
In-Reply-To: <a7467e08-1e53-e974-d14b-13e6ff558dae@molgen.mpg.de>
Hi Paul,
Le 06/05/2021 à 21:32, Paul Menzel a écrit :
> [corrected subject]
>
> Am 06.05.21 um 21:31 schrieb Paul Menzel:
>> Dear Linux folks,
>>
>>
>> On the POWER8 system IBM S822LC, Linux 5.13+, built with USSAN, logs the warning below.
>>
>> ```
>> [ 0.030091] ================================================================================
>> [ 0.030295] UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:359:56
>> [ 0.030325] index -1 is out of range for type 'legacy_serial_info [8]'
>> [ 0.030350] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0+ #2
>> [ 0.030360] Call Trace:
>> [ 0.030363] [c000000024f1bad0] [c0000000009f4330] dump_stack+0xc4/0x114 (unreliable)
>> [ 0.030386] [c000000024f1bb20] [c0000000009efed0] ubsan_epilogue+0x18/0x78
>> [ 0.030400] [c000000024f1bb80] [c0000000009efafc] __ubsan_handle_out_of_bounds+0xac/0xd0
>> [ 0.030414] [c000000024f1bc20] [c000000001711588] ioremap_legacy_serial_console+0x54/0x144
>> [ 0.030430] [c000000024f1bc70] [c0000000000123c0] do_one_initcall+0x60/0x2c0
>> [ 0.030444] [c000000024f1bd40] [c000000001704bc4] kernel_init_freeable+0x19c/0x25c
>> [ 0.030458] [c000000024f1bda0] [c000000000012a2c] kernel_init+0x2c/0x180
>> [ 0.030471] [c000000024f1be10] [c00000000000d6ec] ret_from_kernel_thread+0x5c/0x70
>> [ 0.030484] ================================================================================
>> [ 0.030641] ================================================================================
>> [ 0.030668] UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:360:58
>> [ 0.030697] index -1 is out of range for type 'plat_serial8250_port [9]'
>> [ 0.030721] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0+ #2
>> [ 0.030730] Call Trace:
>> [ 0.030733] [c000000024f1bad0] [c0000000009f4330] dump_stack+0xc4/0x114 (unreliable)
>> [ 0.030749] [c000000024f1bb20] [c0000000009efed0] ubsan_epilogue+0x18/0x78
>> [ 0.030762] [c000000024f1bb80] [c0000000009efafc] __ubsan_handle_out_of_bounds+0xac/0xd0
>> [ 0.030775] [c000000024f1bc20] [c0000000017115a0] ioremap_legacy_serial_console+0x6c/0x144
>> [ 0.030790] [c000000024f1bc70] [c0000000000123c0] do_one_initcall+0x60/0x2c0
>> [ 0.030802] [c000000024f1bd40] [c000000001704bc4] kernel_init_freeable+0x19c/0x25c
>> [ 0.030816] [c000000024f1bda0] [c000000000012a2c] kernel_init+0x2c/0x180
>> [ 0.030829] [c000000024f1be10] [c00000000000d6ec] ret_from_kernel_thread+0x5c/0x70
>> [ 0.030842] ================================================================================ ```
>>
>>
The function is as follows, so when legacy_serial_console == -1 as in your situation, the pointers
are just not used.
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;
...
}
When I look into the generated code (UBSAN not selected), we see the verification and the bail-out
is done prior to any calculation based on legacy_serial_console.
00000000 <ioremap_legacy_serial_console>:
0: 94 21 ff e0 stwu r1,-32(r1)
4: 3d 20 00 00 lis r9,0
6: R_PPC_ADDR16_HA .data
8: 7c 08 02 a6 mflr r0
c: bf 81 00 10 stmw r28,16(r1)
10: 3b 80 00 00 li r28,0
14: 83 a9 00 00 lwz r29,0(r9)
16: R_PPC_ADDR16_LO .data
18: 90 01 00 24 stw r0,36(r1)
1c: 2c 1d 00 00 cmpwi r29,0
20: 41 80 00 80 blt a0 <ioremap_legacy_serial_console+0xa0>
So, is it normal that UBSAN reports an error here ?
Thanks
Christophe
^ permalink raw reply
* Re: UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:359:56
From: Paul Menzel @ 2021-05-07 8:42 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras
Cc: Andrey Ryabinin, linuxppc-dev
In-Reply-To: <3b1febc1-3635-fb5f-1ff3-6f9f0c8feb12@csgroup.eu>
[+Andrey]
Dear Christophe,
Am 07.05.21 um 10:31 schrieb Christophe Leroy:
> Le 06/05/2021 à 21:32, Paul Menzel a écrit :
>> [corrected subject]
>>
>> Am 06.05.21 um 21:31 schrieb Paul Menzel:
>>> On the POWER8 system IBM S822LC, Linux 5.13+, built with USSAN, logs
>>> the warning below.
>>>
>>> ```
>>> [ 0.030091]
>>> ================================================================================
>>> [ 0.030295] UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:359:56
>>> [ 0.030325] index -1 is out of range for type 'legacy_serial_info [8]'
>>> [ 0.030350] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0+ #2
>>> [ 0.030360] Call Trace:
>>> [ 0.030363] [c000000024f1bad0] [c0000000009f4330] dump_stack+0xc4/0x114 (unreliable)
>>> [ 0.030386] [c000000024f1bb20] [c0000000009efed0] ubsan_epilogue+0x18/0x78
>>> [ 0.030400] [c000000024f1bb80] [c0000000009efafc] __ubsan_handle_out_of_bounds+0xac/0xd0
>>> [ 0.030414] [c000000024f1bc20] [c000000001711588] ioremap_legacy_serial_console+0x54/0x144
>>> [ 0.030430] [c000000024f1bc70] [c0000000000123c0] do_one_initcall+0x60/0x2c0
>>> [ 0.030444] [c000000024f1bd40] [c000000001704bc4] kernel_init_freeable+0x19c/0x25c
>>> [ 0.030458] [c000000024f1bda0] [c000000000012a2c] kernel_init+0x2c/0x180
>>> [ 0.030471] [c000000024f1be10] [c00000000000d6ec] ret_from_kernel_thread+0x5c/0x70
>>> [ 0.030484] ================================================================================
>>>
>>> [ 0.030641] ================================================================================
>>> [ 0.030668] UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:360:58
>>> [ 0.030697] index -1 is out of range for type 'plat_serial8250_port [9]'
>>> [ 0.030721] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0+ #2
>>> [ 0.030730] Call Trace:
>>> [ 0.030733] [c000000024f1bad0] [c0000000009f4330] dump_stack+0xc4/0x114 (unreliable)
>>> [ 0.030749] [c000000024f1bb20] [c0000000009efed0] ubsan_epilogue+0x18/0x78
>>> [ 0.030762] [c000000024f1bb80] [c0000000009efafc] __ubsan_handle_out_of_bounds+0xac/0xd0
>>> [ 0.030775] [c000000024f1bc20] [c0000000017115a0] ioremap_legacy_serial_console+0x6c/0x144
>>> [ 0.030790] [c000000024f1bc70] [c0000000000123c0] do_one_initcall+0x60/0x2c0
>>> [ 0.030802] [c000000024f1bd40] [c000000001704bc4] kernel_init_freeable+0x19c/0x25c
>>> [ 0.030816] [c000000024f1bda0] [c000000000012a2c] kernel_init+0x2c/0x180
>>> [ 0.030829] [c000000024f1be10] [c00000000000d6ec] ret_from_kernel_thread+0x5c/0x70
>>> [ 0.030842] ================================================================================
>>> ```
>
> The function is as follows, so when legacy_serial_console == -1 as in
> your situation, the pointers are just not used.
>
> 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;
>
> ...
> }
>
> When I look into the generated code (UBSAN not selected), we see the
> verification and the bail-out is done prior to any calculation based on
> legacy_serial_console.
>
> 00000000 <ioremap_legacy_serial_console>:
> 0: 94 21 ff e0 stwu r1,-32(r1)
> 4: 3d 20 00 00 lis r9,0
> 6: R_PPC_ADDR16_HA .data
> 8: 7c 08 02 a6 mflr r0
> c: bf 81 00 10 stmw r28,16(r1)
> 10: 3b 80 00 00 li r28,0
> 14: 83 a9 00 00 lwz r29,0(r9)
> 16: R_PPC_ADDR16_LO .data
> 18: 90 01 00 24 stw r0,36(r1)
> 1c: 2c 1d 00 00 cmpwi r29,0
> 20: 41 80 00 80 blt a0 <ioremap_legacy_serial_console+0xa0>
>
> So, is it normal that UBSAN reports an error here ?
If it’s useful, I could disassemble the code here. But please tell me how.
Sorry, I do not know. I just selected the option, and saw the error.
Maybe Andrey has an idea.
Kind regards,
Paul
^ permalink raw reply
* Re: UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:359:56
From: Christophe Leroy @ 2021-05-07 8:59 UTC (permalink / raw)
To: Paul Menzel, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras
Cc: Andrey Ryabinin, linuxppc-dev
In-Reply-To: <64a8b6fd-5fee-c0bf-7f8f-946dbe6b7973@molgen.mpg.de>
Le 07/05/2021 à 10:42, Paul Menzel a écrit :
> [+Andrey]
>
> Dear Christophe,
>
>
> Am 07.05.21 um 10:31 schrieb Christophe Leroy:
>
>> Le 06/05/2021 à 21:32, Paul Menzel a écrit :
>>> [corrected subject]
>>>
>>> Am 06.05.21 um 21:31 schrieb Paul Menzel:
>
>>>> On the POWER8 system IBM S822LC, Linux 5.13+, built with USSAN, logs the warning below.
>>>>
>>>> ```
>>>> [ 0.030091] ================================================================================
>>>> [ 0.030295] UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:359:56
>>>> [ 0.030325] index -1 is out of range for type 'legacy_serial_info [8]'
>>>> [ 0.030350] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0+ #2
>>>> [ 0.030360] Call Trace:
>>>> [ 0.030363] [c000000024f1bad0] [c0000000009f4330] dump_stack+0xc4/0x114 (unreliable)
>>>> [ 0.030386] [c000000024f1bb20] [c0000000009efed0] ubsan_epilogue+0x18/0x78
>>>> [ 0.030400] [c000000024f1bb80] [c0000000009efafc] __ubsan_handle_out_of_bounds+0xac/0xd0
>>>> [ 0.030414] [c000000024f1bc20] [c000000001711588] ioremap_legacy_serial_console+0x54/0x144
>>>> [ 0.030430] [c000000024f1bc70] [c0000000000123c0] do_one_initcall+0x60/0x2c0
>>>> [ 0.030444] [c000000024f1bd40] [c000000001704bc4] kernel_init_freeable+0x19c/0x25c
>>>> [ 0.030458] [c000000024f1bda0] [c000000000012a2c] kernel_init+0x2c/0x180
>>>> [ 0.030471] [c000000024f1be10] [c00000000000d6ec] ret_from_kernel_thread+0x5c/0x70
>>>> [ 0.030484] ================================================================================
>>>> [ 0.030641] ================================================================================
>>>> [ 0.030668] UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:360:58
>>>> [ 0.030697] index -1 is out of range for type 'plat_serial8250_port [9]'
>>>> [ 0.030721] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0+ #2
>>>> [ 0.030730] Call Trace:
>>>> [ 0.030733] [c000000024f1bad0] [c0000000009f4330] dump_stack+0xc4/0x114 (unreliable)
>>>> [ 0.030749] [c000000024f1bb20] [c0000000009efed0] ubsan_epilogue+0x18/0x78
>>>> [ 0.030762] [c000000024f1bb80] [c0000000009efafc] __ubsan_handle_out_of_bounds+0xac/0xd0
>>>> [ 0.030775] [c000000024f1bc20] [c0000000017115a0] ioremap_legacy_serial_console+0x6c/0x144
>>>> [ 0.030790] [c000000024f1bc70] [c0000000000123c0] do_one_initcall+0x60/0x2c0
>>>> [ 0.030802] [c000000024f1bd40] [c000000001704bc4] kernel_init_freeable+0x19c/0x25c
>>>> [ 0.030816] [c000000024f1bda0] [c000000000012a2c] kernel_init+0x2c/0x180
>>>> [ 0.030829] [c000000024f1be10] [c00000000000d6ec] ret_from_kernel_thread+0x5c/0x70
>>>> [ 0.030842] ================================================================================
>>>> ```
>>
>> The function is as follows, so when legacy_serial_console == -1 as in your situation, the pointers
>> are just not used.
>>
>> 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;
>>
>> ...
>> }
>>
>> When I look into the generated code (UBSAN not selected), we see the verification and the bail-out
>> is done prior to any calculation based on legacy_serial_console.
>>
>> 00000000 <ioremap_legacy_serial_console>:
>> 0: 94 21 ff e0 stwu r1,-32(r1)
>> 4: 3d 20 00 00 lis r9,0
>> 6: R_PPC_ADDR16_HA .data
>> 8: 7c 08 02 a6 mflr r0
>> c: bf 81 00 10 stmw r28,16(r1)
>> 10: 3b 80 00 00 li r28,0
>> 14: 83 a9 00 00 lwz r29,0(r9)
>> 16: R_PPC_ADDR16_LO .data
>> 18: 90 01 00 24 stw r0,36(r1)
>> 1c: 2c 1d 00 00 cmpwi r29,0
>> 20: 41 80 00 80 blt a0 <ioremap_legacy_serial_console+0xa0>
>>
>> So, is it normal that UBSAN reports an error here ?
>
> If it’s useful, I could disassemble the code here. But please tell me how.
>
> Sorry, I do not know. I just selected the option, and saw the error. Maybe Andrey has an idea.
>
No need for you to disassemble, I just wanted to show that without UBSAN there is no problem with
the index as it is used only after boundary checking. (But if you want to do so, if is just an
'objdump -dr legacy_serial.o')
Now, with UBSAN, I see that UBSAN does the verification of the index earlier than expected. So what
to do here, we can modify the code, but that modification would just be to make UBSAN happy as there
is no problem in itself.
Christophe
^ permalink raw reply
* [PATCH] powerpc: Define NR_CPUS all the time
From: Christophe Leroy @ 2021-05-07 9:24 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
include/linux/threads.h defines a default value for CONFIG_NR_CPUS
but suggests the architectures should always define it.
So do it, when CONFIG_SMP is not selected set CONFIG_NR_CPUS to 1.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/platforms/Kconfig.cputype | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index f998e655b570..885140055b7a 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -476,9 +476,9 @@ config SMP
If you don't know what to do here, say N.
config NR_CPUS
- int "Maximum number of CPUs (2-8192)"
- range 2 8192
- depends on SMP
+ int "Maximum number of CPUs (2-8192)" if SMP
+ range 2 8192 if SMP
+ default "1" if !SMP
default "32" if PPC64
default "4"
--
2.25.0
^ permalink raw reply related
* [PATCH] powerpc/xive: Fix unsigned comparison with less than zero
From: Jiapeng Chong @ 2021-05-07 10:20 UTC (permalink / raw)
To: mpe; +Cc: Jiapeng Chong, paulus, linuxppc-dev, linux-kernel
The return from the call to irq_domain_alloc_irqs() is int, it can be
a negative error code, however this is being assigned to an unsigned
int variable 'irq', this may lead to invalid if statement detection,
so that kfree execution is invalid. So making 'irq' an int.
Clean up the following coccicheck warning:
./arch/powerpc/sysdev/xive/common.c:1157:6-14: WARNING: Unsigned
expression compared with zero: xid -> irq < 0.
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
arch/powerpc/sysdev/xive/common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
index a830432..a5fd45a 100644
--- a/arch/powerpc/sysdev/xive/common.c
+++ b/arch/powerpc/sysdev/xive/common.c
@@ -65,7 +65,7 @@
#ifdef CONFIG_SMP
/* The IPIs use the same logical irq number when on the same chip */
static struct xive_ipi_desc {
- unsigned int irq;
+ int irq;
char name[16];
} *xive_ipis;
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v7 4/6] KVM: PPC: Book3S HV: Nested support in H_RPT_INVALIDATE
From: Aneesh Kumar K.V @ 2021-05-07 10:30 UTC (permalink / raw)
To: Nicholas Piggin, Bharata B Rao, kvm-ppc, linuxppc-dev; +Cc: david, farosas
In-Reply-To: <1620280840.cytifux60q.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
...
> + */
>> +long do_h_rpt_invalidate_pat(struct kvm_vcpu *vcpu, unsigned long lpid,
>> + unsigned long type, unsigned long pg_sizes,
>> + unsigned long start, unsigned long end)
>> +{
>> + struct kvm_nested_guest *gp;
>> + long ret;
>> + unsigned long psize, ap;
>> +
>> + /*
>> + * If L2 lpid isn't valid, we need to return H_PARAMETER.
>> + *
>> + * However, nested KVM issues a L2 lpid flush call when creating
>> + * partition table entries for L2. This happens even before the
>> + * corresponding shadow lpid is created in HV which happens in
>> + * H_ENTER_NESTED call. Since we can't differentiate this case from
>> + * the invalid case, we ignore such flush requests and return success.
>> + */
>> + gp = kvmhv_find_nested(vcpu->kvm, lpid);
>> + if (!gp)
>> + return H_SUCCESS;
>> +
>> + /*
>> + * A flush all request can be handled by a full lpid flush only.
>> + */
>> + if ((type & H_RPTI_TYPE_NESTED_ALL) == H_RPTI_TYPE_NESTED_ALL)
>> + return do_tlb_invalidate_nested_all(vcpu, lpid, RIC_FLUSH_ALL);
>> +
>> +#if 0
>> + /*
>> + * We don't need to handle a PWC flush like process table here,
>> + * because intermediate partition scoped table in nested guest doesn't
>> + * really have PWC. Only level we have PWC is in L0 and for nested
>> + * invalidate at L0 we always do kvm_flush_lpid() which does
>> + * radix__flush_all_lpid(). For range invalidate at any level, we
>> + * are not removing the higher level page tables and hence there is
>> + * no PWC invalidate needed.
>> + */
>> + if (type & H_RPTI_TYPE_PWC) {
>> + ret = do_tlb_invalidate_nested_all(vcpu, lpid, RIC_FLUSH_PWC);
>> + if (ret)
>> + return H_P4;
>> + }
>> +#endif
>
> I think removing this #if 0 and the unnecessary code is fine, just a bit
> more explanation in the comment would help. And "doesn't really" implies
> it sort of might a little bit, I think what you want is "really doesn't"
> :)
yes.
>
> As I understand it, the L0 does not cache any intermediate levels of the
> nested guest's partition scope at all. Only the nested HV's pte entries
> are copied into the shadow page table, so we only care if the PTEs are
> changed, and the PWCs that the processor creates for the shadow page
> table are managed by the kvmppc_unmap_pte() etc functions... I think?
That is correct. The reason I added the comment there was to clarify why
the PWC type is not handled in case of partition scoped invalidate
similar to process scoped invalidate. The code fragment was left as an
indication of what should happen theoretically.
All higher levels of guest (L1, L2.. etc) have partition tables that are
not really used for hardware page table walk. H_RPT_INVALIDATE hcall is used as
a hint to free those page table entries. L0 on receiving the hcall will
forward the same to higher levels guest which after invalidating its
shadow pte will further issue the H_RPT_INVALIDATE hcall to clear
parition scoped entries of the current guest.
If it is a range TLB flush, we just clear the shadow pte, higher levels
page tables are not modified and hence no PWC flush is required.
If it is full lpid flush because of RIC=1/2 or because range is 0 -> -1
we do free the full partition table and does a kvmhv_flush_lpid()
which will eventually ends up calling radix__flush_all_lpid().
These function names are kept in the comment so that a new person
looking at the code can easily follow the code path.
-aneesh
^ permalink raw reply
* [PATCH v2] powerpc/papr_scm: Make 'perf_stats' invisible if perf-stats unavailable
From: Vaibhav Jain @ 2021-05-07 11:39 UTC (permalink / raw)
To: linuxppc-dev, linux-nvdimm
Cc: Santosh Sivaraj, Aneesh Kumar K . V, Vaibhav Jain, Dan Williams,
Ira Weiny
In case performance stats for an nvdimm are not available, reading the
'perf_stats' sysfs file returns an -ENOENT error. A better approach is
to make the 'perf_stats' file entirely invisible to indicate that
performance stats for an nvdimm are unavailable.
So this patch updates 'papr_nd_attribute_group' to add a 'is_visible'
callback implemented as newly introduced 'papr_nd_attribute_visible()'
that returns an appropriate mode in case performance stats aren't
supported in a given nvdimm.
Also the initialization of 'papr_scm_priv.stat_buffer_len' is moved
from papr_scm_nvdimm_init() to papr_scm_probe() so that it value is
available when 'papr_nd_attribute_visible()' is called during nvdimm
initialization.
Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Changelog:
v2:
* Removed a redundant dev_info() from pap_scm_nvdimm_init() [ Aneesh ]
* Marked papr_nd_attribute_visible() as static which also fixed the
build warning reported by kernel build robot
---
arch/powerpc/platforms/pseries/papr_scm.c | 35 ++++++++++++++++-------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index e2b69cc3beaf..11e7b90a3360 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -907,6 +907,20 @@ static ssize_t flags_show(struct device *dev,
}
DEVICE_ATTR_RO(flags);
+static umode_t papr_nd_attribute_visible(struct kobject *kobj,
+ struct attribute *attr, int n)
+{
+ struct device *dev = container_of(kobj, typeof(*dev), kobj);
+ struct nvdimm *nvdimm = to_nvdimm(dev);
+ struct papr_scm_priv *p = nvdimm_provider_data(nvdimm);
+
+ /* For if perf-stats not available remove perf_stats sysfs */
+ if (attr == &dev_attr_perf_stats.attr && p->stat_buffer_len == 0)
+ return 0;
+
+ return attr->mode;
+}
+
/* papr_scm specific dimm attributes */
static struct attribute *papr_nd_attributes[] = {
&dev_attr_flags.attr,
@@ -916,6 +930,7 @@ static struct attribute *papr_nd_attributes[] = {
static struct attribute_group papr_nd_attribute_group = {
.name = "papr",
+ .is_visible = papr_nd_attribute_visible,
.attrs = papr_nd_attributes,
};
@@ -931,7 +946,6 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
struct nd_region_desc ndr_desc;
unsigned long dimm_flags;
int target_nid, online_nid;
- ssize_t stat_size;
p->bus_desc.ndctl = papr_scm_ndctl;
p->bus_desc.module = THIS_MODULE;
@@ -1016,16 +1030,6 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
list_add_tail(&p->region_list, &papr_nd_regions);
mutex_unlock(&papr_ndr_lock);
- /* Try retriving the stat buffer and see if its supported */
- stat_size = drc_pmem_query_stats(p, NULL, 0);
- if (stat_size > 0) {
- p->stat_buffer_len = stat_size;
- dev_dbg(&p->pdev->dev, "Max perf-stat size %lu-bytes\n",
- p->stat_buffer_len);
- } else {
- dev_info(&p->pdev->dev, "Dimm performance stats unavailable\n");
- }
-
return 0;
err: nvdimm_bus_unregister(p->bus);
@@ -1102,6 +1106,7 @@ static int papr_scm_probe(struct platform_device *pdev)
u64 blocks, block_size;
struct papr_scm_priv *p;
const char *uuid_str;
+ ssize_t stat_size;
u64 uuid[2];
int rc;
@@ -1179,6 +1184,14 @@ static int papr_scm_probe(struct platform_device *pdev)
p->res.name = pdev->name;
p->res.flags = IORESOURCE_MEM;
+ /* Try retriving the stat buffer and see if its supported */
+ stat_size = drc_pmem_query_stats(p, NULL, 0);
+ if (stat_size > 0) {
+ p->stat_buffer_len = stat_size;
+ dev_dbg(&p->pdev->dev, "Max perf-stat size %lu-bytes\n",
+ p->stat_buffer_len);
+ }
+
rc = papr_scm_nvdimm_init(p);
if (rc)
goto err2;
--
2.31.1
^ permalink raw reply related
* Re: [PATCH] powerpc/papr_scm: Make 'perf_stats' invisible if perf-stats unavailable
From: Vaibhav Jain @ 2021-05-07 11:40 UTC (permalink / raw)
To: Aneesh Kumar K.V, linuxppc-dev, linux-nvdimm
In-Reply-To: <8735v078v7.fsf@linux.ibm.com>
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>
>> In case performance stats for an nvdimm are not available, reading the
>> 'perf_stats' sysfs file returns an -ENOENT error. A better approach is
>> to make the 'perf_stats' file entirely invisible to indicate that
>> performance stats for an nvdimm are unavailable.
>>
>> So this patch updates 'papr_nd_attribute_group' to add a 'is_visible'
>> callback implemented as newly introduced 'papr_nd_attribute_visible()'
>> that returns an appropriate mode in case performance stats aren't
>> supported in a given nvdimm.
>>
>> Also the initialization of 'papr_scm_priv.stat_buffer_len' is moved
>> from papr_scm_nvdimm_init() to papr_scm_probe() so that it value is
>> available when 'papr_nd_attribute_visible()' is called during nvdimm
>> initialization.
>>
>> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> arch/powerpc/platforms/pseries/papr_scm.c | 37 ++++++++++++++++-------
>> 1 file changed, 26 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 12f1513f0fca..90f0af8fefe8 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -907,6 +907,20 @@ static ssize_t flags_show(struct device *dev,
>> }
>> DEVICE_ATTR_RO(flags);
>>
>> +umode_t papr_nd_attribute_visible(struct kobject *kobj, struct attribute *attr,
>> + int n)
>> +{
>> + struct device *dev = container_of(kobj, typeof(*dev), kobj);
>> + struct nvdimm *nvdimm = to_nvdimm(dev);
>> + struct papr_scm_priv *p = nvdimm_provider_data(nvdimm);
>> +
>> + /* For if perf-stats not available remove perf_stats sysfs */
>> + if (attr == &dev_attr_perf_stats.attr && p->stat_buffer_len == 0)
>> + return 0;
>> +
>> + return attr->mode;
>> +}
>> +
>> /* papr_scm specific dimm attributes */
>> static struct attribute *papr_nd_attributes[] = {
>> &dev_attr_flags.attr,
>> @@ -916,6 +930,7 @@ static struct attribute *papr_nd_attributes[] = {
>>
>> static struct attribute_group papr_nd_attribute_group = {
>> .name = "papr",
>> + .is_visible = papr_nd_attribute_visible,
>> .attrs = papr_nd_attributes,
>> };
>>
>> @@ -931,7 +946,6 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
>> struct nd_region_desc ndr_desc;
>> unsigned long dimm_flags;
>> int target_nid, online_nid;
>> - ssize_t stat_size;
>>
>> p->bus_desc.ndctl = papr_scm_ndctl;
>> p->bus_desc.module = THIS_MODULE;
>> @@ -1016,16 +1030,6 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
>> list_add_tail(&p->region_list, &papr_nd_regions);
>> mutex_unlock(&papr_ndr_lock);
>>
>> - /* Try retriving the stat buffer and see if its supported */
>> - stat_size = drc_pmem_query_stats(p, NULL, 0);
>> - if (stat_size > 0) {
>> - p->stat_buffer_len = stat_size;
>> - dev_dbg(&p->pdev->dev, "Max perf-stat size %lu-bytes\n",
>> - p->stat_buffer_len);
>> - } else {
>> - dev_info(&p->pdev->dev, "Dimm performance stats unavailable\n");
>> - }
>> -
>> return 0;
>>
>> err: nvdimm_bus_unregister(p->bus);
>> @@ -1102,6 +1106,7 @@ static int papr_scm_probe(struct platform_device *pdev)
>> u64 blocks, block_size;
>> struct papr_scm_priv *p;
>> const char *uuid_str;
>> + ssize_t stat_size;
>> u64 uuid[2];
>> int rc;
>>
>> @@ -1179,6 +1184,16 @@ static int papr_scm_probe(struct platform_device *pdev)
>> p->res.name = pdev->name;
>> p->res.flags = IORESOURCE_MEM;
>>
>> + /* Try retriving the stat buffer and see if its supported */
>> + stat_size = drc_pmem_query_stats(p, NULL, 0);
>> + if (stat_size > 0) {
>> + p->stat_buffer_len = stat_size;
>> + dev_dbg(&p->pdev->dev, "Max perf-stat size %lu-bytes\n",
>> + p->stat_buffer_len);
>> + } else {
>> + dev_info(&p->pdev->dev, "Dimm performance stats unavailable\n");
>> + }
>
> With this patch https://lore.kernel.org/linuxppc-dev/20210505191606.51666-1-vaibhav@linux.ibm.com
> We are adding details of whyy performance stat query hcall failed. Do we
> need to print again here? Are we being more verbose here?
>
Yes agree this looks more verbose with the other patch you mentioned. I
have sent out a v2 of this patch with this dev_info removed.
> -aneesh
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
--
Cheers
~ Vaibhav
^ permalink raw reply
* [PATCH] lockdown, selinux: fix bogus SELinux lockdown permission checks
From: Ondrej Mosnacek @ 2021-05-07 11:40 UTC (permalink / raw)
To: linux-security-module, James Morris
Cc: selinux, netdev, Stephen Smalley, linux-kernel, Steven Rostedt,
Ingo Molnar, linux-fsdevel, bpf, linuxppc-dev
Commit 59438b46471a ("security,lockdown,selinux: implement SELinux
lockdown") added an implementation of the locked_down LSM hook to
SELinux, with the aim to restrict which domains are allowed to perform
operations that would breach lockdown.
However, in several places the security_locked_down() hook is called in
situations where the current task isn't doing any action that would
directly breach lockdown, leading to SELinux checks that are basically
bogus.
Since in most of these situations converting the callers such that
security_locked_down() is called in a context where the current task
would be meaningful for SELinux is impossible or very non-trivial (and
could lead to TOCTOU issues for the classic Lockdown LSM
implementation), fix this by adding a separate hook
security_locked_down_globally() that is to be used in such situations
and convert all these problematic callers to call this hook instead. The
new hook is then left unimplemented in SELinux and in Lockdown LSM it is
backed by the same implementation as the locked_down hook.
The callers migrated to the new hook are:
1. arch/powerpc/xmon/xmon.c
Here the hook seems to be called from non-task context and is only
used for redacting some sensitive values from output sent to
userspace.
2. fs/tracefs/inode.c:tracefs_create_file()
Here the call is used to prevent creating new tracefs entries when
the kernel is locked down. Assumes that locking down is one-way -
i.e. if the hook returns non-zero once, it will never return zero
again, thus no point in creating these files.
3. kernel/trace/bpf_trace.c:bpf_probe_read_kernel{,_str}_common()
Called when a BPF program calls a helper that could leak kernel
memory. The task context is not relevant here, since the program
may very well be run in the context of a different task than the
consumer of the data.
See: https://bugzilla.redhat.com/show_bug.cgi?id=1955585
4. net/xfrm/xfrm_user.c:copy_to_user_*()
Here a cryptographic secret is redacted based on the value returned
from the hook. There are two possible actions that may lead here:
a) A netlink message XFRM_MSG_GETSA with NLM_F_DUMP set - here the
task context is relevant, since the dumped data is sent back to
the current task.
b) When deleting an SA via XFRM_MSG_DELSA, the dumped SAs are
broadcasted to tasks subscribed to XFRM events - here the
SELinux check is not meningful as the current task's creds do
not represent the tasks that could potentially see the secret.
It really doesn't seem worth it to try to preserve the check in the
a) case, since the eventual leak can be circumvented anyway via b),
plus there is no way for the task to indicate that it doesn't care
about the actual key value, so the check could generate a lot of
noise.
Fixes: 59438b46471a ("security,lockdown,selinux: implement SELinux lockdown")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
arch/powerpc/xmon/xmon.c | 4 ++--
fs/tracefs/inode.c | 2 +-
include/linux/lsm_hook_defs.h | 1 +
include/linux/security.h | 5 +++++
kernel/trace/bpf_trace.c | 4 ++--
net/xfrm/xfrm_user.c | 2 +-
security/lockdown/lockdown.c | 1 +
security/security.c | 6 ++++++
8 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 3fe37495f63d..a4bad825d424 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -298,7 +298,7 @@ static bool xmon_is_locked_down(void)
static bool lockdown;
if (!lockdown) {
- lockdown = !!security_locked_down(LOCKDOWN_XMON_RW);
+ lockdown = !!security_locked_down_globally(LOCKDOWN_XMON_RW);
if (lockdown) {
printf("xmon: Disabled due to kernel lockdown\n");
xmon_is_ro = true;
@@ -306,7 +306,7 @@ static bool xmon_is_locked_down(void)
}
if (!xmon_is_ro) {
- xmon_is_ro = !!security_locked_down(LOCKDOWN_XMON_WR);
+ xmon_is_ro = !!security_locked_down_globally(LOCKDOWN_XMON_WR);
if (xmon_is_ro)
printf("xmon: Read-only due to kernel lockdown\n");
}
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 4b83cbded559..07241435efec 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -396,7 +396,7 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
struct dentry *dentry;
struct inode *inode;
- if (security_locked_down(LOCKDOWN_TRACEFS))
+ if (security_locked_down_globally(LOCKDOWN_TRACEFS))
return NULL;
if (!(mode & S_IFMT))
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 477a597db013..d6e2a6b59277 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -390,6 +390,7 @@ LSM_HOOK(void, LSM_RET_VOID, bpf_prog_free_security, struct bpf_prog_aux *aux)
#endif /* CONFIG_BPF_SYSCALL */
LSM_HOOK(int, 0, locked_down, enum lockdown_reason what)
+LSM_HOOK(int, 0, locked_down_globally, enum lockdown_reason what)
#ifdef CONFIG_PERF_EVENTS
LSM_HOOK(int, 0, perf_event_open, struct perf_event_attr *attr, int type)
diff --git a/include/linux/security.h b/include/linux/security.h
index 8aeebd6646dc..e683dee84f46 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -468,6 +468,7 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
int security_locked_down(enum lockdown_reason what);
+int security_locked_down_globally(enum lockdown_reason what);
#else /* CONFIG_SECURITY */
static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
@@ -1329,6 +1330,10 @@ static inline int security_locked_down(enum lockdown_reason what)
{
return 0;
}
+static inline int security_locked_down_globally(enum lockdown_reason what)
+{
+ return 0;
+}
#endif /* CONFIG_SECURITY */
#if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index b0c45d923f0f..f43bca95b261 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -215,7 +215,7 @@ const struct bpf_func_proto bpf_probe_read_user_str_proto = {
static __always_inline int
bpf_probe_read_kernel_common(void *dst, u32 size, const void *unsafe_ptr)
{
- int ret = security_locked_down(LOCKDOWN_BPF_READ);
+ int ret = security_locked_down_globally(LOCKDOWN_BPF_READ);
if (unlikely(ret < 0))
goto fail;
@@ -246,7 +246,7 @@ const struct bpf_func_proto bpf_probe_read_kernel_proto = {
static __always_inline int
bpf_probe_read_kernel_str_common(void *dst, u32 size, const void *unsafe_ptr)
{
- int ret = security_locked_down(LOCKDOWN_BPF_READ);
+ int ret = security_locked_down_globally(LOCKDOWN_BPF_READ);
if (unlikely(ret < 0))
goto fail;
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 5a0ef4361e43..5a56f74262d8 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -851,7 +851,7 @@ static int copy_user_offload(struct xfrm_state_offload *xso, struct sk_buff *skb
static bool xfrm_redact(void)
{
return IS_ENABLED(CONFIG_SECURITY) &&
- security_locked_down(LOCKDOWN_XFRM_SECRET);
+ security_locked_down_globally(LOCKDOWN_XFRM_SECRET);
}
static int copy_to_user_auth(struct xfrm_algo_auth *auth, struct sk_buff *skb)
diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
index 87cbdc64d272..4ac172eaa4b7 100644
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -73,6 +73,7 @@ static int lockdown_is_locked_down(enum lockdown_reason what)
static struct security_hook_list lockdown_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(locked_down, lockdown_is_locked_down),
+ LSM_HOOK_INIT(locked_down_globally, lockdown_is_locked_down),
};
static int __init lockdown_lsm_init(void)
diff --git a/security/security.c b/security/security.c
index 5ac96b16f8fa..b9b990681ae9 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2547,6 +2547,12 @@ int security_locked_down(enum lockdown_reason what)
}
EXPORT_SYMBOL(security_locked_down);
+int security_locked_down_globally(enum lockdown_reason what)
+{
+ return call_int_hook(locked_down_globally, 0, what);
+}
+EXPORT_SYMBOL(security_locked_down_globally);
+
#ifdef CONFIG_PERF_EVENTS
int security_perf_event_open(struct perf_event_attr *attr, int type)
{
--
2.31.1
^ permalink raw reply related
* Re: [PATCH v2 3/3] i2c: mpc: implement erratum A-004447 workaround
From: Andy Shevchenko @ 2021-05-07 11:46 UTC (permalink / raw)
To: Chris Packham
Cc: devicetree, Linux Kernel Mailing List, Wolfram Sang, Rob Herring,
linux-i2c, Andy Shevchenko,
open list:LINUX FOR POWERPC PA SEMI PWRFICIENT
In-Reply-To: <20210507004047.4454-4-chris.packham@alliedtelesis.co.nz>
On Fri, May 7, 2021 at 3:40 AM Chris Packham
<chris.packham@alliedtelesis.co.nz> wrote:
>
> The P2040/P2041 has an erratum where the normal i2c recovery mechanism
> does not work. Implement the alternative recovery mechanism documented
> in the P2040 Chip Errata Rev Q.
Thanks.
> +static int i2c_mpc_wait_sr(struct mpc_i2c *i2c, int mask)
> +{
> + int ret;
> + u8 val;
> +
> + ret = readb_poll_timeout(i2c->base + MPC_I2C_SR, val,
> + val & mask, 0, 100);
> +
> + return ret;
> +}
So, now you may shrink it even further, i.e.
void __iomem *sr = i2c->base + MPC_I2C_SR;
u8 val;
return readb_poll_timeout(sr, val, val & mask, 0, 100);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC PATCH 0/7] Memory hotplug/hotremove at subsection size
From: Michal Hocko @ 2021-05-07 11:55 UTC (permalink / raw)
To: Zi Yan, David Hildenbrand
Cc: linux-ia64, Wei Yang, Anshuman Khandual, Rafael J . Wysocki, x86,
Dan Williams, linux-kernel, linux-mm, Andy Lutomirski,
Thomas Gleixner, linuxppc-dev, Andrew Morton, Mike Rapoport,
Oscar Salvador
In-Reply-To: <fb60eabd-f8ef-2cb1-7338-7725efe3c286@redhat.com>
[I haven't read through respective patches due to lack of time but let
me comment on the general idea and the underlying justification]
On Thu 06-05-21 17:31:09, David Hildenbrand wrote:
> On 06.05.21 17:26, Zi Yan wrote:
> > From: Zi Yan <ziy@nvidia.com>
> >
> > Hi all,
> >
> > This patchset tries to remove the restriction on memory hotplug/hotremove
> > granularity, which is always greater or equal to memory section size[1].
> > With the patchset, kernel is able to online/offline memory at a size independent
> > of memory section size, as small as 2MB (the subsection size).
>
> ... which doesn't make any sense as we can only online/offline whole memory
> block devices.
Agreed. The subsection thingy is just a hack to workaround pmem
alignement problems. For the real memory hotplug it is quite hard to
argue for reasonable hotplug scenarios for very small physical memory
ranges wrt. to the existing sparsemem memory model.
> > The motivation is to increase MAX_ORDER of the buddy allocator and pageblock
> > size without increasing memory hotplug/hotremove granularity at the same time,
>
> Gah, no. Please no. No.
Agreed. Those are completely independent concepts. MAX_ORDER is can be
really arbitrary irrespective of the section size with vmemmap sparse
model. The existing restriction is due to old sparse model not being
able to do page pointer arithmetic across memory sections. Is there any
reason to stick with that memory model for an advance feature you are
working on?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
From: Vaibhav Jain @ 2021-05-07 12:18 UTC (permalink / raw)
To: Ira Weiny; +Cc: Aneesh Kumar K . V, linuxppc-dev, linux-nvdimm
In-Reply-To: <20210506045817.GF1068722@iweiny-DESK2.sc.intel.com>
Hi Ira,
Thanks for looking into this patch
Ira Weiny <ira.weiny@intel.com> writes:
> On Thu, May 06, 2021 at 12:46:06AM +0530, Vaibhav Jain wrote:
>> Currently drc_pmem_qeury_stats() generates a dev_err in case
>> "Enable Performance Information Collection" feature is disabled from
>> HMC or performance stats are not available for an nvdimm. The error is
>> of the form below:
>>
>> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
>> performance stats, Err:-10
>>
>> This error message confuses users as it implies a possible problem
>> with the nvdimm even though its due to a disabled/unavailable
>> feature. We fix this by explicitly handling the H_AUTHORITY and
>> H_UNSUPPORTED errors from the H_SCM_PERFORMANCE_STATS hcall.
>>
>> In case of H_AUTHORITY error an info message is logged instead of an
>> error, saying that "Permission denied while accessing performance
>> stats". Also '-EACCES' error is return instead of -EPERM.
>
> I thought you clarified before that this was a permission issue. So why change
> the error to EACCES?
>
EACCESS("Permission Denied") felt like a more accurate error code for
this case than EPERM("Operation not permitted"). So switched the usage
of EPERM error code to handle the case if this hcall is not supported
for an nvdimm.
>>
>> In case of H_UNSUPPORTED error we return a -EPERM error back from
>> drc_pmem_query_stats() indicating that performance stats-query
>> operation is not supported on this nvdimm.
>
> EPERM seems wrong here too... ENOTSUP?
Yes, will change it to EOPNOTSUPP in v3.
>
> Ira
> _______________________________________________
> Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
> To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
--
Cheers
~ Vaibhav
^ permalink raw reply
* Re: [RFC PATCH 0/7] Memory hotplug/hotremove at subsection size
From: David Hildenbrand @ 2021-05-07 14:00 UTC (permalink / raw)
To: Michal Hocko, Zi Yan
Cc: linux-ia64, Wei Yang, Anshuman Khandual, Rafael J . Wysocki, x86,
Dan Williams, linux-kernel, linux-mm, Andy Lutomirski,
Thomas Gleixner, linuxppc-dev, Andrew Morton, Mike Rapoport,
Oscar Salvador
In-Reply-To: <YJUqrOacyqI+kiKW@dhcp22.suse.cz>
On 07.05.21 13:55, Michal Hocko wrote:
> [I haven't read through respective patches due to lack of time but let
> me comment on the general idea and the underlying justification]
>
> On Thu 06-05-21 17:31:09, David Hildenbrand wrote:
>> On 06.05.21 17:26, Zi Yan wrote:
>>> From: Zi Yan <ziy@nvidia.com>
>>>
>>> Hi all,
>>>
>>> This patchset tries to remove the restriction on memory hotplug/hotremove
>>> granularity, which is always greater or equal to memory section size[1].
>>> With the patchset, kernel is able to online/offline memory at a size independent
>>> of memory section size, as small as 2MB (the subsection size).
>>
>> ... which doesn't make any sense as we can only online/offline whole memory
>> block devices.
>
> Agreed. The subsection thingy is just a hack to workaround pmem
> alignement problems. For the real memory hotplug it is quite hard to
> argue for reasonable hotplug scenarios for very small physical memory
> ranges wrt. to the existing sparsemem memory model.
>
>>> The motivation is to increase MAX_ORDER of the buddy allocator and pageblock
>>> size without increasing memory hotplug/hotremove granularity at the same time,
>>
>> Gah, no. Please no. No.
>
> Agreed. Those are completely independent concepts. MAX_ORDER is can be
> really arbitrary irrespective of the section size with vmemmap sparse
> model. The existing restriction is due to old sparse model not being
> able to do page pointer arithmetic across memory sections. Is there any
> reason to stick with that memory model for an advance feature you are
> working on?
>
I gave it some more thought yesterday. I guess the first thing we should
look into is increasing MAX_ORDER and leaving pageblock_order and
section size as is -- finding out what we have to tweak to get that up
and running. Once we have that in place, we can actually look into
better fragmentation avoidance etc. One step at a time.
Because that change itself might require some thought. Requiring that
bigger MAX_ORDER depends on SPARSE_VMEMMAP is something reasonable to do.
As stated somewhere here already, we'll have to look into making
alloc_contig_range() (and main users CMA and virtio-mem) independent of
MAX_ORDER and mainly rely on pageblock_order. The current handling in
alloc_contig_range() is far from optimal as we have to isolate a whole
MAX_ORDER - 1 page -- and on ZONE_NORMAL we'll fail easily if any part
contains something unmovable although we don't even want to allocate
that part. I actually have that on my list (to be able to fully support
pageblock_order instead of MAX_ORDER -1 chunks in virtio-mem), however
didn't have time to look into it.
Further, page onlining / offlining code and early init code most
probably also needs care if MAX_ORDER - 1 crosses sections. Memory holes
we might suddenly have in MAX_ORDER - 1 pages might become a problem and
will have to be handled. Not sure which other code has to be tweaked
(compaction? page isolation?).
Figuring out what needs care itself might take quite some effort.
One thing I was thinking about as well: The bigger our MAX_ORDER, the
slower it could be to allocate smaller pages. If we have 1G pages,
splitting them down to 4k then takes 8 additional steps if I'm, not
wrong. Of course, that's the worst case. Would be interesting to evaluate.
--
Thanks,
David / dhildenb
^ permalink raw reply
* [PATCH] powerpc/perf: Simplify Makefile
From: Christophe Leroy @ 2021-05-07 14:01 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
arch/powerpc/Kbuild decend into arch/powerpc/perf/ only when
CONFIG_PERF_EVENTS is selected, so there is not need to take
CONFIG_PERF_EVENTS into account in arch/powerpc/perf/Makefile.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/perf/Makefile | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/perf/Makefile b/arch/powerpc/perf/Makefile
index c02854dea2b2..2f46e31c7612 100644
--- a/arch/powerpc/perf/Makefile
+++ b/arch/powerpc/perf/Makefile
@@ -1,9 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_PERF_EVENTS) += callchain.o callchain_$(BITS).o perf_regs.o
-ifdef CONFIG_COMPAT
-obj-$(CONFIG_PERF_EVENTS) += callchain_32.o
-endif
+obj-y += callchain.o callchain_$(BITS).o perf_regs.o
+obj-$(CONFIG_COMPAT) += callchain_32.o
obj-$(CONFIG_PPC_PERF_CTRS) += core-book3s.o bhrb.o
obj64-$(CONFIG_PPC_PERF_CTRS) += ppc970-pmu.o power5-pmu.o \
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] powerpc/perf: Simplify Makefile
From: Michal Suchánek @ 2021-05-07 14:11 UTC (permalink / raw)
To: Christophe Leroy; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <d37f61afca55b5b33787b643890e061ae1c18f5f.1620396045.git.christophe.leroy@csgroup.eu>
On Fri, May 07, 2021 at 02:01:09PM +0000, Christophe Leroy wrote:
> arch/powerpc/Kbuild decend into arch/powerpc/perf/ only when
> CONFIG_PERF_EVENTS is selected, so there is not need to take
> CONFIG_PERF_EVENTS into account in arch/powerpc/perf/Makefile.
So long as CONFIG_PERF_EVENTS stays boolean.
If it were tristate the result is less clear.
Reviewed-by: Michal Suchánek <msuchanek@suse.de>
Thanks
Michal
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/perf/Makefile | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/perf/Makefile b/arch/powerpc/perf/Makefile
> index c02854dea2b2..2f46e31c7612 100644
> --- a/arch/powerpc/perf/Makefile
> +++ b/arch/powerpc/perf/Makefile
> @@ -1,9 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
>
> -obj-$(CONFIG_PERF_EVENTS) += callchain.o callchain_$(BITS).o perf_regs.o
> -ifdef CONFIG_COMPAT
> -obj-$(CONFIG_PERF_EVENTS) += callchain_32.o
> -endif
> +obj-y += callchain.o callchain_$(BITS).o perf_regs.o
> +obj-$(CONFIG_COMPAT) += callchain_32.o
>
> obj-$(CONFIG_PPC_PERF_CTRS) += core-book3s.o bhrb.o
> obj64-$(CONFIG_PPC_PERF_CTRS) += ppc970-pmu.o power5-pmu.o \
> --
> 2.25.0
>
^ permalink raw reply
* Re: [PATCH v2 3/3] i2c: mpc: implement erratum A-004447 workaround
From: Joakim Tjernlund @ 2021-05-07 14:52 UTC (permalink / raw)
To: andy.shevchenko@gmail.com, chris.packham@alliedtelesis.co.nz
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
wsa@kernel.org, robh+dt@kernel.org, linux-i2c@vger.kernel.org,
andriy.shevchenko@linux.intel.com, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <CAHp75VcEzgKjOD6WQ1=YAwK_hhZr=XtcmXZL8rK78gb7iXDbDg@mail.gmail.com>
On Fri, 2021-05-07 at 14:46 +0300, Andy Shevchenko wrote:
> On Fri, May 7, 2021 at 3:40 AM Chris Packham
> <chris.packham@alliedtelesis.co.nz> wrote:
> >
> > The P2040/P2041 has an erratum where the normal i2c recovery mechanism
> > does not work. Implement the alternative recovery mechanism documented
> > in the P2040 Chip Errata Rev Q.
>
> Thanks.
>
> > +static int i2c_mpc_wait_sr(struct mpc_i2c *i2c, int mask)
> > +{
> > + int ret;
> > + u8 val;
> > +
> > + ret = readb_poll_timeout(i2c->base + MPC_I2C_SR, val,
> > + val & mask, 0, 100);
> > +
> > + return ret;
> > +}
>
> So, now you may shrink it even further, i.e.
>
> void __iomem *sr = i2c->base + MPC_I2C_SR;
> u8 val;
>
> return readb_poll_timeout(sr, val, val & mask, 0, 100);
>
val looks uninitialised before use?
^ permalink raw reply
* Re: [PATCH v2 3/3] i2c: mpc: implement erratum A-004447 workaround
From: Andy Shevchenko @ 2021-05-07 15:36 UTC (permalink / raw)
To: Joakim Tjernlund
Cc: devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
robh+dt@kernel.org, wsa@kernel.org,
chris.packham@alliedtelesis.co.nz, linux-i2c@vger.kernel.org,
andriy.shevchenko@linux.intel.com, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <c4fa78e8f2c43da240c932618b87b34ba2d1d046.camel@infinera.com>
On Fri, May 7, 2021 at 5:52 PM Joakim Tjernlund
<Joakim.Tjernlund@infinera.com> wrote:
> On Fri, 2021-05-07 at 14:46 +0300, Andy Shevchenko wrote:
> > On Fri, May 7, 2021 at 3:40 AM Chris Packham
> > <chris.packham@alliedtelesis.co.nz> wrote:
...
> > So, now you may shrink it even further, i.e.
> >
> > void __iomem *sr = i2c->base + MPC_I2C_SR;
> > u8 val;
> >
> > return readb_poll_timeout(sr, val, val & mask, 0, 100);
> >
>
> val looks uninitialised before use?
Nope.
Thinking about naming, perhaps
void __iomem *addr = i2c->base + MPC_I2C_SR;
u8 sr; // or leave as val?
return readb_poll_timeout(addr, sr, sr & mask, 0, 100);
would be more clear.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/3] hotplug-memory.c: enhance dlpar_memory_remove* LMB checks
From: Daniel Henrique Barboza @ 2021-05-07 16:36 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev
In-Reply-To: <YJCdNJRSLCOV59dE@yekko>
On 5/3/21 10:02 PM, David Gibson wrote:
> On Fri, Apr 30, 2021 at 09:09:16AM -0300, Daniel Henrique Barboza wrote:
>> dlpar_memory_remove_by_ic() validates the amount of LMBs to be removed
>> by checking !DRCONF_MEM_RESERVED, and in the following loop before
>> dlpar_remove_lmb() a check for DRCONF_MEM_ASSIGNED is made before
>> removing it. This means that a LMB that is both !DRCONF_MEM_RESERVED and
>> !DRCONF_MEM_ASSIGNED will be counted as valid, but then not being
>> removed. The function will end up not removing all 'lmbs_to_remove'
>> LMBs while also not reporting any errors.
>>
>> Comparing it to dlpar_memory_remove_by_count(), the validation is done
>> via lmb_is_removable(), which checks for DRCONF_MEM_ASSIGNED and fadump
>> constraints. No additional check is made afterwards, and
>> DRCONF_MEM_RESERVED is never checked before dlpar_remove_lmb(). The
>> function doesn't have the same 'check A for validation, then B for
>> removal' issue as remove_by_ic(), but it's not checking if the LMB is
>> reserved.
>>
>> There is no reason for these functions to validate the same operation in
>> two different manners.
>
> Actually, I think there is: remove_by_ic() is handling a request to
> remove a specific range of LMBs. If any are reserved, they can't be
> removed and so this needs to fail. But if they are !ASSIGNED, that
> essentially means they're *already* removed (or never added), so
> "removing" them is, correctly, a no-op.
I guess that makes sense. Although I am not aware of any situation, at least
thinking about how QEMU adds/removes LMBs, where some LMBs would be removed
'ad-hoc' in the middle of a LMB range that maps to a QEMU DIMM, I can't say
that this wouldn't never happen either. It is sensible to make remove_by_ic()
resilient to this situation.
I'll re-send this patch just with the remove_by_count() change.
Thanks,
Daniel
>
> remove_by_count(), in contrast, is being asked to remove a fixed
> number of LMBs from wherever they can be found, and for that it needs
> to find LMBs that haven't already been removed.
>
> Basically remove_by_ic() is an absolute request: "make this set of
> LMBs be not-plugged", whereas remove_by_count() is a relative request
> "make N less LMBs be plugged".
>
>
> So I think remove_by_ic()s existing handling is correct. I'm less
> sure if remove_by_count() ignoring RESERVED is correct - I couldn't
> quickly find under what circumstances RESERVED gets set.
>
>
>> This patch addresses that by changing
>> lmb_is_removable() to also check for DRCONF_MEM_RESERVED to tell if a
>> lmb is removable, making dlpar_memory_remove_by_count() take the
>> reservation state into account when counting the LMBs.
>> lmb_is_removable() is then used in the validation step of
>> dlpar_memory_remove_by_ic(), which is already checking for both states
>> but in different stages, to avoid counting a LMB that is not assigned as
>> eligible for removal. We can then skip the check before
>> dlpar_remove_lmb() since we're validating all LMBs beforehand.
>>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
>> ---
>> arch/powerpc/platforms/pseries/hotplug-memory.c | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index bb98574a84a2..4e6d162c3f1a 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -348,7 +348,8 @@ static int pseries_remove_mem_node(struct device_node *np)
>>
>> static bool lmb_is_removable(struct drmem_lmb *lmb)
>> {
>> - if (!(lmb->flags & DRCONF_MEM_ASSIGNED))
>> + if ((lmb->flags & DRCONF_MEM_RESERVED) ||
>> + !(lmb->flags & DRCONF_MEM_ASSIGNED))
>> return false;
>>
>> #ifdef CONFIG_FA_DUMP
>> @@ -523,7 +524,7 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
>>
>> /* Validate that there are enough LMBs to satisfy the request */
>> for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
>> - if (lmb->flags & DRCONF_MEM_RESERVED)
>> + if (!lmb_is_removable(lmb))
>> break;
>>
>> lmbs_available++;
>> @@ -533,9 +534,6 @@ static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
>> return -EINVAL;
>>
>> for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
>> - if (!(lmb->flags & DRCONF_MEM_ASSIGNED))
>> - continue;
>> -
>> rc = dlpar_remove_lmb(lmb);
>> if (rc)
>> break;
>
^ permalink raw reply
* [PATCH] powerpc/8xx: Update mpc885_ads_defconfig to improve CI
From: Christophe Leroy @ 2021-05-07 16:37 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
mpc885_ads_defconfig is used by several CI robots.
A few functionnalities are specific to 8xx and are not covered
by other default configuration, so improve build test coverage
by adding them to mpc885_ads_defconfig.
8xx is the only platform supporting 16k page size in addition
to 4k page size. Considering that 4k page size is widely tested
by other configurations, lets make 16k pages the selection for
8xx, as it has demonstrated in the past to be a weakness.
CONFIG_PIN_TLB is specific to 8xx, select it as it mainly adds
code with removing much.
CONFIG_BDI_SWITCH is specific to PPC32 and adds codes.
CONFIG_PPC_PTDUMP has specific part for 8xx.
CONFIG_MODULES has specific handling for 8xx.
CONFIG_SMC_UCODE_PATCH is specific to 8xx for loading microcode.
CONFIG_PERF_EVENTS has specific parts for 8xx.
CONFIG_MATH_EMULATION is used by 8xx.
CONFIG_STRICT_KERNEL_RWX has specificities for 8xx.
CONFIG_VIRT_CPU_ACCOUNTING_NATIVE has specific parts for PPC32.
CONFIG_IPV6 has specificities for PPC32.
CONFIG_BPF_JIT has specificities for PPC32.
A few drivers are tightly linked to the 8xx:
- CONFIG_SPI_FSL_SPI
- CONFIG_CRYPTO_DEV_TALITOS
- CONFIG_8xxx_WDT
- CONFIG_8xx_GPIO
- CONFIG_PPC_EARLY_DEBUG_CPM
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/configs/mpc885_ads_defconfig | 25 +++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/arch/powerpc/configs/mpc885_ads_defconfig b/arch/powerpc/configs/mpc885_ads_defconfig
index 949ff9ccda5e..d21f266cea9a 100644
--- a/arch/powerpc/configs/mpc885_ads_defconfig
+++ b/arch/powerpc/configs/mpc885_ads_defconfig
@@ -57,3 +57,28 @@ CONFIG_CRC32_SLICEBY4=y
CONFIG_DEBUG_INFO=y
CONFIG_MAGIC_SYSRQ=y
CONFIG_DETECT_HUNG_TASK=y
+CONFIG_PPC_16K_PAGES=y
+CONFIG_DEBUG_KERNEL=y
+CONFIG_DEBUG_FS=y
+CONFIG_PPC_PTDUMP=y
+CONFIG_MODULES=y
+CONFIG_SPI=y
+CONFIG_SPI_FSL_SPI=y
+CONFIG_CRYPTO=y
+CONFIG_CRYPTO_DEV_TALITOS=y
+CONFIG_8xx_GPIO=y
+CONFIG_WATCHDOG=y
+CONFIG_8xxx_WDT=y
+CONFIG_SMC_UCODE_PATCH=y
+CONFIG_ADVANCED_OPTIONS=y
+CONFIG_PIN_TLB=y
+CONFIG_PERF_EVENTS=y
+CONFIG_MATH_EMULATION=y
+CONFIG_VIRT_CPU_ACCOUNTING_NATIVE=y
+CONFIG_STRICT_KERNEL_RWX=y
+CONFIG_IPV6=y
+CONFIG_BPF_JIT=y
+CONFIG_DEBUG_VM_PGTABLE=y
+CONFIG_BDI_SWITCH=y
+CONFIG_PPC_EARLY_DEBUG=y
+CONFIG_PPC_EARLY_DEBUG_CPM_ADDR=0xff002008
--
2.25.0
^ permalink raw reply related
* Re: UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:359:56
From: Paul Menzel @ 2021-05-07 17:52 UTC (permalink / raw)
To: Christophe Leroy, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras
Cc: Andrey Ryabinin, linuxppc-dev
In-Reply-To: <2fee0345-e8e1-b25f-b8be-4ce38e9c6939@csgroup.eu>
Dear Christophe,
Am 07.05.21 um 10:59 schrieb Christophe Leroy:
> Le 07/05/2021 à 10:42, Paul Menzel a écrit :
>> [+Andrey]
>> Am 07.05.21 um 10:31 schrieb Christophe Leroy:
>>
>>> Le 06/05/2021 à 21:32, Paul Menzel a écrit :
>>>> [corrected subject]
>>>>
>>>> Am 06.05.21 um 21:31 schrieb Paul Menzel:
>>
>>>>> On the POWER8 system IBM S822LC, Linux 5.13+, built with USSAN,
>>>>> logs the warning below.
>>>>>
>>>>> ```
>>>>> [ 0.030091] ================================================================================
>>>>> [ 0.030295] UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:359:56
>>>>> [ 0.030325] index -1 is out of range for type 'legacy_serial_info [8]'
>>>>> [ 0.030350] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0+ #2
>>>>> [ 0.030360] Call Trace:
>>>>> [ 0.030363] [c000000024f1bad0] [c0000000009f4330] dump_stack+0xc4/0x114 (unreliable)
>>>>> [ 0.030386] [c000000024f1bb20] [c0000000009efed0] ubsan_epilogue+0x18/0x78
>>>>> [ 0.030400] [c000000024f1bb80] [c0000000009efafc] __ubsan_handle_out_of_bounds+0xac/0xd0
>>>>> [ 0.030414] [c000000024f1bc20] [c000000001711588] ioremap_legacy_serial_console+0x54/0x144
>>>>> [ 0.030430] [c000000024f1bc70] [c0000000000123c0] do_one_initcall+0x60/0x2c0
>>>>> [ 0.030444] [c000000024f1bd40] [c000000001704bc4] kernel_init_freeable+0x19c/0x25c
>>>>> [ 0.030458] [c000000024f1bda0] [c000000000012a2c] kernel_init+0x2c/0x180
>>>>> [ 0.030471] [c000000024f1be10] [c00000000000d6ec] ret_from_kernel_thread+0x5c/0x70
>>>>> [ 0.030484] ================================================================================
>>>>> [ 0.030641] ================================================================================
>>>>> [ 0.030668] UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:360:58
>>>>> [ 0.030697] index -1 is out of range for type 'plat_serial8250_port [9]'
>>>>> [ 0.030721] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0+ #2
>>>>> [ 0.030730] Call Trace:
>>>>> [ 0.030733] [c000000024f1bad0] [c0000000009f4330] dump_stack+0xc4/0x114 (unreliable)
>>>>> [ 0.030749] [c000000024f1bb20] [c0000000009efed0] ubsan_epilogue+0x18/0x78
>>>>> [ 0.030762] [c000000024f1bb80] [c0000000009efafc] __ubsan_handle_out_of_bounds+0xac/0xd0
>>>>> [ 0.030775] [c000000024f1bc20] [c0000000017115a0] ioremap_legacy_serial_console+0x6c/0x144
>>>>> [ 0.030790] [c000000024f1bc70] [c0000000000123c0] do_one_initcall+0x60/0x2c0
>>>>> [ 0.030802] [c000000024f1bd40] [c000000001704bc4] kernel_init_freeable+0x19c/0x25c
>>>>> [ 0.030816] [c000000024f1bda0] [c000000000012a2c] kernel_init+0x2c/0x180
>>>>> [ 0.030829] [c000000024f1be10] [c00000000000d6ec] ret_from_kernel_thread+0x5c/0x70
>>>>> [ 0.030842] ================================================================================
>>>>> ```
>>>
>>> The function is as follows, so when legacy_serial_console == -1 as in
>>> your situation, the pointers are just not used.
>>>
>>> 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;
>>>
>>> ...
>>> }
>>>
>>> When I look into the generated code (UBSAN not selected), we see the
>>> verification and the bail-out is done prior to any calculation based
>>> on legacy_serial_console.
>>>
>>> 00000000 <ioremap_legacy_serial_console>:
>>> 0: 94 21 ff e0 stwu r1,-32(r1)
>>> 4: 3d 20 00 00 lis r9,0
>>> 6: R_PPC_ADDR16_HA .data
>>> 8: 7c 08 02 a6 mflr r0
>>> c: bf 81 00 10 stmw r28,16(r1)
>>> 10: 3b 80 00 00 li r28,0
>>> 14: 83 a9 00 00 lwz r29,0(r9)
>>> 16: R_PPC_ADDR16_LO .data
>>> 18: 90 01 00 24 stw r0,36(r1)
>>> 1c: 2c 1d 00 00 cmpwi r29,0
>>> 20: 41 80 00 80 blt a0 <ioremap_legacy_serial_console+0xa0>
>>>
>>> So, is it normal that UBSAN reports an error here ?
>>
>> If it’s useful, I could disassemble the code here. But please tell me
>> how.
>>
>> Sorry, I do not know. I just selected the option, and saw the error.
>> Maybe Andrey has an idea.
>
> No need for you to disassemble, I just wanted to show that without UBSAN
> there is no problem with the index as it is used only after boundary
> checking. (But if you want to do so, if is just an 'objdump -dr
> legacy_serial.o')
Thank you for the hint.
> Now, with UBSAN, I see that UBSAN does the verification of the index
> earlier than expected. So what to do here, we can modify the code, but
> that modification would just be to make UBSAN happy as there is no
> problem in itself.
In #gcc@irc.freenode.net I was told by zid (they weren’t so happy with
the wording), but maybe you understand it:
> It's not legal C to generate pointers to things other than 0,
> objects, or 1 past the end of an object, not just dereference them,
> so technically that's not legal per the C spec.
>
> In practice it won't matter until it's dereferenced of course unless
> you're doing something weird, let's say.. instrumenting the code
Kind regards,
Paul
^ permalink raw reply
* Re: [RESEND PATCH v4 10/11] powerpc: Protect patching_mm with a lock
From: Christopher M. Riedl @ 2021-05-07 20:03 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: tglx, x86, linuxppc-dev, linux-hardening, keescook
In-Reply-To: <YJPKNSNVf34a39qC@hirez.programming.kicks-ass.net>
On Thu May 6, 2021 at 5:51 AM CDT, Peter Zijlstra wrote:
> On Wed, May 05, 2021 at 11:34:51PM -0500, Christopher M. Riedl wrote:
> > Powerpc allows for multiple CPUs to patch concurrently. When patching
> > with STRICT_KERNEL_RWX a single patching_mm is allocated for use by all
> > CPUs for the few times that patching occurs. Use a spinlock to protect
> > the patching_mm from concurrent use.
> >
> > Modify patch_instruction() to acquire the lock, perform the patch op,
> > and then release the lock.
> >
> > Also introduce {lock,unlock}_patching() along with
> > patch_instruction_unlocked() to avoid per-iteration lock overhead when
> > patch_instruction() is called in a loop. A follow-up patch converts some
> > uses of patch_instruction() to use patch_instruction_unlocked() instead.
>
> x86 uses text_mutex for all this, why not do the same?
I wasn't entirely sure if there is a problem with potentially going to
sleep in some of the places where patch_instruction() is called - the
spinlock avoids that (hypothetical) problem.
I just tried switching to text_mutex and at least on a P9 machine the
series boots w/ the Hash and Radix MMUs (with some lockdep errors). I
can rework this in the next version to use text_mutex if I don't find
any new problems with more extensive testing. It does mean more changes
to use patch_instruction_unlocked() in kprobe/optprobe/ftace in
arch/powerpc since iirc those are called with text_mutex already held.
Thanks!
Chris R.
^ permalink raw reply
* Re: UBSAN: array-index-out-of-bounds in arch/powerpc/kernel/legacy_serial.c:359:56
From: Segher Boessenkool @ 2021-05-07 20:59 UTC (permalink / raw)
To: Christophe Leroy; +Cc: Paul Menzel, linuxppc-dev, Paul Mackerras
In-Reply-To: <3b1febc1-3635-fb5f-1ff3-6f9f0c8feb12@csgroup.eu>
On Fri, May 07, 2021 at 10:31:42AM +0200, Christophe Leroy wrote:
> The function is as follows, so when legacy_serial_console == -1 as in your
> situation, the pointers are just not used.
And it is still undefined behaviour. C11 6.5.6/8 has
If both the pointer operand and the result point to elements of the
same array object, or one past the last element of the array object,
the evaluation shall not produce an overflow; otherwise, the behavior
is undefined.
(this is for adding an integer to a pointer).
> When I look into the generated code (UBSAN not selected), we see the
> verification and the bail-out is done prior to any calculation based on
> legacy_serial_console.
Yes, you got lucky. Generating the code you wanted is one of the things
the compiler is allowed to do for UB.
> So, is it normal that UBSAN reports an error here ?
Yes. It detected undefined behaviour just fine, it did exactly its
job :-)
Segher
^ permalink raw reply
* Re: [PATCH v2 1/3] dt-bindings: i2c: mpc: Add fsl,i2c-erratum-a004447 flag
From: Rob Herring @ 2021-05-07 21:49 UTC (permalink / raw)
To: Chris Packham
Cc: devicetree, linux-kernel, wsa, andy.shevchenko, linux-i2c,
andriy.shevchenko, linuxppc-dev
In-Reply-To: <20210507004047.4454-2-chris.packham@alliedtelesis.co.nz>
On Fri, May 07, 2021 at 12:40:45PM +1200, Chris Packham wrote:
> Document the fsl,i2c-erratum-a004447 flag which indicates the presence
> of an i2c erratum on some QorIQ SoCs.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> Documentation/devicetree/bindings/i2c/i2c-mpc.yaml | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
> index 7b553d559c83..98c6fcf7bf26 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml
> @@ -46,6 +46,13 @@ properties:
> description: |
> I2C bus timeout in microseconds
>
> + fsl,i2c-erratum-a004447:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: |
> + Indicates the presence of QorIQ erratum A-004447, which
> + says that the standard i2c recovery scheme mechanism does
> + not work and an alternate implementation is needed.
The problem with adding a property for an errata is you have to update
the dtb. If you use the compatible string, then only an OS update is
needed. That assumes you have specific enough compatible strings.
Rob
^ 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