Re: [PATCH v2 03/13] iommu: make use of type-safe BFN and MFN in exported functions

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 11 July 2018 08:57
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Jun Nakajima <jun.nakajima@intel.com>; Kevin Tian
> <kevin.tian@intel.com>; Stefano Stabellini <sstabellini@kernel.org>; xen-
> devel <xen-devel@lists.xenproject.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Tim (Xen.org) <tim@xen.org>
> 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, <Paul.Durrant@citrix.com> wrote:
> >> From: George Dunlap [mailto:george.dunlap@citrix.com]
> >> 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
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel