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@xxxxxxxxxxxxxxxxxx>
>>>
>>> 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
