Hi Bharat, On 11/23/18 10:14 AM, Bharat Bhushan wrote: > Hi Eric, > >> -----Original Message----- >> From: Auger Eric <eric.au...@redhat.com> >> Sent: Friday, November 23, 2018 1:23 PM >> To: Bharat Bhushan <bharat.bhus...@nxp.com>; >> eric.auger....@gmail.com; qemu-devel@nongnu.org; qemu- >> a...@nongnu.org; peter.mayd...@linaro.org; m...@redhat.com; jean- >> philippe.bruc...@arm.com >> Cc: t...@semihalf.com; kevin.t...@intel.com; pet...@redhat.com >> Subject: Re: [Qemu-devel] [RFC v9 06/17] virtio-iommu: Endpoint and >> domains structs and helpers >> >> Hi Bharat, >> >> On 11/23/18 7:38 AM, Bharat Bhushan wrote: >>> Hi Eric, >>> >>>> -----Original Message----- >>>> From: Eric Auger <eric.au...@redhat.com> >>>> Sent: Thursday, November 22, 2018 10:45 PM >>>> To: eric.auger....@gmail.com; eric.au...@redhat.com; qemu- >>>> de...@nongnu.org; qemu-...@nongnu.org; peter.mayd...@linaro.org; >>>> m...@redhat.com; jean-philippe.bruc...@arm.com >>>> Cc: kevin.t...@intel.com; t...@semihalf.com; Bharat Bhushan >>>> <bharat.bhus...@nxp.com>; pet...@redhat.com >>>> Subject: [RFC v9 06/17] virtio-iommu: Endpoint and domains structs >>>> and helpers >>>> >>>> This patch introduce domain and endpoint internal datatypes. Both are >>>> stored in RB trees. The domain owns a list of endpoints attached to it. >>>> >>>> Helpers to get/put end points and domains are introduced. >>>> get() helpers will become static in subsequent patches. >>>> >>>> Signed-off-by: Eric Auger <eric.au...@redhat.com> >>>> >>>> --- >>>> >>>> v6 -> v7: >>>> - on virtio_iommu_find_add_as the bus number computation may >>>> not be finalized yet so we cannot register the EPs at that time. >>>> Hence, let's remove the get_endpoint and also do not use the >>>> bus number for building the memory region name string (only >>>> used for debug though). >>> >>> Endpoint registration from virtio_iommu_find_add_as to PROBE request. >>> It is mentioned that " the bus number computation may not be finalized ". >> Can you please give some more information. >>> I am asking this because from vfio perspective translate/replay will be >> called much before the PROBE request and endpoint needed to be >> registered by that time. >> When from virtio_iommu_find_add() gets called, there are cases where the >> BDF of the device is not yet computed, typically if the EP is plugged on a >> secondary bus. That's why I postponed the registration. Do you have idea >> When you would need the registration to happen? > > We want the endpoint registeration before replay/translate() is called for > both virtio/vfio and I am trying to understand when we should register the > endpoint. > I am looking at amd iommu, there pci_setup_iommu() provides the callback > function which is called with "devfn" from pci_device_iommu_address_space(). > Are you saying that devfn provided by pci_device_iommu_address_space() can be > invalid? pci_bus_num() can return something wrong if called from virtio_iommu_find_add_as.
That's what on smmuv3 (same on Intel), we use a separate smmu_find_smmu_pcibus() called later. See comment for smmu_find_smmu_pcibus. Thanks Eric > > Thanks > -Bharat > >> >> Thanks >> >> Eric >>> >>> >>> Thanks >>> -Bharat >>> >>>> >>>> v4 -> v5: >>>> - initialize as->endpoint_list >>>> >>>> v3 -> v4: >>>> - new separate patch >>>> --- >>>> hw/virtio/trace-events | 4 ++ >>>> hw/virtio/virtio-iommu.c | 125 >>>> ++++++++++++++++++++++++++++++++++++++- >>>> 2 files changed, 128 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events index >>>> 9270b0463e..4b15086872 100644 >>>> --- a/hw/virtio/trace-events >>>> +++ b/hw/virtio/trace-events >>>> @@ -61,3 +61,7 @@ virtio_iommu_map(uint32_t domain_id, uint64_t >>>> virt_start, uint64_t virt_end, uin 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 >>>> dead062baf..1b9c3ba416 100644 >>>> --- a/hw/virtio/virtio-iommu.c >>>> +++ b/hw/virtio/virtio-iommu.c >>>> @@ -33,20 +33,124 @@ >>>> #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 viommu_domain { >>>> + uint32_t id; >>>> + GTree *mappings; >>>> + QLIST_HEAD(, viommu_endpoint) endpoint_list; } viommu_domain; >>>> + >>>> +typedef struct viommu_endpoint { >>>> + uint32_t id; >>>> + viommu_domain *domain; >>>> + QLIST_ENTRY(viommu_endpoint) next; >>>> + VirtIOIOMMU *viommu; >>>> +} viommu_endpoint; >>>> + >>>> +typedef struct viommu_interval { >>>> + uint64_t low; >>>> + uint64_t high; >>>> +} viommu_interval; >>>> + >>>> static inline uint16_t virtio_iommu_get_sid(IOMMUDevice *dev) { >>>> return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn); } >>>> >>>> +static gint interval_cmp(gconstpointer a, gconstpointer b, gpointer >>>> +user_data) { >>>> + viommu_interval *inta = (viommu_interval *)a; >>>> + viommu_interval *intb = (viommu_interval *)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(viommu_endpoint >>>> +*ep) { >>>> + QLIST_REMOVE(ep, next); >>>> + ep->domain = NULL; >>>> +} >>>> + >>>> +viommu_endpoint *virtio_iommu_get_endpoint(VirtIOIOMMU *s, >>>> uint32_t >>>> +ep_id); viommu_endpoint *virtio_iommu_get_endpoint(VirtIOIOMMU >> *s, >>>> +uint32_t ep_id) { >>>> + viommu_endpoint *ep; >>>> + >>>> + ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(ep_id)); >>>> + if (ep) { >>>> + return ep; >>>> + } >>>> + ep = g_malloc0(sizeof(*ep)); >>>> + ep->id = ep_id; >>>> + ep->viommu = s; >>>> + 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) { >>>> + viommu_endpoint *ep = (viommu_endpoint *)data; >>>> + >>>> + if (ep->domain) { >>>> + virtio_iommu_detach_endpoint_from_domain(ep); >>>> + g_tree_unref(ep->domain->mappings); >>>> + } >>>> + >>>> + trace_virtio_iommu_put_endpoint(ep->id); >>>> + g_free(ep); >>>> +} >>>> + >>>> +viommu_domain *virtio_iommu_get_domain(VirtIOIOMMU *s, >> uint32_t >>>> +domain_id); viommu_domain >> *virtio_iommu_get_domain(VirtIOIOMMU >>>> *s, >>>> +uint32_t domain_id) { >>>> + viommu_domain *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) { >>>> + viommu_domain *domain = (viommu_domain *)data; >>>> + viommu_endpoint *iter, *tmp; >>>> + >>>> + QLIST_FOREACH_SAFE(iter, &domain->endpoint_list, next, tmp) { >>>> + virtio_iommu_detach_endpoint_from_domain(iter); >>>> + } >>>> + g_tree_destroy(domain->mappings); >>>> + 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) { >>>> @@ -60,7 +164,7 @@ static AddressSpace >>>> *virtio_iommu_find_add_as(PCIBus *bus, void *opaque, >>>> if (!sdev) { >>>> char *name = g_strdup_printf("%s-%d-%d", >>>> TYPE_VIRTIO_IOMMU_MEMORY_REGION, >>>> - pci_bus_num(bus), devfn); >>>> + mr_index++, devfn); >>>> sdev = sbus->pbdev[devfn] = g_malloc0(sizeof(IOMMUDevice)); >>>> >>>> sdev->viommu = s; >>>> @@ -75,6 +179,7 @@ static AddressSpace >>>> *virtio_iommu_find_add_as(PCIBus *bus, void *opaque, >>>> UINT64_MAX); >>>> address_space_init(&sdev->as, >>>> MEMORY_REGION(&sdev->iommu_mr), >>>> TYPE_VIRTIO_IOMMU); >>>> + g_free(name); >>>> } >>>> >>>> return &sdev->as; >>>> @@ -332,6 +437,13 @@ static const VMStateDescription >>>> vmstate_virtio_iommu_device = { >>>> }, >>>> }; >>>> >>>> +static gint int_cmp(gconstpointer a, gconstpointer b, gpointer >>>> +user_data) { >>>> + uint ua = GPOINTER_TO_UINT(a); >>>> + uint 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); @@ -356,6 +468,8 @@ >>>> static void virtio_iommu_device_realize(DeviceState *dev, Error **errp) >>>> virtio_add_feature(&s->features, VIRTIO_IOMMU_F_MAP_UNMAP); >>>> virtio_add_feature(&s->features, VIRTIO_IOMMU_F_BYPASS); >>>> >>>> + qemu_mutex_init(&s->mutex); >>>> + >>>> memset(s->as_by_bus_num, 0, sizeof(s->as_by_bus_num)); >>>> s->as_by_busptr = g_hash_table_new(NULL, NULL); >>>> >>>> @@ -364,11 +478,20 @@ static void >>>> virtio_iommu_device_realize(DeviceState *dev, Error **errp) >>>> } else { >>>> error_setg(errp, "VIRTIO-IOMMU is not attached to any PCI bus!"); >>>> } >>>> + >>>> + 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_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); >>>> } >>>> -- >>>> 2.17.2 >>> >>>