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

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

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
>