Re: [Qemu-devel] [PATCH RFC 1/1] KVM: s390: Add MEMOP ioctl for reading/writing guest memory

[Thomas Huth]

On Tue, 03 Feb 2015 14:04:43 +0100
Paolo Bonzini <address@hidden> wrote:

> On 03/02/2015 13:11, Thomas Huth wrote:
> > On s390, we've got to make sure to hold the IPTE lock while accessing
> > virtual memory. So let's add an ioctl for reading and writing virtual
> > memory to provide this feature for userspace, too.
> > 
> > Signed-off-by: Thomas Huth <address@hidden>
> > Reviewed-by: Dominik Dingel <address@hidden>
> > Reviewed-by: David Hildenbrand <address@hidden>
> > ---
> >  Documentation/virtual/kvm/api.txt |   44 +++++++++++++++++++++++++
> >  arch/s390/kvm/gaccess.c           |   22 +++++++++++++
> >  arch/s390/kvm/gaccess.h           |    2 +
> >  arch/s390/kvm/kvm-s390.c          |   63 
> > +++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/kvm.h          |   21 ++++++++++++
> >  5 files changed, 152 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/virtual/kvm/api.txt 
> > b/Documentation/virtual/kvm/api.txt
> > index b112efc..bf44b53 100644
> > --- a/Documentation/virtual/kvm/api.txt
> > +++ b/Documentation/virtual/kvm/api.txt
> > @@ -2716,6 +2716,50 @@ 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_GUEST_MEM_OP
> > +
> > +Capability: KVM_CAP_MEM_OP
> 
> Put "virtual" somewhere in the ioctl name and capability?

Actually, I'd prefer to keep the "virtual" in the defines for the type
of operation below: When it comes to s390 storage keys, we likely might
need some calls for reading and writing to physical memory, too. Then
we could simply extend this ioctl instead of inventing a new one.

> > +Architectures: s390
> > +Type: vcpu ioctl
> > +Parameters: struct kvm_guest_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 virtual memory of a VPCU.
> > +
> > +Parameters are specified via the following structure:
> > +
> > +struct kvm_guest_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 reserved[32];      /* should be set to 0 */
> > +};
> > +
> > +The type of operation is specified in the "op" field, either 
> > KVM_MEMOP_VIRTREAD
> > +for reading from memory, KVM_MEMOP_VIRTWRITE for writing to memory, or
> > +KVM_MEMOP_CHECKVIRTREAD or KVM_MEMOP_CHECKVIRTWRITE to check whether the
> 
> Better:
> 
> #define KVM_MEMOP_READ       0
> #define KVM_MEMOP_WRITE      1
> 
> and in the flags field:
> 
> #define KVM_MEMOP_F_CHECK_ONLY (1 << 1)

Ok, a flag for the check operations is fine for me, too.

...
> > +The logical (virtual) 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_MEMOP_VIRTREAD, or where the data that should
> > +be written is stored for a KVM_MEMOP_VIRTWRITE. "buf" can be NULL for both
> > +CHECK operations.
> 
> "buf" is unused and can be NULL for both CHECK operations.
> 
> > +The "reserved" field is meant for future extensions. It must currently be
> > +set to 0.
> 
> Not really true, as you don't check it.  So "It is not used by KVM with
> the currently defined set of flags" is a better explanation.

ok ... and maybe add "should be set to zero" ?

> Paolo

Thanks for the review!

 Thomas