Re: [Qemu-devel] [PATCH v3] intel_iommu: allow dynamic switch of IOMMU region

[Tian, Kevin]

> From: Alex Williamson [mailto:address@hidden
> Sent: Wednesday, January 04, 2017 5:21 AM
> 
> On Mon, 26 Dec 2016 10:45:55 +0800
> Jason Wang <address@hidden> wrote:
> 
> > On 2016年12月23日 11:26, Peter Xu wrote:
> > > On Thu, Dec 22, 2016 at 07:34:10PM +0800, Jason Wang wrote:
> > >>
> > >> On 2016年12月22日 19:04, Peter Xu wrote:
> > >>> On Thu, Dec 22, 2016 at 05:52:58PM +0800, Jason Wang wrote:
> > >>>> On 2016年12月22日 17:48, Peter Xu wrote:
> > >>>>>   /* Handle Translation Enable/Disable */
> > >>>>>   static void vtd_handle_gcmd_te(IntelIOMMUState *s, bool en)
> > >>>>>   {
> > >>>>> +    if (s->dmar_enabled == en) {
> > >>>>> +        return;
> > >>>>> +    }
> > >>>>> +
> > >>>>>       VTD_DPRINTF(CSR, "Translation Enable %s", (en ? "on" : "off"));
> > >>>>>       if (en) {
> > >>>>> @@ -1196,6 +1237,8 @@ static void vtd_handle_gcmd_te(IntelIOMMUState 
> > >>>>> *s,
> bool en)
> > >>>>>           /* Ok - report back to driver */
> > >>>>>           vtd_set_clear_mask_long(s, DMAR_GSTS_REG, VTD_GSTS_TES, 0);
> > >>>>>       }
> > >>>>> +
> > >>>>> +    vtd_switch_address_space_all(s, en);
> > >>>>>   }
> > >>>> We may need something like notifier here to tell e.g vhost to stop 
> > >>>> device
> > >>>> IOTLB. (Since it's likely this series were applied after device IOTLB
> > >>>> patches)
> > >>> Yes, I missed vhost case.
> > >>>
> > >>> To notify vhost, IMO we should be able to use memory listeners just
> > >>> like how vfio devices do (please refer to vfio_memory_listener).
> > >> Just for switching? This seems an overkill since we don't depends on it 
> > >> for
> > >> all other things. Guest should setup correct mappings by explicitly 
> > >> notify
> > >> device IOTLB. A quick glance at ATS spec, for enabling and disabling, 
> > >> looks
> > >> like it was done through enable bit of ASTctl instead of here.
> > >>
> > >> So we are probably ok here :)
> > > So we need a more general way to notify ATS about this, right? (e.g.,
> > > for future devices that support ATS as well, not only vhost)
> >
> > Yes, if we want to make ATS visible to guest. But looks like it needs
> > more e.g VFIO support for device IOTLB invalidation.
> 
> ATS is already visible for vfio-pci devices, it will be enabled if the
> host IOMMU supports ATS.  If the host IOMMU does not support ATS,
> enabling it on the device by the guest will generate unsupported
> requests.  This is yet another case, along with address width, where
> it's potentially really complicated and not amenable to hotplug to have
> a guest IOMMU.  AFAICT, there's nothing to be added to vfio for device
> iotlb invalidations, this occurs automatically between the host IOMMU
> and device when a DMA unmap occurs.  Thanks,
> 

True for current usage - use only 2nd level translation for DMA protection
purpose (IOVA->PA). There is a potential requirement of handling iotlb
invalidation through VFIO in future though - when enabling shared virtual
Memory (SVM) capability in VM where physical IOMMU will be configured
in a nested translation mode, with 1st level translation for GVA->GPA and
the 2nd level translation for GPA->HPA. It's designed in a way that guest 
1st level translation table is directly linked to context entry (all the 
addresses 
in that table are interpreted as GPA), w/o need of shadowing like for 2nd
level. In such case any IOTLB invalidation related to 1st level translation 
need be forwarded from Qemu vIOMMU to host IOMMU driver, through VFIO.

Not an immediate worry (not for this patch) but a thing we will need think 
later when adding SVM support. 

Thanks
Kevin