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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 04 December 2018 14:25
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Brian Woods <brian.woods@xxxxxxx>; Suravee Suthikulpanit
> <suravee.suthikulpanit@xxxxxxx>; Andrew Cooper
> <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne <roger.pau@xxxxxxxxxx>; Wei
> Liu <wei.liu2@xxxxxxxxxx>; xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v2 1/4] amd-iommu: add flush iommu_ops
> 
> >>> On 03.12.18 at 18:40, <paul.durrant@xxxxxxxxxx> wrote:
> > +static unsigned long flush_count(unsigned long dfn, unsigned int
> page_count,
> > +                                 unsigned int order)
> > +{
> > +    unsigned long start = dfn / (1u << order);
> > +    unsigned long end = DIV_ROUND_UP(dfn + page_count, (1u << order));
> 
> Luckily this in not in generic code, so the anomaly at the upper address
> space end is not going to surface, and in particular not cause ...
> 
> > +    ASSERT(end > start);
> 
> ... this to trigger. I therefore nevertheless wonder whether it
> would't be better to use
> 
>     unsigned long start = dfn >> order;
>     unsigned long end = (dfn + page_count - 1) >> order) + 1;
> 
> instead.

Yes, that's much better... don't know why I didn't do it that way in the first 
place.

> 
> > --- 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)
> > +{
> > +    return dfn_x(x) < dfn_x(y);
> > +}
> 
> The revision log says this is gone ...
> 

Oh. I must have messed up my git commands and accidentally put it back during 
rebase.

> With it really gone, and irrespective of the other comment
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 

Thanks.

> Of course one or both adjustments could be easily done while
> committing, provided you agree and provided there's no other
> need for a v3.

Ok, you're comments on patch #2 suggest there will probably by a v3 so I can 
fix.

  Paul

> 
> Jan
> 


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