Re: [PATCH] intel_iommu: support snoop control

[Jason Wang <jasowang@redhat.com>]

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

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