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 >