> -----Original Message----- > From: Jan Beulich [mailto:[email protected]] > Sent: 11 July 2018 08:57 > To: Paul Durrant <[email protected]> > Cc: Julien Grall <[email protected]>; Andrew Cooper > <[email protected]>; George Dunlap > <[email protected]>; Ian Jackson <[email protected]>; Wei Liu > <[email protected]>; Jun Nakajima <[email protected]>; Kevin Tian > <[email protected]>; Stefano Stabellini <[email protected]>; xen- > devel <[email protected]>; Konrad Rzeszutek Wilk > <[email protected]>; Tim (Xen.org) <[email protected]> > Subject: RE: [PATCH v2 03/13] iommu: make use of type-safe BFN and MFN > in exported functions > > >>> On 10.07.18 at 18:18, <[email protected]> wrote: > >> From: George Dunlap [mailto:[email protected]] > >> Sent: 10 July 2018 17:13 > >> On 07/07/2018 12:05 PM, Paul Durrant wrote: > >> > @@ -1144,14 +1146,13 @@ map_grant_ref( > >> > !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) ) > >> > { > >> > if ( !(kind & MAPKIND_WRITE) ) > >> > - err = iommu_map_page(ld, mfn_x(mfn), mfn_x(mfn), > >> > - IOMMUF_readable|IOMMUF_writable); > >> > + err = iommu_map_page(ld, bfn, mfn, > >> > + IOMMUF_readable | IOMMUF_writable); > >> > } > >> > else if ( act_pin && !old_pin ) > >> > { > >> > if ( !kind ) > >> > - err = iommu_map_page(ld, mfn_x(mfn), mfn_x(mfn), > >> > - IOMMUF_readable); > >> > + err = iommu_map_page(ld, bfn, mfn, IOMMUF_readable); > >> > >> Here's an example where I think having an extra variable is somewhat > >> dangerous. Before this change, it's obvious that you have a 1:1 > >> mapping; now, looking just at this line, it's not obvious that bfn == > >> mfn. Worse, there's a risk that there will be some sort of bug > >> introduced which changes bfn, such that bfn != mfn anymore. > >> > >> If you have to use an intermediate variable here, this should be > >> > >> iommu_map_page(..., _bfn(frame), _mfn(frame), ...); > >> > >> But I really think > >> > >> iommu_map_page(..., _bfn(mfn_x(mfn)), mfn, ...); > >> > >> makes the most sense here. > > > > How about: > > > > #define mfn_to_bfn(mfn) (_bfn(mfn_x(mfn)) > > > > iommu_map_page(..., mfn_to_bfn(mfn), mfn, ...); > > > > ? > > > > I can similarly define gfn_to_bfn() for places where it is needed. > > Please don't: Such macros (see others like mfn_to_gmfn() or > pfn_to_pdx()) imply translation between address spaces, yet there's > no translation here. If anything, the macro name would have to make > clear there's no translation, e.g. cast_mfn_to_bfn(), but I think the > open coding is then more clear and not significantly longer. >
Ok. I will get rid of the intermediates again and open code as before. Paul > Jan > _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
