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

[Peter Xu]

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?

> 
> > 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?).

Thanks,

-- peterx