| Message ID | 20220505103601.322110-2-michal.orzel@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: gnttab: macros modifications | expand |
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
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
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,
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
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
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
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(-)