* [PATCH v22 3/3] virtio-balloon: don't report free pages when page poisoning is enabled
From: Wei Wang @ 2018-01-17 5:10 UTC (permalink / raw)
To: virtio-dev, linux-kernel, virtualization, kvm, linux-mm, mst,
mhocko, akpm
Cc: yang.zhang.wz, riel, quan.xu0, liliang.opensource, pbonzini,
nilal
In-Reply-To: <1516165812-3995-1-git-send-email-wei.w.wang@intel.com>
The guest free pages should not be discarded by the live migration thread
when page poisoning is enabled with PAGE_POISONING_NO_SANITY=n, because
skipping the transfer of such poisoned free pages will trigger false
positive when new pages are allocated and checked on the destination.
This patch adds a config field, poison_val. Guest writes to the config
field to tell the host about the poisoning value. The value will be 0 in
the following cases:
1) PAGE_POISONING_NO_SANITY is enabled;
2) page poisoning is disabled; or
3) PAGE_POISONING_ZERO is enabled.
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Cc: Michal Hocko <mhocko@suse.com>
---
drivers/virtio/virtio_balloon.c | 8 ++++++++
include/uapi/linux/virtio_balloon.h | 2 ++
2 files changed, 10 insertions(+)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index b9561a5..5a42235 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -706,6 +706,7 @@ static struct file_system_type balloon_fs = {
static int virtballoon_probe(struct virtio_device *vdev)
{
struct virtio_balloon *vb;
+ __u32 poison_val;
int err;
if (!vdev->config->get) {
@@ -740,6 +741,13 @@ static int virtballoon_probe(struct virtio_device *vdev)
WQ_FREEZABLE | WQ_CPU_INTENSIVE, 0);
INIT_WORK(&vb->report_free_page_work, report_free_page_func);
vb->stop_cmd_id = VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID;
+ if (IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY) ||
+ !page_poisoning_enabled())
+ poison_val = 0;
+ else
+ poison_val = PAGE_POISON;
+ virtio_cwrite(vb->vdev, struct virtio_balloon_config,
+ poison_val, &poison_val);
}
vb->nb.notifier_call = virtballoon_oom_notify;
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 55e2456..5861876 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -47,6 +47,8 @@ struct virtio_balloon_config {
__u32 actual;
/* Free page report command id, readonly by guest */
__u32 free_page_report_cmd_id;
+ /* Stores PAGE_POISON if page poisoning with sanity check is in use */
+ __u32 poison_val;
};
#define VIRTIO_BALLOON_S_SWAP_IN 0 /* Amount of memory swapped in */
--
2.7.4
^ permalink raw reply related
* [PATCH v22 2/3] virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
From: Wei Wang @ 2018-01-17 5:10 UTC (permalink / raw)
To: virtio-dev, linux-kernel, virtualization, kvm, linux-mm, mst,
mhocko, akpm
Cc: yang.zhang.wz, riel, quan.xu0, liliang.opensource, pbonzini,
nilal
In-Reply-To: <1516165812-3995-1-git-send-email-wei.w.wang@intel.com>
Negotiation of the VIRTIO_BALLOON_F_FREE_PAGE_VQ feature indicates the
support of reporting hints of guest free pages to host via virtio-balloon.
Host requests the guest to report free pages by sending a new cmd
id to the guest via the free_page_report_cmd_id configuration register.
When the guest starts to report, the first element added to the free page
vq is the cmd id given by host. When the guest finishes the reporting
of all the free pages, VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID is added
to the vq to tell host that the reporting is done. Host may also requests
the guest to stop the reporting in advance by sending the stop cmd id to
the guest via the configuration register.
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Liang Li <liang.z.li@intel.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Michal Hocko <mhocko@kernel.org>
---
drivers/virtio/virtio_balloon.c | 242 +++++++++++++++++++++++++++++++-----
include/uapi/linux/virtio_balloon.h | 4 +
2 files changed, 214 insertions(+), 32 deletions(-)
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index a1fb52c..b9561a5 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -53,7 +53,12 @@ static struct vfsmount *balloon_mnt;
struct virtio_balloon {
struct virtio_device *vdev;
- struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
+ struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *free_page_vq;
+
+ /* Balloon's own wq for cpu-intensive work items */
+ struct workqueue_struct *balloon_wq;
+ /* The free page reporting work item submitted to the balloon wq */
+ struct work_struct report_free_page_work;
/* The balloon servicing is delegated to a freezable workqueue. */
struct work_struct update_balloon_stats_work;
@@ -63,6 +68,13 @@ struct virtio_balloon {
spinlock_t stop_update_lock;
bool stop_update;
+ /* Start to report free pages */
+ bool report_free_page;
+ /* Stores the cmd id given by host to start the free page reporting */
+ uint32_t start_cmd_id;
+ /* Stores STOP_ID as a sign to tell host that the reporting is done */
+ uint32_t stop_cmd_id;
+
/* Waiting for host to ack the pages we released. */
wait_queue_head_t acked;
@@ -281,6 +293,71 @@ static unsigned int update_balloon_stats(struct virtio_balloon *vb)
return idx;
}
+static void add_one_sg(struct virtqueue *vq, unsigned long pfn, uint32_t len)
+{
+ struct scatterlist sg;
+ unsigned int unused;
+ int err;
+
+ sg_init_table(&sg, 1);
+ sg_set_page(&sg, pfn_to_page(pfn), len, 0);
+
+ /* Detach all the used buffers from the vq */
+ while (virtqueue_get_buf(vq, &unused))
+ ;
+
+ /*
+ * Since this is an optimization feature, losing a couple of free
+ * pages to report isn't important. We simply resturn without adding
+ * the page if the vq is full. We are adding one entry each time,
+ * which essentially results in no memory allocation, so the
+ * GFP_KERNEL flag below can be ignored.
+ */
+ if (vq->num_free) {
+ err = virtqueue_add_inbuf(vq, &sg, 1, vq, GFP_KERNEL);
+ /*
+ * This is expected to never fail, because there is always an
+ * entry available on the vq.
+ */
+ BUG_ON(err);
+ }
+}
+
+static void batch_free_page_sg(struct virtqueue *vq,
+ unsigned long pfn,
+ uint32_t len)
+{
+ add_one_sg(vq, pfn, len);
+
+ /* Batch till the vq is full */
+ if (!vq->num_free)
+ virtqueue_kick(vq);
+}
+
+static void send_cmd_id(struct virtqueue *vq, void *addr)
+{
+ struct scatterlist sg;
+ unsigned int unused;
+ int err;
+
+ sg_init_one(&sg, addr, sizeof(uint32_t));
+
+ /*
+ * This handles the cornercase that the vq happens to be full when
+ * adding a cmd id. Rarely happen in practice.
+ */
+ while (!vq->num_free)
+ virtqueue_get_buf(vq, &unused);
+
+ err = virtqueue_add_outbuf(vq, &sg, 1, vq, GFP_KERNEL);
+ /*
+ * This is expected to never fail, because there is always an
+ * entry available on the vq.
+ */
+ BUG_ON(err);
+ virtqueue_kick(vq);
+}
+
/*
* While most virtqueues communicate guest-initiated requests to the hypervisor,
* the stats queue operates in reverse. The driver initializes the virtqueue
@@ -316,17 +393,6 @@ static void stats_handle_request(struct virtio_balloon *vb)
virtqueue_kick(vq);
}
-static void virtballoon_changed(struct virtio_device *vdev)
-{
- struct virtio_balloon *vb = vdev->priv;
- unsigned long flags;
-
- spin_lock_irqsave(&vb->stop_update_lock, flags);
- if (!vb->stop_update)
- queue_work(system_freezable_wq, &vb->update_balloon_size_work);
- spin_unlock_irqrestore(&vb->stop_update_lock, flags);
-}
-
static inline s64 towards_target(struct virtio_balloon *vb)
{
s64 target;
@@ -343,6 +409,36 @@ static inline s64 towards_target(struct virtio_balloon *vb)
return target - vb->num_pages;
}
+static void virtballoon_changed(struct virtio_device *vdev)
+{
+ struct virtio_balloon *vb = vdev->priv;
+ unsigned long flags;
+ __u32 cmd_id;
+ s64 diff = towards_target(vb);
+
+ if (diff) {
+ spin_lock_irqsave(&vb->stop_update_lock, flags);
+ if (!vb->stop_update)
+ queue_work(system_freezable_wq,
+ &vb->update_balloon_size_work);
+ spin_unlock_irqrestore(&vb->stop_update_lock, flags);
+ }
+
+ virtio_cread(vb->vdev, struct virtio_balloon_config,
+ free_page_report_cmd_id, &cmd_id);
+ if (cmd_id == VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID) {
+ WRITE_ONCE(vb->report_free_page, false);
+ } else if (cmd_id != vb->start_cmd_id) {
+ /*
+ * Host requests to start the reporting by sending a new cmd
+ * id.
+ */
+ WRITE_ONCE(vb->report_free_page, true);
+ vb->start_cmd_id = cmd_id;
+ queue_work(vb->balloon_wq, &vb->report_free_page_work);
+ }
+}
+
static void update_balloon_size(struct virtio_balloon *vb)
{
u32 actual = vb->num_pages;
@@ -417,40 +513,113 @@ static void update_balloon_size_func(struct work_struct *work)
static int init_vqs(struct virtio_balloon *vb)
{
- struct virtqueue *vqs[3];
- vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
- static const char * const names[] = { "inflate", "deflate", "stats" };
- int err, nvqs;
+ struct virtqueue **vqs;
+ vq_callback_t **callbacks;
+ const char **names;
+ struct scatterlist sg;
+ int i, nvqs, err = -ENOMEM;
+
+ /* Inflateq and deflateq are used unconditionally */
+ nvqs = 2;
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ))
+ nvqs++;
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ))
+ nvqs++;
+
+ /* Allocate space for find_vqs parameters */
+ vqs = kcalloc(nvqs, sizeof(*vqs), GFP_KERNEL);
+ if (!vqs)
+ goto err_vq;
+ callbacks = kmalloc_array(nvqs, sizeof(*callbacks), GFP_KERNEL);
+ if (!callbacks)
+ goto err_callback;
+ names = kmalloc_array(nvqs, sizeof(*names), GFP_KERNEL);
+ if (!names)
+ goto err_names;
+
+ callbacks[0] = balloon_ack;
+ names[0] = "inflate";
+ callbacks[1] = balloon_ack;
+ names[1] = "deflate";
+
+ i = 2;
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
+ callbacks[i] = stats_request;
+ names[i] = "stats";
+ i++;
+ }
- /*
- * We expect two virtqueues: inflate and deflate, and
- * optionally stat.
- */
- nvqs = virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ) ? 3 : 2;
- err = virtio_find_vqs(vb->vdev, nvqs, vqs, callbacks, names, NULL);
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) {
+ callbacks[i] = NULL;
+ names[i] = "free_page_vq";
+ }
+
+ err = vb->vdev->config->find_vqs(vb->vdev, nvqs, vqs, callbacks, names,
+ NULL, NULL);
if (err)
- return err;
+ goto err_find;
vb->inflate_vq = vqs[0];
vb->deflate_vq = vqs[1];
+ i = 2;
if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
- struct scatterlist sg;
- unsigned int num_stats;
- vb->stats_vq = vqs[2];
-
+ vb->stats_vq = vqs[i++];
/*
* Prime this virtqueue with one buffer so the hypervisor can
* use it to signal us later (it can't be broken yet!).
*/
- num_stats = update_balloon_stats(vb);
-
- sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
+ sg_init_one(&sg, vb->stats, sizeof(vb->stats));
if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
- < 0)
- BUG();
+ < 0) {
+ dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n",
+ __func__);
+ goto err_find;
+ }
virtqueue_kick(vb->stats_vq);
}
+
+ if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ))
+ vb->free_page_vq = vqs[i];
+
+ kfree(names);
+ kfree(callbacks);
+ kfree(vqs);
return 0;
+
+err_find:
+ kfree(names);
+err_names:
+ kfree(callbacks);
+err_callback:
+ kfree(vqs);
+err_vq:
+ return err;
+}
+
+static bool virtio_balloon_send_free_pages(void *opaque, unsigned long pfn,
+ unsigned long nr_pages)
+{
+ struct virtio_balloon *vb = (struct virtio_balloon *)opaque;
+ uint32_t len = nr_pages << PAGE_SHIFT;
+
+ if (!READ_ONCE(vb->report_free_page))
+ return false;
+
+ batch_free_page_sg(vb->free_page_vq, pfn, len);
+
+ return true;
+}
+
+static void report_free_page_func(struct work_struct *work)
+{
+ struct virtio_balloon *vb;
+
+ vb = container_of(work, struct virtio_balloon, report_free_page_work);
+ /* Start by sending the obtained cmd id to the host with an outbuf */
+ send_cmd_id(vb->free_page_vq, &vb->start_cmd_id);
+ walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
+ /* End by sending the stop id to the host with an outbuf */
+ send_cmd_id(vb->free_page_vq, &vb->stop_cmd_id);
}
#ifdef CONFIG_BALLOON_COMPACTION
@@ -566,6 +735,13 @@ static int virtballoon_probe(struct virtio_device *vdev)
if (err)
goto out_free_vb;
+ if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_VQ)) {
+ vb->balloon_wq = alloc_workqueue("balloon-wq",
+ WQ_FREEZABLE | WQ_CPU_INTENSIVE, 0);
+ INIT_WORK(&vb->report_free_page_work, report_free_page_func);
+ vb->stop_cmd_id = VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID;
+ }
+
vb->nb.notifier_call = virtballoon_oom_notify;
vb->nb.priority = VIRTBALLOON_OOM_NOTIFY_PRIORITY;
err = register_oom_notifier(&vb->nb);
@@ -630,6 +806,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
spin_unlock_irq(&vb->stop_update_lock);
cancel_work_sync(&vb->update_balloon_size_work);
cancel_work_sync(&vb->update_balloon_stats_work);
+ cancel_work_sync(&vb->report_free_page_work);
remove_common(vb);
#ifdef CONFIG_BALLOON_COMPACTION
@@ -682,6 +859,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_FREE_PAGE_VQ,
};
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..55e2456 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -34,15 +34,19 @@
#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_FREE_PAGE_VQ 3 /* VQ to report free pages */
/* Size of a PFN in the balloon interface. */
#define VIRTIO_BALLOON_PFN_SHIFT 12
+#define VIRTIO_BALLOON_FREE_PAGE_REPORT_STOP_ID 0
struct virtio_balloon_config {
/* Number of pages host wants Guest to give up. */
__u32 num_pages;
/* Number of pages we've actually got in balloon. */
__u32 actual;
+ /* Free page report command id, readonly by guest */
+ __u32 free_page_report_cmd_id;
};
#define VIRTIO_BALLOON_S_SWAP_IN 0 /* Amount of memory swapped in */
--
2.7.4
^ permalink raw reply related
* [PATCH v22 1/3] mm: support reporting free page blocks
From: Wei Wang @ 2018-01-17 5:10 UTC (permalink / raw)
To: virtio-dev, linux-kernel, virtualization, kvm, linux-mm, mst,
mhocko, akpm
Cc: yang.zhang.wz, riel, quan.xu0, liliang.opensource, pbonzini,
nilal
In-Reply-To: <1516165812-3995-1-git-send-email-wei.w.wang@intel.com>
This patch adds support to walk through the free page blocks in the
system and report them via a callback function. Some page blocks may
leave the free list after zone->lock is released, so it is the caller's
responsibility to either detect or prevent the use of such pages.
One use example of this patch is to accelerate live migration by skipping
the transfer of free pages reported from the guest. A popular method used
by the hypervisor to track which part of memory is written during live
migration is to write-protect all the guest memory. So, those pages that
are reported as free pages but are written after the report function
returns will be captured by the hypervisor, and they will be added to the
next round of memory transfer.
Signed-off-by: Wei Wang <wei.w.wang@intel.com>
Signed-off-by: Liang Li <liang.z.li@intel.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Michal Hocko <mhocko@kernel.org>
---
include/linux/mm.h | 6 ++++
mm/page_alloc.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 97 insertions(+)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ea818ff..b3077dd 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1938,6 +1938,12 @@ extern void free_area_init_node(int nid, unsigned long * zones_size,
unsigned long zone_start_pfn, unsigned long *zholes_size);
extern void free_initmem(void);
+extern void walk_free_mem_block(void *opaque,
+ int min_order,
+ bool (*report_pfn_range)(void *opaque,
+ unsigned long pfn,
+ unsigned long num));
+
/*
* Free reserved pages within range [PAGE_ALIGN(start), end & PAGE_MASK)
* into the buddy system. The freed pages will be poisoned with pattern
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 76c9688..705de22 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4899,6 +4899,97 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
show_swap_cache_info();
}
+/*
+ * Walk through a free page list and report the found pfn range via the
+ * callback.
+ *
+ * Return false if the callback requests to stop reporting. Otherwise,
+ * return true.
+ */
+static bool walk_free_page_list(void *opaque,
+ struct zone *zone,
+ int order,
+ enum migratetype mt,
+ bool (*report_pfn_range)(void *,
+ unsigned long,
+ unsigned long))
+{
+ struct page *page;
+ struct list_head *list;
+ unsigned long pfn, flags;
+ bool ret;
+
+ spin_lock_irqsave(&zone->lock, flags);
+ list = &zone->free_area[order].free_list[mt];
+ list_for_each_entry(page, list, lru) {
+ pfn = page_to_pfn(page);
+ ret = report_pfn_range(opaque, pfn, 1 << order);
+ if (!ret)
+ break;
+ }
+ spin_unlock_irqrestore(&zone->lock, flags);
+
+ return ret;
+}
+
+/**
+ * walk_free_mem_block - Walk through the free page blocks in the system
+ * @opaque: the context passed from the caller
+ * @min_order: the minimum order of free lists to check
+ * @report_pfn_range: the callback to report the pfn range of the free pages
+ *
+ * If the callback returns false, stop iterating the list of free page blocks.
+ * Otherwise, continue to report.
+ *
+ * Please note that there are no locking guarantees for the callback and
+ * that the reported pfn range might be freed or disappear after the
+ * callback returns so the caller has to be very careful how it is used.
+ *
+ * The callback itself must not sleep or perform any operations which would
+ * require any memory allocations directly (not even GFP_NOWAIT/GFP_ATOMIC)
+ * or via any lock dependency. It is generally advisable to implement
+ * the callback as simple as possible and defer any heavy lifting to a
+ * different context.
+ *
+ * There is no guarantee that each free range will be reported only once
+ * during one walk_free_mem_block invocation.
+ *
+ * pfn_to_page on the given range is strongly discouraged and if there is
+ * an absolute need for that make sure to contact MM people to discuss
+ * potential problems.
+ *
+ * The function itself might sleep so it cannot be called from atomic
+ * contexts.
+ *
+ * In general low orders tend to be very volatile and so it makes more
+ * sense to query larger ones first for various optimizations which like
+ * ballooning etc... This will reduce the overhead as well.
+ */
+void walk_free_mem_block(void *opaque,
+ int min_order,
+ bool (*report_pfn_range)(void *opaque,
+ unsigned long pfn,
+ unsigned long num))
+{
+ struct zone *zone;
+ int order;
+ enum migratetype mt;
+ bool ret;
+
+ for_each_populated_zone(zone) {
+ for (order = MAX_ORDER - 1; order >= min_order; order--) {
+ for (mt = 0; mt < MIGRATE_TYPES; mt++) {
+ ret = walk_free_page_list(opaque, zone,
+ order, mt,
+ report_pfn_range);
+ if (!ret)
+ return;
+ }
+ }
+ }
+}
+EXPORT_SYMBOL_GPL(walk_free_mem_block);
+
static void zoneref_set_zone(struct zone *zone, struct zoneref *zoneref)
{
zoneref->zone = zone;
--
2.7.4
^ permalink raw reply related
* [PATCH v22 0/3] Virtio-balloon: support free page reporting
From: Wei Wang @ 2018-01-17 5:10 UTC (permalink / raw)
To: virtio-dev, linux-kernel, virtualization, kvm, linux-mm, mst,
mhocko, akpm
Cc: yang.zhang.wz, riel, quan.xu0, liliang.opensource, pbonzini,
nilal
This patch series is separated from the previous "Virtio-balloon
Enhancement" series. The new feature, VIRTIO_BALLOON_F_FREE_PAGE_VQ,
implemented by this series enables the virtio-balloon driver to report
hints of guest free pages to the host. It can be used to accelerate live
migration of VMs. Here is an introduction of this usage:
Live migration needs to transfer the VM's memory from the source machine
to the destination round by round. For the 1st round, all the VM's memory
is transferred. From the 2nd round, only the pieces of memory that were
written by the guest (after the 1st round) are transferred. One method
that is popularly used by the hypervisor to track which part of memory is
written is to write-protect all the guest memory.
The second feature enables the optimization of the 1st round memory
transfer - the hypervisor can skip the transfer of guest free pages in the
1st round. It is not concerned that the memory pages are used after they
are given to the hypervisor as a hint of the free pages, because they will
be tracked by the hypervisor and transferred in the next round if they are
used and written.
ChangeLog:
v21-v22:
- add_one_sg: some code and comment re-arrangement
- send_cmd_id: handle a cornercase
For precious ChangeLog, please reference
https://lwn.net/Articles/743660/
Wei Wang (3):
mm: support reporting free page blocks
virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_VQ
virtio-balloon: don't report free pages when page poisoning is enabled
drivers/virtio/virtio_balloon.c | 250 +++++++++++++++++++++++++++++++-----
include/linux/mm.h | 6 +
include/uapi/linux/virtio_balloon.h | 6 +
mm/page_alloc.c | 91 +++++++++++++
4 files changed, 321 insertions(+), 32 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [RFC PATCH v2 2/5] iommu/virtio-iommu: Add probe request
From: Auger Eric @ 2018-01-16 23:26 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu, devel, linux-acpi, kvm, kvmarm,
virtualization, virtio-dev
Cc: Jayachandran.Nair, lorenzo.pieralisi, ashok.raj, mst,
marc.zyngier, will.deacon, rjw, robert.moore, lv.zheng,
sudeep.holla, lenb, robin.murphy, joro, hanjun.guo
In-Reply-To: <20171117185211.32593-3-jean-philippe.brucker@arm.com>
Hi Jean-Philippe,
On 17/11/17 19:52, Jean-Philippe Brucker wrote:
> When the device offers the probe feature, send a probe request for each
> device managed by the IOMMU. Extract RESV_MEM information. When we
> encounter a MSI doorbell region, set it up as a IOMMU_RESV_MSI region.
> This will tell other subsystems that there is no need to map the MSI
> doorbell in the virtio-iommu, because MSIs bypass it.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
> drivers/iommu/virtio-iommu.c | 165 ++++++++++++++++++++++++++++++++++++--
> include/uapi/linux/virtio_iommu.h | 37 +++++++++
> 2 files changed, 195 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index feb8c8925c3a..79e0add94e05 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -45,6 +45,7 @@ struct viommu_dev {
> struct iommu_domain_geometry geometry;
> u64 pgsize_bitmap;
> u8 domain_bits;
> + u32 probe_size;
> };
>
> struct viommu_mapping {
> @@ -72,6 +73,7 @@ struct viommu_domain {
> struct viommu_endpoint {
> struct viommu_dev *viommu;
> struct viommu_domain *vdomain;
> + struct list_head resv_regions;
> };
>
> struct viommu_request {
> @@ -139,6 +141,10 @@ static int viommu_get_req_size(struct viommu_dev *viommu,
> case VIRTIO_IOMMU_T_UNMAP:
> size = sizeof(r->unmap);
> break;
> + case VIRTIO_IOMMU_T_PROBE:
> + *bottom += viommu->probe_size;
> + size = sizeof(r->probe) + *bottom;
> + break;
> default:
> return -EINVAL;
> }
> @@ -448,6 +454,106 @@ static int viommu_replay_mappings(struct viommu_domain *vdomain)
> return ret;
> }
>
> +static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
> + struct virtio_iommu_probe_resv_mem *mem,
> + size_t len)
> +{
> + struct iommu_resv_region *region = NULL;
> + unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +
> + u64 addr = le64_to_cpu(mem->addr);
> + u64 size = le64_to_cpu(mem->size);
> +
> + if (len < sizeof(*mem))
> + return -EINVAL;
> +
> + switch (mem->subtype) {
> + case VIRTIO_IOMMU_RESV_MEM_T_MSI:
> + region = iommu_alloc_resv_region(addr, size, prot,
> + IOMMU_RESV_MSI);
if (!region)
return -ENOMEM;
> + break;
> + case VIRTIO_IOMMU_RESV_MEM_T_RESERVED:
> + default:
> + region = iommu_alloc_resv_region(addr, size, 0,
> + IOMMU_RESV_RESERVED);
same.
There is another issue related to the exclusion of iovas belonging to
reserved regions. Typically on x86, when attempting to run
virtio-blk-pci with iommu I eventually saw the driver using iova
belonging to the IOAPIC regions to map phys addr and this stalled qemu
with a drown trace:
"virtio: bogus descriptor or out of resources"
those regions need to be excluded from the iova allocator. This was
resolved by adding
if (iommu_dma_init_domain(domain,
vdev->viommu->geometry.aperture_start,
vdev->viommu->geometry.aperture_end,
dev))
in viommu_attach_dev()
Thanks
Eric
> + break;
> + }
> +
> + list_add(&vdev->resv_regions, ®ion->list);
> +
> + if (mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_RESERVED &&
> + mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_MSI) {
> + /* Please update your driver. */
> + pr_warn("unknown resv mem subtype 0x%x\n", mem->subtype);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
> +{
> + int ret;
> + u16 type, len;
> + size_t cur = 0;
> + struct virtio_iommu_req_probe *probe;
> + struct virtio_iommu_probe_property *prop;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> + struct viommu_endpoint *vdev = fwspec->iommu_priv;
> +
> + if (!fwspec->num_ids)
> + /* Trouble ahead. */
> + return -EINVAL;
> +
> + probe = kzalloc(sizeof(*probe) + viommu->probe_size +
> + sizeof(struct virtio_iommu_req_tail), GFP_KERNEL);
> + if (!probe)
> + return -ENOMEM;
> +
> + probe->head.type = VIRTIO_IOMMU_T_PROBE;
> + /*
> + * For now, assume that properties of an endpoint that outputs multiple
> + * IDs are consistent. Only probe the first one.
> + */
> + probe->endpoint = cpu_to_le32(fwspec->ids[0]);
> +
> + ret = viommu_send_req_sync(viommu, probe);
> + if (ret) {
> + kfree(probe);
> + return ret;
> + }
> +
> + prop = (void *)probe->properties;
> + type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
> +
> + while (type != VIRTIO_IOMMU_PROBE_T_NONE &&
> + cur < viommu->probe_size) {
> + len = le16_to_cpu(prop->length);
> +
> + switch (type) {
> + case VIRTIO_IOMMU_PROBE_T_RESV_MEM:
> + ret = viommu_add_resv_mem(vdev, (void *)prop->value, len);
> + break;
> + default:
> + dev_dbg(dev, "unknown viommu prop 0x%x\n", type);
> + }
> +
> + if (ret)
> + dev_err(dev, "failed to parse viommu prop 0x%x\n", type);
> +
> + cur += sizeof(*prop) + len;
> + if (cur >= viommu->probe_size)
> + break;
> +
> + prop = (void *)probe->properties + cur;
> + type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
> + }
> +
> + kfree(probe);
> +
> + return 0;
> +}
> +
> /* IOMMU API */
>
> static bool viommu_capable(enum iommu_cap cap)
> @@ -706,6 +812,7 @@ static struct viommu_dev *viommu_get_by_fwnode(struct fwnode_handle *fwnode)
>
> static int viommu_add_device(struct device *dev)
> {
> + int ret;
> struct iommu_group *group;
> struct viommu_endpoint *vdev;
> struct viommu_dev *viommu = NULL;
> @@ -723,8 +830,16 @@ static int viommu_add_device(struct device *dev)
> return -ENOMEM;
>
> vdev->viommu = viommu;
> + INIT_LIST_HEAD(&vdev->resv_regions);
> fwspec->iommu_priv = vdev;
>
> + if (viommu->probe_size) {
> + /* Get additional information for this endpoint */
> + ret = viommu_probe_endpoint(viommu, dev);
> + if (ret)
> + return ret;
> + }
> +
> /*
> * Last step creates a default domain and attaches to it. Everything
> * must be ready.
> @@ -738,7 +853,19 @@ static int viommu_add_device(struct device *dev)
>
> static void viommu_remove_device(struct device *dev)
> {
> - kfree(dev->iommu_fwspec->iommu_priv);
> + struct viommu_endpoint *vdev;
> + struct iommu_resv_region *entry, *next;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> + if (!fwspec || fwspec->ops != &viommu_ops)
> + return;
> +
> + vdev = fwspec->iommu_priv;
> +
> + list_for_each_entry_safe(entry, next, &vdev->resv_regions, list)
> + kfree(entry);
> +
> + kfree(vdev);
> }
>
> static struct iommu_group *viommu_device_group(struct device *dev)
> @@ -756,15 +883,34 @@ static int viommu_of_xlate(struct device *dev, struct of_phandle_args *args)
>
> static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
> {
> - struct iommu_resv_region *region;
> + struct iommu_resv_region *entry, *new_entry, *msi = NULL;
> + struct viommu_endpoint *vdev = dev->iommu_fwspec->iommu_priv;
> int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>
> - region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, prot,
> - IOMMU_RESV_SW_MSI);
> - if (!region)
> - return;
> + list_for_each_entry(entry, &vdev->resv_regions, list) {
> + /*
> + * If the device registered a bypass MSI windows, use it.
> + * Otherwise add a software-mapped region
> + */
> + if (entry->type == IOMMU_RESV_MSI)
> + msi = entry;
> +
> + new_entry = kmemdup(entry, sizeof(*entry), GFP_KERNEL);
> + if (!new_entry)
> + return;
> + list_add_tail(&new_entry->list, head);
> + }
>
> - list_add_tail(®ion->list, head);
> + if (!msi) {
> + msi = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> + prot, IOMMU_RESV_SW_MSI);
> + if (!msi)
> + return;
> +
> + list_add_tail(&msi->list, head);
> + }
> +
> + iommu_dma_get_resv_regions(dev, head);
> }
>
> static void viommu_put_resv_regions(struct device *dev, struct list_head *head)
> @@ -854,6 +1000,10 @@ static int viommu_probe(struct virtio_device *vdev)
> struct virtio_iommu_config, domain_bits,
> &viommu->domain_bits);
>
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_PROBE,
> + struct virtio_iommu_config, probe_size,
> + &viommu->probe_size);
> +
> viommu->geometry = (struct iommu_domain_geometry) {
> .aperture_start = input_start,
> .aperture_end = input_end,
> @@ -931,6 +1081,7 @@ static unsigned int features[] = {
> VIRTIO_IOMMU_F_MAP_UNMAP,
> VIRTIO_IOMMU_F_DOMAIN_BITS,
> VIRTIO_IOMMU_F_INPUT_RANGE,
> + VIRTIO_IOMMU_F_PROBE,
> };
>
> static struct virtio_device_id id_table[] = {
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> index 2b4cd2042897..eec90a2ab547 100644
> --- a/include/uapi/linux/virtio_iommu.h
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -38,6 +38,7 @@
> #define VIRTIO_IOMMU_F_DOMAIN_BITS 1
> #define VIRTIO_IOMMU_F_MAP_UNMAP 2
> #define VIRTIO_IOMMU_F_BYPASS 3
> +#define VIRTIO_IOMMU_F_PROBE 4
>
> struct virtio_iommu_config {
> /* Supported page sizes */
> @@ -49,6 +50,9 @@ struct virtio_iommu_config {
> } input_range;
> /* Max domain ID size */
> __u8 domain_bits;
> + __u8 padding[3];
> + /* Probe buffer size */
> + __u32 probe_size;
> } __packed;
>
> /* Request types */
> @@ -56,6 +60,7 @@ struct virtio_iommu_config {
> #define VIRTIO_IOMMU_T_DETACH 0x02
> #define VIRTIO_IOMMU_T_MAP 0x03
> #define VIRTIO_IOMMU_T_UNMAP 0x04
> +#define VIRTIO_IOMMU_T_PROBE 0x05
>
> /* Status types */
> #define VIRTIO_IOMMU_S_OK 0x00
> @@ -128,6 +133,37 @@ struct virtio_iommu_req_unmap {
> struct virtio_iommu_req_tail tail;
> } __packed;
>
> +#define VIRTIO_IOMMU_RESV_MEM_T_RESERVED 0
> +#define VIRTIO_IOMMU_RESV_MEM_T_MSI 1
> +
> +struct virtio_iommu_probe_resv_mem {
> + __u8 subtype;
> + __u8 reserved[3];
> + __le64 addr;
> + __le64 size;
> +} __packed;
> +
> +#define VIRTIO_IOMMU_PROBE_T_NONE 0
> +#define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1
> +
> +#define VIRTIO_IOMMU_PROBE_T_MASK 0xfff
> +
> +struct virtio_iommu_probe_property {
> + __le16 type;
> + __le16 length;
> + __u8 value[];
> +} __packed;
> +
> +struct virtio_iommu_req_probe {
> + struct virtio_iommu_req_head head;
> + __le32 endpoint;
> + __u8 reserved[64];
> +
> + __u8 properties[];
> +
> + /* Tail follows the variable-length properties array (no padding) */
> +} __packed;
> +
> union virtio_iommu_req {
> struct virtio_iommu_req_head head;
>
> @@ -135,6 +171,7 @@ union virtio_iommu_req {
> struct virtio_iommu_req_detach detach;
> struct virtio_iommu_req_map map;
> struct virtio_iommu_req_unmap unmap;
> + struct virtio_iommu_req_probe probe;
> };
>
> #endif
>
^ permalink raw reply
* Re: [RFC PATCH v2 3/5] iommu/virtio-iommu: Add event queue
From: Jean-Philippe Brucker @ 2018-01-16 17:48 UTC (permalink / raw)
To: Auger Eric, iommu@lists.linux-foundation.org, devel@acpica.org,
linux-acpi@vger.kernel.org, kvm@vger.kernel.org,
kvmarm@lists.cs.columbia.edu,
virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: Jayachandran.Nair@cavium.com, Lorenzo Pieralisi,
ashok.raj@intel.com, mst@redhat.com, Marc Zyngier, Will Deacon,
rjw@rjwysocki.net, robert.moore@intel.com, lv.zheng@intel.com,
Sudeep Holla, lenb@kernel.org, Robin Murphy, joro@8bytes.org,
hanjun.guo@linaro.org
In-Reply-To: <3061e653-38bb-bffa-0f16-43047724a0ae@redhat.com>
On 16/01/18 10:10, Auger Eric wrote:
> Hi,
>
> On 17/11/17 19:52, Jean-Philippe Brucker wrote:
>> The event queue offers a way for the device to report access faults from
>> devices.
> end points?
Yes
[...]
>> +static void viommu_event_handler(struct virtqueue *vq)
>> +{
>> + int ret;
>> + unsigned int len;
>> + struct scatterlist sg[1];
>> + struct viommu_event *evt;
>> + struct viommu_dev *viommu = vq->vdev->priv;
>> +
>> + while ((evt = virtqueue_get_buf(vq, &len)) != NULL) {
>> + if (len > sizeof(*evt)) {
>> + dev_err(viommu->dev,
>> + "invalid event buffer (len %u != %zu)\n",
>> + len, sizeof(*evt));
>> + } else if (!(evt->head & VIOMMU_FAULT_RESV_MASK)) {
>> + viommu_fault_handler(viommu, &evt->fault);
>> + }
>> +
>> + sg_init_one(sg, evt, sizeof(*evt));
> in case of above error case, ie. len > sizeof(*evt), is it safe to push
> evt again?
I think it is, len is just what the device reports as written. The above
error would be a device bug, very unlikely and not worth a special
treatment (freeing the buffer), maybe not even worth the dev_err()
actually.
>> + ret = virtqueue_add_inbuf(vq, sg, 1, evt, GFP_ATOMIC);
>> + if (ret)
>> + dev_err(viommu->dev, "could not add event buffer\n");
>> + }
>> +
>> + if (!virtqueue_kick(vq))
>> + dev_err(viommu->dev, "kick failed\n");
>> +}
>> +
>> /* IOMMU API */
>>
>> static bool viommu_capable(enum iommu_cap cap)
>> @@ -938,19 +1018,44 @@ static struct iommu_ops viommu_ops = {
>> .put_resv_regions = viommu_put_resv_regions,
>> };
>>
>> -static int viommu_init_vq(struct viommu_dev *viommu)
>> +static int viommu_init_vqs(struct viommu_dev *viommu)
>> {
>> struct virtio_device *vdev = dev_to_virtio(viommu->dev);
>> - const char *name = "request";
>> - void *ret;
>> + const char *names[] = { "request", "event" };
>> + vq_callback_t *callbacks[] = {
>> + NULL, /* No async requests */
>> + viommu_event_handler,
>> + };
>> +
>> + return virtio_find_vqs(vdev, VIOMMU_NUM_VQS, viommu->vqs, callbacks,
>> + names, NULL);
>> +}
>>
>> - ret = virtio_find_single_vq(vdev, NULL, name);
>> - if (IS_ERR(ret)) {
>> - dev_err(viommu->dev, "cannot find VQ\n");
>> - return PTR_ERR(ret);
>> +static int viommu_fill_evtq(struct viommu_dev *viommu)
>> +{
>> + int i, ret;
>> + struct scatterlist sg[1];
>> + struct viommu_event *evts;
>> + struct virtqueue *vq = viommu->vqs[VIOMMU_EVENT_VQ];
>> + size_t nr_evts = min_t(size_t, PAGE_SIZE / sizeof(struct viommu_event),
>> + viommu->vqs[VIOMMU_EVENT_VQ]->num_free);
>> +
>> + evts = devm_kmalloc_array(viommu->dev, nr_evts, sizeof(*evts),
>> + GFP_KERNEL);
>> + if (!evts)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < nr_evts; i++) {
>> + sg_init_one(sg, &evts[i], sizeof(*evts));
>> + ret = virtqueue_add_inbuf(vq, sg, 1, &evts[i], GFP_KERNEL);
> who does the deallocation of evts?
I think it should be viommu_remove, which should also remove the
virtqueues. I'll add this.
Thanks,
Jean
^ permalink raw reply
* Re: [RFC PATCH v2 2/5] iommu/virtio-iommu: Add probe request
From: Jean-Philippe Brucker @ 2018-01-16 17:46 UTC (permalink / raw)
To: Auger Eric, iommu@lists.linux-foundation.org, devel@acpica.org,
linux-acpi@vger.kernel.org, kvm@vger.kernel.org,
kvmarm@lists.cs.columbia.edu,
virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: Jayachandran.Nair@cavium.com, Lorenzo Pieralisi,
ashok.raj@intel.com, mst@redhat.com, Marc Zyngier, Will Deacon,
rjw@rjwysocki.net, robert.moore@intel.com, lv.zheng@intel.com,
Sudeep Holla, lenb@kernel.org, Robin Murphy, joro@8bytes.org,
hanjun.guo@linaro.org
In-Reply-To: <d4640ecc-8029-413b-1485-a6062c3b8ece@redhat.com>
On 16/01/18 09:25, Auger Eric wrote:
[...]
>> +static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
>> + struct virtio_iommu_probe_resv_mem *mem,
>> + size_t len)
>> +{
>> + struct iommu_resv_region *region = NULL;
>> + unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>> +
>> + u64 addr = le64_to_cpu(mem->addr);
>> + u64 size = le64_to_cpu(mem->size);
>> +
>> + if (len < sizeof(*mem))
>> + return -EINVAL;
>> +
>> + switch (mem->subtype) {
>> + case VIRTIO_IOMMU_RESV_MEM_T_MSI:
>> + region = iommu_alloc_resv_region(addr, size, prot,
>> + IOMMU_RESV_MSI);
>> + break;
>> + case VIRTIO_IOMMU_RESV_MEM_T_RESERVED:
>> + default:
>> + region = iommu_alloc_resv_region(addr, size, 0,
>> + IOMMU_RESV_RESERVED);
>> + break;
>> + }
>> +
>> + list_add(&vdev->resv_regions, ®ion->list);
>> +
>> + if (mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_RESERVED &&
>> + mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_MSI) {
>> + /* Please update your driver. */
>> + pr_warn("unknown resv mem subtype 0x%x\n", mem->subtype);
>> + return -EINVAL;
>> + }
> why not adding this in the switch default case and do not call list_add
> in case the subtype region is not recognized?
Even if the subtype isn't recognized, I think the range should still be
reserved, so that the guest kernel doesn't map it and break something.
That's why I put the following in the spec, 2.6.8.2.1 Driver
Requirements: Property RESV_MEM:
"""
The driver SHOULD treat any subtype it doesn’t recognize as if it was
VIRTIO_IOMMU_RESV_MEM_T_RESERVED.
"""
>> +
>> + return 0;
>> +}
>> +
>> +static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
>> +{
>> + int ret;
>> + u16 type, len;
>> + size_t cur = 0;
>> + struct virtio_iommu_req_probe *probe;
>> + struct virtio_iommu_probe_property *prop;
>> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
>> + struct viommu_endpoint *vdev = fwspec->iommu_priv;
>> +
>> + if (!fwspec->num_ids)
>> + /* Trouble ahead. */
>> + return -EINVAL;
>> +
>> + probe = kzalloc(sizeof(*probe) + viommu->probe_size +
>> + sizeof(struct virtio_iommu_req_tail), GFP_KERNEL);
>> + if (!probe)
>> + return -ENOMEM;
>> +
>> + probe->head.type = VIRTIO_IOMMU_T_PROBE;
>> + /*
>> + * For now, assume that properties of an endpoint that outputs multiple
>> + * IDs are consistent. Only probe the first one.
>> + */
>> + probe->endpoint = cpu_to_le32(fwspec->ids[0]);
>> +
>> + ret = viommu_send_req_sync(viommu, probe);
>> + if (ret) {
> goto out?
Ok
[...]
>> +
>> + iommu_dma_get_resv_regions(dev, head);
> this change may belong to the 1st patch.
Indeed
Thanks,
Jean
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH v2 1/5] iommu: Add virtio-iommu driver
From: Jean-Philippe Brucker @ 2018-01-16 17:45 UTC (permalink / raw)
To: Auger Eric, iommu@lists.linux-foundation.org, devel@acpica.org,
linux-acpi@vger.kernel.org, kvm@vger.kernel.org,
kvmarm@lists.cs.columbia.edu,
virtualization@lists.linux-foundation.org,
virtio-dev@lists.oasis-open.org
Cc: Jayachandran.Nair@cavium.com, Lorenzo Pieralisi,
ashok.raj@intel.com, mst@redhat.com, Marc Zyngier, Will Deacon,
rjw@rjwysocki.net, robert.moore@intel.com, lv.zheng@intel.com,
Sudeep Holla, lenb@kernel.org, Robin Murphy, joro@8bytes.org,
hanjun.guo@linaro.org
In-Reply-To: <54320f26-05a5-9a09-f6cf-ae610c41e077@redhat.com>
On 15/01/18 15:12, Auger Eric wrote:
[...]
>> +/*
>> + * viommu_get_req_size - compute request size
>> + *
>> + * A virtio-iommu request is split into one device-read-only part (top) and one
>> + * device-write-only part (bottom). Given a request, return the sizes of the two
>> + * parts in @top and @bottom.
> for all but virtio_iommu_req_probe, which has a variable bottom size
The comment still stands for the probe request, viommu_get_req_size will
return @bottom depending on viommu->probe_size.
[...]
>> +/* Must be called with request_lock held */
>> +static int _viommu_send_reqs_sync(struct viommu_dev *viommu,
>> + struct viommu_request *req, int nr,
>> + int *nr_sent)
>> +{
>> + int i, ret;
>> + ktime_t timeout;
>> + LIST_HEAD(pending);
>> + int nr_received = 0;
>> + struct scatterlist *sg[2];
>> + /*
>> + * Yes, 1s timeout. As a guest, we don't necessarily have a precise
>> + * notion of time and this just prevents locking up a CPU if the device
>> + * dies.
>> + */
>> + unsigned long timeout_ms = 1000;
>> +
>> + *nr_sent = 0;
>> +
>> + for (i = 0; i < nr; i++, req++) {
>> + req->written = 0;
>> +
>> + sg[0] = &req->top;
>> + sg[1] = &req->bottom;
>> +
>> + ret = virtqueue_add_sgs(viommu->vq, sg, 1, 1, req,
>> + GFP_ATOMIC);
>> + if (ret)
>> + break;
>> +
>> + list_add_tail(&req->list, &pending);
>> + }
>> +
>> + if (i && !virtqueue_kick(viommu->vq))
>> + return -EPIPE;
>> +
>> + timeout = ktime_add_ms(ktime_get(), timeout_ms * i);
> I don't really understand how you choose your timeout value: 1s per sent
> request.
1s isn't a good timeout value, but I don't know what's good. In a
prototype I have, even 1s isn't enough. The attach request (for nested
mode) requires my device to pin down the whole guest memory, and the guest
ends up timing out on the fastmodel because the request takes too long and
the model timer runs too fast...
I was tempted to simply remove this timeout, but I still think having a
way out when the host device fails is preferable. Otherwise this
completely locks up the CPU.
>> + while (nr_received < i && ktime_before(ktime_get(), timeout)) {
>> + nr_received += viommu_receive_resp(viommu, i - nr_received,
>> + &pending);
>> + if (nr_received < i) {
>> + /*
>> + * FIXME: what's a good way to yield to host? A second
>> + * virtqueue_kick won't have any effect since we haven't
>> + * added any descriptor.
>> + */
>> + udelay(10);
> could you explain why udelay gets used here?
I was hoping this could switch to the host quicker than cpu_relax(),
allowing it to handle the request faster (on ARM udelay could do WFE
instead of YIELD). The value is completely arbitrary.
Maybe I can replace this with cpu_relax for now. I'd like to refine this
function anyway when working on performance improvements, but I'm not too
hopeful we'll get something nicer here.
[...]
>> +/*
>> + * viommu_send_req_sync - send one request and wait for reply
>> + *
>> + * @top: pointer to a virtio_iommu_req_* structure
>> + *
>> + * Returns 0 if the request was successful, or an error number otherwise. No
>> + * distinction is done between transport and request errors.
>> + */
>> +static int viommu_send_req_sync(struct viommu_dev *viommu, void *top)
>> +{
>> + int ret;
>> + int nr_sent;
>> + void *bottom;
>> + struct viommu_request req = {0};
> ^
> drivers/iommu/virtio-iommu.c:326:9: warning: (near initialization for
> ‘req.top’) [-Wmissing-braces]
Ok
[...]
>> +/*
>> + * viommu_del_mappings - remove mappings from the internal tree
>> + *
>> + * @vdomain: the domain
>> + * @iova: start of the range
>> + * @size: size of the range. A size of 0 corresponds to the entire address
>> + * space.
>> + * @out_mapping: if not NULL, the first removed mapping is returned in there.
>> + * This allows the caller to reuse the buffer for the unmap request. Caller
>> + * must always free the returned mapping, whether the function succeeds or
>> + * not.
> if unmapped > 0?
Ok
>> + *
>> + * On success, returns the number of unmapped bytes (>= size)
>> + */
>> +static size_t viommu_del_mappings(struct viommu_domain *vdomain,
>> + unsigned long iova, size_t size,
>> + struct viommu_mapping **out_mapping)
>> +{
>> + size_t unmapped = 0;
>> + unsigned long flags;
>> + unsigned long last = iova + size - 1;
>> + struct viommu_mapping *mapping = NULL;
>> + struct interval_tree_node *node, *next;
>> +
>> + spin_lock_irqsave(&vdomain->mappings_lock, flags);
>> + next = interval_tree_iter_first(&vdomain->mappings, iova, last);
>> +
>> + if (next) {
>> + mapping = container_of(next, struct viommu_mapping, iova);
>> + /* Trying to split a mapping? */
>> + if (WARN_ON(mapping->iova.start < iova))
>> + next = NULL;
>> + }
>> +
>> + while (next) {
>> + node = next;
>> + mapping = container_of(node, struct viommu_mapping, iova);
>> +
>> + next = interval_tree_iter_next(node, iova, last);
>> +
>> + /*
>> + * Note that for a partial range, this will return the full
>> + * mapping so we avoid sending split requests to the device.
>> + */
>> + unmapped += mapping->iova.last - mapping->iova.start + 1;
>> +
>> + interval_tree_remove(node, &vdomain->mappings);
>> +
>> + if (out_mapping && !(*out_mapping))
>> + *out_mapping = mapping;
>> + else
>> + kfree(mapping);
>> + }
>> + spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
>> +
>> + return unmapped;
>> +}
>> +
>> +/*
>> + * viommu_replay_mappings - re-send MAP requests
>> + *
>> + * When reattaching a domain that was previously detached from all devices,
>> + * mappings were deleted from the device. Re-create the mappings available in
>> + * the internal tree.
>> + *
>> + * Caller should hold the mapping lock if necessary.
> the only caller does not hold the lock. At this point we are attaching
> our fisrt ep to the domain. I think it would be worth a comment in the
> caller.
Right
[...]
>> + /*
>> + * When attaching the device to a new domain, it will be detached from
>> + * the old one and, if as as a result the old domain isn't attached to
> as as
>> + * any device, all mappings are removed from the old domain and it is
>> + * freed. (Note that we can't use get_domain_for_dev here, it returns
>> + * the default domain during initial attach.)
> I don't see where the old domain is freed. I see you descrement the
> endpoints ref count. Also if you replay the mapping, I guess the
> mappings were not destroyed?
That comment applies to the virtio-iommu domain - the virtio-iommu
device needs to destroy the old domain when attaching to a new domain,
to avoid any mapping leak. I'll change the comment to clarify this.
In the kernel the domain lifetime differs from the virtio-iommu domain,
the detached domain isn't freed until someone calls free_domain()
explicitly:
- Initially each device is attached to the default domain, used for
kernel DMA.
- Then VFIO attaches the group to a new domain. The IOMMU core calls
attach_dev(). Default domain is detached and the VFIO domain is
attached.
- When VFIO removes the device from its container, the IOMMU core
attaches the default domain again. The default domain may still have
mappings (SW_MSI for instance) but the device deleted them, so they
need to be replayed after the attach request.
- VFIO issues iommu_domain_free when no group is attached to the VFIO
domain anymore.
>> + *
>> + * Take note of the device disappearing, so we can ignore unmap request
>> + * on stale domains (that is, between this detach and the upcoming
>> + * free.)
>> + *
>> + * vdev->vdomain is protected by group->mutex
>> + */
>> + if (vdev->vdomain)
>> + refcount_dec(&vdev->vdomain->endpoints);
>> +
>> + /* DMA to the stack is forbidden, store request on the heap */
>> + req = kzalloc(sizeof(*req), GFP_KERNEL);
>> + if (!req)
>> + return -ENOMEM;
>> +
>> + *req = (struct virtio_iommu_req_attach) {
>> + .head.type = VIRTIO_IOMMU_T_ATTACH,
>> + .domain = cpu_to_le32(vdomain->id),
>> + };
>> +
>> + for (i = 0; i < fwspec->num_ids; i++) {
>> + req->endpoint = cpu_to_le32(fwspec->ids[i]);
>> +
>> + ret = viommu_send_req_sync(vdomain->viommu, req);
>> + if (ret)
>> + break;
>> + }
>> +
>> + kfree(req);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + if (!refcount_read(&vdomain->endpoints)) {
>> + /*
>> + * This endpoint is the first to be attached to the domain.
>> + * Replay existing mappings if any.
>> + */
>> + ret = viommu_replay_mappings(vdomain);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + refcount_inc(&vdomain->endpoints);
> This does not work for me as the ref counter is initialized to 0 and
> ref_count does not work if the counter is 0. It emits a WARN_ON and
> stays at 0. I worked around the issue by explicitly setting
> recount_set(&vdomain->endpoints, 1) if it was 0 and reffount_inc otherwise.
Yes, I went back to a simple int instead of refcount
[...]
>> +
>> +struct virtio_iommu_config {
>> + /* Supported page sizes */
>> + __u64 page_size_mask;
> Get a warning
> ./usr/include/linux/virtio_iommu.h:45: found __[us]{8,16,32,64} type
> without #include <linux/types.h>
Right, adding HEADERS_CHECK to my config :)
Thanks a lot the review,
Jean
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [RFC PATCH v2 3/5] iommu/virtio-iommu: Add event queue
From: Auger Eric @ 2018-01-16 10:10 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu, devel, linux-acpi, kvm, kvmarm,
virtualization, virtio-dev
Cc: Jayachandran.Nair, lorenzo.pieralisi, ashok.raj, mst,
marc.zyngier, will.deacon, rjw, robert.moore, lv.zheng,
sudeep.holla, lenb, robin.murphy, joro, hanjun.guo
In-Reply-To: <20171117185211.32593-4-jean-philippe.brucker@arm.com>
Hi,
On 17/11/17 19:52, Jean-Philippe Brucker wrote:
> The event queue offers a way for the device to report access faults from
> devices.
end points?
It is implemented on virtqueue #1, whenever the host needs to
> signal a fault it fills one of the buffers offered by the guest and
> interrupts it.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
> drivers/iommu/virtio-iommu.c | 138 ++++++++++++++++++++++++++++++++++----
> include/uapi/linux/virtio_iommu.h | 18 +++++
> 2 files changed, 142 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index 79e0add94e05..fe0d449bf489 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -30,6 +30,12 @@
> #define MSI_IOVA_BASE 0x8000000
> #define MSI_IOVA_LENGTH 0x100000
>
> +enum viommu_vq_idx {
> + VIOMMU_REQUEST_VQ = 0,
> + VIOMMU_EVENT_VQ = 1,
> + VIOMMU_NUM_VQS = 2,
> +};
> +
> struct viommu_dev {
> struct iommu_device iommu;
> struct device *dev;
> @@ -37,7 +43,7 @@ struct viommu_dev {
>
> struct ida domain_ids;
>
> - struct virtqueue *vq;
> + struct virtqueue *vqs[VIOMMU_NUM_VQS];
> /* Serialize anything touching the request queue */
> spinlock_t request_lock;
>
> @@ -84,6 +90,15 @@ struct viommu_request {
> struct list_head list;
> };
>
> +#define VIOMMU_FAULT_RESV_MASK 0xffffff00
> +
> +struct viommu_event {
> + union {
> + u32 head;
> + struct virtio_iommu_fault fault;
> + };
> +};
> +
> #define to_viommu_domain(domain) container_of(domain, struct viommu_domain, domain)
>
> /* Virtio transport */
> @@ -160,12 +175,13 @@ static int viommu_receive_resp(struct viommu_dev *viommu, int nr_sent,
> unsigned int len;
> int nr_received = 0;
> struct viommu_request *req, *pending;
> + struct virtqueue *vq = viommu->vqs[VIOMMU_REQUEST_VQ];
>
> pending = list_first_entry_or_null(sent, struct viommu_request, list);
> if (WARN_ON(!pending))
> return 0;
>
> - while ((req = virtqueue_get_buf(viommu->vq, &len)) != NULL) {
> + while ((req = virtqueue_get_buf(vq, &len)) != NULL) {
> if (req != pending) {
> dev_warn(viommu->dev, "discarding stale request\n");
> continue;
> @@ -202,6 +218,7 @@ static int _viommu_send_reqs_sync(struct viommu_dev *viommu,
> * dies.
> */
> unsigned long timeout_ms = 1000;
> + struct virtqueue *vq = viommu->vqs[VIOMMU_REQUEST_VQ];
>
> *nr_sent = 0;
>
> @@ -211,15 +228,14 @@ static int _viommu_send_reqs_sync(struct viommu_dev *viommu,
> sg[0] = &req->top;
> sg[1] = &req->bottom;
>
> - ret = virtqueue_add_sgs(viommu->vq, sg, 1, 1, req,
> - GFP_ATOMIC);
> + ret = virtqueue_add_sgs(vq, sg, 1, 1, req, GFP_ATOMIC);
> if (ret)
> break;
>
> list_add_tail(&req->list, &pending);
> }
>
> - if (i && !virtqueue_kick(viommu->vq))
> + if (i && !virtqueue_kick(vq))
> return -EPIPE;
>
> timeout = ktime_add_ms(ktime_get(), timeout_ms * i);
> @@ -554,6 +570,70 @@ static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
> return 0;
> }
>
> +static int viommu_fault_handler(struct viommu_dev *viommu,
> + struct virtio_iommu_fault *fault)
> +{
> + char *reason_str;
> +
> + u8 reason = fault->reason;
> + u32 flags = le32_to_cpu(fault->flags);
> + u32 endpoint = le32_to_cpu(fault->endpoint);
> + u64 address = le64_to_cpu(fault->address);
> +
> + switch (reason) {
> + case VIRTIO_IOMMU_FAULT_R_DOMAIN:
> + reason_str = "domain";
> + break;
> + case VIRTIO_IOMMU_FAULT_R_MAPPING:
> + reason_str = "page";
> + break;
> + case VIRTIO_IOMMU_FAULT_R_UNKNOWN:
> + default:
> + reason_str = "unknown";
> + break;
> + }
> +
> + /* TODO: find EP by ID and report_iommu_fault */
> + if (flags & VIRTIO_IOMMU_FAULT_F_ADDRESS)
> + dev_err_ratelimited(viommu->dev, "%s fault from EP %u at %#llx [%s%s%s]\n",
> + reason_str, endpoint, address,
> + flags & VIRTIO_IOMMU_FAULT_F_READ ? "R" : "",
> + flags & VIRTIO_IOMMU_FAULT_F_WRITE ? "W" : "",
> + flags & VIRTIO_IOMMU_FAULT_F_EXEC ? "X" : "");
> + else
> + dev_err_ratelimited(viommu->dev, "%s fault from EP %u\n",
> + reason_str, endpoint);
> +
> + return 0;
> +}
> +
> +static void viommu_event_handler(struct virtqueue *vq)
> +{
> + int ret;
> + unsigned int len;
> + struct scatterlist sg[1];
> + struct viommu_event *evt;
> + struct viommu_dev *viommu = vq->vdev->priv;
> +
> + while ((evt = virtqueue_get_buf(vq, &len)) != NULL) {
> + if (len > sizeof(*evt)) {
> + dev_err(viommu->dev,
> + "invalid event buffer (len %u != %zu)\n",
> + len, sizeof(*evt));
> + } else if (!(evt->head & VIOMMU_FAULT_RESV_MASK)) {
> + viommu_fault_handler(viommu, &evt->fault);
> + }
> +
> + sg_init_one(sg, evt, sizeof(*evt));
in case of above error case, ie. len > sizeof(*evt), is it safe to push
evt again?
> + ret = virtqueue_add_inbuf(vq, sg, 1, evt, GFP_ATOMIC);
> + if (ret)
> + dev_err(viommu->dev, "could not add event buffer\n");
> + }
> +
> + if (!virtqueue_kick(vq))
> + dev_err(viommu->dev, "kick failed\n");
> +}
> +
> /* IOMMU API */
>
> static bool viommu_capable(enum iommu_cap cap)
> @@ -938,19 +1018,44 @@ static struct iommu_ops viommu_ops = {
> .put_resv_regions = viommu_put_resv_regions,
> };
>
> -static int viommu_init_vq(struct viommu_dev *viommu)
> +static int viommu_init_vqs(struct viommu_dev *viommu)
> {
> struct virtio_device *vdev = dev_to_virtio(viommu->dev);
> - const char *name = "request";
> - void *ret;
> + const char *names[] = { "request", "event" };
> + vq_callback_t *callbacks[] = {
> + NULL, /* No async requests */
> + viommu_event_handler,
> + };
> +
> + return virtio_find_vqs(vdev, VIOMMU_NUM_VQS, viommu->vqs, callbacks,
> + names, NULL);
> +}
>
> - ret = virtio_find_single_vq(vdev, NULL, name);
> - if (IS_ERR(ret)) {
> - dev_err(viommu->dev, "cannot find VQ\n");
> - return PTR_ERR(ret);
> +static int viommu_fill_evtq(struct viommu_dev *viommu)
> +{
> + int i, ret;
> + struct scatterlist sg[1];
> + struct viommu_event *evts;
> + struct virtqueue *vq = viommu->vqs[VIOMMU_EVENT_VQ];
> + size_t nr_evts = min_t(size_t, PAGE_SIZE / sizeof(struct viommu_event),
> + viommu->vqs[VIOMMU_EVENT_VQ]->num_free);
> +
> + evts = devm_kmalloc_array(viommu->dev, nr_evts, sizeof(*evts),
> + GFP_KERNEL);
> + if (!evts)
> + return -ENOMEM;
> +
> + for (i = 0; i < nr_evts; i++) {
> + sg_init_one(sg, &evts[i], sizeof(*evts));
> + ret = virtqueue_add_inbuf(vq, sg, 1, &evts[i], GFP_KERNEL);
who does the deallocation of evts?
Thanks
Eric
> + if (ret)
> + return ret;
> }
>
> - viommu->vq = ret;
> + if (!virtqueue_kick(vq))
> + return -EPIPE;
> +
> + dev_info(viommu->dev, "%zu event buffers\n", nr_evts);
>
> return 0;
> }
> @@ -973,7 +1078,7 @@ static int viommu_probe(struct virtio_device *vdev)
> viommu->dev = dev;
> viommu->vdev = vdev;
>
> - ret = viommu_init_vq(viommu);
> + ret = viommu_init_vqs(viommu);
> if (ret)
> goto err_free_viommu;
>
> @@ -1014,6 +1119,11 @@ static int viommu_probe(struct virtio_device *vdev)
>
> virtio_device_ready(vdev);
>
> + /* Populate the event queue with buffers */
> + ret = viommu_fill_evtq(viommu);
> + if (ret)
> + goto err_free_viommu;
> +
> ret = iommu_device_sysfs_add(&viommu->iommu, dev, NULL, "%s",
> virtio_bus_name(vdev));
> if (ret)
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> index eec90a2ab547..b57543b4145b 100644
> --- a/include/uapi/linux/virtio_iommu.h
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -174,4 +174,22 @@ union virtio_iommu_req {
> struct virtio_iommu_req_probe probe;
> };
>
> +/* Fault types */
> +#define VIRTIO_IOMMU_FAULT_R_UNKNOWN 0
> +#define VIRTIO_IOMMU_FAULT_R_DOMAIN 1
> +#define VIRTIO_IOMMU_FAULT_R_MAPPING 2
> +
> +#define VIRTIO_IOMMU_FAULT_F_READ (1 << 0)
> +#define VIRTIO_IOMMU_FAULT_F_WRITE (1 << 1)
> +#define VIRTIO_IOMMU_FAULT_F_EXEC (1 << 2)
> +#define VIRTIO_IOMMU_FAULT_F_ADDRESS (1 << 8)
> +
> +struct virtio_iommu_fault {
> + __u8 reason;
> + __u8 padding[3];
> + __le32 flags;
> + __le32 endpoint;
> + __le64 address;
> +} __packed;
> +
> #endif
>
^ permalink raw reply
* Re: [RFC PATCH v2 2/5] iommu/virtio-iommu: Add probe request
From: Auger Eric @ 2018-01-16 9:25 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu, devel, linux-acpi, kvm, kvmarm,
virtualization, virtio-dev
Cc: Jayachandran.Nair, lorenzo.pieralisi, ashok.raj, mst,
marc.zyngier, will.deacon, rjw, robert.moore, lv.zheng,
sudeep.holla, lenb, robin.murphy, joro, hanjun.guo
In-Reply-To: <20171117185211.32593-3-jean-philippe.brucker@arm.com>
Hi Jean-Philippe,
On 17/11/17 19:52, Jean-Philippe Brucker wrote:
> When the device offers the probe feature, send a probe request for each
> device managed by the IOMMU. Extract RESV_MEM information. When we
> encounter a MSI doorbell region, set it up as a IOMMU_RESV_MSI region.
> This will tell other subsystems that there is no need to map the MSI
> doorbell in the virtio-iommu, because MSIs bypass it.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
> drivers/iommu/virtio-iommu.c | 165 ++++++++++++++++++++++++++++++++++++--
> include/uapi/linux/virtio_iommu.h | 37 +++++++++
> 2 files changed, 195 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> index feb8c8925c3a..79e0add94e05 100644
> --- a/drivers/iommu/virtio-iommu.c
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -45,6 +45,7 @@ struct viommu_dev {
> struct iommu_domain_geometry geometry;
> u64 pgsize_bitmap;
> u8 domain_bits;
> + u32 probe_size;
> };
>
> struct viommu_mapping {
> @@ -72,6 +73,7 @@ struct viommu_domain {
> struct viommu_endpoint {
> struct viommu_dev *viommu;
> struct viommu_domain *vdomain;
> + struct list_head resv_regions;
> };
>
> struct viommu_request {
> @@ -139,6 +141,10 @@ static int viommu_get_req_size(struct viommu_dev *viommu,
> case VIRTIO_IOMMU_T_UNMAP:
> size = sizeof(r->unmap);
> break;
> + case VIRTIO_IOMMU_T_PROBE:
> + *bottom += viommu->probe_size;
> + size = sizeof(r->probe) + *bottom;
> + break;
> default:
> return -EINVAL;
> }
> @@ -448,6 +454,106 @@ static int viommu_replay_mappings(struct viommu_domain *vdomain)
> return ret;
> }
>
> +static int viommu_add_resv_mem(struct viommu_endpoint *vdev,
> + struct virtio_iommu_probe_resv_mem *mem,
> + size_t len)
> +{
> + struct iommu_resv_region *region = NULL;
> + unsigned long prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +
> + u64 addr = le64_to_cpu(mem->addr);
> + u64 size = le64_to_cpu(mem->size);
> +
> + if (len < sizeof(*mem))
> + return -EINVAL;
> +
> + switch (mem->subtype) {
> + case VIRTIO_IOMMU_RESV_MEM_T_MSI:
> + region = iommu_alloc_resv_region(addr, size, prot,
> + IOMMU_RESV_MSI);
> + break;
> + case VIRTIO_IOMMU_RESV_MEM_T_RESERVED:
> + default:
> + region = iommu_alloc_resv_region(addr, size, 0,
> + IOMMU_RESV_RESERVED);
> + break;
> + }
> +
> + list_add(&vdev->resv_regions, ®ion->list);
> +
> + if (mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_RESERVED &&
> + mem->subtype != VIRTIO_IOMMU_RESV_MEM_T_MSI) {
> + /* Please update your driver. */
> + pr_warn("unknown resv mem subtype 0x%x\n", mem->subtype);
> + return -EINVAL;
> + }
why not adding this in the switch default case and do not call list_add
in case the subtype region is not recognized?
> +
> + return 0;
> +}
> +
> +static int viommu_probe_endpoint(struct viommu_dev *viommu, struct device *dev)
> +{
> + int ret;
> + u16 type, len;
> + size_t cur = 0;
> + struct virtio_iommu_req_probe *probe;
> + struct virtio_iommu_probe_property *prop;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> + struct viommu_endpoint *vdev = fwspec->iommu_priv;
> +
> + if (!fwspec->num_ids)
> + /* Trouble ahead. */
> + return -EINVAL;
> +
> + probe = kzalloc(sizeof(*probe) + viommu->probe_size +
> + sizeof(struct virtio_iommu_req_tail), GFP_KERNEL);
> + if (!probe)
> + return -ENOMEM;
> +
> + probe->head.type = VIRTIO_IOMMU_T_PROBE;
> + /*
> + * For now, assume that properties of an endpoint that outputs multiple
> + * IDs are consistent. Only probe the first one.
> + */
> + probe->endpoint = cpu_to_le32(fwspec->ids[0]);
> +
> + ret = viommu_send_req_sync(viommu, probe);
> + if (ret) {
goto out?
> + kfree(probe);
> + return ret;
> + }
> +
> + prop = (void *)probe->properties;
> + type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
> +
> + while (type != VIRTIO_IOMMU_PROBE_T_NONE &&
> + cur < viommu->probe_size) {
> + len = le16_to_cpu(prop->length);
> +
> + switch (type) {
> + case VIRTIO_IOMMU_PROBE_T_RESV_MEM:
> + ret = viommu_add_resv_mem(vdev, (void *)prop->value, len);
> + break;
> + default:
> + dev_dbg(dev, "unknown viommu prop 0x%x\n", type);
> + }
> +
> + if (ret)
> + dev_err(dev, "failed to parse viommu prop 0x%x\n", type);
> +
> + cur += sizeof(*prop) + len;
> + if (cur >= viommu->probe_size)
> + break;
> +
> + prop = (void *)probe->properties + cur;
> + type = le16_to_cpu(prop->type) & VIRTIO_IOMMU_PROBE_T_MASK;
> + }
> +
> + kfree(probe);
> +
> + return 0;
> +}
> +
> /* IOMMU API */
>
> static bool viommu_capable(enum iommu_cap cap)
> @@ -706,6 +812,7 @@ static struct viommu_dev *viommu_get_by_fwnode(struct fwnode_handle *fwnode)
>
> static int viommu_add_device(struct device *dev)
> {
> + int ret;
> struct iommu_group *group;
> struct viommu_endpoint *vdev;
> struct viommu_dev *viommu = NULL;
> @@ -723,8 +830,16 @@ static int viommu_add_device(struct device *dev)
> return -ENOMEM;
>
> vdev->viommu = viommu;
> + INIT_LIST_HEAD(&vdev->resv_regions);
> fwspec->iommu_priv = vdev;
>
> + if (viommu->probe_size) {
> + /* Get additional information for this endpoint */
> + ret = viommu_probe_endpoint(viommu, dev);
> + if (ret)
> + return ret;
> + }
> +
> /*
> * Last step creates a default domain and attaches to it. Everything
> * must be ready.
> @@ -738,7 +853,19 @@ static int viommu_add_device(struct device *dev)
>
> static void viommu_remove_device(struct device *dev)
> {
> - kfree(dev->iommu_fwspec->iommu_priv);
> + struct viommu_endpoint *vdev;
> + struct iommu_resv_region *entry, *next;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> + if (!fwspec || fwspec->ops != &viommu_ops)
> + return;
> +
> + vdev = fwspec->iommu_priv;
> +
> + list_for_each_entry_safe(entry, next, &vdev->resv_regions, list)
> + kfree(entry);
> +
> + kfree(vdev);
> }
>
> static struct iommu_group *viommu_device_group(struct device *dev)
> @@ -756,15 +883,34 @@ static int viommu_of_xlate(struct device *dev, struct of_phandle_args *args)
>
> static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
> {
> - struct iommu_resv_region *region;
> + struct iommu_resv_region *entry, *new_entry, *msi = NULL;
> + struct viommu_endpoint *vdev = dev->iommu_fwspec->iommu_priv;
> int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
>
> - region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, prot,
> - IOMMU_RESV_SW_MSI);
> - if (!region)
> - return;
> + list_for_each_entry(entry, &vdev->resv_regions, list) {
> + /*
> + * If the device registered a bypass MSI windows, use it.
> + * Otherwise add a software-mapped region
> + */
> + if (entry->type == IOMMU_RESV_MSI)
> + msi = entry;
> +
> + new_entry = kmemdup(entry, sizeof(*entry), GFP_KERNEL);
> + if (!new_entry)
> + return;
> + list_add_tail(&new_entry->list, head);
> + }
>
> - list_add_tail(®ion->list, head);
> + if (!msi) {
> + msi = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> + prot, IOMMU_RESV_SW_MSI);
> + if (!msi)
> + return;
> +
> + list_add_tail(&msi->list, head);
> + }
> +
> + iommu_dma_get_resv_regions(dev, head);
this change may belong to the 1st patch.
> }
>
> static void viommu_put_resv_regions(struct device *dev, struct list_head *head)
> @@ -854,6 +1000,10 @@ static int viommu_probe(struct virtio_device *vdev)
> struct virtio_iommu_config, domain_bits,
> &viommu->domain_bits);
>
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_PROBE,
> + struct virtio_iommu_config, probe_size,
> + &viommu->probe_size);
> +
> viommu->geometry = (struct iommu_domain_geometry) {
> .aperture_start = input_start,
> .aperture_end = input_end,
> @@ -931,6 +1081,7 @@ static unsigned int features[] = {
> VIRTIO_IOMMU_F_MAP_UNMAP,
> VIRTIO_IOMMU_F_DOMAIN_BITS,
> VIRTIO_IOMMU_F_INPUT_RANGE,
> + VIRTIO_IOMMU_F_PROBE,
> };
>
> static struct virtio_device_id id_table[] = {
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> index 2b4cd2042897..eec90a2ab547 100644
> --- a/include/uapi/linux/virtio_iommu.h
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -38,6 +38,7 @@
> #define VIRTIO_IOMMU_F_DOMAIN_BITS 1
> #define VIRTIO_IOMMU_F_MAP_UNMAP 2
> #define VIRTIO_IOMMU_F_BYPASS 3
> +#define VIRTIO_IOMMU_F_PROBE 4
>
> struct virtio_iommu_config {
> /* Supported page sizes */
> @@ -49,6 +50,9 @@ struct virtio_iommu_config {
> } input_range;
> /* Max domain ID size */
> __u8 domain_bits;
> + __u8 padding[3];
> + /* Probe buffer size */
> + __u32 probe_size;
> } __packed;
>
> /* Request types */
> @@ -56,6 +60,7 @@ struct virtio_iommu_config {
> #define VIRTIO_IOMMU_T_DETACH 0x02
> #define VIRTIO_IOMMU_T_MAP 0x03
> #define VIRTIO_IOMMU_T_UNMAP 0x04
> +#define VIRTIO_IOMMU_T_PROBE 0x05
>
> /* Status types */
> #define VIRTIO_IOMMU_S_OK 0x00
> @@ -128,6 +133,37 @@ struct virtio_iommu_req_unmap {
> struct virtio_iommu_req_tail tail;
> } __packed;
>
> +#define VIRTIO_IOMMU_RESV_MEM_T_RESERVED 0
> +#define VIRTIO_IOMMU_RESV_MEM_T_MSI 1
> +
> +struct virtio_iommu_probe_resv_mem {
> + __u8 subtype;
> + __u8 reserved[3];
> + __le64 addr;
> + __le64 size;
> +} __packed;
> +
> +#define VIRTIO_IOMMU_PROBE_T_NONE 0
> +#define VIRTIO_IOMMU_PROBE_T_RESV_MEM 1
> +
> +#define VIRTIO_IOMMU_PROBE_T_MASK 0xfff
> +
> +struct virtio_iommu_probe_property {
> + __le16 type;
> + __le16 length;
> + __u8 value[];
> +} __packed;
> +
> +struct virtio_iommu_req_probe {
> + struct virtio_iommu_req_head head;
> + __le32 endpoint;
> + __u8 reserved[64];
> +
> + __u8 properties[];
> +
> + /* Tail follows the variable-length properties array (no padding) */
> +} __packed;
> +
> union virtio_iommu_req {
> struct virtio_iommu_req_head head;
>
> @@ -135,6 +171,7 @@ union virtio_iommu_req {
> struct virtio_iommu_req_detach detach;
> struct virtio_iommu_req_map map;
> struct virtio_iommu_req_unmap unmap;
> + struct virtio_iommu_req_probe probe;
> };
>
> #endif
>
Besides those minor comments, looks good to me.
Thanks
Eric
^ permalink raw reply
* Re: PCIe ordering and new VIRTIO packed ring format.
From: Michael S. Tsirkin @ 2018-01-16 2:40 UTC (permalink / raw)
To: Ilya Lesokhin
Cc: virtio-dev@lists.oasis-open.org, Or Gerlitz, Liran Liss,
virtualization@lists.linux-foundation.org
In-Reply-To: <AM4PR0501MB27230E63B2FC6BBF79D1419FD4150@AM4PR0501MB2723.eurprd05.prod.outlook.com>
On Sun, Jan 14, 2018 at 07:45:50AM +0000, Ilya Lesokhin wrote:
> Hi,
> I have a concern about the portability of offloading the new VIRTIO packed ring format to hardware.
>
> According to the PCIe rev 2.0, paragraph 2.4.2. Update Ordering and Granularity Observed by a Read Transaction"
> " if a host CPU writes a QWORD to host memory, a Requester reading that QWORD from host memory may observe a portion of the QWORD updated and another portion of it containing the old value."
>
> This means that after the device reads a 16byte descriptor, it cannot know that all the values In the descriptor are up to date even if the VIRTQ_DESC_F_AVAIL bit is set.
> This is true even if the driver uses the appropriate memory barriers.
>
> We encountered this behavior in practice on x86 servers. Our solution was to add an index to the latest valid descriptor
>
> Note that in practice the update granularity in x86 seems to be a cacheline, But this is not guaranteed by the spec.
> The spec only makes the following recommendation:
> "While not required by this specification, it is strongly recommended that host platforms guarantee that when a host CPU writes aligned DWORDs or aligned QWORDs to host memory, the update granularity observed by a PCI Express read will not be smaller than a DWORD."
>
> Thanks,
> Ilya
This is a very good point. This consideration is one of the reasons I
included last valid descriptor in the driver notification. My guess
would be that such hardware should never use driver event suppression.
As a result, driver will always send notifications after each batch of
descriptors. Device can use that to figure out which descriptors to
fetch. Luckily, with pass-through device memory can be mapped
directly into the VM, so no the notification will not trigger
a VM exit.
It would be interesting to find out whether specific host systems
give a stronger guarantee than what is required by the PCIE spec.
If so we could add e.g. a feature bit to let the device
know it's safe to read beyond the index supplied in the kick
notification. Drivers would detect this and use it to reduce
the overhead.
Conversely, this is also why I selected:
#define VIRTQ_DESC_F_USED 15
this way we don't have the same issue in the reverse order:
the last byte is used to mark buffer as used,
which actually seems to be guaranteed to happen the last from
software point of view in a portable.
--
MST
^ permalink raw reply
* Re: [RFC PATCH v2 1/5] iommu: Add virtio-iommu driver
From: Auger Eric @ 2018-01-15 15:12 UTC (permalink / raw)
To: Jean-Philippe Brucker, iommu, devel, linux-acpi, kvm, kvmarm,
virtualization, virtio-dev
Cc: Jayachandran.Nair, lorenzo.pieralisi, ashok.raj, mst,
marc.zyngier, will.deacon, rjw, robert.moore, lv.zheng,
sudeep.holla, lenb, robin.murphy, joro, hanjun.guo
In-Reply-To: <20171117185211.32593-2-jean-philippe.brucker@arm.com>
Hi Jean-Philippe,
please find some comments below.
On 17/11/17 19:52, Jean-Philippe Brucker wrote:
> The virtio IOMMU is a para-virtualized device, allowing to send IOMMU
> requests such as map/unmap over virtio-mmio transport without emulating
> page tables. This implementation handle ATTACH, DETACH, MAP and UNMAP
handles
> requests.
>
> The bulk of the code is to create requests and send them through virtio.
> Implementing the IOMMU API is fairly straightforward since the
> virtio-iommu MAP/UNMAP interface is almost identical.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
> drivers/iommu/Kconfig | 11 +
> drivers/iommu/Makefile | 1 +
> drivers/iommu/virtio-iommu.c | 958 ++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/virtio_ids.h | 1 +
> include/uapi/linux/virtio_iommu.h | 140 ++++++
> 5 files changed, 1111 insertions(+)
> create mode 100644 drivers/iommu/virtio-iommu.c
> create mode 100644 include/uapi/linux/virtio_iommu.h
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 17b212f56e6a..7271e59e8b23 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -403,4 +403,15 @@ config QCOM_IOMMU
> help
> Support for IOMMU on certain Qualcomm SoCs.
>
> +config VIRTIO_IOMMU
> + bool "Virtio IOMMU driver"
> + depends on VIRTIO_MMIO
> + select IOMMU_API
> + select INTERVAL_TREE
> + select ARM_DMA_USE_IOMMU if ARM
> + help
> + Para-virtualised IOMMU driver with virtio.
> +
> + Say Y here if you intend to run this kernel as a guest.
> +
> endif # IOMMU_SUPPORT
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index dca71fe1c885..432242f3a328 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -31,3 +31,4 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
> obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
> obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
> obj-$(CONFIG_QCOM_IOMMU) += qcom_iommu.o
> +obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
> diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> new file mode 100644
> index 000000000000..feb8c8925c3a
> --- /dev/null
> +++ b/drivers/iommu/virtio-iommu.c
> @@ -0,0 +1,958 @@
> +/*
> + * Virtio driver for the paravirtualized IOMMU
> + *
> + * Copyright (C) 2017 ARM Limited
> + * Author: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/amba/bus.h>
> +#include <linux/delay.h>
> +#include <linux/dma-iommu.h>
> +#include <linux/freezer.h>
> +#include <linux/interval_tree.h>
> +#include <linux/iommu.h>
> +#include <linux/module.h>
> +#include <linux/of_iommu.h>
> +#include <linux/of_platform.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/virtio.h>
> +#include <linux/virtio_config.h>
> +#include <linux/virtio_ids.h>
> +#include <linux/wait.h>
> +
> +#include <uapi/linux/virtio_iommu.h>
> +
> +#define MSI_IOVA_BASE 0x8000000
> +#define MSI_IOVA_LENGTH 0x100000
> +
> +struct viommu_dev {
> + struct iommu_device iommu;
> + struct device *dev;
> + struct virtio_device *vdev;
> +
> + struct ida domain_ids;
> +
> + struct virtqueue *vq;
> + /* Serialize anything touching the request queue */
> + spinlock_t request_lock;
> +
> + /* Device configuration */
> + struct iommu_domain_geometry geometry;
> + u64 pgsize_bitmap;
> + u8 domain_bits;
> +};
> +
> +struct viommu_mapping {
> + phys_addr_t paddr;
> + struct interval_tree_node iova;
> + union {
> + struct virtio_iommu_req_map map;
> + struct virtio_iommu_req_unmap unmap;
> + } req;
> +};
> +
> +struct viommu_domain {
> + struct iommu_domain domain;
> + struct viommu_dev *viommu;
> + struct mutex mutex;
> + unsigned int id;
> +
> + spinlock_t mappings_lock;
> + struct rb_root_cached mappings;
> +
> + /* Number of endpoints attached to this domain */
> + refcount_t endpoints;
> +};
> +
> +struct viommu_endpoint {
> + struct viommu_dev *viommu;
> + struct viommu_domain *vdomain;
> +};
> +
> +struct viommu_request {
> + struct scatterlist top;
> + struct scatterlist bottom;
> +
> + int written;
> + struct list_head list;
> +};
> +
> +#define to_viommu_domain(domain) container_of(domain, struct viommu_domain, domain)
> +
> +/* Virtio transport */
> +
> +static int viommu_status_to_errno(u8 status)
> +{
> + switch (status) {
> + case VIRTIO_IOMMU_S_OK:
> + return 0;
> + case VIRTIO_IOMMU_S_UNSUPP:
> + return -ENOSYS;
> + case VIRTIO_IOMMU_S_INVAL:
> + return -EINVAL;
> + case VIRTIO_IOMMU_S_RANGE:
> + return -ERANGE;
> + case VIRTIO_IOMMU_S_NOENT:
> + return -ENOENT;
> + case VIRTIO_IOMMU_S_FAULT:
> + return -EFAULT;
> + case VIRTIO_IOMMU_S_IOERR:
> + case VIRTIO_IOMMU_S_DEVERR:
> + default:
> + return -EIO;
> + }
> +}
> +
> +/*
> + * viommu_get_req_size - compute request size
> + *
> + * A virtio-iommu request is split into one device-read-only part (top) and one
> + * device-write-only part (bottom). Given a request, return the sizes of the two
> + * parts in @top and @bottom.
for all but virtio_iommu_req_probe, which has a variable bottom size
> + *
> + * Return 0 on success, or an error when the request seems invalid.
> + */
> +static int viommu_get_req_size(struct viommu_dev *viommu,
> + struct virtio_iommu_req_head *req, size_t *top,
> + size_t *bottom)
> +{
> + size_t size;
> + union virtio_iommu_req *r = (void *)req;
> +
> + *bottom = sizeof(struct virtio_iommu_req_tail);
> +
> + switch (req->type) {
> + case VIRTIO_IOMMU_T_ATTACH:
> + size = sizeof(r->attach);
> + break;
> + case VIRTIO_IOMMU_T_DETACH:
> + size = sizeof(r->detach);
> + break;
> + case VIRTIO_IOMMU_T_MAP:
> + size = sizeof(r->map);
> + break;
> + case VIRTIO_IOMMU_T_UNMAP:
> + size = sizeof(r->unmap);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + *top = size - *bottom;
> + return 0;
> +}
> +
> +static int viommu_receive_resp(struct viommu_dev *viommu, int nr_sent,
> + struct list_head *sent)
> +{
> +
> + unsigned int len;
> + int nr_received = 0;
> + struct viommu_request *req, *pending;
> +
> + pending = list_first_entry_or_null(sent, struct viommu_request, list);
> + if (WARN_ON(!pending))
> + return 0;
> +
> + while ((req = virtqueue_get_buf(viommu->vq, &len)) != NULL) {
> + if (req != pending) {
> + dev_warn(viommu->dev, "discarding stale request\n");
> + continue;
> + }
> +
> + pending->written = len;
> +
> + if (++nr_received == nr_sent) {
> + WARN_ON(!list_is_last(&pending->list, sent));
> + break;
> + } else if (WARN_ON(list_is_last(&pending->list, sent))) {
> + break;
> + }
> +
> + pending = list_next_entry(pending, list);
> + }
> +
> + return nr_received;
> +}
> +
> +/* Must be called with request_lock held */
> +static int _viommu_send_reqs_sync(struct viommu_dev *viommu,
> + struct viommu_request *req, int nr,
> + int *nr_sent)
> +{
> + int i, ret;
> + ktime_t timeout;
> + LIST_HEAD(pending);
> + int nr_received = 0;
> + struct scatterlist *sg[2];
> + /*
> + * Yes, 1s timeout. As a guest, we don't necessarily have a precise
> + * notion of time and this just prevents locking up a CPU if the device
> + * dies.
> + */
> + unsigned long timeout_ms = 1000;
> +
> + *nr_sent = 0;
> +
> + for (i = 0; i < nr; i++, req++) {
> + req->written = 0;
> +
> + sg[0] = &req->top;
> + sg[1] = &req->bottom;
> +
> + ret = virtqueue_add_sgs(viommu->vq, sg, 1, 1, req,
> + GFP_ATOMIC);
> + if (ret)
> + break;
> +
> + list_add_tail(&req->list, &pending);
> + }
> +
> + if (i && !virtqueue_kick(viommu->vq))
> + return -EPIPE;
> +
> + timeout = ktime_add_ms(ktime_get(), timeout_ms * i);
I don't really understand how you choose your timeout value: 1s per sent
request.
> + while (nr_received < i && ktime_before(ktime_get(), timeout)) {
> + nr_received += viommu_receive_resp(viommu, i - nr_received,
> + &pending);
> + if (nr_received < i) {
> + /*
> + * FIXME: what's a good way to yield to host? A second
> + * virtqueue_kick won't have any effect since we haven't
> + * added any descriptor.
> + */
> + udelay(10);
could you explain why udelay gets used here?
> + }
> + }
> +
> + if (nr_received != i)
> + ret = -ETIMEDOUT;
> +
> + if (ret == -ENOSPC && nr_received)
> + /*
> + * We've freed some space since virtio told us that the ring is
> + * full, tell the caller to come back for more.
> + */
> + ret = -EAGAIN;
> +
> + *nr_sent = nr_received;
> +
> + return ret;
> +}
> +
> +/*
> + * viommu_send_reqs_sync - add a batch of requests, kick the host and wait for
> + * them to return
> + *
> + * @req: array of requests
> + * @nr: array length
> + * @nr_sent: on return, contains the number of requests actually sent
> + *
> + * Return 0 on success, or an error if we failed to send some of the requests.
> + */
> +static int viommu_send_reqs_sync(struct viommu_dev *viommu,
> + struct viommu_request *req, int nr,
> + int *nr_sent)
> +{
> + int ret;
> + int sent = 0;
> + unsigned long flags;
> +
> + *nr_sent = 0;
> + do {
> + spin_lock_irqsave(&viommu->request_lock, flags);
> + ret = _viommu_send_reqs_sync(viommu, req, nr, &sent);
> + spin_unlock_irqrestore(&viommu->request_lock, flags);
> +
> + *nr_sent += sent;
> + req += sent;
> + nr -= sent;
> + } while (ret == -EAGAIN);
> +
> + return ret;
> +}
> +
> +/*
> + * viommu_send_req_sync - send one request and wait for reply
> + *
> + * @top: pointer to a virtio_iommu_req_* structure
> + *
> + * Returns 0 if the request was successful, or an error number otherwise. No
> + * distinction is done between transport and request errors.
> + */
> +static int viommu_send_req_sync(struct viommu_dev *viommu, void *top)
> +{
> + int ret;
> + int nr_sent;
> + void *bottom;
> + struct viommu_request req = {0};
^
drivers/iommu/virtio-iommu.c:326:9: warning: (near initialization for
‘req.top’) [-Wmissing-braces]
> + size_t top_size, bottom_size;
> + struct virtio_iommu_req_tail *tail;
> + struct virtio_iommu_req_head *head = top;
> +
> + ret = viommu_get_req_size(viommu, head, &top_size, &bottom_size);
> + if (ret)
> + return ret;
> +
> + bottom = top + top_size;
> + tail = bottom + bottom_size - sizeof(*tail);
> +
> + sg_init_one(&req.top, top, top_size);
> + sg_init_one(&req.bottom, bottom, bottom_size);
> +
> + ret = viommu_send_reqs_sync(viommu, &req, 1, &nr_sent);
> + if (ret || !req.written || nr_sent != 1) {
> + dev_err(viommu->dev, "failed to send request\n");
> + return -EIO;
> + }
> +
> + return viommu_status_to_errno(tail->status);
> +}
> +
> +/*
> + * viommu_add_mapping - add a mapping to the internal tree
> + *
> + * On success, return the new mapping. Otherwise return NULL.
> + */
> +static struct viommu_mapping *
> +viommu_add_mapping(struct viommu_domain *vdomain, unsigned long iova,
> + phys_addr_t paddr, size_t size)
> +{
> + unsigned long flags;
> + struct viommu_mapping *mapping;
> +
> + mapping = kzalloc(sizeof(*mapping), GFP_ATOMIC);
> + if (!mapping)
> + return NULL;
> +
> + mapping->paddr = paddr;
> + mapping->iova.start = iova;
> + mapping->iova.last = iova + size - 1;
> +
> + spin_lock_irqsave(&vdomain->mappings_lock, flags);
> + interval_tree_insert(&mapping->iova, &vdomain->mappings);
> + spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
> +
> + return mapping;
> +}
> +
> +/*
> + * viommu_del_mappings - remove mappings from the internal tree
> + *
> + * @vdomain: the domain
> + * @iova: start of the range
> + * @size: size of the range. A size of 0 corresponds to the entire address
> + * space.
> + * @out_mapping: if not NULL, the first removed mapping is returned in there.
> + * This allows the caller to reuse the buffer for the unmap request. Caller
> + * must always free the returned mapping, whether the function succeeds or
> + * not.
if unmapped > 0?
> + *
> + * On success, returns the number of unmapped bytes (>= size)
> + */
> +static size_t viommu_del_mappings(struct viommu_domain *vdomain,
> + unsigned long iova, size_t size,
> + struct viommu_mapping **out_mapping)
> +{
> + size_t unmapped = 0;
> + unsigned long flags;
> + unsigned long last = iova + size - 1;
> + struct viommu_mapping *mapping = NULL;
> + struct interval_tree_node *node, *next;
> +
> + spin_lock_irqsave(&vdomain->mappings_lock, flags);
> + next = interval_tree_iter_first(&vdomain->mappings, iova, last);
> +
> + if (next) {
> + mapping = container_of(next, struct viommu_mapping, iova);
> + /* Trying to split a mapping? */
> + if (WARN_ON(mapping->iova.start < iova))
> + next = NULL;
> + }
> +
> + while (next) {
> + node = next;
> + mapping = container_of(node, struct viommu_mapping, iova);
> +
> + next = interval_tree_iter_next(node, iova, last);
> +
> + /*
> + * Note that for a partial range, this will return the full
> + * mapping so we avoid sending split requests to the device.
> + */
> + unmapped += mapping->iova.last - mapping->iova.start + 1;
> +
> + interval_tree_remove(node, &vdomain->mappings);
> +
> + if (out_mapping && !(*out_mapping))
> + *out_mapping = mapping;
> + else
> + kfree(mapping);
> + }
> + spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
> +
> + return unmapped;
> +}
> +
> +/*
> + * viommu_replay_mappings - re-send MAP requests
> + *
> + * When reattaching a domain that was previously detached from all devices,
> + * mappings were deleted from the device. Re-create the mappings available in
> + * the internal tree.
> + *
> + * Caller should hold the mapping lock if necessary.
the only caller does not hold the lock. At this point we are attaching
our fisrt ep to the domain. I think it would be worth a comment in the
caller.
> + */
> +static int viommu_replay_mappings(struct viommu_domain *vdomain)
> +{
> + int i = 1, ret, nr_sent;
> + struct viommu_request *reqs;
> + struct viommu_mapping *mapping;
> + struct interval_tree_node *node;
> + size_t top_size, bottom_size;
> +
> + node = interval_tree_iter_first(&vdomain->mappings, 0, -1UL);
> + if (!node)
> + return 0;
> +
> + while ((node = interval_tree_iter_next(node, 0, -1UL)) != NULL)
> + i++;
> +
> + reqs = kcalloc(i, sizeof(*reqs), GFP_KERNEL);
> + if (!reqs)
> + return -ENOMEM;
> +
> + bottom_size = sizeof(struct virtio_iommu_req_tail);
> + top_size = sizeof(struct virtio_iommu_req_map) - bottom_size;
> +
> + i = 0;
> + node = interval_tree_iter_first(&vdomain->mappings, 0, -1UL);
> + while (node) {
> + mapping = container_of(node, struct viommu_mapping, iova);
> + sg_init_one(&reqs[i].top, &mapping->req.map, top_size);
> + sg_init_one(&reqs[i].bottom, &mapping->req.map.tail,
> + bottom_size);
> +
> + node = interval_tree_iter_next(node, 0, -1UL);
> + i++;
> + }
> +
> + ret = viommu_send_reqs_sync(vdomain->viommu, reqs, i, &nr_sent);
> + kfree(reqs);
> +
> + return ret;
> +}
> +
> +/* IOMMU API */
> +
> +static bool viommu_capable(enum iommu_cap cap)
> +{
> + return false; /* :( */
> +}
> +
> +static struct iommu_domain *viommu_domain_alloc(unsigned type)
> +{
> + struct viommu_domain *vdomain;
> +
> + if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
> + return NULL;
> +
> + vdomain = kzalloc(sizeof(*vdomain), GFP_KERNEL);
> + if (!vdomain)
> + return NULL;
> +
> + mutex_init(&vdomain->mutex);
> + spin_lock_init(&vdomain->mappings_lock);
> + vdomain->mappings = RB_ROOT_CACHED;
> + refcount_set(&vdomain->endpoints, 0);
> +
> + if (type == IOMMU_DOMAIN_DMA &&
> + iommu_get_dma_cookie(&vdomain->domain)) {
> + kfree(vdomain);
> + return NULL;
> + }
> +
> + return &vdomain->domain;
> +}
> +
> +static int viommu_domain_finalise(struct viommu_dev *viommu,
> + struct iommu_domain *domain)
> +{
> + int ret;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> + /* ida limits size to 31 bits. A value of 0 means "max" */
> + unsigned int max_domain = viommu->domain_bits >= 31 ? 0 :
> + 1U << viommu->domain_bits;
> +
> + vdomain->viommu = viommu;
> +
> + domain->pgsize_bitmap = viommu->pgsize_bitmap;
> + domain->geometry = viommu->geometry;
> +
> + ret = ida_simple_get(&viommu->domain_ids, 0, max_domain, GFP_KERNEL);
> + if (ret >= 0)
> + vdomain->id = (unsigned int)ret;
> +
> + return ret > 0 ? 0 : ret;
> +}
> +
> +static void viommu_domain_free(struct iommu_domain *domain)
> +{
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + iommu_put_dma_cookie(domain);
> +
> + /* Free all remaining mappings (size 2^64) */
> + viommu_del_mappings(vdomain, 0, 0, NULL);
> +
> + if (vdomain->viommu)
> + ida_simple_remove(&vdomain->viommu->domain_ids, vdomain->id);
> +
> + kfree(vdomain);
> +}
> +
> +static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev)
> +{
> + int i;
> + int ret = 0;
> + struct virtio_iommu_req_attach *req;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> + struct viommu_endpoint *vdev = fwspec->iommu_priv;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + mutex_lock(&vdomain->mutex);
> + if (!vdomain->viommu) {
> + /*
> + * Initialize the domain proper now that we know which viommu
> + * owns it.
> + */
> + ret = viommu_domain_finalise(vdev->viommu, domain);
> + } else if (vdomain->viommu != vdev->viommu) {
> + dev_err(dev, "cannot attach to foreign vIOMMU\n");
> + ret = -EXDEV;
> + }
> + mutex_unlock(&vdomain->mutex);
> +
> + if (ret)
> + return ret;
> +
> + /*
> + * When attaching the device to a new domain, it will be detached from
> + * the old one and, if as as a result the old domain isn't attached to
as as
> + * any device, all mappings are removed from the old domain and it is
> + * freed. (Note that we can't use get_domain_for_dev here, it returns
> + * the default domain during initial attach.)
I don't see where the old domain is freed. I see you descrement the
endpoints ref count. Also if you replay the mapping, I guess the
mappings were not destroyed?
> + *
> + * Take note of the device disappearing, so we can ignore unmap request
> + * on stale domains (that is, between this detach and the upcoming
> + * free.)
> + *
> + * vdev->vdomain is protected by group->mutex
> + */
> + if (vdev->vdomain)
> + refcount_dec(&vdev->vdomain->endpoints);
> +
> + /* DMA to the stack is forbidden, store request on the heap */
> + req = kzalloc(sizeof(*req), GFP_KERNEL);
> + if (!req)
> + return -ENOMEM;
> +
> + *req = (struct virtio_iommu_req_attach) {
> + .head.type = VIRTIO_IOMMU_T_ATTACH,
> + .domain = cpu_to_le32(vdomain->id),
> + };
> +
> + for (i = 0; i < fwspec->num_ids; i++) {
> + req->endpoint = cpu_to_le32(fwspec->ids[i]);
> +
> + ret = viommu_send_req_sync(vdomain->viommu, req);
> + if (ret)
> + break;
> + }
> +
> + kfree(req);
> +
> + if (ret)
> + return ret;
> +
> + if (!refcount_read(&vdomain->endpoints)) {
> + /*
> + * This endpoint is the first to be attached to the domain.
> + * Replay existing mappings if any.
> + */
> + ret = viommu_replay_mappings(vdomain);
> + if (ret)
> + return ret;
> + }
> +
> + refcount_inc(&vdomain->endpoints);
This does not work for me as the ref counter is initialized to 0 and
ref_count does not work if the counter is 0. It emits a WARN_ON and
stays at 0. I worked around the issue by explicitly setting
recount_set(&vdomain->endpoints, 1) if it was 0 and reffount_inc otherwise.
> + vdev->vdomain = vdomain;
> +
> + return 0;
> +}
> +
> +static int viommu_map(struct iommu_domain *domain, unsigned long iova,
> + phys_addr_t paddr, size_t size, int prot)
> +{
> + int ret;
> + int flags;
> + struct viommu_mapping *mapping;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + mapping = viommu_add_mapping(vdomain, iova, paddr, size);
> + if (!mapping)
> + return -ENOMEM;
> +
> + flags = (prot & IOMMU_READ ? VIRTIO_IOMMU_MAP_F_READ : 0) |
> + (prot & IOMMU_WRITE ? VIRTIO_IOMMU_MAP_F_WRITE : 0);
> +
> + mapping->req.map = (struct virtio_iommu_req_map) {
> + .head.type = VIRTIO_IOMMU_T_MAP,
> + .domain = cpu_to_le32(vdomain->id),
> + .virt_addr = cpu_to_le64(iova),
> + .phys_addr = cpu_to_le64(paddr),
> + .size = cpu_to_le64(size),
> + .flags = cpu_to_le32(flags),
> + };
> +
> + if (!refcount_read(&vdomain->endpoints))
> + return 0;
> +
> + ret = viommu_send_req_sync(vdomain->viommu, &mapping->req);
> + if (ret)
> + viommu_del_mappings(vdomain, iova, size, NULL);
> +
> + return ret;
> +}
> +
> +static size_t viommu_unmap(struct iommu_domain *domain, unsigned long iova,
> + size_t size)
> +{
> + int ret = 0;
> + size_t unmapped;
> + struct viommu_mapping *mapping = NULL;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + unmapped = viommu_del_mappings(vdomain, iova, size, &mapping);
> + if (unmapped < size) {
> + ret = -EINVAL;
> + goto out_free;
> + }
> +
> + /* Device already removed all mappings after detach. */
> + if (!refcount_read(&vdomain->endpoints))
> + goto out_free;
> +
> + if (WARN_ON(!mapping))
> + return 0;
> +
> + mapping->req.unmap = (struct virtio_iommu_req_unmap) {
> + .head.type = VIRTIO_IOMMU_T_UNMAP,
> + .domain = cpu_to_le32(vdomain->id),
> + .virt_addr = cpu_to_le64(iova),
> + .size = cpu_to_le64(unmapped),
> + };
> +
> + ret = viommu_send_req_sync(vdomain->viommu, &mapping->req);
> +
> +out_free:
> + if (mapping)
> + kfree(mapping);
> +
> + return ret ? 0 : unmapped;
> +}
> +
> +static phys_addr_t viommu_iova_to_phys(struct iommu_domain *domain,
> + dma_addr_t iova)
> +{
> + u64 paddr = 0;
> + unsigned long flags;
> + struct viommu_mapping *mapping;
> + struct interval_tree_node *node;
> + struct viommu_domain *vdomain = to_viommu_domain(domain);
> +
> + spin_lock_irqsave(&vdomain->mappings_lock, flags);
> + node = interval_tree_iter_first(&vdomain->mappings, iova, iova);
> + if (node) {
> + mapping = container_of(node, struct viommu_mapping, iova);
> + paddr = mapping->paddr + (iova - mapping->iova.start);
> + }
> + spin_unlock_irqrestore(&vdomain->mappings_lock, flags);
> +
> + return paddr;
> +}
> +
> +static struct iommu_ops viommu_ops;
> +static struct virtio_driver virtio_iommu_drv;
> +
> +static int viommu_match_node(struct device *dev, void *data)
> +{
> + return dev->parent->fwnode == data;
> +}
> +
> +static struct viommu_dev *viommu_get_by_fwnode(struct fwnode_handle *fwnode)
> +{
> + struct device *dev = driver_find_device(&virtio_iommu_drv.driver, NULL,
> + fwnode, viommu_match_node);
> + put_device(dev);
> +
> + return dev ? dev_to_virtio(dev)->priv : NULL;
> +}
> +
> +static int viommu_add_device(struct device *dev)
> +{
> + struct iommu_group *group;
> + struct viommu_endpoint *vdev;
> + struct viommu_dev *viommu = NULL;
> + struct iommu_fwspec *fwspec = dev->iommu_fwspec;
> +
> + if (!fwspec || fwspec->ops != &viommu_ops)
> + return -ENODEV;
> +
> + viommu = viommu_get_by_fwnode(fwspec->iommu_fwnode);
> + if (!viommu)
> + return -ENODEV;
> +
> + vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
> + if (!vdev)
> + return -ENOMEM;
> +
> + vdev->viommu = viommu;
> + fwspec->iommu_priv = vdev;
> +
> + /*
> + * Last step creates a default domain and attaches to it. Everything
> + * must be ready.
> + */
> + group = iommu_group_get_for_dev(dev);
> + if (!IS_ERR(group))
> + iommu_group_put(group);
> +
> + return PTR_ERR_OR_ZERO(group);
> +}
> +
> +static void viommu_remove_device(struct device *dev)
> +{
> + kfree(dev->iommu_fwspec->iommu_priv);
> +}
> +
> +static struct iommu_group *viommu_device_group(struct device *dev)
> +{
> + if (dev_is_pci(dev))
> + return pci_device_group(dev);
> + else
> + return generic_device_group(dev);
> +}
> +
> +static int viommu_of_xlate(struct device *dev, struct of_phandle_args *args)
> +{
> + return iommu_fwspec_add_ids(dev, args->args, 1);
> +}
> +
> +static void viommu_get_resv_regions(struct device *dev, struct list_head *head)
> +{
> + struct iommu_resv_region *region;
> + int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +
> + region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH, prot,
> + IOMMU_RESV_SW_MSI);
> + if (!region)
> + return;
> +
> + list_add_tail(®ion->list, head);
> +}
> +
> +static void viommu_put_resv_regions(struct device *dev, struct list_head *head)
> +{
> + struct iommu_resv_region *entry, *next;
> +
> + list_for_each_entry_safe(entry, next, head, list)
> + kfree(entry);
> +}
> +
> +static struct iommu_ops viommu_ops = {
> + .capable = viommu_capable,
> + .domain_alloc = viommu_domain_alloc,
> + .domain_free = viommu_domain_free,
> + .attach_dev = viommu_attach_dev,
> + .map = viommu_map,
> + .unmap = viommu_unmap,
> + .map_sg = default_iommu_map_sg,
> + .iova_to_phys = viommu_iova_to_phys,
> + .add_device = viommu_add_device,
> + .remove_device = viommu_remove_device,
> + .device_group = viommu_device_group,
> + .of_xlate = viommu_of_xlate,
> + .get_resv_regions = viommu_get_resv_regions,
> + .put_resv_regions = viommu_put_resv_regions,
> +};
> +
> +static int viommu_init_vq(struct viommu_dev *viommu)
> +{
> + struct virtio_device *vdev = dev_to_virtio(viommu->dev);
> + const char *name = "request";
> + void *ret;
> +
> + ret = virtio_find_single_vq(vdev, NULL, name);
> + if (IS_ERR(ret)) {
> + dev_err(viommu->dev, "cannot find VQ\n");
> + return PTR_ERR(ret);
> + }
> +
> + viommu->vq = ret;
> +
> + return 0;
> +}
> +
> +static int viommu_probe(struct virtio_device *vdev)
> +{
> + struct device *parent_dev = vdev->dev.parent;
> + struct viommu_dev *viommu = NULL;
> + struct device *dev = &vdev->dev;
> + u64 input_start = 0;
> + u64 input_end = -1UL;
> + int ret;
> +
> + viommu = kzalloc(sizeof(*viommu), GFP_KERNEL);
> + if (!viommu)
> + return -ENOMEM;
> +
> + spin_lock_init(&viommu->request_lock);
> + ida_init(&viommu->domain_ids);
> + viommu->dev = dev;
> + viommu->vdev = vdev;
> +
> + ret = viommu_init_vq(viommu);
> + if (ret)
> + goto err_free_viommu;
> +
> + virtio_cread(vdev, struct virtio_iommu_config, page_size_mask,
> + &viommu->pgsize_bitmap);
> +
> + if (!viommu->pgsize_bitmap) {
> + ret = -EINVAL;
> + goto err_free_viommu;
> + }
> +
> + viommu->domain_bits = 32;
> +
> + /* Optional features */
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
> + struct virtio_iommu_config, input_range.start,
> + &input_start);
> +
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_INPUT_RANGE,
> + struct virtio_iommu_config, input_range.end,
> + &input_end);
> +
> + virtio_cread_feature(vdev, VIRTIO_IOMMU_F_DOMAIN_BITS,
> + struct virtio_iommu_config, domain_bits,
> + &viommu->domain_bits);
> +
> + viommu->geometry = (struct iommu_domain_geometry) {
> + .aperture_start = input_start,
> + .aperture_end = input_end,
> + .force_aperture = true,
> + };
> +
> + viommu_ops.pgsize_bitmap = viommu->pgsize_bitmap;
> +
> + virtio_device_ready(vdev);
> +
> + ret = iommu_device_sysfs_add(&viommu->iommu, dev, NULL, "%s",
> + virtio_bus_name(vdev));
> + if (ret)
> + goto err_free_viommu;
> +
> + iommu_device_set_ops(&viommu->iommu, &viommu_ops);
> + iommu_device_set_fwnode(&viommu->iommu, parent_dev->fwnode);
> +
> + iommu_device_register(&viommu->iommu);
> +
> +#ifdef CONFIG_PCI
> + if (pci_bus_type.iommu_ops != &viommu_ops) {
> + pci_request_acs();
> + ret = bus_set_iommu(&pci_bus_type, &viommu_ops);
> + if (ret)
> + goto err_unregister;
> + }
> +#endif
> +#ifdef CONFIG_ARM_AMBA
> + if (amba_bustype.iommu_ops != &viommu_ops) {
> + ret = bus_set_iommu(&amba_bustype, &viommu_ops);
> + if (ret)
> + goto err_unregister;
> + }
> +#endif
> + if (platform_bus_type.iommu_ops != &viommu_ops) {
> + ret = bus_set_iommu(&platform_bus_type, &viommu_ops);
> + if (ret)
> + goto err_unregister;
> + }
> +
> + vdev->priv = viommu;
> +
> + dev_info(dev, "input address: %u bits\n",
> + order_base_2(viommu->geometry.aperture_end));
> + dev_info(dev, "page mask: %#llx\n", viommu->pgsize_bitmap);
> +
> + return 0;
> +
> +err_unregister:
> + iommu_device_unregister(&viommu->iommu);
> +
> +err_free_viommu:
> + kfree(viommu);
> +
> + return ret;
> +}
> +
> +static void viommu_remove(struct virtio_device *vdev)
> +{
> + struct viommu_dev *viommu = vdev->priv;
> +
> + iommu_device_unregister(&viommu->iommu);
> + kfree(viommu);
> +
> + dev_info(&vdev->dev, "device removed\n");
> +}
> +
> +static void viommu_config_changed(struct virtio_device *vdev)
> +{
> + dev_warn(&vdev->dev, "config changed\n");
> +}
> +
> +static unsigned int features[] = {
> + VIRTIO_IOMMU_F_MAP_UNMAP,
> + VIRTIO_IOMMU_F_DOMAIN_BITS,
> + VIRTIO_IOMMU_F_INPUT_RANGE,
> +};
> +
> +static struct virtio_device_id id_table[] = {
> + { VIRTIO_ID_IOMMU, VIRTIO_DEV_ANY_ID },
> + { 0 },
> +};
> +
> +static struct virtio_driver virtio_iommu_drv = {
> + .driver.name = KBUILD_MODNAME,
> + .driver.owner = THIS_MODULE,
> + .id_table = id_table,
> + .feature_table = features,
> + .feature_table_size = ARRAY_SIZE(features),
> + .probe = viommu_probe,
> + .remove = viommu_remove,
> + .config_changed = viommu_config_changed,
> +};
> +
> +module_virtio_driver(virtio_iommu_drv);
> +
> +IOMMU_OF_DECLARE(viommu, "virtio,mmio", NULL);
> +
> +MODULE_DESCRIPTION("Virtio IOMMU driver");
> +MODULE_AUTHOR("Jean-Philippe Brucker <jean-philippe.brucker@arm.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/uapi/linux/virtio_ids.h b/include/uapi/linux/virtio_ids.h
> index 6d5c3b2d4f4d..934ed3d3cd3f 100644
> --- a/include/uapi/linux/virtio_ids.h
> +++ b/include/uapi/linux/virtio_ids.h
> @@ -43,5 +43,6 @@
> #define VIRTIO_ID_INPUT 18 /* virtio input */
> #define VIRTIO_ID_VSOCK 19 /* virtio vsock transport */
> #define VIRTIO_ID_CRYPTO 20 /* virtio crypto */
> +#define VIRTIO_ID_IOMMU 61216 /* virtio IOMMU (temporary) */
>
> #endif /* _LINUX_VIRTIO_IDS_H */
> diff --git a/include/uapi/linux/virtio_iommu.h b/include/uapi/linux/virtio_iommu.h
> new file mode 100644
> index 000000000000..2b4cd2042897
> --- /dev/null
> +++ b/include/uapi/linux/virtio_iommu.h
> @@ -0,0 +1,140 @@
> +/*
> + * Virtio-iommu definition v0.5
> + *
> + * Copyright (C) 2017 ARM Ltd.
> + *
> + * This header is BSD licensed so anyone can use the definitions
> + * to implement compatible drivers/servers:
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of ARM Ltd. nor the names of its contributors
> + * may be used to endorse or promote products derived from this software
> + * without specific prior written permission.
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL IBM OR
> + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +#ifndef _UAPI_LINUX_VIRTIO_IOMMU_H
> +#define _UAPI_LINUX_VIRTIO_IOMMU_H
> +
> +/* Feature bits */
> +#define VIRTIO_IOMMU_F_INPUT_RANGE 0
> +#define VIRTIO_IOMMU_F_DOMAIN_BITS 1
> +#define VIRTIO_IOMMU_F_MAP_UNMAP 2
> +#define VIRTIO_IOMMU_F_BYPASS 3
> +
> +struct virtio_iommu_config {
> + /* Supported page sizes */
> + __u64 page_size_mask;
Get a warning
./usr/include/linux/virtio_iommu.h:45: found __[us]{8,16,32,64} type
without #include <linux/types.h>
> + /* Supported IOVA range */
> + struct virtio_iommu_range {
> + __u64 start;
> + __u64 end;
> + } input_range;
> + /* Max domain ID size */
> + __u8 domain_bits;
> +} __packed;
> +
> +/* Request types */
> +#define VIRTIO_IOMMU_T_ATTACH 0x01
> +#define VIRTIO_IOMMU_T_DETACH 0x02
> +#define VIRTIO_IOMMU_T_MAP 0x03
> +#define VIRTIO_IOMMU_T_UNMAP 0x04
> +
> +/* Status types */
> +#define VIRTIO_IOMMU_S_OK 0x00
> +#define VIRTIO_IOMMU_S_IOERR 0x01
> +#define VIRTIO_IOMMU_S_UNSUPP 0x02
> +#define VIRTIO_IOMMU_S_DEVERR 0x03
> +#define VIRTIO_IOMMU_S_INVAL 0x04
> +#define VIRTIO_IOMMU_S_RANGE 0x05
> +#define VIRTIO_IOMMU_S_NOENT 0x06
> +#define VIRTIO_IOMMU_S_FAULT 0x07
> +
> +struct virtio_iommu_req_head {
> + __u8 type;
> + __u8 reserved[3];
> +} __packed;
> +
> +struct virtio_iommu_req_tail {
> + __u8 status;
> + __u8 reserved[3];
> +} __packed;
> +
> +struct virtio_iommu_req_attach {
> + struct virtio_iommu_req_head head;
> +
> + __le32 domain;
> + __le32 endpoint;
> + __le32 reserved;
> +
> + struct virtio_iommu_req_tail tail;
> +} __packed;
> +
> +struct virtio_iommu_req_detach {
> + struct virtio_iommu_req_head head;
> +
> + __le32 endpoint;
> + __le32 reserved;
> +
> + struct virtio_iommu_req_tail tail;
> +} __packed;
> +
> +#define VIRTIO_IOMMU_MAP_F_READ (1 << 0)
> +#define VIRTIO_IOMMU_MAP_F_WRITE (1 << 1)
> +#define VIRTIO_IOMMU_MAP_F_EXEC (1 << 2)
> +
> +#define VIRTIO_IOMMU_MAP_F_MASK (VIRTIO_IOMMU_MAP_F_READ | \
> + VIRTIO_IOMMU_MAP_F_WRITE | \
> + VIRTIO_IOMMU_MAP_F_EXEC)
> +
> +struct virtio_iommu_req_map {
> + struct virtio_iommu_req_head head;
> +
> + __le32 domain;
> + __le64 virt_addr;
> + __le64 phys_addr;
> + __le64 size;
> + __le32 flags;
> +
> + struct virtio_iommu_req_tail tail;
> +} __packed;
> +
> +__packed
> +struct virtio_iommu_req_unmap {
> + struct virtio_iommu_req_head head;
> +
> + __le32 domain;
> + __le64 virt_addr;
> + __le64 size;
> + __le32 reserved;
> +
> + struct virtio_iommu_req_tail tail;
> +} __packed;
> +
> +union virtio_iommu_req {
> + struct virtio_iommu_req_head head;
> +
> + struct virtio_iommu_req_attach attach;
> + struct virtio_iommu_req_detach detach;
> + struct virtio_iommu_req_map map;
> + struct virtio_iommu_req_unmap unmap;
> +};
> +
> +#endif
>
Thanks
Eric
^ permalink raw reply
* PCIe ordering and new VIRTIO packed ring format.
From: Ilya Lesokhin @ 2018-01-14 7:45 UTC (permalink / raw)
To: Michael S. Tsirkin, virtualization@lists.linux-foundation.org
Cc: virtio-dev@lists.oasis-open.org, Or Gerlitz, Liran Liss
Hi,
I have a concern about the portability of offloading the new VIRTIO packed ring format to hardware.
According to the PCIe rev 2.0, paragraph 2.4.2. Update Ordering and Granularity Observed by a Read Transaction"
" if a host CPU writes a QWORD to host memory, a Requester reading that QWORD from host memory may observe a portion of the QWORD updated and another portion of it containing the old value."
This means that after the device reads a 16byte descriptor, it cannot know that all the values In the descriptor are up to date even if the VIRTQ_DESC_F_AVAIL bit is set.
This is true even if the driver uses the appropriate memory barriers.
We encountered this behavior in practice on x86 servers. Our solution was to add an index to the latest valid descriptor
Note that in practice the update granularity in x86 seems to be a cacheline, But this is not guaranteed by the spec.
The spec only makes the following recommendation:
"While not required by this specification, it is strongly recommended that host platforms guarantee that when a host CPU writes aligned DWORDs or aligned QWORDs to host memory, the update granularity observed by a PCI Express read will not be smaller than a DWORD."
Thanks,
Ilya
^ permalink raw reply
* Re: [PATCH v21 2/5 RESEND] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Wei Wang @ 2018-01-12 9:14 UTC (permalink / raw)
To: Tetsuo Handa, mst
Cc: aarcange, virtio-dev, riel, quan.xu0, kvm, mawilcox, qemu-devel,
amit.shah, liliang.opensource, linux-kernel, willy,
virtualization, linux-mm, yang.zhang.wz, nilal, cornelia.huck,
pbonzini, akpm, mhocko, mgorman
In-Reply-To: <201801112006.EHD48461.LOtVFFSOJMOFHQ@I-love.SAKURA.ne.jp>
On 01/11/2018 07:06 PM, Tetsuo Handa wrote:
> Wei Wang wrote:
>> Michael, could we merge patch 3-5 first?
> No! I'm repeatedly asking you to propose only VIRTIO_BALLOON_F_SG changes.
> Please don't ignore me.
>
>
>
> Patch 4 depends on patch 2. Thus, back to patch 2.
There is not strict dependence per se. I plan to split the two features
into 2 series, and post out 3-5 first, and the corresponding hypervisor
code.
After that's done, I'll get back to the discussion of patch 2.
> Now, proceeding to patch 4.
>
> Your patch is trying to call add_one_sg() for multiple times based on
>
> ----------------------------------------
> + /*
> + * This is expected to never fail: there is always at least 1 entry
> + * available on the vq, because when the vq is full the worker thread
> + * that adds the sg will be put into sleep until at least 1 entry is
> + * available to use.
> + */
This will be more clear in the new version which is not together with
patch 2.
>
> Now, I suspect we need to add VIRTIO_BALLOON_F_FREE_PAGE_VQ flag. I want to see
> the patch for the hypervisor side which makes use of VIRTIO_BALLOON_F_FREE_PAGE_VQ
> flag because its usage becomes tricky. Between the guest kernel obtains snapshot of
> free memory blocks and the hypervisor is told that some pages are currently free,
> these pages can become in use. That is, I don't think
>
> The second feature enables the optimization of the 1st round memory
> transfer - the hypervisor can skip the transfer of guest free pages in the
> 1st round.
>
> is accurate. The hypervisor is allowed to mark pages which are told as "currently
> unused" by the guest kernel as "write-protected" before starting the 1st round.
> Then, the hypervisor performs copying all pages except write-protected pages as
> the 1st round. Then, the 2nd and later rounds will be the same. That is,
> VIRTIO_BALLOON_F_FREE_PAGE_VQ requires the hypervisor to do 0th round as
> preparation. Thus, I want to see the patch for the hypervisor side.
>
> Now, what if all free pages in the guest kernel were reserved as ballooned pages?
> There will be no free pages which VIRTIO_BALLOON_F_FREE_PAGE_VQ flag would help.
> The hypervisor will have to copy all pages because all pages are either currently
> in-use or currently in balloons. After ballooning to appropriate size, there will
> be little free memory in the guest kernel, and the hypervisor already knows which
> pages are in the balloon. Thus, the hypervisor can skip copying the content of
> pages in the balloon, without using VIRTIO_BALLOON_F_FREE_PAGE_VQ flag.
>
> Then, why can't we do "inflate the balloon up to reasonable level (e.g. no need to
> wait for reclaim and no need to deflate)" instead of "find all the free pages as of
> specific moment" ? That is, code for VIRTIO_BALLOON_F_DEFLATE_ON_OOM could be reused
> instead of VIRTIO_BALLOON_F_FREE_PAGE_VQ ?
>
I think you misunderstood the work, which seems not easy to explain
everything from the beginning here. I wish to review patch 4 (I'll send
out a new independent version) with Michael if possible.
I'll discuss with you about patch 2 later. Thanks.
Best,
Wei
^ permalink raw reply
* Re: [PATCH] drm/virtio: Add window server support
From: Tomeu Vizoso @ 2018-01-12 7:59 UTC (permalink / raw)
To: Dave Airlie
Cc: Michael S. Tsirkin, David Airlie, LKML, dri-devel,
open list:VIRTIO CORE, NET...
In-Reply-To: <CAPM=9twT_jtfJteuUpRvu1Xn3nNfxG-5oLfcbHYVnHVeS9TzcA@mail.gmail.com>
On 01/12/2018 05:11 AM, Dave Airlie wrote:
>>
>> this work is based on the virtio_wl driver in the ChromeOS kernel by
>> Zach Reizner, currently at:
>>
>> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/virtio/virtio_wl.c
>>
>> There's two features missing in this patch when compared with virtio_wl:
>>
>> * Allow the guest access directly host memory, without having to resort
>> to TRANSFER_TO_HOST
>>
>> * Pass FDs from host to guest (Wayland specifies that the compositor
>> shares keyboard data with the guest via a shared buffer)
>>
>> I plan to work on this next, but I would like to get some comments on
>> the general approach so I can better choose which patch to follow.
>
> Shouldn't qemu expose some kind of capability to enable this so we know to
> look for the extra vqs?
Sounds good. I'm unsure though on whether it should be done
unconditionally if the hypervisor's code supports this, or if only if
the user pass the -proxy-wayland switch and the hypervisor was able to
open the socket to the compositor. I'm leaning towards the latter.
> What happens if you run this on plain qemu, does it fallback correctly?
Will work on this.
> Are there any scenarios where we don't want to expose this API because there
> is nothing to back it.
I'm not sure what the overhead of the extra queues is, but I guess the
ioctls could immediately return -ENODEV if the hypervisor doesn't have
that capability.
Happy to see that there aren't any major concerns with the general approach.
Thanks,
Tomeu
^ permalink raw reply
* Re: [PATCH 2/6] crypto: engine - Permit to enqueue all async requests
From: Herbert Xu @ 2018-01-12 7:14 UTC (permalink / raw)
To: Corentin Labbe
Cc: alexandre.torgue, corbet, mst, linux-doc, linux-kernel,
fabien.dessenne, virtualization, linux-crypto, mcoquelin.stm32,
davem, linux-arm-kernel
In-Reply-To: <20180103201109.16077-3-clabbe.montjoie@gmail.com>
On Wed, Jan 03, 2018 at 09:11:05PM +0100, Corentin Labbe wrote:
> The crypto engine could actually only enqueue hash and ablkcipher request.
> This patch permit it to enqueue any type of crypto_async_request.
>
> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
> ---
> crypto/crypto_engine.c | 230 ++++++++++++++++++++++++------------------------
> include/crypto/engine.h | 59 +++++++------
> 2 files changed, 148 insertions(+), 141 deletions(-)
>
> diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
> index 61e7c4e02fd2..036270b61648 100644
> --- a/crypto/crypto_engine.c
> +++ b/crypto/crypto_engine.c
> @@ -15,7 +15,6 @@
> #include <linux/err.h>
> #include <linux/delay.h>
> #include <crypto/engine.h>
> -#include <crypto/internal/hash.h>
> #include <uapi/linux/sched/types.h>
> #include "internal.h"
>
> @@ -34,11 +33,10 @@ static void crypto_pump_requests(struct crypto_engine *engine,
> bool in_kthread)
> {
> struct crypto_async_request *async_req, *backlog;
> - struct ahash_request *hreq;
> - struct ablkcipher_request *breq;
> unsigned long flags;
> bool was_busy = false;
> - int ret, rtype;
> + int ret;
> + struct crypto_engine_reqctx *enginectx;
This all looks very good. Just one minor nit, since you're storing
this in the tfm ctx as opposed to the request ctx (which is indeed
an improvement), you should remove the "req" from its name.
Thanks!
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [RFC PATCH 1/1] qemu: Introduce VIRTIO_NET_F_BACKUP feature bit to virtio_net
From: Sridhar Samudrala @ 2018-01-12 5:58 UTC (permalink / raw)
To: mst, stephen, davem, netdev, virtualization, virtio-dev,
jesse.brandeburg, alexander.h.duyck, kubakici, sridhar.samudrala
In-Reply-To: <1515736720-39368-1-git-send-email-sridhar.samudrala@intel.com>
This feature bit can be used by hypervisor to indicate virtio_net device to
act as a backup for another device with the same MAC address.
Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index cd63659140..fa47e723b9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2193,6 +2193,8 @@ static Property virtio_net_properties[] = {
true),
DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
+ DEFINE_PROP_BIT64("backup", VirtIONet, host_features,
+ VIRTIO_NET_F_BACKUP, false),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
index 17c8531a22..6afca6dcaf 100644
--- a/include/standard-headers/linux/virtio_net.h
+++ b/include/standard-headers/linux/virtio_net.h
@@ -57,6 +57,9 @@
* Steering */
#define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
+#define VIRTIO_NET_F_BACKUP 62 /* Act as backup for another device
+ * with the same MAC.
+ */
#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
#ifndef VIRTIO_NET_NO_LEGACY
^ permalink raw reply related
* [RFC PATCH net-next v2 2/2] virtio_net: Extend virtio to use VF datapath when available
From: Sridhar Samudrala @ 2018-01-12 5:58 UTC (permalink / raw)
To: mst, stephen, davem, netdev, virtualization, virtio-dev,
jesse.brandeburg, alexander.h.duyck, kubakici, sridhar.samudrala
In-Reply-To: <1515736720-39368-1-git-send-email-sridhar.samudrala@intel.com>
This patch enables virtio_net to switch over to a VF datapath when a VF
netdev is present with the same MAC address. The VF datapath is only used
for unicast traffic. Broadcasts/multicasts go via virtio datapath so that
east-west broadcasts don't use the PCI bandwidth. It allows live migration
of a VM with a direct attached VF without the need to setup a bond/team
between a VF and virtio net device in the guest.
The hypervisor needs to unplug the VF device from the guest on the source
host and reset the MAC filter of the VF to initiate failover of datapath to
virtio before starting the migration. After the migration is completed, the
destination hypervisor sets the MAC filter on the VF and plugs it back to
the guest to switch over to VF datapath.
This patch is based on the discussion initiated by Jesse on this thread.
https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
---
drivers/net/virtio_net.c | 307 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 305 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f149a160a8c5..0e58d364fde9 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -30,6 +30,8 @@
#include <linux/cpu.h>
#include <linux/average.h>
#include <linux/filter.h>
+#include <linux/netdevice.h>
+#include <linux/netpoll.h>
#include <net/route.h>
#include <net/xdp.h>
@@ -120,6 +122,15 @@ struct receive_queue {
struct xdp_rxq_info xdp_rxq;
};
+struct virtnet_vf_pcpu_stats {
+ u64 rx_packets;
+ u64 rx_bytes;
+ u64 tx_packets;
+ u64 tx_bytes;
+ struct u64_stats_sync syncp;
+ u32 tx_dropped;
+};
+
struct virtnet_info {
struct virtio_device *vdev;
struct virtqueue *cvq;
@@ -182,6 +193,10 @@ struct virtnet_info {
u32 speed;
unsigned long guest_offloads;
+
+ /* State to manage the associated VF interface. */
+ struct net_device __rcu *vf_netdev;
+ struct virtnet_vf_pcpu_stats __percpu *vf_stats;
};
struct padded_vnet_hdr {
@@ -1314,16 +1329,53 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
}
+/* Send skb on the slave VF device. */
+static int virtnet_vf_xmit(struct net_device *dev, struct net_device *vf_netdev,
+ struct sk_buff *skb)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+ unsigned int len = skb->len;
+ int rc;
+
+ skb->dev = vf_netdev;
+ skb->queue_mapping = qdisc_skb_cb(skb)->slave_dev_queue_mapping;
+
+ rc = dev_queue_xmit(skb);
+ if (likely(rc == NET_XMIT_SUCCESS || rc == NET_XMIT_CN)) {
+ struct virtnet_vf_pcpu_stats *pcpu_stats
+ = this_cpu_ptr(vi->vf_stats);
+
+ u64_stats_update_begin(&pcpu_stats->syncp);
+ pcpu_stats->tx_packets++;
+ pcpu_stats->tx_bytes += len;
+ u64_stats_update_end(&pcpu_stats->syncp);
+ } else {
+ this_cpu_inc(vi->vf_stats->tx_dropped);
+ }
+
+ return rc;
+}
+
static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
{
struct virtnet_info *vi = netdev_priv(dev);
int qnum = skb_get_queue_mapping(skb);
struct send_queue *sq = &vi->sq[qnum];
+ struct net_device *vf_netdev;
int err;
struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
bool kick = !skb->xmit_more;
bool use_napi = sq->napi.weight;
+ /* If VF is present and up then redirect packets
+ * called with rcu_read_lock_bh
+ */
+ vf_netdev = rcu_dereference_bh(vi->vf_netdev);
+ if (vf_netdev && netif_running(vf_netdev) &&
+ !netpoll_tx_running(dev) &&
+ is_unicast_ether_addr(eth_hdr(skb)->h_dest))
+ return virtnet_vf_xmit(dev, vf_netdev, skb);
+
/* Free up any pending old buffers before queueing new ones. */
free_old_xmit_skbs(sq);
@@ -1470,10 +1522,41 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
return ret;
}
+static void virtnet_get_vf_stats(struct net_device *dev,
+ struct virtnet_vf_pcpu_stats *tot)
+{
+ struct virtnet_info *vi = netdev_priv(dev);
+ int i;
+
+ memset(tot, 0, sizeof(*tot));
+
+ for_each_possible_cpu(i) {
+ const struct virtnet_vf_pcpu_stats *stats
+ = per_cpu_ptr(vi->vf_stats, i);
+ u64 rx_packets, rx_bytes, tx_packets, tx_bytes;
+ unsigned int start;
+
+ do {
+ start = u64_stats_fetch_begin_irq(&stats->syncp);
+ rx_packets = stats->rx_packets;
+ tx_packets = stats->tx_packets;
+ rx_bytes = stats->rx_bytes;
+ tx_bytes = stats->tx_bytes;
+ } while (u64_stats_fetch_retry_irq(&stats->syncp, start));
+
+ tot->rx_packets += rx_packets;
+ tot->tx_packets += tx_packets;
+ tot->rx_bytes += rx_bytes;
+ tot->tx_bytes += tx_bytes;
+ tot->tx_dropped += stats->tx_dropped;
+ }
+}
+
static void virtnet_stats(struct net_device *dev,
struct rtnl_link_stats64 *tot)
{
struct virtnet_info *vi = netdev_priv(dev);
+ struct virtnet_vf_pcpu_stats vf_stats;
int cpu;
unsigned int start;
@@ -1504,6 +1587,13 @@ static void virtnet_stats(struct net_device *dev,
tot->rx_dropped = dev->stats.rx_dropped;
tot->rx_length_errors = dev->stats.rx_length_errors;
tot->rx_frame_errors = dev->stats.rx_frame_errors;
+
+ virtnet_get_vf_stats(dev, &vf_stats);
+ tot->rx_packets += vf_stats.rx_packets;
+ tot->tx_packets += vf_stats.tx_packets;
+ tot->rx_bytes += vf_stats.rx_bytes;
+ tot->tx_bytes += vf_stats.tx_bytes;
+ tot->tx_dropped += vf_stats.tx_dropped;
}
#ifdef CONFIG_NET_POLL_CONTROLLER
@@ -2635,6 +2725,13 @@ static int virtnet_probe(struct virtio_device *vdev)
INIT_WORK(&vi->config_work, virtnet_config_changed_work);
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_BACKUP)) {
+ vi->vf_stats =
+ netdev_alloc_pcpu_stats(struct virtnet_vf_pcpu_stats);
+ if (!vi->vf_stats)
+ goto free_stats;
+ }
+
/* If we can receive ANY GSO packets, we must allocate large ones. */
if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6) ||
@@ -2668,7 +2765,7 @@ static int virtnet_probe(struct virtio_device *vdev)
*/
dev_err(&vdev->dev, "device MTU appears to have changed "
"it is now %d < %d", mtu, dev->min_mtu);
- goto free_stats;
+ goto free_vf_stats;
}
dev->mtu = mtu;
@@ -2692,7 +2789,7 @@ static int virtnet_probe(struct virtio_device *vdev)
/* Allocate/initialize the rx/tx queues, and invoke find_vqs */
err = init_vqs(vi);
if (err)
- goto free_stats;
+ goto free_vf_stats;
#ifdef CONFIG_SYSFS
if (vi->mergeable_rx_bufs)
@@ -2747,6 +2844,8 @@ static int virtnet_probe(struct virtio_device *vdev)
cancel_delayed_work_sync(&vi->refill);
free_receive_page_frags(vi);
virtnet_del_vqs(vi);
+free_vf_stats:
+ free_percpu(vi->vf_stats);
free_stats:
free_percpu(vi->stats);
free:
@@ -2768,19 +2867,184 @@ static void remove_vq_common(struct virtnet_info *vi)
virtnet_del_vqs(vi);
}
+static struct net_device *get_virtio_bymac(const u8 *mac)
+{
+ struct net_device *dev;
+
+ ASSERT_RTNL();
+
+ for_each_netdev(&init_net, dev) {
+ if (dev->netdev_ops != &virtnet_netdev)
+ continue; /* not a virtio_net device */
+
+ if (ether_addr_equal(mac, dev->perm_addr))
+ return dev;
+ }
+
+ return NULL;
+}
+
+static struct net_device *get_virtio_byref(struct net_device *vf_netdev)
+{
+ struct net_device *dev;
+
+ ASSERT_RTNL();
+
+ for_each_netdev(&init_net, dev) {
+ struct virtnet_info *vi;
+
+ if (dev->netdev_ops != &virtnet_netdev)
+ continue; /* not a virtio_net device */
+
+ vi = netdev_priv(dev);
+ if (rtnl_dereference(vi->vf_netdev) == vf_netdev)
+ return dev; /* a match */
+ }
+
+ return NULL;
+}
+
+/* Called when VF is injecting data into network stack.
+ * Change the associated network device from VF to virtio.
+ * note: already called with rcu_read_lock
+ */
+static rx_handler_result_t virtnet_vf_handle_frame(struct sk_buff **pskb)
+{
+ struct sk_buff *skb = *pskb;
+ struct net_device *ndev = rcu_dereference(skb->dev->rx_handler_data);
+ struct virtnet_info *vi = netdev_priv(ndev);
+ struct virtnet_vf_pcpu_stats *pcpu_stats =
+ this_cpu_ptr(vi->vf_stats);
+
+ skb->dev = ndev;
+
+ u64_stats_update_begin(&pcpu_stats->syncp);
+ pcpu_stats->rx_packets++;
+ pcpu_stats->rx_bytes += skb->len;
+ u64_stats_update_end(&pcpu_stats->syncp);
+
+ return RX_HANDLER_ANOTHER;
+}
+
+static int virtnet_vf_join(struct net_device *vf_netdev,
+ struct net_device *ndev)
+{
+ int ret;
+
+ ret = netdev_rx_handler_register(vf_netdev,
+ virtnet_vf_handle_frame, ndev);
+ if (ret != 0) {
+ netdev_err(vf_netdev,
+ "can not register virtio VF receive handler (err = %d)\n",
+ ret);
+ goto rx_handler_failed;
+ }
+
+ ret = netdev_upper_dev_link(vf_netdev, ndev, NULL);
+ if (ret != 0) {
+ netdev_err(vf_netdev,
+ "can not set master device %s (err = %d)\n",
+ ndev->name, ret);
+ goto upper_link_failed;
+ }
+
+ vf_netdev->flags |= IFF_SLAVE;
+
+ /* Align MTU of VF with master */
+ ret = dev_set_mtu(vf_netdev, ndev->mtu);
+ if (ret)
+ netdev_warn(vf_netdev,
+ "unable to change mtu to %u\n", ndev->mtu);
+
+ call_netdevice_notifiers(NETDEV_JOIN, vf_netdev);
+
+ netdev_info(vf_netdev, "joined to %s\n", ndev->name);
+ return 0;
+
+upper_link_failed:
+ netdev_rx_handler_unregister(vf_netdev);
+rx_handler_failed:
+ return ret;
+}
+
+static int virtnet_register_vf(struct net_device *vf_netdev)
+{
+ struct net_device *ndev;
+ struct virtnet_info *vi;
+
+ if (vf_netdev->addr_len != ETH_ALEN)
+ return NOTIFY_DONE;
+
+ /* We will use the MAC address to locate the virtio_net interface to
+ * associate with the VF interface. If we don't find a matching
+ * virtio interface, move on.
+ */
+ ndev = get_virtio_bymac(vf_netdev->perm_addr);
+ if (!ndev)
+ return NOTIFY_DONE;
+
+ vi = netdev_priv(ndev);
+ if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_BACKUP))
+ return NOTIFY_DONE;
+
+ if (rtnl_dereference(vi->vf_netdev))
+ return NOTIFY_DONE;
+
+ if (virtnet_vf_join(vf_netdev, ndev) != 0)
+ return NOTIFY_DONE;
+
+ netdev_info(ndev, "VF registering %s\n", vf_netdev->name);
+
+ dev_hold(vf_netdev);
+ rcu_assign_pointer(vi->vf_netdev, vf_netdev);
+
+ return NOTIFY_OK;
+}
+
+static int virtnet_unregister_vf(struct net_device *vf_netdev)
+{
+ struct net_device *ndev;
+ struct virtnet_info *vi;
+
+ ndev = get_virtio_byref(vf_netdev);
+ if (!ndev)
+ return NOTIFY_DONE;
+
+ vi = netdev_priv(ndev);
+ if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_BACKUP))
+ return NOTIFY_DONE;
+
+ netdev_info(ndev, "VF unregistering %s\n", vf_netdev->name);
+
+ netdev_rx_handler_unregister(vf_netdev);
+ netdev_upper_dev_unlink(vf_netdev, ndev);
+ RCU_INIT_POINTER(vi->vf_netdev, NULL);
+ dev_put(vf_netdev);
+
+ return NOTIFY_OK;
+}
+
static void virtnet_remove(struct virtio_device *vdev)
{
struct virtnet_info *vi = vdev->priv;
+ struct net_device *vf_netdev;
virtnet_cpu_notif_remove(vi);
/* Make sure no work handler is accessing the device. */
flush_work(&vi->config_work);
+ rtnl_lock();
+ vf_netdev = rtnl_dereference(vi->vf_netdev);
+ if (vf_netdev)
+ virtnet_unregister_vf(vf_netdev);
+ rtnl_unlock();
+
unregister_netdev(vi->dev);
remove_vq_common(vi);
+ free_percpu(vi->vf_stats);
free_percpu(vi->stats);
free_netdev(vi->dev);
}
@@ -2859,6 +3123,42 @@ static struct virtio_driver virtio_net_driver = {
#endif
};
+static int virtio_netdev_event(struct notifier_block *this,
+ unsigned long event, void *ptr)
+{
+ struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
+
+ /* Skip our own events */
+ if (event_dev->netdev_ops == &virtnet_netdev)
+ return NOTIFY_DONE;
+
+ /* Avoid non-Ethernet type devices */
+ if (event_dev->type != ARPHRD_ETHER)
+ return NOTIFY_DONE;
+
+ /* Avoid Vlan dev with same MAC registering as VF */
+ if (is_vlan_dev(event_dev))
+ return NOTIFY_DONE;
+
+ /* Avoid Bonding master dev with same MAC registering as VF */
+ if ((event_dev->priv_flags & IFF_BONDING) &&
+ (event_dev->flags & IFF_MASTER))
+ return NOTIFY_DONE;
+
+ switch (event) {
+ case NETDEV_REGISTER:
+ return virtnet_register_vf(event_dev);
+ case NETDEV_UNREGISTER:
+ return virtnet_unregister_vf(event_dev);
+ default:
+ return NOTIFY_DONE;
+ }
+}
+
+static struct notifier_block virtio_netdev_notifier = {
+ .notifier_call = virtio_netdev_event,
+};
+
static __init int virtio_net_driver_init(void)
{
int ret;
@@ -2877,6 +3177,8 @@ static __init int virtio_net_driver_init(void)
ret = register_virtio_driver(&virtio_net_driver);
if (ret)
goto err_virtio;
+
+ register_netdevice_notifier(&virtio_netdev_notifier);
return 0;
err_virtio:
cpuhp_remove_multi_state(CPUHP_VIRT_NET_DEAD);
@@ -2889,6 +3191,7 @@ module_init(virtio_net_driver_init);
static __exit void virtio_net_driver_exit(void)
{
+ unregister_netdevice_notifier(&virtio_netdev_notifier);
unregister_virtio_driver(&virtio_net_driver);
cpuhp_remove_multi_state(CPUHP_VIRT_NET_DEAD);
cpuhp_remove_multi_state(virtionet_online);
--
2.14.3
^ permalink raw reply related
* [RFC PATCH net-next v2 1/2] virtio_net: Introduce VIRTIO_NET_F_BACKUP feature bit
From: Sridhar Samudrala @ 2018-01-12 5:58 UTC (permalink / raw)
To: mst, stephen, davem, netdev, virtualization, virtio-dev,
jesse.brandeburg, alexander.h.duyck, kubakici, sridhar.samudrala
In-Reply-To: <1515736720-39368-1-git-send-email-sridhar.samudrala@intel.com>
This feature bit can be used by hypervisor to indicate virtio_net device to
act as a backup for another device with the same MAC address.
Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
---
drivers/net/virtio_net.c | 2 +-
include/uapi/linux/virtio_net.h | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 12dfc5fee58e..f149a160a8c5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2829,7 +2829,7 @@ static struct virtio_device_id id_table[] = {
VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
VIRTIO_NET_F_CTRL_MAC_ADDR, \
VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
- VIRTIO_NET_F_SPEED_DUPLEX
+ VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_BACKUP
static unsigned int features[] = {
VIRTNET_FEATURES,
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 5de6ed37695b..c7c35fd1a5ed 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -57,6 +57,9 @@
* Steering */
#define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */
+#define VIRTIO_NET_F_BACKUP 62 /* Act as backup for another device
+ * with the same MAC.
+ */
#define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */
#ifndef VIRTIO_NET_NO_LEGACY
--
2.14.3
^ permalink raw reply related
* [RFC PATCH net-next v2 0/2] Enable virtio to act as a backup for a passthru device
From: Sridhar Samudrala @ 2018-01-12 5:58 UTC (permalink / raw)
To: mst, stephen, davem, netdev, virtualization, virtio-dev,
jesse.brandeburg, alexander.h.duyck, kubakici, sridhar.samudrala
This patch series extends virtio_net to take over VF datapath by
simulating a transparent bond without creating any additional netdev.
I understand that there are some comments suggesting an alternate model
that is based on 3 driver model(virtio_net, VF driver, a new driver
virt_bond that acts as a master to virtio_net and VF).
Would like to get some feedback on the right way to solve the live
migration problem with direct attached devices in KVM environment.
Stephen,
Is the netvsc transparent bond implemenation robust enough and deployed
in real environments? Or would netvsc switch over to a 3-driver model if
that solution becomes available?
Can we start with this implementation that is similar to netvsc and if
needed we can move to the 3 driver model later?
This patch series enables virtio to switch over to a VF datapath when a
VF netdev is present with the same MAC address. It allows live migration
of a VM with a direct attached VF without the need to setup a bond/team
between a VF and virtio net device in the guest.
The hypervisor needs to unplug the VF device from the guest on the source
host and reset the MAC filter of the VF to initiate failover of datapath
to virtio before starting the migration. After the migration is completed,
the destination hypervisor sets the MAC filter on the VF and plugs it back
to the guest to switch over to VF datapath.
It is based on netvsc implementation and it should be possible to make this
code generic and move it to a common location that can be shared by netvsc
and virtio.
This patch series is based on the discussion initiated by Jesse on this thread.
https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
v2:
- Changed VIRTIO_NET_F_MASTER to VIRTIO_NET_F_BACKUP (mst)
- made a small change to the virtio-net xmit path to only use VF datapath
for unicasts. Broadcasts/multicasts use virtio datapath. This avoids
east-west broadcasts to go over the PCI link.
- added suppport for the feature bit in qemu
Sridhar Samudrala (2):
virtio_net: Introduce VIRTIO_NET_F_BACKUP feature bit
virtio_net: Extend virtio to use VF datapath when available
drivers/net/virtio_net.c | 309 +++++++++++++++++++++++++++++++++++++++-
include/uapi/linux/virtio_net.h | 3 +
2 files changed, 309 insertions(+), 3 deletions(-)
Sridhar Samudrala (1):
qemu: Introduce VIRTIO_NET_F_BACKUP feature bit to virtio_net
hw/net/virtio-net.c | 2 ++
include/standard-headers/linux/virtio_net.h | 3 +++
2 files changed, 5 insertions(+)
--
2.14.3
^ permalink raw reply
* Re: [PATCH] drm/virtio: Add window server support
From: Dave Airlie @ 2018-01-12 4:11 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: Michael S. Tsirkin, David Airlie, LKML, dri-devel,
open list:VIRTIO CORE, NET...
In-Reply-To: <20171228115333.12289-1-tomeu.vizoso@collabora.com>
>
> this work is based on the virtio_wl driver in the ChromeOS kernel by
> Zach Reizner, currently at:
>
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/virtio/virtio_wl.c
>
> There's two features missing in this patch when compared with virtio_wl:
>
> * Allow the guest access directly host memory, without having to resort
> to TRANSFER_TO_HOST
>
> * Pass FDs from host to guest (Wayland specifies that the compositor
> shares keyboard data with the guest via a shared buffer)
>
> I plan to work on this next, but I would like to get some comments on
> the general approach so I can better choose which patch to follow.
Shouldn't qemu expose some kind of capability to enable this so we know to
look for the extra vqs?
What happens if you run this on plain qemu, does it fallback correctly?
Are there any scenarios where we don't want to expose this API because there
is nothing to back it.
Dave.
^ permalink raw reply
* Re: [PATCH 6/6] crypto: stm32-cryp: convert to the new crypto engine API
From: Fabien DESSENNE @ 2018-01-11 7:45 UTC (permalink / raw)
To: Corentin Labbe, Alexandre TORGUE, arei.gonglei@huawei.com,
corbet@lwn.net, davem@davemloft.net, herbert@gondor.apana.org.au,
jasowang@redhat.com, mcoquelin.stm32@gmail.com, mst@redhat.com
Cc: Benjamin GAIGNARD, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org,
linux-crypto@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <ccffd847-3df7-d690-4102-5a10f6439f87@st.com>
(Adding my tested by)
On 10/01/18 15:25, Fabien DESSENNE wrote:
>
> On 03/01/18 21:11, Corentin Labbe wrote:
>> This patch convert the stm32-cryp driver to the new crypto engine API.
>> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
Tested-by: Fabien Dessenne <fabien.dessenne@st.com>
>> ---
>> drivers/crypto/stm32/stm32-cryp.c | 21 ++++++++++++++++-----
>> 1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/crypto/stm32/stm32-cryp.c b/drivers/crypto/stm32/stm32-cryp.c
>> index cf1dddbeaa2c..99e0473ef247 100644
>> --- a/drivers/crypto/stm32/stm32-cryp.c
>> +++ b/drivers/crypto/stm32/stm32-cryp.c
>> @@ -91,6 +91,7 @@
>> #define _walked_out (cryp->out_walk.offset - cryp->out_sg->offset)
>>
>> struct stm32_cryp_ctx {
>> + struct crypto_engine_reqctx enginectx;
>> struct stm32_cryp *cryp;
>> int keylen;
>> u32 key[AES_KEYSIZE_256 / sizeof(u32)];
>> @@ -494,10 +495,20 @@ static int stm32_cryp_cpu_start(struct stm32_cryp *cryp)
>> return 0;
>> }
>>
>> +static int stm32_cryp_cipher_one_req(struct crypto_engine *engine,
>> + void *areq);
> Merge these 2 lines in a single one
>
>> +static int stm32_cryp_prepare_cipher_req(struct crypto_engine *engine,
>> + void *areq);
>> +
>> static int stm32_cryp_cra_init(struct crypto_tfm *tfm)
>> {
>> + struct stm32_cryp_ctx *ctx = crypto_tfm_ctx(tfm);
>> +
>> tfm->crt_ablkcipher.reqsize = sizeof(struct stm32_cryp_reqctx);
>>
>> + ctx->enginectx.op.do_one_request = stm32_cryp_cipher_one_req;
>> + ctx->enginectx.op.prepare_request = stm32_cryp_prepare_cipher_req;
>> + ctx->enginectx.op.unprepare_request = NULL;
>> return 0;
>> }
>>
>> @@ -695,14 +706,17 @@ static int stm32_cryp_prepare_req(struct crypto_engine *engine,
>> }
>>
>> static int stm32_cryp_prepare_cipher_req(struct crypto_engine *engine,
>> - struct ablkcipher_request *req)
>> + void *areq)
>> {
>> + struct ablkcipher_request *req = container_of(areq, struct ablkcipher_request, base);
> > 80 characters (CHECKPATCH)
>
>> +
>> return stm32_cryp_prepare_req(engine, req);
>> }
>>
>> static int stm32_cryp_cipher_one_req(struct crypto_engine *engine,
>> - struct ablkcipher_request *req)
>> + void *areq)
> Merge these 2 lines in a single one
>
>> {
>> + struct ablkcipher_request *req = container_of(areq, struct ablkcipher_request, base);
> > 80 characters (CHECKPATCH)
>
>> struct stm32_cryp_ctx *ctx = crypto_ablkcipher_ctx(
>> crypto_ablkcipher_reqtfm(req));
>> struct stm32_cryp *cryp = ctx->cryp;
>> @@ -1104,9 +1118,6 @@ static int stm32_cryp_probe(struct platform_device *pdev)
>> goto err_engine1;
>> }
>>
>> - cryp->engine->prepare_cipher_request = stm32_cryp_prepare_cipher_req;
>> - cryp->engine->cipher_one_request = stm32_cryp_cipher_one_req;
>> -
>> ret = crypto_engine_start(cryp->engine);
>> if (ret) {
>> dev_err(dev, "Could not start crypto engine\n");
^ permalink raw reply
* Re: [PATCH 5/6] crypto: stm32-hash: convert to the new crypto engine API
From: Fabien DESSENNE @ 2018-01-11 7:44 UTC (permalink / raw)
To: Corentin Labbe, Alexandre TORGUE, arei.gonglei@huawei.com,
corbet@lwn.net, davem@davemloft.net, herbert@gondor.apana.org.au,
jasowang@redhat.com, mcoquelin.stm32@gmail.com, mst@redhat.com
Cc: Lionel DEBIEVE, Benjamin GAIGNARD, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org,
linux-crypto@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <17d0497d-5c13-f93e-4249-8adff5e71b19@st.com>
(adding my tested my)
On 10/01/18 15:24, Fabien DESSENNE wrote:
>
> On 03/01/18 21:11, Corentin Labbe wrote:
>> This patch convert the stm32-hash driver to the new crypto engine API.
>>
>> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
Tested-by: Fabien Dessenne <fabien.dessenne@st.com>
>> ---
>> drivers/crypto/stm32/stm32-hash.c | 18 +++++++++++++-----
>> 1 file changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
>> index 4ca4a264a833..9790c2c936c7 100644
>> --- a/drivers/crypto/stm32/stm32-hash.c
>> +++ b/drivers/crypto/stm32/stm32-hash.c
>> @@ -122,6 +122,7 @@ enum stm32_hash_data_format {
>> #define HASH_DMA_THRESHOLD 50
>>
>> struct stm32_hash_ctx {
>> + struct crypto_engine_reqctx enginectx;
>> struct stm32_hash_dev *hdev;
>> unsigned long flags;
>>
>> @@ -828,6 +829,11 @@ static int stm32_hash_hw_init(struct stm32_hash_dev *hdev,
>> return 0;
>> }
>>
>> +static int stm32_hash_one_request(struct crypto_engine *engine,
>> + void *areq);
> merge these two lines in a single one
>
>> +static int stm32_hash_prepare_req(struct crypto_engine *engine,
>> + void *areq);
> merge these two lines in a single one
>
>> +
>> static int stm32_hash_handle_queue(struct stm32_hash_dev *hdev,
>> struct ahash_request *req)
>> {
>> @@ -835,8 +841,9 @@ static int stm32_hash_handle_queue(struct stm32_hash_dev *hdev,
>> }
>>
>> static int stm32_hash_prepare_req(struct crypto_engine *engine,
>> - struct ahash_request *req)
>> + void *areq)
> merge these two lines in a single one
>
>> {
>> + struct ahash_request *req = container_of(areq, struct ahash_request, base);
> > 80 characters (CHECKPATCH)
>
>> struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
>> struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
>> struct stm32_hash_request_ctx *rctx;
>> @@ -855,8 +862,9 @@ static int stm32_hash_prepare_req(struct crypto_engine *engine,
>> }
>>
>> static int stm32_hash_one_request(struct crypto_engine *engine,
>> - struct ahash_request *req)
>> + void *areq)
> merge these two lines in a single one
>
>> {
>> + struct ahash_request *req = container_of(areq, struct ahash_request, base);
> > 80 characters (CHECKPATCH)
>
>> struct stm32_hash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(req));
>> struct stm32_hash_dev *hdev = stm32_hash_find_dev(ctx);
>> struct stm32_hash_request_ctx *rctx;
>> @@ -1033,6 +1041,9 @@ static int stm32_hash_cra_init_algs(struct crypto_tfm *tfm,
>> if (algs_hmac_name)
>> ctx->flags |= HASH_FLAGS_HMAC;
>>
>> + ctx->enginectx.op.do_one_request = stm32_hash_one_request;
>> + ctx->enginectx.op.prepare_request = stm32_hash_prepare_req;
>> + ctx->enginectx.op.unprepare_request = NULL;
>> return 0;
>> }
>>
>> @@ -1493,9 +1504,6 @@ static int stm32_hash_probe(struct platform_device *pdev)
>> goto err_engine;
>> }
>>
>> - hdev->engine->prepare_hash_request = stm32_hash_prepare_req;
>> - hdev->engine->hash_one_request = stm32_hash_one_request;
>> -
>> ret = crypto_engine_start(hdev->engine);
>> if (ret)
>> goto err_engine_start;
^ permalink raw reply
* Re: [PATCH 2/6] crypto: engine - Permit to enqueue all async requests
From: Fabien DESSENNE @ 2018-01-11 7:44 UTC (permalink / raw)
To: Corentin Labbe, Alexandre TORGUE, arei.gonglei@huawei.com,
corbet@lwn.net, davem@davemloft.net, herbert@gondor.apana.org.au,
jasowang@redhat.com, mcoquelin.stm32@gmail.com, mst@redhat.com
Cc: Lionel DEBIEVE, Benjamin GAIGNARD, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org,
virtualization@lists.linux-foundation.org,
linux-crypto@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <83fa00f2-6e93-d83f-fd9a-0048bd6d60ec@st.com>
(adding my tested by)
On 10/01/18 15:19, Fabien DESSENNE wrote:
> On 03/01/18 21:11, Corentin Labbe wrote:
>> The crypto engine could actually only enqueue hash and ablkcipher request.
>> This patch permit it to enqueue any type of crypto_async_request.
>>
>> Signed-off-by: Corentin Labbe <clabbe.montjoie@gmail.com>
Tested-by: Fabien Dessenne <fabien.dessenne@st.com>
>> ---
>> crypto/crypto_engine.c | 230 ++++++++++++++++++++++++------------------------
>> include/crypto/engine.h | 59 +++++++------
>> 2 files changed, 148 insertions(+), 141 deletions(-)
>>
>> diff --git a/crypto/crypto_engine.c b/crypto/crypto_engine.c
>> index 61e7c4e02fd2..036270b61648 100644
>> --- a/crypto/crypto_engine.c
>> +++ b/crypto/crypto_engine.c
>> @@ -15,7 +15,6 @@
>> #include <linux/err.h>
>> #include <linux/delay.h>
>> #include <crypto/engine.h>
>> -#include <crypto/internal/hash.h>
>> #include <uapi/linux/sched/types.h>
>> #include "internal.h"
>>
>> @@ -34,11 +33,10 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>> bool in_kthread)
>> {
>> struct crypto_async_request *async_req, *backlog;
>> - struct ahash_request *hreq;
>> - struct ablkcipher_request *breq;
>> unsigned long flags;
>> bool was_busy = false;
>> - int ret, rtype;
>> + int ret;
>> + struct crypto_engine_reqctx *enginectx;
>>
>> spin_lock_irqsave(&engine->queue_lock, flags);
>>
>> @@ -94,7 +92,6 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>>
>> spin_unlock_irqrestore(&engine->queue_lock, flags);
>>
>> - rtype = crypto_tfm_alg_type(engine->cur_req->tfm);
>> /* Until here we get the request need to be encrypted successfully */
>> if (!was_busy && engine->prepare_crypt_hardware) {
>> ret = engine->prepare_crypt_hardware(engine);
>> @@ -104,57 +101,31 @@ static void crypto_pump_requests(struct crypto_engine *engine,
>> }
>> }
>>
>> - switch (rtype) {
>> - case CRYPTO_ALG_TYPE_AHASH:
>> - hreq = ahash_request_cast(engine->cur_req);
>> - if (engine->prepare_hash_request) {
>> - ret = engine->prepare_hash_request(engine, hreq);
>> - if (ret) {
>> - dev_err(engine->dev, "failed to prepare request: %d\n",
>> - ret);
>> - goto req_err;
>> - }
>> - engine->cur_req_prepared = true;
>> - }
>> - ret = engine->hash_one_request(engine, hreq);
>> - if (ret) {
>> - dev_err(engine->dev, "failed to hash one request from queue\n");
>> - goto req_err;
>> - }
>> - return;
>> - case CRYPTO_ALG_TYPE_ABLKCIPHER:
>> - breq = ablkcipher_request_cast(engine->cur_req);
>> - if (engine->prepare_cipher_request) {
>> - ret = engine->prepare_cipher_request(engine, breq);
>> - if (ret) {
>> - dev_err(engine->dev, "failed to prepare request: %d\n",
>> - ret);
>> - goto req_err;
>> - }
>> - engine->cur_req_prepared = true;
>> - }
>> - ret = engine->cipher_one_request(engine, breq);
>> + enginectx = crypto_tfm_ctx(async_req->tfm);
>> +
>> + if (enginectx->op.prepare_request) {
>> + ret = enginectx->op.prepare_request(engine, async_req);
>> if (ret) {
>> - dev_err(engine->dev, "failed to cipher one request from queue\n");
>> + dev_err(engine->dev, "failed to prepare request: %d\n",
>> + ret);
>> goto req_err;
>> }
>> - return;
>> - default:
>> - dev_err(engine->dev, "failed to prepare request of unknown type\n");
>> - return;
>> + engine->cur_req_prepared = true;
>> + }
>> + if (!enginectx->op.do_one_request) {
>> + dev_err(engine->dev, "failed to do request\n");
>> + ret = -EINVAL;
>> + goto req_err;
>> + }
>> + ret = enginectx->op.do_one_request(engine, async_req);
>> + if (ret) {
>> + dev_err(engine->dev, "Failed to do one request from queue: %d\n", ret);
>> + goto req_err;
>> }
>> + return;
>>
>> req_err:
>> - switch (rtype) {
>> - case CRYPTO_ALG_TYPE_AHASH:
>> - hreq = ahash_request_cast(engine->cur_req);
>> - crypto_finalize_hash_request(engine, hreq, ret);
>> - break;
>> - case CRYPTO_ALG_TYPE_ABLKCIPHER:
>> - breq = ablkcipher_request_cast(engine->cur_req);
>> - crypto_finalize_cipher_request(engine, breq, ret);
>> - break;
>> - }
>> + crypto_finalize_request(engine, async_req, ret);
>> return;
>>
>> out:
>> @@ -170,13 +141,12 @@ static void crypto_pump_work(struct kthread_work *work)
>> }
>>
>> /**
>> - * crypto_transfer_cipher_request - transfer the new request into the
>> - * enginequeue
>> + * crypto_transfer_request - transfer the new request into the engine queue
>> * @engine: the hardware engine
>> * @req: the request need to be listed into the engine queue
>> */
>> -int crypto_transfer_cipher_request(struct crypto_engine *engine,
>> - struct ablkcipher_request *req,
>> +static int crypto_transfer_request(struct crypto_engine *engine,
>> + struct crypto_async_request *req,
>> bool need_pump)
>> {
>> unsigned long flags;
>> @@ -189,7 +159,7 @@ int crypto_transfer_cipher_request(struct crypto_engine *engine,
>> return -ESHUTDOWN;
>> }
>>
>> - ret = ablkcipher_enqueue_request(&engine->queue, req);
>> + ret = crypto_enqueue_request(&engine->queue, req);
>>
>> if (!engine->busy && need_pump)
>> kthread_queue_work(engine->kworker, &engine->pump_requests);
>> @@ -197,85 +167,97 @@ int crypto_transfer_cipher_request(struct crypto_engine *engine,
>> spin_unlock_irqrestore(&engine->queue_lock, flags);
>> return ret;
>> }
>> -EXPORT_SYMBOL_GPL(crypto_transfer_cipher_request);
>> +EXPORT_SYMBOL_GPL(crypto_transfer_request);
> Do not export this function which is a static one.
>
>>
>> /**
>> - * crypto_transfer_cipher_request_to_engine - transfer one request to list
>> + * crypto_transfer_request_to_engine - transfer one request to list
>> * into the engine queue
>> * @engine: the hardware engine
>> * @req: the request need to be listed into the engine queue
>> */
>> +static int crypto_transfer_request_to_engine(struct crypto_engine *engine,
>> + struct crypto_async_request *req)
>> +{
>> + return crypto_transfer_request(engine, req, true);
>> +}
>> +
>> +/**
>> + * crypto_transfer_cipher_request_to_engine - transfer one ablkcipher_request
>> + * to list into the engine queue
>> + * @engine: the hardware engine
>> + * @req: the request need to be listed into the engine queue
>> + * TODO: Remove this function when skcipher conversion is finished
>> + */
>> int crypto_transfer_cipher_request_to_engine(struct crypto_engine *engine,
>> struct ablkcipher_request *req)
>> {
>> - return crypto_transfer_cipher_request(engine, req, true);
>> + return crypto_transfer_request_to_engine(engine, &req->base);
>> }
>> EXPORT_SYMBOL_GPL(crypto_transfer_cipher_request_to_engine);
>>
>> /**
>> - * crypto_transfer_hash_request - transfer the new request into the
>> - * enginequeue
>> + * crypto_transfer_skcipher_request_to_engine - transfer one skcipher_request
>> + * to list into the engine queue
>> * @engine: the hardware engine
>> * @req: the request need to be listed into the engine queue
>> */
>> -int crypto_transfer_hash_request(struct crypto_engine *engine,
>> - struct ahash_request *req, bool need_pump)
>> +int crypto_transfer_skcipher_request_to_engine(struct crypto_engine *engine,
>> + struct skcipher_request *req)
>> {
>> - unsigned long flags;
>> - int ret;
>> -
>> - spin_lock_irqsave(&engine->queue_lock, flags);
>> -
>> - if (!engine->running) {
>> - spin_unlock_irqrestore(&engine->queue_lock, flags);
>> - return -ESHUTDOWN;
>> - }
>> -
>> - ret = ahash_enqueue_request(&engine->queue, req);
>> -
>> - if (!engine->busy && need_pump)
>> - kthread_queue_work(engine->kworker, &engine->pump_requests);
>> + return crypto_transfer_request_to_engine(engine, &req->base);
>> +}
>> +EXPORT_SYMBOL_GPL(crypto_transfer_skcipher_request_to_engine);
>>
>> - spin_unlock_irqrestore(&engine->queue_lock, flags);
>> - return ret;
>> +/**
>> + * crypto_transfer_akcipher_request_to_engine - transfer one akcipher_request
>> + * to list into the engine queue
>> + * @engine: the hardware engine
>> + * @req: the request need to be listed into the engine queue
>> + */
>> +int crypto_transfer_akcipher_request_to_engine(struct crypto_engine *engine,
>> + struct akcipher_request *req)
>> +{
>> + return crypto_transfer_request_to_engine(engine, &req->base);
>> }
>> -EXPORT_SYMBOL_GPL(crypto_transfer_hash_request);
>> +EXPORT_SYMBOL_GPL(crypto_transfer_akcipher_request_to_engine);
>>
>> /**
>> - * crypto_transfer_hash_request_to_engine - transfer one request to list
>> - * into the engine queue
>> + * crypto_transfer_hash_request_to_engine - transfer one ahash_request
>> + * to list into the engine queue
>> * @engine: the hardware engine
>> * @req: the request need to be listed into the engine queue
>> */
>> int crypto_transfer_hash_request_to_engine(struct crypto_engine *engine,
>> struct ahash_request *req)
>> {
>> - return crypto_transfer_hash_request(engine, req, true);
>> + return crypto_transfer_request_to_engine(engine, &req->base);
>> }
>> EXPORT_SYMBOL_GPL(crypto_transfer_hash_request_to_engine);
>>
> Please add this EXPORTed function:
>
> crypto_transfer_aead_request_to_engine(struct crypto_engine *engine,
> struct aead_request *req)
>
>> /**
>> - * crypto_finalize_cipher_request - finalize one request if the request is done
>> + * crypto_finalize_request - finalize one request if the request is done
>> * @engine: the hardware engine
>> * @req: the request need to be finalized
>> * @err: error number
>> */
>> -void crypto_finalize_cipher_request(struct crypto_engine *engine,
>> - struct ablkcipher_request *req, int err)
>> +void crypto_finalize_request(struct crypto_engine *engine,
> shall be static
>
>> + struct crypto_async_request *req, int err)
>> {
>> unsigned long flags;
>> bool finalize_cur_req = false;
>> int ret;
>> + struct crypto_engine_reqctx *enginectx;
>>
>> spin_lock_irqsave(&engine->queue_lock, flags);
>> - if (engine->cur_req == &req->base)
>> + if (engine->cur_req == req)
>> finalize_cur_req = true;
>> spin_unlock_irqrestore(&engine->queue_lock, flags);
>>
>> if (finalize_cur_req) {
>> + enginectx = crypto_tfm_ctx(req->tfm);
>> if (engine->cur_req_prepared &&
>> - engine->unprepare_cipher_request) {
>> - ret = engine->unprepare_cipher_request(engine, req);
>> + enginectx->op.unprepare_request) {
>> + ret = enginectx->op.unprepare_request(engine, req);
>> if (ret)
>> dev_err(engine->dev, "failed to unprepare request\n");
>> }
>> @@ -285,46 +267,64 @@ void crypto_finalize_cipher_request(struct crypto_engine *engine,
>> spin_unlock_irqrestore(&engine->queue_lock, flags);
>> }
>>
>> - req->base.complete(&req->base, err);
>> + req->complete(req, err);
>>
>> kthread_queue_work(engine->kworker, &engine->pump_requests);
>> }
>> -EXPORT_SYMBOL_GPL(crypto_finalize_cipher_request);
>>
>> /**
>> - * crypto_finalize_hash_request - finalize one request if the request is done
>> + * crypto_finalize_cipher_request - finalize one ablkcipher_request if
>> + * the request is done
>> * @engine: the hardware engine
>> * @req: the request need to be finalized
>> * @err: error number
>> */
>> -void crypto_finalize_hash_request(struct crypto_engine *engine,
>> - struct ahash_request *req, int err)
>> +void crypto_finalize_cipher_request(struct crypto_engine *engine,
>> + struct ablkcipher_request *req, int err)
>> {
>> - unsigned long flags;
>> - bool finalize_cur_req = false;
>> - int ret;
>> -
>> - spin_lock_irqsave(&engine->queue_lock, flags);
>> - if (engine->cur_req == &req->base)
>> - finalize_cur_req = true;
>> - spin_unlock_irqrestore(&engine->queue_lock, flags);
>> + return crypto_finalize_request(engine, &req->base, err);
>> +}
>> +EXPORT_SYMBOL_GPL(crypto_finalize_cipher_request);
>>
>> - if (finalize_cur_req) {
>> - if (engine->cur_req_prepared &&
>> - engine->unprepare_hash_request) {
>> - ret = engine->unprepare_hash_request(engine, req);
>> - if (ret)
>> - dev_err(engine->dev, "failed to unprepare request\n");
>> - }
>> - spin_lock_irqsave(&engine->queue_lock, flags);
>> - engine->cur_req = NULL;
>> - engine->cur_req_prepared = false;
>> - spin_unlock_irqrestore(&engine->queue_lock, flags);
>> - }
>> +/**
>> + * crypto_finalize_skcipher_request - finalize one skcipher_request if
>> + * the request is done
>> + * @engine: the hardware engine
>> + * @req: the request need to be finalized
>> + * @err: error number
>> + */
>> +void crypto_finalize_skcipher_request(struct crypto_engine *engine,
>> + struct skcipher_request *req, int err)
>> +{
>> + return crypto_finalize_request(engine, &req->base, err);
>> +}
>> +EXPORT_SYMBOL_GPL(crypto_finalize_skcipher_request);
>>
>> - req->base.complete(&req->base, err);
>> +/**
>> + * crypto_finalize_akcipher_request - finalize one akcipher_request if
>> + * the request is done
>> + * @engine: the hardware engine
>> + * @req: the request need to be finalized
>> + * @err: error number
>> + */
>> +void crypto_finalize_akcipher_request(struct crypto_engine *engine,
>> + struct akcipher_request *req, int err)
>> +{
>> + return crypto_finalize_request(engine, &req->base, err);
>> +}
>> +EXPORT_SYMBOL_GPL(crypto_finalize_akcipher_request);
>>
>> - kthread_queue_work(engine->kworker, &engine->pump_requests);
>> +/**
>> + * crypto_finalize_hash_request - finalize one ahash_request if
>> + * the request is done
>> + * @engine: the hardware engine
>> + * @req: the request need to be finalized
>> + * @err: error number
>> + */
>> +void crypto_finalize_hash_request(struct crypto_engine *engine,
>> + struct ahash_request *req, int err)
>> +{
>> + return crypto_finalize_request(engine, &req->base, err);
>> }
>> EXPORT_SYMBOL_GPL(crypto_finalize_hash_request);
> Add
> crypto_finalize_aead_request(struct crypto_engine *engine, struct
> aead_request *req, int err)
>
>>
>> diff --git a/include/crypto/engine.h b/include/crypto/engine.h
>> index dd04c1699b51..1ea7cbe92eaf 100644
>> --- a/include/crypto/engine.h
>> +++ b/include/crypto/engine.h
>> @@ -17,7 +17,9 @@
>> #include <linux/kernel.h>
>> #include <linux/kthread.h>
>> #include <crypto/algapi.h>
>> +#include <crypto/akcipher.h>
>> #include <crypto/hash.h>
>> +#include <crypto/skcipher.h>
>>
>> #define ENGINE_NAME_LEN 30
>> /*
>> @@ -37,12 +39,6 @@
>> * @unprepare_crypt_hardware: there are currently no more requests on the
>> * queue so the subsystem notifies the driver that it may relax the
>> * hardware by issuing this call
>> - * @prepare_cipher_request: do some prepare if need before handle the current request
>> - * @unprepare_cipher_request: undo any work done by prepare_cipher_request()
>> - * @cipher_one_request: do encryption for current request
>> - * @prepare_hash_request: do some prepare if need before handle the current request
>> - * @unprepare_hash_request: undo any work done by prepare_hash_request()
>> - * @hash_one_request: do hash for current request
>> * @kworker: kthread worker struct for request pump
>> * @pump_requests: work struct for scheduling work to the request pump
>> * @priv_data: the engine private data
>> @@ -65,19 +61,6 @@ struct crypto_engine {
>> int (*prepare_crypt_hardware)(struct crypto_engine *engine);
>> int (*unprepare_crypt_hardware)(struct crypto_engine *engine);
>>
>> - int (*prepare_cipher_request)(struct crypto_engine *engine,
>> - struct ablkcipher_request *req);
>> - int (*unprepare_cipher_request)(struct crypto_engine *engine,
>> - struct ablkcipher_request *req);
>> - int (*prepare_hash_request)(struct crypto_engine *engine,
>> - struct ahash_request *req);
>> - int (*unprepare_hash_request)(struct crypto_engine *engine,
>> - struct ahash_request *req);
>> - int (*cipher_one_request)(struct crypto_engine *engine,
>> - struct ablkcipher_request *req);
>> - int (*hash_one_request)(struct crypto_engine *engine,
>> - struct ahash_request *req);
>> -
>> struct kthread_worker *kworker;
>> struct kthread_work pump_requests;
>>
>> @@ -85,19 +68,43 @@ struct crypto_engine {
>> struct crypto_async_request *cur_req;
>> };
>>
>> -int crypto_transfer_cipher_request(struct crypto_engine *engine,
>> - struct ablkcipher_request *req,
>> - bool need_pump);
>> +/*
>> + * struct crypto_engine_op - crypto hardware engine operations
>> + * @prepare__request: do some prepare if need before handle the current request
>> + * @unprepare_request: undo any work done by prepare_request()
>> + * @do_one_request: do encryption for current request
>> + */
>> +struct crypto_engine_op {
>> + int (*prepare_request)(struct crypto_engine *engine,
>> + void *areq);
>> + int (*unprepare_request)(struct crypto_engine *engine,
>> + void *areq);
>> + int (*do_one_request)(struct crypto_engine *engine,
>> + void *areq);
>> +};
>> +
>> +struct crypto_engine_reqctx {
>> + struct crypto_engine_op op;
>> +};
>> +
>> +int crypto_transfer_akcipher_request_to_engine(struct crypto_engine *engine,
>> + struct akcipher_request *req);
>> int crypto_transfer_cipher_request_to_engine(struct crypto_engine *engine,
>> - struct ablkcipher_request *req);
>> -int crypto_transfer_hash_request(struct crypto_engine *engine,
>> - struct ahash_request *req, bool need_pump);
>> + struct ablkcipher_request *req);
>> int crypto_transfer_hash_request_to_engine(struct crypto_engine *engine,
>> - struct ahash_request *req);
>> + struct ahash_request *req);
>> +int crypto_transfer_skcipher_request_to_engine(struct crypto_engine *engine,
>> + struct skcipher_request *req);
> + transfer_aead
>
>> +void crypto_finalize_request(struct crypto_engine *engine,
>> + struct crypto_async_request *req, int err);
> static (+move to .c file?)
>
>> +void crypto_finalize_akcipher_request(struct crypto_engine *engine,
>> + struct akcipher_request *req, int err);
>> void crypto_finalize_cipher_request(struct crypto_engine *engine,
>> struct ablkcipher_request *req, int err);
>> void crypto_finalize_hash_request(struct crypto_engine *engine,
>> struct ahash_request *req, int err);
>> +void crypto_finalize_skcipher_request(struct crypto_engine *engine,
>> + struct skcipher_request *req, int err);
> + finalize_aead
>
>> int crypto_engine_start(struct crypto_engine *engine);
>> int crypto_engine_stop(struct crypto_engine *engine);
>> struct crypto_engine *crypto_engine_alloc_init(struct device *dev, bool rt);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next] vhost_net: batch used ring update in rx
From: David Miller @ 2018-01-10 20:04 UTC (permalink / raw)
To: jasowang; +Cc: willemb, kvm, mst, netdev, linux-kernel, virtualization
In-Reply-To: <1515493665-35306-1-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Tue, 9 Jan 2018 18:27:45 +0800
> This patch tries to batched used ring update during RX. This is pretty
> fit for the case when guest is much faster (e.g dpdk based
> backend). In this case, used ring is almost empty:
>
> - we may get serious cache line misses/contending on both used ring
> and used idx.
> - at most 1 packet could be dequeued at one time, batching in guest
> does not make much effect.
>
> Update used ring in a batch can help since guest won't access the used
> ring until used idx was advanced for several descriptors and since we
> advance used ring for every N packets, guest will only need to access
> used idx for every N packet since it can cache the used idx. To have a
> better interaction for both batch dequeuing and dpdk batching,
> VHOST_RX_BATCH was used as the maximum number of descriptors that
> could be batched.
>
> Test were done between two machines with 2.40GHz Intel(R) Xeon(R) CPU
> E5-2630 connected back to back through ixgbe. Traffic were generated
> on one remote ixgbe through MoonGen and measure the RX pps through
> testpmd in guest when do xdp_redirect_map from local ixgbe to
> tap. RX pps were increased from 3.05 Mpps to 4.00 Mpps (about 31%
> improvement).
>
> One possible concern for this is the implications for TCP (especially
> latency sensitive workload). Result[1] does not show obvious changes
> for most of the netperf test (RR, TX, and RX). And we do get some
> improvements for RX on some specific size.
...
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Applied, thanks Jason.
^ 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