[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v12 05/13] virtio-iommu: Endpoint and domains structs and hel
From: |
Auger Eric |
Subject: |
Re: [PATCH v12 05/13] virtio-iommu: Endpoint and domains structs and helpers |
Date: |
Wed, 15 Jan 2020 14:00:22 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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,
>
[PATCH v12 06/13] virtio-iommu: Implement attach/detach command, Eric Auger, 2020/01/09
[PATCH v12 07/13] virtio-iommu: Implement map/unmap, Eric Auger, 2020/01/09
[PATCH v12 08/13] virtio-iommu: Implement translate, Eric Auger, 2020/01/09
[PATCH v12 09/13] virtio-iommu: Implement fault reporting, Eric Auger, 2020/01/09