Skip to contentSkip to conversation

RFC: provide synchronous registers in kvm_run

PATCH In review Christian Borntraeger 4 participants 18 messages Dec 22, 2011 – Jan 9, 2012 2 messages not in the archive

Subsystems (8)
Documentation x86 KVM PowerPC +4 more subsystems
lore
Share & tools
lore

Christian Borntraeger

author
#0 lore raw

cover RFC: provide synchronous registers in kvm_run

Avi, Marcelo,

currently userspace can access guest registers via several ioctls. Some of these registers might be useful very often. Here the system call overhead for ioctl can make an exit more expensive than necessary.
In a discussion with Alex Graf we concluded that it might be beneficial to have a subset of registers available in kvm_run. (The ioctls will also be available).

This series provides a prototype implementation together with two example users for s390.

Opionions?

author
#1 lore raw

PATCH 1/3 kvm: provide synchronous registers in kvm_run

From: Christian Borntraeger <[email protected]>

On some cpus the overhead for virtualization instructions is in the same range as a system call. Having to call multiple ioctls to get set registers will make userspace handled exits more expensive than necessary. Lets provide two sections in kvm_run to have a shared save area for guest registers.
1. the first section is read-only, to handle registers that have side-effects
2. the second section is read/write, e.g. for general purpose registers.

Signed-off-by Christian Borntraeger <[email protected]>
---
 Documentation/virtual/kvm/api.txt |   16 ++++++++++++++++
 arch/ia64/include/asm/kvm.h       |    7 +++++++
 arch/powerpc/include/asm/kvm.h    |    7 +++++++
 arch/s390/include/asm/kvm.h       |    6 ++++++
 arch/x86/include/asm/kvm.h        |    7 +++++++
 include/linux/kvm.h               |   13 +++++++++++++
 6 files changed, 56 insertions(+)
Documentation/virtual/kvm/api.txt +16 -0
Index: b/Documentation/virtual/kvm/api.txt
===================================================================
@@ -1652,7 +1652,23 @@ developer registration required to acces
 		/* Fix the size of the union. */
 		char padding[256];
 	};
+	/* Here are two fields that allow to access often used registers
+         * directly, to avoid the overhead of the ioctl system call */
+	union {
+		/* registers which can be only read */
+		struct sync_ro_regs sync_ro_regs;
+		char padding[1024];
+	};
+	union {
+		/* read/write guest registers */
+		struct sync_rw_regs sync_rw_regs;
+		char padding[1024];
+	};
 };
+These fields allow userspace to access certain guest registers without
+having to call SET/GET_*REGS. Thus we can avoid some system call
+overhead if userspace has to handle the exit. (only available if
+KVM_CAP_SYNC_REGS is set). The ioctls will still work.
 
 6. Capabilities that can be enabled
arch/ia64/include/asm/kvm.h +7 -0
Index: b/arch/ia64/include/asm/kvm.h
===================================================================
@@ -261,4 +261,11 @@ struct kvm_debug_exit_arch {
 struct kvm_guest_debug_arch {
 };
 
+/* definition of registers in kvm_run */
+struct sync_ro_regs {
+};
+
+struct sync_rw_regs {
+};
+
 #endif
arch/powerpc/include/asm/kvm.h +7 -0
Index: b/arch/powerpc/include/asm/kvm.h
===================================================================
@@ -265,6 +265,13 @@ struct kvm_debug_exit_arch {
 struct kvm_guest_debug_arch {
 };
 
+/* definition of registers in kvm_run */
+struct sync_ro_regs {
+};
+
+struct sync_rw_regs {
+};
+
 #define KVM_REG_MASK		0x001f
 #define KVM_REG_EXT_MASK	0xffe0
 #define KVM_REG_GPR		0x0000
arch/s390/include/asm/kvm.h +6 -0
Index: b/arch/s390/include/asm/kvm.h
===================================================================
@@ -41,4 +41,10 @@ struct kvm_debug_exit_arch {
 struct kvm_guest_debug_arch {
 };
 
+/* definition of registers in kvm_run */
+struct sync_ro_regs {
+};
+
+struct sync_rw_regs {
+};
 #endif
arch/x86/include/asm/kvm.h +7 -0
Index: b/arch/x86/include/asm/kvm.h
===================================================================
@@ -321,4 +321,11 @@ struct kvm_xcrs {
 	__u64 padding[16];
 };
 
+/* definition of registers in kvm_run */
+struct sync_ro_regs {
+};
+
+struct sync_rw_regs {
+};
+
 #endif /* _ASM_X86_KVM_H */
include/linux/kvm.h +13 -0
Index: b/include/linux/kvm.h
===================================================================
@@ -273,6 +273,18 @@ struct kvm_run {
 		/* Fix the size of the union. */
 		char padding[256];
 	};
+	/* Here are two fields that allow to access often used registers
+         * directly, to avoid the overhead of the ioctl system call */
+	union {
+		/* registers which can be only read */
+		struct sync_ro_regs sync_ro_regs;
+		char padding[1024];
+	};
+	union {
+		/* read/write guest registers */
+		struct sync_rw_regs sync_rw_regs;
+		char padding[1024];
+	};
 };
 
 /* for KVM_REGISTER_COALESCED_MMIO / KVM_UNREGISTER_COALESCED_MMIO */

@@ -557,6 +569,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_SYNC_REGS 72
 
 #ifdef KVM_CAP_IRQ_ROUTING
#2 lore raw

Re: [patch 1/3] kvm: provide synchronous registers in kvm_run

Quoted 9 lines · Christian Borntraeger

From: Christian Borntraeger <[email protected]>

On some cpus the overhead for virtualization instructions is in the same range as a system call. Having to call multiple ioctls to get set registers will make userspace handled exits more expensive than necessary. Lets provide two sections in kvm_run to have a shared save area for guest registers.
1. the first section is read-only, to handle registers that have side-effects
2. the second section is read/write, e.g. for general purpose registers.

An alternative is to have a single section, with a bitmask set by userspace to indicate which registers have not been changed (a la svm's CleanBits). Is this worthwhile for s390?

Documentation/virtual/kvm/api.txt developer registration required to acces · line 1659

Quoted 6 lines
Documentation/virtual/kvm/api.txt +5 -0
 	};
+	/* Here are two fields that allow to access often used registers
+         * directly, to avoid the overhead of the ioctl system call */
+	union {
+		/* registers which can be only read */
+		struct sync_ro_regs sync_ro_regs;

kvm_

Documentation/virtual/kvm/api.txt developer registration required to acces · line 1664

Quoted 5 lines
Documentation/virtual/kvm/api.txt +5 -0
+		char padding[1024];
+	};
+	union {
+		/* read/write guest registers */
+		struct sync_rw_regs sync_rw_regs;

kvm_

Documentation/virtual/kvm/api.txt developer registration required to acces · line 1667

Quoted 3 lines
Documentation/virtual/kvm/api.txt +2 -0
+		char padding[1024];
+	};
 };

blank line

Quoted 6 lines
Documentation/virtual/kvm/api.txt +4 -0
+These fields allow userspace to access certain guest registers without
+having to call SET/GET_*REGS. Thus we can avoid some system call
+overhead if userspace has to handle the exit. (only available if
+KVM_CAP_SYNC_REGS is set). The ioctls will still work.
Signature · 1 line
error compiling committee.c: too many arguments to function
#3 lore raw

Re: [patch 1/3] kvm: provide synchronous registers in kvm_run

In reply to Christian Borntraeger
Quoted 31 lines · Christian Borntraeger

From: Christian Borntraeger <[email protected]>

On some cpus the overhead for virtualization instructions is in the same range as a system call. Having to call multiple ioctls to get set registers will make userspace handled exits more expensive than necessary. Lets provide two sections in kvm_run to have a shared save area for guest registers.
1. the first section is read-only, to handle registers that have side-effects
2. the second section is read/write, e.g. for general purpose registers.

Signed-off-by Christian Borntraeger <[email protected]>

---

Documentation/virtual/kvm/api.txt |   16 ++++++++++++++++
arch/ia64/include/asm/kvm.h       |    7 +++++++
arch/powerpc/include/asm/kvm.h    |    7 +++++++
arch/s390/include/asm/kvm.h       |    6 ++++++
arch/x86/include/asm/kvm.h        |    7 +++++++
include/linux/kvm.h               |   13 +++++++++++++
6 files changed, 56 insertions(+)

Index: b/Documentation/virtual/kvm/api.txt
===================================================================

Documentation/virtual/kvm/api.txt +2 -0
@@ -1652,7 +1652,23 @@ developer registration required to acces
 		/* Fix the size of the union. */
 		char padding[256];
 	};
+	/* Here are two fields that allow to access often used registers
+         * directly, to avoid the overhead of the ioctl system call */

Documentation/virtual/kvm/api.txt developer registration required to acces · line 1666

Quoted 10 lines
Documentation/virtual/kvm/api.txt +10 -0
+	union {
+		/* registers which can be only read */
+		struct sync_ro_regs sync_ro_regs;
+		char padding[1024];
+	};
+	union {
+		/* read/write guest registers */
+		struct sync_rw_regs sync_rw_regs;
+		char padding[1024];
+	};

Why have 2 different structs?

Unions shouldn't be anonymous. Please give the union a name :).

Alex

author
#4 lore raw

PATCH 2/3 kvm-s390: provide the prefix register via kvm_run

In reply to Christian Borntraeger

From: Christian Borntraeger <[email protected]>

The prefix register is a read-mostly value that is necessary to emulate memory accesses in an architecture compliant-way. Avoid an additional ioctl by providing the prefix content in the r/o section of kvm_run.

Signed-off-by Christian Borntraeger <[email protected]>
---
 arch/s390/include/asm/kvm.h |    1 +
 arch/s390/kvm/kvm-s390.c    |    2 ++
 2 files changed, 3 insertions(+)
arch/s390/include/asm/kvm.h +1 -0
Index: b/arch/s390/include/asm/kvm.h
===================================================================
@@ -43,6 +43,7 @@ struct kvm_guest_debug_arch {
 
 /* definition of registers in kvm_run */
 struct sync_ro_regs {
+	__u32 prefix;	/* prefix register */
 };
 
 struct sync_rw_regs {
arch/s390/kvm/kvm-s390.c +2 -0
Index: 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_SYNC_REGS:
 		r = 1;
 		break;
 	default:

@@ -556,6 +557,7 @@ rerun_vcpu:
 
 	kvm_run->psw_mask     = vcpu->arch.sie_block->gpsw.mask;
 	kvm_run->psw_addr     = vcpu->arch.sie_block->gpsw.addr;
+	kvm_run->sync_ro_regs.prefix = vcpu->arch.sie_block->prefix;
 
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
author
#5 lore raw

PATCH 3/3 kvm-s390: provide general purpose registers via kvm_run

In reply to Christian Borntraeger

From: Christian Borntraeger <[email protected]>

The general purpose registers are often necessary to handle SIE exits. Avoid additional ioctls by providing the guest registers in the r/w section of the kvm_run structure.

Signed-off-by Christian Borntraeger <[email protected]>
---
 arch/s390/include/asm/kvm.h      |    1 +
 arch/s390/include/asm/kvm_host.h |    2 +-
 arch/s390/kvm/diag.c             |    6 +++---
 arch/s390/kvm/intercept.c        |    4 ++--
 arch/s390/kvm/kvm-s390.c         |   16 ++++++++--------
 arch/s390/kvm/priv.c             |   24 ++++++++++++------------
 arch/s390/kvm/sigp.c             |   14 +++++++-------
 7 files changed, 34 insertions(+), 33 deletions(-)

==================================================================

arch/s390/include/asm/kvm.h +1 -0
Index: b/arch/s390/include/asm/kvm.h
===================================================================
@@ -47,5 +47,6 @@ struct sync_ro_regs {
 };
 
 struct sync_rw_regs {
+	__u64 gprs[16];	/* general purpose registers */
 };
 #endif
arch/s390/include/asm/kvm_host.h +1 -1
Index: b/arch/s390/include/asm/kvm_host.h
===================================================================
@@ -253,5 +253,5 @@ struct kvm_arch{
 	struct gmap *gmap;
 };
 
-extern int sie64a(struct kvm_s390_sie_block *, unsigned long *);
+extern int sie64a(struct kvm_s390_sie_block *, u64 *);
 #endif
arch/s390/kvm/diag.c +3 -3
Index: b/arch/s390/kvm/diag.c
===================================================================
@@ -20,8 +20,8 @@ static int diag_release_pages(struct kvm
 	unsigned long start, end;
 	unsigned long prefix  = vcpu->arch.sie_block->prefix;
 
-	start = vcpu->arch.guest_gprs[(vcpu->arch.sie_block->ipa & 0xf0) >> 4];
-	end = vcpu->arch.guest_gprs[vcpu->arch.sie_block->ipa & 0xf] + 4096;
+	start = vcpu->run->sync_rw_regs.gprs[(vcpu->arch.sie_block->ipa & 0xf0) >> 4];
+	end = vcpu->run->sync_rw_regs.gprs[vcpu->arch.sie_block->ipa & 0xf] + 4096;
 
 	if (start & ~PAGE_MASK || end & ~PAGE_MASK || start > end
 	    || start < 2 * PAGE_SIZE)

@@ -56,7 +56,7 @@ static int __diag_time_slice_end(struct 
 static int __diag_ipl_functions(struct kvm_vcpu *vcpu)
 {
 	unsigned int reg = vcpu->arch.sie_block->ipa & 0xf;
-	unsigned long subcode = vcpu->arch.guest_gprs[reg] & 0xffff;
+	unsigned long subcode = vcpu->run->sync_rw_regs.gprs[reg] & 0xffff;
 
 	VCPU_EVENT(vcpu, 5, "diag ipl functions, subcode %lx", subcode);
 	switch (subcode) {
arch/s390/kvm/intercept.c +2 -2
Index: b/arch/s390/kvm/intercept.c
===================================================================
@@ -36,7 +36,7 @@ static int handle_lctlg(struct kvm_vcpu 
 
 	useraddr = disp2;
 	if (base2)
-		useraddr += vcpu->arch.guest_gprs[base2];
+		useraddr += vcpu->run->sync_rw_regs.gprs[base2];
 
 	if (useraddr & 7)
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);

@@ -75,7 +75,7 @@ static int handle_lctl(struct kvm_vcpu *
 
 	useraddr = disp2;
 	if (base2)
-		useraddr += vcpu->arch.guest_gprs[base2];
+		useraddr += vcpu->run->sync_rw_regs.gprs[base2];
 
 	if (useraddr & 3)
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
arch/s390/kvm/kvm-s390.c +8 -8
Index: b/arch/s390/kvm/kvm-s390.c
===================================================================
@@ -389,13 +389,13 @@ static int kvm_arch_vcpu_ioctl_initial_r
 
 int kvm_arch_vcpu_ioctl_set_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
-	memcpy(&vcpu->arch.guest_gprs, &regs->gprs, sizeof(regs->gprs));
+	memcpy(&vcpu->run->sync_rw_regs.gprs, &regs->gprs, sizeof(regs->gprs));
 	return 0;
 }
 
 int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 {
-	memcpy(&regs->gprs, &vcpu->arch.guest_gprs, sizeof(regs->gprs));
+	memcpy(&regs->gprs, &vcpu->run->sync_rw_regs.gprs, sizeof(regs->gprs));
 	return 0;
 }
 

@@ -468,9 +468,9 @@ int kvm_arch_vcpu_ioctl_set_mpstate(stru
 	return -EINVAL; /* not implemented yet */
 }
 
-static void __vcpu_run(struct kvm_vcpu *vcpu)
+static void __vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 {
-	memcpy(&vcpu->arch.sie_block->gg14, &vcpu->arch.guest_gprs[14], 16);
+	memcpy(&vcpu->arch.sie_block->gg14, &kvm_run->sync_rw_regs.gprs[14], 16);
 
 	if (need_resched())
 		schedule();

@@ -486,7 +486,7 @@ static void __vcpu_run(struct kvm_vcpu *
 	local_irq_enable();
 	VCPU_EVENT(vcpu, 6, "entering sie flags %x",
 		   atomic_read(&vcpu->arch.sie_block->cpuflags));
-	if (sie64a(vcpu->arch.sie_block, vcpu->arch.guest_gprs)) {
+	if (sie64a(vcpu->arch.sie_block, kvm_run->sync_rw_regs.gprs)) {
 		VCPU_EVENT(vcpu, 3, "%s", "fault in sie instruction");
 		kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
 	}

@@ -496,7 +496,7 @@ static void __vcpu_run(struct kvm_vcpu *
 	kvm_guest_exit();
 	local_irq_enable();
 
-	memcpy(&vcpu->arch.guest_gprs[14], &vcpu->arch.sie_block->gg14, 16);
+	memcpy(&kvm_run->sync_rw_regs.gprs[14], &vcpu->arch.sie_block->gg14, 16);
 }
 
 int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)

@@ -528,7 +528,7 @@ rerun_vcpu:
 	might_fault();
 
 	do {
-		__vcpu_run(vcpu);
+		__vcpu_run(vcpu, kvm_run);
 		rc = kvm_handle_sie_intercept(vcpu);
 	} while (!signal_pending(current) && !rc);
 

@@ -604,7 +604,7 @@ int kvm_s390_vcpu_store_status(struct kv
 		return -EFAULT;
 
 	if (__guestcopy(vcpu, addr + offsetof(struct save_area, gp_regs),
-			vcpu->arch.guest_gprs, 128, prefix))
+			vcpu->run->sync_rw_regs.gprs, 128, prefix))
 		return -EFAULT;
 
 	if (__guestcopy(vcpu, addr + offsetof(struct save_area, psw),
arch/s390/kvm/priv.c +12 -12
Index: b/arch/s390/kvm/priv.c
===================================================================
@@ -33,7 +33,7 @@ static int handle_set_prefix(struct kvm_
 
 	operand2 = disp2;
 	if (base2)
-		operand2 += vcpu->arch.guest_gprs[base2];
+		operand2 += vcpu->run->sync_rw_regs.gprs[base2];
 
 	/* must be word boundary */
 	if (operand2 & 3) {

@@ -74,7 +74,7 @@ static int handle_store_prefix(struct kv
 	vcpu->stat.instruction_stpx++;
 	operand2 = disp2;
 	if (base2)
-		operand2 += vcpu->arch.guest_gprs[base2];
+		operand2 += vcpu->run->sync_rw_regs.gprs[base2];
 
 	/* must be word boundary */
 	if (operand2 & 3) {

@@ -106,7 +106,7 @@ static int handle_store_cpu_address(stru
 	vcpu->stat.instruction_stap++;
 	useraddr = disp2;
 	if (base2)
-		useraddr += vcpu->arch.guest_gprs[base2];
+		useraddr += vcpu->run->sync_rw_regs.gprs[base2];
 
 	if (useraddr & 1) {
 		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);

@@ -181,7 +181,7 @@ static int handle_stidp(struct kvm_vcpu 
 	vcpu->stat.instruction_stidp++;
 	operand2 = disp2;
 	if (base2)
-		operand2 += vcpu->arch.guest_gprs[base2];
+		operand2 += vcpu->run->sync_rw_regs.gprs[base2];
 
 	if (operand2 & 7) {
 		kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);

@@ -232,9 +232,9 @@ static void handle_stsi_3_2_2(struct kvm
 
 static int handle_stsi(struct kvm_vcpu *vcpu)
 {
-	int fc = (vcpu->arch.guest_gprs[0] & 0xf0000000) >> 28;
-	int sel1 = vcpu->arch.guest_gprs[0] & 0xff;
-	int sel2 = vcpu->arch.guest_gprs[1] & 0xffff;
+	int fc = (vcpu->run->sync_rw_regs.gprs[0] & 0xf0000000) >> 28;
+	int sel1 = vcpu->run->sync_rw_regs.gprs[0] & 0xff;
+	int sel2 = vcpu->run->sync_rw_regs.gprs[1] & 0xffff;
 	int base2 = vcpu->arch.sie_block->ipb >> 28;
 	int disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
 	u64 operand2;

@@ -245,14 +245,14 @@ static int handle_stsi(struct kvm_vcpu *
 
 	operand2 = disp2;
 	if (base2)
-		operand2 += vcpu->arch.guest_gprs[base2];
+		operand2 += vcpu->run->sync_rw_regs.gprs[base2];
 
 	if (operand2 & 0xfff && fc > 0)
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
 
 	switch (fc) {
 	case 0:
-		vcpu->arch.guest_gprs[0] = 3 << 28;
+		vcpu->run->sync_rw_regs.gprs[0] = 3 << 28;
 		vcpu->arch.sie_block->gpsw.mask &= ~(3ul << 44);
 		return 0;
 	case 1: /* same handling for 1 and 2 */

@@ -281,7 +281,7 @@ static int handle_stsi(struct kvm_vcpu *
 	}
 	free_page(mem);
 	vcpu->arch.sie_block->gpsw.mask &= ~(3ul << 44);
-	vcpu->arch.guest_gprs[0] = 0;
+	vcpu->run->sync_rw_regs.gprs[0] = 0;
 	return 0;
 out_mem:
 	free_page(mem);

@@ -333,8 +333,8 @@ static int handle_tprot(struct kvm_vcpu 
 	int disp1 = (vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16;
 	int base2 = (vcpu->arch.sie_block->ipb & 0xf000) >> 12;
 	int disp2 = vcpu->arch.sie_block->ipb & 0x0fff;
-	u64 address1 = disp1 + base1 ? vcpu->arch.guest_gprs[base1] : 0;
-	u64 address2 = disp2 + base2 ? vcpu->arch.guest_gprs[base2] : 0;
+	u64 address1 = disp1 + base1 ? vcpu->run->sync_rw_regs.gprs[base1] : 0;
+	u64 address2 = disp2 + base2 ? vcpu->run->sync_rw_regs.gprs[base2] : 0;
 	struct vm_area_struct *vma;
 	unsigned long user_address;
arch/s390/kvm/sigp.c +7 -7
Index: b/arch/s390/kvm/sigp.c
===================================================================
@@ -316,7 +316,7 @@ int kvm_s390_handle_sigp(struct kvm_vcpu
 	int base2 = vcpu->arch.sie_block->ipb >> 28;
 	int disp2 = ((vcpu->arch.sie_block->ipb & 0x0fff0000) >> 16);
 	u32 parameter;
-	u16 cpu_addr = vcpu->arch.guest_gprs[r3];
+	u16 cpu_addr = vcpu->run->sync_rw_regs.gprs[r3];
 	u8 order_code;
 	int rc;
 

@@ -327,18 +327,18 @@ int kvm_s390_handle_sigp(struct kvm_vcpu
 
 	order_code = disp2;
 	if (base2)
-		order_code += vcpu->arch.guest_gprs[base2];
+		order_code += vcpu->run->sync_rw_regs.gprs[base2];
 
 	if (r1 % 2)
-		parameter = vcpu->arch.guest_gprs[r1];
+		parameter = vcpu->run->sync_rw_regs.gprs[r1];
 	else
-		parameter = vcpu->arch.guest_gprs[r1 + 1];
+		parameter = vcpu->run->sync_rw_regs.gprs[r1 + 1];
 
 	switch (order_code) {
 	case SIGP_SENSE:
 		vcpu->stat.instruction_sigp_sense++;
 		rc = __sigp_sense(vcpu, cpu_addr,
-				  &vcpu->arch.guest_gprs[r1]);
+				  &vcpu->run->sync_rw_regs.gprs[r1]);
 		break;
 	case SIGP_EXTERNAL_CALL:
 		vcpu->stat.instruction_sigp_external_call++;

@@ -363,12 +363,12 @@ int kvm_s390_handle_sigp(struct kvm_vcpu
 	case SIGP_SET_PREFIX:
 		vcpu->stat.instruction_sigp_prefix++;
 		rc = __sigp_set_prefix(vcpu, cpu_addr, parameter,
-				       &vcpu->arch.guest_gprs[r1]);
+				       &vcpu->run->sync_rw_regs.gprs[r1]);
 		break;
 	case SIGP_SENSE_RUNNING:
 		vcpu->stat.instruction_sigp_sense_running++;
 		rc = __sigp_sense_running(vcpu, cpu_addr,
-					  &vcpu->arch.guest_gprs[r1]);
+					  &vcpu->run->sync_rw_regs.gprs[r1]);
 		break;
 	case SIGP_RESTART:
 		vcpu->stat.instruction_sigp_restart++;
#6 lore raw

Re: [patch 3/3] kvm-s390: provide general purpose registers via kvm_run

Quoted 6 lines · Christian Borntraeger

From: Christian Borntraeger <[email protected]>

The general purpose registers are often necessary to handle SIE exits. Avoid additional ioctls by providing the guest registers in the r/w section of the kvm_run structure.

This is only needed for S390_UCONTROL?

arch/s390/include/asm/kvm.h struct sync_ro_regs { · line 52

Quoted 5 lines

struct sync_rw_regs {

arch/s390/include/asm/kvm.h +1 -0
+	__u64 gprs[16];	/* general purpose registers */
 };
 #endif

Don't you have to remove arch.guest_gprs too? (interesting approach btw).

arch/s390/kvm/kvm-s390.c rerun_vcpu: · line 531

Quoted 8 lines

int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)

arch/s390/kvm/kvm-s390.c +1 -1
@@ -528,7 +528,7 @@ rerun_vcpu:
 	might_fault();
 
 	do {
-		__vcpu_run(vcpu);
+		__vcpu_run(vcpu, kvm_run);

kvm_run == vcpu->run, no need for the extra param.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#7 lore raw

Re: [patch 3/3] kvm-s390: provide general purpose registers via kvm_run

Quoted 6 lines · Avi Kivity
Quoted 4 lines

The general purpose registers are often necessary to handle SIE exits. Avoid additional ioctls by providing the guest registers in the r/w section of the kvm_run structure.

This is only needed for S390_UCONTROL?

No for the standard path. It was more like a "we copy the regs around anyway inside the kernel, so why not use kvm_run as a place to store the guest regs". So I will probably also have a look at floating point regs and access registers.

Quoted 8 lines

arch/s390/include/asm/kvm.h struct sync_ro_regs { · line 52

Quoted 6 lines

struct sync_rw_regs {

arch/s390/include/asm/kvm.h +1 -0
+	__u64 gprs[16];	/* general purpose registers */
 };
 #endif

Don't you have to remove arch.guest_gprs too? (interesting approach btw).

Yes. Done

#8 lore raw

Re: [patch 3/3] kvm-s390: provide general purpose registers via kvm_run

Quoted 6 lines · Christian Borntraeger
Quoted 5 lines · Avi Kivity
Quoted 4 lines

The general purpose registers are often necessary to handle SIE exits. Avoid additional ioctls by providing the guest registers in the r/w section of the kvm_run structure.

Quoted 6 lines
Quoted 1 line

This is only needed for S390_UCONTROL?

No for the standard path. It was more like a "we copy the regs around anyway inside the kernel, so why not use kvm_run as a place to store the guest regs". So I will probably also have a look at floating point regs and access registers.

Sure, for side effect free registers that's a good approach. I'd copy it for x86, except vmx keeps %rsp cached in the cpu and won't let it out except via an expensive instruction, and also because it's very rarely used (mostly by "info registers" in qemu and live migration).

Signature · 1 line
error compiling committee.c: too many arguments to function
#9 lore raw

Re: [patch 3/3] kvm-s390: provide general purpose registers via kvm_run

In reply to Christian Borntraeger

arch/s390/include/asm/kvm.h struct sync_ro_regs { · line 50

Quoted 10 lines · Christian Borntraeger

From: Christian Borntraeger <[email protected]>

The general purpose registers are often necessary to handle SIE exits. Avoid additional ioctls by providing the guest registers in the r/w section of the kvm_run structure.

Signed-off-by Christian Borntraeger <[email protected]>

---
struct sync_rw_regs {

arch/s390/include/asm/kvm.h +1 -0
+	__u64 gprs[16];	/* general purpose registers */

FWIW, you should be able to get rid of guest_gprs[] from the kvm_vcpu_arch structure.

Quoted 2 lines

Index: b/arch/s390/include/asm/kvm_host.h
===================================================================

Btw. you may want to set QUILT_NO_DIFF_INDEX to get rid of the Index lines junk :)

#10 lore raw

Re: [patch 3/3] kvm-s390: provide general purpose registers via kvm_run

Quoted 5 lines · Heiko Carstens
Quoted 2 lines

Index: b/arch/s390/include/asm/kvm_host.h
===================================================================

Btw. you may want to set QUILT_NO_DIFF_INDEX to get rid of the Index lines junk :)

Or just use git, when I switched some years ago I found I didn't miss anything in quilt.

Signature · 1 line
error compiling committee.c: too many arguments to function
#11 lore raw
In reply to Christian Borntraeger
Quoted 9 lines · Christian Borntraeger

Avi, Marcelo,

currently userspace can access guest registers via several ioctls. Some of these registers might be useful very often. Here the system call overhead for ioctl can make an exit more expensive than necessary.
In a discussion with Alex Graf we concluded that it might be beneficial to have a subset of registers available in kvm_run. (The ioctls will also be available).

Quoted 4 lines

This series provides a prototype implementation together with two example users for s390.

Interesting. Other archs emulate everything to do with registers in the kernel, so this is not a fast path.

What workload does this benefit?

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#12 lore raw
Quoted 15 lines · Avi Kivity
Quoted 9 lines · Christian Borntraeger

Avi, Marcelo,

currently userspace can access guest registers via several ioctls. Some of these registers might be useful very often. Here the system call overhead for ioctl can make an exit more expensive than necessary.
In a discussion with Alex Graf we concluded that it might be beneficial to have a subset of registers available in kvm_run. (The ioctls will also be available).

Quoted 4 lines

This series provides a prototype implementation together with two example users for s390.

Quoted 4 lines

Interesting. Other archs emulate everything to do with registers in the kernel, so this is not a fast path.

What workload does this benefit?

My main concern was the prefix register (this is a per cpu register that defines the address of two pages that are swapped with the pages at 0 for this cpu). SMP on s390 is done that way (e.g. interrupt things are stored in page 0 for this cpu) The storage that qemu sees is storage without prefix. For architecture compliance we actually must check _every_ memory access if it hits the prefix/swpa area and the add/subtract the prefix value.

I just added the ability to share other registers later after some discussions with Alexander Graf because it seems to be doable for almost no cost.

Christian

#13 lore raw
Quoted 11 lines · Christian Borntraeger
Quoted 4 lines

Interesting. Other archs emulate everything to do with registers in the kernel, so this is not a fast path.

What workload does this benefit?

My main concern was the prefix register (this is a per cpu register that defines the address of two pages that are swapped with the pages at 0 for this cpu). SMP on s390 is done that way (e.g. interrupt things are stored in page 0 for this cpu) The storage that qemu sees is storage without prefix. For architecture compliance we actually must check _every_ memory access if it hits the prefix/swpa area and the add/subtract the prefix value.

Those are only memory accesses coming from the cpu, yes? Why does userspace have to access them at all? I imagine DMA ignores it completely since it doesn't come from the cpu.

Quoted 2 lines

I just added the ability to share other registers later after some discussions with Alexander Graf because it seems to be doable for almost no cost.

I doubt other archs will benefit, but it makes sense to reserve the space generically as you did.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#14 lore raw
Quoted 10 lines · Avi Kivity
Quoted 6 lines

My main concern was the prefix register (this is a per cpu register that defines the address of two pages that are swapped with the pages at 0 for this cpu). SMP on s390 is done that way (e.g. interrupt things are stored in page 0 for this cpu) The storage that qemu sees is storage without prefix. For architecture compliance we actually must check _every_ memory access if it hits the prefix/swpa area and the add/subtract the prefix value.

Those are only memory accesses coming from the cpu, yes? Why does userspace have to access them at all? I imagine DMA ignores it completely since it doesn't come from the cpu.

Not sure if I got you question...(just ask again if that doesnt aswer it)

The prefix page contains HW-defined content (like the PSWs for the different interrupt types) as well as some OS-defined values (for CPU local data structures as well as a place to store information in critical sections)
The prefix page (and the swap area) must not be used for device I/O (since it will be broken as you pointed out), but some I/O instructions can and will write status information to the prefix page. For example the channel subsystem driver in Linux will use an area in the prefix page as a store address for some instructions.

So let me phrase the above sentence differently:
For architecture compliance we actually must check every memory access that is done on behalf of a guest cpu and was not already handled by the host kernel.

PS: Most of the things are really handled in the kernel. As you can see, the current paravirtual I/O stack does not need the prefix at all

#15 lore raw
Quoted 22 lines · Christian Borntraeger
Quoted 10 lines · Avi Kivity
Quoted 6 lines

My main concern was the prefix register (this is a per cpu register that defines the address of two pages that are swapped with the pages at 0 for this cpu). SMP on s390 is done that way (e.g. interrupt things are stored in page 0 for this cpu) The storage that qemu sees is storage without prefix. For architecture compliance we actually must check _every_ memory access if it hits the prefix/swpa area and the add/subtract the prefix value.

Those are only memory accesses coming from the cpu, yes? Why does userspace have to access them at all? I imagine DMA ignores it completely since it doesn't come from the cpu.

Not sure if I got you question...(just ask again if that doesnt aswer it)

The prefix page contains HW-defined content (like the PSWs for the different interrupt types) as well as some OS-defined values (for CPU local data structures as well as a place to store information in critical sections)
The prefix page (and the swap area) must not be used for device I/O (since it will be broken as you pointed out), but some I/O instructions can and will write status information to the prefix page. For example the channel subsystem driver in Linux will use an area in the prefix page as a store address for some instructions.

Quoted 3 lines

So let me phrase the above sentence differently:
For architecture compliance we actually must check every memory access that is done on behalf of a guest cpu and was not already handled by the host kernel.

"on behalf of the guest cpu" is the key phrase. This never happens in userspace for x86, but I guess that s390 is different here. Thanks for the clarification.

Signature · 1 line
error compiling committee.c: too many arguments to function

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

#18 lore raw

Re: [PATCH 3/3] kvm-s390: provide standard guest registers via kvm_run

Review

Quoted 4 lines · Christian Borntraeger

struct kvm_sync_rw_regs {

(quoted diff) +3 -0
+	__u64          gprs[16];	/* general purpose registers */
+	s390_fp_regs   fpregs;
+	unsigned int   acrs[16];

So, if I got this right, userspace is allowed to map this shared read/write with the kernel? If that's the case then...

arch/s390/kvm/kvm-s390.c void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) · line 273

Quoted 13 lines
arch/s390/kvm/kvm-s390.c +2 -3
index 0badc9f..a7b7ae0 100644
@@ -269,9 +269,9 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 {
 	save_fp_regs(&vcpu->arch.host_fpregs);
 	save_access_regs(vcpu->arch.host_acrs);
-	vcpu->arch.guest_fpregs.fpc &= FPC_VALID_MASK;
-	restore_fp_regs(&vcpu->arch.guest_fpregs);
-	restore_access_regs(vcpu->arch.guest_acrs);
+	vcpu->run->sync_rw.fpregs.fpc &= FPC_VALID_MASK;
+	restore_fp_regs(&vcpu->run->sync_rw.fpregs);

...this is broken, since userspace can update the floating point control register contents after the kernel has masked out the offending bits but before the register is actually loaded.
Which in turn could cause a kernel crash, hm?

author
#19 lore raw

Re: [PATCH 3/3] kvm-s390: provide standard guest registers via kvm_run

Quoted 4 lines · Heiko Carstens
Quoted 4 lines
(quoted diff) +0 -2
 	save_fp_regs(&vcpu->arch.host_fpregs);
 	save_access_regs(vcpu->arch.host_acrs);
-	vcpu->arch.guest_fpregs.fpc &= FPC_VALID_MASK;
-	restore_fp_regs(&vcpu->arch.guest_fpregs);
Quoted 8 lines

Review

Quoted 3 lines
(quoted diff) +2 -1
-	restore_access_regs(vcpu->arch.guest_acrs);
+	vcpu->run->sync_rw.fpregs.fpc &= FPC_VALID_MASK;
+	restore_fp_regs(&vcpu->run->sync_rw.fpregs);

...this is broken, since userspace can update the floating point control register contents after the kernel has masked out the offending bits but before the register is actually loaded.
Which in turn could cause a kernel crash, hm?

Indeed, I need to fix that. Thanks for spotting.
I will respin patch 2 and 3 as soon as the interface in patch 1 is settled.

Christian