* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
[not found] <20231020092320.209234-1-suhui@nfschina.com>
@ 2023-10-20 9:28 ` Xuan Zhuo
2023-10-20 9:34 ` Michael S. Tsirkin
1 sibling, 0 replies; 13+ messages in thread
From: Xuan Zhuo @ 2023-10-20 9:28 UTC (permalink / raw)
To: Su Hui; +Cc: Su Hui, mst, kernel-janitors, linux-kernel, virtualization
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 <suhui@nfschina.com> 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 <suhui@nfschina.com>
> ---
>
> 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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
[not found] <20231020092320.209234-1-suhui@nfschina.com>
2023-10-20 9:28 ` [PATCH] virtio_ring: add an error code check in virtqueue_resize Xuan Zhuo
@ 2023-10-20 9:34 ` Michael S. Tsirkin
2023-10-20 9:36 ` Xuan Zhuo
1 sibling, 1 reply; 13+ messages in thread
From: Michael S. Tsirkin @ 2023-10-20 9:34 UTC (permalink / raw)
To: Su Hui; +Cc: xuanzhuo, kernel-janitors, linux-kernel, virtualization
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 <suhui@nfschina.com>
> ---
>
> 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
2023-10-20 9:34 ` Michael S. Tsirkin
@ 2023-10-20 9:36 ` Xuan Zhuo
2023-10-20 9:42 ` Michael S. Tsirkin
0 siblings, 1 reply; 13+ messages in thread
From: Xuan Zhuo @ 2023-10-20 9:36 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Su Hui, kernel-janitors, linux-kernel, virtualization
On Fri, 20 Oct 2023 05:34:32 -0400, "Michael S. Tsirkin" <mst@redhat.com> 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 <suhui@nfschina.com>
> > ---
> >
> > 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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
2023-10-20 9:36 ` Xuan Zhuo
@ 2023-10-20 9:42 ` Michael S. Tsirkin
2023-10-20 9:50 ` Xuan Zhuo
0 siblings, 1 reply; 13+ messages in thread
From: Michael S. Tsirkin @ 2023-10-20 9:42 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Su Hui, kernel-janitors, linux-kernel, virtualization
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" <mst@redhat.com> 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 <suhui@nfschina.com>
> > > ---
> > >
> > > 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
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
2023-10-20 9:42 ` Michael S. Tsirkin
@ 2023-10-20 9:50 ` Xuan Zhuo
2023-10-20 10:08 ` Michael S. Tsirkin
0 siblings, 1 reply; 13+ messages in thread
From: Xuan Zhuo @ 2023-10-20 9:50 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Su Hui, kernel-janitors, linux-kernel, virtualization
On Fri, 20 Oct 2023 05:42:14 -0400, "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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 <suhui@nfschina.com>
> > > > ---
> > > >
> > > > 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
> > >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
2023-10-20 9:50 ` Xuan Zhuo
@ 2023-10-20 10:08 ` Michael S. Tsirkin
2023-10-23 2:26 ` Xuan Zhuo
0 siblings, 1 reply; 13+ messages in thread
From: Michael S. Tsirkin @ 2023-10-20 10:08 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Su Hui, kernel-janitors, linux-kernel, virtualization
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" <mst@redhat.com> 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" <mst@redhat.com> 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 <suhui@nfschina.com>
> > > > > ---
> > > > >
> > > > > 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
> > > >
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
2023-10-20 10:08 ` Michael S. Tsirkin
@ 2023-10-23 2:26 ` Xuan Zhuo
[not found] ` <d4aa3f76-3e08-a852-a948-b88226a37fdd@nfschina.com>
0 siblings, 1 reply; 13+ messages in thread
From: Xuan Zhuo @ 2023-10-23 2:26 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization, kernel-janitors, Su Hui, linux-kernel
On Fri, 20 Oct 2023 06:08:06 -0400, "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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" <mst@redhat.com> 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 <suhui@nfschina.com>
> > > > > > ---
> > > > > >
> > > > > > 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
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
[not found] ` <d4aa3f76-3e08-a852-a948-b88226a37fdd@nfschina.com>
@ 2023-10-23 2:53 ` Xuan Zhuo
[not found] ` <46aee820-6c01-ed8a-613b-5c57258d749e@nfschina.com>
0 siblings, 1 reply; 13+ messages in thread
From: Xuan Zhuo @ 2023-10-23 2:53 UTC (permalink / raw)
To: Su Hui; +Cc: Michael S. Tsirkin, kernel-janitors, linux-kernel, virtualization
On Mon, 23 Oct 2023 10:51:59 +0800, Su Hui <suhui@nfschina.com> 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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
[not found] ` <46aee820-6c01-ed8a-613b-5c57258d749e@nfschina.com>
@ 2023-10-23 5:46 ` Xuan Zhuo
[not found] ` <6a7d1006-0988-77ea-0991-9c7b422d78e1@nfschina.com>
0 siblings, 1 reply; 13+ messages in thread
From: Xuan Zhuo @ 2023-10-23 5:46 UTC (permalink / raw)
To: Su Hui; +Cc: Michael S. Tsirkin, kernel-janitors, linux-kernel, virtualization
On Mon, 23 Oct 2023 11:06:48 +0800, Su Hui <suhui@nfschina.com> wrote:
>
> On 2023/10/23 10:53, Xuan Zhuo wrote:
> > On Mon, 23 Oct 2023 10:51:59 +0800, Su Hui <suhui@nfschina.com> 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
> >>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
[not found] ` <6a7d1006-0988-77ea-0991-9c7b422d78e1@nfschina.com>
@ 2023-10-23 9:52 ` Xuan Zhuo
2023-10-23 10:52 ` Xuan Zhuo
2023-10-23 11:17 ` Michael S. Tsirkin
0 siblings, 2 replies; 13+ messages in thread
From: Xuan Zhuo @ 2023-10-23 9:52 UTC (permalink / raw)
To: Su Hui; +Cc: Michael S. Tsirkin, kernel-janitors, linux-kernel, virtualization
On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <suhui@nfschina.com> 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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
2023-10-23 9:52 ` Xuan Zhuo
@ 2023-10-23 10:52 ` Xuan Zhuo
2023-10-23 11:18 ` Michael S. Tsirkin
2023-10-23 11:17 ` Michael S. Tsirkin
1 sibling, 1 reply; 13+ messages in thread
From: Xuan Zhuo @ 2023-10-23 10:52 UTC (permalink / raw)
To: Xuan Zhuo
Cc: Su Hui, Michael S. Tsirkin, kernel-janitors, linux-kernel,
virtualization
On Mon, 23 Oct 2023 17:52:02 +0800, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <suhui@nfschina.com> 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
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
2023-10-23 9:52 ` Xuan Zhuo
2023-10-23 10:52 ` Xuan Zhuo
@ 2023-10-23 11:17 ` Michael S. Tsirkin
1 sibling, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2023-10-23 11:17 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: virtualization, kernel-janitors, Su Hui, linux-kernel
On Mon, Oct 23, 2023 at 05:52:02PM +0800, Xuan Zhuo wrote:
> On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <suhui@nfschina.com> 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
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize
2023-10-23 10:52 ` Xuan Zhuo
@ 2023-10-23 11:18 ` Michael S. Tsirkin
0 siblings, 0 replies; 13+ messages in thread
From: Michael S. Tsirkin @ 2023-10-23 11:18 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Su Hui, kernel-janitors, linux-kernel, virtualization
On Mon, Oct 23, 2023 at 06:52:34PM +0800, Xuan Zhuo wrote:
> On Mon, 23 Oct 2023 17:52:02 +0800, Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > On Mon, 23 Oct 2023 17:50:46 +0800, Su Hui <suhui@nfschina.com> 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
> > >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-10-23 11:18 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20231020092320.209234-1-suhui@nfschina.com>
2023-10-20 9:28 ` [PATCH] virtio_ring: add an error code check in virtqueue_resize Xuan Zhuo
2023-10-20 9:34 ` Michael S. Tsirkin
2023-10-20 9:36 ` Xuan Zhuo
2023-10-20 9:42 ` Michael S. Tsirkin
2023-10-20 9:50 ` Xuan Zhuo
2023-10-20 10:08 ` Michael S. Tsirkin
2023-10-23 2:26 ` Xuan Zhuo
[not found] ` <d4aa3f76-3e08-a852-a948-b88226a37fdd@nfschina.com>
2023-10-23 2:53 ` Xuan Zhuo
[not found] ` <46aee820-6c01-ed8a-613b-5c57258d749e@nfschina.com>
2023-10-23 5:46 ` Xuan Zhuo
[not found] ` <6a7d1006-0988-77ea-0991-9c7b422d78e1@nfschina.com>
2023-10-23 9:52 ` Xuan Zhuo
2023-10-23 10:52 ` Xuan Zhuo
2023-10-23 11:18 ` Michael S. Tsirkin
2023-10-23 11:17 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).