Re: [PATCH] iommu: add preemption support to iommu_{un,}map()

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

On Thu, Jun 23, 2022 at 11:49:00AM +0200, Jan Beulich wrote:
> On 10.06.2022 10:32, Roger Pau Monne wrote:
> > The loop in iommu_{un,}map() can be arbitrary large, and as such it
> > needs to handle preemption.  Introduce a new parameter that allow
> > returning the number of pages that have been processed, and which
> > presence also signals whether the function should do preemption
> > checks.
> > 
> > Note that the cleanup done in iommu_map() can now be incomplete if
> > preemption has happened, and hence callers would need to take care of
> > unmapping the whole range (ie: ranges already mapped by previously
> > preempted calls).  So far none of the callers care about having those
> > ranges unmapped, so error handling in iommu_memory_setup() and
> > arch_iommu_hwdom_init() can be kept as-is.
> > 
> > Note that iommu_legacy_{un,}map() is left without preemption handling:
> > callers of those interfaces are not modified to pass bigger chunks,
> > and hence the functions won't be modified as are legacy and should be
> > replaced with iommu_{un,}map() instead if preemption is required.
> > 
> > Fixes: f3185c165d ('IOMMU/x86: perform PV Dom0 mappings in batches')
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> >  xen/arch/x86/pv/dom0_build.c        | 15 ++++++++++++---
> >  xen/drivers/passthrough/iommu.c     | 26 +++++++++++++++++++-------
> >  xen/drivers/passthrough/x86/iommu.c | 13 +++++++++++--
> >  xen/include/xen/iommu.h             |  4 ++--
> >  4 files changed, 44 insertions(+), 14 deletions(-)
> 
> I'm a little confused, I guess: On irc you did, if I'm not mistaken,
> say you'd post what you have, but that would be incomplete. Now this
> looks pretty complete when leaving aside the fact that the referenced
> commit has meanwhile been reverted, and there's also no post-commit-
> message remark towards anything else that needs doing. I'd like to
> include this change in the next version of my series (ahead of the
> previously reverted change), doing the re-basing as necessary. But
> for that I first need to understand the state of this change.

It ended up not being as complicated as I thought at first, and hence
the change seemed correct.  I've posted it quickly without realizing
that you had already reverted the original change, and hence might
have sharp edges.

> > @@ -327,6 +327,12 @@ int iommu_map(struct domain *d, dfn_t dfn0, mfn_t mfn0,
> >          dfn_t dfn = dfn_add(dfn0, i);
> >          mfn_t mfn = mfn_add(mfn0, i);
> >  
> > +        if ( done && !(++j & 0xfffff) && general_preempt_check() )
> 
> 0xfffff seems rather high to me; I'd be inclined to move down to 0xffff
> or even 0xfff.

That's fine.  I picked this from arch_iommu_hwdom_init().  We might
want to adjust the check there.

> > --- a/xen/include/xen/iommu.h
> > +++ b/xen/include/xen/iommu.h
> > @@ -155,10 +155,10 @@ enum
> >  
> >  int __must_check iommu_map(struct domain *d, dfn_t dfn, mfn_t mfn,
> >                             unsigned long page_count, unsigned int flags,
> > -                           unsigned int *flush_flags);
> > +                           unsigned int *flush_flags, unsigned long *done);
> >  int __must_check iommu_unmap(struct domain *d, dfn_t dfn,
> >                               unsigned long page_count,
> > -                             unsigned int *flush_flags);
> > +                             unsigned int *flush_flags, unsigned long 
> > *done);
> 
> While I'm okay with adding a 6th parameter to iommu_unmap(), I'm afraid
> I don't really like adding a 7th one to iommu_map(). I'd instead be
> inclined to overload the return values of both functions, with positive
> values indicating "partially done, this many completed".

We need to be careful then so that the returned value is not
overflowed by the input count of pages, which is of type unsigned
long.

> The 6th
> parameter of iommu_unmap() would then be a "flags" one, with one bit
> identifying whether preemption is to be checked for. Thoughts?

Seems fine, but we migth want to do the same for iommu_unmap() in
order to keep a consistent interface between both?  Not strictly
required, but it's always better in order to avoid mistakes.

Are you OK with doing the changes and incorporating into your series?

Thanks, Roger.