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

[Peter Xu <peterx@redhat.com>]

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)

> 
> >However, I think the bigger issue is we still don't have a dynamic way
> >to turn vhost DMAR on/off, right?
> 
> The API was ready for this, just issue another set_feature ioctl without
> IOMMU_PLATFORM. (But unfortunately, vhost need a small patch to make this
> work).

That'll be nice. :)

> 
> >
> >If so, we may need to re-touch all the parts to enable the dynamic
> >switching of DMA remapping - QEMU vhost, kernel vhost, and virtio on
> >the guest side... I start to doubt whether that effort will worth it
> >due to this single change, especially if this feature (dynamic on/off
> >DMA remapping) won't be used by most VMs (i.e., Linux should only turn
> >VT-d on when kernel detects it, and should never turn it off in
> >anyway).
> 
> For vhost part, the changes should be very minor, probably just:
> 
> - a patch to make sure vhost can disable device IOTLB during feature set
> - properly handling enabling bit of ATSctl in qemu (probably through an
> notifier)

Do you think I should provide another patch for the domain switch
notification (along with this one)? Or I can just postpone this patch
until DMAR series merged (after all this patch is not an urgent one).

> 
> >
> >(However I do think this is an improvement to current VT-d though)
> >
> >Thanks,
> >
> >-- peterx
> >
> 
> +1. We should try to emulate exactly what hardware does to avoid breaking
> all kinds of guest or userspace drivers.

Yes. Thanks,

-- peterx