Re: [Qemu-devel] [PATCH v3 1/3] IOMMU: add VTD_CAP_CM to vIOMMU capability exposed to guest

[Huang, Kai]

On 6/8/2016 6:46 AM, Alex Williamson wrote:
> On Tue, 7 Jun 2016 17:21:06 +1200
> "Huang, Kai" <address@hidden> wrote:
>
> > On 6/7/2016 3:58 PM, Alex Williamson wrote:
> > > On Tue, 7 Jun 2016 11:20:32 +0800
> > > Peter Xu <address@hidden> wrote:
> > >
> > > > On Mon, Jun 06, 2016 at 11:02:11AM -0600, Alex Williamson wrote:
> > > > > On Mon, 6 Jun 2016 21:43:17 +0800
> > > > > Peter Xu <address@hidden> wrote:
> > > > >
> > > > > > On Mon, Jun 06, 2016 at 07:11:41AM -0600, Alex Williamson wrote:
> > > > > > > On Mon, 6 Jun 2016 13:04:07 +0800
> > > > > > > Peter Xu <address@hidden> wrote:
> > > > > > [...]
> > > > > > > > Besides the reason that there might have guests that do not support
> > > > > > > > CM=1, will there be performance considerations? When user's
> > > > > > > > configuration does not require CM capability (e.g., generic VM
> > > > > > > > configuration, without VFIO), shall we allow user to disable the CM
> > > > > > > > bit so that we can have better IOMMU performance (avoid extra and
> > > > > > > > useless invalidations)?
> > > > > > >
> > > > > > > With Alexey's proposed patch to have callback ops when the iommu
> > > > > > > notifier list adds its first entry and removes its last, any of the
> > > > > > > additional overhead to generate notifies when nobody is listening can
> > > > > > > be avoided.  These same callbacks would be the ones that need to
> > > > > > > generate a hw_error if a notifier is added while running in CM=0.
> > > > > >
> > > > > > Not familar with Alexey's patch
> > > > >
> > > > > https://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg00079.html
> > > >
> > > > Thanks for the pointer. :)
> > > >
> > > > > > , but is that for VFIO only?
> > > > >
> > > > > vfio is currently the only user of the iommu notifier, but the
> > > > > interface is generic, which is how it should (must) be.
> > > >
> > > > Yes.
> > > >
> > > > > > I mean, if
> > > > > > we configured CMbit=1, guest kernel will send invalidation request
> > > > > > every time it creates new entries (context entries, or iotlb
> > > > > > entries). Even without VFIO notifiers, guest need to trap into QEMU
> > > > > > and process the invalidation requests. This is avoidable if we are not
> > > > > > using VFIO devices at all (so no need to maintain any mappings),
> > > > > > right?
> > > > >
> > > > > CM=1 only defines that not-present and invalid entries can be cached,
> > > > > any changes to existing entries requires an invalidation regardless of
> > > > > CM.  What you're looking for sounds more like ECAP.C:
> > > >
> > > > Yes, but I guess what I was talking about is CM bit but not ECAP.C.
> > > > When we clear/replace one context entry, guest kernel will definitely
> > > > send one context entry invalidation to QEMU:
> > > >
> > > > static void domain_context_clear_one(struct intel_iommu *iommu, u8 bus, u8 
> > > > devfn)
> > > > {
> > > >         if (!iommu)
> > > >                 return;
> > > >
> > > >         clear_context_table(iommu, bus, devfn);
> > > >         iommu->flush.flush_context(iommu, 0, 0, 0,
> > > >                                            DMA_CCMD_GLOBAL_INVL);
> > > >         iommu->flush.flush_iotlb(iommu, 0, 0, 0, DMA_TLB_GLOBAL_FLUSH);
> > > > }
> > > >
> > > > ... While if we are creating a new one (like attaching a new VFIO
> > > > device?), it's an optional behavior depending on whether CM bit is
> > > > set:
> > > >
> > > > static int domain_context_mapping_one(struct dmar_domain *domain,
> > > >                                       struct intel_iommu *iommu,
> > > >                                       u8 bus, u8 devfn)
> > > > {
> > > >     ...
> > > >         /*
> > > >          * It's a non-present to present mapping. If hardware doesn't cache
> > > >          * non-present entry we only need to flush the write-buffer. If the
> > > >          * _does_ cache non-present entries, then it does so in the special
> > > >          * domain #0, which we have to flush:
> > > >          */
> > > >         if (cap_caching_mode(iommu->cap)) {
> > > >                 iommu->flush.flush_context(iommu, 0,
> > > >                                            (((u16)bus) << 8) | devfn,
> > > >                                            DMA_CCMD_MASK_NOBIT,
> > > >                                            DMA_CCMD_DEVICE_INVL);
> > > >                 iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
> > > >         } else {
> > > >                 iommu_flush_write_buffer(iommu);
> > > >         }
> > > >     ...
> > > > }
> > > >
> > > > Only if cap_caching_mode() is set (which is bit 7, the CM bit), we
> > > > will send these invalidations. What I meant is that, we should allow
> > > > user to specify the CM bit, so that when we are not using VFIO
> > > > devices, we can skip the above flush_content() and flush_iotlb()
> > > > etc... So, besides the truth that we have some guests do not support
> > > > CM bit (like Jailhouse), performance might be another consideration
> > > > point that we should allow user to specify the CM bit themselfs.
> > >
> > > I'm dubious of this, IOMMU drivers are already aware that hardware
> > > flushes are expensive and do batching to optimize it.  The queued
> > > invalidation mechanism itself is meant to allow asynchronous
> > > invalidations.  QEMU invalidating a virtual IOMMU might very well be
> > > faster than hardware.
> >
> > Do batching doesn't mean we can eliminate the IOTLB flush for mappings
> > from non-present to present, in case of CM=1, while in case CM=0 those
> > IOTLB flush are not necessary, just like the code above shows. Therefore
> > generally speaking CM=0 should have better performance than CM=1, even
> > for Qemu's vIOMMU.
> >
> > In my understanding the purpose of exposing CM=1 is to force guest do
> > IOTLB flush for each mapping change (including from non-present to
> > present) so Qemu is able to emulate each mapping change from guest
> > (correct me if I was wrong). If previous statement stands, CM=1 is
> > really a workaround for making vfio assigned devices and vIOMMU work
> > together, and unfortunately this cannot work on other vendor's IOMMU
> > without CM bit, such as AMD's IOMMU.
> >
> > So what's the requirements of making vfio assigned devices and vIOMMU
> > work together? I think it should be more helpful to implement a more
> > generic solution to monitor and emulate guest vIOMMU's page table,
> > rather than simply exposing CM=1 to guest, as it only works on intel IOMMU.
> >
> > And what do you mean asynchronous invalidations? I think the iova of the
> > changed mappings cannot be used until the mappings are invalidated. It
> > doesn't matter whether the invalidation is done via QI or register.
>
> Ok, so you're arguing that CM=0 is more efficient than CM=1 because it
> eliminates some portion of the invalidations necessary by the guest,
> while at the same time arguing for a more general solution of shadow
> page tables which would trap into the vIOMMU at every update,
> eliminating all the batching done by the guest IOMMU driver code
> attempting to reduce and consolidate the number of flushes done.  All
> of this with only speculation on what might be more efficient.  Can we
> get vIOMMU working, especially with assigned devices, before we
> speculate further?
>
> How do we expect a vIOMMU to be used in a guest?  In the case of
> emulated devices, what value does it provide?  Are emulated devices
> isolated from one another by the vIOMMU?  No.  Do we have 32-bit
> emulated devices for which DMA translation at the vIOMMU is
> significantly more efficient than bounce buffers within the guest?
> Probably not, and if we did we could just emulate 64bit devices.  So I
> assume that beyond being a development tool, our primary useful feature
> of a vIOMMU is to expose devices to guest userspace (and thus nested
> guests) via tools like vfio.  Am I wrong here?  In this use case, it's
> most efficient to run with iommu=pt in the L1 guest, which would make
> any sort of invalidations a very rare event.  Making use of vfio inside
> the L1 guest would then move a device from the static-identity domain
> in the L1 guest into a new domain where the mappings for that domain
> are also relatively static.  So I really don't see why we're trying to
> optimize the invalidation of the vIOMMU at this point.  I also have to
> believe that the hardware folks that designed VT-d believed there to be
> a performance advantage to using emulated VT-d with CM=1 versus
> shadowing all of the VT-d data structures in the hypervisor or they
> wouldn't have added this bit to the specification.  Thanks,
Hi Alex,

Sorry for jumping to this discussion suddenly. Yes I absolutely agree 
that getting vIOMMU working with assigned devices is more important 
thing than arguing on vIOMMU performance on CM bit. Actually I am very 
eager to make vIOMMU working with vfio for assigned device in guest as I 
want to try DPDK via VFIO in guest (this is the reason I searched vIOMMU 
support in Qemu and found this discussion). My concern for CM bit is not 
performance, but it is not generic way, but still it is better than 
nothing :)

Thanks,
-Kai

> Alex