[PATCH] guestcopy: evaluate {, __}copy{, _field}_to_guest*() arguments just once

[PATCH] guestcopy: evaluate {, __}copy{, _field}_to_guest*() arguments just once

[Jan Beulich]

There's nothing wrong with having e.g.

    copy_to_guest(uarg, ptr++, 1);

yet until now this would increment "ptr" twice.

Also drop a pair of unneeded parentheses from every instance at this
occasion.

Fixes: b7954cc59831 ("Enhance guest memory accessor macros so that source operands can be")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Arm side untested so far, as I don't have all the tool chain pieces
available at home.
---
This goes on top of the assumed v2 of Julien's "xen/guest_access: Harden
copy_to_guest_offset to prevent const dest operand".

--- a/xen/include/asm-arm/guest_access.h
+++ b/xen/include/asm-arm/guest_access.h
@@ -79,7 +79,7 @@ int access_guest_memory_by_ipa(struct do
     const typeof(*(ptr)) *_s = (ptr);                   \
     char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
     void *__maybe_unused _t = (hnd).p;                  \
-    ((void)((hnd).p == (ptr)));                         \
+    (void)((hnd).p == _s);                              \
     raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
 })
 
@@ -106,7 +106,7 @@ int access_guest_memory_by_ipa(struct do
 #define copy_field_to_guest(hnd, ptr, field) ({         \
     const typeof(&(ptr)->field) _s = &(ptr)->field;     \
     void *_d = &(hnd).p->field;                         \
-    ((void)(&(hnd).p->field == &(ptr)->field));         \
+    (void)(&(hnd).p->field == _s);                      \
     raw_copy_to_guest(_d, _s, sizeof(*_s));             \
 })
 
@@ -129,7 +129,7 @@ int access_guest_memory_by_ipa(struct do
     const typeof(*(ptr)) *_s = (ptr);                   \
     char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
     void *__maybe_unused _t = (hnd).p;                  \
-    ((void)((hnd).p == (ptr)));                         \
+    (void)((hnd).p == _s);                              \
     __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
 })
 
@@ -146,7 +146,7 @@ int access_guest_memory_by_ipa(struct do
 #define __copy_field_to_guest(hnd, ptr, field) ({       \
     const typeof(&(ptr)->field) _s = &(ptr)->field;     \
     void *_d = &(hnd).p->field;                         \
-    ((void)(&(hnd).p->field == &(ptr)->field));         \
+    (void)(&(hnd).p->field == _s);                      \
     __raw_copy_to_guest(_d, _s, sizeof(*_s));           \
 })
 
--- a/xen/include/asm-x86/guest_access.h
+++ b/xen/include/asm-x86/guest_access.h
@@ -88,7 +88,7 @@
     const typeof(*(ptr)) *_s = (ptr);                   \
     char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
     void *__maybe_unused _t = (hnd).p;                  \
-    ((void)((hnd).p == (ptr)));                         \
+    (void)((hnd).p == _s);                              \
     raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));  \
 })
 
@@ -111,7 +111,7 @@
 #define copy_field_to_guest(hnd, ptr, field) ({         \
     const typeof(&(ptr)->field) _s = &(ptr)->field;     \
     void *_d = &(hnd).p->field;                         \
-    ((void)(&(hnd).p->field == &(ptr)->field));         \
+    (void)(&(hnd).p->field == _s);                      \
     raw_copy_to_guest(_d, _s, sizeof(*_s));             \
 })
 
@@ -139,7 +139,7 @@
     const typeof(*(ptr)) *_s = (ptr);                   \
     char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
     void *__maybe_unused _t = (hnd).p;                  \
-    ((void)((hnd).p == (ptr)));                         \
+    (void)((hnd).p == _s);                              \
     __raw_copy_to_guest(_d+(off), _s, sizeof(*_s)*(nr));\
 })
 
@@ -157,7 +157,7 @@
 #define __copy_field_to_guest(hnd, ptr, field) ({       \
     const typeof(&(ptr)->field) _s = &(ptr)->field;     \
     void *_d = &(hnd).p->field;                         \
-    ((void)(&(hnd).p->field == &(ptr)->field));         \
+    (void)(&(hnd).p->field == _s);                      \
     __raw_copy_to_guest(_d, _s, sizeof(*_s));           \
 })
 

Re: [PATCH] guestcopy: evaluate {,__}copy{,_field}_to_guest*() arguments just once

[Julien Grall]

Hi,

On 01/04/2020 15:29, Jan Beulich wrote:
> There's nothing wrong with having e.g.
> 
>      copy_to_guest(uarg, ptr++, 1);
> 
> yet until now this would increment "ptr" twice.

Is there such use in Xen today? I guess not as we would have noticed any 
issue.

> Also drop a pair of unneeded parentheses from every instance at this
> occasion.
> 
> Fixes: b7954cc59831 ("Enhance guest memory accessor macros so that source operands can be")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Arm side untested so far, as I don't have all the tool chain pieces
> available at home.

I will have a look.

> ---
> This goes on top of the assumed v2 of Julien's "xen/guest_access: Harden
> copy_to_guest_offset to prevent const dest operand".
> 
> --- a/xen/include/asm-arm/guest_access.h
> +++ b/xen/include/asm-arm/guest_access.h
> @@ -79,7 +79,7 @@ int access_guest_memory_by_ipa(struct do
>       const typeof(*(ptr)) *_s = (ptr);                   \
>       char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
>       void *__maybe_unused _t = (hnd).p;                  \
> -    ((void)((hnd).p == (ptr)));                         \
> +    (void)((hnd).p == _s);                              \

May I ask why this is a problem with 'ptr' but not 'hnd'? Wouldn't it 
theorically possible to have an array of handle?

Cheers,

-- 
Julien Grall

Re: [PATCH] guestcopy: evaluate {,__}copy{,_field}_to_guest*() arguments just once

[Jan Beulich]

On 01.04.2020 23:28, Julien Grall wrote:
> On 01/04/2020 15:29, Jan Beulich wrote:
>> There's nothing wrong with having e.g.
>>
>>      copy_to_guest(uarg, ptr++, 1);
>>
>> yet until now this would increment "ptr" twice.
> 
> Is there such use in Xen today? I guess not as we would have noticed any issue.

I'm not aware of any.

>> --- a/xen/include/asm-arm/guest_access.h
>> +++ b/xen/include/asm-arm/guest_access.h
>> @@ -79,7 +79,7 @@ int access_guest_memory_by_ipa(struct do
>>       const typeof(*(ptr)) *_s = (ptr);                   \
>>       char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
>>       void *__maybe_unused _t = (hnd).p;                  \
>> -    ((void)((hnd).p == (ptr)));                         \
>> +    (void)((hnd).p == _s);                              \
> 
> May I ask why this is a problem with 'ptr' but not 'hnd'?
> Wouldn't it theorically possible to have an array of handle?

Theoretically yes, but I view issues with the handle as far less
likely than issues with any of the other parameters (in particular
I don't see what an array of handles could be used for). So yes,
if we want to be eager, we could deal with that one as well.

Jan

Re: [PATCH] guestcopy: evaluate {,__}copy{,_field}_to_guest*() arguments just once

[Julien Grall]

Hi Jan,

On 02/04/2020 07:20, Jan Beulich wrote:
> On 01.04.2020 23:28, Julien Grall wrote:
>> On 01/04/2020 15:29, Jan Beulich wrote:
>>> There's nothing wrong with having e.g.
>>>
>>>       copy_to_guest(uarg, ptr++, 1);
>>>
>>> yet until now this would increment "ptr" twice.
>>
>> Is there such use in Xen today? I guess not as we would have noticed any issue.
> 
> I'm not aware of any.

I have looked at the existing callers in staging and can't find such 
pattern as well.

> 
>>> --- a/xen/include/asm-arm/guest_access.h
>>> +++ b/xen/include/asm-arm/guest_access.h
>>> @@ -79,7 +79,7 @@ int access_guest_memory_by_ipa(struct do
>>>        const typeof(*(ptr)) *_s = (ptr);                   \
>>>        char (*_d)[sizeof(*_s)] = (void *)(hnd).p;          \
>>>        void *__maybe_unused _t = (hnd).p;                  \
>>> -    ((void)((hnd).p == (ptr)));                         \
>>> +    (void)((hnd).p == _s);                              \
>>
>> May I ask why this is a problem with 'ptr' but not 'hnd'?
>> Wouldn't it theorically possible to have an array of handle?
> 
> Theoretically yes, but I view issues with the handle as far less
> likely than issues with any of the other parameters (in particular
> I don't see what an array of handles could be used for). So yes,
> if we want to be eager, we could deal with that one as well.

That's a fair point. I am happy either way.

I have also resent my patch (see [1]). This patch still applies on top 
of it and I have compile tested for arm32, arm64 and x86.

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,


[1] https://lore.kernel.org/xen-devel/20200404130613.26428-1-julien@xen.org/

> 
> Jan
> 

-- 
Julien Grall