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 <marma...@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
