[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Tue, 28 Jun 2022 15:08:45 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=B3SwzpiVtop0VuA72AvRWz3+VKwiDk7gvmWxnGRbUwo=; b=Ze9epkjFjtBEG1IafI9XfSCM72FaQtnwiCk3pbmU2FsLSFTMcGVnb/Yyzcjp6dZJNSYJJ3q2ogbLRlafNBQJgSEcTXWSMoWN2qoAtMsc198LAOQbf7khsT7cYel45TQDmoNXCDlMbFc8kLPVU2yEVsr8SDGmgXVztACPiRhOqQReJDhTgq7AmcgOQT7K+UGZypu1rh8vAV/ufpPka1Lk6GwEL9sh3raqWBFTNkPApgP8IugBL9rkyZpxcmqUVsxEpA0+7EnoltQNZt5smUj7ScojdOiSJra11n9/J5IGnACEYJtVJf9KvzLqBCLJRfW+xtS16GCf9rB1NuCKqal/Eg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=JnFGzWE53ILN7PrAKAlv0vvuYax7yV0hmLFIWx/qa1IeZP3Rt4USO/BoO1S2PjjYZhRI9ursCoJPi/6Y5qtUVrkVVhQOMumuPrDp4sfxXF3iljG3KJEf7PqsyCIq6vgsGr4OXifzfa1cfMGm/NRigLnwDxXz6TdLYJi2SjwwwFSTNvatFKyyuI6+U7sj5Ecpyl/G28kvhztCZk+J/7GYor+gA/3YB8FF9MNjhZEAFjm2nGarwI/p4QYjDvJvPpIiH8xLxu9SOLiuPSp3RifQoi9+qs+7CxXx458yKse61pgZRNLjRuSYrT4SqlF/gmenKAXkcH84cxba0GYUTvPgGA==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=citrix.com;
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Paul Durrant <paul@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Tue, 28 Jun 2022 13:09:06 +0000
  • Ironport-data: A9a23:dOIASawatEYGT6o8oLN6t+dNxyrEfRIJ4+MujC+fZmUNrF6WrkVVx mccCzjVb/6PMGWjf4p3bIm09UsEv5PQydQySQdrrCAxQypGp/SeCIXCJC8cHc8zwu4v7q5Dx 59DAjUVBJlsFhcwnj/0bv656yMUOZigHtIQMsadUsxKbVIiGX1JZS5LwbZj2NY224jhWmthh PupyyHhEA79s9JLGjp8B5Kr8HuDa9yr5Vv0FnRnDRx6lAe2e0s9VfrzFonoR5fMeaFGH/bSe gr25OrRElU1XfsaIojNfr7TKiXmS1NJVOSEoiI+t6OK2nCuqsGuu0qS2TV1hUp/0l20c95NJ Nplus21FwwXN7D2puVNSzxUFz0nO4tp9+qSSZS/mZT7I0zuVVLJmqwrIGRoeIoS96BwHH1E8 uEeJHYVdBefiumqwbW9DO5xmsAkK8qtN4Qa0p1i5WiBUbB6HtaeHOOTuoEwMDQY36iiGd7EY MUUc3x3ZQnoaBxTIFYHTpk5mY9Eg1GgKGwB+Q/I/sLb5UDS1xd/ienyDub2XfWORt4FlUaHt E/ZqjGR7hYycYb3JSC+2nCmi/LLnCj7cJkPD7D+/flv6HWMwkQDBRtQUkG0ydGhg1O6c8JSL QoT4CVGhbg/8gmnQ8fwWzW8oWWYpVgMVtxICeo45QqRjK3O7G6k6nMsSzdAbJkqsZEwTDlzj 1uRxYq2W3poraGfTm+b+vGMtzSuNCMJLGgEIygZUQ8C5Nqlq4Y25v7Scute/GeOpoWdMVnNL /qi9kDSW517YRY36piG
  • Ironport-hdrordr: A9a23:69cW6KHFWkmI86d6pLqFc5HXdLJyesId70hD6qkvc3Fom52j/f xGws5x6faVslkssb8b6LW90Y27MAvhHPlOkPIs1NaZLXDbUQ6TQL2KgrGD/9SNIVycygcZ79 YbT0EcMqyOMbEZt7ec3ODQKb9Jrri6GeKT9IHjJh9WPH1XgspbnmNE42igYy9LrF4sP+tFKH PQ3LswmxOQPVAsKuirDHgMWObO4/XNiZLdeBYDQzoq8hOHgz+E4KPzV0Hw5GZXbxp/hZMZtU TVmQ3w4auu99m91x/nzmfWq7BbgsHoxNdvDNGFzuIVNjLvoAC1Y5kJYczKgBkF5MWUrHo6mt jFpBkte+x19nPqZ2mw5SDg3gHxuQxenkPK+Bu9uz/OsMb5TDU1B45qnoRCaCbU7EImoZVVzL 9L93jxjesaMTrw2ADGo/TYXRBjkUS55VA4l/QIsnBZWYwCLJdMsI0k+l9PGptoJlO21GkeKp ghMCjg3ocWTbvDBEqp/lWHgebcFEjbJy32DXTr4aeuontrdHMQ9Tpr+CVQpAZDyHsHceg02w 31CNUXqFhwdL5nUUtcPpZ0fSLlMB27fTv8dESvHH/AKIYrf1rwlr+f2sRH2AjtQu1C8KcP
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.