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

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

On 18.01.18 15:33, David Hildenbrand wrote:
> On 18.01.2018 15:29, Michael Mueller wrote:
>>
>>
>> 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.
>>
> 
> Using unsigned long * bitmap operations to modify an u8 type atomically
> just seems very very wrong >

I have to reconsider this...

>