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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> --- 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.

> --- 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 ...

> @@ -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.

> @@ -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).

> @@ -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.

Jan

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