Function unmap_common_complete defines and sets a variable ld that is
later on passed to a macro gnttab_host_mapping_get_page_type. On arm
this macro does not make use of any arguments causing a compiler to
warn about unused-but-set variable (when -Wunused-but-set-variable is
enabled). Fix this by removing ld and directly passing current->domain
to gnttab_host_mapping_get_page_type.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/common/grant_table.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index febbe12eab..71b1107999 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1556,7 +1556,7 @@ unmap_common(
 static void
 unmap_common_complete(struct gnttab_unmap_common *op)
 {
-    struct domain *ld, *rd = op->rd;
+    struct domain *rd = op->rd;
     struct grant_table *rgt;
     struct active_grant_entry *act;
     grant_entry_header_t *sha;
@@ -1569,8 +1569,6 @@ unmap_common_complete(struct gnttab_unmap_common *op)
         return;
     }
 
-    ld = current->domain;
-
     rcu_lock_domain(rd);
     rgt = rd->grant_table;
 
@@ -1608,7 +1606,7 @@ unmap_common_complete(struct gnttab_unmap_common *op)
         if ( pg )
         {
             if ( gnttab_host_mapping_get_page_type(op->done & GNTMAP_readonly,
-                                                   ld, rd) )
+                                                   current->domain, rd) )
                 put_page_type(pg);
             put_page(pg);
         }
-- 
2.25.1

On 27.04.2022 11:49, Michal Orzel wrote:
> Function unmap_common_complete defines and sets a variable ld that is
> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
> this macro does not make use of any arguments causing a compiler to
> warn about unused-but-set variable (when -Wunused-but-set-variable is
> enabled). Fix this by removing ld and directly passing current->domain
> to gnttab_host_mapping_get_page_type.

I think we want to retain the ld / rd notation. Therefore I think it's
rather the Arm macro which wants adjusting to not leave this argument
unused.

Jan

Hi Jan,

On 27.04.2022 11:59, Jan Beulich wrote:
> On 27.04.2022 11:49, Michal Orzel wrote:
>> Function unmap_common_complete defines and sets a variable ld that is
>> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
>> this macro does not make use of any arguments causing a compiler to
>> warn about unused-but-set variable (when -Wunused-but-set-variable is
>> enabled). Fix this by removing ld and directly passing current->domain
>> to gnttab_host_mapping_get_page_type.
> 
> I think we want to retain the ld / rd notation. Therefore I think it's
> rather the Arm macro which wants adjusting to not leave this argument
> unused.
> 
I would agree provided that the ld variable was used in more than one place.
As it is not, it does not seem very beneficial to keep a variable that is used
just in one place and stores the macro value.

When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0)
so modifying it seems to be a quite big overhead.

> Jan
> 

Cheers,
Michal

On 27/04/2022 12:06, Michal Orzel wrote:
> Hi Jan,
>
> On 27.04.2022 11:59, Jan Beulich wrote:
>> On 27.04.2022 11:49, Michal Orzel wrote:
>>> Function unmap_common_complete defines and sets a variable ld that is
>>> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
>>> this macro does not make use of any arguments causing a compiler to
>>> warn about unused-but-set variable (when -Wunused-but-set-variable is
>>> enabled). Fix this by removing ld and directly passing current->domain
>>> to gnttab_host_mapping_get_page_type.
>> I think we want to retain the ld / rd notation. Therefore I think it's
>> rather the Arm macro which wants adjusting to not leave this argument
>> unused.
>>
> I would agree provided that the ld variable was used in more than one place.
> As it is not, it does not seem very beneficial to keep a variable that is used
> just in one place and stores the macro value.
>
> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0)
> so modifying it seems to be a quite big overhead.

diff --git a/xen/arch/arm/include/asm/grant_table.h
b/xen/arch/arm/include/asm/grant_table.h
index d31a4d6805d6..9f68c2a37eb6 100644
--- a/xen/arch/arm/include/asm/grant_table.h
+++ b/xen/arch/arm/include/asm/grant_table.h
@@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
*d, mfn_t mfn)
 
 int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
                               unsigned int flags, unsigned int
cache_flags);
-#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
+#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
 int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
                                unsigned long new_gpaddr, unsigned int
flags);
-#define gnttab_release_host_mappings(domain) 1
+#define gnttab_release_host_mappings(domain) (domain, 1)
 
 /*
  * The region used by Xen on the memory will never be mapped in DOM0

It's about parameter evaluation, not about adding extra code when compiled.

~Andrew

Hi Andrew, Jan

On 27.04.2022 14:33, Andrew Cooper wrote:
> On 27/04/2022 12:06, Michal Orzel wrote:
>> Hi Jan,
>>
>> On 27.04.2022 11:59, Jan Beulich wrote:
>>> On 27.04.2022 11:49, Michal Orzel wrote:
>>>> Function unmap_common_complete defines and sets a variable ld that is
>>>> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
>>>> this macro does not make use of any arguments causing a compiler to
>>>> warn about unused-but-set variable (when -Wunused-but-set-variable is
>>>> enabled). Fix this by removing ld and directly passing current->domain
>>>> to gnttab_host_mapping_get_page_type.
>>> I think we want to retain the ld / rd notation. Therefore I think it's
>>> rather the Arm macro which wants adjusting to not leave this argument
>>> unused.
>>>
>> I would agree provided that the ld variable was used in more than one place.
>> As it is not, it does not seem very beneficial to keep a variable that is used
>> just in one place and stores the macro value.
>>
>> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0)
>> so modifying it seems to be a quite big overhead.
> 
> diff --git a/xen/arch/arm/include/asm/grant_table.h
> b/xen/arch/arm/include/asm/grant_table.h
> index d31a4d6805d6..9f68c2a37eb6 100644
> --- a/xen/arch/arm/include/asm/grant_table.h
> +++ b/xen/arch/arm/include/asm/grant_table.h
> @@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
> *d, mfn_t mfn)
>  
>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                unsigned int flags, unsigned int
> cache_flags);
> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                 unsigned long new_gpaddr, unsigned int
> flags);
> -#define gnttab_release_host_mappings(domain) 1
> +#define gnttab_release_host_mappings(domain) (domain, 1)
>  
>  /*
>   * The region used by Xen on the memory will never be mapped in DOM0
> 
> It's about parameter evaluation, not about adding extra code when compiled.
> 

Unfortunately, solution presented by Andrew does not work.
We will get the following error due to -Werror=unused-value:
"left-hand operand of comma expression has no effect"

If we want to keep this variable, how about using unused attribute?

struct domain *__maybe_unused ld

Cheers,
Michal

On 28.04.2022 09:16, Michal Orzel wrote:
> Hi Andrew, Jan
> 
> On 27.04.2022 14:33, Andrew Cooper wrote:
>> On 27/04/2022 12:06, Michal Orzel wrote:
>>> Hi Jan,
>>>
>>> On 27.04.2022 11:59, Jan Beulich wrote:
>>>> On 27.04.2022 11:49, Michal Orzel wrote:
>>>>> Function unmap_common_complete defines and sets a variable ld that is
>>>>> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
>>>>> this macro does not make use of any arguments causing a compiler to
>>>>> warn about unused-but-set variable (when -Wunused-but-set-variable is
>>>>> enabled). Fix this by removing ld and directly passing current->domain
>>>>> to gnttab_host_mapping_get_page_type.
>>>> I think we want to retain the ld / rd notation. Therefore I think it's
>>>> rather the Arm macro which wants adjusting to not leave this argument
>>>> unused.
>>>>
>>> I would agree provided that the ld variable was used in more than one place.
>>> As it is not, it does not seem very beneficial to keep a variable that is used
>>> just in one place and stores the macro value.
>>>
>>> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0)
>>> so modifying it seems to be a quite big overhead.
>>
>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>> b/xen/arch/arm/include/asm/grant_table.h
>> index d31a4d6805d6..9f68c2a37eb6 100644
>> --- a/xen/arch/arm/include/asm/grant_table.h
>> +++ b/xen/arch/arm/include/asm/grant_table.h
>> @@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
>> *d, mfn_t mfn)
>>  
>>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>                                unsigned int flags, unsigned int
>> cache_flags);
>> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
>> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
>>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>                                 unsigned long new_gpaddr, unsigned int
>> flags);
>> -#define gnttab_release_host_mappings(domain) 1
>> +#define gnttab_release_host_mappings(domain) (domain, 1)
>>  
>>  /*
>>   * The region used by Xen on the memory will never be mapped in DOM0
>>
>> It's about parameter evaluation, not about adding extra code when compiled.
>>
> 
> Unfortunately, solution presented by Andrew does not work.
> We will get the following error due to -Werror=unused-value:
> "left-hand operand of comma expression has no effect"

Perhaps

#define gnttab_host_mapping_get_page_type(ro, ld, rd) \
    ((void)(ro), (void)(ld), (void)(rd), 0)

?

Jan

On 28.04.2022 09:27, Jan Beulich wrote:
>>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>>> b/xen/arch/arm/include/asm/grant_table.h
>>> index d31a4d6805d6..9f68c2a37eb6 100644
>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>> +++ b/xen/arch/arm/include/asm/grant_table.h
>>> @@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
>>> *d, mfn_t mfn)
>>>  
>>>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>>                                unsigned int flags, unsigned int
>>> cache_flags);
>>> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
>>> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
>>>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>>                                 unsigned long new_gpaddr, unsigned int
>>> flags);
>>> -#define gnttab_release_host_mappings(domain) 1
>>> +#define gnttab_release_host_mappings(domain) (domain, 1)
>>>  
>>>  /*
>>>   * The region used by Xen on the memory will never be mapped in DOM0
>>>
>>> It's about parameter evaluation, not about adding extra code when compiled.
>>>
>>
>> Unfortunately, solution presented by Andrew does not work.
>> We will get the following error due to -Werror=unused-value:
>> "left-hand operand of comma expression has no effect"
> 
> Perhaps
> 
> #define gnttab_host_mapping_get_page_type(ro, ld, rd) \
>     ((void)(ro), (void)(ld), (void)(rd), 0)
> 
> ?
I already tried that and it won't work producing the following:
"error: void value not ignored as it ought to be"

Michal

On 28.04.2022 09:32, Michal Orzel wrote:
> 
> 
> On 28.04.2022 09:27, Jan Beulich wrote:
>>>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>>>> b/xen/arch/arm/include/asm/grant_table.h
>>>> index d31a4d6805d6..9f68c2a37eb6 100644
>>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>>> +++ b/xen/arch/arm/include/asm/grant_table.h
>>>> @@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
>>>> *d, mfn_t mfn)
>>>>  
>>>>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>>>                                unsigned int flags, unsigned int
>>>> cache_flags);
>>>> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
>>>> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
>>>>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>>>                                 unsigned long new_gpaddr, unsigned int
>>>> flags);
>>>> -#define gnttab_release_host_mappings(domain) 1
>>>> +#define gnttab_release_host_mappings(domain) (domain, 1)
>>>>  
>>>>  /*
>>>>   * The region used by Xen on the memory will never be mapped in DOM0
>>>>
>>>> It's about parameter evaluation, not about adding extra code when compiled.
>>>>
>>>
>>> Unfortunately, solution presented by Andrew does not work.
>>> We will get the following error due to -Werror=unused-value:
>>> "left-hand operand of comma expression has no effect"
>>
>> Perhaps
>>
>> #define gnttab_host_mapping_get_page_type(ro, ld, rd) \
>>     ((void)(ro), (void)(ld), (void)(rd), 0)
>>
>> ?
> I already tried that and it won't work producing the following:
> "error: void value not ignored as it ought to be"
> 
> Michal

Sorry about that but I was wrong. Your solution does work.
I did not enclose the params in parentheses.

Michal

Hi Andrew,

On 27.04.2022 14:33, Andrew Cooper wrote:
> On 27/04/2022 12:06, Michal Orzel wrote:
>> Hi Jan,
>>
>> On 27.04.2022 11:59, Jan Beulich wrote:
>>> On 27.04.2022 11:49, Michal Orzel wrote:
>>>> Function unmap_common_complete defines and sets a variable ld that is
>>>> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
>>>> this macro does not make use of any arguments causing a compiler to
>>>> warn about unused-but-set variable (when -Wunused-but-set-variable is
>>>> enabled). Fix this by removing ld and directly passing current->domain
>>>> to gnttab_host_mapping_get_page_type.
>>> I think we want to retain the ld / rd notation. Therefore I think it's
>>> rather the Arm macro which wants adjusting to not leave this argument
>>> unused.
>>>
>> I would agree provided that the ld variable was used in more than one place.
>> As it is not, it does not seem very beneficial to keep a variable that is used
>> just in one place and stores the macro value.
>>
>> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0)
>> so modifying it seems to be a quite big overhead.
> 
> diff --git a/xen/arch/arm/include/asm/grant_table.h
> b/xen/arch/arm/include/asm/grant_table.h
> index d31a4d6805d6..9f68c2a37eb6 100644
> --- a/xen/arch/arm/include/asm/grant_table.h
> +++ b/xen/arch/arm/include/asm/grant_table.h
> @@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
> *d, mfn_t mfn)
>  
>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                unsigned int flags, unsigned int
> cache_flags);
> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                 unsigned long new_gpaddr, unsigned int
> flags);
> -#define gnttab_release_host_mappings(domain) 1
> +#define gnttab_release_host_mappings(domain) (domain, 1)
>  
>  /*
>   * The region used by Xen on the memory will never be mapped in DOM0
> 
> It's about parameter evaluation, not about adding extra code when compiled.
> 
You're right, thanks. I will do it your way in v2.

> ~Andrew

Cheers,
Michal