Re: [PATCH 1/1] kvm-s390: Provide guest TOD Clock Get/Set Controls

[Christian Borntraeger <borntraeger@de.ibm.com>]

Am 05.11.2014 18:37, schrieb Alexander Graf:
> 
> 
> On 05.11.14 17:48, Christian Borntraeger wrote:
>> Am 05.11.2014 14:11, schrieb Paolo Bonzini:
>>>
>>>
>>> On 05/11/2014 13:28, Christian Borntraeger wrote:
>>>> Am 05.11.2014 11:07, schrieb Alexander Graf:
>>>>>
>>>>>
>>>>> On 27.10.14 16:44, Jason J. Herne wrote:
>>>>>> From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
>>>>>>
>>>>>> Enable KVM_SET_CLOCK and KVM_GET_CLOCK ioctls on s390 for
>>>>>> managing guest Time Of Day clock value.
>>>>>>
>>>>>> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> 
>>>>>> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>>>>>
>>>>> I like it.
>>>>>
>>>>> Reviewed-by: Alexander Graf <agraf@suse.de>
>>>>
>>>> Paolo, are you ok with that patch as well? If yes I will send it with
>>>> the next bunch of s390 patches.
>>>>
>>>> PS: I remember that you were considering some different take on the
>>>> interface: IIRC you suggest to have the same format in
>>>> kvm_clock_data->clock as x86, and that we might want to use a flag
>>>> and a new field in the padding area that then contains the TOD value.
>>>> Now looking again at Documentation/virtual/kvm/api.txt I actually
>>>> prefer Jasons implementation since the api does not mention the
>>>> value/format/offset. It seems to be ns since boot, correct?
>>>>
>>>> So if any changes, I would prefer a small change to the
>>>> documentation, that makes the meaning of clock explicit per
>>>> architecture?
>>>
>>> After a quick refresh on IRC, I remembered our previous discussion.
>>>
>>> I was a bit worried that the interface did not let us pass the extra
>>> byte for the stcke instruction's overflow counter.  The question then is
>>> whether to:
>>>
>>> 1) keep an x86-consistent interface for KVM_GET/SET_CLOCK, and put the
>>> whole 16 byte stcke output in the padding
>>>
>>> 2) put the 8-byte stck value (stcke bytes 1-8) in the value, and the
>>> overflow counter (stcke byte 0) in the padding (with the presence
>>> governed by a flag).  As you explained, bytes 9-13 are computed by the
>>> CPU and we do not care anyway of accuracy beyond 0.25 ns, while bytes
>>> 14-15 are accessed separately via ONEREG.
>>>
>>> 3) use ONEREG instead of KVM_GET/SET_CLOCK.  You can decide whether to
>>> use a 72 (or 96) bit value, or two separate 8+64 values.
>>>
>>> 1 or 3 seem the cleanest.  On the other hand s390 doesn't have a use for
>>> a bootbased counter, which makes 1 much less interesting/useful than I
>>> imagined.
>>>
>>> PPC uses a combination of KVM_GET_SREGS and KVM_GET/SET_ONEREG for the
>>> closest equivalent (TBL/TBU), not KVM_GET/SET_CLOCK.  MIPS is also
>>> ONEREG-based.  This makes me lean towards 3.
>>>
>>> Of course 2 has code written, but it should be a small change to use
>>> ONEREG instead.  What do you think?
>>>
>>
>> I think the change to a ONEREG should be trivial. (it will be synced between all guest CPUs, so we could also use a VM attribute but a ONEREG should be ok as well.
>>
>> I think two registers (one 64bit and another 8bit register (which must be 0 all the time as of today) is preferred.
>>
>> I think we could even defer the 2nd register until we know what the hardware folks will come up with before 2042. (stcke in the POP indicates an 8bit counter).
>>
>> So Paolo, Alex two simple questions:
>>
>> - ONEREG or VM attribute?
> 
> On PPC we have core granularity, so while the interface is on one vcpu,
> it really only affects every 8th vcpu (or whatever you configure the
> number of threads as).
> 
> So there is precedence for an interface that modifies other vcpus while
> the ONE_REG is only targeting a single vcpu.
> 
> Whether you want to follow that approach or do it as VM attribute
> straight away, I don't mind much :).

given that top programmable field and epoch are available as ONEREG, lets do the same for TOD.

> 
>> - Just one 64bit value today and the other one later or both now (64+8)
> 
> Make it both today with a check that the second one has to be 0 maybe?
> Then we only need to modify the kernel itself, not the API later.

Makes sense.