Re: [Xen-devel] [PATCH v2 04/13] iommu: push use of type-safe BFN and MFN into iommu_ops

[George Dunlap <dunlapg@xxxxxxxxx>]

On Sat, Jul 7, 2018 at 12:05 PM, Paul Durrant <paul.durrant@xxxxxxxxxx> wrote:
> @@ -787,7 +793,9 @@ int amd_iommu_reserve_domain_unity_map(struct domain 
> *domain,
>      gfn = phys_addr >> PAGE_SHIFT;
>      for ( i = 0; i < npages; i++ )
>      {
> -        rt = amd_iommu_map_page(domain, gfn +i, gfn +i, flags);
> +        unsigned long frame = gfn + i;
> +
> +        rt = amd_iommu_map_page(domain, _bfn(frame), _mfn(frame), flags);

Here's an example where the intermediate variable may actually improve
things, if only by avoiding code duplication. :-)

> @@ -2766,14 +2766,14 @@ static int __must_check arm_smmu_map_page(struct 
> domain *d, unsigned long bfn,
>          * The function guest_physmap_add_entry replaces the current mapping
>          * if there is already one...
>          */
> -       return guest_physmap_add_entry(d, _gfn(gfn), _mfn(mfn), 0, t);
> +       return guest_physmap_add_entry(d, gfn, mfn, 0, t);
>  }
>
> -static int __must_check arm_smmu_unmap_page(struct domain *d, unsigned long 
> bfn)
> +static int __must_check arm_smmu_unmap_page(struct domain *d, bfn_t bfn)
>  {
> -       unsigned long frame = bfn;
> -       unsigned long gfn = frame;
> -       unsigned long mfn = frame;
> +       unsigned long frame = bfn_x(bfn);
> +       gfn_t gfn = _gfn(frame);
> +       mfn_t mfn = _mfn(frame);

Even if we decide to use an intermediate variable in many other cases,
this construct in particular seems excessive.

Everything else looks good, thanks.

 -George

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