Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
On 5/3/2022 10:44 PM, Eli Cohen wrote:
-Original Message-
From: Si-Wei Liu
Sent: Wednesday, May 4, 2022 7:44 AM
To: Eli Cohen ; m...@redhat.com; jasow...@redhat.com
Cc: virtualization@lists.linux-foundation.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
On 5/2/2022 10:13 PM, Eli Cohen wrote:
-Original Message-
From: Si-Wei Liu
Sent: Tuesday, May 3, 2022 2:48 AM
To: Eli Cohen ; m...@redhat.com; jasow...@redhat.com
Cc: virtualization@lists.linux-foundation.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
On 5/2/2022 3:22 AM, Eli Cohen wrote:
Allows to read vendor statistics of a vdpa device. The specific
statistics data are received from the upstream driver in the form of an
(attribute name, attribute value) pairs.
An example of statistics for mlx5_vdpa device are:
received_desc - number of descriptors received by the virtqueue
completed_desc - number of descriptors completed by the virtqueue
A descriptor using indirect buffers is still counted as 1. In addition,
N chained descriptors are counted correctly N times as one would expect.
A new callback was added to vdpa_config_ops which provides the means for
the vdpa driver to return statistics results.
The interface allows for reading all the supported virtqueues, including
the control virtqueue if it exists.
Below are some examples taken from mlx5_vdpa which are introduced in the
following patch:
1. Read statistics for the virtqueue at index 1
$ vdpa dev vstats show vdpa-a qidx 1
vdpa-a:
queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
2. Read statistics for the virtqueue at index 32
$ vdpa dev vstats show vdpa-a qidx 32
vdpa-a:
queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
3. Read statisitics for the virtqueue at index 0 with json output
$ vdpa -j dev vstats show vdpa-a qidx 0
{"vstats":{"vdpa-a":{
"queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
"name":"completed_desc","value":417548}}}
4. Read statistics for the virtqueue at index 0 with preety json output
$ vdpa -jp dev vstats show vdpa-a qidx 0
{
"vstats": {
"vdpa-a": {
"queue_type": "rx",
"queue_index": 0,
"name": "received_desc",
"value": 417776,
"name": "completed_desc",
"value": 417548
}
}
}
Signed-off-by: Eli Cohen
---
drivers/vdpa/vdpa.c | 129 ++
include/linux/vdpa.h | 5 ++
include/uapi/linux/vdpa.h | 6 ++
3 files changed, 140 insertions(+)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 2b75c00b1005..933466f61ca8 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct
sk_buff *msg, u32 portid,
return err;
}
+static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
+ struct genl_info *info, u32 index)
+{
+ int err;
+
+ err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
+ if (err)
+ return err;
+
+ if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
+ return -EMSGSIZE;
+
+ return 0;
+}
+
+static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
+struct genl_info *info, u32 index)
+{
+ int err;
+
+ if (!vdev->config->get_vendor_vq_stats)
+ return -EOPNOTSUPP;
+
+ err = vdpa_fill_stats_rec(vdev, msg, info, index);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
+ struct sk_buff *msg,
+ struct genl_info *info, u32 index)
+{
+ u32 device_id;
+ void *hdr;
+ int err;
+ u32 portid = info->snd_portid;
+ u32 seq = info->snd_seq;
+ u32 flags = 0;
+
+ hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
+ VDPA_CMD_DEV_VSTATS_GET);
+ if (!hdr)
+ return -EMSGSIZE;
+
+ if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
+ err = -EMSGSIZE;
+ goto undo_msg;
+ }
+
+ device_id = vdev->config->get_device_id(vdev);
+ if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
+ err = -EMSGSIZE;
+ goto undo_msg;
+ }
+
You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
otherwise I can't image how you can ensure the atomicity to infer
queue_type for control vq.
And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
into vendor_stats_fill()?
It is done in the hardware driver. In this case, in m
Re: [PATCH v4 11/15] drm/shmem-helper: Add generic memory shrinker
On Thu, May 05, 2022 at 10:34:02AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 18.04.22 um 00:37 schrieb Dmitry Osipenko:
> > Introduce a common DRM SHMEM shrinker. It allows to reduce code
> > duplication among DRM drivers that implement theirs own shrinkers.
> > This is initial version of the shrinker that covers basic needs of
> > GPU drivers, both purging and eviction of shmem objects are supported.
> >
> > This patch is based on a couple ideas borrowed from Rob's Clark MSM
> > shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
> >
> > In order to start using DRM SHMEM shrinker drivers should:
> >
> > 1. Implement new purge(), evict() + swap_in() GEM callbacks.
> > 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device).
> > 3. Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API
> > functions to activate shrinking of GEMs.
>
> Honestly speaking, after reading the patch and the discussion here I really
> don't like where all tis is going. The interfaces and implementation are
> overengineered. Descisions about evicting and purging should be done by the
> memory manager. For the most part, it's none of the driver's business.
>
> I'd like to ask you to reduce the scope of the patchset and build the
> shrinker only for virtio-gpu. I know that I first suggested to build upon
> shmem helpers, but it seems that it's easier to do that in a later patchset.
We have a few shrinkers already all over, so extracting that does make
sense I think. I do agree that there's probably a few more steps than
necessary involved right now in all this for the helper<->driver
interface.
-Daniel
>
> Best regards
> Thomas
>
> >
> > Signed-off-by: Daniel Almeida
> > Signed-off-by: Dmitry Osipenko
> > ---
> > drivers/gpu/drm/drm_gem_shmem_helper.c | 765 -
> > include/drm/drm_device.h | 4 +
> > include/drm/drm_gem.h | 35 ++
> > include/drm/drm_gem_shmem_helper.h | 105 +++-
> > 4 files changed, 877 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 3ecef571eff3..3838fb8d6f3a 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -88,6 +88,13 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t
> > size, bool private)
> > INIT_LIST_HEAD(&shmem->madv_list);
> > + /*
> > +* Eviction and purging are disabled by default, shmem user must enable
> > +* them explicitly using drm_gem_shmem_set_evictable/purgeable().
> > +*/
> > + shmem->eviction_disable_count = 1;
> > + shmem->purging_disable_count = 1;
> > +
> > if (!private) {
> > /*
> > * Our buffers are kept pinned, so allocating them
> > @@ -126,6 +133,107 @@ struct drm_gem_shmem_object
> > *drm_gem_shmem_create(struct drm_device *dev, size_t
> > }
> > EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
> > +static void
> > +drm_gem_shmem_add_pages_to_shrinker(struct drm_gem_shmem_object *shmem)
> > +{
> > + struct drm_gem_object *obj = &shmem->base;
> > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
> > + size_t page_count = obj->size >> PAGE_SHIFT;
> > +
> > + if (!shmem->pages_shrinkable) {
> > + WARN_ON(gem_shrinker->shrinkable_count + page_count <
> > page_count);
> > + gem_shrinker->shrinkable_count += page_count;
> > + shmem->pages_shrinkable = true;
> > + }
> > +}
> > +
> > +static void
> > +drm_gem_shmem_remove_pages_from_shrinker(struct drm_gem_shmem_object
> > *shmem)
> > +{
> > + struct drm_gem_object *obj = &shmem->base;
> > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
> > + size_t page_count = obj->size >> PAGE_SHIFT;
> > +
> > + if (shmem->pages_shrinkable) {
> > + WARN_ON(gem_shrinker->shrinkable_count < page_count);
> > + gem_shrinker->shrinkable_count -= page_count;
> > + shmem->pages_shrinkable = false;
> > + }
> > +}
> > +
> > +static void
> > +drm_gem_shmem_set_pages_state_locked(struct drm_gem_shmem_object *shmem,
> > +enum drm_gem_shmem_pages_state new_state)
> > +{
> > + struct drm_gem_object *obj = &shmem->base;
> > + struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
> > +
> > + lockdep_assert_held(&gem_shrinker->lock);
> > + lockdep_assert_held(&obj->resv->lock.base);
> > +
> > + if (new_state >= DRM_GEM_SHMEM_PAGES_STATE_PINNED) {
> > + if (drm_gem_shmem_is_evictable(shmem))
> > + new_state = DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE;
> > +
> > + if (drm_gem_shmem_is_purgeable(shmem))
> > + new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE;
> > +
> > + if (!shmem->pages)
> > + new_state = DRM_GEM_SHMEM_PAGES_STATE_UNPINNED;
> > +
> > + if (shmem->evicted
Re: [PATCH v2] arm64: paravirt: Use RCU read locks to guard stolen_time
Hi Elliot,
On Thu, Apr 28, 2022 at 11:35:36AM -0700, Elliot Berman wrote:
> From: Prakruthi Deepak Heragu
>
> During hotplug, the stolen time data structure is unmapped and memset.
> There is a possibility of the timer IRQ being triggered before memset
> and stolen time is getting updated as part of this timer IRQ handler. This
> causes the below crash in timer handler -
>
> [ 3457.473139][C5] Unable to handle kernel paging request at virtual
> address ffc03df05148
> ...
> [ 3458.154398][C5] Call trace:
> [ 3458.157648][C5] para_steal_clock+0x30/0x50
> [ 3458.162319][C5] irqtime_account_process_tick+0x30/0x194
> [ 3458.168148][C5] account_process_tick+0x3c/0x280
> [ 3458.173274][C5] update_process_times+0x5c/0xf4
> [ 3458.178311][C5] tick_sched_timer+0x180/0x384
> [ 3458.183164][C5] __run_hrtimer+0x160/0x57c
> [ 3458.187744][C5] hrtimer_interrupt+0x258/0x684
> [ 3458.192698][C5] arch_timer_handler_virt+0x5c/0xa0
> [ 3458.198002][C5] handle_percpu_devid_irq+0xdc/0x414
> [ 3458.203385][C5] handle_domain_irq+0xa8/0x168
> [ 3458.208241][C5] gic_handle_irq.34493+0x54/0x244
> [ 3458.213359][C5] call_on_irq_stack+0x40/0x70
> [ 3458.218125][C5] do_interrupt_handler+0x60/0x9c
> [ 3458.223156][C5] el1_interrupt+0x34/0x64
> [ 3458.227560][C5] el1h_64_irq_handler+0x1c/0x2c
> [ 3458.232503][C5] el1h_64_irq+0x7c/0x80
> [ 3458.236736][C5] free_vmap_area_noflush+0x108/0x39c
> [ 3458.242126][C5] remove_vm_area+0xbc/0x118
> [ 3458.246714][C5] vm_remove_mappings+0x48/0x2a4
> [ 3458.251656][C5] __vunmap+0x154/0x278
> [ 3458.255796][C5] stolen_time_cpu_down_prepare+0xc0/0xd8
> [ 3458.261542][C5] cpuhp_invoke_callback+0x248/0xc34
> [ 3458.266842][C5] cpuhp_thread_fun+0x1c4/0x248
> [ 3458.271696][C5] smpboot_thread_fn+0x1b0/0x400
> [ 3458.276638][C5] kthread+0x17c/0x1e0
> [ 3458.280691][C5] ret_from_fork+0x10/0x20
>
> As a fix, introduce rcu lock to update stolen time structure.
>
> Fixes: 75df529bec91 ("arm64: paravirt: Initialize steal time when cpu is
> online")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Prakruthi Deepak Heragu
> Signed-off-by: Elliot Berman
> ---
> Changes since v1:
> https://lore.kernel.org/all/20220420204417.155194-1-quic_eber...@quicinc.com/
> - Use RCU instead of disabling interrupts
>
> arch/arm64/kernel/paravirt.c | 24 +++-
> 1 file changed, 19 insertions(+), 5 deletions(-)
I applied this locally, but sparse is complaining because the 'kaddr' field
of 'struct pv_time_stolen_time_region' is missing an '__rcu' annotation:
| arch/arm64/kernel/paravirt.c:112:9: warning: cast adds address space '__rcu'
to expression [sparse]
| arch/arm64/kernel/paravirt.c:112:9: error: incompatible types in comparison
expression (different address spaces): [sparse]
| arch/arm64/kernel/paravirt.c:112:9:struct pvclock_vcpu_stolen_time
[noderef] __rcu * [sparse]
| arch/arm64/kernel/paravirt.c:112:9:struct pvclock_vcpu_stolen_time *
[sparse]
| arch/arm64/kernel/paravirt.c:67:17: warning: cast adds address space '__rcu'
to expression [sparse]
| arch/arm64/kernel/paravirt.c:67:17: error: incompatible types in comparison
expression (different address spaces): [sparse]
| arch/arm64/kernel/paravirt.c:67:17:struct pvclock_vcpu_stolen_time
[noderef] __rcu * [sparse]
| arch/arm64/kernel/paravirt.c:67:17:struct pvclock_vcpu_stolen_time *
[sparse]
| arch/arm64/kernel/paravirt.c:88:9: warning: cast adds address space '__rcu'
to expression [sparse]
| arch/arm64/kernel/paravirt.c:88:9: error: incompatible types in comparison
expression (different address spaces): [sparse]
| arch/arm64/kernel/paravirt.c:88:9:struct pvclock_vcpu_stolen_time
[noderef] __rcu * [sparse]
| arch/arm64/kernel/paravirt.c:88:9:struct pvclock_vcpu_stolen_time *
[sparse]
The diff below seems to make it happy again, but please can you take a
look?
Cheers,
Will
--->8
diff --git a/arch/arm64/kernel/paravirt.c b/arch/arm64/kernel/paravirt.c
index e724ea3d86f0..57c7c211f8c7 100644
--- a/arch/arm64/kernel/paravirt.c
+++ b/arch/arm64/kernel/paravirt.c
@@ -35,7 +35,7 @@ static u64 native_steal_clock(int cpu)
DEFINE_STATIC_CALL(pv_steal_clock, native_steal_clock);
struct pv_time_stolen_time_region {
- struct pvclock_vcpu_stolen_time *kaddr;
+ struct pvclock_vcpu_stolen_time __rcu *kaddr;
};
static DEFINE_PER_CPU(struct pv_time_stolen_time_region, stolen_time_region);
@@ -84,8 +84,7 @@ static int stolen_time_cpu_down_prepare(unsigned int cpu)
if (!reg->kaddr)
return 0;
- kaddr = reg->kaddr;
- rcu_assign_pointer(reg->kaddr, NULL);
+ kaddr = rcu_replace_pointer(reg->kaddr, NULL, true);
synchronize_rcu();
memunmap(kaddr);
@@ -116,8 +115,8 @@ static int stolen_time_cpu_online(unsigned int cpu)
retur
Re: Re: PING: [PATCH v4 0/5] virtio-crypto: Improve performance
On 5/5/22 12:57, Michael S. Tsirkin wrote: On Thu, May 05, 2022 at 03:14:40AM +, Gonglei (Arei) wrote: -Original Message- From: zhenwei pi [mailto:pizhen...@bytedance.com] Sent: Thursday, May 5, 2022 10:35 AM To: Gonglei (Arei) ; m...@redhat.com; jasow...@redhat.com Cc: herb...@gondor.apana.org.au; linux-ker...@vger.kernel.org; virtualization@lists.linux-foundation.org; linux-cry...@vger.kernel.org; helei.si...@bytedance.com; da...@davemloft.net Subject: PING: [PATCH v4 0/5] virtio-crypto: Improve performance Hi, Lei Jason replied in another patch: Still hundreds of lines of changes, I'd leave this change to other maintainers to decide. Quite frankly, the virtio crypto driver changed only a few in the past, and the performance of control queue is not good enough. I am in doubt about that this driver is not used widely. So I'd like to rework a lot, it would be best to complete this work in 5.18 window. This gets different point with Jason. I would appreciate it if you could give me any hint. This is already in my todo list. Regards, -Gonglei It's been out a month though, not really acceptable latency for review. So I would apply this for next, but you need to address Dan Captenter's comment, and look for simular patterns elesewhere in your patch. I fixed this in the v5 series. Thanks! -- zhenwei pi ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v5 5/5] virtio-crypto: enable retry for virtio-crypto-dev
From: lei he
Enable retry for virtio-crypto-dev, so that crypto-engine
can process cipher-requests parallelly.
Cc: Michael S. Tsirkin
Cc: Jason Wang
Cc: Gonglei
Signed-off-by: lei he
Signed-off-by: zhenwei pi
---
drivers/crypto/virtio/virtio_crypto_core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/virtio/virtio_crypto_core.c
b/drivers/crypto/virtio/virtio_crypto_core.c
index 60490ffa3df1..f67e0d4c1b0c 100644
--- a/drivers/crypto/virtio/virtio_crypto_core.c
+++ b/drivers/crypto/virtio/virtio_crypto_core.c
@@ -144,7 +144,8 @@ static int virtcrypto_find_vqs(struct virtio_crypto *vi)
spin_lock_init(&vi->data_vq[i].lock);
vi->data_vq[i].vq = vqs[i];
/* Initialize crypto engine */
- vi->data_vq[i].engine = crypto_engine_alloc_init(dev, 1);
+ vi->data_vq[i].engine = crypto_engine_alloc_init_and_set(dev,
true, NULL, 1,
+
virtqueue_get_vring_size(vqs[i]));
if (!vi->data_vq[i].engine) {
ret = -ENOMEM;
goto err_engine;
--
2.20.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v5 4/5] virtio-crypto: adjust dst_len at ops callback
From: lei he
For some akcipher operations(eg, decryption of pkcs1pad(rsa)),
the length of returned result maybe less than akcipher_req->dst_len,
we need to recalculate the actual dst_len through the virt-queue
protocol.
Cc: Michael S. Tsirkin
Cc: Jason Wang
Cc: Gonglei
Signed-off-by: lei he
Signed-off-by: zhenwei pi
---
drivers/crypto/virtio/virtio_crypto_akcipher_algs.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 382ccec9ab12..2a60d0525cde 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -90,9 +90,12 @@ static void virtio_crypto_dataq_akcipher_callback(struct
virtio_crypto_request *
}
akcipher_req = vc_akcipher_req->akcipher_req;
- if (vc_akcipher_req->opcode != VIRTIO_CRYPTO_AKCIPHER_VERIFY)
+ if (vc_akcipher_req->opcode != VIRTIO_CRYPTO_AKCIPHER_VERIFY) {
+ /* actuall length maybe less than dst buffer */
+ akcipher_req->dst_len = len - sizeof(vc_req->status);
sg_copy_from_buffer(akcipher_req->dst,
sg_nents(akcipher_req->dst),
vc_akcipher_req->dst_buf,
akcipher_req->dst_len);
+ }
virtio_crypto_akcipher_finalize_req(vc_akcipher_req, akcipher_req,
error);
}
--
2.20.1
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v5 3/5] virtio-crypto: wait ctrl queue instead of busy polling
Originally, after submitting request into virtio crypto control
queue, the guest side polls the result from the virt queue. This
works like following:
CPU0 CPU1 ... CPUx CPUy
| | | |
\ \ / /
\spin_lock(&vcrypto->ctrl_lock)---/
|
virtqueue add & kick
|
busy poll virtqueue
|
spin_unlock(&vcrypto->ctrl_lock)
...
There are two problems:
1, The queue depth is always 1, the performance of a virtio crypto
device gets limited. Multi user processes share a single control
queue, and hit spin lock race from control queue. Test on Intel
Platinum 8260, a single worker gets ~35K/s create/close session
operations, and 8 workers get ~40K/s operations with 800% CPU
utilization.
2, The control request is supposed to get handled immediately, but
in the current implementation of QEMU(v6.2), the vCPU thread kicks
another thread to do this work, the latency also gets unstable.
Tracking latency of virtio_crypto_alg_akcipher_close_session in 5s:
usecs : count distribution
0 -> 1 : 0||
2 -> 3 : 7||
4 -> 7 : 72 ||
8 -> 15 : 186485 ||
16 -> 31 : 687 ||
32 -> 63 : 5||
64 -> 127: 3||
128 -> 255: 1||
256 -> 511: 0||
512 -> 1023 : 0||
1024 -> 2047 : 0||
2048 -> 4095 : 0||
4096 -> 8191 : 0||
8192 -> 16383 : 2||
This means that a CPU may hold vcrypto->ctrl_lock as long as 8192~16383us.
To improve the performance of control queue, a request on control queue
waits completion instead of busy polling to reduce lock racing, and gets
completed by control queue callback.
CPU0 CPU1 ... CPUx CPUy
| | | |
\ \ / /
\spin_lock(&vcrypto->ctrl_lock)---/
|
virtqueue add & kick
|
-spin_unlock(&vcrypto->ctrl_lock)--
/ / \ \
| | | |
wait wait wait wait
Test this patch, the guest side get ~200K/s operations with 300% CPU
utilization.
Cc: Michael S. Tsirkin
Cc: Jason Wang
Cc: Gonglei
Signed-off-by: zhenwei pi
---
.../virtio/virtio_crypto_akcipher_algs.c | 29 ++-
drivers/crypto/virtio/virtio_crypto_common.h | 4 ++
drivers/crypto/virtio/virtio_crypto_core.c| 52 ++-
.../virtio/virtio_crypto_skcipher_algs.c | 34 ++--
4 files changed, 64 insertions(+), 55 deletions(-)
diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 698ea57e2649..382ccec9ab12 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -103,7 +103,6 @@ static int virtio_crypto_alg_akcipher_init_session(struct
virtio_crypto_akcipher
struct scatterlist outhdr_sg, key_sg, inhdr_sg, *sgs[3];
struct virtio_crypto *vcrypto = ctx->vcrypto;
uint8_t *pkey;
- unsigned int inlen;
int err;
unsigned int num_out = 0, num_in = 0;
struct virtio_crypto_op_ctrl_req *ctrl;
@@ -135,18 +134,9 @@ static int virtio_crypto_alg_akcipher_init_session(struct
virtio_crypto_akcipher
sg_init_one(&inhdr_sg, input, sizeof(*input));
sgs[num_out + num_in++] = &inhdr_sg;
- spin_lock(&vcrypto->ctrl_lock);
- err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in,
vcrypto, GFP_ATOMIC);
- if (err < 0) {
- spin_unlock(&vcrypto->ctrl_lock);
+ err = virtio_crypto_ctrl_vq_request(vcrypto, sgs, num_out, num_in,
vc_ctrl_req);
+ if (err < 0)
goto out;
- }
-
- virtqueue_kick(vcrypto->ctrl_vq);
- while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) &&
- !virtqueue_is_broken(vcrypto->ctrl_vq))
- cpu_relax();
- spin_unlock(&vcrypto->ctrl_lock);
if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
[PATCH v5 2/5] virtio-crypto: use private buffer for control request
Originally, all of the control requests share a single buffer(
ctrl & input & ctrl_status fields in struct virtio_crypto), this
allows queue depth 1 only, the performance of control queue gets
limited by this design.
In this patch, each request allocates request buffer dynamically, and
free buffer after request, so the scope protected by ctrl_lock also
get optimized here.
It's possible to optimize control queue depth in the next step.
A necessary comment is already in code, still describe it again:
/*
* Note: there are padding fields in request, clear them to zero before
* sending to host to avoid to divulge any information.
* Ex, virtio_crypto_ctrl_request::ctrl::u::destroy_session::padding[48]
*/
So use kzalloc to allocate buffer of struct virtio_crypto_ctrl_request.
Potentially dereferencing uninitialized variables:
Reported-by: kernel test robot
Reported-by: Dan Carpenter
Cc: Michael S. Tsirkin
Cc: Jason Wang
Cc: Gonglei
Signed-off-by: zhenwei pi
---
.../virtio/virtio_crypto_akcipher_algs.c | 57 ---
drivers/crypto/virtio/virtio_crypto_common.h | 17 --
.../virtio/virtio_crypto_skcipher_algs.c | 50 ++--
3 files changed, 79 insertions(+), 45 deletions(-)
diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index 20901a263fc8..698ea57e2649 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -108,16 +108,22 @@ static int virtio_crypto_alg_akcipher_init_session(struct
virtio_crypto_akcipher
unsigned int num_out = 0, num_in = 0;
struct virtio_crypto_op_ctrl_req *ctrl;
struct virtio_crypto_session_input *input;
+ struct virtio_crypto_ctrl_request *vc_ctrl_req;
pkey = kmemdup(key, keylen, GFP_ATOMIC);
if (!pkey)
return -ENOMEM;
- spin_lock(&vcrypto->ctrl_lock);
- ctrl = &vcrypto->ctrl;
+ vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
+ if (!vc_ctrl_req) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ ctrl = &vc_ctrl_req->ctrl;
memcpy(&ctrl->header, header, sizeof(ctrl->header));
memcpy(&ctrl->u, para, sizeof(ctrl->u));
- input = &vcrypto->input;
+ input = &vc_ctrl_req->input;
input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
@@ -129,16 +135,22 @@ static int virtio_crypto_alg_akcipher_init_session(struct
virtio_crypto_akcipher
sg_init_one(&inhdr_sg, input, sizeof(*input));
sgs[num_out + num_in++] = &inhdr_sg;
+ spin_lock(&vcrypto->ctrl_lock);
err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in,
vcrypto, GFP_ATOMIC);
- if (err < 0)
+ if (err < 0) {
+ spin_unlock(&vcrypto->ctrl_lock);
goto out;
+ }
virtqueue_kick(vcrypto->ctrl_vq);
while (!virtqueue_get_buf(vcrypto->ctrl_vq, &inlen) &&
!virtqueue_is_broken(vcrypto->ctrl_vq))
cpu_relax();
+ spin_unlock(&vcrypto->ctrl_lock);
if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
+ pr_err("virtio_crypto: Create session failed status: %u\n",
+ le32_to_cpu(input->status));
err = -EINVAL;
goto out;
}
@@ -148,13 +160,9 @@ static int virtio_crypto_alg_akcipher_init_session(struct
virtio_crypto_akcipher
err = 0;
out:
- spin_unlock(&vcrypto->ctrl_lock);
+ kfree(vc_ctrl_req);
kfree_sensitive(pkey);
- if (err < 0)
- pr_err("virtio_crypto: Create session failed status: %u\n",
- le32_to_cpu(input->status));
-
return err;
}
@@ -167,15 +175,18 @@ static int
virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
int err;
struct virtio_crypto_op_ctrl_req *ctrl;
struct virtio_crypto_inhdr *ctrl_status;
+ struct virtio_crypto_ctrl_request *vc_ctrl_req;
- spin_lock(&vcrypto->ctrl_lock);
- if (!ctx->session_valid) {
- err = 0;
- goto out;
- }
- ctrl_status = &vcrypto->ctrl_status;
+ if (!ctx->session_valid)
+ return 0;
+
+ vc_ctrl_req = kzalloc(sizeof(*vc_ctrl_req), GFP_KERNEL);
+ if (!vc_ctrl_req)
+ return -ENOMEM;
+
+ ctrl_status = &vc_ctrl_req->ctrl_status;
ctrl_status->status = VIRTIO_CRYPTO_ERR;
- ctrl = &vcrypto->ctrl;
+ ctrl = &vc_ctrl_req->ctrl;
ctrl->header.opcode =
cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
ctrl->header.queue_id = 0;
@@ -188,16 +199,22 @@ static int
virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
sg_init_one(&inhdr_sg, &ctrl_status->status,
sizeof(ctrl_status->status));
sgs[num_out + num_in++
[PATCH v5 1/5] virtio-crypto: change code style
Use temporary variable to make code easy to read and maintain.
/* Pad cipher's parameters */
vcrypto->ctrl.u.sym_create_session.op_type =
cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
vcrypto->ctrl.u.sym_create_session.u.cipher.para.algo =
vcrypto->ctrl.header.algo;
vcrypto->ctrl.u.sym_create_session.u.cipher.para.keylen =
cpu_to_le32(keylen);
vcrypto->ctrl.u.sym_create_session.u.cipher.para.op =
cpu_to_le32(op);
-->
sym_create_session = &ctrl->u.sym_create_session;
sym_create_session->op_type = cpu_to_le32(VIRTIO_CRYPTO_SYM_OP_CIPHER);
sym_create_session->u.cipher.para.algo = ctrl->header.algo;
sym_create_session->u.cipher.para.keylen = cpu_to_le32(keylen);
sym_create_session->u.cipher.para.op = cpu_to_le32(op);
The new style shows more obviously:
- the variable we want to operate.
- an assignment statement in a single line.
Cc: Michael S. Tsirkin
Cc: Jason Wang
Cc: Gonglei
Signed-off-by: zhenwei pi
---
.../virtio/virtio_crypto_akcipher_algs.c | 40 ++-
.../virtio/virtio_crypto_skcipher_algs.c | 72 +--
2 files changed, 59 insertions(+), 53 deletions(-)
diff --git a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
index f3ec9420215e..20901a263fc8 100644
--- a/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_akcipher_algs.c
@@ -106,23 +106,27 @@ static int virtio_crypto_alg_akcipher_init_session(struct
virtio_crypto_akcipher
unsigned int inlen;
int err;
unsigned int num_out = 0, num_in = 0;
+ struct virtio_crypto_op_ctrl_req *ctrl;
+ struct virtio_crypto_session_input *input;
pkey = kmemdup(key, keylen, GFP_ATOMIC);
if (!pkey)
return -ENOMEM;
spin_lock(&vcrypto->ctrl_lock);
- memcpy(&vcrypto->ctrl.header, header, sizeof(vcrypto->ctrl.header));
- memcpy(&vcrypto->ctrl.u, para, sizeof(vcrypto->ctrl.u));
- vcrypto->input.status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
+ ctrl = &vcrypto->ctrl;
+ memcpy(&ctrl->header, header, sizeof(ctrl->header));
+ memcpy(&ctrl->u, para, sizeof(ctrl->u));
+ input = &vcrypto->input;
+ input->status = cpu_to_le32(VIRTIO_CRYPTO_ERR);
- sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
+ sg_init_one(&outhdr_sg, ctrl, sizeof(*ctrl));
sgs[num_out++] = &outhdr_sg;
sg_init_one(&key_sg, pkey, keylen);
sgs[num_out++] = &key_sg;
- sg_init_one(&inhdr_sg, &vcrypto->input, sizeof(vcrypto->input));
+ sg_init_one(&inhdr_sg, input, sizeof(*input));
sgs[num_out + num_in++] = &inhdr_sg;
err = virtqueue_add_sgs(vcrypto->ctrl_vq, sgs, num_out, num_in,
vcrypto, GFP_ATOMIC);
@@ -134,12 +138,12 @@ static int virtio_crypto_alg_akcipher_init_session(struct
virtio_crypto_akcipher
!virtqueue_is_broken(vcrypto->ctrl_vq))
cpu_relax();
- if (le32_to_cpu(vcrypto->input.status) != VIRTIO_CRYPTO_OK) {
+ if (le32_to_cpu(input->status) != VIRTIO_CRYPTO_OK) {
err = -EINVAL;
goto out;
}
- ctx->session_id = le64_to_cpu(vcrypto->input.session_id);
+ ctx->session_id = le64_to_cpu(input->session_id);
ctx->session_valid = true;
err = 0;
@@ -149,7 +153,7 @@ static int virtio_crypto_alg_akcipher_init_session(struct
virtio_crypto_akcipher
if (err < 0)
pr_err("virtio_crypto: Create session failed status: %u\n",
- le32_to_cpu(vcrypto->input.status));
+ le32_to_cpu(input->status));
return err;
}
@@ -161,23 +165,27 @@ static int
virtio_crypto_alg_akcipher_close_session(struct virtio_crypto_akciphe
struct virtio_crypto *vcrypto = ctx->vcrypto;
unsigned int num_out = 0, num_in = 0, inlen;
int err;
+ struct virtio_crypto_op_ctrl_req *ctrl;
+ struct virtio_crypto_inhdr *ctrl_status;
spin_lock(&vcrypto->ctrl_lock);
if (!ctx->session_valid) {
err = 0;
goto out;
}
- vcrypto->ctrl_status.status = VIRTIO_CRYPTO_ERR;
- vcrypto->ctrl.header.opcode =
cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
- vcrypto->ctrl.header.queue_id = 0;
+ ctrl_status = &vcrypto->ctrl_status;
+ ctrl_status->status = VIRTIO_CRYPTO_ERR;
+ ctrl = &vcrypto->ctrl;
+ ctrl->header.opcode =
cpu_to_le32(VIRTIO_CRYPTO_AKCIPHER_DESTROY_SESSION);
+ ctrl->header.queue_id = 0;
- destroy_session = &vcrypto->ctrl.u.destroy_session;
+ destroy_session = &ctrl->u.destroy_session;
destroy_session->session_id = cpu_to_le64(ctx->session_id);
- sg_init_one(&outhdr_sg, &vcrypto->ctrl, sizeof(vcrypto->ctrl));
+ sg_init
[PATCH v5 0/5] virtio-crypto: Improve performance
v4 -> v5: - Fix potentially dereferencing uninitialized variables in 'virtio-crypto: use private buffer for control request'. Thanks to Dan Carpenter! v3 -> v4: - Don't create new file virtio_common.c, the new functions are added into virtio_crypto_core.c - Split the first patch into two parts: 1, change code style, 2, use private buffer instead of shared buffer - Remove relevant change. - Other minor changes. v2 -> v3: - Jason suggested that spliting the first patch into two part: 1, using private buffer 2, remove the busy polling Rework as Jason's suggestion, this makes the smaller change in each one and clear. v1 -> v2: - Use kfree instead of kfree_sensitive for insensitive buffer. - Several coding style fix. - Use memory from current node, instead of memory close to device - Add more message in commit, also explain why removing per-device request buffer. - Add necessary comment in code to explain why using kzalloc to allocate struct virtio_crypto_ctrl_request. v1: The main point of this series is to improve the performance for virtio crypto: - Use wait mechanism instead of busy polling for ctrl queue, this reduces CPU and lock racing, it's possiable to create/destroy session parallelly, QPS increases from ~40K/s to ~200K/s. - Enable retry on crypto engine to improve performance for data queue, this allows the larger depth instead of 1. - Fix dst data length in akcipher service. - Other style fix. lei he (2): virtio-crypto: adjust dst_len at ops callback virtio-crypto: enable retry for virtio-crypto-dev zhenwei pi (3): virtio-crypto: change code style virtio-crypto: use private buffer for control request virtio-crypto: wait ctrl queue instead of busy polling .../virtio/virtio_crypto_akcipher_algs.c | 95 ++-- drivers/crypto/virtio/virtio_crypto_common.h | 21 ++- drivers/crypto/virtio/virtio_crypto_core.c| 55 ++- .../virtio/virtio_crypto_skcipher_algs.c | 140 -- 4 files changed, 182 insertions(+), 129 deletions(-) -- 2.20.1 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH
On Thu, May 05, 2022 at 04:26:24PM +0800, Jason Wang wrote:
On Fri, Apr 29, 2022 at 3:14 PM Stefano Garzarella wrote:
On Fri, Apr 29, 2022 at 10:46:40AM +0800, Jason Wang wrote:
>On Thu, Apr 28, 2022 at 11:13 PM Stefano Garzarella
wrote:
>>
>> The simulator behaves like a ramdisk, so we don't have to do
>> anything when a VIRTIO_BLK_T_FLUSH request is received, but it
>> could be useful to test driver behavior.
>>
>> Let's expose the VIRTIO_BLK_F_FLUSH feature to inform the driver
>> that we support the flush command.
>>
>> Signed-off-by: Stefano Garzarella
>> ---
>> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 12
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> index 42d401d43911..a6dd1233797c 100644
>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> @@ -25,6 +25,7 @@
>> #define DRV_LICENSE "GPL v2"
>>
>> #define VDPASIM_BLK_FEATURES (VDPASIM_FEATURES | \
>> +(1ULL << VIRTIO_BLK_F_FLUSH)| \
>> (1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
>> (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
>> (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
>> @@ -166,6 +167,17 @@ static bool vdpasim_blk_handle_req(struct vdpasim
*vdpasim,
>> pushed += bytes;
>> break;
>>
>> + case VIRTIO_BLK_T_FLUSH:
>> + if (sector != 0) {
>> + dev_err(&vdpasim->vdpa.dev,
>> + "A driver MUST set sector to 0 for a
VIRTIO_BLK_T_FLUSH request - sector: 0x%llx\n",
>> + sector);
>
>If this is something that could be triggered by userspace/guest, then
>we should avoid this.
It can only be triggered by an erratic driver.
Right, so guest can try to DOS the host via this.
Yes, but I don't expect the simulator to be used in the real world, but
only for testing and development, so the user should have full control
of the guest.
I was using the simulator to test a virtio-blk driver that I'm writing
in userspace and I forgot to set `sector` to zero, so I thought it would
be useful.
Do you mean to remove the error message?
Some like dev_warn_once() might be better here.
We also have other checks we do for each request (in and out header
length, etc.) where we use dev_err(), should we change those too?
I don't know, from a developer's point of view I'd prefer to have them
all printed, but actually if we have a totally wrong driver in the
guest, we risk to hang our host to print an infinite number of messages.
Maybe we should change all the errors in the data path to
dev_warn_once() and keep returning VIRTIO_BLK_S_IOERR to the guest which
will surely get angry and print something.
If you agree, I'll send a patch to change all the printing and then
repost this with your suggestion as well.
Thanks,
Stefano
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 11/15] drm/shmem-helper: Add generic memory shrinker
Hi
Am 18.04.22 um 00:37 schrieb Dmitry Osipenko:
Introduce a common DRM SHMEM shrinker. It allows to reduce code
duplication among DRM drivers that implement theirs own shrinkers.
This is initial version of the shrinker that covers basic needs of
GPU drivers, both purging and eviction of shmem objects are supported.
This patch is based on a couple ideas borrowed from Rob's Clark MSM
shrinker and Thomas' Zimmermann variant of SHMEM shrinker.
In order to start using DRM SHMEM shrinker drivers should:
1. Implement new purge(), evict() + swap_in() GEM callbacks.
2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device).
3. Use drm_gem_shmem_set_purgeable_and_evictable(shmem) and alike API
functions to activate shrinking of GEMs.
Honestly speaking, after reading the patch and the discussion here I
really don't like where all tis is going. The interfaces and
implementation are overengineered. Descisions about evicting and
purging should be done by the memory manager. For the most part, it's
none of the driver's business.
I'd like to ask you to reduce the scope of the patchset and build the
shrinker only for virtio-gpu. I know that I first suggested to build
upon shmem helpers, but it seems that it's easier to do that in a later
patchset.
Best regards
Thomas
Signed-off-by: Daniel Almeida
Signed-off-by: Dmitry Osipenko
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 765 -
include/drm/drm_device.h | 4 +
include/drm/drm_gem.h | 35 ++
include/drm/drm_gem_shmem_helper.h | 105 +++-
4 files changed, 877 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 3ecef571eff3..3838fb8d6f3a 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -88,6 +88,13 @@ __drm_gem_shmem_create(struct drm_device *dev, size_t size,
bool private)
INIT_LIST_HEAD(&shmem->madv_list);
+ /*
+* Eviction and purging are disabled by default, shmem user must enable
+* them explicitly using drm_gem_shmem_set_evictable/purgeable().
+*/
+ shmem->eviction_disable_count = 1;
+ shmem->purging_disable_count = 1;
+
if (!private) {
/*
* Our buffers are kept pinned, so allocating them
@@ -126,6 +133,107 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct
drm_device *dev, size_t
}
EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
+static void
+drm_gem_shmem_add_pages_to_shrinker(struct drm_gem_shmem_object *shmem)
+{
+ struct drm_gem_object *obj = &shmem->base;
+ struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
+ size_t page_count = obj->size >> PAGE_SHIFT;
+
+ if (!shmem->pages_shrinkable) {
+ WARN_ON(gem_shrinker->shrinkable_count + page_count <
page_count);
+ gem_shrinker->shrinkable_count += page_count;
+ shmem->pages_shrinkable = true;
+ }
+}
+
+static void
+drm_gem_shmem_remove_pages_from_shrinker(struct drm_gem_shmem_object *shmem)
+{
+ struct drm_gem_object *obj = &shmem->base;
+ struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
+ size_t page_count = obj->size >> PAGE_SHIFT;
+
+ if (shmem->pages_shrinkable) {
+ WARN_ON(gem_shrinker->shrinkable_count < page_count);
+ gem_shrinker->shrinkable_count -= page_count;
+ shmem->pages_shrinkable = false;
+ }
+}
+
+static void
+drm_gem_shmem_set_pages_state_locked(struct drm_gem_shmem_object *shmem,
+enum drm_gem_shmem_pages_state new_state)
+{
+ struct drm_gem_object *obj = &shmem->base;
+ struct drm_gem_shmem_shrinker *gem_shrinker = obj->dev->shmem_shrinker;
+
+ lockdep_assert_held(&gem_shrinker->lock);
+ lockdep_assert_held(&obj->resv->lock.base);
+
+ if (new_state >= DRM_GEM_SHMEM_PAGES_STATE_PINNED) {
+ if (drm_gem_shmem_is_evictable(shmem))
+ new_state = DRM_GEM_SHMEM_PAGES_STATE_EVICTABLE;
+
+ if (drm_gem_shmem_is_purgeable(shmem))
+ new_state = DRM_GEM_SHMEM_PAGES_STATE_PURGEABLE;
+
+ if (!shmem->pages)
+ new_state = DRM_GEM_SHMEM_PAGES_STATE_UNPINNED;
+
+ if (shmem->evicted)
+ new_state = DRM_GEM_SHMEM_PAGES_STATE_EVICTED;
+ }
+
+ if (shmem->pages_state == new_state)
+ return;
+
+ switch (new_state) {
+ case DRM_GEM_SHMEM_PAGES_STATE_UNPINNED:
+ case DRM_GEM_SHMEM_PAGES_STATE_PURGED:
+ drm_gem_shmem_remove_pages_from_shrinker(shmem);
+ list_del_init(&shmem->madv_list);
+ break;
+
+ case DRM_GEM_SHMEM_PAGES_STATE_PINNED:
+ drm_gem_shmem_remove_pages_from_shrinker(shmem);
+
Re: [PATCH] vdpa_sim_blk: add support for VIRTIO_BLK_T_FLUSH
On Fri, Apr 29, 2022 at 3:14 PM Stefano Garzarella wrote:
>
> On Fri, Apr 29, 2022 at 10:46:40AM +0800, Jason Wang wrote:
> >On Thu, Apr 28, 2022 at 11:13 PM Stefano Garzarella
> >wrote:
> >>
> >> The simulator behaves like a ramdisk, so we don't have to do
> >> anything when a VIRTIO_BLK_T_FLUSH request is received, but it
> >> could be useful to test driver behavior.
> >>
> >> Let's expose the VIRTIO_BLK_F_FLUSH feature to inform the driver
> >> that we support the flush command.
> >>
> >> Signed-off-by: Stefano Garzarella
> >> ---
> >> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 12
> >> 1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> index 42d401d43911..a6dd1233797c 100644
> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> >> @@ -25,6 +25,7 @@
> >> #define DRV_LICENSE "GPL v2"
> >>
> >> #define VDPASIM_BLK_FEATURES (VDPASIM_FEATURES | \
> >> +(1ULL << VIRTIO_BLK_F_FLUSH)| \
> >> (1ULL << VIRTIO_BLK_F_SIZE_MAX) | \
> >> (1ULL << VIRTIO_BLK_F_SEG_MAX) | \
> >> (1ULL << VIRTIO_BLK_F_BLK_SIZE) | \
> >> @@ -166,6 +167,17 @@ static bool vdpasim_blk_handle_req(struct vdpasim
> >> *vdpasim,
> >> pushed += bytes;
> >> break;
> >>
> >> + case VIRTIO_BLK_T_FLUSH:
> >> + if (sector != 0) {
> >> + dev_err(&vdpasim->vdpa.dev,
> >> + "A driver MUST set sector to 0 for a
> >> VIRTIO_BLK_T_FLUSH request - sector: 0x%llx\n",
> >> + sector);
> >
> >If this is something that could be triggered by userspace/guest, then
> >we should avoid this.
>
> It can only be triggered by an erratic driver.
Right, so guest can try to DOS the host via this.
>
> I was using the simulator to test a virtio-blk driver that I'm writing
> in userspace and I forgot to set `sector` to zero, so I thought it would
> be useful.
>
> Do you mean to remove the error message?
Some like dev_warn_once() might be better here.
Thanks
>
> Thanks,
> Stefano
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] vringh: Fix maximum number check for indirect descriptors
On Thu, May 5, 2022 at 4:06 PM Yongji Xie wrote:
>
> On Thu, May 5, 2022 at 3:47 PM Jason Wang wrote:
> >
> > On Wed, May 4, 2022 at 4:12 PM Xie Yongji wrote:
> > >
> > > We should use size of descriptor chain to check the maximum
> > > number of consumed descriptors in indirect case.
> >
> > AFAIK, it's a guard for loop descriptors.
> >
>
> Yes, but for indirect descriptors, we know the size of the descriptor
> chain. Should we use it to test loop condition rather than using
> virtqueue size?
Yes.
>
> > > And the
> > > statistical counts should also be reset to zero each time
> > > we get an indirect descriptor.
> >
> > What might happen if we don't have this patch?
> >
>
> Then we can't handle the case that one request includes multiple
> indirect descriptors. Although I never see this kind of case now, the
> spec doesn't forbid it.
It looks to me we need to introduce dedicated counters for indirect
descriptors instead of trying to use a single counter?
(All evils came from the move_to_indirect()/return_from_indierct()
logic, vhost have dedicated helper to deal with indirect descriptors -
get_indirect()).
Thanks
>
> > >
> > > Fixes: f87d0fbb5798 ("vringh: host-side implementation of virtio rings.")
> > > Signed-off-by: Xie Yongji
> > > Signed-off-by: Fam Zheng
> > > ---
> > > drivers/vhost/vringh.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > > index 14e2043d7685..c1810b77a05e 100644
> > > --- a/drivers/vhost/vringh.c
> > > +++ b/drivers/vhost/vringh.c
> > > @@ -344,12 +344,13 @@ __vringh_iov(struct vringh *vrh, u16 i,
> > > addr = (void *)(long)(a + range.offset);
> > > err = move_to_indirect(vrh, &up_next, &i, addr,
> > > &desc,
> > >&descs, &desc_max);
> > > + count = 0;
> >
> > Then it looks to me we can detect a loop indirect descriptor chain?
> >
>
> I think so.
>
> Thanks,
> Yongji
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH V3 3/9] virtio: introduce config op to synchronize vring callbacks
在 2022/4/29 15:25, Cornelia Huck 写道: On Fri, Apr 29 2022, Jason Wang wrote: On Thu, Apr 28, 2022 at 5:13 PM Cornelia Huck wrote: On Mon, Apr 25 2022, Jason Wang wrote: This patch introduces new virtio config op to vring callbacks. Transport specific method is required to make sure the write before this function is visible to the vring_interrupt() that is Which kind of writes? I.e., what is the scope? Any writes before synchronize_cbs(). Is something like the following better? The function guarantees that all memory operations before it are visible to the vring_interrupt() that is called after it. Maybe "all memory operations on the queue"? That's fine. Will do. Thanks ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 3/3] vdpa/mlx5: Add RX MAC VLAN filter support
在 2022/5/2 13:38, Eli Cohen 写道:
+static virtio_net_ctrl_ack handle_ctrl_vlan(struct mlx5_vdpa_dev *mvdev, u8
cmd)
+{
+ struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
+ virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
+ struct mlx5_control_vq *cvq = &mvdev->cvq;
+ struct virtio_net_ctrl_vlan vlan;
+ size_t read;
+ u16 id;
+
+ switch (cmd) {
+ case VIRTIO_NET_CTRL_VLAN_ADD:
+ read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->riov, (void
*)&vlan, sizeof(vlan));
+ if (read != sizeof(vlan))
+ break;
+
+ id = mlx5vdpa16_to_cpu(mvdev, vlan.id);
+ if (mac_vlan_add(ndev, ndev->config.mac, id, true))
+ break;
This may work now but I wonder if we had the plan to support
VIRTIO_NET_F_CTRL_RX?
if $mac is not in $mac_table
drop;
if $vlan is not in $vlan_table
drop;
With that features we probably requires the dedicated vlan filters
without a mac? Or do we want to a $mac * $vlans rules?
If yes, maybe it's better to decouple vlan filters from mac now.
If we use dedicated filter tables for VLAN and MAC working in series,
we may not have full control over incoming traffic filtering.
For example, suppose we have VLAN table allowing v1 and v2 to go through,
and a MAC table allowing m1 and m2 to go through.
If we want only (v1, m1) and (v2, m2) to go through but not (v1, m2) or (v2, m1)
then it would not be possible to block the latter.
Yes, but this is currently how virtio-net work in the spec.
As I can see, the spec does not require that finesse
Yes.
but I wonder if this not
what real life requires.
Then we need to extend the spec.
If you think we should follow the spec let me know and will prepare a new
version.
It should be fine now. (But if we want too support CTRL_RX we need some
refactoring on the codes).
So:
Acked-by: Jason Wang
Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4 10/15] drm/shmem-helper: Take reservation lock instead of drm_gem_shmem locks
On Wed, May 04, 2022 at 06:56:09PM +0300, Dmitry Osipenko wrote: > On 5/4/22 11:21, Daniel Vetter wrote: > ... > >>> - Maybe also do what you suggest and keep a separate lock for this, but > >>> the fundamental issue is that this doesn't really work - if you share > >>> buffers both ways with two drivers using shmem helpers, then the > >>> ordering of this vmap_count_mutex vs dma_resv_lock is inconsistent and > >>> you can get some nice deadlocks. So not a great approach (and also the > >>> reason why we really need to get everyone to move towards dma_resv_lock > >>> as _the_ buffer object lock, since otherwise we'll never get a > >>> consistent lock nesting hierarchy). > >> > >> The separate locks should work okay because it will be always the > >> exporter that takes the dma_resv_lock. But I agree that it's less ideal > >> than defining the new rules for dma-bufs since sometime you will take > >> the resv lock and sometime not, potentially hiding bugs related to > >> lockings. > > > > That's the issue, some importers need to take the dma_resv_lock for > > dma_buf_vmap too (e.g. to first nail the buffer in place when it's a > > dynamic memory manager). In practice it'll work as well as what we have > > currently, which is similarly inconsistent, except with per-driver locks > > instead of shared locks from shmem helpers or dma-buf, so less obvious > > that things are inconsistent. > > > > So yeah if it's too messy maybe the approach is to have a separate lock > > for vmap for now, land things, and then fix up dma_buf_vmap in a follow up > > series. > > The amdgpu driver was the fist who introduced the concept of movable > memory for dma-bufs. Now we want to support it for DRM SHMEM too. For > both amdgpu ttm and shmem drivers we will want to hold the reservation > lock when we're touching moveable buffers. The current way of denoting > that dma-buf is movable is to implement the pin/unpin callbacks of the > dma-buf ops, should be doable for shmem. Hm that sounds like a bridge too far? I don't think we want to start adding moveable dma-bufs for shmem, thus far at least no one asked for that. Goal here is just to streamline the locking a bit and align across all the different ways of doing buffers in drm. Or do you mean something else and I'm just completely lost? > A day ago I found that mapping of imported dma-bufs is broken at least > for the Tegra DRM driver (and likely for others too) because driver > doesn't assume that anyone will try to mmap imported buffer and just > doesn't handle this case at all, so we're getting a hard lockup on > touching mapped memory because we're mapping something else than the > dma-buf. Huh that sounds bad, how does this happen? Pretty much all pieces of dma-buf (cpu vmap, userspace mmap, heck even dma_buf_attach) are optional or at least can fail for various reasons. So exporters not providing mmap support is fine, but importers then dying is not. > My plan is to move the dma-buf management code to the level of DRM core > and make it aware of the reservation locks for the dynamic dma-bufs. > This way we will get the proper locking for dma-bufs and fix mapping of > imported dma-bufs for Tegra and other drivers. So maybe we're completely talking past each another, or coffee is not working here on my end, but I've no idea what you mean. We do have some helpers for taking care of the dma_resv_lock dance, and Christian König has an rfc patch set to maybe unify this further. But that should be fairly orthogonal to reworking shmem (it might help a bit with reworking shmem though). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
在 2022/5/4 13:44, Eli Cohen 写道:
-Original Message-
From: Si-Wei Liu
Sent: Wednesday, May 4, 2022 7:44 AM
To: Eli Cohen ; m...@redhat.com; jasow...@redhat.com
Cc: virtualization@lists.linux-foundation.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
On 5/2/2022 10:13 PM, Eli Cohen wrote:
-Original Message-
From: Si-Wei Liu
Sent: Tuesday, May 3, 2022 2:48 AM
To: Eli Cohen ; m...@redhat.com; jasow...@redhat.com
Cc: virtualization@lists.linux-foundation.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH v3 1/2] vdpa: Add support for querying vendor statistics
On 5/2/2022 3:22 AM, Eli Cohen wrote:
Allows to read vendor statistics of a vdpa device. The specific
statistics data are received from the upstream driver in the form of an
(attribute name, attribute value) pairs.
An example of statistics for mlx5_vdpa device are:
received_desc - number of descriptors received by the virtqueue
completed_desc - number of descriptors completed by the virtqueue
A descriptor using indirect buffers is still counted as 1. In addition,
N chained descriptors are counted correctly N times as one would expect.
A new callback was added to vdpa_config_ops which provides the means for
the vdpa driver to return statistics results.
The interface allows for reading all the supported virtqueues, including
the control virtqueue if it exists.
Below are some examples taken from mlx5_vdpa which are introduced in the
following patch:
1. Read statistics for the virtqueue at index 1
$ vdpa dev vstats show vdpa-a qidx 1
vdpa-a:
queue_type tx queue_index 1 received_desc 3844836 completed_desc 3844836
2. Read statistics for the virtqueue at index 32
$ vdpa dev vstats show vdpa-a qidx 32
vdpa-a:
queue_type control_vq queue_index 32 received_desc 62 completed_desc 62
3. Read statisitics for the virtqueue at index 0 with json output
$ vdpa -j dev vstats show vdpa-a qidx 0
{"vstats":{"vdpa-a":{
"queue_type":"rx","queue_index":0,"name":"received_desc","value":417776,\
"name":"completed_desc","value":417548}}}
4. Read statistics for the virtqueue at index 0 with preety json output
$ vdpa -jp dev vstats show vdpa-a qidx 0
{
"vstats": {
"vdpa-a": {
"queue_type": "rx",
"queue_index": 0,
"name": "received_desc",
"value": 417776,
"name": "completed_desc",
"value": 417548
}
}
}
Signed-off-by: Eli Cohen
---
drivers/vdpa/vdpa.c | 129 ++
include/linux/vdpa.h | 5 ++
include/uapi/linux/vdpa.h | 6 ++
3 files changed, 140 insertions(+)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 2b75c00b1005..933466f61ca8 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -909,6 +909,74 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct
sk_buff *msg, u32 portid,
return err;
}
+static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
+ struct genl_info *info, u32 index)
+{
+ int err;
+
+ err = vdev->config->get_vendor_vq_stats(vdev, index, msg, info->extack);
+ if (err)
+ return err;
+
+ if (nla_put_u32(msg, VDPA_ATTR_DEV_QUEUE_INDEX, index))
+ return -EMSGSIZE;
+
+ return 0;
+}
+
+static int vendor_stats_fill(struct vdpa_device *vdev, struct sk_buff *msg,
+struct genl_info *info, u32 index)
+{
+ int err;
+
+ if (!vdev->config->get_vendor_vq_stats)
+ return -EOPNOTSUPP;
+
+ err = vdpa_fill_stats_rec(vdev, msg, info, index);
+ if (err)
+ return err;
+
+ return 0;
+}
+
+static int vdpa_dev_vendor_stats_fill(struct vdpa_device *vdev,
+ struct sk_buff *msg,
+ struct genl_info *info, u32 index)
+{
+ u32 device_id;
+ void *hdr;
+ int err;
+ u32 portid = info->snd_portid;
+ u32 seq = info->snd_seq;
+ u32 flags = 0;
+
+ hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
+ VDPA_CMD_DEV_VSTATS_GET);
+ if (!hdr)
+ return -EMSGSIZE;
+
+ if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
+ err = -EMSGSIZE;
+ goto undo_msg;
+ }
+
+ device_id = vdev->config->get_device_id(vdev);
+ if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
+ err = -EMSGSIZE;
+ goto undo_msg;
+ }
+
You seem to miss VDPA_ATTR_DEV_NEGOTIATED_FEATURES from this function,
otherwise I can't image how you can ensure the atomicity to infer
queue_type for control vq.
And should we make sure VIRTIO_CONFIG_S_FEATURES_OK is set before call
into vendor_stats_fill()?
It is done in the hardware driver. In this case, in mlx5_vdpa.
Re: [PATCH] vringh: Fix maximum number check for indirect descriptors
On Thu, May 5, 2022 at 3:46 PM Jason Wang wrote:
>
> On Wed, May 4, 2022 at 4:12 PM Xie Yongji wrote:
> >
> > We should use size of descriptor chain to check the maximum
> > number of consumed descriptors in indirect case.
>
> AFAIK, it's a guard for loop descriptors.
>
> > And the
> > statistical counts should also be reset to zero each time
> > we get an indirect descriptor.
>
> What might happen if we don't have this patch?
>
> >
> > Fixes: f87d0fbb5798 ("vringh: host-side implementation of virtio rings.")
> > Signed-off-by: Xie Yongji
> > Signed-off-by: Fam Zheng
> > ---
> > drivers/vhost/vringh.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index 14e2043d7685..c1810b77a05e 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -344,12 +344,13 @@ __vringh_iov(struct vringh *vrh, u16 i,
> > addr = (void *)(long)(a + range.offset);
> > err = move_to_indirect(vrh, &up_next, &i, addr,
> > &desc,
> >&descs, &desc_max);
> > + count = 0;
>
> Then it looks to me we can detect a loop indirect descriptor chain?
I meant "can't" actually.
Thanks
>
> Thanks
>
> > if (err)
> > goto fail;
> > continue;
> > }
> >
> > - if (count++ == vrh->vring.num) {
> > + if (count++ == desc_max) {
> > vringh_bad("Descriptor loop in %p", descs);
> > err = -ELOOP;
> > goto fail;
> > @@ -410,6 +411,7 @@ __vringh_iov(struct vringh *vrh, u16 i,
> > if (unlikely(up_next > 0)) {
> > i = return_from_indirect(vrh, &up_next,
> > &descs, &desc_max);
> > + count = 0;
> > slow = false;
> > } else
> > break;
> > --
> > 2.20.1
> >
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v4] vdpa/vp_vdpa : add vdpa tool support in vp_vdpa
在 2022/4/29 17:10, Cindy Lu 写道:
this patch is to add the support for vdpa tool in vp_vdpa
here is the example steps
modprobe vp_vdpa
modprobe vhost_vdpa
echo :00:06.0>/sys/bus/pci/drivers/virtio-pci/unbind
echo 1af4 1041 > /sys/bus/pci/drivers/vp-vdpa/new_id
vdpa dev add name vdpa1 mgmtdev pci/:00:06.0
Signed-off-by: Cindy Lu
Acked-by: Jason Wang
---
drivers/vdpa/virtio_pci/vp_vdpa.c | 161 --
include/linux/vdpa.h | 2 +-
2 files changed, 130 insertions(+), 33 deletions(-)
diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c
b/drivers/vdpa/virtio_pci/vp_vdpa.c
index cce101e6a940..adb75d77eff2 100644
--- a/drivers/vdpa/virtio_pci/vp_vdpa.c
+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
@@ -32,7 +32,7 @@ struct vp_vring {
struct vp_vdpa {
struct vdpa_device vdpa;
- struct virtio_pci_modern_device mdev;
+ struct virtio_pci_modern_device *mdev;
struct vp_vring *vring;
struct vdpa_callback config_cb;
char msix_name[VP_VDPA_NAME_SIZE];
@@ -41,6 +41,12 @@ struct vp_vdpa {
int vectors;
};
+struct vp_vdpa_mgmtdev {
+ struct vdpa_mgmt_dev mgtdev;
+ struct virtio_pci_modern_device *mdev;
+ struct vp_vdpa *vp_vdpa;
+};
+
static struct vp_vdpa *vdpa_to_vp(struct vdpa_device *vdpa)
{
return container_of(vdpa, struct vp_vdpa, vdpa);
@@ -50,7 +56,12 @@ static struct virtio_pci_modern_device *vdpa_to_mdev(struct
vdpa_device *vdpa)
{
struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa);
- return &vp_vdpa->mdev;
+ return vp_vdpa->mdev;
+}
+
+static struct virtio_pci_modern_device *vp_vdpa_to_mdev(struct vp_vdpa
*vp_vdpa)
+{
+ return vp_vdpa->mdev;
}
static u64 vp_vdpa_get_device_features(struct vdpa_device *vdpa)
@@ -96,7 +107,7 @@ static int vp_vdpa_get_vq_irq(struct vdpa_device *vdpa, u16
idx)
static void vp_vdpa_free_irq(struct vp_vdpa *vp_vdpa)
{
- struct virtio_pci_modern_device *mdev = &vp_vdpa->mdev;
+ struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa);
struct pci_dev *pdev = mdev->pci_dev;
int i;
@@ -143,7 +154,7 @@ static irqreturn_t vp_vdpa_config_handler(int irq, void *arg)
static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
{
- struct virtio_pci_modern_device *mdev = &vp_vdpa->mdev;
+ struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa);
struct pci_dev *pdev = mdev->pci_dev;
int i, ret, irq;
int queues = vp_vdpa->queues;
@@ -198,7 +209,7 @@ static int vp_vdpa_request_irq(struct vp_vdpa *vp_vdpa)
static void vp_vdpa_set_status(struct vdpa_device *vdpa, u8 status)
{
struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa);
- struct virtio_pci_modern_device *mdev = &vp_vdpa->mdev;
+ struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa);
u8 s = vp_vdpa_get_status(vdpa);
if (status & VIRTIO_CONFIG_S_DRIVER_OK &&
@@ -212,7 +223,7 @@ static void vp_vdpa_set_status(struct vdpa_device *vdpa, u8
status)
static int vp_vdpa_reset(struct vdpa_device *vdpa)
{
struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa);
- struct virtio_pci_modern_device *mdev = &vp_vdpa->mdev;
+ struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa);
u8 s = vp_vdpa_get_status(vdpa);
vp_modern_set_status(mdev, 0);
@@ -372,7 +383,7 @@ static void vp_vdpa_get_config(struct vdpa_device *vdpa,
void *buf, unsigned int len)
{
struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa);
- struct virtio_pci_modern_device *mdev = &vp_vdpa->mdev;
+ struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa);
u8 old, new;
u8 *p;
int i;
@@ -392,7 +403,7 @@ static void vp_vdpa_set_config(struct vdpa_device *vdpa,
unsigned int len)
{
struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa);
- struct virtio_pci_modern_device *mdev = &vp_vdpa->mdev;
+ struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa);
const u8 *p = buf;
int i;
@@ -412,7 +423,7 @@ static struct vdpa_notification_area
vp_vdpa_get_vq_notification(struct vdpa_device *vdpa, u16 qid)
{
struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa);
- struct virtio_pci_modern_device *mdev = &vp_vdpa->mdev;
+ struct virtio_pci_modern_device *mdev = vp_vdpa_to_mdev(vp_vdpa);
struct vdpa_notification_area notify;
notify.addr = vp_vdpa->vring[qid].notify_pa;
@@ -454,38 +465,31 @@ static void vp_vdpa_free_irq_vectors(void *data)
pci_free_irq_vectors(data);
}
-static int vp_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+static int vp_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
+ const struct vdpa_dev_set_config *add_config)
{
- struct virtio_pci_modern_device *mdev;
+ struct vp_vdpa_mgmtdev *vp_vdpa_mgtdev =
+
Re: [PATCH] vringh: Fix maximum number check for indirect descriptors
On Wed, May 4, 2022 at 4:12 PM Xie Yongji wrote:
>
> We should use size of descriptor chain to check the maximum
> number of consumed descriptors in indirect case.
AFAIK, it's a guard for loop descriptors.
> And the
> statistical counts should also be reset to zero each time
> we get an indirect descriptor.
What might happen if we don't have this patch?
>
> Fixes: f87d0fbb5798 ("vringh: host-side implementation of virtio rings.")
> Signed-off-by: Xie Yongji
> Signed-off-by: Fam Zheng
> ---
> drivers/vhost/vringh.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> index 14e2043d7685..c1810b77a05e 100644
> --- a/drivers/vhost/vringh.c
> +++ b/drivers/vhost/vringh.c
> @@ -344,12 +344,13 @@ __vringh_iov(struct vringh *vrh, u16 i,
> addr = (void *)(long)(a + range.offset);
> err = move_to_indirect(vrh, &up_next, &i, addr, &desc,
>&descs, &desc_max);
> + count = 0;
Then it looks to me we can detect a loop indirect descriptor chain?
Thanks
> if (err)
> goto fail;
> continue;
> }
>
> - if (count++ == vrh->vring.num) {
> + if (count++ == desc_max) {
> vringh_bad("Descriptor loop in %p", descs);
> err = -ELOOP;
> goto fail;
> @@ -410,6 +411,7 @@ __vringh_iov(struct vringh *vrh, u16 i,
> if (unlikely(up_next > 0)) {
> i = return_from_indirect(vrh, &up_next,
> &descs, &desc_max);
> + count = 0;
> slow = false;
> } else
> break;
> --
> 2.20.1
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
