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

[David Hildenbrand <david@redhat.com>]

On 18.01.2018 16:58, Michael Mueller wrote:
> 
> 
> 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...
> 
>>
> 

Actually, the problem is there are no atomic byte-based operations on
s390x without Interlocked-Access-Facility 2.

Even __sync_fetch_and_or(&gisa->ipm, value) falls back to a
Compare-And-Swap loop. And Compare-And-Swap also operates at least on 32bit.

So I assume there isn't too much we can do about it. As storage
locations following the u8 are also written - but in an atomic matter,
it should in general not matter.

But can we avoid starting the bitmap at the beginning of the gisa?

What about something like this:

+void kvm_s390_gisa_set_ipm_gisc(struct kvm_s390_gisa *gisa, u8 gisc)
+{
+       set_bit_inv(gisc, (unsigned long *) &gisa->ipm);
+}

-- 

Thanks,

David / dhildenb