* [PATCH 0/2] xen/arm: gnttab: macros modifications
@ 2022-05-05 10:35 Michal Orzel
2022-05-05 10:36 ` [PATCH 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping* Michal Orzel
2022-05-05 10:36 ` [PATCH 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once Michal Orzel
0 siblings, 2 replies; 12+ messages in thread
From: Michal Orzel @ 2022-05-05 10:35 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk
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 | 80 +++++++++++++++++++-------
1 file changed, 59 insertions(+), 21 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping* 2022-05-05 10:35 [PATCH 0/2] xen/arm: gnttab: macros modifications Michal Orzel @ 2022-05-05 10:36 ` Michal Orzel 2022-05-05 11:13 ` Jan Beulich 2022-05-05 10:36 ` [PATCH 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once Michal Orzel 1 sibling, 1 reply; 12+ messages in thread From: Michal Orzel @ 2022-05-05 10:36 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk 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> --- xen/arch/arm/include/asm/grant_table.h | 13 +++++++++++-- 1 file changed, 11 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..779f6fbdbb 100644 --- a/xen/arch/arm/include/asm/grant_table.h +++ b/xen/arch/arm/include/asm/grant_table.h @@ -29,12 +29,21 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn) #endif } +static inline bool gnttab_host_mapping_get_page_type(bool ro, struct domain *ld, + struct domain *rd) +{ + return false; +} + +static inline bool gnttab_release_host_mappings(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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping* 2022-05-05 10:36 ` [PATCH 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping* Michal Orzel @ 2022-05-05 11:13 ` Jan Beulich 2022-05-06 7:27 ` Michal Orzel 0 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2022-05-05 11:13 UTC (permalink / raw) To: Michal Orzel Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk, xen-devel On 05.05.2022 12:36, Michal Orzel wrote: > --- a/xen/arch/arm/include/asm/grant_table.h > +++ b/xen/arch/arm/include/asm/grant_table.h > @@ -29,12 +29,21 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn) > #endif > } > > +static inline bool gnttab_host_mapping_get_page_type(bool ro, struct domain *ld, > + struct domain *rd) > +{ > + return false; > +} > + > +static inline bool gnttab_release_host_mappings(struct domain *d) > +{ > + return true; > +} Looking at x86 I think all three instances of struct domain * want to be const struct domain *. Then Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping* 2022-05-05 11:13 ` Jan Beulich @ 2022-05-06 7:27 ` Michal Orzel 2022-05-06 9:56 ` Julien Grall 2022-05-06 9:57 ` Jan Beulich 0 siblings, 2 replies; 12+ messages in thread From: Michal Orzel @ 2022-05-06 7:27 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk, xen-devel On 05.05.2022 13:13, Jan Beulich wrote: > On 05.05.2022 12:36, Michal Orzel wrote: >> --- a/xen/arch/arm/include/asm/grant_table.h >> +++ b/xen/arch/arm/include/asm/grant_table.h >> @@ -29,12 +29,21 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn) >> #endif >> } >> >> +static inline bool gnttab_host_mapping_get_page_type(bool ro, struct domain *ld, >> + struct domain *rd) >> +{ >> + return false; >> +} >> + >> +static inline bool gnttab_release_host_mappings(struct domain *d) >> +{ >> + return true; >> +} > > Looking at x86 I think all three instances of struct domain * want to > be const struct domain *. Then > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Jan > Thanks. I think we should mark all parameters as const meaning also const bool ro. Cheers, Michal ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping* 2022-05-06 7:27 ` Michal Orzel @ 2022-05-06 9:56 ` Julien Grall 2022-05-06 10:01 ` Michal Orzel 2022-05-06 9:57 ` Jan Beulich 1 sibling, 1 reply; 12+ messages in thread From: Julien Grall @ 2022-05-06 9:56 UTC (permalink / raw) To: Michal Orzel, Jan Beulich Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk, xen-devel On 06/05/2022 08:27, Michal Orzel wrote: > On 05.05.2022 13:13, Jan Beulich wrote: >> On 05.05.2022 12:36, Michal Orzel wrote: >>> --- a/xen/arch/arm/include/asm/grant_table.h >>> +++ b/xen/arch/arm/include/asm/grant_table.h >>> @@ -29,12 +29,21 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn) >>> #endif >>> } >>> >>> +static inline bool gnttab_host_mapping_get_page_type(bool ro, struct domain *ld, >>> + struct domain *rd) >>> +{ >>> + return false; >>> +} >>> + >>> +static inline bool gnttab_release_host_mappings(struct domain *d) >>> +{ >>> + return true; >>> +} >> >> Looking at x86 I think all three instances of struct domain * want to >> be const struct domain *. Then >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> >> Jan >> > Thanks. I think we should mark all parameters as const meaning also const bool ro. Hmmmm... ro is not a pointer and so the value can only be modified within the inline helper. So isn't it a bit pointless to set it to const? If that's the only comment on the next version, this could be dealt on commit. Cheers, -- Julien Grall ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping* 2022-05-06 9:56 ` Julien Grall @ 2022-05-06 10:01 ` Michal Orzel 0 siblings, 0 replies; 12+ messages in thread From: Michal Orzel @ 2022-05-06 10:01 UTC (permalink / raw) To: Julien Grall, Jan Beulich Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk, xen-devel On 06.05.2022 11:56, Julien Grall wrote: > > > On 06/05/2022 08:27, Michal Orzel wrote: >> On 05.05.2022 13:13, Jan Beulich wrote: >>> On 05.05.2022 12:36, Michal Orzel wrote: >>>> --- a/xen/arch/arm/include/asm/grant_table.h >>>> +++ b/xen/arch/arm/include/asm/grant_table.h >>>> @@ -29,12 +29,21 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn) >>>> #endif >>>> } >>>> +static inline bool gnttab_host_mapping_get_page_type(bool ro, struct domain *ld, >>>> + struct domain *rd) >>>> +{ >>>> + return false; >>>> +} >>>> + >>>> +static inline bool gnttab_release_host_mappings(struct domain *d) >>>> +{ >>>> + return true; >>>> +} >>> >>> Looking at x86 I think all three instances of struct domain * want to >>> be const struct domain *. Then >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> >>> Jan >>> >> Thanks. I think we should mark all parameters as const meaning also const bool ro. > > Hmmmm... ro is not a pointer and so the value can only be modified within the inline helper. So isn't it a bit pointless to set it to const? > > If that's the only comment on the next version, this could be dealt on commit. > > Cheers, > From the code point of view it is pointless. However it is also about self-documenting the code. If this is something that cannot occur in Xen, I'd be greatful for dealing with this on commit. Cheers, Michal ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping* 2022-05-06 7:27 ` Michal Orzel 2022-05-06 9:56 ` Julien Grall @ 2022-05-06 9:57 ` Jan Beulich 1 sibling, 0 replies; 12+ messages in thread From: Jan Beulich @ 2022-05-06 9:57 UTC (permalink / raw) To: Michal Orzel Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk, xen-devel On 06.05.2022 09:27, Michal Orzel wrote: > > > On 05.05.2022 13:13, Jan Beulich wrote: >> On 05.05.2022 12:36, Michal Orzel wrote: >>> --- a/xen/arch/arm/include/asm/grant_table.h >>> +++ b/xen/arch/arm/include/asm/grant_table.h >>> @@ -29,12 +29,21 @@ static inline void gnttab_mark_dirty(struct domain *d, mfn_t mfn) >>> #endif >>> } >>> >>> +static inline bool gnttab_host_mapping_get_page_type(bool ro, struct domain *ld, >>> + struct domain *rd) >>> +{ >>> + return false; >>> +} >>> + >>> +static inline bool gnttab_release_host_mappings(struct domain *d) >>> +{ >>> + return true; >>> +} >> >> Looking at x86 I think all three instances of struct domain * want to >> be const struct domain *. Then >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> > Thanks. I think we should mark all parameters as const meaning also const bool ro. Not really - I did suggest "pointer to const" which is different from the parameter itself being const. We don't normally do the latter, and I'd recommend we don't start, or else we'll end up with static inline bool gnttab_host_mapping_get_page_type(const bool ro, const struct domain *const ld, const struct domain *const rd) { ... Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once 2022-05-05 10:35 [PATCH 0/2] xen/arm: gnttab: macros modifications Michal Orzel 2022-05-05 10:36 ` [PATCH 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping* Michal Orzel @ 2022-05-05 10:36 ` Michal Orzel 2022-05-05 11:20 ` Jan Beulich 1 sibling, 1 reply; 12+ messages in thread From: Michal Orzel @ 2022-05-05 10:36 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk Modify macros to evaluate all the arguments and make sure the arguments are evaluated only once. While doing so, use typeof for basic types and use const qualifier when applicable. Signed-off-by: Michal Orzel <michal.orzel@arm.com> --- xen/arch/arm/include/asm/grant_table.h | 67 ++++++++++++++++++-------- 1 file changed, 48 insertions(+), 19 deletions(-) diff --git a/xen/arch/arm/include/asm/grant_table.h b/xen/arch/arm/include/asm/grant_table.h index 779f6fbdbb..b161d4baf1 100644 --- a/xen/arch/arm/include/asm/grant_table.h +++ b/xen/arch/arm/include/asm/grant_table.h @@ -57,38 +57,44 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, #define gnttab_init_arch(gt) \ ({ \ - unsigned int ngf_ = (gt)->max_grant_frames; \ + struct grant_table *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); \ + struct grant_table *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 struct grant_table *gt_ = (gt); \ + const typeof(st) st_ = (st); \ + const typeof(idx) idx_ = (idx); \ + const gfn_t 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_; \ }) @@ -98,13 +104,36 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, }) #define gnttab_shared_gfn(d, t, i) \ - (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) + ({ \ + const struct domain *d_ = (d); \ + const struct grant_table *t_ = (t); \ + const typeof(i) i_ = (i); \ + \ + if ( d_ != NULL ) \ + ASSERT(d_->grant_table == t_); \ + \ + (i_ >= nr_grant_frames(t_)) ? INVALID_GFN \ + : t_->arch.shared_gfn[i_]; \ + }) #define gnttab_status_gfn(d, t, i) \ - (((i) >= nr_status_frames(t)) ? INVALID_GFN : (t)->arch.status_gfn[i]) + ({ \ + const struct domain *d_ = (d); \ + const struct grant_table *t_ = (t); \ + const typeof(i) i_ = (i); \ + \ + if ( d_ != NULL ) \ + ASSERT(d_->grant_table == t_); \ + \ + (i_ >= nr_status_frames(t_)) ? INVALID_GFN \ + : t_->arch.status_gfn[i_]; \ + }) -#define gnttab_need_iommu_mapping(d) \ - (is_domain_direct_mapped(d) && is_iommu_enabled(d)) +#define gnttab_need_iommu_mapping(d) \ + ({ \ + const struct domain *d_ = (d); \ + is_domain_direct_mapped(d_) && is_iommu_enabled(d_); \ + }) #endif /* __ASM_GRANT_TABLE_H__ */ /* -- 2.25.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once 2022-05-05 10:36 ` [PATCH 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once Michal Orzel @ 2022-05-05 11:20 ` Jan Beulich 2022-05-05 11:25 ` Michal Orzel 0 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2022-05-05 11:20 UTC (permalink / raw) To: Michal Orzel Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk, xen-devel On 05.05.2022 12:36, Michal Orzel wrote: > Modify macros to evaluate all the arguments and make sure the arguments > are evaluated only once. While doing so, use typeof for basic types > and use const qualifier when applicable. Why only for basic types? To take an example, passing void * into gnttab_need_iommu_mapping() would imo also better not work. > @@ -98,13 +104,36 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, > }) > > #define gnttab_shared_gfn(d, t, i) \ > - (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) > + ({ \ > + const struct domain *d_ = (d); \ > + const struct grant_table *t_ = (t); \ > + const typeof(i) i_ = (i); \ > + \ > + if ( d_ != NULL ) \ > + ASSERT(d_->grant_table == t_); \ I'm puzzled by this NULL check (and the similar instance further down): Are you suggesting NULL can legitimately come into here? If not, maybe better ASSERT(d_ && d_->grant_table == t_)? Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once 2022-05-05 11:20 ` Jan Beulich @ 2022-05-05 11:25 ` Michal Orzel 2022-05-05 11:55 ` Jan Beulich 0 siblings, 1 reply; 12+ messages in thread From: Michal Orzel @ 2022-05-05 11:25 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk, xen-devel Hi Jan, On 05.05.2022 13:20, Jan Beulich wrote: > On 05.05.2022 12:36, Michal Orzel wrote: >> Modify macros to evaluate all the arguments and make sure the arguments >> are evaluated only once. While doing so, use typeof for basic types >> and use const qualifier when applicable. > > Why only for basic types? To take an example, passing void * into > gnttab_need_iommu_mapping() would imo also better not work. > Just by looking at the majority of macros in Xen, typeof is used mostly for basic data types. Also I think it is better to explictly use a struct type for better readability. Otherwise one need to search in other files, to what type does typeof evaluates. >> @@ -98,13 +104,36 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, >> }) >> >> #define gnttab_shared_gfn(d, t, i) \ >> - (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) >> + ({ \ >> + const struct domain *d_ = (d); \ >> + const struct grant_table *t_ = (t); \ >> + const typeof(i) i_ = (i); \ >> + \ >> + if ( d_ != NULL ) \ >> + ASSERT(d_->grant_table == t_); \ > > I'm puzzled by this NULL check (and the similar instance further down): > Are you suggesting NULL can legitimately come into here? If not, maybe > better ASSERT(d_ && d_->grant_table == t_)? > Example: NULL is coming explictly from macro gnttab_get_frame_gfn right above gnttab_shared_gfn. > Jan > Cheers, Michal ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once 2022-05-05 11:25 ` Michal Orzel @ 2022-05-05 11:55 ` Jan Beulich 2022-05-05 20:34 ` Stefano Stabellini 0 siblings, 1 reply; 12+ messages in thread From: Jan Beulich @ 2022-05-05 11:55 UTC (permalink / raw) To: Michal Orzel Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk, xen-devel On 05.05.2022 13:25, Michal Orzel wrote: > On 05.05.2022 13:20, Jan Beulich wrote: >> On 05.05.2022 12:36, Michal Orzel wrote: >>> Modify macros to evaluate all the arguments and make sure the arguments >>> are evaluated only once. While doing so, use typeof for basic types >>> and use const qualifier when applicable. >> >> Why only for basic types? To take an example, passing void * into >> gnttab_need_iommu_mapping() would imo also better not work. >> > Just by looking at the majority of macros in Xen, typeof is used mostly for basic data types. > Also I think it is better to explictly use a struct type for better readability. > Otherwise one need to search in other files, to what type does typeof evaluates. > >>> @@ -98,13 +104,36 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, >>> }) >>> >>> #define gnttab_shared_gfn(d, t, i) \ >>> - (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) >>> + ({ \ >>> + const struct domain *d_ = (d); \ >>> + const struct grant_table *t_ = (t); \ >>> + const typeof(i) i_ = (i); \ >>> + \ >>> + if ( d_ != NULL ) \ >>> + ASSERT(d_->grant_table == t_); \ >> >> I'm puzzled by this NULL check (and the similar instance further down): >> Are you suggesting NULL can legitimately come into here? If not, maybe >> better ASSERT(d_ && d_->grant_table == t_)? >> > Example: > NULL is coming explictly from macro gnttab_get_frame_gfn right above gnttab_shared_gfn. Hmm, that's pretty odd (and Arm specific). Just like with the other remark above, it'll be the Arm maintainers to judge, but here I think the NULLs would better be done away with, by introducing intermediate macros, e.g. #define gnttab_shared_gfn_(t, i) ... #define gnttab_shared_gfn(d, t, i) ({ \ const typeof(t) t_ = (t); \ ASSERT((d)->grant_table == t_); \ gnttab_shared_gfn_(t_, i); \ }) #define gnttab_get_frame_gfn(gt, st, idx) ({ \ (st) ? gnttab_status_gfn_(gt, idx) \ : gnttab_shared_gfn_(gt, idx); \ }) Jan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once 2022-05-05 11:55 ` Jan Beulich @ 2022-05-05 20:34 ` Stefano Stabellini 0 siblings, 0 replies; 12+ messages in thread From: Stefano Stabellini @ 2022-05-05 20:34 UTC (permalink / raw) To: Jan Beulich Cc: Michal Orzel, Stefano Stabellini, Julien Grall, Bertrand Marquis, Volodymyr Babchuk, xen-devel On Thu, 5 May 2022, Jan Beulich wrote: > On 05.05.2022 13:25, Michal Orzel wrote: > > On 05.05.2022 13:20, Jan Beulich wrote: > >> On 05.05.2022 12:36, Michal Orzel wrote: > >>> Modify macros to evaluate all the arguments and make sure the arguments > >>> are evaluated only once. While doing so, use typeof for basic types > >>> and use const qualifier when applicable. > >> > >> Why only for basic types? To take an example, passing void * into > >> gnttab_need_iommu_mapping() would imo also better not work. > >> > > Just by looking at the majority of macros in Xen, typeof is used mostly for basic data types. > > Also I think it is better to explictly use a struct type for better readability. > > Otherwise one need to search in other files, to what type does typeof evaluates. > > > >>> @@ -98,13 +104,36 @@ int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn, > >>> }) > >>> > >>> #define gnttab_shared_gfn(d, t, i) \ > >>> - (((i) >= nr_grant_frames(t)) ? INVALID_GFN : (t)->arch.shared_gfn[i]) > >>> + ({ \ > >>> + const struct domain *d_ = (d); \ > >>> + const struct grant_table *t_ = (t); \ > >>> + const typeof(i) i_ = (i); \ > >>> + \ > >>> + if ( d_ != NULL ) \ > >>> + ASSERT(d_->grant_table == t_); \ > >> > >> I'm puzzled by this NULL check (and the similar instance further down): > >> Are you suggesting NULL can legitimately come into here? If not, maybe > >> better ASSERT(d_ && d_->grant_table == t_)? > >> > > Example: > > NULL is coming explictly from macro gnttab_get_frame_gfn right above gnttab_shared_gfn. > > Hmm, that's pretty odd (and Arm specific). Just like with the other remark > above, it'll be the Arm maintainers to judge, but here I think the NULLs > would better be done away with, by introducing intermediate macros, e.g. I am fine with Jan's comments on both patches ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-05-06 10:02 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-05 10:35 [PATCH 0/2] xen/arm: gnttab: macros modifications Michal Orzel
2022-05-05 10:36 ` [PATCH 1/2] xen/arm: gnttab: use static inlines for gnttab_{release_}host_mapping* Michal Orzel
2022-05-05 11:13 ` Jan Beulich
2022-05-06 7:27 ` Michal Orzel
2022-05-06 9:56 ` Julien Grall
2022-05-06 10:01 ` Michal Orzel
2022-05-06 9:57 ` Jan Beulich
2022-05-05 10:36 ` [PATCH 2/2] xen/arm: gnttab: modify macros to evaluate all arguments and only once Michal Orzel
2022-05-05 11:20 ` Jan Beulich
2022-05-05 11:25 ` Michal Orzel
2022-05-05 11:55 ` Jan Beulich
2022-05-05 20:34 ` Stefano Stabellini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.