Re: [PATCH 1/3] virtio-iommu: Add bypass mode support to assigned device

[Eric Auger]

Hi,
On 6/13/22 08:10, Zhenzhong Duan wrote:
> Currently assigned devices can not work in virtio-iommu bypass mode.
> Guest driver fails to probe the device due to DMA failure. And the
> reason is because of lacking GPA -> HPA mappings when VM is created.
>
> Add a root container memory region to hold both bypass memory region
> and iommu memory region, so the switch between them is supported
> just like the implementation in virtual VT-d.
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  hw/virtio/trace-events           |   1 +
>  hw/virtio/virtio-iommu.c         | 115 ++++++++++++++++++++++++++++++-
>  include/hw/virtio/virtio-iommu.h |   2 +
>  3 files changed, 116 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
> index ab8e095b73fa..20af2e7ebd78 100644
> --- a/hw/virtio/trace-events
> +++ b/hw/virtio/trace-events
> @@ -124,6 +124,7 @@ virtio_iommu_remap(const char *name, uint64_t virt_start, 
> uint64_t virt_end, uin
>  virtio_iommu_set_page_size_mask(const char *name, uint64_t old, uint64_t 
> new) "mr=%s old_mask=0x%"PRIx64" new_mask=0x%"PRIx64
>  virtio_iommu_notify_flag_add(const char *name) "add notifier to mr %s"
>  virtio_iommu_notify_flag_del(const char *name) "del notifier from mr %s"
> +virtio_iommu_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn, 
> bool on) "Device %02x:%02x.%x switching address space (iommu enabled=%d)"
>  
>  # virtio-mem.c
>  virtio_mem_send_response(uint16_t type) "type=%" PRIu16
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> index 2597e166f939..ff718107ee02 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -69,6 +69,77 @@ static inline uint16_t virtio_iommu_get_bdf(IOMMUDevice 
> *dev)
>      return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
>  }
>  
> +static bool virtio_iommu_device_bypassed(IOMMUDevice *sdev)
> +{
> +    uint32_t sid;
> +    bool bypassed;
> +    VirtIOIOMMU *s = sdev->viommu;
> +    VirtIOIOMMUEndpoint *ep;
> +
> +    sid = virtio_iommu_get_bdf(sdev);
> +
> +    qemu_mutex_lock(&s->mutex);
> +    /* need to check bypass before system reset */
> +    if (!s->endpoints) {
> +        bypassed = s->config.bypass;
> +        goto unlock;
> +    }
> +
> +    ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(sid));
> +    if (!ep || !ep->domain) {
> +        bypassed = s->config.bypass;
> +    } else {
> +        bypassed = ep->domain->bypass;
> +    }
> +
> +unlock:
> +    qemu_mutex_unlock(&s->mutex);
> +    return bypassed;
> +}
> +
> +/* Return whether the device is using IOMMU translation. */
> +static bool virtio_iommu_switch_address_space(IOMMUDevice *sdev)
> +{
> +    bool use_remapping;
> +
> +    assert(sdev);
> +
> +    use_remapping = !virtio_iommu_device_bypassed(sdev);
> +
> +    trace_virtio_iommu_switch_address_space(pci_bus_num(sdev->bus),
> +                                            PCI_SLOT(sdev->devfn),
> +                                            PCI_FUNC(sdev->devfn),
> +                                            use_remapping);
> +
> +    /* Turn off first then on the other */
> +    if (use_remapping) {
> +        memory_region_set_enabled(&sdev->bypass_mr, false);
> +        memory_region_set_enabled(MEMORY_REGION(&sdev->iommu_mr), true);
> +    } else {
> +        memory_region_set_enabled(MEMORY_REGION(&sdev->iommu_mr), false);
> +        memory_region_set_enabled(&sdev->bypass_mr, true);
> +    }
> +
> +    return use_remapping;
> +}
> +
> +static void virtio_iommu_switch_address_space_all(VirtIOIOMMU *s)
> +{
> +    GHashTableIter iter;
> +    IOMMUPciBus *iommu_pci_bus;
> +    int i;
> +
> +    g_hash_table_iter_init(&iter, s->as_by_busptr);
> +    while (g_hash_table_iter_next(&iter, NULL, (void **)&iommu_pci_bus)) {
> +        for (i = 0; i < PCI_DEVFN_MAX; i++) {
> +            if (!iommu_pci_bus->pbdev[i]) {
> +                continue;
> +            }
> +            virtio_iommu_switch_address_space(iommu_pci_bus->pbdev[i]);
> +        }
> +    }
> +}
> +
>  /**
>   * The bus number is used for lookup when SID based operations occur.
>   * In that case we lazily populate the IOMMUPciBus array from the bus hash
> @@ -213,6 +284,7 @@ static gboolean virtio_iommu_notify_map_cb(gpointer key, 
> gpointer value,
>  static void virtio_iommu_detach_endpoint_from_domain(VirtIOIOMMUEndpoint *ep)
>  {
>      VirtIOIOMMUDomain *domain = ep->domain;
> +    IOMMUDevice *sdev = container_of(ep->iommu_mr, IOMMUDevice, iommu_mr);
>  
>      if (!ep->domain) {
>          return;
> @@ -221,6 +293,7 @@ static void 
> virtio_iommu_detach_endpoint_from_domain(VirtIOIOMMUEndpoint *ep)
>                     ep->iommu_mr);
>      QLIST_REMOVE(ep, next);
>      ep->domain = NULL;
> +    virtio_iommu_switch_address_space(sdev);
>  }
>  
>  static VirtIOIOMMUEndpoint *virtio_iommu_get_endpoint(VirtIOIOMMU *s,
> @@ -323,12 +396,39 @@ static AddressSpace *virtio_iommu_find_add_as(PCIBus 
> *bus, void *opaque,
>  
>          trace_virtio_iommu_init_iommu_mr(name);
>  
> +        memory_region_init(&sdev->root, OBJECT(s), name, UINT64_MAX);
> +        address_space_init(&sdev->as, &sdev->root, TYPE_VIRTIO_IOMMU);
> +
> +        /*
> +         * Build the IOMMU disabled container with aliases to the
> +         * shared MRs.  Note that aliasing to a shared memory region
What do you call the 'disabled container' and the shared MRs?

> +         * could help the memory API to detect same FlatViews so we
> +         * can have devices to share the same FlatView when in bypass
> +         * mode. (either by not configuring virtio-iommu driver or with
> +         * "iommu=pt").  It will greatly reduce the total number of
> +         * FlatViews of the system hence VM runs faster.

Do you mean that we could have used a shared bypass MR instead of
creatingone per device?
> +         */
> +        memory_region_init_alias(&sdev->bypass_mr, OBJECT(s),
> +                                 "system", get_system_memory(), 0,
> +                                 memory_region_size(get_system_memory()));
> +
>          memory_region_init_iommu(&sdev->iommu_mr, sizeof(sdev->iommu_mr),
>                                   TYPE_VIRTIO_IOMMU_MEMORY_REGION,
>                                   OBJECT(s), name,
>                                   UINT64_MAX);
> -        address_space_init(&sdev->as,
> -                           MEMORY_REGION(&sdev->iommu_mr), 
> TYPE_VIRTIO_IOMMU);
> +
> +        /*
> +         * Hook both the containers under the root container, we
did you mean "hook both sub-regions to the root container"?
> +         * switch between iommu & bypass MRs by enable/disable
> +         * corresponding sub-containers
> +         */
> +        memory_region_add_subregion_overlap(&sdev->root, 0,
> +                                            MEMORY_REGION(&sdev->iommu_mr),
> +                                            0);
> +        memory_region_add_subregion_overlap(&sdev->root, 0,
> +                                            &sdev->bypass_mr, 0);
> +
> +        virtio_iommu_switch_address_space(sdev);
>          g_free(name);
>      }
>      return &sdev->as;
> @@ -342,6 +442,7 @@ static int virtio_iommu_attach(VirtIOIOMMU *s,
>      uint32_t flags = le32_to_cpu(req->flags);
>      VirtIOIOMMUDomain *domain;
>      VirtIOIOMMUEndpoint *ep;
> +    IOMMUDevice *sdev;
>  
>      trace_virtio_iommu_attach(domain_id, ep_id);
>  
> @@ -375,6 +476,8 @@ static int virtio_iommu_attach(VirtIOIOMMU *s,
>      QLIST_INSERT_HEAD(&domain->endpoint_list, ep, next);
>  
>      ep->domain = domain;
> +    sdev = container_of(ep->iommu_mr, IOMMUDevice, iommu_mr);
> +    virtio_iommu_switch_address_space(sdev);
>  
>      /* Replay domain mappings on the associated memory region */
>      g_tree_foreach(domain->mappings, virtio_iommu_notify_map_cb,
> @@ -887,6 +990,7 @@ static void virtio_iommu_set_config(VirtIODevice *vdev,
>              return;
>          }
>          dev_config->bypass = in_config->bypass;
> +        virtio_iommu_switch_address_space_all(dev);
>      }
>  
>      trace_virtio_iommu_set_config(in_config->bypass);
> @@ -1026,6 +1130,8 @@ static void virtio_iommu_system_reset(void *opaque)
>       * system reset
>       */
>      s->config.bypass = s->boot_bypass;
> +    virtio_iommu_switch_address_space_all(s);
> +
>  }
>  
>  static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
> @@ -1041,6 +1147,11 @@ static void virtio_iommu_device_realize(DeviceState 
> *dev, Error **errp)
>                               virtio_iommu_handle_command);
>      s->event_vq = virtio_add_queue(vdev, VIOMMU_DEFAULT_QUEUE_SIZE, NULL);
>  
> +    /*
> +     * config.bypass is needed to get initial address space early, such as
> +     * in vfio realize
> +     */
I don't understand this comment, please can you elaborate?
> +    s->config.bypass = s->boot_bypass;
>      s->config.page_size_mask = TARGET_PAGE_MASK;
>      s->config.input_range.end = UINT64_MAX;
>      s->config.domain_range.end = UINT32_MAX;
> diff --git a/include/hw/virtio/virtio-iommu.h 
> b/include/hw/virtio/virtio-iommu.h
> index 84391f844826..102eeefa731d 100644
> --- a/include/hw/virtio/virtio-iommu.h
> +++ b/include/hw/virtio/virtio-iommu.h
> @@ -37,6 +37,8 @@ typedef struct IOMMUDevice {
>      int           devfn;
>      IOMMUMemoryRegion  iommu_mr;
>      AddressSpace  as;
> +    MemoryRegion root;          /* The root container of the device */
> +    MemoryRegion bypass_mr;     /* The alias of shared memory MR */
>  } IOMMUDevice;
>  
>  typedef struct IOMMUPciBus {
Did you test migration? I wonder if we shouldn't call

virtio_iommu_switch_address_space_all(s)
after restoring the endpoints in iommu_post_load() as it is done in intel-iommu

Thanks

Eric