All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
@ 2022-02-23 16:44 Andy Shevchenko
  2022-02-24 11:36 ` Claudio Imbrenda
  2022-03-02 15:45 ` Andy Shevchenko
  0 siblings, 2 replies; 11+ messages in thread
From: Andy Shevchenko @ 2022-02-23 16:44 UTC (permalink / raw)
  To: Christian Borntraeger, kvm, linux-s390, linux-kernel
  Cc: Janosch Frank, Claudio Imbrenda, David Hildenbrand,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Sven Schnelle,
	Andy Shevchenko

While in this particular case it would not be a (critical) issue,
the pattern itself is bad and error prone in case somebody blindly
copies to their code.

Don't cast parameter to unsigned long pointer in the bit operations.
Instead copy to a local variable on stack of a proper type and use.

Fixes: d77e64141e32 ("KVM: s390: implement GISA IPM related primitives")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/s390/include/asm/kvm_host.h | 5 ++++-
 arch/s390/kvm/interrupt.c        | 6 +++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index a22c9266ea05..f1c4a1b9b360 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -867,7 +867,10 @@ struct kvm_s390_gisa {
 			u8  reserved03[11];
 			u32 airq_count;
 		} g1;
-		struct {
+		struct { /* as a 256-bit bitmap */
+			DECLARE_BITMAP(b, 256);
+		} bitmap;
+		struct { /* as a set of 64-bit words */
 			u64 word[4];
 		} u64;
 	};
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index db933c252dbc..04e055cbd080 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -304,7 +304,7 @@ static inline int gisa_in_alert_list(struct kvm_s390_gisa *gisa)
 
 static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
 {
-	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
+	set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
 }
 
 static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa)
@@ -314,12 +314,12 @@ static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa)
 
 static inline void gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
 {
-	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
+	clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
 }
 
 static inline int gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
 {
-	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
+	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
 }
 
 static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
  2022-02-23 16:44 [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations Andy Shevchenko
@ 2022-02-24 11:36 ` Claudio Imbrenda
  2022-02-24 12:10   ` Michael Mueller
  2022-02-24 19:51   ` Andy Shevchenko
  2022-03-02 15:45 ` Andy Shevchenko
  1 sibling, 2 replies; 11+ messages in thread
From: Claudio Imbrenda @ 2022-02-24 11:36 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Christian Borntraeger, kvm, linux-s390, linux-kernel,
	Janosch Frank, David Hildenbrand, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Sven Schnelle

On Wed, 23 Feb 2022 18:44:20 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> While in this particular case it would not be a (critical) issue,
> the pattern itself is bad and error prone in case somebody blindly
> copies to their code.
> 
> Don't cast parameter to unsigned long pointer in the bit operations.
> Instead copy to a local variable on stack of a proper type and use.
> 
> Fixes: d77e64141e32 ("KVM: s390: implement GISA IPM related primitives")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/s390/include/asm/kvm_host.h | 5 ++++-
>  arch/s390/kvm/interrupt.c        | 6 +++---
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index a22c9266ea05..f1c4a1b9b360 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -867,7 +867,10 @@ struct kvm_s390_gisa {
>  			u8  reserved03[11];
>  			u32 airq_count;
>  		} g1;
> -		struct {
> +		struct { /* as a 256-bit bitmap */
> +			DECLARE_BITMAP(b, 256);
> +		} bitmap;
> +		struct { /* as a set of 64-bit words */
>  			u64 word[4];
>  		} u64;
>  	};
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index db933c252dbc..04e055cbd080 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -304,7 +304,7 @@ static inline int gisa_in_alert_list(struct kvm_s390_gisa *gisa)
>  
>  static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>  {
> -	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> +	set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);

wouldn't it be enough to pass gisa->u64.word here?
then no cast would be necessary

>  }
>  
>  static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa)
> @@ -314,12 +314,12 @@ static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa)
>  
>  static inline void gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>  {
> -	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> +	clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
>  }
>  
>  static inline int gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>  {
> -	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> +	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
>  }
>  
>  static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
  2022-02-24 11:36 ` Claudio Imbrenda
@ 2022-02-24 12:10   ` Michael Mueller
  2022-03-02 15:44     ` Andy Shevchenko
  2022-02-24 19:51   ` Andy Shevchenko
  1 sibling, 1 reply; 11+ messages in thread
From: Michael Mueller @ 2022-02-24 12:10 UTC (permalink / raw)
  To: Claudio Imbrenda, Andy Shevchenko
  Cc: Christian Borntraeger, kvm, linux-s390, linux-kernel,
	Janosch Frank, David Hildenbrand, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Sven Schnelle



On 24.02.22 12:36, Claudio Imbrenda wrote:
> On Wed, 23 Feb 2022 18:44:20 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
>> While in this particular case it would not be a (critical) issue,
>> the pattern itself is bad and error prone in case somebody blindly
>> copies to their code.
>>
>> Don't cast parameter to unsigned long pointer in the bit operations.
>> Instead copy to a local variable on stack of a proper type and use.
>>
>> Fixes: d77e64141e32 ("KVM: s390: implement GISA IPM related primitives")
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>   arch/s390/include/asm/kvm_host.h | 5 ++++-
>>   arch/s390/kvm/interrupt.c        | 6 +++---
>>   2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
>> index a22c9266ea05..f1c4a1b9b360 100644
>> --- a/arch/s390/include/asm/kvm_host.h
>> +++ b/arch/s390/include/asm/kvm_host.h
>> @@ -867,7 +867,10 @@ struct kvm_s390_gisa {
>>   			u8  reserved03[11];
>>   			u32 airq_count;
>>   		} g1;
>> -		struct {
>> +		struct { /* as a 256-bit bitmap */
>> +			DECLARE_BITMAP(b, 256);
>> +		} bitmap;
>> +		struct { /* as a set of 64-bit words */
>>   			u64 word[4];
>>   		} u64;
>>   	};
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index db933c252dbc..04e055cbd080 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -304,7 +304,7 @@ static inline int gisa_in_alert_list(struct kvm_s390_gisa *gisa)
>>   
>>   static inline void gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>>   {
>> -	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>> +	set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
> 
> wouldn't it be enough to pass gisa->u64.word here?
> then no cast would be necessary


we do that at several places

arch/s390/kernel/processor.c:	for_each_set_bit_inv(bit, (long 
*)&stfle_fac_list, MAX_FACILITY_BIT)
arch/s390/kvm/interrupt.c:	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned 
long *) gisa);
arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *) 
sca->mcn);
arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *) 
&sca->mcn);

> 
>>   }
>>   
>>   static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa)
>> @@ -314,12 +314,12 @@ static inline u8 gisa_get_ipm(struct kvm_s390_gisa *gisa)
>>   
>>   static inline void gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>>   {
>> -	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>> +	clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
>>   }
>>   
>>   static inline int gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>>   {
>> -	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>> +	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
>>   }
>>   
>>   static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
  2022-02-24 11:36 ` Claudio Imbrenda
  2022-02-24 12:10   ` Michael Mueller
@ 2022-02-24 19:51   ` Andy Shevchenko
  2022-02-24 23:15     ` David Laight
  1 sibling, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2022-02-24 19:51 UTC (permalink / raw)
  To: Claudio Imbrenda
  Cc: Andy Shevchenko, Christian Borntraeger, open list:VFIO DRIVER,
	linux-s390, Linux Kernel Mailing List, Janosch Frank,
	David Hildenbrand, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Sven Schnelle

On Thu, Feb 24, 2022 at 2:51 PM Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
>
> On Wed, 23 Feb 2022 18:44:20 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > While in this particular case it would not be a (critical) issue,
> > the pattern itself is bad and error prone in case somebody blindly
> > copies to their code.
> >
> > Don't cast parameter to unsigned long pointer in the bit operations.
> > Instead copy to a local variable on stack of a proper type and use.

...

> > +             struct { /* as a 256-bit bitmap */
> > +                     DECLARE_BITMAP(b, 256);
> > +             } bitmap;
> > +             struct { /* as a set of 64-bit words */
> >                       u64 word[4];
> >               } u64;

> > -     set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> > +     set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
>
> wouldn't it be enough to pass gisa->u64.word here?
> then no cast would be necessary

No, it will have the same hidden bugs. As I stated in the commit
message, the pattern is quite bad even if in particular code it would
work.

Thanks, Michael, for pointing out other places. They all need to be fixed.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
  2022-02-24 19:51   ` Andy Shevchenko
@ 2022-02-24 23:15     ` David Laight
  0 siblings, 0 replies; 11+ messages in thread
From: David Laight @ 2022-02-24 23:15 UTC (permalink / raw)
  To: 'Andy Shevchenko', Claudio Imbrenda
  Cc: Andy Shevchenko, Christian Borntraeger, open list:VFIO DRIVER,
	linux-s390, Linux Kernel Mailing List, Janosch Frank,
	David Hildenbrand, Heiko Carstens, Vasily Gorbik,
	Alexander Gordeev, Sven Schnelle

From: Andy Shevchenko
> Sent: 24 February 2022 19:51
> 
> On Thu, Feb 24, 2022 at 2:51 PM Claudio Imbrenda <imbrenda@linux.ibm.com> wrote:
> >
> > On Wed, 23 Feb 2022 18:44:20 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >
> > > While in this particular case it would not be a (critical) issue,
> > > the pattern itself is bad and error prone in case somebody blindly
> > > copies to their code.
> > >
> > > Don't cast parameter to unsigned long pointer in the bit operations.
> > > Instead copy to a local variable on stack of a proper type and use.
> 
> ...
> 
> > > +             struct { /* as a 256-bit bitmap */
> > > +                     DECLARE_BITMAP(b, 256);
> > > +             } bitmap;
> > > +             struct { /* as a set of 64-bit words */
> > >                       u64 word[4];
> > >               } u64;
> 
> > > -     set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> > > +     set_bit_inv(IPM_BIT_OFFSET + gisc, gisa->bitmap.b);
> >
> > wouldn't it be enough to pass gisa->u64.word here?
> > then no cast would be necessary
> 
> No, it will have the same hidden bugs. As I stated in the commit
> message, the pattern is quite bad even if in particular code it would
> work.
> 
> Thanks, Michael, for pointing out other places. They all need to be fixed.

It may even be worth writing some alternate bitmap functions
that use u64[] and unlocked operations?

Although I think I'd still want to encapsulate the actual array
(somehow) so that what is defined has to be the bitmap type.

	David

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
  2022-02-24 12:10   ` Michael Mueller
@ 2022-03-02 15:44     ` Andy Shevchenko
  2022-03-02 17:18       ` Yury Norov
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2022-03-02 15:44 UTC (permalink / raw)
  To: Michael Mueller, Yury Norov
  Cc: Claudio Imbrenda, Christian Borntraeger, kvm, linux-s390,
	linux-kernel, Janosch Frank, David Hildenbrand, Heiko Carstens,
	Vasily Gorbik, Alexander Gordeev, Sven Schnelle

On Thu, Feb 24, 2022 at 01:10:34PM +0100, Michael Mueller wrote:
> On 24.02.22 12:36, Claudio Imbrenda wrote:

...

> we do that at several places

Thanks for pointing out.

> arch/s390/kernel/processor.c:	for_each_set_bit_inv(bit, (long
> *)&stfle_fac_list, MAX_FACILITY_BIT)

This one requires a separate change, not related to this patch.

> arch/s390/kvm/interrupt.c:	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long
> *) gisa);

This is done in the patch. Not sure how it appears in your list.

> arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *)
> sca->mcn);
> arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *)
> &sca->mcn);

These two should be fixed in a separate change.

Also this kind of stuff:

	bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat,
	            KVM_S390_VM_CPU_FEAT_NR_BITS);

might require a new API like

bitmap_from_u64_array()
bitmap_to_u64_array()

Yury?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
  2022-02-23 16:44 [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations Andy Shevchenko
  2022-02-24 11:36 ` Claudio Imbrenda
@ 2022-03-02 15:45 ` Andy Shevchenko
  1 sibling, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2022-03-02 15:45 UTC (permalink / raw)
  To: Christian Borntraeger, kvm, linux-s390, linux-kernel
  Cc: Janosch Frank, Claudio Imbrenda, David Hildenbrand,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Sven Schnelle

On Wed, Feb 23, 2022 at 06:44:20PM +0200, Andy Shevchenko wrote:
> While in this particular case it would not be a (critical) issue,
> the pattern itself is bad and error prone in case somebody blindly
> copies to their code.
> 
> Don't cast parameter to unsigned long pointer in the bit operations.
> Instead copy to a local variable on stack of a proper type and use.

After looking into other similar cases I may conclude they
- need to be fixed
- out of scope of this change

Hence, can this fix be applied?

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
  2022-03-02 15:44     ` Andy Shevchenko
@ 2022-03-02 17:18       ` Yury Norov
  2022-03-02 17:31         ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Yury Norov @ 2022-03-02 17:18 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Michael Mueller, Claudio Imbrenda, Christian Borntraeger, kvm,
	linux-s390, linux-kernel, Janosch Frank, David Hildenbrand,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Sven Schnelle

On Wed, Mar 02, 2022 at 05:44:03PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 24, 2022 at 01:10:34PM +0100, Michael Mueller wrote:
> > On 24.02.22 12:36, Claudio Imbrenda wrote:
> 
> ...
> 
> > we do that at several places
> 
> Thanks for pointing out.
> 
> > arch/s390/kernel/processor.c:	for_each_set_bit_inv(bit, (long
> > *)&stfle_fac_list, MAX_FACILITY_BIT)
> 
> This one requires a separate change, not related to this patch.
> 
> > arch/s390/kvm/interrupt.c:	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long
> > *) gisa);
> 
> This is done in the patch. Not sure how it appears in your list.
> 
> > arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *)
> > sca->mcn);
> > arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *)
> > &sca->mcn);
> 
> These two should be fixed in a separate change.
> 
> Also this kind of stuff:
> 
> 	bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat,
> 	            KVM_S390_VM_CPU_FEAT_NR_BITS);
> 
> might require a new API like
> 
> bitmap_from_u64_array()
> bitmap_to_u64_array()
> 
> Yury?

If BE32 is still the case then yes.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
  2022-03-02 17:18       ` Yury Norov
@ 2022-03-02 17:31         ` Andy Shevchenko
  2022-03-02 18:43           ` Yury Norov
  0 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2022-03-02 17:31 UTC (permalink / raw)
  To: Yury Norov
  Cc: Michael Mueller, Claudio Imbrenda, Christian Borntraeger, kvm,
	linux-s390, linux-kernel, Janosch Frank, David Hildenbrand,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Sven Schnelle

On Wed, Mar 02, 2022 at 09:18:35AM -0800, Yury Norov wrote:
> On Wed, Mar 02, 2022 at 05:44:03PM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 24, 2022 at 01:10:34PM +0100, Michael Mueller wrote:
> > > On 24.02.22 12:36, Claudio Imbrenda wrote:
> > 
> > ...
> > 
> > > we do that at several places
> > 
> > Thanks for pointing out.
> > 
> > > arch/s390/kernel/processor.c:	for_each_set_bit_inv(bit, (long
> > > *)&stfle_fac_list, MAX_FACILITY_BIT)
> > 
> > This one requires a separate change, not related to this patch.
> > 
> > > arch/s390/kvm/interrupt.c:	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long
> > > *) gisa);
> > 
> > This is done in the patch. Not sure how it appears in your list.
> > 
> > > arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *)
> > > sca->mcn);
> > > arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *)
> > > &sca->mcn);
> > 
> > These two should be fixed in a separate change.
> > 
> > Also this kind of stuff:
> > 
> > 	bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat,
> > 	            KVM_S390_VM_CPU_FEAT_NR_BITS);
> > 
> > might require a new API like
> > 
> > bitmap_from_u64_array()
> > bitmap_to_u64_array()
> > 
> > Yury?
> 
> If BE32 is still the case then yes.

The whole point is to get rid of the bad pattern, while it may still work
in the particular case.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
  2022-03-02 17:31         ` Andy Shevchenko
@ 2022-03-02 18:43           ` Yury Norov
  2022-03-03 10:19             ` Andy Shevchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Yury Norov @ 2022-03-02 18:43 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Michael Mueller, Claudio Imbrenda, Christian Borntraeger, kvm,
	linux-s390, linux-kernel, Janosch Frank, David Hildenbrand,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Sven Schnelle

On Wed, Mar 02, 2022 at 07:31:58PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 02, 2022 at 09:18:35AM -0800, Yury Norov wrote:
> > On Wed, Mar 02, 2022 at 05:44:03PM +0200, Andy Shevchenko wrote:
> > > On Thu, Feb 24, 2022 at 01:10:34PM +0100, Michael Mueller wrote:
> > > > On 24.02.22 12:36, Claudio Imbrenda wrote:
> > > 
> > > ...
> > > 
> > > > we do that at several places
> > > 
> > > Thanks for pointing out.
> > > 
> > > > arch/s390/kernel/processor.c:	for_each_set_bit_inv(bit, (long
> > > > *)&stfle_fac_list, MAX_FACILITY_BIT)
> > > 
> > > This one requires a separate change, not related to this patch.
> > > 
> > > > arch/s390/kvm/interrupt.c:	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long
> > > > *) gisa);
> > > 
> > > This is done in the patch. Not sure how it appears in your list.
> > > 
> > > > arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *)
> > > > sca->mcn);
> > > > arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *)
> > > > &sca->mcn);
> > > 
> > > These two should be fixed in a separate change.
> > > 
> > > Also this kind of stuff:
> > > 
> > > 	bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat,
> > > 	            KVM_S390_VM_CPU_FEAT_NR_BITS);
> > > 
> > > might require a new API like
> > > 
> > > bitmap_from_u64_array()
> > > bitmap_to_u64_array()
> > > 
> > > Yury?
> > 
> > If BE32 is still the case then yes.
> 
> The whole point is to get rid of the bad pattern, while it may still work
> in the particular case.

Then yes unconditionally. Is it already on table of s390 folks? If no,
I can do it myself.

We have bitmap_from_arr32 and bitmap_to_arr32, so for 64-bit versions,
we'd start from that.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations
  2022-03-02 18:43           ` Yury Norov
@ 2022-03-03 10:19             ` Andy Shevchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2022-03-03 10:19 UTC (permalink / raw)
  To: Yury Norov
  Cc: Michael Mueller, Claudio Imbrenda, Christian Borntraeger, kvm,
	linux-s390, linux-kernel, Janosch Frank, David Hildenbrand,
	Heiko Carstens, Vasily Gorbik, Alexander Gordeev, Sven Schnelle

On Wed, Mar 02, 2022 at 10:43:54AM -0800, Yury Norov wrote:
> On Wed, Mar 02, 2022 at 07:31:58PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 02, 2022 at 09:18:35AM -0800, Yury Norov wrote:
> > > On Wed, Mar 02, 2022 at 05:44:03PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Feb 24, 2022 at 01:10:34PM +0100, Michael Mueller wrote:
> > > > > On 24.02.22 12:36, Claudio Imbrenda wrote:
> > > > 
> > > > ...
> > > > 
> > > > > we do that at several places
> > > > 
> > > > Thanks for pointing out.
> > > > 
> > > > > arch/s390/kernel/processor.c:	for_each_set_bit_inv(bit, (long
> > > > > *)&stfle_fac_list, MAX_FACILITY_BIT)
> > > > 
> > > > This one requires a separate change, not related to this patch.
> > > > 
> > > > > arch/s390/kvm/interrupt.c:	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long
> > > > > *) gisa);
> > > > 
> > > > This is done in the patch. Not sure how it appears in your list.
> > > > 
> > > > > arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *)
> > > > > sca->mcn);
> > > > > arch/s390/kvm/kvm-s390.c:		set_bit_inv(vcpu->vcpu_id, (unsigned long *)
> > > > > &sca->mcn);
> > > > 
> > > > These two should be fixed in a separate change.
> > > > 
> > > > Also this kind of stuff:
> > > > 
> > > > 	bitmap_copy(kvm->arch.cpu_feat, (unsigned long *) data.feat,
> > > > 	            KVM_S390_VM_CPU_FEAT_NR_BITS);
> > > > 
> > > > might require a new API like
> > > > 
> > > > bitmap_from_u64_array()
> > > > bitmap_to_u64_array()
> > > > 
> > > > Yury?
> > > 
> > > If BE32 is still the case then yes.
> > 
> > The whole point is to get rid of the bad pattern, while it may still work
> > in the particular case.
> 
> Then yes unconditionally. Is it already on table of s390 folks? If no,
> I can do it myself.
> 
> We have bitmap_from_arr32 and bitmap_to_arr32, so for 64-bit versions,
> we'd start from that.

Yep, thanks!

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-03-03 10:20 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-23 16:44 [PATCH v1 1/1] KVM: s390: Don't cast parameter in bit operations Andy Shevchenko
2022-02-24 11:36 ` Claudio Imbrenda
2022-02-24 12:10   ` Michael Mueller
2022-03-02 15:44     ` Andy Shevchenko
2022-03-02 17:18       ` Yury Norov
2022-03-02 17:31         ` Andy Shevchenko
2022-03-02 18:43           ` Yury Norov
2022-03-03 10:19             ` Andy Shevchenko
2022-02-24 19:51   ` Andy Shevchenko
2022-02-24 23:15     ` David Laight
2022-03-02 15:45 ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.