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

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

Regards,
Yi L