| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <59b38f07-73c2-07c2-ef2d-660445f593e1@bytedance.com>
Date: Fri, 12 May 2023 19:53:50 +0800
From: zhenwei pi <pizhenwei@...edance.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: stefanha@...hat.com, jasowang@...hat.com,
xuanzhuo@...ux.alibaba.com,
virtualization@...ts.linux-foundation.org,
linux-kernel@...r.kernel.org
Subject: Re: Re: Re: [PATCH 1/2] virtio: abstract virtqueue related methods
On 5/12/23 19:35, Michael S. Tsirkin wrote:
> On Fri, May 12, 2023 at 07:09:40PM +0800, zhenwei pi wrote:
>> On 5/12/23 18:46, Michael S. Tsirkin wrote:
>>> On Fri, May 12, 2023 at 05:46:17PM +0800, zhenwei pi wrote:
>>>> There is already a virtqueue abstract structure in virtio subsystem
>>>> (see struct virtqueue in include/linux/virtio.h), however the vring
>>>> based virtqueue is used only in the past years, the virtqueue related
>>>> methods mix much with vring(just look like the codes, virtqueue_xxx
>>>> functions are implemented in virtio_ring.c).
>>>>
>>>> Abstract virtqueue related methods(see struct virtqueue_ops), and
>>>> separate virtqueue_xxx symbols from vring. This allows a non-vring
>>>> based transport in the future. With this change, the following symbols
>>>> are exported from virtio.ko instead of virtio_ring.ko:
>>>> virtqueue_add_sgs
>>>> virtqueue_add_outbuf
>>>> virtqueue_add_inbuf
>>>> virtqueue_add_inbuf_ctx
>>>> virtqueue_kick_prepare
>>>> virtqueue_notify
>>>> virtqueue_kick
>>>> virtqueue_enable_cb_prepare
>>>> virtqueue_enable_cb
>>>> virtqueue_enable_cb_delayed
>>>> virtqueue_disable_cb
>>>> virtqueue_poll
>>>> virtqueue_get_buf_ctx
>>>> virtqueue_get_buf
>>>> virtqueue_detach_unused_buf
>>>> virtqueue_get_vring_size
>>>> virtqueue_resize
>>>> virtqueue_is_broken
>>>> virtio_break_device
>>>> __virtio_unbreak_device
>>>>
>>>> Cc: Stefan Hajnoczi <stefanha@...hat.com>
>>>> Signed-off-by: zhenwei pi <pizhenwei@...edance.com>
>>>> ---
>>>> drivers/virtio/virtio.c | 362 +++++++++++++++++++++++++++++++++++
>>>> drivers/virtio/virtio_ring.c | 282 +++++----------------------
>>>> include/linux/virtio.h | 29 +++
>>>> 3 files changed, 443 insertions(+), 230 deletions(-)
>>>>
>>>> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>>>> index 3893dc29eb26..8d8715a10f26 100644
>>>> --- a/drivers/virtio/virtio.c
>>>> +++ b/drivers/virtio/virtio.c
>>>> @@ -553,6 +553,368 @@ int virtio_device_restore(struct virtio_device *dev)
>>>> EXPORT_SYMBOL_GPL(virtio_device_restore);
>>>> #endif
>>>> +/**
>>>> + * virtqueue_add_sgs - expose buffers to other end
>>>> + * @vq: the struct virtqueue we're talking about.
>>>> + * @sgs: array of terminated scatterlists.
>>>> + * @out_sgs: the number of scatterlists readable by other side
>>>> + * @in_sgs: the number of scatterlists which are writable (after readable ones)
>>>> + * @data: the token identifying the buffer.
>>>> + * @gfp: how to do memory allocations (if necessary).
>>>> + *
>>>> + * Caller must ensure we don't call this with other virtqueue operations
>>>> + * at the same time (except where noted).
>>>> + *
>>>> + * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
>>>> + */
>>>> +int virtqueue_add_sgs(struct virtqueue *vq, struct scatterlist *sgs[],
>>>> + unsigned int out_sgs, unsigned int in_sgs,
>>>> + void *data, gfp_t gfp)
>>>> +{
>>>> + unsigned int i, total_sg = 0;
>>>> +
>>>> + /* Count them first. */
>>>> + for (i = 0; i < out_sgs + in_sgs; i++) {
>>>> + struct scatterlist *sg;
>>>> +
>>>> + for (sg = sgs[i]; sg; sg = sg_next(sg))
>>>> + total_sg++;
>>>> + }
>>>> + return vq->ops->add_sgs(vq, sgs, total_sg, out_sgs, in_sgs,
>>>> + data, NULL, gfp);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(virtqueue_add_sgs);
>>>
>>>
>>> Hmm this kind of indirection on data path is going to be costly
>>> performance-wise especially when retpolines are in place.
>>>
>>> Any data on this?
>>>
>>
>> Hi,
>>
>> 1, What about moving these functions into virtio.h and declare them as
>> static inline?
>
> This will do nothing to remove indirection.
>
>> 2, what about moving method fields into struct virtqueue?
>
> This gets rid of one level of indirection but the big problem
> is indirect function call due to retpolines, this remains.
>
>
>> Then we have struct like:
>> struct virtqueue {
>> struct list_head list;
>> ...
>> void *priv;
>>
>> /* virtqueue specific operations */
>> int (*add_sgs)(struct virtqueue *vq, struct scatterlist *sgs[],
>> unsigned int total_sg,
>> unsigned int out_sgs, unsigned int in_sgs,
>> void *data, void *ctx, gfp_t gfp);
>> ...
>> }
>>
>> and functions like:
>> static inline int virtqueue_add_sgs(...)
>> {
>> unsigned int i, total_sg = 0;
>>
>> /* Count them first. */
>> for (i = 0; i < out_sgs + in_sgs; i++) {
>> struct scatterlist *sg;
>>
>> for (sg = sgs[i]; sg; sg = sg_next(sg))
>> total_sg++;
>> }
>> return vq->add_sgs(vq, sgs, total_sg, out_sgs, in_sgs,
>> data, NULL, gfp);
>> }
>
> Maybe a flag in vq:
> bool abstract; /* use ops to add/get bufs and kick */
> and then
> if (unlikely(vq->abstract))
> return vq->ops->add_sgs(vq, sgs, total_sg, out_sgs, in_sgs,
> data, NULL, gfp);
>
> transport then just sets the ops if it wants abstract vqs,
> and core then skips the vring.
>
>
>> If [1] is acceptable, we can also reduce changes in patch 'tools/virtio:
>> implement virtqueue in test'.
>
> Yea that one shouldn't be there.
>
>> --
>> zhenwei pi
>
OK, I'll try and send a next version a few days later. Thanks!
--
zhenwei pi
Powered by blists - more mailing lists