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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> @@ -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.

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

Jan