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

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

[Jason J. Herne]

From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>

This is the simpler version of a previous patch. For reference, the previous
patch and related discussion can be found here:
http://www.spinics.net/lists/kvm/msg108448.html

Jason J. Herne (1):
  kvm-s390: Provide guest TOD Clock Get/Set Controls

 Documentation/virtual/kvm/api.txt |  4 +--
 arch/s390/kvm/kvm-s390.c          | 74 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 2 deletions(-)

-- 
1.8.3.2

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

[Jason J. Herne]

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>
---
 Documentation/virtual/kvm/api.txt |  4 +--
 arch/s390/kvm/kvm-s390.c          | 74 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 7610eaa..f7eb604 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -745,7 +745,7 @@ struct kvm_xen_hvm_config {
 4.29 KVM_GET_CLOCK
 
 Capability: KVM_CAP_ADJUST_CLOCK
-Architectures: x86
+Architectures: x86, s390
 Type: vm ioctl
 Parameters: struct kvm_clock_data (out)
 Returns: 0 on success, -1 on error
@@ -764,7 +764,7 @@ struct kvm_clock_data {
 4.30 KVM_SET_CLOCK
 
 Capability: KVM_CAP_ADJUST_CLOCK
-Architectures: x86
+Architectures: x86, s390
 Type: vm ioctl
 Parameters: struct kvm_clock_data (in)
 Returns: 0 on success, -1 on error
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 55aade4..774a2a6 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -31,6 +31,7 @@
 #include <asm/switch_to.h>
 #include <asm/facility.h>
 #include <asm/sclp.h>
+#include<asm/timex.h>
 #include "kvm-s390.h"
 #include "gaccess.h"
 
@@ -159,6 +160,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_S390_IRQCHIP:
 	case KVM_CAP_VM_ATTRIBUTES:
 	case KVM_CAP_MP_STATE:
+	case KVM_CAP_ADJUST_CLOCK:
 		r = 1;
 		break;
 	case KVM_CAP_NR_VCPUS:
@@ -328,6 +330,56 @@ static int kvm_s390_vm_has_attr(struct kvm *kvm, struct kvm_device_attr *attr)
 	return ret;
 }
 
+static int kvm_s390_get_guest_tod(struct kvm *kvm,
+				  struct kvm_clock_data *user_clock)
+{
+	u64 current_host_tod;
+	u64 epoch = 0;
+	struct kvm_vcpu *vcpu;
+	unsigned int vcpu_idx;
+	int r;
+
+	/* All vcpu's epochs are in sync. Just Grab the 1st one */
+	kvm_for_each_vcpu(vcpu_idx, vcpu, kvm) {
+		epoch = vcpu->arch.sie_block->epoch;
+		break;
+	}
+
+	r = store_tod_clock(&current_host_tod);
+	if (r)
+		return r;
+
+	user_clock->clock = current_host_tod + epoch;
+	return 0;
+}
+
+/*
+Set the guest's effective TOD clock to the given value. The guest's
+TOD clock is determined by the following formula: gtod = host_tod + epoch.
+NOTE: Even though the epoch value is associated with a vcpu, there is only
+one TOD clock and epoch value per guest.  All vcpu's epoch values must be kept
+synchronized.
+*/
+static int kvm_s390_set_guest_tod(struct kvm *kvm,
+				  struct kvm_clock_data *user_clock)
+{
+	u64 current_host_tod, epoch;
+	struct kvm_vcpu *vcpu;
+	unsigned int vcpu_idx;
+	int r;
+
+	r = store_tod_clock(&current_host_tod);
+	if (r)
+		return r;
+
+	epoch = user_clock->clock - current_host_tod;
+
+	kvm_for_each_vcpu(vcpu_idx, vcpu, kvm)
+		vcpu->arch.sie_block->epoch = epoch;
+
+	return 0;
+}
+
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg)
 {
@@ -387,6 +439,28 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		r = kvm_s390_vm_has_attr(kvm, &attr);
 		break;
 	}
+	case KVM_GET_CLOCK: {
+		struct kvm_clock_data user_clock;
+
+		r = kvm_s390_get_guest_tod(kvm, &user_clock);
+		if (r)
+			break;
+
+		if (copy_to_user(argp, &user_clock, sizeof(user_clock)))
+			r = -EFAULT;
+
+		break;
+	}
+	case KVM_SET_CLOCK: {
+		struct kvm_clock_data user_clock;
+
+		r = -EFAULT;
+		if (copy_from_user(&user_clock, argp,
+				   sizeof(struct kvm_clock_data)))
+			break;
+		r = kvm_s390_set_guest_tod(kvm, &user_clock);
+		break;
+	}
 	default:
 		r = -ENOTTY;
 	}
-- 
1.8.3.2

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

[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>


Alex

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

[Christian Borntraeger]

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?

Christian

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

[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?

Paolo

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

[Alexander Graf]

On 05.11.14 14:11, Paolo Bonzini wrote:
> 
> 
> 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?

How far does the existing nanosecond number get us until we hit the
64bit limit? And by the time we hit it, wouldn't we hit it on x86 as well?


Alex

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

[Paolo Bonzini]

On 05/11/2014 15:32, Alexander Graf wrote:
> > Of course 2 has code written, but it should be a small change to use
> > ONEREG instead.  What do you think?
>
> How far does the existing nanosecond number get us until we hit the
> 64bit limit?

2042.

> And by the time we hit it, wouldn't we hit it on x86 as well?

No, because x86 counts up since boot.

Paolo

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

[Christian Borntraeger]

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?
- Just one 64bit value today and the other one later or both now (64+8)

Christian

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

[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 :).

> - 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.


Alex

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

[Christian Borntraeger]

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.

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

[Paolo Bonzini]

On 05/11/2014 18:56, Christian Borntraeger wrote:
>> > 
>> > 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.

Is the epoch per-cpu?

Paolo

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

[Christian Borntraeger]

Am 05.11.2014 20:45, schrieb Paolo Bonzini:
> 
> 
> On 05/11/2014 18:56, Christian Borntraeger wrote:
>>>>
>>>> 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.
> 
> Is the epoch per-cpu?

two answers :-)

- the implementation is one epoch per control block, so someone could do that per CPU...but:
- guest TOD == host TOD + epochdiff. architecture mandates that there is only one TOD per system, so all guest TODs must be synced and so must be all epochdiffs

Some background. We provided access to the epoch value about 2 years ago with other things as ONEREG. Asumming that all hosts are time synced, we could just migrate the epoch value.
Now: this is not the case all the time. Just migrating the epoch could result in time jumping forth and back.

This thing is now: QEMU cannot calculate a correction reliably, because it cannot rely on the value of the TOD (by using stck) since the kernel might do tricks with the host TOD value as soon as we enable time synching between z boxes.
(Thats why normal userspace should not use stck either, it should use gettimeofday because the kernel might have offsets etc due to NTP or time synching between boxes). So we finally cam up with just migrating the guest visible TOD, which seems to work fine.

As a recap we have now:
#define KVM_REG_S390_TODPR     (KVM_REG_S390 | KVM_REG_SIZE_U32 | 0x1)
#define KVM_REG_S390_EPOCHDIFF (KVM_REG_S390 | KVM_REG_SIZE_U64 | 0x2)

and we would
add
#define KVM_REG_S390_TOD       (KVM_REG_S390 | KVM_REG_SIZE_U64 | 0x3) 
#define KVM_REG_S390_TOD_INDEX (KVM_REG_S390 | KVM_REG_SIZE_U8 | 0x4)
(any better name?)

Makes sense?

Christian

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

[Christian Borntraeger]

Am 06.11.2014 09:43, schrieb Christian Borntraeger:
> Am 05.11.2014 20:45, schrieb Paolo Bonzini:
>>
>>
>> On 05/11/2014 18:56, Christian Borntraeger wrote:
>>>>>
>>>>> 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.
>>
>> Is the epoch per-cpu?
> 
> two answers :-)
> 
> - the implementation is one epoch per control block, so someone could do that per CPU...but:
> - guest TOD == host TOD + epochdiff. architecture mandates that there is only one TOD per system, so all guest TODs must be synced and so must be all epochdiffs
> 
> Some background. We provided access to the epoch value about 2 years ago with other things as ONEREG. Asumming that all hosts are time synced, we could just migrate the epoch value.
> Now: this is not the case all the time. Just migrating the epoch could result in time jumping forth and back.
> 
> This thing is now: QEMU cannot calculate a correction reliably, because it cannot rely on the value of the TOD (by using stck) since the kernel might do tricks with the host TOD value as soon as we enable time synching between z boxes.
> (Thats why normal userspace should not use stck either, it should use gettimeofday because the kernel might have offsets etc due to NTP or time synching between boxes). So we finally cam up with just migrating the guest visible TOD, which seems to work fine.
> 
> As a recap we have now:
> #define KVM_REG_S390_TODPR     (KVM_REG_S390 | KVM_REG_SIZE_U32 | 0x1)
> #define KVM_REG_S390_EPOCHDIFF (KVM_REG_S390 | KVM_REG_SIZE_U64 | 0x2)
> 
> and we would
> add
> #define KVM_REG_S390_TOD       (KVM_REG_S390 | KVM_REG_SIZE_U64 | 0x3) 
> #define KVM_REG_S390_TOD_INDEX (KVM_REG_S390 | KVM_REG_SIZE_U8 | 0x4)

obviously not 0x3 and 0x4 but something higher..

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

[Paolo Bonzini]

On 06/11/2014 09:43, Christian Borntraeger wrote:
> Am 05.11.2014 20:45, schrieb Paolo Bonzini:
>>
>>
>> On 05/11/2014 18:56, Christian Borntraeger wrote:
>>>>>
>>>>> 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.
>>
>> Is the epoch per-cpu?
> 
> two answers :-)
> 
> - the implementation is one epoch per control block, so someone could do that per CPU...but:
> - guest TOD == host TOD + epochdiff. architecture mandates that there is only one TOD per system, so all guest TODs must be synced and so must be all epochdiffs

Got it.  Using VM attrs or ONEREG is the same, you choose.

> As a recap we have now:
> #define KVM_REG_S390_TODPR     (KVM_REG_S390 | KVM_REG_SIZE_U32 | 0x1)
> #define KVM_REG_S390_EPOCHDIFF (KVM_REG_S390 | KVM_REG_SIZE_U64 | 0x2)
> 
> and we would
> add
> #define KVM_REG_S390_TOD       (KVM_REG_S390 | KVM_REG_SIZE_U64 | 0x3) 
> #define KVM_REG_S390_TOD_INDEX (KVM_REG_S390 | KVM_REG_SIZE_U8 | 0x4)

TOD_HIGH?  TOD_EXTRA?  TOD_EXTRA_HIGH?

Paolo