* Re: [PATCH 1/4] dma-mapping: move the remaining DMA API calls out of line
From: Alexey Kardashevskiy @ 2020-04-15 11:21 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Greg Kroah-Hartman, Joerg Roedel, linuxppc-dev, linux-kernel,
iommu, Robin Murphy, Lu Baolu
In-Reply-To: <20200415061859.GA32392@lst.de>
On 15/04/2020 16:18, Christoph Hellwig wrote:
> On Wed, Apr 15, 2020 at 12:26:04PM +1000, Alexey Kardashevskiy wrote:
>> May be this is correct and allowed (no idea) but removing exported
>> symbols at least deserves a mention in the commit log, does not it?
>>
>> The rest of the series is fine and works. Thanks,
>
> Maybe I can throw in a line, but the point is that dma_direct_*
> was exported as dma_* called them inline. Now dma_* is out of line
> and exported instead, which always was the actual API.
They become inline in 2/4.
And the fact they were exported leaves possibility that there is a
driver somewhere relying on these symbols or distro kernel won't build
because the symbol disappeared from exports (I do not know what KABI
guarantees or if mainline kernel cares). I do not care in particular but
some might, a line separated with empty lines in the commit log would do.
--
Alexey
^ permalink raw reply
* Re: [PATCH] i2c: powermac: Simplify reading the "reg" and "i2c-address" property
From: Wolfram Sang @ 2020-04-15 11:12 UTC (permalink / raw)
To: Aishwarya R
Cc: linux-kernel, Richard Fontana, Paul Mackerras, Greg Kroah-Hartman,
Thomas Gleixner, linuxppc-dev, linux-i2c
In-Reply-To: <20200408100354.17782-1-aishwaryarj100@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 323 bytes --]
On Wed, Apr 08, 2020 at 03:33:53PM +0530, Aishwarya R wrote:
> Use of_property_read_u32 to read the "reg" and "i2c-address" property
> instead of using of_get_property to check the return values.
>
> Signed-off-by: Aishwarya R <aishwaryarj100@gmail.com>
This is quite a fragile driver. Have you tested it on HW?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 4/4] mm/vmalloc: Hugepage vmalloc mappings
From: Will Deacon @ 2020-04-15 10:47 UTC (permalink / raw)
To: Nicholas Piggin
Cc: linux-arch, Catalin Marinas, x86, linux-kernel, linux-mm,
Ingo Molnar, Borislav Petkov, H. Peter Anvin, Thomas Gleixner,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20200413125303.423864-5-npiggin@gmail.com>
Hi Nick,
On Mon, Apr 13, 2020 at 10:53:03PM +1000, Nicholas Piggin wrote:
> For platforms that define HAVE_ARCH_HUGE_VMAP and support PMD vmap mappings,
> have vmalloc attempt to allocate PMD-sized pages first, before falling back
> to small pages. Allocations which use something other than PAGE_KERNEL
> protections are not permitted to use huge pages yet, not all callers expect
> this (e.g., module allocations vs strict module rwx).
>
> This gives a 6x reduction in dTLB misses for a `git diff` (of linux), from
> 45600 to 6500 and a 2.2% reduction in cycles on a 2-node POWER9.
I wonder if it's worth extending vmap() to handle higher order pages in
a similar way? That might be helpful for tracing PMUs such as Arm SPE,
where the CPU streams tracing data out to a virtually addressed buffer
(see rb_alloc_aux_page()).
> This can result in more internal fragmentation and memory overhead for a
> given allocation. It can also cause greater NUMA unbalance on hashdist
> allocations.
>
> There may be other callers that expect small pages under vmalloc but use
> PAGE_KERNEL, I'm not sure if it's feasible to catch them all. An
> alternative would be a new function or flag which enables large mappings,
> and use that in callers.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> include/linux/vmalloc.h | 2 +
> mm/vmalloc.c | 135 +++++++++++++++++++++++++++++-----------
> 2 files changed, 102 insertions(+), 35 deletions(-)
>
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index 291313a7e663..853b82eac192 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -24,6 +24,7 @@ struct notifier_block; /* in notifier.h */
> #define VM_UNINITIALIZED 0x00000020 /* vm_struct is not fully initialized */
> #define VM_NO_GUARD 0x00000040 /* don't add guard page */
> #define VM_KASAN 0x00000080 /* has allocated kasan shadow memory */
> +#define VM_HUGE_PAGES 0x00000100 /* may use huge pages */
Please can you add a check for this in the arm64 change_memory_common()
code? Other architectures might need something similar, but we need to
forbid changing memory attributes for portions of the huge page.
In general, I'm a bit wary of software table walkers tripping over this.
For example, I don't think apply_to_existing_page_range() can handle
huge mappings at all, but the one user (KASAN) only ever uses page mappings
so it's ok there.
> @@ -2325,9 +2356,11 @@ static struct vm_struct *__get_vm_area_node(unsigned long size,
> if (unlikely(!size))
> return NULL;
>
> - if (flags & VM_IOREMAP)
> - align = 1ul << clamp_t(int, get_count_order_long(size),
> - PAGE_SHIFT, IOREMAP_MAX_ORDER);
> + if (flags & VM_IOREMAP) {
> + align = max(align,
> + 1ul << clamp_t(int, get_count_order_long(size),
> + PAGE_SHIFT, IOREMAP_MAX_ORDER));
> + }
I don't follow this part. Please could you explain why you're potentially
aligning above IOREMAP_MAX_ORDER? It doesn't seem to follow from the rest
of the patch.
Cheers,
Will
^ permalink raw reply
* Re: [PATCH v2] Fix: buffer overflow during hvc_alloc().
From: Greg KH @ 2020-04-15 10:34 UTC (permalink / raw)
To: andrew; +Cc: jslaby, linuxppc-dev, linux-kernel, virtualization
In-Reply-To: <20200414191503.3471783-1-andrew@daynix.com>
On Tue, Apr 14, 2020 at 10:15:03PM +0300, andrew@daynix.com wrote:
> From: Andrew Melnychenko <andrew@daynix.com>
>
> If there is a lot(more then 16) of virtio-console devices
> or virtio_console module is reloaded
> - buffers 'vtermnos' and 'cons_ops' are overflowed.
> In older kernels it overruns spinlock which leads to kernel freezing:
> https://bugzilla.redhat.com/show_bug.cgi?id=1786239
>
> To reproduce the issue, you can try simple script that
> loads/unloads module. Something like this:
> while [ 1 ]
> do
> modprobe virtio_console
> sleep 2
> modprobe -r virtio_console
> sleep 2
> done
>
> Description of problem:
> Guest get 'Call Trace' when loading module "virtio_console"
> and unloading it frequently - clearly reproduced on kernel-4.18.0:
>
> [ 81.498208] ------------[ cut here ]------------
> [ 81.499263] pvqspinlock: lock 0xffffffff92080020 has corrupted value 0xc0774ca0!
> [ 81.501000] WARNING: CPU: 0 PID: 785 at kernel/locking/qspinlock_paravirt.h:500 __pv_queued_spin_unlock_slowpath+0xc0/0xd0
> [ 81.503173] Modules linked in: virtio_console fuse xt_CHECKSUM ipt_MASQUERADE xt_conntrack ipt_REJECT nft_counter nf_nat_tftp nft_objref nf_conntrack_tftp tun bridge stp llc nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nf_tables_set nft_chain_nat_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 nft_chain_route_ipv6 nft_chain_nat_ipv4 nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack nft_chain_route_ipv4 ip6_tables nft_compat ip_set nf_tables nfnetlink sunrpc bochs_drm drm_vram_helper ttm drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops drm i2c_piix4 pcspkr crct10dif_pclmul crc32_pclmul joydev ghash_clmulni_intel ip_tables xfs libcrc32c sd_mod sg ata_generic ata_piix virtio_net libata crc32c_intel net_failover failover serio_raw virtio_scsi dm_mirror dm_region_hash dm_log dm_mod [last unloaded: virtio_console]
> [ 81.517019] CPU: 0 PID: 785 Comm: kworker/0:2 Kdump: loaded Not tainted 4.18.0-167.el8.x86_64 #1
> [ 81.518639] Hardware name: Red Hat KVM, BIOS 1.12.0-5.scrmod+el8.2.0+5159+d8aa4d83 04/01/2014
> [ 81.520205] Workqueue: events control_work_handler [virtio_console]
> [ 81.521354] RIP: 0010:__pv_queued_spin_unlock_slowpath+0xc0/0xd0
> [ 81.522450] Code: 07 00 48 63 7a 10 e8 bf 64 f5 ff 66 90 c3 8b 05 e6 cf d6 01 85 c0 74 01 c3 8b 17 48 89 fe 48 c7 c7 38 4b 29 91 e8 3a 6c fa ff <0f> 0b c3 0f 0b 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 48
> [ 81.525830] RSP: 0018:ffffb51a01ffbd70 EFLAGS: 00010282
> [ 81.526798] RAX: 0000000000000000 RBX: 0000000000000010 RCX: 0000000000000000
> [ 81.528110] RDX: ffff9e66f1826480 RSI: ffff9e66f1816a08 RDI: ffff9e66f1816a08
> [ 81.529437] RBP: ffffffff9153ff10 R08: 000000000000026c R09: 0000000000000053
> [ 81.530732] R10: 0000000000000000 R11: ffffb51a01ffbc18 R12: ffff9e66cd682200
> [ 81.532133] R13: ffffffff9153ff10 R14: ffff9e6685569500 R15: ffff9e66cd682000
> [ 81.533442] FS: 0000000000000000(0000) GS:ffff9e66f1800000(0000) knlGS:0000000000000000
> [ 81.534914] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 81.535971] CR2: 00005624c55b14d0 CR3: 00000003a023c000 CR4: 00000000003406f0
> [ 81.537283] Call Trace:
> [ 81.537763] __raw_callee_save___pv_queued_spin_unlock_slowpath+0x11/0x20
> [ 81.539011] .slowpath+0x9/0xe
> [ 81.539585] hvc_alloc+0x25e/0x300
> [ 81.540237] init_port_console+0x28/0x100 [virtio_console]
> [ 81.541251] handle_control_message.constprop.27+0x1c4/0x310 [virtio_console]
> [ 81.542546] control_work_handler+0x70/0x10c [virtio_console]
> [ 81.543601] process_one_work+0x1a7/0x3b0
> [ 81.544356] worker_thread+0x30/0x390
> [ 81.545025] ? create_worker+0x1a0/0x1a0
> [ 81.545749] kthread+0x112/0x130
> [ 81.546358] ? kthread_flush_work_fn+0x10/0x10
> [ 81.547183] ret_from_fork+0x22/0x40
> [ 81.547842] ---[ end trace aa97649bd16c8655 ]---
> [ 83.546539] general protection fault: 0000 [#1] SMP NOPTI
> [ 83.547422] CPU: 5 PID: 3225 Comm: modprobe Kdump: loaded Tainted: G W --------- - - 4.18.0-167.el8.x86_64 #1
> [ 83.549191] Hardware name: Red Hat KVM, BIOS 1.12.0-5.scrmod+el8.2.0+5159+d8aa4d83 04/01/2014
> [ 83.550544] RIP: 0010:__pv_queued_spin_lock_slowpath+0x19a/0x2a0
> [ 83.551504] Code: c4 c1 ea 12 41 be 01 00 00 00 4c 8d 6d 14 41 83 e4 03 8d 42 ff 49 c1 e4 05 48 98 49 81 c4 40 a5 02 00 4c 03 24 c5 60 48 34 91 <49> 89 2c 24 b8 00 80 00 00 eb 15 84 c0 75 0a 41 0f b6 54 24 14 84
> [ 83.554449] RSP: 0018:ffffb51a0323fdb0 EFLAGS: 00010202
> [ 83.555290] RAX: 000000000000301c RBX: ffffffff92080020 RCX: 0000000000000001
> [ 83.556426] RDX: 000000000000301d RSI: 0000000000000000 RDI: 0000000000000000
> [ 83.557556] RBP: ffff9e66f196a540 R08: 000000000000028a R09: ffff9e66d2757788
> [ 83.558688] R10: 0000000000000000 R11: 0000000000000000 R12: 646e61725f770b07
> [ 83.559821] R13: ffff9e66f196a554 R14: 0000000000000001 R15: 0000000000180000
> [ 83.560958] FS: 00007fd5032e8740(0000) GS:ffff9e66f1940000(0000) knlGS:0000000000000000
> [ 83.562233] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 83.563149] CR2: 00007fd5022b0da0 CR3: 000000038c334000 CR4: 00000000003406e0
>
> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> ---
> drivers/tty/hvc/hvc_console.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
What changed from v1? Always put this below the --- line.
v3 please?
thanks,
greg k-h
^ permalink raw reply
* [PATCH] powerpc/8xx: Reduce time spent in allow_user_access() and friends
From: Christophe Leroy @ 2020-04-15 10:06 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
To enable/disable kernel access to user space, the 8xx has to
modify the properties of access group 1. This is done by writing
predefined values into SPRN_Mx_AP registers.
As of today, a __put_user() gives:
00000d64 <my_test>:
d64: 3d 20 4f ff lis r9,20479
d68: 61 29 ff ff ori r9,r9,65535
d6c: 7d 3a c3 a6 mtspr 794,r9
d70: 39 20 00 00 li r9,0
d74: 90 83 00 00 stw r4,0(r3)
d78: 3d 20 6f ff lis r9,28671
d7c: 61 29 ff ff ori r9,r9,65535
d80: 7d 3a c3 a6 mtspr 794,r9
d84: 4e 80 00 20 blr
Because only groups 0 and 1 are used, the definition of
groups 2 to 15 doesn't matter.
By setting unused bits to 0 instead on 1, one instruction is
removed for each lock and unlock action:
00000d5c <my_test>:
d5c: 3d 20 40 00 lis r9,16384
d60: 7d 3a c3 a6 mtspr 794,r9
d64: 39 20 00 00 li r9,0
d68: 90 83 00 00 stw r4,0(r3)
d6c: 3d 20 60 00 lis r9,24576
d70: 7d 3a c3 a6 mtspr 794,r9
d74: 4e 80 00 20 blr
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/nohash/32/mmu-8xx.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/include/asm/nohash/32/mmu-8xx.h b/arch/powerpc/include/asm/nohash/32/mmu-8xx.h
index 76af5b0cb16e..6aa3464a88ed 100644
--- a/arch/powerpc/include/asm/nohash/32/mmu-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/mmu-8xx.h
@@ -37,16 +37,16 @@
* Therefore, we define 2 APG groups. lsb is _PMD_USER
* 0 => Kernel => 01 (all accesses performed according to page definition)
* 1 => User => 00 (all accesses performed as supervisor iaw page definition)
- * 2-16 => NA => 11 (all accesses performed as user iaw page definition)
+ * 2-15 => Not Used
*/
-#define MI_APG_INIT 0x4fffffff
+#define MI_APG_INIT 0x40000000
/*
* 0 => Kernel => 01 (all accesses performed according to page definition)
* 1 => User => 10 (all accesses performed according to swaped page definition)
- * 2-16 => NA => 11 (all accesses performed as user iaw page definition)
+ * 2-15 => Not Used
*/
-#define MI_APG_KUEP 0x6fffffff
+#define MI_APG_KUEP 0x60000000
/* The effective page number register. When read, contains the information
* about the last instruction TLB miss. When MI_RPN is written, bits in
@@ -117,16 +117,16 @@
* Therefore, we define 2 APG groups. lsb is _PMD_USER
* 0 => Kernel => 01 (all accesses performed according to page definition)
* 1 => User => 00 (all accesses performed as supervisor iaw page definition)
- * 2-16 => NA => 11 (all accesses performed as user iaw page definition)
+ * 2-15 => Not Used
*/
-#define MD_APG_INIT 0x4fffffff
+#define MD_APG_INIT 0x40000000
/*
* 0 => No user => 01 (all accesses performed according to page definition)
* 1 => User => 10 (all accesses performed according to swaped page definition)
- * 2-16 => NA => 11 (all accesses performed as user iaw page definition)
+ * 2-15 => Not Used
*/
-#define MD_APG_KUAP 0x6fffffff
+#define MD_APG_KUAP 0x60000000
/* The effective page number register. When read, contains the information
* about the last instruction TLB miss. When MD_RPN is written, bits in
--
2.25.0
^ permalink raw reply related
* [PATCH v2] powerpc/uaccess: Implement unsafe_put_user() using 'asm goto'
From: Christophe Leroy @ 2020-04-15 9:25 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin,
segher
Cc: linuxppc-dev, linux-kernel
unsafe_put_user() is designed to take benefit of 'asm goto'.
Instead of using the standard __put_user() approach and branch
based on the returned error, use 'asm goto' and make the
exception code branch directly to the error label. There is
no code anymore in the fixup section.
This change significantly simplifies functions using
unsafe_put_user()
Small exemple of the benefit with the following code:
struct test {
u32 item1;
u16 item2;
u8 item3;
u64 item4;
};
int set_test_to_user(struct test __user *test, u32 item1, u16 item2, u8 item3, u64 item4)
{
unsafe_put_user(item1, &test->item1, failed);
unsafe_put_user(item2, &test->item2, failed);
unsafe_put_user(item3, &test->item3, failed);
unsafe_put_user(item4, &test->item4, failed);
return 0;
failed:
return -EFAULT;
}
Before the patch:
00000be8 <set_test_to_user>:
be8: 39 20 00 00 li r9,0
bec: 90 83 00 00 stw r4,0(r3)
bf0: 2f 89 00 00 cmpwi cr7,r9,0
bf4: 40 9e 00 38 bne cr7,c2c <set_test_to_user+0x44>
bf8: b0 a3 00 04 sth r5,4(r3)
bfc: 2f 89 00 00 cmpwi cr7,r9,0
c00: 40 9e 00 2c bne cr7,c2c <set_test_to_user+0x44>
c04: 98 c3 00 06 stb r6,6(r3)
c08: 2f 89 00 00 cmpwi cr7,r9,0
c0c: 40 9e 00 20 bne cr7,c2c <set_test_to_user+0x44>
c10: 90 e3 00 08 stw r7,8(r3)
c14: 91 03 00 0c stw r8,12(r3)
c18: 21 29 00 00 subfic r9,r9,0
c1c: 7d 29 49 10 subfe r9,r9,r9
c20: 38 60 ff f2 li r3,-14
c24: 7d 23 18 38 and r3,r9,r3
c28: 4e 80 00 20 blr
c2c: 38 60 ff f2 li r3,-14
c30: 4e 80 00 20 blr
00000000 <.fixup>:
...
b8: 39 20 ff f2 li r9,-14
bc: 48 00 00 00 b bc <.fixup+0xbc>
bc: R_PPC_REL24 .text+0xbf0
c0: 39 20 ff f2 li r9,-14
c4: 48 00 00 00 b c4 <.fixup+0xc4>
c4: R_PPC_REL24 .text+0xbfc
c8: 39 20 ff f2 li r9,-14
cc: 48 00 00 00 b cc <.fixup+0xcc>
cc: R_PPC_REL24 .text+0xc08
d0: 39 20 ff f2 li r9,-14
d4: 48 00 00 00 b d4 <.fixup+0xd4>
d4: R_PPC_REL24 .text+0xc18
00000000 <__ex_table>:
...
a0: R_PPC_REL32 .text+0xbec
a4: R_PPC_REL32 .fixup+0xb8
a8: R_PPC_REL32 .text+0xbf8
ac: R_PPC_REL32 .fixup+0xc0
b0: R_PPC_REL32 .text+0xc04
b4: R_PPC_REL32 .fixup+0xc8
b8: R_PPC_REL32 .text+0xc10
bc: R_PPC_REL32 .fixup+0xd0
c0: R_PPC_REL32 .text+0xc14
c4: R_PPC_REL32 .fixup+0xd0
After the patch:
00000be8 <set_test_to_user>:
be8: 90 83 00 00 stw r4,0(r3)
bec: b0 a3 00 04 sth r5,4(r3)
bf0: 98 c3 00 06 stb r6,6(r3)
bf4: 90 e3 00 08 stw r7,8(r3)
bf8: 91 03 00 0c stw r8,12(r3)
bfc: 38 60 00 00 li r3,0
c00: 4e 80 00 20 blr
c04: 38 60 ff f2 li r3,-14
c08: 4e 80 00 20 blr
00000000 <__ex_table>:
...
a0: R_PPC_REL32 .text+0xbe8
a4: R_PPC_REL32 .text+0xc04
a8: R_PPC_REL32 .text+0xbec
ac: R_PPC_REL32 .text+0xc04
b0: R_PPC_REL32 .text+0xbf0
b4: R_PPC_REL32 .text+0xc04
b8: R_PPC_REL32 .text+0xbf4
bc: R_PPC_REL32 .text+0xc04
c0: R_PPC_REL32 .text+0xbf8
c4: R_PPC_REL32 .text+0xc04
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2:
- Grouped most __goto() macros together
- Removed stuff in .fixup section, referencing the error label
directly from the extable
- Using more flexible addressing in asm.
---
arch/powerpc/include/asm/uaccess.h | 61 +++++++++++++++++++++++++-----
1 file changed, 52 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index dee71e9c7618..5d323e4f2ce1 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -93,12 +93,12 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
#define __get_user(x, ptr) \
__get_user_nocheck((x), (ptr), sizeof(*(ptr)), true)
#define __put_user(x, ptr) \
- __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), true)
+ __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
+#define __put_user_goto(x, ptr, label) \
+ __put_user_nocheck_goto((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), label)
#define __get_user_allowed(x, ptr) \
__get_user_nocheck((x), (ptr), sizeof(*(ptr)), false)
-#define __put_user_allowed(x, ptr) \
- __put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), false)
#define __get_user_inatomic(x, ptr) \
__get_user_nosleep((x), (ptr), sizeof(*(ptr)))
@@ -162,17 +162,14 @@ do { \
prevent_write_to_user(ptr, size); \
} while (0)
-#define __put_user_nocheck(x, ptr, size, do_allow) \
+#define __put_user_nocheck(x, ptr, size) \
({ \
long __pu_err; \
__typeof__(*(ptr)) __user *__pu_addr = (ptr); \
if (!is_kernel_addr((unsigned long)__pu_addr)) \
might_fault(); \
__chk_user_ptr(ptr); \
- if (do_allow) \
- __put_user_size((x), __pu_addr, (size), __pu_err); \
- else \
- __put_user_size_allowed((x), __pu_addr, (size), __pu_err); \
+ __put_user_size((x), __pu_addr, (size), __pu_err); \
__pu_err; \
})
@@ -196,6 +193,52 @@ do { \
})
+#define __put_user_asm_goto(x, addr, label, op) \
+ asm volatile goto( \
+ "1: " op "%U1%X1 %0,%1 # put_user\n" \
+ EX_TABLE(1b, %l2) \
+ : \
+ : "r" (x), "m" (*addr) \
+ : \
+ : label)
+
+#ifdef __powerpc64__
+#define __put_user_asm2_goto(x, ptr, label) \
+ __put_user_asm_goto(x, ptr, label, "std")
+#else /* __powerpc64__ */
+#define __put_user_asm2_goto(x, addr, label) \
+ asm volatile goto( \
+ "1: stw%U1%X1 %0, %1\n" \
+ "2: stw%U1%X1 %L0, %L1\n" \
+ EX_TABLE(1b, %l2) \
+ EX_TABLE(2b, %l2) \
+ : \
+ : "r" (x), "m" (*addr) \
+ : \
+ : label)
+#endif /* __powerpc64__ */
+
+#define __put_user_size_goto(x, ptr, size, label) \
+do { \
+ switch (size) { \
+ case 1: __put_user_asm_goto(x, ptr, label, "stb"); break; \
+ case 2: __put_user_asm_goto(x, ptr, label, "sth"); break; \
+ case 4: __put_user_asm_goto(x, ptr, label, "stw"); break; \
+ case 8: __put_user_asm2_goto(x, ptr, label); break; \
+ default: __put_user_bad(); \
+ } \
+} while (0)
+
+#define __put_user_nocheck_goto(x, ptr, size, label) \
+do { \
+ __typeof__(*(ptr)) __user *__pu_addr = (ptr); \
+ if (!is_kernel_addr((unsigned long)__pu_addr)) \
+ might_fault(); \
+ __chk_user_ptr(ptr); \
+ __put_user_size_goto((x), __pu_addr, (size), label); \
+} while (0)
+
+
extern long __get_user_bad(void);
/*
@@ -470,7 +513,7 @@ static __must_check inline bool user_access_begin(const void __user *ptr, size_t
#define unsafe_op_wrap(op, err) do { if (unlikely(op)) goto err; } while (0)
#define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
-#define unsafe_put_user(x, p, e) unsafe_op_wrap(__put_user_allowed(x, p), e)
+#define unsafe_put_user(x, p, e) __put_user_goto(x, p, e)
#define unsafe_copy_to_user(d, s, l, e) \
unsafe_op_wrap(raw_copy_to_user_allowed(d, s, l), e)
--
2.25.0
^ permalink raw reply related
* [PATCH] powerpc/uaccess: Use flexible addressing with __put_user()/__get_user()
From: Christophe Leroy @ 2020-04-15 9:20 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, npiggin,
segher
Cc: linuxppc-dev, linux-kernel
At the time being, __put_user()/__get_user() and friends only use
register indirect with immediate index addressing, with the index
set to 0. Ex:
lwz reg1, 0(reg2)
Give the compiler the opportunity to use other adressing modes
whenever possible, to get more optimised code.
Hereunder is a small exemple:
struct test {
u32 item1;
u16 item2;
u8 item3;
u64 item4;
};
int set_test_user(struct test __user *from, struct test __user *to, int idx)
{
int err;
u32 item1;
u16 item2;
u8 item3;
u64 item4;
err = __get_user(item1, &from->item1);
err |= __get_user(item2, &from->item2);
err |= __get_user(item3, &from->item3);
err |= __get_user(item4, &from->item4);
err |= __put_user(item1, &to->item1);
err |= __put_user(item2, &to->item2);
err |= __put_user(item3, &to->item3);
err |= __put_user(item4, &to->item4);
return err;
}
Before the patch:
00000df0 <set_test_user>:
df0: 94 21 ff f0 stwu r1,-16(r1)
df4: 39 40 00 00 li r10,0
df8: 93 c1 00 08 stw r30,8(r1)
dfc: 93 e1 00 0c stw r31,12(r1)
e00: 7d 49 53 78 mr r9,r10
e04: 80 a3 00 00 lwz r5,0(r3)
e08: 38 e3 00 04 addi r7,r3,4
e0c: 7d 46 53 78 mr r6,r10
e10: a0 e7 00 00 lhz r7,0(r7)
e14: 7d 29 33 78 or r9,r9,r6
e18: 39 03 00 06 addi r8,r3,6
e1c: 7d 46 53 78 mr r6,r10
e20: 89 08 00 00 lbz r8,0(r8)
e24: 7d 29 33 78 or r9,r9,r6
e28: 38 63 00 08 addi r3,r3,8
e2c: 7d 46 53 78 mr r6,r10
e30: 83 c3 00 00 lwz r30,0(r3)
e34: 83 e3 00 04 lwz r31,4(r3)
e38: 7d 29 33 78 or r9,r9,r6
e3c: 7d 43 53 78 mr r3,r10
e40: 90 a4 00 00 stw r5,0(r4)
e44: 7d 29 1b 78 or r9,r9,r3
e48: 38 c4 00 04 addi r6,r4,4
e4c: 7d 43 53 78 mr r3,r10
e50: b0 e6 00 00 sth r7,0(r6)
e54: 7d 29 1b 78 or r9,r9,r3
e58: 38 e4 00 06 addi r7,r4,6
e5c: 7d 43 53 78 mr r3,r10
e60: 99 07 00 00 stb r8,0(r7)
e64: 7d 23 1b 78 or r3,r9,r3
e68: 38 84 00 08 addi r4,r4,8
e6c: 93 c4 00 00 stw r30,0(r4)
e70: 93 e4 00 04 stw r31,4(r4)
e74: 7c 63 53 78 or r3,r3,r10
e78: 83 c1 00 08 lwz r30,8(r1)
e7c: 83 e1 00 0c lwz r31,12(r1)
e80: 38 21 00 10 addi r1,r1,16
e84: 4e 80 00 20 blr
After the patch:
00000dbc <set_test_user>:
dbc: 39 40 00 00 li r10,0
dc0: 7d 49 53 78 mr r9,r10
dc4: 80 03 00 00 lwz r0,0(r3)
dc8: 7d 48 53 78 mr r8,r10
dcc: a1 63 00 04 lhz r11,4(r3)
dd0: 7d 29 43 78 or r9,r9,r8
dd4: 7d 48 53 78 mr r8,r10
dd8: 88 a3 00 06 lbz r5,6(r3)
ddc: 7d 29 43 78 or r9,r9,r8
de0: 7d 48 53 78 mr r8,r10
de4: 80 c3 00 08 lwz r6,8(r3)
de8: 80 e3 00 0c lwz r7,12(r3)
dec: 7d 29 43 78 or r9,r9,r8
df0: 7d 43 53 78 mr r3,r10
df4: 90 04 00 00 stw r0,0(r4)
df8: 7d 29 1b 78 or r9,r9,r3
dfc: 7d 43 53 78 mr r3,r10
e00: b1 64 00 04 sth r11,4(r4)
e04: 7d 29 1b 78 or r9,r9,r3
e08: 7d 43 53 78 mr r3,r10
e0c: 98 a4 00 06 stb r5,6(r4)
e10: 7d 23 1b 78 or r3,r9,r3
e14: 90 c4 00 08 stw r6,8(r4)
e18: 90 e4 00 0c stw r7,12(r4)
e1c: 7c 63 53 78 or r3,r3,r10
e20: 4e 80 00 20 blr
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/include/asm/uaccess.h | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 2f500debae21..dee71e9c7618 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -114,7 +114,7 @@ extern long __put_user_bad(void);
*/
#define __put_user_asm(x, addr, err, op) \
__asm__ __volatile__( \
- "1: " op " %1,0(%2) # put_user\n" \
+ "1: " op "%U2%X2 %1,%2 # put_user\n" \
"2:\n" \
".section .fixup,\"ax\"\n" \
"3: li %0,%3\n" \
@@ -122,7 +122,7 @@ extern long __put_user_bad(void);
".previous\n" \
EX_TABLE(1b, 3b) \
: "=r" (err) \
- : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
+ : "r" (x), "m" (*addr), "i" (-EFAULT), "0" (err))
#ifdef __powerpc64__
#define __put_user_asm2(x, ptr, retval) \
@@ -130,8 +130,8 @@ extern long __put_user_bad(void);
#else /* __powerpc64__ */
#define __put_user_asm2(x, addr, err) \
__asm__ __volatile__( \
- "1: stw %1,0(%2)\n" \
- "2: stw %1+1,4(%2)\n" \
+ "1: stw%U2%X2 %1,%2\n" \
+ "2: stw%U2%X2 %L1,%L2\n" \
"3:\n" \
".section .fixup,\"ax\"\n" \
"4: li %0,%3\n" \
@@ -140,7 +140,7 @@ extern long __put_user_bad(void);
EX_TABLE(1b, 4b) \
EX_TABLE(2b, 4b) \
: "=r" (err) \
- : "r" (x), "b" (addr), "i" (-EFAULT), "0" (err))
+ : "r" (x), "m" (*addr), "i" (-EFAULT), "0" (err))
#endif /* __powerpc64__ */
#define __put_user_size_allowed(x, ptr, size, retval) \
@@ -217,7 +217,7 @@ extern long __get_user_bad(void);
#define __get_user_asm(x, addr, err, op) \
__asm__ __volatile__( \
- "1: "op" %1,0(%2) # get_user\n" \
+ "1: "op"%U2%X2 %1, %2 # get_user\n" \
"2:\n" \
".section .fixup,\"ax\"\n" \
"3: li %0,%3\n" \
@@ -226,7 +226,7 @@ extern long __get_user_bad(void);
".previous\n" \
EX_TABLE(1b, 3b) \
: "=r" (err), "=r" (x) \
- : "b" (addr), "i" (-EFAULT), "0" (err))
+ : "m" (*addr), "i" (-EFAULT), "0" (err))
#ifdef __powerpc64__
#define __get_user_asm2(x, addr, err) \
@@ -234,8 +234,8 @@ extern long __get_user_bad(void);
#else /* __powerpc64__ */
#define __get_user_asm2(x, addr, err) \
__asm__ __volatile__( \
- "1: lwz %1,0(%2)\n" \
- "2: lwz %1+1,4(%2)\n" \
+ "1: lwz%U2%X2 %1, %2\n" \
+ "2: lwz%U2%X2 %L1, %L2\n" \
"3:\n" \
".section .fixup,\"ax\"\n" \
"4: li %0,%3\n" \
@@ -246,7 +246,7 @@ extern long __get_user_bad(void);
EX_TABLE(1b, 4b) \
EX_TABLE(2b, 4b) \
: "=r" (err), "=&r" (x) \
- : "b" (addr), "i" (-EFAULT), "0" (err))
+ : "m" (*addr), "i" (-EFAULT), "0" (err))
#endif /* __powerpc64__ */
#define __get_user_size_allowed(x, ptr, size, retval) \
@@ -256,10 +256,10 @@ do { \
if (size > sizeof(x)) \
(x) = __get_user_bad(); \
switch (size) { \
- case 1: __get_user_asm(x, ptr, retval, "lbz"); break; \
- case 2: __get_user_asm(x, ptr, retval, "lhz"); break; \
- case 4: __get_user_asm(x, ptr, retval, "lwz"); break; \
- case 8: __get_user_asm2(x, ptr, retval); break; \
+ case 1: __get_user_asm(x, (u8 __user *)ptr, retval, "lbz"); break; \
+ case 2: __get_user_asm(x, (u16 __user *)ptr, retval, "lhz"); break; \
+ case 4: __get_user_asm(x, (u32 __user *)ptr, retval, "lwz"); break; \
+ case 8: __get_user_asm2(x, (u64 __user *)ptr, retval); break; \
default: (x) = __get_user_bad(); \
} \
} while (0)
--
2.25.0
^ permalink raw reply related
* Re: [RFC PATCH] powerpc/lib: Fixing use a temporary mm for code patching
From: Christophe Leroy @ 2020-04-15 9:12 UTC (permalink / raw)
To: Christopher M Riedl; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <581069710.188209.1586927814880@privateemail.com>
Le 15/04/2020 à 07:16, Christopher M Riedl a écrit :
>> On March 26, 2020 9:42 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>
>>
>> This patch fixes the RFC series identified below.
>> It fixes three points:
>> - Failure with CONFIG_PPC_KUAP
>> - Failure to write do to lack of DIRTY bit set on the 8xx
>> - Inadequaly complex WARN post verification
>>
>> However, it has an impact on the CPU load. Here is the time
>> needed on an 8xx to run the ftrace selftests without and
>> with this series:
>> - Without CONFIG_STRICT_KERNEL_RWX ==> 38 seconds
>> - With CONFIG_STRICT_KERNEL_RWX ==> 40 seconds
>> - With CONFIG_STRICT_KERNEL_RWX + this series ==> 43 seconds
>>
>> Link: https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/lib/code-patching.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
>> index f156132e8975..4ccff427592e 100644
>> --- a/arch/powerpc/lib/code-patching.c
>> +++ b/arch/powerpc/lib/code-patching.c
>> @@ -97,6 +97,7 @@ static int map_patch(const void *addr, struct patch_mapping *patch_mapping)
>> }
>>
>> pte = mk_pte(page, pgprot);
>> + pte = pte_mkdirty(pte);
>> set_pte_at(patching_mm, patching_addr, ptep, pte);
>>
>> init_temp_mm(&patch_mapping->temp_mm, patching_mm);
>> @@ -168,7 +169,9 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
>> (offset_in_page((unsigned long)addr) /
>> sizeof(unsigned int));
>>
>> + allow_write_to_user(patch_addr, sizeof(instr));
>> __patch_instruction(addr, instr, patch_addr);
>> + prevent_write_to_user(patch_addr, sizeof(instr));
>>
>
> On radix we can map the page with PAGE_KERNEL protection which ends up
> setting EAA[0] in the radix PTE. This means the KUAP (AMR) protection is
> ignored (ISA v3.0b Fig. 35) since we are accessing the page from MSR[PR]=0.
>
> Can we employ a similar approach on the 8xx? I would prefer *not* to wrap
> the __patch_instruction() with the allow_/prevent_write_to_user() KUAP things
> because this is a temporary kernel mapping which really isn't userspace in
> the usual sense.
On the 8xx, that's pretty different.
The PTE doesn't control whether a page is user page or a kernel page.
The only thing that is set in the PTE is whether a page is linked to a
given PID or not.
PAGE_KERNEL tells that the page can be addressed with any PID.
The user access right is given by a kind of zone, which is in the PGD
entry. Every pages above PAGE_OFFSET are defined as belonging to zone 0.
Every pages below PAGE_OFFSET are defined as belonging to zone 1.
By default, zone 0 can only be accessed by kernel, and zone 1 can only
be accessed by user. When kernel wants to access zone 1, it temporarily
changes properties of zone 1 to allow both kernel and user accesses.
So, if your mapping is below PAGE_OFFSET, it is in zone 1 and kernel
must unlock it to access it.
And this is more or less the same on hash/32. This is managed by segment
registers. One segment register corresponds to a 256Mbytes area. Every
pages below PAGE_OFFSET can only be read by default by kernel. Only user
can write if the PTE allows it. When the kernel needs to write at an
address below PAGE_OFFSET, it must change the segment properties in the
corresponding segment register.
So, for both cases, if we want to have it local to a task while still
allowing kernel access, it means we have to define a new special area
between TASK_SIZE and PAGE_OFFSET which belongs to kernel zone.
That looks complex to me for a small benefit, especially as 8xx is not
SMP and neither are most of the hash/32 targets.
Christophe
^ permalink raw reply
* Re: [RFC PATCH 3/3] powerpc/lib: Use a temporary mm for code patching
From: Christophe Leroy @ 2020-04-15 8:45 UTC (permalink / raw)
To: Christopher M Riedl, linuxppc-dev
In-Reply-To: <32766971.188162.1586927476788@privateemail.com>
Le 15/04/2020 à 07:11, Christopher M Riedl a écrit :
>> On March 24, 2020 11:25 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>
>>
>> Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
>>> Currently, code patching a STRICT_KERNEL_RWX exposes the temporary
>>> mappings to other CPUs. These mappings should be kept local to the CPU
>>> doing the patching. Use the pre-initialized temporary mm and patching
>>> address for this purpose. Also add a check after patching to ensure the
>>> patch succeeded.
>>>
>>> Based on x86 implementation:
>>>
>>> commit b3fd8e83ada0
>>> ("x86/alternatives: Use temporary mm for text poking")
>>>
>>> Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
>>> ---
>>> arch/powerpc/lib/code-patching.c | 128 ++++++++++++++-----------------
>>> 1 file changed, 57 insertions(+), 71 deletions(-)
>>>
>>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
>>> index 18b88ecfc5a8..f156132e8975 100644
>>> --- a/arch/powerpc/lib/code-patching.c
>>> +++ b/arch/powerpc/lib/code-patching.c
>>> @@ -19,6 +19,7 @@
>>> #include <asm/page.h>
>>> #include <asm/code-patching.h>
>>> #include <asm/setup.h>
>>> +#include <asm/mmu_context.h>
>>>
>>> static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
>>> unsigned int *patch_addr)
>>> @@ -65,99 +66,79 @@ void __init poking_init(void)
>>> pte_unmap_unlock(ptep, ptl);
>>> }
>>>
>>> -static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
>>> -
>>> -static int text_area_cpu_up(unsigned int cpu)
>>> -{
>>> - struct vm_struct *area;
>>> -
>>> - area = get_vm_area(PAGE_SIZE, VM_ALLOC);
>>> - if (!area) {
>>> - WARN_ONCE(1, "Failed to create text area for cpu %d\n",
>>> - cpu);
>>> - return -1;
>>> - }
>>> - this_cpu_write(text_poke_area, area);
>>> -
>>> - return 0;
>>> -}
>>> -
>>> -static int text_area_cpu_down(unsigned int cpu)
>>> -{
>>> - free_vm_area(this_cpu_read(text_poke_area));
>>> - return 0;
>>> -}
>>> -
>>> -/*
>>> - * Run as a late init call. This allows all the boot time patching to be done
>>> - * simply by patching the code, and then we're called here prior to
>>> - * mark_rodata_ro(), which happens after all init calls are run. Although
>>> - * BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge
>>> - * it as being preferable to a kernel that will crash later when someone tries
>>> - * to use patch_instruction().
>>> - */
>>> -static int __init setup_text_poke_area(void)
>>> -{
>>> - BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
>>> - "powerpc/text_poke:online", text_area_cpu_up,
>>> - text_area_cpu_down));
>>> -
>>> - return 0;
>>> -}
>>> -late_initcall(setup_text_poke_area);
>>> +struct patch_mapping {
>>> + spinlock_t *ptl; /* for protecting pte table */
>>> + struct temp_mm temp_mm;
>>> +};
>>>
>>> /*
>>> * This can be called for kernel text or a module.
>>> */
>>> -static int map_patch_area(void *addr, unsigned long text_poke_addr)
>>> +static int map_patch(const void *addr, struct patch_mapping *patch_mapping)
>>
>> Why change the name ?
>>
>
> It's not really an "area" anymore.
>
>>> {
>>> - unsigned long pfn;
>>> - int err;
>>> + struct page *page;
>>> + pte_t pte, *ptep;
>>> + pgprot_t pgprot;
>>>
>>> if (is_vmalloc_addr(addr))
>>> - pfn = vmalloc_to_pfn(addr);
>>> + page = vmalloc_to_page(addr);
>>> else
>>> - pfn = __pa_symbol(addr) >> PAGE_SHIFT;
>>> + page = virt_to_page(addr);
>>>
>>> - err = map_kernel_page(text_poke_addr, (pfn << PAGE_SHIFT), PAGE_KERNEL);
>>> + if (radix_enabled())
>>> + pgprot = __pgprot(pgprot_val(PAGE_KERNEL));
>>> + else
>>> + pgprot = PAGE_SHARED;
>>
>> Can you explain the difference between radix and non radix ?
>>
>> Why PAGE_KERNEL for a page that is mapped in userspace ?
>>
>> Why do you need to do __pgprot(pgprot_val(PAGE_KERNEL)) instead of just
>> using PAGE_KERNEL ?
>>
>
> On hash there is a manual check which prevents setting _PAGE_PRIVILEGED for
> kernel to userspace access in __hash_page - hence we cannot access the mapping
> if the page is mapped PAGE_KERNEL on hash. However, I would like to use
> PAGE_KERNEL here as well and am working on understanding why this check is
> done in hash and if this can change. On radix this works just fine.
>
> The page is mapped PAGE_KERNEL because the address is technically a userspace
> address - but only to keep the mapping local to this CPU doing the patching.
> PAGE_KERNEL makes it clear both in intent and protection that this is a kernel
> mapping.
>
> I think the correct way is pgprot_val(PAGE_KERNEL) since PAGE_KERNEL is defined
> as:
>
> #define PAGE_KERNEL __pgprot(_PAGE_BASE | _PAGE_KERNEL_RW)
>
> and __pgprot() is defined as:
>
> typedef struct { unsigned long pgprot; } pgprot_t;
> #define pgprot_val(x) ((x).pgprot)
> #define __pgprot(x) ((pgprot_t) { (x) })
Yes, so:
pgprot_val(__pgprot(x)) == x
You do:
pgprot = __pgprot(pgprot_val(PAGE_KERNEL));
Which is:
pgprot = __pgprot(pgprot_val(__pgprot(_PAGE_BASE | _PAGE_KERNEL_RW)));
Which is equivalent to:
pgprot = __pgprot(_PAGE_BASE | _PAGE_KERNEL_RW);
So at the end it should simply be:
pgprot = PAGE_KERNEL;
Christophe
^ permalink raw reply
* Re: [PATCH 4/8] binfmt_elf: open code copy_siginfo_to_user to kernelspace buffer
From: Arnd Bergmann @ 2020-04-15 8:20 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jeremy Kerr, linux-kernel@vger.kernel.org, Eric W . Biederman,
Linux FS-devel Mailing List, Andrew Morton, linuxppc-dev,
Alexander Viro
In-Reply-To: <20200415074514.GA1393@lst.de>
On Wed, Apr 15, 2020 at 9:45 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Tue, Apr 14, 2020 at 03:15:09PM +0200, Arnd Bergmann wrote:
> > I don't think you are changing the behavior here, but I still wonder if it
> > is in fact correct for x32: is in_x32_syscall() true here when dumping an
> > x32 compat elf process, or should this rather be set according to which
> > binfmt_elf copy is being used?
>
> The infrastructure could enable that, although it would require more
> arch hooks I think.
I was more interested in whether you can tell if it's currently broken
or not. If my feeling is right that the current code does the wrong thing
here, it would be good to at least put a FIXME comment in there.
> I'd rather keep it out of this series and to
> an interested party. Then again x32 doesn't seem to have a whole lot
> of interested parties..
Fine with me. It's on my mental list of things that we want to kill off
eventually as soon as the remaining users stop replying to questions
about it.
In fact I should really turn that into a properly maintained list in
Documentation/... that contains any options that someone has
asked about removing in the past, along with the reasons for keeping
it around and a time at which we should ask about it again.
Arnd
^ permalink raw reply
* Re: [PATCH v5 18/21] powerpc64: Add prefixed instructions to instruction data type
From: Balamuruhan S @ 2020-04-15 8:14 UTC (permalink / raw)
To: Jordan Niethe
Cc: Alistair Popple, linuxppc-dev, Nicholas Piggin, Daniel Axtens
In-Reply-To: <CACzsE9rc+g5eWko0RQdhbvWpPXUwORdKVU8Za2650ROoZAjBdg@mail.gmail.com>
On Wed, 2020-04-15 at 14:40 +1000, Jordan Niethe wrote:
> On Mon, Apr 13, 2020 at 10:04 PM Balamuruhan S <bala24@linux.ibm.com> wrote:
> > On Mon, 2020-04-06 at 18:09 +1000, Jordan Niethe wrote:
> > > For powerpc64, redefine the ppc_inst type so both word and prefixed
> > > instructions can be represented. On powerpc32 the type will remain the
> > > same. Update places which had assumed instructions to be 4 bytes long.
> > >
> > > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > > ---
> > > v4: New to series
> > > v5: - Distinguish normal instructions from prefixed instructions with a
> > > 0xff marker for the suffix.
> > > - __patch_instruction() using std for prefixed instructions
> > > ---
> > > arch/powerpc/include/asm/inst.h | 71 ++++++++++++++++++++++++++--
> > > arch/powerpc/include/asm/kprobes.h | 2 +-
> > > arch/powerpc/include/asm/uaccess.h | 31 ++++++++++--
> > > arch/powerpc/include/asm/uprobes.h | 2 +-
> > > arch/powerpc/kernel/optprobes.c | 42 ++++++++--------
> > > arch/powerpc/kernel/optprobes_head.S | 3 ++
> > > arch/powerpc/kernel/trace/ftrace.c | 26 +++++++++-
> > > arch/powerpc/lib/code-patching.c | 19 +++++---
> > > arch/powerpc/lib/feature-fixups.c | 5 +-
> > > arch/powerpc/lib/sstep.c | 4 +-
> > > arch/powerpc/xmon/xmon.c | 6 +--
> > > arch/powerpc/xmon/xmon_bpts.S | 4 +-
> > > 12 files changed, 171 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/arch/powerpc/include/asm/inst.h
> > > b/arch/powerpc/include/asm/inst.h
> > > index 70b37a35a91a..7e23e7146c66 100644
> > > --- a/arch/powerpc/include/asm/inst.h
> > > +++ b/arch/powerpc/include/asm/inst.h
> > > @@ -8,23 +8,67 @@
> > >
> > > struct ppc_inst {
> > > u32 val;
> > > +#ifdef __powerpc64__
> > > + u32 suffix;
> > > +#endif /* __powerpc64__ */
> > > } __packed;
> > >
> > > -#define ppc_inst(x) ((struct ppc_inst){ .val = x })
> > > +static inline int ppc_inst_opcode(struct ppc_inst x)
> > > +{
> > > + return x.val >> 26;
> >
> > why don't we wrap here and in `ppc_inst_opcode()` in patch 9 using
> > `ppc_inst_val()` ?
> Will do.
> >
> > > +}
> > >
> > > static inline u32 ppc_inst_val(struct ppc_inst x)
> >
> > There is another same definition below for the same function in
> > #else part of __powerpc64__ ifdef.
> Thanks
> >
> > > {
> > > return x.val;
> > > }
> > >
> > > -static inline bool ppc_inst_len(struct ppc_inst x)
> > > +#ifdef __powerpc64__
> > > +#define ppc_inst(x) ((struct ppc_inst){ .val = (x), .suffix = 0xff })
> > > +
> > > +#define ppc_inst_prefix(x, y) ((struct ppc_inst){ .val = (x), .suffix =
> > > (y)
> > > })
> > > +
> > > +static inline u32 ppc_inst_suffix(struct ppc_inst x)
> > > {
> > > - return sizeof(struct ppc_inst);
> > > + return x.suffix;
> > > }
> > >
> > > -static inline int ppc_inst_opcode(struct ppc_inst x)
> > > +static inline bool ppc_inst_prefixed(struct ppc_inst x) {
> > > + return ((ppc_inst_val(x) >> 26) == 1) && ppc_inst_suffix(x) !=
> > > 0xff;
> > > +}
> > > +
> > > +static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)
> > > {
> > > - return x.val >> 26;
> > > + return ppc_inst_prefix(swab32(ppc_inst_val(x)),
> > > + swab32(ppc_inst_suffix(x)));
> > > +}
> > > +
> > > +static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)
> > > +{
> > > + u32 val, suffix = 0xff;
> > > + val = *(u32 *)ptr;
> > > + if ((val >> 26) == 1)
> > > + suffix = *((u32 *)ptr + 1);
> > > + return ppc_inst_prefix(val, suffix);
> > > +}
> > > +
> > > +static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst
> > > x)
> > > +{
> > > + if (ppc_inst_prefixed(x)) {
> > > + *(u32 *)ptr = x.val;
> > > + *((u32 *)ptr + 1) = x.suffix;
> > > + } else {
> > > + *(u32 *)ptr = x.val;
> >
> > can we wrap here as well with `ppc_inst_val()` and `ppc_inst_suffix()` ?
> Yeah no reason not too.
> >
> > > + }
> > > +}
> > > +
> > > +#else
> > > +
> > > +#define ppc_inst(x) ((struct ppc_inst){ .val = x })
> > > +
> > > +static inline bool ppc_inst_prefixed(ppc_inst x)
> > > +{
> > > + return 0;
> >
> > Is it return !!0 or return false ?
> False probably will make more sense.
> >
> > > }
> > >
> > > static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)
> > > @@ -32,14 +76,31 @@ static inline struct ppc_inst ppc_inst_swab(struct
> > > ppc_inst x)
> > > return ppc_inst(swab32(ppc_inst_val(x)));
> > > }
> > >
> > > +static inline u32 ppc_inst_val(struct ppc_inst x)
> >
> > [...] duplicate definition that is defined outside __powerpc64__ above.
> >
> >
> > > +{
> > > + return x.val;
> > > +}
> > > +
> > > static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)
> > > {
> > > return *ptr;
> > > }
> > >
> > > +static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst
> > > x)
> > > +{
> > > + *ptr = x;
> > > +}
> > > +
> > > +#endif /* __powerpc64__ */
> > > +
> > > static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)
> > > {
> > > return !memcmp(&x, &y, sizeof(struct ppc_inst));
> > > }
> > >
> > > +static inline int ppc_inst_len(struct ppc_inst x)
> > > +{
> > > + return (ppc_inst_prefixed(x)) ? 8 : 4;
> > > +}
> > > +
> > > #endif /* _ASM_INST_H */
> > > diff --git a/arch/powerpc/include/asm/kprobes.h
> > > b/arch/powerpc/include/asm/kprobes.h
> > > index 66b3f2983b22..4fc0e15e23a5 100644
> > > --- a/arch/powerpc/include/asm/kprobes.h
> > > +++ b/arch/powerpc/include/asm/kprobes.h
> > > @@ -43,7 +43,7 @@ extern kprobe_opcode_t optprobe_template_ret[];
> > > extern kprobe_opcode_t optprobe_template_end[];
> > >
> > > /* Fixed instruction size for powerpc */
> > > -#define MAX_INSN_SIZE 1
> > > +#define MAX_INSN_SIZE 2
> > > #define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */
> > > #define MAX_OPTINSN_SIZE (optprobe_template_end -
> > > optprobe_template_entry)
> > > #define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */
> > > diff --git a/arch/powerpc/include/asm/uaccess.h
> > > b/arch/powerpc/include/asm/uaccess.h
> > > index c0a35e4586a5..5a3f486ddf02 100644
> > > --- a/arch/powerpc/include/asm/uaccess.h
> > > +++ b/arch/powerpc/include/asm/uaccess.h
> > > @@ -105,11 +105,34 @@ static inline int __access_ok(unsigned long addr,
> > > unsigned long size,
> > > #define __put_user_inatomic(x, ptr) \
> > > __put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
> > >
> > > -#define __get_user_instr(x, ptr) \
> > > - __get_user_nocheck((x).val, (u32 *)(ptr), sizeof(u32), true)
> > > +#define __get_user_instr(x, ptr) \
> > > +({ \
> > > + long __gui_ret = 0; \
> > > + unsigned int prefix, suffix; \
> > > + __gui_ret = __get_user(prefix, (unsigned int __user *)ptr);
> > > \
> > > + if (!__gui_ret && (prefix >> 26) == 1) { \
> > > + __gui_ret = __get_user(suffix, (unsigned int __user *)ptr +
> > > 1);
> > > \
> > > + (x) = ppc_inst_prefix(prefix, suffix); \
> > > + } else { \
> > > + (x) = ppc_inst(prefix); \
> > > + } \
> > > + __gui_ret; \
> > > +})
> > > +
> > > +#define __get_user_instr_inatomic(x, ptr) \
> > > +({ \
> > > + long __gui_ret = 0; \
> > > + unsigned int prefix, suffix; \
> > > + __gui_ret = __get_user_inatomic(prefix, (unsigned int __user
> > > *)ptr);
> > > \
> > > + if (!__gui_ret && (prefix >> 26) == 1) { \
> > > + __gui_ret = __get_user_inatomic(suffix, (unsigned int
> > > __user
> > > *)ptr + 1); \
> > > + (x) = ppc_inst_prefix(prefix, suffix); \
> > > + } else { \
> > > + (x) = ppc_inst(prefix); \
> > > + } \
> > > + __gui_ret; \
> > > +})
> > >
> > > -#define __get_user_instr_inatomic(x, ptr) \
> > > - __get_user_nosleep((x).val, (u32 *)(ptr), sizeof(u32))
> > > extern long __put_user_bad(void);
> > >
> > > /*
> > > diff --git a/arch/powerpc/include/asm/uprobes.h
> > > b/arch/powerpc/include/asm/uprobes.h
> > > index 7e3b329ba2d3..5bf65f5d44a9 100644
> > > --- a/arch/powerpc/include/asm/uprobes.h
> > > +++ b/arch/powerpc/include/asm/uprobes.h
> > > @@ -15,7 +15,7 @@
> > >
> > > typedef ppc_opcode_t uprobe_opcode_t;
> > >
> > > -#define MAX_UINSN_BYTES 4
> > > +#define MAX_UINSN_BYTES 8
> > > #define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
> > >
> > > /* The following alias is needed for reference from arch-agnostic code
> > > */
> > > diff --git a/arch/powerpc/kernel/optprobes.c
> > > b/arch/powerpc/kernel/optprobes.c
> > > index 684640b8fa2e..689daf430161 100644
> > > --- a/arch/powerpc/kernel/optprobes.c
> > > +++ b/arch/powerpc/kernel/optprobes.c
> > > @@ -159,38 +159,38 @@ void patch_imm32_load_insns(unsigned int val,
> > > kprobe_opcode_t *addr)
> > >
> > > /*
> > > * Generate instructions to load provided immediate 64-bit value
> > > - * to register 'r3' and patch these instructions at 'addr'.
> > > + * to register 'reg' and patch these instructions at 'addr'.
> > > */
> > > -void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
> > > +void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t
> > > *addr)
> > > {
> > > - /* lis r3,(op)@highest */
> > > - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ADDIS
> > > |
> > > ___PPC_RT(3) |
> > > + /* lis reg,(op)@highest */
> > > + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ADDIS
> > > |
> > > ___PPC_RT(reg) |
> > > ((val >> 48) & 0xffff)));
> > > addr++;
> > >
> > > - /* ori r3,r3,(op)@higher */
> > > - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> > > ___PPC_RA(3) |
> > > - ___PPC_RS(3) | ((val >> 32) & 0xffff)));
> > > + /* ori reg,reg,(op)@higher */
> > > + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> > > ___PPC_RA(reg) |
> > > + ___PPC_RS(reg) | ((val >> 32) & 0xffff)));
> > > addr++;
> > >
> > > - /* rldicr r3,r3,32,31 */
> > > - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_RLDICR
> > > |
> > > ___PPC_RA(3) |
> > > - ___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31)));
> > > + /* rldicr reg,reg,32,31 */
> > > + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_RLDICR
> > > |
> > > ___PPC_RA(reg) |
> > > + ___PPC_RS(reg) | __PPC_SH64(32) |
> > > __PPC_ME64(31)));
> > > addr++;
> > >
> > > - /* oris r3,r3,(op)@h */
> > > - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORIS |
> > > ___PPC_RA(3) |
> > > - ___PPC_RS(3) | ((val >> 16) & 0xffff)));
> > > + /* oris reg,reg,(op)@h */
> > > + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORIS |
> > > ___PPC_RA(reg) |
> > > + ___PPC_RS(reg) | ((val >> 16) & 0xffff)));
> > > addr++;
> > >
> > > - /* ori r3,r3,(op)@l */
> > > - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> > > ___PPC_RA(3) |
> > > - ___PPC_RS(3) | (val & 0xffff)));
> > > + /* ori reg,reg,(op)@l */
> > > + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> > > ___PPC_RA(reg) |
> > > + ___PPC_RS(reg) | (val & 0xffff)));
> > > }
> > >
> > > int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct
> > > kprobe
> > > *p)
> > > {
> > > - struct ppc_inst branch_op_callback, branch_emulate_step;
> > > + struct ppc_inst branch_op_callback, branch_emulate_step, temp;
> > > kprobe_opcode_t *op_callback_addr, *emulate_step_addr, *buff;
> > > long b_offset;
> > > unsigned long nip, size;
> > > @@ -240,7 +240,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, buff + TMPL_OP_IDX);
> > > + patch_imm64_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX);
> > >
> > > /*
> > > * 2. branch to optimized_callback() and emulate_step()
> > > @@ -271,7 +271,11 @@ int arch_prepare_optimized_kprobe(struct
> > > optimized_kprobe *op, struct kprobe *p)
> > > /*
> > > * 3. load instruction to be emulated into relevant register, and
> > > */
> > > - patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX);
> > > + temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
> > > + patch_imm64_load_insns(ppc_inst_val(temp) |
> > > + ((u64)ppc_inst_suffix(temp) << 32),
> >
> > did we check building for ppc32 ?
> Yeah I need to do so.
> > I doubt we might hit build failure as `ppc_inst_suffix()` macro is not
> > defined.
> >
> >
> > > + 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 cf383520843f..ff8ba4d3824d 100644
> > > --- a/arch/powerpc/kernel/optprobes_head.S
> > > +++ b/arch/powerpc/kernel/optprobes_head.S
> > > @@ -94,6 +94,9 @@ optprobe_template_insn:
> > > /* 2, Pass instruction to be emulated in r4 */
> > > nop
> > > nop
> > > + nop
> > > + nop
> > > + nop
> > >
> > > .global optprobe_template_call_emulate
> > > optprobe_template_call_emulate:
> > > diff --git a/arch/powerpc/kernel/trace/ftrace.c
> > > b/arch/powerpc/kernel/trace/ftrace.c
> > > index e78742613b36..16041a5c86d5 100644
> > > --- a/arch/powerpc/kernel/trace/ftrace.c
> > > +++ b/arch/powerpc/kernel/trace/ftrace.c
> > > @@ -41,11 +41,35 @@
> > > #define NUM_FTRACE_TRAMPS 8
> > > static unsigned long ftrace_tramps[NUM_FTRACE_TRAMPS];
> > >
> > > +#ifdef __powerpc64__
> > > static long
> > > probe_kernel_read_inst(struct ppc_inst *inst, const void *src)
> > > {
> > > - return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE);
> > > + u32 val, suffix = 0;
> >
> > don't we need to initialize suffix with 0xff ?
> Good catch, that was causing problems.
Also we can do,
if ((val >> 26) == 1)
{
err = probe_kernel_read((void *)&suffix,
src + 4, MCOUNT_INSN_SIZE);
if (err)
return err;
*inst = ppc_inst_prefix(val, suffix);
}
else
*inst = ppc_inst(val);
to explicitly show a non-prefixed instruction and we do not have to
initialize suffix to 0xff, as ppc_inst() would take care.
-- Bala
> >
> > > + long err;
> > > +
> > > + err = probe_kernel_read((void *)&val,
> > > + src, sizeof(val));
> > > + if (err)
> > > + return err;
> > > +
> > > + if ((val >> 26) == 1)
> > > + err = probe_kernel_read((void *)&suffix,
> > > + src + 4, MCOUNT_INSN_SIZE);
> > > + if (err)
> > > + return err;
> > > +
> > > + *inst = ppc_inst_prefix(val, suffix);
> > > +
> > > + return 0;
> > > }
> > > +#else
> > > +static long
> > > +probe_kernel_read_inst(struct ppc_inst *inst, const void *src)
> > > +{
> > > + return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE)
> > > +}
> > > +#endif
> > >
> > > static struct ppc_inst
> > > ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
> > > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-
> > > patching.c
> > > index c329ad657302..b4007e03d8fa 100644
> > > --- a/arch/powerpc/lib/code-patching.c
> > > +++ b/arch/powerpc/lib/code-patching.c
> > > @@ -24,12 +24,19 @@ static int __patch_instruction(struct ppc_inst
> > > *exec_addr, struct ppc_inst instr
> > > {
> > > int err = 0;
> > >
> > > - __put_user_asm(ppc_inst_val(instr), patch_addr, err, "stw");
> > > - if (err)
> > > - return err;
> > > -
> > > - asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r"
> > > (patch_addr),
> > > - "r"
> > > (exec_addr));
> > > + if (!ppc_inst_prefixed(instr)) {
> > > + __put_user_asm(ppc_inst_val(instr), patch_addr, err,
> > > "stw");
> > > + if (err)
> > > + return err;
> > > + asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r"
> > > (patch_addr),
> > > + "r"
> > > (exec_addr));
> > > + } else {
> > > + __put_user_asm((u64)ppc_inst_suffix(instr) << 32 |
> > > ppc_inst_val(instr), patch_addr, err, "std");
> > > + if (err)
> > > + return err;
> > > + asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r"
> > > (patch_addr),
> > > + "r"
> > > (exec_addr));
> > > + }
> >
> > can we keep these 2 lines out of conditions as it remains to be the same ?
> True.
> > if (err)
> > return err;
> > asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr), "r"
> > (exec_addr));
> >
> >
> > > return 0;
> > > }
> > > diff --git a/arch/powerpc/lib/feature-fixups.c
> > > b/arch/powerpc/lib/feature-
> > > fixups.c
> > > index f00dd13b1c3c..5519cec83cc8 100644
> > > --- a/arch/powerpc/lib/feature-fixups.c
> > > +++ b/arch/powerpc/lib/feature-fixups.c
> > > @@ -84,12 +84,13 @@ static int patch_feature_section(unsigned long value,
> > > struct fixup_entry *fcur)
> > > src = alt_start;
> > > dest = start;
> > >
> > > - for (; src < alt_end; src++, dest++) {
> > > + for (; src < alt_end; src = (void *)src +
> > > ppc_inst_len(ppc_inst_read(src)),
> > > + (dest = (void *)dest + ppc_inst_len(ppc_inst_read(dest)))) {
> > > if (patch_alt_instruction(src, dest, alt_start, alt_end))
> > > return 1;
> > > }
> > >
> > > - for (; dest < end; dest++)
> > > + for (; dest < end; dest = (void *)dest +
> > > ppc_inst_len(ppc_inst(PPC_INST_NOP)))
> > > raw_patch_instruction(dest, ppc_inst(PPC_INST_NOP));
> > >
> > > return 0;
> > > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> > > index 52ddd3122dc8..8b285bf11218 100644
> > > --- a/arch/powerpc/lib/sstep.c
> > > +++ b/arch/powerpc/lib/sstep.c
> > > @@ -1169,10 +1169,12 @@ int analyse_instr(struct instruction_op *op,
> > > const
> > > struct pt_regs *regs,
> > > unsigned long int imm;
> > > unsigned long int val, val2;
> > > unsigned int mb, me, sh;
> > > - unsigned int word;
> > > + unsigned int word, suffix;
> > > long ival;
> > >
> > > word = ppc_inst_val(instr);
> > > + suffix = ppc_inst_suffix(instr);
> >
> > same here, I doubt it might break for ppc32.
> Yeah I need to work on ppc32.
> >
> > -- Bala
> > > +
> > > op->type = COMPUTE;
> > >
> > > opcode = word >> 26;
> > > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > > index 6f3bcdcfc9c7..b704aebb099a 100644
> > > --- a/arch/powerpc/xmon/xmon.c
> > > +++ b/arch/powerpc/xmon/xmon.c
> > > @@ -761,8 +761,8 @@ static int xmon_bpt(struct pt_regs *regs)
> > >
> > > /* Are we at the trap at bp->instr[1] for some bp? */
> > > bp = in_breakpoint_table(regs->nip, &offset);
> > > - if (bp != NULL && offset == 4) {
> > > - regs->nip = bp->address + 4;
> > > + if (bp != NULL && (offset == 4 || offset == 8)) {
> > > + regs->nip = bp->address + offset;
> > > atomic_dec(&bp->ref_count);
> > > return 1;
> > > }
> > > @@ -863,7 +863,7 @@ static struct bpt *in_breakpoint_table(unsigned long
> > > nip,
> > > unsigned long *offp)
> > > if (off >= sizeof(bpt_table))
> > > return NULL;
> > > *offp = off % BPT_SIZE;
> > > - if (*offp != 0 && *offp != 4)
> > > + if (*offp != 0 && *offp != 4 && *offp != 8)
> > > return NULL;
> > > return bpts + (off / BPT_SIZE);
> > > }
> > > diff --git a/arch/powerpc/xmon/xmon_bpts.S
> > > b/arch/powerpc/xmon/xmon_bpts.S
> > > index ebb2dbc70ca8..09058eb6abbd 100644
> > > --- a/arch/powerpc/xmon/xmon_bpts.S
> > > +++ b/arch/powerpc/xmon/xmon_bpts.S
> > > @@ -3,6 +3,8 @@
> > > #include <asm/asm-compat.h>
> > > #include "xmon_bpts.h"
> > >
> > > +/* Prefixed instructions can not cross 64 byte boundaries */
> > > +.align 6
> > > .global bpt_table
> > > bpt_table:
> > > - .space NBPTS * 8
> > > + .space NBPTS * 16
^ permalink raw reply
* Re: [PATCH 4/8] binfmt_elf: open code copy_siginfo_to_user to kernelspace buffer
From: Christoph Hellwig @ 2020-04-15 7:45 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel@vger.kernel.org, Alexander Viro, Jeremy Kerr,
Linux FS-devel Mailing List, Andrew Morton, linuxppc-dev,
Christoph Hellwig, Eric W . Biederman
In-Reply-To: <CAK8P3a3HvbPKTkwfWr6PbZ96koO_NrJP1qgk8H1mgk=qUScGkQ@mail.gmail.com>
On Tue, Apr 14, 2020 at 03:15:09PM +0200, Arnd Bergmann wrote:
> I don't think you are changing the behavior here, but I still wonder if it
> is in fact correct for x32: is in_x32_syscall() true here when dumping an
> x32 compat elf process, or should this rather be set according to which
> binfmt_elf copy is being used?
The infrastructure cold enable that, although it would require more
arch hooks I think. I'd rather keep it out of this series and to
an interested party. Then again x32 doesn't seem to have a whole lot
of interested parties..
^ permalink raw reply
* Re: [EXTERNAL] [PATCH] target/ppc: Fix mtmsr(d) L=1 variant that loses interrupts
From: Cédric Le Goater @ 2020-04-15 6:49 UTC (permalink / raw)
To: Nicholas Piggin, qemu-devel
Cc: qemu-stable, qemu-ppc, Nathan Chancellor, linuxppc-dev,
David Gibson
In-Reply-To: <20200414111131.465560-1-npiggin@gmail.com>
On 4/14/20 1:11 PM, Nicholas Piggin wrote:
> If mtmsr L=1 sets MSR[EE] while there is a maskable exception pending,
> it does not cause an interrupt. This causes the test case to hang:
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.gnu.org_archive_html_qemu-2Dppc_2019-2D10_msg00826.html&d=DwIDAg&c=jf_iaSHvJObTbx-siA1ZOg&r=XHJsZhhuWSw9713Fp0ciew&m=TQfi_v-8XYgz7MiMDAZ_CjkyalSh9-EXhQ3oDesUm74&s=pFoesXbioVBh5wCuzEnzwgfze6X7e-a9unkfUgsRwiw&e=
>
> More recently, Linux reduced the occurance of operations (e.g., rfi)
> which stop translation and allow pending interrupts to be processed.
> This started causing hangs in Linux boot in long-running kernel tests,
> running with '-d int' shows the decrementer stops firing despite DEC
> wrapping and MSR[EE]=1.
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.ozlabs.org_pipermail_linuxppc-2Ddev_2020-2DApril_208301.html&d=DwIDAg&c=jf_iaSHvJObTbx-siA1ZOg&r=XHJsZhhuWSw9713Fp0ciew&m=TQfi_v-8XYgz7MiMDAZ_CjkyalSh9-EXhQ3oDesUm74&s=EhkRfxvQMomvneYweWDEIUktCkKykgIqEmdhA0PtiwU&e=
>
> The cause is the broken mtmsr L=1 behaviour, which is contrary to the
> architecture. From Power ISA v3.0B, p.977, Move To Machine State Register,
> Programming Note states:
>
> If MSR[EE]=0 and an External, Decrementer, or Performance Monitor
> exception is pending, executing an mtmsrd instruction that sets
> MSR[EE] to 1 will cause the interrupt to occur before the next
> instruction is executed, if no higher priority exception exists
>
> Fix this by handling L=1 exactly the same way as L=0, modulo the MSR
> bits altered.
>
> The confusion arises from L=0 being "context synchronizing" whereas L=1
> is "execution synchronizing", which is a weaker semantic. However this
> is not a relaxation of the requirement that these exceptions cause
> interrupts when MSR[EE]=1 (e.g., when mtmsr executes to completion as
> TCG is doing here), rather it specifies how a pipelined processor can
> have multiple instructions in flight where one may influence how another
> behaves.
I was expecting more changes but this looks fine.
Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Cc: qemu-stable@nongnu.org
> Reported-by: Anton Blanchard <anton@ozlabs.org>
> Reported-by: Nathan Chancellor <natechancellor@gmail.com>
> Tested-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
I gave it a try on PowerNV, pseries and mac99. All good.
Tested-by: Cédric Le Goater <clg@kaod.org>
I don't know how we could include tests in QEMU such as the one Anton
sent. These are good exercisers for our exception model.
Thanks,
C.
> ---
> Thanks very much to Nathan for reporting and testing it, I added his
> Tested-by tag despite a more polished patch, as the the basics are
> still the same (and still fixes his test case here).
>
> This bug possibly goes back to early v2.04 / mtmsrd L=1 support around
> 2007, and the code has been changed several times since then so may
> require some backporting.
>
> 32-bit / mtmsr untested at the moment, I don't have an environment
> handy.
>
>
> target/ppc/translate.c | 46 +++++++++++++++++++++++++-----------------
> 1 file changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index b207fb5386..9959259dba 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4361,30 +4361,34 @@ static void gen_mtmsrd(DisasContext *ctx)
> CHK_SV;
>
> #if !defined(CONFIG_USER_ONLY)
> + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
> + gen_io_start();
> + }
> if (ctx->opcode & 0x00010000) {
> - /* Special form that does not need any synchronisation */
> + /* L=1 form only updates EE and RI */
> TCGv t0 = tcg_temp_new();
> + TCGv t1 = tcg_temp_new();
> tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)],
> (1 << MSR_RI) | (1 << MSR_EE));
> - tcg_gen_andi_tl(cpu_msr, cpu_msr,
> + tcg_gen_andi_tl(t1, cpu_msr,
> ~(target_ulong)((1 << MSR_RI) | (1 << MSR_EE)));
> - tcg_gen_or_tl(cpu_msr, cpu_msr, t0);
> + tcg_gen_or_tl(t1, t1, t0);
> +
> + gen_helper_store_msr(cpu_env, t1);
> tcg_temp_free(t0);
> + tcg_temp_free(t1);
> +
> } else {
> /*
> * XXX: we need to update nip before the store if we enter
> * power saving mode, we will exit the loop directly from
> * ppc_store_msr
> */
> - if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
> - gen_io_start();
> - }
> gen_update_nip(ctx, ctx->base.pc_next);
> gen_helper_store_msr(cpu_env, cpu_gpr[rS(ctx->opcode)]);
> - /* Must stop the translation as machine state (may have) changed */
> - /* Note that mtmsr is not always defined as context-synchronizing */
> - gen_stop_exception(ctx);
> }
> + /* Must stop the translation as machine state (may have) changed */
> + gen_stop_exception(ctx);
> #endif /* !defined(CONFIG_USER_ONLY) */
> }
> #endif /* defined(TARGET_PPC64) */
> @@ -4394,15 +4398,23 @@ static void gen_mtmsr(DisasContext *ctx)
> CHK_SV;
>
> #if !defined(CONFIG_USER_ONLY)
> - if (ctx->opcode & 0x00010000) {
> - /* Special form that does not need any synchronisation */
> + if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
> + gen_io_start();
> + }
> + if (ctx->opcode & 0x00010000) {
> + /* L=1 form only updates EE and RI */
> TCGv t0 = tcg_temp_new();
> + TCGv t1 = tcg_temp_new();
> tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)],
> (1 << MSR_RI) | (1 << MSR_EE));
> - tcg_gen_andi_tl(cpu_msr, cpu_msr,
> + tcg_gen_andi_tl(t1, cpu_msr,
> ~(target_ulong)((1 << MSR_RI) | (1 << MSR_EE)));
> - tcg_gen_or_tl(cpu_msr, cpu_msr, t0);
> + tcg_gen_or_tl(t1, t1, t0);
> +
> + gen_helper_store_msr(cpu_env, t1);
> tcg_temp_free(t0);
> + tcg_temp_free(t1);
> +
> } else {
> TCGv msr = tcg_temp_new();
>
> @@ -4411,9 +4423,6 @@ static void gen_mtmsr(DisasContext *ctx)
> * power saving mode, we will exit the loop directly from
> * ppc_store_msr
> */
> - if (tb_cflags(ctx->base.tb) & CF_USE_ICOUNT) {
> - gen_io_start();
> - }
> gen_update_nip(ctx, ctx->base.pc_next);
> #if defined(TARGET_PPC64)
> tcg_gen_deposit_tl(msr, cpu_msr, cpu_gpr[rS(ctx->opcode)], 0, 32);
> @@ -4422,10 +4431,9 @@ static void gen_mtmsr(DisasContext *ctx)
> #endif
> gen_helper_store_msr(cpu_env, msr);
> tcg_temp_free(msr);
> - /* Must stop the translation as machine state (may have) changed */
> - /* Note that mtmsr is not always defined as context-synchronizing */
> - gen_stop_exception(ctx);
> }
> + /* Must stop the translation as machine state (may have) changed */
> + gen_stop_exception(ctx);
> #endif
> }
>
>
^ permalink raw reply
* Re: [PATCH 3/4] powerpc/eeh: Remove workaround from eeh_add_device_late()
From: Sam Bobroff @ 2020-04-15 6:44 UTC (permalink / raw)
To: Oliver O'Halloran; +Cc: linuxppc-dev
In-Reply-To: <CAOSf1CHA+ZzWpLtuPrvkOvWO13Dikv86UjiBdppyG4GrexGSpA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2690 bytes --]
On Wed, Apr 08, 2020 at 04:53:36PM +1000, Oliver O'Halloran wrote:
> On Wed, Apr 8, 2020 at 4:22 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote:
> >
> > On Fri, Apr 03, 2020 at 05:08:32PM +1100, Oliver O'Halloran wrote:
> > > On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote:
> > > > When EEH device state was released asynchronously by the device
> > > > release handler, it was possible for an outstanding reference to
> > > > prevent it's release and it was necessary to work around that if a
> > > > device was re-discovered at the same PCI location.
> > >
> > > I think this is a bit misleading. The main situation where you'll hit
> > > this hack is when recovering a device with a driver that doesn't
> > > implement the error handling callbacks. In that case the device is
> > > removed, reset, then re-probed by the PCI core, but we assume it's the
> > > same physical device so the eeh_device state remains active.
> > >
> > > If you actually changed the underlying device I suspect something bad
> > > would happen.
> >
> > I'm not sure I understand. Isn't the case you're talking about caught by
> > the earlier check (just above the patch)?
> >
> > if (edev->pdev == dev) {
> > eeh_edev_dbg(edev, "Device already referenced!\n");
> > return;
> > }
>
> No, in the case I'm talking about the pci_dev is torn down and
> freed(). After the PE is reset we re-probe the device and create a new
> pci_dev. If the release of the old pci_dev is delayed we need the
> hack this patch is removing.
Oh, yes, that is the case I was intending to change here. But I must be
missing something, isn't it also the case that's changed by patch 2/4?
What I intended was, after patch 2, eeh_remove_device() is called from
the bus notifier so it happens imediately when recovery calls
pci_stop_and_remove_bus_device(). Once it returns, edev->pdev has
already been set to NULL by eeh_remove_device() so this case can't be
hit anymore, and we should clean it up (this patch).
(There is a slight difference in the way EEH_PE_KEEP is handled between
the code removed here and the body of eeh_remove_device(), but checking
and explaining that is already on my list for v2.)
(I did test recovery on an unaware device and didn't hit the
WARN_ON_ONCE().)
> The check above should probably be a WARN_ON() since we should never
> be re-running the EEH probe on the same device. I think there is a
> case where that can happen, but I don't remember the details.
Yeah, I also certainly see the "Device already referenced!" message
while debugging, and it would be good to track down.
> Oliver
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 4/8] binfmt_elf: open code copy_siginfo_to_user to kernelspace buffer
From: Christoph Hellwig @ 2020-04-15 6:19 UTC (permalink / raw)
To: Michael Ellerman
Cc: Arnd Bergmann, linux-kernel, Alexander Viro, Jeremy Kerr,
linux-fsdevel, Andrew Morton, linuxppc-dev, Christoph Hellwig,
Eric W . Biederman
In-Reply-To: <87y2qxih94.fsf@mpe.ellerman.id.au>
On Wed, Apr 15, 2020 at 01:01:59PM +1000, Michael Ellerman wrote:
> > + to_compat_siginfo(csigdata, siginfo, compat_siginfo_flags()); \
> > + fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata); \
> > +} while (0)
>
> This doesn't build on ppc (cell_defconfig):
>
> ../fs/binfmt_elf.c: In function 'fill_note_info':
> ../fs/compat_binfmt_elf.c:44:39: error: implicit declaration of function 'compat_siginfo_flags'; did you mean 'to_compat_siginfo'? [-Werror=implicit-function-d
> eclaration]
> to_compat_siginfo(csigdata, siginfo, compat_siginfo_flags()); \
> ^~~~~~~~~~~~~~~~~~~~
> ../fs/binfmt_elf.c:1846:2: note: in expansion of macro 'fill_siginfo_note'
> fill_siginfo_note(&info->signote, &info->csigdata, siginfo);
> ^~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
> make[2]: *** [../scripts/Makefile.build:266: fs/compat_binfmt_elf.o] Error 1
>
>
> I guess the empty version from kernel/signal.c needs to move into a
> header somewhere.
Yes, it should. Odd that the buildbut hasn't complained yet so far..
^ permalink raw reply
* Re: [PATCH 1/4] dma-mapping: move the remaining DMA API calls out of line
From: Christoph Hellwig @ 2020-04-15 6:18 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: Greg Kroah-Hartman, Joerg Roedel, Robin Murphy, linux-kernel,
iommu, linuxppc-dev, Christoph Hellwig, Lu Baolu
In-Reply-To: <c2572d30-f03c-450d-e257-3a8673b42d44@ozlabs.ru>
On Wed, Apr 15, 2020 at 12:26:04PM +1000, Alexey Kardashevskiy wrote:
> May be this is correct and allowed (no idea) but removing exported
> symbols at least deserves a mention in the commit log, does not it?
>
> The rest of the series is fine and works. Thanks,
Maybe I can throw in a line, but the point is that dma_direct_*
was exported as dma_* called them inline. Now dma_* is out of line
and exported instead, which always was the actual API.
^ permalink raw reply
* Re: [RFC PATCH] powerpc/lib: Fixing use a temporary mm for code patching
From: Christopher M Riedl @ 2020-04-15 5:16 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev, linux-kernel
In-Reply-To: <c88b13ede49744d81fdab32e037a7ae10f0b241f.1585233657.git.christophe.leroy@c-s.fr>
> On March 26, 2020 9:42 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
> This patch fixes the RFC series identified below.
> It fixes three points:
> - Failure with CONFIG_PPC_KUAP
> - Failure to write do to lack of DIRTY bit set on the 8xx
> - Inadequaly complex WARN post verification
>
> However, it has an impact on the CPU load. Here is the time
> needed on an 8xx to run the ftrace selftests without and
> with this series:
> - Without CONFIG_STRICT_KERNEL_RWX ==> 38 seconds
> - With CONFIG_STRICT_KERNEL_RWX ==> 40 seconds
> - With CONFIG_STRICT_KERNEL_RWX + this series ==> 43 seconds
>
> Link: https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/lib/code-patching.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index f156132e8975..4ccff427592e 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -97,6 +97,7 @@ static int map_patch(const void *addr, struct patch_mapping *patch_mapping)
> }
>
> pte = mk_pte(page, pgprot);
> + pte = pte_mkdirty(pte);
> set_pte_at(patching_mm, patching_addr, ptep, pte);
>
> init_temp_mm(&patch_mapping->temp_mm, patching_mm);
> @@ -168,7 +169,9 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
> (offset_in_page((unsigned long)addr) /
> sizeof(unsigned int));
>
> + allow_write_to_user(patch_addr, sizeof(instr));
> __patch_instruction(addr, instr, patch_addr);
> + prevent_write_to_user(patch_addr, sizeof(instr));
>
On radix we can map the page with PAGE_KERNEL protection which ends up
setting EAA[0] in the radix PTE. This means the KUAP (AMR) protection is
ignored (ISA v3.0b Fig. 35) since we are accessing the page from MSR[PR]=0.
Can we employ a similar approach on the 8xx? I would prefer *not* to wrap
the __patch_instruction() with the allow_/prevent_write_to_user() KUAP things
because this is a temporary kernel mapping which really isn't userspace in
the usual sense.
> err = unmap_patch(&patch_mapping);
> if (err)
> @@ -179,7 +182,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
> * think we just wrote.
> * XXX: BUG_ON() instead?
> */
> - WARN_ON(memcmp(addr, &instr, sizeof(instr)));
> + WARN_ON(*addr != instr);
>
> out:
> local_irq_restore(flags);
> --
> 2.25.0
^ permalink raw reply
* Re: [RFC PATCH 3/3] powerpc/lib: Use a temporary mm for code patching
From: Christopher M Riedl @ 2020-04-15 5:11 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <db40ec6a-1d81-91e3-00d8-cd86fd5262d5@c-s.fr>
> On March 24, 2020 11:25 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
> Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
> > Currently, code patching a STRICT_KERNEL_RWX exposes the temporary
> > mappings to other CPUs. These mappings should be kept local to the CPU
> > doing the patching. Use the pre-initialized temporary mm and patching
> > address for this purpose. Also add a check after patching to ensure the
> > patch succeeded.
> >
> > Based on x86 implementation:
> >
> > commit b3fd8e83ada0
> > ("x86/alternatives: Use temporary mm for text poking")
> >
> > Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
> > ---
> > arch/powerpc/lib/code-patching.c | 128 ++++++++++++++-----------------
> > 1 file changed, 57 insertions(+), 71 deletions(-)
> >
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> > index 18b88ecfc5a8..f156132e8975 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -19,6 +19,7 @@
> > #include <asm/page.h>
> > #include <asm/code-patching.h>
> > #include <asm/setup.h>
> > +#include <asm/mmu_context.h>
> >
> > static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
> > unsigned int *patch_addr)
> > @@ -65,99 +66,79 @@ void __init poking_init(void)
> > pte_unmap_unlock(ptep, ptl);
> > }
> >
> > -static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
> > -
> > -static int text_area_cpu_up(unsigned int cpu)
> > -{
> > - struct vm_struct *area;
> > -
> > - area = get_vm_area(PAGE_SIZE, VM_ALLOC);
> > - if (!area) {
> > - WARN_ONCE(1, "Failed to create text area for cpu %d\n",
> > - cpu);
> > - return -1;
> > - }
> > - this_cpu_write(text_poke_area, area);
> > -
> > - return 0;
> > -}
> > -
> > -static int text_area_cpu_down(unsigned int cpu)
> > -{
> > - free_vm_area(this_cpu_read(text_poke_area));
> > - return 0;
> > -}
> > -
> > -/*
> > - * Run as a late init call. This allows all the boot time patching to be done
> > - * simply by patching the code, and then we're called here prior to
> > - * mark_rodata_ro(), which happens after all init calls are run. Although
> > - * BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge
> > - * it as being preferable to a kernel that will crash later when someone tries
> > - * to use patch_instruction().
> > - */
> > -static int __init setup_text_poke_area(void)
> > -{
> > - BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> > - "powerpc/text_poke:online", text_area_cpu_up,
> > - text_area_cpu_down));
> > -
> > - return 0;
> > -}
> > -late_initcall(setup_text_poke_area);
> > +struct patch_mapping {
> > + spinlock_t *ptl; /* for protecting pte table */
> > + struct temp_mm temp_mm;
> > +};
> >
> > /*
> > * This can be called for kernel text or a module.
> > */
> > -static int map_patch_area(void *addr, unsigned long text_poke_addr)
> > +static int map_patch(const void *addr, struct patch_mapping *patch_mapping)
>
> Why change the name ?
>
It's not really an "area" anymore.
> > {
> > - unsigned long pfn;
> > - int err;
> > + struct page *page;
> > + pte_t pte, *ptep;
> > + pgprot_t pgprot;
> >
> > if (is_vmalloc_addr(addr))
> > - pfn = vmalloc_to_pfn(addr);
> > + page = vmalloc_to_page(addr);
> > else
> > - pfn = __pa_symbol(addr) >> PAGE_SHIFT;
> > + page = virt_to_page(addr);
> >
> > - err = map_kernel_page(text_poke_addr, (pfn << PAGE_SHIFT), PAGE_KERNEL);
> > + if (radix_enabled())
> > + pgprot = __pgprot(pgprot_val(PAGE_KERNEL));
> > + else
> > + pgprot = PAGE_SHARED;
>
> Can you explain the difference between radix and non radix ?
>
> Why PAGE_KERNEL for a page that is mapped in userspace ?
>
> Why do you need to do __pgprot(pgprot_val(PAGE_KERNEL)) instead of just
> using PAGE_KERNEL ?
>
On hash there is a manual check which prevents setting _PAGE_PRIVILEGED for
kernel to userspace access in __hash_page - hence we cannot access the mapping
if the page is mapped PAGE_KERNEL on hash. However, I would like to use
PAGE_KERNEL here as well and am working on understanding why this check is
done in hash and if this can change. On radix this works just fine.
The page is mapped PAGE_KERNEL because the address is technically a userspace
address - but only to keep the mapping local to this CPU doing the patching.
PAGE_KERNEL makes it clear both in intent and protection that this is a kernel
mapping.
I think the correct way is pgprot_val(PAGE_KERNEL) since PAGE_KERNEL is defined
as:
#define PAGE_KERNEL __pgprot(_PAGE_BASE | _PAGE_KERNEL_RW)
and __pgprot() is defined as:
typedef struct { unsigned long pgprot; } pgprot_t;
#define pgprot_val(x) ((x).pgprot)
#define __pgprot(x) ((pgprot_t) { (x) })
> >
> > - pr_devel("Mapped addr %lx with pfn %lx:%d\n", text_poke_addr, pfn, err);
> > - if (err)
> > + ptep = get_locked_pte(patching_mm, patching_addr, &patch_mapping->ptl);
> > + if (unlikely(!ptep)) {
> > + pr_warn("map patch: failed to allocate pte for patching\n");
> > return -1;
> > + }
> > +
> > + pte = mk_pte(page, pgprot);
> > + set_pte_at(patching_mm, patching_addr, ptep, pte);
> > +
> > + init_temp_mm(&patch_mapping->temp_mm, patching_mm);
> > + use_temporary_mm(&patch_mapping->temp_mm);
> >
> > return 0;
> > }
> >
> > -static inline int unmap_patch_area(unsigned long addr)
> > +static int unmap_patch(struct patch_mapping *patch_mapping)
> > {
> > pte_t *ptep;
> > pmd_t *pmdp;
> > pud_t *pudp;
> > pgd_t *pgdp;
> >
> > - pgdp = pgd_offset_k(addr);
> > + pgdp = pgd_offset(patching_mm, patching_addr);
> > if (unlikely(!pgdp))
> > return -EINVAL;
> >
> > - pudp = pud_offset(pgdp, addr);
> > + pudp = pud_offset(pgdp, patching_addr);
> > if (unlikely(!pudp))
> > return -EINVAL;
> >
> > - pmdp = pmd_offset(pudp, addr);
> > + pmdp = pmd_offset(pudp, patching_addr);
> > if (unlikely(!pmdp))
> > return -EINVAL;
> >
> > - ptep = pte_offset_kernel(pmdp, addr);
> > + ptep = pte_offset_kernel(pmdp, patching_addr);
>
> ptep should be stored in the patch_mapping struct instead of walking
> again the page tables.
>
Oh yes - this will be in the next version.
> > if (unlikely(!ptep))
> > return -EINVAL;
> >
> > - pr_devel("clearing mm %p, pte %p, addr %lx\n", &init_mm, ptep, addr);
> > + /*
> > + * In hash, pte_clear flushes the tlb
> > + */
> > + pte_clear(patching_mm, patching_addr, ptep);
> > + unuse_temporary_mm(&patch_mapping->temp_mm);
> >
> > /*
> > - * In hash, pte_clear flushes the tlb, in radix, we have to
> > + * In radix, we have to explicitly flush the tlb (no-op in hash)
> > */
> > - pte_clear(&init_mm, addr, ptep);
> > - flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> > + local_flush_tlb_mm(patching_mm);
> > + pte_unmap_unlock(ptep, patch_mapping->ptl);
> >
> > return 0;
> > }
> > @@ -167,33 +148,38 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
> > int err;
> > unsigned int *patch_addr = NULL;
> > unsigned long flags;
> > - unsigned long text_poke_addr;
> > - unsigned long kaddr = (unsigned long)addr;
> > + struct patch_mapping patch_mapping;
> >
> > /*
> > - * During early early boot patch_instruction is called
> > - * when text_poke_area is not ready, but we still need
> > - * to allow patching. We just do the plain old patching
> > + * The patching_mm is initialized before calling mark_rodata_ro. Prior
> > + * to this, patch_instruction is called when we don't have (and don't
> > + * need) the patching_mm so just do plain old patching.
> > */
> > - if (!this_cpu_read(text_poke_area))
> > + if (!patching_mm)
> > return raw_patch_instruction(addr, instr);
> >
> > local_irq_save(flags);
> >
> > - text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;
> > - if (map_patch_area(addr, text_poke_addr)) {
> > - err = -1;
> > + err = map_patch(addr, &patch_mapping);
> > + if (err)
> > goto out;
> > - }
> >
> > - patch_addr = (unsigned int *)(text_poke_addr) +
> > - ((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
> > + patch_addr = (unsigned int *)(patching_addr) +
> > + (offset_in_page((unsigned long)addr) /
> > + sizeof(unsigned int));
> >
> > __patch_instruction(addr, instr, patch_addr);
>
> The error returned by __patch_instruction() should be managed.
>
Agreed, will do something in the next spin.
> >
> > - err = unmap_patch_area(text_poke_addr);
> > + err = unmap_patch(&patch_mapping);
> > if (err)
> > - pr_warn("failed to unmap %lx\n", text_poke_addr);
> > + pr_warn("unmap patch: failed to unmap patch\n");
> > +
> > + /*
> > + * Something is wrong if what we just wrote doesn't match what we
> > + * think we just wrote.
> > + * XXX: BUG_ON() instead?
>
> No, not a BUG_ON(). If patching fails, that's no a vital fault, we can
> fail gracefully. You should return a fault instead.
>
Yup - will make these changes in the next version.
> > + */
> > + WARN_ON(memcmp(addr, &instr, sizeof(instr)));
>
> Come on. addr is an *int, instr is an int. By doing a memcmp() on
> &instr, you for the compiler to write instr into the stack whereas local
> vars are mainly in registers on RISC processors like powerpc. Following
> should do it:
>
> WARN_ON(*addr != instr);
>
Oh man - I agree, that's just embarrassing.
Appreciate your feedback on this RFC series, thanks!
> >
> > out:
> > local_irq_restore(flags);
> >
>
> Christophe
^ permalink raw reply
* Re: [PATCH 1/2] mm, treewide: Rename kzfree() to kfree_sensitive()
From: Johannes Weiner @ 2020-04-15 5:01 UTC (permalink / raw)
To: Waiman Long
Cc: linux-btrfs, Jarkko Sakkinen, virtualization, David Howells,
linux-mm, linux-sctp, keyrings, kasan-dev, samba-technical,
linux-stm32, devel, linux-s390, linux-scsi, x86, James Morris,
Matthew Wilcox, cocci, linux-wpan, intel-wired-lan,
David Rientjes, Serge E. Hallyn, linux-pm, ecryptfs, linux-nfs,
linux-fscrypt, linux-mediatek, linux-amlogic, linux-integrity,
linux-arm-kernel, linux-cifs, Linus Torvalds, linux-wireless,
linux-kernel, linux-bluetooth, linux-security-module,
target-devel, tipc-discussion, linux-crypto, netdev, Joe Perches,
Andrew Morton, linuxppc-dev, wireguard, linux-ppp
In-Reply-To: <20200413211550.8307-2-longman@redhat.com>
On Mon, Apr 13, 2020 at 05:15:49PM -0400, Waiman Long wrote:
> As said by Linus:
>
> A symmetric naming is only helpful if it implies symmetries in use.
> Otherwise it's actively misleading.
As the btrfs example proves - people can be tempted by this false
symmetry to pair kzalloc with kzfree, which isn't what we wanted.
> In "kzalloc()", the z is meaningful and an important part of what the
> caller wants.
>
> In "kzfree()", the z is actively detrimental, because maybe in the
> future we really _might_ want to use that "memfill(0xdeadbeef)" or
> something. The "zero" part of the interface isn't even _relevant_.
>
> The main reason that kzfree() exists is to clear sensitive information
> that should not be leaked to other future users of the same memory
> objects.
>
> Rename kzfree() to kfree_sensitive() to follow the example of the
> recently added kvfree_sensitive() and make the intention of the API
> more explicit. In addition, memzero_explicit() is used to clear the
> memory to make sure that it won't get optimized away by the compiler.
>
> The renaming is done by using the command sequence:
>
> git grep -w --name-only kzfree |\
> xargs sed -i 's/\bkzfree\b/kfree_sensitive/'
>
> followed by some editing of the kfree_sensitive() kerneldoc and the
> use of memzero_explicit() instead of memset().
>
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Waiman Long <longman@redhat.com>
Looks good to me. Thanks for fixing this very old mistake.
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
^ permalink raw reply
* [PATCH] ASoC: fsl_micfil: Omit superfluous error message in fsl_micfil_probe()
From: Tang Bin @ 2020-04-15 4:45 UTC (permalink / raw)
To: broonie, timur, nicoleotsuka, Xiubo.Lee, perex, tiwai
Cc: alsa-devel, Shengju Zhang, linuxppc-dev, linux-kernel, Tang Bin
In the function fsl_micfil_probe(), when get irq failed, the function
platform_get_irq() logs an error message, so remove redundant message here.
Signed-off-by: Tang Bin <tangbin@cmss.chinamobile.com>
Signed-off-by: Shengju Zhang <zhangshengju@cmss.chinamobile.com>
---
sound/soc/fsl/fsl_micfil.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/sound/soc/fsl/fsl_micfil.c b/sound/soc/fsl/fsl_micfil.c
index f7f2d29f1..e73bd6570 100644
--- a/sound/soc/fsl/fsl_micfil.c
+++ b/sound/soc/fsl/fsl_micfil.c
@@ -702,10 +702,8 @@ static int fsl_micfil_probe(struct platform_device *pdev)
for (i = 0; i < MICFIL_IRQ_LINES; i++) {
micfil->irq[i] = platform_get_irq(pdev, i);
dev_err(&pdev->dev, "GET IRQ: %d\n", micfil->irq[i]);
- if (micfil->irq[i] < 0) {
- dev_err(&pdev->dev, "no irq for node %s\n", pdev->name);
+ if (micfil->irq[i] < 0)
return micfil->irq[i];
- }
}
if (of_property_read_bool(np, "fsl,shared-interrupt"))
--
2.20.1.windows.1
^ permalink raw reply related
* Re: [PATCH v5 18/21] powerpc64: Add prefixed instructions to instruction data type
From: Jordan Niethe @ 2020-04-15 4:40 UTC (permalink / raw)
To: Balamuruhan S
Cc: Alistair Popple, linuxppc-dev, Nicholas Piggin, Daniel Axtens
In-Reply-To: <435c17d3180d560a723efcae42de351ae7152cb3.camel@linux.ibm.com>
On Mon, Apr 13, 2020 at 10:04 PM Balamuruhan S <bala24@linux.ibm.com> wrote:
>
> On Mon, 2020-04-06 at 18:09 +1000, Jordan Niethe wrote:
> > For powerpc64, redefine the ppc_inst type so both word and prefixed
> > instructions can be represented. On powerpc32 the type will remain the
> > same. Update places which had assumed instructions to be 4 bytes long.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v4: New to series
> > v5: - Distinguish normal instructions from prefixed instructions with a
> > 0xff marker for the suffix.
> > - __patch_instruction() using std for prefixed instructions
> > ---
> > arch/powerpc/include/asm/inst.h | 71 ++++++++++++++++++++++++++--
> > arch/powerpc/include/asm/kprobes.h | 2 +-
> > arch/powerpc/include/asm/uaccess.h | 31 ++++++++++--
> > arch/powerpc/include/asm/uprobes.h | 2 +-
> > arch/powerpc/kernel/optprobes.c | 42 ++++++++--------
> > arch/powerpc/kernel/optprobes_head.S | 3 ++
> > arch/powerpc/kernel/trace/ftrace.c | 26 +++++++++-
> > arch/powerpc/lib/code-patching.c | 19 +++++---
> > arch/powerpc/lib/feature-fixups.c | 5 +-
> > arch/powerpc/lib/sstep.c | 4 +-
> > arch/powerpc/xmon/xmon.c | 6 +--
> > arch/powerpc/xmon/xmon_bpts.S | 4 +-
> > 12 files changed, 171 insertions(+), 44 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/inst.h
> > b/arch/powerpc/include/asm/inst.h
> > index 70b37a35a91a..7e23e7146c66 100644
> > --- a/arch/powerpc/include/asm/inst.h
> > +++ b/arch/powerpc/include/asm/inst.h
> > @@ -8,23 +8,67 @@
> >
> > struct ppc_inst {
> > u32 val;
> > +#ifdef __powerpc64__
> > + u32 suffix;
> > +#endif /* __powerpc64__ */
> > } __packed;
> >
> > -#define ppc_inst(x) ((struct ppc_inst){ .val = x })
> > +static inline int ppc_inst_opcode(struct ppc_inst x)
> > +{
> > + return x.val >> 26;
>
>
> why don't we wrap here and in `ppc_inst_opcode()` in patch 9 using
> `ppc_inst_val()` ?
Will do.
>
>
> > +}
> >
> > static inline u32 ppc_inst_val(struct ppc_inst x)
>
>
> There is another same definition below for the same function in
> #else part of __powerpc64__ ifdef.
Thanks
>
>
> > {
> > return x.val;
> > }
> >
> > -static inline bool ppc_inst_len(struct ppc_inst x)
> > +#ifdef __powerpc64__
> > +#define ppc_inst(x) ((struct ppc_inst){ .val = (x), .suffix = 0xff })
> > +
> > +#define ppc_inst_prefix(x, y) ((struct ppc_inst){ .val = (x), .suffix = (y)
> > })
> > +
> > +static inline u32 ppc_inst_suffix(struct ppc_inst x)
> > {
> > - return sizeof(struct ppc_inst);
> > + return x.suffix;
> > }
> >
> > -static inline int ppc_inst_opcode(struct ppc_inst x)
> > +static inline bool ppc_inst_prefixed(struct ppc_inst x) {
> > + return ((ppc_inst_val(x) >> 26) == 1) && ppc_inst_suffix(x) != 0xff;
> > +}
> > +
> > +static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)
> > {
> > - return x.val >> 26;
> > + return ppc_inst_prefix(swab32(ppc_inst_val(x)),
> > + swab32(ppc_inst_suffix(x)));
> > +}
> > +
> > +static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)
> > +{
> > + u32 val, suffix = 0xff;
> > + val = *(u32 *)ptr;
> > + if ((val >> 26) == 1)
> > + suffix = *((u32 *)ptr + 1);
> > + return ppc_inst_prefix(val, suffix);
> > +}
> > +
> > +static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst x)
> > +{
> > + if (ppc_inst_prefixed(x)) {
> > + *(u32 *)ptr = x.val;
> > + *((u32 *)ptr + 1) = x.suffix;
> > + } else {
> > + *(u32 *)ptr = x.val;
>
>
> can we wrap here as well with `ppc_inst_val()` and `ppc_inst_suffix()` ?
Yeah no reason not too.
>
>
> > + }
> > +}
> > +
> > +#else
> > +
> > +#define ppc_inst(x) ((struct ppc_inst){ .val = x })
> > +
> > +static inline bool ppc_inst_prefixed(ppc_inst x)
> > +{
> > + return 0;
>
>
> Is it return !!0 or return false ?
False probably will make more sense.
>
>
> > }
> >
> > static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)
> > @@ -32,14 +76,31 @@ static inline struct ppc_inst ppc_inst_swab(struct
> > ppc_inst x)
> > return ppc_inst(swab32(ppc_inst_val(x)));
> > }
> >
> > +static inline u32 ppc_inst_val(struct ppc_inst x)
>
>
> [...] duplicate definition that is defined outside __powerpc64__ above.
>
>
> > +{
> > + return x.val;
> > +}
> > +
> > static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr)
> > {
> > return *ptr;
> > }
> >
> > +static inline void ppc_inst_write(struct ppc_inst *ptr, struct ppc_inst x)
> > +{
> > + *ptr = x;
> > +}
> > +
> > +#endif /* __powerpc64__ */
> > +
> > static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)
> > {
> > return !memcmp(&x, &y, sizeof(struct ppc_inst));
> > }
> >
> > +static inline int ppc_inst_len(struct ppc_inst x)
> > +{
> > + return (ppc_inst_prefixed(x)) ? 8 : 4;
> > +}
> > +
> > #endif /* _ASM_INST_H */
> > diff --git a/arch/powerpc/include/asm/kprobes.h
> > b/arch/powerpc/include/asm/kprobes.h
> > index 66b3f2983b22..4fc0e15e23a5 100644
> > --- a/arch/powerpc/include/asm/kprobes.h
> > +++ b/arch/powerpc/include/asm/kprobes.h
> > @@ -43,7 +43,7 @@ extern kprobe_opcode_t optprobe_template_ret[];
> > extern kprobe_opcode_t optprobe_template_end[];
> >
> > /* Fixed instruction size for powerpc */
> > -#define MAX_INSN_SIZE 1
> > +#define MAX_INSN_SIZE 2
> > #define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */
> > #define MAX_OPTINSN_SIZE (optprobe_template_end -
> > optprobe_template_entry)
> > #define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */
> > diff --git a/arch/powerpc/include/asm/uaccess.h
> > b/arch/powerpc/include/asm/uaccess.h
> > index c0a35e4586a5..5a3f486ddf02 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -105,11 +105,34 @@ static inline int __access_ok(unsigned long addr,
> > unsigned long size,
> > #define __put_user_inatomic(x, ptr) \
> > __put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
> >
> > -#define __get_user_instr(x, ptr) \
> > - __get_user_nocheck((x).val, (u32 *)(ptr), sizeof(u32), true)
> > +#define __get_user_instr(x, ptr) \
> > +({ \
> > + long __gui_ret = 0; \
> > + unsigned int prefix, suffix; \
> > + __gui_ret = __get_user(prefix, (unsigned int __user *)ptr);
> > \
> > + if (!__gui_ret && (prefix >> 26) == 1) { \
> > + __gui_ret = __get_user(suffix, (unsigned int __user *)ptr + 1);
> > \
> > + (x) = ppc_inst_prefix(prefix, suffix); \
> > + } else { \
> > + (x) = ppc_inst(prefix); \
> > + } \
> > + __gui_ret; \
> > +})
> > +
> > +#define __get_user_instr_inatomic(x, ptr) \
> > +({ \
> > + long __gui_ret = 0; \
> > + unsigned int prefix, suffix; \
> > + __gui_ret = __get_user_inatomic(prefix, (unsigned int __user *)ptr);
> > \
> > + if (!__gui_ret && (prefix >> 26) == 1) { \
> > + __gui_ret = __get_user_inatomic(suffix, (unsigned int __user
> > *)ptr + 1); \
> > + (x) = ppc_inst_prefix(prefix, suffix); \
> > + } else { \
> > + (x) = ppc_inst(prefix); \
> > + } \
> > + __gui_ret; \
> > +})
> >
> > -#define __get_user_instr_inatomic(x, ptr) \
> > - __get_user_nosleep((x).val, (u32 *)(ptr), sizeof(u32))
> > extern long __put_user_bad(void);
> >
> > /*
> > diff --git a/arch/powerpc/include/asm/uprobes.h
> > b/arch/powerpc/include/asm/uprobes.h
> > index 7e3b329ba2d3..5bf65f5d44a9 100644
> > --- a/arch/powerpc/include/asm/uprobes.h
> > +++ b/arch/powerpc/include/asm/uprobes.h
> > @@ -15,7 +15,7 @@
> >
> > typedef ppc_opcode_t uprobe_opcode_t;
> >
> > -#define MAX_UINSN_BYTES 4
> > +#define MAX_UINSN_BYTES 8
> > #define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
> >
> > /* The following alias is needed for reference from arch-agnostic code */
> > diff --git a/arch/powerpc/kernel/optprobes.c
> > b/arch/powerpc/kernel/optprobes.c
> > index 684640b8fa2e..689daf430161 100644
> > --- a/arch/powerpc/kernel/optprobes.c
> > +++ b/arch/powerpc/kernel/optprobes.c
> > @@ -159,38 +159,38 @@ void patch_imm32_load_insns(unsigned int val,
> > kprobe_opcode_t *addr)
> >
> > /*
> > * Generate instructions to load provided immediate 64-bit value
> > - * to register 'r3' and patch these instructions at 'addr'.
> > + * to register 'reg' and patch these instructions at 'addr'.
> > */
> > -void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
> > +void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t
> > *addr)
> > {
> > - /* lis r3,(op)@highest */
> > - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ADDIS |
> > ___PPC_RT(3) |
> > + /* lis reg,(op)@highest */
> > + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ADDIS |
> > ___PPC_RT(reg) |
> > ((val >> 48) & 0xffff)));
> > addr++;
> >
> > - /* ori r3,r3,(op)@higher */
> > - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> > ___PPC_RA(3) |
> > - ___PPC_RS(3) | ((val >> 32) & 0xffff)));
> > + /* ori reg,reg,(op)@higher */
> > + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> > ___PPC_RA(reg) |
> > + ___PPC_RS(reg) | ((val >> 32) & 0xffff)));
> > addr++;
> >
> > - /* rldicr r3,r3,32,31 */
> > - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_RLDICR |
> > ___PPC_RA(3) |
> > - ___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31)));
> > + /* rldicr reg,reg,32,31 */
> > + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_RLDICR |
> > ___PPC_RA(reg) |
> > + ___PPC_RS(reg) | __PPC_SH64(32) | __PPC_ME64(31)));
> > addr++;
> >
> > - /* oris r3,r3,(op)@h */
> > - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORIS |
> > ___PPC_RA(3) |
> > - ___PPC_RS(3) | ((val >> 16) & 0xffff)));
> > + /* oris reg,reg,(op)@h */
> > + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORIS |
> > ___PPC_RA(reg) |
> > + ___PPC_RS(reg) | ((val >> 16) & 0xffff)));
> > addr++;
> >
> > - /* ori r3,r3,(op)@l */
> > - patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> > ___PPC_RA(3) |
> > - ___PPC_RS(3) | (val & 0xffff)));
> > + /* ori reg,reg,(op)@l */
> > + patch_instruction((struct ppc_inst *)addr, ppc_inst(PPC_INST_ORI |
> > ___PPC_RA(reg) |
> > + ___PPC_RS(reg) | (val & 0xffff)));
> > }
> >
> > int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe
> > *p)
> > {
> > - struct ppc_inst branch_op_callback, branch_emulate_step;
> > + struct ppc_inst branch_op_callback, branch_emulate_step, temp;
> > kprobe_opcode_t *op_callback_addr, *emulate_step_addr, *buff;
> > long b_offset;
> > unsigned long nip, size;
> > @@ -240,7 +240,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, buff + TMPL_OP_IDX);
> > + patch_imm64_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX);
> >
> > /*
> > * 2. branch to optimized_callback() and emulate_step()
> > @@ -271,7 +271,11 @@ int arch_prepare_optimized_kprobe(struct
> > optimized_kprobe *op, struct kprobe *p)
> > /*
> > * 3. load instruction to be emulated into relevant register, and
> > */
> > - patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX);
> > + temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
> > + patch_imm64_load_insns(ppc_inst_val(temp) |
> > + ((u64)ppc_inst_suffix(temp) << 32),
>
>
> did we check building for ppc32 ?
Yeah I need to do so.
>
> I doubt we might hit build failure as `ppc_inst_suffix()` macro is not defined.
>
>
> > + 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 cf383520843f..ff8ba4d3824d 100644
> > --- a/arch/powerpc/kernel/optprobes_head.S
> > +++ b/arch/powerpc/kernel/optprobes_head.S
> > @@ -94,6 +94,9 @@ optprobe_template_insn:
> > /* 2, Pass instruction to be emulated in r4 */
> > nop
> > nop
> > + nop
> > + nop
> > + nop
> >
> > .global optprobe_template_call_emulate
> > optprobe_template_call_emulate:
> > diff --git a/arch/powerpc/kernel/trace/ftrace.c
> > b/arch/powerpc/kernel/trace/ftrace.c
> > index e78742613b36..16041a5c86d5 100644
> > --- a/arch/powerpc/kernel/trace/ftrace.c
> > +++ b/arch/powerpc/kernel/trace/ftrace.c
> > @@ -41,11 +41,35 @@
> > #define NUM_FTRACE_TRAMPS 8
> > static unsigned long ftrace_tramps[NUM_FTRACE_TRAMPS];
> >
> > +#ifdef __powerpc64__
> > static long
> > probe_kernel_read_inst(struct ppc_inst *inst, const void *src)
> > {
> > - return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE);
> > + u32 val, suffix = 0;
>
>
> don't we need to initialize suffix with 0xff ?
Good catch, that was causing problems.
>
>
> > + long err;
> > +
> > + err = probe_kernel_read((void *)&val,
> > + src, sizeof(val));
> > + if (err)
> > + return err;
> > +
> > + if ((val >> 26) == 1)
> > + err = probe_kernel_read((void *)&suffix,
> > + src + 4, MCOUNT_INSN_SIZE);
> > + if (err)
> > + return err;
> > +
> > + *inst = ppc_inst_prefix(val, suffix);
> > +
> > + return 0;
> > }
> > +#else
> > +static long
> > +probe_kernel_read_inst(struct ppc_inst *inst, const void *src)
> > +{
> > + return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE)
> > +}
> > +#endif
> >
> > static struct ppc_inst
> > ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-
> > patching.c
> > index c329ad657302..b4007e03d8fa 100644
> > --- a/arch/powerpc/lib/code-patching.c
> > +++ b/arch/powerpc/lib/code-patching.c
> > @@ -24,12 +24,19 @@ static int __patch_instruction(struct ppc_inst
> > *exec_addr, struct ppc_inst instr
> > {
> > int err = 0;
> >
> > - __put_user_asm(ppc_inst_val(instr), patch_addr, err, "stw");
> > - if (err)
> > - return err;
> > -
> > - asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr),
> > - "r" (exec_addr));
> > + if (!ppc_inst_prefixed(instr)) {
> > + __put_user_asm(ppc_inst_val(instr), patch_addr, err, "stw");
> > + if (err)
> > + return err;
> > + asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r"
> > (patch_addr),
> > + "r"
> > (exec_addr));
> > + } else {
> > + __put_user_asm((u64)ppc_inst_suffix(instr) << 32 |
> > ppc_inst_val(instr), patch_addr, err, "std");
> > + if (err)
> > + return err;
> > + asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r"
> > (patch_addr),
> > + "r"
> > (exec_addr));
> > + }
>
>
> can we keep these 2 lines out of conditions as it remains to be the same ?
True.
>
> if (err)
> return err;
> asm ("dcbst 0, %0; sync; icbi 0,%1; sync; isync" :: "r" (patch_addr), "r"
> (exec_addr));
>
>
> >
> > return 0;
> > }
> > diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-
> > fixups.c
> > index f00dd13b1c3c..5519cec83cc8 100644
> > --- a/arch/powerpc/lib/feature-fixups.c
> > +++ b/arch/powerpc/lib/feature-fixups.c
> > @@ -84,12 +84,13 @@ static int patch_feature_section(unsigned long value,
> > struct fixup_entry *fcur)
> > src = alt_start;
> > dest = start;
> >
> > - for (; src < alt_end; src++, dest++) {
> > + for (; src < alt_end; src = (void *)src +
> > ppc_inst_len(ppc_inst_read(src)),
> > + (dest = (void *)dest + ppc_inst_len(ppc_inst_read(dest)))) {
> > if (patch_alt_instruction(src, dest, alt_start, alt_end))
> > return 1;
> > }
> >
> > - for (; dest < end; dest++)
> > + for (; dest < end; dest = (void *)dest +
> > ppc_inst_len(ppc_inst(PPC_INST_NOP)))
> > raw_patch_instruction(dest, ppc_inst(PPC_INST_NOP));
> >
> > return 0;
> > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> > index 52ddd3122dc8..8b285bf11218 100644
> > --- a/arch/powerpc/lib/sstep.c
> > +++ b/arch/powerpc/lib/sstep.c
> > @@ -1169,10 +1169,12 @@ int analyse_instr(struct instruction_op *op, const
> > struct pt_regs *regs,
> > unsigned long int imm;
> > unsigned long int val, val2;
> > unsigned int mb, me, sh;
> > - unsigned int word;
> > + unsigned int word, suffix;
> > long ival;
> >
> > word = ppc_inst_val(instr);
> > + suffix = ppc_inst_suffix(instr);
>
>
> same here, I doubt it might break for ppc32.
Yeah I need to work on ppc32.
>
>
> -- Bala
> > +
> > op->type = COMPUTE;
> >
> > opcode = word >> 26;
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index 6f3bcdcfc9c7..b704aebb099a 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -761,8 +761,8 @@ static int xmon_bpt(struct pt_regs *regs)
> >
> > /* Are we at the trap at bp->instr[1] for some bp? */
> > bp = in_breakpoint_table(regs->nip, &offset);
> > - if (bp != NULL && offset == 4) {
> > - regs->nip = bp->address + 4;
> > + if (bp != NULL && (offset == 4 || offset == 8)) {
> > + regs->nip = bp->address + offset;
> > atomic_dec(&bp->ref_count);
> > return 1;
> > }
> > @@ -863,7 +863,7 @@ static struct bpt *in_breakpoint_table(unsigned long nip,
> > unsigned long *offp)
> > if (off >= sizeof(bpt_table))
> > return NULL;
> > *offp = off % BPT_SIZE;
> > - if (*offp != 0 && *offp != 4)
> > + if (*offp != 0 && *offp != 4 && *offp != 8)
> > return NULL;
> > return bpts + (off / BPT_SIZE);
> > }
> > diff --git a/arch/powerpc/xmon/xmon_bpts.S b/arch/powerpc/xmon/xmon_bpts.S
> > index ebb2dbc70ca8..09058eb6abbd 100644
> > --- a/arch/powerpc/xmon/xmon_bpts.S
> > +++ b/arch/powerpc/xmon/xmon_bpts.S
> > @@ -3,6 +3,8 @@
> > #include <asm/asm-compat.h>
> > #include "xmon_bpts.h"
> >
> > +/* Prefixed instructions can not cross 64 byte boundaries */
> > +.align 6
> > .global bpt_table
> > bpt_table:
> > - .space NBPTS * 8
> > + .space NBPTS * 16
>
^ permalink raw reply
* Re: [RFC PATCH 2/3] powerpc/lib: Initialize a temporary mm for code patching
From: Christopher M Riedl @ 2020-04-15 4:39 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <8ff6d279-fdd9-4e4d-b4e0-f5c5cba480a4@c-s.fr>
> On April 8, 2020 6:01 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
> Le 31/03/2020 à 05:19, Christopher M Riedl a écrit :
> >> On March 24, 2020 11:10 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >>
> >>
> >> Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
> >>> When code patching a STRICT_KERNEL_RWX kernel the page containing the
> >>> address to be patched is temporarily mapped with permissive memory
> >>> protections. Currently, a per-cpu vmalloc patch area is used for this
> >>> purpose. While the patch area is per-cpu, the temporary page mapping is
> >>> inserted into the kernel page tables for the duration of the patching.
> >>> The mapping is exposed to CPUs other than the patching CPU - this is
> >>> undesirable from a hardening perspective.
> >>>
> >>> Use the `poking_init` init hook to prepare a temporary mm and patching
> >>> address. Initialize the temporary mm by copying the init mm. Choose a
> >>> randomized patching address inside the temporary mm userspace address
> >>> portion. The next patch uses the temporary mm and patching address for
> >>> code patching.
> >>>
> >>> Based on x86 implementation:
> >>>
> >>> commit 4fc19708b165
> >>> ("x86/alternatives: Initialize temporary mm for patching")
> >>>
> >>> Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
> >>> ---
> >>> arch/powerpc/lib/code-patching.c | 26 ++++++++++++++++++++++++++
> >>> 1 file changed, 26 insertions(+)
> >>>
> >>> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> >>> index 3345f039a876..18b88ecfc5a8 100644
> >>> --- a/arch/powerpc/lib/code-patching.c
> >>> +++ b/arch/powerpc/lib/code-patching.c
> >>> @@ -11,6 +11,8 @@
> >>> #include <linux/cpuhotplug.h>
> >>> #include <linux/slab.h>
> >>> #include <linux/uaccess.h>
> >>> +#include <linux/sched/task.h>
> >>> +#include <linux/random.h>
> >>>
> >>> #include <asm/pgtable.h>
> >>> #include <asm/tlbflush.h>
> >>> @@ -39,6 +41,30 @@ int raw_patch_instruction(unsigned int *addr, unsigned int instr)
> >>> }
> >>>
> >>> #ifdef CONFIG_STRICT_KERNEL_RWX
> >>> +
> >>> +__ro_after_init struct mm_struct *patching_mm;
> >>> +__ro_after_init unsigned long patching_addr;
> >>
> >> Can we make those those static ?
> >>
> >
> > Yes, makes sense to me.
> >
> >>> +
> >>> +void __init poking_init(void)
> >>> +{
> >>> + spinlock_t *ptl; /* for protecting pte table */
> >>> + pte_t *ptep;
> >>> +
> >>> + patching_mm = copy_init_mm();
> >>> + BUG_ON(!patching_mm);
> >>
> >> Does it needs to be a BUG_ON() ? Can't we fail gracefully with just a
> >> WARN_ON ?
> >>
> >
> > I'm not sure what failing gracefully means here? The main reason this could
> > fail is if there is not enough memory to allocate the patching_mm. The
> > previous implementation had this justification for BUG_ON():
>
> But the system can continue running just fine after this failure.
> Only the things that make use of code patching will fail (ftrace, kgdb, ...)
>
> Checkpatch tells: "Avoid crashing the kernel - try using WARN_ON &
> recovery code rather than BUG() or BUG_ON()"
>
> All vital code patching has already been done previously, so I think a
> WARN_ON() should be enough, plus returning non 0 to indicate that the
> late_initcall failed.
>
>
Got it, makes sense to me. I will make these changes in the next version.
Thanks!
> >
> > /*
> > * Run as a late init call. This allows all the boot time patching to be done
> > * simply by patching the code, and then we're called here prior to
> > * mark_rodata_ro(), which happens after all init calls are run. Although
> > * BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge
> > * it as being preferable to a kernel that will crash later when someone tries
> > * to use patch_instruction().
> > */
> > static int __init setup_text_poke_area(void)
> > {
> > BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> > "powerpc/text_poke:online", text_area_cpu_up,
> > text_area_cpu_down));
> >
> > return 0;
> > }
> > late_initcall(setup_text_poke_area);
> >
> > I think the BUG_ON() is appropriate even if only to adhere to the previous
> > judgement call. I can add a similar comment explaining the reasoning if
> > that helps.
> >
> >>> +
> >>> + /*
> >>> + * In hash we cannot go above DEFAULT_MAP_WINDOW easily.
> >>> + * XXX: Do we want additional bits of entropy for radix?
> >>> + */
> >>> + patching_addr = (get_random_long() & PAGE_MASK) %
> >>> + (DEFAULT_MAP_WINDOW - PAGE_SIZE);
> >>> +
> >>> + ptep = get_locked_pte(patching_mm, patching_addr, &ptl);
> >>> + BUG_ON(!ptep);
> >>
> >> Same here, can we fail gracefully instead ?
> >>
> >
> > Same reasoning as above.
>
> Here as well, a WARN_ON() should be enough, the system will continue
> running after that.
>
> >
> >>> + pte_unmap_unlock(ptep, ptl);
> >>> +}
> >>> +
> >>> static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
> >>>
> >>> static int text_area_cpu_up(unsigned int cpu)
> >>>
> >>
> >> Christophe
>
> Christophe
^ permalink raw reply
* Re: [PATCH 4/8] binfmt_elf: open code copy_siginfo_to_user to kernelspace buffer
From: Michael Ellerman @ 2020-04-15 3:01 UTC (permalink / raw)
To: Christoph Hellwig, Andrew Morton, Alexander Viro
Cc: Arnd Bergmann, linux-kernel, Jeremy Kerr, linux-fsdevel,
linuxppc-dev, Eric W . Biederman
In-Reply-To: <20200414070142.288696-5-hch@lst.de>
Christoph Hellwig <hch@lst.de> writes:
> Instead of messing with the address limit just open code the trivial
> memcpy + memset logic for the native version, and a call to
> to_compat_siginfo for the compat version.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/binfmt_elf.c | 9 +++++----
> fs/compat_binfmt_elf.c | 6 +++++-
> 2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 13f25e241ac4..607c5a5f855e 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1553,15 +1553,16 @@ static void fill_auxv_note(struct memelfnote *note, struct mm_struct *mm)
> fill_note(note, "CORE", NT_AUXV, i * sizeof(elf_addr_t), auxv);
> }
>
> +#ifndef fill_siginfo_note
> static void fill_siginfo_note(struct memelfnote *note, user_siginfo_t *csigdata,
> const kernel_siginfo_t *siginfo)
> {
> - mm_segment_t old_fs = get_fs();
> - set_fs(KERNEL_DS);
> - copy_siginfo_to_user((user_siginfo_t __user *) csigdata, siginfo);
> - set_fs(old_fs);
> + memcpy(csigdata, siginfo, sizeof(struct kernel_siginfo));
> + memset((char *)csigdata + sizeof(struct kernel_siginfo), 0,
> + SI_EXPANSION_SIZE);
> fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata);
> }
> +#endif
>
> #define MAX_FILE_NOTE_SIZE (4*1024*1024)
> /*
> diff --git a/fs/compat_binfmt_elf.c b/fs/compat_binfmt_elf.c
> index aaad4ca1217e..ab84e095618b 100644
> --- a/fs/compat_binfmt_elf.c
> +++ b/fs/compat_binfmt_elf.c
> @@ -39,7 +39,11 @@
> */
> #define user_long_t compat_long_t
> #define user_siginfo_t compat_siginfo_t
> -#define copy_siginfo_to_user copy_siginfo_to_user32
> +#define fill_siginfo_note(note, csigdata, siginfo) \
> +do { \
> + to_compat_siginfo(csigdata, siginfo, compat_siginfo_flags()); \
> + fill_note(note, "CORE", NT_SIGINFO, sizeof(*csigdata), csigdata); \
> +} while (0)
This doesn't build on ppc (cell_defconfig):
../fs/binfmt_elf.c: In function 'fill_note_info':
../fs/compat_binfmt_elf.c:44:39: error: implicit declaration of function 'compat_siginfo_flags'; did you mean 'to_compat_siginfo'? [-Werror=implicit-function-d
eclaration]
to_compat_siginfo(csigdata, siginfo, compat_siginfo_flags()); \
^~~~~~~~~~~~~~~~~~~~
../fs/binfmt_elf.c:1846:2: note: in expansion of macro 'fill_siginfo_note'
fill_siginfo_note(&info->signote, &info->csigdata, siginfo);
^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
make[2]: *** [../scripts/Makefile.build:266: fs/compat_binfmt_elf.o] Error 1
I guess the empty version from kernel/signal.c needs to move into a
header somewhere.
cheers
^ permalink raw reply
* [PATCH kernel] powerpc/dma: Call indirect dma_ops when persistent memory present
From: Alexey Kardashevskiy @ 2020-04-15 2:58 UTC (permalink / raw)
To: linuxppc-dev
Cc: Brian J King, Alexey Kardashevskiy, Sam Bobroff,
Aneesh Kumar K . V, Wen Xiong, Christoph Hellwig, David Gibson
Unlike normal memory ("memory" compatible type in the FDT),
the persistent memory ("ibm,pmemory" in the FDT) can be mapped anywhere
in the guest physical space and it can be used for DMA.
In order to maintain 1:1 mapping via the huge DMA window, we need to
know the maximum physical address at the time of the window setup.
So far we've been looking at "memory" nodes but "ibm,pmemory" does not
have fixed addresses and the persistent memory may be mapped afterwards.
When the maximum window size is not big enough to fit persistent memory,
this clears the dma_ops_bypass flag to tell the generic code that indirect
dma_ops call is needed. This lets the platform code check the DMA
boundaries and call direct DMA API if DMA-ing to/from generic RAM
or call IOMMU API otherwise.
This adds dma_max to device::archdata to tell the direct DMA mapping
limit. At the moment only pseries sets the limit so powernv is
unaffected by this change.
As persistent memory is backed with page structs, this uses
MAX_PHYSMEM_BITS as the upper limit (rather than simple 64bit).
This should not change the existing behaviour when no persistent memory.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
This is based on
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=170152
---
arch/powerpc/include/asm/device.h | 2 +
arch/powerpc/kernel/dma-iommu.c | 68 +++++++++++++++++++++++++-
arch/powerpc/platforms/pseries/iommu.c | 23 ++++++++-
3 files changed, 89 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/device.h b/arch/powerpc/include/asm/device.h
index 452402215e12..380e92684a16 100644
--- a/arch/powerpc/include/asm/device.h
+++ b/arch/powerpc/include/asm/device.h
@@ -18,6 +18,8 @@ struct iommu_table;
* drivers/macintosh/macio_asic.c
*/
struct dev_archdata {
+ /* Maximum DMA address for 1:1 mapping (when enabled) */
+ dma_addr_t dma_max;
/*
* These two used to be a union. However, with the hybrid ops we need
* both so here we store both a DMA offset for direct mappings and
diff --git a/arch/powerpc/kernel/dma-iommu.c b/arch/powerpc/kernel/dma-iommu.c
index 569fecd7b5b2..8c67bfffdef6 100644
--- a/arch/powerpc/kernel/dma-iommu.c
+++ b/arch/powerpc/kernel/dma-iommu.c
@@ -10,6 +10,16 @@
#include <linux/pci.h>
#include <asm/iommu.h>
+static inline bool can_map_direct(struct device *dev, phys_addr_t addr)
+{
+ return dev->archdata.dma_max >= phys_to_dma(dev, addr);
+}
+
+static inline bool dma_handle_direct(struct device *dev, dma_addr_t dma_handle)
+{
+ return dma_handle >= dev->archdata.dma_offset;
+}
+
/*
* Generic iommu implementation
*/
@@ -44,6 +54,12 @@ static dma_addr_t dma_iommu_map_page(struct device *dev, struct page *page,
enum dma_data_direction direction,
unsigned long attrs)
{
+ if (dev->archdata.dma_max &&
+ can_map_direct(dev, (phys_addr_t) page_to_phys(page) +
+ offset + size))
+ return dma_direct_map_page(dev, page, offset, size, direction,
+ attrs);
+
return iommu_map_page(dev, get_iommu_table_base(dev), page, offset,
size, dma_get_mask(dev), direction, attrs);
}
@@ -53,6 +69,12 @@ static void dma_iommu_unmap_page(struct device *dev, dma_addr_t dma_handle,
size_t size, enum dma_data_direction direction,
unsigned long attrs)
{
+ if (dev->archdata.dma_max &&
+ dma_handle_direct(dev, dma_handle + size)) {
+ dma_direct_unmap_page(dev, dma_handle, size, direction, attrs);
+ return;
+ }
+
iommu_unmap_page(get_iommu_table_base(dev), dma_handle, size, direction,
attrs);
}
@@ -62,6 +84,22 @@ static int dma_iommu_map_sg(struct device *dev, struct scatterlist *sglist,
int nelems, enum dma_data_direction direction,
unsigned long attrs)
{
+ if (dev->archdata.dma_max) {
+ struct scatterlist *s;
+ bool direct = true;
+ int i;
+
+ for_each_sg(sglist, s, nelems, i) {
+ direct = can_map_direct(dev,
+ sg_phys(s) + s->offset + s->length);
+ if (!direct)
+ break;
+ }
+ if (direct)
+ return dma_direct_map_sg(dev, sglist, nelems, direction,
+ attrs);
+ }
+
return ppc_iommu_map_sg(dev, get_iommu_table_base(dev), sglist, nelems,
dma_get_mask(dev), direction, attrs);
}
@@ -70,6 +108,24 @@ static void dma_iommu_unmap_sg(struct device *dev, struct scatterlist *sglist,
int nelems, enum dma_data_direction direction,
unsigned long attrs)
{
+ if (dev->archdata.dma_max) {
+ struct scatterlist *s;
+ bool direct = true;
+ int i;
+
+ for_each_sg(sglist, s, nelems, i) {
+ direct = dma_handle_direct(dev,
+ s->dma_address + s->length);
+ if (!direct)
+ break;
+ }
+ if (direct) {
+ dma_direct_unmap_sg(dev, sglist, nelems, direction,
+ attrs);
+ return;
+ }
+ }
+
ppc_iommu_unmap_sg(get_iommu_table_base(dev), sglist, nelems,
direction, attrs);
}
@@ -90,8 +146,16 @@ int dma_iommu_dma_supported(struct device *dev, u64 mask)
struct iommu_table *tbl = get_iommu_table_base(dev);
if (dev_is_pci(dev) && dma_iommu_bypass_supported(dev, mask)) {
- dev->dma_ops_bypass = true;
- dev_dbg(dev, "iommu: 64-bit OK, using fixed ops\n");
+ /*
+ * dma_iommu_bypass_supported() sets dma_max when there is
+ * 1:1 mapping but it is somehow limited.
+ * ibm,pmemory is one example.
+ */
+ dev->dma_ops_bypass = dev->archdata.dma_max == 0;
+ if (!dev->dma_ops_bypass)
+ dev_warn(dev, "iommu: 64-bit OK but using default ops\n");
+ else
+ dev_dbg(dev, "iommu: 64-bit OK, using fixed ops\n");
return 1;
}
diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
index 6d47b4a3ce39..227055a4fbe2 100644
--- a/arch/powerpc/platforms/pseries/iommu.c
+++ b/arch/powerpc/platforms/pseries/iommu.c
@@ -994,7 +994,7 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
struct ddw_query_response query;
struct ddw_create_response create;
int page_shift;
- u64 dma_addr, max_addr;
+ u64 dma_addr, max_addr = 0, dma_max_sz = 0;
struct device_node *dn;
u32 ddw_avail[3];
struct direct_window *window;
@@ -1066,7 +1066,23 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
}
/* verify the window * number of ptes will map the partition */
/* check largest block * page size > max memory hotplug addr */
- max_addr = ddw_memory_hotplug_max();
+ /*
+ * The "ibm,pmemory" can appear anywhere in the address space.
+ * Assuming it is still backed by page structs, try MAX_PHYSMEM_BITS
+ * for the upper limit and fallback to max RAM otherwise but this
+ * disables device::dma_ops_bypass.
+ */
+ if (of_find_node_by_type(NULL, "ibm,pmemory")) {
+ max_addr = 1ULL << (MAX_PHYSMEM_BITS - page_shift);
+ if (query.largest_available_block < max_addr) {
+ dev_info(&dev->dev, "Skipping ibm,pmemory");
+ max_addr = ddw_memory_hotplug_max();
+ dma_max_sz = max_addr;
+ }
+ } else {
+ max_addr = ddw_memory_hotplug_max();
+ }
+
if (query.largest_available_block < (max_addr >> page_shift)) {
dev_dbg(&dev->dev, "can't map partition max 0x%llx with %u "
"%llu-sized pages\n", max_addr, query.largest_available_block,
@@ -1151,6 +1167,9 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
out_unlock:
mutex_unlock(&direct_window_init_mutex);
+ if (!dev->dev.archdata.dma_max && dma_addr && dma_max_sz)
+ dev->dev.archdata.dma_max = dma_addr + dma_max_sz;
+
return dma_addr;
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v6 6/7] ASoC: dt-bindings: fsl_easrc: Add document for EASRC
From: Shengjiu Wang @ 2020-04-15 2:55 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, Nicolin Chen, Mark Brown,
linuxppc-dev, linux-kernel
In-Reply-To: <20200414154643.GA29098@bogus>
Hi Rob
On Tue, Apr 14, 2020 at 11:49 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Apr 01, 2020 at 04:45:39PM +0800, Shengjiu Wang wrote:
> > EASRC (Enhanced Asynchronous Sample Rate Converter) is a new
> > IP module found on i.MX8MN.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> > .../devicetree/bindings/sound/fsl,easrc.yaml | 101 ++++++++++++++++++
> > 1 file changed, 101 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/sound/fsl,easrc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/sound/fsl,easrc.yaml b/Documentation/devicetree/bindings/sound/fsl,easrc.yaml
> > new file mode 100644
> > index 000000000000..14ea60084420
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/fsl,easrc.yaml
> > @@ -0,0 +1,101 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/fsl,easrc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP Asynchronous Sample Rate Converter (ASRC) Controller
> > +
> > +maintainers:
> > + - Shengjiu Wang <shengjiu.wang@nxp.com>
> > +
> > +properties:
> > + $nodename:
> > + pattern: "^easrc@.*"
> > +
> > + compatible:
> > + const: fsl,imx8mn-easrc
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + clocks:
> > + items:
> > + - description: Peripheral clock
> > +
> > + clock-names:
> > + items:
> > + - const: mem
> > +
> > + dmas:
> > + maxItems: 8
> > +
> > + dma-names:
> > + items:
> > + - const: ctx0_rx
> > + - const: ctx0_tx
> > + - const: ctx1_rx
> > + - const: ctx1_tx
> > + - const: ctx2_rx
> > + - const: ctx2_tx
> > + - const: ctx3_rx
> > + - const: ctx3_tx
> > +
> > + firmware-name:
> > + allOf:
> > + - $ref: /schemas/types.yaml#/definitions/string
> > + - const: imx/easrc/easrc-imx8mn.bin
> > + description: The coefficient table for the filters
> > +
> > + fsl,asrc-rate:
>
> fsl,asrc-rate-hz
Can we keep "fsl,asrc-rate", because I want this property
align with the one in fsl,asrc.txt. These two asrc modules
can share same property name.
best regards
wang shengjiu
^ 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