Re: [PATCH] net: ucc_geth: Drop extraneous parentheses in comparison
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Fri, 23 Oct 2020 14:32:36 +1100 you wrote: > Clang warns about the extra parentheses in this comparison: > > drivers/net/ethernet/freescale/ucc_geth.c:1361:28: > warning: equality comparison with extraneous parentheses > if ((ugeth->phy_interface == PHY_INTERFACE_MODE_SGMII)) > ~^~~ > > [...] Here is the summary with links: - net: ucc_geth: Drop extraneous parentheses in comparison https://git.kernel.org/netdev/net/c/dab234227cbd You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH] net: ucc_geth: Drop extraneous parentheses in comparison
On Fri, 23 Oct 2020 14:32:36 +1100 Michael Ellerman wrote: > Clang warns about the extra parentheses in this comparison: > > drivers/net/ethernet/freescale/ucc_geth.c:1361:28: > warning: equality comparison with extraneous parentheses > if ((ugeth->phy_interface == PHY_INTERFACE_MODE_SGMII)) > ~^~~ > > It seems clear the intent here is to do a comparison not an > assignment, so drop the extra parentheses to avoid any confusion. > > Signed-off-by: Michael Ellerman Applied, thanks!
RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Segher Boessenkool > Sent: 23 October 2020 19:27 > > On Fri, Oct 23, 2020 at 06:58:57PM +0100, Al Viro wrote: > > On Fri, Oct 23, 2020 at 03:09:30PM +0200, David Hildenbrand wrote: > > > > > Now, I am not a compiler expert, but as I already cited, at least on > > > x86-64 clang expects that the high bits were cleared by the caller - in > > > contrast to gcc. I suspect it's the same on arm64, but again, I am no > > > compiler expert. > > > > > > If what I said and cites for x86-64 is correct, if the function expects > > > an "unsigned int", it will happily use 64bit operations without further > > > checks where valid when assuming high bits are zero. That's why even > > > converting everything to "unsigned int" as proposed by me won't work on > > > clang - it assumes high bits are zero (as indicated by Nick). > > > > > > As I am neither a compiler experts (did I mention that already? ;) ) nor > > > an arm64 experts, I can't tell if this is a compiler BUG or not. > > > > On arm64 when callee expects a 32bit argument, the caller is *not* > > responsible > > for clearing the upper half of 64bit register used to pass the value - it > > only > > needs to store the actual value into the lower half. The callee must > > consider > > the contents of the upper half of that register as undefined. See AAPCS64 > > (e.g. > > https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#parameter-passing-rules > > ); AFAICS, the relevant bit is > > "Unlike in the 32-bit AAPCS, named integral values must be narrowed by > > the callee rather than the caller." > > Or the formal rule: > > C.9 If the argument is an Integral or Pointer Type, the size of the > argument is less than or equal to 8 bytes and the NGRN is less > than 8, the argument is copied to the least significant bits in > x[NGRN]. The NGRN is incremented by one. The argument has now > been allocated. So, in essence, if the value is in a 64bit register the calling code is independent of the actual type of the formal parameter. Clearly a value might need explicit widening. I've found a copy of the 64 bit arm instruction set. Unfortunately it is alpha sorted and repetitive so shows none of the symmetry and makes things difficult to find. But, contrary to what someone suggested most register writes (eg from arithmetic) seem to zero/extend the high bits. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH] treewide: Convert macro and uses of __section(foo) to __section("foo")
On Fri, Oct 23, 2020 at 10:03 AM Joe Perches wrote: > > Thanks Miguel, but IMO it doesn't need time in next. You're welcome! It never hurts to keep things for a bit there. > Applying it just before an rc1 minimizes conflicts. There shouldn't be many conflicts after -rc1. The amount of changes is reasonable too, so no need to apply the script directly. In any case, if you prefer that Linus picks it up himself right away for this -rc1, it looks good to me (with the caveat that it isn't tested): Reviewed-by: Miguel Ojeda Cheers, Miguel
Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
On Fri, Oct 23, 2020 at 06:58:57PM +0100, Al Viro wrote: > On Fri, Oct 23, 2020 at 03:09:30PM +0200, David Hildenbrand wrote: > > > Now, I am not a compiler expert, but as I already cited, at least on > > x86-64 clang expects that the high bits were cleared by the caller - in > > contrast to gcc. I suspect it's the same on arm64, but again, I am no > > compiler expert. > > > > If what I said and cites for x86-64 is correct, if the function expects > > an "unsigned int", it will happily use 64bit operations without further > > checks where valid when assuming high bits are zero. That's why even > > converting everything to "unsigned int" as proposed by me won't work on > > clang - it assumes high bits are zero (as indicated by Nick). > > > > As I am neither a compiler experts (did I mention that already? ;) ) nor > > an arm64 experts, I can't tell if this is a compiler BUG or not. > > On arm64 when callee expects a 32bit argument, the caller is *not* responsible > for clearing the upper half of 64bit register used to pass the value - it only > needs to store the actual value into the lower half. The callee must consider > the contents of the upper half of that register as undefined. See AAPCS64 > (e.g. > https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#parameter-passing-rules > ); AFAICS, the relevant bit is > "Unlike in the 32-bit AAPCS, named integral values must be narrowed by > the callee rather than the caller." Or the formal rule: C.9 If the argument is an Integral or Pointer Type, the size of the argument is less than or equal to 8 bytes and the NGRN is less than 8, the argument is copied to the least significant bits in x[NGRN]. The NGRN is incremented by one. The argument has now been allocated. Segher
Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
On Fri, Oct 23, 2020 at 03:09:30PM +0200, David Hildenbrand wrote: > Now, I am not a compiler expert, but as I already cited, at least on > x86-64 clang expects that the high bits were cleared by the caller - in > contrast to gcc. I suspect it's the same on arm64, but again, I am no > compiler expert. > > If what I said and cites for x86-64 is correct, if the function expects > an "unsigned int", it will happily use 64bit operations without further > checks where valid when assuming high bits are zero. That's why even > converting everything to "unsigned int" as proposed by me won't work on > clang - it assumes high bits are zero (as indicated by Nick). > > As I am neither a compiler experts (did I mention that already? ;) ) nor > an arm64 experts, I can't tell if this is a compiler BUG or not. On arm64 when callee expects a 32bit argument, the caller is *not* responsible for clearing the upper half of 64bit register used to pass the value - it only needs to store the actual value into the lower half. The callee must consider the contents of the upper half of that register as undefined. See AAPCS64 (e.g. https://github.com/ARM-software/abi-aa/blob/master/aapcs64/aapcs64.rst#parameter-passing-rules ); AFAICS, the relevant bit is "Unlike in the 32-bit AAPCS, named integral values must be narrowed by the callee rather than the caller."
Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
On 23.10.20 16:47, 'Greg KH' wrote: > On Fri, Oct 23, 2020 at 02:39:24PM +, David Laight wrote: >> From: David Hildenbrand >>> Sent: 23 October 2020 15:33 >> ... >>> I just checked against upstream code generated by clang 10 and it >>> properly discards the upper 32bit via a mov w23 w2. >>> >>> So at least clang 10 indeed properly assumes we could have garbage and >>> masks it off. >>> >>> Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11 >>> behaves differently. >> >> We'll need the disassembly from a failing kernel image. >> It isn't that big to hand annotate. > > I've worked around the merge at the moment in the android tree, but it > is still quite reproducable, and will try to get a .o file to > disassemble on Monday or so... I just compiled pre and post fb041b598997d63c0f7d7305dfae70046bf66fe1 with clang version 11.0.0 (Fedora 11.0.0-0.2.rc1.fc33) for aarch64 with defconfig and extracted import_iovec and rw_copy_check_uvector (skipping the compat things) Pre fb041b598997d63c0f7d7305dfae70046bf66fe1 import_iovec -> https://pastebin.com/LtnYMLJt Post fb041b598997d63c0f7d7305dfae70046bf66fe1 import_iovec -> https://pastebin.com/BWPmXrAf Pre fb041b598997d63c0f7d7305dfae70046bf66fe1 rw_copy_check_uvector -> https://pastebin.com/4nSBYRbf Post fb041b598997d63c0f7d7305dfae70046bf66fe1 rw_copy_check_uvector -> https://pastebin.com/hPtEgaEW I'm only able to spot minor differences ... less gets inlined than I would have expected. But there are some smaller differences. Maybe someone wants to have a look before we have object files as used by Greg ... -- Thanks, David / dhildenb
Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
On Fri, Oct 23, 2020 at 02:39:24PM +, David Laight wrote: > From: David Hildenbrand > > Sent: 23 October 2020 15:33 > ... > > I just checked against upstream code generated by clang 10 and it > > properly discards the upper 32bit via a mov w23 w2. > > > > So at least clang 10 indeed properly assumes we could have garbage and > > masks it off. > > > > Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11 > > behaves differently. > > We'll need the disassembly from a failing kernel image. > It isn't that big to hand annotate. I've worked around the merge at the moment in the android tree, but it is still quite reproducable, and will try to get a .o file to disassemble on Monday or so... thanks, greg k-h
RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: David Hildenbrand > Sent: 23 October 2020 15:33 ... > I just checked against upstream code generated by clang 10 and it > properly discards the upper 32bit via a mov w23 w2. > > So at least clang 10 indeed properly assumes we could have garbage and > masks it off. > > Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11 > behaves differently. We'll need the disassembly from a failing kernel image. It isn't that big to hand annotate. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
On 23.10.20 15:09, David Hildenbrand wrote: > On 23.10.20 14:46, David Laight wrote: >> From: Greg KH >>> Sent: 22 October 2020 14:51 >> >> I've rammed the code into godbolt. >> >> https://godbolt.org/z/9v5PPW >> >> Definitely a clang bug. >> >> Search for [wx]24 in the clang output. >> nr_segs comes in as w2 and the initial bound checks are done on w2. >> w24 is loaded from w2 - I don't believe this changes the high bits. >> There are no references to w24, just x24. >> So the kmalloc_array() is passed 'huge' and will fail. >> The iov_iter_init also gets the 64bit value. >> >> Note that the gcc code has a sign-extend copy of w2. > > Do we have a result from using "unsigned long" in the base function and > explicitly masking of the high bits? That should definitely work. > > Now, I am not a compiler expert, but as I already cited, at least on > x86-64 clang expects that the high bits were cleared by the caller - in > contrast to gcc. I suspect it's the same on arm64, but again, I am no > compiler expert. > > If what I said and cites for x86-64 is correct, if the function expects > an "unsigned int", it will happily use 64bit operations without further > checks where valid when assuming high bits are zero. That's why even > converting everything to "unsigned int" as proposed by me won't work on > clang - it assumes high bits are zero (as indicated by Nick). > > As I am neither a compiler experts (did I mention that already? ;) ) nor > an arm64 experts, I can't tell if this is a compiler BUG or not. > I just checked against upstream code generated by clang 10 and it properly discards the upper 32bit via a mov w23 w2. So at least clang 10 indeed properly assumes we could have garbage and masks it off. Maybe the issue is somewhere else, unrelated to nr_pages ... or clang 11 behaves differently. -- Thanks, David / dhildenb
Re: [PATCH v8 2/8] powerpc/vdso: Remove __kernel_datapage_offset and simplify __get_datapage()
Hi Christophe, Will, On 10/23/20 12:57 PM, Christophe Leroy wrote: > > > Le 23/10/2020 à 13:25, Will Deacon a écrit : >> On Fri, Oct 23, 2020 at 01:22:04PM +0200, Christophe Leroy wrote: >>> Hi Dmitry, [..] >>> I haven't seen the patches, did you sent them out finally ? I was working on .close() hook, but while cooking it, I thought it may be better to make tracking of user landing generic. Note that the vdso base address is mostly needed by kernel as an address to land in userspace after processing a signal. I have some raw patches that add +#ifdef CONFIG_ARCH_HAS_USER_LANDING + struct vm_area_struct *user_landing; +#endif inside mm_struct and I plan to finish them after rc1 gets released. While working on that, I noticed that arm32 and some other architectures track vdso position in mm.context with the only reason to add AT_SYSINFO_EHDR in the elf header that's being loaded. That's quite overkill to have a pointer in mm.context that rather can be a local variable in elf binfmt loader. Also, I found some issues with mremap code. The patches series mentioned are at the base of the branch with generic user landing. I have sent only those patches not the full branch as I remember there was a policy that during merge window one should send only fixes, rather than refactoring/new code. >> I think it's this series: >> >> https://lore.kernel.org/r/20201013013416.390574-1-d...@arista.com >> >> but they look really invasive to me, so I may cook a small hack for arm64 >> in the meantine / for stable. I don't mind small hacks, but I'm concerned that the suggested fix which sets `mm->context.vdso_base = 0` on munmap() may have it's issue: that won't work if a user for whatever-broken-reason will mremap() vdso on 0 address. As the fix supposes to fix an issue that hasn't fired for anyone yet, it probably shouldn't introduce another. That's why I've used vm_area_struct to track vdso position in the patches set. Probably, temporary, you could use something like: #define BAD_VDSO_ADDRESS(-1)UL Or non-page-aligned address. But the signal code that checks if it can land on vdso/sigpage should be also aligned with the new definition. > Not sure we are talking about the same thing. > > I can't see any new .close function added to vm_special_mapping in order > to replace arch_unmap() hook. Thanks, Dmitry
RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Arnd Bergmann > Sent: 23 October 2020 14:23 > > On Fri, Oct 23, 2020 at 2:46 PM David Laight wrote: > > > > From: Greg KH > > > Sent: 22 October 2020 14:51 > > > > I've rammed the code into godbolt. > > > > https://godbolt.org/z/9v5PPW > > > > Definitely a clang bug. > > > > Search for [wx]24 in the clang output. > > nr_segs comes in as w2 and the initial bound checks are done on w2. > > w24 is loaded from w2 - I don't believe this changes the high bits. > > You believe wrong, "mov w24, w2" is a zero-extending operation. Ah ok, but gcc uses utxw for the same task. I guess they could be the same opcode. Last time I wrote ARM thumb didn't really exist - never mind 64bit David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: C vdso
Christophe Leroy writes: > Le 24/09/2020 à 15:17, Christophe Leroy a écrit : >> Le 17/09/2020 à 14:33, Michael Ellerman a écrit : >>> Christophe Leroy writes: What is the status with the generic C vdso merge ? In some mail, you mentionned having difficulties getting it working on ppc64, any progress ? What's the problem ? Can I help ? >>> >>> Yeah sorry I was hoping to get time to work on it but haven't been able >>> to. >>> >>> It's causing crashes on ppc64 ie. big endian. ... >> >> Can you tell what defconfig you are using ? I have been able to setup a full >> glibc PPC64 cross >> compilation chain and been able to test it under QEMU with success, using >> Nathan's vdsotest tool. > > What config are you using ? ppc64_defconfig + guest.config Or pseries_defconfig. I'm using Ubuntu GCC 9.3.0 mostly, but it happens with other toolchains too. At a minimum we're seeing relocations in the output, which is a problem: $ readelf -r build\~/arch/powerpc/kernel/vdso64/vdso64.so Relocation section '.rela.dyn' at offset 0x12a8 contains 8 entries: Offset Info Type Sym. ValueSym. Name + Addend 1368 0016 R_PPC64_RELATIVE 7c0 1370 0016 R_PPC64_RELATIVE 9300 1380 0016 R_PPC64_RELATIVE 970 1388 0016 R_PPC64_RELATIVE 9300 1398 0016 R_PPC64_RELATIVE a90 13a0 0016 R_PPC64_RELATIVE 9300 13b0 0016 R_PPC64_RELATIVE b20 13b8 0016 R_PPC64_RELATIVE 9300 cheers
Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
On Fri, Oct 23, 2020 at 2:46 PM David Laight wrote: > > From: Greg KH > > Sent: 22 October 2020 14:51 > > I've rammed the code into godbolt. > > https://godbolt.org/z/9v5PPW > > Definitely a clang bug. > > Search for [wx]24 in the clang output. > nr_segs comes in as w2 and the initial bound checks are done on w2. > w24 is loaded from w2 - I don't believe this changes the high bits. You believe wrong, "mov w24, w2" is a zero-extending operation. Arnd
Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
On 22.10.20 21:24, Al Viro wrote:
> On Thu, Oct 22, 2020 at 12:04:52PM -0700, Nick Desaulniers wrote:
>
>> Passing an `unsigned long` as an `unsigned int` does no such
>> narrowing: https://godbolt.org/z/TvfMxe (same vice-versa, just tail
>> calls, no masking instructions).
>> So if rw_copy_check_uvector() is inlined into import_iovec() (looking
>> at the mainline@1028ae406999), then children calls of
>> `rw_copy_check_uvector()` will be interpreting the `nr_segs` register
>> unmodified, ie. garbage in the upper 32b.
>
> FWIW,
>
> void f(unsinged long v)
> {
> if (v != 1)
> printf("failed\n");
> }
>
> void g(unsigned int v)
> {
> f(v);
> }
>
> void h(unsigned long v)
> {
> g(v);
> }
>
> main()
> {
> h(0x10001);
> }
>
> must not produce any output on a host with 32bit int and 64bit long,
> regardless of
> the inlining, having functions live in different compilation units, etc.
>
> Depending upon the calling conventions, compiler might do truncation in
> caller or
> in a callee, but it must be done _somewhere_.
The interesting case is having g() in a separate compilation unit and
force-calling g() with 0x10001 via inline ASM. So forcing garbage
into high bits.
I'll paly with it.
--
Thanks,
David / dhildenb
Re: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
On 23.10.20 14:46, David Laight wrote: > From: Greg KH >> Sent: 22 October 2020 14:51 > > I've rammed the code into godbolt. > > https://godbolt.org/z/9v5PPW > > Definitely a clang bug. > > Search for [wx]24 in the clang output. > nr_segs comes in as w2 and the initial bound checks are done on w2. > w24 is loaded from w2 - I don't believe this changes the high bits. > There are no references to w24, just x24. > So the kmalloc_array() is passed 'huge' and will fail. > The iov_iter_init also gets the 64bit value. > > Note that the gcc code has a sign-extend copy of w2. Do we have a result from using "unsigned long" in the base function and explicitly masking of the high bits? That should definitely work. Now, I am not a compiler expert, but as I already cited, at least on x86-64 clang expects that the high bits were cleared by the caller - in contrast to gcc. I suspect it's the same on arm64, but again, I am no compiler expert. If what I said and cites for x86-64 is correct, if the function expects an "unsigned int", it will happily use 64bit operations without further checks where valid when assuming high bits are zero. That's why even converting everything to "unsigned int" as proposed by me won't work on clang - it assumes high bits are zero (as indicated by Nick). As I am neither a compiler experts (did I mention that already? ;) ) nor an arm64 experts, I can't tell if this is a compiler BUG or not. Main issue seems to be garbage in high bits as originally suggested by me. -- Thanks, David / dhildenb
RE: Buggy commit tracked to: "Re: [PATCH 2/9] iov_iter: move rw_copy_check_uvector() into lib/iov_iter.c"
From: Greg KH > Sent: 22 October 2020 14:51 I've rammed the code into godbolt. https://godbolt.org/z/9v5PPW Definitely a clang bug. Search for [wx]24 in the clang output. nr_segs comes in as w2 and the initial bound checks are done on w2. w24 is loaded from w2 - I don't believe this changes the high bits. There are no references to w24, just x24. So the kmalloc_array() is passed 'huge' and will fail. The iov_iter_init also gets the 64bit value. Note that the gcc code has a sign-extend copy of w2. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH] x86/mpx: fix recursive munmap() corruption
Hi Laurent Le 07/05/2019 à 18:35, Laurent Dufour a écrit : Le 01/05/2019 à 12:32, Michael Ellerman a écrit : Laurent Dufour writes: Le 23/04/2019 à 18:04, Dave Hansen a écrit : On 4/23/19 4:16 AM, Laurent Dufour wrote: ... There are 2 assumptions here: 1. 'start' and 'end' are page aligned (this is guaranteed by __do_munmap(). 2. the VDSO is 1 page (this is guaranteed by the union vdso_data_store on powerpc) Are you sure about #2? The 'vdso64_pages' variable seems rather unnecessary if the VDSO is only 1 page. ;) Hum, not so sure now ;) I got confused, only the header is one page. The test is working as a best effort, and don't cover the case where only few pages inside the VDSO are unmmapped (start > mm->context.vdso_base). This is not what CRIU is doing and so this was enough for CRIU support. Michael, do you think there is a need to manage all the possibility here, since the only user is CRIU and unmapping the VDSO is not a so good idea for other processes ? Couldn't we implement the semantic that if any part of the VDSO is unmapped then vdso_base is set to zero? That should be fairly easy, eg: if (start < vdso_end && end >= mm->context.vdso_base) mm->context.vdso_base = 0; We might need to add vdso_end to the mm->context, but that should be OK. That seems like it would work for CRIU and make sense in general? Sorry for the late answer, yes this would make more sense. Here is a patch doing that. In your patch, the test seems overkill: + if ((start <= vdso_base && vdso_end <= end) || /* 1 */ + (vdso_base <= start && start < vdso_end) || /* 3,4 */ + (vdso_base < end && end <= vdso_end)) /* 2,3 */ + mm->context.vdso_base = mm->context.vdso_end = 0; What about if (start < vdso_end && vdso_start < end) mm->context.vdso_base = mm->context.vdso_end = 0; This should cover all cases, or am I missing something ? And do we really need to store vdso_end in the context ? I think it should be possible to re-calculate it: the size of the VDSO should be (&vdso32_end - &vdso32_start) + PAGE_SIZE for 32 bits VDSO, and (&vdso64_end - &vdso64_start) + PAGE_SIZE for the 64 bits VDSO. Christophe
Re: [PATCH v8 2/8] powerpc/vdso: Remove __kernel_datapage_offset and simplify __get_datapage()
Le 23/10/2020 à 13:25, Will Deacon a écrit : On Fri, Oct 23, 2020 at 01:22:04PM +0200, Christophe Leroy wrote: Hi Dmitry, Le 28/09/2020 à 17:08, Dmitry Safonov a écrit : On 9/27/20 8:43 AM, Christophe Leroy wrote: Le 21/09/2020 à 13:26, Will Deacon a écrit : On Fri, Aug 28, 2020 at 12:14:28PM +1000, Michael Ellerman wrote: Dmitry Safonov <0x7f454...@gmail.com> writes: [..] I'll cook a patch for vm_special_mapping if you don't mind :-) That would be great, thanks! I lost track of this one. Is there a patch kicking around to resolve this, or is the segfault expected behaviour? IIUC dmitry said he will cook a patch. I have not seen any patch yet. Yes, sorry about the delay - I was a bit busy with xfrm patches. I'll send patches for .close() this week, working on them now. I haven't seen the patches, did you sent them out finally ? I think it's this series: https://lore.kernel.org/r/20201013013416.390574-1-d...@arista.com but they look really invasive to me, so I may cook a small hack for arm64 in the meantine / for stable. Not sure we are talking about the same thing. I can't see any new .close function added to vm_special_mapping in order to replace arch_unmap() hook. Christophe
Re: [PATCH v8 2/8] powerpc/vdso: Remove __kernel_datapage_offset and simplify __get_datapage()
On Fri, Oct 23, 2020 at 01:22:04PM +0200, Christophe Leroy wrote: > Hi Dmitry, > > Le 28/09/2020 à 17:08, Dmitry Safonov a écrit : > > On 9/27/20 8:43 AM, Christophe Leroy wrote: > > > > > > > > > Le 21/09/2020 à 13:26, Will Deacon a écrit : > > > > On Fri, Aug 28, 2020 at 12:14:28PM +1000, Michael Ellerman wrote: > > > > > Dmitry Safonov <0x7f454...@gmail.com> writes: > > [..] > > > > > > I'll cook a patch for vm_special_mapping if you don't mind :-) > > > > > > > > > > That would be great, thanks! > > > > > > > > I lost track of this one. Is there a patch kicking around to resolve > > > > this, > > > > or is the segfault expected behaviour? > > > > > > > > > > IIUC dmitry said he will cook a patch. I have not seen any patch yet. > > > > Yes, sorry about the delay - I was a bit busy with xfrm patches. > > > > I'll send patches for .close() this week, working on them now. > > I haven't seen the patches, did you sent them out finally ? I think it's this series: https://lore.kernel.org/r/20201013013416.390574-1-d...@arista.com but they look really invasive to me, so I may cook a small hack for arm64 in the meantine / for stable. Will
Re: [PATCH v8 2/8] powerpc/vdso: Remove __kernel_datapage_offset and simplify __get_datapage()
Hi Dmitry, Le 28/09/2020 à 17:08, Dmitry Safonov a écrit : On 9/27/20 8:43 AM, Christophe Leroy wrote: Le 21/09/2020 à 13:26, Will Deacon a écrit : On Fri, Aug 28, 2020 at 12:14:28PM +1000, Michael Ellerman wrote: Dmitry Safonov <0x7f454...@gmail.com> writes: [..] I'll cook a patch for vm_special_mapping if you don't mind :-) That would be great, thanks! I lost track of this one. Is there a patch kicking around to resolve this, or is the segfault expected behaviour? IIUC dmitry said he will cook a patch. I have not seen any patch yet. Yes, sorry about the delay - I was a bit busy with xfrm patches. I'll send patches for .close() this week, working on them now. I haven't seen the patches, did you sent them out finally ? Christophe
Re: [PATCH v2 1/2] powerpc: Introduce POWER10_DD1 feature
+static void __init fixup_cpu_features(void)
+{
+ unsigned long version = mfspr(SPRN_PVR);
+
+ if ((version & 0x) == 0x00800100)
+ cur_cpu_spec->cpu_features |= CPU_FTR_POWER10_DD1;
+}
+
I am just wondering why this is needed here, but the same thing is not
done for, say, CPU_FTR_POWER9_DD2_1?
When we don't use DT cpu_features (PowerVM / kvm geusts), we call
identify_cpu() twice. First with Real PVR which sets "raw" cpu_spec
as cur_cpu_spec and then 2nd time with Logical PVR (0x0f...) which
(mostly) overwrites the cur_cpu_spec with "architected" mode cpu_spec.
I don't see DD version specific entries for "architected" mode in
cpu_specs[] for any previous processors. So I've introduced this
function to tweak cpu_features.
Though, I don't know why we don't have similar thing for
CPU_FTR_POWER9_DD2_1. I've to check that.
And should we get a /* Power10 DD 1 */ added to cpu_specs[]?
IIUC, we don't need such entry. For PowerVM / kvm guests, we overwrite
cpu_spec, so /* Power10 */ "raw" entry is sufficient. And For baremetal,
we don't use cpu_specs[] at all.
I think even for powernv, using dt features can be disabled by the
cmdline with dt_cpu_ftrs=off, then cpu_specs[] will then be used.
Ok... with dt_cpu_ftrs=off, we seem to be using raw mode cpu_specs[] entry on
baremetal. So yeah, I'll add /* Power10 DD1 */ raw mode entry into cpu_specs[].
Thanks for pointing it out.
-Ravi
Re: [PATCH] powerpc: Send SIGBUS from machine_check
On Fri, 2020-10-23 at 11:57 +1100, Michael Ellerman wrote:
>
>
> Joakim Tjernlund writes:
> > Embedded PPC CPU should send SIGBUS to user space when applicable.
>
> Yeah, but it's not clear that it's applicable in all cases.
>
> At least I need some reasoning for why it's safe in all cases below to
> just send a SIGBUS and take no other action.
For me this came from an User SDK accessing a PCI device(also using PCI IRQs)
and this
SDK did some strange stuff during shutdown which disabled the device before SW
was done.
This caused PCI accesses, both from User Space and kernel PCI IRQs access) to
the device
which caused an Machine Check(PCI transfer failed). Without this patch, the
kernel
would just OOPS and hang/do strange things even for an access made by User
space.
Now the User app just gets a SIGBUS and the kernel still works as it should.
Perhaps a SIGBUS and recover isn't right in all cases but without it there will
be a
system break down.
> Is there a particular CPU you're working on? Can we start with that and
> look at all the machine check causes and which can be safely handled.
This was a T1042(e5500) but we have e500 and mpc832x as well.
>
> Some comments below ...
>
>
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index 0381242920d9..12715d24141c 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -621,6 +621,11 @@ int machine_check_e500mc(struct pt_regs *regs)
>
> At the beginning of the function we have:
>
> printk("Machine check in kernel mode.\n");
>
> Which should be updated.
Sure, just remove the "in kernel mode" perhaps?
>
> > reason & MCSR_MEA ? "Effective" : "Physical", addr);
> > }
> >
> > + if ((user_mode(regs))) {
> > + _exception(SIGBUS, regs, reason, regs->nip);
> > + recoverable = 1;
> > + }
>
> For most of the error causes we take no action and set recoverable = 0.
>
> Then you just declare that it is recoverable because it hit in
> userspace. Depending on the cause that might be OK, but it's not
> obviously correct in all cases.
Not so familiar with PPC that I can make out what is OK or not.
I do think you stand a better chance now that before though.
>
>
> > +
> > silent_out:
> > mtspr(SPRN_MCSR, mcsr);
> > return mfspr(SPRN_MCSR) == 0 && recoverable;
> > @@ -665,6 +670,10 @@ int machine_check_e500(struct pt_regs *regs)
>
> Same comment about the printk().
>
> > if (reason & MCSR_BUS_RPERR)
> > printk("Bus - Read Parity Error\n");
> >
> > + if ((user_mode(regs))) {
> > + _exception(SIGBUS, regs, reason, regs->nip);
> > + return 1;
> > + }
>
> And same comment more or less.
>
> Other than the MCSR_BUS_RBERR cases that are explicitly checked, the
> function does nothing to clear the cause of the machine check.
>
> > return 0;
> > }
> >
> > @@ -695,6 +704,10 @@ int machine_check_e200(struct pt_regs *regs)
> > if (reason & MCSR_BUS_WRERR)
> > printk("Bus - Write Bus Error on buffered store or cache line
> > push\n");
> >
> > + if ((user_mode(regs))) {
> > + _exception(SIGBUS, regs, reason, regs->nip);
> > + return 1;
> > + }
>
> Same.
>
> > return 0;
> > }
> > #elif defined(CONFIG_PPC32)
> > @@ -731,6 +744,10 @@ int machine_check_generic(struct pt_regs *regs)
> > default:
> > printk("Unknown values in msr\n");
> > }
> > + if ((user_mode(regs))) {
> > + _exception(SIGBUS, regs, reason, regs->nip);
> > + return 1;
> > + }
>
> Same.
>
> > return 0;
> > }
> > #endif /* everything else */
> > --
> > 2.26.2
>
>
> cheers
Re: [PATCH] treewide: Convert macro and uses of __section(foo) to __section("foo")
On Fri, 2020-10-23 at 08:08 +0200, Miguel Ojeda wrote:
> On Thu, Oct 22, 2020 at 4:36 AM Joe Perches wrote:
> >
> > Use a more generic form for __section that requires quotes to avoid
> > complications with clang and gcc differences.
>
> I performed visual inspection (one by one...) and the only thing I saw
> is that sometimes the `__attribute__` has a whitespace afterwards and
> sometimes it doesn't, same for the commas inside, e.g.:
>
> - __used __attribute__((section(".modinfo"), unused, aligned(1))) \
> + __used __section(".modinfo") __attribute__((unused, aligned(1))) \
>
> and:
>
> -__attribute__ ((unused,__section__ ("__param"),aligned(sizeof(void *
> \
> +__section("__param") __attribute__ ((unused, aligned(sizeof(void * \
>
> I think the patch tries to follow the style of the replaced line, but
> for the commas in this last case it didn't. Anyway, it is not
> important.
Here the change follows the kernel style of space after comma.
> I can pick it up in my queue along with the __alias one and keep it
> for a few weeks in -next.
Thanks Miguel, but IMO it doesn't need time in next.
Applying it just before an rc1 minimizes conflicts.
