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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: dunlapg@gmail.com [mailto:dunlapg@gmail.com] On Behalf Of
> George Dunlap
> Sent: 10 July 2018 14:47
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel <xen-devel@lists.xenproject.org>; Kevin Tian
> <kevin.tian@intel.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Julien Grall
> <julien.grall@arm.com>; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [Xen-devel] [PATCH v2 02/13] iommu: introduce the concept of
> BFN...
> 
> On Sat, Jul 7, 2018 at 12:05 PM, Paul Durrant <paul.durrant@citrix.com>
> 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.

I added these at Jan's request. I can defer introducing them to the later patch introducing typesafe bfns and gfns if that's preferable. The eventual code will be the same.

  Paul

> 
> >
> >         /*
> >          * 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel