Re: [RFC PATCH v2 1/5] iommu: Add virtio-iommu driver

[Jean-Philippe Brucker]

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] = >top;
>> +sg[1] = >bottom;
>> +
>> +ret = virtqueue_add_sgs(viommu->vq, sg, 1, 1, req,
>> +GFP_ATOMIC);
>> +if (ret)
>> +break;
>> +
>> +list_add_tail(>list, );
>> +}
>> +
>> +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,
>> +   );
>> +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(>mappings_lock, flags);
>> +next = interval_tree_iter_first(>mappings, iova, last);
>> +
>> +if (next) 

Re: [RFC PATCH v2 1/5] iommu: Add virtio-iommu driver

[Auger Eric]

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 
> ---
>  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,  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 ..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 
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#define MSI_IOVA_BASE0x800
> +#define MSI_IOVA_LENGTH  0x10
> +
> +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_geometrygeometry;
> + 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 mutexmutex;
> + unsigned intid;
> +
> + 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_headlist;
> +};
> +
> +#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:
>