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. */
if driver reads data larger than VIRTIO_BLK_F_SIZE_MAX, it will cause some issue to the DMA engine.
Signed-off-by: Andy Pei andy.pei@intel.com Signed-off-by: Ding Limin dinglimin@cmss.chinamobile.com --- src/hw/virtio-blk.c | 92 +++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 78 insertions(+), 14 deletions(-)
diff --git a/src/hw/virtio-blk.c b/src/hw/virtio-blk.c index 7935f0f..2068823 100644 --- a/src/hw/virtio-blk.c +++ b/src/hw/virtio-blk.c @@ -30,6 +30,33 @@ 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) { @@ -56,26 +83,63 @@ 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;
- /* Add to virtqueue and kick host */ - if (write) - vring_add_buf(vq, sg, 2, 1, 0, 0); + if (vdrive->drive.blksize != 0 && max_io_size != 0) + blk_num_max = (u16)max_io_size / vdrive->drive.blksize; else - vring_add_buf(vq, sg, 1, 2, 0, 0); - vring_kick(&vdrive->vp, vq, 1); + /* default blk_num_max if hardware doesnot advise a proper value */ + blk_num_max = 8;
- /* Wait for reply */ - while (!vring_more_used(vq)) - usleep(5); + if (op->count <= blk_num_max) { + /* 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);
- /* Reclaim virtqueue element */ - vring_get_buf(vq, NULL); + /* Wait for reply */ + while (!vring_more_used(vq)) + usleep(5);
- /* 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); + /* 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); + } 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; }
On Wed, Dec 01, 2021 at 09:22:02PM +0800, Andy Pei wrote:
if driver reads data larger than VIRTIO_BLK_F_SIZE_MAX, it will cause some issue to the DMA engine.
More verbose commit messages please.
+void +virtio_blk_op_one_segment(struct virtiodrive_s *vdrive,
while (count > blk_num_max) {
So this is two changes in one:
(1) move code to the new virtio_blk_op_one_segment() helper function without changing functionality, and (2) actually fix the issue by splitting large requests into multiple smaller ones.
Can we have that as two patches please? It's easier to review the actual code changes in (2) then.
thanks, Gerd
-
Andy Pei -
Gerd Hoffmann