From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753912AbcGDWln (ORCPT ); Mon, 4 Jul 2016 18:41:43 -0400 Received: from mail.kernel.org ([198.145.29.136]:35564 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753490AbcGDWlm (ORCPT ); Mon, 4 Jul 2016 18:41:42 -0400 Date: Mon, 4 Jul 2016 19:41:36 -0300 From: Arnaldo Carvalho de Melo To: Yury Norov Cc: Madhavan Srinivasan , linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Peter Zijlstra , Ingo Molnar , Alexander Shishkin , Jiri Olsa , Michael Ellerman Subject: Re: [PATCH] perf/core: Fix the mask in perf_output_sample_regs Message-ID: <20160704224136.GY5324@kernel.org> References: <1467568918-24377-1-git-send-email-maddy@linux.vnet.ibm.com> <20160704071906.GA9901@yury-N73SV> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160704071906.GA9901@yury-N73SV> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Mon, Jul 04, 2016 at 10:19:06AM +0300, Yury Norov escreveu: > On Sun, Jul 03, 2016 at 11:31:58PM +0530, Madhavan Srinivasan wrote: > > When decoding the perf_regs mask in perf_output_sample_regs(), > > we loop through the mask using find_first_bit and find_next_bit functions. > > While the exisitng code works fine in most of the case, > > the logic is broken for 32bit kernel (Big Endian). > > When reading u64 mask using (u32 *)(&val)[0], find_*_bit() assumes it gets > > lower 32bits of u64 but instead gets upper 32bits which is wrong. > > Proposed fix is to swap the words of the u64 to handle this case. > > This is _not_ endianness swap. > > > In fact, it's broken for 32-bit LE as well if mask is zero and next > word on stack is not zero. The rest is OK. > > Reviewed-by: Yury Norov Waiting a bit for more acks, as IIRC several other people had comments on this patch, ok? - Arnaldo > > > > Suggested-by: Yury Norov > > Cc: Yury Norov > > Cc: Peter Zijlstra > > Cc: Ingo Molnar > > Cc: Arnaldo Carvalho de Melo > > Cc: Alexander Shishkin > > Cc: Jiri Olsa > > Cc: Michael Ellerman > > Signed-off-by: Madhavan Srinivasan > > --- > > include/linux/bitmap.h | 2 ++ > > kernel/events/core.c | 4 +++- > > lib/bitmap.c | 19 +++++++++++++++++++ > > 3 files changed, 24 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h > > index e9b0b9ab07e5..d95b422db183 100644 > > --- a/include/linux/bitmap.h > > +++ b/include/linux/bitmap.h > > @@ -188,6 +188,8 @@ extern int bitmap_print_to_pagebuf(bool list, char *buf, > > #define small_const_nbits(nbits) \ > > (__builtin_constant_p(nbits) && (nbits) <= BITS_PER_LONG) > > > > +extern void bitmap_from_u64(unsigned long *dst, u64 mask); > > + > > static inline void bitmap_zero(unsigned long *dst, unsigned int nbits) > > { > > if (small_const_nbits(nbits)) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > index 9c51ec3f0f44..613fec95ea4c 100644 > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -5205,8 +5205,10 @@ perf_output_sample_regs(struct perf_output_handle *handle, > > struct pt_regs *regs, u64 mask) > > { > > int bit; > > + DECLARE_BITMAP(_mask, 64); > > > > - for_each_set_bit(bit, (const unsigned long *) &mask, > > + bitmap_from_u64(_mask, mask); > > + for_each_set_bit(bit, _mask, > > sizeof(mask) * BITS_PER_BYTE) { > > u64 val; > > > > diff --git a/lib/bitmap.c b/lib/bitmap.c > > index c66da508cbf7..522f1b4c6078 100644 > > --- a/lib/bitmap.c > > +++ b/lib/bitmap.c > > @@ -1170,3 +1170,22 @@ void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n > > } > > EXPORT_SYMBOL(bitmap_copy_le); > > #endif > > + > > +/* > > + * bitmap_from_u64 - Check and swap words within u64. > > + * @mask: source bitmap > > + * @dst: destination bitmap > > + * > > + * In 32bit Big Endian kernel, when using (u32 *)(&val)[*] > > + * to read u64 mask, we will get wrong word. > > + * That is "(u32 *)(&val)[0]" gets upper 32 bits, > > + * but expected could be lower 32bits of u64. > > + */ > > +void bitmap_from_u64(unsigned long *dst, u64 mask) > > +{ > > + dst[0] = mask & ULONG_MAX; > > + > > + if (sizeof(mask) > sizeof(unsigned long)) > > + dst[1] = mask >> 32; > > +} > > +EXPORT_SYMBOL(bitmap_from_u64); > > -- > > 1.9.1