synchronous register in kvm_run

synchronous register in kvm_run

[Christian Borntraeger]

Avi, Marcelo,

here is the latest update of my patch set about guest registers in kvm_run.
I have added an u64 flag field to make the whole structure extensible
(and shrinkable).
For s390 I added the prefix register as read-only and access and general
purpose as read write.

[PATCH 1/4] kvm: provide synchronous registers in kvm_run

[Christian Borntraeger]

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 certain 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.
We also provide two 64bit flags fields (architecture specific), that will
specify which parts of these fields are valid. Each bit will define that
a group of registers (like general purpose) or a single register is valid.
In that way we can extend and shrink the interface. (The structure definition
itself can only grow of course).

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 Documentation/virtual/kvm/api.txt |   28 ++++++++++++++++++++++++++++
 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               |   20 ++++++++++++++++++++
 6 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index da1f8fd..a149e22 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1693,6 +1693,34 @@ developer registration required to access it).
 		/* 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. Each
+         * register can be extended or reduced by having flag bits specifying
+         * if a group of registers is valid.
+         */
+	__u64 kvm_valid_sync_ro_regs;
+	__u64 kvm_valid_sync_rw_regs;
+	union {
+		/* registers which can be only read */
+		struct kvm_sync_ro_regs sync_ro;
+		char padding[1024];
+	};
+	union {
+		/* read/write guest registers */
+		struct kvm_sync_rw_regs sync_rw;
+		char padding[1024];
+	};
+
+If KVM_CAP_SYNC_REGS is defined, 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.
+Userspace can query the validity of the structure by checking
+kvm_valid_sync_r[wo]_regs for specific bits. These bits are architecture
+specific and usually define the validity of a groups of registers. (e.g.
+one bit for general purpose registers)
+
 };
 
 6. Capabilities that can be enabled
diff --git a/arch/ia64/include/asm/kvm.h b/arch/ia64/include/asm/kvm.h
index bc90c75..bae1f1e 100644
--- 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 kvm_sync_ro_regs {
+};
+
+struct kvm_sync_rw_regs {
+};
+
 #endif
diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
index f7727d9..14b766b 100644
--- 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 kvm_sync_ro_regs {
+};
+
+struct kvm_sync_rw_regs {
+};
+
 #define KVM_REG_MASK		0x001f
 #define KVM_REG_EXT_MASK	0xffe0
 #define KVM_REG_GPR		0x0000
diff --git a/arch/s390/include/asm/kvm.h b/arch/s390/include/asm/kvm.h
index 82b32a1..dda27a0 100644
--- 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 kvm_sync_ro_regs {
+};
+
+struct kvm_sync_rw_regs {
+};
 #endif
diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
index 4d8dcbd..fb32847 100644
--- 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 kvm_sync_ro_regs {
+};
+
+struct kvm_sync_rw_regs {
+};
+
 #endif /* _ASM_X86_KVM_H */
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 68e67e5..9398672 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -273,6 +273,25 @@ 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. Each
+	 * register can be extended or reduced by having flag bits specifying
+	 * if a group of registers is valid.
+	 */
+	__u64 kvm_valid_sync_ro_regs;
+	__u64 kvm_valid_sync_rw_regs;
+	union {
+		/* registers which can be only read */
+		struct kvm_sync_ro_regs sync_ro;
+		char padding[1024];
+	};
+	union {
+		/* read/write guest registers */
+		struct kvm_sync_rw_regs sync_rw;
+		char padding[1024];
+	};
 };
 
 /* for KVM_REGISTER_COALESCED_MMIO / KVM_UNREGISTER_COALESCED_MMIO */
@@ -558,6 +577,7 @@ struct kvm_ppc_pvinfo {
 #define KVM_CAP_PPC_PAPR 68
 #define KVM_CAP_S390_GMAP 71
 #define KVM_CAP_TSC_DEADLINE_TIMER 72
+#define KVM_CAP_SYNC_REGS 73
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
1.7.8.2

Re: [PATCH 1/4] kvm: provide synchronous registers in kvm_run

[Alexander Graf]

On 05.01.2012, at 10:54, Christian Borntraeger wrote:

> 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 certain 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.
> We also provide two 64bit flags fields (architecture specific), that will
> specify which parts of these fields are valid. Each bit will define that
> a group of registers (like general purpose) or a single register is valid.
> In that way we can extend and shrink the interface. (The structure definition
> itself can only grow of course).
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> Documentation/virtual/kvm/api.txt |   28 ++++++++++++++++++++++++++++
> 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               |   20 ++++++++++++++++++++
> 6 files changed, 75 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index da1f8fd..a149e22 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1693,6 +1693,34 @@ developer registration required to access it).
> 		/* 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. Each
> +         * register can be extended or reduced by having flag bits specifying
> +         * if a group of registers is valid.
> +         */
> +	__u64 kvm_valid_sync_ro_regs;
> +	__u64 kvm_valid_sync_rw_regs;
> +	union {
> +		/* registers which can be only read */
> +		struct kvm_sync_ro_regs sync_ro;
> +		char padding[1024];
> +	};
> +	union {
> +		/* read/write guest registers */
> +		struct kvm_sync_rw_regs sync_rw;
> +		char padding[1024];
> +	};

Ah, new patch set. Same comment here. Please give the union a name. Also, I still don't understand why we need 2 structs. All we need is a bitmap that says "this field is dirty" and then kernel space can decide to ignore it.


Alex

Re: [PATCH 1/4] kvm: provide synchronous registers in kvm_run

[Avi Kivity]

On 01/09/2012 03:01 PM, Alexander Graf wrote:
> On 05.01.2012, at 10:54, Christian Borntraeger wrote:
>
> > 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 certain 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.
> > We also provide two 64bit flags fields (architecture specific), that will
> > specify which parts of these fields are valid. Each bit will define that
> > a group of registers (like general purpose) or a single register is valid.
> > In that way we can extend and shrink the interface. (The structure definition
> > itself can only grow of course).
> > 
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> > Documentation/virtual/kvm/api.txt |   28 ++++++++++++++++++++++++++++
> > 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               |   20 ++++++++++++++++++++
> > 6 files changed, 75 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> > index da1f8fd..a149e22 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -1693,6 +1693,34 @@ developer registration required to access it).
> > 		/* 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. Each
> > +         * register can be extended or reduced by having flag bits specifying
> > +         * if a group of registers is valid.
> > +         */
> > +	__u64 kvm_valid_sync_ro_regs;
> > +	__u64 kvm_valid_sync_rw_regs;
> > +	union {
> > +		/* registers which can be only read */
> > +		struct kvm_sync_ro_regs sync_ro;
> > +		char padding[1024];
> > +	};
> > +	union {
> > +		/* read/write guest registers */
> > +		struct kvm_sync_rw_regs sync_rw;
> > +		char padding[1024];
> > +	};
>
> Ah, new patch set. Same comment here. Please give the union a name. Also, I still don't understand why we need 2 structs. 

Right.

> All we need is a bitmap that says "this field is dirty" and then kernel space can decide to ignore it.

Makes sense.

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

[PATCH 2/4] kvm-s390: provide the prefix register via kvm_run

[Christian Borntraeger]

The prefix register is a read-mostly value that is necessary to emulate
memory accesses on behalf of the guest cpu 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 |    2 ++
 arch/s390/kvm/kvm-s390.c    |    3 +++
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/s390/include/asm/kvm.h b/arch/s390/include/asm/kvm.h
index dda27a0..b48edc3 100644
--- a/arch/s390/include/asm/kvm.h
+++ b/arch/s390/include/asm/kvm.h
@@ -41,8 +41,10 @@ struct kvm_debug_exit_arch {
 struct kvm_guest_debug_arch {
 };
 
+#define KVM_SYNC_RO_PREFIX (1UL << 0)
 /* definition of registers in kvm_run */
 struct kvm_sync_ro_regs {
+	__u32 prefix;	/* prefix register */
 };
 
 struct kvm_sync_rw_regs {
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index d1c44573..1b48a73 100644
--- 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 ext)
 	case KVM_CAP_S390_PSW:
 	case KVM_CAP_S390_GMAP:
 	case KVM_CAP_SYNC_MMU:
+	case KVM_CAP_SYNC_REGS:
 		r = 1;
 		break;
 	default:
@@ -256,6 +257,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.gmap = vcpu->kvm->arch.gmap;
+	vcpu->run->kvm_valid_sync_ro_regs = KVM_SYNC_RO_PREFIX;
 	return 0;
 }
 
@@ -556,6 +558,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.prefix = vcpu->arch.sie_block->prefix;
 
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
-- 
1.7.8.2

Re: [PATCH 2/4] kvm-s390: provide the prefix register via kvm_run

[Alexander Graf]

On 05.01.2012, at 10:54, Christian Borntraeger wrote:

> The prefix register is a read-mostly value that is necessary to emulate
> memory accesses on behalf of the guest cpu 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 |    2 ++
> arch/s390/kvm/kvm-s390.c    |    3 +++
> 2 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm.h b/arch/s390/include/asm/kvm.h
> index dda27a0..b48edc3 100644
> --- a/arch/s390/include/asm/kvm.h
> +++ b/arch/s390/include/asm/kvm.h
> @@ -41,8 +41,10 @@ struct kvm_debug_exit_arch {
> struct kvm_guest_debug_arch {
> };
> 
> +#define KVM_SYNC_RO_PREFIX (1UL << 0)
> /* definition of registers in kvm_run */
> struct kvm_sync_ro_regs {
> +	__u32 prefix;	/* prefix register */

Yes, the prefix register is 32 bits today, but it doesn't have to stay that way. Why not just make it a u64?


Alex

[PATCH 3/4] kvm-s390: provide general purpose guest registers via kvm_run

[Christian Borntraeger]

This patch adds the general purpose registers to the kvm_run structure.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/kvm.h      |    2 ++
 arch/s390/include/asm/kvm_host.h |    3 +--
 arch/s390/kvm/diag.c             |    6 +++---
 arch/s390/kvm/intercept.c        |    4 ++--
 arch/s390/kvm/kvm-s390.c         |   13 +++++++------
 arch/s390/kvm/priv.c             |   24 ++++++++++++------------
 arch/s390/kvm/sigp.c             |   20 ++++++++++----------
 7 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/arch/s390/include/asm/kvm.h b/arch/s390/include/asm/kvm.h
index b48edc3..5dfe143b29 100644
--- a/arch/s390/include/asm/kvm.h
+++ b/arch/s390/include/asm/kvm.h
@@ -47,6 +47,8 @@ struct kvm_sync_ro_regs {
 	__u32 prefix;	/* prefix register */
 };
 
+#define KVM_SYNC_RW_GPRS   (1UL << 0)
 struct kvm_sync_rw_regs {
+	__u64          gprs[16];	/* general purpose registers */
 };
 #endif
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index b0c235c..173f44d 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -227,7 +227,6 @@ struct kvm_s390_float_interrupt {
 
 struct kvm_vcpu_arch {
 	struct kvm_s390_sie_block *sie_block;
-	unsigned long	  guest_gprs[16];
 	s390_fp_regs      host_fpregs;
 	unsigned int      host_acrs[NUM_ACRS];
 	s390_fp_regs      guest_fpregs;
@@ -253,5 +252,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
diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
index 8943e82..9a1ef76 100644
--- a/arch/s390/kvm/diag.c
+++ b/arch/s390/kvm/diag.c
@@ -20,8 +20,8 @@ static int diag_release_pages(struct kvm_vcpu *vcpu)
 	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.gprs[(vcpu->arch.sie_block->ipa & 0xf0) >> 4];
+	end = vcpu->run->sync_rw.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 kvm_vcpu *vcpu)
 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.gprs[reg] & 0xffff;
 
 	VCPU_EVENT(vcpu, 5, "diag ipl functions, subcode %lx", subcode);
 	switch (subcode) {
diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 0243454..b93a908 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -36,7 +36,7 @@ static int handle_lctlg(struct kvm_vcpu *vcpu)
 
 	useraddr = disp2;
 	if (base2)
-		useraddr += vcpu->arch.guest_gprs[base2];
+		useraddr += vcpu->run->sync_rw.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 *vcpu)
 
 	useraddr = disp2;
 	if (base2)
-		useraddr += vcpu->arch.guest_gprs[base2];
+		useraddr += vcpu->run->sync_rw.gprs[base2];
 
 	if (useraddr & 3)
 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 1b48a73..0f9bbb2 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -258,6 +258,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.gmap = vcpu->kvm->arch.gmap;
 	vcpu->run->kvm_valid_sync_ro_regs = KVM_SYNC_RO_PREFIX;
+	vcpu->run->kvm_valid_sync_rw_regs = KVM_SYNC_RW_GPRS;
 	return 0;
 }
 
@@ -390,13 +391,13 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
 
 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.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.gprs, sizeof(regs->gprs));
 	return 0;
 }
 
@@ -471,7 +472,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 
 static void __vcpu_run(struct kvm_vcpu *vcpu)
 {
-	memcpy(&vcpu->arch.sie_block->gg14, &vcpu->arch.guest_gprs[14], 16);
+	memcpy(&vcpu->arch.sie_block->gg14, &vcpu->run->sync_rw.gprs[14], 16);
 
 	if (need_resched())
 		schedule();
@@ -487,7 +488,7 @@ static void __vcpu_run(struct kvm_vcpu *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, vcpu->run->sync_rw.gprs)) {
 		VCPU_EVENT(vcpu, 3, "%s", "fault in sie instruction");
 		kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
 	}
@@ -497,7 +498,7 @@ static void __vcpu_run(struct kvm_vcpu *vcpu)
 	kvm_guest_exit();
 	local_irq_enable();
 
-	memcpy(&vcpu->arch.guest_gprs[14], &vcpu->arch.sie_block->gg14, 16);
+	memcpy(&vcpu->run->sync_rw.gprs[14], &vcpu->arch.sie_block->gg14, 16);
 }
 
 int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
@@ -605,7 +606,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
 		return -EFAULT;
 
 	if (__guestcopy(vcpu, addr + offsetof(struct save_area, gp_regs),
-			vcpu->arch.guest_gprs, 128, prefix))
+			vcpu->run->sync_rw.gprs, 128, prefix))
 		return -EFAULT;
 
 	if (__guestcopy(vcpu, addr + offsetof(struct save_area, psw),
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
index d026389..738bc43 100644
--- a/arch/s390/kvm/priv.c
+++ b/arch/s390/kvm/priv.c
@@ -33,7 +33,7 @@ static int handle_set_prefix(struct kvm_vcpu *vcpu)
 
 	operand2 = disp2;
 	if (base2)
-		operand2 += vcpu->arch.guest_gprs[base2];
+		operand2 += vcpu->run->sync_rw.gprs[base2];
 
 	/* must be word boundary */
 	if (operand2 & 3) {
@@ -74,7 +74,7 @@ static int handle_store_prefix(struct kvm_vcpu *vcpu)
 	vcpu->stat.instruction_stpx++;
 	operand2 = disp2;
 	if (base2)
-		operand2 += vcpu->arch.guest_gprs[base2];
+		operand2 += vcpu->run->sync_rw.gprs[base2];
 
 	/* must be word boundary */
 	if (operand2 & 3) {
@@ -106,7 +106,7 @@ static int handle_store_cpu_address(struct kvm_vcpu *vcpu)
 	vcpu->stat.instruction_stap++;
 	useraddr = disp2;
 	if (base2)
-		useraddr += vcpu->arch.guest_gprs[base2];
+		useraddr += vcpu->run->sync_rw.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)
 	vcpu->stat.instruction_stidp++;
 	operand2 = disp2;
 	if (base2)
-		operand2 += vcpu->arch.guest_gprs[base2];
+		operand2 += vcpu->run->sync_rw.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_vcpu *vcpu, struct sysinfo_3_2_2 *mem)
 
 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.gprs[0] & 0xf0000000) >> 28;
+	int sel1 = vcpu->run->sync_rw.gprs[0] & 0xff;
+	int sel2 = vcpu->run->sync_rw.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 *vcpu)
 
 	operand2 = disp2;
 	if (base2)
-		operand2 += vcpu->arch.guest_gprs[base2];
+		operand2 += vcpu->run->sync_rw.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.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 *vcpu)
 	}
 	free_page(mem);
 	vcpu->arch.sie_block->gpsw.mask &= ~(3ul << 44);
-	vcpu->arch.guest_gprs[0] = 0;
+	vcpu->run->sync_rw.gprs[0] = 0;
 	return 0;
 out_mem:
 	free_page(mem);
@@ -333,8 +333,8 @@ static int handle_tprot(struct kvm_vcpu *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.gprs[base1] : 0;
+	u64 address2 = disp2 + base2 ? vcpu->run->sync_rw.gprs[base2] : 0;
 	struct vm_area_struct *vma;
 	unsigned long user_address;
 
diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
index 0a7941d..b7883ac 100644
--- a/arch/s390/kvm/sigp.c
+++ b/arch/s390/kvm/sigp.c
@@ -48,7 +48,7 @@
 
 
 static int __sigp_sense(struct kvm_vcpu *vcpu, u16 cpu_addr,
-			unsigned long *reg)
+			u64 *reg)
 {
 	struct kvm_s390_float_interrupt *fi = &vcpu->kvm->arch.float_int;
 	int rc;
@@ -220,7 +220,7 @@ static int __sigp_set_arch(struct kvm_vcpu *vcpu, u32 parameter)
 }
 
 static int __sigp_set_prefix(struct kvm_vcpu *vcpu, u16 cpu_addr, u32 address,
-			     unsigned long *reg)
+			     u64 *reg)
 {
 	struct kvm_s390_float_interrupt *fi = &vcpu->kvm->arch.float_int;
 	struct kvm_s390_local_interrupt *li = NULL;
@@ -278,7 +278,7 @@ out_fi:
 }
 
 static int __sigp_sense_running(struct kvm_vcpu *vcpu, u16 cpu_addr,
-				unsigned long *reg)
+				u64 *reg)
 {
 	int rc;
 	struct kvm_s390_float_interrupt *fi = &vcpu->kvm->arch.float_int;
@@ -316,7 +316,7 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *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.gprs[r3];
 	u8 order_code;
 	int rc;
 
@@ -327,18 +327,18 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu)
 
 	order_code = disp2;
 	if (base2)
-		order_code += vcpu->arch.guest_gprs[base2];
+		order_code += vcpu->run->sync_rw.gprs[base2];
 
 	if (r1 % 2)
-		parameter = vcpu->arch.guest_gprs[r1];
+		parameter = vcpu->run->sync_rw.gprs[r1];
 	else
-		parameter = vcpu->arch.guest_gprs[r1 + 1];
+		parameter = vcpu->run->sync_rw.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.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 *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.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.gprs[r1]);
 		break;
 	case SIGP_RESTART:
 		vcpu->stat.instruction_sigp_restart++;
-- 
1.7.8.2

Re: [PATCH 3/4] kvm-s390: provide general purpose guest registers via kvm_run

[Alexander Graf]

On 05.01.2012, at 10:54, Christian Borntraeger wrote:

> This patch adds the general purpose registers to the kvm_run structure.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> arch/s390/include/asm/kvm.h      |    2 ++
> arch/s390/include/asm/kvm_host.h |    3 +--
> arch/s390/kvm/diag.c             |    6 +++---
> arch/s390/kvm/intercept.c        |    4 ++--
> arch/s390/kvm/kvm-s390.c         |   13 +++++++------
> arch/s390/kvm/priv.c             |   24 ++++++++++++------------
> arch/s390/kvm/sigp.c             |   20 ++++++++++----------
> 7 files changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/s390/include/asm/kvm.h b/arch/s390/include/asm/kvm.h
> index b48edc3..5dfe143b29 100644
> --- a/arch/s390/include/asm/kvm.h
> +++ b/arch/s390/include/asm/kvm.h
> @@ -47,6 +47,8 @@ struct kvm_sync_ro_regs {
> 	__u32 prefix;	/* prefix register */
> };
> 
> +#define KVM_SYNC_RW_GPRS   (1UL << 0)
> struct kvm_sync_rw_regs {
> +	__u64          gprs[16];	/* general purpose registers */

I would recommend having a bitmap in parallel to the "these fields are available" one that says "these fields are dirty". It doesn't make too much sense for GPRs, since they're reread on every guest entry anyway, but might come in handy later for other registers.

> };
> #endif
> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
> index b0c235c..173f44d 100644
> --- a/arch/s390/include/asm/kvm_host.h
> +++ b/arch/s390/include/asm/kvm_host.h
> @@ -227,7 +227,6 @@ struct kvm_s390_float_interrupt {
> 
> struct kvm_vcpu_arch {
> 	struct kvm_s390_sie_block *sie_block;
> -	unsigned long	  guest_gprs[16];
> 	s390_fp_regs      host_fpregs;
> 	unsigned int      host_acrs[NUM_ACRS];
> 	s390_fp_regs      guest_fpregs;
> @@ -253,5 +252,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
> diff --git a/arch/s390/kvm/diag.c b/arch/s390/kvm/diag.c
> index 8943e82..9a1ef76 100644
> --- a/arch/s390/kvm/diag.c
> +++ b/arch/s390/kvm/diag.c
> @@ -20,8 +20,8 @@ static int diag_release_pages(struct kvm_vcpu *vcpu)
> 	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.gprs[(vcpu->arch.sie_block->ipa & 0xf0) >> 4];
> +	end = vcpu->run->sync_rw.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 kvm_vcpu *vcpu)
> 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.gprs[reg] & 0xffff;
> 
> 	VCPU_EVENT(vcpu, 5, "diag ipl functions, subcode %lx", subcode);
> 	switch (subcode) {
> diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
> index 0243454..b93a908 100644
> --- a/arch/s390/kvm/intercept.c
> +++ b/arch/s390/kvm/intercept.c
> @@ -36,7 +36,7 @@ static int handle_lctlg(struct kvm_vcpu *vcpu)
> 
> 	useraddr = disp2;
> 	if (base2)
> -		useraddr += vcpu->arch.guest_gprs[base2];
> +		useraddr += vcpu->run->sync_rw.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 *vcpu)
> 
> 	useraddr = disp2;
> 	if (base2)
> -		useraddr += vcpu->arch.guest_gprs[base2];
> +		useraddr += vcpu->run->sync_rw.gprs[base2];
> 
> 	if (useraddr & 3)
> 		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index 1b48a73..0f9bbb2 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -258,6 +258,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> {
> 	vcpu->arch.gmap = vcpu->kvm->arch.gmap;
> 	vcpu->run->kvm_valid_sync_ro_regs = KVM_SYNC_RO_PREFIX;
> +	vcpu->run->kvm_valid_sync_rw_regs = KVM_SYNC_RW_GPRS;
> 	return 0;
> }
> 
> @@ -390,13 +391,13 @@ static int kvm_arch_vcpu_ioctl_initial_reset(struct kvm_vcpu *vcpu)
> 
> 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.gprs, &regs->gprs, sizeof(regs->gprs));

The &s look suspicious here. You're already pointing at arrays, no?

> 	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.gprs, sizeof(regs->gprs));
> 	return 0;
> }
> 
> @@ -471,7 +472,7 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> 
> static void __vcpu_run(struct kvm_vcpu *vcpu)
> {
> -	memcpy(&vcpu->arch.sie_block->gg14, &vcpu->arch.guest_gprs[14], 16);
> +	memcpy(&vcpu->arch.sie_block->gg14, &vcpu->run->sync_rw.gprs[14], 16);
> 
> 	if (need_resched())
> 		schedule();
> @@ -487,7 +488,7 @@ static void __vcpu_run(struct kvm_vcpu *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, vcpu->run->sync_rw.gprs)) {
> 		VCPU_EVENT(vcpu, 3, "%s", "fault in sie instruction");
> 		kvm_s390_inject_program_int(vcpu, PGM_ADDRESSING);
> 	}
> @@ -497,7 +498,7 @@ static void __vcpu_run(struct kvm_vcpu *vcpu)
> 	kvm_guest_exit();
> 	local_irq_enable();
> 
> -	memcpy(&vcpu->arch.guest_gprs[14], &vcpu->arch.sie_block->gg14, 16);
> +	memcpy(&vcpu->run->sync_rw.gprs[14], &vcpu->arch.sie_block->gg14, 16);
> }
> 
> int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
> @@ -605,7 +606,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
> 		return -EFAULT;
> 
> 	if (__guestcopy(vcpu, addr + offsetof(struct save_area, gp_regs),
> -			vcpu->arch.guest_gprs, 128, prefix))
> +			vcpu->run->sync_rw.gprs, 128, prefix))
> 		return -EFAULT;
> 
> 	if (__guestcopy(vcpu, addr + offsetof(struct save_area, psw),
> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c
> index d026389..738bc43 100644
> --- a/arch/s390/kvm/priv.c
> +++ b/arch/s390/kvm/priv.c
> @@ -33,7 +33,7 @@ static int handle_set_prefix(struct kvm_vcpu *vcpu)
> 
> 	operand2 = disp2;
> 	if (base2)
> -		operand2 += vcpu->arch.guest_gprs[base2];
> +		operand2 += vcpu->run->sync_rw.gprs[base2];
> 
> 	/* must be word boundary */
> 	if (operand2 & 3) {
> @@ -74,7 +74,7 @@ static int handle_store_prefix(struct kvm_vcpu *vcpu)
> 	vcpu->stat.instruction_stpx++;
> 	operand2 = disp2;
> 	if (base2)
> -		operand2 += vcpu->arch.guest_gprs[base2];
> +		operand2 += vcpu->run->sync_rw.gprs[base2];
> 
> 	/* must be word boundary */
> 	if (operand2 & 3) {
> @@ -106,7 +106,7 @@ static int handle_store_cpu_address(struct kvm_vcpu *vcpu)
> 	vcpu->stat.instruction_stap++;
> 	useraddr = disp2;
> 	if (base2)
> -		useraddr += vcpu->arch.guest_gprs[base2];
> +		useraddr += vcpu->run->sync_rw.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)
> 	vcpu->stat.instruction_stidp++;
> 	operand2 = disp2;
> 	if (base2)
> -		operand2 += vcpu->arch.guest_gprs[base2];
> +		operand2 += vcpu->run->sync_rw.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_vcpu *vcpu, struct sysinfo_3_2_2 *mem)
> 
> 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.gprs[0] & 0xf0000000) >> 28;
> +	int sel1 = vcpu->run->sync_rw.gprs[0] & 0xff;
> +	int sel2 = vcpu->run->sync_rw.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 *vcpu)
> 
> 	operand2 = disp2;
> 	if (base2)
> -		operand2 += vcpu->arch.guest_gprs[base2];
> +		operand2 += vcpu->run->sync_rw.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.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 *vcpu)
> 	}
> 	free_page(mem);
> 	vcpu->arch.sie_block->gpsw.mask &= ~(3ul << 44);
> -	vcpu->arch.guest_gprs[0] = 0;
> +	vcpu->run->sync_rw.gprs[0] = 0;
> 	return 0;
> out_mem:
> 	free_page(mem);
> @@ -333,8 +333,8 @@ static int handle_tprot(struct kvm_vcpu *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.gprs[base1] : 0;
> +	u64 address2 = disp2 + base2 ? vcpu->run->sync_rw.gprs[base2] : 0;
> 	struct vm_area_struct *vma;
> 	unsigned long user_address;
> 
> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c
> index 0a7941d..b7883ac 100644
> --- a/arch/s390/kvm/sigp.c
> +++ b/arch/s390/kvm/sigp.c
> @@ -48,7 +48,7 @@
> 
> 
> static int __sigp_sense(struct kvm_vcpu *vcpu, u16 cpu_addr,
> -			unsigned long *reg)
> +			u64 *reg)

Looks short enough to eliminate the line wrap now :).

> {
> 	struct kvm_s390_float_interrupt *fi = &vcpu->kvm->arch.float_int;
> 	int rc;
> @@ -220,7 +220,7 @@ static int __sigp_set_arch(struct kvm_vcpu *vcpu, u32 parameter)
> }
> 
> static int __sigp_set_prefix(struct kvm_vcpu *vcpu, u16 cpu_addr, u32 address,
> -			     unsigned long *reg)
> +			     u64 *reg)
> {
> 	struct kvm_s390_float_interrupt *fi = &vcpu->kvm->arch.float_int;
> 	struct kvm_s390_local_interrupt *li = NULL;
> @@ -278,7 +278,7 @@ out_fi:
> }
> 
> static int __sigp_sense_running(struct kvm_vcpu *vcpu, u16 cpu_addr,
> -				unsigned long *reg)
> +				u64 *reg)
> {
> 	int rc;
> 	struct kvm_s390_float_interrupt *fi = &vcpu->kvm->arch.float_int;
> @@ -316,7 +316,7 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *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.gprs[r3];
> 	u8 order_code;
> 	int rc;
> 
> @@ -327,18 +327,18 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu)
> 
> 	order_code = disp2;
> 	if (base2)
> -		order_code += vcpu->arch.guest_gprs[base2];
> +		order_code += vcpu->run->sync_rw.gprs[base2];
> 
> 	if (r1 % 2)
> -		parameter = vcpu->arch.guest_gprs[r1];
> +		parameter = vcpu->run->sync_rw.gprs[r1];
> 	else
> -		parameter = vcpu->arch.guest_gprs[r1 + 1];
> +		parameter = vcpu->run->sync_rw.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.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 *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.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.gprs[r1]);
> 		break;
> 	case SIGP_RESTART:
> 		vcpu->stat.instruction_sigp_restart++;
> -- 
> 1.7.8.2
> 

[PATCH 4/4] kvm-s390: provide access guest registers via kvm_run

[Christian Borntraeger]

This patch adds the access registers to the kvm_run structure.

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

diff --git a/arch/s390/include/asm/kvm.h b/arch/s390/include/asm/kvm.h
index 5dfe143b29..9d17881 100644
--- a/arch/s390/include/asm/kvm.h
+++ b/arch/s390/include/asm/kvm.h
@@ -48,7 +48,9 @@ struct kvm_sync_ro_regs {
 };
 
 #define KVM_SYNC_RW_GPRS   (1UL << 0)
+#define KVM_SYNC_RW_ACRS   (1UL << 1)
 struct kvm_sync_rw_regs {
 	__u64          gprs[16];	/* general purpose registers */
+	__u32          acrs[16];	/* access registers */
 };
 #endif
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 173f44d..8cf6ce7 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -230,7 +230,6 @@ struct kvm_vcpu_arch {
 	s390_fp_regs      host_fpregs;
 	unsigned int      host_acrs[NUM_ACRS];
 	s390_fp_regs      guest_fpregs;
-	unsigned int      guest_acrs[NUM_ACRS];
 	struct kvm_s390_local_interrupt local_int;
 	struct hrtimer    ckc_timer;
 	struct tasklet_struct tasklet;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 0f9bbb2..095682d 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -258,7 +258,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 {
 	vcpu->arch.gmap = vcpu->kvm->arch.gmap;
 	vcpu->run->kvm_valid_sync_ro_regs = KVM_SYNC_RO_PREFIX;
-	vcpu->run->kvm_valid_sync_rw_regs = KVM_SYNC_RW_GPRS;
+	vcpu->run->kvm_valid_sync_rw_regs = KVM_SYNC_RW_GPRS |
+					    KVM_SYNC_RW_ACRS;
 	return 0;
 }
 
@@ -273,7 +274,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	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);
+	restore_access_regs(vcpu->run->sync_rw.acrs);
 	gmap_enable(vcpu->arch.gmap);
 	atomic_set_mask(CPUSTAT_RUNNING, &vcpu->arch.sie_block->cpuflags);
 }
@@ -283,7 +284,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 	atomic_clear_mask(CPUSTAT_RUNNING, &vcpu->arch.sie_block->cpuflags);
 	gmap_disable(vcpu->arch.gmap);
 	save_fp_regs(&vcpu->arch.guest_fpregs);
-	save_access_regs(vcpu->arch.guest_acrs);
+	save_access_regs(vcpu->run->sync_rw.acrs);
 	restore_fp_regs(&vcpu->arch.host_fpregs);
 	restore_access_regs(vcpu->arch.host_acrs);
 }
@@ -404,16 +405,16 @@ int kvm_arch_vcpu_ioctl_get_regs(struct kvm_vcpu *vcpu, struct kvm_regs *regs)
 int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
 				  struct kvm_sregs *sregs)
 {
-	memcpy(&vcpu->arch.guest_acrs, &sregs->acrs, sizeof(sregs->acrs));
+	memcpy(&vcpu->run->sync_rw.acrs, &sregs->acrs, sizeof(sregs->acrs));
 	memcpy(&vcpu->arch.sie_block->gcr, &sregs->crs, sizeof(sregs->crs));
-	restore_access_regs(vcpu->arch.guest_acrs);
+	restore_access_regs(vcpu->run->sync_rw.acrs);
 	return 0;
 }
 
 int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu,
 				  struct kvm_sregs *sregs)
 {
-	memcpy(&sregs->acrs, &vcpu->arch.guest_acrs, sizeof(sregs->acrs));
+	memcpy(&sregs->acrs, &vcpu->run->sync_rw.acrs, sizeof(sregs->acrs));
 	memcpy(&sregs->crs, &vcpu->arch.sie_block->gcr, sizeof(sregs->crs));
 	return 0;
 }
@@ -635,7 +636,7 @@ int kvm_s390_vcpu_store_status(struct kvm_vcpu *vcpu, unsigned long addr)
 		return -EFAULT;
 
 	if (__guestcopy(vcpu, addr + offsetof(struct save_area, acc_regs),
-			&vcpu->arch.guest_acrs, 64, prefix))
+			&vcpu->run->sync_rw.acrs, 64, prefix))
 		return -EFAULT;
 
 	if (__guestcopy(vcpu,
-- 
1.7.8.2

Re: synchronous register in kvm_run

[Christian Borntraeger]

On 05/01/12 10:54, Christian Borntraeger wrote:
> Avi, Marcelo,
> 
> here is the latest update of my patch set about guest registers in kvm_run.
> I have added an u64 flag field to make the whole structure extensible
> (and shrinkable).
> For s390 I added the prefix register as read-only and access and general
> purpose as read write.

Any feedback or suggestions on these patches?

Re: synchronous register in kvm_run

[Marcelo Tosatti]

On Mon, Jan 09, 2012 at 01:37:17PM +0100, Christian Borntraeger wrote:
> On 05/01/12 10:54, Christian Borntraeger wrote:
> > Avi, Marcelo,
> > 
> > here is the latest update of my patch set about guest registers in kvm_run.
> > I have added an u64 flag field to make the whole structure extensible
> > (and shrinkable).
> > For s390 I added the prefix register as read-only and access and general
> > purpose as read write.
> 
> Any feedback or suggestions on these patches?

I don't have any comments - Avi?

Probably have to regenerate against next branch (which contains 
the ucontrol patches).

Re: synchronous register in kvm_run

[Avi Kivity]

On 01/09/2012 02:49 PM, Marcelo Tosatti wrote:
> On Mon, Jan 09, 2012 at 01:37:17PM +0100, Christian Borntraeger wrote:
> > On 05/01/12 10:54, Christian Borntraeger wrote:
> > > Avi, Marcelo,
> > > 
> > > here is the latest update of my patch set about guest registers in kvm_run.
> > > I have added an u64 flag field to make the whole structure extensible
> > > (and shrinkable).
> > > For s390 I added the prefix register as read-only and access and general
> > > purpose as read write.
> > 
> > Any feedback or suggestions on these patches?
>
> I don't have any comments - Avi?

Looks fine to me too.

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

kvm: provide synchronous registers in kvm_run

[Christian Borntraeger]

Avi, Marcelo,

here is the next version of the sync register patch series.

[PATCH 1/4] kvm: provide synchronous registers in kvm_run

[Christian Borntraeger]

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 certain userspace handled exits more expensive than necessary.
Lets provide a section in kvm_run that works as a shared save area
for guest registers.
We also provide two 64bit flags fields (architecture specific), that will
specify
1. which parts of these fields are valid.
2. which registers were modified by userspace

Each bit for these flag fields will define a group of registers (like
general purpose) or a single register.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 Documentation/virtual/kvm/api.txt |   23 +++++++++++++++++++++++
 arch/ia64/include/asm/kvm.h       |    4 ++++
 arch/powerpc/include/asm/kvm.h    |    4 ++++
 arch/s390/include/asm/kvm.h       |    3 +++
 arch/x86/include/asm/kvm.h        |    4 ++++
 include/linux/kvm.h               |   15 +++++++++++++++
 6 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index a67fb35..7ca6962 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1771,6 +1771,29 @@ developer registration required to access it).
 		/* Fix the size of the union. */
 		char padding[256];
 	};
+
+	/*
+	 * shared registers between kvm and userspace.
+	 * kvm_valid_regs specifies the register classes set by the host
+	 * kvm_dirty_regs specified the register classes dirtied by userspace
+	 * struct kvm_sync_regs is architecture specific, as well as the
+	 * bits for kvm_valid_regs and kvm_dirty_regs
+	 */
+	__u64 kvm_valid_regs;
+	__u64 kvm_dirty_regs;
+	union {
+		struct kvm_sync_regs regs;
+		char padding[1024];
+	} s;
+
+If KVM_CAP_SYNC_REGS is defined, 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.
+Userspace can query the validity of the structure by checking
+kvm_valid_regs for specific bits. These bits are architecture specific
+and usually define the validity of a groups of registers. (e.g. one bit
+ for general purpose registers)
+
 };
 
 6. Capabilities that can be enabled
diff --git a/arch/ia64/include/asm/kvm.h b/arch/ia64/include/asm/kvm.h
index bc90c75..b9f82c8 100644
--- a/arch/ia64/include/asm/kvm.h
+++ b/arch/ia64/include/asm/kvm.h
@@ -261,4 +261,8 @@ struct kvm_debug_exit_arch {
 struct kvm_guest_debug_arch {
 };
 
+/* definition of registers in kvm_run */
+struct kvm_sync_regs {
+};
+
 #endif
diff --git a/arch/powerpc/include/asm/kvm.h b/arch/powerpc/include/asm/kvm.h
index f7727d9..7d9d4de 100644
--- a/arch/powerpc/include/asm/kvm.h
+++ b/arch/powerpc/include/asm/kvm.h
@@ -265,6 +265,10 @@ struct kvm_debug_exit_arch {
 struct kvm_guest_debug_arch {
 };
 
+/* definition of registers in kvm_run */
+struct kvm_sync_regs {
+};
+
 #define KVM_REG_MASK		0x001f
 #define KVM_REG_EXT_MASK	0xffe0
 #define KVM_REG_GPR		0x0000
diff --git a/arch/s390/include/asm/kvm.h b/arch/s390/include/asm/kvm.h
index 82b32a1..325560a 100644
--- a/arch/s390/include/asm/kvm.h
+++ b/arch/s390/include/asm/kvm.h
@@ -41,4 +41,7 @@ struct kvm_debug_exit_arch {
 struct kvm_guest_debug_arch {
 };
 
+/* definition of registers in kvm_run */
+struct kvm_sync_regs {
+};
 #endif
diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
index 4d8dcbd..e7d1c19 100644
--- a/arch/x86/include/asm/kvm.h
+++ b/arch/x86/include/asm/kvm.h
@@ -321,4 +321,8 @@ struct kvm_xcrs {
 	__u64 padding[16];
 };
 
+/* definition of registers in kvm_run */
+struct kvm_sync_regs {
+};
+
 #endif /* _ASM_X86_KVM_H */
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 6cf048d..245bcb3 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -279,6 +279,20 @@ struct kvm_run {
 		/* Fix the size of the union. */
 		char padding[256];
 	};
+
+	/*
+	 * shared registers between kvm and userspace.
+	 * kvm_valid_regs specifies the register classes set by the host
+	 * kvm_dirty_regs specified the register classes dirtied by userspace
+	 * struct kvm_sync_regs is architecture specific, as well as the
+	 * bits for kvm_valid_regs and kvm_dirty_regs
+	 */
+	__u64 kvm_valid_regs;
+	__u64 kvm_dirty_regs;
+	union {
+		struct kvm_sync_regs regs;
+		char padding[1024];
+	} s;
 };
 
 /* for KVM_REGISTER_COALESCED_MMIO / KVM_UNREGISTER_COALESCED_MMIO */
@@ -570,6 +584,7 @@ struct kvm_ppc_pvinfo {
 #define KVM_CAP_S390_GMAP 71
 #define KVM_CAP_TSC_DEADLINE_TIMER 72
 #define KVM_CAP_S390_UCONTROL 73
+#define KVM_CAP_SYNC_REGS 74
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
-- 
1.7.8.2