Hi,
here is the third version of this patch-set. Previous
versions can be found here:
V1: https://lore.kernel.org/lkml/[email protected]/
V2: https://lore.kernel.org/lkml/[email protected]/
The problem solved here is a limitation of the SWIOTLB implementation,
which does not support allocations larger than 256kb. When the
virtio-blk driver tries to read/write a block larger than that, the
allocation of the dma-handle fails and an IO error is reported.
Changes to v2 are:
* Check if SWIOTLB is active before returning its limit in
dma_direct_max_mapping_size()
* Only apply the maximum segment limit in virtio-blk when
DMA-API is used for the vring
Please review.
Thanks,
Joerg
Joerg Roedel (5):
swiotlb: Introduce swiotlb_max_mapping_size()
swiotlb: Add is_swiotlb_active() function
dma: Introduce dma_max_mapping_size()
virtio: Introduce virtio_max_dma_size()
virtio-blk: Consider virtio_max_dma_size() for maximum segment size
drivers/block/virtio_blk.c | 10 ++++++----
drivers/virtio/virtio_ring.c | 10 ++++++++++
include/linux/dma-mapping.h | 16 ++++++++++++++++
include/linux/swiotlb.h | 11 +++++++++++
include/linux/virtio.h | 2 ++
kernel/dma/direct.c | 11 +++++++++++
kernel/dma/swiotlb.c | 10 ++++++++++
7 files changed, 66 insertions(+), 4 deletions(-)
--
2.17.1
From: Joerg Roedel <[email protected]>
The function returns the maximum size that can be mapped
using DMA-API functions. The patch also adds the
implementation for direct DMA and a new dma_map_ops pointer
so that other implementations can expose their limit.
Signed-off-by: Joerg Roedel <[email protected]>
---
include/linux/dma-mapping.h | 16 ++++++++++++++++
kernel/dma/direct.c | 11 +++++++++++
2 files changed, 27 insertions(+)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index f6ded992c183..a3ca8a71a704 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -130,6 +130,7 @@ struct dma_map_ops {
enum dma_data_direction direction);
int (*dma_supported)(struct device *dev, u64 mask);
u64 (*get_required_mask)(struct device *dev);
+ size_t (*max_mapping_size)(struct device *dev);
};
#define DMA_MAPPING_ERROR (~(dma_addr_t)0)
@@ -257,6 +258,8 @@ static inline void dma_direct_sync_sg_for_cpu(struct device *dev,
}
#endif
+size_t dma_direct_max_mapping_size(struct device *dev);
+
#ifdef CONFIG_HAS_DMA
#include <asm/dma-mapping.h>
@@ -440,6 +443,19 @@ static inline int dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
return 0;
}
+static inline size_t dma_max_mapping_size(struct device *dev)
+{
+ const struct dma_map_ops *ops = get_dma_ops(dev);
+ size_t size = SIZE_MAX;
+
+ if (dma_is_direct(ops))
+ size = dma_direct_max_mapping_size(dev);
+ else if (ops && ops->max_mapping_size)
+ size = ops->max_mapping_size(dev);
+
+ return size;
+}
+
void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
gfp_t flag, unsigned long attrs);
void dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 355d16acee6d..6310ad01f915 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -380,3 +380,14 @@ int dma_direct_supported(struct device *dev, u64 mask)
*/
return mask >= __phys_to_dma(dev, min_mask);
}
+
+size_t dma_direct_max_mapping_size(struct device *dev)
+{
+ size_t size = SIZE_MAX;
+
+ /* If SWIOTLB is active, use its maximum mapping size */
+ if (is_swiotlb_active())
+ size = swiotlb_max_mapping_size(dev);
+
+ return size;
+}
--
2.17.1
From: Joerg Roedel <[email protected]>
Segments can't be larger than the maximum DMA mapping size
supported on the platform. Take that into account when
setting the maximum segment size for a block device.
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/block/virtio_blk.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index b16a887bbd02..4bc083b7c9b5 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -723,7 +723,7 @@ static int virtblk_probe(struct virtio_device *vdev)
struct request_queue *q;
int err, index;
- u32 v, blk_size, sg_elems, opt_io_size;
+ u32 v, blk_size, max_size, sg_elems, opt_io_size;
u16 min_io_size;
u8 physical_block_exp, alignment_offset;
@@ -826,14 +826,16 @@ static int virtblk_probe(struct virtio_device *vdev)
/* No real sector limit. */
blk_queue_max_hw_sectors(q, -1U);
+ max_size = virtio_max_dma_size(vdev);
+
/* Host can optionally specify maximum segment size and number of
* segments. */
err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SIZE_MAX,
struct virtio_blk_config, size_max, &v);
if (!err)
- blk_queue_max_segment_size(q, v);
- else
- blk_queue_max_segment_size(q, -1U);
+ max_size = min(max_size, v);
+
+ blk_queue_max_segment_size(q, max_size);
/* Host can optionally specify the block size of the device */
err = virtio_cread_feature(vdev, VIRTIO_BLK_F_BLK_SIZE,
--
2.17.1
From: Joerg Roedel <[email protected]>
This function returns the maximum segment size for a single
dma transaction of a virtio device. The possible limit comes
from the SWIOTLB implementation in the Linux kernel, that
has an upper limit of (currently) 256kb of contiguous
memory it can map. Other DMA-API implementations might also
have limits.
Use the new dma_max_mapping_size() function to determine the
maximum mapping size when DMA-API is in use for virtio.
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/virtio/virtio_ring.c | 10 ++++++++++
include/linux/virtio.h | 2 ++
2 files changed, 12 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index cd7e755484e3..9ca3fe6af9fa 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -266,6 +266,16 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
return false;
}
+size_t virtio_max_dma_size(struct virtio_device *vdev)
+{
+ size_t max_segment_size = SIZE_MAX;
+
+ if (vring_use_dma_api(vdev))
+ max_segment_size = dma_max_mapping_size(&vdev->dev);
+
+ return max_segment_size;
+}
+
static void *vring_alloc_queue(struct virtio_device *vdev, size_t size,
dma_addr_t *dma_handle, gfp_t flag)
{
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index fa1b5da2804e..673fe3ef3607 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -157,6 +157,8 @@ int virtio_device_freeze(struct virtio_device *dev);
int virtio_device_restore(struct virtio_device *dev);
#endif
+size_t virtio_max_dma_size(struct virtio_device *vdev);
+
#define virtio_device_for_each_vq(vdev, vq) \
list_for_each_entry(vq, &vdev->vqs, list)
--
2.17.1
From: Joerg Roedel <[email protected]>
This function will be used from dma_direct code to determine
the maximum segment size of a dma mapping.
Signed-off-by: Joerg Roedel <[email protected]>
---
include/linux/swiotlb.h | 6 ++++++
kernel/dma/swiotlb.c | 5 +++++
2 files changed, 11 insertions(+)
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index ceb623321f38..5c087d330b4b 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -63,6 +63,7 @@ extern void swiotlb_tbl_sync_single(struct device *hwdev,
extern int
swiotlb_dma_supported(struct device *hwdev, u64 mask);
extern size_t swiotlb_max_mapping_size(struct device *dev);
+bool is_swiotlb_active(void);
#ifdef CONFIG_SWIOTLB
extern enum swiotlb_force swiotlb_force;
@@ -100,6 +101,11 @@ static inline size_t swiotlb_max_mapping_size(struct device *dev)
{
return SIZE_MAX;
}
+
+static inline bool is_swiotlb_active(void)
+{
+ return false;
+}
#endif /* CONFIG_SWIOTLB */
extern void swiotlb_print_info(void);
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 9cb21259cb0b..9fbd075081d9 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -667,3 +667,8 @@ size_t swiotlb_max_mapping_size(struct device *dev)
{
return ((size_t)1 << IO_TLB_SHIFT) * IO_TLB_SEGSIZE;
}
+
+bool is_swiotlb_active(void)
+{
+ return !no_iotlb_memory;
+}
--
2.17.1
From: Joerg Roedel <[email protected]>
The function returns the maximum size that can be remapped
by the SWIOTLB implementation. This function will be later
exposed to users through the DMA-API.
Signed-off-by: Joerg Roedel <[email protected]>
---
include/linux/swiotlb.h | 5 +++++
kernel/dma/swiotlb.c | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index 7c007ed7505f..ceb623321f38 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -62,6 +62,7 @@ extern void swiotlb_tbl_sync_single(struct device *hwdev,
extern int
swiotlb_dma_supported(struct device *hwdev, u64 mask);
+extern size_t swiotlb_max_mapping_size(struct device *dev);
#ifdef CONFIG_SWIOTLB
extern enum swiotlb_force swiotlb_force;
@@ -95,6 +96,10 @@ static inline unsigned int swiotlb_max_segment(void)
{
return 0;
}
+static inline size_t swiotlb_max_mapping_size(struct device *dev)
+{
+ return SIZE_MAX;
+}
#endif /* CONFIG_SWIOTLB */
extern void swiotlb_print_info(void);
diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 1fb6fd68b9c7..9cb21259cb0b 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -662,3 +662,8 @@ swiotlb_dma_supported(struct device *hwdev, u64 mask)
{
return __phys_to_dma(hwdev, io_tlb_end - 1) <= mask;
}
+
+size_t swiotlb_max_mapping_size(struct device *dev)
+{
+ return ((size_t)1 << IO_TLB_SHIFT) * IO_TLB_SEGSIZE;
+}
--
2.17.1
On Wed, Jan 23, 2019 at 05:30:44PM +0100, Joerg Roedel wrote:
> Hi,
>
> here is the third version of this patch-set. Previous
> versions can be found here:
>
> V1: https://lore.kernel.org/lkml/[email protected]/
>
> V2: https://lore.kernel.org/lkml/[email protected]/
>
> The problem solved here is a limitation of the SWIOTLB implementation,
> which does not support allocations larger than 256kb. When the
> virtio-blk driver tries to read/write a block larger than that, the
> allocation of the dma-handle fails and an IO error is reported.
>
> Changes to v2 are:
>
> * Check if SWIOTLB is active before returning its limit in
> dma_direct_max_mapping_size()
>
> * Only apply the maximum segment limit in virtio-blk when
> DMA-API is used for the vring
>
> Please review.
>
> Thanks,
>
> Joerg
>
> Joerg Roedel (5):
> swiotlb: Introduce swiotlb_max_mapping_size()
> swiotlb: Add is_swiotlb_active() function
> dma: Introduce dma_max_mapping_size()
> virtio: Introduce virtio_max_dma_size()
> virtio-blk: Consider virtio_max_dma_size() for maximum segment size
>
> drivers/block/virtio_blk.c | 10 ++++++----
> drivers/virtio/virtio_ring.c | 10 ++++++++++
The kvm-devel mailing list should have been copied on those.
When you do can you please put 'Reviewed-by: Konrad Rzeszutek Wilk
<[email protected]>' on all of them?
Thank you!
P.S.
Christopher, I am assuming you are OK with this idea, if so - and once
the owners of 'virtio' Ack, do you want to put it in my tree?
Thanks!
> include/linux/dma-mapping.h | 16 ++++++++++++++++
> include/linux/swiotlb.h | 11 +++++++++++
> include/linux/virtio.h | 2 ++
> kernel/dma/direct.c | 11 +++++++++++
> kernel/dma/swiotlb.c | 10 ++++++++++
> 7 files changed, 66 insertions(+), 4 deletions(-)
>
> --
> 2.17.1
>
On Wed, Jan 23, 2019 at 05:30:44PM +0100, Joerg Roedel wrote:
> Hi,
>
> here is the third version of this patch-set. Previous
> versions can be found here:
>
> V1: https://lore.kernel.org/lkml/[email protected]/
>
> V2: https://lore.kernel.org/lkml/[email protected]/
>
> The problem solved here is a limitation of the SWIOTLB implementation,
> which does not support allocations larger than 256kb. When the
> virtio-blk driver tries to read/write a block larger than that, the
> allocation of the dma-handle fails and an IO error is reported.
OK looks good to me.
I will park this in my tree for now this way it will get
testing in linux-next.
Can I get an ack from DMA maintainers on the DMA bits for
merging this in 5.0?
> Changes to v2 are:
>
> * Check if SWIOTLB is active before returning its limit in
> dma_direct_max_mapping_size()
>
> * Only apply the maximum segment limit in virtio-blk when
> DMA-API is used for the vring
>
> Please review.
>
> Thanks,
>
> Joerg
>
> Joerg Roedel (5):
> swiotlb: Introduce swiotlb_max_mapping_size()
> swiotlb: Add is_swiotlb_active() function
> dma: Introduce dma_max_mapping_size()
> virtio: Introduce virtio_max_dma_size()
> virtio-blk: Consider virtio_max_dma_size() for maximum segment size
>
> drivers/block/virtio_blk.c | 10 ++++++----
> drivers/virtio/virtio_ring.c | 10 ++++++++++
> include/linux/dma-mapping.h | 16 ++++++++++++++++
> include/linux/swiotlb.h | 11 +++++++++++
> include/linux/virtio.h | 2 ++
> kernel/dma/direct.c | 11 +++++++++++
> kernel/dma/swiotlb.c | 10 ++++++++++
> 7 files changed, 66 insertions(+), 4 deletions(-)
>
> --
> 2.17.1
On Wed, Jan 23, 2019 at 01:51:29PM -0500, Michael S. Tsirkin wrote:
> On Wed, Jan 23, 2019 at 05:30:44PM +0100, Joerg Roedel wrote:
> > Hi,
> >
> > here is the third version of this patch-set. Previous
> > versions can be found here:
> >
> > V1: https://lore.kernel.org/lkml/[email protected]/
> >
> > V2: https://lore.kernel.org/lkml/[email protected]/
> >
> > The problem solved here is a limitation of the SWIOTLB implementation,
> > which does not support allocations larger than 256kb. When the
> > virtio-blk driver tries to read/write a block larger than that, the
> > allocation of the dma-handle fails and an IO error is reported.
>
>
> OK looks good to me.
> I will park this in my tree for now this way it will get
> testing in linux-next.
> Can I get an ack from DMA maintainers on the DMA bits for
> merging this in 5.0?
You got mine (SWIOTBL is my area).
>
> > Changes to v2 are:
> >
> > * Check if SWIOTLB is active before returning its limit in
> > dma_direct_max_mapping_size()
> >
> > * Only apply the maximum segment limit in virtio-blk when
> > DMA-API is used for the vring
> >
> > Please review.
> >
> > Thanks,
> >
> > Joerg
> >
> > Joerg Roedel (5):
> > swiotlb: Introduce swiotlb_max_mapping_size()
> > swiotlb: Add is_swiotlb_active() function
> > dma: Introduce dma_max_mapping_size()
> > virtio: Introduce virtio_max_dma_size()
> > virtio-blk: Consider virtio_max_dma_size() for maximum segment size
> >
> > drivers/block/virtio_blk.c | 10 ++++++----
> > drivers/virtio/virtio_ring.c | 10 ++++++++++
> > include/linux/dma-mapping.h | 16 ++++++++++++++++
> > include/linux/swiotlb.h | 11 +++++++++++
> > include/linux/virtio.h | 2 ++
> > kernel/dma/direct.c | 11 +++++++++++
> > kernel/dma/swiotlb.c | 10 ++++++++++
> > 7 files changed, 66 insertions(+), 4 deletions(-)
> >
> > --
> > 2.17.1
On Wed, Jan 23, 2019 at 05:30:46PM +0100, Joerg Roedel wrote:
> +bool is_swiotlb_active(void)
> +{
> + return !no_iotlb_memory;
> +}
As I've just introduced and fixed a bug in this area in the current
cycle - I don't think no_iotlb_memory is what your want (and maybe
not useful at all): if the arch valls swiotlb_exit after previously
initializing a buffer it won't be set. You probably want to check
for non-zero io_tlb_start and/or io_tlb_end.
This looks ok, but could really use some documentation in
Documentation/DMA-API.txt.
On Wed, Jan 23, 2019 at 05:30:45PM +0100, Joerg Roedel wrote:
> +extern size_t swiotlb_max_mapping_size(struct device *dev);
No need for the extern keyword on function declarations in headers.
On Wed, Jan 23, 2019 at 05:30:49PM +0100, Joerg Roedel wrote:
> + max_size = virtio_max_dma_size(vdev);
> +
> /* Host can optionally specify maximum segment size and number of
> * segments. */
> err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SIZE_MAX,
> struct virtio_blk_config, size_max, &v);
> if (!err)
> - blk_queue_max_segment_size(q, v);
> - else
> - blk_queue_max_segment_size(q, -1U);
> + max_size = min(max_size, v);
> +
> + blk_queue_max_segment_size(q, max_size);
I wonder if we should just move the dma max segment size check
into blk_queue_max_segment_size so that all block drivers benefit
from it. Even if not I think at least the SCSI midlayer should
be updated to support it.
Btw, I wonder why virtio-scsi sticks to the default segment size,
unlike virtio-blk.
On Wed, Jan 23, 2019 at 10:31:39PM +0100, Christoph Hellwig wrote:
> On Wed, Jan 23, 2019 at 05:30:49PM +0100, Joerg Roedel wrote:
> > + max_size = virtio_max_dma_size(vdev);
> > +
> > /* Host can optionally specify maximum segment size and number of
> > * segments. */
> > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SIZE_MAX,
> > struct virtio_blk_config, size_max, &v);
> > if (!err)
> > - blk_queue_max_segment_size(q, v);
> > - else
> > - blk_queue_max_segment_size(q, -1U);
> > + max_size = min(max_size, v);
> > +
> > + blk_queue_max_segment_size(q, max_size);
>
> I wonder if we should just move the dma max segment size check
> into blk_queue_max_segment_size so that all block drivers benefit
> from it. Even if not I think at least the SCSI midlayer should
> be updated to support it.
>
> Btw, I wonder why virtio-scsi sticks to the default segment size,
> unlike virtio-blk.
Well no one bothered exposing that through that device.
Why does virtio block have it? Hard for me to say. First driver version
had it already but QEMU does not seem to use it and it seems that it
never did.
--
MST
On Wed, Jan 23, 2019 at 10:28:13PM +0100, Christoph Hellwig wrote:
> On Wed, Jan 23, 2019 at 05:30:45PM +0100, Joerg Roedel wrote:
> > +extern size_t swiotlb_max_mapping_size(struct device *dev);
>
> No need for the extern keyword on function declarations in headers.
Right, but all other function declarations in that header file have
'extern' too, so I added it also for that one.
Regards,
Joerg
On Wed, Jan 23, 2019 at 10:27:55PM +0100, Christoph Hellwig wrote:
> On Wed, Jan 23, 2019 at 05:30:46PM +0100, Joerg Roedel wrote:
> > +bool is_swiotlb_active(void)
> > +{
> > + return !no_iotlb_memory;
> > +}
>
> As I've just introduced and fixed a bug in this area in the current
> cycle - I don't think no_iotlb_memory is what your want (and maybe
> not useful at all): if the arch valls swiotlb_exit after previously
> initializing a buffer it won't be set. You probably want to check
> for non-zero io_tlb_start and/or io_tlb_end.
Okay, but that requires that I also set io_tlb_start and friends back to
zero in the failure path of swiotlb_init(). Otherwise it could be left
non-zero in case swiotlb_init_with_tbl() returns an error.
Regards,
Joerg
On Wed, Jan 23, 2019 at 10:31:39PM +0100, Christoph Hellwig wrote:
> On Wed, Jan 23, 2019 at 05:30:49PM +0100, Joerg Roedel wrote:
> > + max_size = virtio_max_dma_size(vdev);
> > +
> > /* Host can optionally specify maximum segment size and number of
> > * segments. */
> > err = virtio_cread_feature(vdev, VIRTIO_BLK_F_SIZE_MAX,
> > struct virtio_blk_config, size_max, &v);
> > if (!err)
> > - blk_queue_max_segment_size(q, v);
> > - else
> > - blk_queue_max_segment_size(q, -1U);
> > + max_size = min(max_size, v);
> > +
> > + blk_queue_max_segment_size(q, max_size);
>
> I wonder if we should just move the dma max segment size check
> into blk_queue_max_segment_size so that all block drivers benefit
> from it. Even if not I think at least the SCSI midlayer should
> be updated to support it.
In that case the limit would also apply to virtio-blk even if it doesn't
use the DMA-API. If that is acceptable we can move the check to
blk_queue_max_segment_size().
Regards,
Joerg
On Thu, Jan 24, 2019 at 09:29:23AM +0100, Joerg Roedel wrote:
> > As I've just introduced and fixed a bug in this area in the current
> > cycle - I don't think no_iotlb_memory is what your want (and maybe
> > not useful at all): if the arch valls swiotlb_exit after previously
> > initializing a buffer it won't be set. You probably want to check
> > for non-zero io_tlb_start and/or io_tlb_end.
>
> Okay, but that requires that I also set io_tlb_start and friends back to
> zero in the failure path of swiotlb_init(). Otherwise it could be left
> non-zero in case swiotlb_init_with_tbl() returns an error.
Indeed, and we'll need to do that anyway as otherwise the dma mapping
path might cause problems similar to the one when swiotlb_exit is
called that I fixed.
On Thu, Jan 24, 2019 at 09:40:11AM +0100, Joerg Roedel wrote:
> > I wonder if we should just move the dma max segment size check
> > into blk_queue_max_segment_size so that all block drivers benefit
> > from it. Even if not I think at least the SCSI midlayer should
> > be updated to support it.
>
> In that case the limit would also apply to virtio-blk even if it doesn't
> use the DMA-API. If that is acceptable we can move the check to
> blk_queue_max_segment_size().
Yes. But more importantly it would fix the limit for all other block
drivers that set large segment sizes when running over swiotlb.
On Thu, Jan 24, 2019 at 09:24:31AM +0100, Joerg Roedel wrote:
> On Wed, Jan 23, 2019 at 10:28:13PM +0100, Christoph Hellwig wrote:
> > On Wed, Jan 23, 2019 at 05:30:45PM +0100, Joerg Roedel wrote:
> > > +extern size_t swiotlb_max_mapping_size(struct device *dev);
> >
> > No need for the extern keyword on function declarations in headers.
>
> Right, but all other function declarations in that header file have
> 'extern' too, so I added it also for that one.
Your patch 3 also doesn't use an extern. And I have to say I
much prefer it that way..
On Thu, Jan 24, 2019 at 09:42:21AM +0100, Christoph Hellwig wrote:
> Yes. But more importantly it would fix the limit for all other block
> drivers that set large segment sizes when running over swiotlb.
True, so it would be something like the diff below? I havn't worked on
the block layer, so I don't know if that needs additional checks for
->dev or anything.
diff --git a/block/blk-settings.c b/block/blk-settings.c
index 3e7038e475ee..9a927280c904 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -1,6 +1,7 @@
/*
* Functions related to setting various queue properties from drivers
*/
+#include <linux/dma-mapping.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
@@ -303,13 +304,17 @@ EXPORT_SYMBOL_GPL(blk_queue_max_discard_segments);
**/
void blk_queue_max_segment_size(struct request_queue *q, unsigned int max_size)
{
+ unsigned int dma_max_size;
+
if (max_size < PAGE_SIZE) {
max_size = PAGE_SIZE;
printk(KERN_INFO "%s: set to minimum %d\n",
__func__, max_size);
}
- q->limits.max_segment_size = max_size;
+ dma_max_size = dma_max_mapping_size(q->backing_dev_info->dev);
+
+ q->limits.max_segment_size = min(max_size, dma_max_size);
}
EXPORT_SYMBOL(blk_queue_max_segment_size);
On Thu, Jan 24, 2019 at 09:41:07AM +0100, Christoph Hellwig wrote:
> On Thu, Jan 24, 2019 at 09:29:23AM +0100, Joerg Roedel wrote:
> > > As I've just introduced and fixed a bug in this area in the current
> > > cycle - I don't think no_iotlb_memory is what your want (and maybe
> > > not useful at all): if the arch valls swiotlb_exit after previously
> > > initializing a buffer it won't be set. You probably want to check
> > > for non-zero io_tlb_start and/or io_tlb_end.
> >
> > Okay, but that requires that I also set io_tlb_start and friends back to
> > zero in the failure path of swiotlb_init(). Otherwise it could be left
> > non-zero in case swiotlb_init_with_tbl() returns an error.
>
> Indeed, and we'll need to do that anyway as otherwise the dma mapping
> path might cause problems similar to the one when swiotlb_exit is
> called that I fixed.
Turns out the the error path in swiotlb_init() is redundant because it
will never be executed. If the function returns it will always return 0
because in case of failure it will just panic (through memblock_alloc).
I'll clean that up in a separate patch-set. There are more users of that
function and all of them panic when the function fails.
Joerg
On Thu, Jan 24, 2019 at 10:51:51AM +0100, Joerg Roedel wrote:
> On Thu, Jan 24, 2019 at 09:42:21AM +0100, Christoph Hellwig wrote:
> > Yes. But more importantly it would fix the limit for all other block
> > drivers that set large segment sizes when running over swiotlb.
>
> True, so it would be something like the diff below? I havn't worked on
> the block layer, so I don't know if that needs additional checks for
> ->dev or anything.
Looks sensible. Maybe for now we'll just do the virtio-blk case
that triggered it, and we'll do something like this patch for the
next merge window. We'll also need to apply the same magic to the
DMA boundary.
On Wed, Jan 23, 2019 at 04:14:53PM -0500, Konrad Rzeszutek Wilk wrote:
> On Wed, Jan 23, 2019 at 01:51:29PM -0500, Michael S. Tsirkin wrote:
> > On Wed, Jan 23, 2019 at 05:30:44PM +0100, Joerg Roedel wrote:
> > > Hi,
> > >
> > > here is the third version of this patch-set. Previous
> > > versions can be found here:
> > >
> > > V1: https://lore.kernel.org/lkml/[email protected]/
> > >
> > > V2: https://lore.kernel.org/lkml/[email protected]/
> > >
> > > The problem solved here is a limitation of the SWIOTLB implementation,
> > > which does not support allocations larger than 256kb. When the
> > > virtio-blk driver tries to read/write a block larger than that, the
> > > allocation of the dma-handle fails and an IO error is reported.
> >
> >
> > OK looks good to me.
> > I will park this in my tree for now this way it will get
> > testing in linux-next.
> > Can I get an ack from DMA maintainers on the DMA bits for
> > merging this in 5.0?
>
> You got mine (SWIOTBL is my area).
OK so
Reviewed-by: Konrad Rzeszutek Wilk <[email protected]>
> >
> > > Changes to v2 are:
> > >
> > > * Check if SWIOTLB is active before returning its limit in
> > > dma_direct_max_mapping_size()
> > >
> > > * Only apply the maximum segment limit in virtio-blk when
> > > DMA-API is used for the vring
> > >
> > > Please review.
> > >
> > > Thanks,
> > >
> > > Joerg
> > >
> > > Joerg Roedel (5):
> > > swiotlb: Introduce swiotlb_max_mapping_size()
> > > swiotlb: Add is_swiotlb_active() function
> > > dma: Introduce dma_max_mapping_size()
> > > virtio: Introduce virtio_max_dma_size()
> > > virtio-blk: Consider virtio_max_dma_size() for maximum segment size
> > >
> > > drivers/block/virtio_blk.c | 10 ++++++----
> > > drivers/virtio/virtio_ring.c | 10 ++++++++++
> > > include/linux/dma-mapping.h | 16 ++++++++++++++++
> > > include/linux/swiotlb.h | 11 +++++++++++
> > > include/linux/virtio.h | 2 ++
> > > kernel/dma/direct.c | 11 +++++++++++
> > > kernel/dma/swiotlb.c | 10 ++++++++++
> > > 7 files changed, 66 insertions(+), 4 deletions(-)
> > >
> > > --
> > > 2.17.1
On Mon, Jan 28, 2019 at 09:05:26AM +0100, Christoph Hellwig wrote:
> On Thu, Jan 24, 2019 at 10:51:51AM +0100, Joerg Roedel wrote:
> > On Thu, Jan 24, 2019 at 09:42:21AM +0100, Christoph Hellwig wrote:
> > > Yes. But more importantly it would fix the limit for all other block
> > > drivers that set large segment sizes when running over swiotlb.
> >
> > True, so it would be something like the diff below? I havn't worked on
> > the block layer, so I don't know if that needs additional checks for
> > ->dev or anything.
>
> Looks sensible. Maybe for now we'll just do the virtio-blk case
> that triggered it, and we'll do something like this patch for the
> next merge window. We'll also need to apply the same magic to the
> DMA boundary.
So do I get an ack for this patch then?
--
MST
On Thu, Jan 24, 2019 at 04:00:00PM +0100, Joerg Roedel wrote:
> On Thu, Jan 24, 2019 at 09:41:07AM +0100, Christoph Hellwig wrote:
> > On Thu, Jan 24, 2019 at 09:29:23AM +0100, Joerg Roedel wrote:
> > > > As I've just introduced and fixed a bug in this area in the current
> > > > cycle - I don't think no_iotlb_memory is what your want (and maybe
> > > > not useful at all): if the arch valls swiotlb_exit after previously
> > > > initializing a buffer it won't be set. You probably want to check
> > > > for non-zero io_tlb_start and/or io_tlb_end.
> > >
> > > Okay, but that requires that I also set io_tlb_start and friends back to
> > > zero in the failure path of swiotlb_init(). Otherwise it could be left
> > > non-zero in case swiotlb_init_with_tbl() returns an error.
> >
> > Indeed, and we'll need to do that anyway as otherwise the dma mapping
> > path might cause problems similar to the one when swiotlb_exit is
> > called that I fixed.
>
> Turns out the the error path in swiotlb_init() is redundant because it
> will never be executed. If the function returns it will always return 0
> because in case of failure it will just panic (through memblock_alloc).
>
> I'll clean that up in a separate patch-set. There are more users of that
> function and all of them panic when the function fails.
>
>
> Joerg
OK so are you going to post a new version then? Time's running out for 5.0.
This isn't a regression so maybe we should just defer it all to 5.1.
--
MST
On Mon, Jan 28, 2019 at 10:20:05AM -0500, Michael S. Tsirkin wrote:
> On Wed, Jan 23, 2019 at 04:14:53PM -0500, Konrad Rzeszutek Wilk wrote:
> > On Wed, Jan 23, 2019 at 01:51:29PM -0500, Michael S. Tsirkin wrote:
> > > On Wed, Jan 23, 2019 at 05:30:44PM +0100, Joerg Roedel wrote:
> > > > Hi,
> > > >
> > > > here is the third version of this patch-set. Previous
> > > > versions can be found here:
> > > >
> > > > V1: https://lore.kernel.org/lkml/[email protected]/
> > > >
> > > > V2: https://lore.kernel.org/lkml/[email protected]/
> > > >
> > > > The problem solved here is a limitation of the SWIOTLB implementation,
> > > > which does not support allocations larger than 256kb. When the
> > > > virtio-blk driver tries to read/write a block larger than that, the
> > > > allocation of the dma-handle fails and an IO error is reported.
> > >
> > >
> > > OK looks good to me.
> > > I will park this in my tree for now this way it will get
> > > testing in linux-next.
> > > Can I get an ack from DMA maintainers on the DMA bits for
> > > merging this in 5.0?
> >
> > You got mine (SWIOTBL is my area).
>
> OK so
>
> Reviewed-by: Konrad Rzeszutek Wilk <[email protected]>
Yes :-)
>
>
>
> > >
> > > > Changes to v2 are:
> > > >
> > > > * Check if SWIOTLB is active before returning its limit in
> > > > dma_direct_max_mapping_size()
> > > >
> > > > * Only apply the maximum segment limit in virtio-blk when
> > > > DMA-API is used for the vring
> > > >
> > > > Please review.
> > > >
> > > > Thanks,
> > > >
> > > > Joerg
> > > >
> > > > Joerg Roedel (5):
> > > > swiotlb: Introduce swiotlb_max_mapping_size()
> > > > swiotlb: Add is_swiotlb_active() function
> > > > dma: Introduce dma_max_mapping_size()
> > > > virtio: Introduce virtio_max_dma_size()
> > > > virtio-blk: Consider virtio_max_dma_size() for maximum segment size
> > > >
> > > > drivers/block/virtio_blk.c | 10 ++++++----
> > > > drivers/virtio/virtio_ring.c | 10 ++++++++++
> > > > include/linux/dma-mapping.h | 16 ++++++++++++++++
> > > > include/linux/swiotlb.h | 11 +++++++++++
> > > > include/linux/virtio.h | 2 ++
> > > > kernel/dma/direct.c | 11 +++++++++++
> > > > kernel/dma/swiotlb.c | 10 ++++++++++
> > > > 7 files changed, 66 insertions(+), 4 deletions(-)
> > > >
> > > > --
> > > > 2.17.1
On Mon, Jan 28, 2019 at 12:14:33PM -0500, Michael S. Tsirkin wrote:
> On Mon, Jan 28, 2019 at 09:05:26AM +0100, Christoph Hellwig wrote:
> > On Thu, Jan 24, 2019 at 10:51:51AM +0100, Joerg Roedel wrote:
> > > On Thu, Jan 24, 2019 at 09:42:21AM +0100, Christoph Hellwig wrote:
> > > > Yes. But more importantly it would fix the limit for all other block
> > > > drivers that set large segment sizes when running over swiotlb.
> > >
> > > True, so it would be something like the diff below? I havn't worked on
> > > the block layer, so I don't know if that needs additional checks for
> > > ->dev or anything.
> >
> > Looks sensible. Maybe for now we'll just do the virtio-blk case
> > that triggered it, and we'll do something like this patch for the
> > next merge window. We'll also need to apply the same magic to the
> > DMA boundary.
>
> So do I get an ack for this patch then?
I'll wait for a resend of the series to review the whole thing.