fix DMA aligment issues around virtio
Subsystems (16)
Michael S. Tsirkin
maintainerv2 cover fix DMA aligment issues around virtio
Cong Wang reported dma debug warnings with virtio-vsock and proposed a patch, see:
https://lore.kernel.org/all/[email protected]/
however, the issue is more widespread.
This is an attempt to fix it systematically.
Note: i2c and gio might also be affected, I am still looking into it. Help from maintainers welcome.
Lightly tested. Cursor/claude used liberally, mostly for refactoring/API updates/English.
DMA maintainers, could you please confirm the DMA core changes are ok with you?
Thanks!
Michael S. Tsirkin (15):
dma-mapping: add __dma_from_device_group_begin()/end()
docs: dma-api: document __dma_from_device_group_begin()/end()
dma-mapping: add DMA_ATTR_CPU_CACHE_CLEAN
docs: dma-api: document DMA_ATTR_CPU_CACHE_CLEAN
dma-debug: track cache clean flag in entries
virtio: add virtqueue_add_inbuf_cache_clean API vsock/virtio: fix DMA alignment for event_list vsock/virtio: use virtqueue_add_inbuf_cache_clean for events
virtio_input: fix DMA alignment for evts
virtio_scsi: fix DMA cacheline issues for events
virtio-rng: fix DMA alignment for data buffer
virtio_input: use virtqueue_add_inbuf_cache_clean for events vsock/virtio: reorder fields to reduce padding
gpio: virtio: fix DMA alignment
gpio: virtio: reorder fields to reduce struct padding
Documentation/core-api/dma-api-howto.rst | 52 ++++++++++++++ Documentation/core-api/dma-attributes.rst | 9 +++ drivers/char/hw_random/virtio-rng.c | 3 + drivers/gpio/gpio-virtio.c | 15 ++-- drivers/scsi/virtio_scsi.c | 17 +++-- drivers/virtio/virtio_input.c | 5 +- drivers/virtio/virtio_ring.c | 83 ++++++++++++++++------- include/linux/dma-mapping.h | 20 ++++++ include/linux/virtio.h | 5 ++ kernel/dma/debug.c | 28 ++++++-- net/vmw_vsock/virtio_transport.c | 8 ++- 11 files changed, 205 insertions(+), 40 deletions(-)
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v2 1/15 dma-mapping: add __dma_from_device_group_begin()/end()
When a structure contains a buffer that DMA writes to alongside fields that the CPU writes to, cache line sharing between the DMA buffer and CPU-written fields can cause data corruption on non-cache-coherent platforms.
Add __dma_from_device_group_begin()/end() annotations to ensure proper alignment to prevent this:
struct my_device {
spinlock_t lock1;
__dma_from_device_group_begin();
char dma_buffer1[16];
char dma_buffer2[16];
__dma_from_device_group_end();
spinlock_t lock2;
};--- include/linux/dma-mapping.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
include/linux/dma-mapping.h +13 -0
@@ -7,6 +7,7 @@#include <linux/dma-direction.h> #include <linux/scatterlist.h> #include <linux/bug.h> +#include <linux/cache.h> /** * List of possible attributes associated with a DMA mapping. The semantics
@@ -703,6 +704,18 @@ static inline int dma_get_cache_alignment(void) }#endif +#ifdef ARCH_HAS_DMA_MINALIGN +#define ____dma_from_device_aligned __aligned(ARCH_DMA_MINALIGN) +#else +#define ____dma_from_device_aligned +#endif +/* Mark start of DMA buffer */ +#define __dma_from_device_group_begin (GROUP ) \ + __cacheline_group_begin(GROUP) ____dma_from_device_aligned +/* Mark end of DMA buffer */ +#define __dma_from_device_group_end (GROUP ) \ + __cacheline_group_end(GROUP) ____dma_from_device_aligned +static inline void *dmam_alloc_coherent (struct device *dev,size_t size,dma_addr_t *dma_handle,gfp_t gfp) {
Signature · 1 line
MST
Re: [PATCH v2 01/15] dma-mapping: add __dma_from_device_group_begin()/end()
Quoted 17 lines · On Mon, 5 Jan 2026 03:22:54 -0500 "Michael S. Tsirkin" <[email protected]> wrote:
When a structure contains a buffer that DMA writes to alongside fields that the CPU writes to, cache line sharing between the DMA buffer and CPU-written fields can cause data corruption on non-cache-coherent platforms.
Add __dma_from_device_group_begin()/end() annotations to ensure proper alignment to prevent this:
struct my_device { spinlock_t lock1; __dma_from_device_group_begin(); char dma_buffer1[16]; char dma_buffer2[16]; __dma_from_device_group_end(); spinlock_t lock2; };
Quoted 1 line
Signed-off-by Michael S. Tsirkin <[email protected]>
LGTM. I'm not formally a reviewer, but FWIW:
Quoted 35 lines
--- include/linux/dma-mapping.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)include/linux/dma-mapping.h +13
-0@@ -7,6 +7,7 @@#include <linux/dma-direction.h> #include <linux/scatterlist.h> #include <linux/bug.h> +#include <linux/cache.h> /** * List of possible attributes associated with a DMA mapping. The semantics @@ -703,6 +704,18 @@ static inline int dma_get_cache_alignment(void) }#endif +#ifdef ARCH_HAS_DMA_MINALIGN +#define ____dma_from_device_aligned __aligned(ARCH_DMA_MINALIGN) +#else +#define ____dma_from_device_aligned +#endif +/* Mark start of DMA buffer */ +#define __dma_from_device_group_begin (GROUP ) \ + __cacheline_group_begin(GROUP) ____dma_from_device_aligned +/* Mark end of DMA buffer */ +#define __dma_from_device_group_end (GROUP ) \ + __cacheline_group_end(GROUP) ____dma_from_device_aligned +static inline void *dmam_alloc_coherent (struct device *dev,size_t size,dma_addr_t *dma_handle,gfp_t gfp) {
Re: [PATCH v2 01/15] dma-mapping: add __dma_from_device_group_begin()/end()
In reply to Michael S. TsirkinQuoted 17 lines · Michael S. Tsirkin
When a structure contains a buffer that DMA writes to alongside fields that the CPU writes to, cache line sharing between the DMA buffer and CPU-written fields can cause data corruption on non-cache-coherent platforms.
Add __dma_from_device_group_begin()/end() annotations to ensure proper alignment to prevent this:
struct my_device { spinlock_t lock1; __dma_from_device_group_begin(); char dma_buffer1[16]; char dma_buffer2[16]; __dma_from_device_group_end(); spinlock_t lock2; };
Quoted 4 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- include/linux/dma-mapping.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
Right, this was one of the long standing issues, how to make DMA to the buffers embedded into some structures safe and this solution looks really nice.
Quoted 21 lines
include/linux/dma-mapping.h +6
-0@@ -7,6 +7,7 @@#include <linux/dma-direction.h> #include <linux/scatterlist.h> #include <linux/bug.h> +#include <linux/cache.h> /** * List of possible attributes associated with a DMA mapping. The semantics @@ -703,6 +704,18 @@ static inline int dma_get_cache_alignment(void) }#endif +#ifdef ARCH_HAS_DMA_MINALIGN +#define ____dma_from_device_aligned __aligned(ARCH_DMA_MINALIGN) +#else +#define ____dma_from_device_aligned +#endif
include/linux/dma-mapping.h static inline int dma_get_cache_alignment(void) · line 721
Quoted 10 lines
include/linux/dma-mapping.h +7
-0+/* Mark start of DMA buffer */ +#define __dma_from_device_group_begin (GROUP ) \ + __cacheline_group_begin(GROUP) ____dma_from_device_aligned +/* Mark end of DMA buffer */ +#define __dma_from_device_group_end (GROUP ) \ + __cacheline_group_end(GROUP) ____dma_from_device_aligned +static inline void *dmam_alloc_coherent (struct device *dev,size_t size,dma_addr_t *dma_handle,gfp_t gfp) {
Best regards
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland
Michael S. Tsirkin
maintainerPATCH v2 2/15 docs: dma-api: document __dma_from_device_group_begin()/end()
In reply to Michael S. TsirkinDocument the __dma_from_device_group_begin()/end() annotations.
--- Documentation/core-api/dma-api-howto.rst | 52 ++++++++++++++++++++++++ 1 file changed, 52 insertions(+)
Documentation/core-api/dma-api-howto.rst +52 -0
@@ -146,6 +146,58 @@ What about block I/O and networking buffers? The block I/O and networking subsystems make sure that the buffers they use are valid for you to DMA from/to. +__dma_from_device_group_begin/end annotations +============================================= + +As explained previously, when a structure contains a DMA_FROM_DEVICE / +DMA_BIDIRECTIONAL buffer (device writes to memory) alongside fields that the +CPU writes to, cache line sharing between the DMA buffer and CPU-written fields +can cause data corruption on CPUs with DMA-incoherent caches. + +The ``__dma_from_device_group_begin(GROUP)/__dma_from_device_group_end(GROUP)`` +macros ensure proper alignment to prevent this:: + + struct my_device { + spinlock_t lock1; + __dma_from_device_group_begin(); + char dma_buffer1[16]; + char dma_buffer2[16]; + __dma_from_device_group_end(); + spinlock_t lock2; + }; + +To isolate a DMA buffer from adjacent fields, use +``__dma_from_device_group_begin(GROUP)`` before the first DMA buffer +field and ``__dma_from_device_group_end(GROUP)`` after the last DMA +buffer field (with the same GROUP name). This protects both the head +and tail of the buffer from cache line sharing. + +The GROUP parameter is an optional identifier that names the DMA buffer group +(in case you have several in the same structure):: + + struct my_device { + spinlock_t lock1; + __dma_from_device_group_begin(buffer1); + char dma_buffer1[16]; + __dma_from_device_group_end(buffer1); + spinlock_t lock2; + __dma_from_device_group_begin(buffer2); + char dma_buffer2[16]; + __dma_from_device_group_end(buffer2); + }; + +On cache-coherent platforms these macros expand to zero-length array markers. +On non-coherent platforms, they also ensure the minimal DMA alignment, which +can be as large as 128 bytes. + +.. note:: + + It is allowed (though somewhat fragile) to include extra fields, not + intended for DMA from the device, within the group (in order to pack the + structure tightly) - but only as long as the CPU does not write these + fields while any fields in the group are mapped for DMA_FROM_DEVICE or + DMA_BIDIRECTIONAL. + DMA addressing capabilities ===========================
Signature · 1 line
MST
Re: [PATCH v2 02/15] docs: dma-api: document __dma_from_device_group_begin()/end()
Quoted 3 lines · On Mon, 5 Jan 2026 03:22:57 -0500 "Michael S. Tsirkin" <[email protected]> wrote:
Document the __dma_from_device_group_begin()/end() annotations.
Signed-off-by Michael S. Tsirkin <[email protected]>
I really like your wording ("CPU does not write"), which rightly refers to what happens on the bus rather then what may or may not make a specific CPU architecture initiate a bus write.
I'm not formally a reviewer, but FWIW:
Quoted 67 lines
--- Documentation/core-api/dma-api-howto.rst | 52 ++++++++++++++++++++++++ 1 file changed, 52 insertions(+)Documentation/core-api/dma-api-howto.rst +52
-0@@ -146,6 +146,58 @@ What about block I/O and networking buffers? The block I/O and networking subsystems make sure that the buffers they use are valid for you to DMA from/to. +__dma_from_device_group_begin/end annotations +============================================= + +As explained previously, when a structure contains a DMA_FROM_DEVICE / +DMA_BIDIRECTIONAL buffer (device writes to memory) alongside fields that the +CPU writes to, cache line sharing between the DMA buffer and CPU-written fields +can cause data corruption on CPUs with DMA-incoherent caches. + +The ``__dma_from_device_group_begin(GROUP)/__dma_from_device_group_end(GROUP)`` +macros ensure proper alignment to prevent this:: + + struct my_device { + spinlock_t lock1; + __dma_from_device_group_begin(); + char dma_buffer1[16]; + char dma_buffer2[16]; + __dma_from_device_group_end(); + spinlock_t lock2; + }; + +To isolate a DMA buffer from adjacent fields, use +``__dma_from_device_group_begin(GROUP)`` before the first DMA buffer +field and ``__dma_from_device_group_end(GROUP)`` after the last DMA +buffer field (with the same GROUP name). This protects both the head +and tail of the buffer from cache line sharing. + +The GROUP parameter is an optional identifier that names the DMA buffer group +(in case you have several in the same structure):: + + struct my_device { + spinlock_t lock1; + __dma_from_device_group_begin(buffer1); + char dma_buffer1[16]; + __dma_from_device_group_end(buffer1); + spinlock_t lock2; + __dma_from_device_group_begin(buffer2); + char dma_buffer2[16]; + __dma_from_device_group_end(buffer2); + }; + +On cache-coherent platforms these macros expand to zero-length array markers. +On non-coherent platforms, they also ensure the minimal DMA alignment, which +can be as large as 128 bytes. + +.. note:: + + It is allowed (though somewhat fragile) to include extra fields, not + intended for DMA from the device, within the group (in order to pack the + structure tightly) - but only as long as the CPU does not write these + fields while any fields in the group are mapped for DMA_FROM_DEVICE or + DMA_BIDIRECTIONAL. + DMA addressing capabilities ===========================
Re: [PATCH v2 02/15] docs: dma-api: document __dma_from_device_group_begin()/end()
In reply to Michael S. TsirkinQuoted 6 lines · Michael S. Tsirkin
Document the __dma_from_device_group_begin()/end() annotations.
Signed-off-by Michael S. Tsirkin <[email protected]>--- Documentation/core-api/dma-api-howto.rst | 52 ++++++++++++++++++++++++ 1 file changed, 52 insertions(+)
Quoted 53 lines
Documentation/core-api/dma-api-howto.rst +45
-0@@ -146,6 +146,58 @@ What about block I/O and networking buffers? The block I/O and networking subsystems make sure that the buffers they use are valid for you to DMA from/to. +__dma_from_device_group_begin/end annotations +============================================= + +As explained previously, when a structure contains a DMA_FROM_DEVICE / +DMA_BIDIRECTIONAL buffer (device writes to memory) alongside fields that the +CPU writes to, cache line sharing between the DMA buffer and CPU-written fields +can cause data corruption on CPUs with DMA-incoherent caches. + +The ``__dma_from_device_group_begin(GROUP)/__dma_from_device_group_end(GROUP)`` +macros ensure proper alignment to prevent this:: + + struct my_device { + spinlock_t lock1; + __dma_from_device_group_begin(); + char dma_buffer1[16]; + char dma_buffer2[16]; + __dma_from_device_group_end(); + spinlock_t lock2; + }; + +To isolate a DMA buffer from adjacent fields, use +``__dma_from_device_group_begin(GROUP)`` before the first DMA buffer +field and ``__dma_from_device_group_end(GROUP)`` after the last DMA +buffer field (with the same GROUP name). This protects both the head +and tail of the buffer from cache line sharing. + +The GROUP parameter is an optional identifier that names the DMA buffer group +(in case you have several in the same structure):: + + struct my_device { + spinlock_t lock1; + __dma_from_device_group_begin(buffer1); + char dma_buffer1[16]; + __dma_from_device_group_end(buffer1); + spinlock_t lock2; + __dma_from_device_group_begin(buffer2); + char dma_buffer2[16]; + __dma_from_device_group_end(buffer2); + }; + +On cache-coherent platforms these macros expand to zero-length array markers. +On non-coherent platforms, they also ensure the minimal DMA alignment, which +can be as large as 128 bytes. + +.. note::
Documentation/core-api/dma-api-howto.rst What about block I/O and networking buffers? The block I/O and · line 203
Quoted 10 lines
Documentation/core-api/dma-api-howto.rst +7
-0+ + It is allowed (though somewhat fragile) to include extra fields, not + intended for DMA from the device, within the group (in order to pack the + structure tightly) - but only as long as the CPU does not write these + fields while any fields in the group are mapped for DMA_FROM_DEVICE or + DMA_BIDIRECTIONAL. + DMA addressing capabilities ===========================
Best regards
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland
Michael S. Tsirkin
maintainerPATCH v2 3/15 dma-mapping: add DMA_ATTR_CPU_CACHE_CLEAN
In reply to Michael S. TsirkinWhen multiple small DMA_FROM_DEVICE or DMA_BIDIRECTIONAL buffers share a cacheline, and DMA_API_DEBUG is enabled, we get this warning:
cacheline tracking EEXIST, overlapping mappings aren't supported.
This is because when one of the mappings is removed, while another one is active, CPU might write into the buffer.
Add an attribute for the driver to promise not to do this, making the overlapping safe, and suppressing the warning.
--- include/linux/dma-mapping.h | 7 +++++++ kernel/dma/debug.c | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-)
include/linux/dma-mapping.h +7 -0
@@ -79,6 +79,13 @@*/ #define DMA_ATTR_MMIO (1UL << 10) +/* +* DMA_ATTR_CPU_CACHE_CLEAN: Indicates the CPU will not dirty any cacheline +* overlapping this buffer while it is mapped for DMA. All mappings sharing +* a cacheline must have this attribute for this to be considered safe. +*/ +#define DMA_ATTR_CPU_CACHE_CLEAN (1UL << 11) +/* * A dma_addr_t can hold any valid DMA or bus address for the platform. It can * be given to a device to use as a DMA source or target. It is specific to a
kernel/dma/debug.c +2 -1
@@ -595,7 +595,8 @@ static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attrs)if (rc == -ENOMEM ) {pr_err_once ("cacheline tracking ENOMEM, dma-debug disabled\n" ); global_disable = true; - }else if (rc == -EEXIST && !(attrs &DMA_ATTR_SKIP_CPU_SYNC ) && + }else if (rc == -EEXIST && + !(attrs & (DMA_ATTR_SKIP_CPU_SYNC |DMA_ATTR_CPU_CACHE_CLEAN )) && !(IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) &&is_swiotlb_active (entry->dev))) { err_printk(entry->dev, entry,
Signature · 1 line
MST
Re: [PATCH v2 03/15] dma-mapping: add DMA_ATTR_CPU_CACHE_CLEAN
Quoted 11 lines · On Mon, 5 Jan 2026 03:23:01 -0500 "Michael S. Tsirkin" <[email protected]> wrote:
When multiple small
DMA_FROM_DEVICEorDMA_BIDIRECTIONALbuffers share a cacheline, andDMA_API_DEBUGis enabled, we get this warning:
cacheline tracking EEXIST, overlapping mappings aren't supported.This is because when one of the mappings is removed, while another one is active, CPU might write into the buffer.
Add an attribute for the driver to promise not to do this, making the overlapping safe, and suppressing the warning.
Signed-off-by Michael S. Tsirkin <[email protected]>
LGTM. I'm not formally a reviewer, but FWIW:
Quoted 37 lines
--- include/linux/dma-mapping.h | 7 +++++++ kernel/dma/debug.c | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-)include/linux/dma-mapping.h +7
-0@@ -79,6 +79,13 @@*/ #define DMA_ATTR_MMIO (1UL << 10) +/* +* DMA_ATTR_CPU_CACHE_CLEAN: Indicates the CPU will not dirty any cacheline +* overlapping this buffer while it is mapped for DMA. All mappings sharing +* a cacheline must have this attribute for this to be considered safe. +*/ +#define DMA_ATTR_CPU_CACHE_CLEAN (1UL << 11) +/* * A dma_addr_t can hold any valid DMA or bus address for the platform. It can * be given to a device to use as a DMA source or target. It is specific to a kernel/dma/debug.c +2
-1@@ -595,7 +595,8 @@ static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attrs)if (rc == -ENOMEM ) {pr_err_once ("cacheline tracking ENOMEM, dma-debug disabled\n" ); global_disable = true; - }else if (rc == -EEXIST && !(attrs &DMA_ATTR_SKIP_CPU_SYNC ) && + }else if (rc == -EEXIST && + !(attrs & (DMA_ATTR_SKIP_CPU_SYNC |DMA_ATTR_CPU_CACHE_CLEAN )) && !(IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) &&is_swiotlb_active (entry->dev))) { err_printk(entry->dev, entry,
Re: [PATCH v2 03/15] dma-mapping: add DMA_ATTR_CPU_CACHE_CLEAN
In reply to Michael S. TsirkinQuoted 11 lines · Michael S. Tsirkin
When multiple small
DMA_FROM_DEVICEorDMA_BIDIRECTIONALbuffers share a cacheline, andDMA_API_DEBUGis enabled, we get this warning:
cacheline tracking EEXIST, overlapping mappings aren't supported.This is because when one of the mappings is removed, while another one is active, CPU might write into the buffer.
Add an attribute for the driver to promise not to do this, making the overlapping safe, and suppressing the warning.
Signed-off-by Michael S. Tsirkin <[email protected]>
It is somehow similar to DMA_ATTR_SKIP_CPU_SYNC in its concept, so I see no reason not to accept it.
Quoted 28 lines
--- include/linux/dma-mapping.h | 7 +++++++ kernel/dma/debug.c | 3 ++- 2 files changed, 9 insertions(+), 1 deletion(-)include/linux/dma-mapping.h +7
-0@@ -79,6 +79,13 @@*/ #define DMA_ATTR_MMIO (1UL << 10) +/* +* DMA_ATTR_CPU_CACHE_CLEAN: Indicates the CPU will not dirty any cacheline +* overlapping this buffer while it is mapped for DMA. All mappings sharing +* a cacheline must have this attribute for this to be considered safe. +*/ +#define DMA_ATTR_CPU_CACHE_CLEAN (1UL << 11) +/* * A dma_addr_t can hold any valid DMA or bus address for the platform. It can * be given to a device to use as a DMA source or target. It is specific to a kernel/dma/debug.c +0
-0
kernel/dma/debug.c static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attrs) · line 602
Quoted 9 lines
kernel/dma/debug.c +2
-1@@ -595,7 +595,8 @@ static void add_dma_entry(struct dma_debug_entry *entry, unsigned long attrs)if (rc == -ENOMEM ) {pr_err_once ("cacheline tracking ENOMEM, dma-debug disabled\n" ); global_disable = true; - }else if (rc == -EEXIST && !(attrs &DMA_ATTR_SKIP_CPU_SYNC ) && + }else if (rc == -EEXIST && + !(attrs & (DMA_ATTR_SKIP_CPU_SYNC |DMA_ATTR_CPU_CACHE_CLEAN )) && !(IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) &&is_swiotlb_active (entry->dev))) { err_printk(entry->dev, entry,
Best regards
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland
Michael S. Tsirkin
maintainerPATCH v2 4/15 docs: dma-api: document DMA_ATTR_CPU_CACHE_CLEAN
In reply to Michael S. TsirkinDocument DMA_ATTR_CPU_CACHE_CLEAN as implemented in the previous patch.
--- Documentation/core-api/dma-attributes.rst | 9 +++++++++ 1 file changed, 9 insertions(+)
Documentation/core-api/dma-attributes.rst +9 -0
@@ -148,3 +148,12 @@ DMA_ATTR_MMIO is appropriate. For architectures that require cache flushing for DMA coherence DMA_ATTR_MMIO will not perform any cache flushing. The address provided must never be mapped cacheable into the CPU. + +DMA_ATTR_CPU_CACHE_CLEAN +------------------------ + +This attribute indicates the CPU will not dirty any cacheline overlapping this +DMA_FROM_DEVICE/DMA_BIDIRECTIONAL buffer while it is mapped. This allows +multiple small buffers to safely share a cacheline without risk of data +corruption, suppressing DMA debug warnings about overlapping mappings. +All mappings sharing a cacheline should have this attribute.
Signature · 1 line
MST
Re: [PATCH v2 04/15] docs: dma-api: document DMA_ATTR_CPU_CACHE_CLEAN
Quoted 4 lines · On Mon, 5 Jan 2026 03:23:05 -0500 "Michael S. Tsirkin" <[email protected]> wrote:
Document
DMA_ATTR_CPU_CACHE_CLEANas implemented in the previous patch.Signed-off-by Michael S. Tsirkin <[email protected]>
LGTM. I'm not formally a reviewer, but FWIW:
Quoted 21 lines
--- Documentation/core-api/dma-attributes.rst | 9 +++++++++ 1 file changed, 9 insertions(+)Documentation/core-api/dma-attributes.rst +9
-0@@ -148,3 +148,12 @@ DMA_ATTR_MMIO is appropriate. For architectures that require cache flushing for DMA coherence DMA_ATTR_MMIO will not perform any cache flushing. The address provided must never be mapped cacheable into the CPU. + +DMA_ATTR_CPU_CACHE_CLEAN +------------------------ + +This attribute indicates the CPU will not dirty any cacheline overlapping this +DMA_FROM_DEVICE/DMA_BIDIRECTIONAL buffer while it is mapped. This allows +multiple small buffers to safely share a cacheline without risk of data +corruption, suppressing DMA debug warnings about overlapping mappings. +All mappings sharing a cacheline should have this attribute.
Re: [PATCH v2 04/15] docs: dma-api: document DMA_ATTR_CPU_CACHE_CLEAN
In reply to Michael S. TsirkinQuoted 4 lines · Michael S. Tsirkin
Document
DMA_ATTR_CPU_CACHE_CLEANas implemented in the previous patch.Signed-off-by Michael S. Tsirkin <[email protected]>
Quoted 11 lines
--- Documentation/core-api/dma-attributes.rst | 9 +++++++++ 1 file changed, 9 insertions(+)Documentation/core-api/dma-attributes.rst +0
-0@@ -148,3 +148,12 @@ DMA_ATTR_MMIO is appropriate. For architectures that require cache flushing for DMA coherence DMA_ATTR_MMIO will not perform any cache flushing. The address
Documentation/core-api/dma-attributes.rst DMA_ATTR_MMIO is appropriate. · line 159
Quoted 10 lines
Documentation/core-api/dma-attributes.rst +9
-0provided must never be mapped cacheable into the CPU. + +DMA_ATTR_CPU_CACHE_CLEAN +------------------------ + +This attribute indicates the CPU will not dirty any cacheline overlapping this +DMA_FROM_DEVICE/DMA_BIDIRECTIONAL buffer while it is mapped. This allows +multiple small buffers to safely share a cacheline without risk of data +corruption, suppressing DMA debug warnings about overlapping mappings. +All mappings sharing a cacheline should have this attribute.
Best regards
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland
Michael S. Tsirkin
maintainerPATCH v2 5/15 dma-debug: track cache clean flag in entries
In reply to Michael S. TsirkinIf a driver is buggy and has 2 overlapping mappings but only sets cache clean flag on the 1st one of them, we warn. But if it only does it for the 2nd one, we don't.
Fix by tracking cache clean flag in the entry.
--- kernel/dma/debug.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)
kernel/dma/debug.c +22 -5
@@ -63,6 +63,7 @@ enum map_err_types {* @sg_mapped_ents: 'mapped_ents' from dma_map_sg * @paddr: physical start address of the mapping * @map_err_type: track whether dma_mapping_error() was checked +* @is_cache_clean: driver promises not to write to buffer while mapped * @stack_len: number of backtrace entries in @stack_entries * @stack_entries: stack of backtrace history */
@@ -76,7 +77,8 @@ struct dma_debug_entry {int sg_call_ents;int sg_mapped_ents;phys_addr_t paddr; -enum map_err_types map_err_type; +enum map_err_types map_err_type; +bool is_cache_clean;#ifdef CONFIG_STACKTRACE unsigned int stack_len;unsigned long stack_entries[DMA_DEBUG_STACKTRACE_ENTRIES ];
@@ -472,12 +474,15 @@ static int active_cacheline_dec_overlap(phys_addr_t cln)return active_cacheline_set_overlap (cln, --overlap); } -static int active_cacheline_insert (struct dma_debug_entry *entry) +static int active_cacheline_insert (struct dma_debug_entry *entry, +bool *overlap_cache_clean) {phys_addr_t cln =to_cacheline_number (entry);unsigned long flags;int rc; + *overlap_cache_clean = false; +/* If the device is not writing memory then we don't have any * concerns about the cpu consuming stale data. This mitigates * legitimate usages of overlapping mappings.
@@ -487,8 +492,16 @@ static int active_cacheline_insert(struct dma_debug_entry *entry)spin_lock_irqsave (&radix_lock, flags); rc =radix_tree_insert (&dma_active_cacheline, cln, entry); -if (rc == -EEXIST ) +if (rc == -EEXIST ) { +struct dma_debug_entry *existing; +active_cacheline_inc_overlap (cln); + existing =radix_tree_lookup (&dma_active_cacheline, cln); +/* A lookup failure here after we got -EEXIST is unexpected. */ +WARN_ON (!existing); +if (existing) + *overlap_cache_clean = existing->is_cache_clean; + }spin_unlock_irqrestore (&radix_lock, flags);return rc;
@@ -583,20 +596,24 @@ DEFINE_SHOW_ATTRIBUTE(dump);*/ static void add_dma_entry (struct dma_debug_entry *entry,unsigned long attrs) { +bool overlap_cache_clean;struct hash_bucket *bucket;unsigned long flags;int rc; + entry->is_cache_clean = !!(attrs &DMA_ATTR_CPU_CACHE_CLEAN ); + bucket =get_hash_bucket (entry, &flags);hash_bucket_add (bucket, entry);put_hash_bucket (bucket, flags); - rc =active_cacheline_insert (entry); + rc =active_cacheline_insert (entry, &overlap_cache_clean);if (rc == -ENOMEM ) {pr_err_once ("cacheline tracking ENOMEM, dma-debug disabled\n" ); global_disable = true; }else if (rc == -EEXIST && - !(attrs & (DMA_ATTR_SKIP_CPU_SYNC |DMA_ATTR_CPU_CACHE_CLEAN )) && + !(attrs &DMA_ATTR_SKIP_CPU_SYNC ) && + !(entry->is_cache_clean && overlap_cache_clean) && !(IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) &&is_swiotlb_active (entry->dev))) { err_printk(entry->dev, entry,
Signature · 1 line
MST
Re: [PATCH v2 05/15] dma-debug: track cache clean flag in entries
Quoted 24 lines · On Mon, 5 Jan 2026 03:23:10 -0500 "Michael S. Tsirkin" <[email protected]> wrote:
If a driver is buggy and has 2 overlapping mappings but only sets cache clean flag on the 1st one of them, we warn. But if it only does it for the 2nd one, we don't.
Fix by tracking cache clean flag in the entry.
Signed-off-by Michael S. Tsirkin <[email protected]>--- kernel/dma/debug.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)kernel/dma/debug.c +1
-0@@ -63,6 +63,7 @@ enum map_err_types {* @sg_mapped_ents: 'mapped_ents' from dma_map_sg * @paddr: physical start address of the mapping * @map_err_type: track whether dma_mapping_error() was checked +* @is_cache_clean: driver promises not to write to buffer while mapped * @stack_len: number of backtrace entries in @stack_entries * @stack_entries: stack of backtrace history */
kernel/dma/debug.c struct dma_debug_entry { · line 80
Quoted 5 lines
kernel/dma/debug.c +1
-1@@ -76,7 +77,8 @@ struct dma_debug_entry {int sg_call_ents;int sg_mapped_ents;phys_addr_t paddr; -enum map_err_types map_err_type; +enum map_err_types map_err_type;
*nitpick* unnecessary change in white space (breaks git-blame).
Other than that, LGTM. I'm not formally a reviewer, but FWIW:
Petr T
Quoted 66 lines
kernel/dma/debug.c +20
-4+bool is_cache_clean;#ifdef CONFIG_STACKTRACE unsigned int stack_len;unsigned long stack_entries[DMA_DEBUG_STACKTRACE_ENTRIES ];@@ -472,12 +474,15 @@ static int active_cacheline_dec_overlap(phys_addr_t cln)return active_cacheline_set_overlap (cln, --overlap); } -static int active_cacheline_insert (struct dma_debug_entry *entry) +static int active_cacheline_insert (struct dma_debug_entry *entry, +bool *overlap_cache_clean) {phys_addr_t cln =to_cacheline_number (entry);unsigned long flags;int rc; + *overlap_cache_clean = false; +/* If the device is not writing memory then we don't have any * concerns about the cpu consuming stale data. This mitigates * legitimate usages of overlapping mappings. @@ -487,8 +492,16 @@ static int active_cacheline_insert(struct dma_debug_entry *entry)spin_lock_irqsave (&radix_lock, flags); rc =radix_tree_insert (&dma_active_cacheline, cln, entry); -if (rc == -EEXIST ) +if (rc == -EEXIST ) { +struct dma_debug_entry *existing; +active_cacheline_inc_overlap (cln); + existing =radix_tree_lookup (&dma_active_cacheline, cln); +/* A lookup failure here after we got -EEXIST is unexpected. */ +WARN_ON (!existing); +if (existing) + *overlap_cache_clean = existing->is_cache_clean; + }spin_unlock_irqrestore (&radix_lock, flags);return rc;@@ -583,20 +596,24 @@ DEFINE_SHOW_ATTRIBUTE(dump);*/ static void add_dma_entry (struct dma_debug_entry *entry,unsigned long attrs) { +bool overlap_cache_clean;struct hash_bucket *bucket;unsigned long flags;int rc; + entry->is_cache_clean = !!(attrs &DMA_ATTR_CPU_CACHE_CLEAN ); + bucket =get_hash_bucket (entry, &flags);hash_bucket_add (bucket, entry);put_hash_bucket (bucket, flags); - rc =active_cacheline_insert (entry); + rc =active_cacheline_insert (entry, &overlap_cache_clean);if (rc == -ENOMEM ) {pr_err_once ("cacheline tracking ENOMEM, dma-debug disabled\n" ); global_disable = true; }else if (rc == -EEXIST && - !(attrs & (DMA_ATTR_SKIP_CPU_SYNC |DMA_ATTR_CPU_CACHE_CLEAN )) && + !(attrs &DMA_ATTR_SKIP_CPU_SYNC ) && + !(entry->is_cache_clean && overlap_cache_clean) && !(IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) &&is_swiotlb_active (entry->dev))) { err_printk(entry->dev, entry,
Michael S. Tsirkin
maintainerRe: [PATCH v2 05/15] dma-debug: track cache clean flag in entries
Quoted 35 lines · Petr Tesarik
Quoted 24 lines · On Mon, 5 Jan 2026 03:23:10 -0500 "Michael S. Tsirkin" <[email protected]> wrote:
If a driver is buggy and has 2 overlapping mappings but only sets cache clean flag on the 1st one of them, we warn. But if it only does it for the 2nd one, we don't.
Fix by tracking cache clean flag in the entry.
Signed-off-by Michael S. Tsirkin <[email protected]>--- kernel/dma/debug.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)kernel/dma/debug.c +1
-0@@ -63,6 +63,7 @@ enum map_err_types {* @sg_mapped_ents: 'mapped_ents' from dma_map_sg * @paddr: physical start address of the mapping * @map_err_type: track whether dma_mapping_error() was checked +* @is_cache_clean: driver promises not to write to buffer while mapped * @stack_len: number of backtrace entries in @stack_entries * @stack_entries: stack of backtrace history */ kernel/dma/debug.c struct dma_debug_entry { · line 80
Quoted 5 lines
kernel/dma/debug.c +1
-1@@ -76,7 +77,8 @@ struct dma_debug_entry {int sg_call_ents;int sg_mapped_ents;phys_addr_t paddr; -enum map_err_types map_err_type; +enum map_err_types map_err_type;*nitpick* unnecessary change in white space (breaks git-blame).
Quoted 5 lines
Other than that, LGTM. I'm not formally a reviewer, but FWIW:
Reviewed-by Petr Tesarik <[email protected]>Petr T
I mean, yes it's not really required here, but the padding we had before was broken (two spaces not aligning to anything).
Quoted 66 lines
Quoted 66 lines
kernel/dma/debug.c +20
-4+bool is_cache_clean;#ifdef CONFIG_STACKTRACE unsigned int stack_len;unsigned long stack_entries[DMA_DEBUG_STACKTRACE_ENTRIES ];@@ -472,12 +474,15 @@ static int active_cacheline_dec_overlap(phys_addr_t cln)return active_cacheline_set_overlap (cln, --overlap); } -static int active_cacheline_insert (struct dma_debug_entry *entry) +static int active_cacheline_insert (struct dma_debug_entry *entry, +bool *overlap_cache_clean) {phys_addr_t cln =to_cacheline_number (entry);unsigned long flags;int rc; + *overlap_cache_clean = false; +/* If the device is not writing memory then we don't have any * concerns about the cpu consuming stale data. This mitigates * legitimate usages of overlapping mappings. @@ -487,8 +492,16 @@ static int active_cacheline_insert(struct dma_debug_entry *entry)spin_lock_irqsave (&radix_lock, flags); rc =radix_tree_insert (&dma_active_cacheline, cln, entry); -if (rc == -EEXIST ) +if (rc == -EEXIST ) { +struct dma_debug_entry *existing; +active_cacheline_inc_overlap (cln); + existing =radix_tree_lookup (&dma_active_cacheline, cln); +/* A lookup failure here after we got -EEXIST is unexpected. */ +WARN_ON (!existing); +if (existing) + *overlap_cache_clean = existing->is_cache_clean; + }spin_unlock_irqrestore (&radix_lock, flags);return rc;@@ -583,20 +596,24 @@ DEFINE_SHOW_ATTRIBUTE(dump);*/ static void add_dma_entry (struct dma_debug_entry *entry,unsigned long attrs) { +bool overlap_cache_clean;struct hash_bucket *bucket;unsigned long flags;int rc; + entry->is_cache_clean = !!(attrs &DMA_ATTR_CPU_CACHE_CLEAN ); + bucket =get_hash_bucket (entry, &flags);hash_bucket_add (bucket, entry);put_hash_bucket (bucket, flags); - rc =active_cacheline_insert (entry); + rc =active_cacheline_insert (entry, &overlap_cache_clean);if (rc == -ENOMEM ) {pr_err_once ("cacheline tracking ENOMEM, dma-debug disabled\n" ); global_disable = true; }else if (rc == -EEXIST && - !(attrs & (DMA_ATTR_SKIP_CPU_SYNC |DMA_ATTR_CPU_CACHE_CLEAN )) && + !(attrs &DMA_ATTR_SKIP_CPU_SYNC ) && + !(entry->is_cache_clean && overlap_cache_clean) && !(IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) &&is_swiotlb_active (entry->dev))) { err_printk(entry->dev, entry,
Re: [PATCH v2 05/15] dma-debug: track cache clean flag in entries
Quoted 38 lines · On Mon, 5 Jan 2026 07:37:31 -0500 "Michael S. Tsirkin" <[email protected]> wrote:
Quoted 37 lines · Petr Tesarik
Quoted 24 lines · On Mon, 5 Jan 2026 03:23:10 -0500 "Michael S. Tsirkin" <[email protected]> wrote:
If a driver is buggy and has 2 overlapping mappings but only sets cache clean flag on the 1st one of them, we warn. But if it only does it for the 2nd one, we don't.
Fix by tracking cache clean flag in the entry.
Signed-off-by Michael S. Tsirkin <[email protected]>--- kernel/dma/debug.c | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-)kernel/dma/debug.c +1
-0@@ -63,6 +63,7 @@ enum map_err_types {* @sg_mapped_ents: 'mapped_ents' from dma_map_sg * @paddr: physical start address of the mapping * @map_err_type: track whether dma_mapping_error() was checked +* @is_cache_clean: driver promises not to write to buffer while mapped * @stack_len: number of backtrace entries in @stack_entries * @stack_entries: stack of backtrace history */ kernel/dma/debug.c struct dma_debug_entry { · line 80
Quoted 5 lines
kernel/dma/debug.c +1
-1@@ -76,7 +77,8 @@ struct dma_debug_entry {int sg_call_ents;int sg_mapped_ents;phys_addr_t paddr; -enum map_err_types map_err_type; +enum map_err_types map_err_type;*nitpick* unnecessary change in white space (breaks git-blame).
Other than that, LGTM. I'm not formally a reviewer, but FWIW:
Quoted 7 lines
Quoted 3 lines
Reviewed-by Petr Tesarik <[email protected]>Petr T
I mean, yes it's not really required here, but the padding we had before was broken (two spaces not aligning to anything).
Oh, you're right! Yes, then let's fix it now, because you touch the neighbouring line.
Sorry for the noise.
Petr T
Quoted 67 lines
Quoted 66 lines
Quoted 66 lines
kernel/dma/debug.c +20
-4+bool is_cache_clean;#ifdef CONFIG_STACKTRACE unsigned int stack_len;unsigned long stack_entries[DMA_DEBUG_STACKTRACE_ENTRIES ];@@ -472,12 +474,15 @@ static int active_cacheline_dec_overlap(phys_addr_t cln)return active_cacheline_set_overlap (cln, --overlap); } -static int active_cacheline_insert (struct dma_debug_entry *entry) +static int active_cacheline_insert (struct dma_debug_entry *entry, +bool *overlap_cache_clean) {phys_addr_t cln =to_cacheline_number (entry);unsigned long flags;int rc; + *overlap_cache_clean = false; +/* If the device is not writing memory then we don't have any * concerns about the cpu consuming stale data. This mitigates * legitimate usages of overlapping mappings. @@ -487,8 +492,16 @@ static int active_cacheline_insert(struct dma_debug_entry *entry)spin_lock_irqsave (&radix_lock, flags); rc =radix_tree_insert (&dma_active_cacheline, cln, entry); -if (rc == -EEXIST ) +if (rc == -EEXIST ) { +struct dma_debug_entry *existing; +active_cacheline_inc_overlap (cln); + existing =radix_tree_lookup (&dma_active_cacheline, cln); +/* A lookup failure here after we got -EEXIST is unexpected. */ +WARN_ON (!existing); +if (existing) + *overlap_cache_clean = existing->is_cache_clean; + }spin_unlock_irqrestore (&radix_lock, flags);return rc;@@ -583,20 +596,24 @@ DEFINE_SHOW_ATTRIBUTE(dump);*/ static void add_dma_entry (struct dma_debug_entry *entry,unsigned long attrs) { +bool overlap_cache_clean;struct hash_bucket *bucket;unsigned long flags;int rc; + entry->is_cache_clean = !!(attrs &DMA_ATTR_CPU_CACHE_CLEAN ); + bucket =get_hash_bucket (entry, &flags);hash_bucket_add (bucket, entry);put_hash_bucket (bucket, flags); - rc =active_cacheline_insert (entry); + rc =active_cacheline_insert (entry, &overlap_cache_clean);if (rc == -ENOMEM ) {pr_err_once ("cacheline tracking ENOMEM, dma-debug disabled\n" ); global_disable = true; }else if (rc == -EEXIST && - !(attrs & (DMA_ATTR_SKIP_CPU_SYNC |DMA_ATTR_CPU_CACHE_CLEAN )) && + !(attrs &DMA_ATTR_SKIP_CPU_SYNC ) && + !(entry->is_cache_clean && overlap_cache_clean) && !(IS_ENABLED (CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC ) &&is_swiotlb_active (entry->dev))) { err_printk(entry->dev, entry,
Michael S. Tsirkin
maintainerPATCH v2 6/15 virtio: add virtqueue_add_inbuf_cache_clean API
In reply to Michael S. TsirkinAdd virtqueue_add_inbuf_cache_clean() for passing DMA_ATTR_CPU_CACHE_CLEAN to virtqueue operations. This suppresses DMA debug cacheline overlap warnings for buffers where proper cache management is ensured by the caller.
--- drivers/virtio/virtio_ring.c | 83 ++++++++++++++++++++++++++---------- include/linux/virtio.h | 5 +++ 2 files changed, 65 insertions(+), 23 deletions(-)
drivers/virtio/virtio_ring.c +60 -23
@@ -174,7 +174,8 @@ struct virtqueue_ops {int (*add)(struct vring_virtqueue *vq,struct scatterlist *sgs[],unsigned int total_sg,unsigned int out_sgs,unsigned int in_sgs,void *data, -void *ctx,bool premapped,gfp_t gfp); +void *ctx,bool premapped,gfp_t gfp, +unsigned long attr);void *(*get)(struct vring_virtqueue *vq,unsigned int *len,void **ctx);bool (*kick_prepare)(struct vring_virtqueue *vq);void (*disable_cb)(struct vring_virtqueue *vq);
@@ -444,7 +445,7 @@ static int vring_mapping_error(const struct vring_virtqueue *vq,/* Map one sg entry. */ static int vring_map_one_sg (const struct vring_virtqueue *vq,struct scatterlist *sg,enum dma_data_direction direction,dma_addr_t *addr, -u32 *len,bool premapped) +u32 *len,bool premapped,unsigned long attr) {if (premapped) { *addr =sg_dma_address (sg);
@@ -472,7 +473,7 @@ static int vring_map_one_sg(const struct vring_virtqueue *vq, struct scatterlist*/ *addr =virtqueue_map_page_attrs (&vq->vq,sg_page (sg), sg->offset, sg->length, - direction,0 ); + direction, attr);if (vring_mapping_error (vq, *addr))return -ENOMEM ;
@@ -603,7 +604,8 @@ static inline int virtqueue_add_split(struct vring_virtqueue *vq,void *data, void *ctx, bool premapped, - gfp_t gfp) + gfp_t gfp, + unsigned long attr) { struct vring_desc_extra *extra;struct scatterlist *sg;
@@ -675,7 +677,8 @@ static inline int virtqueue_add_split(struct vring_virtqueue *vq,if (++sg_count != total_sg) flags |=VRING_DESC_F_NEXT ; -if (vring_map_one_sg (vq, sg,DMA_TO_DEVICE , &addr, &len, premapped)) +if (vring_map_one_sg (vq, sg,DMA_TO_DEVICE , &addr, &len, + premapped, attr)) goto unmap_release;/* Note that we trust indirect descriptor
@@ -694,7 +697,8 @@ static inline int virtqueue_add_split(struct vring_virtqueue *vq,if (++sg_count != total_sg) flags |=VRING_DESC_F_NEXT ; -if (vring_map_one_sg (vq, sg,DMA_FROM_DEVICE , &addr, &len, premapped)) +if (vring_map_one_sg (vq, sg,DMA_FROM_DEVICE , &addr, &len, + premapped, attr)) goto unmap_release;/* Note that we trust indirect descriptor
@@ -1487,7 +1491,8 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,void *data, bool premapped, gfp_t gfp, - u16 id) + u16 id, + unsigned long attr) { struct vring_desc_extra *extra;struct vring_packed_desc *desc;
@@ -1516,7 +1521,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,for (sg = sgs[n]; sg; sg =sg_next (sg)) {if (vring_map_one_sg (vq, sg, n < out_sgs ?DMA_TO_DEVICE :DMA_FROM_DEVICE , - &addr, &len, premapped)) + &addr, &len, premapped, attr)) goto unmap_release; desc[i].flags = cpu_to_le16(n < out_sgs ?
@@ -1615,7 +1620,8 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,void *data, void *ctx, bool premapped, - gfp_t gfp) + gfp_t gfp, + unsigned long attr) { struct vring_packed_desc *desc;struct scatterlist *sg;
@@ -1642,8 +1648,8 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq, id = vq->free_head;BUG_ON (id == vq->packed.vring.num); err =virtqueue_add_indirect_packed (vq, sgs, total_sg, out_sgs, - in_sgs, data, premapped, - gfp, id); + in_sgs, data, premapped, gfp, + id, attr);if (err != -ENOMEM ) {END_USE (vq);return err;
@@ -1679,7 +1685,7 @@ static inline int virtqueue_add_packed(struct vring_virtqueue *vq,if (vring_map_one_sg (vq, sg, n < out_sgs ?DMA_TO_DEVICE :DMA_FROM_DEVICE , - &addr, &len, premapped)) + &addr, &len, premapped, attr)) goto unmap_release; flags = cpu_to_le16(vq->packed.avail_used_flags |
@@ -1772,7 +1778,8 @@ static inline int virtqueue_add_packed_in_order(struct vring_virtqueue *vq,void *data, void *ctx, bool premapped, - gfp_t gfp) + gfp_t gfp, + unsigned long attr) { struct vring_packed_desc *desc;struct scatterlist *sg;
@@ -1799,7 +1806,8 @@ static inline int virtqueue_add_packed_in_order(struct vring_virtqueue *vq,if (virtqueue_use_indirect (vq, total_sg)) { err =virtqueue_add_indirect_packed (vq, sgs, total_sg, out_sgs, in_sgs, data, premapped, gfp, - vq->packed.next_avail_idx); + vq->packed.next_avail_idx, + attr);if (err != -ENOMEM ) {END_USE (vq);return err;
@@ -1838,7 +1846,7 @@ static inline int virtqueue_add_packed_in_order(struct vring_virtqueue *vq,if (vring_map_one_sg (vq, sg, n < out_sgs ?DMA_TO_DEVICE :DMA_FROM_DEVICE , - &addr, &len, premapped)) + &addr, &len, premapped, attr)) goto unmap_release; flags |=cpu_to_le16 (vq->packed.avail_used_flags);
@@ -2781,13 +2789,14 @@ static inline int virtqueue_add(struct virtqueue *_vq,void *data, void *ctx, bool premapped, - gfp_t gfp) + gfp_t gfp, + unsigned long attr) { struct vring_virtqueue *vq = to_vvq(_vq);return VIRTQUEUE_CALL (vq, add, sgs, total_sg, out_sgs, in_sgs, data, - ctx, premapped, gfp); + ctx, premapped, gfp, attr); }/**
@@ -2825,7 +2834,7 @@ int virtqueue_add_sgs(struct virtqueue *_vq, total_sg++; }return virtqueue_add (_vq, sgs, total_sg, out_sgs, in_sgs, - data,NULL , false, gfp); + data,NULL , false, gfp,0 ); }EXPORT_SYMBOL_GPL (virtqueue_add_sgs);
@@ -2847,7 +2856,7 @@ int virtqueue_add_outbuf(struct virtqueue *vq, void *data, gfp_t gfp) { -return virtqueue_add (vq, &sg, num,1 ,0 , data,NULL , false, gfp); +return virtqueue_add (vq, &sg, num,1 ,0 , data,NULL , false, gfp,0 ); }EXPORT_SYMBOL_GPL (virtqueue_add_outbuf);
@@ -2870,7 +2879,7 @@ int virtqueue_add_outbuf_premapped(struct virtqueue *vq, void *data, gfp_t gfp) { -return virtqueue_add (vq, &sg, num,1 ,0 , data,NULL , true, gfp); +return virtqueue_add (vq, &sg, num,1 ,0 , data,NULL , true, gfp,0 ); }EXPORT_SYMBOL_GPL (virtqueue_add_outbuf_premapped);
@@ -2892,10 +2901,38 @@ int virtqueue_add_inbuf(struct virtqueue *vq, void *data, gfp_t gfp) { -return virtqueue_add (vq, &sg, num,0 ,1 , data,NULL , false, gfp); +return virtqueue_add (vq, &sg, num,0 ,1 , data,NULL , false, gfp,0 ); }EXPORT_SYMBOL_GPL (virtqueue_add_inbuf); +/** +* virtqueue_add_inbuf_cache_clean - expose input buffers with cache clean +* @vq: the struct virtqueue we're talking about. +* @sg: scatterlist (must be well-formed and terminated!) +* @num: the number of entries in @sg writable by other side +* @data: the token identifying the buffer. +* @gfp: how to do memory allocations (if necessary). +* +* Same as virtqueue_add_inbuf but passes DMA_ATTR_CPU_CACHE_CLEAN to indicate +* that the CPU will not dirty any cacheline overlapping this buffer while it +* is available, and to suppress overlapping cacheline warnings in DMA debug +* builds. +* +* Caller must ensure we don't call this with other virtqueue operations +* at the same time (except where noted). +* +* Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO). +*/ +int virtqueue_add_inbuf_cache_clean (struct virtqueue *vq, +struct scatterlist *sg,unsigned int num, +void *data, +gfp_t gfp) +{ +return virtqueue_add (vq, &sg, num,0 ,1 , data,NULL , false, gfp, +DMA_ATTR_CPU_CACHE_CLEAN ); +} +EXPORT_SYMBOL_GPL (virtqueue_add_inbuf_cache_clean); +/** * virtqueue_add_inbuf_ctx - expose input buffers to other end * @vq: the struct virtqueue we're talking about.
@@ -2916,7 +2953,7 @@ int virtqueue_add_inbuf_ctx(struct virtqueue *vq, void *ctx, gfp_t gfp) { -return virtqueue_add (vq, &sg, num,0 ,1 , data, ctx, false, gfp); +return virtqueue_add (vq, &sg, num,0 ,1 , data, ctx, false, gfp,0 ); }EXPORT_SYMBOL_GPL (virtqueue_add_inbuf_ctx);
@@ -2941,7 +2978,7 @@ int virtqueue_add_inbuf_premapped(struct virtqueue *vq, void *ctx, gfp_t gfp) { -return virtqueue_add (vq, &sg, num,0 ,1 , data, ctx, true, gfp); +return virtqueue_add (vq, &sg, num,0 ,1 , data, ctx, true, gfp,0 ); }EXPORT_SYMBOL_GPL (virtqueue_add_inbuf_premapped);
include/linux/virtio.h +5 -0
@@ -62,6 +62,11 @@ int virtqueue_add_inbuf(struct virtqueue *vq,void *data, gfp_t gfp); +int virtqueue_add_inbuf_cache_clean (struct virtqueue *vq, +struct scatterlist sg[],unsigned int num, +void *data, +gfp_t gfp); + int virtqueue_add_inbuf_ctx(struct virtqueue *vq, struct scatterlist sg[], unsigned int num, void *data,
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v2 7/15 vsock/virtio: fix DMA alignment for event_list
In reply to Michael S. TsirkinOn non-cache-coherent platforms, when a structure contains a buffer used for DMA alongside fields that the CPU writes to, cacheline sharing can cause data corruption.
The event_list array is used for DMA_FROM_DEVICE operations via virtqueue_add_inbuf(). The adjacent event_run and guest_cid fields are written by the CPU while the buffer is available, so mapped for the device. If these share cachelines with event_list, CPU writes can corrupt DMA data.
Add __dma_from_device_group_begin()/end() annotations to ensure event_list is isolated in its own cachelines.
--- net/vmw_vsock/virtio_transport.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
net/vmw_vsock/virtio_transport.c +3 -1
@@ -17,6 +17,7 @@#include <linux/virtio_ids.h> #include <linux/virtio_config.h> #include <linux/virtio_vsock.h> +#include <linux/dma-mapping.h> #include <net/sock.h> #include <linux/mutex.h> #include <net/af_vsock.h>
@@ -59,8 +60,9 @@ struct virtio_vsock {*/ struct mutex event_lock;bool event_run; +__dma_from_device_group_begin ();struct virtio_vsock_event event_list[8 ]; - +__dma_from_device_group_end ();u32 guest_cid;bool seqpacket_allow;
Signature · 1 line
MST
Stefano Garzarella
maintainerRe: [PATCH v2 07/15] vsock/virtio: fix DMA alignment for event_list
Quoted 31 lines · Michael S. Tsirkin
On non-cache-coherent platforms, when a structure contains a buffer used for DMA alongside fields that the CPU writes to, cacheline sharing can cause data corruption.
The
event_listarray is used forDMA_FROM_DEVICEoperations viavirtqueue_add_inbuf(). The adjacentevent_runandguest_cidfields are written by the CPU while the buffer is available, so mapped for the device. If these share cachelines withevent_list, CPU writes can corrupt DMA data.Add __dma_from_device_group_begin()/end() annotations to ensure
event_listis isolated in its own cachelines.Signed-off-by Michael S. Tsirkin <[email protected]>---
net/vmw_vsock/virtio_transport.c| 4 +++-1 file changed, 3 insertions(+), 1 deletion(-)net/vmw_vsock/virtio_transport.c +1
-0@@ -17,6 +17,7 @@#include <linux/virtio_ids.h> #include <linux/virtio_config.h> #include <linux/virtio_vsock.h> +#include <linux/dma-mapping.h> #include <net/sock.h> #include <linux/mutex.h> #include <net/af_vsock.h>
net/vmw_vsock/virtio_transport.c struct virtio_vsock { · line 65
Quoted 7 lines
net/vmw_vsock/virtio_transport.c +2
-1@@ -59,8 +60,9 @@ struct virtio_vsock {*/ struct mutex event_lock;bool event_run; +__dma_from_device_group_begin ();struct virtio_vsock_event event_list[8 ]; - +__dma_from_device_group_end ();
Can we keep the blank line before guest_cid so that the comment before this section makes sense? (regarding the lock required to access these fields)
Thanks,
Stefano
Quoted 6 lines
net/vmw_vsock/virtio_transport.c +0
-0u32 guest_cid;bool seqpacket_allow;Signature · 1 line
MST
Michael S. Tsirkin
maintainerRe: [PATCH v2 07/15] vsock/virtio: fix DMA alignment for event_list
Quoted 44 lines · Stefano Garzarella
Quoted 31 lines · Michael S. Tsirkin
On non-cache-coherent platforms, when a structure contains a buffer used for DMA alongside fields that the CPU writes to, cacheline sharing can cause data corruption.
The
event_listarray is used forDMA_FROM_DEVICEoperations viavirtqueue_add_inbuf(). The adjacentevent_runandguest_cidfields are written by the CPU while the buffer is available, so mapped for the device. If these share cachelines withevent_list, CPU writes can corrupt DMA data.Add __dma_from_device_group_begin()/end() annotations to ensure
event_listis isolated in its own cachelines.Signed-off-by Michael S. Tsirkin <[email protected]>---
net/vmw_vsock/virtio_transport.c| 4 +++-1 file changed, 3 insertions(+), 1 deletion(-)net/vmw_vsock/virtio_transport.c +1
-0@@ -17,6 +17,7 @@#include <linux/virtio_ids.h> #include <linux/virtio_config.h> #include <linux/virtio_vsock.h> +#include <linux/dma-mapping.h> #include <net/sock.h> #include <linux/mutex.h> #include <net/af_vsock.h> net/vmw_vsock/virtio_transport.c struct virtio_vsock { · line 65
Quoted 7 lines
net/vmw_vsock/virtio_transport.c +2
-1@@ -59,8 +60,9 @@ struct virtio_vsock {*/ struct mutex event_lock;bool event_run; +__dma_from_device_group_begin ();struct virtio_vsock_event event_list[8 ]; - +__dma_from_device_group_end ();Can we keep the blank line before
guest_cidso that the comment before this section makes sense? (regarding the lock required to access these fields)
Quoted 2 lines
Thanks,
Stefano
A follow up patch re-introduces it, so I don't think it matters?
Quoted 6 lines
Quoted 6 lines
net/vmw_vsock/virtio_transport.c +0
-0u32 guest_cid;bool seqpacket_allow;Signature · 1 line
MST
Re: [PATCH v2 07/15] vsock/virtio: fix DMA alignment for event_list
Quoted 45 lines · Michael S. Tsirkin
Quoted 44 lines · Stefano Garzarella
Quoted 31 lines · Michael S. Tsirkin
On non-cache-coherent platforms, when a structure contains a buffer used for DMA alongside fields that the CPU writes to, cacheline sharing can cause data corruption.
The
event_listarray is used forDMA_FROM_DEVICEoperations viavirtqueue_add_inbuf(). The adjacentevent_runandguest_cidfields are written by the CPU while the buffer is available, so mapped for the device. If these share cachelines withevent_list, CPU writes can corrupt DMA data.Add __dma_from_device_group_begin()/end() annotations to ensure
event_listis isolated in its own cachelines.Signed-off-by Michael S. Tsirkin <[email protected]>---
net/vmw_vsock/virtio_transport.c| 4 +++-1 file changed, 3 insertions(+), 1 deletion(-)net/vmw_vsock/virtio_transport.c +1
-0@@ -17,6 +17,7 @@#include <linux/virtio_ids.h> #include <linux/virtio_config.h> #include <linux/virtio_vsock.h> +#include <linux/dma-mapping.h> #include <net/sock.h> #include <linux/mutex.h> #include <net/af_vsock.h> net/vmw_vsock/virtio_transport.c struct virtio_vsock { · line 65
Quoted 7 lines
net/vmw_vsock/virtio_transport.c +2
-1@@ -59,8 +60,9 @@ struct virtio_vsock {*/ struct mutex event_lock;bool event_run; +__dma_from_device_group_begin ();struct virtio_vsock_event event_list[8 ]; - +__dma_from_device_group_end ();Can we keep the blank line before
guest_cidso that the comment before this section makes sense? (regarding the lock required to access these fields)
Quoted 4 lines
Quoted 2 lines
Thanks,
StefanoA follow up patch re-introduces it, so I don't think it matters?
Yes, I saw it later. Of course I don't want you to resend the whole series just for this. So if you have to resend the series for other reasons, I would avoid removing the line here because I don't see any value on removing it and add back later.
In both cases:
Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
net/vmw_vsock/virtio_transport.c +0
-1u32 guest_cid;bool seqpacket_allow; --MST
Michael S. Tsirkin
maintainerPATCH v2 8/15 vsock/virtio: use virtqueue_add_inbuf_cache_clean for events
In reply to Michael S. TsirkinThe event_list array contains 8 small (4-byte) events that share cachelines with each other. When CONFIG_DMA_API_DEBUG is enabled, this can trigger warnings about overlapping DMA mappings within the same cacheline.
The previous patch isolated event_list in its own cache lines so the warnings are spurious.
Use virtqueue_add_inbuf_cache_clean() to indicate that the CPU does not write into these fields, suppressing the warnings.
--- net/vmw_vsock/virtio_transport.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/vmw_vsock/virtio_transport.c +1 -1
@@ -392,7 +392,7 @@ static int virtio_vsock_event_fill_one(struct virtio_vsock *vsock,sg_init_one (&sg, event,sizeof (*event)); -return virtqueue_add_inbuf (vq, &sg,1 , event,GFP_KERNEL ); +return virtqueue_add_inbuf_cache_clean (vq, &sg,1 , event,GFP_KERNEL ); }/* event_lock must be held */
Signature · 1 line
MST
Re: [PATCH v2 08/15] vsock/virtio: use virtqueue_add_inbuf_cache_clean for events
Quoted 8 lines · Michael S. Tsirkin
The
event_listarray contains 8 small (4-byte) events that share cachelines with each other. WhenCONFIG_DMA_API_DEBUGis enabled, this can trigger warnings about overlapping DMA mappings within the same cacheline.The previous patch isolated
event_listin its own cache lines so the warnings are spurious.
Quoted 8 lines
Use
virtqueue_add_inbuf_cache_clean()to indicate that the CPU does not write into these fields, suppressing the warnings.Reported-by Cong Wang <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>---
net/vmw_vsock/virtio_transport.c| 2 +-1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 17 lines
net/vmw_vsock/virtio_transport.c +1
-1@@ -392,7 +392,7 @@ static int virtio_vsock_event_fill_one(struct virtio_vsock *vsock,sg_init_one (&sg, event,sizeof (*event)); -return virtqueue_add_inbuf (vq, &sg,1 , event,GFP_KERNEL ); +return virtqueue_add_inbuf_cache_clean (vq, &sg,1 , event,GFP_KERNEL ); }/* event_lock must be held */ Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v2 9/15 virtio_input: fix DMA alignment for evts
In reply to Michael S. TsirkinOn non-cache-coherent platforms, when a structure contains a buffer used for DMA alongside fields that the CPU writes to, cacheline sharing can cause data corruption.
The evts array is used for DMA_FROM_DEVICE operations via virtqueue_add_inbuf(). The adjacent lock and ready fields are written by the CPU during normal operation. If these share cachelines with evts, CPU writes can corrupt DMA data.
Add __dma_from_device_group_begin()/end() annotations to ensure evts is isolated in its own cachelines.
--- drivers/virtio/virtio_input.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/virtio/virtio_input.c +3 -0
@@ -4,6 +4,7 @@#include <linux/virtio_config.h> #include <linux/input.h> #include <linux/slab.h> +#include <linux/dma-mapping.h> #include <uapi/linux/virtio_ids.h> #include <uapi/linux/virtio_input.h>
@@ -16,7 +17,9 @@ struct virtio_input {char serial[64 ];char phys[64 ];struct virtqueue *evt, *sts; +__dma_from_device_group_begin ();struct virtio_input_event evts[64 ]; +__dma_from_device_group_end ();spinlock_t lock;bool ready; };
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v2 10/15 virtio_scsi: fix DMA cacheline issues for events
In reply to Michael S. TsirkinCurrent struct virtio_scsi_event_node layout has two problems:
The event (DMA_FROM_DEVICE) and work (CPU-written via INIT_WORK/queue_work) fields share a cacheline. On non-cache-coherent platforms, CPU writes to work can corrupt device-written event data.
If ARCH_DMA_MINALIGN is large enough, the 8 events in event_list share cachelines, triggering CONFIG_DMA_API_DEBUG warnings.
Fix the corruption by moving event buffers to a separate array and aligning using __dma_from_device_group_begin()/end().
Suppress the (now spurious) DMA debug warnings using virtqueue_add_inbuf_cache_clean().
--- drivers/scsi/virtio_scsi.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)
drivers/scsi/virtio_scsi.c +12 -5
@@ -29,6 +29,7 @@#include <scsi/scsi_tcq.h> #include <scsi/scsi_devinfo.h> #include <linux/seqlock.h> +#include <linux/dma-mapping.h> #include "sd.h"
@@ -61,7 +62,7 @@ struct virtio_scsi_cmd {struct virtio_scsi_event_node {struct virtio_scsi *vscsi; -struct virtio_scsi_event event; +struct virtio_scsi_event *event;struct work_struct work; };
@@ -89,6 +90,11 @@ struct virtio_scsi {struct virtio_scsi_vq ctrl_vq;struct virtio_scsi_vq event_vq; + +__dma_from_device_group_begin (); +struct virtio_scsi_event events[VIRTIO_SCSI_EVENT_LEN ]; +__dma_from_device_group_end (); +struct virtio_scsi_vq req_vqs[]; };
@@ -237,12 +243,12 @@ static int virtscsi_kick_event(struct virtio_scsi *vscsi,unsigned long flags;INIT_WORK (&event_node->work, virtscsi_handle_event); -sg_init_one (&sg, &event_node->event,sizeof (struct virtio_scsi_event )); +sg_init_one (&sg, event_node->event,sizeof (struct virtio_scsi_event ));spin_lock_irqsave (&vscsi->event_vq.vq_lock, flags); - err =virtqueue_add_inbuf (vscsi->event_vq.vq, &sg,1 , event_node, -GFP_ATOMIC ); + err =virtqueue_add_inbuf_cache_clean (vscsi->event_vq.vq, &sg,1 , event_node, +GFP_ATOMIC );if (!err)virtqueue_kick (vscsi->event_vq.vq);
@@ -257,6 +263,7 @@ static int virtscsi_kick_event_all(struct virtio_scsi *vscsi)for (i =0 ; i <VIRTIO_SCSI_EVENT_LEN ; i++) { vscsi->event_list[i].vscsi = vscsi; + vscsi->event_list[i].event = &vscsi->events[i];virtscsi_kick_event (vscsi, &vscsi->event_list[i]); }
@@ -380,7 +387,7 @@ static void virtscsi_handle_event(struct work_struct *work)struct virtio_scsi_event_node *event_node = container_of(work, struct virtio_scsi_event_node, work);struct virtio_scsi *vscsi = event_node->vscsi; -struct virtio_scsi_event *event = &event_node->event; +struct virtio_scsi_event *event = event_node->event;if (event->event &cpu_to_virtio32 (vscsi->vdev,VIRTIO_SCSI_T_EVENTS_MISSED )) {
Signature · 1 line
MST
Stefan Hajnoczi
maintainerRe: [PATCH v2 10/15] virtio_scsi: fix DMA cacheline issues for events
Quoted 10 lines · Michael S. Tsirkin
drivers/scsi/virtio_scsi.c +1
-1@@ -61,7 +62,7 @@ struct virtio_scsi_cmd {struct virtio_scsi_event_node {struct virtio_scsi *vscsi; -struct virtio_scsi_event event; +struct virtio_scsi_event *event;struct work_struct work; };
drivers/scsi/virtio_scsi.c struct virtio_scsi { · line 96
Quoted 7 lines
drivers/scsi/virtio_scsi.c +4
-0@@ -89,6 +90,11 @@ struct virtio_scsi {struct virtio_scsi_vq ctrl_vq;struct virtio_scsi_vq event_vq; + +__dma_from_device_group_begin (); +struct virtio_scsi_event events[VIRTIO_SCSI_EVENT_LEN ]; +__dma_from_device_group_end ();
If the device emits two events in rapid succession, could the CPU see stale data for the second event because it already holds the cache line for reading the first event?
In other words, it's not obvious to me that the DMA warnings are indeed spurious and should be silenced here.
It seems safer and simpler to align and pad the struct virtio_scsi_event field in struct virtio_scsi_event_node rather than packing these structs into a single array here they might share cache lines.
Stefan
Michael S. Tsirkin
maintainerRe: [PATCH v2 10/15] virtio_scsi: fix DMA cacheline issues for events
Quoted 12 lines · Stefan Hajnoczi
Quoted 11 lines · Michael S. Tsirkin
drivers/scsi/virtio_scsi.c +1
-1@@ -61,7 +62,7 @@ struct virtio_scsi_cmd {struct virtio_scsi_event_node {struct virtio_scsi *vscsi; -struct virtio_scsi_event event; +struct virtio_scsi_event *event;struct work_struct work; };@@ -89,6 +90,11 @@ struct virtio_scsi {
Quoted 10 lines
drivers/scsi/virtio_scsi.c struct virtio_scsi { · line 96
Quoted 6 lines
drivers/scsi/virtio_scsi.c +4
-0struct virtio_scsi_vq ctrl_vq;struct virtio_scsi_vq event_vq; + +__dma_from_device_group_begin (); +struct virtio_scsi_event events[VIRTIO_SCSI_EVENT_LEN ]; +__dma_from_device_group_end ();If the device emits two events in rapid succession, could the CPU see stale data for the second event because it already holds the cache line for reading the first event?
No because virtio does unmap and syncs the cache line.
In other words, CPU reads cause no issues.
The issues are exclusively around CPU writes dirtying the cache and writeback overwriting DMA data.
Quoted 8 lines
In other words, it's not obvious to me that the DMA warnings are indeed spurious and should be silenced here.
It seems safer and simpler to align and pad the struct
virtio_scsi_eventfield in structvirtio_scsi_event_noderather than packing these structs into a single array here they might share cache lines.Stefan
Re: [PATCH v2 10/15] virtio_scsi: fix DMA cacheline issues for events
Quoted 24 lines · Michael S. Tsirkin
Quoted 12 lines · Stefan Hajnoczi
Quoted 11 lines · Michael S. Tsirkin
drivers/scsi/virtio_scsi.c +1
-1@@ -61,7 +62,7 @@ struct virtio_scsi_cmd {struct virtio_scsi_event_node {struct virtio_scsi *vscsi; -struct virtio_scsi_event event; +struct virtio_scsi_event *event;struct work_struct work; };@@ -89,6 +90,11 @@ struct virtio_scsi {Quoted 10 lines
drivers/scsi/virtio_scsi.c struct virtio_scsi { · line 96
Quoted 6 lines
drivers/scsi/virtio_scsi.c +4
-0struct virtio_scsi_vq ctrl_vq;struct virtio_scsi_vq event_vq; + +__dma_from_device_group_begin (); +struct virtio_scsi_event events[VIRTIO_SCSI_EVENT_LEN ]; +__dma_from_device_group_end ();If the device emits two events in rapid succession, could the CPU see stale data for the second event because it already holds the cache line for reading the first event?
Quoted 6 lines
No because virtio does unmap and syncs the cache line.
In other words, CPU reads cause no issues.
The issues are exclusively around CPU writes dirtying the cache and writeback overwriting DMA data.
I see. In that case I'm happy with the virtio-scsi change:
Michael S. Tsirkin
maintainerRe: [PATCH v2 10/15] virtio_scsi: fix DMA cacheline issues for events
In reply to Stefan HajnocziQuoted 23 lines · Stefan Hajnoczi
Quoted 10 lines · Michael S. Tsirkin
drivers/scsi/virtio_scsi.c +1
-1@@ -61,7 +62,7 @@ struct virtio_scsi_cmd {struct virtio_scsi_event_node {struct virtio_scsi *vscsi; -struct virtio_scsi_event event; +struct virtio_scsi_event *event;struct work_struct work; };drivers/scsi/virtio_scsi.c struct virtio_scsi { · line 96
Quoted 7 lines
drivers/scsi/virtio_scsi.c +4
-0@@ -89,6 +90,11 @@ struct virtio_scsi {struct virtio_scsi_vq ctrl_vq;struct virtio_scsi_vq event_vq; + +__dma_from_device_group_begin (); +struct virtio_scsi_event events[VIRTIO_SCSI_EVENT_LEN ]; +__dma_from_device_group_end ();If the device emits two events in rapid succession, could the CPU see stale data for the second event because it already holds the cache line for reading the first event?
Quoted 8 lines
In other words, it's not obvious to me that the DMA warnings are indeed spurious and should be silenced here.
It seems safer and simpler to align and pad the struct
virtio_scsi_eventfield in structvirtio_scsi_event_noderather than packing these structs into a single array here they might share cache lines.Stefan
To add to what I wrote, that's a lot of overhead: 8 * 128 - about 1K on some platforms, and these happen to be low end ones.
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v2 11/15 virtio-rng: fix DMA alignment for data buffer
In reply to Michael S. TsirkinThe data buffer in struct virtrng_info is used for DMA_FROM_DEVICE via virtqueue_add_inbuf() and shares cachelines with the adjacent CPU-written fields (data_avail, data_idx).
The device writing to the DMA buffer and the CPU writing to adjacent fields could corrupt each other's data on non-cache-coherent platforms.
Add __dma_from_device_group_begin()/end() annotations to place these in distinct cache lines.
--- drivers/char/hw_random/virtio-rng.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/char/hw_random/virtio-rng.c +3 -0
@@ -11,6 +11,7 @@#include <linux/spinlock.h> #include <linux/virtio.h> #include <linux/virtio_rng.h> +#include <linux/dma-mapping.h> #include <linux/module.h> #include <linux/slab.h>
@@ -28,11 +29,13 @@ struct virtrng_info {unsigned int data_avail;unsigned int data_idx;/* minimal size returned by rng_buffer_size() */ +__dma_from_device_group_begin ();#if SMP_CACHE_BYTES <32 u8 data[32 ];#else u8 data[SMP_CACHE_BYTES ];#endif +__dma_from_device_group_end (); };static void random_recv_done (struct virtqueue *vq)
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v2 12/15 virtio_input: use virtqueue_add_inbuf_cache_clean for events
In reply to Michael S. TsirkinThe evts array contains 64 small (8-byte) input events that share cachelines with each other. When CONFIG_DMA_API_DEBUG is enabled, this can trigger warnings about overlapping DMA mappings within the same cacheline.
Previous patch isolated the array in its own cachelines, so the warnings are now spurious.
Use virtqueue_add_inbuf_cache_clean() to indicate that the CPU does not write into these cache lines, suppressing these warnings.
--- drivers/virtio/virtio_input.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/virtio/virtio_input.c +1 -1
@@ -30,7 +30,7 @@ static void virtinput_queue_evtbuf(struct virtio_input *vi,struct scatterlist sg[1 ];sg_init_one (sg, evtbuf,sizeof (*evtbuf)); -virtqueue_add_inbuf (vi->evt, sg,1 , evtbuf,GFP_ATOMIC ); +virtqueue_add_inbuf_cache_clean (vi->evt, sg,1 , evtbuf,GFP_ATOMIC ); }static void virtinput_recv_events (struct virtqueue *vq)
Signature · 1 line
MST
Michael S. Tsirkin
maintainerPATCH v2 13/15 vsock/virtio: reorder fields to reduce padding
In reply to Michael S. TsirkinReorder struct virtio_vsock fields to place the DMA buffer (event_list) last. This eliminates the padding from aligning the struct size on ARCH_DMA_MINALIGN.
--- net/vmw_vsock/virtio_transport.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
net/vmw_vsock/virtio_transport.c +5 -3
@@ -60,9 +60,7 @@ struct virtio_vsock {*/ struct mutex event_lock;bool event_run; -__dma_from_device_group_begin (); -struct virtio_vsock_event event_list[8 ]; -__dma_from_device_group_end (); +u32 guest_cid;bool seqpacket_allow;
@@ -76,6 +74,10 @@ struct virtio_vsock {*/ struct scatterlist *out_sgs[MAX_SKB_FRAGS +1 ];struct scatterlist out_bufs[MAX_SKB_FRAGS +1 ]; + +__dma_from_device_group_begin (); +struct virtio_vsock_event event_list[8 ]; +__dma_from_device_group_end (); };static u32 virtio_transport_get_local_cid (void )
Signature · 1 line
MST
Stefano Garzarella
maintainerRe: [PATCH v2 13/15] vsock/virtio: reorder fields to reduce padding
Quoted 25 lines · Michael S. Tsirkin
Reorder struct
virtio_vsockfields to place the DMA buffer (event_list) last. This eliminates the padding from aligning the struct size onARCH_DMA_MINALIGN.Signed-off-by Michael S. Tsirkin <[email protected]>---
net/vmw_vsock/virtio_transport.c| 8 +++++---1 file changed, 5 insertions(+), 3 deletions(-)net/vmw_vsock/virtio_transport.c +1
-3@@ -60,9 +60,7 @@ struct virtio_vsock {*/ struct mutex event_lock;bool event_run; -__dma_from_device_group_begin (); -struct virtio_vsock_event event_list[8 ]; -__dma_from_device_group_end (); +u32 guest_cid;bool seqpacket_allow;
net/vmw_vsock/virtio_transport.c struct virtio_vsock { · line 77
Quoted 4 lines
net/vmw_vsock/virtio_transport.c +1
-0@@ -76,6 +74,10 @@ struct virtio_vsock {*/ struct scatterlist *out_sgs[MAX_SKB_FRAGS +1 ];struct scatterlist out_bufs[MAX_SKB_FRAGS +1 ]; +
IIUC we would like to have these fields always on the bottom of this struct, so would be better to add a comment here to make sure we will not add other fields in the future after this?
Maybe we should also add a comment about the event_lock requirement we have in the section above.
Thanks,
Stefano
Quoted 9 lines
net/vmw_vsock/virtio_transport.c +3
-0+__dma_from_device_group_begin (); +struct virtio_vsock_event event_list[8 ]; +__dma_from_device_group_end (); };static u32 virtio_transport_get_local_cid (void )Signature · 1 line
MST
Michael S. Tsirkin
maintainerRe: [PATCH v2 13/15] vsock/virtio: reorder fields to reduce padding
Quoted 25 lines · Stefano Garzarella
Quoted 24 lines · Michael S. Tsirkin
Reorder struct
virtio_vsockfields to place the DMA buffer (event_list) last. This eliminates the padding from aligning the struct size onARCH_DMA_MINALIGN.Signed-off-by Michael S. Tsirkin <[email protected]>---
net/vmw_vsock/virtio_transport.c| 8 +++++---1 file changed, 5 insertions(+), 3 deletions(-)net/vmw_vsock/virtio_transport.c +1
-3@@ -60,9 +60,7 @@ struct virtio_vsock {*/ struct mutex event_lock;bool event_run; -__dma_from_device_group_begin (); -struct virtio_vsock_event event_list[8 ]; -__dma_from_device_group_end (); +u32 guest_cid;bool seqpacket_allow;
Quoted 9 lines
net/vmw_vsock/virtio_transport.c struct virtio_vsock { · line 77
Quoted 5 lines
net/vmw_vsock/virtio_transport.c +1
-0@@ -76,6 +74,10 @@ struct virtio_vsock {*/ struct scatterlist *out_sgs[MAX_SKB_FRAGS +1 ];struct scatterlist out_bufs[MAX_SKB_FRAGS +1 ]; +IIUC we would like to have these fields always on the bottom of this struct, so would be better to add a comment here to make sure we will not add other fields in the future after this?
not necessarily - you can add fields after, too - it's just that __dma_from_device_group_begin already adds a bunch of padding, so adding fields in this padding is cheaper.
do we really need to add comments to teach people about the art of struct packing?
Quoted 5 lines
Maybe we should also add a comment about the
event_lockrequirement we have in the section above.Thanks,
Stefano
hmm which requirement do you mean?
Quoted 10 lines
Quoted 10 lines
net/vmw_vsock/virtio_transport.c +3
-0+__dma_from_device_group_begin (); +struct virtio_vsock_event event_list[8 ]; +__dma_from_device_group_end (); };static u32 virtio_transport_get_local_cid (void )Signature · 1 line
MST
Stefano Garzarella
maintainerRe: [PATCH v2 13/15] vsock/virtio: reorder fields to reduce padding
Quoted 36 lines · Michael S. Tsirkin
Quoted 25 lines · Stefano Garzarella
Quoted 24 lines · Michael S. Tsirkin
Reorder struct
virtio_vsockfields to place the DMA buffer (event_list) last. This eliminates the padding from aligning the struct size onARCH_DMA_MINALIGN.Signed-off-by Michael S. Tsirkin <[email protected]>---
net/vmw_vsock/virtio_transport.c| 8 +++++---1 file changed, 5 insertions(+), 3 deletions(-)net/vmw_vsock/virtio_transport.c +1
-3@@ -60,9 +60,7 @@ struct virtio_vsock {*/ struct mutex event_lock;bool event_run; -__dma_from_device_group_begin (); -struct virtio_vsock_event event_list[8 ]; -__dma_from_device_group_end (); +u32 guest_cid;bool seqpacket_allow;Quoted 9 lines
net/vmw_vsock/virtio_transport.c struct virtio_vsock { · line 77
Quoted 5 lines
net/vmw_vsock/virtio_transport.c +1
-0@@ -76,6 +74,10 @@ struct virtio_vsock {*/ struct scatterlist *out_sgs[MAX_SKB_FRAGS +1 ];struct scatterlist out_bufs[MAX_SKB_FRAGS +1 ]; +IIUC we would like to have these fields always on the bottom of this struct, so would be better to add a comment here to make sure we will not add other fields in the future after this?
Quoted 4 lines
not necessarily - you can add fields after, too - it's just that
__dma_from_device_group_beginalready adds a bunch of padding, so adding fields in this padding is cheaper.
Okay, I see.
Quoted 3 lines
do we really need to add comments to teach people about the art of struct packing?
I can do it later if you prefer, I don't want to block this work, but yes, I'd prefer to have a comment because otherwise I'll have to ask every time to avoid, especially for new contributors xD
Quoted 8 lines
Quoted 6 lines
Maybe we should also add a comment about the
evnt_lock` requirement we have in the section above.Thanks,
Stefanohmm which requirement do you mean?
That event_list must be accessed with event_lock.
So maybe we can move also event_lock and event_run, so we can just move that comment. I mean something like this:
@@ -74,6 +67,15 @@ struct virtio_vsock {
*/
struct scatterlist *out_sgs[MAX_SKB_FRAGS + 1];
struct scatterlist out_bufs[MAX_SKB_FRAGS + 1];+
+ /* The following fields are protected by event_lock.
+ * vqs[VSOCK_VQ_EVENT] must be accessed with event_lock held.
+ */
+ struct mutex event_lock; + bool event_run; + __dma_from_device_group_begin(); + struct virtio_vsock_event event_list[8]; + __dma_from_device_group_end();
};
static u32 virtio_transport_get_local_cid(void)
Thanks,
Stefano
Michael S. Tsirkin
maintainerRe: [PATCH v2 13/15] vsock/virtio: reorder fields to reduce padding
Quoted 45 lines · Stefano Garzarella
Quoted 36 lines · Michael S. Tsirkin
Quoted 25 lines · Stefano Garzarella
Quoted 24 lines · Michael S. Tsirkin
Reorder struct
virtio_vsockfields to place the DMA buffer (event_list) last. This eliminates the padding from aligning the struct size onARCH_DMA_MINALIGN.Signed-off-by Michael S. Tsirkin <[email protected]>---
net/vmw_vsock/virtio_transport.c| 8 +++++---1 file changed, 5 insertions(+), 3 deletions(-)net/vmw_vsock/virtio_transport.c +1
-3@@ -60,9 +60,7 @@ struct virtio_vsock {*/ struct mutex event_lock;bool event_run; -__dma_from_device_group_begin (); -struct virtio_vsock_event event_list[8 ]; -__dma_from_device_group_end (); +u32 guest_cid;bool seqpacket_allow;Quoted 9 lines
net/vmw_vsock/virtio_transport.c struct virtio_vsock { · line 77
Quoted 5 lines
net/vmw_vsock/virtio_transport.c +1
-0@@ -76,6 +74,10 @@ struct virtio_vsock {*/ struct scatterlist *out_sgs[MAX_SKB_FRAGS +1 ];struct scatterlist out_bufs[MAX_SKB_FRAGS +1 ]; +IIUC we would like to have these fields always on the bottom of this struct, so would be better to add a comment here to make sure we will not add other fields in the future after this?
Quoted 4 lines
not necessarily - you can add fields after, too - it's just that
__dma_from_device_group_beginalready adds a bunch of padding, so adding fields in this padding is cheaper.Okay, I see.
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
do we really need to add comments to teach people about the art of struct packing?
I can do it later if you prefer, I don't want to block this work, but yes, I'd prefer to have a comment because otherwise I'll have to ask every time to avoid, especially for new contributors xD
On the one hand you are right on the other I don't want it duplicated each time __dma_from_device_group_begin is invoked. Pls come up with something you like, and we'll discuss.
Quoted 22 lines
Quoted 9 lines
Quoted 7 lines
Maybe we should also add a comment about the
evnt_lock` requirement we
have in the section above.Thanks,
Stefanohmm which requirement do you mean?
That
event_listmust be accessed withevent_lock.So maybe we can move also
event_lockandevent_run, so we can just move that comment. I mean something like this:net/vmw_vsock/virtio_transport.c +2
-0@@ -74,6 +67,15 @@ struct virtio_vsock {*/ struct scatterlist *out_sgs[MAX_SKB_FRAGS +1 ];struct scatterlist out_bufs[MAX_SKB_FRAGS +1 ]; + +/* The following fields are protected by event_lock.
net/vmw_vsock/virtio_transport.c struct virtio_vsock { · line 81
Quoted 10 lines
net/vmw_vsock/virtio_transport.c +7
-0+* vqs[VSOCK_VQ_EVENT] must be accessed with event_lock held. +*/ +struct mutex event_lock; +bool event_run; +__dma_from_device_group_begin (); +struct virtio_vsock_event event_list[8 ]; +__dma_from_device_group_end (); };static u32 virtio_transport_get_local_cid (void )
Yea this makes sense.
Quoted 3 lines
Thanks,
Stefano
Stefano Garzarella
maintainerRe: [PATCH v2 13/15] vsock/virtio: reorder fields to reduce padding
Quoted 49 lines · Michael S. Tsirkin
Quoted 48 lines · Stefano Garzarella
Quoted 36 lines · Michael S. Tsirkin
Quoted 25 lines · Stefano Garzarella
Quoted 24 lines · Michael S. Tsirkin
Reorder struct
virtio_vsockfields to place the DMA buffer (event_list) last. This eliminates the padding from aligning the struct size onARCH_DMA_MINALIGN.Signed-off-by Michael S. Tsirkin <[email protected]>---
net/vmw_vsock/virtio_transport.c| 8 +++++---1 file changed, 5 insertions(+), 3 deletions(-)net/vmw_vsock/virtio_transport.c +1
-3@@ -60,9 +60,7 @@ struct virtio_vsock {*/ struct mutex event_lock;bool event_run; -__dma_from_device_group_begin (); -struct virtio_vsock_event event_list[8 ]; -__dma_from_device_group_end (); +u32 guest_cid;bool seqpacket_allow;Quoted 9 lines
net/vmw_vsock/virtio_transport.c struct virtio_vsock { · line 77
Quoted 5 lines
net/vmw_vsock/virtio_transport.c +1
-0@@ -76,6 +74,10 @@ struct virtio_vsock {*/ struct scatterlist *out_sgs[MAX_SKB_FRAGS +1 ];struct scatterlist out_bufs[MAX_SKB_FRAGS +1 ]; +IIUC we would like to have these fields always on the bottom of this struct, so would be better to add a comment here to make sure we will not add other fields in the future after this?
Quoted 4 lines
not necessarily - you can add fields after, too - it's just that
__dma_from_device_group_beginalready adds a bunch of padding, so adding fields in this padding is cheaper.Okay, I see.
Quoted 3 lines
do we really need to add comments to teach people about the art of struct packing?
Quoted 6 lines
Quoted 3 lines
I can do it later if you prefer, I don't want to block this work, but yes, I'd prefer to have a comment because otherwise I'll have to ask every time to avoid, especially for new contributors xD
On the one hand you are right on the other I don't want it duplicated each time
__dma_from_device_group_beginis invoked.
yeah, I see.
Quoted 1 line
Pls come up with something you like, and we'll discuss.
sure, I'll check a bit similar cases to have some inspiration.
Quoted 32 lines
Quoted 32 lines
Quoted 10 lines
Quoted 8 lines
Maybe we should also add a comment about the
evnt_lock` requirement we
have in the section above.Thanks,
Stefanohmm which requirement do you mean?
That
event_listmust be accessed withevent_lock.So maybe we can move also
event_lockandevent_run, so we can just move that comment. I mean something like this:net/vmw_vsock/virtio_transport.c +9
-0@@ -74,6 +67,15 @@ struct virtio_vsock {*/ struct scatterlist *out_sgs[MAX_SKB_FRAGS +1 ];struct scatterlist out_bufs[MAX_SKB_FRAGS +1 ]; + +/* The following fields are protected by event_lock. +* vqs[VSOCK_VQ_EVENT] must be accessed with event_lock held. +*/ +struct mutex event_lock; +bool event_run; +__dma_from_device_group_begin (); +struct virtio_vsock_event event_list[8 ]; +__dma_from_device_group_end (); };
Quoted 3 lines
net/vmw_vsock/virtio_transport.c struct virtio_vsock { · line 83
Quoted 1 line
net/vmw_vsock/virtio_transport.c +0
-0static u32 virtio_transport_get_local_cid (void )Yea this makes sense.
Thanks for that!
Stefano
Michael S. Tsirkin
maintainerPATCH v2 14/15 gpio: virtio: fix DMA alignment
In reply to Michael S. TsirkinThe res and ires buffers in struct virtio_gpio_line and struct vgpio_irq_line respectively are used for DMA_FROM_DEVICE via virtqueue_add_sgs(). However, within these structs, even though these elements are tagged as ____cacheline_aligned, adjacent struct elements can share DMA cachelines on platforms where ARCH_DMA_MINALIGN > L1_CACHE_BYTES (e.g., arm64 with 128-byte DMA alignment but 64-byte cache lines).
The existing ____cacheline_aligned annotation aligns to L1_CACHE_BYTES which is not always sufficient for DMA alignment. For example, with L1_CACHE_BYTES = 32 and ARCH_DMA_MINALIGN = 128
- irq_lines[0].ires at offset 128
- irq_lines[1].type at offset 192
both in same 128-byte DMA cacheline [128-256)
When the device writes to irq_lines[0].ires and the CPU concurrently modifies one of irq_lines[1].type/disabled/masked/queued flags, corruption can occur on non-cache-coherent platforms.
Fix by using __dma_from_device_group_begin()/end() annotations on the DMA buffers. Drop ____cacheline_aligned - it's not required to isolate request and response, and keeping them would increase the memory cost.
--- drivers/gpio/gpio-virtio.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/gpio/gpio-virtio.c +10 -4
@@ -10,6 +10,7 @@*/ #include <linux/completion.h> +#include <linux/dma-mapping.h> #include <linux/err.h> #include <linux/gpio/driver.h> #include <linux/io.h>
@@ -24,8 +25,11 @@struct virtio_gpio_line {struct mutex lock;/* Protects line operation */ struct completion completion; -struct virtio_gpio_request req____cacheline_aligned ; -struct virtio_gpio_response res____cacheline_aligned ; + + __dma_from_device_group_begin(); +struct virtio_gpio_request req; +struct virtio_gpio_response res; + __dma_from_device_group_end();unsigned int rxlen; };
@@ -37,8 +41,10 @@ struct vgpio_irq_line {bool update_pending;bool queue_pending; -struct virtio_gpio_irq_request ireq____cacheline_aligned ; -struct virtio_gpio_irq_response ires____cacheline_aligned ; +__dma_from_device_group_begin (); +struct virtio_gpio_irq_request ireq; +struct virtio_gpio_irq_response ires; +__dma_from_device_group_end (); };struct virtio_gpio {
Signature · 1 line
MST
Re: [PATCH v2 14/15] gpio: virtio: fix DMA alignment
Quoted 19 lines · Michael S. Tsirkin
The res and ires buffers in struct
virtio_gpio_lineand structvgpio_irq_linerespectively are used forDMA_FROM_DEVICEviavirtqueue_add_sgs(). However, within these structs, even though these elements are tagged as____cacheline_aligned, adjacent struct elements can share DMA cachelines on platforms whereARCH_DMA_MINALIGN>L1_CACHE_BYTES(e.g., arm64 with 128-byte DMA alignment but 64-byte cache lines).The existing
____cacheline_alignedannotation aligns toL1_CACHE_BYTESwhich is not always sufficient for DMA alignment. For example, withL1_CACHE_BYTES= 32 andARCH_DMA_MINALIGN= 128
- irq_lines[0].ires at offset 128
- irq_lines[1].type at offset 192
both in same 128-byte DMA cacheline [128-256)When the device writes to irq_lines[0].ires and the CPU concurrently modifies one of irq_lines[1].type/disabled/masked/queued flags, corruption can occur on non-cache-coherent platforms.
Quoted 7 lines
Fix by using __dma_from_device_group_begin()/end() annotations on the DMA buffers. Drop
____cacheline_aligned- it's not required to isolate request and response, and keeping them would increase the memory cost.Acked-by Viresh Kumar <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>---
Michael S. Tsirkin
maintainerPATCH v2 15/15 gpio: virtio: reorder fields to reduce struct padding
In reply to Michael S. TsirkinReorder struct virtio_gpio_line fields to place the DMA buffers (req/res) last.
This eliminates the padding from aligning struct size on ARCH_DMA_MINALIGN.
--- drivers/gpio/gpio-virtio.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpio/gpio-virtio.c +2 -1
@@ -26,11 +26,12 @@ struct virtio_gpio_line {struct mutex lock;/* Protects line operation */ struct completion completion; +unsigned int rxlen; +__dma_from_device_group_begin ();struct virtio_gpio_request req;struct virtio_gpio_response res;__dma_from_device_group_end (); -unsigned int rxlen; };struct vgpio_irq_line {
Signature · 1 line
MST
Re: [PATCH v2 15/15] gpio: virtio: reorder fields to reduce struct padding
Quoted 31 lines · Michael S. Tsirkin
Reorder struct
virtio_gpio_linefields to place the DMA buffers (req/res) last.This eliminates the padding from aligning struct size on
ARCH_DMA_MINALIGN.Acked-by Viresh Kumar <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>--- drivers/gpio/gpio-virtio.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/gpio/gpio-virtio.c +2
-2@@ -26,11 +26,12 @@ struct virtio_gpio_line {struct mutex lock;/* Protects line operation */ struct completion completion; +unsigned int rxlen; +__dma_from_device_group_begin ();struct virtio_gpio_request req;struct virtio_gpio_response res;__dma_from_device_group_end (); -unsigned int rxlen; };struct vgpio_irq_line { --
Quoted 3 lines
MST
Michael S. Tsirkin
maintainerPATCH v3 15/15 vsock/virtio: reorder fields to reduce padding
In reply to Michael S. TsirkinReorder struct virtio_vsock fields to place the DMA buffer (event_list) last. This eliminates the padding from aligning the struct size on ARCH_DMA_MINALIGN.
---
changes from v2:
move event_lock and event_run too, to keep event things logically together, as suggested by Stefano Garzarella.
Note: this is the only change in v3 and it's cosmetic, so I am not reposting the whole patchset.
net/vmw_vsock/virtio_transport.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
net/vmw_vsock/virtio_transport.c +9 -9
@@ -55,15 +55,6 @@ struct virtio_vsock {int rx_buf_nr;int rx_buf_max_nr; -/* The following fields are protected by event_lock. -* vqs[VSOCK_VQ_EVENT] must be accessed with event_lock held. -*/ -struct mutex event_lock; -bool event_run; -__dma_from_device_group_begin (); -struct virtio_vsock_event event_list[8 ]; -__dma_from_device_group_end (); -u32 guest_cid;bool seqpacket_allow;
@@ -77,6 +68,15 @@ struct virtio_vsock {*/ struct scatterlist *out_sgs[MAX_SKB_FRAGS +1 ];struct scatterlist out_bufs[MAX_SKB_FRAGS +1 ]; + +/* The following fields are protected by event_lock. +* vqs[VSOCK_VQ_EVENT] must be accessed with event_lock held. +*/ +struct mutex event_lock; +bool event_run; +__dma_from_device_group_begin (); +struct virtio_vsock_event event_list[8 ]; +__dma_from_device_group_end (); };static u32 virtio_transport_get_local_cid (void )
Signature · 1 line
MST
Re: [PATCH v3 15/15] vsock/virtio: reorder fields to reduce padding
Quoted 4 lines · Michael S. Tsirkin
Reorder struct
virtio_vsockfields to place the DMA buffer (event_list) last. This eliminates the padding from aligning the struct size onARCH_DMA_MINALIGN.
Quoted 8 lines
Suggested-by Stefano Garzarella <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>---
changes from v2:
moveevent_lockandevent_runtoo, to keep event things logically together, as suggested by Stefano Garzarella.
Thanks for that!
Quoted 47 lines
Note: this is the only change in v3 and it's cosmetic, so I am not reposting the whole patchset.
net/vmw_vsock/virtio_transport.c| 18 +++++++++---------1 file changed, 9 insertions(+), 9 deletions(-)net/vmw_vsock/virtio_transport.c +9
-9@@ -55,15 +55,6 @@ struct virtio_vsock {int rx_buf_nr;int rx_buf_max_nr; -/* The following fields are protected by event_lock. -* vqs[VSOCK_VQ_EVENT] must be accessed with event_lock held. -*/ -struct mutex event_lock; -bool event_run; -__dma_from_device_group_begin (); -struct virtio_vsock_event event_list[8 ]; -__dma_from_device_group_end (); -u32 guest_cid;bool seqpacket_allow;@@ -77,6 +68,15 @@ struct virtio_vsock {*/ struct scatterlist *out_sgs[MAX_SKB_FRAGS +1 ];struct scatterlist out_bufs[MAX_SKB_FRAGS +1 ]; + +/* The following fields are protected by event_lock. +* vqs[VSOCK_VQ_EVENT] must be accessed with event_lock held. +*/ +struct mutex event_lock; +bool event_run; +__dma_from_device_group_begin (); +struct virtio_vsock_event event_list[8 ]; +__dma_from_device_group_end (); };static u32 virtio_transport_get_local_cid (void )Signature · 1 line
MST