Re: [Qemu-devel] [PATCH v2 7/7] intel_iommu: support passthrough (PT)

[Liu, Yi L]

> -----Original Message-----
> From: Peter Xu [mailto:address@hidden
> Sent: Thursday, April 20, 2017 1:40 PM
> To: Liu, Yi L <address@hidden>
> Cc: Lan, Tianyu <address@hidden>; address@hidden; Michael S .
> Tsirkin <address@hidden>; Jason Wang <address@hidden>; Marcel
> Apfelbaum <address@hidden>; David Gibson <address@hidden>;
> Tian, Kevin <address@hidden>
> Subject: Re: [Qemu-devel] [PATCH v2 7/7] intel_iommu: support passthrough (PT)
> 
> On Thu, Apr 20, 2017 at 04:55:24AM +0000, Liu, Yi L wrote:
> 
> [...]
> 
> > > > In my previous RFC patchset of fault event reporting, I registered
> > > > fault notifier when there is a VFIO group attached to VFIO
> > > > container and used the address space to check whether vIOMMU is
> > > > added. The address space is returned by vtd_host_dma_iommu().
> > > > vtd_find_add_as() initializes device's IOMMU memory region and put
> > > > it into device's address space as root memory region.
> > > > Does this make sense?
> > > >
> > > > @@ -1103,6 +1132,14 @@ static int vfio_connect_container(VFIOGroup
> > > > *group, AddressSpace *as,
> > > >          goto listener_release_exit;
> > > >      }
> > > >
> > > > +    if (memory_region_is_iommu(container->space->as->root)) {
> > >
> > > I would suggest we don't play with as->root any more. After vtd vfio
> > > series, this may not be true if passthrough mode is enabled (then
> > > the root may be switched to an system memory alias). I don't know
> > > the best way to check this, one alternative might be that we check
> > > whether
> > > container->space->as == system_memory(), it should be workable, but
> > > container->space->in
> 
> Sorry, I was meaning &address_space_memory.

correct, I got your point all the same.

> 
> > > a slightly hackish way.
> >
> > In my understanding, container->space->as->root cannot work here no
> > matter passthru-mode is enabled or not. The code here is aiming to
> > check if vIOMMU exists. After the vfio series, the vtd_dev_as->root is
> > not initialized to be a iommu MemoryRegion. Compared with checking if
> > it is system_memory(), I think adding a mechanism to get the iommu
> MemoryRegion may be a better choice. Just like the current
> pci_device_iommu_address_space().
> 
> Isn't pci_device_iommu_address_space() used to get that IOMMU memory region?

It actually returns the AddressSpace, and the AddressSpace includes a memory 
region.
It is as->root. But after adding the vfio series, through the IOMMU memory 
region
is got, but it has no iommu_ops. Just as the following code shows. That's why I 
said
even without passthru-mode, Tianyu's that code snippet is not able to get the 
correct
check.

        memory_region_init(&vtd_dev_as->root, OBJECT(s),
                           "vtd_root", UINT64_MAX);
        address_space_init(&vtd_dev_as->as, &vtd_dev_as->root, name);

> And, one thing to mention is that container->space->as is actually derived 
> from
> pci_device_iommu_address_space() (when calling vfio_get_group()).

yes, it is.

> I feel like that playing around with an IOMMU memory region is still not clear
> enough in many cases. I still feel like some day we would like an "IOMMU 
> object".
> Then, we can register non-iotlb notifiers against that IOMMU object, rather 
> than
> memory regions...
> 
haven’t think much about it. if you have any early patch, may be glad to have a 
look.

Thanks,
Yi L