| Message ID | 20220214060346.72455-1-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | intel_iommu: support snoop control | expand |
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;
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 >
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,
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 > >
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. :-)
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 >
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) */
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(-)