[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: |
Peter Xu |
Subject: |
Re: [PATCH v12 05/13] virtio-iommu: Endpoint and domains structs and helpers |
Date: |
Tue, 14 Jan 2020 13:07:34 -0500 |
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?
Also, I think the name of "virtio_iommu_mr" is confusing on that it
returned explicitly a MemoryRegion however it's never been used:
(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.
Thanks,
--
Peter Xu
[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