Re: [PATCH v13 03/10] virtio-iommu: Implement attach/detach command

[Auger Eric]

Hi Peter,

On 2/3/20 2:49 PM, Peter Xu wrote:
> On Sat, Jan 25, 2020 at 06:19:48PM +0100, Eric Auger wrote:
>> This patch implements the endpoint attach/detach to/from
>> a domain.
>>
>> Domain and endpoint internal datatypes are introduced.
>> Both are stored in RB trees. The domain owns a list of
>> endpoints attached to it. Also helpers to get/put
>> end points and domains are introduced.
>>
>> As for the IOMMU memory regions, a callback is called on
>> PCI bus enumeration that initializes for a given device
>> on the bus hierarchy an IOMMU memory region. The PCI bus
>> hierarchy is stored locally in IOMMUPciBus and IOMMUDevice
>> objects.
>>
>> At the time of the enumeration, the bus number may not be
>> computed yet.
>>
>> So operations that will need to retrieve the IOMMUdevice
>> and its IOMMU memory region from the bus number and devfn,
>> once the bus number is garanteed to be frozen, use an array
>> of IOMMUPciBus, lazily populated.
>>
>> Signed-off-by: Eric Auger <address@hidden>
>>
>> ---
>>
>> v12 -> v13:
>> - squashed v12 4, 5, 6 into this patch
>> - rename virtio_iommu_get_sid into virtio_iommu_get_bdf
>>
>> v11 -> v12:
>> - check the device is protected by the iommu on attach
>> - on detach, check the domain id the device is attached to matches
>>   the one used in the detach command
>> - fix mapping ref counter and destroy the domain when no end-points
>>   are attached anymore
>> ---
>>  hw/virtio/trace-events           |   6 +
>>  hw/virtio/virtio-iommu.c         | 315 ++++++++++++++++++++++++++++++-
>>  include/hw/virtio/virtio-iommu.h |   3 +
>>  3 files changed, 322 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
>> index f7141aa2f6..15595f8cd7 100644
>> --- a/hw/virtio/trace-events
>> +++ b/hw/virtio/trace-events
>> @@ -64,3 +64,9 @@ virtio_iommu_attach(uint32_t domain_id, uint32_t ep_id) 
>> "domain=%d endpoint=%d"
>>  virtio_iommu_detach(uint32_t domain_id, uint32_t ep_id) "domain=%d 
>> endpoint=%d"
>>  virtio_iommu_map(uint32_t domain_id, uint64_t virt_start, uint64_t 
>> virt_end, uint64_t phys_start, uint32_t flags) "domain=%d 
>> virt_start=0x%"PRIx64" virt_end=0x%"PRIx64 " phys_start=0x%"PRIx64" flags=%d"
>>  virtio_iommu_unmap(uint32_t domain_id, uint64_t virt_start, uint64_t 
>> virt_end) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
>> +virtio_iommu_translate(const char *name, uint32_t rid, uint64_t iova, int 
>> flag) "mr=%s rid=%d addr=0x%"PRIx64" flag=%d"
>> +virtio_iommu_init_iommu_mr(char *iommu_mr) "init %s"
>> +virtio_iommu_get_endpoint(uint32_t ep_id) "Alloc endpoint=%d"
>> +virtio_iommu_put_endpoint(uint32_t ep_id) "Free endpoint=%d"
>> +virtio_iommu_get_domain(uint32_t domain_id) "Alloc domain=%d"
>> +virtio_iommu_put_domain(uint32_t domain_id) "Free domain=%d"
>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
>> index 9d1b997df7..e5cc94138b 100644
>> --- a/hw/virtio/virtio-iommu.c
>> +++ b/hw/virtio/virtio-iommu.c
>> @@ -23,6 +23,8 @@
>>  #include "hw/qdev-properties.h"
>>  #include "hw/virtio/virtio.h"
>>  #include "sysemu/kvm.h"
>> +#include "qapi/error.h"
>> +#include "qemu/error-report.h"
>>  #include "trace.h"
>>  
>>  #include "standard-headers/linux/virtio_ids.h"
>> @@ -30,19 +32,234 @@
>>  #include "hw/virtio/virtio-bus.h"
>>  #include "hw/virtio/virtio-access.h"
>>  #include "hw/virtio/virtio-iommu.h"
>> +#include "hw/pci/pci_bus.h"
>> +#include "hw/pci/pci.h"
>>  
>>  /* Max size */
>>  #define VIOMMU_DEFAULT_QUEUE_SIZE 256
>>  
>> +typedef struct VirtIOIOMMUDomain {
>> +    uint32_t id;
>> +    GTree *mappings;
>> +    QLIST_HEAD(, VirtIOIOMMUEndpoint) endpoint_list;
>> +} VirtIOIOMMUDomain;
>> +
>> +typedef struct VirtIOIOMMUEndpoint {
>> +    uint32_t id;
>> +    VirtIOIOMMUDomain *domain;
>> +    QLIST_ENTRY(VirtIOIOMMUEndpoint) next;
>> +} VirtIOIOMMUEndpoint;
>> +
>> +typedef struct VirtIOIOMMUInterval {
>> +    uint64_t low;
>> +    uint64_t high;
>> +} VirtIOIOMMUInterval;
>> +
>> +static inline uint16_t virtio_iommu_get_bdf(IOMMUDevice *dev)
>> +{
>> +    return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
>> +}
>> +
>> +/**
>> + * 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
>> + * table. At the time the IOMMUPciBus is created (iommu_find_add_as), the 
>> bus
>> + * numbers may not be always initialized yet.
>> + */
>> +static IOMMUPciBus *iommu_find_iommu_pcibus(VirtIOIOMMU *s, uint8_t bus_num)
>> +{
>> +    IOMMUPciBus *iommu_pci_bus = s->iommu_pcibus_by_bus_num[bus_num];
>> +
>> +    if (!iommu_pci_bus) {
>> +        GHashTableIter iter;
>> +
>> +        g_hash_table_iter_init(&iter, s->as_by_busptr);
>> +        while (g_hash_table_iter_next(&iter, NULL, (void 
>> **)&iommu_pci_bus)) {
>> +            if (pci_bus_num(iommu_pci_bus->bus) == bus_num) {
>> +                s->iommu_pcibus_by_bus_num[bus_num] = iommu_pci_bus;
>> +                return iommu_pci_bus;
>> +            }
>> +        }
>> +        return NULL;
>> +    }
>> +    return iommu_pci_bus;
>> +}
>> +
>> +static IOMMUMemoryRegion *virtio_iommu_mr(VirtIOIOMMU *s, uint32_t sid)
>> +{
>> +    uint8_t bus_n, devfn;
>> +    IOMMUPciBus *iommu_pci_bus;
>> +    IOMMUDevice *dev;
>> +
>> +    bus_n = PCI_BUS_NUM(sid);
>> +    iommu_pci_bus = iommu_find_iommu_pcibus(s, bus_n);
>> +    if (iommu_pci_bus) {
>> +        devfn = sid & PCI_DEVFN_MAX;
>> +        dev = iommu_pci_bus->pbdev[devfn];
>> +        if (dev) {
>> +            return &dev->iommu_mr;
>> +        }
>> +    }
>> +    return NULL;
>> +}
>> +
>> +static gint interval_cmp(gconstpointer a, gconstpointer b, gpointer 
>> user_data)
>> +{
>> +    VirtIOIOMMUInterval *inta = (VirtIOIOMMUInterval *)a;
>> +    VirtIOIOMMUInterval *intb = (VirtIOIOMMUInterval *)b;
>> +
>> +    if (inta->high < intb->low) {
>> +        return -1;
>> +    } else if (intb->high < inta->low) {
>> +        return 1;
>> +    } else {
>> +        return 0;
>> +    }
>> +}
>> +
>> +static void virtio_iommu_detach_endpoint_from_domain(VirtIOIOMMUEndpoint 
>> *ep)
>> +{
>> +    QLIST_REMOVE(ep, next);
>> +    g_tree_unref(ep->domain->mappings);
> 
> Here domain->mapping is unreferenced for each endpoint, while at [1]
> below you only reference the domain->mappings if it's the first
> endpoint.  Is that problematic?
in [1] I take a ref to the domain->mappings if it is *not* the 1st
endpoint. This aims at deleting the mappings gtree when the last EP is
detached from the domain.

This fixes the issue reported by Jean in:
https://patchwork.kernel.org/patch/11258267/#23046313
> 
>> +    ep->domain = NULL;
>> +}
>> +
>> +static VirtIOIOMMUEndpoint *virtio_iommu_get_endpoint(VirtIOIOMMU *s,
>> +                                                      uint32_t ep_id)
>> +{
>> +    VirtIOIOMMUEndpoint *ep;
>> +
>> +    ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(ep_id));
>> +    if (ep) {
>> +        return ep;
>> +    }
>> +    if (!virtio_iommu_mr(s, ep_id)) {
>> +        return NULL;
>> +    }
>> +    ep = g_malloc0(sizeof(*ep));
>> +    ep->id = ep_id;
>> +    trace_virtio_iommu_get_endpoint(ep_id);
>> +    g_tree_insert(s->endpoints, GUINT_TO_POINTER(ep_id), ep);
>> +    return ep;
>> +}
>> +
>> +static void virtio_iommu_put_endpoint(gpointer data)
>> +{
>> +    VirtIOIOMMUEndpoint *ep = (VirtIOIOMMUEndpoint *)data;
>> +
>> +    assert(!ep->domain);
>> +
>> +    trace_virtio_iommu_put_endpoint(ep->id);
>> +    g_free(ep);
>> +}
>> +
>> +static VirtIOIOMMUDomain *virtio_iommu_get_domain(VirtIOIOMMU *s,
>> +                                                  uint32_t domain_id)
>> +{
>> +    VirtIOIOMMUDomain *domain;
>> +
>> +    domain = g_tree_lookup(s->domains, GUINT_TO_POINTER(domain_id));
>> +    if (domain) {
>> +        return domain;
>> +    }
>> +    domain = g_malloc0(sizeof(*domain));
>> +    domain->id = domain_id;
>> +    domain->mappings = g_tree_new_full((GCompareDataFunc)interval_cmp,
>> +                                   NULL, (GDestroyNotify)g_free,
>> +                                   (GDestroyNotify)g_free);
>> +    g_tree_insert(s->domains, GUINT_TO_POINTER(domain_id), domain);
>> +    QLIST_INIT(&domain->endpoint_list);
>> +    trace_virtio_iommu_get_domain(domain_id);
>> +    return domain;
>> +}
>> +
>> +static void virtio_iommu_put_domain(gpointer data)
>> +{
>> +    VirtIOIOMMUDomain *domain = (VirtIOIOMMUDomain *)data;
>> +    VirtIOIOMMUEndpoint *iter, *tmp;
>> +
>> +    QLIST_FOREACH_SAFE(iter, &domain->endpoint_list, next, tmp) {
>> +        virtio_iommu_detach_endpoint_from_domain(iter);
>> +    }
> 
> Do you need to destroy the domain->mappings here?  Is it leaked?
AFIU as we detach all EPs in the loop above, the whole "mappings" gtree
is destroyed so there is no leak.
> 
>> +    trace_virtio_iommu_put_domain(domain->id);
>> +    g_free(domain);
>> +}
>> +
>> +static AddressSpace *virtio_iommu_find_add_as(PCIBus *bus, void *opaque,
>> +                                              int devfn)
>> +{
>> +    VirtIOIOMMU *s = opaque;
>> +    IOMMUPciBus *sbus = g_hash_table_lookup(s->as_by_busptr, bus);
>> +    static uint32_t mr_index;
>> +    IOMMUDevice *sdev;
>> +
>> +    if (!sbus) {
>> +        sbus = g_malloc0(sizeof(IOMMUPciBus) +
>> +                         sizeof(IOMMUDevice *) * PCI_DEVFN_MAX);
>> +        sbus->bus = bus;
>> +        g_hash_table_insert(s->as_by_busptr, bus, sbus);
>> +    }
>> +
>> +    sdev = sbus->pbdev[devfn];
>> +    if (!sdev) {
>> +        char *name = g_strdup_printf("%s-%d-%d",
>> +                                     TYPE_VIRTIO_IOMMU_MEMORY_REGION,
>> +                                     mr_index++, devfn);
>> +        sdev = sbus->pbdev[devfn] = g_malloc0(sizeof(IOMMUDevice));
>> +
>> +        sdev->viommu = s;
>> +        sdev->bus = bus;
>> +        sdev->devfn = devfn;
>> +
>> +        trace_virtio_iommu_init_iommu_mr(name);
>> +
>> +        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);
>> +        g_free(name);
>> +    }
>> +    return &sdev->as;
>> +}
>> +
>>  static int virtio_iommu_attach(VirtIOIOMMU *s,
>>                                 struct virtio_iommu_req_attach *req)
>>  {
>>      uint32_t domain_id = le32_to_cpu(req->domain);
>>      uint32_t ep_id = le32_to_cpu(req->endpoint);
>> +    VirtIOIOMMUDomain *domain;
>> +    VirtIOIOMMUEndpoint *ep;
>>  
>>      trace_virtio_iommu_attach(domain_id, ep_id);
>>  
>> -    return VIRTIO_IOMMU_S_UNSUPP;
>> +    ep = virtio_iommu_get_endpoint(s, ep_id);
>> +    if (!ep) {
>> +        return VIRTIO_IOMMU_S_NOENT;
>> +    }
>> +
>> +    if (ep->domain) {
>> +        VirtIOIOMMUDomain *previous_domain = ep->domain;
>> +        /*
>> +         * the device is already attached to a domain,
>> +         * detach it first
>> +         */
>> +        virtio_iommu_detach_endpoint_from_domain(ep);
>> +        if (QLIST_EMPTY(&previous_domain->endpoint_list)) {
>> +            g_tree_remove(s->domains, 
>> GUINT_TO_POINTER(previous_domain->id));
>> +        }
>> +    }
>> +
>> +    domain = virtio_iommu_get_domain(s, domain_id);
>> +    if (!QLIST_EMPTY(&domain->endpoint_list)) {
>> +        g_tree_ref(domain->mappings);
> 
> [1]
!QLIST_EMPTY
> 
>> +    }
>> +    QLIST_INSERT_HEAD(&domain->endpoint_list, ep, next);
>> +
>> +    ep->domain = domain;
>> +
>> +    return VIRTIO_IOMMU_S_OK;
>>  }
>>  
>>  static int virtio_iommu_detach(VirtIOIOMMU *s,
>> @@ -50,10 +267,34 @@ static int virtio_iommu_detach(VirtIOIOMMU *s,
>>  {
>>      uint32_t domain_id = le32_to_cpu(req->domain);
>>      uint32_t ep_id = le32_to_cpu(req->endpoint);
>> +    VirtIOIOMMUDomain *domain;
>> +    VirtIOIOMMUEndpoint *ep;
>>  
>>      trace_virtio_iommu_detach(domain_id, ep_id);
>>  
>> -    return VIRTIO_IOMMU_S_UNSUPP;
>> +    ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(ep_id));
>> +    if (!ep) {
>> +        return VIRTIO_IOMMU_S_NOENT;
>> +    }
>> +
>> +    domain = ep->domain;
>> +
>> +    if (!domain || domain->id != domain_id) {
>> +        return VIRTIO_IOMMU_S_INVAL;
>> +    }
>> +
>> +    virtio_iommu_detach_endpoint_from_domain(ep);
>> +
>> +    /*
>> +     * when the last EP is detached, simply remove the domain for
>> +     * the domain list and destroy it. Note its mappings were already
>> +     * freed by the ref count mechanism. Next operation involving
>> +     * the same domain id will re-create one domain object.
>> +     */
>> +    if (QLIST_EMPTY(&domain->endpoint_list)) {
>> +        g_tree_remove(s->domains, GUINT_TO_POINTER(domain->id));
>> +    }
>> +    return VIRTIO_IOMMU_S_OK;
>>  }
>>  
>>  static int virtio_iommu_map(VirtIOIOMMU *s,
>> @@ -172,6 +413,27 @@ out:
>>      }
>>  }
>>  
>> +static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr 
>> addr,
>> +                                            IOMMUAccessFlags flag,
>> +                                            int iommu_idx)
>> +{
>> +    IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr);
>> +    uint32_t sid;
>> +
>> +    IOMMUTLBEntry entry = {
>> +        .target_as = &address_space_memory,
>> +        .iova = addr,
>> +        .translated_addr = addr,
>> +        .addr_mask = ~(hwaddr)0,
>> +        .perm = IOMMU_NONE,
>> +    };
>> +
>> +    sid = virtio_iommu_get_bdf(sdev);
>> +
>> +    trace_virtio_iommu_translate(mr->parent_obj.name, sid, addr, flag);
>> +    return entry;
>> +}
>> +
>>  static void virtio_iommu_get_config(VirtIODevice *vdev, uint8_t 
>> *config_data)
>>  {
>>      VirtIOIOMMU *dev = VIRTIO_IOMMU(vdev);
>> @@ -218,6 +480,13 @@ static const VMStateDescription 
>> vmstate_virtio_iommu_device = {
>>      .unmigratable = 1,
>>  };
>>  
>> +static gint int_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
>> +{
>> +    guint ua = GPOINTER_TO_UINT(a);
>> +    guint ub = GPOINTER_TO_UINT(b);
>> +    return (ua > ub) - (ua < ub);
>> +}
>> +
>>  static void virtio_iommu_device_realize(DeviceState *dev, Error **errp)
>>  {
>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> @@ -226,6 +495,8 @@ static void virtio_iommu_device_realize(DeviceState 
>> *dev, Error **errp)
>>      virtio_init(vdev, "virtio-iommu", VIRTIO_ID_IOMMU,
>>                  sizeof(struct virtio_iommu_config));
>>  
>> +    memset(s->iommu_pcibus_by_bus_num, 0, 
>> sizeof(s->iommu_pcibus_by_bus_num));
>> +
>>      s->req_vq = virtio_add_queue(vdev, VIOMMU_DEFAULT_QUEUE_SIZE,
>>                               virtio_iommu_handle_command);
>>      s->event_vq = virtio_add_queue(vdev, VIOMMU_DEFAULT_QUEUE_SIZE, NULL);
>> @@ -244,18 +515,43 @@ static void virtio_iommu_device_realize(DeviceState 
>> *dev, Error **errp)
>>      virtio_add_feature(&s->features, VIRTIO_IOMMU_F_MMIO);
>>  
>>      qemu_mutex_init(&s->mutex);
>> +
>> +    s->as_by_busptr = g_hash_table_new_full(NULL, NULL, NULL, g_free);
>> +
>> +    if (s->primary_bus) {
>> +        pci_setup_iommu(s->primary_bus, virtio_iommu_find_add_as, s);
>> +    } else {
>> +        error_setg(errp, "VIRTIO-IOMMU is not attached to any PCI bus!");
>> +    }
>>  }
>>  
>>  static void virtio_iommu_device_unrealize(DeviceState *dev, Error **errp)
>>  {
>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> +    VirtIOIOMMU *s = VIRTIO_IOMMU(dev);
>> +
>> +    g_tree_destroy(s->domains);
>> +    g_tree_destroy(s->endpoints);
>>  
>>      virtio_cleanup(vdev);
>>  }
>>  
>>  static void virtio_iommu_device_reset(VirtIODevice *vdev)
>>  {
>> +    VirtIOIOMMU *s = VIRTIO_IOMMU(vdev);
>> +
>>      trace_virtio_iommu_device_reset();
>> +
>> +    if (s->domains) {
>> +        g_tree_destroy(s->domains);
>> +    }
>> +    if (s->endpoints) {
>> +        g_tree_destroy(s->endpoints);
>> +    }
> 
> Is it a must to free domians first then the endpoints here?
> 
> I see that virtio_iommu_put_domain() will unlink the domains and
> endpoints, then in virtio_iommu_put_endpoint() you assert that
> ep->domain is NULL.  It's fine but I'm a bit curious on why not call
> virtio_iommu_detach_endpoint_from_domain() too when put the endpoint
> then iiuc we don't even need this ordering constraint.  Though in
> virtio_iommu_detach_endpoint_from_domain() we should also need:

Yes today an EP is meant to be deleted if it is detached from any domain.

I may add virtio_iommu_detach_endpoint_from_domain in put_domain though.

> 
>   if (!ep->domain)
>     return;
> 
> Otherwise it looks good to me.  Thanks,
Thanks

Eric
> 
>> +    s->domains = g_tree_new_full((GCompareDataFunc)int_cmp,
>> +                                 NULL, NULL, virtio_iommu_put_domain);
>> +    s->endpoints = g_tree_new_full((GCompareDataFunc)int_cmp,
>> +                                   NULL, NULL, virtio_iommu_put_endpoint);
>>  }
>>  
>>  static void virtio_iommu_set_status(VirtIODevice *vdev, uint8_t status)
>> @@ -301,6 +597,14 @@ static void virtio_iommu_class_init(ObjectClass *klass, 
>> void *data)
>>      vdc->vmsd = &vmstate_virtio_iommu_device;
>>  }
>>  
>> +static void virtio_iommu_memory_region_class_init(ObjectClass *klass,
>> +                                                  void *data)
>> +{
>> +    IOMMUMemoryRegionClass *imrc = IOMMU_MEMORY_REGION_CLASS(klass);
>> +
>> +    imrc->translate = virtio_iommu_translate;
>> +}
>> +
>>  static const TypeInfo virtio_iommu_info = {
>>      .name = TYPE_VIRTIO_IOMMU,
>>      .parent = TYPE_VIRTIO_DEVICE,
>> @@ -309,9 +613,16 @@ static const TypeInfo virtio_iommu_info = {
>>      .class_init = virtio_iommu_class_init,
>>  };
>>  
>> +static const TypeInfo virtio_iommu_memory_region_info = {
>> +    .parent = TYPE_IOMMU_MEMORY_REGION,
>> +    .name = TYPE_VIRTIO_IOMMU_MEMORY_REGION,
>> +    .class_init = virtio_iommu_memory_region_class_init,
>> +};
>> +
>>  static void virtio_register_types(void)
>>  {
>>      type_register_static(&virtio_iommu_info);
>> +    type_register_static(&virtio_iommu_memory_region_info);
>>  }
>>  
>>  type_init(virtio_register_types)
>> diff --git a/include/hw/virtio/virtio-iommu.h 
>> b/include/hw/virtio/virtio-iommu.h
>> index 041f2c9390..2a2c2ecf83 100644
>> --- a/include/hw/virtio/virtio-iommu.h
>> +++ b/include/hw/virtio/virtio-iommu.h
>> @@ -28,6 +28,8 @@
>>  #define VIRTIO_IOMMU(obj) \
>>          OBJECT_CHECK(VirtIOIOMMU, (obj), TYPE_VIRTIO_IOMMU)
>>  
>> +#define TYPE_VIRTIO_IOMMU_MEMORY_REGION "virtio-iommu-memory-region"
>> +
>>  typedef struct IOMMUDevice {
>>      void         *viommu;
>>      PCIBus       *bus;
>> @@ -48,6 +50,7 @@ typedef struct VirtIOIOMMU {
>>      struct virtio_iommu_config config;
>>      uint64_t features;
>>      GHashTable *as_by_busptr;
>> +    IOMMUPciBus *iommu_pcibus_by_bus_num[PCI_BUS_MAX];
>>      PCIBus *primary_bus;
>>      GTree *domains;
>>      QemuMutex mutex;
>> -- 
>> 2.20.1
>>
>