Re: [Xen-devel] [PATCH 1/2] amd-iommu: add flush iommu_ops

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 30 November 2018 13:04
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@xxxxxxx>; Brian Woods
> <brian.woods@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH 1/2] amd-iommu: add flush iommu_ops
> 
> On 30/11/2018 10:45, Paul Durrant wrote:
> > diff --git a/xen/drivers/passthrough/amd/iommu_map.c
> b/xen/drivers/passthrough/amd/iommu_map.c
> > index 04cb7b3182..c05b042821 100644
> > --- a/xen/drivers/passthrough/amd/iommu_map.c
> > +++ b/xen/drivers/passthrough/amd/iommu_map.c
> > @@ -631,6 +631,54 @@ int amd_iommu_unmap_page(struct domain *d, dfn_t
> dfn)
> >      spin_unlock(&hd->arch.mapping_lock);
> >
> >      amd_iommu_flush_pages(d, dfn_x(dfn), 0);
> > +    return 0;
> > +}
> > +
> > +static unsigned long flush_count(dfn_t dfn, unsigned int page_count,
> > +                                 unsigned int order)
> > +{
> > +    unsigned long start = dfn_x(dfn) / (1u << order);
> > +    unsigned long end = DIV_ROUND_UP(dfn_x(dfn) + page_count,
> > +                                     (1u << order));
> > +
> > +    ASSERT(end > start);
> > +    return end - start;
> > +}
> > +
> > +int amd_iommu_flush_iotlb_pages(struct domain *d, dfn_t dfn,
> > +                                unsigned int page_count)
> 
> What are the semantics here?  Why page_count rather than order?

Because that's the way the iommu_ops method is declared in the header?

>  Are we
> guaranteed to be actually flushing consecutive dfn's ?

I believe so. Looking at the wrapper code and its callers, that seems to be the 
assumption.

> 
> > +{
> > +    /* Match VT-d semantics */
> > +    if ( !page_count || dfn_eq(dfn, INVALID_DFN) ||
> 
> Do we really have callers passing these?  I'd argue that these should be
> invalid to pass (accepting that we might need to tolerate them until
> other cleanup can occur).

I could look at cleaning that up. There aren't many callers.

> 
> > +         dfn_lt(dfn_add(dfn, page_count), dfn) /* overflow */ )
> 
> Given that all users of dfn here want it in unsigned long form, I'd make
> a local dfn_l and use that.  I'm not sure that we want to start
> accumulating a sporadic set of binary operator functions for the
> typesafe variables.
> 

Ok, I don't really mind. I thought it was neater this way though.

> > diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> > index 3d78126801..da8294bac8 100644
> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -52,6 +52,11 @@ static inline bool_t dfn_eq(dfn_t x, dfn_t y)
> >      return dfn_x(x) == dfn_x(y);
> >  }
> >
> > +static inline bool_t dfn_lt(dfn_t x, dfn_t y)
> 
> No new introductions of bool_t please.  dfn_eq() shouldn't have been
> bool_t either.

Oops, yes. Cut'n'pasted the wrong thing.

  Paul

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