if driver reads data larger than VIRTIO_BLK_F_SIZE_MAX, it will cause some issue to the DMA engine. So add VIRTIO_BLK_F_SIZE_MAX feature support, when upper software wants to read data larger than VIRTIO_BLK_F_SIZE_MAX, virtio-blk driver split one large request into multiple smaller ones.
Andy Pei (3): virtio-blk: add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX virtio-blk: abstract a function named virtio_blk_op_one_segment to handle r/w request virtio-blk.: split large IO according to size_max
src/block.h | 2 + src/hw/virtio-blk.c | 132 +++++++++++++++++++++++++++++++++++++++++++++------- src/hw/virtio-blk.h | 3 ++ 3 files changed, 120 insertions(+), 17 deletions(-)
according to virtio spec, add feature VIRTIO_BLK_F_SIZE_MAX and VIRTIO_BLK_F_SEG_MAX parse to virtio blk driver.
Signed-off-by: Andy Pei andy.pei@intel.com Signed-off-by: Ding Limin dinglimin@cmss.chinamobile.com --- src/block.h | 2 ++ src/hw/virtio-blk.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++-- src/hw/virtio-blk.h | 3 +++ 3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/src/block.h b/src/block.h index c1b8d73..3af2d71 100644 --- a/src/block.h +++ b/src/block.h @@ -57,6 +57,8 @@ struct drive_s { u8 translation; // type of translation u16 blksize; // block size struct chs_s pchs; // Physical CHS + u32 max_segment_size; //max_segment_size + u32 max_segments; //max_segments };
#define DISK_SECTOR_SIZE 512 diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index 3b19896..7935f0f 100644 --- a/src/hw/virtio-blk.c +++ b/src/hw/virtio-blk.c @@ -121,12 +121,16 @@ init_virtio_blk(void *data) u64 version1 = 1ull << VIRTIO_F_VERSION_1; u64 iommu_platform = 1ull << VIRTIO_F_IOMMU_PLATFORM; u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE; + u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX; + u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX; + if (!(features & version1)) { dprintf(1, "modern device without virtio_1 feature bit: %pP\n", pci); goto fail; }
- features = features & (version1 | iommu_platform | blk_size); + features = features & (version1 | iommu_platform | blk_size + | max_segments | max_segment_size); vp_set_features(vp, features); status |= VIRTIO_CONFIG_S_FEATURES_OK; vp_set_status(vp, status); @@ -135,6 +139,22 @@ init_virtio_blk(void *data) goto fail; }
+ if (features & max_segment_size) + vdrive->drive.max_segment_size = + vp_read(&vp->device, struct virtio_blk_config, size_max); + else + vdrive->drive.max_segment_size = 0; + dprintf(3, "virtio-blk %pP size_max=%u.\n", pci, + vdrive->drive.max_segment_size); + + if (features & max_segments) + vdrive->drive.max_segments = + vp_read(&vp->device, struct virtio_blk_config, seg_max); + else + vdrive->drive.max_segments = 0; + dprintf(3, "virtio-blk %pP seg_max=%u.\n", pci, + vdrive->drive.max_segments); + vdrive->drive.sectors = vp_read(&vp->device, struct virtio_blk_config, capacity); if (features & blk_size) { @@ -177,6 +197,21 @@ init_virtio_blk(void *data) vdrive->drive.pchs.cylinder = cfg.cylinders; vdrive->drive.pchs.head = cfg.heads; vdrive->drive.pchs.sector = cfg.sectors; + + if (f & (1 << VIRTIO_BLK_F_SIZE_MAX)) + vdrive->drive.max_segment_size = cfg.size_max; + else + vdrive->drive.max_segment_size = 0; + dprintf(3, "virtio-blk %pP size_max=%u.\n", pci, + vdrive->drive.max_segment_size); + + if (f & (1 << VIRTIO_BLK_F_SEG_MAX)) + vdrive->drive.max_segments = cfg.seg_max; + else + vdrive->drive.max_segments = 0; + dprintf(3, "virtio-blk %pP seg_max=%u.\n", pci, + vdrive->drive.max_segments); + }
char *desc = znprintf(MAXDESCSIZE, "Virtio disk PCI:%pP", pci); @@ -218,8 +253,11 @@ init_virtio_blk_mmio(void *mmio) u64 features = vp_get_features(vp); u64 version1 = 1ull << VIRTIO_F_VERSION_1; u64 blk_size = 1ull << VIRTIO_BLK_F_BLK_SIZE; + u64 max_segments = 1ull << VIRTIO_BLK_F_SEG_MAX; + u64 max_segment_size = 1ull << VIRTIO_BLK_F_SIZE_MAX;
- features = features & (version1 | blk_size); + features = features & (version1 | blk_size + | max_segments | max_segment_size); vp_set_features(vp, features); status |= VIRTIO_CONFIG_S_FEATURES_OK; vp_set_status(vp, status); @@ -228,6 +266,20 @@ init_virtio_blk_mmio(void *mmio) goto fail; }
+ if (features & max_segment_size) + vdrive->drive.max_segment_size = + vp_read(&vp->device, struct virtio_blk_config, size_max); + else + vdrive->drive.max_segment_size = 0; + dprintf(3, "virtio-blk size_max=%u.\n", vdrive->drive.max_segment_size); + + if (features & max_segments) + vdrive->drive.max_segments = + vp_read(&vp->device, struct virtio_blk_config, seg_max); + else + vdrive->drive.max_segments = 0; + dprintf(3, "virtio-blk seg_max=%u.\n", vdrive->drive.max_segments); + vdrive->drive.sectors = vp_read(&vp->device, struct virtio_blk_config, capacity); if (features & blk_size) { diff --git a/src/hw/virtio-blk.h b/src/hw/virtio-blk.h index d20461a..0294291 100644 --- a/src/hw/virtio-blk.h +++ b/src/hw/virtio-blk.h @@ -16,6 +16,9 @@ struct virtio_blk_config u32 opt_io_size; } __attribute__((packed));
+/* Feature bits */ +#define VIRTIO_BLK_F_SIZE_MAX 1 /* Maximum size of any single segment */ +#define VIRTIO_BLK_F_SEG_MAX 2 /* Maximum number of segments in a request */ #define VIRTIO_BLK_F_BLK_SIZE 6
/* These two define direction. */
Hi,
if (features & max_segment_size)vdrive->drive.max_segment_size =vp_read(&vp->device, struct virtio_blk_config, size_max);
elsevdrive->drive.max_segment_size = 0;
Not needed, there is a memset(0) for the complete vdrive struct.
dprintf(3, "virtio-blk %pP size_max=%u.\n", pci,vdrive->drive.max_segment_size);
Can you just add that to the existing dprintf instead of adding a new line to the log?
thanks, Gerd
Reply in line
if (features & max_segment_size)vdrive->drive.max_segment_size =vp_read(&vp->device, struct virtio_blk_config,- size_max);
elsevdrive->drive.max_segment_size = 0;
Not needed, there is a memset(0) for the complete vdrive struct. Andy: OK, address it in V4.
dprintf(3, "virtio-blk %pP size_max=%u.\n", pci,vdrive->drive.max_segment_size);
Can you just add that to the existing dprintf instead of adding a new line to the log? Andy: OK, address it in V4.
thanks, Gerd
abstract virtio-blk queue operation to form a function named virtio_blk_op_one_segment
Signed-off-by: Andy Pei andy.pei@intel.com Signed-off-by: Ding Limin dinglimin@cmss.chinamobile.com --- src/hw/virtio-blk.c | 48 ++++++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 20 deletions(-)
diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index 7935f0f..0bc8743 100644 --- a/src/hw/virtio-blk.c +++ b/src/hw/virtio-blk.c @@ -30,12 +30,38 @@ struct virtiodrive_s { struct vp_device vp; };
+void +virtio_blk_op_one_segment(struct virtiodrive_s *vdrive, + int write, struct vring_list sg[]) +{ + struct vring_virtqueue *vq = vdrive->vq; + + /* Add to virtqueue and kick host */ + if (write) + vring_add_buf(vq, sg, 2, 1, 0, 0); + else + vring_add_buf(vq, sg, 1, 2, 0, 0); + vring_kick(&vdrive->vp, vq, 1); + + /* Wait for reply */ + while (!vring_more_used(vq)) + usleep(5); + + /* Reclaim virtqueue element */ + vring_get_buf(vq, NULL); + + /** + ** Clear interrupt status register. Avoid leaving interrupts stuck + ** if VRING_AVAIL_F_NO_INTERRUPT was ignored and interrupts were raised. + **/ + vp_get_isr(&vdrive->vp); +} + static int virtio_blk_op(struct disk_op_s *op, int write) { struct virtiodrive_s *vdrive = container_of(op->drive_fl, struct virtiodrive_s, drive); - struct vring_virtqueue *vq = vdrive->vq; struct virtio_blk_outhdr hdr = { .type = write ? VIRTIO_BLK_T_OUT : VIRTIO_BLK_T_IN, .ioprio = 0, @@ -57,25 +83,7 @@ virtio_blk_op(struct disk_op_s *op, int write) }, };
- /* Add to virtqueue and kick host */ - if (write) - vring_add_buf(vq, sg, 2, 1, 0, 0); - else - vring_add_buf(vq, sg, 1, 2, 0, 0); - vring_kick(&vdrive->vp, vq, 1); - - /* Wait for reply */ - while (!vring_more_used(vq)) - usleep(5); - - /* Reclaim virtqueue element */ - vring_get_buf(vq, NULL); - - /* Clear interrupt status register. Avoid leaving interrupts stuck if - * VRING_AVAIL_F_NO_INTERRUPT was ignored and interrupts were raised. - */ - vp_get_isr(&vdrive->vp); - + virtio_blk_op_one_segment(vdrive, write, sg); return status == VIRTIO_BLK_S_OK ? DISK_RET_SUCCESS : DISK_RET_EBADTRACK; }
On Fri, Dec 03, 2021 at 01:22:43PM +0800, Andy Pei wrote:
abstract virtio-blk queue operation to form a function named virtio_blk_op_one_segment
Signed-off-by: Andy Pei andy.pei@intel.com Signed-off-by: Ding Limin dinglimin@cmss.chinamobile.com
Reviewed-by: Gerd Hoffmann kraxel@redhat.com
if driver reads data larger than VIRTIO_BLK_F_SIZE_MAX, it will cause some issue to the DMA engine.
So when upper software wants to read data larger than VIRTIO_BLK_F_SIZE_MAX, virtio-blk driver split one large request into multiple smaller ones.
Signed-off-by: Andy Pei andy.pei@intel.com Signed-off-by: Ding Limin dinglimin@cmss.chinamobile.com --- src/hw/virtio-blk.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index 0bc8743..664396c 100644 --- a/src/hw/virtio-blk.c +++ b/src/hw/virtio-blk.c @@ -82,8 +82,46 @@ virtio_blk_op(struct disk_op_s *op, int write) .length = sizeof(status), }, }; + u32 max_io_size = + vdrive->drive.max_segment_size * vdrive->drive.max_segments; + u16 blk_num_max;
- virtio_blk_op_one_segment(vdrive, write, sg); + if (vdrive->drive.blksize != 0 && max_io_size != 0) + blk_num_max = (u16)max_io_size / vdrive->drive.blksize; + else + /* default blk_num_max if hardware doesnot advise a proper value */ + blk_num_max = 8; + + if (op->count <= blk_num_max) { + virtio_blk_op_one_segment(vdrive, write, sg); + } else { + struct vring_list *p_sg = &sg[1]; + void *p = op->buf_fl; + u16 count = op->count; + + while (count > blk_num_max) { + p_sg->addr = p; + p_sg->length = vdrive->drive.blksize * blk_num_max; + virtio_blk_op_one_segment(vdrive, write, sg); + if (status == VIRTIO_BLK_S_OK) { + hdr.sector += blk_num_max; + p += p_sg->length; + count -= blk_num_max; + } else { + break; + } + } + + if (status != VIRTIO_BLK_S_OK) + return DISK_RET_EBADTRACK; + + if (count > 0) { + p_sg->addr = p; + p_sg->length = vdrive->drive.blksize * count; + virtio_blk_op_one_segment(vdrive, write, sg); + } + + } return status == VIRTIO_BLK_S_OK ? DISK_RET_SUCCESS : DISK_RET_EBADTRACK; }
- } else {
struct vring_list *p_sg = &sg[1];
Not needed, you can just use sg[1].addr
void *p = op->buf_fl;u16 count = op->count;while (count > blk_num_max) {
while (count > 0) { u16 blk_num = min(count, blk_num_max);
} else {break;}}if (status != VIRTIO_BLK_S_OK)return DISK_RET_EBADTRACK;
Can be moved into the while loop, you don't need the break then.
if (count > 0) {p_sg->addr = p;p_sg->length = vdrive->drive.blksize * count;virtio_blk_op_one_segment(vdrive, write, sg);}
Not needed with the change above.
take care, Gerd
Reply in line
- } else {
struct vring_list *p_sg = &sg[1];
Not needed, you can just use sg[1].addr Andy: I used to think sg[1]. Length, but I am not sure. Could you please tell me why sg[1]. Length is not needed ?
void *p = op->buf_fl;u16 count = op->count;while (count > blk_num_max) {
while (count > 0) { u16 blk_num = min(count, blk_num_max);
} else {break;}}if (status != VIRTIO_BLK_S_OK)return DISK_RET_EBADTRACK;
Can be moved into the while loop, you don't need the break then. Andy: OK, address it in V4.
if (count > 0) {p_sg->addr = p;p_sg->length = vdrive->drive.blksize * count;virtio_blk_op_one_segment(vdrive, write, sg);}
Not needed with the change above. Andy: OK, address it in V4.
take care, Gerd
On Mon, Dec 06, 2021 at 01:38:16PM +0000, Pei, Andy wrote:
Reply in line
- } else {
struct vring_list *p_sg = &sg[1];Not needed, you can just use sg[1].addr Andy: I used to think sg[1]. Length, but I am not sure. Could you please tell me why sg[1]. Length is not needed ?
length too of course, but you can likewise use sg[1].length and there is no need to create that p_sg pointer.
take care, Gerd
-
Andy Pei -
Gerd Hoffmann -
Pei, Andy