| 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: <cd774778-6cdc-9ebe-141c-1f47ad1c3109@redhat.com>
Date: Fri, 11 Mar 2022 13:01:23 +0800
From: Jason Wang <jasowang@...hat.com>
To: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Cc: Jeff Dike <jdike@...toit.com>, Richard Weinberger <richard@....at>,
Anton Ivanov <anton.ivanov@...bridgegreys.com>,
"Michael S. Tsirkin" <mst@...hat.com>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Hans de Goede <hdegoede@...hat.com>,
Mark Gross <markgross@...nel.org>,
Vadim Pasternak <vadimp@...dia.com>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Mathieu Poirier <mathieu.poirier@...aro.org>,
Cornelia Huck <cohuck@...hat.com>,
Halil Pasic <pasic@...ux.ibm.com>,
Heiko Carstens <hca@...ux.ibm.com>,
Vasily Gorbik <gor@...ux.ibm.com>,
Christian Borntraeger <borntraeger@...ux.ibm.com>,
Alexander Gordeev <agordeev@...ux.ibm.com>,
Sven Schnelle <svens@...ux.ibm.com>,
Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
Jesper Dangaard Brouer <hawk@...nel.org>,
John Fastabend <john.fastabend@...il.com>,
Johannes Berg <johannes.berg@...el.com>,
Vincent Whitchurch <vincent.whitchurch@...s.com>,
linux-um@...ts.infradead.org, platform-driver-x86@...r.kernel.org,
linux-remoteproc@...r.kernel.org, linux-s390@...r.kernel.org,
kvm@...r.kernel.org, bpf@...r.kernel.org,
virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org
Subject: Re: [PATCH v7 09/26] virtio_ring: split: implement
virtqueue_reset_vring_split()
在 2022/3/10 下午12:46, Xuan Zhuo 写道:
> On Wed, 9 Mar 2022 15:55:44 +0800, Jason Wang <jasowang@...hat.com> wrote:
>> 在 2022/3/8 下午8:35, Xuan Zhuo 写道:
>>> virtio ring supports reset.
>>>
>>> Queue reset is divided into several stages.
>>>
>>> 1. notify device queue reset
>>> 2. vring release
>>> 3. attach new vring
>>> 4. notify device queue re-enable
>>>
>>> After the first step is completed, the vring reset operation can be
>>> performed. If the newly set vring num does not change, then just reset
>>> the vq related value.
>>>
>>> Otherwise, the vring will be released and the vring will be reallocated.
>>> And the vring will be attached to the vq. If this process fails, the
>>> function will exit, and the state of the vq will be the vring release
>>> state. You can call this function again to reallocate the vring.
>>>
>>> In addition, vring_align, may_reduce_num are necessary for reallocating
>>> vring, so they are retained when creating vq.
>>>
>>> Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
>>> ---
>>> drivers/virtio/virtio_ring.c | 69 ++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 69 insertions(+)
>>>
>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>> index e0422c04c903..148fb1fd3d5a 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -158,6 +158,12 @@ struct vring_virtqueue {
>>> /* DMA address and size information */
>>> dma_addr_t queue_dma_addr;
>>> size_t queue_size_in_bytes;
>>> +
>>> + /* The parameters for creating vrings are reserved for
>>> + * creating new vrings when enabling reset queue.
>>> + */
>>> + u32 vring_align;
>>> + bool may_reduce_num;
>>> } split;
>>>
>>> /* Available for packed ring */
>>> @@ -217,6 +223,12 @@ struct vring_virtqueue {
>>> #endif
>>> };
>>>
>>> +static void vring_free(struct virtqueue *vq);
>>> +static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
>>> + struct virtio_device *vdev);
>>> +static int __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
>>> + struct virtio_device *vdev,
>>> + struct vring vring);
>>>
>>> /*
>>> * Helpers.
>>> @@ -1012,6 +1024,8 @@ static struct virtqueue *vring_create_virtqueue_split(
>>> return NULL;
>>> }
>>>
>>> + to_vvq(vq)->split.vring_align = vring_align;
>>> + to_vvq(vq)->split.may_reduce_num = may_reduce_num;
>>> to_vvq(vq)->split.queue_dma_addr = vring.dma_addr;
>>> to_vvq(vq)->split.queue_size_in_bytes = vring.queue_size_in_bytes;
>>> to_vvq(vq)->we_own_ring = true;
>>> @@ -1019,6 +1033,59 @@ static struct virtqueue *vring_create_virtqueue_split(
>>> return vq;
>>> }
>>>
>>> +static int virtqueue_reset_vring_split(struct virtqueue *_vq, u32 num)
>>> +{
>>
>> So what this function does is to resize the virtqueue actually, I
>> suggest to rename it as virtqueue_resize_split().
> In addition to resize, when num is 0, the function is to reinitialize vq ring
> related variables. For example avail_idx_shadow.
We need to move those logic to virtio_reset_vq() (I think we agree to
have a better name of it).
> So I think 'reset' is more appropriate.
The name is confusing at least to me, since we've already had
virtio_reset_vq() and most of the logic is to do the resize.
Thanks
>
> Thanks.
>
>>
>>> + struct vring_virtqueue *vq = to_vvq(_vq);
>>> + struct virtio_device *vdev = _vq->vdev;
>>> + struct vring_split vring;
>>> + int err;
>>> +
>>> + if (num > _vq->num_max)
>>> + return -E2BIG;
>>> +
>>> + switch (vq->vq.reset) {
>>> + case VIRTIO_VQ_RESET_STEP_NONE:
>>> + return -ENOENT;
>>> +
>>> + case VIRTIO_VQ_RESET_STEP_VRING_ATTACH:
>>> + case VIRTIO_VQ_RESET_STEP_DEVICE:
>>> + if (vq->split.vring.num == num || !num)
>>> + break;
>>> +
>>> + vring_free(_vq);
>>> +
>>> + fallthrough;
>>> +
>>> + case VIRTIO_VQ_RESET_STEP_VRING_RELEASE:
>>> + if (!num)
>>> + num = vq->split.vring.num;
>>> +
>>> + err = vring_create_vring_split(&vring, vdev,
>>> + vq->split.vring_align,
>>> + vq->weak_barriers,
>>> + vq->split.may_reduce_num, num);
>>> + if (err)
>>> + return -ENOMEM;
>>
>> We'd better need a safe fallback here like:
>>
>> If we can't allocate new memory, we can keep using the current one.
>> Otherwise an ethtool -G fail may make the device not usable.
>>
>> This could be done by not freeing the old vring and virtqueue states
>> until new is allocated.
>>
>>
>>> +
>>> + err = __vring_virtqueue_attach_split(vq, vdev, vring.vring);
>>> + if (err) {
>>> + vring_free_queue(vdev, vring.queue_size_in_bytes,
>>> + vring.queue,
>>> + vring.dma_addr);
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + vq->split.queue_dma_addr = vring.dma_addr;
>>> + vq->split.queue_size_in_bytes = vring.queue_size_in_bytes;
>>> + }
>>> +
>>> + __vring_virtqueue_init_split(vq, vdev);
>>> + vq->we_own_ring = true;
>>
>> This seems wrong, we have the transport (rproc/mlxtbf) that allocate the
>> vring by themselves. I think we need to fail the resize for we_own_ring
>> == false.
>>
>> Thanks
>>
>>
>>
>>> + vq->vq.reset = VIRTIO_VQ_RESET_STEP_VRING_ATTACH;
>>> +
>>> + return 0;
>>> +}
>>> +
>>>
>>> /*
>>> * Packed ring specific functions - *_packed().
>>> @@ -2317,6 +2384,8 @@ static int __vring_virtqueue_attach_split(struct vring_virtqueue *vq,
>>> static void __vring_virtqueue_init_split(struct vring_virtqueue *vq,
>>> struct virtio_device *vdev)
>>> {
>>> + vq->vq.reset = VIRTIO_VQ_RESET_STEP_NONE;
>>> +
>>> vq->packed_ring = false;
>>> vq->we_own_ring = false;
>>> vq->broken = false;
Powered by blists - more mailing lists