v1: https://lore.kernel.org/lkml/93f207c0-61e6-3696-f218-e7d7ea9a7c93@redhat.com/
This series is the second part of the v1 linked above. The first part with
refactoring of vdpa_sim has already been merged.
The patches are based on Max Gurtovoy's work and extend the block simulator to
have a ramdisk behaviour.
As mentioned in the v1 there was 2 issues and I fixed them in this series:
1. The identical mapping in the IOMMU used until now in vdpa_sim created issues
when mapping different virtual pages with the same physical address.
Fixed by patch "vdpa_sim: use iova module to allocate IOVA addresses"
2. There was a race accessing the IOMMU between the vdpasim_blk_work() and the
device driver that map/unmap DMA regions. Fixed by patch "vringh: add
'iotlb_lock' to synchronize iotlb accesses"
Since this series is still a RFC, I used the Xie's patch as is to allow
vhost-vdpa to use block devices, but I'll rebase when he splits it into
multiple patches.
The series also includes small fixes for vdpa_sim that I discovered while
implementing the block simulator.
Thanks for your feedback,
Stefano
Max Gurtovoy (1):
vdpa: add vdpa simulator for block device
Stefano Garzarella (8):
vdpa_sim: use iova module to allocate IOVA addresses
vringh: add 'iotlb_lock' to synchronize iotlb accesses
vringh: reset kiov 'consumed' field in __vringh_iov()
vringh: implement vringh_kiov_advance()
vringh: add vringh_kiov_length() helper
vdpa_sim: cleanup kiovs in vdpasim_free()
vdpa_sim_blk: implement ramdisk behaviour
vdpa_sim_blk: handle VIRTIO_BLK_T_GET_ID
Xie Yongji (1):
vdpa: Remove the restriction that only supports virtio-net devices
drivers/vdpa/vdpa_sim/vdpa_sim.h | 2 +
include/linux/vringh.h | 19 +-
drivers/vdpa/vdpa_sim/vdpa_sim.c | 118 +++++++----
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 288 +++++++++++++++++++++++++++
drivers/vhost/vdpa.c | 28 +--
drivers/vhost/vringh.c | 54 +++--
drivers/vdpa/Kconfig | 8 +
drivers/vdpa/vdpa_sim/Makefile | 1 +
8 files changed, 433 insertions(+), 85 deletions(-)
create mode 100644 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
--
2.29.2
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
__vringh_iov() overwrites the contents of riov and wiov, in fact it
resets the 'i' and 'used' fields, but also the consumed field should
be reset to avoid an inconsistent state.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
drivers/vhost/vringh.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
In some cases, it may be useful to provide a way to skip a number
of bytes in a vringh_kiov.
Let's implement vringh_kiov_advance() for this purpose, reusing the
code from vringh_iov_xfer().
We replace that code calling the new vringh_kiov_advance().
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
include/linux/vringh.h | 2 ++
drivers/vhost/vringh.c | 41 +++++++++++++++++++++++++++++------------
2 files changed, 31 insertions(+), 12 deletions(-)
@@ -75,6 +75,34 @@ static inline int __vringh_get_head(const struct vringh *vrh,returnhead;}+/**+*vringh_kiov_advance-skipbytesfromvring_kiov+*@iov:aniovpassedtovringh_getdesc_*()(updatedasweconsume)+*@len:themaximumlengthtoadvance+*/+voidvringh_kiov_advance(structvringh_kiov*iov,size_tlen)+{+while(len&&iov->i<iov->used){+size_tpartlen=min(iov->iov[iov->i].iov_len,len);++iov->consumed+=partlen;+iov->iov[iov->i].iov_len-=partlen;+iov->iov[iov->i].iov_base+=partlen;++if(!iov->iov[iov->i].iov_len){+/* Fix up old iov element then increment. */+iov->iov[iov->i].iov_len=iov->consumed;+iov->iov[iov->i].iov_base-=iov->consumed;++iov->consumed=0;+iov->i++;+}++len-=partlen;+}+}+EXPORT_SYMBOL(vringh_kiov_advance);+/* Copy some bytes to/from the iovec. Returns num copied. */staticinlinessize_tvringh_iov_xfer(structvringh*vrh,structvringh_kiov*iov,
@@ -95,19 +123,8 @@ static inline ssize_t vringh_iov_xfer(struct vringh *vrh,done+=partlen;len-=partlen;ptr+=partlen;-iov->consumed+=partlen;-iov->iov[iov->i].iov_len-=partlen;-iov->iov[iov->i].iov_base+=partlen;-if(!iov->iov[iov->i].iov_len){-/* Fix up old iov element then increment. */-iov->iov[iov->i].iov_len=iov->consumed;-iov->iov[iov->i].iov_base-=iov->consumed;---iov->consumed=0;-iov->i++;-}+vringh_kiov_advance(iov,partlen);}returndone;}
--
2.29.2
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Xie Yongji <redacted>
With VDUSE, we should be able to support all kinds of virtio devices.
Signed-off-by: Xie Yongji <redacted>
---
drivers/vhost/vdpa.c | 28 ++--------------------------
1 file changed, 2 insertions(+), 26 deletions(-)
@@ -215,7 +195,7 @@ static long vhost_vdpa_get_config(struct vhost_vdpa *v,if(copy_from_user(&config,c,size))return-EFAULT;-if(vhost_vdpa_config_validate(v,&config))+if(config.len==0)return-EINVAL;buf=kvzalloc(config.len,GFP_KERNEL);if(!buf)
@@ -243,7 +223,7 @@ static long vhost_vdpa_set_config(struct vhost_vdpa *v,if(copy_from_user(&config,c,size))return-EFAULT;-if(vhost_vdpa_config_validate(v,&config))+if(config.len==0)return-EINVAL;buf=vmemdup_user(c->buf,config.len);
@@ -1021,10 +1001,6 @@ static int vhost_vdpa_probe(struct vdpa_device *vdpa)intminor;intr;-/* Currently, we only accept the network devices. */-if(ops->get_device_id(vdpa)!=VIRTIO_ID_NET)-return-ENOTSUPP;-v=kzalloc(sizeof(*v),GFP_KERNEL|__GFP_RETRY_MAYFAIL);if(!v)return-ENOMEM;
--
2.29.2
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
vringh_getdesc_iotlb() allocates memory to store the kvec, that
is freed with vringh_kiov_cleanup().
vringh_getdesc_iotlb() is able to reuse a kvec previously allocated,
so in order to avoid to allocate the kvec for each request, we are
not calling vringh_kiov_cleanup() when we finished to handle a
request, but we should call it when we free the entire device.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 7 +++++++
1 file changed, 7 insertions(+)
From: Max Gurtovoy <mgurtovoy@nvidia.com>
This will allow running vDPA for virtio block protocol.
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
[sgarzare: various cleanups/fixes]
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v2:
- rebased on top of other changes (dev_attr, get_config(), notify(), etc.)
- memset to 0 the config structure in vdpasim_blk_get_config()
- used vdpasim pointer in vdpasim_blk_get_config()
v1:
- Removed unused headers
- Used cpu_to_vdpasim*() to store config fields
- Replaced 'select VDPA_SIM' with 'depends on VDPA_SIM' since selected
option can not depend on other [Jason]
- Start with a single queue for now [Jason]
- Add comments to memory barriers
---
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 145 +++++++++++++++++++++++++++
drivers/vdpa/Kconfig | 7 ++
drivers/vdpa/vdpa_sim/Makefile | 1 +
3 files changed, 153 insertions(+)
create mode 100644 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -0,0 +1,145 @@+// SPDX-License-Identifier: GPL-2.0-only+/*+*VDPAsimulatorforblockdevice.+*+*Copyright(c)2020,MellanoxTechnologies.Allrightsreserved.+*+*/++#include<linux/init.h>+#include<linux/module.h>+#include<linux/device.h>+#include<linux/kernel.h>+#include<linux/sched.h>+#include<linux/vringh.h>+#include<linux/vdpa.h>+#include<uapi/linux/virtio_blk.h>++#include"vdpa_sim.h"++#define DRV_VERSION "0.1"+#define DRV_AUTHOR "Max Gurtovoy <mgurtovoy@nvidia.com>"+#define DRV_DESC "vDPA Device Simulator for block device"+#define DRV_LICENSE "GPL v2"++#define VDPASIM_BLK_FEATURES (VDPASIM_FEATURES | \+(1ULL<<VIRTIO_BLK_F_SIZE_MAX)|\+(1ULL<<VIRTIO_BLK_F_SEG_MAX)|\+(1ULL<<VIRTIO_BLK_F_BLK_SIZE)|\+(1ULL<<VIRTIO_BLK_F_TOPOLOGY)|\+(1ULL<<VIRTIO_BLK_F_MQ))++#define VDPASIM_BLK_CAPACITY 0x40000+#define VDPASIM_BLK_SIZE_MAX 0x1000+#define VDPASIM_BLK_SEG_MAX 32+#define VDPASIM_BLK_VQ_NUM 1++staticstructvdpasim*vdpasim_blk_dev;++staticvoidvdpasim_blk_work(structwork_struct*work)+{+structvdpasim*vdpasim=container_of(work,structvdpasim,work);+u8status=VIRTIO_BLK_S_OK;+inti;++spin_lock(&vdpasim->lock);++if(!(vdpasim->status&VIRTIO_CONFIG_S_DRIVER_OK))+gotoout;++for(i=0;i<VDPASIM_BLK_VQ_NUM;i++){+structvdpasim_virtqueue*vq=&vdpasim->vqs[i];++if(!vq->ready)+continue;++while(vringh_getdesc_iotlb(&vq->vring,&vq->out_iov,+&vq->in_iov,&vq->head,+GFP_ATOMIC)>0){+intwrite;++vq->in_iov.i=vq->in_iov.used-1;+write=vringh_iov_push_iotlb(&vq->vring,&vq->in_iov,+&status,1);+if(write<=0)+break;++/* Make sure data is wrote before advancing index */+smp_wmb();++vringh_complete_iotlb(&vq->vring,vq->head,write);++/* Make sure used is visible before rasing the interrupt. */+smp_wmb();++local_bh_disable();+if(vringh_need_notify_iotlb(&vq->vring)>0)+vringh_notify(&vq->vring);+local_bh_enable();+}+}+out:+spin_unlock(&vdpasim->lock);+}++staticvoidvdpasim_blk_get_config(structvdpasim*vdpasim,void*config)+{+structvirtio_blk_config*blk_config=+(structvirtio_blk_config*)config;++memset(config,0,sizeof(structvirtio_blk_config));++blk_config->capacity=cpu_to_vdpasim64(vdpasim,VDPASIM_BLK_CAPACITY);+blk_config->size_max=cpu_to_vdpasim32(vdpasim,VDPASIM_BLK_SIZE_MAX);+blk_config->seg_max=cpu_to_vdpasim32(vdpasim,VDPASIM_BLK_SEG_MAX);+blk_config->num_queues=cpu_to_vdpasim16(vdpasim,VDPASIM_BLK_VQ_NUM);+blk_config->min_io_size=cpu_to_vdpasim16(vdpasim,1);+blk_config->opt_io_size=cpu_to_vdpasim32(vdpasim,1);+blk_config->blk_size=cpu_to_vdpasim32(vdpasim,SECTOR_SIZE);+}++staticint__initvdpasim_blk_init(void)+{+structvdpasim_dev_attrdev_attr={};+intret;++dev_attr.id=VIRTIO_ID_BLOCK;+dev_attr.supported_features=VDPASIM_BLK_FEATURES;+dev_attr.nvqs=VDPASIM_BLK_VQ_NUM;+dev_attr.config_size=sizeof(structvirtio_blk_config);+dev_attr.get_config=vdpasim_blk_get_config;+dev_attr.work_fn=vdpasim_blk_work;+dev_attr.buffer_size=PAGE_SIZE;++vdpasim_blk_dev=vdpasim_create(&dev_attr);+if(IS_ERR(vdpasim_blk_dev)){+ret=PTR_ERR(vdpasim_blk_dev);+gotoout;+}++ret=vdpa_register_device(&vdpasim_blk_dev->vdpa);+if(ret)+gotoput_dev;++return0;++put_dev:+put_device(&vdpasim_blk_dev->vdpa.dev);+out:+returnret;+}++staticvoid__exitvdpasim_blk_exit(void)+{+structvdpa_device*vdpa=&vdpasim_blk_dev->vdpa;++vdpa_unregister_device(vdpa);+}++module_init(vdpasim_blk_init)+module_exit(vdpasim_blk_exit)++MODULE_VERSION(DRV_VERSION);+MODULE_LICENSE(DRV_LICENSE);+MODULE_AUTHOR(DRV_AUTHOR);+MODULE_DESCRIPTION(DRV_DESC);
The previous implementation wrote only the status of each request.
This patch implements a more accurate block device simulator,
providing a ramdisk-like behavior.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v2:
- used %zd %zx to print size_t and ssize_t variables in dev_err()
- removed unnecessary new line [Jason]
- moved VIRTIO_BLK_T_GET_ID in another patch [Jason]
- used push/pull instead of write/read terminology
- added vdpasim_blk_check_range() to avoid overflows [Stefan]
- use vdpasim*_to_cpu instead of le*_to_cpu
- used vringh_kiov_length() helper [Jason]
---
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 164 ++++++++++++++++++++++++---
1 file changed, 146 insertions(+), 18 deletions(-)
@@ -36,10 +38,151 @@staticstructvdpasim*vdpasim_blk_dev;+staticboolvdpasim_blk_check_range(u64start_sector,size_trange_size)+{+u64range_sectors=range_size>>SECTOR_SHIFT;++if(range_size>VDPASIM_BLK_SIZE_MAX*VDPASIM_BLK_SEG_MAX)+returnfalse;++if(start_sector>VDPASIM_BLK_CAPACITY)+returnfalse;++if(range_sectors>VDPASIM_BLK_CAPACITY-start_sector)+returnfalse;++returntrue;+}++/* Returns 'true' if the request is handled (with or without an I/O error)+*andthestatusiscorrectlywritteninthelastbyteofthe'iniov',+*'false'otherwise.+*/+staticboolvdpasim_blk_handle_req(structvdpasim*vdpasim,+structvdpasim_virtqueue*vq)+{+size_tpushed=0,to_pull,to_push;+structvirtio_blk_outhdrhdr;+ssize_tbytes;+loff_toffset;+u64sector;+u8status;+u32type;+intret;++ret=vringh_getdesc_iotlb(&vq->vring,&vq->out_iov,&vq->in_iov,+&vq->head,GFP_ATOMIC);+if(ret!=1)+returnfalse;++if(vq->out_iov.used<1||vq->in_iov.used<1){+dev_err(&vdpasim->vdpa.dev,"missing headers - out_iov: %u in_iov %u\n",+vq->out_iov.used,vq->in_iov.used);+returnfalse;+}++if(vq->in_iov.iov[vq->in_iov.used-1].iov_len<1){+dev_err(&vdpasim->vdpa.dev,"request in header too short\n");+returnfalse;+}++/* The last byte is the status and we checked if the last iov has+*enoughroomforit.+*/+to_push=vringh_kiov_length(&vq->in_iov)-1;++to_pull=vringh_kiov_length(&vq->out_iov);++bytes=vringh_iov_pull_iotlb(&vq->vring,&vq->out_iov,&hdr,+sizeof(hdr));+if(bytes!=sizeof(hdr)){+dev_err(&vdpasim->vdpa.dev,"request out header too short\n");+returnfalse;+}++to_pull-=bytes;++type=vdpasim32_to_cpu(vdpasim,hdr.type);+sector=vdpasim64_to_cpu(vdpasim,hdr.sector);+offset=sector<<SECTOR_SHIFT;+status=VIRTIO_BLK_S_OK;++switch(type){+caseVIRTIO_BLK_T_IN:+if(!vdpasim_blk_check_range(sector,to_push)){+dev_err(&vdpasim->vdpa.dev,+"reading over the capacity - offset: 0x%llx len: 0x%zx\n",+offset,to_push);+status=VIRTIO_BLK_S_IOERR;+break;+}++bytes=vringh_iov_push_iotlb(&vq->vring,&vq->in_iov,+vdpasim->buffer+offset,+to_push);+if(bytes<0){+dev_err(&vdpasim->vdpa.dev,+"vringh_iov_push_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",+bytes,offset,to_push);+status=VIRTIO_BLK_S_IOERR;+break;+}++pushed+=bytes;+break;++caseVIRTIO_BLK_T_OUT:+if(!vdpasim_blk_check_range(sector,to_pull)){+dev_err(&vdpasim->vdpa.dev,+"writing over the capacity - offset: 0x%llx len: 0x%zx\n",+offset,to_pull);+status=VIRTIO_BLK_S_IOERR;+break;+}++bytes=vringh_iov_pull_iotlb(&vq->vring,&vq->out_iov,+vdpasim->buffer+offset,+to_pull);+if(bytes<0){+dev_err(&vdpasim->vdpa.dev,+"vringh_iov_pull_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",+bytes,offset,to_pull);+status=VIRTIO_BLK_S_IOERR;+break;+}+break;++default:+dev_warn(&vdpasim->vdpa.dev,+"Unsupported request type %d\n",type);+status=VIRTIO_BLK_S_IOERR;+break;+}++/* If some operations fail, we need to skip the remaining bytes+*toputthestatusinthelastbyte+*/+if(to_push-pushed>0)+vringh_kiov_advance(&vq->in_iov,to_push-pushed);++/* Last byte is the status */+bytes=vringh_iov_push_iotlb(&vq->vring,&vq->in_iov,&status,1);+if(bytes!=1)+returnfalse;++pushed+=bytes;++/* Make sure data is wrote before advancing index */+smp_wmb();++vringh_complete_iotlb(&vq->vring,vq->head,pushed);++returntrue;+}+staticvoidvdpasim_blk_work(structwork_struct*work){structvdpasim*vdpasim=container_of(work,structvdpasim,work);-u8status=VIRTIO_BLK_S_OK;inti;spin_lock(&vdpasim->lock);
@@ -53,22 +196,7 @@ static void vdpasim_blk_work(struct work_struct *work)if(!vq->ready)continue;-while(vringh_getdesc_iotlb(&vq->vring,&vq->out_iov,-&vq->in_iov,&vq->head,-GFP_ATOMIC)>0){-intwrite;--vq->in_iov.i=vq->in_iov.used-1;-write=vringh_iov_push_iotlb(&vq->vring,&vq->in_iov,-&status,1);-if(write<=0)-break;--/* Make sure data is wrote before advancing index */-smp_wmb();--vringh_complete_iotlb(&vq->vring,vq->head,write);-+while(vdpasim_blk_handle_req(vdpasim,vq)){/* Make sure used is visible before rasing the interrupt. */smp_wmb();
@@ -109,7 +237,7 @@ static int __init vdpasim_blk_init(void)dev_attr.config_size=sizeof(structvirtio_blk_config);dev_attr.get_config=vdpasim_blk_get_config;dev_attr.work_fn=vdpasim_blk_work;-dev_attr.buffer_size=PAGE_SIZE;+dev_attr.buffer_size=VDPASIM_BLK_CAPACITY<<SECTOR_SHIFT;vdpasim_blk_dev=vdpasim_create(&dev_attr);if(IS_ERR(vdpasim_blk_dev)){
--
2.29.2
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
This new helper returns the total number of bytes covered by
a vringh_kiov.
Suggested-by: Jason Wang <redacted>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
include/linux/vringh.h | 11 +++++++++++
1 file changed, 11 insertions(+)
Usually iotlb accesses are synchronized with a spinlock.
Let's request it as a new parameter in vringh_set_iotlb() and
hold it when we navigate the iotlb in iotlb_translate() to avoid
race conditions with any new additions/deletions of ranges from
the ioltb.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
include/linux/vringh.h | 6 +++++-
drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 ++-
drivers/vhost/vringh.c | 9 ++++++++-
3 files changed, 15 insertions(+), 3 deletions(-)
@@ -46,6 +46,9 @@ struct vringh {/* IOTLB for this vring */structvhost_iotlb*iotlb;+/* spinlock to synchronize IOTLB accesses */+spinlock_t*iotlb_lock;+/* The function to call to notify the guest about added buffers */void(*notify)(structvringh*);};
The identical mapping used until now created issues when mapping
different virtual pages with the same physical address.
To solve this issue, we can use the iova module, to handle the IOVA
allocation.
For simplicity we use an IOVA allocator with byte granularity.
We add two new functions, vdpasim_map_range() and vdpasim_unmap_range(),
to handle the IOVA allocation and the registration into the IOMMU/IOTLB.
These functions are used by dma_map_ops callbacks.
Acked-by: Jason Wang <redacted>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v2:
- used ULONG_MAX instead of ~0UL [Jason]
- fixed typos in comment and patch description [Jason]
---
drivers/vdpa/vdpa_sim/vdpa_sim.h | 2 +
drivers/vdpa/vdpa_sim/vdpa_sim.c | 108 +++++++++++++++++++------------
drivers/vdpa/Kconfig | 1 +
3 files changed, 69 insertions(+), 42 deletions(-)
@@ -57,6 +58,7 @@ struct vdpasim {/* virtio config according to device type */void*config;structvhost_iotlb*iommu;+structiova_domainiova;void*buffer;u32status;u32generation;
@@ -128,30 +129,57 @@ static int dir_to_perm(enum dma_data_direction dir)returnperm;}+staticdma_addr_tvdpasim_map_range(structvdpasim*vdpasim,phys_addr_tpaddr,+size_tsize,unsignedintperm)+{+structiova*iova;+dma_addr_tdma_addr;+intret;++/* We set the limit_pfn to the maximum (ULONG_MAX - 1) */+iova=alloc_iova(&vdpasim->iova,size,ULONG_MAX-1,true);+if(!iova)+returnDMA_MAPPING_ERROR;++dma_addr=iova_dma_addr(&vdpasim->iova,iova);++spin_lock(&vdpasim->iommu_lock);+ret=vhost_iotlb_add_range(vdpasim->iommu,(u64)dma_addr,+(u64)dma_addr+size-1,(u64)paddr,perm);+spin_unlock(&vdpasim->iommu_lock);++if(ret){+__free_iova(&vdpasim->iova,iova);+returnDMA_MAPPING_ERROR;+}++returndma_addr;+}++staticvoidvdpasim_unmap_range(structvdpasim*vdpasim,dma_addr_tdma_addr,+size_tsize)+{+spin_lock(&vdpasim->iommu_lock);+vhost_iotlb_del_range(vdpasim->iommu,(u64)dma_addr,+(u64)dma_addr+size-1);+spin_unlock(&vdpasim->iommu_lock);++free_iova(&vdpasim->iova,iova_pfn(&vdpasim->iova,dma_addr));+}+staticdma_addr_tvdpasim_map_page(structdevice*dev,structpage*page,unsignedlongoffset,size_tsize,enumdma_data_directiondir,unsignedlongattrs){structvdpasim*vdpasim=dev_to_sim(dev);-structvhost_iotlb*iommu=vdpasim->iommu;-u64pa=(page_to_pfn(page)<<PAGE_SHIFT)+offset;-intret,perm=dir_to_perm(dir);+phys_addr_tpaddr=page_to_phys(page)+offset;+intperm=dir_to_perm(dir);if(perm<0)returnDMA_MAPPING_ERROR;-/* For simplicity, use identical mapping to avoid e.g iova-*allocator.-*/-spin_lock(&vdpasim->iommu_lock);-ret=vhost_iotlb_add_range(iommu,pa,pa+size-1,-pa,dir_to_perm(dir));-spin_unlock(&vdpasim->iommu_lock);-if(ret)-returnDMA_MAPPING_ERROR;--return(dma_addr_t)(pa);+returnvdpasim_map_range(vdpasim,paddr,size,perm);}staticvoidvdpasim_unmap_page(structdevice*dev,dma_addr_tdma_addr,
@@ -271,6 +286,13 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *dev_attr)for(i=0;i<dev_attr->nvqs;i++)vringh_set_iotlb(&vdpasim->vqs[i].vring,vdpasim->iommu);+ret=iova_cache_get();+if(ret)+gotoerr_iommu;++/* For simplicity we use an IOVA allocator with byte granularity */+init_iova_domain(&vdpasim->iova,1,0);+vdpasim->vdpa.dma_dev=dev;returnvdpasim;
From: Jason Wang <hidden> Date: 2021-01-29 07:45:38
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
Usually iotlb accesses are synchronized with a spinlock.
Let's request it as a new parameter in vringh_set_iotlb() and
hold it when we navigate the iotlb in iotlb_translate() to avoid
race conditions with any new additions/deletions of ranges from
the ioltb.
Patch looks fine but I wonder if this is the best approach comparing to
do locking by the caller.
Thanks
@@ -46,6 +46,9 @@ struct vringh {/* IOTLB for this vring */structvhost_iotlb*iotlb;+/* spinlock to synchronize IOTLB accesses */+spinlock_t*iotlb_lock;+/* The function to call to notify the guest about added buffers */void(*notify)(structvringh*);};
On Fri, Jan 29, 2021 at 03:43:40PM +0800, Jason Wang wrote:
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
quoted
Usually iotlb accesses are synchronized with a spinlock.
Let's request it as a new parameter in vringh_set_iotlb() and
hold it when we navigate the iotlb in iotlb_translate() to avoid
race conditions with any new additions/deletions of ranges from
the ioltb.
Patch looks fine but I wonder if this is the best approach comparing
to do locking by the caller.
Initially I tried to hold the lock in the vdpasim_blk_work(), but since
we have a lot of different functions for vringh, I opted to take the
lock at the beginning and release it at the end.
Also because several times I went to see if that call used
iotlb_translate or not.
This could be a problem for example if we have multiple workers to
handle multiple queues.
Also, some functions are quite long (e.g. vringh_getdesc_iotlb) and
holding the lock for that long could reduce parallelism.
For these reasons I thought it was better to hide everything from the
caller who doesn't have to worry about which function calls
iotlb_translate() and thus hold the lock.
Thanks,
Stefano
@@ -46,6 +46,9 @@ struct vringh {/* IOTLB for this vring */structvhost_iotlb*iotlb;+/* spinlock to synchronize IOTLB accesses */+spinlock_t*iotlb_lock;+/* The function to call to notify the guest about added buffers */void(*notify)(structvringh*);};
From: Jason Wang <hidden> Date: 2021-02-01 05:49:01
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
In some cases, it may be useful to provide a way to skip a number
of bytes in a vringh_kiov.
Let's implement vringh_kiov_advance() for this purpose, reusing the
code from vringh_iov_xfer().
We replace that code calling the new vringh_kiov_advance().
Acked-by: Jason Wang <redacted>
In the long run we need to switch to use iov iterator library instead.
Thanks
@@ -75,6 +75,34 @@ static inline int __vringh_get_head(const struct vringh *vrh,returnhead;}+/**+*vringh_kiov_advance-skipbytesfromvring_kiov+*@iov:aniovpassedtovringh_getdesc_*()(updatedasweconsume)+*@len:themaximumlengthtoadvance+*/+voidvringh_kiov_advance(structvringh_kiov*iov,size_tlen)+{+while(len&&iov->i<iov->used){+size_tpartlen=min(iov->iov[iov->i].iov_len,len);++iov->consumed+=partlen;+iov->iov[iov->i].iov_len-=partlen;+iov->iov[iov->i].iov_base+=partlen;++if(!iov->iov[iov->i].iov_len){+/* Fix up old iov element then increment. */+iov->iov[iov->i].iov_len=iov->consumed;+iov->iov[iov->i].iov_base-=iov->consumed;++iov->consumed=0;+iov->i++;+}++len-=partlen;+}+}+EXPORT_SYMBOL(vringh_kiov_advance);+/* Copy some bytes to/from the iovec. Returns num copied. */staticinlinessize_tvringh_iov_xfer(structvringh*vrh,structvringh_kiov*iov,
@@ -95,19 +123,8 @@ static inline ssize_t vringh_iov_xfer(struct vringh *vrh,done+=partlen;len-=partlen;ptr+=partlen;-iov->consumed+=partlen;-iov->iov[iov->i].iov_len-=partlen;-iov->iov[iov->i].iov_base+=partlen;-if(!iov->iov[iov->i].iov_len){-/* Fix up old iov element then increment. */-iov->iov[iov->i].iov_len=iov->consumed;-iov->iov[iov->i].iov_base-=iov->consumed;---iov->consumed=0;-iov->i++;-}+vringh_kiov_advance(iov,partlen);}returndone;}
From: Jason Wang <hidden> Date: 2021-02-01 05:55:54
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
From: Max Gurtovoy <mgurtovoy@nvidia.com>
This will allow running vDPA for virtio block protocol.
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
[sgarzare: various cleanups/fixes]
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
Acked-by: Jason Wang <redacted>
quoted hunk
---
v2:
- rebased on top of other changes (dev_attr, get_config(), notify(), etc.)
- memset to 0 the config structure in vdpasim_blk_get_config()
- used vdpasim pointer in vdpasim_blk_get_config()
v1:
- Removed unused headers
- Used cpu_to_vdpasim*() to store config fields
- Replaced 'select VDPA_SIM' with 'depends on VDPA_SIM' since selected
option can not depend on other [Jason]
- Start with a single queue for now [Jason]
- Add comments to memory barriers
---
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 145 +++++++++++++++++++++++++++
drivers/vdpa/Kconfig | 7 ++
drivers/vdpa/vdpa_sim/Makefile | 1 +
3 files changed, 153 insertions(+)
create mode 100644 drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -0,0 +1,145 @@+// SPDX-License-Identifier: GPL-2.0-only+/*+*VDPAsimulatorforblockdevice.+*+*Copyright(c)2020,MellanoxTechnologies.Allrightsreserved.+*+*/++#include<linux/init.h>+#include<linux/module.h>+#include<linux/device.h>+#include<linux/kernel.h>+#include<linux/sched.h>+#include<linux/vringh.h>+#include<linux/vdpa.h>+#include<uapi/linux/virtio_blk.h>++#include"vdpa_sim.h"++#define DRV_VERSION "0.1"+#define DRV_AUTHOR "Max Gurtovoy <mgurtovoy@nvidia.com>"+#define DRV_DESC "vDPA Device Simulator for block device"+#define DRV_LICENSE "GPL v2"++#define VDPASIM_BLK_FEATURES (VDPASIM_FEATURES | \+(1ULL<<VIRTIO_BLK_F_SIZE_MAX)|\+(1ULL<<VIRTIO_BLK_F_SEG_MAX)|\+(1ULL<<VIRTIO_BLK_F_BLK_SIZE)|\+(1ULL<<VIRTIO_BLK_F_TOPOLOGY)|\+(1ULL<<VIRTIO_BLK_F_MQ))++#define VDPASIM_BLK_CAPACITY 0x40000+#define VDPASIM_BLK_SIZE_MAX 0x1000+#define VDPASIM_BLK_SEG_MAX 32+#define VDPASIM_BLK_VQ_NUM 1++staticstructvdpasim*vdpasim_blk_dev;++staticvoidvdpasim_blk_work(structwork_struct*work)+{+structvdpasim*vdpasim=container_of(work,structvdpasim,work);+u8status=VIRTIO_BLK_S_OK;+inti;++spin_lock(&vdpasim->lock);++if(!(vdpasim->status&VIRTIO_CONFIG_S_DRIVER_OK))+gotoout;++for(i=0;i<VDPASIM_BLK_VQ_NUM;i++){+structvdpasim_virtqueue*vq=&vdpasim->vqs[i];++if(!vq->ready)+continue;++while(vringh_getdesc_iotlb(&vq->vring,&vq->out_iov,+&vq->in_iov,&vq->head,+GFP_ATOMIC)>0){+intwrite;++vq->in_iov.i=vq->in_iov.used-1;+write=vringh_iov_push_iotlb(&vq->vring,&vq->in_iov,+&status,1);+if(write<=0)+break;++/* Make sure data is wrote before advancing index */+smp_wmb();++vringh_complete_iotlb(&vq->vring,vq->head,write);++/* Make sure used is visible before rasing the interrupt. */+smp_wmb();++local_bh_disable();+if(vringh_need_notify_iotlb(&vq->vring)>0)+vringh_notify(&vq->vring);+local_bh_enable();+}+}+out:+spin_unlock(&vdpasim->lock);+}++staticvoidvdpasim_blk_get_config(structvdpasim*vdpasim,void*config)+{+structvirtio_blk_config*blk_config=+(structvirtio_blk_config*)config;++memset(config,0,sizeof(structvirtio_blk_config));++blk_config->capacity=cpu_to_vdpasim64(vdpasim,VDPASIM_BLK_CAPACITY);+blk_config->size_max=cpu_to_vdpasim32(vdpasim,VDPASIM_BLK_SIZE_MAX);+blk_config->seg_max=cpu_to_vdpasim32(vdpasim,VDPASIM_BLK_SEG_MAX);+blk_config->num_queues=cpu_to_vdpasim16(vdpasim,VDPASIM_BLK_VQ_NUM);+blk_config->min_io_size=cpu_to_vdpasim16(vdpasim,1);+blk_config->opt_io_size=cpu_to_vdpasim32(vdpasim,1);+blk_config->blk_size=cpu_to_vdpasim32(vdpasim,SECTOR_SIZE);+}++staticint__initvdpasim_blk_init(void)+{+structvdpasim_dev_attrdev_attr={};+intret;++dev_attr.id=VIRTIO_ID_BLOCK;+dev_attr.supported_features=VDPASIM_BLK_FEATURES;+dev_attr.nvqs=VDPASIM_BLK_VQ_NUM;+dev_attr.config_size=sizeof(structvirtio_blk_config);+dev_attr.get_config=vdpasim_blk_get_config;+dev_attr.work_fn=vdpasim_blk_work;+dev_attr.buffer_size=PAGE_SIZE;++vdpasim_blk_dev=vdpasim_create(&dev_attr);+if(IS_ERR(vdpasim_blk_dev)){+ret=PTR_ERR(vdpasim_blk_dev);+gotoout;+}++ret=vdpa_register_device(&vdpasim_blk_dev->vdpa);+if(ret)+gotoput_dev;++return0;++put_dev:+put_device(&vdpasim_blk_dev->vdpa.dev);+out:+returnret;+}++staticvoid__exitvdpasim_blk_exit(void)+{+structvdpa_device*vdpa=&vdpasim_blk_dev->vdpa;++vdpa_unregister_device(vdpa);+}++module_init(vdpasim_blk_init)+module_exit(vdpasim_blk_exit)++MODULE_VERSION(DRV_VERSION);+MODULE_LICENSE(DRV_LICENSE);+MODULE_AUTHOR(DRV_AUTHOR);+MODULE_DESCRIPTION(DRV_DESC);
From: Jason Wang <hidden> Date: 2021-02-01 06:33:53
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
The previous implementation wrote only the status of each request.
This patch implements a more accurate block device simulator,
providing a ramdisk-like behavior.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v2:
- used %zd %zx to print size_t and ssize_t variables in dev_err()
- removed unnecessary new line [Jason]
- moved VIRTIO_BLK_T_GET_ID in another patch [Jason]
- used push/pull instead of write/read terminology
- added vdpasim_blk_check_range() to avoid overflows [Stefan]
- use vdpasim*_to_cpu instead of le*_to_cpu
- used vringh_kiov_length() helper [Jason]
@@ -36,10 +38,151 @@staticstructvdpasim*vdpasim_blk_dev;+staticboolvdpasim_blk_check_range(u64start_sector,size_trange_size)+{+u64range_sectors=range_size>>SECTOR_SHIFT;++if(range_size>VDPASIM_BLK_SIZE_MAX*VDPASIM_BLK_SEG_MAX)+returnfalse;++if(start_sector>VDPASIM_BLK_CAPACITY)+returnfalse;++if(range_sectors>VDPASIM_BLK_CAPACITY-start_sector)+returnfalse;++returntrue;+}++/* Returns 'true' if the request is handled (with or without an I/O error)+*andthestatusiscorrectlywritteninthelastbyteofthe'iniov',+*'false'otherwise.+*/+staticboolvdpasim_blk_handle_req(structvdpasim*vdpasim,+structvdpasim_virtqueue*vq)+{+size_tpushed=0,to_pull,to_push;+structvirtio_blk_outhdrhdr;+ssize_tbytes;+loff_toffset;+u64sector;+u8status;+u32type;+intret;++ret=vringh_getdesc_iotlb(&vq->vring,&vq->out_iov,&vq->in_iov,+&vq->head,GFP_ATOMIC);+if(ret!=1)+returnfalse;++if(vq->out_iov.used<1||vq->in_iov.used<1){+dev_err(&vdpasim->vdpa.dev,"missing headers - out_iov: %u in_iov %u\n",+vq->out_iov.used,vq->in_iov.used);+returnfalse;+}++if(vq->in_iov.iov[vq->in_iov.used-1].iov_len<1){+dev_err(&vdpasim->vdpa.dev,"request in header too short\n");+returnfalse;+}++/* The last byte is the status and we checked if the last iov has+*enoughroomforit.+*/+to_push=vringh_kiov_length(&vq->in_iov)-1;++to_pull=vringh_kiov_length(&vq->out_iov);++bytes=vringh_iov_pull_iotlb(&vq->vring,&vq->out_iov,&hdr,+sizeof(hdr));+if(bytes!=sizeof(hdr)){+dev_err(&vdpasim->vdpa.dev,"request out header too short\n");+returnfalse;+}++to_pull-=bytes;++type=vdpasim32_to_cpu(vdpasim,hdr.type);+sector=vdpasim64_to_cpu(vdpasim,hdr.sector);+offset=sector<<SECTOR_SHIFT;+status=VIRTIO_BLK_S_OK;++switch(type){+caseVIRTIO_BLK_T_IN:+if(!vdpasim_blk_check_range(sector,to_push)){+dev_err(&vdpasim->vdpa.dev,+"reading over the capacity - offset: 0x%llx len: 0x%zx\n",+offset,to_push);+status=VIRTIO_BLK_S_IOERR;+break;+}++bytes=vringh_iov_push_iotlb(&vq->vring,&vq->in_iov,+vdpasim->buffer+offset,+to_push);+if(bytes<0){+dev_err(&vdpasim->vdpa.dev,+"vringh_iov_push_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",+bytes,offset,to_push);+status=VIRTIO_BLK_S_IOERR;+break;+}++pushed+=bytes;+break;++caseVIRTIO_BLK_T_OUT:+if(!vdpasim_blk_check_range(sector,to_pull)){+dev_err(&vdpasim->vdpa.dev,+"writing over the capacity - offset: 0x%llx len: 0x%zx\n",+offset,to_pull);+status=VIRTIO_BLK_S_IOERR;+break;+}++bytes=vringh_iov_pull_iotlb(&vq->vring,&vq->out_iov,+vdpasim->buffer+offset,+to_pull);+if(bytes<0){+dev_err(&vdpasim->vdpa.dev,+"vringh_iov_pull_iotlb() error: %zd offset: 0x%llx len: 0x%zx\n",+bytes,offset,to_pull);+status=VIRTIO_BLK_S_IOERR;+break;+}+break;++default:+dev_warn(&vdpasim->vdpa.dev,+"Unsupported request type %d\n",type);+status=VIRTIO_BLK_S_IOERR;+break;+}++/* If some operations fail, we need to skip the remaining bytes+*toputthestatusinthelastbyte+*/+if(to_push-pushed>0)+vringh_kiov_advance(&vq->in_iov,to_push-pushed);++/* Last byte is the status */+bytes=vringh_iov_push_iotlb(&vq->vring,&vq->in_iov,&status,1);+if(bytes!=1)+returnfalse;++pushed+=bytes;++/* Make sure data is wrote before advancing index */+smp_wmb();++vringh_complete_iotlb(&vq->vring,vq->head,pushed);++returntrue;+}+staticvoidvdpasim_blk_work(structwork_struct*work){structvdpasim*vdpasim=container_of(work,structvdpasim,work);-u8status=VIRTIO_BLK_S_OK;inti;spin_lock(&vdpasim->lock);
@@ -53,22 +196,7 @@ static void vdpasim_blk_work(struct work_struct *work)if(!vq->ready)continue;-while(vringh_getdesc_iotlb(&vq->vring,&vq->out_iov,-&vq->in_iov,&vq->head,-GFP_ATOMIC)>0){-intwrite;--vq->in_iov.i=vq->in_iov.used-1;-write=vringh_iov_push_iotlb(&vq->vring,&vq->in_iov,-&status,1);-if(write<=0)-break;--/* Make sure data is wrote before advancing index */-smp_wmb();--vringh_complete_iotlb(&vq->vring,vq->head,write);-+while(vdpasim_blk_handle_req(vdpasim,vq)){/* Make sure used is visible before rasing the interrupt. */smp_wmb();
@@ -109,7 +237,7 @@ static int __init vdpasim_blk_init(void)dev_attr.config_size=sizeof(structvirtio_blk_config);dev_attr.get_config=vdpasim_blk_get_config;dev_attr.work_fn=vdpasim_blk_work;-dev_attr.buffer_size=PAGE_SIZE;+dev_attr.buffer_size=VDPASIM_BLK_CAPACITY<<SECTOR_SHIFT;vdpasim_blk_dev=vdpasim_create(&dev_attr);if(IS_ERR(vdpasim_blk_dev)){
From: Jason Wang <hidden> Date: 2021-02-01 06:37:25
On 2021/1/29 下午5:18, Stefano Garzarella wrote:
On Fri, Jan 29, 2021 at 03:43:40PM +0800, Jason Wang wrote:
quoted
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
quoted
Usually iotlb accesses are synchronized with a spinlock.
Let's request it as a new parameter in vringh_set_iotlb() and
hold it when we navigate the iotlb in iotlb_translate() to avoid
race conditions with any new additions/deletions of ranges from
the ioltb.
Patch looks fine but I wonder if this is the best approach comparing
to do locking by the caller.
Initially I tried to hold the lock in the vdpasim_blk_work(), but since
we have a lot of different functions for vringh, I opted to take the
lock at the beginning and release it at the end.
Also because several times I went to see if that call used
iotlb_translate or not.
This could be a problem for example if we have multiple workers to
handle multiple queues.
Also, some functions are quite long (e.g. vringh_getdesc_iotlb) and
holding the lock for that long could reduce parallelism.
For these reasons I thought it was better to hide everything from the
caller who doesn't have to worry about which function calls
iotlb_translate() and thus hold the lock.
Fine with me.
Acked-by: Jason Wang <redacted>
Thanks
/* IOTLB for this vring */
struct vhost_iotlb *iotlb;
+ /* spinlock to synchronize IOTLB accesses */
+ spinlock_t *iotlb_lock;
+
/* The function to call to notify the guest about added buffers */
void (*notify)(struct vringh *);
};
vdpasim_dev_attr *dev_attr)
goto err_iommu;
for (i = 0; i < dev_attr->nvqs; i++)
- vringh_set_iotlb(&vdpasim->vqs[i].vring, vdpasim->iommu);
+ vringh_set_iotlb(&vdpasim->vqs[i].vring, vdpasim->iommu,
+ &vdpasim->iommu_lock);
ret = iova_cache_get();
if (ret)
From: Jason Wang <hidden> Date: 2021-02-01 06:47:38
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
__vringh_iov() overwrites the contents of riov and wiov, in fact it
resets the 'i' and 'used' fields, but also the consumed field should
be reset to avoid an inconsistent state.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
I had a question(I remember we had some discussion like this but I
forget the conclusion):
I see e.g in vringh_getdesc_kern() it has the following comment:
/*
* Note that you may need to clean up riov and wiov, even on error!
*/
So it looks to me the correct way is to call vringh_kiov_cleanup() before?
Thanks
From: Jason Wang <hidden> Date: 2021-02-01 06:47:38
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
This new helper returns the total number of bytes covered by
a vringh_kiov.
Suggested-by: Jason Wang <redacted>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
From: Jason Wang <hidden> Date: 2021-02-01 06:47:46
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
vringh_getdesc_iotlb() allocates memory to store the kvec, that
is freed with vringh_kiov_cleanup().
vringh_getdesc_iotlb() is able to reuse a kvec previously allocated,
so in order to avoid to allocate the kvec for each request, we are
not calling vringh_kiov_cleanup() when we finished to handle a
request, but we should call it when we free the entire device.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
On Mon, Feb 01, 2021 at 01:40:01PM +0800, Jason Wang wrote:
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
quoted
__vringh_iov() overwrites the contents of riov and wiov, in fact it
resets the 'i' and 'used' fields, but also the consumed field should
be reset to avoid an inconsistent state.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
I had a question(I remember we had some discussion like this but I
forget the conclusion):
Sorry, I forgot to update you.
I see e.g in vringh_getdesc_kern() it has the following comment:
/*
* Note that you may need to clean up riov and wiov, even on error!
*/
So it looks to me the correct way is to call vringh_kiov_cleanup()
before?
Looking at the code the right pattern should be:
vringh_getdesc_*(..., &out_iov, &in_iov, ...);
// use out_iov and in_iov
vringh_kiov_cleanup(&out_iov);
vringh_kiov_cleanup(&in_iov);
This because vringh_getdesc_*() calls __vringh_iov() where
resize_iovec() is called to allocate the iov wrapped by 'struct
vringh_kiov' and vringh_kiov_cleanup() frees that memory.
Looking better, __vringh_iov() is able to extend a 'vringh_kiov'
pre-allocated, so in order to avoid to allocate and free the iov for
each request we can avoid to call vringh_kiov_cleanup(), but this patch
is needed to avoid an inconsistent state.
And also patch "vdpa_sim: cleanup kiovs in vdpasim_free()" is required
to free the iov when the device is going away.
Does that make sense to you?
Maybe I should add a comment in vringh.c to explain this better.
Thanks,
Stefano
On Mon, Feb 01, 2021 at 01:43:23PM +0800, Jason Wang wrote:
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
quoted
In some cases, it may be useful to provide a way to skip a number
of bytes in a vringh_kiov.
Let's implement vringh_kiov_advance() for this purpose, reusing the
code from vringh_iov_xfer().
We replace that code calling the new vringh_kiov_advance().
Acked-by: Jason Wang <redacted>
In the long run we need to switch to use iov iterator library instead.
Yes I agree.
I've tried to do this, but it requires quite a bit of work to change
vringh, I'll put it on my todo list.
Thanks,
Stefano
@@ -75,6 +75,34 @@ static inline int __vringh_get_head(const struct vringh *vrh,returnhead;}+/**+*vringh_kiov_advance-skipbytesfromvring_kiov+*@iov:aniovpassedtovringh_getdesc_*()(updatedasweconsume)+*@len:themaximumlengthtoadvance+*/+voidvringh_kiov_advance(structvringh_kiov*iov,size_tlen)+{+while(len&&iov->i<iov->used){+size_tpartlen=min(iov->iov[iov->i].iov_len,len);++iov->consumed+=partlen;+iov->iov[iov->i].iov_len-=partlen;+iov->iov[iov->i].iov_base+=partlen;++if(!iov->iov[iov->i].iov_len){+/* Fix up old iov element then increment. */+iov->iov[iov->i].iov_len=iov->consumed;+iov->iov[iov->i].iov_base-=iov->consumed;++iov->consumed=0;+iov->i++;+}++len-=partlen;+}+}+EXPORT_SYMBOL(vringh_kiov_advance);+/* Copy some bytes to/from the iovec. Returns num copied. */staticinlinessize_tvringh_iov_xfer(structvringh*vrh,structvringh_kiov*iov,
@@ -95,19 +123,8 @@ static inline ssize_t vringh_iov_xfer(struct vringh *vrh,done+=partlen;len-=partlen;ptr+=partlen;-iov->consumed+=partlen;-iov->iov[iov->i].iov_len-=partlen;-iov->iov[iov->i].iov_base+=partlen;-if(!iov->iov[iov->i].iov_len){-/* Fix up old iov element then increment. */-iov->iov[iov->i].iov_len=iov->consumed;-iov->iov[iov->i].iov_base-=iov->consumed;---iov->consumed=0;-iov->i++;-}+vringh_kiov_advance(iov,partlen);}returndone;}
From: Jason Wang <hidden> Date: 2021-02-02 03:28:30
On 2021/2/1 下午6:21, Stefano Garzarella wrote:
On Mon, Feb 01, 2021 at 01:40:01PM +0800, Jason Wang wrote:
quoted
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
quoted
__vringh_iov() overwrites the contents of riov and wiov, in fact it
resets the 'i' and 'used' fields, but also the consumed field should
be reset to avoid an inconsistent state.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
I had a question(I remember we had some discussion like this but I
forget the conclusion):
Sorry, I forgot to update you.
quoted
I see e.g in vringh_getdesc_kern() it has the following comment:
/*
* Note that you may need to clean up riov and wiov, even on error!
*/
So it looks to me the correct way is to call vringh_kiov_cleanup()
before?
Looking at the code the right pattern should be:
vringh_getdesc_*(..., &out_iov, &in_iov, ...);
// use out_iov and in_iov
vringh_kiov_cleanup(&out_iov);
vringh_kiov_cleanup(&in_iov);
This because vringh_getdesc_*() calls __vringh_iov() where
resize_iovec() is called to allocate the iov wrapped by 'struct
vringh_kiov' and vringh_kiov_cleanup() frees that memory.
Looking better, __vringh_iov() is able to extend a 'vringh_kiov'
pre-allocated, so in order to avoid to allocate and free the iov for
each request we can avoid to call vringh_kiov_cleanup(), but this
patch is needed to avoid an inconsistent state.
And also patch "vdpa_sim: cleanup kiovs in vdpasim_free()" is required
to free the iov when the device is going away.
Does that make sense to you?
Make sense.
Maybe I should add a comment in vringh.c to explain this better.
From: Stefan Hajnoczi <stefanha@redhat.com> Date: 2021-02-02 15:02:37
On Thu, Jan 28, 2021 at 03:41:26PM +0100, Stefano Garzarella wrote:
The previous implementation wrote only the status of each request.
This patch implements a more accurate block device simulator,
providing a ramdisk-like behavior.
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
v2:
- used %zd %zx to print size_t and ssize_t variables in dev_err()
- removed unnecessary new line [Jason]
- moved VIRTIO_BLK_T_GET_ID in another patch [Jason]
- used push/pull instead of write/read terminology
- added vdpasim_blk_check_range() to avoid overflows [Stefan]
- use vdpasim*_to_cpu instead of le*_to_cpu
- used vringh_kiov_length() helper [Jason]
---
drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 164 ++++++++++++++++++++++++---
1 file changed, 146 insertions(+), 18 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
From: Stefan Hajnoczi <stefanha@redhat.com> Date: 2021-02-02 15:03:01
On Thu, Jan 28, 2021 at 03:41:25PM +0100, Stefano Garzarella wrote:
+static void vdpasim_blk_work(struct work_struct *work)
+{
+ struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
+ u8 status = VIRTIO_BLK_S_OK;
+ int i;
+
+ spin_lock(&vdpasim->lock);
+
+ if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
+ goto out;
+
+ for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
+ struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
+
+ if (!vq->ready)
+ continue;
+
+ while (vringh_getdesc_iotlb(&vq->vring, &vq->out_iov,
+ &vq->in_iov, &vq->head,
+ GFP_ATOMIC) > 0) {
+ int write;
+
+ vq->in_iov.i = vq->in_iov.used - 1;
+ write = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov,
+ &status, 1);
+ if (write <= 0)
+ break;
This code looks fragile:
1. Relying on unsigned underflow and the while loop in
vringh_iov_push_iotlb() to handle the case where in_iov.used == 0 is
risky and could break.
2. Does this assume that the last in_iov element has size 1? For
example, the guest driver may send a single "in" iovec with size 513
when reading 512 bytes (with an extra byte for the request status).
Please validate inputs fully, even in test/development code, because
it's likely to be copied by others when writing production code (or
deployed in production by unsuspecting users) :).
On Tue, Feb 02, 2021 at 09:34:12AM +0000, Stefan Hajnoczi wrote:
On Thu, Jan 28, 2021 at 03:41:25PM +0100, Stefano Garzarella wrote:
quoted
+static void vdpasim_blk_work(struct work_struct *work)
+{
+ struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
+ u8 status = VIRTIO_BLK_S_OK;
+ int i;
+
+ spin_lock(&vdpasim->lock);
+
+ if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
+ goto out;
+
+ for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
+ struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
+
+ if (!vq->ready)
+ continue;
+
+ while (vringh_getdesc_iotlb(&vq->vring, &vq->out_iov,
+ &vq->in_iov, &vq->head,
+ GFP_ATOMIC) > 0) {
+ int write;
+
+ vq->in_iov.i = vq->in_iov.used - 1;
+ write = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov,
+ &status, 1);
+ if (write <= 0)
+ break;
This code looks fragile:
1. Relying on unsigned underflow and the while loop in
vringh_iov_push_iotlb() to handle the case where in_iov.used == 0 is
risky and could break.
2. Does this assume that the last in_iov element has size 1? For
example, the guest driver may send a single "in" iovec with size 513
when reading 512 bytes (with an extra byte for the request status).
Please validate inputs fully, even in test/development code, because
it's likely to be copied by others when writing production code (or
deployed in production by unsuspecting users) :).
Perfectly agree on that, so I addressed these things, also following
your review on the previous version, on the next patch of this series:
"vdpa_sim_blk: implement ramdisk behaviour".
Do you think should I move these checks in this patch?
I did this to leave Max credit for this patch and add more code to
emulate a ramdisk in later patches.
Thanks,
Stefano
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Stefan Hajnoczi <stefanha@redhat.com> Date: 2021-02-03 16:47:31
On Tue, Feb 02, 2021 at 04:49:50PM +0100, Stefano Garzarella wrote:
On Tue, Feb 02, 2021 at 09:34:12AM +0000, Stefan Hajnoczi wrote:
quoted
On Thu, Jan 28, 2021 at 03:41:25PM +0100, Stefano Garzarella wrote:
quoted
+static void vdpasim_blk_work(struct work_struct *work)
+{
+ struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
+ u8 status = VIRTIO_BLK_S_OK;
+ int i;
+
+ spin_lock(&vdpasim->lock);
+
+ if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
+ goto out;
+
+ for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
+ struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
+
+ if (!vq->ready)
+ continue;
+
+ while (vringh_getdesc_iotlb(&vq->vring, &vq->out_iov,
+ &vq->in_iov, &vq->head,
+ GFP_ATOMIC) > 0) {
+ int write;
+
+ vq->in_iov.i = vq->in_iov.used - 1;
+ write = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov,
+ &status, 1);
+ if (write <= 0)
+ break;
This code looks fragile:
1. Relying on unsigned underflow and the while loop in
vringh_iov_push_iotlb() to handle the case where in_iov.used == 0 is
risky and could break.
2. Does this assume that the last in_iov element has size 1? For
example, the guest driver may send a single "in" iovec with size 513
when reading 512 bytes (with an extra byte for the request status).
Please validate inputs fully, even in test/development code, because
it's likely to be copied by others when writing production code (or
deployed in production by unsuspecting users) :).
Perfectly agree on that, so I addressed these things, also following your
review on the previous version, on the next patch of this series:
"vdpa_sim_blk: implement ramdisk behaviour".
Do you think should I move these checks in this patch?
I did this to leave Max credit for this patch and add more code to emulate a
ramdisk in later patches.
You could update the commit description so it's clear that input
validation is missing and will be added in the next commit.
Stefan
On Wed, Feb 03, 2021 at 04:45:51PM +0000, Stefan Hajnoczi wrote:
On Tue, Feb 02, 2021 at 04:49:50PM +0100, Stefano Garzarella wrote:
quoted
On Tue, Feb 02, 2021 at 09:34:12AM +0000, Stefan Hajnoczi wrote:
quoted
On Thu, Jan 28, 2021 at 03:41:25PM +0100, Stefano Garzarella wrote:
quoted
+static void vdpasim_blk_work(struct work_struct *work)
+{
+ struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
+ u8 status = VIRTIO_BLK_S_OK;
+ int i;
+
+ spin_lock(&vdpasim->lock);
+
+ if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
+ goto out;
+
+ for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
+ struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
+
+ if (!vq->ready)
+ continue;
+
+ while (vringh_getdesc_iotlb(&vq->vring, &vq->out_iov,
+ &vq->in_iov, &vq->head,
+ GFP_ATOMIC) > 0) {
+ int write;
+
+ vq->in_iov.i = vq->in_iov.used - 1;
+ write = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov,
+ &status, 1);
+ if (write <= 0)
+ break;
This code looks fragile:
1. Relying on unsigned underflow and the while loop in
vringh_iov_push_iotlb() to handle the case where in_iov.used == 0 is
risky and could break.
2. Does this assume that the last in_iov element has size 1? For
example, the guest driver may send a single "in" iovec with size 513
when reading 512 bytes (with an extra byte for the request status).
Please validate inputs fully, even in test/development code, because
it's likely to be copied by others when writing production code (or
deployed in production by unsuspecting users) :).
Perfectly agree on that, so I addressed these things, also following your
review on the previous version, on the next patch of this series:
"vdpa_sim_blk: implement ramdisk behaviour".
Do you think should I move these checks in this patch?
I did this to leave Max credit for this patch and add more code to emulate a
ramdisk in later patches.
You could update the commit description so it's clear that input
validation is missing and will be added in the next commit.