* [PATCH 19/29] drm/qxl: switch to drm_*{get,put} helpers
From: Cihangir Akturk @ 2017-08-03 11:58 UTC (permalink / raw)
Cc: David Airlie, linux-kernel, dri-devel, virtualization,
Cihangir Akturk, Dave Airlie
In-Reply-To: <1501761585-11757-1-git-send-email-cakturk@gmail.com>
drm_*_reference() and drm_*_unreference() functions are just
compatibility alias for drm_*_get() and drm_*_put() adn should not be
used by new code. So convert all users of compatibility functions to use
the new APIs.
Signed-off-by: Cihangir Akturk <cakturk@gmail.com>
---
drivers/gpu/drm/qxl/qxl_display.c | 4 ++--
drivers/gpu/drm/qxl/qxl_dumb.c | 2 +-
drivers/gpu/drm/qxl/qxl_fb.c | 6 +++---
drivers/gpu/drm/qxl/qxl_gem.c | 2 +-
drivers/gpu/drm/qxl/qxl_ioctl.c | 4 ++--
drivers/gpu/drm/qxl/qxl_object.c | 6 +++---
6 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 03fe182..7aa5097 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -306,7 +306,7 @@ void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
{
struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
- drm_gem_object_unreference_unlocked(qxl_fb->obj);
+ drm_gem_object_put_unlocked(qxl_fb->obj);
drm_framebuffer_cleanup(fb);
kfree(qxl_fb);
}
@@ -1109,7 +1109,7 @@ qxl_user_framebuffer_create(struct drm_device *dev,
ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj, &qxl_fb_funcs);
if (ret) {
kfree(qxl_fb);
- drm_gem_object_unreference_unlocked(obj);
+ drm_gem_object_put_unlocked(obj);
return NULL;
}
diff --git a/drivers/gpu/drm/qxl/qxl_dumb.c b/drivers/gpu/drm/qxl/qxl_dumb.c
index 5e65d5d..a0c9e73 100644
--- a/drivers/gpu/drm/qxl/qxl_dumb.c
+++ b/drivers/gpu/drm/qxl/qxl_dumb.c
@@ -81,6 +81,6 @@ int qxl_mode_dumb_mmap(struct drm_file *file_priv,
return -ENOENT;
qobj = gem_to_qxl_bo(gobj);
*offset_p = qxl_bo_mmap_offset(qobj);
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
return 0;
}
diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
index 573e7e9..47a25b0 100644
--- a/drivers/gpu/drm/qxl/qxl_fb.c
+++ b/drivers/gpu/drm/qxl/qxl_fb.c
@@ -95,7 +95,7 @@ static void qxlfb_destroy_pinned_object(struct drm_gem_object *gobj)
qxl_bo_kunmap(qbo);
qxl_bo_unpin(qbo);
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
}
int qxl_get_handle_for_primary_fb(struct qxl_device *qdev,
@@ -320,11 +320,11 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev,
qxl_bo_unpin(qbo);
}
if (fb && ret) {
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
drm_framebuffer_cleanup(fb);
kfree(fb);
}
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
return ret;
}
diff --git a/drivers/gpu/drm/qxl/qxl_gem.c b/drivers/gpu/drm/qxl/qxl_gem.c
index 85f5467..f5c1e78 100644
--- a/drivers/gpu/drm/qxl/qxl_gem.c
+++ b/drivers/gpu/drm/qxl/qxl_gem.c
@@ -98,7 +98,7 @@ int qxl_gem_object_create_with_handle(struct qxl_device *qdev,
return r;
/* drop reference from allocate - handle holds it now */
*qobj = gem_to_qxl_bo(gobj);
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
return 0;
}
diff --git a/drivers/gpu/drm/qxl/qxl_ioctl.c b/drivers/gpu/drm/qxl/qxl_ioctl.c
index 0b82a87..dfefd45 100644
--- a/drivers/gpu/drm/qxl/qxl_ioctl.c
+++ b/drivers/gpu/drm/qxl/qxl_ioctl.c
@@ -121,7 +121,7 @@ static int qxlhw_handle_to_bo(struct drm_file *file_priv, uint64_t handle,
qobj = gem_to_qxl_bo(gobj);
ret = qxl_release_list_add(release, qobj);
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
if (ret)
return ret;
@@ -342,7 +342,7 @@ static int qxl_update_area_ioctl(struct drm_device *dev, void *data,
qxl_bo_unreserve(qobj);
out:
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
return ret;
}
diff --git a/drivers/gpu/drm/qxl/qxl_object.c b/drivers/gpu/drm/qxl/qxl_object.c
index 9a7eef7..cdf1455 100644
--- a/drivers/gpu/drm/qxl/qxl_object.c
+++ b/drivers/gpu/drm/qxl/qxl_object.c
@@ -211,13 +211,13 @@ void qxl_bo_unref(struct qxl_bo **bo)
if ((*bo) == NULL)
return;
- drm_gem_object_unreference_unlocked(&(*bo)->gem_base);
+ drm_gem_object_put_unlocked(&(*bo)->gem_base);
*bo = NULL;
}
struct qxl_bo *qxl_bo_ref(struct qxl_bo *bo)
{
- drm_gem_object_reference(&bo->gem_base);
+ drm_gem_object_get(&bo->gem_base);
return bo;
}
@@ -316,7 +316,7 @@ void qxl_bo_force_delete(struct qxl_device *qdev)
list_del_init(&bo->list);
mutex_unlock(&qdev->gem.mutex);
/* this should unref the ttm bo */
- drm_gem_object_unreference_unlocked(&bo->gem_base);
+ drm_gem_object_put_unlocked(&bo->gem_base);
}
}
--
2.7.4
^ permalink raw reply related
* [PATCH 27/29] drm/virtio: switch to drm_*{get,put} helpers
From: Cihangir Akturk @ 2017-08-03 11:58 UTC (permalink / raw)
Cc: David Airlie, linux-kernel, dri-devel, virtualization,
Cihangir Akturk
In-Reply-To: <1501761585-11757-1-git-send-email-cakturk@gmail.com>
drm_*_reference() and drm_*_unreference() functions are just
compatibility alias for drm_*_get() and drm_*_put() adn should not be
used by new code. So convert all users of compatibility functions to use
the new APIs.
Signed-off-by: Cihangir Akturk <cakturk@gmail.com>
---
drivers/gpu/drm/virtio/virtgpu_display.c | 4 ++--
drivers/gpu/drm/virtio/virtgpu_gem.c | 4 ++--
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 14 +++++++-------
3 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c
index d51bd45..ef2d450 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -53,7 +53,7 @@ static void virtio_gpu_user_framebuffer_destroy(struct drm_framebuffer *fb)
struct virtio_gpu_framebuffer *virtio_gpu_fb
= to_virtio_gpu_framebuffer(fb);
- drm_gem_object_unreference_unlocked(virtio_gpu_fb->obj);
+ drm_gem_object_put_unlocked(virtio_gpu_fb->obj);
drm_framebuffer_cleanup(fb);
kfree(virtio_gpu_fb);
}
@@ -326,7 +326,7 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev,
ret = virtio_gpu_framebuffer_init(dev, virtio_gpu_fb, mode_cmd, obj);
if (ret) {
kfree(virtio_gpu_fb);
- drm_gem_object_unreference_unlocked(obj);
+ drm_gem_object_put_unlocked(obj);
return NULL;
}
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index cc025d8..78da210 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -72,7 +72,7 @@ int virtio_gpu_gem_create(struct drm_file *file,
*obj_p = &obj->gem_base;
/* drop reference from allocate - handle holds it now */
- drm_gem_object_unreference_unlocked(&obj->gem_base);
+ drm_gem_object_put_unlocked(&obj->gem_base);
*handle_p = handle;
return 0;
@@ -137,7 +137,7 @@ int virtio_gpu_mode_dumb_mmap(struct drm_file *file_priv,
return -ENOENT;
obj = gem_to_virtio_gpu_obj(gobj);
*offset_p = virtio_gpu_object_mmap_offset(obj);
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
return 0;
}
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index b94bd54..461f81a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -86,7 +86,7 @@ static void virtio_gpu_unref_list(struct list_head *head)
bo = buf->bo;
qobj = container_of(bo, struct virtio_gpu_object, tbo);
- drm_gem_object_unreference_unlocked(&qobj->gem_base);
+ drm_gem_object_put_unlocked(&qobj->gem_base);
}
}
@@ -261,7 +261,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
ret = virtio_gpu_object_attach(vgdev, qobj, res_id, NULL);
} else {
/* use a gem reference since unref list undoes them */
- drm_gem_object_reference(&qobj->gem_base);
+ drm_gem_object_get(&qobj->gem_base);
mainbuf.bo = &qobj->tbo;
list_add(&mainbuf.head, &validate_list);
@@ -304,7 +304,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
}
return ret;
}
- drm_gem_object_unreference_unlocked(obj);
+ drm_gem_object_put_unlocked(obj);
rc->res_handle = res_id; /* similiar to a VM address */
rc->bo_handle = handle;
@@ -341,7 +341,7 @@ static int virtio_gpu_resource_info_ioctl(struct drm_device *dev, void *data,
ri->size = qobj->gem_base.size;
ri->res_handle = qobj->hw_res_handle;
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
return 0;
}
@@ -389,7 +389,7 @@ static int virtio_gpu_transfer_from_host_ioctl(struct drm_device *dev,
out_unres:
virtio_gpu_object_unreserve(qobj);
out:
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
return ret;
}
@@ -439,7 +439,7 @@ static int virtio_gpu_transfer_to_host_ioctl(struct drm_device *dev, void *data,
out_unres:
virtio_gpu_object_unreserve(qobj);
out:
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
return ret;
}
@@ -462,7 +462,7 @@ static int virtio_gpu_wait_ioctl(struct drm_device *dev, void *data,
nowait = true;
ret = virtio_gpu_object_wait(qobj, nowait);
- drm_gem_object_unreference_unlocked(gobj);
+ drm_gem_object_put_unlocked(gobj);
return ret;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Michael S. Tsirkin @ 2017-08-03 14:22 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mawilcox, amit.shah,
liliang.opensource, linux-kernel, mhocko, linux-mm, yang.zhang.wz,
quan.xu, cornelia.huck, pbonzini, akpm, virtualization, mgorman
In-Reply-To: <1501742299-4369-4-git-send-email-wei.w.wang@intel.com>
On Thu, Aug 03, 2017 at 02:38:17PM +0800, Wei Wang wrote:
> Add a new feature, VIRTIO_BALLOON_F_SG, which enables the transfer
> of balloon (i.e. inflated/deflated) pages using scatter-gather lists
> to the host.
>
> The implementation of the previous virtio-balloon is not very
> efficient, because the balloon pages are transferred to the
> host one by one. Here is the breakdown of the time in percentage
> spent on each step of the balloon inflating process (inflating
> 7GB of an 8GB idle guest).
>
> 1) allocating pages (6.5%)
> 2) sending PFNs to host (68.3%)
> 3) address translation (6.1%)
> 4) madvise (19%)
>
> It takes about 4126ms for the inflating process to complete.
> The above profiling shows that the bottlenecks are stage 2)
> and stage 4).
>
> This patch optimizes step 2) by transferring pages to the host in
> sgs. An sg describes a chunk of guest physically continuous pages.
> With this mechanism, step 4) can also be optimized by doing address
> translation and madvise() in chunks rather than page by page.
>
> With this new feature, the above ballooning process takes ~541ms
> resulting in an improvement of ~87%.
>
> TODO: optimize stage 1) by allocating/freeing a chunk of pages
> instead of a single page each time.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/virtio/virtio_balloon.c | 150 ++++++++++++++++++++++++++++++++----
> include/uapi/linux/virtio_balloon.h | 1 +
> 2 files changed, 134 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index f0b3a0b..29aca0c 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -32,6 +32,7 @@
> #include <linux/mm.h>
> #include <linux/mount.h>
> #include <linux/magic.h>
> +#include <linux/xbitmap.h>
>
> /*
> * Balloon device works in 4K page units. So each page is pointed to by
> @@ -79,6 +80,9 @@ struct virtio_balloon {
> /* Synchronize access/update to this struct virtio_balloon elements */
> struct mutex balloon_lock;
>
> + /* The xbitmap used to record ballooned pages */
> + struct xb page_xb;
> +
> /* The array of pfns we tell the Host about. */
> unsigned int num_pfns;
> __virtio32 pfns[VIRTIO_BALLOON_ARRAY_PFNS_MAX];
> @@ -141,13 +145,90 @@ static void set_page_pfns(struct virtio_balloon *vb,
> page_to_balloon_pfn(page) + i);
> }
>
> +static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
> + void *addr, uint32_t size)
> +{
> + struct scatterlist sg;
> + unsigned int len;
> +
> + sg_init_one(&sg, addr, size);
> + while (unlikely(virtqueue_add_inbuf(vq, &sg, 1, vb, GFP_KERNEL)
> + == -ENOSPC)) {
> + /*
> + * It is uncommon to see the vq is full, because the sg is sent
> + * one by one and the device is able to handle it in time. But
> + * if that happens, we kick and wait for an entry is released.
is released -> to get used.
> + */
> + virtqueue_kick(vq);
> + while (!virtqueue_get_buf(vq, &len) &&
> + !virtqueue_is_broken(vq))
> + cpu_relax();
Please rework to use wait_event in that case too.
> + }
> + virtqueue_kick(vq);
> + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> +}
> +
> +/*
> + * Send balloon pages in sgs to host. The balloon pages are recorded in the
> + * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
> + * The page xbitmap is searched for continuous "1" bits, which correspond
> + * to continuous pages, to chunk into sgs.
> + *
> + * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
> + * need to be searched.
> + */
> +static void tell_host_sgs(struct virtio_balloon *vb,
> + struct virtqueue *vq,
> + unsigned long page_xb_start,
> + unsigned long page_xb_end)
> +{
> + unsigned long sg_pfn_start, sg_pfn_end;
> + void *sg_addr;
> + uint32_t sg_len, sg_max_len = round_down(UINT_MAX, PAGE_SIZE);
> +
> + sg_pfn_start = page_xb_start;
> + while (sg_pfn_start < page_xb_end) {
> + sg_pfn_start = xb_find_next_bit(&vb->page_xb, sg_pfn_start,
> + page_xb_end, 1);
> + if (sg_pfn_start == page_xb_end + 1)
> + break;
> + sg_pfn_end = xb_find_next_bit(&vb->page_xb, sg_pfn_start + 1,
> + page_xb_end, 0);
> + sg_addr = pfn_to_kaddr(sg_pfn_start);
> + sg_len = (sg_pfn_end - sg_pfn_start) << PAGE_SHIFT;
> + while (sg_len > sg_max_len) {
> + send_one_sg(vb, vq, sg_addr, sg_max_len);
> + sg_addr += sg_max_len;
> + sg_len -= sg_max_len;
> + }
> + send_one_sg(vb, vq, sg_addr, sg_len);
> + xb_zero(&vb->page_xb, sg_pfn_start, sg_pfn_end);
> + sg_pfn_start = sg_pfn_end + 1;
> + }
> +}
> +
> +static inline void xb_set_page(struct virtio_balloon *vb,
> + struct page *page,
> + unsigned long *pfn_min,
> + unsigned long *pfn_max)
> +{
> + unsigned long pfn = page_to_pfn(page);
> +
> + *pfn_min = min(pfn, *pfn_min);
> + *pfn_max = max(pfn, *pfn_max);
> + xb_set_bit(&vb->page_xb, pfn);
> +}
> +
> static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> {
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> unsigned num_allocated_pages;
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + if (!use_sg)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> mutex_lock(&vb->balloon_lock);
> for (vb->num_pfns = 0; vb->num_pfns < num;
> @@ -162,7 +243,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
> msleep(200);
> break;
> }
> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> + if (use_sg)
> + xb_set_page(vb, page, &pfn_min, &pfn_max);
> + else
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
> if (!virtio_has_feature(vb->vdev,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> @@ -171,8 +257,12 @@ static unsigned fill_balloon(struct virtio_balloon *vb, size_t num)
>
> num_allocated_pages = vb->num_pfns;
> /* Did we get any? */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->inflate_vq);
> + if (vb->num_pfns) {
> + if (use_sg)
> + tell_host_sgs(vb, vb->inflate_vq, pfn_min, pfn_max);
> + else
> + tell_host(vb, vb->inflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> return num_allocated_pages;
> @@ -198,9 +288,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> struct page *page;
> struct balloon_dev_info *vb_dev_info = &vb->vb_dev_info;
> LIST_HEAD(pages);
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> + unsigned long pfn_max = 0, pfn_min = ULONG_MAX;
>
> - /* We can only do one array worth at a time. */
> - num = min(num, ARRAY_SIZE(vb->pfns));
> + /* Traditionally, we can only do one array worth at a time. */
> + if (!use_sg)
> + num = min(num, ARRAY_SIZE(vb->pfns));
>
> mutex_lock(&vb->balloon_lock);
> /* We can't release more pages than taken */
> @@ -210,7 +303,11 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> page = balloon_page_dequeue(vb_dev_info);
> if (!page)
> break;
> - set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> + if (use_sg)
> + xb_set_page(vb, page, &pfn_min, &pfn_max);
> + else
> + set_page_pfns(vb, vb->pfns + vb->num_pfns, page);
> +
> list_add(&page->lru, &pages);
> vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
> }
> @@ -221,8 +318,12 @@ static unsigned leak_balloon(struct virtio_balloon *vb, size_t num)
> * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
> * is true, we *have* to do it in this order
> */
> - if (vb->num_pfns != 0)
> - tell_host(vb, vb->deflate_vq);
> + if (vb->num_pfns) {
> + if (use_sg)
> + tell_host_sgs(vb, vb->deflate_vq, pfn_min, pfn_max);
> + else
> + tell_host(vb, vb->deflate_vq);
> + }
> release_pages_balloon(vb, &pages);
> mutex_unlock(&vb->balloon_lock);
> return num_freed_pages;
> @@ -441,6 +542,7 @@ static int init_vqs(struct virtio_balloon *vb)
> }
>
> #ifdef CONFIG_BALLOON_COMPACTION
> +
> /*
> * virtballoon_migratepage - perform the balloon page migration on behalf of
> * a compation thread. (called under page lock)
> @@ -464,6 +566,7 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> {
> struct virtio_balloon *vb = container_of(vb_dev_info,
> struct virtio_balloon, vb_dev_info);
> + bool use_sg = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_SG);
> unsigned long flags;
>
> /*
> @@ -485,16 +588,24 @@ static int virtballoon_migratepage(struct balloon_dev_info *vb_dev_info,
> vb_dev_info->isolated_pages--;
> __count_vm_event(BALLOON_MIGRATE);
> spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags);
> - vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> - set_page_pfns(vb, vb->pfns, newpage);
> - tell_host(vb, vb->inflate_vq);
> -
> + if (use_sg) {
> + send_one_sg(vb, vb->inflate_vq, page_address(newpage),
> + PAGE_SIZE);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, newpage);
> + tell_host(vb, vb->inflate_vq);
> + }
> /* balloon's page migration 2nd step -- deflate "page" */
> balloon_page_delete(page);
> - vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> - set_page_pfns(vb, vb->pfns, page);
> - tell_host(vb, vb->deflate_vq);
> -
> + if (use_sg) {
> + send_one_sg(vb, vb->deflate_vq, page_address(page),
> + PAGE_SIZE);
> + } else {
> + vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE;
> + set_page_pfns(vb, vb->pfns, page);
> + tell_host(vb, vb->deflate_vq);
> + }
> mutex_unlock(&vb->balloon_lock);
>
> put_page(page); /* balloon reference */
> @@ -553,6 +664,9 @@ static int virtballoon_probe(struct virtio_device *vdev)
> if (err)
> goto out_free_vb;
>
> + if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_SG))
> + xb_init(&vb->page_xb);
> +
> vb->nb.notifier_call = virtballoon_oom_notify;
> vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
> err = register_oom_notifier(&vb->nb);
> @@ -618,6 +732,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
> cancel_work_sync(&vb->update_balloon_size_work);
> cancel_work_sync(&vb->update_balloon_stats_work);
>
> + xb_empty(&vb->page_xb);
> remove_common(vb);
> #ifdef CONFIG_BALLOON_COMPACTION
> if (vb->vb_dev_info.inode)
> @@ -669,6 +784,7 @@ static unsigned int features[] = {
> VIRTIO_BALLOON_F_MUST_TELL_HOST,
> VIRTIO_BALLOON_F_STATS_VQ,
> VIRTIO_BALLOON_F_DEFLATE_ON_OOM,
> + VIRTIO_BALLOON_F_SG,
> };
>
> static struct virtio_driver virtio_balloon_driver = {
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 343d7dd..37780a7 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -34,6 +34,7 @@
> #define VIRTIO_BALLOON_F_MUST_TELL_HOST 0 /* Tell before reclaiming pages */
> #define VIRTIO_BALLOON_F_STATS_VQ 1 /* Memory Stats virtqueue */
> #define VIRTIO_BALLOON_F_DEFLATE_ON_OOM 2 /* Deflate balloon on OOM */
> +#define VIRTIO_BALLOON_F_SG 3 /* Use sg instead of PFN lists */
>
> /* Size of a PFN in the balloon interface. */
> #define VIRTIO_BALLOON_PFN_SHIFT 12
> --
> 2.7.4
^ permalink raw reply
* RE: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Wang, Wei W @ 2017-08-03 15:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mawilcox@microsoft.com, amit.shah@redhat.com,
liliang.opensource@gmail.com, linux-kernel@vger.kernel.org,
mhocko@kernel.org, linux-mm@kvack.org, yang.zhang.wz@gmail.com,
quan.xu@aliyun.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org,
virtualization@lists.linux-foundation.org,
mgorman@techsingularity.net
In-Reply-To: <20170803151212-mutt-send-email-mst@kernel.org>
On Thursday, August 3, 2017 10:23 PM, Michael S. Tsirkin wrote:
> On Thu, Aug 03, 2017 at 02:38:17PM +0800, Wei Wang wrote:
> > +static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
> > + void *addr, uint32_t size)
> > +{
> > + struct scatterlist sg;
> > + unsigned int len;
> > +
> > + sg_init_one(&sg, addr, size);
> > + while (unlikely(virtqueue_add_inbuf(vq, &sg, 1, vb, GFP_KERNEL)
> > + == -ENOSPC)) {
> > + /*
> > + * It is uncommon to see the vq is full, because the sg is sent
> > + * one by one and the device is able to handle it in time. But
> > + * if that happens, we kick and wait for an entry is released.
>
> is released -> to get used.
>
> > + */
> > + virtqueue_kick(vq);
> > + while (!virtqueue_get_buf(vq, &len) &&
> > + !virtqueue_is_broken(vq))
> > + cpu_relax();
>
> Please rework to use wait_event in that case too.
For the balloon page case here, it is fine to use wait_event. But for the free page
case, I think it might not be suitable because the mm lock is being held.
Best,
Wei
^ permalink raw reply
* RE: [PATCH v13 4/5] mm: support reporting free page blocks
From: Wang, Wei W @ 2017-08-03 15:20 UTC (permalink / raw)
To: Michal Hocko
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mawilcox@microsoft.com, amit.shah@redhat.com,
liliang.opensource@gmail.com, mst@redhat.com,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, mgorman@techsingularity.net
In-Reply-To: <20170803135047.GV12521@dhcp22.suse.cz>
On Thursday, August 3, 2017 9:51 PM, Michal Hocko:
> As I've said earlier. Start simple optimize incrementally with some numbers to
> justify a more subtle code.
> --
OK. Let's start with the simple implementation as you suggested.
Best,
Wei
^ permalink raw reply
* RE: [PATCH] MAINTAINERS: copy virtio on balloon_compaction.c
From: Wang, Wei W @ 2017-08-03 15:23 UTC (permalink / raw)
To: Rafael Aquini, Michael S. Tsirkin
Cc: Minchan Kim, Konstantin Khlebnikov, mawilcox@microsoft.com,
Hansen, Dave, linux-kernel@vger.kernel.org, mhocko@kernel.org,
linux-mm@kvack.org, Vlastimil Babka, Andrew Morton,
zhenwei.pi@youruncloud.com,
virtualization@lists.linux-foundation.org, Gioh Kim
In-Reply-To: <20170803133622.GD26205@xps>
On Thursday, August 3, 2017 9:36 PM, Rafael Aquini wrote:
> On Thu, Aug 03, 2017 at 03:42:52PM +0300, Michael S. Tsirkin wrote:
> > Changes to mm/balloon_compaction.c can easily break virtio, and virtio
> > is the only user of that interface. Add a line to MAINTAINERS so
> > whoever changes that file remembers to copy us.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > MAINTAINERS | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index f66488d..6b1d60e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -13996,6 +13996,7 @@ F: drivers/block/virtio_blk.c
> > F: include/linux/virtio*.h
> > F: include/uapi/linux/virtio_*.h
> > F: drivers/crypto/virtio/
> > +F: mm/balloon_compaction.c
> >
> > VIRTIO CRYPTO DRIVER
> > M: Gonglei <arei.gonglei@huawei.com>
> > --
> > MST
>
> Acked-by: Rafael Aquini <aquini@redhat.com>
Acked-by: Wei Wang <wei.w.wang@intel.com>
^ permalink raw reply
* Re: [PATCH v13 3/5] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Michael S. Tsirkin @ 2017-08-03 15:55 UTC (permalink / raw)
To: Wang, Wei W
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mawilcox@microsoft.com, amit.shah@redhat.com,
liliang.opensource@gmail.com, linux-kernel@vger.kernel.org,
mhocko@kernel.org, linux-mm@kvack.org, yang.zhang.wz@gmail.com,
quan.xu@aliyun.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org,
virtualization@lists.linux-foundation.org,
mgorman@techsingularity.net
In-Reply-To: <286AC319A985734F985F78AFA26841F73928C952@shsmsx102.ccr.corp.intel.com>
On Thu, Aug 03, 2017 at 03:17:59PM +0000, Wang, Wei W wrote:
> On Thursday, August 3, 2017 10:23 PM, Michael S. Tsirkin wrote:
> > On Thu, Aug 03, 2017 at 02:38:17PM +0800, Wei Wang wrote:
> > > +static void send_one_sg(struct virtio_balloon *vb, struct virtqueue *vq,
> > > + void *addr, uint32_t size)
> > > +{
> > > + struct scatterlist sg;
> > > + unsigned int len;
> > > +
> > > + sg_init_one(&sg, addr, size);
> > > + while (unlikely(virtqueue_add_inbuf(vq, &sg, 1, vb, GFP_KERNEL)
> > > + == -ENOSPC)) {
> > > + /*
> > > + * It is uncommon to see the vq is full, because the sg is sent
> > > + * one by one and the device is able to handle it in time. But
> > > + * if that happens, we kick and wait for an entry is released.
> >
> > is released -> to get used.
> >
> > > + */
> > > + virtqueue_kick(vq);
> > > + while (!virtqueue_get_buf(vq, &len) &&
> > > + !virtqueue_is_broken(vq))
> > > + cpu_relax();
> >
> > Please rework to use wait_event in that case too.
>
> For the balloon page case here, it is fine to use wait_event. But for the free page
> case, I think it might not be suitable because the mm lock is being held.
>
> Best,
> Wei
You will have to find a way to drop the lock and restart from where you
stopped then.
--
MST
^ permalink raw reply
* Re: [PATCH v13 5/5] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: kbuild test robot @ 2017-08-03 16:11 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mst, amit.shah, liliang.opensource,
mawilcox, linux-kernel, virtualization, linux-mm, yang.zhang.wz,
kbuild-all, quan.xu, cornelia.huck, pbonzini, akpm, mhocko,
mgorman
In-Reply-To: <1501742299-4369-6-git-send-email-wei.w.wang@intel.com>
[-- Attachment #1: Type: text/plain, Size: 2019 bytes --]
Hi Wei,
[auto build test WARNING on v4.13-rc3]
[also build test WARNING on next-20170803]
[cannot apply to linus/master linux/master mmotm/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Wei-Wang/Virtio-balloon-Enhancement/20170803-223740
config: xtensa-allmodconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=xtensa
All warnings (new ones prefixed by >>):
drivers//virtio/virtio_balloon.c: In function 'tell_host_sgs':
drivers//virtio/virtio_balloon.c:210:3: error: implicit declaration of function 'pfn_to_kaddr' [-Werror=implicit-function-declaration]
sg_addr = pfn_to_kaddr(sg_pfn_start);
^
drivers//virtio/virtio_balloon.c:210:11: warning: assignment makes pointer from integer without a cast
sg_addr = pfn_to_kaddr(sg_pfn_start);
^
drivers//virtio/virtio_balloon.c: In function 'virtio_balloon_send_free_pages':
>> drivers//virtio/virtio_balloon.c:523:15: warning: initialization makes pointer from integer without a cast
void *addr = pfn_to_kaddr(pfn);
^
cc1: some warnings being treated as errors
vim +523 drivers//virtio/virtio_balloon.c
518
519 static void virtio_balloon_send_free_pages(void *opaque, unsigned long pfn,
520 unsigned long nr_pages)
521 {
522 struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
> 523 void *addr = pfn_to_kaddr(pfn);
524 uint32_t len = nr_pages << PAGE_SHIFT;
525
526 send_one_sg(vb, vb->free_page_vq, addr, len, 1);
527 }
528
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50920 bytes --]
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v13 4/5] mm: support reporting free page blocks
From: Michael S. Tsirkin @ 2017-08-03 21:02 UTC (permalink / raw)
To: Wang, Wei W
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mawilcox@microsoft.com, amit.shah@redhat.com,
liliang.opensource@gmail.com, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, Michal Hocko,
mgorman@techsingularity.net
In-Reply-To: <286AC319A985734F985F78AFA26841F73928C971@shsmsx102.ccr.corp.intel.com>
On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote:
> On Thursday, August 3, 2017 9:51 PM, Michal Hocko:
> > As I've said earlier. Start simple optimize incrementally with some numbers to
> > justify a more subtle code.
> > --
>
> OK. Let's start with the simple implementation as you suggested.
>
> Best,
> Wei
The tricky part is when you need to drop the lock and
then restart because the device is busy. Would it maybe
make sense to rotate the list so that new head
will consist of pages not yet sent to device?
--
MST
^ permalink raw reply
* Re: [PATCH v13 4/5] mm: support reporting free page blocks
From: Michal Hocko @ 2017-08-04 7:53 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mawilcox@microsoft.com, amit.shah@redhat.com,
liliang.opensource@gmail.com, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, mgorman@techsingularity.net
In-Reply-To: <20170804000043-mutt-send-email-mst@kernel.org>
On Fri 04-08-17 00:02:01, Michael S. Tsirkin wrote:
> On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote:
> > On Thursday, August 3, 2017 9:51 PM, Michal Hocko:
> > > As I've said earlier. Start simple optimize incrementally with some numbers to
> > > justify a more subtle code.
> > > --
> >
> > OK. Let's start with the simple implementation as you suggested.
> >
> > Best,
> > Wei
>
> The tricky part is when you need to drop the lock and
> then restart because the device is busy. Would it maybe
> make sense to rotate the list so that new head
> will consist of pages not yet sent to device?
No, I this should be strictly non-modifying API.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v13 4/5] mm: support reporting free page blocks
From: Wei Wang @ 2017-08-04 8:15 UTC (permalink / raw)
To: Michal Hocko, Michael S. Tsirkin
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mawilcox@microsoft.com, amit.shah@redhat.com,
liliang.opensource@gmail.com, linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, mgorman@techsingularity.net
In-Reply-To: <20170804075337.GC26029@dhcp22.suse.cz>
On 08/04/2017 03:53 PM, Michal Hocko wrote:
> On Fri 04-08-17 00:02:01, Michael S. Tsirkin wrote:
>> On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote:
>>> On Thursday, August 3, 2017 9:51 PM, Michal Hocko:
>>>> As I've said earlier. Start simple optimize incrementally with some numbers to
>>>> justify a more subtle code.
>>>> --
>>> OK. Let's start with the simple implementation as you suggested.
>>>
>>> Best,
>>> Wei
>> The tricky part is when you need to drop the lock and
>> then restart because the device is busy. Would it maybe
>> make sense to rotate the list so that new head
>> will consist of pages not yet sent to device?
> No, I this should be strictly non-modifying API.
Just get the context here for discussion:
spin_lock_irqsave(&zone->lock, flags);
...
visit(opaque2, pfn, 1<<order);
spin_unlock_irqrestore(&zone->lock, flags);
The concern is that the callback may cause the lock be
taken too long.
I think here we can have two options:
- Option 1: Put a Note for the callback: the callback function
should not block and it should finish as soon as possible.
(when implementing an interrupt handler, we also have
such similar rules in mind, right?).
For our use case, the callback just puts the reported page
block to the ring, then returns. If the ring is full as the host
is busy, then I think it should skip this one, and just return.
Because:
A. This is an optimization feature, losing a couple of free
pages to report isn't that important;
B. In reality, I think it's uncommon to see this ring getting
full (I didn't observe ring full in the tests), since the host
(consumer) is notified to take out the page block right
after it is added.
- Option 2: Put the callback function outside the lock
What's input into the callback is just a pfn, and the callback
won't access the corresponding pages. So, I still think it won't
be an issue no matter what status of the pages is after they
are reported (even they doesn't exit due to hot-remove).
What would you guys think?
Best,
Wei
^ permalink raw reply
* Re: [PATCH v13 4/5] mm: support reporting free page blocks
From: Michal Hocko @ 2017-08-04 8:24 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mawilcox@microsoft.com, amit.shah@redhat.com,
liliang.opensource@gmail.com, Michael S. Tsirkin,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, mgorman@techsingularity.net
In-Reply-To: <59842D1C.5020608@intel.com>
On Fri 04-08-17 16:15:24, Wei Wang wrote:
> On 08/04/2017 03:53 PM, Michal Hocko wrote:
> >On Fri 04-08-17 00:02:01, Michael S. Tsirkin wrote:
> >>On Thu, Aug 03, 2017 at 03:20:09PM +0000, Wang, Wei W wrote:
> >>>On Thursday, August 3, 2017 9:51 PM, Michal Hocko:
> >>>>As I've said earlier. Start simple optimize incrementally with some numbers to
> >>>>justify a more subtle code.
> >>>>--
> >>>OK. Let's start with the simple implementation as you suggested.
> >>>
> >>>Best,
> >>>Wei
> >>The tricky part is when you need to drop the lock and
> >>then restart because the device is busy. Would it maybe
> >>make sense to rotate the list so that new head
> >>will consist of pages not yet sent to device?
> >No, I this should be strictly non-modifying API.
>
>
> Just get the context here for discussion:
>
> spin_lock_irqsave(&zone->lock, flags);
> ...
> visit(opaque2, pfn, 1<<order);
> spin_unlock_irqrestore(&zone->lock, flags);
>
> The concern is that the callback may cause the lock be
> taken too long.
>
>
> I think here we can have two options:
> - Option 1: Put a Note for the callback: the callback function
> should not block and it should finish as soon as possible.
> (when implementing an interrupt handler, we also have
> such similar rules in mind, right?).
absolutely
> For our use case, the callback just puts the reported page
> block to the ring, then returns. If the ring is full as the host
> is busy, then I think it should skip this one, and just return.
> Because:
> A. This is an optimization feature, losing a couple of free
> pages to report isn't that important;
> B. In reality, I think it's uncommon to see this ring getting
> full (I didn't observe ring full in the tests), since the host
> (consumer) is notified to take out the page block right
> after it is added.
I thought you only updated a pre allocated bitmat... Anyway, I cannot
comment on this part much as I am not familiar with your usecase.
> - Option 2: Put the callback function outside the lock
> What's input into the callback is just a pfn, and the callback
> won't access the corresponding pages. So, I still think it won't
> be an issue no matter what status of the pages is after they
> are reported (even they doesn't exit due to hot-remove).
This would make the API implementation more complex and I am not yet
convinced we really need that.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v13 4/5] mm: support reporting free page blocks
From: Wei Wang @ 2017-08-04 8:55 UTC (permalink / raw)
To: Michal Hocko
Cc: aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
kvm@vger.kernel.org, mawilcox@microsoft.com, amit.shah@redhat.com,
liliang.opensource@gmail.com, Michael S. Tsirkin,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
yang.zhang.wz@gmail.com, quan.xu@aliyun.com,
cornelia.huck@de.ibm.com, pbonzini@redhat.com,
akpm@linux-foundation.org, mgorman@techsingularity.net
In-Reply-To: <20170804082423.GG26029@dhcp22.suse.cz>
On 08/04/2017 04:24 PM, Michal Hocko wrote:
>
>> For our use case, the callback just puts the reported page
>> block to the ring, then returns. If the ring is full as the host
>> is busy, then I think it should skip this one, and just return.
>> Because:
>> A. This is an optimization feature, losing a couple of free
>> pages to report isn't that important;
>> B. In reality, I think it's uncommon to see this ring getting
>> full (I didn't observe ring full in the tests), since the host
>> (consumer) is notified to take out the page block right
>> after it is added.
> I thought you only updated a pre allocated bitmat... Anyway, I cannot
> comment on this part much as I am not familiar with your usecase.
>
Actually the bitmap is in the hypervisor (host). The callback puts the
(pfn,size) on a ring which is shared with the hypervisor, then the
hypervisor takes that info from the ring and updates that bitmap.
Best,
Wei
^ permalink raw reply
* Re: [PATCH] virtio_blk: fix incorrect message when disk is resized
From: Stefan Hajnoczi @ 2017-08-04 9:37 UTC (permalink / raw)
To: linux-kernel; +Cc: virtualization, Michael S. Tsirkin
In-Reply-To: <20170726143223.2769-1-stefanha@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 2558 bytes --]
On Wed, Jul 26, 2017 at 03:32:23PM +0100, Stefan Hajnoczi wrote:
> The message printed on disk resize is incorrect. The following is
> printed when resizing to 2 GiB:
>
> $ truncate -s 1G test.img
> $ qemu -device virtio-blk-pci,logical_block_size=4096,...
> (qemu) block_resize drive1 2G
>
> virtio_blk virtio0: new size: 4194304 4096-byte logical blocks (17.2 GB/16.0 GiB)
>
> The virtio_blk capacity config field is in 512-byte sector units
> regardless of logical_block_size as per the VIRTIO specification.
> Therefore the message should read:
>
> virtio_blk virtio0: new size: 524288 4096-byte logical blocks (2.15 GB/2.0 GiB)
>
> Note that this only affects the printed message. Thankfully the actual
> block device has the correct size because the block layer expects
> capacity in sectors.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> drivers/block/virtio_blk.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
Ping?
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 4e02aa5fdac0..69a2d1748743 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -381,6 +381,7 @@ static void virtblk_config_changed_work(struct work_struct *work)
> struct request_queue *q = vblk->disk->queue;
> char cap_str_2[10], cap_str_10[10];
> char *envp[] = { "RESIZE=1", NULL };
> + unsigned long long nblocks;
> u64 capacity;
>
> /* Host must always specify the capacity. */
> @@ -393,16 +394,19 @@ static void virtblk_config_changed_work(struct work_struct *work)
> capacity = (sector_t)-1;
> }
>
> - string_get_size(capacity, queue_logical_block_size(q),
> + nblocks = DIV_ROUND_UP_ULL(capacity, queue_logical_block_size(q) >> 9);
> +
> + string_get_size(nblocks, queue_logical_block_size(q),
> STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
> - string_get_size(capacity, queue_logical_block_size(q),
> + string_get_size(nblocks, queue_logical_block_size(q),
> STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
>
> dev_notice(&vdev->dev,
> - "new size: %llu %d-byte logical blocks (%s/%s)\n",
> - (unsigned long long)capacity,
> - queue_logical_block_size(q),
> - cap_str_10, cap_str_2);
> + "new size: %llu %d-byte logical blocks (%s/%s)\n",
> + nblocks,
> + queue_logical_block_size(q),
> + cap_str_10,
> + cap_str_2);
>
> set_capacity(vblk->disk, capacity);
> revalidate_disk(vblk->disk);
> --
> 2.13.3
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH] virtio_blk: fix incorrect message when disk is resized
From: Michael S. Tsirkin @ 2017-08-04 13:24 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: linux-kernel, virtualization
In-Reply-To: <20170804093754.GA9867@stefanha-x1.localdomain>
On Fri, Aug 04, 2017 at 10:37:54AM +0100, Stefan Hajnoczi wrote:
> On Wed, Jul 26, 2017 at 03:32:23PM +0100, Stefan Hajnoczi wrote:
> > The message printed on disk resize is incorrect. The following is
> > printed when resizing to 2 GiB:
> >
> > $ truncate -s 1G test.img
> > $ qemu -device virtio-blk-pci,logical_block_size=4096,...
> > (qemu) block_resize drive1 2G
> >
> > virtio_blk virtio0: new size: 4194304 4096-byte logical blocks (17.2 GB/16.0 GiB)
> >
> > The virtio_blk capacity config field is in 512-byte sector units
> > regardless of logical_block_size as per the VIRTIO specification.
> > Therefore the message should read:
> >
> > virtio_blk virtio0: new size: 524288 4096-byte logical blocks (2.15 GB/2.0 GiB)
> >
> > Note that this only affects the printed message. Thankfully the actual
> > block device has the correct size because the block layer expects
> > capacity in sectors.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > drivers/block/virtio_blk.c | 16 ++++++++++------
> > 1 file changed, 10 insertions(+), 6 deletions(-)
>
> Ping?
Oops - I thought I sent it up. Will do.
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 4e02aa5fdac0..69a2d1748743 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -381,6 +381,7 @@ static void virtblk_config_changed_work(struct work_struct *work)
> > struct request_queue *q = vblk->disk->queue;
> > char cap_str_2[10], cap_str_10[10];
> > char *envp[] = { "RESIZE=1", NULL };
> > + unsigned long long nblocks;
> > u64 capacity;
> >
> > /* Host must always specify the capacity. */
> > @@ -393,16 +394,19 @@ static void virtblk_config_changed_work(struct work_struct *work)
> > capacity = (sector_t)-1;
> > }
> >
> > - string_get_size(capacity, queue_logical_block_size(q),
> > + nblocks = DIV_ROUND_UP_ULL(capacity, queue_logical_block_size(q) >> 9);
> > +
> > + string_get_size(nblocks, queue_logical_block_size(q),
> > STRING_UNITS_2, cap_str_2, sizeof(cap_str_2));
> > - string_get_size(capacity, queue_logical_block_size(q),
> > + string_get_size(nblocks, queue_logical_block_size(q),
> > STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
> >
> > dev_notice(&vdev->dev,
> > - "new size: %llu %d-byte logical blocks (%s/%s)\n",
> > - (unsigned long long)capacity,
> > - queue_logical_block_size(q),
> > - cap_str_10, cap_str_2);
> > + "new size: %llu %d-byte logical blocks (%s/%s)\n",
> > + nblocks,
> > + queue_logical_block_size(q),
> > + cap_str_10,
> > + cap_str_2);
> >
> > set_capacity(vblk->disk, capacity);
> > revalidate_disk(vblk->disk);
> > --
> > 2.13.3
> >
^ permalink raw reply
* [RFC] virtio-iommu version 0.4
From: Jean-Philippe Brucker @ 2017-08-04 18:19 UTC (permalink / raw)
To: iommu, kvm, virtualization, virtio-dev
Cc: lorenzo.pieralisi, mst, marc.zyngier, will.deacon, eric.auger,
robin.murphy, eric.auger.pro
This is the continuation of my proposal for virtio-iommu, the para-
virtualized IOMMU. Here is a summary of the changes since last time [1]:
* The virtio-iommu document now resembles an actual specification. It is
split into a formal description of the virtio device, and implementation
notes. Please find sources and binaries at [2].
* Added a probe request to describe to the guest different properties that
do not fit in firmware or in the virtio config space. This is a
necessary stepping stone for extending the virtio-iommu.
* There is a working Qemu prototype [3], thanks to Eric Auger and Bharat
Bhushan.
You can find the Linux driver and kvmtool device at [4] and [5]. I
plan to rework driver and kvmtool device slightly before sending the
patches.
To understand the virtio-iommu, I advise to first read introduction and
motivation, then skim through implementation notes and finally look at the
device specification.
I wasn't sure how to organize the review. For those who prefer to comment
inline, I attached v0.4 of device-operations.tex and topology.tex+MSI.tex
to this thread. They are the biggest chunks of the document. But LaTeX
isn't very pleasant to read, so you can simply send a list of comments in
relation to section numbers and a few words of context, we'll manage.
---
Version numbers 0.1-0.4 are arbitrary. I'm hoping they allow to compare
more easily differences since the RFC (see [6]), but haven't been made
public so far. This is the first public posting since initial proposal
[1], and the following describes all changes.
## v0.1 ##
Content is the same as the RFC, but formatted to LaTeX. 'make' generates
one PDF and one HTML document.
## v0.2 ##
Add introductions, improve topology example and firmware description based
on feedback and a number of useful discussions.
## v0.3 ##
Add normative sections (MUST, SHOULD, etc). Clarify some things, tighten
the device and driver behaviour. Unmap semantics are consolidated; they
are now closer to VFIO Type1 v2 semantics.
## v0.4 ##
Introduce PROBE requests. They provide per-endpoint information to the
driver that couldn't be described otherwise.
For the moment, they allow to handle MSIs on x86 virtual platforms (see
3.2). To do that we communicate reserved IOVA regions, that will also be
useful for describing regions that cannot be mapped for a given endpoint,
for instance addresses that correspond to a PCI bridge window.
Introducing such a large framework for this tiny feature may seem
overkill, but it is needed for future extensions of the virtio-iommu and I
believe it really is worth the effort.
## Future ##
Other extensions are in preparation. I won't detail them here because v0.4
already is a lot to digest, but in short, building on top of PROBE:
* First, since the IOMMU is paravirtualized, the device can expose some
properties of the physical topology to the guest, and let it allocate
resources more efficiently. For example, when the virtio-iommu manages
both physical and emulated endpoints, with different underlying IOMMUs,
we now have a way to describe multiple page and block granularities,
instead of forcing the guest to use the most restricted one for all
endpoints. This will most likely be in v0.5.
* Then on top of that, a major improvement will describe hardware
acceleration features available to the guest. There is what I call "Page
Table Handover" (or simply, from the host POV, "Nested"), the ability
for the guest to manipulate its own page tables instead of sending
MAP/UNMAP requests to the host. This, along with IO Page Fault
reporting, will also permit SVM virtualization on different platforms.
Thanks,
Jean
[1] http://www.spinics.net/lists/kvm/msg147990.html
[2] git://linux-arm.org/virtio-iommu.git branch viommu/v0.4
http://www.linux-arm.org/git?p=virtio-iommu.git;a=blob;f=dist/v0.4/virtio-iommu-v0.4.pdf
I reiterate the disclaimers: don't use this document as a reference,
it's a draft. It's also not an OASIS document yet. It may be riddled
with mistakes. As this is a working draft, it is unstable and I do not
guarantee backward compatibility of future versions.
[3] https://lists.gnu.org/archive/html/qemu-arm/2017-08/msg00004.html
[4] git://linux-arm.org/linux-jpb.git virtio-iommu/v0.4
Warning: UAPI headers have changed! They didn't follow the spec,
please update. (Use branch v0.1, that has the old headers, for the
Qemu prototype [3])
[5] git://linux-arm.org/kvmtool-jpb.git virtio-iommu/v0.4
Warning: command-line has changed! Use --viommu vfio[,opts] and
--viommu virtio[,opts] to instantiate a device.
[6] http://www.linux-arm.org/git?p=virtio-iommu.git;a=tree;f=dist/diffs
^ permalink raw reply
* [RFC] virtio-iommu v0.4 - IOMMU Device
From: Jean-Philippe Brucker @ 2017-08-04 18:19 UTC (permalink / raw)
To: iommu, kvm, virtualization, virtio-dev
Cc: lorenzo.pieralisi, mst, marc.zyngier, will.deacon, eric.auger,
robin.murphy, eric.auger.pro
In-Reply-To: <20170804181927.12148-1-jean-philippe.brucker@arm.com>
The following is roughly the content of device-operations.tex
---
\section{IOMMU device}\label{sec:Device Types / IOMMU Device}
The virtio-iommu device manages Direct Memory Access (DMA) from one or
more endpoints. It may act as a proxy for multiple physical IOMMUs
managing devices assigned to the guest, and as standalone IOMMU for
virtual devices.
The driver first discovers endpoints managed by the virtio-iommu device
using standard firmware mechanisms. It then sends requests to create
virtual address spaces and virtual-to-physical mappings for these
endpoints. In its simplest form, the virtio-iommu supports four request
types:
\begin{enumerate}
\item Create an address space and attach an endpoint to it. \\
\texttt{attach(device = 0x104, address space = 1)}
\item Create a mapping between a range of guest-virtual and guest-physical
address. \\
\texttt{map(address space = 1, virt = 0x1000, phys = 0xa000,
size = 0x1000, flags = READ)}
Endpoint 0x104, for example a hardware PCI endpoint, can now read at
addresses 0x1000-0x1fff. These accesses are translated into
system-physical addresses by the IOMMU.
\item Remove the mapping.\\
\texttt{unmap(address space = 1, virt = 0x1000, size = 0x1000)}
Any access to addresses 0x1000-0x1fff by endpoint 0x104 would now be
rejected.
\item Detach the device and remove the address space.\\
\texttt{detach(device = 0x104)}
\end{enumerate}
\subsection{Device ID}\label{sec:Device Types / IOMMU Device / Device ID}
TBD. During development, use 61216.
\subsection{Virtqueues}\label{sec:Device Types / IOMMU Device / Virtqueues}
\begin{description}
\item[0] requestq
\end{description}
\subsection{Feature bits}\label{sec:Device Types / IOMMU Device / Feature bits}
\begin{description}
\item[VIRTIO_IOMMU_F_INPUT_RANGE (0)]
Available range of virtual addresses is described in \field{input_range}
\item[VIRTIO_IOMMU_F_IOASID_BITS (1)]
The number of address spaces supported is described in \field{ioasid_bits}
\item[VIRTIO_IOMMU_F_MAP_UNMAP (2)]
Map and unmap requests are available.\footnote{Future extensions may add
different modes of operations. At the moment, only
VIRTIO_IOMMU_F_MAP_UNMAP is supported.}
\item[VIRTIO_IOMMU_F_BYPASS (3)]
When not attached to an address space, endpoints downstream of the IOMMU
can access the guest-physical address space.
\item[VIRTIO_IOMMU_F_PROBE (4)]
Probe request is available.
\end{description}
\drivernormative{\subsubsection}{Feature bits}{Device Types / IOMMU Device / Feature bits}
The driver SHOULD accept any of the VIRTIO_IOMMU_F_INPUT_RANGE,
VIRTIO_IOMMU_F_IOASID_BITS, VIRTIO_IOMMU_F_MAP_UNMAP and
VIRTIO_IOMMU_F_PROBE feature bits if offered by the device.
% XXX F_MAP_UNMAP will be optional when introducing PTH. But a 0.2 driver
% must implement it (otherwise the device is useless)
\devicenormative{\subsubsection}{Feature bits}{Device Types / IOMMU Device / Feature bits}
If the device offers any of VIRTIO_IOMMU_F_INPUT_RANGE,
VIRTIO_IOMMU_F_IOASID_BITS or VIRTIO_IOMMU_F_PROBE feature bits, and if
the driver did not accept this feature bit, then the device MAY signal
failure by failing to set FEATURES_OK \field{device status} bit when the
driver writes it.
If the device offers the VIRTIO_IOMMU_F_MAP_UNMAP feature bit, and if the
driver did not accept this feature bit, then the device SHOULD behave as
if the feature was negotiated.
% This takes into account all the following "If the F_MAP_UNMAP feature
% was negotiated..."
% If the driver supports F_PTH but not F_MAP_UNMAP, then the driver MUST
% give up upon seeing that the 0.2 device doesn't support F_PTH. If it
% supports F_MAP_UNMAP, then it SHOULD use F_MAP_UNMAP (this is described
% in "Driver Requirements: Feature Bits")
% If the driver supports F_PTH but the device doesn't, and the driver
% stupidly sets F_PTH but not F_MAP_UNMAP, then the device SHOULD reject
% any PTH request and PTH flag in attach.
% When not using the legacy interface, if the driver doesn't negotiate
% F_MAP_UNMAP, then the device may disable it an reject any request.
\subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / IOMMU Device / Feature bits / Legacy Interface: Feature bits}
When using the legacy interface, transitional devices MUST support guests
which do not negotiate any of the VIRTIO_IOMMU_F_INPUT_RANGE,
VIRTIO_IOMMU_F_IOASID_BITS, VIRTIO_IOMMU_F_MAP_UNMAP or
VIRTIO_IOMMU_F_PROBE features, and MUST behave as if the feature was
negotiated.
%\subsection{Feature Bits Requirements}\label{sec:Device Types / IOMMU Device / Feature bits requirements}
\subsection{Device configuration layout}\label{sec:Device Types / IOMMU Device / Device configuration layout}
The \field{page_size_mask} field is always present. Availability of the
others depend on various feature bits as indicated above.
\begin{lstlisting}
struct virtio_iommu_config {
u64 page_size_mask;
struct virtio_iommu_range {
u64 start;
u64 end;
} input_range;
u8 ioasid_bits;
u8 padding[3];
u32 probe_size;
};
\end{lstlisting}
\drivernormative{\subsubsection}{Device configuration layout}{Device Types / IOMMU Device / Device configuration layout}
The driver MUST NOT write to device configuration fields.
\devicenormative{\subsubsection}{Device configuration layout}{Device Types / IOMMU Device / Device configuration layout}
The device SHOULD set \field{padding} to zero.
The device MUST set at least one bit in \field{page_size_mask}, describing
the page granularity. The device MAY set more than one bit in
\field{page_size_mask}.
\subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device Types / IOMMU Device / Device configuration layout / Legacy Interface: Device configuration layout}
When using the legacy interface, transitional devices and drivers
MUST format the fields in struct virtio_iommu_config
according to the native endian of the guest rather than
(necessarily when not using the legacy interface) little-endian.
\subsection{Device initialization}\label{sec:Device Types / IOMMU Device / Device initialization}
When the device is reset, endpoints are not attached to any address space.
If the VIRTIO_IOMMU_F_BYPASS feature is negotiated, all endpoints can
access guest-physical addresses ("bypass mode"). If the feature is not
negotiated, then any memory access from endpoints will fault. Upon
attaching an endpoint in bypass mode to a new address space, any memory
access from the endpoint will fault, since the address space does not
contain any mapping.
The driver chooses operating mode depending on its capabilities. In this
revision of the virtio-iommu specification, the only supported mode is
VIRTIO_IOMMU_F_MAP_UNMAP.
\drivernormative{\subsubsection}{Device Initialization}{Device Types / IOMMU Device / Device Initialization}
The driver MUST NOT negotiate VIRTIO_IOMMU_F_MAP_UNMAP if it is incapable
of sending VIRTIO_IOMMU_T_MAP and VIRTIO_IOMMU_T_UNMAP requests.
If the VIRTIO_IOMMU_F_PROBE feature is offered, the driver SHOULD send a
VIRTIO_IOMMU_T_PROBE request for each endpoint before attaching the
endpoint to an address space.
\devicenormative{\subsubsection}{Device Initialization}{Device Types / IOMMU Device / Device Initialization}
If the driver does not accept the VIRTIO_IOMMU_F_BYPASS feature, the
device SHOULD NOT let endpoints access the guest-physical address space.
However, the device MAY let endpoints access memory regions negotiated
with VIRTIO_IOMMU_PROBE_RESV_MEM_T_BYPASS (see
\ref{devicenormative:Device Types / IOMMU Device / Device operations / PROBE properties / RESV_MEM}).
\subsection{Device operations}\label{sec:Device Types / IOMMU Device / Device operations}
Driver send requests on the request virtqueue, notifies the device and
waits for the device to return the request with a status in the used ring.
All requests are split in two parts: one device-readable, one device-
writable. Each request is therefore described with at least two
descriptors, as illustrated below.
\begin{figure}[htb]
\centering
\includegraphics[width=0.7\textwidth]{img/request-wrapping.png}
\caption{Anatomy of a virtio-iommu request}
\end{figure}
\begin{lstlisting}
struct virtio_iommu_req_head {
u8 type;
u8 reserved[3];
};
struct virtio_iommu_req_tail {
u8 status;
u8 reserved[3];
};
\end{lstlisting}
\rfc{% TODO
There is a problem with framing using multiple chains... If a request
isn't recognized by the device and is scattered across multiple descriptor
chains, how would the device know where this request ends and where the
next one begins? It assumes that a transition from WO descriptors to RO
descriptors is a new request, ok. But if this unknown request is from a
future extension that uses interleaved RO, WO, RO, WO descriptors for a
single request, then the device is doomed.\\
The extension will have to force that third buffer to start with 0 so our
device, that thinks it's a new request, doesn't recognize the request type
and waits for the next one.\\
Alternatively, we could add a 16-bit 'size' field into
virtio_iommu_req_head. We'd loose some space for future flags, and force
requests to be at most 64k, or 256k if we drop size bits [1:0]. (head
couldn't be extended in the future, with a field to describe a bigger
size, because our legacy device here would ignore it.) We'd have to resort
to multiple "container" requests transporting a single big one.\\
Personally I think we can let future extensions deal with interleaved
RO/WO/RO descriptors, but do you think a 'size' field would be better?
}
Type may be one of:
\begin{lstlisting}
#define VIRTIO_IOMMU_T_ATTACH 1
#define VIRTIO_IOMMU_T_DETACH 2
#define VIRTIO_IOMMU_T_MAP 3
#define VIRTIO_IOMMU_T_UNMAP 4
#define VIRTIO_IOMMU_T_PROBE 5
\end{lstlisting}
A few general-purpose status codes are defined here. Unless explicitly
described in a \textbf{Requirements} section, these values are hints to
make troubleshooting easier.
When the device fails to parse a request, for instance if a request seems
too small for its type and the device cannot find the tail, then it will
be unable to set \field{status}. In that case, it should return the
buffers without writing in them.
\begin{lstlisting}
/* All good! Carry on. */
#define VIRTIO_IOMMU_S_OK 0
/* Virtio communication error */
#define VIRTIO_IOMMU_S_IOERR 1
/* Unsupported request */
#define VIRTIO_IOMMU_S_UNSUPP 2
/* Internal device error */
#define VIRTIO_IOMMU_S_DEVERR 3
/* Invalid parameters */
#define VIRTIO_IOMMU_S_INVAL 4
/* Out-of-range parameters */
#define VIRTIO_IOMMU_S_RANGE 5
/* Entry not found */
#define VIRTIO_IOMMU_S_NOENT 6
/* Bad address */
#define VIRTIO_IOMMU_S_FAULT 7
\end{lstlisting}
Range limits of some request fields are described in the device
configuration:
\begin{itemize}
\item \field{page_size_mask} contains the bitmask of all page sizes that
can be mapped. The least significant bit set defines the page
granularity of IOMMU mappings. Other bits in the mask are hints
describing page sizes that the IOMMU can merge into a single mapping
(page blocks).
The smallest page granularity supported by the IOMMU is one byte. It is
legal for the driver to map one byte at a time if bit 0 of
\field{page_size_mask} is set.
\item If the VIRTIO_IOMMU_F_IOASID_BITS feature is offered,
\field{ioasid_bits} contains the number of bits supported in an I/O
Address Space ID, the identifier used in most requests. A value of 0 is
valid, and means that a single address space is supported.
If the feature is not negotiated, address space identifiers can use up
to 32 bits.
\item If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered,
\field{input_range} contains the virtual address range that the IOMMU is
able to translate. Any mapping request to virtual addresses outside of
this range will fail.
If the feature is not negotiated, virtual mappings span over the whole
64-bit address space (\texttt{start = 0, end = 0xffffffff ffffffff})
\end{itemize}
\drivernormative{\subsubsection}{Device operations}{Device Types / IOMMU Device / Device operations}
The driver SHOULD set reserved fields of the head and the tail of a
request to zero.
When a device returns a complete request in the used queue without having
written to it, the driver SHOULD interpret it as a failure from the device
to parse the request.
If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered, the driver SHOULD
NOT send requests with \field{virt_addr} less than
\field{input_range.start} or greater than \field{input_range.end}.
If the VIRTIO_IOMMU_F_IOASID_BITS feature is offered, the driver SHOULD
NOT send requests with \field{address_space} greater than the size
described by \field{ioasid_bits}.
% We mandate truncation to allow a future extension X.Y that would store
% information in addresses and address space IDs.
%
% If device is 0.2 and driver is X.Y, then device ignores ext. bits. But
% if device is X.Y and device is 0.2, then driver *might* set ext. bits to
% garbage. But this extension would be negotiated with a feature bit
% anyway. If it's not, then device must assume that driver is 0.2 and must
% keep truncating the fields.
\devicenormative{\subsubsection}{Device operations}{Device Types / IOMMU Device / Device operations}
The device SHOULD NOT set \field{status} to VIRTIO_IOMMU_S_OK if a request
didn't succeed. \footnote{%
For IMPLEMENTATION DEFINED values of 'succeed'... For example,
virtio_iommu_req_detach.reserved is allowed to be non-zero. If it is
non-zero, the device may consider it to be a failure and abort the
request. Or it may go on with the detach and return OK.}
If a request \field{type} is not recognized, the device SHOULD return the
buffers on the used ring and set the \field{len} field of the used element
to zero.
The device MUST ignore reserved fields of the head and the tail of a
request.
If the VIRTIO_IOMMU_F_INPUT_RANGE feature is offered, the device MUST
truncate the range described by \field{virt_addr} and \field{size} in
requests to fit in the range described by \field{input_range}.
If the VIRTIO_IOMMU_F_IOASID_BITS is offered, the device MUST ignore bits
above \field{ioasid_bits} in field \field{address_space} of requests.
\subsubsection{ATTACH request}\label{sec:Device Types / IOMMU Device / Device operations / ATTACH request}
\begin{lstlisting}
struct virtio_iommu_req_attach {
le32 address_space;
le32 device;
le32 reserved;
};
\end{lstlisting}
Attach an endpoint to an address space. \field{address_space} is an
identifier unique to the virtio-iommu device. If the address space doesn't
exist in the device, it is created. \field{device} is an endpoint
identifier unique to the virtio-iommu device. The host communicates unique
device IDs to the guest using methods outside the scope of this
specification, but the following rules apply:
\begin{itemize}
\item The device ID is unique from the virtio-iommu point of view. Multiple
endpoints whose DMA transactions are not translated by the same
virtio-iommu may have the same device ID. Endpoints whose DMA
transactions may be translated by the same virtio-iommu must have
different device IDs.
\item Sometimes the host cannot completely isolate two endpoints from each
others. For example on a legacy PCI bus, endpoints can snoop DMA
transactions from their neighbours. In this case, the host must
communicate to the guest that it cannot isolate these endpoints from
each others, or that the physical IOMMU cannot distinguish transactions
coming from these endpoints. The method used to communicate this is
outside the scope of this specification.
\end{itemize}
Multiple endpoints may be added to the same address space. An endpoint
cannot be attached to multiple address spaces in VIRTIO_IOMMU_F_MAP_UNMAP
mode.
\drivernormative{\paragraph}{ATTACH request}{Device Types / IOMMU Device / Device operations / ATTACH request}
The driver SHOULD set \field{reserved} to zero.
The driver SHOULD ensure that endpoints that cannot be isolated by the
host are attached to the same address space.
\devicenormative{\paragraph}{ATTACH request}{Device Types / IOMMU Device / Device operations / ATTACH request}
If the \field{reserved} field of an ATTACH request is not zero, the device
SHOULD set the request \field{status} to VIRTIO_IOMMU_S_INVAL and SHOULD
NOT attach the endpoint to the address space. \footnote{The device should
validate input of ATTACH requests in case the driver attempts to attach in
a mode that is unimplemented by the device, and would be incompatible with
the modes implemented by the device.}
If the endpoint identified by \field{device} doesn't exist, then the
device SHOULD set the request \field{status} to VIRTIO_IOMMU_S_NOENT.
If another endpoint is already attached to the address space identified by
\field{address_space}, then the device MAY attach the endpoint identified
by \field{device} to the address space. If it cannot do so, the device
MUST set the request \field{status} to VIRTIO_IOMMU_S_UNSUPP.
If the endpoint identified by \field{device} is already attached to
another address space, then the device SHOULD first detach it from that
address space and attach it to the one identified by
\field{address_space}. In that case the device behaves as if the driver
issued a DETACH request with this \field{device}, followed by the ATTACH
request. If the device cannot do so, it MUST set the request
\field{status} to VIRTIO_IOMMU_S_UNSUPP.
\subsubsection{DETACH request}
\begin{lstlisting}
struct virtio_iommu_req_detach {
le32 device;
le32 reserved;
};
\end{lstlisting}
Detach an endpoint from its address space. When this request completes,
the endpoint cannot access any mapping from that address space anymore.
After all endpoints have been successfully detached from an address space,
it ceases to exist and its ID can be reused by the driver for another
address space.
\drivernormative{\paragraph}{DETACH request}{Device Types / IOMMU Device / Device operations / DETACH request}
The driver SHOULD set \field{reserved} to zero.
\devicenormative{\paragraph}{DETACH request}{Device Types / IOMMU Device / Device operations / DETACH request}
If the \field{reserved} field of an DETACH request is not zero, the device
MAY set the request \field{status} to VIRTIO_IOMMU_S_INVAL, in which case
the device MAY perform the DETACH operation.
% If it returns OK, the device SHOULD go on with the detach, as required
% by the VIRTIO_IOMMU_S_OK rule.
If the endpoint identified by \field{device} doesn't exist, then the
device SHOULD set the request \field{status} to VIRTIO_IOMMU_S_NOENT.
If the endpoint identified by \field{device} wasn't attached to any
address space, then the device MAY set the request \field{status} to
VIRTIO_IOMMU_S_INVAL.
The device MUST ensure that after being detached from an address space,
the endpoint cannot access any mapping from that address space.
\subsubsection{MAP request}\label{sec:Device Types / IOMMU Device / Device operations / MAP request}
\begin{lstlisting}
struct virtio_iommu_req_map {
le32 address_space;
le64 phys_addr;
le64 virt_addr;
le64 size;
le32 flags;
};
/* Flags are: */
#define VIRTIO_IOMMU_MAP_F_READ (1 << 0)
#define VIRTIO_IOMMU_MAP_F_WRITE (1 << 1)
#define VIRTIO_IOMMU_MAP_F_EXEC (1 << 2)
\end{lstlisting}
Map a range of virtually-contiguous addresses to a range of
physically-contiguous addresses of the same size. After the request
succeeds, all endpoints attached to this address space can access memory
in the range $[phys\_addr; phys\_addr + size[$. For example, if an endpoint
accesses address $VA \in [virt\_addr; virt\_addr + size[$, the device (or the
physical IOMMU) translates the address: $PA = VA - virt\_addr +
phys\_addr$. If the access parameters are compatible with \field{flags}
(for instance, the access is write and \field{flags} are
VIRTIO_IOMMU_MAP_F_READ | VIRTIO_IOMMU_MAP_F_WRITE) then the IOMMU allows
the access to reach $PA$.
The range defined by (\field{virt_addr}, \field{size}) must be within the
limits specified by \field{input_range}. The range defined by
(\field{phys_addr}, \field{size}) must be within the guest-physical
address space. This includes upper and lower limits, as well as any
carving of guest-physical addresses for use by the host (for instance MSI
doorbells). Guest physical boundaries are set by the host using a firmware
mechanism outside the scope of this specification.
\begin{note}
This format prevents from creating the identity mapping in a single
request \texttt{[0x0; 0xfff....fff] $\rightarrow$ [0x0; 0xfff...fff]},
since it would result in a size of zero. Hopefully allowing
VIRTIO_IOMMU_F_BYPASS eliminates the need for issuing such request. It
would also be unlikely to conform to the physical range restrictions
from the previous paragraph.
\end{note}
\begin{note}
On flags: it is unlikely that all possible combinations of flags will be
supported by the physical IOMMU. For instance, $W \& !R$ or $X \& W$ might
be invalid. We do not have a way to advertise supported and implicit (for
instance $W \rightarrow R$) flags or combination thereof for the moment,
you are free to send any suggestions for describing this. Please keep in
mind that we might soon want to add more flags, such as privileged,
device, transient, shared, etc. (whatever these would mean).
\end{note}
This request is only available when VIRTIO_IOMMU_F_MAP_UNMAP has been
negotiated.
\drivernormative{\paragraph}{MAP request}{Device Types / IOMMU Device / Device operations / MAP request}
The driver SHOULD set undefined \field{flags} bits to zero.
\devicenormative{\paragraph}{MAP request}{Device Types / IOMMU Device / Device operations / MAP request}
If \field{virt_addr}, \field{phys_addr} or \field{size} is not aligned on
the page granularity, the device SHOULD set the request \field{status} to
VIRTIO_IOMMU_S_RANGE and SHOULD NOT create the mapping.
If the device doesn't recognize a \field{flags} bit, it SHOULD set the
request \field{status} to VIRTIO_IOMMU_S_INVAL. In this case the device
SHOULD NOT create the mapping. \footnote{Validating the input is important
here, because the driver might be attempting to map with special flags
that the device doesn't recognize. Creating the mapping with incompatible
flags may introduce a security hazard.}
If \field{address_space} does not exist, the device SHOULD set the request
\field{status} to VIRTIO_IOMMU_S_NOENT.
\subsubsection{UNMAP request}\label{sec:Device Types / IOMMU Device / Device operations / UNMAP request}
\begin{lstlisting}
struct virtio_iommu_req_unmap {
le32 address_space;
le64 virt_addr;
le64 size;
le32 reserved;
};
\end{lstlisting}
Unmap a range of addresses mapped with VIRTIO_IOMMU_T_MAP. We define here
a mapping as a virtual region created with a single MAP request. All
mappings covered by the range $[virt\_addr; virt\_addr + size [$ are
removed.
The semantics of unmapping are specified below, and illustrated with the
following requests, assuming each example sequence starts with a blank
address space. We define two pseudocode functions \texttt{map(virt\_addr,
size) -> mapping} and \texttt{unmap(virt\_addr, size)}.
\begin{lstlisting}
(1) unmap(addr=0, size=5) -> succeeds, doesn't unmap anything
(2) a = map(addr=0, size=10);
unmap(0, 10) -> succeeds, unmaps a
(3) a = map(0, 5);
b = map(5, 5);
unmap(0, 10) -> succeeds, unmaps a and b
(4) a = map(0, 10);
unmap(0, 5) -> faults, doesn't unmap anything
(5) a = map(0, 5);
b = map(5, 5);
unmap(0, 5) -> succeeds, unmaps a
(6) a = map(0, 5);
unmap(0, 10) -> succeeds, unmaps a
(7) a = map(0, 5);
b = map(10, 5);
unmap(0, 15) -> succeeds, unmaps a and b
\end{lstlisting}
This request is only available when VIRTIO_IOMMU_F_MAP_UNMAP has been
negotiated.
\drivernormative{\paragraph}{UNMAP request}{Device Types / IOMMU Device / Device operations / UNMAP request}
The driver SHOULD set the \field{reserved} field to zero.
The range, defined by \field{virt_addr} and \field{size}, SHOULD cover one
or more contiguous mappings created with MAP requests. The range MAY spill
over unmapped virtual addresses.
The first address of a range SHOULD either be the first address of a
mapping or be outside any mapping. The last address of a range SHOULD
either be the last address of a mapping or be outside any mapping.
\devicenormative{\paragraph}{UNMAP request}{Device Types / IOMMU Device / Device operations / UNMAP request}
If the \field{reserved} field of an UNMAP request is not zero, the device
MAY set the request \field{status} to VIRTIO_IOMMU_S_INVAL, in which case
the device MAY perform the UNMAP operation.
% If it returns OK, the device SHOULD go on with the unmap, as required by
% the VIRTIO_IOMMU_S_OK rule.
If \field{address_space} does not exist, the device SHOULD set the request
\field{status} to VIRTIO_IOMMU_S_NOENT.
If a mapping affected by the range is not covered in its entirety by the
range (the UNMAP request would split the mapping), then the device SHOULD
set the request \field{status} to VIRTIO_IOMMU_S_RANGE, and SHOULD NOT
remove any mapping.
If part of the range or the full range is not covered by an existing
mapping, then the device SHOULD remove all mappings affected by the range
and set the request \field{status} to VIRTIO_IOMMU_S_OK.
\subsubsection{PROBE request}\label{sec:Device Types / IOMMU Device / Device operations / PROBE request}
If the VIRTIO_IOMMU_F_PROBE feature bit is present, the driver sends a
VIRTIO_IOMMU_T_PROBE request for each endpoint that the virtio-iommu
device manages. This probe is performed before attaching the endpoint to
an address space.
\begin{lstlisting}
struct virtio_iommu_req_probe {
/* Device-readable */
le32 device;
le32 flags;
u8 reserved[60];
/* Device-writable when not ACK */
u8 properties[];
};
/* Flags are: */
#define VIRTIO_IOMMU_PROBE_F_ACK (1 << 0)
\end{lstlisting}
\begin{description}
\item[\field{device}] has the same meaning as in ATTACH and DETACH
requests.
\item[\field{flags}] contain additional information about the request.
The VIRTIO_IOMMU_PROBE_F_ACK flag changes the descriptor chain layout:
when ACK is clear, the \field{properties} field is device-writable;
when it is set, the \field{properties} field is device-readable.
\item[\field{reserved}] is used as padding, so that future extensions can
add fields to the device-readable part.
\item[\field{properties}] contains a list of properties of endpoint
\field{device}, filled by the device. This field is exactly
\field{probe_size} bytes. Each property is described with a type, four
flag bits, a length, and a value:
\begin{lstlisting}
#define VIRTIO_IOMMU_PROBE_PROPERTY_TYPE_MASK 0xfff
#define VIRTIO_IOMMU_PROBE_PROPERTY_F_ACK (1 << 12)
struct virtio_iommu_probe_property {
le16 type;
le16 length;
u8 value[];
};
\end{lstlisting}
\end{description}
The driver allocates a buffer of adequate size for the probe request,
writes \field{device} and adds it to the request queue. The device fills
the \field{properties} field with a list of properties for this endpoint.
The driver parses the first property by reading \field{type}, then
\field{length}. If the driver recognizes \field{type}, it reads and
handles \field{value}. The driver then reads the next property, that is
located $(\field{length} + 4)$ bytes after the beginning of the first one,
and so on. The driver parses all properties until it reaches a NONE
property or the end of \field{properties}.
The upper nibble of property \field{type} is reserved for flags.
Therefore only 4096 types are available. The actual type of a property is
extracted like this:
\begin{lstlisting}
u16 type = le16_to_cpu(property.type) & VIRTIO_IOMMU_PROBE_PROPERTY_TYPE_MASK;
\end{lstlisting}
If a property is correctly understood by the driver, then it sets the ACK
bit in \field{type}:
\begin{lstlisting}
property.type |= cpu_to_le16(VIRTIO_IOMMU_PROBE_PROPERTY_F_ACK);
\end{lstlisting}
Then, to signal to the device which properties are understood, the device
sends the probe again with the VIRTIO_IOMMU_PROBE_F_ACK flag. In all
properties understood and accepted by the driver, \field{type} has the
VIRTIO_IOMMU_PROBE_PROPERTY_F_ACK bit set. The other properties are left
as is.
This second phase of the probe request allows the device to ensure that
all properties crucial for good operations are recognized and handled by
the driver. This is analogous to the initial feature negotiation of virtio
devices: an endpoint property is \emph{offered} by the device to the
driver during the first PROBE, and it is \emph{negotiated} after the
driver acknowledges it during the second PROBE.
Available property types are described in section
\ref{sec:Device Types / IOMMU Device / Device operations / PROBE properties}.
When attaching multiple devices to the same address space, their
properties are combined. \emph{Combination Rules} are given for each
property, and describe the rules to apply when combining properties
obtained during probe.
\drivernormative{\paragraph}{PROBE request}{Device Types / IOMMU Device / Device operations / PROBE request}
The size of \field{properties} MUST be \field{probe_size} bytes.
The driver SHOULD set undefined \field{flags} to zero.
The driver SHOULD set \field{reserved} to zero.
If the driver doesn't recognize the \field{type} of a property, it SHOULD
ignore the property and continue parsing the list.
The driver SHOULD NOT deduce the property length from \field{type}.
If the driver recognizes a property \field{type} and is able to
handle{\footnotemark} the property, then the driver SHOULD set the
VIRTIO_IOMMU_PROBE_PROPERTY_F_ACK bit of that property.
\footnotetext{A driver's ability to handle a property depends on the
property type. Without a specific definition of the ACK requirements for a
given property type, it simply means that the driver read all fields of
that property.}
The driver SHOULD resend the PROBE request with the
VIRTIO_IOMMU_PROBE_F_ACK bit set after parsing and updating the
\field{properties} list. Depending on the properties encountered in the
list, the driver MAY modify some of their fields between the first and
second probe, but it SHOULD NOT modify the \field{length} field or bits
[11:0] of field \field{type}.
\devicenormative{\paragraph}{PROBE request}{Device Types / IOMMU Device / Device operations / PROBE request}
If an undefined bit is set in \field{flags}, the device MAY set the
request \field{status} to VIRTIO_IOMMU_S_INVAL.
If the \field{reserved} field of a PROBE request is not zero, the device
MAY set the request \field{status} to VIRTIO_IOMMU_S_INVAL.
If the endpoint identified by \field{device} doesn't exist, then the
device SHOULD set the request \field{status} to VIRTIO_IOMMU_S_NOENT.
If the device does not offer the VIRTIO_IOMMU_F_PROBE feature, and if the
driver sends a VIRTIO_IOMMU_T_PROBE request, then the device SHOULD return
the buffers on the used ring and set the \field{len} field of the used
element to zero.
The device SHOULD set bits [15:13] of property \field{type} to zero.
The device MUST write the size of \field{value}, in bytes, into
\field{length}.
When two properties follow each others, the device MUST put the second
property exactly $(\field{length} + 4)$ bytes after the beginning of the
first one.
If the device doesn't fill all \field{probe_size} bytes with properties,
it SHOULD terminate the list with a property of type NONE and size 0. The
device MAY fill the remaining bytes of \field{properties}, if any, with
zeroes. If there isn't enough space remaining in \field{properties} to
terminate the list with a complete NONE property (4 bytes), then the
device SHOULD fill the remaining bytes with zeroes.
If the PROBE request has VIRTIO_IOMMU_PROBE_F_ACK bit set, the device MAY
ignore the request and set the request \field{status} to
VIRTIO_IOMMU_S_OK.
\subsubsection{PROBE properties}\label{sec:Device Types / IOMMU Device / Device operations / PROBE properties}
\begin{lstlisting}
#define VIRTIO_IOMMU_PROBE_T_NONE 0
#define VIRTIO_IOMMU_PROBE_T_RESV_MEM 2
\end{lstlisting}
\paragraph{Property NONE}\label{sec:Device Types / IOMMU Device / Device operations / PROBE properties / NONE}
Marks the end of the property list. This property doesn't have any value,
and should have \field{length} 0.
\paragraph{Property RESV_MEM}\label{sec:Device Types / IOMMU Device / Device operations / PROBE properties / RESV_MEM}
The RESV_MEM property describes a chunk of reserved virtual memory. It may
be used by the device to describe virtual address ranges that shouldn't be
allocated by the driver, or that are special.
\begin{lstlisting}
struct virtio_iommu_probe_resv_mem {
u8 subtype;
u8 reserved[3];
le64 addr;
le64 size;
le32 flags;
};
\end{lstlisting}
Fields \field{addr} and \field{size} describe the range of reserved
addresses. \field{subtype} may be one of:
\begin{description}
\item[VIRTIO_IOMMU_PROBE_RESV_MEM_T_ABORT (0)]
Accesses to this region are aborted. This subtype does not accept any
flag.
\item[VIRTIO_IOMMU_PROBE_RESV_MEM_T_BYPASS (1)]
Accesses to this region behave as if the IOMMU was bypassed, and reach
the bus upstream of the IOMMU untranslated.
The following \field{flags} are defined for BYPASS regions:
\begin{description}
\item[VIRTIO_IOMMU_PROBE_RESV_MEM_F_MSI (1)]
Provides a hint to the guest that this is a doorbell for Message
Signaled Interrupts.
If the device doesn't provide such a region, then MSIs are normal
write accesses from the IOMMU point of view, and arbitrary virtual
addresses should be allocated by the driver to map MSI doorbells.
Otherwise, the guest should use the guest-physical doorbell
address when programming MSIs for this endpoint.
\end{description}
%\item[VIRTIO_IOMMU_PROBE_RESV_MEM_T_IDENTITY (2)]
% This region should be identity-mapped by the guest. TODO: is this
% useful for anyone?
\end{description}
\propcombination{\subparagraph}{Property RESV_MEM}{Device Types / IOMMU Device / Device operations / PROBE properties / RESV_MEM}
Multiple overlapping RESV_MEM properties are merged together. Difference
in subtype on the intersecting range doesn't make a difference from the
driver point of view.
\drivernormative{\subparagraph}{Property RESV_MEM}{Device Types / IOMMU Device / Device operations / PROBE properties / RESV_MEM}
The driver SHOULD NOT map any virtual address described by a
VIRTIO_IOMMU_PROBE_RESV_MEM_T_ABORT or
VIRTIO_IOMMU_PROBE_RESV_MEM_T_BYPASS property.
% An old driver that doesn't find or understand this property will
% allocate and map virtual addresses. We really can't do anything about
% that. We're not introducing a regression, MSIs never worked for x86
% before we introduced the F_MSI flag.
The driver SHOULD ignore \field{reserved}.
For a given \field{subtype}, the driver SHOULD ignore undefined
\field{flags} bits.
The driver SHOULD treat any \field{subtype} it doesn't recognize as if it
was VIRTIO_IOMMU_PROBE_RESV_MEM_T_ABORT.
\devicenormative{\subparagraph}{Property RESV_MEM}{Device Types / IOMMU Device / Device operations / PROBE properties / RESV_MEM}
The device SHOULD set \field{reserved} to zero.
For a given \field{subtype}, the device SHOULD set undefined \field{flags}
bits to zero.
The device MAY abort any transaction targeting a
VIRTIO_IOMMU_PROBE_RESV_MEM_T_ABORT region.
If an endpoint is attached to an address space, the device SHOULD leave
any access targeting one of its VIRTIO_IOMMU_PROBE_RESV_MEM_T_BYPASS
regions pass through untranslated. In other words, the device SHOULD
handle such a region as if it was identity-mapped (virtual address equal
to physical address). If the endpoint is not attached to any address
space, then the device MAY abort the transaction.
The device MAY abort any transaction that isn't a write access and that
targets a VIRTIO_IOMMU_PROBE_RESV_MEM_T_BYPASS region with flag
VIRTIO_IOMMU_PROBE_RESV_MEM_F_MSI.
^ permalink raw reply
* [RFC] virtio-iommu v0.4 - Implementation notes
From: Jean-Philippe Brucker @ 2017-08-04 18:19 UTC (permalink / raw)
To: iommu, kvm, virtualization, virtio-dev
Cc: lorenzo.pieralisi, mst, marc.zyngier, will.deacon, eric.auger,
robin.murphy, eric.auger.pro
In-Reply-To: <20170804181927.12148-1-jean-philippe.brucker@arm.com>
The following is roughly the content of topology.tex and MSI.tex
---
\section{Implementation notes}\label{sec:viommu}
\subsection{Virtual system topology}\label{sec:viommu / Virtual topology}
\subsubsection{Example virtual topology}\label{sec:viommu / Virtual topology / Example}
\begin{figure}[htb]
\centering
\includegraphics[width=\textwidth]{img/virtual-topology.png}
\caption{An example IOMMU topology}
\label{fig:viommu / Virtual topology / Topology}
\end{figure}
Diagram~\ref{fig:viommu / Virtual topology / Topology} shows an example
system topology centered around an IOMMU. On the left, the IOMMU manages
traffic from two PCI root complexes. On the right, the IOMMU manages
traffic from three platform devices (or "integrated devices").
Within a PCI domain, devices are identified by a Requester ID. It is a
16-bit identifier also called Bus/Device/Function (BDF). In a BDF, Bus is
8 bits, Device is 5 bits, and Function is 3 bits.
The bottom PCI domain has four endpoints connected to the root complex via
two bridges. The first endpoint is identified by BDF 01:04.0. On the other
bus, the first endpoint is identified by BDF 02:00.0, and the two other
endpoints are two functions of the same device, identified by BDFs 02:01.0
and 02:01.1. The bridges and the root complex may also issue transactions
with BDFs 00:00.0, 00:01.0 and 00:02.0.
In order for the IOMMU to differentiate devices in multiple PCI domains,
the root bridge expands the BDF with a domain ID. In example
\ref{fig:viommu / Virtual topology / Topology},
the PCI domain on top gets ID 0 and the one on the bottom gets ID 1.
Therefore when reaching the IOMMU, a transaction coming from endpoint
01:04.0 (= 0x0120) is identified by Device ID 0x10120.
We define here "platform" devices as endpoints that are on the system bus,
as opposed to behind a PCI host bridge. Unlike PCI devices, platform
devices do not have a standardized identifier scheme to be used with the
IOMMU. Their Device IDs are chosen arbitrarily during system integration
in such way that they don't overlap PCI domains or each others.
\subsubsection{Firmware description}\label{sec:viommu / Virtual topology / Firmware description}
The host describes the relation between IOMMU and devices to the guest
using either device-tree or ACPI. Topology description is outside the
scope of virtio-iommu, because the virtio-iommu does not and should not
need to know about vendor-specific buses. The virtual IOMMU identifies
each virtual endpoint with an abstract 32-bit ID, that is called "Device
ID" in this document\footnote{Other IOMMU architectures use different
names, such as "stream ID" on ARM SMMU or "source ID" on Intel VT-d}.
Device IDs are not necessarily unique system-wide, but they should not
overlap within a single virtio-iommu. Device IDs of physical endpoints do
not need to match IDs seen by the physical IOMMU.
We strongly advise to implement the virtio-iommu using virtio-mmio
transport. Nothing prevents an implementation to use virtio-pci instead,
but existing firmware interfaces do not easily allow to describe an IOMMU
$\leftrightarrow$ master relations between PCI endpoints. Device models in
Operating Systems might not be designed to support such complicated
system.
Device-tree offers a way to describe the IOMMU topology for PCI and
platform devices. Here's an excerpt of the device-tree describing examples
\ref{fig:viommu / Virtual topology / Topology}.
\begin{lstlisting}
/* The virtual IOMMU is described with a virtio-mmio node */
viommu: virtio@9050000 {
compatible = "virtio,mmio";
reg = <0x09050000 0x200>;
dma-coherent;
interrupts = <0x0 0x5 0x1>;
#iommu-cells = <1>
};
/* PCI domain 0 */
pcie@3eff0000 {
...
/* Identity map */
iommu-map = <0x0 &viommu 0x0 0x10000>;
};
/* PCI domain 1 */
pcie@3f000000 {
...
/* Linear map: deviceID = RID + 0x10000 */
iommu-map = <0x0 &viommu 0x10000 0x10000>;
};
someplatformdevice@a000000 {
...
iommus = <&viommu 0x20000>;
};
\end{lstlisting}
For more details, please refer to \hyperref[intro:IOMMU DT Bindings]{[IOMMU DT]}.
In ACPI, the plan would be to add a new node type to the IO Remapping
Table specification \hyperref[intro:ACPI IORT]{[ACPI IORT]}, that provides
a mechanism similar to DT for describing IOMMU topology.
The OS would parse the IORT table to build a map of ID relations between
IOMMU and devices. ID Array is used to find correspondence between IOMMU
IDs and PCI or platform devices. Later on, the virtio-iommu driver finds
the associated LNRO0005 descriptor via the "Device object name" field, and
probes the virtio device to find out more about its capabilities. Since
all properties of the IOMMU will be obtained during virtio probing, the
IORT node can stay simple.
The following table shows the possible\protect\footnotemark\ format for a
paravirtualized IOMMU IORT node.
\footnotetext{This table IS NOT authoritative, only a suggestion.
Such a node would be described in \hyperref[intro:ACPI IORT]{[ACPI IORT]}}.
\begin{center}
\begin{tabular}{| l | l | l | p{.4\textwidth} |}
\hline
\textbf{Field} & \textbf{Length} & \textbf{Offset} & \textbf{Description} \\
\hline
Type & 1 & 0 & 5: Paravirtualized IOMMU \\
\hline
Length & 2 & 1 & The length of the node. \\
\hline
Revision & 1 & 3 & 0 \\
\hline
Reserved & 4 & 4 & Must be zero. \\
\hline
Number of ID mapping & 4 & 8 & \\
\hline
Reference to ID Array & 4 & 12 &
Offset from the start of the ID Array IORT node to the start of its
Array ID mappings.\\
\hline
Model & 4 & 16 & 0: virtio-iommu \\
\hline
Device object name & & 20 &
ASCII Null terminated string with the full path to the entry in the
namespace for this IOMMU. \\
\hline
Padding & & & To keep 32-bit alignment and leave space for future models. \\
\hline
Array of ID mappings & 20xN & & ID Array. \\
\hline
\end{tabular}
\end{center}
---
\subsection{Message Signaled Interrupts}\label{sec:viommu / MSI}
Some buses, such as PCI, implement Message Signaled Interrupts. Instead of
requesting an interrupt via a wire that runs from the endpoint to the irqchip,
the endpoint can request interrupts by performing a memory write to a specific
register (the "doorbell").
By combining the data written to the doorbell, the address itself, and the
originator of the write, the IRQ chip deduces the destination interrupt
number and destination processing units. Additional devices between the
endpoint and the IRQ chip may translate the doorbell address, the IRQ
number and verify that the endpoint is allowed to send this interrupt.
Different platforms implement IRQ remapping and routing in different ways.
This section describes three ways of dealing with Message Signaled
Interrupts in virtio-iommu devices and drivers.
In simplest systems, the endpoint writes the plain interrupt number to the
doorbell, and the IRQ chip signals the interruption to destination CPUs
programmed by software. Section \ref{sec:viommu / MSI / Address bypass}
describes how to implement a simple system with virtio-iommu. Section
\ref{sec:viommu / MSI / Address translation} describes the added complexity
(from the host point of view) of translating the IRQ chip doorbell.
More complex systems add a level of indirection in the MSI message. The address
or data contains an index into a remapping table, that describes interrupt
delivery in details and is programmed by software either into the IRQ chip or
the IOMMU. Section \ref{sec:viommu / MSI / IRQ remapping} describes how to use
the remapping feature of virtio-iommu.
\subsubsection{Address bypass}\label{sec:viommu / MSI / Address bypass}
\begin{figure}[htb]
\centering
\includegraphics{img/MSI-addr-noremap.png}
\caption{MSI remapping with address bypass}
\end{figure}
Bypassing translation for MSIs is the simplest implementation from the host
perspective. The virtio-iommu device has a special IOVA window that it does not
translate. Any access from devices to that region is forwarded upstream of the
IOMMU without being translated or even checked.
The IRQ chip may or may not have an IRQ remapping component. It may be as
simple as generating the interrupt number described in data, without checking
if the device was allowed to send that interrupt. If there is another
component performing the isolation, one might consider translating the
doorbell address superfluous.
With virtio-iommu, the device can advertise the doorbell address as
untranslated by using the PROBE request with a reserved region (see
\ref{sec:Device Types / IOMMU Device / Device operations / PROBE properties / RESV_MEM}).
For example, if the virtual platform has an IRQ remapping module with a
doorbell in the physical address range 0xfee00000-0xfeefffff, then the
device can present the following property to the driver:
\begin{lstlisting}
struct __attribute__((packed)) {
struct virtio_iommu_probe_property head;
struct virtio_iommu_probe_resv_mem mem;
} doorbell = {
.head = {
.type = VIRTIO_IOMMU_PROBE_T_RESV_MEM,
.length = sizeof(doorbell.mem),
},
.mem = {
.subtype = VIRTIO_IOMMU_PROBE_RESV_MEM_T_BYPASS,
.flags = VIRTIO_IOMMU_PROBE_RESV_MEM_F_MSI,
.addr = 0xfee00000,
.size = 0x00100000,
},
};
\end{lstlisting}
\subsubsection{Address translation}\label{sec:viommu / MSI / Address translation}
\begin{figure}[htb]
\centering
\includegraphics{img/MSI-addr-remap.png}
\caption{MSI remapping with address translation}
\end{figure}
On some systems (e.g. ARM-based platforms) the IOMMU does not have a special
MSI window, and MSIs are treated like any other memory write. The MSI address
therefore has to be translated by the IOMMU before reaching the IRQ chip.
Address translation may be used as a rudimentary form of MSI isolation,
but multiple endpoints will typically access the same doorbell. Address
translation can only forbid an endpoint from sending interrupts. If it is
allowed to send MSIs, the endpoint can easily spoof another endpoint by
sending interrupts that were not assigned to it.
From the virtio-iommu point of view, this is the simplest to implement, because
there is no special address range. The whole address space is treated the same
by the virtio-iommu device.
However, this mode of operations may add significant complexity in the host
implementation.
\subsubsection{IRQ remapping}\label{sec:viommu / MSI / IRQ remapping}
Some IOMMUs (e.g. Intel and AMD IOMMUs) are able to remap IRQs themselves.
\begin{figure}[htb]
\centering
\includegraphics{img/MSI-irq-remap.png}
\caption{MSI remapping with address bypass}
\end{figure}
This version of virtio-iommu doesn't support IRQ remapping.
^ permalink raw reply
* Re: [PATCH 00/11] virtio-console: Fine-tuning for 14 function implementations
From: SF Markus Elfring @ 2017-08-06 10:56 UTC (permalink / raw)
To: virtualization, Amit Shah, Arnd Bergmann, Greg Kroah-Hartman,
Michael S. Tsirkin, Rusty Russell
Cc: kernel-janitors, LKML
In-Reply-To: <020438b9-a7f8-0050-04c1-43382ba60b75@users.sourceforge.net>
> Date: Wed, 14 Sep 2016 15:43:21 +0200
>
> Several update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (11):
> Use kmalloc_array() in init_vqs()
> Less function calls in init_vqs() after error detection
> Rename a jump label in init()
> Rename jump labels in virtcons_probe()
> Rename jump labels in add_port()
> Rename a jump label in port_fops_open()
> Rename a jump label in port_fops_splice_write()
> Rename jump labels in port_fops_write()
> Rename a jump label in __send_to_port()
> Rename jump labels in alloc_buf()
> Rename a jump label in five functions
>
> drivers/char/virtio_console.c | 155 ++++++++++++++++++++++++------------------
> 1 file changed, 87 insertions(+), 68 deletions(-)
Would you like to take another look at change possibilities
for this software module?
Regards,
Markus
^ permalink raw reply
* Re: [virtio-dev] repost: af_packet vs virtio (was packed ring layout proposal v2)
From: Adam Tao @ 2017-08-07 4:16 UTC (permalink / raw)
To: Michael S. Tsirkin, g
Cc: virtio-dev, kvm, netdev, john.fastabend, virtualization,
Steven Luong, alexei.starovoitov
In-Reply-To: <20170802164931-mutt-send-email-mst@kernel.org>
On Wed, Aug 02, 2017 at 04:50:03PM +0300, Michael S. Tsirkin wrote:
> On Tue, Aug 01, 2017 at 08:54:27PM -0700, Steven Luong wrote:
> > * Descriptor ring:
> >
> > Guest adds descriptors with unique index values and DESC_HW set in flags.
> > Host overwrites used descriptors with correct len, index, and DESC_HW
> > clear.? Flags are always set/cleared last.
> >
> > #define DESC_HW 0x0080
> >
> > struct desc {
> > ? ? ? ? __le64 addr;
> > ? ? ? ? __le32 len;
> > ? ? ? ? __le16 index;
> > ? ? ? ? __le16 flags;
> > };
> >
> > When DESC_HW is set, descriptor belongs to device. When it is clear,
> > it belongs to the driver.
> >
> > We can use 1 bit to set direction
> > /* This marks a buffer as write-only (otherwise read-only). */
> > #define VRING_DESC_F_WRITE? ? ? 2
> >
> > * Scatter/gather support
> >
> > We can use 1 bit to chain s/g entries in a request, same as virtio 1.0:
> >
> > /* This marks a buffer as continuing via the next field. */
next field seems like a structure field in the software, maybe we need
to change the "next field" to "next desc" to avoid misunderstanding.
> >
> >
> > This comment here is confusing to me. In 1.0, virtq_desc has the next field.
> > When the flag VRING_DESC_F_NEXT is set, the next entry to continue is specified
> > in the next field.
> >
> > Here in 1.1, struct desc does not have the next field, only addr, len, index,
> > and flags. So when VRING_DESC_F_NEXT is set in struct desc's flags field, where
> > is the next entry to continue the current descriptor, the entry immediately
> > following the current entry? ie, if the current entry is at index 10 in the
> > descriptor table and its flags is set for VRING_DESC_F_NEXT, is the entry
> > continuing the current entry in index 11?
> >
> > Steven
>
> Exactly, you got it right.
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply
* Re: [PATCH v13 1/5] Introduce xbitmap
From: Wei Wang @ 2017-08-07 6:58 UTC (permalink / raw)
To: linux-kernel, virtualization, kvm, linux-mm, mst, mhocko,
mawilcox, akpm, willy
Cc: aarcange, virtio-dev, liliang.opensource, amit.shah, pbonzini,
cornelia.huck, quan.xu, yang.zhang.wz, mgorman
In-Reply-To: <1501742299-4369-2-git-send-email-wei.w.wang@intel.com>
On 08/03/2017 02:38 PM, Wei Wang wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
>
> The eXtensible Bitmap is a sparse bitmap representation which is
> efficient for set bits which tend to cluster. It supports up to
> 'unsigned long' worth of bits, and this commit adds the bare bones --
> xb_set_bit(), xb_clear_bit() and xb_test_bit().
>
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> include/linux/radix-tree.h | 2 +
> include/linux/xbitmap.h | 49 ++++++++++++++++
> lib/radix-tree.c | 139 ++++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 188 insertions(+), 2 deletions(-)
> create mode 100644 include/linux/xbitmap.h
>
> diff --git a/include/linux/radix-tree.h b/include/linux/radix-tree.h
> index 3e57350..428ccc9 100644
> --- a/include/linux/radix-tree.h
> +++ b/include/linux/radix-tree.h
Hi Matthew,
Could you please help to upstream this patch?
Best,
Wei
^ permalink raw reply
* Re: [PATCH RESEND] mm: don't zero ballooned pages
From: David Hildenbrand @ 2017-08-07 8:44 UTC (permalink / raw)
To: Wei Wang, linux-kernel, linux-mm, virtualization, mhocko, mst,
zhenwei.pi
Cc: Andrea Arcangeli, dave.hansen, akpm, mawilcox
In-Reply-To: <1501761557-9758-1-git-send-email-wei.w.wang@intel.com>
On 03.08.2017 13:59, Wei Wang wrote:
> This patch is a revert of 'commit bb01b64cfab7 ("mm/balloon_compaction.c:
> enqueue zero page to balloon device")'
>
> Ballooned pages will be marked as MADV_DONTNEED by the hypervisor and
> shouldn't be given to the host ksmd to scan. Therefore, it is not
> necessary to zero ballooned pages, which is very time consuming when
> the page amount is large. The ongoing fast balloon tests show that the
> time to balloon 7G pages is increased from ~491ms to 2.8 seconds with
> __GFP_ZERO added. So, this patch removes the flag.
>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> ---
> mm/balloon_compaction.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index 9075aa5..b06d9fe 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -24,7 +24,7 @@ struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info)
> {
> unsigned long flags;
> struct page *page = alloc_page(balloon_mapping_gfp_mask() |
> - __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_ZERO);
> + __GFP_NOMEMALLOC | __GFP_NORETRY);
> if (!page)
> return NULL;
>
>
Your assumption here is, that the hypervisor will always supply a zero
page. Unfortunately, this assumption is wrong (and it stems from the
lack of different page size support in virtio-balloon).
Think about these examples:
1. Guest is backed by huge pages (hugetbfs). Ballooning kicks in.
MADV_DONTNEED is simply ignored in the hypervisor (hugetlbfs requires
fallocate punshhole). Also, trying to zap 4k on e.g. 1MB pages will
simply be ignored.
2. Guest on PPC uses 4k pages. Hypervisor uses 64k pages. trying to
MADV_DONTNEED 4K on 64k pages will simply be ignored.
So unfortunately, zeroing the page is the right thing to do to cover all
cases.
--
Thanks,
David
^ permalink raw reply
* Re: [PATCH RESEND] mm: don't zero ballooned pages
From: Michal Hocko @ 2017-08-07 9:25 UTC (permalink / raw)
To: David Hildenbrand
Cc: Andrea Arcangeli, mst, mawilcox, dave.hansen, linux-kernel,
virtualization, linux-mm, akpm, zhenwei.pi
In-Reply-To: <9ac31505-0996-2822-752e-8ec055373aa0@redhat.com>
On Mon 07-08-17 10:44:50, David Hildenbrand wrote:
> On 03.08.2017 13:59, Wei Wang wrote:
> > This patch is a revert of 'commit bb01b64cfab7 ("mm/balloon_compaction.c:
> > enqueue zero page to balloon device")'
> >
> > Ballooned pages will be marked as MADV_DONTNEED by the hypervisor and
> > shouldn't be given to the host ksmd to scan. Therefore, it is not
> > necessary to zero ballooned pages, which is very time consuming when
> > the page amount is large. The ongoing fast balloon tests show that the
> > time to balloon 7G pages is increased from ~491ms to 2.8 seconds with
> > __GFP_ZERO added. So, this patch removes the flag.
> >
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > Cc: Michal Hocko <mhocko@kernel.org>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > mm/balloon_compaction.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> > index 9075aa5..b06d9fe 100644
> > --- a/mm/balloon_compaction.c
> > +++ b/mm/balloon_compaction.c
> > @@ -24,7 +24,7 @@ struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info)
> > {
> > unsigned long flags;
> > struct page *page = alloc_page(balloon_mapping_gfp_mask() |
> > - __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_ZERO);
> > + __GFP_NOMEMALLOC | __GFP_NORETRY);
> > if (!page)
> > return NULL;
> >
> >
>
> Your assumption here is, that the hypervisor will always supply a zero
> page. Unfortunately, this assumption is wrong (and it stems from the
> lack of different page size support in virtio-balloon).
>
> Think about these examples:
>
> 1. Guest is backed by huge pages (hugetbfs). Ballooning kicks in.
>
> MADV_DONTNEED is simply ignored in the hypervisor (hugetlbfs requires
> fallocate punshhole). Also, trying to zap 4k on e.g. 1MB pages will
> simply be ignored.
>
> 2. Guest on PPC uses 4k pages. Hypervisor uses 64k pages. trying to
> MADV_DONTNEED 4K on 64k pages will simply be ignored.
>
> So unfortunately, zeroing the page is the right thing to do to cover all
> cases.
Maybe it is my absolute lack of familiarity with what the host actually
does with balloon pages but I fail to see why the above matters at all.
ksm will not try to merge sub page units (4k for hugetlb or a large base
page). And if you need to hide the guest contents then the host can
clear the respective subpage just fine. So could you be more explicit
why MADV_DONTNEED matters at all? Also does any host actually share sub
pages between different guests? This sounds like a bad idea to me in
general.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH RESEND] mm: don't zero ballooned pages
From: Wei Wang @ 2017-08-07 9:35 UTC (permalink / raw)
To: David Hildenbrand, linux-kernel, linux-mm, virtualization, mhocko,
mst, zhenwei.pi
Cc: Andrea Arcangeli, dave.hansen, akpm, mawilcox
In-Reply-To: <9ac31505-0996-2822-752e-8ec055373aa0@redhat.com>
On 08/07/2017 04:44 PM, David Hildenbrand wrote:
> On 03.08.2017 13:59, Wei Wang wrote:
>> This patch is a revert of 'commit bb01b64cfab7 ("mm/balloon_compaction.c:
>> enqueue zero page to balloon device")'
>>
>> Ballooned pages will be marked as MADV_DONTNEED by the hypervisor and
>> shouldn't be given to the host ksmd to scan. Therefore, it is not
>> necessary to zero ballooned pages, which is very time consuming when
>> the page amount is large. The ongoing fast balloon tests show that the
>> time to balloon 7G pages is increased from ~491ms to 2.8 seconds with
>> __GFP_ZERO added. So, this patch removes the flag.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> Cc: Michal Hocko <mhocko@kernel.org>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> ---
>> mm/balloon_compaction.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
>> index 9075aa5..b06d9fe 100644
>> --- a/mm/balloon_compaction.c
>> +++ b/mm/balloon_compaction.c
>> @@ -24,7 +24,7 @@ struct page *balloon_page_enqueue(struct balloon_dev_info *b_dev_info)
>> {
>> unsigned long flags;
>> struct page *page = alloc_page(balloon_mapping_gfp_mask() |
>> - __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_ZERO);
>> + __GFP_NOMEMALLOC | __GFP_NORETRY);
>> if (!page)
>> return NULL;
>>
>>
> Your assumption here is, that the hypervisor will always supply a zero
> page. Unfortunately, this assumption is wrong (and it stems from the
> lack of different page size support in virtio-balloon).
I think this would be something that we can improve the balloon.
For example, the balloon request from the device should be aligned
to the host page size before sending to the guest driver:
On PPC, if the command requests for 140K memory to inflate, it can
be aligned to 128K.
>
> Think about these examples:
>
> 1. Guest is backed by huge pages (hugetbfs). Ballooning kicks in.
>
> MADV_DONTNEED is simply ignored in the hypervisor (hugetlbfs requires
> fallocate punshhole). Also, trying to zap 4k on e.g. 1MB pages will
> simply be ignored.
For the hugetlbfs case, I think the balloon size can be aligned to
the huge page size (i.e 2M or 1GB).
Best,
Wei
^ permalink raw reply
* Re: [PATCH RESEND] mm: don't zero ballooned pages
From: David Hildenbrand @ 2017-08-07 9:37 UTC (permalink / raw)
To: Michal Hocko
Cc: Andrea Arcangeli, mst, mawilcox, dave.hansen, linux-kernel,
virtualization, linux-mm, akpm, zhenwei.pi
In-Reply-To: <20170807092525.GE32434@dhcp22.suse.cz>
> Maybe it is my absolute lack of familiarity with what the host actually
> does with balloon pages but I fail to see why the above matters at all.
> ksm will not try to merge sub page units (4k for hugetlb or a large base
> page). And if you need to hide the guest contents then the host can
> clear the respective subpage just fine. So could you be more explicit
> why MADV_DONTNEED matters at all? Also does any host actually share sub
> pages between different guests? This sounds like a bad idea to me in
> general.
>
Okay, I think I got the issue wrong. I thought that the original patch
tried to also fix a corner case where the guest would assume that it
would get supplied zero pages afterwards. Please ignore the noise. :)
--
Thanks,
David
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox