* [PATCH] virtio_ring: add an error code check in virtqueue_resize @ 2023-10-20 9:23 Su Hui 2023-10-20 9:28 ` Xuan Zhuo 2023-10-20 9:34 ` Michael S. Tsirkin 0 siblings, 2 replies; 17+ messages in thread From: Su Hui @ 2023-10-20 9:23 UTC (permalink / raw) To: mst, jasowang, xuanzhuo Cc: Su Hui, virtualization, linux-kernel, kernel-janitors 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 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize 2023-10-20 9:23 [PATCH] virtio_ring: add an error code check in virtqueue_resize Su Hui @ 2023-10-20 9:28 ` Xuan Zhuo 2023-10-20 9:34 ` Michael S. Tsirkin 1 sibling, 0 replies; 17+ messages in thread From: Xuan Zhuo @ 2023-10-20 9:28 UTC (permalink / raw) To: Su Hui; +Cc: Su Hui, virtualization, linux-kernel, kernel-janitors, mst, jasowang 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize 2023-10-20 9:23 [PATCH] virtio_ring: add an error code check in virtqueue_resize Su Hui 2023-10-20 9:28 ` Xuan Zhuo @ 2023-10-20 9:34 ` Michael S. Tsirkin 2023-10-20 9:36 ` Xuan Zhuo 1 sibling, 1 reply; 17+ messages in thread From: Michael S. Tsirkin @ 2023-10-20 9:34 UTC (permalink / raw) To: Su Hui; +Cc: jasowang, xuanzhuo, virtualization, linux-kernel, kernel-janitors 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 ^ permalink raw reply [flat|nested] 17+ 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; 17+ messages in thread From: Xuan Zhuo @ 2023-10-20 9:36 UTC (permalink / raw) To: Michael S. Tsirkin Cc: jasowang, virtualization, linux-kernel, kernel-janitors, Su Hui 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 > ^ permalink raw reply [flat|nested] 17+ 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; 17+ messages in thread From: Michael S. Tsirkin @ 2023-10-20 9:42 UTC (permalink / raw) To: Xuan Zhuo; +Cc: jasowang, virtualization, linux-kernel, kernel-janitors, Su Hui 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 > > ^ permalink raw reply [flat|nested] 17+ 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; 17+ messages in thread From: Xuan Zhuo @ 2023-10-20 9:50 UTC (permalink / raw) To: Michael S. Tsirkin Cc: jasowang, virtualization, linux-kernel, kernel-janitors, Su Hui 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 > > > > ^ permalink raw reply [flat|nested] 17+ 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; 17+ messages in thread From: Michael S. Tsirkin @ 2023-10-20 10:08 UTC (permalink / raw) To: Xuan Zhuo; +Cc: jasowang, virtualization, linux-kernel, kernel-janitors, Su Hui 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 > > > > > > ^ permalink raw reply [flat|nested] 17+ 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 2023-10-23 2:51 ` Su Hui 0 siblings, 1 reply; 17+ messages in thread From: Xuan Zhuo @ 2023-10-23 2:26 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: Su Hui, kernel-janitors, linux-kernel, virtualization 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize 2023-10-23 2:26 ` Xuan Zhuo @ 2023-10-23 2:51 ` Su Hui 2023-10-23 2:53 ` Xuan Zhuo 0 siblings, 1 reply; 17+ messages in thread From: Su Hui @ 2023-10-23 2:51 UTC (permalink / raw) To: Xuan Zhuo, Michael S. Tsirkin Cc: kernel-janitors, linux-kernel, 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize 2023-10-23 2:51 ` Su Hui @ 2023-10-23 2:53 ` Xuan Zhuo 2023-10-23 3:06 ` Su Hui 0 siblings, 1 reply; 17+ messages in thread From: Xuan Zhuo @ 2023-10-23 2:53 UTC (permalink / raw) To: Su Hui; +Cc: kernel-janitors, linux-kernel, virtualization, Michael S. Tsirkin 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize 2023-10-23 2:53 ` Xuan Zhuo @ 2023-10-23 3:06 ` Su Hui 2023-10-23 5:46 ` Xuan Zhuo 0 siblings, 1 reply; 17+ messages in thread From: Su Hui @ 2023-10-23 3:06 UTC (permalink / raw) To: Xuan Zhuo Cc: kernel-janitors, linux-kernel, virtualization, Michael S. Tsirkin 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 ? > Thanks. > > >> + vq->broken = true; >> + WARN_ON(err_reset); >> + return err; >> + } >> + >> + return err_reset; >> } >> >> Thanks. >> Su Hui >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize 2023-10-23 3:06 ` Su Hui @ 2023-10-23 5:46 ` Xuan Zhuo 2023-10-23 9:50 ` Su Hui 0 siblings, 1 reply; 17+ messages in thread From: Xuan Zhuo @ 2023-10-23 5:46 UTC (permalink / raw) To: Su Hui; +Cc: kernel-janitors, linux-kernel, virtualization, Michael S. Tsirkin 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 > >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize 2023-10-23 5:46 ` Xuan Zhuo @ 2023-10-23 9:50 ` Su Hui 2023-10-23 9:52 ` Xuan Zhuo 0 siblings, 1 reply; 17+ messages in thread From: Su Hui @ 2023-10-23 9:50 UTC (permalink / raw) To: Xuan Zhuo Cc: kernel-janitors, linux-kernel, virtualization, Michael S. Tsirkin 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] virtio_ring: add an error code check in virtqueue_resize 2023-10-23 9:50 ` Su Hui @ 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; 17+ messages in thread From: Xuan Zhuo @ 2023-10-23 9:52 UTC (permalink / raw) To: Su Hui; +Cc: kernel-janitors, linux-kernel, virtualization, Michael S. Tsirkin 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 > ^ permalink raw reply [flat|nested] 17+ 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; 17+ messages in thread From: Xuan Zhuo @ 2023-10-23 10:52 UTC (permalink / raw) To: Xuan Zhuo Cc: kernel-janitors, linux-kernel, virtualization, Michael S. Tsirkin, Su Hui 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 > > ^ permalink raw reply [flat|nested] 17+ 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; 17+ messages in thread From: Michael S. Tsirkin @ 2023-10-23 11:18 UTC (permalink / raw) To: Xuan Zhuo; +Cc: kernel-janitors, linux-kernel, virtualization, 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 <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 > > > ^ permalink raw reply [flat|nested] 17+ 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; 17+ messages in thread From: Michael S. Tsirkin @ 2023-10-23 11:17 UTC (permalink / raw) To: Xuan Zhuo; +Cc: Su Hui, kernel-janitors, linux-kernel, virtualization 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 > > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2023-10-23 11:19 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-20 9:23 [PATCH] virtio_ring: add an error code check in virtqueue_resize Su Hui 2023-10-20 9:28 ` 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 2023-10-23 2:51 ` Su Hui 2023-10-23 2:53 ` Xuan Zhuo 2023-10-23 3:06 ` Su Hui 2023-10-23 5:46 ` Xuan Zhuo 2023-10-23 9:50 ` Su Hui 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