[PATCH v2 0/2] xen/arm: gnttab: macros modifications

[PATCH v2 0/2] xen/arm: gnttab: macros modifications

[Michal Orzel]

This patch series modifies macros inside xen/arch/arm/include/asm/grant_table.h
to be resistant to static analyzer/compiler warnings about unused-but-set
variables.

The first patch acts as a fix for a gcc warning when -Wunused-but-set-variable
flag is enabled by converting the macro to static inline helper. The
adjacent macro is modified as well. These macros can be converted to inline
helpers as they do not take argument of type struct grant_table which is not
fully defined yet.

The second patch modifies the remaining macros to evaluate all their arguments
and only once.

Previous discussion:
https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg119708.html

Michal Orzel (2):
  xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping*
  xen/arm: gnttab: modify macros to evaluate all arguments and only once

 xen/arch/arm/include/asm/grant_table.h | 89 +++++++++++++++++++-------
 1 file changed, 66 insertions(+), 23 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping*

[Michal Orzel]

Function unmap_common_complete (common/grant_table.c) 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 it by converting this macro to a static inline
helper and using the boolean return type.

While there, also convert macro gnttab_release_host_mappings.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v1:
- mark parameters as const
---
 xen/arch/arm/include/asm/grant_table.h | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h
index d31a4d6805..5ccaf6d51f 100644
--- a/xen/arch/arm/include/asm/grant_table.h
+++ b/xen/arch/arm/include/asm/grant_table.h
@@ -29,12 +29,22 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn)
 #endif
 }
 
+static inline bool gnttab_host_mapping_get_page_type(const bool ro,
+                                                     const struct domain *ld,
+                                                     const struct domain *rd)
+{
+    return false;
+}
+
+static inline bool gnttab_release_host_mappings(const struct domain *d)
+{
+    return true;
+}
+
 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)
 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
 
 /*
  * The region used by Xen on the memory will never be mapped in DOM0
-- 
2.25.1

[PATCH v2 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once

[Michal Orzel]

Modify macros to evaluate all the arguments and make sure the arguments
are evaluated only once. Introduce following intermediate macros:
gnttab_status_gfn_, gnttab_shared_gfn_ that do not take domain as a
parameter. These are to be used locally and allow us to avoid passing
NULL from gnttab_get_frame_gfn to the respective macros (without _ suffix).
Make use of a domain parameter from gnttab_shared_gfn and gnttab_status_gfn
by adding an ASSERT.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes since v1:
- use typeof for all data types
- introduce intermediate macros
---
 xen/arch/arm/include/asm/grant_table.h | 75 ++++++++++++++++++--------
 1 file changed, 54 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h
index 5ccaf6d51f..3550990ceb 100644
--- a/xen/arch/arm/include/asm/grant_table.h
+++ b/xen/arch/arm/include/asm/grant_table.h
@@ -58,54 +58,87 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
 
 #define gnttab_init_arch(gt)                                             \
 ({                                                                       \
-    unsigned int ngf_ = (gt)->max_grant_frames;                          \
+    typeof(gt) gt_ = (gt);                                               \
+    unsigned int ngf_ = gt_->max_grant_frames;                           \
     unsigned int nsf_ = grant_to_status_frames(ngf_);                    \
                                                                          \
-    (gt)->arch.shared_gfn = xmalloc_array(gfn_t, ngf_);                  \
-    (gt)->arch.status_gfn = xmalloc_array(gfn_t, nsf_);                  \
-    if ( (gt)->arch.shared_gfn && (gt)->arch.status_gfn )                \
+    gt_->arch.shared_gfn = xmalloc_array(gfn_t, ngf_);                   \
+    gt_->arch.status_gfn = xmalloc_array(gfn_t, nsf_);                   \
+    if ( gt_->arch.shared_gfn && gt_->arch.status_gfn )                  \
     {                                                                    \
         while ( ngf_-- )                                                 \
-            (gt)->arch.shared_gfn[ngf_] = INVALID_GFN;                   \
+            gt_->arch.shared_gfn[ngf_] = INVALID_GFN;                    \
         while ( nsf_-- )                                                 \
-            (gt)->arch.status_gfn[nsf_] = INVALID_GFN;                   \
+            gt_->arch.status_gfn[nsf_] = INVALID_GFN;                    \
     }                                                                    \
     else                                                                 \
-        gnttab_destroy_arch(gt);                                         \
-    (gt)->arch.shared_gfn ? 0 : -ENOMEM;                                 \
+        gnttab_destroy_arch(gt_);                                        \
+    gt_->arch.shared_gfn ? 0 : -ENOMEM;                                  \
 })
 
 #define gnttab_destroy_arch(gt)                                          \
     do {                                                                 \
-        XFREE((gt)->arch.shared_gfn);                                    \
-        XFREE((gt)->arch.status_gfn);                                    \
+        typeof(gt) gt_ = (gt);                                           \
+        XFREE(gt_->arch.shared_gfn);                                     \
+        XFREE(gt_->arch.status_gfn);                                     \
     } while ( 0 )
 
 #define gnttab_set_frame_gfn(gt, st, idx, gfn, mfn)                      \
     ({                                                                   \
         int rc_ = 0;                                                     \
-        gfn_t ogfn = gnttab_get_frame_gfn(gt, st, idx);                  \
-        if ( gfn_eq(ogfn, INVALID_GFN) || gfn_eq(ogfn, gfn) ||           \
-             (rc_ = guest_physmap_remove_page((gt)->domain, ogfn, mfn,   \
+        const typeof(gt) gt_ = (gt);                                     \
+        const typeof(st) st_ = (st);                                     \
+        const typeof(idx) idx_ = (idx);                                  \
+        const typeof(gfn) gfn_ = (gfn);                                  \
+        const gfn_t ogfn_ = gnttab_get_frame_gfn(gt_, st_, idx_);        \
+        if ( gfn_eq(ogfn_, INVALID_GFN) || gfn_eq(ogfn_, gfn_) ||        \
+             (rc_ = guest_physmap_remove_page(gt_->domain, ogfn_, mfn,   \
                                               0)) == 0 )                 \
-            ((st) ? (gt)->arch.status_gfn                                \
-                  : (gt)->arch.shared_gfn)[idx] = (gfn);                 \
+            (st_ ? gt_->arch.status_gfn                                  \
+                 : gt_->arch.shared_gfn)[idx_] = gfn_;                   \
         rc_;                                                             \
     })
 
 #define gnttab_get_frame_gfn(gt, st, idx) ({                             \
-   (st) ? gnttab_status_gfn(NULL, gt, idx)                               \
-        : gnttab_shared_gfn(NULL, gt, idx);                              \
+   (st) ? gnttab_status_gfn_(gt, idx)                                    \
+        : gnttab_shared_gfn_(gt, idx);                                   \
 })
 
+#define gnttab_shared_gfn_(t, i)                                         \
+    ({                                                                   \
+        const typeof(t) t_ = (t);                                        \
+        const typeof(i) i_ = (i);                                        \
+        (i_ >= nr_grant_frames(t_)) ? INVALID_GFN                        \
+                                    : t_->arch.shared_gfn[i_];           \
+    })
+
+#define gnttab_status_gfn_(t, i)                                         \
+    ({                                                                   \
+        const typeof(t) t_ = (t);                                        \
+        const typeof(i) i_ = (i);                                        \
+        (i_ >= nr_status_frames(t_)) ? INVALID_GFN                       \
+                                     : t_->arch.status_gfn[i_];          \
+    })
+
 #define gnttab_shared_gfn(d, t, i)                                       \
-    (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i])
+    ({                                                                   \
+        const typeof(t) t_ = (t);                                        \
+        ASSERT((d)->grant_table == t_);                                  \
+        gnttab_shared_gfn_(t_, i);                                       \
+    })
 
 #define gnttab_status_gfn(d, t, i)                                       \
-    (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i])
+    ({                                                                   \
+        const typeof(t) t_ = (t);                                        \
+        ASSERT((d)->grant_table == t_);                                  \
+        gnttab_status_gfn_(t_, i);                                       \
+    })
 
-#define gnttab_need_iommu_mapping(d)                    \
-    (is_domain_direct_mapped(d) && is_iommu_enabled(d))
+#define gnttab_need_iommu_mapping(d)                                     \
+    ({                                                                   \
+        const typeof(d) d_ = (d);                                        \
+        is_domain_direct_mapped(d_) && is_iommu_enabled(d_);             \
+    })
 
 #endif /* __ASM_GRANT_TABLE_H__ */
 /*
-- 
2.25.1

Re: [PATCH v2 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping*

[Jan Beulich]

On 06.05.2022 11:42, Michal Orzel wrote:
> Function unmap_common_complete (common/grant_table.c) 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 it by converting this macro to a static inline
> helper and using the boolean return type.
> 
> While there, also convert macro gnttab_release_host_mappings.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

This R-b applies only ...

> --- a/xen/arch/arm/include/asm/grant_table.h
> +++ b/xen/arch/arm/include/asm/grant_table.h
> @@ -29,12 +29,22 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn)
>  #endif
>  }
>  
> +static inline bool gnttab_host_mapping_get_page_type(const bool ro,

... with this const dropped again. As said elsewhere, while not
technically wrong we don't normally do so elsewhere, and this ends
up inconsistent with ...

> +                                                     const struct domain *ld,
> +                                                     const struct domain *rd)

... there being just a single const here.

Jan

Re: [PATCH v2 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping*

[Michal Orzel]

Hi Jan,

On 06.05.2022 12:03, Jan Beulich wrote:
> On 06.05.2022 11:42, Michal Orzel wrote:
>> Function unmap_common_complete (common/grant_table.c) 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 it by converting this macro to a static inline
>> helper and using the boolean return type.
>>
>> While there, also convert macro gnttab_release_host_mappings.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> This R-b applies only ...
> 
>> --- a/xen/arch/arm/include/asm/grant_table.h
>> +++ b/xen/arch/arm/include/asm/grant_table.h
>> @@ -29,12 +29,22 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn)
>>  #endif
>>  }
>>  
>> +static inline bool gnttab_host_mapping_get_page_type(const bool ro,
> 
> ... with this const dropped again. As said elsewhere, while not
> technically wrong we don't normally do so elsewhere, and this ends
> up inconsistent with ...
> 
>> +                                                     const struct domain *ld,
>> +                                                     const struct domain *rd)
> 
> ... there being just a single const here.
> 
> Jan
> 

Do you have any remarks related to the second patch in this series?
If yes, I will handle removal of const in the next version.
If not, Julien said in v1 that this can be handled on commit.

Cheers,
Michal

Re: [PATCH v2 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping*

[Julien Grall]

Hi,

On 10/05/2022 07:52, Michal Orzel wrote:
> On 06.05.2022 12:03, Jan Beulich wrote:
>> On 06.05.2022 11:42, Michal Orzel wrote:
>>> Function unmap_common_complete (common/grant_table.c) 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 it by converting this macro to a static inline
>>> helper and using the boolean return type.
>>>
>>> While there, also convert macro gnttab_release_host_mappings.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>

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

>>
>> This R-b applies only ...
>>
>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>> +++ b/xen/arch/arm/include/asm/grant_table.h
>>> @@ -29,12 +29,22 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn)
>>>   #endif
>>>   }
>>>   
>>> +static inline bool gnttab_host_mapping_get_page_type(const bool ro,
>>
>> ... with this const dropped again. As said elsewhere, while not
>> technically wrong we don't normally do so elsewhere, and this ends
>> up inconsistent with ...
>>
>>> +                                                     const struct domain *ld,
>>> +                                                     const struct domain *rd)
>>
>> ... there being just a single const here.
>>
>> Jan
>>
> 
> Do you have any remarks related to the second patch in this series?

FYI, Jan is away this week.

> If yes, I will handle removal of const in the next version.
> If not, Julien said in v1 that this can be handled on commit.

I have committed this patch with the change discussed. I need a bit more 
time to review the second patch.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once

[Julien Grall]

Hi Michal,

On 06/05/2022 10:42, Michal Orzel wrote:
> Modify macros to evaluate all the arguments and make sure the arguments
> are evaluated only once. Introduce following intermediate macros:
> gnttab_status_gfn_, gnttab_shared_gfn_ that do not take domain as a
> parameter. These are to be used locally and allow us to avoid passing
> NULL from gnttab_get_frame_gfn to the respective macros (without _ suffix).
> Make use of a domain parameter from gnttab_shared_gfn and gnttab_status_gfn
> by adding an ASSERT.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

Most of the helpers below are going to disappear with Oleksandr latest 
work (see [1]).

Looking at Oleksandr's patch, I think only gnttab_set_frame_gfn() would 
end up to use one of the macro parameters twice. So I would like to 
suggest to chat with Oleksandr if we can tweak his patch (can be done on 
commit) or we rebase this patch on top of his work.

Cheers,

[1] 
https://lore.kernel.org/xen-devel/1652294845-13980-1-git-send-email-olekstysh@gmail.com/

-- 
Julien Grall

Re: [PATCH v2 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once

[Michal Orzel]

Hi Julien,

On 16.05.2022 12:19, Julien Grall wrote:
> Hi Michal,
> 
> On 06/05/2022 10:42, Michal Orzel wrote:
>> Modify macros to evaluate all the arguments and make sure the arguments
>> are evaluated only once. Introduce following intermediate macros:
>> gnttab_status_gfn_, gnttab_shared_gfn_ that do not take domain as a
>> parameter. These are to be used locally and allow us to avoid passing
>> NULL from gnttab_get_frame_gfn to the respective macros (without _ suffix).
>> Make use of a domain parameter from gnttab_shared_gfn and gnttab_status_gfn
>> by adding an ASSERT.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> 
> Most of the helpers below are going to disappear with Oleksandr latest work (see [1]).
> 
> Looking at Oleksandr's patch, I think only gnttab_set_frame_gfn() would end up to use one of the macro parameters twice. So I would like to suggest to chat with Oleksandr if we can tweak his patch (can be done on commit) or we rebase this patch on top of his work.
> 
> Cheers,
> 
> [1] https://lore.kernel.org/xen-devel/1652294845-13980-1-git-send-email-olekstysh@gmail.com/
> 

By looking at Oleksandr patch:
1. there are 2 macros: gnttab_set_frame_gfn, gnttab_need_iommu_mapping that use one of the macro parameters twice.
2. gnttab_get_frame_gfn still passes NULL as a domain parameter to gnttab_shared_gfn/gnttab_status_gfn that do not evaluate domain parameter

I agree that point 1 could be fixed on commit but point 2 requires in my opinion adding intermediate macros to avoid passing NULL (just like I did).

As this would require more work from Oleksandr, I'm ok to rebase my patch on top of his work once merged.

Cheers,
Michal

Re: [PATCH v2 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping*

[Jan Beulich]

On 10.05.2022 08:52, Michal Orzel wrote:
> Do you have any remarks related to the second patch in this series?

No, I have no further comments there.

Jan

Re: [PATCH v2 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once

[Julien Grall]

Hi Michal,

On 17/05/2022 08:14, Michal Orzel wrote:
> On 16.05.2022 12:19, Julien Grall wrote:
>> Hi Michal,
>>
>> On 06/05/2022 10:42, Michal Orzel wrote:
>>> Modify macros to evaluate all the arguments and make sure the arguments
>>> are evaluated only once. Introduce following intermediate macros:
>>> gnttab_status_gfn_, gnttab_shared_gfn_ that do not take domain as a
>>> parameter. These are to be used locally and allow us to avoid passing
>>> NULL from gnttab_get_frame_gfn to the respective macros (without _ suffix).
>>> Make use of a domain parameter from gnttab_shared_gfn and gnttab_status_gfn
>>> by adding an ASSERT.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
>>
>> Most of the helpers below are going to disappear with Oleksandr latest work (see [1]).
>>
>> Looking at Oleksandr's patch, I think only gnttab_set_frame_gfn() would end up to use one of the macro parameters twice. So I would like to suggest to chat with Oleksandr if we can tweak his patch (can be done on commit) or we rebase this patch on top of his work.
>>
>> Cheers,
>>
>> [1] https://lore.kernel.org/xen-devel/1652294845-13980-1-git-send-email-olekstysh@gmail.com/
>>
> 
> By looking at Oleksandr patch:
> 1. there are 2 macros: gnttab_set_frame_gfn, gnttab_need_iommu_mapping that use one of the macro parameters twice.
> 2. gnttab_get_frame_gfn still passes NULL as a domain parameter to gnttab_shared_gfn/gnttab_status_gfn that do not evaluate domain parameter
> 
> I agree that point 1 could be fixed on commit but point 2 requires in my opinion adding intermediate macros to avoid passing NULL (just like I did).

Ok. I think we could avoid the intermediate macros by implementing the 
helpers the other way around. I.e gnttab_{status}_gfn() call 
gnttab_get_frame_gfn().

> 
> As this would require more work from Oleksandr, I'm ok to rebase my patch on top of his work once merged.

Thanks!

Cheers,

-- 
Julien Grall