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

[Paolo Bonzini <pbonzini@redhat.com>]

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?

Paolo