[PATCH v1 0/4] vfio/iommu: Flag to allow userspace to set DMA buffers system cacheable

[PATCH v1 0/4] vfio/iommu: Flag to allow userspace to set DMA buffers system cacheable

[Manoj Vishwanathan]

Hi maintainers,

This RFC patch introduces the ability for userspace to control whether
device (DMA) buffers are marked as cacheable, enabling them to utilize
the system-level cache.

The specific changes made in this patch are:

* Introduce a new flag in `include/linux/iommu.h`: 
    * `IOMMU_SYS_CACHE` -  Indicates if the associated page should be cached in the system's cache hierarchy.
* Add `VFIO_DMA_MAP_FLAG_SYS_CACHE` to `include/uapi/linux/vfio.h`:
    * Allows userspace to set the cacheable attribute to PTE when mapping DMA regions using the VFIO interface.
* Update `vfio_iommu_type1.c`:
    * Handle the `VFIO_DMA_MAP_FLAG_SYS_CACHE` flag during DMA mapping operations.
    * Set the `IOMMU_SYS_CACHE` flag in the IOMMU page table entry if the `VFIO_DMA_MAP_FLAG_SYS_CACHE` is set.

* arm/smmu/io-pgtable-arm: Set the MAIR for SYS_CACHE

The reasoning behind these changes is to provide userspace with finer-grained control over memory access patterns for devices,
potentially improving performance in scenarios where caching is beneficial. We saw in some of the use cases where the buffers were
for transient data ( in and out without processing).

I have tested this patch on certain arm64 machines and observed the following:

* There is 14-21% improvement in jitter measurements, where the buffer on System Level Cache vs DDR buffers
* There was not much of an improvement in latency in the diration of the tests that I have tried.

I am open to feedback and suggestions for further improvements. Please let me know if you have any questions or concerns.
Also, I am working on adding a check in the VFIO layer to ensure that if there is no architecture supported implementation for
sys cache, if should not apply them.

Thanks,
Manoj Vishwanathan

Manoj Vishwanathan (4):
  iommu: Add IOMMU_SYS_CACHE flag for system cache control
  iommu/io-pgtable-arm: Force outer cache for page-level MAIR via user
    flag
  vfio: Add VFIO_DMA_MAP_FLAG_SYS_CACHE to control device access to
    system cache
  vfio/type1: Add support for VFIO_DMA_MAP_FLAG_SYS_CACHE

 drivers/iommu/io-pgtable-arm.c  | 3 +++
 drivers/vfio/vfio_iommu_type1.c | 5 +++--
 include/linux/iommu.h           | 6 ++++++
 include/uapi/linux/vfio.h       | 1 +
 4 files changed, 13 insertions(+), 2 deletions(-)

-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v1 1/4] iommu: Add IOMMU_SYS_CACHE flag for system cache control

[Manoj Vishwanathan]

This flag indicates whether the associated page should be cached in the
system's cache hierarchy.

Signed-off-by: Manoj Vishwanathan <manojvishy@google.com>
---
 include/linux/iommu.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 04cbdae0052e..ca895c83c24f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -31,6 +31,12 @@
  */
 #define IOMMU_PRIV	(1 << 5)
 
+/*
+ * Flag to indicate whether the associated page should be cached in the
+ * system's cache hierarchy.
+ */
+#define IOMMU_SYS_CACHE (1 << 6)
+
 struct iommu_ops;
 struct iommu_group;
 struct bus_type;
-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v1 2/4] iommu/io-pgtable-arm: Force outer cache for page-level MAIR via user flag

[Manoj Vishwanathan]

This change introduces a user-accessible flag that allows controlling
the system-level outer cache behavior for mapped buffers.

By setting this flag, the page-level MAIR attribute will be forced to
use the outer cache, potentially improving performance for frequently
accessed data.

Signed-off-by: Manoj Vishwanathan <manojvishy@google.com>
---
 drivers/iommu/io-pgtable-arm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/io-pgtable-arm.c b/drivers/iommu/io-pgtable-arm.c
index f5d9fd1f45bf..31dd6203db96 100644
--- a/drivers/iommu/io-pgtable-arm.c
+++ b/drivers/iommu/io-pgtable-arm.c
@@ -471,6 +471,9 @@ static arm_lpae_iopte arm_lpae_prot_to_pte(struct arm_lpae_io_pgtable *data,
 		else if (prot & IOMMU_CACHE)
 			pte |= (ARM_LPAE_MAIR_ATTR_IDX_CACHE
 				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
+		else if (prot & IOMMU_SYS_CACHE)
+			pte |= (ARM_LPAE_MAIR_ATTR_IDX_INC_OCACHE
+				<< ARM_LPAE_PTE_ATTRINDX_SHIFT);
 	}
 
 	/*
-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v1 3/4] vfio: Add VFIO_DMA_MAP_FLAG_SYS_CACHE to control device access to system cache

[Manoj Vishwanathan]

This new flag allows controlling whether a device buffers should be mapped to
sys cache for mapped DMA regions.

Signed-off-by: Manoj Vishwanathan <manojvishy@google.com>
---
 include/uapi/linux/vfio.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 2b68e6cdf190..494611a046d3 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1560,6 +1560,7 @@ struct vfio_iommu_type1_dma_map {
 #define VFIO_DMA_MAP_FLAG_READ (1 << 0)		/* readable from device */
 #define VFIO_DMA_MAP_FLAG_WRITE (1 << 1)	/* writable from device */
 #define VFIO_DMA_MAP_FLAG_VADDR (1 << 2)
+#define VFIO_DMA_MAP_FLAG_SYS_CACHE (1 << 6)	/* Sys Cache able from device */
 	__u64	vaddr;				/* Process virtual address */
 	__u64	iova;				/* IO virtual address */
 	__u64	size;				/* Size of mapping (bytes) */
-- 
2.46.0.295.g3b9ea8a38a-goog

[PATCH v1 4/4] vfio/type1: Add support for VFIO_DMA_MAP_FLAG_SYS_CACHE

[Manoj Vishwanathan]

Introducing the VFIO_DMA_MAP_FLAG_SYS_CACHE flag to control
whether mapped DMA regions are cached in the system cache.

Signed-off-by: Manoj Vishwanathan <manojvishy@google.com>
---
 drivers/vfio/vfio_iommu_type1.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 0960699e7554..c84bb6c8b12f 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1562,7 +1562,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 		prot |= IOMMU_WRITE;
 	if (map->flags & VFIO_DMA_MAP_FLAG_READ)
 		prot |= IOMMU_READ;
-
+	if (map->flags & VFIO_DMA_MAP_FLAG_SYS_CACHE)
+		prot |= IOMMU_SYS_CACHE;
 	if ((prot && set_vaddr) || (!prot && !set_vaddr))
 		return -EINVAL;
 
@@ -2815,7 +2816,7 @@ static int vfio_iommu_type1_map_dma(struct vfio_iommu *iommu,
 	struct vfio_iommu_type1_dma_map map;
 	unsigned long minsz;
 	uint32_t mask = VFIO_DMA_MAP_FLAG_READ | VFIO_DMA_MAP_FLAG_WRITE |
-			VFIO_DMA_MAP_FLAG_VADDR;
+			VFIO_DMA_MAP_FLAG_SYS_CACHE | VFIO_DMA_MAP_FLAG_VADDR;
 
 	minsz = offsetofend(struct vfio_iommu_type1_dma_map, size);
 
-- 
2.46.0.295.g3b9ea8a38a-goog

Re: [PATCH v1 0/4] vfio/iommu: Flag to allow userspace to set DMA buffers system cacheable

[Alex Williamson]

On Mon, 26 Aug 2024 07:16:37 +0000
Manoj Vishwanathan <manojvishy@google.com> wrote:

> Hi maintainers,
> 
> This RFC patch introduces the ability for userspace to control whether
> device (DMA) buffers are marked as cacheable, enabling them to utilize
> the system-level cache.
> 
> The specific changes made in this patch are:
> 
> * Introduce a new flag in `include/linux/iommu.h`: 
>     * `IOMMU_SYS_CACHE` -  Indicates if the associated page should be cached in the system's cache hierarchy.
> * Add `VFIO_DMA_MAP_FLAG_SYS_CACHE` to `include/uapi/linux/vfio.h`:
>     * Allows userspace to set the cacheable attribute to PTE when mapping DMA regions using the VFIO interface.
> * Update `vfio_iommu_type1.c`:
>     * Handle the `VFIO_DMA_MAP_FLAG_SYS_CACHE` flag during DMA mapping operations.
>     * Set the `IOMMU_SYS_CACHE` flag in the IOMMU page table entry if
> the `VFIO_DMA_MAP_FLAG_SYS_CACHE` is set.

We intend to eventually drop vfio type1 in favor of using IOMMUFD,
therefore all new type1 features need to first be available in IOMMUFD.
Once there we may consider use cases for providing the feature in the
legacy type1 interface and the IOMMUFD compatibility interface.  Thanks,

Alex

> * arm/smmu/io-pgtable-arm: Set the MAIR for SYS_CACHE
> 
> The reasoning behind these changes is to provide userspace with
> finer-grained control over memory access patterns for devices,
> potentially improving performance in scenarios where caching is
> beneficial. We saw in some of the use cases where the buffers were
> for transient data ( in and out without processing).
> 
> I have tested this patch on certain arm64 machines and observed the
> following:
> 
> * There is 14-21% improvement in jitter measurements, where the
> buffer on System Level Cache vs DDR buffers
> * There was not much of an improvement in latency in the diration of
> the tests that I have tried.
> 
> I am open to feedback and suggestions for further improvements.
> Please let me know if you have any questions or concerns. Also, I am
> working on adding a check in the VFIO layer to ensure that if there
> is no architecture supported implementation for sys cache, if should
> not apply them.
> 
> Thanks,
> Manoj Vishwanathan
> 
> Manoj Vishwanathan (4):
>   iommu: Add IOMMU_SYS_CACHE flag for system cache control
>   iommu/io-pgtable-arm: Force outer cache for page-level MAIR via user
>     flag
>   vfio: Add VFIO_DMA_MAP_FLAG_SYS_CACHE to control device access to
>     system cache
>   vfio/type1: Add support for VFIO_DMA_MAP_FLAG_SYS_CACHE
> 
>  drivers/iommu/io-pgtable-arm.c  | 3 +++
>  drivers/vfio/vfio_iommu_type1.c | 5 +++--
>  include/linux/iommu.h           | 6 ++++++
>  include/uapi/linux/vfio.h       | 1 +
>  4 files changed, 13 insertions(+), 2 deletions(-)
> 

Re: [PATCH v1 0/4] vfio/iommu: Flag to allow userspace to set DMA buffers system cacheable

[Manoj Vishwanathan]

On Mon, Aug 26, 2024 at 10:04 AM Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> On Mon, 26 Aug 2024 07:16:37 +0000
> Manoj Vishwanathan <manojvishy@google.com> wrote:
>
> > Hi maintainers,
> >
> > This RFC patch introduces the ability for userspace to control whether
> > device (DMA) buffers are marked as cacheable, enabling them to utilize
> > the system-level cache.
> >
> > The specific changes made in this patch are:
> >
> > * Introduce a new flag in `include/linux/iommu.h`:
> >     * `IOMMU_SYS_CACHE` -  Indicates if the associated page should be cached in the system's cache hierarchy.
> > * Add `VFIO_DMA_MAP_FLAG_SYS_CACHE` to `include/uapi/linux/vfio.h`:
> >     * Allows userspace to set the cacheable attribute to PTE when mapping DMA regions using the VFIO interface.
> > * Update `vfio_iommu_type1.c`:
> >     * Handle the `VFIO_DMA_MAP_FLAG_SYS_CACHE` flag during DMA mapping operations.
> >     * Set the `IOMMU_SYS_CACHE` flag in the IOMMU page table entry if
> > the `VFIO_DMA_MAP_FLAG_SYS_CACHE` is set.
>
> We intend to eventually drop vfio type1 in favor of using IOMMUFD,
> therefore all new type1 features need to first be available in IOMMUFD.
> Once there we may consider use cases for providing the feature in the
> legacy type1 interface and the IOMMUFD compatibility interface.  Thanks,
>
> Alex
Thank You, Alex! I will redirect this patch to iommufd in the next version.
> > * arm/smmu/io-pgtable-arm: Set the MAIR for SYS_CACHE
> >
> > The reasoning behind these changes is to provide userspace with
> > finer-grained control over memory access patterns for devices,
> > potentially improving performance in scenarios where caching is
> > beneficial. We saw in some of the use cases where the buffers were
> > for transient data ( in and out without processing).
> >
> > I have tested this patch on certain arm64 machines and observed the
> > following:
> >
> > * There is 14-21% improvement in jitter measurements, where the
> > buffer on System Level Cache vs DDR buffers
> > * There was not much of an improvement in latency in the diration of
> > the tests that I have tried.
> >
> > I am open to feedback and suggestions for further improvements.
> > Please let me know if you have any questions or concerns. Also, I am
> > working on adding a check in the VFIO layer to ensure that if there
> > is no architecture supported implementation for sys cache, if should
> > not apply them.
> >
> > Thanks,
> > Manoj Vishwanathan
> >
> > Manoj Vishwanathan (4):
> >   iommu: Add IOMMU_SYS_CACHE flag for system cache control
> >   iommu/io-pgtable-arm: Force outer cache for page-level MAIR via user
> >     flag
> >   vfio: Add VFIO_DMA_MAP_FLAG_SYS_CACHE to control device access to
> >     system cache
> >   vfio/type1: Add support for VFIO_DMA_MAP_FLAG_SYS_CACHE
> >
> >  drivers/iommu/io-pgtable-arm.c  | 3 +++
> >  drivers/vfio/vfio_iommu_type1.c | 5 +++--
> >  include/linux/iommu.h           | 6 ++++++
> >  include/uapi/linux/vfio.h       | 1 +
> >  4 files changed, 13 insertions(+), 2 deletions(-)
> >
>

Re: [PATCH v1 0/4] vfio/iommu: Flag to allow userspace to set DMA buffers system cacheable

[Jason Gunthorpe]

On Mon, Aug 26, 2024 at 11:04:47AM -0600, Alex Williamson wrote:
> On Mon, 26 Aug 2024 07:16:37 +0000
> Manoj Vishwanathan <manojvishy@google.com> wrote:
> 
> > Hi maintainers,
> > 
> > This RFC patch introduces the ability for userspace to control whether
> > device (DMA) buffers are marked as cacheable, enabling them to utilize
> > the system-level cache.
> > 
> > The specific changes made in this patch are:
> > 
> > * Introduce a new flag in `include/linux/iommu.h`: 
> >     * `IOMMU_SYS_CACHE` -  Indicates if the associated page should be cached in the system's cache hierarchy.
> > * Add `VFIO_DMA_MAP_FLAG_SYS_CACHE` to `include/uapi/linux/vfio.h`:

You'll need a much better description of what this is supposed to do
when you resend it.

IOMMU_CACHE already largely means that pages should be cached.

So I don't know what ARM's "INC_OCACHE" actually is doing. Causing
writes to land in a cache somewhere in hierarchy? Something platform
specific? I have no idea. By your description it sounds similar to the
x86 data placement stuff, whatever that was called, and the more
modern TPH approach.

Jason

Re: [PATCH v1 0/4] vfio/iommu: Flag to allow userspace to set DMA buffers system cacheable

[Robin Murphy]

On 27/08/2024 12:17 am, Jason Gunthorpe wrote:
> On Mon, Aug 26, 2024 at 11:04:47AM -0600, Alex Williamson wrote:
>> On Mon, 26 Aug 2024 07:16:37 +0000
>> Manoj Vishwanathan <manojvishy@google.com> wrote:
>>
>>> Hi maintainers,
>>>
>>> This RFC patch introduces the ability for userspace to control whether
>>> device (DMA) buffers are marked as cacheable, enabling them to utilize
>>> the system-level cache.
>>>
>>> The specific changes made in this patch are:
>>>
>>> * Introduce a new flag in `include/linux/iommu.h`:
>>>      * `IOMMU_SYS_CACHE` -  Indicates if the associated page should be cached in the system's cache hierarchy.
>>> * Add `VFIO_DMA_MAP_FLAG_SYS_CACHE` to `include/uapi/linux/vfio.h`:
> 
> You'll need a much better description of what this is supposed to do
> when you resend it.
> 
> IOMMU_CACHE already largely means that pages should be cached.
> 
> So I don't know what ARM's "INC_OCACHE" actually is doing. Causing
> writes to land in a cache somewhere in hierarchy? Something platform
> specific?

Kinda both - the Inner Non-Cacheable attribute means it's still 
fundamentally non-snooping and non-coherent with CPU caches, but the 
Outer Write-back Write-allocate attribute can still control allocation 
in a system cache downstream of the point of coherency if the platform 
is built with such a thing (it's not overly common).

However, as you point out, this is in direct conflict with the Inner 
Write-back Write-allocate attribute implied by the IOMMU_CACHE which 
VFIO adds in further down in vfio_iommu_map(). Plus the way it's 
actually implemented in patch #2, IOMMU_CACHE still takes precedence and 
would lead to this new value being completely ignored, so there's a lot 
which smells suspicious here...

Thanks,
Robin.

> I have no idea. By your description it sounds similar to the
> x86 data placement stuff, whatever that was called, and the more
> modern TPH approach.
> 
> Jason

Re: [PATCH v1 0/4] vfio/iommu: Flag to allow userspace to set DMA buffers system cacheable

[Manoj Vishwanathan]

On Tue, Aug 27, 2024 at 10:31 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 27/08/2024 12:17 am, Jason Gunthorpe wrote:
> > On Mon, Aug 26, 2024 at 11:04:47AM -0600, Alex Williamson wrote:
> >> On Mon, 26 Aug 2024 07:16:37 +0000
> >> Manoj Vishwanathan <manojvishy@google.com> wrote:
> >>
> >>> Hi maintainers,
> >>>
> >>> This RFC patch introduces the ability for userspace to control whether
> >>> device (DMA) buffers are marked as cacheable, enabling them to utilize
> >>> the system-level cache.
> >>>
> >>> The specific changes made in this patch are:
> >>>
> >>> * Introduce a new flag in `include/linux/iommu.h`:
> >>>      * `IOMMU_SYS_CACHE` -  Indicates if the associated page should be cached in the system's cache hierarchy.
> >>> * Add `VFIO_DMA_MAP_FLAG_SYS_CACHE` to `include/uapi/linux/vfio.h`:
> >
> > You'll need a much better description of what this is supposed to do
> > when you resend it.
> >
Thanks Jason, I will add more information before re-sending the patch.
> > IOMMU_CACHE already largely means that pages should be cached.
> >
> > So I don't know what ARM's "INC_OCACHE" actually is doing. Causing
> > writes to land in a cache somewhere in hierarchy? Something platform
> > specific?
>
> Kinda both - the Inner Non-Cacheable attribute means it's still
> fundamentally non-snooping and non-coherent with CPU caches, but the
> Outer Write-back Write-allocate attribute can still control allocation
> in a system cache downstream of the point of coherency if the platform
> is built with such a thing (it's not overly common).
>
> However, as you point out, this is in direct conflict with the Inner
> Write-back Write-allocate attribute implied by the IOMMU_CACHE which
> VFIO adds in further down in vfio_iommu_map(). Plus the way it's
> actually implemented in patch #2, IOMMU_CACHE still takes precedence and
> would lead to this new value being completely ignored, so there's a lot
> which smells suspicious here...
>
Thanks for your quick feedback.
I tested this with a 5.15-based kernel and applied the patch to get
early results.
I see the issue with IOMMU_CACHE overriding IOMMU_SYS_CACHE in patch #2.
This would likely be a problem on 5.15 as well, and I need to
investigate further
to understand how we observed better performance in our tests
while trying to exercise the system cache.
Let me do some more testing and get back.
Thanks,
Manoj
> Thanks,
> Robin.
>
> > I have no idea. By your description it sounds similar to the
> > x86 data placement stuff, whatever that was called, and the more
> > modern TPH approach.
> >
> > Jason