[PATCH]: kvm-s390: add KVM_S390_GET/SET_SREGS2 call for additional hw regs

[PATCH]: kvm-s390: add KVM_S390_GET/SET_SREGS2 call for additional hw regs

[Christian Borntraeger]

Avi, Marcelo,

let me know if you would prefer to reuse another register load/save ioctls
that is still unused for s390 (e.g. XCRS).


From: Christian Borntraeger <borntraeger@de.ibm.com>

For guest relocation and virsh dump qemu needs an interface to
get/set additional registers from kvm. We also need the prefix
register for all guest memory accesses to the prefix pages.

The prefix register could also be set via the KVM_S390_SIGP_SET_PREFIX
interrupt ioctl, but I also added the synchronous operation to have

o symmetry: we want to have the same struct for get/set routine
o the interrupt is only delivered before entering the SIE, we also
  want to cover the sequence set prefix/store status at prefix

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/kvm.h |    9 +++++++++
 arch/s390/kvm/kvm-s390.c    |   24 ++++++++++++++++++++++++
 include/linux/kvm.h         |    4 ++++
 3 files changed, 37 insertions(+)

Index: b/arch/s390/include/asm/kvm.h
===================================================================
--- a/arch/s390/include/asm/kvm.h
+++ b/arch/s390/include/asm/kvm.h
@@ -28,6 +28,15 @@ struct kvm_sregs {
 	__u64 crs[16];
 };
 
+/* for KVM_S390_GET_SREGS2 and KVM_S390_SET_SREGS2 */
+struct kvm_s390_sregs2 {
+	__u64 ckc;
+	__u64 cputm;
+	__u64 gbea;
+	__u32 todpr;
+	__u32 prefix;
+};
+
 /* for KVM_GET_FPU and KVM_SET_FPU */
 struct kvm_fpu {
 	__u32 fpc;
Index: b/arch/s390/kvm/kvm-s390.c
===================================================================
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -129,6 +129,7 @@ int kvm_dev_ioctl_check_extension(long e
 	case KVM_CAP_S390_PSW:
 	case KVM_CAP_S390_GMAP:
 	case KVM_CAP_SYNC_MMU:
+	case KVM_CAP_S390_SREGS2:
 		r = 1;
 		break;
 	default:
@@ -673,6 +674,29 @@ long kvm_arch_vcpu_ioctl(struct file *fi
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;
+	case KVM_S390_GET_SREGS2: {
+		struct kvm_s390_sregs2 sregs2;
+
+		sregs2.prefix = vcpu->arch.sie_block->prefix;
+		sregs2.gbea = vcpu->arch.sie_block->gbea;
+		sregs2.cputm = vcpu->arch.sie_block->cputm;
+		sregs2.ckc = vcpu->arch.sie_block->ckc;
+		sregs2.todpr = vcpu->arch.sie_block->todpr;
+		r = copy_to_user(argp, &sregs2, sizeof(sregs2));
+		break;
+	}
+	case KVM_S390_SET_SREGS2: {
+		struct kvm_s390_sregs2 sregs2;
+
+		vcpu->arch.sie_block->prefix = sregs2.prefix;
+		vcpu->arch.sie_block->gbea = sregs2.gbea;
+		vcpu->arch.sie_block->cputm = sregs2.cputm;
+		vcpu->arch.sie_block->ckc = sregs2.ckc;
+		vcpu->arch.sie_block->todpr = sregs2.todpr;
+		r = copy_from_user(&sregs2, argp, sizeof(sregs2));
+	        vcpu->arch.sie_block->ihcpu     = 0xffff;
+		break;
+	}
 	default:
 		r = -EINVAL;
 	}
Index: b/include/linux/kvm.h
===================================================================
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -557,6 +557,7 @@ struct kvm_ppc_pvinfo {
 #define KVM_CAP_MAX_VCPUS 66       /* returns max vcpus per vm */
 #define KVM_CAP_PPC_PAPR 68
 #define KVM_CAP_S390_GMAP 71
+#define KVM_CAP_S390_SREGS2 72
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -762,6 +763,9 @@ struct kvm_clock_data {
 #define KVM_CREATE_SPAPR_TCE	  _IOW(KVMIO,  0xa8, struct kvm_create_spapr_tce)
 /* Available with KVM_CAP_RMA */
 #define KVM_ALLOCATE_RMA	  _IOR(KVMIO,  0xa9, struct kvm_allocate_rma)
+/* Available with KVM_CAP_S390_SREGS2 */
+#define KVM_S390_GET_SREGS2        _IOR(KVMIO,  0xaa, struct kvm_s390_sregs2)
+#define KVM_S390_SET_SREGS2        _IOW(KVMIO,  0xab, struct kvm_s390_sregs2)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 

Re: [PATCH]: kvm-s390: add KVM_S390_GET/SET_SREGS2 call for additional hw regs

[Avi Kivity]

On 12/20/2011 11:38 AM, Christian Borntraeger wrote:
> Avi, Marcelo,
>
> let me know if you would prefer to reuse another register load/save ioctls
> that is still unused for s390 (e.g. XCRS).

No, the proposed names are fine.

>
> From: Christian Borntraeger <borntraeger@de.ibm.com>
>
> For guest relocation and virsh dump qemu needs an interface to
> get/set additional registers from kvm. We also need the prefix
> register for all guest memory accesses to the prefix pages.
>
> The prefix register could also be set via the KVM_S390_SIGP_SET_PREFIX
> interrupt ioctl, but I also added the synchronous operation to have
>
> o symmetry: we want to have the same struct for get/set routine
> o the interrupt is only delivered before entering the SIE, we also
>   want to cover the sequence set prefix/store status at prefix
>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/include/asm/kvm.h |    9 +++++++++
>  arch/s390/kvm/kvm-s390.c    |   24 ++++++++++++++++++++++++
>  include/linux/kvm.h         |    4 ++++
>  3 files changed, 37 insertions(+)

The lack of documentation is not.


> @@ -673,6 +674,29 @@ long kvm_arch_vcpu_ioctl(struct file *fi
>  	case KVM_S390_INITIAL_RESET:
>  		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>  		break;
> +	case KVM_S390_GET_SREGS2: {
> +		struct kvm_s390_sregs2 sregs2;
> +
> +		sregs2.prefix = vcpu->arch.sie_block->prefix;
> +		sregs2.gbea = vcpu->arch.sie_block->gbea;
> +		sregs2.cputm = vcpu->arch.sie_block->cputm;
> +		sregs2.ckc = vcpu->arch.sie_block->ckc;
> +		sregs2.todpr = vcpu->arch.sie_block->todpr;
> +		r = copy_to_user(argp, &sregs2, sizeof(sregs2));

Need to return -EFAULT, not the number of remaining bytes to copy.

> +		break;
> +	}
> +	case KVM_S390_SET_SREGS2: {
> +		struct kvm_s390_sregs2 sregs2;
> +
> +		vcpu->arch.sie_block->prefix = sregs2.prefix;
> +		vcpu->arch.sie_block->gbea = sregs2.gbea;
> +		vcpu->arch.sie_block->cputm = sregs2.cputm;
> +		vcpu->arch.sie_block->ckc = sregs2.ckc;
> +		vcpu->arch.sie_block->todpr = sregs2.todpr;

Copying uninitialized data.

> +		r = copy_from_user(&sregs2, argp, sizeof(sregs2));

Then initializing it.

> +	        vcpu->arch.sie_block->ihcpu     = 0xffff;

What's this?



-- 
error compiling committee.c: too many arguments to function

Re: [PATCH]: kvm-s390: add KVM_S390_GET/SET_SREGS2 call for additional hw regs

[Christian Borntraeger]

>>  arch/s390/include/asm/kvm.h |    9 +++++++++
>>  arch/s390/kvm/kvm-s390.c    |   24 ++++++++++++++++++++++++
>>  include/linux/kvm.h         |    4 ++++
>>  3 files changed, 37 insertions(+)
> 
> The lack of documentation is not.

Ok, will do.
> 
> 
>> @@ -673,6 +674,29 @@ long kvm_arch_vcpu_ioctl(struct file *fi
>>  	case KVM_S390_INITIAL_RESET:
>>  		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
>>  		break;
>> +	case KVM_S390_GET_SREGS2: {
>> +		struct kvm_s390_sregs2 sregs2;
>> +
>> +		sregs2.prefix = vcpu->arch.sie_block->prefix;
>> +		sregs2.gbea = vcpu->arch.sie_block->gbea;
>> +		sregs2.cputm = vcpu->arch.sie_block->cputm;
>> +		sregs2.ckc = vcpu->arch.sie_block->ckc;
>> +		sregs2.todpr = vcpu->arch.sie_block->todpr;
>> +		r = copy_to_user(argp, &sregs2, sizeof(sregs2));
> 
> Need to return -EFAULT, not the number of remaining bytes to copy.

Will fix.

>> +	case KVM_S390_SET_SREGS2: {
>> +		struct kvm_s390_sregs2 sregs2;
>> +
>> +		vcpu->arch.sie_block->prefix = sregs2.prefix;
>> +		vcpu->arch.sie_block->gbea = sregs2.gbea;
>> +		vcpu->arch.sie_block->cputm = sregs2.cputm;
>> +		vcpu->arch.sie_block->ckc = sregs2.ckc;
>> +		vcpu->arch.sie_block->todpr = sregs2.todpr;
> 
> Copying uninitialized data.
> 
>> +		r = copy_from_user(&sregs2, argp, sizeof(sregs2));
> 
> Then initializing it.

Hmm, a brown paper bag bug. Since life migration does not yet work
I only tested the get case (via dump). Sorry about that.

> 
>> +	        vcpu->arch.sie_block->ihcpu     = 0xffff;
> 
> What's this?

tlb flush. Necessary after setting the prefix register.

Thanks

Re: [PATCH]: kvm-s390: add KVM_S390_GET/SET_SREGS2 call for additional hw regs

[Avi Kivity]

On 12/20/2011 11:59 AM, Christian Borntraeger wrote:
> > 
> >> +	        vcpu->arch.sie_block->ihcpu     = 0xffff;
> > 
> > What's this?
>
> tlb flush. Necessary after setting the prefix register.

Perhaps worth wrapping into an inline with a descriptive name later on.

-- 
error compiling committee.c: too many arguments to function

[PATCH 1/2]: kvm-s390: use guest flush inline function

[Christian Borntraeger]

From: Christian Borntraeger <borntraeger@de.ibm.com>

Move all open-coded guest flush operations into an inline function.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/interrupt.c |    2 +-
 arch/s390/kvm/kvm-s390.c  |    2 +-
 arch/s390/kvm/kvm-s390.h  |    7 ++++++-
 3 files changed, 8 insertions(+), 3 deletions(-)

Index: b/arch/s390/kvm/interrupt.c
===================================================================
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -237,7 +237,7 @@ static void __do_deliver_interrupt(struc
 			   inti->prefix.address);
 		vcpu->stat.deliver_prefix_signal++;
 		vcpu->arch.sie_block->prefix = inti->prefix.address;
-		vcpu->arch.sie_block->ihcpu = 0xffff;
+		flush_guest_cpu(vcpu);
 		break;
 
 	case KVM_S390_RESTART:
Index: b/arch/s390/kvm/kvm-s390.c
===================================================================
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -291,7 +291,6 @@ static void kvm_s390_vcpu_initial_reset(
 	vcpu->arch.sie_block->gpsw.mask = 0UL;
 	vcpu->arch.sie_block->gpsw.addr = 0UL;
 	vcpu->arch.sie_block->prefix    = 0UL;
-	vcpu->arch.sie_block->ihcpu     = 0xffff;
 	vcpu->arch.sie_block->cputm     = 0UL;
 	vcpu->arch.sie_block->ckc       = 0UL;
 	vcpu->arch.sie_block->todpr     = 0;
@@ -301,6 +300,7 @@ static void kvm_s390_vcpu_initial_reset(
 	vcpu->arch.guest_fpregs.fpc = 0;
 	asm volatile("lfpc %0" : : "Q" (vcpu->arch.guest_fpregs.fpc));
 	vcpu->arch.sie_block->gbea = 1;
+	flush_guest_cpu(vcpu);
 }
 
 int kvm_arch_vcpu_setup(struct kvm_vcpu *vcpu)
Index: b/arch/s390/kvm/kvm-s390.h
===================================================================
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -1,7 +1,7 @@
 /*
  * kvm_s390.h -  definition for kvm on s390
  *
- * Copyright IBM Corp. 2008,2009
+ * Copyright IBM Corp. 2008,2011
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License (version 2 only)
@@ -47,6 +47,11 @@ static inline int __cpu_is_stopped(struc
 	return atomic_read(&vcpu->arch.sie_block->cpuflags) & CPUSTAT_STOP_INT;
 }
 
+static inline void flush_guest_cpu(struct kvm_vcpu *vcpu)
+{
+	vcpu->arch.sie_block->ihcpu = 0xffff;
+}
+
 int kvm_s390_handle_wait(struct kvm_vcpu *vcpu);
 enum hrtimer_restart kvm_s390_idle_wakeup(struct hrtimer *timer);
 void kvm_s390_tasklet(unsigned long parm);

[PATCH 2/2]: kvm-s390: add KVM_S390_GET/SET_SREGS2 call for additional hw regs

[Christian Borntraeger]

From: Christian Borntraeger <borntraeger@de.ibm.com>

For guest relocation and virsh dump qemu needs an interface to
get/set additional registers from kvm. We also need the prefix
register for all guest memory accesses to the prefix pages.

The prefix register could also be set via the KVM_S390_SIGP_SET_PREFIX
interrupt ioctl, but I also added the synchronous operation to have

o symmetry: we want to have the same struct for get/set routine
o the interrupt is only delivered before entering the SIE, we also
  want to cover the sequence set prefix/store status at prefix

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 Documentation/virtual/kvm/api.txt |   31 +++++++++++++++++++++++++++++++
 arch/s390/include/asm/kvm.h       |    9 +++++++++
 arch/s390/kvm/kvm-s390.c          |   30 ++++++++++++++++++++++++++++++
 include/linux/kvm.h               |    4 ++++
 4 files changed, 74 insertions(+)

Index: b/Documentation/virtual/kvm/api.txt
===================================================================
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1450,6 +1450,37 @@ is supported; 2 if the processor require
 an RMA, or 1 if the processor can use an RMA but doesn't require it,
 because it supports the Virtual RMA (VRMA) facility.
 
+4.64 KVM_S390_GET_SREGS2
+
+Capability: KVM_CAP_S390_SREGS2
+Architectures: s390x
+Type: vcpu ioctl
+Parameters: struct kvm_sregs2 (out)
+Returns: 0 on success, -1 on error
+
+Reads special registers from the vcpu which are not covered by sregs.
+
+/* s390x */
+struct kvm_sregs2 {
+	__u64 ckc;	/* clock comparator */
+	__u64 cputm;	/* cpu timer */
+	__u64 gbea;	/* guest breaking event address */
+	__u32 todpr;	/* tod programmable field */
+	__u32 prefix;	/* prefix register */
+};
+
+4.65 KVM_S390_SET_SREGS2
+
+Capability: KVM_CAP_S390_SREGS2
+Architectures: s390x
+Type: vcpu ioctl
+Parameters: struct kvm_sregs2 (in)
+Returns: 0 on success, -1 on error
+
+Writes special registers into the vcpu.  See KVM_S390_GET_SREGS2 for the
+data structures.
+
+
 5. The kvm_run structure
 
 Application code obtains a pointer to the kvm_run structure by
Index: b/arch/s390/include/asm/kvm.h
===================================================================
--- a/arch/s390/include/asm/kvm.h
+++ b/arch/s390/include/asm/kvm.h
@@ -28,6 +28,15 @@ struct kvm_sregs {
 	__u64 crs[16];
 };
 
+/* for KVM_S390_GET_SREGS2 and KVM_S390_SET_SREGS2 */
+struct kvm_s390_sregs2 {
+	__u64 ckc;	/* clock comparator */
+	__u64 cputm;	/* cpu timer */
+	__u64 gbea;	/* guest breaking event address */
+	__u32 todpr;	/* tod programmable field */
+	__u32 prefix;	/* prefix register */
+};
+
 /* for KVM_GET_FPU and KVM_SET_FPU */
 struct kvm_fpu {
 	__u32 fpc;
Index: b/arch/s390/kvm/kvm-s390.c
===================================================================
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -129,6 +129,7 @@ int kvm_dev_ioctl_check_extension(long e
 	case KVM_CAP_S390_PSW:
 	case KVM_CAP_S390_GMAP:
 	case KVM_CAP_SYNC_MMU:
+	case KVM_CAP_S390_SREGS2:
 		r = 1;
 		break;
 	default:
@@ -673,6 +674,35 @@ long kvm_arch_vcpu_ioctl(struct file *fi
 	case KVM_S390_INITIAL_RESET:
 		r = kvm_arch_vcpu_ioctl_initial_reset(vcpu);
 		break;
+	case KVM_S390_GET_SREGS2: {
+		struct kvm_s390_sregs2 sregs2;
+
+		sregs2.prefix = vcpu->arch.sie_block->prefix;
+		sregs2.gbea = vcpu->arch.sie_block->gbea;
+		sregs2.cputm = vcpu->arch.sie_block->cputm;
+		sregs2.ckc = vcpu->arch.sie_block->ckc;
+		sregs2.todpr = vcpu->arch.sie_block->todpr;
+		r = -EFAULT;
+		if (copy_to_user(argp, &sregs2, sizeof(sregs2)))
+			break;
+		r = 0;
+		break;
+	}
+	case KVM_S390_SET_SREGS2: {
+		struct kvm_s390_sregs2 sregs2;
+
+		r = -EFAULT;
+		if (copy_from_user(&sregs2, argp, sizeof(sregs2)))
+			break;
+		vcpu->arch.sie_block->prefix = sregs2.prefix;
+		vcpu->arch.sie_block->gbea = sregs2.gbea;
+		vcpu->arch.sie_block->cputm = sregs2.cputm;
+		vcpu->arch.sie_block->ckc = sregs2.ckc;
+		vcpu->arch.sie_block->todpr = sregs2.todpr;
+		flush_guest_cpu(vcpu);
+		r = 0;
+		break;
+	}
 	default:
 		r = -EINVAL;
 	}
Index: b/include/linux/kvm.h
===================================================================
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -557,6 +557,7 @@ struct kvm_ppc_pvinfo {
 #define KVM_CAP_MAX_VCPUS 66       /* returns max vcpus per vm */
 #define KVM_CAP_PPC_PAPR 68
 #define KVM_CAP_S390_GMAP 71
+#define KVM_CAP_S390_SREGS2 72
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -762,6 +763,9 @@ struct kvm_clock_data {
 #define KVM_CREATE_SPAPR_TCE	  _IOW(KVMIO,  0xa8, struct kvm_create_spapr_tce)
 /* Available with KVM_CAP_RMA */
 #define KVM_ALLOCATE_RMA	  _IOR(KVMIO,  0xa9, struct kvm_allocate_rma)
+/* Available with KVM_CAP_S390_SREGS2 */
+#define KVM_S390_GET_SREGS2        _IOR(KVMIO,  0xaa, struct kvm_s390_sregs2)
+#define KVM_S390_SET_SREGS2        _IOW(KVMIO,  0xab, struct kvm_s390_sregs2)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 

Re: [PATCH 2/2]: kvm-s390: add KVM_S390_GET/SET_SREGS2 call for additional hw regs

[Alexander Graf]

[resending because I mistakenly sent an html mime mail before, which got rejected by kvm@vger]

On 20.12.2011, at 12:41, Christian Borntraeger wrote:

> From: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> For guest relocation and virsh dump qemu needs an interface to
> get/set additional registers from kvm. We also need the prefix
> register for all guest memory accesses to the prefix pages.
> 
> The prefix register could also be set via the KVM_S390_SIGP_SET_PREFIX
> interrupt ioctl, but I also added the synchronous operation to have
> 
> o symmetry: we want to have the same struct for get/set routine
> o the interrupt is only delivered before entering the SIE, we also
>  want to cover the sequence set prefix/store status at prefix
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> Documentation/virtual/kvm/api.txt |   31 +++++++++++++++++++++++++++++++
> arch/s390/include/asm/kvm.h       |    9 +++++++++
> arch/s390/kvm/kvm-s390.c          |   30 ++++++++++++++++++++++++++++++
> include/linux/kvm.h               |    4 ++++
> 4 files changed, 74 insertions(+)
> 
> Index: b/Documentation/virtual/kvm/api.txt
> ===================================================================
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1450,6 +1450,37 @@ is supported; 2 if the processor require
> an RMA, or 1 if the processor can use an RMA but doesn't require it,
> because it supports the Virtual RMA (VRMA) facility.
> 
> +4.64 KVM_S390_GET_SREGS2
> +
> +Capability: KVM_CAP_S390_SREGS2
> +Architectures: s390x
> +Type: vcpu ioctl
> +Parameters: struct kvm_sregs2 (out)
> +Returns: 0 on success, -1 on error
> +
> +Reads special registers from the vcpu which are not covered by sregs.
> +
> +/* s390x */
> +struct kvm_sregs2 {
> +	__u64 ckc;	/* clock comparator */
> +	__u64 cputm;	/* cpu timer */
> +	__u64 gbea;	/* guest breaking event address */
> +	__u32 todpr;	/* tod programmable field */
> +	__u32 prefix;	/* prefix register */
> +};

Would it make sense to instead use the GET_ONE_REG and SET_ONE_REG interfaces?

  http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/80854

> +
> +4.65 KVM_S390_SET_SREGS2

I would very much appreciate if you could use 4.66 here. That makes merging the sections for GET_ONE_REG and SET_ONE_REG easier :).


Alex

Re: [PATCH 2/2]: kvm-s390: add KVM_S390_GET/SET_SREGS2 call for additional hw regs

[Christian Borntraeger]

On 20/12/11 15:20, Alexander Graf wrote:
>> +Reads special registers from the vcpu which are not covered by sregs.
>> +
>> +/* s390x */
>> +struct kvm_sregs2 {
>> +	__u64 ckc;	/* clock comparator */
>> +	__u64 cputm;	/* cpu timer */
>> +	__u64 gbea;	/* guest breaking event address */
>> +	__u32 todpr;	/* tod programmable field */
>> +	__u32 prefix;	/* prefix register */
>> +};
> 
> Would it make sense to instead use the GET_ONE_REG and SET_ONE_REG interfaces?
> 
>   http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/80854

Still not sure if I like that interface or not, but it should work.
I have some questions, though

- how should userspace check if the kernel supports this specific register?
- How would a GET_MANY_REGS / SET_MANY_REGS look like?
- Is the interface limited to 56 registers? (see the ID)
- scalability and performance. I dont know about other platforms, but the 
  exit overhead on s390 is in the same order of magnitute as a system call
  overhead, so multiple ioctls on the exit path will make the exit overhead 
  noticably more expensive (probably can be solved by a MANY variant). This
  might be a micro optimization though.
  (actually the only register that bothers me regarding performance right now 
  is prefix. qemu will need the content if it has to write to the prefix page. 
  Would be good to have an interface to get that without doing another system
  call)

Christian

Re: [PATCH 2/2]: kvm-s390: add KVM_S390_GET/SET_SREGS2 call for additional hw regs

[Alexander Graf]

On 20.12.2011, at 17:16, Christian Borntraeger wrote:

> On 20/12/11 15:20, Alexander Graf wrote:
>>> +Reads special registers from the vcpu which are not covered by sregs.
>>> +
>>> +/* s390x */
>>> +struct kvm_sregs2 {
>>> +	__u64 ckc;	/* clock comparator */
>>> +	__u64 cputm;	/* cpu timer */
>>> +	__u64 gbea;	/* guest breaking event address */
>>> +	__u32 todpr;	/* tod programmable field */
>>> +	__u32 prefix;	/* prefix register */
>>> +};
>> 
>> Would it make sense to instead use the GET_ONE_REG and SET_ONE_REG interfaces?
>> 
>>  http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/80854
> 
> Still not sure if I like that interface or not, but it should work.
> I have some questions, though
> 
> - how should userspace check if the kernel supports this specific register?

It would try to get/set it and get the respective return value.

> - How would a GET_MANY_REGS / SET_MANY_REGS look like?

struct many_regs {
    __u64 nr_regs; (array length for the latter two)
    __u64 ids_ptr; (ptr to user space memory with an array where we store ids we request)
    __u64 ret_ptr; (ptr to user space memory with an array where we store return values)
    __u64 reg_ptr; (ptr to user space memory with an array where the registers are)
}

for (i = 0; i < nr_regs; i++) {
    u32 __user *ret;
    u64 __user *ids;
    struct one_reg __user *reg;
    struct one_reg tmp_reg;
    int size;

    size = do_normal_one_reg(get_user(&ids[i]), &tmp_reg);
    if (size < 0)
        return size;
    copy_reg(&reg[i], &tmp_reg, size);

    return 0;
}

Something like this maybe. We could also combine the 3 pointers into a single struct and just have the user pass an array of that struct to kernel space.

> - Is the interface limited to 56 registers? (see the ID)

Uh. It's limited to 0x0fffffffffffffff registers per architecture :).

> - scalability and performance. I dont know about other platforms, but the 
>  exit overhead on s390 is in the same order of magnitute as a system call
>  overhead, so multiple ioctls on the exit path will make the exit overhead 
>  noticably more expensive (probably can be solved by a MANY variant). This
>  might be a micro optimization though.
>  (actually the only register that bothers me regarding performance right now 
>  is prefix. qemu will need the content if it has to write to the prefix page. 
>  Would be good to have an interface to get that without doing another system
>  call)

Do you expect the prefix register to be synced often? If so, then you should maybe put it into kvm_struct
and always have it shared between kernel and user space, always updating it on every user space exit
and entry (you can optimize by checking if it changed).

I don't think user space should worry about prefix too often though. Unless you expect anyone to DMA
into the CPU prefix area :).


Alex

Re: [PATCH 2/2]: kvm-s390: add KVM_S390_GET/SET_SREGS2 call for additional hw regs

[Christian Borntraeger]

On 20/12/11 17:28, Alexander Graf wrote:
>>> Would it make sense to instead use the GET_ONE_REG and SET_ONE_REG interfaces?
>>>
>>>  http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/80854

What is the status of these patches?

> 
>> - Is the interface limited to 56 registers? (see the ID)
> 
> Uh. It's limited to 0x0fffffffffffffff registers per architecture :).

Yes, I see.



> Do you expect the prefix register to be synced often? If so, then you should maybe put it into kvm_struct
> and always have it shared between kernel and user space, always updating it on every user space exit
> and entry (you can optimize by checking if it changed).
> 
> I don't think user space should worry about prefix too often though. Unless you expect anyone to DMA
> into the CPU prefix area :).

In theory:
To be architecture compliant we actually must check for _every_ memory access to the guest 
if it is 0 < address < 8192  or prefix < address < prefix+8192 and swap that. Usually this
should not not happen very often, but you dont know until you check.
As of today:
Only a small part of the guest OS actually touches the lowcore and current KVM guests never
cause exits to qemu that have to deal with such an case - it was handled by SIE or by the
host kernel - so not a problem.
Future:
Additional components and non-para-virtualized I/O will pass addresses in the prefix pages
to some  instructions that qemu might have to handle, so we might need that more often.

To play safe, we might want to do the prefix translation for all handled intercepts.
Then we would need the prefix value almost always.


Christian

Re: [PATCH 2/2]: kvm-s390: add KVM_S390_GET/SET_SREGS2 call for additional hw regs

[Alexander Graf]

On 21.12.2011, at 07:52, Christian Borntraeger wrote:

> On 20/12/11 17:28, Alexander Graf wrote:
>>>> Would it make sense to instead use the GET_ONE_REG and SET_ONE_REG interfaces?
>>>> 
>>>> http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/80854
> 
> What is the status of these patches?

They're in my queue. I'll have to go through a full set of tests and a few more applies before I'm sending it out again though.

> 
>> 
>>> - Is the interface limited to 56 registers? (see the ID)
>> 
>> Uh. It's limited to 0x0fffffffffffffff registers per architecture :).
> 
> Yes, I see.
> 
> 
> 
>> Do you expect the prefix register to be synced often? If so, then you should maybe put it into kvm_struct
>> and always have it shared between kernel and user space, always updating it on every user space exit
>> and entry (you can optimize by checking if it changed).
>> 
>> I don't think user space should worry about prefix too often though. Unless you expect anyone to DMA
>> into the CPU prefix area :).
> 
> In theory:
> To be architecture compliant we actually must check for _every_ memory access to the guest 
> if it is 0 < address < 8192  or prefix < address < prefix+8192 and swap that. Usually this
> should not not happen very often, but you dont know until you check.
> As of today:
> Only a small part of the guest OS actually touches the lowcore and current KVM guests never
> cause exits to qemu that have to deal with such an case - it was handled by SIE or by the
> host kernel - so not a problem.
> Future:
> Additional components and non-para-virtualized I/O will pass addresses in the prefix pages
> to some  instructions that qemu might have to handle, so we might need that more often.
> 
> To play safe, we might want to do the prefix translation for all handled intercepts.
> Then we would need the prefix value almost always.

But you only need to read it from user space, right? So you can put a R/O version of it in the shared kvm_run struct or create your own MMIO mapping for shared values (which should be possible thanks to Carsten's patch set).

That way you get almost 0 overhead from fetching the prefix register, but can use an extensible interface for registers that don't change that often and actually get an event (ioctl) in the kernel for prefix writes from user space, which should almost never occur.


Alex