| Message ID | 20200109144319.15912-6-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | VIRTIO-IOMMU device | expand |
On Thu, Jan 09, 2020 at 03:43:11PM +0100, Eric Auger wrote: [...] > +VirtIOIOMMUEndpoint *virtio_iommu_get_endpoint(VirtIOIOMMU *s, uint32_t ep_id); > +VirtIOIOMMUEndpoint *virtio_iommu_get_endpoint(VirtIOIOMMU *s, uint32_t ep_id) Is the extra definition trying to workaround the compiler warning/error? I'm not sure whether it's only me who prefer this, but again I'd really perfer we move the function into the caller patch, add "static" as needed altogether, even if that patch can be big. > +{ > + VirtIOIOMMUEndpoint *ep; > + > + ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(ep_id)); > + if (ep) { > + return ep; > + } > + if (!virtio_iommu_mr(s, ep_id)) { Could I ask when this will trigger? > + 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; > +} Thanks,
Hi Peter, On 1/13/20 9:23 PM, Peter Xu wrote: > On Thu, Jan 09, 2020 at 03:43:11PM +0100, Eric Auger wrote: > > [...] > >> +VirtIOIOMMUEndpoint *virtio_iommu_get_endpoint(VirtIOIOMMU *s, uint32_t ep_id); >> +VirtIOIOMMUEndpoint *virtio_iommu_get_endpoint(VirtIOIOMMU *s, uint32_t ep_id) > > Is the extra definition trying to workaround the compiler > warning/error? yes it does > > I'm not sure whether it's only me who prefer this, but again I'd > really perfer we move the function into the caller patch, add "static" > as needed altogether, even if that patch can be big. OK I will do that. > >> +{ >> + VirtIOIOMMUEndpoint *ep; >> + >> + ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(ep_id)); >> + if (ep) { >> + return ep; >> + } >> + if (!virtio_iommu_mr(s, ep_id)) { > > Could I ask when this will trigger? This can happen when a device is attached to a domain and its RID does not correspond to one of the devices protected by the iommu. Thanks Eric > >> + 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; >> +} > > Thanks, >
On Tue, Jan 14, 2020 at 09:51:59AM +0100, Auger Eric wrote: > Hi Peter, Hi, Eric, [...] > > > >> +{ > >> + VirtIOIOMMUEndpoint *ep; > >> + > >> + ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(ep_id)); > >> + if (ep) { > >> + return ep; > >> + } > >> + if (!virtio_iommu_mr(s, ep_id)) { > > > > Could I ask when this will trigger? > > This can happen when a device is attached to a domain and its RID does > not correspond to one of the devices protected by the iommu. So will it happen only because of a kernel driver bug? Also, I think the name of "virtio_iommu_mr" is confusing on that it returned explicitly a MemoryRegion however it's never been used: (since they're not in the same patch I'm pasting) 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 & 0xFF; dev = iommu_pci_bus->pbdev[devfn]; if (dev) { return &dev->iommu_mr; } } return NULL; } Maybe "return !!dev" would be enough, then make the return a boolean? Then we can rename it to virtio_iommu_has_device(). PS. I think we can also drop IOMMU_PCI_DEVFN_MAX (after all you even didn't use it here!) and use PCI_DEVFN_MAX, and replace 0xFF. Thanks,
Hi Peter, On 1/14/20 7:07 PM, Peter Xu wrote: > On Tue, Jan 14, 2020 at 09:51:59AM +0100, Auger Eric wrote: >> Hi Peter, > > Hi, Eric, > > [...] > >>> >>>> +{ >>>> + VirtIOIOMMUEndpoint *ep; >>>> + >>>> + ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(ep_id)); >>>> + if (ep) { >>>> + return ep; >>>> + } >>>> + if (!virtio_iommu_mr(s, ep_id)) { >>> >>> Could I ask when this will trigger? >> >> This can happen when a device is attached to a domain and its RID does >> not correspond to one of the devices protected by the iommu. > > So will it happen only because of a kernel driver bug? Yes, at the moment, because virtio_iommu_mr() only gets called on device attach to a domain. The spec says: "If the endpoint identified by endpoint doesn’t exist, the device MUST reject the request and set status to VIRTIO_IOMMU_S_NOENT" > > Also, I think the name of "virtio_iommu_mr" is confusing on that it > returned explicitly a MemoryRegion however it's never been used: I use the same prototype as for smmu_iommu_mr(). Returning the iommu mr will allow to proceed with further RID based operations like invalidations. The same logic is used in vtd_context_device_invalidate. > > (since they're not in the same patch I'm pasting) > > 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 & 0xFF; > dev = iommu_pci_bus->pbdev[devfn]; > if (dev) { > return &dev->iommu_mr; > } > } > return NULL; > } > > Maybe "return !!dev" would be enough, then make the return a boolean? > Then we can rename it to virtio_iommu_has_device(). > > PS. I think we can also drop IOMMU_PCI_DEVFN_MAX (after all you even > didn't use it here!) and use PCI_DEVFN_MAX, and replace 0xFF. well intel iommu and smmu use a similar constant (PCI_DEVFN_MAX, SMMU_PCI_DEVFN_MAX resp.). I use it in virtio_iommu_find_add_as Thanks Eric > > Thanks, >
Hi Peter, On 1/14/20 7:07 PM, Peter Xu wrote: > On Tue, Jan 14, 2020 at 09:51:59AM +0100, Auger Eric wrote: >> Hi Peter, > > Hi, Eric, > > [...] > >>> >>>> +{ >>>> + VirtIOIOMMUEndpoint *ep; >>>> + >>>> + ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(ep_id)); >>>> + if (ep) { >>>> + return ep; >>>> + } >>>> + if (!virtio_iommu_mr(s, ep_id)) { >>> >>> Could I ask when this will trigger? >> >> This can happen when a device is attached to a domain and its RID does >> not correspond to one of the devices protected by the iommu. > > So will it happen only because of a kernel driver bug? > > Also, I think the name of "virtio_iommu_mr" is confusing on that it > returned explicitly a MemoryRegion however it's never been used: > > (since they're not in the same patch I'm pasting) > > 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 & 0xFF; > dev = iommu_pci_bus->pbdev[devfn]; > if (dev) { > return &dev->iommu_mr; > } > } > return NULL; > } > > Maybe "return !!dev" would be enough, then make the return a boolean? > Then we can rename it to virtio_iommu_has_device(). > > PS. I think we can also drop IOMMU_PCI_DEVFN_MAX (after all you even > didn't use it here!) and use PCI_DEVFN_MAX, and replace 0xFF. Oh yes I can use PCI_DEVFN_MAX directly. Sorry. Eric > > Thanks, >
On Wed, Jan 15, 2020 at 02:00:22PM +0100, Auger Eric wrote: > Hi Peter, > > > On 1/14/20 7:07 PM, Peter Xu wrote: > > On Tue, Jan 14, 2020 at 09:51:59AM +0100, Auger Eric wrote: > >> Hi Peter, > > > > Hi, Eric, > > > > [...] > > > >>> > >>>> +{ > >>>> + VirtIOIOMMUEndpoint *ep; > >>>> + > >>>> + ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(ep_id)); > >>>> + if (ep) { > >>>> + return ep; > >>>> + } > >>>> + if (!virtio_iommu_mr(s, ep_id)) { > >>> > >>> Could I ask when this will trigger? > >> > >> This can happen when a device is attached to a domain and its RID does > >> not correspond to one of the devices protected by the iommu. > > > > > So will it happen only because of a kernel driver bug? > > Yes, at the moment, because virtio_iommu_mr() only gets called on device > attach to a domain. > > The spec says: > "If the endpoint identified by endpoint doesn’t exist, the device MUST > reject the request and set status to VIRTIO_IOMMU_S_NOENT" Sure. I just wanted to make sure I didn't miss anything, because I really can't see when this extra logic can help, say, right now we only have one vIOMMU at least for VT-d, so all devices will be managed by that. But yeah if that's explicitly mentioned in the spec, I'd agree we should follow that. > > > > Also, I think the name of "virtio_iommu_mr" is confusing on that it > > returned explicitly a MemoryRegion however it's never been used: > > I use the same prototype as for smmu_iommu_mr(). Returning the iommu mr > will allow to proceed with further RID based operations like invalidations. > > The same logic is used in vtd_context_device_invalidate. I'm fine with this. Let's keep virtio_iommu_mr() as you prefer. Another thing I'd like to mention is that, I don't think "the same logic is used in VT-d" matters much. If we think something is wrong (even if it's the same in VT-d), why not we fix both? :-) Thanks, > > > > > > (since they're not in the same patch I'm pasting) > > > > 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 & 0xFF; > > dev = iommu_pci_bus->pbdev[devfn]; > > if (dev) { > > return &dev->iommu_mr; > > } > > } > > return NULL; > > } > > > > Maybe "return !!dev" would be enough, then make the return a boolean? > > Then we can rename it to virtio_iommu_has_device(). > > > > PS. I think we can also drop IOMMU_PCI_DEVFN_MAX (after all you even > > didn't use it here!) and use PCI_DEVFN_MAX, and replace 0xFF. > well intel iommu and smmu use a similar constant (PCI_DEVFN_MAX, > SMMU_PCI_DEVFN_MAX resp.). I use it in virtio_iommu_find_add_as
Hi Peter, On 1/15/20 3:59 PM, Peter Xu wrote: > On Wed, Jan 15, 2020 at 02:00:22PM +0100, Auger Eric wrote: >> Hi Peter, >> >> >> On 1/14/20 7:07 PM, Peter Xu wrote: >>> On Tue, Jan 14, 2020 at 09:51:59AM +0100, Auger Eric wrote: >>>> Hi Peter, >>> >>> Hi, Eric, >>> >>> [...] >>> >>>>> >>>>>> +{ >>>>>> + VirtIOIOMMUEndpoint *ep; >>>>>> + >>>>>> + ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(ep_id)); >>>>>> + if (ep) { >>>>>> + return ep; >>>>>> + } >>>>>> + if (!virtio_iommu_mr(s, ep_id)) { >>>>> >>>>> Could I ask when this will trigger? >>>> >>>> This can happen when a device is attached to a domain and its RID does >>>> not correspond to one of the devices protected by the iommu. >> >>> >>> So will it happen only because of a kernel driver bug? >> >> Yes, at the moment, because virtio_iommu_mr() only gets called on device >> attach to a domain. >> >> The spec says: >> "If the endpoint identified by endpoint doesn’t exist, the device MUST >> reject the request and set status to VIRTIO_IOMMU_S_NOENT" > > Sure. I just wanted to make sure I didn't miss anything, because I > really can't see when this extra logic can help, say, right now we > only have one vIOMMU at least for VT-d, so all devices will be managed > by that. But yeah if that's explicitly mentioned in the spec, I'd > agree we should follow that. > >>> >>> Also, I think the name of "virtio_iommu_mr" is confusing on that it >>> returned explicitly a MemoryRegion however it's never been used: >> >> I use the same prototype as for smmu_iommu_mr(). Returning the iommu mr >> will allow to proceed with further RID based operations like invalidations. >> >> The same logic is used in vtd_context_device_invalidate. > > I'm fine with this. Let's keep virtio_iommu_mr() as you prefer. > > Another thing I'd like to mention is that, I don't think "the same > logic is used in VT-d" matters much. If we think something is wrong > (even if it's the same in VT-d), why not we fix both? :-) Sure ;-) Thank you for your time Eric > > Thanks, > >> >> >>> >>> (since they're not in the same patch I'm pasting) >>> >>> 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 & 0xFF; >>> dev = iommu_pci_bus->pbdev[devfn]; >>> if (dev) { >>> return &dev->iommu_mr; >>> } >>> } >>> return NULL; >>> } >>> >>> Maybe "return !!dev" would be enough, then make the return a boolean? >>> Then we can rename it to virtio_iommu_has_device(). >>> >>> PS. I think we can also drop IOMMU_PCI_DEVFN_MAX (after all you even >>> didn't use it here!) and use PCI_DEVFN_MAX, and replace 0xFF. >> well intel iommu and smmu use a similar constant (PCI_DEVFN_MAX, >> SMMU_PCI_DEVFN_MAX resp.). I use it in virtio_iommu_find_add_as >
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events index 10a2b120f3..15595f8cd7 100644 --- a/hw/virtio/trace-events +++ b/hw/virtio/trace-events @@ -66,3 +66,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 acc939f334..6a03c3d7ae 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -32,10 +32,29 @@ #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_sid(IOMMUDevice *dev) { return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn); @@ -65,8 +84,7 @@ static IOMMUPciBus *iommu_find_iommu_pcibus(VirtIOIOMMU *s, uint8_t bus_num) return iommu_pci_bus; } -IOMMUMemoryRegion *virtio_iommu_mr(VirtIOIOMMU *s, uint32_t sid); -IOMMUMemoryRegion *virtio_iommu_mr(VirtIOIOMMU *s, uint32_t sid) +static IOMMUMemoryRegion *virtio_iommu_mr(VirtIOIOMMU *s, uint32_t sid) { uint8_t bus_n, devfn; IOMMUPciBus *iommu_pci_bus; @@ -84,6 +102,88 @@ IOMMUMemoryRegion *virtio_iommu_mr(VirtIOIOMMU *s, uint32_t sid) 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); + ep->domain = NULL; +} + +VirtIOIOMMUEndpoint *virtio_iommu_get_endpoint(VirtIOIOMMU *s, uint32_t ep_id); +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); +} + +VirtIOIOMMUDomain *virtio_iommu_get_domain(VirtIOIOMMU *s, uint32_t domain_id); +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); + } + 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) { @@ -328,6 +428,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); @@ -369,13 +476,30 @@ static void virtio_iommu_device_realize(DeviceState *dev, Error **errp) 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); + } + 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)
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.auger@redhat.com> --- v11 -> v12: - s/uint/guint (docker-mingw@fedora build test failure) - s/viommu_/VirtIOIOMMU + CamelCase - Before creating an endpoint, check the sid is one from a device protected by the iommu, otherwise returns NULL - add assert of endpoint put to check the endpoint was detached from any domain - destroy and re-allocate the trees on reset v10 -> v11: - fixed interval_cmp (<= -> < and >= -> >) - removed unused viommu field from endpoint - removed Bharat's R-b v9 -> v10: - added Bharat's R-b 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). v4 -> v5: - initialize as->endpoint_list v3 -> v4: - new separate patch --- hw/virtio/trace-events | 4 ++ hw/virtio/virtio-iommu.c | 128 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 130 insertions(+), 2 deletions(-)