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?
>
> 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.
Oh yes I can use PCI_DEVFN_MAX directly. Sorry.
Eric
>
> Thanks,
>
