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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Julien Grall <julien.grall@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Cc: Jun Nakajima <jun.nakajima@xxxxxxxxx>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> 
> 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.

 -George

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