Re: [PATCH v12 05/13] virtio-iommu: Endpoint and domains structs and helpers

[Peter Xu]

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