Re: [PATCH v4 04/21] IOMMU: have iommu_{,un}map() split requests into largest possible chunks

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, May 03, 2022 at 04:44:45PM +0200, Jan Beulich wrote:
> On 03.05.2022 14:37, Roger Pau Monné wrote:
> > On Mon, Apr 25, 2022 at 10:33:32AM +0200, Jan Beulich wrote:
> >> --- a/xen/drivers/passthrough/iommu.c
> >> +++ b/xen/drivers/passthrough/iommu.c
> >> @@ -307,11 +338,10 @@ int iommu_map(struct domain *d, dfn_t df
> >>          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_add(dfn, i)),
> >> -                   mfn_x(mfn_add(mfn, i)), rc);
> >> +                   d->domain_id, dfn_x(dfn), mfn_x(mfn), rc);
> > 
> > Since you are already adjusting the line, I wouldn't mind if you also
> > switched to use %pd at once (and in the same adjustment done to
> > iommu_unmap).
> 
> I did consider doing so, but decided against since this would lead
> to also touching the format string (which right now is unaltered).
> 
> >>  
> >>          /* while statement to satisfy __must_check */
> >> -        while ( iommu_unmap(d, dfn, i, flush_flags) )
> >> +        while ( iommu_unmap(d, dfn0, i, flush_flags) )
> > 
> > To match previous behavior you likely need to use i + (1UL << order),
> > so pages covered by the map_page call above are also taken care in the
> > unmap request?
> 
> I'm afraid I don't follow: Prior behavior was to unmap only what
> was mapped on earlier iterations. This continues to be that way.

My bad, I was wrong and somehow assumed that the previous behavior
would also pass the failed map entry, but that's not the case.

> > With that fixed:
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Thanks, but I'll wait with applying this.

I withdraw my previous comment, feel free to apply this.

Thanks, Roger.