Re: [Xen-devel] [PATCH 1/7] iommu: introduce the concept of BFN...

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.02.18 at 11:47, <paul.durrant@xxxxxxxxxx> wrote:
> @@ -367,9 +367,9 @@ void amd_iommu_flush_all_pages(struct domain *d)
>  }
>  
>  void amd_iommu_flush_pages(struct domain *d,
> -                           unsigned long gfn, unsigned int order)
> +                           unsigned long bfn, unsigned int order)
>  {
> -    _amd_iommu_flush_pages(d, (uint64_t) gfn << PAGE_SHIFT, order);
> +    _amd_iommu_flush_pages(d, (uint64_t) bfn << PAGE_SHIFT, order);
>  }

I assume you've simply used sed or alike to do the replacements,
but we prefer to make style corrections at the same time when
already touching a line: There's a stray space after the cast here,
and really this wants to be bfn_to_baddr() (which then also
shouldn't use the MMU's PAGE_SHIFT).

> @@ -651,34 +651,34 @@ int amd_iommu_map_page(struct domain *d, unsigned long 
> gfn, unsigned long mfn,
>      if ( rc )
>      {
>          spin_unlock(&hd->arch.mapping_lock);
> -        AMD_IOMMU_DEBUG("Root table alloc failed, gfn = %lx\n", gfn);
> +        AMD_IOMMU_DEBUG("Root table alloc failed, bfn = %lx\n", bfn);
>          domain_crash(d);
>          return rc;
>      }
>  
>      /* Since HVM domain is initialized with 2 level IO page table,
> -     * we might need a deeper page table for lager gfn now */
> +     * we might need a deeper page table for lager bfn now */

Similarly here: Mind making this say "larger" (or "wider")? There's at
least one more instance further down.

> @@ -2763,10 +2763,10 @@ static int __must_check arm_smmu_map_page(struct 
> domain *d, unsigned long gfn,
>        * 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(bfn), _mfn(mfn), 0, t);

Hmm, very bad a change, but I presume unavoidable. I'd prefer if
such could at least be accompanied by a comment clarifying why
this mix of address spaces is correct in the specific case.

> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -23,11 +23,15 @@
>  #include <xen/page-defs.h>
>  #include <xen/spinlock.h>
>  #include <xen/pci.h>
> +#include <xen/typesafe.h>
>  #include <public/hvm/ioreq.h>
>  #include <public/domctl.h>
>  #include <asm/device.h>
>  #include <asm/iommu.h>
>  
> +TYPE_SAFE(unsigned long, bfn);
> +#define INVALID_BFN      _bfn(~0UL)

Please accompany this by a grep fodder (like the others have) and
perhaps also PRI_bfn. And while the type definition logically belongs
here, you will also want to add bfn_t with a description of its
purpose into the comment at the top of xen/mm.h. I guess you'll
need to replace / amend "host" in the MFN description there at the
same time.

I ask for this in particular because the description saying "mapped
in the IOMMU rather than the MMU" is ambiguous: Is it the input
frame number, or the output one (and things are even more
complicated when IOMMUs do two stages of translation). That in
turn affects whether I'd consider correct some of the changes
done elsewhere in this patch.

Jan


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