Re: [PATCH/RFC 4/9] KVM: s390: Add MEMOP ioctls for reading/writing guest memory

Re: [PATCH/RFC 4/9] KVM: s390: Add MEMOP ioctls for reading/writing guest memory

[Thomas Huth]

> Date: Wed, 18 Mar 2015 21:23:48 -0300
> From: Marcelo Tosatti <mtosatti@redhat.com>
>
> On Mon, Mar 16, 2015 at 09:51:40AM +0100, Christian Borntraeger wrote:
> > From: Thomas Huth <thuth@linux.vnet.ibm.com>
> >
> > On s390, we've got to make sure to hold the IPTE lock while accessing
> > logical memory. So let's add an ioctl for reading and writing logical
> > memory to provide this feature for userspace, too.
...
>
> What i was wondering is why you can't translate the address
> in the kernel and let userspace perform the actual read/write?

The idea here is to protect the read/write access with the ipte-lock, too.
That way, the whole address translation _and_ the read/write access are
protected together against invalidate-page-table operations from other
CPUs,
so the whole memory access looks atomic for other VCPUs. And since we do
not
want to expose the ipte lock directly to user space, both has to be done
in the kernel.

We already had a long internal discussion about this in our team, and
indeed, if the ipte-lock would be the only "problem" that we face on s390,
we might also come up with a solution where the memory read/write access
is done in userspace instead. However, for full architecture compliance,
we later have got to support the so-called "storage keys" during memory
accesses, too, and this can hardly be done accurately and safely from
userspace. So I'm afraid, it's somewhat ugly that we've got to provide an
ioctl here to read/write the guest memory, but it's the only feasible
solution that I could think of.

 Thomas

Re: [PATCH/RFC 4/9] KVM: s390: Add MEMOP ioctls for reading/writing guest memory

[Marcelo Tosatti]

On Thu, Mar 19, 2015 at 02:15:18PM +0100, Thomas Huth wrote:
> > Date: Wed, 18 Mar 2015 21:23:48 -0300
> > From: Marcelo Tosatti <mtosatti@redhat.com>
> >
> > On Mon, Mar 16, 2015 at 09:51:40AM +0100, Christian Borntraeger wrote:
> > > From: Thomas Huth <thuth@linux.vnet.ibm.com>
> > >
> > > On s390, we've got to make sure to hold the IPTE lock while accessing
> > > logical memory. So let's add an ioctl for reading and writing logical
> > > memory to provide this feature for userspace, too.
> ...
> >
> > What i was wondering is why you can't translate the address
> > in the kernel and let userspace perform the actual read/write?
> 
> The idea here is to protect the read/write access with the ipte-lock, too.
> That way, the whole address translation _and_ the read/write access are
> protected together against invalidate-page-table operations from other
> CPUs,
> so the whole memory access looks atomic for other VCPUs. And since we do
> not
> want to expose the ipte lock directly to user space, both has to be done
> in the kernel.
> We already had a long internal discussion about this in our team, and
> indeed, if the ipte-lock would be the only "problem" that we face on s390,
> we might also come up with a solution where the memory read/write access
> is done in userspace instead. However, for full architecture compliance,
> we later have got to support the so-called "storage keys" during memory
> accesses, too, and this can hardly be done accurately and safely from
> userspace. So I'm afraid, it's somewhat ugly that we've got to provide an
> ioctl here to read/write the guest memory, but it's the only feasible
> solution that I could think of.

I see, thanks.

[PATCH/RFC 0/9] Next bunch of KVM/s390x changes for next

[Christian Borntraeger]

Paolo, Marcelo, Alex,

this is my current patch queue that I want to send for the
next pull request.
As it contains 3 API extensions
- KVM: s390: introduce post handlers for STSI
- KVM: s390: Create ioctl for Getting/Setting guest storage keys
- KVM: s390: Add MEMOP ioctls for reading/writing guest memory

some time for review is warranted.


Alexander Yarygin (4):
  KVM: s390: Fix low-address protection for real addresses
  KVM: s390: Guest's memory access functions get access registers
  KVM: s390: Optimize paths where get_vcpu_asce() is invoked
  KVM: s390: Add access register mode

Dominik Dingel (1):
  KVM: s390: cleanup jump lables in kvm_arch_init_vm

Ekaterina Tumanova (1):
  KVM: s390: introduce post handlers for STSI

Geert Uytterhoeven (1):
  KVM: s390: Spelling s/intance/instance/

Jason J. Herne (1):
  KVM: s390: Create ioctl for Getting/Setting guest storage keys

Thomas Huth (1):
  KVM: s390: Add MEMOP ioctls for reading/writing guest memory

 Documentation/virtual/kvm/api.txt | 132 +++++++++++++++++
 arch/s390/include/asm/kvm_host.h  |   1 +
 arch/s390/kvm/diag.c              |   4 +-
 arch/s390/kvm/gaccess.c           | 294 +++++++++++++++++++++++++++++++-------
 arch/s390/kvm/gaccess.h           |  21 +--
 arch/s390/kvm/intercept.c         |   4 +-
 arch/s390/kvm/kvm-s390.c          | 217 ++++++++++++++++++++++++++--
 arch/s390/kvm/kvm-s390.h          |  28 +++-
 arch/s390/kvm/priv.c              |  93 +++++++-----
 arch/s390/kvm/sigp.c              |   4 +-
 include/uapi/linux/kvm.h          |  46 ++++++
 11 files changed, 730 insertions(+), 114 deletions(-)

-- 
2.3.0

[PATCH/RFC 4/9] KVM: s390: Add MEMOP ioctls for reading/writing guest memory

[Christian Borntraeger]

From: Thomas Huth <thuth@linux.vnet.ibm.com>

On s390, we've got to make sure to hold the IPTE lock while accessing
logical memory. So let's add an ioctl for reading and writing logical
memory to provide this feature for userspace, too.
The maximum transfer size of this call is limited to 64kB to prevent
that the guest can trigger huge copy_from/to_user transfers. QEMU
currently only requests up to one or two pages so far, so 16*4kB seems
to be a reasonable limit here.

Signed-off-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 Documentation/virtual/kvm/api.txt | 46 ++++++++++++++++++++++++
 arch/s390/kvm/gaccess.c           | 22 ++++++++++++
 arch/s390/kvm/gaccess.h           |  2 ++
 arch/s390/kvm/kvm-s390.c          | 74 +++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/kvm.h          | 21 +++++++++++
 5 files changed, 165 insertions(+)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index ee47998e..f03178d 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2716,6 +2716,52 @@ The fields in each entry are defined as follows:
    eax, ebx, ecx, edx: the values returned by the cpuid instruction for
          this function/index combination
 
+4.89 KVM_S390_MEM_OP
+
+Capability: KVM_CAP_S390_MEM_OP
+Architectures: s390
+Type: vcpu ioctl
+Parameters: struct kvm_s390_mem_op (in)
+Returns: = 0 on success,
+         < 0 on generic error (e.g. -EFAULT or -ENOMEM),
+         > 0 if an exception occurred while walking the page tables
+
+Read or write data from/to the logical (virtual) memory of a VPCU.
+
+Parameters are specified via the following structure:
+
+struct kvm_s390_mem_op {
+	__u64 gaddr;		/* the guest address */
+	__u64 flags;		/* arch specific flags */
+	__u32 size;		/* amount of bytes */
+	__u32 op;		/* type of operation */
+	__u64 buf;		/* buffer in userspace */
+	__u8 ar;		/* the access register number */
+	__u8 reserved[31];	/* should be set to 0 */
+};
+
+The type of operation is specified in the "op" field. It is either
+KVM_S390_MEMOP_LOGICAL_READ for reading from logical memory space or
+KVM_S390_MEMOP_LOGICAL_WRITE for writing to logical memory space. The
+KVM_S390_MEMOP_F_CHECK_ONLY flag can be set in the "flags" field to check
+whether the corresponding memory access would create an access exception
+(without touching the data in the memory at the destination). In case an
+access exception occurred while walking the MMU tables of the guest, the
+ioctl returns a positive error number to indicate the type of exception.
+This exception is also raised directly at the corresponding VCPU if the
+flag KVM_S390_MEMOP_F_INJECT_EXCEPTION is set in the "flags" field.
+
+The start address of the memory region has to be specified in the "gaddr"
+field, and the length of the region in the "size" field. "buf" is the buffer
+supplied by the userspace application where the read data should be written
+to for KVM_S390_MEMOP_LOGICAL_READ, or where the data that should be written
+is stored for a KVM_S390_MEMOP_LOGICAL_WRITE. "buf" is unused and can be NULL
+when KVM_S390_MEMOP_F_CHECK_ONLY is specified. "ar" designates the access
+register number to be used.
+
+The "reserved" field is meant for future extensions. It is not used by
+KVM with the currently defined set of flags.
+
 5. The kvm_run structure
 ------------------------
 
diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
index c230904..04a7c67 100644
--- a/arch/s390/kvm/gaccess.c
+++ b/arch/s390/kvm/gaccess.c
@@ -697,6 +697,28 @@ int guest_translate_address(struct kvm_vcpu *vcpu, unsigned long gva,
 }
 
 /**
+ * check_gva_range - test a range of guest virtual addresses for accessibility
+ */
+int check_gva_range(struct kvm_vcpu *vcpu, unsigned long gva,
+		    unsigned long length, int is_write)
+{
+	unsigned long gpa;
+	unsigned long currlen;
+	int rc = 0;
+
+	ipte_lock(vcpu);
+	while (length > 0 && !rc) {
+		currlen = min(length, PAGE_SIZE - (gva % PAGE_SIZE));
+		rc = guest_translate_address(vcpu, gva, &gpa, is_write);
+		gva += currlen;
+		length -= currlen;
+	}
+	ipte_unlock(vcpu);
+
+	return rc;
+}
+
+/**
  * kvm_s390_check_low_addr_prot_real - check for low-address protection
  * @gra: Guest real address
  *
diff --git a/arch/s390/kvm/gaccess.h b/arch/s390/kvm/gaccess.h
index 20de77e..d0de110 100644
--- a/arch/s390/kvm/gaccess.h
+++ b/arch/s390/kvm/gaccess.h
@@ -157,6 +157,8 @@ int read_guest_lc(struct kvm_vcpu *vcpu, unsigned long gra, void *data,
 
 int guest_translate_address(struct kvm_vcpu *vcpu, unsigned long gva,
 			    unsigned long *gpa, int write);
+int check_gva_range(struct kvm_vcpu *vcpu, unsigned long gva,
+		    unsigned long length, int is_write);
 
 int access_guest(struct kvm_vcpu *vcpu, unsigned long ga, void *data,
 		 unsigned long len, int write);
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 4075acb..92855e2 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -25,6 +25,7 @@
 #include <linux/random.h>
 #include <linux/slab.h>
 #include <linux/timer.h>
+#include <linux/vmalloc.h>
 #include <asm/asm-offsets.h>
 #include <asm/lowcore.h>
 #include <asm/pgtable.h>
@@ -38,6 +39,8 @@
 #include "trace.h"
 #include "trace-s390.h"
 
+#define MEM_OP_MAX_SIZE 65536	/* Maximum transfer size for KVM_S390_MEM_OP */
+
 #define VCPU_STAT(x) offsetof(struct kvm_vcpu, stat.x), KVM_STAT_VCPU
 
 struct kvm_stats_debugfs_item debugfs_entries[] = {
@@ -177,6 +180,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_S390_USER_SIGP:
 		r = 1;
 		break;
+	case KVM_CAP_S390_MEM_OP:
+		r = MEM_OP_MAX_SIZE;
+		break;
 	case KVM_CAP_NR_VCPUS:
 	case KVM_CAP_MAX_VCPUS:
 		r = KVM_MAX_VCPUS;
@@ -2185,6 +2191,65 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
 	return r;
 }
 
+static long kvm_s390_guest_mem_op(struct kvm_vcpu *vcpu,
+				  struct kvm_s390_mem_op *mop)
+{
+	void __user *uaddr = (void __user *)mop->buf;
+	void *tmpbuf = NULL;
+	int r, srcu_idx;
+	const u64 supported_flags = KVM_S390_MEMOP_F_INJECT_EXCEPTION
+				    | KVM_S390_MEMOP_F_CHECK_ONLY;
+
+	if (mop->flags & ~supported_flags)
+		return -EINVAL;
+
+	if (mop->size > MEM_OP_MAX_SIZE)
+		return -E2BIG;
+
+	if (!(mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY)) {
+		tmpbuf = vmalloc(mop->size);
+		if (!tmpbuf)
+			return -ENOMEM;
+	}
+
+	srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
+
+	switch (mop->op) {
+	case KVM_S390_MEMOP_LOGICAL_READ:
+		if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
+			r = check_gva_range(vcpu, mop->gaddr, mop->size, false);
+			break;
+		}
+		r = read_guest(vcpu, mop->gaddr, tmpbuf, mop->size);
+		if (r == 0) {
+			if (copy_to_user(uaddr, tmpbuf, mop->size))
+				r = -EFAULT;
+		}
+		break;
+	case KVM_S390_MEMOP_LOGICAL_WRITE:
+		if (mop->flags & KVM_S390_MEMOP_F_CHECK_ONLY) {
+			r = check_gva_range(vcpu, mop->gaddr, mop->size, true);
+			break;
+		}
+		if (copy_from_user(tmpbuf, uaddr, mop->size)) {
+			r = -EFAULT;
+			break;
+		}
+		r = write_guest(vcpu, mop->gaddr, tmpbuf, mop->size);
+		break;
+	default:
+		r = -EINVAL;
+	}
+
+	srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx);
+
+	if (r > 0 && (mop->flags & KVM_S390_MEMOP_F_INJECT_EXCEPTION) != 0)
+		kvm_s390_inject_prog_irq(vcpu, &vcpu->arch.pgm);
+
+	vfree(tmpbuf);
+	return r;
+}
+
 long kvm_arch_vcpu_ioctl(struct file *filp,
 			 unsigned int ioctl, unsigned long arg)
 {
@@ -2284,6 +2349,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = kvm_vcpu_ioctl_enable_cap(vcpu, &cap);
 		break;
 	}
+	case KVM_S390_MEM_OP: {
+		struct kvm_s390_mem_op mem_op;
+
+		if (copy_from_user(&mem_op, argp, sizeof(mem_op)) == 0)
+			r = kvm_s390_guest_mem_op(vcpu, &mem_op);
+		else
+			r = -EFAULT;
+		break;
+	}
 	default:
 		r = -ENOTTY;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 82634a4..74bbd9a 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -365,6 +365,24 @@ struct kvm_translation {
 	__u8  pad[5];
 };
 
+/* for KVM_S390_MEM_OP */
+struct kvm_s390_mem_op {
+	/* in */
+	__u64 gaddr;		/* the guest address */
+	__u64 flags;		/* arch specific flags */
+	__u32 size;		/* amount of bytes */
+	__u32 op;		/* type of operation */
+	__u64 buf;		/* buffer in userspace */
+	__u8 ar;		/* the access register number */
+	__u8 reserved[31];	/* should be set to 0 */
+};
+/* types for kvm_s390_mem_op->op */
+#define KVM_S390_MEMOP_LOGICAL_READ	0
+#define KVM_S390_MEMOP_LOGICAL_WRITE	1
+/* flags for kvm_s390_mem_op->flags */
+#define KVM_S390_MEMOP_F_CHECK_ONLY		(1ULL << 0)
+#define KVM_S390_MEMOP_F_INJECT_EXCEPTION	(1ULL << 1)
+
 /* for KVM_INTERRUPT */
 struct kvm_interrupt {
 	/* in */
@@ -761,6 +779,7 @@ struct kvm_ppc_smmu_info {
 #define KVM_CAP_CHECK_EXTENSION_VM 105
 #define KVM_CAP_S390_USER_SIGP 106
 #define KVM_CAP_S390_VECTOR_REGISTERS 107
+#define KVM_CAP_S390_MEM_OP 108
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1136,6 +1155,8 @@ struct kvm_s390_ucas_mapping {
 #define KVM_ARM_VCPU_INIT	  _IOW(KVMIO,  0xae, struct kvm_vcpu_init)
 #define KVM_ARM_PREFERRED_TARGET  _IOR(KVMIO,  0xaf, struct kvm_vcpu_init)
 #define KVM_GET_REG_LIST	  _IOWR(KVMIO, 0xb0, struct kvm_reg_list)
+/* Available with KVM_CAP_S390_MEM_OP */
+#define KVM_S390_MEM_OP		  _IOW(KVMIO,  0xb1, struct kvm_s390_mem_op)
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 #define KVM_DEV_ASSIGN_PCI_2_3		(1 << 1)
-- 
2.3.0

Re: [PATCH/RFC 4/9] KVM: s390: Add MEMOP ioctls for reading/writing guest memory

[Cornelia Huck]

On Mon, 16 Mar 2015 09:51:40 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> From: Thomas Huth <thuth@linux.vnet.ibm.com>
> 
> On s390, we've got to make sure to hold the IPTE lock while accessing
> logical memory. So let's add an ioctl for reading and writing logical
> memory to provide this feature for userspace, too.
> The maximum transfer size of this call is limited to 64kB to prevent
> that the guest can trigger huge copy_from/to_user transfers. QEMU
> currently only requests up to one or two pages so far, so 16*4kB seems
> to be a reasonable limit here.
> 
> Signed-off-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  Documentation/virtual/kvm/api.txt | 46 ++++++++++++++++++++++++
>  arch/s390/kvm/gaccess.c           | 22 ++++++++++++
>  arch/s390/kvm/gaccess.h           |  2 ++
>  arch/s390/kvm/kvm-s390.c          | 74 +++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h          | 21 +++++++++++
>  5 files changed, 165 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index ee47998e..f03178d 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2716,6 +2716,52 @@ The fields in each entry are defined as follows:
>     eax, ebx, ecx, edx: the values returned by the cpuid instruction for
>           this function/index combination
> 
> +4.89 KVM_S390_MEM_OP
> +
> +Capability: KVM_CAP_S390_MEM_OP
> +Architectures: s390
> +Type: vcpu ioctl
> +Parameters: struct kvm_s390_mem_op (in)
> +Returns: = 0 on success,
> +         < 0 on generic error (e.g. -EFAULT or -ENOMEM),
> +         > 0 if an exception occurred while walking the page tables
> +
> +Read or write data from/to the logical (virtual) memory of a VPCU.
> +
> +Parameters are specified via the following structure:
> +
> +struct kvm_s390_mem_op {
> +	__u64 gaddr;		/* the guest address */
> +	__u64 flags;		/* arch specific flags */

Drop the "arch"? This is a s390-only ioctl :)

> +	__u32 size;		/* amount of bytes */
> +	__u32 op;		/* type of operation */
> +	__u64 buf;		/* buffer in userspace */
> +	__u8 ar;		/* the access register number */

This makes me wonder whether the patches should be reordered to
introduce access register mode first: It seems strange that you can
specify a parameter that is ignored. Same for the STSI patch.

> +	__u8 reserved[31];	/* should be set to 0 */
> +};
> +
> +The type of operation is specified in the "op" field. It is either
> +KVM_S390_MEMOP_LOGICAL_READ for reading from logical memory space or
> +KVM_S390_MEMOP_LOGICAL_WRITE for writing to logical memory space. The
> +KVM_S390_MEMOP_F_CHECK_ONLY flag can be set in the "flags" field to check
> +whether the corresponding memory access would create an access exception
> +(without touching the data in the memory at the destination). In case an
> +access exception occurred while walking the MMU tables of the guest, the
> +ioctl returns a positive error number to indicate the type of exception.
> +This exception is also raised directly at the corresponding VCPU if the
> +flag KVM_S390_MEMOP_F_INJECT_EXCEPTION is set in the "flags" field.
> +
> +The start address of the memory region has to be specified in the "gaddr"
> +field, and the length of the region in the "size" field. "buf" is the buffer
> +supplied by the userspace application where the read data should be written
> +to for KVM_S390_MEMOP_LOGICAL_READ, or where the data that should be written
> +is stored for a KVM_S390_MEMOP_LOGICAL_WRITE. "buf" is unused and can be NULL
> +when KVM_S390_MEMOP_F_CHECK_ONLY is specified. "ar" designates the access
> +register number to be used.
> +
> +The "reserved" field is meant for future extensions. It is not used by
> +KVM with the currently defined set of flags.
> +
>  5. The kvm_run structure
>  ------------------------
> 

Re: [PATCH/RFC 4/9] KVM: s390: Add MEMOP ioctls for reading/writing guest memory

[Christian Borntraeger]

Am 16.03.2015 um 13:16 schrieb Cornelia Huck:
> On Mon, 16 Mar 2015 09:51:40 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> From: Thomas Huth <thuth@linux.vnet.ibm.com>
>>
>> On s390, we've got to make sure to hold the IPTE lock while accessing
>> logical memory. So let's add an ioctl for reading and writing logical
>> memory to provide this feature for userspace, too.
>> The maximum transfer size of this call is limited to 64kB to prevent
>> that the guest can trigger huge copy_from/to_user transfers. QEMU
>> currently only requests up to one or two pages so far, so 16*4kB seems
>> to be a reasonable limit here.
>>
>> Signed-off-by: Thomas Huth <thuth@linux.vnet.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  Documentation/virtual/kvm/api.txt | 46 ++++++++++++++++++++++++
>>  arch/s390/kvm/gaccess.c           | 22 ++++++++++++
>>  arch/s390/kvm/gaccess.h           |  2 ++
>>  arch/s390/kvm/kvm-s390.c          | 74 +++++++++++++++++++++++++++++++++++++++
>>  include/uapi/linux/kvm.h          | 21 +++++++++++
>>  5 files changed, 165 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index ee47998e..f03178d 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -2716,6 +2716,52 @@ The fields in each entry are defined as follows:
>>     eax, ebx, ecx, edx: the values returned by the cpuid instruction for
>>           this function/index combination
>>
>> +4.89 KVM_S390_MEM_OP
>> +
>> +Capability: KVM_CAP_S390_MEM_OP
>> +Architectures: s390
>> +Type: vcpu ioctl
>> +Parameters: struct kvm_s390_mem_op (in)
>> +Returns: = 0 on success,
>> +         < 0 on generic error (e.g. -EFAULT or -ENOMEM),
>> +         > 0 if an exception occurred while walking the page tables
>> +
>> +Read or write data from/to the logical (virtual) memory of a VPCU.
>> +
>> +Parameters are specified via the following structure:
>> +
>> +struct kvm_s390_mem_op {
>> +	__u64 gaddr;		/* the guest address */
>> +	__u64 flags;		/* arch specific flags */
> 
> Drop the "arch"? This is a s390-only ioctl :)

Will fixup.

> 
>> +	__u32 size;		/* amount of bytes */
>> +	__u32 op;		/* type of operation */
>> +	__u64 buf;		/* buffer in userspace */
>> +	__u8 ar;		/* the access register number */
> 
> This makes me wonder whether the patches should be reordered to
> introduce access register mode first: It seems strange that you can
> specify a parameter that is ignored. Same for the STSI patch.

Yes make sense. Will be some ripple when reordering but certainly doable.


> 
>> +	__u8 reserved[31];	/* should be set to 0 */
>> +};
>> +
>> +The type of operation is specified in the "op" field. It is either
>> +KVM_S390_MEMOP_LOGICAL_READ for reading from logical memory space or
>> +KVM_S390_MEMOP_LOGICAL_WRITE for writing to logical memory space. The
>> +KVM_S390_MEMOP_F_CHECK_ONLY flag can be set in the "flags" field to check
>> +whether the corresponding memory access would create an access exception
>> +(without touching the data in the memory at the destination). In case an
>> +access exception occurred while walking the MMU tables of the guest, the
>> +ioctl returns a positive error number to indicate the type of exception.
>> +This exception is also raised directly at the corresponding VCPU if the
>> +flag KVM_S390_MEMOP_F_INJECT_EXCEPTION is set in the "flags" field.
>> +
>> +The start address of the memory region has to be specified in the "gaddr"
>> +field, and the length of the region in the "size" field. "buf" is the buffer
>> +supplied by the userspace application where the read data should be written
>> +to for KVM_S390_MEMOP_LOGICAL_READ, or where the data that should be written
>> +is stored for a KVM_S390_MEMOP_LOGICAL_WRITE. "buf" is unused and can be NULL
>> +when KVM_S390_MEMOP_F_CHECK_ONLY is specified. "ar" designates the access
>> +register number to be used.
>> +
>> +The "reserved" field is meant for future extensions. It is not used by
>> +KVM with the currently defined set of flags.
>> +
>>  5. The kvm_run structure
>>  ------------------------
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH/RFC 4/9] KVM: s390: Add MEMOP ioctls for reading/writing guest memory

[Marcelo Tosatti]

On Mon, Mar 16, 2015 at 09:51:40AM +0100, Christian Borntraeger wrote:
> From: Thomas Huth <thuth@linux.vnet.ibm.com>
> 
> On s390, we've got to make sure to hold the IPTE lock while accessing
> logical memory. So let's add an ioctl for reading and writing logical
> memory to provide this feature for userspace, too.
> The maximum transfer size of this call is limited to 64kB to prevent
> that the guest can trigger huge copy_from/to_user transfers. QEMU
> currently only requests up to one or two pages so far, so 16*4kB seems
> to be a reasonable limit here.
> 
> Signed-off-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  Documentation/virtual/kvm/api.txt | 46 ++++++++++++++++++++++++
>  arch/s390/kvm/gaccess.c           | 22 ++++++++++++
>  arch/s390/kvm/gaccess.h           |  2 ++
>  arch/s390/kvm/kvm-s390.c          | 74 +++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/kvm.h          | 21 +++++++++++
>  5 files changed, 165 insertions(+)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index ee47998e..f03178d 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2716,6 +2716,52 @@ The fields in each entry are defined as follows:
>     eax, ebx, ecx, edx: the values returned by the cpuid instruction for
>           this function/index combination
>  
> +4.89 KVM_S390_MEM_OP
> +
> +Capability: KVM_CAP_S390_MEM_OP
> +Architectures: s390
> +Type: vcpu ioctl
> +Parameters: struct kvm_s390_mem_op (in)
> +Returns: = 0 on success,
> +         < 0 on generic error (e.g. -EFAULT or -ENOMEM),
> +         > 0 if an exception occurred while walking the page tables
> +
> +Read or write data from/to the logical (virtual) memory of a VPCU.
> +
> +Parameters are specified via the following structure:
> +
> +struct kvm_s390_mem_op {
> +	__u64 gaddr;		/* the guest address */
> +	__u64 flags;		/* arch specific flags */
> +	__u32 size;		/* amount of bytes */
> +	__u32 op;		/* type of operation */
> +	__u64 buf;		/* buffer in userspace */
> +	__u8 ar;		/* the access register number */
> +	__u8 reserved[31];	/* should be set to 0 */
> +};
> +
> +The type of operation is specified in the "op" field. It is either
> +KVM_S390_MEMOP_LOGICAL_READ for reading from logical memory space or
> +KVM_S390_MEMOP_LOGICAL_WRITE for writing to logical memory space. The
> +KVM_S390_MEMOP_F_CHECK_ONLY flag can be set in the "flags" field to check
> +whether the corresponding memory access would create an access exception
> +(without touching the data in the memory at the destination). In case an
> +access exception occurred while walking the MMU tables of the guest, the
> +ioctl returns a positive error number to indicate the type of exception.
> +This exception is also raised directly at the corresponding VCPU if the
> +flag KVM_S390_MEMOP_F_INJECT_EXCEPTION is set in the "flags" field.
> +
> +The start address of the memory region has to be specified in the "gaddr"
> +field, and the length of the region in the "size" field. "buf" is the buffer
> +supplied by the userspace application where the read data should be written
> +to for KVM_S390_MEMOP_LOGICAL_READ, or where the data that should be written
> +is stored for a KVM_S390_MEMOP_LOGICAL_WRITE. "buf" is unused and can be NULL
> +when KVM_S390_MEMOP_F_CHECK_ONLY is specified. "ar" designates the access
> +register number to be used.
> +
> +The "reserved" field is meant for future extensions. It is not used by
> +KVM with the currently defined set of flags.
> +
>  5. The kvm_run structure
>  ------------------------
>  
> diff --git a/arch/s390/kvm/gaccess.c b/arch/s390/kvm/gaccess.c
> index c230904..04a7c67 100644
> --- a/arch/s390/kvm/gaccess.c
> +++ b/arch/s390/kvm/gaccess.c
> @@ -697,6 +697,28 @@ int guest_translate_address(struct kvm_vcpu *vcpu, unsigned long gva,
>  }
>  
>  /**
> + * check_gva_range - test a range of guest virtual addresses for accessibility
> + */
> +int check_gva_range(struct kvm_vcpu *vcpu, unsigned long gva,
> +		    unsigned long length, int is_write)
> +{
> +	unsigned long gpa;
> +	unsigned long currlen;
> +	int rc = 0;
> +
> +	ipte_lock(vcpu);
> +	while (length > 0 && !rc) {
> +		currlen = min(length, PAGE_SIZE - (gva % PAGE_SIZE));
> +		rc = guest_translate_address(vcpu, gva, &gpa, is_write);
> +		gva += currlen;
> +		length -= currlen;
> +	}
> +	ipte_unlock(vcpu);
> +
> +	return rc;
> +}

What i was wondering is why you can't translate the address
in the kernel and let userspace perform the actual read/write?