Re: [Xen-devel] [PATCH v2] iommu / p2m: add a page_order parameter to iommu_map/unmap_page()

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Roger Pau Monne
> Sent: 30 October 2018 14:44
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Kevin Tian <kevin.tian@xxxxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Jun Nakajima <jun.nakajima@xxxxxxxxx>; Konrad
> Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Ian Jackson <Ian.Jackson@xxxxxxxxxx>; George
> Dunlap <George.Dunlap@xxxxxxxxxx>; Tim (Xen.org) <tim@xxxxxxx>; Julien
> Grall <julien.grall@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Subject: Re: [Xen-devel] [PATCH v2] iommu / p2m: add a page_order
> parameter to iommu_map/unmap_page()
> 
> On Mon, Oct 29, 2018 at 01:29:28PM +0000, Paul Durrant wrote:
> > The P2M code currently contains many loops to deal with the fact that,
> > while it may be require to handle page orders greater than 4k, the
> > IOMMU map and unmap functions do not.
> > This patch adds a page_order parameter to those functions and implements
> > the necessary loops within. This allows the P2M code to be substantially
> > simplified.
> >
> > NOTE: This patch does not modify the underlying vendor IOMMU
> >       implementations to deal with page orders of more than 4k.
> 
> I'm wondering if it would make sense to drop the _page suffix from
> those functions now that they take an order parameter.

Yes, that might well be a good idea at this point since I have to hit all the 
call-sites anyway.

> 
> > diff --git a/xen/drivers/passthrough/iommu.c
> b/xen/drivers/passthrough/iommu.c
> > index 8b438ae4bc..e02dcb101f 100644
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -305,47 +305,76 @@ void iommu_domain_destroy(struct domain *d)
> >  }
> >
> >  int iommu_map_page(struct domain *d, dfn_t dfn, mfn_t mfn,
> > -                   unsigned int flags)
> > +                   unsigned int page_order, unsigned int flags)
> >  {
> >      const struct domain_iommu *hd = dom_iommu(d);
> > -    int rc;
> > +    unsigned long i;
> >
> >      if ( !iommu_enabled || !hd->platform_ops )
> >          return 0;
> >
> > -    rc = hd->platform_ops->map_page(d, dfn, mfn, flags);
> > -    if ( unlikely(rc) )
> > +    ASSERT(!(dfn_x(dfn) & ((1ul << page_order) - 1)));
> > +    ASSERT(!(mfn_x(mfn) & ((1ul << page_order) - 1)));
> 
> I would consider using IS_ALIGNED for clarity.
> 

Ok.

> > +
> > +    for ( i = 0; i < (1ul << page_order); i++ )
> >      {
> > +        int ignored, err = hd->platform_ops->map_page(d, dfn_add(dfn,
> i),
> > +                                                      mfn_add(mfn, i),
> > +                                                      flags);
> > +
> > +        if ( likely(!err) )
> > +            continue;
> > +
> >          if ( !d->is_shutting_down && printk_ratelimit() )
> >              printk(XENLOG_ERR
> >                     "d%d: IOMMU mapping dfn %"PRI_dfn" to mfn %"PRI_mfn"
> failed: %d\n",
> > -                   d->domain_id, dfn_x(dfn), mfn_x(mfn), rc);
> > +                   d->domain_id, dfn_x(dfn_add(dfn, i)),
> > +                   mfn_x(mfn_add(mfn, i)), err);
> > +
> > +        while (i--)
> 
> Missing spaces in the condition.
> 

Yes. Jan mentioned this too and I forgot to fix it.

> > +            /* assign to something to avoid compiler warning */
> > +            ignored = hd->platform_ops->unmap_page(d, dfn_add(dfn, i));
> 
> You could likely declare ignored here to further limit it's scope?
> 

I'll re-work as Jan prefers.

> >
> >          if ( !is_hardware_domain(d) )
> >              domain_crash(d);
> > +
> > +        return err;
> 
> I might prefer to keep the global rc variable here and just break on
> error, also keeping the 'return rc' below as it was. But that's just a
> question of taste IMO.
> 

Ok. I'll see what that looks like. It might be nicer.

  Paul

> Thanks, Roger.

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