Re: [Xen-devel] [PATCH v2 02/13] iommu: introduce the concept of BFN...

[George Dunlap <dunlapg@xxxxxxxxx>]

On Sat, Jul 7, 2018 at 12:05 PM, Paul Durrant <paul.durrant@xxxxxxxxxx> wrote:

> @@ -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 wider bfn now */

Oh, come on, no lager gfns? ;-)

> @@ -2737,10 +2737,12 @@ static void arm_smmu_iommu_domain_teardown(struct 
> domain *d)
>         xfree(xen_domain);
>  }
>
> -static int __must_check arm_smmu_map_page(struct domain *d, unsigned long 
> gfn,
> +static int __must_check arm_smmu_map_page(struct domain *d, unsigned long 
> bfn,
>                         unsigned long mfn, unsigned int flags)
>  {
>         p2m_type_t t;
> +       unsigned long frame = bfn;
> +       unsigned long gfn;

I don't understand what the poind of these temporary variables are.
They don't seem to really add any information at this point.

>
>         /*
>          * Grant mappings can be used for DMA requests. The dev_bus_addr
> @@ -2748,10 +2750,11 @@ static int __must_check arm_smmu_map_page(struct 
> domain *d, unsigned long gfn,
>          * protected by an IOMMU, Xen needs to add a 1:1 mapping in the domain
>          * p2m to allow DMA request to work.
>          * This is only valid when the domain is directed mapped. Hence this
> -        * function should only be used by gnttab code with gfn == mfn.
> +        * function should only be used by gnttab code with gfn == mfn == bfn.
>          */
>         BUG_ON(!is_domain_direct_mapped(d));
> -       BUG_ON(mfn != gfn);
> +       BUG_ON(mfn != frame);
> +       gfn = frame;
>
>         /* We only support readable and writable flags */
>         if (!(flags & (IOMMUF_readable | IOMMUF_writable)))
> @@ -2766,8 +2769,12 @@ static int __must_check arm_smmu_map_page(struct 
> domain *d, unsigned long gfn,
>         return guest_physmap_add_entry(d, _gfn(gfn), _mfn(mfn), 0, t);
>  }
>
> -static int __must_check arm_smmu_unmap_page(struct domain *d, unsigned long 
> gfn)
> +static int __must_check arm_smmu_unmap_page(struct domain *d, unsigned long 
> bfn)
>  {
> +       unsigned long frame = bfn;
> +       unsigned long gfn = frame;
> +       unsigned long mfn = frame;

Same here.

> diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
> index 2c44fabf99..9a3bb6a43e 100644
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -185,7 +185,8 @@ void __hwdom_init iommu_hwdom_init(struct domain *d)
>          page_list_for_each ( page, &d->page_list )
>          {
>              unsigned long mfn = mfn_x(page_to_mfn(page));
> -            unsigned long gfn = mfn_to_gmfn(d, mfn);
> +            unsigned long frame = mfn_to_gmfn(d, mfn);
> +            unsigned long bfn = frame;

And here.

Everything else looks good.

 -George

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