1. Patchew
  2. Xen
  3. View series

[PATCH] xen: Add RING_COPY_RESPONSE()

Marek Marczykowski-Górecki posted 1 patch 3 years, 2 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20210128031653.1640771-1-marmarek@invisiblethingslab.com
There is a newer version of this series
xen/include/public/io/ring.h | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
[PATCH] xen: Add RING_COPY_RESPONSE()
Posted by Marek Marczykowski-Górecki 3 years, 2 months ago 
Using RING_GET_RESPONSE() on a shared ring is easy to use incorrectly
(i.e., by not considering that the other end may alter the data in the
shared ring while it is being inspected). Safe usage of a response
generally requires taking a local copy.

Provide a RING_COPY_RESPONSE() macro to use instead of
RING_GET_RESPONSE() and an open-coded memcpy().  This takes care of
ensuring that the copy is done correctly regardless of any possible
compiler optimizations.

Use a volatile source to prevent the compiler from reordering or
omitting the copy.

This generalizes similar RING_COPY_REQUEST() macro added in 3f20b8def0.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/include/public/io/ring.h | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index d68615ae2f67..b63d7362f0e9 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -231,22 +231,25 @@ typedef struct __name##_back_ring __name##_back_ring_t
 #define RING_GET_REQUEST(_r, _idx)                                      \
     (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req))
 
+#define RING_GET_RESPONSE(_r, _idx)                                     \
+    (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
+
 /*
- * Get a local copy of a request.
+ * Get a local copy of a request/response.
  *
- * Use this in preference to RING_GET_REQUEST() so all processing is
+ * Use this in preference to RING_GET_{REQUEST,RESPONSE}() so all processing is
  * done on a local copy that cannot be modified by the other end.
  *
  * Note that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145 may cause this
  * to be ineffective where _req is a struct which consists of only bitfields.
  */
-#define RING_COPY_REQUEST(_r, _idx, _req) do {				\
+#define RING_COPY_(action, _r, _idx, _req) do {				\
 	/* Use volatile to force the copy into _req. */			\
-	*(_req) = *(volatile typeof(_req))RING_GET_REQUEST(_r, _idx);	\
+	*(_req) = *(volatile typeof(_req))RING_GET_##action(_r, _idx);	\
 } while (0)
 
-#define RING_GET_RESPONSE(_r, _idx)                                     \
-    (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
+#define RING_COPY_REQUEST(_r, _idx, _req)  RING_COPY_(REQUEST, _r, _idx, _req)
+#define RING_COPY_RESPONSE(_r, _idx, _req) RING_COPY_(RESPONSE, _r, _idx, _req)
 
 /* Loop termination condition: Would the specified index overflow the ring? */
 #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)                           \
-- 
2.25.4
Re: [PATCH] xen: Add RING_COPY_RESPONSE()
Posted by Jürgen Groß 3 years, 2 months ago 
On 28.01.21 04:16, Marek Marczykowski-Górecki wrote:
> Using RING_GET_RESPONSE() on a shared ring is easy to use incorrectly
> (i.e., by not considering that the other end may alter the data in the
> shared ring while it is being inspected). Safe usage of a response
> generally requires taking a local copy.
> 
> Provide a RING_COPY_RESPONSE() macro to use instead of
> RING_GET_RESPONSE() and an open-coded memcpy().  This takes care of
> ensuring that the copy is done correctly regardless of any possible
> compiler optimizations.
> 
> Use a volatile source to prevent the compiler from reordering or
> omitting the copy.
> 
> This generalizes similar RING_COPY_REQUEST() macro added in 3f20b8def0.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>   xen/include/public/io/ring.h | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
> index d68615ae2f67..b63d7362f0e9 100644
> --- a/xen/include/public/io/ring.h
> +++ b/xen/include/public/io/ring.h
> @@ -231,22 +231,25 @@ typedef struct __name##_back_ring __name##_back_ring_t
>   #define RING_GET_REQUEST(_r, _idx)                                      \
>       (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req))
>   
> +#define RING_GET_RESPONSE(_r, _idx)                                     \
> +    (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
> +
>   /*
> - * Get a local copy of a request.
> + * Get a local copy of a request/response.
>    *
> - * Use this in preference to RING_GET_REQUEST() so all processing is
> + * Use this in preference to RING_GET_{REQUEST,RESPONSE}() so all processing is
>    * done on a local copy that cannot be modified by the other end.
>    *
>    * Note that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145 may cause this
>    * to be ineffective where _req is a struct which consists of only bitfields.
>    */
> -#define RING_COPY_REQUEST(_r, _idx, _req) do {				\
> +#define RING_COPY_(action, _r, _idx, _req) do {				\

What about renaming _req to _dest in order to reflect that it can be
a request _or_ a response?

"action" is misnamed, IMO. What about "type"?

>   	/* Use volatile to force the copy into _req. */			\
> -	*(_req) = *(volatile typeof(_req))RING_GET_REQUEST(_r, _idx);	\
> +	*(_req) = *(volatile typeof(_req))RING_GET_##action(_r, _idx);	\
>   } while (0)
>   
> -#define RING_GET_RESPONSE(_r, _idx)                                     \
> -    (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
> +#define RING_COPY_REQUEST(_r, _idx, _req)  RING_COPY_(REQUEST, _r, _idx, _req)
> +#define RING_COPY_RESPONSE(_r, _idx, _req) RING_COPY_(RESPONSE, _r, _idx, _req)

Use _rsp instead of _req for RING_COPY_RESPONSE()?


Juergen
Re: [PATCH] xen: Add RING_COPY_RESPONSE()
Posted by Jan Beulich 3 years, 2 months ago 
On 28.01.2021 06:38, Jürgen Groß wrote:
> On 28.01.21 04:16, Marek Marczykowski-Górecki wrote:
>> Using RING_GET_RESPONSE() on a shared ring is easy to use incorrectly
>> (i.e., by not considering that the other end may alter the data in the
>> shared ring while it is being inspected). Safe usage of a response
>> generally requires taking a local copy.
>>
>> Provide a RING_COPY_RESPONSE() macro to use instead of
>> RING_GET_RESPONSE() and an open-coded memcpy().  This takes care of
>> ensuring that the copy is done correctly regardless of any possible
>> compiler optimizations.
>>
>> Use a volatile source to prevent the compiler from reordering or
>> omitting the copy.
>>
>> This generalizes similar RING_COPY_REQUEST() macro added in 3f20b8def0.
>>
>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>> ---
>>   xen/include/public/io/ring.h | 15 +++++++++------
>>   1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
>> index d68615ae2f67..b63d7362f0e9 100644
>> --- a/xen/include/public/io/ring.h
>> +++ b/xen/include/public/io/ring.h
>> @@ -231,22 +231,25 @@ typedef struct __name##_back_ring __name##_back_ring_t
>>   #define RING_GET_REQUEST(_r, _idx)                                      \
>>       (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req))
>>   
>> +#define RING_GET_RESPONSE(_r, _idx)                                     \
>> +    (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
>> +
>>   /*
>> - * Get a local copy of a request.
>> + * Get a local copy of a request/response.
>>    *
>> - * Use this in preference to RING_GET_REQUEST() so all processing is
>> + * Use this in preference to RING_GET_{REQUEST,RESPONSE}() so all processing is
>>    * done on a local copy that cannot be modified by the other end.
>>    *
>>    * Note that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145 may cause this
>>    * to be ineffective where _req is a struct which consists of only bitfields.
>>    */
>> -#define RING_COPY_REQUEST(_r, _idx, _req) do {				\
>> +#define RING_COPY_(action, _r, _idx, _req) do {				\
> 
> What about renaming _req to _dest in order to reflect that it can be
> a request _or_ a response?
> 
> "action" is misnamed, IMO. What about "type"?
> 
>>   	/* Use volatile to force the copy into _req. */			\
>> -	*(_req) = *(volatile typeof(_req))RING_GET_REQUEST(_r, _idx);	\
>> +	*(_req) = *(volatile typeof(_req))RING_GET_##action(_r, _idx);	\
>>   } while (0)
>>   
>> -#define RING_GET_RESPONSE(_r, _idx)                                     \
>> -    (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
>> +#define RING_COPY_REQUEST(_r, _idx, _req)  RING_COPY_(REQUEST, _r, _idx, _req)
>> +#define RING_COPY_RESPONSE(_r, _idx, _req) RING_COPY_(RESPONSE, _r, _idx, _req)
> 
> Use _rsp instead of _req for RING_COPY_RESPONSE()?

And, while at it, get rid of the leading underscores of
macro parameter names wherever possible without extra
code churn?

Jan

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.