[PATCH] intel_iommu: support snoop control

[PATCH] intel_iommu: support snoop control

[Jason Wang]

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.

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;
+    }
+
     /* Update per-address-space notifier flags */
     vtd_as->notifier_flags = new;
 
@@ -3105,6 +3112,7 @@ static Property vtd_properties[] = {
                       VTD_HOST_ADDRESS_WIDTH),
     DEFINE_PROP_BOOL("caching-mode", IntelIOMMUState, caching_mode, FALSE),
     DEFINE_PROP_BOOL("x-scalable-mode", IntelIOMMUState, scalable_mode, FALSE),
+    DEFINE_PROP_BOOL("snoop-control", IntelIOMMUState, snoop_control, false),
     DEFINE_PROP_BOOL("dma-drain", IntelIOMMUState, dma_drain, true),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -3635,7 +3643,7 @@ static void vtd_init(IntelIOMMUState *s)
     vtd_spte_rsvd_large[3] = VTD_SPTE_LPAGE_L3_RSVD_MASK(s->aw_bits,
                                                          x86_iommu->dt_supported);
 
-    if (s->scalable_mode) {
+    if (s->scalable_mode || s->snoop_control) {
         vtd_spte_rsvd[1] &= ~VTD_SPTE_SNP;
         vtd_spte_rsvd_large[2] &= ~VTD_SPTE_SNP;
         vtd_spte_rsvd_large[3] &= ~VTD_SPTE_SNP;
@@ -3666,6 +3674,10 @@ static void vtd_init(IntelIOMMUState *s)
         s->ecap |= VTD_ECAP_SMTS | VTD_ECAP_SRS | VTD_ECAP_SLTS;
     }
 
+    if (s->snoop_control) {
+        s->ecap |= VTD_ECAP_SC;
+    }
+
     vtd_reset_caches(s);
 
     /* Define registers with default values and bit semantics */
diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
index a6c788049b..1ff13b40f9 100644
--- a/hw/i386/intel_iommu_internal.h
+++ b/hw/i386/intel_iommu_internal.h
@@ -188,6 +188,7 @@
 #define VTD_ECAP_IR                 (1ULL << 3)
 #define VTD_ECAP_EIM                (1ULL << 4)
 #define VTD_ECAP_PT                 (1ULL << 6)
+#define VTD_ECAP_SC                 (1ULL << 7)
 #define VTD_ECAP_MHMV               (15ULL << 20)
 #define VTD_ECAP_SRS                (1ULL << 31)
 #define VTD_ECAP_SMTS               (1ULL << 43)
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
index 41783ee46d..3b5ac869db 100644
--- a/include/hw/i386/intel_iommu.h
+++ b/include/hw/i386/intel_iommu.h
@@ -228,6 +228,7 @@ struct IntelIOMMUState {
 
     bool caching_mode;              /* RO - is cap CM enabled? */
     bool scalable_mode;             /* RO - is Scalable Mode supported? */
+    bool snoop_control;             /* RO - is SNP filed supported? */
 
     dma_addr_t root;                /* Current root table pointer */
     bool root_scalable;             /* Type of root table (scalable or not) */
-- 
2.25.1

Re: [PATCH] intel_iommu: support snoop control

[Peter Xu]

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?  How about also mention what
this patch is used for (perhaps for some pure vdpa tests on fully emulated)?

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

Thanks,

> +
>      /* Update per-address-space notifier flags */
>      vtd_as->notifier_flags = new;

-- 
Peter Xu

Re: [PATCH] intel_iommu: support snoop control

[Jason Wang]

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
>

Re: [PATCH] intel_iommu: support snoop control

[Peter Xu]

On Mon, Feb 14, 2022 at 02:35:18PM +0800, Jason Wang 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).
> 
> > 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.

Yeah, that looks okay.  Leave it be or add some commit message would work too,
either way:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

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

Re: [PATCH] intel_iommu: support snoop control

[Peter Xu]

On Mon, Feb 14, 2022 at 02:40:20PM +0800, Jason Wang wrote:
> 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.

If not any use, then probably not at all. :-)

-- 
Peter Xu

Re: [PATCH] intel_iommu: support snoop control

[Jason Wang]

On Mon, Feb 14, 2022 at 3:05 PM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Feb 14, 2022 at 02:40:20PM +0800, Jason Wang wrote:
> > 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.
>
> If not any use, then probably not at all. :-)
>

Right. Let's leave it until there's a real user.

Thanks

> --
> Peter Xu
>