[PATCH 0/4] bitmap: fix conversion from/to fix-sized arrays

[PATCH 0/4] bitmap: fix conversion from/to fix-sized arrays

[Yury Norov]

In the kernel codebase we have functions that call bitmap_copy()
to convert bitmaps to and from fix-sized 32 or 64-bit arrays. It
works well for LE architectures when size of long is equal to the
size of fixed type.

If the system is BE and/or size of long is not equal to the size of
fixed type of the array, bitmap_copy() may produce wrong result either
because of endianness issue, or because of out-of-bound access.

To address this problem we already have bitmap_{from,to}_arr32().
In recent discussion it was spotted that we also need 64-bit
analogue of it:
https://lore.kernel.org/all/YiCWNdWd+AsLbDkp@smile.fi.intel.com/T/#m754da92acb0003e12b99293d07ddcd46dbe04ada

This series takes care of it.

Yury Norov (4):
  lib/bitmap: extend comment for bitmap_(from,to)_arr32()
  lib: add bitmap_{from,to}_arr64
  KVM: s390: replace bitmap_copy with bitmap_{from,to}_arr64 where
    appropriate
  drm/amd/pm: use bitmap_{from,to}_arr32 where appropriate

 arch/s390/kvm/kvm-s390.c                      | 10 ++--
 .../gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c    |  2 +-
 .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c    |  2 +-
 include/linux/bitmap.h                        | 31 +++++++++---
 lib/bitmap.c                                  | 47 +++++++++++++++++++
 5 files changed, 77 insertions(+), 15 deletions(-)

-- 
2.32.0

[PATCH 1/4] lib/bitmap: extend comment for bitmap_(from,to)_arr32()

[Yury Norov]

On LE systems bitmaps are naturally ordered, therefore we can potentially
use bitmap_copy routines when converting from 32-bit arrays, even if host
system is 64-bit. But it may lead to out-of-bond access due to unsafe
typecast, and the bitmap_(from,to)_arr32 comment doesn't explain that
clearly.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 include/linux/bitmap.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index a89b626d0fbe..10d805c2893c 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -271,8 +271,12 @@ static inline void bitmap_copy_clear_tail(unsigned long *dst,
 }
 
 /*
- * On 32-bit systems bitmaps are represented as u32 arrays internally, and
- * therefore conversion is not needed when copying data from/to arrays of u32.
+ * On 32-bit systems bitmaps are represented as u32 arrays internally. On LE64
+ * machines the order of hi and lo parts of nubmers match the bitmap structure.
+ * In both cases conversion is not needed when copying data from/to arrays of
+ * u32. But in LE64 case, typecast in bitmap_copy_clear_tail() may lead to the
+ * out-of-bound access. To avoid that, both LE and BE variants of 64-bit
+ * architectures are not using bitmap_copy_clear_tail().
  */
 #if BITS_PER_LONG == 64
 void bitmap_from_arr32(unsigned long *bitmap, const u32 *buf,
-- 
2.32.0

[PATCH 2/4] lib: add bitmap_{from,to}_arr64

[Yury Norov]

Manipulating 64-bit arrays with bitmap functions is potentially dangerous
because on 32-bit BE machines the order of halfwords doesn't match. Another
issue is that compiler may throw a warning about out-of-boundary access.

This patch adds bitmap_{from,to}_arr64 functions in addition to existing
bitmap_{from,to}_arr32.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 include/linux/bitmap.h | 23 +++++++++++++++++----
 lib/bitmap.c           | 47 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 10d805c2893c..f78c534fb814 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -292,6 +292,24 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
 			(const unsigned long *) (bitmap), (nbits))
 #endif
 
+/*
+ * On 64-bit systems bitmaps are represented as u64 arrays internally. On LE32
+ * machines the order of hi and lo parts of nubmers match the bitmap structure.
+ * In both cases conversion is not needed when copying data from/to arrays of
+ * u64.
+ */
+#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
+void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits);
+void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits);
+#else
+#define bitmap_from_arr64(bitmap, buf, nbits)			\
+	bitmap_copy_clear_tail((unsigned long *) (bitmap),	\
+			(const unsigned long *) (buf), (nbits))
+#define bitmap_to_arr64(buf, bitmap, nbits)			\
+	bitmap_copy_clear_tail((unsigned long *) (buf),		\
+			(const unsigned long *) (bitmap), (nbits))
+#endif
+
 static inline int bitmap_and(unsigned long *dst, const unsigned long *src1,
 			const unsigned long *src2, unsigned int nbits)
 {
@@ -596,10 +614,7 @@ static inline void bitmap_next_set_region(unsigned long *bitmap,
  */
 static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
 {
-	dst[0] = mask & ULONG_MAX;
-
-	if (sizeof(mask) > sizeof(unsigned long))
-		dst[1] = mask >> 32;
+	bitmap_from_arr64(dst, &mask, 64);
 }
 
 /**
diff --git a/lib/bitmap.c b/lib/bitmap.c
index d9a4480af5b9..aea9493f4216 100644
--- a/lib/bitmap.c
+++ b/lib/bitmap.c
@@ -1533,5 +1533,52 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits)
 		buf[halfwords - 1] &= (u32) (UINT_MAX >> ((-nbits) & 31));
 }
 EXPORT_SYMBOL(bitmap_to_arr32);
+#endif
+
+#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
+/**
+ * bitmap_from_arr64 - copy the contents of u64 array of bits to bitmap
+ *	@bitmap: array of unsigned longs, the destination bitmap
+ *	@buf: array of u64 (in host byte order), the source bitmap
+ *	@nbits: number of bits in @bitmap
+ */
+void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits)
+{
+	while (nbits > 0) {
+		u64 val = *buf++;
+
+		*bitmap++ = (unsigned long)val;
+		if (nbits > 32)
+			*bitmap++ = (unsigned long)(val >> 32);
+		nbits -= 64;
+	}
 
+	/* Clear tail bits in last word beyond nbits. */
+	if (nbits % BITS_PER_LONG)
+		bitmap[-1] &= BITMAP_LAST_WORD_MASK(nbits);
+}
+EXPORT_SYMBOL(bitmap_from_arr64);
+
+/**
+ * bitmap_to_arr64 - copy the contents of bitmap to a u64 array of bits
+ *	@buf: array of u64 (in host byte order), the dest bitmap
+ *	@bitmap: array of unsigned longs, the source bitmap
+ *	@nbits: number of bits in @bitmap
+ */
+void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits)
+{
+	unsigned long *end = bitmap + BITS_TO_LONGS(nbits);
+
+	while (bitmap < end) {
+		*buf = *bitmap++;
+		if (bitmap < end)
+			*buf |= *bitmap++ << 32;
+		buf++;
+	}
+
+	/* Clear tail bits in last element of array beyond nbits. */
+	if (nbits % 64)
+		buf[-1] &= GENMASK_ULL(nbits, 0);
+}
+EXPORT_SYMBOL(bitmap_to_arr64);
 #endif
-- 
2.32.0

RE: [PATCH 2/4] lib: add bitmap_{from,to}_arr64

[David Laight]

From: Yury Norov
> Sent: 20 April 2022 23:25
> 
> Manipulating 64-bit arrays with bitmap functions is potentially dangerous
> because on 32-bit BE machines the order of halfwords doesn't match. Another
> issue is that compiler may throw a warning about out-of-boundary access.
> 
> This patch adds bitmap_{from,to}_arr64 functions in addition to existing
> bitmap_{from,to}_arr32.
> 
> Signed-off-by: Yury Norov <yury.norov@gmail.com>
> ---
>  include/linux/bitmap.h | 23 +++++++++++++++++----
>  lib/bitmap.c           | 47 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 10d805c2893c..f78c534fb814 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -292,6 +292,24 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
>  			(const unsigned long *) (bitmap), (nbits))
>  #endif
> 
> +/*
> + * On 64-bit systems bitmaps are represented as u64 arrays internally. On LE32
> + * machines the order of hi and lo parts of nubmers match the bitmap structure.
> + * In both cases conversion is not needed when copying data from/to arrays of
> + * u64.
> + */
> +#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)

I think I'd change the condition to (inverting it):
#if (BITS_PER_LONG == 64) || defined(__LITTLE_ENDIAN)
since that is the condition when the layout matches.

> +void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits);
> +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits);
> +#else
> +#define bitmap_from_arr64(bitmap, buf, nbits)			\
> +	bitmap_copy_clear_tail((unsigned long *) (bitmap),	\
> +			(const unsigned long *) (buf), (nbits))
> +#define bitmap_to_arr64(buf, bitmap, nbits)			\
> +	bitmap_copy_clear_tail((unsigned long *) (buf),		\
> +			(const unsigned long *) (bitmap), (nbits))
> +#endif
> +
>  static inline int bitmap_and(unsigned long *dst, const unsigned long *src1,
>  			const unsigned long *src2, unsigned int nbits)
>  {
> @@ -596,10 +614,7 @@ static inline void bitmap_next_set_region(unsigned long *bitmap,
>   */
>  static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
>  {
> -	dst[0] = mask & ULONG_MAX;
> -
> -	if (sizeof(mask) > sizeof(unsigned long))
> -		dst[1] = mask >> 32;
> +	bitmap_from_arr64(dst, &mask, 64);
>  }

I'd leave this alone.

> 
>  /**
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index d9a4480af5b9..aea9493f4216 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -1533,5 +1533,52 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits)
>  		buf[halfwords - 1] &= (u32) (UINT_MAX >> ((-nbits) & 31));
>  }
>  EXPORT_SYMBOL(bitmap_to_arr32);
> +#endif
> +
> +#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
> +/**
> + * bitmap_from_arr64 - copy the contents of u64 array of bits to bitmap
> + *	@bitmap: array of unsigned longs, the destination bitmap
> + *	@buf: array of u64 (in host byte order), the source bitmap
> + *	@nbits: number of bits in @bitmap
> + */
> +void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits)
> +{
> +	while (nbits > 0) {

This looks like a for look to me...

> +		u64 val = *buf++;
> +
> +		*bitmap++ = (unsigned long)val;
> +		if (nbits > 32)
> +			*bitmap++ = (unsigned long)(val >> 32);

No need for either cast.

> +		nbits -= 64;
> +	}
> 
> +	/* Clear tail bits in last word beyond nbits. */
> +	if (nbits % BITS_PER_LONG)
> +		bitmap[-1] &= BITMAP_LAST_WORD_MASK(nbits);
> +}
> +EXPORT_SYMBOL(bitmap_from_arr64);
> +
> +/**
> + * bitmap_to_arr64 - copy the contents of bitmap to a u64 array of bits
> + *	@buf: array of u64 (in host byte order), the dest bitmap
> + *	@bitmap: array of unsigned longs, the source bitmap
> + *	@nbits: number of bits in @bitmap
> + */
> +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits)
> +{
> +	unsigned long *end = bitmap + BITS_TO_LONGS(nbits);
> +
> +	while (bitmap < end) {

Another for loop...

> +		*buf = *bitmap++;
> +		if (bitmap < end)
> +			*buf |= *bitmap++ << 32;

That is UB.
Did you even compile this??

	David

> +		buf++;
> +	}
> +
> +	/* Clear tail bits in last element of array beyond nbits. */
> +	if (nbits % 64)
> +		buf[-1] &= GENMASK_ULL(nbits, 0);
> +}
> +EXPORT_SYMBOL(bitmap_to_arr64);
>  #endif
> --
> 2.32.0

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 2/4] lib: add bitmap_{from,to}_arr64

[Yury Norov]

On Thu, Apr 21, 2022 at 07:40:25AM +0000, David Laight wrote:
> From: Yury Norov
> > Sent: 20 April 2022 23:25
> > 
> > Manipulating 64-bit arrays with bitmap functions is potentially dangerous
> > because on 32-bit BE machines the order of halfwords doesn't match. Another
> > issue is that compiler may throw a warning about out-of-boundary access.
> > 
> > This patch adds bitmap_{from,to}_arr64 functions in addition to existing
> > bitmap_{from,to}_arr32.
> > 
> > Signed-off-by: Yury Norov <yury.norov@gmail.com>
> > ---
> >  include/linux/bitmap.h | 23 +++++++++++++++++----
> >  lib/bitmap.c           | 47 ++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > index 10d805c2893c..f78c534fb814 100644
> > --- a/include/linux/bitmap.h
> > +++ b/include/linux/bitmap.h
> > @@ -292,6 +292,24 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap,
> >  			(const unsigned long *) (bitmap), (nbits))
> >  #endif
> > 
> > +/*
> > + * On 64-bit systems bitmaps are represented as u64 arrays internally. On LE32
> > + * machines the order of hi and lo parts of nubmers match the bitmap structure.
> > + * In both cases conversion is not needed when copying data from/to arrays of
> > + * u64.
> > + */
> > +#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
> 
> I think I'd change the condition to (inverting it):
> #if (BITS_PER_LONG == 64) || defined(__LITTLE_ENDIAN)
> since that is the condition when the layout matches.

Sorry, I don't understand about 'layout matches'. Why this way is
better than another?
 
> > +void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits);
> > +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits);
> > +#else
> > +#define bitmap_from_arr64(bitmap, buf, nbits)			\
> > +	bitmap_copy_clear_tail((unsigned long *) (bitmap),	\
> > +			(const unsigned long *) (buf), (nbits))
> > +#define bitmap_to_arr64(buf, bitmap, nbits)			\
> > +	bitmap_copy_clear_tail((unsigned long *) (buf),		\
> > +			(const unsigned long *) (bitmap), (nbits))
> > +#endif
> > +
> >  static inline int bitmap_and(unsigned long *dst, const unsigned long *src1,
> >  			const unsigned long *src2, unsigned int nbits)
> >  {
> > @@ -596,10 +614,7 @@ static inline void bitmap_next_set_region(unsigned long *bitmap,
> >   */
> >  static inline void bitmap_from_u64(unsigned long *dst, u64 mask)
> >  {
> > -	dst[0] = mask & ULONG_MAX;
> > -
> > -	if (sizeof(mask) > sizeof(unsigned long))
> > -		dst[1] = mask >> 32;
> > +	bitmap_from_arr64(dst, &mask, 64);
> >  }
> 
> I'd leave this alone.
 
I'd change it in sake of consistency. Let's see what others say.

> >  /**
> > diff --git a/lib/bitmap.c b/lib/bitmap.c
> > index d9a4480af5b9..aea9493f4216 100644
> > --- a/lib/bitmap.c
> > +++ b/lib/bitmap.c
> > @@ -1533,5 +1533,52 @@ void bitmap_to_arr32(u32 *buf, const unsigned long *bitmap, unsigned int nbits)
> >  		buf[halfwords - 1] &= (u32) (UINT_MAX >> ((-nbits) & 31));
> >  }
> >  EXPORT_SYMBOL(bitmap_to_arr32);
> > +#endif
> > +
> > +#if (BITS_PER_LONG == 32) && defined(__BIG_ENDIAN)
> > +/**
> > + * bitmap_from_arr64 - copy the contents of u64 array of bits to bitmap
> > + *	@bitmap: array of unsigned longs, the destination bitmap
> > + *	@buf: array of u64 (in host byte order), the source bitmap
> > + *	@nbits: number of bits in @bitmap
> > + */
> > +void bitmap_from_arr64(unsigned long *bitmap, const u64 *buf, unsigned int nbits)
> > +{
> > +	while (nbits > 0) {
> 
> This looks like a for look to me...

Can you explain why for() is better then while() here? Is generated
code better, or something else?

> > +		u64 val = *buf++;
> > +
> > +		*bitmap++ = (unsigned long)val;
> > +		if (nbits > 32)
> > +			*bitmap++ = (unsigned long)(val >> 32);
> 
> No need for either cast.

Yep, thanks.

> > +		nbits -= 64;
> > +	}
> > 
> > +	/* Clear tail bits in last word beyond nbits. */
> > +	if (nbits % BITS_PER_LONG)
> > +		bitmap[-1] &= BITMAP_LAST_WORD_MASK(nbits);
> > +}
> > +EXPORT_SYMBOL(bitmap_from_arr64);
> > +
> > +/**
> > + * bitmap_to_arr64 - copy the contents of bitmap to a u64 array of bits
> > + *	@buf: array of u64 (in host byte order), the dest bitmap
> > + *	@bitmap: array of unsigned longs, the source bitmap
> > + *	@nbits: number of bits in @bitmap
> > + */
> > +void bitmap_to_arr64(u64 *buf, const unsigned long *bitmap, unsigned int nbits)
> > +{
> > +	unsigned long *end = bitmap + BITS_TO_LONGS(nbits);

This should be const unsigned long.

> > +
> > +	while (bitmap < end) {
> 
> Another for loop...
> 
> > +		*buf = *bitmap++;
> > +		if (bitmap < end)
> > +			*buf |= *bitmap++ << 32;
> 
> That is UB.

It's -Wshift-count-overflow. Should be
        *buf |= (u64)(*bitmap++) << 32;

> Did you even compile this??

Yes. That's it, my BE32 platform is arm, and I had to disable CONFIG_WERROR
because arm breaks build with that, and forgot about it. :(

I boot-tested it on mips with the fix above with no issues.
 
> 	David
> 
> > +		buf++;
> > +	}
> > +
> > +	/* Clear tail bits in last element of array beyond nbits. */
> > +	if (nbits % 64)
> > +		buf[-1] &= GENMASK_ULL(nbits, 0);
> > +}
> > +EXPORT_SYMBOL(bitmap_to_arr64);
> >  #endif
> > --
> > 2.32.0
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)

[PATCH 3/4] KVM: s390: replace bitmap_copy with bitmap_{from,to}_arr64 where appropriate

[Yury Norov]

Copying bitmaps from/to 64-bit arrays with bitmap_copy is not safe
in general case. Use designated functions instead.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 arch/s390/kvm/kvm-s390.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 156d1c25a3c1..a353bb43ee48 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1332,8 +1332,7 @@ static int kvm_s390_set_processor_feat(struct kvm *kvm,
 		mutex_unlock(&kvm->lock);
 		return -EBUSY;
 	}
-	bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat,
-		    KVM_S390_VM_CPU_FEAT_NR_BITS);
+	bitmap_from_arr64(kvm->arch.cpu_feat, data.feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
 	mutex_unlock(&kvm->lock);
 	VM_EVENT(kvm, 3, "SET: guest feat: 0x%16.16llx.0x%16.16llx.0x%16.16llx",
 			 data.feat[0],
@@ -1504,8 +1503,7 @@ static int kvm_s390_get_processor_feat(struct kvm *kvm,
 {
 	struct kvm_s390_vm_cpu_feat data;
 
-	bitmap_copy((unsigned long *) data.feat, kvm->arch.cpu_feat,
-		    KVM_S390_VM_CPU_FEAT_NR_BITS);
+	bitmap_to_arr64(data.feat, kvm->arch.cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
 	if (copy_to_user((void __user *)attr->addr, &data, sizeof(data)))
 		return -EFAULT;
 	VM_EVENT(kvm, 3, "GET: guest feat: 0x%16.16llx.0x%16.16llx.0x%16.16llx",
@@ -1520,9 +1518,7 @@ static int kvm_s390_get_machine_feat(struct kvm *kvm,
 {
 	struct kvm_s390_vm_cpu_feat data;
 
-	bitmap_copy((unsigned long *) data.feat,
-		    kvm_s390_available_cpu_feat,
-		    KVM_S390_VM_CPU_FEAT_NR_BITS);
+	bitmap_to_arr64(data.feat, kvm_s390_available_cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS);
 	if (copy_to_user((void __user *)attr->addr, &data, sizeof(data)))
 		return -EFAULT;
 	VM_EVENT(kvm, 3, "GET: host feat:  0x%16.16llx.0x%16.16llx.0x%16.16llx",
-- 
2.32.0

Re: [PATCH 3/4] KVM: s390: replace bitmap_copy with bitmap_{from,to}_arr64 where appropriate

[David Hildenbrand]

On 21.04.22 00:25, Yury Norov wrote:
> Copying bitmaps from/to 64-bit arrays with bitmap_copy is not safe
> in general case. Use designated functions instead.
> 

Just so I understand correctly: there is no BUG, it's just cleaner to do
it that way, correct?

IIUC, bitmap_to_arr64() translates to bitmap_copy_clear_tail() on s390x.

As the passed length is always 1024 (KVM_S390_VM_CPU_FEAT_NR_BITS), we
essentially end up with bitmap_copy() again.


Looks cleaner to me

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

Re: [PATCH 3/4] KVM: s390: replace bitmap_copy with bitmap_{from,to}_arr64 where appropriate

[Yury Norov]

On Thu, Apr 21, 2022 at 09:24:20AM +0200, David Hildenbrand wrote:
> On 21.04.22 00:25, Yury Norov wrote:
> > Copying bitmaps from/to 64-bit arrays with bitmap_copy is not safe
> > in general case. Use designated functions instead.
> > 
> 
> Just so I understand correctly: there is no BUG, it's just cleaner to do
> it that way, correct?

Yes. there's no bug, but the pattern is considered bad.

https://lore.kernel.org/all/YiCWNdWd+AsLbDkp@smile.fi.intel.com/T/#m9080cbb8a8235d7d4b7e38292cee8e4903f9afe4q
 
> IIUC, bitmap_to_arr64() translates to bitmap_copy_clear_tail() on s390x.

Yes.
 
> As the passed length is always 1024 (KVM_S390_VM_CPU_FEAT_NR_BITS), we
> essentially end up with bitmap_copy() again.
> 
> 
> Looks cleaner to me

Thanks.

> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> 
> -- 
> Thanks,
> 
> David / dhildenb

[PATCH 4/4] drm/amd/pm: use bitmap_{from,to}_arr32 where appropriate

[Yury Norov]

The smu_v1X_0_set_allowed_mask() uses bitmap_copy() to convert
bitmap to 32-bit array. This may be wrong due to endianness issues.
Fix it by switching to bitmap_{from,to}_arr32.

Signed-off-by: Yury Norov <yury.norov@gmail.com>
---
 drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c | 2 +-
 drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
index b87f550af26b..5f8809f6990d 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu11/smu_v11_0.c
@@ -781,7 +781,7 @@ int smu_v11_0_set_allowed_mask(struct smu_context *smu)
 		goto failed;
 	}
 
-	bitmap_copy((unsigned long *)feature_mask, feature->allowed, 64);
+	bitmap_to_arr32(feature_mask, feature->allowed, 64);
 
 	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetAllowedFeaturesMaskHigh,
 					  feature_mask[1], NULL);
diff --git a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
index cf09e30bdfe0..747430ce6394 100644
--- a/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
+++ b/drivers/gpu/drm/amd/pm/swsmu/smu13/smu_v13_0.c
@@ -730,7 +730,7 @@ int smu_v13_0_set_allowed_mask(struct smu_context *smu)
 	    feature->feature_num < 64)
 		return -EINVAL;
 
-	bitmap_copy((unsigned long *)feature_mask, feature->allowed, 64);
+	bitmap_to_arr32(feature_mask, feature->allowed, 64);
 
 	ret = smu_cmn_send_smc_msg_with_param(smu, SMU_MSG_SetAllowedFeaturesMaskHigh,
 					      feature_mask[1], NULL);
-- 
2.32.0