Re: [PATCH 06/11] KVM: s390: Multiple Epoch Facility support

[David Hildenbrand <david@redhat.com>]

On 29.08.2017 14:46, Christian Borntraeger wrote:
> 
> 
> On 08/29/2017 02:24 PM, David Hildenbrand wrote:
>> On 28.08.2017 10:07, Christian Borntraeger wrote:
>>> From: "Collin L. Walling" <walling@linux.vnet.ibm.com>
>>>
>>> Allow for the enablement of MEF and the support for the extended
>>> epoch in SIE and VSIE for the extended guest TOD-Clock.
>>>
>>> A new interface is used for getting/setting a guest's extended
>>> TOD-Clock that uses a single ioctl invocation, KVM_S390_VM_TOD_EXT.
>>> The old method of getting and setting the guest TOD-Clock is
>>> retained and is used when the old ioctls are called.
>>
>> After talking to Christian, I understand that we have to set it in "one
>> shot" as we can otherwise run into problems when getting/setting the TOD
>> as we are looking at a moving target. This should go into the cover letter.
> 
> You mean in the patch description or in the git tag description?

Patch description. But just my opinion.

> 
>>
>> I need some more info regarding the architecture change.
>>
>> For now, epoch was an unsigned value that is interpreted as an signed value.
> 
> It is always considered a signed value, (so the guest can be before or after the host)
> we just used u64 to make the wraparound defined (doing all the math in the unsigned realm
> as signed overflow is undefined)
> 
>>
>> a) Is that still true with multiple epoch?
>> b) What is the format of the epoch index? Can it also be "negative"?
> 
> The concatenation of both is considered a 72bit signed value.

Ah okay. So subtracting 1 from idx=0, epoch=0 will result in
idx=0xff epoch=0xffffff

> 
>>
>> Why I am asking: I can see comparison made with the epoch:
>>
>>> +	if (test_kvm_facility(vcpu->kvm, 139)) {
>>> +		scb_s->epdx += vcpu->kvm->arch.epdx;
>>> +		if (scb_s->epoch < vcpu->kvm->arch.epoch)
> 
> 	This checks for an overflow after the addition and
> 
>>> +			scb_s->epdx += 1;
> 
> 	corrects this. Since this is all unsigned math this is
> 	defined to wrap around and should work unless I missed 
> 	something

I just had in mind:

-1 < 0

but 0xffffffff is clearly > 0x0

I might be mistaking, but we could get both an overflow or an underflow
here.

But my brain has to consume new information first, so ignore the noise
for now :)


> 
>>> +	}
>>
>> or
>>
>>> +	if (kvm->arch.epoch > gtod->tod)
>>> +		kvm->arch.epdx -= 1;
>>
>>
>> If I remember correctly, such comparisons won't work correctly when
>> having this signed/unsigned duality. But I might be wrong.
> 
> As far as I can see we have unsigned data types everywhere.
> 

-- 

Thanks,

David