Re: [PATCH 04/12] KVM: s390: implement GISA IPM related primitives

[Michael Mueller <mimu@linux.vnet.ibm.com>]

On 17.01.18 15:35, David Hildenbrand wrote:
> On 16.01.2018 21:02, Christian Borntraeger wrote:
>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
>>
>> The patch implements routines to access the GISA to test and modify
>> its Interruption Pending Mask (IPM) from the host side.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
>> Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>   arch/s390/kvm/interrupt.c | 23 +++++++++++++++++++++++
>>   arch/s390/kvm/kvm-s390.h  |  4 ++++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index b94173560dcf..dfdecff302d2 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -2682,3 +2682,26 @@ int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu, __u8 __user *buf, int len)
>>   
>>   	return n;
>>   }
>> +
>> +#define IPM_BIT_OFFSET (offsetof(struct kvm_s390_gisa, ipm) * 8)
> 
> 8 -> BITS_PER_BYTE, but ...
> 
> Am I wrong or can we only modify the 8 ipm bits this way? But we
> want/have to do it in an atomic fashion?
> 
> Using an unsigned long seems wrong, because we "rewrite" more than we
> should. Esp. everything beyond ipm. oi / ni and friends are not
> available on older machines.
> 
> What about something as simple as the following
> 
> 
> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
> +{
> +       u8 value = (0x80 >> gisc);
> +
> +       __sync_fetch_and_or(&gisa->ipm, value);
> +}
> +
> 

Nobody is using this compiler build-in in the kernel and a quick compile 
shows that it produces an ORK and CS instead of a LAOG beside a lot of 
supporting instructions. Unfortunately it's not saving what you promise 
... ;) Will not change.

> 
>> +
>> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>> +{
>> +	set_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>> +}
>> +
>> +u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa)
>> +{
>> +	return (u8) READ_ONCE(gisa->ipm);
>> +}
>> +
>> +void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>> +{
>> +	clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>> +}
>> +
>> +int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
>> +{
>> +	return test_and_clear_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
>> +}
>> +
> 
> shouldn't these be static inline instead?

Well, I get requests in both directions for that... my opinion is also 
yes and I will change it a very last time!

> 
>> diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
>> index 8877116f0159..b17e4dea7ea5 100644
>> --- a/arch/s390/kvm/kvm-s390.h
>> +++ b/arch/s390/kvm/kvm-s390.h
>> @@ -367,6 +367,10 @@ int kvm_s390_set_irq_state(struct kvm_vcpu *vcpu,
>>   			   void __user *buf, int len);
>>   int kvm_s390_get_irq_state(struct kvm_vcpu *vcpu,
>>   			   __u8 __user *buf, int len);
>> +void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
>> +u8 kvm_s390_gisa_get_ipm(struct kvm_s390_gisa *gisa);
>> +void kvm_s390_gisa_clear_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
>> +int kvm_s390_gisa_tac_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc);
>>   
>>   /* implemented in guestdbg.c */
>>   void kvm_s390_backup_guest_per_regs(struct kvm_vcpu *vcpu);
>>
> 

Thanks a lot for your comments David.

Michael

>