| Message ID | 1575444716-17632-1-git-send-email-pannengyuan@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] virtio: add ability to delete vq through a pointer | expand |
> From: Pan Nengyuan <pannengyuan@huawei.com> > > Devices tend to maintain vq pointers, allow deleting them trough a vq > pointer. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > --- > Changes v2 to v1: > - add a new function virtio_delete_queue to cleanup vq through a vq pointer > --- > hw/virtio/virtio.c | 16 +++++++++++----- > include/hw/virtio/virtio.h | 2 ++ > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 04716b5..6de3cfd 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int > queue_size, > return &vdev->vq[i]; > } > > +void virtio_delete_queue(VirtQueue *vq) > +{ > + vq->vring.num = 0; > + vq->vring.num_default = 0; > + vq->handle_output = NULL; > + vq->handle_aio_output = NULL; > + g_free(vq->used_elems); > + vq->used_elems = NULL; > +} > + > void virtio_del_queue(VirtIODevice *vdev, int n) > { > if (n < 0 || n >= VIRTIO_QUEUE_MAX) { > abort(); > } > > - vdev->vq[n].vring.num = 0; > - vdev->vq[n].vring.num_default = 0; > - vdev->vq[n].handle_output = NULL; > - vdev->vq[n].handle_aio_output = NULL; > - g_free(vdev->vq[n].used_elems); > + virtio_delete_queue(&vdev->vq[n]); > } > > static void virtio_set_isr(VirtIODevice *vdev, int value) > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index c32a815..e18756d 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int > queue_size, > > void virtio_del_queue(VirtIODevice *vdev, int n); > > +void virtio_delete_queue(VirtQueue *vq); > + > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len); > void virtqueue_flush(VirtQueue *vq, unsigned int count); > -- > 2.7.2.windows.1 > > Overall it ooks good to me. Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function We are doing : virtio_del_queue(vdev, 0); One can directly call "virtio_delete_queue". It can become confusing to call multiple functions for same purpose. Instead, Can we make "virtio_delete_queue" static inline? Other than that: Reviewed-by: Pankaj Gupta <pagupta@redhat.com> > >
On 04/12/2019 08:31, pannengyuan@huawei.com wrote: > From: Pan Nengyuan <pannengyuan@huawei.com> > > Devices tend to maintain vq pointers, allow deleting them trough a vq pointer. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > --- > Changes v2 to v1: > - add a new function virtio_delete_queue to cleanup vq through a vq pointer > --- > hw/virtio/virtio.c | 16 +++++++++++----- > include/hw/virtio/virtio.h | 2 ++ > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 04716b5..6de3cfd 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, > return &vdev->vq[i]; > } > > +void virtio_delete_queue(VirtQueue *vq) > +{ > + vq->vring.num = 0; > + vq->vring.num_default = 0; > + vq->handle_output = NULL; > + vq->handle_aio_output = NULL; > + g_free(vq->used_elems); > + vq->used_elems = NULL; > +} > + > void virtio_del_queue(VirtIODevice *vdev, int n) > { > if (n < 0 || n >= VIRTIO_QUEUE_MAX) { > abort(); > } > > - vdev->vq[n].vring.num = 0; > - vdev->vq[n].vring.num_default = 0; > - vdev->vq[n].handle_output = NULL; > - vdev->vq[n].handle_aio_output = NULL; > - g_free(vdev->vq[n].used_elems); > + virtio_delete_queue(&vdev->vq[n]); > } > > static void virtio_set_isr(VirtIODevice *vdev, int value) > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index c32a815..e18756d 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, > > void virtio_del_queue(VirtIODevice *vdev, int n); > > +void virtio_delete_queue(VirtQueue *vq); > + > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len); > void virtqueue_flush(VirtQueue *vq, unsigned int count); > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
On 12/4/19 1:31 AM, pannengyuan@huawei.com wrote: > From: Pan Nengyuan <pannengyuan@huawei.com> > > Devices tend to maintain vq pointers, allow deleting them trough a vq pointer. through > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > --- Also, don't forget to send a 0/3 cover letter (any series longer than one patch should have a cover letter; it is possible to configure git to do this automatically: https://wiki.qemu.org/Contribute/SubmitAPatch has this tip and others)
On 2019/12/4 16:33, Pankaj Gupta wrote: > >> From: Pan Nengyuan <pannengyuan@huawei.com> >> >> Devices tend to maintain vq pointers, allow deleting them trough a vq >> pointer. >> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> >> --- >> Changes v2 to v1: >> - add a new function virtio_delete_queue to cleanup vq through a vq pointer >> --- >> hw/virtio/virtio.c | 16 +++++++++++----- >> include/hw/virtio/virtio.h | 2 ++ >> 2 files changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 04716b5..6de3cfd 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int >> queue_size, >> return &vdev->vq[i]; >> } >> >> +void virtio_delete_queue(VirtQueue *vq) >> +{ >> + vq->vring.num = 0; >> + vq->vring.num_default = 0; >> + vq->handle_output = NULL; >> + vq->handle_aio_output = NULL; >> + g_free(vq->used_elems); >> + vq->used_elems = NULL; >> +} >> + >> void virtio_del_queue(VirtIODevice *vdev, int n) >> { >> if (n < 0 || n >= VIRTIO_QUEUE_MAX) { >> abort(); >> } >> >> - vdev->vq[n].vring.num = 0; >> - vdev->vq[n].vring.num_default = 0; >> - vdev->vq[n].handle_output = NULL; >> - vdev->vq[n].handle_aio_output = NULL; >> - g_free(vdev->vq[n].used_elems); >> + virtio_delete_queue(&vdev->vq[n]); >> } >> >> static void virtio_set_isr(VirtIODevice *vdev, int value) >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >> index c32a815..e18756d 100644 >> --- a/include/hw/virtio/virtio.h >> +++ b/include/hw/virtio/virtio.h >> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int >> queue_size, >> >> void virtio_del_queue(VirtIODevice *vdev, int n); >> >> +void virtio_delete_queue(VirtQueue *vq); >> + >> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, >> unsigned int len); >> void virtqueue_flush(VirtQueue *vq, unsigned int count); >> -- >> 2.7.2.windows.1 >> >> > Overall it ooks good to me. > > Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function > We are doing : virtio_del_queue(vdev, 0); > > One can directly call "virtio_delete_queue". It can become confusing > to call multiple functions for same purpose. Instead, Can we make > "virtio_delete_queue" static inline? > yes, It will be a little confused, but I think it will have the same problem if we make "virtio_delete_queue" static inline. We can directly call it aslo. （e.g virtio-serial-bus.c virtio-balloon.c). How about replacing the function name to make it more clear (e.g virtio_delete_queue -> virtio_queue_cleanup) ? It's too similar between "virtio_del_queue" and "virtio_delete_queue". > Other than that: > Reviewed-by: Pankaj Gupta <pagupta@redhat.com> > >> >> > > > . >
On 2019/12/4 22:40, Eric Blake wrote: > On 12/4/19 1:31 AM, pannengyuan@huawei.com wrote: >> From: Pan Nengyuan <pannengyuan@huawei.com> >> >> Devices tend to maintain vq pointers, allow deleting them trough a vq >> pointer. > > through Thanks. I'm sorry for my carelessness. > >> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> >> --- > > Also, don't forget to send a 0/3 cover letter (any series longer than > one patch should have a cover letter; it is possible to configure git to > do this automatically: https://wiki.qemu.org/Contribute/SubmitAPatch has > this tip and others) ok, thanks. >
> > On 2019/12/4 16:33, Pankaj Gupta wrote: > > > >> From: Pan Nengyuan <pannengyuan@huawei.com> > >> > >> Devices tend to maintain vq pointers, allow deleting them trough a vq > >> pointer. > >> > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > >> --- > >> Changes v2 to v1: > >> - add a new function virtio_delete_queue to cleanup vq through a vq > >> pointer > >> --- > >> hw/virtio/virtio.c | 16 +++++++++++----- > >> include/hw/virtio/virtio.h | 2 ++ > >> 2 files changed, 13 insertions(+), 5 deletions(-) > >> > >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >> index 04716b5..6de3cfd 100644 > >> --- a/hw/virtio/virtio.c > >> +++ b/hw/virtio/virtio.c > >> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, > >> int > >> queue_size, > >> return &vdev->vq[i]; > >> } > >> > >> +void virtio_delete_queue(VirtQueue *vq) > >> +{ > >> + vq->vring.num = 0; > >> + vq->vring.num_default = 0; > >> + vq->handle_output = NULL; > >> + vq->handle_aio_output = NULL; > >> + g_free(vq->used_elems); > >> + vq->used_elems = NULL; > >> +} > >> + > >> void virtio_del_queue(VirtIODevice *vdev, int n) > >> { > >> if (n < 0 || n >= VIRTIO_QUEUE_MAX) { > >> abort(); > >> } > >> > >> - vdev->vq[n].vring.num = 0; > >> - vdev->vq[n].vring.num_default = 0; > >> - vdev->vq[n].handle_output = NULL; > >> - vdev->vq[n].handle_aio_output = NULL; > >> - g_free(vdev->vq[n].used_elems); > >> + virtio_delete_queue(&vdev->vq[n]); > >> } > >> > >> static void virtio_set_isr(VirtIODevice *vdev, int value) > >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > >> index c32a815..e18756d 100644 > >> --- a/include/hw/virtio/virtio.h > >> +++ b/include/hw/virtio/virtio.h > >> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int > >> queue_size, > >> > >> void virtio_del_queue(VirtIODevice *vdev, int n); > >> > >> +void virtio_delete_queue(VirtQueue *vq); > >> + > >> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > >> unsigned int len); > >> void virtqueue_flush(VirtQueue *vq, unsigned int count); > >> -- > >> 2.7.2.windows.1 > >> > >> > > Overall it ooks good to me. > > > > Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function > > We are doing : virtio_del_queue(vdev, 0); > > > > One can directly call "virtio_delete_queue". It can become confusing > > to call multiple functions for same purpose. Instead, Can we make > > "virtio_delete_queue" static inline? > > > yes, It will be a little confused, but I think it will have the same > problem if we make "virtio_delete_queue" static inline. We can directly > call it aslo. （e.g virtio-serial-bus.c virtio-balloon.c). > > How about replacing the function name to make it more clear (e.g > virtio_delete_queue -> virtio_queue_cleanup) ? It's too similar between > "virtio_del_queue" and "virtio_delete_queue". I am just thinking if we need these two separate functions. Yes, changing name of virtio_delete_queue -> virtio_queue_cleanup should be good enough. Thanks, Pankaj > > > Other than that: > > Reviewed-by: Pankaj Gupta <pagupta@redhat.com> > > > >> > >> > > > > > > . > > > > >
On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote: > From: Pan Nengyuan <pannengyuan@huawei.com> Shouldn't this be From: mst? I didn't find a ref to the original patch to confirm if you had to adapt it in any way, though. > Devices tend to maintain vq pointers, allow deleting them trough a vq > pointer. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > --- > Changes v2 to v1: > - add a new function virtio_delete_queue to cleanup vq through a vq > pointer > --- > hw/virtio/virtio.c | 16 +++++++++++----- > include/hw/virtio/virtio.h | 2 ++ > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 04716b5..6de3cfd 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice > *vdev, int queue_size, > return &vdev->vq[i]; > } > > +void virtio_delete_queue(VirtQueue *vq) > +{ > + vq->vring.num = 0; > + vq->vring.num_default = 0; > + vq->handle_output = NULL; > + vq->handle_aio_output = NULL; > + g_free(vq->used_elems); > + vq->used_elems = NULL; > +} > + > void virtio_del_queue(VirtIODevice *vdev, int n) > { > if (n < 0 || n >= VIRTIO_QUEUE_MAX) { > abort(); > } > > - vdev->vq[n].vring.num = 0; > - vdev->vq[n].vring.num_default = 0; > - vdev->vq[n].handle_output = NULL; > - vdev->vq[n].handle_aio_output = NULL; > - g_free(vdev->vq[n].used_elems); > + virtio_delete_queue(&vdev->vq[n]); > } > > static void virtio_set_isr(VirtIODevice *vdev, int value) > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index c32a815..e18756d 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, > int queue_size, > > void virtio_del_queue(VirtIODevice *vdev, int n); > > +void virtio_delete_queue(VirtQueue *vq); > + > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len); > void virtqueue_flush(VirtQueue *vq, unsigned int count);
On 2019/12/6 0:45, Amit Shah wrote: > On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote: >> From: Pan Nengyuan <pannengyuan@huawei.com> > > Shouldn't this be From: mst? > > I didn't find a ref to the original patch to confirm if you had to > adapt it in any way, though. > Here is the original patch: https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00402.html I just change one line(set used_elems to NULL). In next version, I will change function name from virtio_delete_queue to virtio_queue_cleanup (It's too similar between "virtio_del_queue" and "virtio_delete_queue"): https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00877.html. According to these, should I change it in next version? Thanks. >> Devices tend to maintain vq pointers, allow deleting them trough a vq >> pointer. >> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> >> --- >> Changes v2 to v1: >> - add a new function virtio_delete_queue to cleanup vq through a vq >> pointer >> --- >> hw/virtio/virtio.c | 16 +++++++++++----- >> include/hw/virtio/virtio.h | 2 ++ >> 2 files changed, 13 insertions(+), 5 deletions(-) >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 04716b5..6de3cfd 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice >> *vdev, int queue_size, >> return &vdev->vq[i]; >> } >> >> +void virtio_delete_queue(VirtQueue *vq) >> +{ >> + vq->vring.num = 0; >> + vq->vring.num_default = 0; >> + vq->handle_output = NULL; >> + vq->handle_aio_output = NULL; >> + g_free(vq->used_elems); >> + vq->used_elems = NULL; >> +} >> + >> void virtio_del_queue(VirtIODevice *vdev, int n) >> { >> if (n < 0 || n >= VIRTIO_QUEUE_MAX) { >> abort(); >> } >> >> - vdev->vq[n].vring.num = 0; >> - vdev->vq[n].vring.num_default = 0; >> - vdev->vq[n].handle_output = NULL; >> - vdev->vq[n].handle_aio_output = NULL; >> - g_free(vdev->vq[n].used_elems); >> + virtio_delete_queue(&vdev->vq[n]); >> } >> >> static void virtio_set_isr(VirtIODevice *vdev, int value) >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >> index c32a815..e18756d 100644 >> --- a/include/hw/virtio/virtio.h >> +++ b/include/hw/virtio/virtio.h >> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, >> int queue_size, >> >> void virtio_del_queue(VirtIODevice *vdev, int n); >> >> +void virtio_delete_queue(VirtQueue *vq); >> + >> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, >> unsigned int len); >> void virtqueue_flush(VirtQueue *vq, unsigned int count); > > > . >
On Fri, 2019-12-06 at 10:17 +0800, Pan Nengyuan wrote: > On 2019/12/6 0:45, Amit Shah wrote: > > On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote: > > > From: Pan Nengyuan <pannengyuan@huawei.com> > > > > Shouldn't this be From: mst? > > > > I didn't find a ref to the original patch to confirm if you had to > > adapt it in any way, though. > > > > Here is the original > patch: > https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00402.html > > I just change one line(set used_elems to NULL). In next version, I > will > change function name from virtio_delete_queue to virtio_queue_cleanup > (It's too similar between "virtio_del_queue" and > "virtio_delete_queue"): > https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00877.html > . > > According to these, should I change it in next version? Sure, please change it. Please ensure 'From:' is Michael, but in the sign-off area, you can mention how you changed the original patch, e.g. see the "[PMM: ...]" in https://lore.kernel.org/qemu-devel/20191126141239.8219-5-peter.maydell@linaro.org/ Also, please CC me on the entire series. Thanks,
On 2019/12/6 16:56, Amit Shah wrote: > On Fri, 2019-12-06 at 10:17 +0800, Pan Nengyuan wrote: >> On 2019/12/6 0:45, Amit Shah wrote: >>> On Wed, 2019-12-04 at 15:31 +0800, pannengyuan@huawei.com wrote: >>>> From: Pan Nengyuan <pannengyuan@huawei.com> >>> >>> Shouldn't this be From: mst? >>> >>> I didn't find a ref to the original patch to confirm if you had to >>> adapt it in any way, though. >>> >> >> Here is the original >> patch: >> https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00402.html >> >> I just change one line(set used_elems to NULL). In next version, I >> will >> change function name from virtio_delete_queue to virtio_queue_cleanup >> (It's too similar between "virtio_del_queue" and >> "virtio_delete_queue"): >> > https://lists.nongnu.org/archive/html/qemu-devel/2019-12/msg00877.html >> . >> >> According to these, should I change it in next version? > > Sure, please change it. Please ensure 'From:' is Michael, but in the > sign-off area, you can mention how you changed the original patch, e.g. > see the "[PMM: ...]" in > > > > https://lore.kernel.org/qemu-devel/20191126141239.8219-5-peter.maydell@linaro.org/ > > Also, please CC me on the entire series. > > Thanks, > > OK, I will change it. Thanks. > > . >
On Wed, Dec 04, 2019 at 03:33:07AM -0500, Pankaj Gupta wrote: > > > From: Pan Nengyuan <pannengyuan@huawei.com> > > > > Devices tend to maintain vq pointers, allow deleting them trough a vq > > pointer. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > > --- > > Changes v2 to v1: > > - add a new function virtio_delete_queue to cleanup vq through a vq pointer > > --- > > hw/virtio/virtio.c | 16 +++++++++++----- > > include/hw/virtio/virtio.h | 2 ++ > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index 04716b5..6de3cfd 100644 > > --- a/hw/virtio/virtio.c > > +++ b/hw/virtio/virtio.c > > @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int > > queue_size, > > return &vdev->vq[i]; > > } > > > > +void virtio_delete_queue(VirtQueue *vq) > > +{ > > + vq->vring.num = 0; > > + vq->vring.num_default = 0; > > + vq->handle_output = NULL; > > + vq->handle_aio_output = NULL; > > + g_free(vq->used_elems); > > + vq->used_elems = NULL; > > +} > > + > > void virtio_del_queue(VirtIODevice *vdev, int n) > > { > > if (n < 0 || n >= VIRTIO_QUEUE_MAX) { > > abort(); > > } > > > > - vdev->vq[n].vring.num = 0; > > - vdev->vq[n].vring.num_default = 0; > > - vdev->vq[n].handle_output = NULL; > > - vdev->vq[n].handle_aio_output = NULL; > > - g_free(vdev->vq[n].used_elems); > > + virtio_delete_queue(&vdev->vq[n]); > > } > > > > static void virtio_set_isr(VirtIODevice *vdev, int value) > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > index c32a815..e18756d 100644 > > --- a/include/hw/virtio/virtio.h > > +++ b/include/hw/virtio/virtio.h > > @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int > > queue_size, > > > > void virtio_del_queue(VirtIODevice *vdev, int n); > > > > +void virtio_delete_queue(VirtQueue *vq); > > + > > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > > unsigned int len); > > void virtqueue_flush(VirtQueue *vq, unsigned int count); > > -- > > 2.7.2.windows.1 > > > > > Overall it ooks good to me. > > Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function > We are doing : virtio_del_queue(vdev, 0); Yea. Let's just bite the bullet and convert all callers. Not so many of them. > One can directly call "virtio_delete_queue". It can become confusing > to call multiple functions for same purpose. Instead, Can we make > "virtio_delete_queue" static inline? We can't really. > Other than that: > Reviewed-by: Pankaj Gupta <pagupta@redhat.com> > > > > >
On Wed, Dec 04, 2019 at 03:31:54PM +0800, pannengyuan@huawei.com wrote: > From: Pan Nengyuan <pannengyuan@huawei.com> > > Devices tend to maintain vq pointers, allow deleting them trough a vq pointer. You want to also mention something about clearing .used_elems to avoid chances of double free. > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> So let's just name the new one virtio_del_queue then, and drop the old one. > --- > Changes v2 to v1: > - add a new function virtio_delete_queue to cleanup vq through a vq pointer > --- > hw/virtio/virtio.c | 16 +++++++++++----- > include/hw/virtio/virtio.h | 2 ++ > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 04716b5..6de3cfd 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, > return &vdev->vq[i]; > } > > +void virtio_delete_queue(VirtQueue *vq) > +{ > + vq->vring.num = 0; > + vq->vring.num_default = 0; > + vq->handle_output = NULL; > + vq->handle_aio_output = NULL; > + g_free(vq->used_elems); > + vq->used_elems = NULL; > +} > + > void virtio_del_queue(VirtIODevice *vdev, int n) > { > if (n < 0 || n >= VIRTIO_QUEUE_MAX) { > abort(); > } > > - vdev->vq[n].vring.num = 0; > - vdev->vq[n].vring.num_default = 0; > - vdev->vq[n].handle_output = NULL; > - vdev->vq[n].handle_aio_output = NULL; > - g_free(vdev->vq[n].used_elems); > + virtio_delete_queue(&vdev->vq[n]); > } > > static void virtio_set_isr(VirtIODevice *vdev, int value) > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index c32a815..e18756d 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, > > void virtio_del_queue(VirtIODevice *vdev, int n); > > +void virtio_delete_queue(VirtQueue *vq); > + > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len); > void virtqueue_flush(VirtQueue *vq, unsigned int count); > -- > 2.7.2.windows.1 >
On Mon, Dec 09, 2019 at 11:43:20AM -0500, Michael S. Tsirkin wrote: > On Wed, Dec 04, 2019 at 03:31:54PM +0800, pannengyuan@huawei.com wrote: > > From: Pan Nengyuan <pannengyuan@huawei.com> > > > > Devices tend to maintain vq pointers, allow deleting them trough a vq pointer. > > You want to also mention something about clearing > .used_elems to avoid chances of double free. > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > > > So let's just name the new one virtio_del_queue then, > and drop the old one. I tried but it seems like too much work. > > > --- > > Changes v2 to v1: > > - add a new function virtio_delete_queue to cleanup vq through a vq pointer > > --- > > hw/virtio/virtio.c | 16 +++++++++++----- > > include/hw/virtio/virtio.h | 2 ++ > > 2 files changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index 04716b5..6de3cfd 100644 > > --- a/hw/virtio/virtio.c > > +++ b/hw/virtio/virtio.c > > @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, > > return &vdev->vq[i]; > > } > > > > +void virtio_delete_queue(VirtQueue *vq) > > +{ > > + vq->vring.num = 0; > > + vq->vring.num_default = 0; > > + vq->handle_output = NULL; > > + vq->handle_aio_output = NULL; > > + g_free(vq->used_elems); > > + vq->used_elems = NULL; > > +} > > + > > void virtio_del_queue(VirtIODevice *vdev, int n) > > { > > if (n < 0 || n >= VIRTIO_QUEUE_MAX) { > > abort(); > > } > > > > - vdev->vq[n].vring.num = 0; > > - vdev->vq[n].vring.num_default = 0; > > - vdev->vq[n].handle_output = NULL; > > - vdev->vq[n].handle_aio_output = NULL; > > - g_free(vdev->vq[n].used_elems); > > + virtio_delete_queue(&vdev->vq[n]); > > } > > > > static void virtio_set_isr(VirtIODevice *vdev, int value) > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > > index c32a815..e18756d 100644 > > --- a/include/hw/virtio/virtio.h > > +++ b/include/hw/virtio/virtio.h > > @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, > > > > void virtio_del_queue(VirtIODevice *vdev, int n); > > > > +void virtio_delete_queue(VirtQueue *vq); > > + > > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > > unsigned int len); > > void virtqueue_flush(VirtQueue *vq, unsigned int count); > > -- > > 2.7.2.windows.1 > >
On 2019/12/10 0:58, Michael S. Tsirkin wrote: > On Mon, Dec 09, 2019 at 11:43:20AM -0500, Michael S. Tsirkin wrote: >> On Wed, Dec 04, 2019 at 03:31:54PM +0800, pannengyuan@huawei.com wrote: >>> From: Pan Nengyuan <pannengyuan@huawei.com> >>> >>> Devices tend to maintain vq pointers, allow deleting them trough a vq pointer. >> >> You want to also mention something about clearing >> .used_elems to avoid chances of double free. >> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> >> >> >> So let's just name the new one virtio_del_queue then, >> and drop the old one. > > I tried but it seems like too much work. Yes, some of them do not maintain the vq pointer, so can we just rename the virtio_delete_queue to avoid confusion? I have sent a new version before your reply, can you check whether it's appropriate or not? > >> >>> --- >>> Changes v2 to v1: >>> - add a new function virtio_delete_queue to cleanup vq through a vq pointer >>> --- >>> hw/virtio/virtio.c | 16 +++++++++++----- >>> include/hw/virtio/virtio.h | 2 ++ >>> 2 files changed, 13 insertions(+), 5 deletions(-) >>> >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>> index 04716b5..6de3cfd 100644 >>> --- a/hw/virtio/virtio.c >>> +++ b/hw/virtio/virtio.c >>> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, >>> return &vdev->vq[i]; >>> } >>> >>> +void virtio_delete_queue(VirtQueue *vq) >>> +{ >>> + vq->vring.num = 0; >>> + vq->vring.num_default = 0; >>> + vq->handle_output = NULL; >>> + vq->handle_aio_output = NULL; >>> + g_free(vq->used_elems); >>> + vq->used_elems = NULL; >>> +} >>> + >>> void virtio_del_queue(VirtIODevice *vdev, int n) >>> { >>> if (n < 0 || n >= VIRTIO_QUEUE_MAX) { >>> abort(); >>> } >>> >>> - vdev->vq[n].vring.num = 0; >>> - vdev->vq[n].vring.num_default = 0; >>> - vdev->vq[n].handle_output = NULL; >>> - vdev->vq[n].handle_aio_output = NULL; >>> - g_free(vdev->vq[n].used_elems); >>> + virtio_delete_queue(&vdev->vq[n]); >>> } >>> >>> static void virtio_set_isr(VirtIODevice *vdev, int value) >>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >>> index c32a815..e18756d 100644 >>> --- a/include/hw/virtio/virtio.h >>> +++ b/include/hw/virtio/virtio.h >>> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, >>> >>> void virtio_del_queue(VirtIODevice *vdev, int n); >>> >>> +void virtio_delete_queue(VirtQueue *vq); >>> + >>> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, >>> unsigned int len); >>> void virtqueue_flush(VirtQueue *vq, unsigned int count); >>> -- >>> 2.7.2.windows.1 >>> > > > . >
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 04716b5..6de3cfd 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, return &vdev->vq[i]; } +void virtio_delete_queue(VirtQueue *vq) +{ + vq->vring.num = 0; + vq->vring.num_default = 0; + vq->handle_output = NULL; + vq->handle_aio_output = NULL; + g_free(vq->used_elems); + vq->used_elems = NULL; +} + void virtio_del_queue(VirtIODevice *vdev, int n) { if (n < 0 || n >= VIRTIO_QUEUE_MAX) { abort(); } - vdev->vq[n].vring.num = 0; - vdev->vq[n].vring.num_default = 0; - vdev->vq[n].handle_output = NULL; - vdev->vq[n].handle_aio_output = NULL; - g_free(vdev->vq[n].used_elems); + virtio_delete_queue(&vdev->vq[n]); } static void virtio_set_isr(VirtIODevice *vdev, int value) diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index c32a815..e18756d 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size, void virtio_del_queue(VirtIODevice *vdev, int n); +void virtio_delete_queue(VirtQueue *vq); + void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len); void virtqueue_flush(VirtQueue *vq, unsigned int count);