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

[Auger Eric <eric.auger@redhat.com>]

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,
>