Re: [PATCH] intel_iommu: support snoop control

[Jason Wang]

On Mon, Feb 14, 2022 at 2:35 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Feb 14, 2022 at 2:31 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Mon, Feb 14, 2022 at 02:03:46PM +0800, Jason Wang wrote:
> > > SC is required for some kernel features like vhost-vDPA. So this patch
> > > implements basic SC feature. The idea is pretty simple, for software
> > > emulated DMA it would be always coherent. In this case we can simple
> > > advertise ECAP_SC bit. For VFIO and vhost, thing will be more much
> > > complicated, so this patch simply fail the IOMMU notifier
> > > registration.
> >
> > Could we spell out which vhost branch won't work?
>
> For vhost, it should work but the problem is that we need to introduce
> more logics to demonstrate the notifier ability (e.g a dedicated
> notifier flag for cc).

Or we can do tricks like checking IOMMU_NOTIFIER_DEVIOTLB_UNMAP and
assume it's vhost.

But I'm not sure it's worth it.

Thanks

>
> > How about also mention what
> > this patch is used for (perhaps for some pure vdpa tests on fully emulated)?
>
> That's fine, the main use case so far is to test vDPA in L1 guest.
>
> >
> > >
> > > In the future, we may want to have a dedicated notifiers flag or
> > > similar mechanism to demonstrate the coherency so VFIO could advertise
> > > that if it has VFIO_DMA_CC_IOMMU, for vhost kernel backend we don't
> > > need that since it's a software backend.
> > >
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  hw/i386/intel_iommu.c          | 14 +++++++++++++-
> > >  hw/i386/intel_iommu_internal.h |  1 +
> > >  include/hw/i386/intel_iommu.h  |  1 +
> > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index 5b865ac08c..5fa8e361b8 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -3022,6 +3022,13 @@ static int 
> > > vtd_iommu_notify_flag_changed(IOMMUMemoryRegion *iommu,
> > >      VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, 
> > > iommu);
> > >      IntelIOMMUState *s = vtd_as->iommu_state;
> > >
> > > +    /* TODO: add support for VFIO and vhost users */
> > > +    if (s->snoop_control) {
> > > +        error_setg_errno(errp, -ENOTSUP,
> > > +                         "Snoop Control with vhost or VFIO is not 
> > > supported");
> > > +        return -ENOTSUP;
> > > +    }
> >
> > IIUC this will also fail things like e.g. vhost-kernel but afaiu that can be
> > fully emulated too.  That's expected, am I right?
>
> Yes, I try to make the patch as simple as possible, so VFIO or any
> kind of vhost won't work.
>
> Thanks
>
> >
> > Thanks,
> >
> > > +
> > >      /* Update per-address-space notifier flags */
> > >      vtd_as->notifier_flags = new;
> >
> > --
> > Peter Xu
> >