[patch 0/3] RFC: provide synchronous registers in kvm_run

[patch 0/3] RFC: provide synchronous registers in kvm_run

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

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

Opionions?

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

[Christian Borntraeger]

[-- Attachment #1: kvm_run_sync_regs.patch --]
[-- Type: text/plain, Size: 4397 bytes --]

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

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 <borntraeger@de.ibm.com>
---
 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
===================================================================
--- a/Documentation/virtual/kvm/api.txt
+++ 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
 
Index: b/arch/ia64/include/asm/kvm.h
===================================================================
--- a/arch/ia64/include/asm/kvm.h
+++ 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
Index: b/arch/powerpc/include/asm/kvm.h
===================================================================
--- a/arch/powerpc/include/asm/kvm.h
+++ 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
Index: b/arch/s390/include/asm/kvm.h
===================================================================
--- a/arch/s390/include/asm/kvm.h
+++ 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
Index: b/arch/x86/include/asm/kvm.h
===================================================================
--- a/arch/x86/include/asm/kvm.h
+++ 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 */
Index: b/include/linux/kvm.h
===================================================================
--- a/include/linux/kvm.h
+++ 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
 

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

[Avi Kivity]

On 12/22/2011 01:56 PM, Christian Borntraeger wrote:
> From: Christian Borntraeger <borntraeger@de.ibm.com>
>
> 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?

>  	};
> +	/* 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_

> +		char padding[1024];
> +	};
> +	union {
> +		/* read/write guest registers */
> +		struct sync_rw_regs sync_rw_regs;

kvm_

> +		char padding[1024];
> +	};
>  };

blank line

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


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

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

[Alexander Graf]

On 22.12.2011, at 12:56, Christian Borntraeger wrote:

> From: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 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 <borntraeger@de.ibm.com>
> ---
> 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
> ===================================================================
> --- a/Documentation/virtual/kvm/api.txt
> +++ 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];
> +	};

Why have 2 different structs?

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


Alex

[patch 2/3] kvm-s390: provide the prefix register via kvm_run

[Christian Borntraeger]

[-- Attachment #1: kvm_run_prefix.patch --]
[-- Type: text/plain, Size: 1448 bytes --]

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

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 <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/kvm.h |    1 +
 arch/s390/kvm/kvm-s390.c    |    2 ++
 2 files changed, 3 insertions(+)

Index: b/arch/s390/include/asm/kvm.h
===================================================================
--- a/arch/s390/include/asm/kvm.h
+++ 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 {
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_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);

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

[Christian Borntraeger]

[-- Attachment #1: kvm_run_gprs.patch --]
[-- Type: text/plain, Size: 10586 bytes --]

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

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 <borntraeger@de.ibm.com>
---
 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(-)

==================================================================
Index: b/arch/s390/include/asm/kvm.h
===================================================================
--- a/arch/s390/include/asm/kvm.h
+++ 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
Index: b/arch/s390/include/asm/kvm_host.h
===================================================================
--- a/arch/s390/include/asm/kvm_host.h
+++ 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
Index: b/arch/s390/kvm/diag.c
===================================================================
--- a/arch/s390/kvm/diag.c
+++ 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) {
Index: b/arch/s390/kvm/intercept.c
===================================================================
--- a/arch/s390/kvm/intercept.c
+++ 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);
Index: b/arch/s390/kvm/kvm-s390.c
===================================================================
--- a/arch/s390/kvm/kvm-s390.c
+++ 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),
Index: b/arch/s390/kvm/priv.c
===================================================================
--- a/arch/s390/kvm/priv.c
+++ 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;
 
Index: b/arch/s390/kvm/sigp.c
===================================================================
--- a/arch/s390/kvm/sigp.c
+++ 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++;

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

[Avi Kivity]

On 12/22/2011 01:56 PM, Christian Borntraeger wrote:
> From: Christian Borntraeger <borntraeger@de.ibm.com>
>
> 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?

>  
>  struct sync_rw_regs {
> +	__u64 gprs[16];	/* general purpose registers */
>  };
>  #endif

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

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

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

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

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

[Christian Borntraeger]

On 22/12/11 13:34, Avi Kivity wrote:
>> 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.

> 
>>  
>>  struct sync_rw_regs {
>> +	__u64 gprs[16];	/* general purpose registers */
>>  };
>>  #endif
> 
> Don't you have to remove arch.guest_gprs too? (interesting approach btw).

Yes. Done

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

[Avi Kivity]

On 12/22/2011 02:39 PM, Christian Borntraeger wrote:
> On 22/12/11 13:34, Avi Kivity wrote:
> >> 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.

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

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

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

[Heiko Carstens]

On Thu, Dec 22, 2011 at 12:56:49PM +0100, Christian Borntraeger wrote:
> From: Christian Borntraeger <borntraeger@de.ibm.com>
> 
> 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 <borntraeger@de.ibm.com>
> ---
>  struct sync_rw_regs {
> +	__u64 gprs[16];	/* general purpose registers */

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

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

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

[Avi Kivity]

On 12/22/2011 02:41 PM, Heiko Carstens wrote:
> > 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.

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

Re: [patch 0/3] RFC: provide synchronous registers in kvm_run

[Avi Kivity]

On 12/22/2011 01:56 PM, Christian Borntraeger wrote:
> 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.
>
>

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

What workload does this benefit?

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

Re: [patch 0/3] RFC: provide synchronous registers in kvm_run

[Christian Borntraeger]

On 22/12/11 13:35, Avi Kivity wrote:
> On 12/22/2011 01:56 PM, Christian Borntraeger wrote:
>> 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.
>>
>>
> 
> 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

Re: [patch 0/3] RFC: provide synchronous registers in kvm_run

[Avi Kivity]

On 12/22/2011 02:49 PM, Christian Borntraeger wrote:
> > 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.

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

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

Re: [patch 0/3] RFC: provide synchronous registers in kvm_run

[Christian Borntraeger]

On 22/12/11 13:54, Avi Kivity wrote:
>> 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

Re: [patch 0/3] RFC: provide synchronous registers in kvm_run

[Avi Kivity]

On 12/22/2011 03:17 PM, Christian Borntraeger wrote:
> On 22/12/11 13:54, Avi Kivity wrote:
> >> 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.

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

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

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

[Heiko Carstens]

On Tue, Jan 03, 2012 at 04:50:58PM +0100, Christian Borntraeger wrote:
>  struct kvm_sync_rw_regs {
> +	__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...

> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 0badc9f..a7b7ae0 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -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?

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

[Christian Borntraeger]

On 04/01/12 09:16, Heiko Carstens wrote:
>>  	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?

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