Re: [PATCH v3 19/19] vdpa: split vdpasim to core and net modules
On 2020/12/4 上午1:05, Stefano Garzarella wrote: From: Max Gurtovoy Introduce new vdpa_sim_net and vdpa_sim (core) drivers. This is a preparation for adding a vdpa simulator module for block devices. Signed-off-by: Max Gurtovoy [sgarzare: various cleanups/fixes] Signed-off-by: Stefano Garzarella --- v2: - Fixed "warning: variable 'dev' is used uninitialized" reported by 'kernel test robot' and Dan Carpenter - rebased on top of other changes (dev_attr, get_config(), notify(), etc.) - left batch_mapping module parameter in the core [Jason] v1: - Removed unused headers - Removed empty module_init() module_exit() - Moved vdpasim_is_little_endian() in vdpa_sim.h - Moved vdpasim16_to_cpu/cpu_to_vdpasim16() in vdpa_sim.h - Added vdpasim*_to_cpu/cpu_to_vdpasim*() also for 32 and 64 - Replaced 'select VDPA_SIM' with 'depends on VDPA_SIM' since selected option can not depend on other [Jason] --- drivers/vdpa/vdpa_sim/vdpa_sim.h | 105 + drivers/vdpa/vdpa_sim/vdpa_sim.c | 224 +-- drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 171 drivers/vdpa/Kconfig | 13 +- drivers/vdpa/vdpa_sim/Makefile | 1 + 5 files changed, 292 insertions(+), 222 deletions(-) create mode 100644 drivers/vdpa/vdpa_sim/vdpa_sim.h create mode 100644 drivers/vdpa/vdpa_sim/vdpa_sim_net.c Acked-by: Jason Wang ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 15/19] vdpa_sim: set vringh notify callback
On 2020/12/4 上午1:05, Stefano Garzarella wrote:
Instead of calling the vq callback directly, we can leverage the
vringh_notify() function, adding vdpasim_vq_notify() and setting it
in the vringh notify callback.
Suggested-by: Jason Wang
Signed-off-by: Stefano Garzarella
Acked-by: Jason Wang
---
v3:
- cleared notify during reset [Jason]
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 23 +++
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 03a8717f80ea..1243b02488f7 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -123,6 +123,17 @@ static struct vdpasim *dev_to_sim(struct device *dev)
return vdpa_to_sim(vdpa);
}
+static void vdpasim_vq_notify(struct vringh *vring)
+{
+ struct vdpasim_virtqueue *vq =
+ container_of(vring, struct vdpasim_virtqueue, vring);
+
+ if (!vq->cb)
+ return;
+
+ vq->cb(vq->private);
+}
+
static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx)
{
struct vdpasim_virtqueue *vq = >vqs[idx];
@@ -134,6 +145,8 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim,
unsigned int idx)
(uintptr_t)vq->driver_addr,
(struct vring_used *)
(uintptr_t)vq->device_addr);
+
+ vq->vring.notify = vdpasim_vq_notify;
}
static void vdpasim_vq_reset(struct vdpasim *vdpasim,
@@ -147,6 +160,8 @@ static void vdpasim_vq_reset(struct vdpasim *vdpasim,
vq->private = NULL;
vringh_init_iotlb(>vring, vdpasim->dev_attr.supported_features,
VDPASIM_QUEUE_MAX, false, NULL, NULL, NULL);
+
+ vq->vring.notify = NULL;
}
static void vdpasim_reset(struct vdpasim *vdpasim)
@@ -223,10 +238,10 @@ static void vdpasim_net_work(struct work_struct *work)
smp_wmb();
local_bh_disable();
- if (txq->cb)
- txq->cb(txq->private);
- if (rxq->cb)
- rxq->cb(rxq->private);
+ if (vringh_need_notify_iotlb(>vring) > 0)
+ vringh_notify(>vring);
+ if (vringh_need_notify_iotlb(>vring) > 0)
+ vringh_notify(>vring);
local_bh_enable();
if (++pkts > 4) {
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 14/19] vdpa_sim: add set_config callback in vdpasim_dev_attr
On 2020/12/4 上午1:05, Stefano Garzarella wrote:
The set_config callback can be used by the device to parse the
config structure modified by the driver.
The callback will be invoked, if set, in vdpasim_set_config() after
copying bytes from caller buffer into vdpasim->config buffer.
Signed-off-by: Stefano Garzarella
Acked-by: Jason Wang
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index f935ade0806b..03a8717f80ea 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -70,6 +70,7 @@ struct vdpasim_dev_attr {
work_func_t work_fn;
void (*get_config)(struct vdpasim *vdpasim, void *config);
+ void (*set_config)(struct vdpasim *vdpasim, const void *config);
};
/* State of each vdpasim device */
@@ -598,7 +599,15 @@ static void vdpasim_get_config(struct vdpa_device *vdpa,
unsigned int offset,
static void vdpasim_set_config(struct vdpa_device *vdpa, unsigned int offset,
const void *buf, unsigned int len)
{
- /* No writable config supportted by vdpasim */
+ struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+
+ if (offset + len > vdpasim->dev_attr.config_size)
+ return;
+
+ memcpy(vdpasim->config + offset, buf, len);
+
+ if (vdpasim->dev_attr.set_config)
+ vdpasim->dev_attr.set_config(vdpasim, vdpasim->config);
}
static u32 vdpasim_get_generation(struct vdpa_device *vdpa)
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 13/19] vdpa_sim: add get_config callback in vdpasim_dev_attr
On 2020/12/4 上午1:05, Stefano Garzarella wrote:
The get_config callback can be used by the device to fill the
config structure.
The callback will be invoked in vdpasim_get_config() before copying
bytes into caller buffer.
Move vDPA-net config updates from vdpasim_set_features() in the
new vdpasim_net_get_config() callback.
Signed-off-by: Stefano Garzarella
---
v3:
- checked if get_config callback is set before call it
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 35 +++-
1 file changed, 21 insertions(+), 14 deletions(-)
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index fe71ed7890e1..f935ade0806b 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -60,6 +60,8 @@ struct vdpasim_virtqueue {
#define VDPASIM_NET_FEATURES (VDPASIM_FEATURES | \
(1ULL << VIRTIO_NET_F_MAC))
+struct vdpasim;
+
struct vdpasim_dev_attr {
u64 supported_features;
size_t config_size;
@@ -67,6 +69,7 @@ struct vdpasim_dev_attr {
u32 id;
work_func_t work_fn;
+ void (*get_config)(struct vdpasim *vdpasim, void *config);
};
/* State of each vdpasim device */
@@ -522,8 +525,6 @@ static u64 vdpasim_get_features(struct vdpa_device *vdpa)
static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
- struct virtio_net_config *config =
- (struct virtio_net_config *)vdpasim->config;
/* DMA mapping must be done by driver */
if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
@@ -531,16 +532,6 @@ static int vdpasim_set_features(struct vdpa_device *vdpa,
u64 features)
vdpasim->features = features & vdpasim->dev_attr.supported_features;
- /* We generally only know whether guest is using the legacy interface
-* here, so generally that's the earliest we can set config fields.
-* Note: We actually require VIRTIO_F_ACCESS_PLATFORM above which
-* implies VIRTIO_F_VERSION_1, but let's not try to be clever here.
-*/
-
- config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
- config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
- memcpy(config->mac, macaddr_buf, ETH_ALEN);
Patch looks good to me.
But we need Michael to confirm whether doing moving like this is safe. I
guess what has been done were trying to make sure get_config() fail
before set_features(), but it's not clear to me whether it's useful.
Thanks
-
return 0;
}
@@ -595,8 +586,13 @@ static void vdpasim_get_config(struct vdpa_device *vdpa, unsigned int offset,
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
- if (offset + len < vdpasim->dev_attr.config_size)
- memcpy(buf, vdpasim->config + offset, len);
+ if (offset + len > vdpasim->dev_attr.config_size)
+ return;
+
+ if (vdpasim->dev_attr.get_config)
+ vdpasim->dev_attr.get_config(vdpasim, vdpasim->config);
+
+ memcpy(buf, vdpasim->config + offset, len);
}
static void vdpasim_set_config(struct vdpa_device *vdpa, unsigned int offset,
@@ -739,6 +735,16 @@ static const struct vdpa_config_ops
vdpasim_batch_config_ops = {
.free = vdpasim_free,
};
+static void vdpasim_net_get_config(struct vdpasim *vdpasim, void *config)
+{
+ struct virtio_net_config *net_config =
+ (struct virtio_net_config *)config;
+
+ net_config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
+ net_config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
+ memcpy(net_config->mac, macaddr_buf, ETH_ALEN);
+}
+
static int __init vdpasim_dev_init(void)
{
struct vdpasim_dev_attr dev_attr = {};
@@ -747,6 +753,7 @@ static int __init vdpasim_dev_init(void)
dev_attr.supported_features = VDPASIM_NET_FEATURES;
dev_attr.nvqs = VDPASIM_VQ_NUM;
dev_attr.config_size = sizeof(struct virtio_net_config);
+ dev_attr.get_config = vdpasim_net_get_config;
dev_attr.work_fn = vdpasim_net_work;
vdpasim_dev = vdpasim_create(_attr);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 12/19] vdpa_sim: make 'config' generic and usable for any device type
On 2020/12/4 上午1:05, Stefano Garzarella wrote:
Add new 'config_size' attribute in 'vdpasim_dev_attr' and allocates
'config' dynamically to support any device types.
Signed-off-by: Stefano Garzarella
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
Acked-by: Jason Wang
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 949f4231d08a..fe71ed7890e1 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -62,6 +62,7 @@ struct vdpasim_virtqueue {
struct vdpasim_dev_attr {
u64 supported_features;
+ size_t config_size;
int nvqs;
u32 id;
@@ -76,7 +77,8 @@ struct vdpasim {
struct vdpasim_dev_attr dev_attr;
/* spinlock to synchronize virtqueue state */
spinlock_t lock;
- struct virtio_net_config config;
+ /* virtio config according to device type */
+ void *config;
struct vhost_iotlb *iommu;
void *buffer;
u32 status;
@@ -376,6 +378,10 @@ static struct vdpasim *vdpasim_create(struct
vdpasim_dev_attr *dev_attr)
goto err_iommu;
set_dma_ops(dev, _dma_ops);
+ vdpasim->config = kzalloc(dev_attr->config_size, GFP_KERNEL);
+ if (!vdpasim->config)
+ goto err_iommu;
+
vdpasim->vqs = kcalloc(dev_attr->nvqs, sizeof(struct vdpasim_virtqueue),
GFP_KERNEL);
if (!vdpasim->vqs)
@@ -516,7 +522,8 @@ static u64 vdpasim_get_features(struct vdpa_device *vdpa)
static int vdpasim_set_features(struct vdpa_device *vdpa, u64 features)
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
- struct virtio_net_config *config = >config;
+ struct virtio_net_config *config =
+ (struct virtio_net_config *)vdpasim->config;
/* DMA mapping must be done by driver */
if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)))
@@ -588,8 +595,8 @@ static void vdpasim_get_config(struct vdpa_device *vdpa,
unsigned int offset,
{
struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
- if (offset + len < sizeof(struct virtio_net_config))
- memcpy(buf, (u8 *)>config + offset, len);
+ if (offset + len < vdpasim->dev_attr.config_size)
+ memcpy(buf, vdpasim->config + offset, len);
}
static void vdpasim_set_config(struct vdpa_device *vdpa, unsigned int offset,
@@ -676,6 +683,7 @@ static void vdpasim_free(struct vdpa_device *vdpa)
if (vdpasim->iommu)
vhost_iotlb_free(vdpasim->iommu);
kfree(vdpasim->vqs);
+ kfree(vdpasim->config);
}
static const struct vdpa_config_ops vdpasim_config_ops = {
@@ -738,6 +746,7 @@ static int __init vdpasim_dev_init(void)
dev_attr.id = VIRTIO_ID_NET;
dev_attr.supported_features = VDPASIM_NET_FEATURES;
dev_attr.nvqs = VDPASIM_VQ_NUM;
+ dev_attr.config_size = sizeof(struct virtio_net_config);
dev_attr.work_fn = vdpasim_net_work;
vdpasim_dev = vdpasim_create(_attr);
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vhost scsi: fix error return code in vhost_scsi_set_endpoint()
On 2020/12/4 下午4:43, Zhang Changzhong wrote:
Fix to return a negative error code from the error handling
case instead of 0, as done elsewhere in this function.
Fixes: 25b98b64e284 ("vhost scsi: alloc cmds per vq instead of session")
Reported-by: Hulk Robot
Signed-off-by: Zhang Changzhong
---
drivers/vhost/scsi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 6ff8a5096..4ce9f00 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1643,7 +1643,8 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
if (!vhost_vq_is_setup(vq))
continue;
- if (vhost_scsi_setup_vq_cmds(vq, vq->num))
+ ret = vhost_scsi_setup_vq_cmds(vq, vq->num);
+ if (ret)
goto destroy_vq_cmds;
}
Acked-by: Jason Wang
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [RFC PATCH 5/8] vhost: allow userspace to bind vqs to CPUs
On 2020/12/5 上午12:32, Mike Christie wrote:
On 12/4/20 2:09 AM, Jason Wang wrote:
On 2020/12/4 下午3:56, Mike Christie wrote:
+static long vhost_vring_set_cpu(struct vhost_dev *d, struct
vhost_virtqueue *vq,
+ void __user *argp)
+{
+ struct vhost_vring_state s;
+ int ret = 0;
+
+ if (vq->private_data)
+ return -EBUSY;
+
+ if (copy_from_user(, argp, sizeof s))
+ return -EFAULT;
+
+ if (s.num == -1) {
+ vq->cpu = s.num;
+ return 0;
+ }
+
+ if (s.num >= nr_cpu_ids)
+ return -EINVAL;
+
+ if (!d->ops || !d->ops->get_workqueue)
+ return -EINVAL;
+
+ if (!d->wq)
+ d->wq = d->ops->get_workqueue();
+ if (!d->wq)
+ return -EINVAL;
+
+ vq->cpu = s.num;
+ return ret;
+}
So one question here. Who is in charge of doing this set_cpu? Note
that sched_setaffinity(2) requires CAP_SYS_NICE to work, so I wonder
whether or not it's legal for unprivileged Qemu to do this.
I was having qemu do it when it's setting up the vqs since it had the
info there already.
Is it normally the tool that makes calls into qemu that does the
operations that require CAP_SYS_NICE?
My understanding is that it only matter scheduling. And this patch wants
to change the affinity which should check that capability.
If so, then I see the interface needs to be changed.
Actually, if I read this patch correctly it requires e.g qemu to make
the decision instead of the management layer. This may bring some
troubles to for e.g the libvirt emulatorpin[1] implementation.
Thanks
[1]
https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/virtualization_tuning_and_optimization_guide/sect-virtualization_tuning_optimization_guide-numa-numa_and_libvirt
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 05/19] vdpa_sim: remove the limit of IOTLB entries
On 2020/12/4 上午1:04, Stefano Garzarella wrote: The simulated devices can support multiple queues, so this limit should be defined according to the number of queues supported by the device. Since we are in a simulator, let's simply remove that limit. Suggested-by: Jason Wang Acked-by: Jason Wang Signed-off-by: Stefano Garzarella Rethink about this, since simulator can be used by VM, so the allocation is actually guest trigger-able when vIOMMU is enabled. This means we need a limit somehow, (e.g I remember swiotlb is about 64MB by default). Or having a module parameter for this. Btw, have you met any issue when using 2048, I guess it can happen when we run several processes in parallel? --- v3: - used VHOST_IOTLB_UNLIMITED macro [Jason] v2: - added VDPASIM_IOTLB_LIMIT macro [Jason] --- drivers/vdpa/vdpa_sim/vdpa_sim.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c index 295a770caac0..688aceaa6543 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c @@ -368,7 +368,7 @@ static struct vdpasim *vdpasim_create(void) if (!vdpasim->vqs) goto err_iommu; - vdpasim->iommu = vhost_iotlb_alloc(2048, 0); + vdpasim->iommu = vhost_iotlb_alloc(VHOST_IOTLB_UNLIMITED, 0); if (!vdpasim->iommu) goto err_iommu; ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 04/19] vhost/iotlb: add VHOST_IOTLB_UNLIMITED macro
On 2020/12/4 上午1:04, Stefano Garzarella wrote:
It's possible to allocate an unlimited IOTLB calling
vhost_iotlb_alloc() with 'limit' = 0.
Add a new macro (VHOST_IOTLB_UNLIMITED) for this case and document
it in the vhost_iotlb_alloc() documentation block.
Suggested-by: Jason Wang
Signed-off-by: Stefano Garzarella
Acked-by: Jason Wang
---
include/linux/vhost_iotlb.h | 2 ++
drivers/vhost/iotlb.c | 3 ++-
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/linux/vhost_iotlb.h b/include/linux/vhost_iotlb.h
index 6b09b786a762..47019f97f795 100644
--- a/include/linux/vhost_iotlb.h
+++ b/include/linux/vhost_iotlb.h
@@ -4,6 +4,8 @@
#include
+#define VHOST_IOTLB_UNLIMITED 0
+
struct vhost_iotlb_map {
struct rb_node rb;
struct list_head link;
diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c
index 0fd3f87e913c..80fdde78ee5a 100644
--- a/drivers/vhost/iotlb.c
+++ b/drivers/vhost/iotlb.c
@@ -100,7 +100,8 @@ EXPORT_SYMBOL_GPL(vhost_iotlb_del_range);
/**
* vhost_iotlb_alloc - add a new vhost IOTLB
- * @limit: maximum number of IOTLB entries
+ * @limit: maximum number of IOTLB entries (use VHOST_IOTLB_UNLIMITED for an
+ * unlimited IOTLB)
* @flags: VHOST_IOTLB_FLAG_XXX
*
* Returns an error is memory allocation fails
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH net-next] tun: fix ubuf refcount incorrectly on error path
On 2020/12/4 下午6:22, wangyunjian wrote:
-Original Message-
From: Jason Wang [mailto:jasow...@redhat.com]
Sent: Friday, December 4, 2020 2:11 PM
To: wangyunjian ; m...@redhat.com
Cc: virtualization@lists.linux-foundation.org; net...@vger.kernel.org; Lilijun
(Jerry) ; xudingke
Subject: Re: [PATCH net-next] tun: fix ubuf refcount incorrectly on error path
On 2020/12/3 下午4:00, wangyunjian wrote:
From: Yunjian Wang
After setting callback for ubuf_info of skb, the callback
(vhost_net_zerocopy_callback) will be called to decrease the refcount
when freeing skb. But when an exception occurs afterwards, the error
handling in vhost handle_tx() will try to decrease the same refcount
again. This is wrong and fix this by clearing ubuf_info when meeting
errors.
Fixes: 4477138fa0ae ("tun: properly test for IFF_UP")
Fixes: 90e33d459407 ("tun: enable napi_gro_frags() for TUN/TAP
driver")
Signed-off-by: Yunjian Wang
---
drivers/net/tun.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index
2dc1988a8973..3614bb1b6d35 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1861,6 +1861,12 @@ static ssize_t tun_get_user(struct tun_struct
*tun, struct tun_file *tfile,
if (unlikely(!(tun->dev->flags & IFF_UP))) {
err = -EIO;
rcu_read_unlock();
+ if (zerocopy) {
+ skb_shinfo(skb)->destructor_arg = NULL;
+ skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
+ skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG;
+ }
+
goto drop;
}
@@ -1874,6 +1880,11 @@ static ssize_t tun_get_user(struct tun_struct
*tun, struct tun_file *tfile,
if (unlikely(headlen > skb_headlen(skb))) {
atomic_long_inc(>dev->rx_dropped);
+ if (zerocopy) {
+ skb_shinfo(skb)->destructor_arg = NULL;
+ skb_shinfo(skb)->tx_flags &=
~SKBTX_DEV_ZEROCOPY;
+ skb_shinfo(skb)->tx_flags &= ~SKBTX_SHARED_FRAG;
+ }
napi_free_frags(>napi);
rcu_read_unlock();
mutex_unlock(>napi_mutex);
It looks to me then we miss the failure feedback.
The issues comes from the inconsistent error handling in tun.
I wonder whether we can simply do uarg->callback(uarg, false) if necessary on
every failture path on tun_get_user().
How about this?
---
drivers/net/tun.c | 29 ++---
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 2dc1988a8973..36a8d8eacd7b 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1637,6 +1637,19 @@ static struct sk_buff *tun_build_skb(struct tun_struct
*tun,
return NULL;
}
+/* copy ubuf_info for callback when skb has no error */
+inline static tun_copy_ubuf_info(struct sk_buff *skb, bool zerocopy, void
*msg_control)
+{
+ if (zerocopy) {
+ skb_shinfo(skb)->destructor_arg = msg_control;
+ skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
+ skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
+ } else if (msg_control) {
+ struct ubuf_info *uarg = msg_control;
+ uarg->callback(uarg, false);
+ }
+}
+
/* Get packet from user space buffer */
static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
void *msg_control, struct iov_iter *from,
@@ -1812,16 +1825,6 @@ static ssize_t tun_get_user(struct tun_struct *tun,
struct tun_file *tfile,
break;
}
- /* copy skb_ubuf_info for callback when skb has no error */
- if (zerocopy) {
- skb_shinfo(skb)->destructor_arg = msg_control;
- skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
- skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
- } else if (msg_control) {
- struct ubuf_info *uarg = msg_control;
- uarg->callback(uarg, false);
- }
-
skb_reset_network_header(skb);
skb_probe_transport_header(skb);
skb_record_rx_queue(skb, tfile->queue_index);
@@ -1830,6 +1833,7 @@ static ssize_t tun_get_user(struct tun_struct *tun,
struct tun_file *tfile,
struct bpf_prog *xdp_prog;
int ret;
+ tun_copy_ubuf_info(skb, zerocopy, msg_control);
If you think disabling zerocopy for XDP (which I think it makes sense).
It's better to do this in another patch.
local_bh_disable();
rcu_read_lock();
xdp_prog = rcu_dereference(tun->xdp_prog);
@@ -1880,7 +1884,7 @@ static ssize_t tun_get_user(struct tun_struct *tun,
struct tun_file *tfile,
WARN_ON(1);
return -ENOMEM;
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 2020/12/4 下午5:36, Enrico Weigelt, metux IT consult wrote:
On 04.12.20 04:35, Jason Wang wrote:
Hi,
Is the plan to keep this doc synced with the one in the virtio
specification?
Yes, of course. I'm still in progress of doing the beaurocratic stuff w/
virtio-tc folks (ID registration, ...) - yet have to see whether they
wanna add it to their spec documents ...
BTW: if you feel, sometings not good w/ the current spec, please raise
your voice now.
But, has the spec path posted?
I think it's better to use u8 ot uint8_t here.Git grep told me the
former is more popular under Documentation/.
thx, I'll fix that
+- for version field currently only value 1 supported.
+- the line names block holds a stream of zero-terminated strings,
+ holding the individual line names.
I'm not sure but does this mean we don't have a fixed length of config
space? Need to check whether it can bring any trouble to
migration(compatibility).
Yes, it depends on how many gpio lines are present and how much space
their names take up.
A fixed size would either put unpleasent limits on the max number of
lines or waste a lot space when only few lines present.
Not that virtio-gpio is also meant for small embedded workloads running
under some hypervisor.
+- unspecified fields are reserved for future use and should be zero.
+
+
+Virtqueues and messages:
+
+
+- Queue #0: transmission from host to guest
+- Queue #1: transmission from guest to host
Virtio became more a popular in the area without virtualization. So I
think it's better to use "device/driver" instead of "host/guest" here.
Good point. But I'd prefer "cpu" instead of "driver" in that case.
Not a native speaker but event sounds like something driver read from
device. Looking at the below lists, most of them except for
VIRTIO_GPIO_EV_HOST_LEVEL looks more like a command.
okay, shall I name it "message" ?
It might be better.
Another question is, what's the benefit of unifying the message format
of the two queues. E.g VIRTIO_GPIO_EV_HOST_LEVEL can only works fro rxq.
Simplicity. Those fields that aren't really relevant (eg. replies also
carry the line id), can just be ignored.
Not familiar with GPIO but I wonder the value of a standalone
VIRTIO_GPIO_EV_GUEST_DIRECTION_INPUT/OUTPUT. Can we simply imply them in
SET/GET_VALUE?
Would introduce more complexity. Somewhere I'd have to fit in some extra
bit for differenciating between line state and line direction. The
direction tells whether the line currently acts as input or output. The
"value" (hmm, maybe I should rethink terminology here) is the current
line level (high/low or active/inactive).
Ok.
+--
+Data flow:
+--
+
+- all operations, except ``VIRTIO_GPIO_EV_HOST_LEVEL``, are
guest-initiated
+- host replies ``VIRTIO_GPIO_EV_HOST_LEVEL`` OR'ed to the ``type`` field
+- ``VIRTIO_GPIO_EV_HOST_LEVEL`` is only sent asynchronically from
host to guest
+- in replies, a negative ``value`` field denotes an unix-style errno
code
Virtio is in a different scope, so we need to define the error code on
our own.
E.g for virtio-net we define:
#define VIRTIO_NET_OK 0
#define VIRTIO_NET_ERR 1
hmm, so I'd need to define all the error codes that possibly could happen ?
Yes, I think you need.
+config GPIO_VIRTIO
+ tristate "VirtIO GPIO support"
+ depends on VIRTIO
Let's use select, since there's no prompt for VIRTIO and it doesn't have
any dependencies.
Ok. I just was under the impression that subsystems and busses should
not be select'ed, but depends on (eg. some time ago tried that w/ gpio
subsys and failed).
+ help
+ Say Y here to enable guest support for virtio-based GPIOs.
+
+ These virtual GPIOs can be routed to real GPIOs or attached to
+ simulators on the host (qemu).
It's better to avoid talking host and qemu here for new virtio devices.
Ok, dropped that line.
+static int virtio_gpio_xmit(struct virtio_gpio_priv *priv, int type,
+ int pin, int value, struct virtio_gpio_event *ev)
+{
+ struct scatterlist sg[1];
+ int ret;
+ unsigned long flags;
+
+ WARN_ON(!ev);
+
+ ev->type = type;
+ ev->pin = pin;
+ ev->value = value;
+
+ sg_init_table(sg, 1);
+ sg_set_buf([0], ev, sizeof(struct virtio_gpio_event));
+
+ spin_lock_irqsave(>vq_lock, flags);
+ ret = virtqueue_add_outbuf(priv->vq_tx, sg, ARRAY_SIZE(sg),
+ priv, GFP_KERNEL);
+ if (ret < 0) {
+ dev_err(>vdev->dev,
+ "virtqueue_add_outbuf() failed: %d\n", ret);
+ goto out;
So except for the error log, the failure is silently ignored by the
caller. Is this intended?
ups, I've forgotten the error handling in the caller. fixed in v3.
+static int virtio_gpio_req(struct virtio_gpio_priv *priv, int type,
+ int pin, int value)
+{
+ struct virtio_gpio_event *ev
+ = kzalloc(>vdev->dev, sizeof(struct
Re: [PATCH V2 19/19] vdpa: introduce virtio pci driver
On 2020/12/5 上午1:12, Randy Dunlap wrote: On 12/4/20 7:20 AM, Stefano Garzarella wrote: On Fri, Dec 04, 2020 at 12:03:53PM +0800, Jason Wang wrote: This patch introduce a vDPA driver for virtio-pci device. It bridges the virtio-pci control command to the vDPA bus. This will be used for features prototyping and testing. Note that get/restore virtqueue state is not supported which needs extension on the virtio specification. Signed-off-by: Jason Wang --- drivers/vdpa/Kconfig | 6 + drivers/vdpa/Makefile | 1 + drivers/vdpa/virtio_pci/Makefile | 2 + drivers/vdpa/virtio_pci/vp_vdpa.c | 455 ++ 4 files changed, 464 insertions(+) create mode 100644 drivers/vdpa/virtio_pci/Makefile create mode 100644 drivers/vdpa/virtio_pci/vp_vdpa.c diff --git a/drivers/vdpa/Kconfig b/drivers/vdpa/Kconfig index 358f6048dd3c..18cad14f533e 100644 --- a/drivers/vdpa/Kconfig +++ b/drivers/vdpa/Kconfig @@ -48,4 +48,10 @@ config MLX5_VDPA_NET be executed by the hardware. It also supports a variety of stateless offloads depending on the actual device used and firmware version. +config VP_VDPA + tristate "Virtio PCI bridge vDPA driver" + depends on PCI_MSI && VIRTIO_PCI_MODERN + help + This kernel module that bridges virtio PCI device to vDPA bus. ^ Without 'that' maybe sound better, but I'm not a native speaker :-) Yes, drop 'that', please. Will fix. Thanks + endif # VDPA ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 2020/12/6 上午4:05, Enrico Weigelt, metux IT consult wrote: On 05.12.20 20:32, Michael S. Tsirkin wrote: Hi, It seems a bit of a mess, at this point I'm not entirely sure when should drivers select VIRTIO and when depend on it. if VIRTIO just enables something that could be seen as library functions, then select should be right, IMHO. The text near it says: # SPDX-License-Identifier: GPL-2.0-only config VIRTIO tristate oh, wait, doesn't have an menu text, so we can't even explicitly enable it (not shown in menu) - only implicitly. Which means that some other option must select it, in order to become availe at all, and in order to make others depending on it becoming available. IMHO, therefore select is the correct approach. help This option is selected by any driver which implements the virtio bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG or CONFIG_S390_GUEST. Which seems clear enough and would indicate drivers for devices *behind* the bus should not select VIRTIO and thus presumably should "depend on" it. This is violated in virtio console and virtio fs drivers. See above: NAK. because it can't even be enabled directly (by the user). If it wasn't meant otherwise, we'd have to add an menu text. For console it says: commit 9f30eb29c514589e16f2999ea070598583d1f6ec Author: Michal Suchanek Date: Mon Aug 31 18:58:50 2020 +0200 char: virtio: Select VIRTIO from VIRTIO_CONSOLE. Make it possible to have virtio console built-in when other virtio drivers are modular. Signed-off-by: Michal Suchanek Reviewed-by: Amit Shah Link: https://lore.kernel.org/r/20200831165850.26163-1-msucha...@suse.de Signed-off-by: Greg Kroah-Hartman which seems kind of bogus - why do we care about allowing a builtin virtio console driver if the pci virtio bus driver is a module? There won't be any devices on the bus to attach to ... When using other transports ? In my current project, eg. I'm using mmio - my kernel has pci completely disabled. I am inclined to fix console and virtio fs to depend on VIRTIO: select is harder to use correctly ... I don't thinkt that would be good - instead everybody should just select VIRTIO, never depend on it (maybe depend on VIRTIO_MENU instead) I'm fine with either. Though I prefer to use select but it looks to me adding a prompt and use enable would be easier. Thanks --mtx ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 2/2] drivers: gpio: add virtio-gpio guest driver
On 2020/12/6 上午3:32, Michael S. Tsirkin wrote: On Sat, Dec 05, 2020 at 08:59:55AM +0100, Enrico Weigelt, metux IT consult wrote: On 04.12.20 04:35, Jason Wang wrote: --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1615,6 +1615,15 @@ config GPIO_MOCKUP        tools/testing/selftests/gpio/gpio-mockup.sh. Reference the usage in        it.  +config GPIO_VIRTIO +   tristate "VirtIO GPIO support" +   depends on VIRTIO Let's use select, since there's no prompt for VIRTIO and it doesn't have any dependencies. whoops, it's not that simple: make: Entering directory '/home/nekrad/src/apu2-dev/pkg/kernel.apu2.git' make[1]: Entering directory '/home/nekrad/src/dk/DistroKit/platform-x86_64/build-target/linux-5.8.9-build' GEN Makefile drivers/gpu/drm/Kconfig:74:error: recursive dependency detected! drivers/gpu/drm/Kconfig:74: symbol DRM_KMS_HELPER is selected by DRM_VIRTIO_GPU drivers/gpu/drm/virtio/Kconfig:2: symbol DRM_VIRTIO_GPU depends on VIRTIO drivers/virtio/Kconfig:2: symbol VIRTIO is selected by GPIO_VIRTIO drivers/gpio/Kconfig:1618: symbol GPIO_VIRTIO depends on GPIOLIB drivers/gpio/Kconfig:14:symbol GPIOLIB is selected by I2C_MUX_LTC4306 drivers/i2c/muxes/Kconfig:47: symbol I2C_MUX_LTC4306 depends on I2C drivers/i2c/Kconfig:8: symbol I2C is selected by FB_DDC drivers/video/fbdev/Kconfig:63: symbol FB_DDC depends on FB drivers/video/fbdev/Kconfig:12: symbol FB is selected by DRM_KMS_FB_HELPER drivers/gpu/drm/Kconfig:80: symbol DRM_KMS_FB_HELPER depends on DRM_KMS_HELPER Seems that we can only depend on or select some symbol - we run into huge trouble if thats mixed. Just changed DRM_VIRTIO_GPU to just select VIRIO instead of depending on it, and now it works. I've posted another patch for fixing drivers/gpu/drm/virtio/Kconfig to use 'select' instead of 'depends on'. It seems a bit of a mess, at this point I'm not entirely sure when should drivers select VIRTIO and when depend on it. The text near it says: # SPDX-License-Identifier: GPL-2.0-only config VIRTIO tristate help This option is selected by any driver which implements the virtio bus, such as CONFIG_VIRTIO_PCI, CONFIG_VIRTIO_MMIO, CONFIG_RPMSG or CONFIG_S390_GUEST. Which seems clear enough and would indicate drivers for devices *behind* the bus should not select VIRTIO and thus presumably should "depend on" it. This is violated in virtio console and virtio fs drivers. For console it says: commit 9f30eb29c514589e16f2999ea070598583d1f6ec Author: Michal Suchanek Date: Mon Aug 31 18:58:50 2020 +0200 char: virtio: Select VIRTIO from VIRTIO_CONSOLE. Make it possible to have virtio console built-in when other virtio drivers are modular. Signed-off-by: Michal Suchanek Reviewed-by: Amit Shah Link: https://lore.kernel.org/r/20200831165850.26163-1-msucha...@suse.de Signed-off-by: Greg Kroah-Hartman which seems kind of bogus - why do we care about allowing a builtin virtio console driver if the pci virtio bus driver is a module? There won't be any devices on the bus to attach to ... For testing like switching bus from pci to MMIO? And for virtio fs it was like this from the beginning. I am inclined to fix console and virtio fs to depend on VIRTIO: select is harder to use correctly ... Jason? I think it works, but we need a prompt for VIRTIO otherwise there's no way to enable it. Thanks -- --- Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren GPG/PGP-Schlüssel zu. --- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering i...@metux.net -- +49-151-27565287 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa/mlx5: Use write memory barrier after updating CQ index
On 2020/12/6 下午6:57, Eli Cohen wrote:
Make sure to put write memory barrier after updating CQ consumer index
so the hardware knows that there are available CQE slots in the queue.
Failure to do this can cause the update of the RX doorbell record to get
updated before the CQ consumer index resulting in CQ overrun.
Change-Id: Ib0ae4c118cce524c9f492b32569179f3c1f04cc1
Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Signed-off-by: Eli Cohen
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 1f4089c6f9d7..295f46eea2a5 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -478,6 +478,11 @@ static int mlx5_vdpa_poll_one(struct mlx5_vdpa_cq *vcq)
static void mlx5_vdpa_handle_completions(struct mlx5_vdpa_virtqueue *mvq, int
num)
{
mlx5_cq_set_ci(>cq.mcq);
+
+ /* make sure CQ cosumer update is visible to the hardware before
updating
+* RX doorbell record.
+*/
+ wmb();
rx_post(>vqqp, num);
if (mvq->event_cb.callback)
mvq->event_cb.callback(mvq->event_cb.private);
Acked-by: Jason Wang
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
