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: George Dunlap [mailto:george.dunlap@citrix.com]
> Sent: 10 July 2018 17:13
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Jan Beulich <jbeulich@suse.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; George Dunlap
> <George.Dunlap@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; Konrad
> Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano Stabellini
> <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>; Tim (Xen.org)
> <tim@xen.org>; Wei Liu <wei.liu2@citrix.com>; Jun Nakajima
> <jun.nakajima@intel.com>; Kevin Tian <kevin.tian@intel.com>
> Subject: Re: [PATCH v2 03/13] iommu: make use of type-safe BFN and MFN
> in exported functions
> 
> On 07/07/2018 12:05 PM, Paul Durrant wrote:
> > This patch modifies the declaration of the entry points to the IOMMU
> > sub-system to use bfn_t and mfn_t in place of unsigned long. A
> subsequent
> > patch will similarly modify the methods in the iommu_ops structure.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: Julien Grall <julien.grall@arm.com>
> > Cc: Tim Deegan <tim@xen.org>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Jun Nakajima <jun.nakajima@intel.com>
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: George Dunlap <george.dunlap@eu.citrix.com>
> >
> > v2:
> >  - Addressed comments from Jan.
> >  - Use intermediate 'frame' variable to avoid directly encapsulating
> >    mfn or gfn values as bfns.
> 
> A couple of comments on particular instances...
> 
> > diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> > index c53cab44d9..ce12bcff42 100644
> > --- a/xen/arch/x86/mm/p2m.c
> > +++ b/xen/arch/x86/mm/p2m.c
> > @@ -714,9 +714,12 @@ p2m_remove_page(struct p2m_domain *p2m,
> unsigned long gfn_l, unsigned long mfn,
> >
> >          if ( need_iommu(p2m->domain) )
> >          {
> > +            unsigned long frame = mfn;
> > +            bfn_t bfn = _bfn(frame);
> > +
> >              for ( i = 0; i < (1 << page_order); i++ )
> >              {
> > -                int ret = iommu_unmap_page(p2m->domain, mfn + i);
> > +                int ret = iommu_unmap_page(p2m->domain, bfn_add(bfn, i));
> 
> Having a 'bfn' variable here makes some sense, because otherwise, if mfn
> ever gets the mfn_t type, you'll have
> 
>   iommu_unmap_page(... _bfn(mfn_x(mfn)+i));
> 
> being able to use bfn_add() is much cleaner.  I don't think the
> intermediate 'frame' variable in the case, really adds anything.
> 
> > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> > index d2610e320c..d0926d13e0 100644
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -1132,6 +1132,8 @@ map_grant_ref(
> >      need_iommu = gnttab_need_iommu_mapping(ld);
> >      if ( need_iommu )
> >      {
> > +        unsigned long frame = mfn_x(mfn);
> > +        bfn_t bfn = _bfn(frame);
> >          unsigned int kind;
> >          int err = 0;
> >
> > @@ -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.

  Paul


> 
>  -George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel