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

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 <[email protected]>

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 <[email protected]>

---
 arch/s390/include/asm/kvm.h |    9 +++++++++
 arch/s390/kvm/kvm-s390.c    |   24 ++++++++++++++++++++++++
 include/linux/kvm.h         |    4 ++++
 3 files changed, 37 insertions(+)

Quoted 4 lines

 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.

Quoted 5 lines

arch/s390/kvm/kvm-s390.c +0 -0

@@ -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);

arch/s390/kvm/kvm-s390.c long kvm_arch_vcpu_ioctl(struct file *fi · line 685

Quoted 10 lines

arch/s390/kvm/kvm-s390.c +9 -0

 		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.

arch/s390/kvm/kvm-s390.c long kvm_arch_vcpu_ioctl(struct file *fi · line 695

Quoted 8 lines

arch/s390/kvm/kvm-s390.c +8 -0

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

arch/s390/kvm/kvm-s390.c long kvm_arch_vcpu_ioctl(struct file *fi · line 696

Quoted 1 line

arch/s390/kvm/kvm-s390.c +1 -0

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

Then initializing it.

arch/s390/kvm/kvm-s390.c long kvm_arch_vcpu_ioctl(struct file *fi · line 697

Quoted 2 lines

arch/s390/kvm/kvm-s390.c +1 -0

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

What's this?

Quoted 4 lines

arch/s390/kvm/kvm-s390.c long kvm_arch_vcpu_ioctl(struct file *fi · line 697

Quoted 2 lines

arch/s390/kvm/kvm-s390.c +1 -0

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

What's this?

tlb flush. Necessary after setting the prefix register.

From: Christian Borntraeger <[email protected]>

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 <[email protected]>

---

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

Documentation/virtual/kvm/api.txt +0 -0

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

Documentation/virtual/kvm/api.txt +7 -0

+
+Capability: KVM_CAP_S390_SREGS2
+Architectures: s390x
+Type: vcpu ioctl
+Parameters: struct kvm_sregs2 (out)
+Returns: 0 on success, -1 on error
+

+
+4.65 KVM_S390_SET_SREGS2

Documentation/virtual/kvm/api.txt is supported; 2 if the processor require · line 1470

Quoted 10 lines

Documentation/virtual/kvm/api.txt +10 -0

+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

Quoted 13 lines · Alexander Graf

Documentation/virtual/kvm/api.txt is supported; 2 if the processor require · line 1470

Quoted 10 lines

Documentation/virtual/kvm/api.txt +10 -0

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

Quoted 1 line

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)

Quoted 3 lines

Quoted 3 lines

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

Quoted 2 lines

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

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

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

Quoted 3 lines · Alexander Graf

Quoted 3 lines

Quoted 3 lines

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?

Quoted 5 lines

Quoted 3 lines

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

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

Yes, I see.

Quoted 6 lines

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.