Re: [Xen-devel] [PATCH 2/7] iommu: make use of type-safe BFN and MFN in exported functions

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 15 March 2018 15:45
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>;
> Jun Nakajima <jun.nakajima@xxxxxxxxx>; Kevin Tian
> <kevin.tian@xxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx; Konrad Rzeszutek Wilk
> <konrad.wilk@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH 2/7] iommu: make use of type-safe BFN and MFN in
> exported functions
> 
> >>> On 12.02.18 at 11:47, <paul.durrant@xxxxxxxxxx> 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.
> >
> > NOTE: Since (with this patch applied) bfn_t is now in use, the patch also
> >       introduces the 'cscope/grep fodder' to allow the type declaration to
> >       be easily found.
> 
> Ah, here we go. But I continue to think this belong in patch 1.
> 

Ok. I debated it with myself when I wrote the original patches. I'll move the 
relevant hunks.

> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2676,13 +2676,12 @@ static int _get_page_type(struct page_info
> *page, unsigned long type,
> >          struct domain *d = page_get_owner(page);
> >          if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
> >          {
> > -            gfn_t gfn = _gfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page))));
> > +            bfn_t bfn = _bfn(mfn_to_gmfn(d, mfn_x(page_to_mfn(page))));
> >
> >              if ( (x & PGT_type_mask) == PGT_writable_page )
> > -                iommu_ret = iommu_unmap_page(d, gfn_x(gfn));
> > +                iommu_ret = iommu_unmap_page(d, bfn);
> >              else if ( type == PGT_writable_page )
> > -                iommu_ret = iommu_map_page(d, gfn_x(gfn),
> > -                                           mfn_x(page_to_mfn(page)),
> > +                iommu_ret = iommu_map_page(d, bfn, page_to_mfn(page),
> 
> Along the lines of what I've said earlier about mixing address spaces,
> this would perhaps not so much need a comment (it's a 1:1 mapping
> after all), but rather making more obvious that it's a 1:1 mapping.
> This in particular would mean to me to latch page_to_mfn(page) into
> a (neutrally named, e.g. "frame") local variable, and use the result in
> a way that makes obviously especially on the "map" path that this
> really requests a 1:1 mapping. By implication from the 1:1 mapping
> it'll then (hopefully) be clear to the reader that which exact name
> space is used doesn't really matter.

Ok, I'll re-phrase things in v2.

> 
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -873,12 +873,14 @@ out:
> >              if ( iommu_flags )
> >                  for ( i = 0; i < (1 << order); i++ )
> >                  {
> > -                    rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, 
> > iommu_flags);
> > +                    rc = iommu_map_page(d, _bfn(gfn + i), mfn_add(mfn, i),
> > +                                        iommu_flags);
> >                      if ( unlikely(rc) )
> >                      {
> >                          while ( i-- )
> >                              /* If statement to satisfy __must_check. */
> > -                            if ( iommu_unmap_page(p2m->domain, gfn + i) )
> > +                            if ( iommu_unmap_page(p2m->domain,
> > +                                                  _bfn(gfn + i)) )
> 
> The fundamental issue of mixed address spaces continues ...
> 

I'll add appropriately an appropriately named stack variable.

> > @@ -781,14 +781,14 @@ guest_physmap_add_entry(struct domain *d,
> gfn_t gfn, mfn_t mfn,
> >          {
> >              for ( i = 0; i < (1 << page_order); i++ )
> >              {
> > -                rc = iommu_map_page(d, mfn_x(mfn_add(mfn, i)),
> > -                                    mfn_x(mfn_add(mfn, i)),
> > +                rc = iommu_map_page(d, _bfn(mfn_x(mfn) + i),
> > +                                    mfn_add(mfn, i),
> 
> Please check whether some line wrapping can now be avoided, like
> apparently here.
> 

Ok.

> > @@ -1164,7 +1164,9 @@ int set_identity_p2m_entry(struct domain *d,
> unsigned long gfn_l,
> >      {
> >          if ( !need_iommu(d) )
> >              return 0;
> > -        return iommu_map_page(d, gfn_l, gfn_l,
> IOMMUF_readable|IOMMUF_writable);
> > +
> > +        return iommu_map_page(d, _bfn(gfn_l), _mfn(gfn_l),
> > +                              IOMMUF_readable|IOMMUF_writable);
> 
> Please add spaces around | as you touch this (also elsewhere).
> 

Ok.

> > @@ -1254,7 +1256,8 @@ int clear_identity_p2m_entry(struct domain *d,
> unsigned long gfn_l)
> >      {
> >          if ( !need_iommu(d) )
> >              return 0;
> > -        return iommu_unmap_page(d, gfn_l);
> > +
> > +        return iommu_unmap_page(d, _bfn(gfn_l));
> >      }
> 
> No real need for the extra blank line here, as this isn't the main return
> point.
> 

Ok.

  Paul

> Jan

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