virtqueue_resize_packed() or virtqueue_resize_split() can return
error code if failed, so add a check for this.
Signed-off-by: Su Hui <[email protected]>
---
I'm not sure that return directly is right or not,
maybe there are some process should do before return.
drivers/virtio/virtio_ring.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 51d8f3299c10..cf662c3a755b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
else
err = virtqueue_resize_split(_vq, num);
+ if (err)
+ return err;
+
return virtqueue_enable_after_reset(_vq);
}
EXPORT_SYMBOL_GPL(virtqueue_resize);
--
2.30.2
If any error happens, this function should restore to the old status.
So, whether the err is true, we should goto the virtqueue_enable_after_reset().
If the err is true, that mean the resize new quuee failed, but the queue
status has restored to the old status.
We should ignore the return value of the virtuqueue_resize_xxx().
Do you find other error in the virtuqueue_resize_xxx().
Thanks.
On Fri, 20 Oct 2023 17:23:21 +0800, Su Hui <[email protected]> wrote:
> virtqueue_resize_packed() or virtqueue_resize_split() can return
> error code if failed, so add a check for this.
>
> Signed-off-by: Su Hui <[email protected]>
> ---
>
> I'm not sure that return directly is right or not,
> maybe there are some process should do before return.
>
> drivers/virtio/virtio_ring.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 51d8f3299c10..cf662c3a755b 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> else
> err = virtqueue_resize_split(_vq, num);
>
> + if (err)
> + return err;
> +
> return virtqueue_enable_after_reset(_vq);
> }
> EXPORT_SYMBOL_GPL(virtqueue_resize);
> --
> 2.30.2
>
On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> virtqueue_resize_packed() or virtqueue_resize_split() can return
> error code if failed, so add a check for this.
>
> Signed-off-by: Su Hui <[email protected]>
> ---
>
> I'm not sure that return directly is right or not,
> maybe there are some process should do before return.
yes - presizely what virtqueue_enable_after_reset does.
Error handling in virtqueue_enable_after_reset is really weird BTW.
For some reason it overrides the error code returned.
> drivers/virtio/virtio_ring.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 51d8f3299c10..cf662c3a755b 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> else
> err = virtqueue_resize_split(_vq, num);
>
> + if (err)
> + return err;
> +
> return virtqueue_enable_after_reset(_vq);
So I think it should be something like:
int err_reset = virtqueue_enable_after_reset(_vq);
BUG_ON(err_reset);
return err;
> }
> EXPORT_SYMBOL_GPL(virtqueue_resize);
> --
> 2.30.2
On Fri, 20 Oct 2023 05:34:32 -0400, "Michael S. Tsirkin" <[email protected]> wrote:
> On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> > virtqueue_resize_packed() or virtqueue_resize_split() can return
> > error code if failed, so add a check for this.
> >
> > Signed-off-by: Su Hui <[email protected]>
> > ---
> >
> > I'm not sure that return directly is right or not,
> > maybe there are some process should do before return.
>
> yes - presizely what virtqueue_enable_after_reset does.
>
> Error handling in virtqueue_enable_after_reset is really weird BTW.
> For some reason it overrides the error code returned.
>
>
>
>
>
> > drivers/virtio/virtio_ring.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 51d8f3299c10..cf662c3a755b 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > else
> > err = virtqueue_resize_split(_vq, num);
> >
> > + if (err)
> > + return err;
> > +
> > return virtqueue_enable_after_reset(_vq);
>
> So I think it should be something like:
>
> int err_reset = virtqueue_enable_after_reset(_vq);
> BUG_ON(err_reset);
>
> return err;
>
How about WARN and vq->broken?
Thanks.
>
>
> > }
> > EXPORT_SYMBOL_GPL(virtqueue_resize);
> > --
> > 2.30.2
>
On Fri, Oct 20, 2023 at 05:36:41PM +0800, Xuan Zhuo wrote:
> On Fri, 20 Oct 2023 05:34:32 -0400, "Michael S. Tsirkin" <[email protected]> wrote:
> > On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> > > virtqueue_resize_packed() or virtqueue_resize_split() can return
> > > error code if failed, so add a check for this.
> > >
> > > Signed-off-by: Su Hui <[email protected]>
> > > ---
> > >
> > > I'm not sure that return directly is right or not,
> > > maybe there are some process should do before return.
> >
> > yes - presizely what virtqueue_enable_after_reset does.
> >
> > Error handling in virtqueue_enable_after_reset is really weird BTW.
> > For some reason it overrides the error code returned.
> >
> >
> >
> >
> >
> > > drivers/virtio/virtio_ring.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 51d8f3299c10..cf662c3a755b 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > > else
> > > err = virtqueue_resize_split(_vq, num);
> > >
> > > + if (err)
> > > + return err;
> > > +
> > > return virtqueue_enable_after_reset(_vq);
> >
> > So I think it should be something like:
> >
> > int err_reset = virtqueue_enable_after_reset(_vq);
> > BUG_ON(err_reset);
> >
> > return err;
> >
>
> How about WARN and vq->broken?
>
> Thanks.
Well, what are the cases where it can happen practically?
>
> >
> >
> > > }
> > > EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > --
> > > 2.30.2
> >
On Fri, 20 Oct 2023 05:42:14 -0400, "Michael S. Tsirkin" <[email protected]> wrote:
> On Fri, Oct 20, 2023 at 05:36:41PM +0800, Xuan Zhuo wrote:
> > On Fri, 20 Oct 2023 05:34:32 -0400, "Michael S. Tsirkin" <[email protected]> wrote:
> > > On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> > > > virtqueue_resize_packed() or virtqueue_resize_split() can return
> > > > error code if failed, so add a check for this.
> > > >
> > > > Signed-off-by: Su Hui <[email protected]>
> > > > ---
> > > >
> > > > I'm not sure that return directly is right or not,
> > > > maybe there are some process should do before return.
> > >
> > > yes - presizely what virtqueue_enable_after_reset does.
> > >
> > > Error handling in virtqueue_enable_after_reset is really weird BTW.
> > > For some reason it overrides the error code returned.
> > >
> > >
> > >
> > >
> > >
> > > > drivers/virtio/virtio_ring.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 51d8f3299c10..cf662c3a755b 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > > > else
> > > > err = virtqueue_resize_split(_vq, num);
> > > >
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > return virtqueue_enable_after_reset(_vq);
> > >
> > > So I think it should be something like:
> > >
> > > int err_reset = virtqueue_enable_after_reset(_vq);
> > > BUG_ON(err_reset);
> > >
> > > return err;
> > >
> >
> > How about WARN and vq->broken?
> >
> > Thanks.
>
> Well, what are the cases where it can happen practically?
Device error. Such as vp_active_vq()
Thanks.
>
> >
> > >
> > >
> > > > }
> > > > EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > > --
> > > > 2.30.2
> > >
>
On Fri, Oct 20, 2023 at 05:50:22PM +0800, Xuan Zhuo wrote:
> On Fri, 20 Oct 2023 05:42:14 -0400, "Michael S. Tsirkin" <[email protected]> wrote:
> > On Fri, Oct 20, 2023 at 05:36:41PM +0800, Xuan Zhuo wrote:
> > > On Fri, 20 Oct 2023 05:34:32 -0400, "Michael S. Tsirkin" <[email protected]> wrote:
> > > > On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> > > > > virtqueue_resize_packed() or virtqueue_resize_split() can return
> > > > > error code if failed, so add a check for this.
> > > > >
> > > > > Signed-off-by: Su Hui <[email protected]>
> > > > > ---
> > > > >
> > > > > I'm not sure that return directly is right or not,
> > > > > maybe there are some process should do before return.
> > > >
> > > > yes - presizely what virtqueue_enable_after_reset does.
> > > >
> > > > Error handling in virtqueue_enable_after_reset is really weird BTW.
> > > > For some reason it overrides the error code returned.
> > > >
> > > >
> > > >
> > > >
> > > >
> > > > > drivers/virtio/virtio_ring.c | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 51d8f3299c10..cf662c3a755b 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > > > > else
> > > > > err = virtqueue_resize_split(_vq, num);
> > > > >
> > > > > + if (err)
> > > > > + return err;
> > > > > +
> > > > > return virtqueue_enable_after_reset(_vq);
> > > >
> > > > So I think it should be something like:
> > > >
> > > > int err_reset = virtqueue_enable_after_reset(_vq);
> > > > BUG_ON(err_reset);
> > > >
> > > > return err;
> > > >
> > >
> > > How about WARN and vq->broken?
> > >
> > > Thanks.
> >
> > Well, what are the cases where it can happen practically?
>
> Device error. Such as vp_active_vq()
>
> Thanks.
Hmm interesting. OK. But do callers know to recover?
>
> >
> > >
> > > >
> > > >
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > > > --
> > > > > 2.30.2
> > > >
> >
On Fri, 20 Oct 2023 06:08:06 -0400, "Michael S. Tsirkin" <[email protected]> wrote:
> On Fri, Oct 20, 2023 at 05:50:22PM +0800, Xuan Zhuo wrote:
> > On Fri, 20 Oct 2023 05:42:14 -0400, "Michael S. Tsirkin" <[email protected]> wrote:
> > > On Fri, Oct 20, 2023 at 05:36:41PM +0800, Xuan Zhuo wrote:
> > > > On Fri, 20 Oct 2023 05:34:32 -0400, "Michael S. Tsirkin" <[email protected]> wrote:
> > > > > On Fri, Oct 20, 2023 at 05:23:21PM +0800, Su Hui wrote:
> > > > > > virtqueue_resize_packed() or virtqueue_resize_split() can return
> > > > > > error code if failed, so add a check for this.
> > > > > >
> > > > > > Signed-off-by: Su Hui <[email protected]>
> > > > > > ---
> > > > > >
> > > > > > I'm not sure that return directly is right or not,
> > > > > > maybe there are some process should do before return.
> > > > >
> > > > > yes - presizely what virtqueue_enable_after_reset does.
> > > > >
> > > > > Error handling in virtqueue_enable_after_reset is really weird BTW.
> > > > > For some reason it overrides the error code returned.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > > drivers/virtio/virtio_ring.c | 3 +++
> > > > > > 1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > index 51d8f3299c10..cf662c3a755b 100644
> > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > @@ -2759,6 +2759,9 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > > > > > else
> > > > > > err = virtqueue_resize_split(_vq, num);
> > > > > >
> > > > > > + if (err)
> > > > > > + return err;
> > > > > > +
> > > > > > return virtqueue_enable_after_reset(_vq);
> > > > >
> > > > > So I think it should be something like:
> > > > >
> > > > > int err_reset = virtqueue_enable_after_reset(_vq);
> > > > > BUG_ON(err_reset);
> > > > >
> > > > > return err;
> > > > >
> > > >
> > > > How about WARN and vq->broken?
> > > >
> > > > Thanks.
> > >
> > > Well, what are the cases where it can happen practically?
> >
> > Device error. Such as vp_active_vq()
> >
> > Thanks.
>
> Hmm interesting. OK. But do callers know to recover?
No.
So I think WARN + broken is suitable.
Thanks.
>
> >
> > >
> > > >
> > > > >
> > > > >
> > > > > > }
> > > > > > EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > > > > --
> > > > > > 2.30.2
> > > > >
> > >
>
> _______________________________________________
> Virtualization mailing list
> [email protected]
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
On 2023/10/23 10:26, Xuan Zhuo wrote:
>>>> Well, what are the cases where it can happen practically?
>>> Device error. Such as vp_active_vq()
>>>
>>> Thanks.
>> Hmm interesting. OK. But do callers know to recover?
>
> No.
>
> So I think WARN + broken is suitable.
>
> Thanks.
Sorry for the late, is the following code okay?
@@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
void (*recycle)(struct virtqueue *vq, void *buf))
{
struct vring_virtqueue *vq = to_vvq(_vq);
- int err;
+ int err, err_reset;
if (num > vq->vq.num_max)
return -E2BIG;
@@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
else
err = virtqueue_resize_split(_vq, num);
- return virtqueue_enable_after_reset(_vq);
+ err_reset = virtqueue_enable_after_reset(_vq);
+
+ if (err) {
+ vq->broken = true;
+ WARN_ON(err_reset);
+ return err;
+ }
+
+ return err_reset;
}
Thanks.
Su Hui
On Mon, 23 Oct 2023 10:51:59 +0800, Su Hui <[email protected]> wrote:
> On 2023/10/23 10:26, Xuan Zhuo wrote:
> >>>> Well, what are the cases where it can happen practically?
> >>> Device error. Such as vp_active_vq()
> >>>
> >>> Thanks.
> >> Hmm interesting. OK. But do callers know to recover?
> >
> > No.
> >
> > So I think WARN + broken is suitable.
> >
> > Thanks.
> Sorry for the late, is the following code okay?
>
> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> void (*recycle)(struct virtqueue *vq, void *buf))
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> - int err;
> + int err, err_reset;
>
> if (num > vq->vq.num_max)
> return -E2BIG;
> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> else
> err = virtqueue_resize_split(_vq, num);
>
> - return virtqueue_enable_after_reset(_vq);
> + err_reset = virtqueue_enable_after_reset(_vq);
> +
> + if (err) {
No err.
err is not important.
You can remove that.
Thanks.
> + vq->broken = true;
> + WARN_ON(err_reset);
> + return err;
> + }
> +
> + return err_reset;
> }
>
> Thanks.
> Su Hui
>
On 2023/10/23 10:53, Xuan Zhuo wrote:
> On Mon, 23 Oct 2023 10:51:59 +0800, Su Hui <[email protected]> wrote:
>> On 2023/10/23 10:26, Xuan Zhuo wrote:
>>>>>> Well, what are the cases where it can happen practically?
>>>>> Device error. Such as vp_active_vq()
>>>>>
>>>>> Thanks.
>>>> Hmm interesting. OK. But do callers know to recover?
>>> No.
>>>
>>> So I think WARN + broken is suitable.
>>>
>>> Thanks.
>> Sorry for the late, is the following code okay?
>>
>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>> void (*recycle)(struct virtqueue *vq, void *buf))
>> {
>> struct vring_virtqueue *vq = to_vvq(_vq);
>> - int err;
>> + int err, err_reset;
>>
>> if (num > vq->vq.num_max)
>> return -E2BIG;
>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>> else
>> err = virtqueue_resize_split(_vq, num);
>>
>> - return virtqueue_enable_after_reset(_vq);
>> + err_reset = virtqueue_enable_after_reset(_vq);
>> +
>> + if (err) {
> No err.
>
> err is not important.
> You can remove that.
Emm, I'm a little confused that which code should I remove ?
> Thanks.
>
>
>> + vq->broken = true;
>> + WARN_ON(err_reset);
>> + return err;
>> + }
>> +
>> + return err_reset;
>> }
>>
>> Thanks.
>> Su Hui
>>
On Mon, 23 Oct 2023 11:06:48 +0800, Su Hui <[email protected]> wrote:
>
> On 2023/10/23 10:53, Xuan Zhuo wrote:
> > On Mon, 23 Oct 2023 10:51:59 +0800, Su Hui <[email protected]> wrote:
> >> On 2023/10/23 10:26, Xuan Zhuo wrote:
> >>>>>> Well, what are the cases where it can happen practically?
> >>>>> Device error. Such as vp_active_vq()
> >>>>>
> >>>>> Thanks.
> >>>> Hmm interesting. OK. But do callers know to recover?
> >>> No.
> >>>
> >>> So I think WARN + broken is suitable.
> >>>
> >>> Thanks.
> >> Sorry for the late, is the following code okay?
> >>
> >> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> >> void (*recycle)(struct virtqueue *vq, void *buf))
> >> {
> >> struct vring_virtqueue *vq = to_vvq(_vq);
> >> - int err;
> >> + int err, err_reset;
> >>
> >> if (num > vq->vq.num_max)
> >> return -E2BIG;
> >> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> >> else
> >> err = virtqueue_resize_split(_vq, num);
> >>
> >> - return virtqueue_enable_after_reset(_vq);
> >> + err_reset = virtqueue_enable_after_reset(_vq);
> >> +
> >> + if (err) {
> > No err.
> >
> > err is not important.
> > You can remove that.
>
> Emm, I'm a little confused that which code should I remove ?
like this:
if (vq->packed_ring)
virtqueue_resize_packed(_vq, num);
else
virtqueue_resize_split(_vq, num);
And we should set broken and warn inside virtqueue_enable_after_reset()?
Thanks.
>
> > Thanks.
> >
> >
> >> + vq->broken = true;
> >> + WARN_ON(err_reset);
> >> + return err;
> >> + }
> >> +
> >> + return err_reset;
> >> }
> >>
> >> Thanks.
> >> Su Hui
> >>
On 2023/10/23 13:46, Xuan Zhuo wrote:
>>>>>>>> Well, what are the cases where it can happen practically?
>>>>>>> Device error. Such as vp_active_vq()
>>>>>>>
>>>>>>> Thanks.
>>>>>> Hmm interesting. OK. But do callers know to recover?
>>>>> No.
>>>>>
>>>>> So I think WARN + broken is suitable.
>>>>>
>>>>> Thanks.
>>>> Sorry for the late, is the following code okay?
>>>>
>>>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>>>> void (*recycle)(struct virtqueue *vq, void *buf))
>>>> {
>>>> struct vring_virtqueue *vq = to_vvq(_vq);
>>>> - int err;
>>>> + int err, err_reset;
>>>>
>>>> if (num > vq->vq.num_max)
>>>> return -E2BIG;
>>>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
>>>> else
>>>> err = virtqueue_resize_split(_vq, num);
>>>>
>>>> - return virtqueue_enable_after_reset(_vq);
>>>> + err_reset = virtqueue_enable_after_reset(_vq);
>>>> +
>>>> + if (err) {
>>> No err.
>>>
>>> err is not important.
>>> You can remove that.
>> Emm, I'm a little confused that which code should I remove ?
>>
>>
>> like this:
>> if (vq->packed_ring)
>> virtqueue_resize_packed(_vq, num);
>> else
>> virtqueue_resize_split(_vq, num);
>>
>> And we should set broken and warn inside virtqueue_enable_after_reset()?
In my opinion, we should return the error code of virtqueue_resize_packed() / virtqueue_resize_split().
But if this err is not important, this patch makes no sense.
Maybe I misunderstand somewhere...
If you think it's worth sending a patch, you can send it :).(I'm not familiar with this code).
Thanks,
Su Hui
On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <[email protected]> wrote:
> On 2023/10/23 13:46, Xuan Zhuo wrote:
> >>>>>>>> Well, what are the cases where it can happen practically?
> >>>>>>> Device error. Such as vp_active_vq()
> >>>>>>>
> >>>>>>> Thanks.
> >>>>>> Hmm interesting. OK. But do callers know to recover?
> >>>>> No.
> >>>>>
> >>>>> So I think WARN + broken is suitable.
> >>>>>
> >>>>> Thanks.
> >>>> Sorry for the late, is the following code okay?
> >>>>
> >>>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> >>>> void (*recycle)(struct virtqueue *vq, void *buf))
> >>>> {
> >>>> struct vring_virtqueue *vq = to_vvq(_vq);
> >>>> - int err;
> >>>> + int err, err_reset;
> >>>>
> >>>> if (num > vq->vq.num_max)
> >>>> return -E2BIG;
> >>>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> >>>> else
> >>>> err = virtqueue_resize_split(_vq, num);
> >>>>
> >>>> - return virtqueue_enable_after_reset(_vq);
> >>>> + err_reset = virtqueue_enable_after_reset(_vq);
> >>>> +
> >>>> + if (err) {
> >>> No err.
> >>>
> >>> err is not important.
> >>> You can remove that.
> >> Emm, I'm a little confused that which code should I remove ?
> >>
> >>
> >> like this:
> >> if (vq->packed_ring)
> >> virtqueue_resize_packed(_vq, num);
> >> else
> >> virtqueue_resize_split(_vq, num);
> >>
> >> And we should set broken and warn inside virtqueue_enable_after_reset()?
>
> In my opinion, we should return the error code of virtqueue_resize_packed() / virtqueue_resize_split().
> But if this err is not important, this patch makes no sense.
> Maybe I misunderstand somewhere...
> If you think it's worth sending a patch, you can send it :).(I'm not familiar with this code).
OK.
Thanks.
>
> Thanks,
> Su Hui
>
On Mon, 23 Oct 2023 17:52:02 +0800, Xuan Zhuo <[email protected]> wrote:
> On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <[email protected]> wrote:
> > On 2023/10/23 13:46, Xuan Zhuo wrote:
> > >>>>>>>> Well, what are the cases where it can happen practically?
> > >>>>>>> Device error. Such as vp_active_vq()
> > >>>>>>>
> > >>>>>>> Thanks.
> > >>>>>> Hmm interesting. OK. But do callers know to recover?
> > >>>>> No.
> > >>>>>
> > >>>>> So I think WARN + broken is suitable.
> > >>>>>
> > >>>>> Thanks.
> > >>>> Sorry for the late, is the following code okay?
> > >>>>
> > >>>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > >>>> void (*recycle)(struct virtqueue *vq, void *buf))
> > >>>> {
> > >>>> struct vring_virtqueue *vq = to_vvq(_vq);
> > >>>> - int err;
> > >>>> + int err, err_reset;
> > >>>>
> > >>>> if (num > vq->vq.num_max)
> > >>>> return -E2BIG;
> > >>>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > >>>> else
> > >>>> err = virtqueue_resize_split(_vq, num);
> > >>>>
> > >>>> - return virtqueue_enable_after_reset(_vq);
> > >>>> + err_reset = virtqueue_enable_after_reset(_vq);
> > >>>> +
> > >>>> + if (err) {
> > >>> No err.
> > >>>
> > >>> err is not important.
> > >>> You can remove that.
> > >> Emm, I'm a little confused that which code should I remove ?
> > >>
> > >>
> > >> like this:
> > >> if (vq->packed_ring)
> > >> virtqueue_resize_packed(_vq, num);
> > >> else
> > >> virtqueue_resize_split(_vq, num);
> > >>
> > >> And we should set broken and warn inside virtqueue_enable_after_reset()?
> >
> > In my opinion, we should return the error code of virtqueue_resize_packed() / virtqueue_resize_split().
> > But if this err is not important, this patch makes no sense.
> > Maybe I misunderstand somewhere...
> > If you think it's worth sending a patch, you can send it :).(I'm not familiar with this code).
>
> OK.
Hi Michael,
The queue reset code is wrote with the CONFIG_VIRTIO_HARDEN_NOTIFICATION.
When we disable the vq, the broken is true until we re-enable it.
So when we re-enable it fail, the vq is broken status.
Normally, this just happens on the buggy device.
So I think that is enough.
Thanks.
static int vp_modern_disable_vq_and_reset(struct virtqueue *vq)
{
[...]
vp_modern_set_queue_reset(mdev, vq->index);
[...]
#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
->> __virtqueue_break(vq);
#endif
[...]
}
static int vp_modern_enable_vq_after_reset(struct virtqueue *vq)
{
[...]
if (vp_modern_get_queue_reset(mdev, index))
return -EBUSY;
if (vp_modern_get_queue_enable(mdev, index))
return -EBUSY;
err = vp_active_vq(vq, info->msix_vector);
if (err)
return err;
}
[...]
#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
->> __virtqueue_unbreak(vq);
#endif
[...]
}
>
> Thanks.
>
>
> >
> > Thanks,
> > Su Hui
> >
On Mon, Oct 23, 2023 at 05:52:02PM +0800, Xuan Zhuo wrote:
> On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <[email protected]> wrote:
> > On 2023/10/23 13:46, Xuan Zhuo wrote:
> > >>>>>>>> Well, what are the cases where it can happen practically?
> > >>>>>>> Device error. Such as vp_active_vq()
> > >>>>>>>
> > >>>>>>> Thanks.
> > >>>>>> Hmm interesting. OK. But do callers know to recover?
> > >>>>> No.
> > >>>>>
> > >>>>> So I think WARN + broken is suitable.
> > >>>>>
> > >>>>> Thanks.
> > >>>> Sorry for the late, is the following code okay?
> > >>>>
> > >>>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > >>>> void (*recycle)(struct virtqueue *vq, void *buf))
> > >>>> {
> > >>>> struct vring_virtqueue *vq = to_vvq(_vq);
> > >>>> - int err;
> > >>>> + int err, err_reset;
> > >>>>
> > >>>> if (num > vq->vq.num_max)
> > >>>> return -E2BIG;
> > >>>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > >>>> else
> > >>>> err = virtqueue_resize_split(_vq, num);
> > >>>>
> > >>>> - return virtqueue_enable_after_reset(_vq);
> > >>>> + err_reset = virtqueue_enable_after_reset(_vq);
> > >>>> +
> > >>>> + if (err) {
> > >>> No err.
> > >>>
> > >>> err is not important.
> > >>> You can remove that.
> > >> Emm, I'm a little confused that which code should I remove ?
> > >>
> > >>
> > >> like this:
> > >> if (vq->packed_ring)
> > >> virtqueue_resize_packed(_vq, num);
> > >> else
> > >> virtqueue_resize_split(_vq, num);
> > >>
> > >> And we should set broken and warn inside virtqueue_enable_after_reset()?
> >
> > In my opinion, we should return the error code of virtqueue_resize_packed() / virtqueue_resize_split().
> > But if this err is not important, this patch makes no sense.
> > Maybe I misunderstand somewhere...
> > If you think it's worth sending a patch, you can send it :).(I'm not familiar with this code).
>
> OK.
>
> Thanks.
I would first try to recover by re-enabling.
If that fails we can set broken.
>
> >
> > Thanks,
> > Su Hui
> >
On Mon, Oct 23, 2023 at 06:52:34PM +0800, Xuan Zhuo wrote:
> On Mon, 23 Oct 2023 17:52:02 +0800, Xuan Zhuo <[email protected]> wrote:
> > On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <[email protected]> wrote:
> > > On 2023/10/23 13:46, Xuan Zhuo wrote:
> > > >>>>>>>> Well, what are the cases where it can happen practically?
> > > >>>>>>> Device error. Such as vp_active_vq()
> > > >>>>>>>
> > > >>>>>>> Thanks.
> > > >>>>>> Hmm interesting. OK. But do callers know to recover?
> > > >>>>> No.
> > > >>>>>
> > > >>>>> So I think WARN + broken is suitable.
> > > >>>>>
> > > >>>>> Thanks.
> > > >>>> Sorry for the late, is the following code okay?
> > > >>>>
> > > >>>> @@ -2739,7 +2739,7 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > > >>>> void (*recycle)(struct virtqueue *vq, void *buf))
> > > >>>> {
> > > >>>> struct vring_virtqueue *vq = to_vvq(_vq);
> > > >>>> - int err;
> > > >>>> + int err, err_reset;
> > > >>>>
> > > >>>> if (num > vq->vq.num_max)
> > > >>>> return -E2BIG;
> > > >>>> @@ -2759,7 +2759,15 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> > > >>>> else
> > > >>>> err = virtqueue_resize_split(_vq, num);
> > > >>>>
> > > >>>> - return virtqueue_enable_after_reset(_vq);
> > > >>>> + err_reset = virtqueue_enable_after_reset(_vq);
> > > >>>> +
> > > >>>> + if (err) {
> > > >>> No err.
> > > >>>
> > > >>> err is not important.
> > > >>> You can remove that.
> > > >> Emm, I'm a little confused that which code should I remove ?
> > > >>
> > > >>
> > > >> like this:
> > > >> if (vq->packed_ring)
> > > >> virtqueue_resize_packed(_vq, num);
> > > >> else
> > > >> virtqueue_resize_split(_vq, num);
> > > >>
> > > >> And we should set broken and warn inside virtqueue_enable_after_reset()?
> > >
> > > In my opinion, we should return the error code of virtqueue_resize_packed() / virtqueue_resize_split().
> > > But if this err is not important, this patch makes no sense.
> > > Maybe I misunderstand somewhere...
> > > If you think it's worth sending a patch, you can send it :).(I'm not familiar with this code).
> >
> > OK.
>
> Hi Michael,
>
> The queue reset code is wrote with the CONFIG_VIRTIO_HARDEN_NOTIFICATION.
>
> When we disable the vq, the broken is true until we re-enable it.
>
> So when we re-enable it fail, the vq is broken status.
>
> Normally, this just happens on the buggy device.
> So I think that is enough.
>
> Thanks.
I don't know what to do about CONFIG_VIRTIO_HARDEN_NOTIFICATION.
It's known to be broken and it does not look like there's
active effort to revive it - should we just drop it for now?
>
> static int vp_modern_disable_vq_and_reset(struct virtqueue *vq)
> {
> [...]
>
> vp_modern_set_queue_reset(mdev, vq->index);
>
> [...]
>
> #ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
> ->> __virtqueue_break(vq);
> #endif
>
> [...]
> }
>
> static int vp_modern_enable_vq_after_reset(struct virtqueue *vq)
> {
> [...]
>
> if (vp_modern_get_queue_reset(mdev, index))
> return -EBUSY;
>
> if (vp_modern_get_queue_enable(mdev, index))
> return -EBUSY;
>
> err = vp_active_vq(vq, info->msix_vector);
> if (err)
> return err;
>
> }
>
> [...]
>
> #ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
> ->> __virtqueue_unbreak(vq);
> #endif
>
> [...]
> }
>
>
>
>
> >
> > Thanks.
> >
> >
> > >
> > > Thanks,
> > > Su Hui
> > >