* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.