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
--
Peter Xu
