* [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
* [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
* [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 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
* 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
* 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 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 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
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.