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
>