Re: [PATCH RFC v2 08/10] vdpa: add vdpa simulator for block device
On Wed, Feb 03, 2021 at 04:45:51PM +, Stefan Hajnoczi wrote:
On Tue, Feb 02, 2021 at 04:49:50PM +0100, Stefano Garzarella wrote:
On Tue, Feb 02, 2021 at 09:34:12AM +, Stefan Hajnoczi wrote:
> 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(>lock);
> > +
> > + if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
> > + goto out;
> > +
> > + for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
> > + struct vdpasim_virtqueue *vq = >vqs[i];
> > +
> > + if (!vq->ready)
> > + continue;
> > +
> > + while (vringh_getdesc_iotlb(>vring, >out_iov,
> > + >in_iov, >head,
> > + GFP_ATOMIC) > 0) {
> > + int write;
> > +
> > + vq->in_iov.i = vq->in_iov.used - 1;
> > + write = vringh_iov_push_iotlb(>vring, >in_iov,
> > + , 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.
I'll do it.
Thanks,
Stefano
Re: [PATCH RFC v2 08/10] vdpa: add vdpa simulator for block device
On Tue, Feb 02, 2021 at 04:49:50PM +0100, Stefano Garzarella wrote:
> On Tue, Feb 02, 2021 at 09:34:12AM +, Stefan Hajnoczi wrote:
> > 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(>lock);
> > > +
> > > + if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
> > > + goto out;
> > > +
> > > + for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
> > > + struct vdpasim_virtqueue *vq = >vqs[i];
> > > +
> > > + if (!vq->ready)
> > > + continue;
> > > +
> > > + while (vringh_getdesc_iotlb(>vring, >out_iov,
> > > + >in_iov, >head,
> > > + GFP_ATOMIC) > 0) {
> > > + int write;
> > > +
> > > + vq->in_iov.i = vq->in_iov.used - 1;
> > > + write = vringh_iov_push_iotlb(>vring, >in_iov,
> > > + , 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
signature.asc
Description: PGP signature
Re: [PATCH RFC v2 08/10] vdpa: add vdpa simulator for block device
On Tue, Feb 02, 2021 at 09:34:12AM +, Stefan Hajnoczi wrote:
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(>lock);
+
+ if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
+ goto out;
+
+ for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
+ struct vdpasim_virtqueue *vq = >vqs[i];
+
+ if (!vq->ready)
+ continue;
+
+ while (vringh_getdesc_iotlb(>vring, >out_iov,
+ >in_iov, >head,
+ GFP_ATOMIC) > 0) {
+ int write;
+
+ vq->in_iov.i = vq->in_iov.used - 1;
+ write = vringh_iov_push_iotlb(>vring, >in_iov,
+ , 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
Re: [PATCH RFC v2 08/10] vdpa: add vdpa simulator for block device
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(>lock);
> +
> + if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
> + goto out;
> +
> + for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
> + struct vdpasim_virtqueue *vq = >vqs[i];
> +
> + if (!vq->ready)
> + continue;
> +
> + while (vringh_getdesc_iotlb(>vring, >out_iov,
> + >in_iov, >head,
> + GFP_ATOMIC) > 0) {
> + int write;
> +
> + vq->in_iov.i = vq->in_iov.used - 1;
> + write = vringh_iov_push_iotlb(>vring, >in_iov,
> + , 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) :).
signature.asc
Description: PGP signature
Re: [PATCH RFC v2 08/10] vdpa: add vdpa simulator for block device
On Sun, Jan 31, 2021 at 05:31:43PM +0200, Max Gurtovoy wrote: On 1/28/2021 4:41 PM, Stefano Garzarella wrote: From: Max Gurtovoy This will allow running vDPA for virtio block protocol. Signed-off-by: Max Gurtovoy [sgarzare: various cleanups/fixes] Signed-off-by: Stefano Garzarella --- 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 diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c new file mode 100644 index ..999f9ca0b628 --- /dev/null +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * VDPA simulator for block device. + * + * Copyright (c) 2020, Mellanox Technologies. All rights reserved. I guess we can change the copyright from Mellanox to: Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved. I'll update in the next version. Thanks, Stefano
Re: [PATCH RFC v2 08/10] vdpa: add vdpa simulator for block device
On 2021/1/28 下午10:41, Stefano Garzarella wrote:
From: Max Gurtovoy
This will allow running vDPA for virtio block protocol.
Signed-off-by: Max Gurtovoy
[sgarzare: various cleanups/fixes]
Signed-off-by: Stefano Garzarella
Acked-by: Jason Wang
---
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
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
new file mode 100644
index ..999f9ca0b628
--- /dev/null
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * VDPA simulator for block device.
+ *
+ * Copyright (c) 2020, Mellanox Technologies. All rights reserved.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "vdpa_sim.h"
+
+#define DRV_VERSION "0.1"
+#define DRV_AUTHOR "Max Gurtovoy "
+#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 0x4
+#define VDPASIM_BLK_SIZE_MAX 0x1000
+#define VDPASIM_BLK_SEG_MAX32
+#define VDPASIM_BLK_VQ_NUM 1
+
+static struct vdpasim *vdpasim_blk_dev;
+
+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(>lock);
+
+ if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
+ goto out;
+
+ for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
+ struct vdpasim_virtqueue *vq = >vqs[i];
+
+ if (!vq->ready)
+ continue;
+
+ while (vringh_getdesc_iotlb(>vring, >out_iov,
+ >in_iov, >head,
+ GFP_ATOMIC) > 0) {
+ int write;
+
+ vq->in_iov.i = vq->in_iov.used - 1;
+ write = vringh_iov_push_iotlb(>vring, >in_iov,
+ , 1);
+ if (write <= 0)
+ break;
+
+ /* Make sure data is wrote before advancing index */
+ smp_wmb();
+
+ vringh_complete_iotlb(>vring, vq->head, write);
+
+ /* Make sure used is visible before rasing the
interrupt. */
+ smp_wmb();
+
+ local_bh_disable();
+ if (vringh_need_notify_iotlb(>vring) > 0)
+ vringh_notify(>vring);
+ local_bh_enable();
+ }
+ }
+out:
+ spin_unlock(>lock);
+}
+
+static void vdpasim_blk_get_config(struct vdpasim *vdpasim, void *config)
+{
+ struct virtio_blk_config *blk_config =
+ (struct virtio_blk_config *)config;
+
+ memset(config, 0, sizeof(struct virtio_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);
+}
+
+static int __init vdpasim_blk_init(void)
+{
+ struct vdpasim_dev_attr dev_attr = {};
+ int ret;
+
+ 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(struct virtio_blk_config);
+ dev_attr.get_config = vdpasim_blk_get_config;
+ dev_attr.work_fn = vdpasim_blk_work;
+
Re: [PATCH RFC v2 08/10] vdpa: add vdpa simulator for block device
On 1/28/2021 4:41 PM, Stefano Garzarella wrote: From: Max Gurtovoy This will allow running vDPA for virtio block protocol. Signed-off-by: Max Gurtovoy [sgarzare: various cleanups/fixes] Signed-off-by: Stefano Garzarella --- 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 diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c new file mode 100644 index ..999f9ca0b628 --- /dev/null +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * VDPA simulator for block device. + * + * Copyright (c) 2020, Mellanox Technologies. All rights reserved. I guess we can change the copyright from Mellanox to: Copyright (c) 2020, NVIDIA CORPORATION. All rights reserved. Thanks.
[PATCH RFC v2 08/10] vdpa: add vdpa simulator for block device
From: Max Gurtovoy
This will allow running vDPA for virtio block protocol.
Signed-off-by: Max Gurtovoy
[sgarzare: various cleanups/fixes]
Signed-off-by: Stefano Garzarella
---
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
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
new file mode 100644
index ..999f9ca0b628
--- /dev/null
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
@@ -0,0 +1,145 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * VDPA simulator for block device.
+ *
+ * Copyright (c) 2020, Mellanox Technologies. All rights reserved.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "vdpa_sim.h"
+
+#define DRV_VERSION "0.1"
+#define DRV_AUTHOR "Max Gurtovoy "
+#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 0x4
+#define VDPASIM_BLK_SIZE_MAX 0x1000
+#define VDPASIM_BLK_SEG_MAX32
+#define VDPASIM_BLK_VQ_NUM 1
+
+static struct vdpasim *vdpasim_blk_dev;
+
+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(>lock);
+
+ if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
+ goto out;
+
+ for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
+ struct vdpasim_virtqueue *vq = >vqs[i];
+
+ if (!vq->ready)
+ continue;
+
+ while (vringh_getdesc_iotlb(>vring, >out_iov,
+ >in_iov, >head,
+ GFP_ATOMIC) > 0) {
+ int write;
+
+ vq->in_iov.i = vq->in_iov.used - 1;
+ write = vringh_iov_push_iotlb(>vring, >in_iov,
+ , 1);
+ if (write <= 0)
+ break;
+
+ /* Make sure data is wrote before advancing index */
+ smp_wmb();
+
+ vringh_complete_iotlb(>vring, vq->head, write);
+
+ /* Make sure used is visible before rasing the
interrupt. */
+ smp_wmb();
+
+ local_bh_disable();
+ if (vringh_need_notify_iotlb(>vring) > 0)
+ vringh_notify(>vring);
+ local_bh_enable();
+ }
+ }
+out:
+ spin_unlock(>lock);
+}
+
+static void vdpasim_blk_get_config(struct vdpasim *vdpasim, void *config)
+{
+ struct virtio_blk_config *blk_config =
+ (struct virtio_blk_config *)config;
+
+ memset(config, 0, sizeof(struct virtio_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);
+}
+
+static int __init vdpasim_blk_init(void)
+{
+ struct vdpasim_dev_attr dev_attr = {};
+ int ret;
+
+ 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(struct virtio_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(_attr);
+
