| Message ID | 20220505103601.322110-3-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: > 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
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
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
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
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__ */ /*
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(-)