> -----Original Message-----
> From: Peter Xu [mailto:pet...@redhat.com]
> Sent: Thursday, April 20, 2017 1:40 PM
> To: Liu, Yi L <yi.l....@intel.com>
> Cc: Lan, Tianyu <tianyu....@intel.com>; qemu-devel@nongnu.org; Michael S .
> Tsirkin <m...@redhat.com>; Jason Wang <jasow...@redhat.com>; Marcel
> Apfelbaum <mar...@redhat.com>; David Gibson <da...@gibson.dropbear.id.au>;
> Tian, Kevin <kevin.t...@intel.com>
> 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
