* [PATCH v3 0/3] hw/virtio: Minor housekeeping patches @ 2021-09-06 10:43 Philippe Mathieu-Daudé 2021-09-06 10:43 ` [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock Philippe Mathieu-Daudé ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-09-06 10:43 UTC (permalink / raw) To: qemu-devel Cc: Michael S. Tsirkin, Jason Wang, Cornelia Huck, Stefan Hajnoczi, Paolo Bonzini, Philippe Mathieu-Daudé, Stefano Garzarella Hi, This series contains few patches I gathered while tooking notes trying to understand issues #300-#302. Since v2: - Rebased on top of 88afdc92b64 ("Merge 'remotes/mst/tags/for_upstream' into staging") Since v1: - Added virtqueue_flush comment (Stefano) - Call RCU_READ_LOCK_GUARD in virtqueue_packed_drop_all (Stefano) Philippe Mathieu-Daudé (3): hw/virtio: Comment virtqueue_flush() must be called with RCU read lock hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all() hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees include/hw/virtio/virtio.h | 7 +++++++ hw/virtio/virtio.c | 32 +++++++++++++++----------------- 2 files changed, 22 insertions(+), 17 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock 2021-09-06 10:43 [PATCH v3 0/3] hw/virtio: Minor housekeeping patches Philippe Mathieu-Daudé @ 2021-09-06 10:43 ` Philippe Mathieu-Daudé 2021-09-27 10:18 ` Cornelia Huck 2021-09-06 10:43 ` [PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all() Philippe Mathieu-Daudé ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-09-06 10:43 UTC (permalink / raw) To: qemu-devel Cc: Michael S. Tsirkin, Jason Wang, Cornelia Huck, Stefan Hajnoczi, Paolo Bonzini, Philippe Mathieu-Daudé, Stefano Garzarella Reported-by: Stefano Garzarella <sgarzare@redhat.com> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- include/hw/virtio/virtio.h | 7 +++++++ hw/virtio/virtio.c | 1 + 2 files changed, 8 insertions(+) diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 8bab9cfb750..c1c5f6e53c8 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq); void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len); +/** + * virtqueue_flush: + * @vq: The #VirtQueue + * @count: Number of elements to flush + * + * Must be called within RCU critical section. + */ void virtqueue_flush(VirtQueue *vq, unsigned int count); void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem, unsigned int len); diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 3a1f6c520cb..97f60017466 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -896,6 +896,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) } } +/* Called within rcu_read_lock(). */ void virtqueue_flush(VirtQueue *vq, unsigned int count) { if (virtio_device_disabled(vq->vdev)) { -- 2.31.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock 2021-09-06 10:43 ` [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock Philippe Mathieu-Daudé @ 2021-09-27 10:18 ` Cornelia Huck 2021-09-27 11:21 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 13+ messages in thread From: Cornelia Huck @ 2021-09-27 10:18 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel Cc: Michael S. Tsirkin, Jason Wang, Stefan Hajnoczi, Paolo Bonzini, Philippe Mathieu-Daudé, Stefano Garzarella On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Reported-by: Stefano Garzarella <sgarzare@redhat.com> > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > include/hw/virtio/virtio.h | 7 +++++++ > hw/virtio/virtio.c | 1 + > 2 files changed, 8 insertions(+) > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > index 8bab9cfb750..c1c5f6e53c8 100644 > --- a/include/hw/virtio/virtio.h > +++ b/include/hw/virtio/virtio.h > @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq); > > void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len); > +/** > + * virtqueue_flush: > + * @vq: The #VirtQueue > + * @count: Number of elements to flush > + * > + * Must be called within RCU critical section. > + */ Hm... do these doc comments belong into .h files, or rather into .c files? > void virtqueue_flush(VirtQueue *vq, unsigned int count); > void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem, > unsigned int len); > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 3a1f6c520cb..97f60017466 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -896,6 +896,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) > } > } > > +/* Called within rcu_read_lock(). */ > void virtqueue_flush(VirtQueue *vq, unsigned int count) > { > if (virtio_device_disabled(vq->vdev)) { The content of the change looks good to me, I'm only wondering about the style... ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock 2021-09-27 10:18 ` Cornelia Huck @ 2021-09-27 11:21 ` Philippe Mathieu-Daudé 2021-09-27 11:29 ` Cornelia Huck 0 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-09-27 11:21 UTC (permalink / raw) To: Cornelia Huck, qemu-devel, Peter Maydell, Markus Armbruster, Eric Blake, Daniel P . Berrange Cc: Michael S. Tsirkin, Jason Wang, Richard Henderson, Stefan Hajnoczi, Marc-André Lureau, Paolo Bonzini, Stefano Garzarella On 9/27/21 12:18, Cornelia Huck wrote: > On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> Reported-by: Stefano Garzarella <sgarzare@redhat.com> >> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> include/hw/virtio/virtio.h | 7 +++++++ >> hw/virtio/virtio.c | 1 + >> 2 files changed, 8 insertions(+) >> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >> index 8bab9cfb750..c1c5f6e53c8 100644 >> --- a/include/hw/virtio/virtio.h >> +++ b/include/hw/virtio/virtio.h >> @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq); >> >> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, >> unsigned int len); >> +/** >> + * virtqueue_flush: >> + * @vq: The #VirtQueue >> + * @count: Number of elements to flush >> + * >> + * Must be called within RCU critical section. >> + */ > > Hm... do these doc comments belong into .h files, or rather into .c files? Maybe we should restart this old thread, vote(?) and settle on a style? https://lore.kernel.org/qemu-devel/349cd87b-0526-30b8-d9cd-0eee537ab5a4@redhat.com/ >> void virtqueue_flush(VirtQueue *vq, unsigned int count); >> void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem, >> unsigned int len); >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 3a1f6c520cb..97f60017466 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -896,6 +896,7 @@ static void virtqueue_packed_flush(VirtQueue *vq, unsigned int count) >> } >> } >> >> +/* Called within rcu_read_lock(). */ >> void virtqueue_flush(VirtQueue *vq, unsigned int count) >> { >> if (virtio_device_disabled(vq->vdev)) { > > The content of the change looks good to me, I'm only wondering about > the style... > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock 2021-09-27 11:21 ` Philippe Mathieu-Daudé @ 2021-09-27 11:29 ` Cornelia Huck 2021-10-04 9:19 ` Stefan Hajnoczi 0 siblings, 1 reply; 13+ messages in thread From: Cornelia Huck @ 2021-09-27 11:29 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel, Peter Maydell, Markus Armbruster, Eric Blake, Daniel P . Berrange Cc: Michael S. Tsirkin, Jason Wang, Richard Henderson, Stefan Hajnoczi, Marc-André Lureau, Paolo Bonzini, Stefano Garzarella On Mon, Sep 27 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 9/27/21 12:18, Cornelia Huck wrote: >> On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >>> Reported-by: Stefano Garzarella <sgarzare@redhat.com> >>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >>> --- >>> include/hw/virtio/virtio.h | 7 +++++++ >>> hw/virtio/virtio.c | 1 + >>> 2 files changed, 8 insertions(+) >>> >>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h >>> index 8bab9cfb750..c1c5f6e53c8 100644 >>> --- a/include/hw/virtio/virtio.h >>> +++ b/include/hw/virtio/virtio.h >>> @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq); >>> >>> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, >>> unsigned int len); >>> +/** >>> + * virtqueue_flush: >>> + * @vq: The #VirtQueue >>> + * @count: Number of elements to flush >>> + * >>> + * Must be called within RCU critical section. >>> + */ >> >> Hm... do these doc comments belong into .h files, or rather into .c files? > > Maybe we should restart this old thread, vote(?) and settle on a style? > > https://lore.kernel.org/qemu-devel/349cd87b-0526-30b8-d9cd-0eee537ab5a4@redhat.com/ My vote would still go to putting this into .c files :) Not sure if we want to go through the hassle of a wholesale cleanup; but if others agree, we could at least start with putting new doc comments next to the implementation. Do we actually consume these comments in an automated way somewhere? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock 2021-09-27 11:29 ` Cornelia Huck @ 2021-10-04 9:19 ` Stefan Hajnoczi 0 siblings, 0 replies; 13+ messages in thread From: Stefan Hajnoczi @ 2021-10-04 9:19 UTC (permalink / raw) To: Cornelia Huck Cc: Peter Maydell, Daniel P . Berrange, Michael S. Tsirkin, Eric Blake, Jason Wang, Richard Henderson, qemu-devel, Markus Armbruster, Marc-André Lureau, Paolo Bonzini, Philippe Mathieu-Daudé, Stefano Garzarella [-- Attachment #1: Type: text/plain, Size: 1969 bytes --] On Mon, Sep 27, 2021 at 01:29:46PM +0200, Cornelia Huck wrote: > On Mon, Sep 27 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > > On 9/27/21 12:18, Cornelia Huck wrote: > >> On Mon, Sep 06 2021, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > >> > >>> Reported-by: Stefano Garzarella <sgarzare@redhat.com> > >>> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >>> --- > >>> include/hw/virtio/virtio.h | 7 +++++++ > >>> hw/virtio/virtio.c | 1 + > >>> 2 files changed, 8 insertions(+) > >>> > >>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h > >>> index 8bab9cfb750..c1c5f6e53c8 100644 > >>> --- a/include/hw/virtio/virtio.h > >>> +++ b/include/hw/virtio/virtio.h > >>> @@ -186,6 +186,13 @@ void virtio_delete_queue(VirtQueue *vq); > >>> > >>> void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem, > >>> unsigned int len); > >>> +/** > >>> + * virtqueue_flush: > >>> + * @vq: The #VirtQueue > >>> + * @count: Number of elements to flush > >>> + * > >>> + * Must be called within RCU critical section. > >>> + */ > >> > >> Hm... do these doc comments belong into .h files, or rather into .c files? > > > > Maybe we should restart this old thread, vote(?) and settle on a style? > > > > https://lore.kernel.org/qemu-devel/349cd87b-0526-30b8-d9cd-0eee537ab5a4@redhat.com/ > > My vote would still go to putting this into .c files :) Not sure if we > want to go through the hassle of a wholesale cleanup; but if others > agree, we could at least start with putting new doc comments next to the > implementation. In the virtio.c/h case doc comments (and especially the RCU-related ones) are in the .c file. The exception is that constants and structs are documented in the header file. I would follow that and avoid duplicating doc comments into the .h file. Stefan [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all() 2021-09-06 10:43 [PATCH v3 0/3] hw/virtio: Minor housekeeping patches Philippe Mathieu-Daudé 2021-09-06 10:43 ` [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock Philippe Mathieu-Daudé @ 2021-09-06 10:43 ` Philippe Mathieu-Daudé 2021-10-04 9:23 ` Stefan Hajnoczi 2021-09-06 10:43 ` [PATCH v3 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees Philippe Mathieu-Daudé 2021-09-07 13:49 ` [PATCH v3 0/3] hw/virtio: Minor housekeeping patches Stefano Garzarella 3 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-09-06 10:43 UTC (permalink / raw) To: qemu-devel Cc: Michael S. Tsirkin, Jason Wang, Cornelia Huck, Stefan Hajnoczi, Paolo Bonzini, Philippe Mathieu-Daudé, Stefano Garzarella vring_get_region_caches() must be called with the RCU read lock acquired. virtqueue_packed_drop_all() does not, and uses the 'caches' pointer. Fix that by using the RCU_READ_LOCK_GUARD() macro. Reported-by: Stefano Garzarella <sgarzare@redhat.com> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/virtio/virtio.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 97f60017466..7d3bf9091ee 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1704,6 +1704,8 @@ static unsigned int virtqueue_packed_drop_all(VirtQueue *vq) VirtIODevice *vdev = vq->vdev; VRingPackedDesc desc; + RCU_READ_LOCK_GUARD(); + caches = vring_get_region_caches(vq); if (!caches) { return 0; -- 2.31.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all() 2021-09-06 10:43 ` [PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all() Philippe Mathieu-Daudé @ 2021-10-04 9:23 ` Stefan Hajnoczi 2021-10-04 9:27 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 13+ messages in thread From: Stefan Hajnoczi @ 2021-10-04 9:23 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Michael S. Tsirkin, Jason Wang, Cornelia Huck, qemu-devel, Paolo Bonzini, Stefano Garzarella [-- Attachment #1: Type: text/plain, Size: 1110 bytes --] On Mon, Sep 06, 2021 at 12:43:17PM +0200, Philippe Mathieu-Daudé wrote: > vring_get_region_caches() must be called with the RCU read lock > acquired. virtqueue_packed_drop_all() does not, and uses the > 'caches' pointer. Fix that by using the RCU_READ_LOCK_GUARD() > macro. Is this a bug that has been encountered, is it a latent bug, a code cleanup, etc? The impact of this isn't clear but it sounds a little scary so I wanted to check. > > Reported-by: Stefano Garzarella <sgarzare@redhat.com> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/virtio/virtio.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 97f60017466..7d3bf9091ee 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -1704,6 +1704,8 @@ static unsigned int virtqueue_packed_drop_all(VirtQueue *vq) > VirtIODevice *vdev = vq->vdev; > VRingPackedDesc desc; > > + RCU_READ_LOCK_GUARD(); > + > caches = vring_get_region_caches(vq); > if (!caches) { > return 0; > -- > 2.31.1 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all() 2021-10-04 9:23 ` Stefan Hajnoczi @ 2021-10-04 9:27 ` Philippe Mathieu-Daudé 2021-10-05 11:42 ` Stefano Garzarella 0 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-10-04 9:27 UTC (permalink / raw) To: Stefan Hajnoczi Cc: Michael S. Tsirkin, Jason Wang, Cornelia Huck, qemu-devel, Paolo Bonzini, Stefano Garzarella On 10/4/21 11:23, Stefan Hajnoczi wrote: > On Mon, Sep 06, 2021 at 12:43:17PM +0200, Philippe Mathieu-Daudé wrote: >> vring_get_region_caches() must be called with the RCU read lock >> acquired. virtqueue_packed_drop_all() does not, and uses the >> 'caches' pointer. Fix that by using the RCU_READ_LOCK_GUARD() >> macro. > > Is this a bug that has been encountered, is it a latent bug, a code > cleanup, etc? The impact of this isn't clear but it sounds a little > scary so I wanted to check. I'll defer to Stefano, but IIUC it is a latent bug discovered during code audit. > >> >> Reported-by: Stefano Garzarella <sgarzare@redhat.com> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> hw/virtio/virtio.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 97f60017466..7d3bf9091ee 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -1704,6 +1704,8 @@ static unsigned int virtqueue_packed_drop_all(VirtQueue *vq) >> VirtIODevice *vdev = vq->vdev; >> VRingPackedDesc desc; >> >> + RCU_READ_LOCK_GUARD(); >> + >> caches = vring_get_region_caches(vq); >> if (!caches) { >> return 0; >> -- >> 2.31.1 >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all() 2021-10-04 9:27 ` Philippe Mathieu-Daudé @ 2021-10-05 11:42 ` Stefano Garzarella 0 siblings, 0 replies; 13+ messages in thread From: Stefano Garzarella @ 2021-10-05 11:42 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Michael S. Tsirkin, Jason Wang, Cornelia Huck, qemu-devel, Stefan Hajnoczi, Paolo Bonzini On Mon, Oct 04, 2021 at 11:27:12AM +0200, Philippe Mathieu-Daudé wrote: >On 10/4/21 11:23, Stefan Hajnoczi wrote: >> On Mon, Sep 06, 2021 at 12:43:17PM +0200, Philippe Mathieu-Daudé wrote: >>> vring_get_region_caches() must be called with the RCU read lock >>> acquired. virtqueue_packed_drop_all() does not, and uses the >>> 'caches' pointer. Fix that by using the RCU_READ_LOCK_GUARD() >>> macro. >> >> Is this a bug that has been encountered, is it a latent bug, a code >> cleanup, etc? The impact of this isn't clear but it sounds a little >> scary so I wanted to check. > >I'll defer to Stefano, but IIUC it is a latent bug discovered >during code audit. Yep, I confirm this. We discovered it by discussing the documentation in a previous series. Thanks, Stefano ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees 2021-09-06 10:43 [PATCH v3 0/3] hw/virtio: Minor housekeeping patches Philippe Mathieu-Daudé 2021-09-06 10:43 ` [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock Philippe Mathieu-Daudé 2021-09-06 10:43 ` [PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all() Philippe Mathieu-Daudé @ 2021-09-06 10:43 ` Philippe Mathieu-Daudé 2021-10-04 9:24 ` Stefan Hajnoczi 2021-09-07 13:49 ` [PATCH v3 0/3] hw/virtio: Minor housekeeping patches Stefano Garzarella 3 siblings, 1 reply; 13+ messages in thread From: Philippe Mathieu-Daudé @ 2021-09-06 10:43 UTC (permalink / raw) To: qemu-devel Cc: Michael S. Tsirkin, Jason Wang, Cornelia Huck, Stefan Hajnoczi, Paolo Bonzini, Philippe Mathieu-Daudé, Stefano Garzarella Both virtqueue_packed_get_avail_bytes() and virtqueue_split_get_avail_bytes() access the region cache, but their caller also does. Simplify by having virtqueue_get_avail_bytes calling both with RCU lock held, and passing the caches as argument. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/virtio/virtio.c | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 7d3bf9091ee..0dbfb53e51b 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -985,28 +985,23 @@ static int virtqueue_split_read_next_desc(VirtIODevice *vdev, VRingDesc *desc, return VIRTQUEUE_READ_DESC_MORE; } +/* Called within rcu_read_lock(). */ static void virtqueue_split_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, unsigned int *out_bytes, - unsigned max_in_bytes, unsigned max_out_bytes) + unsigned max_in_bytes, unsigned max_out_bytes, + VRingMemoryRegionCaches *caches) { VirtIODevice *vdev = vq->vdev; unsigned int max, idx; unsigned int total_bufs, in_total, out_total; - VRingMemoryRegionCaches *caches; MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID; int64_t len = 0; int rc; - RCU_READ_LOCK_GUARD(); - idx = vq->last_avail_idx; total_bufs = in_total = out_total = 0; max = vq->vring.num; - caches = vring_get_region_caches(vq); - if (!caches) { - goto err; - } while ((rc = virtqueue_num_heads(vq, idx)) > 0) { MemoryRegionCache *desc_cache = &caches->desc; @@ -1125,32 +1120,28 @@ static int virtqueue_packed_read_next_desc(VirtQueue *vq, return VIRTQUEUE_READ_DESC_MORE; } +/* Called within rcu_read_lock(). */ static void virtqueue_packed_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, unsigned int *out_bytes, unsigned max_in_bytes, - unsigned max_out_bytes) + unsigned max_out_bytes, + VRingMemoryRegionCaches *caches) { VirtIODevice *vdev = vq->vdev; unsigned int max, idx; unsigned int total_bufs, in_total, out_total; MemoryRegionCache *desc_cache; - VRingMemoryRegionCaches *caches; MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID; int64_t len = 0; VRingPackedDesc desc; bool wrap_counter; - RCU_READ_LOCK_GUARD(); idx = vq->last_avail_idx; wrap_counter = vq->last_avail_wrap_counter; total_bufs = in_total = out_total = 0; max = vq->vring.num; - caches = vring_get_region_caches(vq); - if (!caches) { - goto err; - } for (;;) { unsigned int num_bufs = total_bufs; @@ -1251,6 +1242,8 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, uint16_t desc_size; VRingMemoryRegionCaches *caches; + RCU_READ_LOCK_GUARD(); + if (unlikely(!vq->vring.desc)) { goto err; } @@ -1269,10 +1262,12 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes, if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) { virtqueue_packed_get_avail_bytes(vq, in_bytes, out_bytes, - max_in_bytes, max_out_bytes); + max_in_bytes, max_out_bytes, + caches); } else { virtqueue_split_get_avail_bytes(vq, in_bytes, out_bytes, - max_in_bytes, max_out_bytes); + max_in_bytes, max_out_bytes, + caches); } return; -- 2.31.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees 2021-09-06 10:43 ` [PATCH v3 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees Philippe Mathieu-Daudé @ 2021-10-04 9:24 ` Stefan Hajnoczi 0 siblings, 0 replies; 13+ messages in thread From: Stefan Hajnoczi @ 2021-10-04 9:24 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Michael S. Tsirkin, Jason Wang, Cornelia Huck, qemu-devel, Paolo Bonzini, Stefano Garzarella [-- Attachment #1: Type: text/plain, Size: 568 bytes --] On Mon, Sep 06, 2021 at 12:43:18PM +0200, Philippe Mathieu-Daudé wrote: > Both virtqueue_packed_get_avail_bytes() and > virtqueue_split_get_avail_bytes() access the region cache, but > their caller also does. Simplify by having virtqueue_get_avail_bytes > calling both with RCU lock held, and passing the caches as argument. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > hw/virtio/virtio.c | 29 ++++++++++++----------------- > 1 file changed, 12 insertions(+), 17 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 0/3] hw/virtio: Minor housekeeping patches 2021-09-06 10:43 [PATCH v3 0/3] hw/virtio: Minor housekeeping patches Philippe Mathieu-Daudé ` (2 preceding siblings ...) 2021-09-06 10:43 ` [PATCH v3 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees Philippe Mathieu-Daudé @ 2021-09-07 13:49 ` Stefano Garzarella 3 siblings, 0 replies; 13+ messages in thread From: Stefano Garzarella @ 2021-09-07 13:49 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Michael S. Tsirkin, Jason Wang, Cornelia Huck, qemu-devel, Stefan Hajnoczi, Paolo Bonzini On Mon, Sep 06, 2021 at 12:43:15PM +0200, Philippe Mathieu-Daudé wrote: >Hi, > >This series contains few patches I gathered while tooking notes >trying to understand issues #300-#302. > >Since v2: >- Rebased on top of 88afdc92b64 ("Merge 'remotes/mst/tags/for_upstream' into staging") > >Since v1: >- Added virtqueue_flush comment (Stefano) >- Call RCU_READ_LOCK_GUARD in virtqueue_packed_drop_all (Stefano) > >Philippe Mathieu-Daudé (3): > hw/virtio: Comment virtqueue_flush() must be called with RCU read lock > hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all() > hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees > > include/hw/virtio/virtio.h | 7 +++++++ > hw/virtio/virtio.c | 32 +++++++++++++++----------------- > 2 files changed, 22 insertions(+), 17 deletions(-) > >-- >2.31.1 > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-10-05 11:49 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-09-06 10:43 [PATCH v3 0/3] hw/virtio: Minor housekeeping patches Philippe Mathieu-Daudé 2021-09-06 10:43 ` [PATCH v3 1/3] hw/virtio: Comment virtqueue_flush() must be called with RCU read lock Philippe Mathieu-Daudé 2021-09-27 10:18 ` Cornelia Huck 2021-09-27 11:21 ` Philippe Mathieu-Daudé 2021-09-27 11:29 ` Cornelia Huck 2021-10-04 9:19 ` Stefan Hajnoczi 2021-09-06 10:43 ` [PATCH v3 2/3] hw/virtio: Acquire RCU read lock in virtqueue_packed_drop_all() Philippe Mathieu-Daudé 2021-10-04 9:23 ` Stefan Hajnoczi 2021-10-04 9:27 ` Philippe Mathieu-Daudé 2021-10-05 11:42 ` Stefano Garzarella 2021-09-06 10:43 ` [PATCH v3 3/3] hw/virtio: Have virtqueue_get_avail_bytes() pass caches arg to callees Philippe Mathieu-Daudé 2021-10-04 9:24 ` Stefan Hajnoczi 2021-09-07 13:49 ` [PATCH v3 0/3] hw/virtio: Minor housekeeping patches Stefano Garzarella
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).