Re: [Qemu-devel] [PULL 08/41] intel_iommu: support device iotlb descriptor

[Liu, Yi L]

> -----Original Message-----
> From: Peter Xu [mailto:address@hidden
> Sent: Friday, February 17, 2017 3:00 PM
> To: Liu, Yi L <address@hidden>
> Cc: Michael S. Tsirkin <address@hidden>; address@hidden; Peter
> Maydell <address@hidden>; Eduardo Habkost
> <address@hidden>; Jason Wang <address@hidden>; Paolo Bonzini
> <address@hidden>; Richard Henderson <address@hidden>; Tian, Kevin
> <address@hidden>; Lan, Tianyu <address@hidden>
> Subject: Re: [Qemu-devel] [PULL 08/41] intel_iommu: support device iotlb
> descriptor
> 
> On Fri, Feb 17, 2017 at 06:36:41AM +0000, Liu, Yi L wrote:
> > > -----Original Message-----
> > > From: Peter Xu [mailto:address@hidden
> > > Sent: Friday, February 17, 2017 11:26 AM
> > > To: Liu, Yi L <address@hidden>
> > > Cc: Michael S. Tsirkin <address@hidden>; address@hidden;
> > > Peter Maydell <address@hidden>; Eduardo Habkost
> > > <address@hidden>; Jason Wang <address@hidden>; Paolo
> > > Bonzini <address@hidden>; Richard Henderson <address@hidden>;
> > > Tian, Kevin <address@hidden>; Lan, Tianyu
> > > <address@hidden>
> > > Subject: Re: [Qemu-devel] [PULL 08/41] intel_iommu: support device
> > > iotlb descriptor
> > >
> > > On Thu, Feb 16, 2017 at 05:36:06AM +0000, Liu, Yi L wrote:
> > > > > -----Original Message-----
> > > > > From: Qemu-devel
> > > > > [mailto:address@hidden
> > > > > On Behalf Of Michael S. Tsirkin
> > > > > Sent: Tuesday, January 10, 2017 1:40 PM
> > > > > To: address@hidden
> > > > > Cc: Peter Maydell <address@hidden>; Eduardo Habkost
> > > > > <address@hidden>; Jason Wang <address@hidden>; Peter
> > > > > Xu <address@hidden>; Paolo Bonzini <address@hidden>;
> > > > > Richard Henderson <address@hidden>
> > > > > Subject: [Qemu-devel] [PULL 08/41] intel_iommu: support device
> > > > > iotlb descriptor
> > > > >
> > > > > From: Jason Wang <address@hidden>
> > > > >
> > > > > This patch enables device IOTLB support for intel iommu. The
> > > > > major work is to implement QI device IOTLB descriptor processing
> > > > > and notify the device through iommu notifier.
> > > > >
> > > > Hi Jason/Michael,
> > > >
> > > > Recently Peter Xu's patch also touched intel-iommu emulation. His
> > > > patch shadows second-level page table by capturing iotlb flush
> > > > from guest. It would result in page table updating in host. Does
> > > > this patch also use the same map/umap API provided by VFIO? If it
> > > > is, then I think it
> > > would also update page table in host.
> > >
> > > I haven't considered complex scenarios, but if simply we have a VM
> > > with one vhost device and one vfio-pci device, imho that should not
> > > be a problem - device iotlb is targeting SID rather than DOMAIN. So
> > > device iotlb invalidations for vhost will be sent to vhost device only.
> >
> > Peter, I think for assigned device, all guest iotlb flush should be
> > translated to be targeting device in the scope of host. Besides the
> > scenario which has vhost and vfio-pci device at the same time, how
> > about only having vfio-pci device and this device has ATS support. Then 
> > there
> should be device-iotlb flushing.
> > With this patch and your patch, it would also introduce two flushing.
> 
> Hmm possibly. I'm still not quite familiar with ATS, but here what we need to
> do may be that we forward these device-iotlb invalidations to the hardware
> below, instead of sending UNMAP notifies, right?
yes, that wouldn’t result in duplicate page table updating in host.

> 
> >
> > > However, vhost may receive two invalidations here, but it won't
> > > matter much since vhost is just flushing caches twice.
> >
> > yeah, so far I didn’t see functional issue, may just reduce
> > performance^_^
> >
> > > > It looks to be
> > > > a duplicate update. Pls refer to the following snapshot captured
> > > > from section 6.5.2.5 of vtd spec.
> > > >
> > > > "Since translation requests from a device may be serviced by
> > > > hardware from the IOTLB, software must always request IOTLB
> > > > invalidation
> > > > (iotlb_inv_dsc) before requesting corresponding Device-TLB
> > > > (dev_tlb_inv_dsc) invalidation."
> > > >
> > > > Maybe for device-iotlb, we need a separate API which just pass
> > > > down the invalidate info without updating page table. Any thoughts?
> > >
> > > Now imho I slightly prefer just use the current UNMAP notifier type
> > > even for device iotlb device. But, we may need to do one more check
> > > that we skip sending general iotlb invalidations to ATS enabled
> > > devices like vhost, to avoid duplicated cache flushing. From
> > > virt-svm side, do we have specific requirement to introduce a new flag 
> > > for it?
> > I think it is a practical solution to do such a check to avoid duplicate 
> > flushing.
> >
> > For virt-svm, requirement is a little bit different since it's not
> > shadowing any guest page table. It needs to shadow invalidate
> > operations. So virt-svm will not use MAP/UNMAP notifier. Instead, it
> > may require notifier which passdown invalidate info and then submit
> invalidation directly.(no page table updating in host).
> 
> Again, just want to know whether my above understanding is correct. If so,
> instead of introducing a new flag, maybe we just need to enhance current
> vtd_process_device_iotlb_desc() to behave differently depending on which
> device the SID belongs to. E.g.:
> 
> (1) for emulated devices (virtio-pci/vhost is one of them), we pass
>     down using a UNMAP, or say, do a cache flush
> 
> (2) for assigned devices, we pass it down to hardware by some way
> 
> I don't know whether there'll be a (3) though. Also, I don't know the best 
> way to
> achieve (2) (new vfio driver interface?).
yes. we can think about it. Tianyu and me are preparing RFC patch for virt-svm,
we would have more discussion on this part.

Regards,
Yi L