[Qemu-devel] [PATCH 0/3] Vhost: no more leak QEMU virtual addresses to user backend

[Qemu-devel] [PATCH 0/3] Vhost: no more leak QEMU virtual addresses to user backend

[Maxime Coquelin]

Before this series, QEMU process virtual addresses are sent to the
user backend as user addresses.

Passing these virtual addresses aren't useful, as the backend doesn't
direct access to QEMU address space.

It does make sense however for kernel backend, which can access QEMU
address space.

This series introduce a new enum set by the backend stating whether it
prefers using QEMU Virtual addresses or Guest physical addresses as
User address, and make vhost-user backend to use Guest physical
addresses.

Maxime Coquelin (3):
  vhost-user: rename VhostUserMemory userspace_addr field to user_addr
  vhost: introduce backend's user address type
  vhost-user: no more leak QEMU virtual addresses to user backend

 hw/virtio/vhost-backend.c         |  1 +
 hw/virtio/vhost-user.c            |  6 ++++--
 hw/virtio/vhost.c                 | 16 ++++++++++++----
 include/hw/virtio/vhost-backend.h |  6 ++++++
 4 files changed, 23 insertions(+), 6 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/3] vhost-user: rename VhostUserMemory userspace_addr field to user_addr

[Maxime Coquelin]

The Vhost-user specification does not specify the user address should
be a valid address within master process address space.

Let's name it user_addr to comply with the specification.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 hw/virtio/vhost-user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 093675ed98..43b342a65b 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -77,7 +77,7 @@ typedef enum VhostUserSlaveRequest {
 typedef struct VhostUserMemoryRegion {
     uint64_t guest_phys_addr;
     uint64_t memory_size;
-    uint64_t userspace_addr;
+    uint64_t user_addr;
     uint64_t mmap_offset;
 } VhostUserMemoryRegion;
 
@@ -317,7 +317,7 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
                                      &offset);
         fd = memory_region_get_fd(mr);
         if (fd > 0) {
-            msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
+            msg.payload.memory.regions[fd_num].user_addr = reg->userspace_addr;
             msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
             msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
             msg.payload.memory.regions[fd_num].mmap_offset = offset;
-- 
2.14.3

[Qemu-devel] [PATCH 2/3] vhost: introduce backend's user address type

[Maxime Coquelin]

User backends don't need to know about QEMU virtual addresses.
It is possible to use guest physical addresses as user addresses
without user backends changes.

This patch introduces a new enum in VhostOps to specify whether the
backend expects the user addresses to be host virtual or guest
physical.

This patch makes possible for the backend driver to select whether
it wants to use host virtual or guest physical addresses as user
addresses.

No behavioral changes in this patch for the backensds, as both
backend types still use host virtual addresses.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 hw/virtio/vhost-backend.c         |  1 +
 hw/virtio/vhost-user.c            |  1 +
 hw/virtio/vhost.c                 | 16 ++++++++++++----
 include/hw/virtio/vhost-backend.h |  6 ++++++
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 7f09efab8b..dc53f91d59 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -235,6 +235,7 @@ static void vhost_kernel_set_iotlb_callback(struct vhost_dev *dev,
 
 static const VhostOps kernel_ops = {
         .backend_type = VHOST_BACKEND_TYPE_KERNEL,
+        .uaddr_type = VHOST_UADDR_TYPE_HVA,
         .vhost_backend_init = vhost_kernel_init,
         .vhost_backend_cleanup = vhost_kernel_cleanup,
         .vhost_backend_memslots_limit = vhost_kernel_memslots_limit,
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 43b342a65b..5ebeb8401b 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -924,6 +924,7 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
 
 const VhostOps user_ops = {
         .backend_type = VHOST_BACKEND_TYPE_USER,
+        .uaddr_type = VHOST_UADDR_TYPE_HVA,
         .vhost_backend_init = vhost_user_init,
         .vhost_backend_cleanup = vhost_user_cleanup,
         .vhost_backend_memslots_limit = vhost_user_memslots_limit,
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e4290ce93d..300e307284 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -433,7 +433,8 @@ static int vhost_dev_has_iommu(struct vhost_dev *dev)
 static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
                               hwaddr *plen, int is_write)
 {
-    if (!vhost_dev_has_iommu(dev)) {
+    if (dev->vhost_ops->uaddr_type == VHOST_UADDR_TYPE_HVA &&
+            !vhost_dev_has_iommu(dev)) {
         return cpu_physical_memory_map(addr, plen, is_write);
     } else {
         return (void *)(uintptr_t)addr;
@@ -444,7 +445,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
                                hwaddr len, int is_write,
                                hwaddr access_len)
 {
-    if (!vhost_dev_has_iommu(dev)) {
+    if (dev->vhost_ops->uaddr_type == VHOST_UADDR_TYPE_HVA &&
+            !vhost_dev_has_iommu(dev)) {
         cpu_physical_memory_unmap(buffer, len, is_write, access_len);
     }
 }
@@ -975,7 +977,7 @@ static int vhost_memory_region_lookup(struct vhost_dev *hdev,
 int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write)
 {
     IOMMUTLBEntry iotlb;
-    uint64_t uaddr, len;
+    uint64_t userspace_addr, uaddr, len;
     int ret = -EFAULT;
 
     rcu_read_lock();
@@ -984,7 +986,7 @@ int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write)
                                           iova, write);
     if (iotlb.target_as != NULL) {
         ret = vhost_memory_region_lookup(dev, iotlb.translated_addr,
-                                         &uaddr, &len);
+                                         &userspace_addr, &len);
         if (ret) {
             error_report("Fail to lookup the translated address "
                          "%"PRIx64, iotlb.translated_addr);
@@ -994,6 +996,12 @@ int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write)
         len = MIN(iotlb.addr_mask + 1, len);
         iova = iova & ~iotlb.addr_mask;
 
+        if (dev->vhost_ops->uaddr_type == VHOST_UADDR_TYPE_GPA) {
+            uaddr = iotlb.translated_addr;
+        } else {
+            uaddr = userspace_addr;
+        }
+
         ret = vhost_backend_update_device_iotlb(dev, iova, uaddr,
                                                 len, iotlb.perm);
         if (ret) {
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index a7a5f22bc6..51c1c08c7a 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -20,6 +20,11 @@ typedef enum VhostBackendType {
     VHOST_BACKEND_TYPE_MAX = 3,
 } VhostBackendType;
 
+typedef enum VhostUaddrType {
+    VHOST_UADDR_TYPE_HVA = 0,
+    VHOST_UADDR_TYPE_GPA = 1,
+} VhostUaddrType;
+
 struct vhost_dev;
 struct vhost_log;
 struct vhost_memory;
@@ -87,6 +92,7 @@ typedef int (*vhost_send_device_iotlb_msg_op)(struct vhost_dev *dev,
 
 typedef struct VhostOps {
     VhostBackendType backend_type;
+    VhostUaddrType uaddr_type;
     vhost_backend_init vhost_backend_init;
     vhost_backend_cleanup vhost_backend_cleanup;
     vhost_backend_memslots_limit vhost_backend_memslots_limit;
-- 
2.14.3

[Qemu-devel] [PATCH 3/3] vhost-user: no more leak QEMU virtual addresses to user backend

[Maxime Coquelin]

The user backends use user address from VHOST_USER_SET_MEM_TABLE
to be able to handle VHOST_USER_SET_VRING_ADDR and
VHOST_USER_IOTLB_MSG payloads.

Now that Vhost code supports the use of Guest physical addresses
instead of QEMU process virtual addresses, let's do the switch
to avoid leaking QEMU process VAs to the user backend.

Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 hw/virtio/vhost-user.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 5ebeb8401b..e783d88afe 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -317,7 +317,8 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
                                      &offset);
         fd = memory_region_get_fd(mr);
         if (fd > 0) {
-            msg.payload.memory.regions[fd_num].user_addr = reg->userspace_addr;
+            /* Use GPA as user address not to leak QEMU VAs to the backend */
+            msg.payload.memory.regions[fd_num].user_addr = reg->guest_phys_addr;
             msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
             msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
             msg.payload.memory.regions[fd_num].mmap_offset = offset;
@@ -924,7 +925,7 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
 
 const VhostOps user_ops = {
         .backend_type = VHOST_BACKEND_TYPE_USER,
-        .uaddr_type = VHOST_UADDR_TYPE_HVA,
+        .uaddr_type = VHOST_UADDR_TYPE_GPA,
         .vhost_backend_init = vhost_user_init,
         .vhost_backend_cleanup = vhost_user_cleanup,
         .vhost_backend_memslots_limit = vhost_user_memslots_limit,
-- 
2.14.3

Re: [Qemu-devel] [PATCH 0/3] Vhost: no more leak QEMU virtual addresses to user backend

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On Tue, Dec 19, 2017 at 07:11:26PM +0100, Maxime Coquelin wrote:
> Before this series, QEMU process virtual addresses are sent to the
> user backend as user addresses.
> 
> Passing these virtual addresses aren't useful, as the backend doesn't
> direct access to QEMU address space.
> 
> It does make sense however for kernel backend, which can access QEMU
> address space.
> 
> This series introduce a new enum set by the backend stating whether it
> prefers using QEMU Virtual addresses or Guest physical addresses as
> User address, and make vhost-user backend to use Guest physical
> addresses.
> 
> Maxime Coquelin (3):
>   vhost-user: rename VhostUserMemory userspace_addr field to user_addr
>   vhost: introduce backend's user address type
>   vhost-user: no more leak QEMU virtual addresses to user backend
> 
>  hw/virtio/vhost-backend.c         |  1 +
>  hw/virtio/vhost-user.c            |  6 ++++--
>  hw/virtio/vhost.c                 | 16 ++++++++++++----
>  include/hw/virtio/vhost-backend.h |  6 ++++++
>  4 files changed, 23 insertions(+), 6 deletions(-)
> 
> -- 
> 2.14.3
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 0/3] Vhost: no more leak QEMU virtual addresses to user backend

[Michael S. Tsirkin]

On Wed, Dec 20, 2017 at 04:07:41PM +0000, Stefan Hajnoczi wrote:
> On Tue, Dec 19, 2017 at 07:11:26PM +0100, Maxime Coquelin wrote:
> > Before this series, QEMU process virtual addresses are sent to the
> > user backend as user addresses.
> > 
> > Passing these virtual addresses aren't useful, as the backend doesn't
> > direct access to QEMU address space.
> > 
> > It does make sense however for kernel backend, which can access QEMU
> > address space.
> > 
> > This series introduce a new enum set by the backend stating whether it
> > prefers using QEMU Virtual addresses or Guest physical addresses as
> > User address, and make vhost-user backend to use Guest physical
> > addresses.
> > 
> > Maxime Coquelin (3):
> >   vhost-user: rename VhostUserMemory userspace_addr field to user_addr
> >   vhost: introduce backend's user address type
> >   vhost-user: no more leak QEMU virtual addresses to user backend
> > 
> >  hw/virtio/vhost-backend.c         |  1 +
> >  hw/virtio/vhost-user.c            |  6 ++++--
> >  hw/virtio/vhost.c                 | 16 ++++++++++++----
> >  include/hw/virtio/vhost-backend.h |  6 ++++++
> >  4 files changed, 23 insertions(+), 6 deletions(-)
> > 
> > -- 
> > 2.14.3
> > 
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

This makes make check fail. Any idea why?

Re: [Qemu-devel] [PATCH 0/3] Vhost: no more leak QEMU virtual addresses to user backend

[Maxime Coquelin]

On 12/21/2017 04:53 AM, Michael S. Tsirkin wrote:
> On Wed, Dec 20, 2017 at 04:07:41PM +0000, Stefan Hajnoczi wrote:
>> On Tue, Dec 19, 2017 at 07:11:26PM +0100, Maxime Coquelin wrote:
>>> Before this series, QEMU process virtual addresses are sent to the
>>> user backend as user addresses.
>>>
>>> Passing these virtual addresses aren't useful, as the backend doesn't
>>> direct access to QEMU address space.
>>>
>>> It does make sense however for kernel backend, which can access QEMU
>>> address space.
>>>
>>> This series introduce a new enum set by the backend stating whether it
>>> prefers using QEMU Virtual addresses or Guest physical addresses as
>>> User address, and make vhost-user backend to use Guest physical
>>> addresses.
>>>
>>> Maxime Coquelin (3):
>>>    vhost-user: rename VhostUserMemory userspace_addr field to user_addr
>>>    vhost: introduce backend's user address type
>>>    vhost-user: no more leak QEMU virtual addresses to user backend
>>>
>>>   hw/virtio/vhost-backend.c         |  1 +
>>>   hw/virtio/vhost-user.c            |  6 ++++--
>>>   hw/virtio/vhost.c                 | 16 ++++++++++++----
>>>   include/hw/virtio/vhost-backend.h |  6 ++++++
>>>   4 files changed, 23 insertions(+), 6 deletions(-)
>>>
>>> -- 
>>> 2.14.3
>>>
>>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> This makes make check fail. Any idea why?
> 

That's interesting, it fails because the rings aren't initialized with
vhost-user-test (even without my series, but we don't notice).

It fails in vhost_virtqueue_start(), because vq->desc is NULL.

Adding some debug prints shows that vq->desc_phys, vq->avail_phys and 
vq->used_phys are all 0 for both queues.

So when using QEMU VAs as user addresses, vq->desc, vq->used and
vq->avail are translated to a non-NULL address, and it doesn't fail.

Only the multiqueue test returns non-zero and different addresses
between the rings.

I'm looking into it.

Regards,
Maxime

Re: [Qemu-devel] [PATCH 0/3] Vhost: no more leak QEMU virtual addresses to user backend

[Maxime Coquelin]

On 12/21/2017 03:01 PM, Maxime Coquelin wrote:
> 
> 
> On 12/21/2017 04:53 AM, Michael S. Tsirkin wrote:
>> On Wed, Dec 20, 2017 at 04:07:41PM +0000, Stefan Hajnoczi wrote:
>>> On Tue, Dec 19, 2017 at 07:11:26PM +0100, Maxime Coquelin wrote:
>>>> Before this series, QEMU process virtual addresses are sent to the
>>>> user backend as user addresses.
>>>>
>>>> Passing these virtual addresses aren't useful, as the backend doesn't
>>>> direct access to QEMU address space.
>>>>
>>>> It does make sense however for kernel backend, which can access QEMU
>>>> address space.
>>>>
>>>> This series introduce a new enum set by the backend stating whether it
>>>> prefers using QEMU Virtual addresses or Guest physical addresses as
>>>> User address, and make vhost-user backend to use Guest physical
>>>> addresses.
>>>>
>>>> Maxime Coquelin (3):
>>>>    vhost-user: rename VhostUserMemory userspace_addr field to user_addr
>>>>    vhost: introduce backend's user address type
>>>>    vhost-user: no more leak QEMU virtual addresses to user backend
>>>>
>>>>   hw/virtio/vhost-backend.c         |  1 +
>>>>   hw/virtio/vhost-user.c            |  6 ++++--
>>>>   hw/virtio/vhost.c                 | 16 ++++++++++++----
>>>>   include/hw/virtio/vhost-backend.h |  6 ++++++
>>>>   4 files changed, 23 insertions(+), 6 deletions(-)
>>>>
>>>> -- 
>>>> 2.14.3
>>>>
>>>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>
>> This makes make check fail. Any idea why?
>>
> 
> That's interesting, it fails because the rings aren't initialized with
> vhost-user-test (even without my series, but we don't notice).
> 
> It fails in vhost_virtqueue_start(), because vq->desc is NULL.
> 
> Adding some debug prints shows that vq->desc_phys, vq->avail_phys and 
> vq->used_phys are all 0 for both queues.
> 
> So when using QEMU VAs as user addresses, vq->desc, vq->used and
> vq->avail are translated to a non-NULL address, and it doesn't fail.
> 
> Only the multiqueue test returns non-zero and different addresses
> between the rings.
> 
> I'm looking into it.

I get it now, qvirtqueue_setup() is only called in the multiqueue test.

> Regards,
> Maxime