Message ID | 20230919092523.39286-7-yi.l.liu@intel.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:172:b0:3f2:4152:657d with SMTP id h50csp3242930vqi; Tue, 19 Sep 2023 02:27:58 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFO1QraCWsoeZqzJCSHemfMSiBH9fwgN4aek64H7cMqb+QbnbJ6+Ul9UXvzmb5zwIJ169aX X-Received: by 2002:a05:6871:4584:b0:1d6:7449:e0dc with SMTP id nl4-20020a056871458400b001d67449e0dcmr8732038oab.20.1695115677778; Tue, 19 Sep 2023 02:27:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695115677; cv=none; d=google.com; s=arc-20160816; b=VBSPizspkBZtADFaihoycow/bHDyaOs8Zf1Tzfzb/qndVuekO9qJe/MEuZLuNw+kll Dqz36VIGM/XLV8CT3Y1qmYdqMI/OUhp/Qa63zmFIdwWuICQnqntE0CGrAqwZfComN70L Kxz7ZaFWLomzNgzOQZmuhrCi1HP6/sP878l9Y0rIFzI8TaiFHBsBFDNUBBYLlMKxWIGu Omg/krUtUkNjOwpJhfldxvzo1whACdpg7BRpKPiZVyWdvF68wc2mM7nh1ItDxqmcGmn1 iMenVI9OWEH9hc7HSxLwFCCbih9rnCRyDzaAwQMQ3EG+sVGurRw6tAueE9ivIPKM8My0 ulgg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=qkK7YV873sVdW51UZUIcTDcFfYXiu8MriT05qvT7mbc=; fh=ncJBVmsnOSqrX1O37yfYEzicwaA2e7ARxnsU7aiysyE=; b=Zp9VK7Wt18xEEN9ayMpQtMnYPkUCode7k09gISyz/DdoHmHZa/RihcR198YBGvHtET Rgqpdx54f2g9dC8VDtVbKCdJBh/sH8G0RZMSocKKkRQqiLEMTC3cjKnifZQmqu068yPh dyvGvtnU9Z1y/iPyX2Pr1N6y5izPEzok5MeMDb6QSCdoJ85CHDKpvD+T7/4eMpyUmqmi waeM7By07lYv/Beem9rk9P0hfp+f5mSVnrMwtAmW1/uAkw9UYcP787erzbdiNwVtg81F blR549ASl2+xL9lmvrVRQlbrXi8SVpxU8kNzcJNYZZbXyydrcnRclfYX2P8Dall025GB rrlw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=CHAn9QRK; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from morse.vger.email (morse.vger.email. [2620:137:e000::3:1]) by mx.google.com with ESMTPS id t23-20020a656097000000b0056569ee3ae6si9098383pgu.798.2023.09.19.02.27.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 19 Sep 2023 02:27:57 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=CHAn9QRK; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by morse.vger.email (Postfix) with ESMTP id 6E31680A138A; Tue, 19 Sep 2023 02:26:43 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at morse.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229674AbjISJ0f (ORCPT <rfc822;toshivichauhan@gmail.com> + 26 others); Tue, 19 Sep 2023 05:26:35 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60348 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231436AbjISJ0W (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 19 Sep 2023 05:26:22 -0400 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.100]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 52CA9F2; Tue, 19 Sep 2023 02:26:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1695115576; x=1726651576; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=W3riUZjJN8lKSKgwdYFNtz+cVKj5BdWi3nncFILY6lA=; b=CHAn9QRKv3H3tDCiTWgLAvQCce/ncVyy1qkQ1UoZ+ThsxQWzC+w1kfXb SFwRzv3OBav2+J3ai7Ayn9IjzF+TNRR1G0zM3dqPPqW9XsAme7XgnSGlm shn8TPCMJB7GHzWBSkGw7F++rW13hhE6VTFGcyEElqrAsG3j0tTfhrVon jQkL7u2kACsx/huk51IJWALf/d4if/4CwUr2jpmLSADaTD2aljTvlX72M nAUdIyPJauSCzg6ra/zXhK+mFMiBI2n/Xhf60G64XVbegVr0gkF8H5E/l 0HksFpgr9vXCaMdJl6pfCkgTy2AKFaX7S94eTSko2j1Rh3STA8aoz8vvE w==; X-IronPort-AV: E=McAfee;i="6600,9927,10837"; a="446368969" X-IronPort-AV: E=Sophos;i="6.02,159,1688454000"; d="scan'208";a="446368969" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 Sep 2023 02:25:59 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10837"; a="722804776" X-IronPort-AV: E=Sophos;i="6.02,159,1688454000"; d="scan'208";a="722804776" Received: from 984fee00a4c6.jf.intel.com ([10.165.58.231]) by orsmga006.jf.intel.com with ESMTP; 19 Sep 2023 02:25:36 -0700 From: Yi Liu <yi.l.liu@intel.com> To: joro@8bytes.org, alex.williamson@redhat.com, jgg@nvidia.com, kevin.tian@intel.com, robin.murphy@arm.com, baolu.lu@linux.intel.com Cc: cohuck@redhat.com, eric.auger@redhat.com, nicolinc@nvidia.com, kvm@vger.kernel.org, mjrosato@linux.ibm.com, chao.p.peng@linux.intel.com, yi.l.liu@intel.com, yi.y.sun@linux.intel.com, peterx@redhat.com, jasowang@redhat.com, shameerali.kolothum.thodi@huawei.com, lulu@redhat.com, suravee.suthikulpanit@amd.com, iommu@lists.linux.dev, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, zhenzhong.duan@intel.com, joao.m.martins@oracle.com Subject: [PATCH 6/6] iommu/vt-d: Add domain_alloc_user op Date: Tue, 19 Sep 2023 02:25:23 -0700 Message-Id: <20230919092523.39286-7-yi.l.liu@intel.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20230919092523.39286-1-yi.l.liu@intel.com> References: <20230919092523.39286-1-yi.l.liu@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Tue, 19 Sep 2023 02:26:43 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1777457616960079769 X-GMAIL-MSGID: 1777457616960079769 |
Series |
iommufd support allocating nested parent domain
|
|
Commit Message
Yi Liu
Sept. 19, 2023, 9:25 a.m. UTC
This adds the domain_alloc_user op implementation. It supports allocating
domains to be used as parent under nested translation.
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
drivers/iommu/intel/iommu.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
Comments
On 9/19/23 5:25 PM, Yi Liu wrote: > This adds the domain_alloc_user op implementation. It supports allocating > domains to be used as parent under nested translation. Documentation/process/submitting-patches.rst: Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour. So how about, Add the domain_alloc_user callback to support allocating domains used as parent under nested translation. ? > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > --- > drivers/iommu/intel/iommu.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index 5db283c17e0d..491bcde1ff96 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -4074,6 +4074,25 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type) > return NULL; > } > > +static struct iommu_domain * > +intel_iommu_domain_alloc_user(struct device *dev, u32 flags) > +{ > + struct iommu_domain *domain; > + struct intel_iommu *iommu; > + > + iommu = device_to_iommu(dev, NULL, NULL); > + if (!iommu) > + return ERR_PTR(-ENODEV); > + > + if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ecap_nest(iommu->ecap)) > + return ERR_PTR(-EOPNOTSUPP); > + > + domain = iommu_domain_alloc(dev->bus); No need to bounce between core and driver. Just, intel_iommu_domain_alloc(IOMMU_DOMAIN_UNMANAGED); and fully initialize it before return. > + if (!domain) > + domain = ERR_PTR(-ENOMEM); > + return domain; > +} > + > static void intel_iommu_domain_free(struct iommu_domain *domain) > { > if (domain != &si_domain->domain && domain != &blocking_domain) > @@ -4807,6 +4826,7 @@ const struct iommu_ops intel_iommu_ops = { > .capable = intel_iommu_capable, > .hw_info = intel_iommu_hw_info, > .domain_alloc = intel_iommu_domain_alloc, > + .domain_alloc_user = intel_iommu_domain_alloc_user, > .probe_device = intel_iommu_probe_device, > .probe_finalize = intel_iommu_probe_finalize, > .release_device = intel_iommu_release_device, Best regards, baolu
On 9/19/2023 5:25 PM, Yi Liu wrote: > This adds the domain_alloc_user op implementation. It supports allocating > domains to be used as parent under nested translation. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > --- > drivers/iommu/intel/iommu.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index 5db283c17e0d..491bcde1ff96 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -4074,6 +4074,25 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type) > return NULL; > } > > +static struct iommu_domain * > +intel_iommu_domain_alloc_user(struct device *dev, u32 flags) > +{ > + struct iommu_domain *domain; > + struct intel_iommu *iommu; > + > + iommu = device_to_iommu(dev, NULL, NULL); > + if (!iommu) > + return ERR_PTR(-ENODEV); > + > + if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ecap_nest(iommu->ecap)) > + return ERR_PTR(-EOPNOTSUPP); The outer caller has checked (flags & IOMMU_HWPT_ALLOC_NEST_PARENT) before it comes here. If this callback is dedicated for nested domain allocation, then you may omit the condition here.
On Wed, Sep 20, 2023 at 01:28:41PM +0800, Baolu Lu wrote: > > > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > > index 5db283c17e0d..491bcde1ff96 100644 > > --- a/drivers/iommu/intel/iommu.c > > +++ b/drivers/iommu/intel/iommu.c > > @@ -4074,6 +4074,25 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type) > > return NULL; > > } > > +static struct iommu_domain * > > +intel_iommu_domain_alloc_user(struct device *dev, u32 flags) > > +{ > > + struct iommu_domain *domain; > > + struct intel_iommu *iommu; > > + > > + iommu = device_to_iommu(dev, NULL, NULL); > > + if (!iommu) > > + return ERR_PTR(-ENODEV); > > + > > + if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ecap_nest(iommu->ecap)) > > + return ERR_PTR(-EOPNOTSUPP); There is a check missing for supported flags if (flags & (~IOMMU_HWPT_ALLOC_NEST_PARENT)) return ERR_PTR(-EOPNOTSUPP); > > + > > + domain = iommu_domain_alloc(dev->bus); > > No need to bounce between core and driver. Just, > > intel_iommu_domain_alloc(IOMMU_DOMAIN_UNMANAGED); > > and fully initialize it before return. If you are going to do that then intel_iommu_domain_alloc() should fully initialize the domain, not here. Jason
On Wed, Sep 20, 2023 at 01:41:07PM +0800, Yang, Weijiang wrote: > On 9/19/2023 5:25 PM, Yi Liu wrote: > > This adds the domain_alloc_user op implementation. It supports allocating > > domains to be used as parent under nested translation. > > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > --- > > drivers/iommu/intel/iommu.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > > index 5db283c17e0d..491bcde1ff96 100644 > > --- a/drivers/iommu/intel/iommu.c > > +++ b/drivers/iommu/intel/iommu.c > > @@ -4074,6 +4074,25 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type) > > return NULL; > > } > > +static struct iommu_domain * > > +intel_iommu_domain_alloc_user(struct device *dev, u32 flags) > > +{ > > + struct iommu_domain *domain; > > + struct intel_iommu *iommu; > > + > > + iommu = device_to_iommu(dev, NULL, NULL); > > + if (!iommu) > > + return ERR_PTR(-ENODEV); > > + > > + if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ecap_nest(iommu->ecap)) > > + return ERR_PTR(-EOPNOTSUPP); > > The outer caller has checked (flags & IOMMU_HWPT_ALLOC_NEST_PARENT) before it comes here. > If this callback is dedicated for nested domain allocation, then you may omit the condition here. No, please don't. The point of the flags is to be passed to the driver. The driver should validate them, not the core code. We will add more flags, I don't want to change every driver to do this. Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, September 20, 2023 9:05 PM > > On Wed, Sep 20, 2023 at 01:28:41PM +0800, Baolu Lu wrote: > > > > > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > > > index 5db283c17e0d..491bcde1ff96 100644 > > > --- a/drivers/iommu/intel/iommu.c > > > +++ b/drivers/iommu/intel/iommu.c > > > @@ -4074,6 +4074,25 @@ static struct iommu_domain > *intel_iommu_domain_alloc(unsigned type) > > > return NULL; > > > } > > > +static struct iommu_domain * > > > +intel_iommu_domain_alloc_user(struct device *dev, u32 flags) > > > +{ > > > + struct iommu_domain *domain; > > > + struct intel_iommu *iommu; > > > + > > > + iommu = device_to_iommu(dev, NULL, NULL); > > > + if (!iommu) > > > + return ERR_PTR(-ENODEV); > > > + > > > + if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ecap_nest(iommu- > >ecap)) > > > + return ERR_PTR(-EOPNOTSUPP); > > There is a check missing for supported flags > > if (flags & (~IOMMU_HWPT_ALLOC_NEST_PARENT)) > return ERR_PTR(-EOPNOTSUPP); Well, the iommufd has such check. But I also noticed your another reply to Weijiang. So your preference is to do the flags validation in iommu driver instead of iommufd. Isn't it? > > > + > > > + domain = iommu_domain_alloc(dev->bus); > > > > No need to bounce between core and driver. Just, > > > > intel_iommu_domain_alloc(IOMMU_DOMAIN_UNMANAGED); > > > > and fully initialize it before return. > > If you are going to do that then intel_iommu_domain_alloc() should > fully initialize the domain, not here. I've also considered what Baolu described, but it requires to do some extra initialization which is duplicated with iommu_domain_alloc(). So I chose this simple way. Regards, Yi Liu
> From: Yang, Weijiang <weijiang.yang@intel.com> > Sent: Wednesday, September 20, 2023 1:41 PM > On 9/19/2023 5:25 PM, Yi Liu wrote: > > This adds the domain_alloc_user op implementation. It supports allocating > > domains to be used as parent under nested translation. > > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > --- > > drivers/iommu/intel/iommu.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > > index 5db283c17e0d..491bcde1ff96 100644 > > --- a/drivers/iommu/intel/iommu.c > > +++ b/drivers/iommu/intel/iommu.c > > @@ -4074,6 +4074,25 @@ static struct iommu_domain > *intel_iommu_domain_alloc(unsigned type) > > return NULL; > > } > > > > +static struct iommu_domain * > > +intel_iommu_domain_alloc_user(struct device *dev, u32 flags) > > +{ > > + struct iommu_domain *domain; > > + struct intel_iommu *iommu; > > + > > + iommu = device_to_iommu(dev, NULL, NULL); > > + if (!iommu) > > + return ERR_PTR(-ENODEV); > > + > > + if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ecap_nest(iommu- > >ecap)) > > + return ERR_PTR(-EOPNOTSUPP); > > The outer caller has checked (flags & IOMMU_HWPT_ALLOC_NEST_PARENT) before it > comes here. > If this callback is dedicated for nested domain allocation, then you may omit the > condition here. This check is different. It aims to fail the call if iommu hw does not support nested. I just realized that it may need to check if scalable mode is enabled. This should be more accurate. Regards, Yi Liu
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Wednesday, September 20, 2023 1:29 PM > > On 9/19/23 5:25 PM, Yi Liu wrote: > > This adds the domain_alloc_user op implementation. It supports allocating > > domains to be used as parent under nested translation. > > Documentation/process/submitting-patches.rst: > > Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behaviour. > > So how about, > > Add the domain_alloc_user callback to support allocating domains used as > parent under nested translation. > Sure.
On Wed, Sep 20, 2023 at 01:10:04PM +0000, Liu, Yi L wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Wednesday, September 20, 2023 9:05 PM > > > > On Wed, Sep 20, 2023 at 01:28:41PM +0800, Baolu Lu wrote: > > > > > > > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > > > > index 5db283c17e0d..491bcde1ff96 100644 > > > > --- a/drivers/iommu/intel/iommu.c > > > > +++ b/drivers/iommu/intel/iommu.c > > > > @@ -4074,6 +4074,25 @@ static struct iommu_domain > > *intel_iommu_domain_alloc(unsigned type) > > > > return NULL; > > > > } > > > > +static struct iommu_domain * > > > > +intel_iommu_domain_alloc_user(struct device *dev, u32 flags) > > > > +{ > > > > + struct iommu_domain *domain; > > > > + struct intel_iommu *iommu; > > > > + > > > > + iommu = device_to_iommu(dev, NULL, NULL); > > > > + if (!iommu) > > > > + return ERR_PTR(-ENODEV); > > > > + > > > > + if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ecap_nest(iommu- > > >ecap)) > > > > + return ERR_PTR(-EOPNOTSUPP); > > > > There is a check missing for supported flags > > > > if (flags & (~IOMMU_HWPT_ALLOC_NEST_PARENT)) > > return ERR_PTR(-EOPNOTSUPP); > > Well, the iommufd has such check. But I also noticed your another > reply to Weijiang. So your preference is to do the flags validation > in iommu driver instead of iommufd. Isn't it? The core code should check that only kernel known bits are set The driver code should check that only driver supported bits are set. Today there is only one bit so the checks are the same code. Tomorrow when we add a new bit the checks will not be the same Jason
On 9/20/23 9:10 PM, Liu, Yi L wrote: >>>> + >>>> + domain = iommu_domain_alloc(dev->bus); >>> No need to bounce between core and driver. Just, >>> >>> intel_iommu_domain_alloc(IOMMU_DOMAIN_UNMANAGED); >>> >>> and fully initialize it before return. >> If you are going to do that then intel_iommu_domain_alloc() should >> fully initialize the domain, not here. > I've also considered what Baolu described, but it requires to do some > extra initialization which is duplicated with iommu_domain_alloc(). > So I chose this simple way. Okay, got you. Once Jason's paging domain and Robin's bus->iommu_ops retirement series have landed, the VT-d driver will need some refactoring. Therefore, I'm fine with you using a simpler approach here. I'll refactor everything later. Best regards, baolu
On 2023/9/21 09:31, Baolu Lu wrote: > On 9/20/23 9:10 PM, Liu, Yi L wrote: >>>>> + >>>>> + domain = iommu_domain_alloc(dev->bus); >>>> No need to bounce between core and driver. Just, >>>> >>>> intel_iommu_domain_alloc(IOMMU_DOMAIN_UNMANAGED); >>>> >>>> and fully initialize it before return. >>> If you are going to do that then intel_iommu_domain_alloc() should >>> fully initialize the domain, not here. >> I've also considered what Baolu described, but it requires to do some >> extra initialization which is duplicated with iommu_domain_alloc(). >> So I chose this simple way. > > Okay, got you. > > Once Jason's paging domain and Robin's bus->iommu_ops retirement series > have landed, the VT-d driver will need some refactoring. Therefore, I'm > fine with you using a simpler approach here. I'll refactor everything > later. yes.
On 2023/9/20 21:18, Jason Gunthorpe wrote: > On Wed, Sep 20, 2023 at 01:10:04PM +0000, Liu, Yi L wrote: >>> From: Jason Gunthorpe <jgg@nvidia.com> >>> Sent: Wednesday, September 20, 2023 9:05 PM >>> >>> On Wed, Sep 20, 2023 at 01:28:41PM +0800, Baolu Lu wrote: >>>>> >>>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c >>>>> index 5db283c17e0d..491bcde1ff96 100644 >>>>> --- a/drivers/iommu/intel/iommu.c >>>>> +++ b/drivers/iommu/intel/iommu.c >>>>> @@ -4074,6 +4074,25 @@ static struct iommu_domain >>> *intel_iommu_domain_alloc(unsigned type) >>>>> return NULL; >>>>> } >>>>> +static struct iommu_domain * >>>>> +intel_iommu_domain_alloc_user(struct device *dev, u32 flags) >>>>> +{ >>>>> + struct iommu_domain *domain; >>>>> + struct intel_iommu *iommu; >>>>> + >>>>> + iommu = device_to_iommu(dev, NULL, NULL); >>>>> + if (!iommu) >>>>> + return ERR_PTR(-ENODEV); >>>>> + >>>>> + if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ecap_nest(iommu- >>>> ecap)) >>>>> + return ERR_PTR(-EOPNOTSUPP); >>> >>> There is a check missing for supported flags >>> >>> if (flags & (~IOMMU_HWPT_ALLOC_NEST_PARENT)) >>> return ERR_PTR(-EOPNOTSUPP); >> >> Well, the iommufd has such check. But I also noticed your another >> reply to Weijiang. So your preference is to do the flags validation >> in iommu driver instead of iommufd. Isn't it? > > The core code should check that only kernel known bits are set > > The driver code should check that only driver supported bits are set. > > Today there is only one bit so the checks are the same code. > > Tomorrow when we add a new bit the checks will not be the same fair enough. I'll have the check in both core and iommu driver. if (flags & (~IOMMU_HWPT_ALLOC_NEST_PARENT)) return ERR_PTR(-EOPNOTSUPP);
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Thursday, September 21, 2023 9:31 AM > > On 9/20/23 9:10 PM, Liu, Yi L wrote: > >>>> + > >>>> + domain = iommu_domain_alloc(dev->bus); > >>> No need to bounce between core and driver. Just, > >>> > >>> intel_iommu_domain_alloc(IOMMU_DOMAIN_UNMANAGED); > >>> > >>> and fully initialize it before return. > >> If you are going to do that then intel_iommu_domain_alloc() should > >> fully initialize the domain, not here. > > I've also considered what Baolu described, but it requires to do some > > extra initialization which is duplicated with iommu_domain_alloc(). > > So I chose this simple way. > > Okay, got you. > Please add a comment for this temporary option.
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 5db283c17e0d..491bcde1ff96 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4074,6 +4074,25 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type) return NULL; } +static struct iommu_domain * +intel_iommu_domain_alloc_user(struct device *dev, u32 flags) +{ + struct iommu_domain *domain; + struct intel_iommu *iommu; + + iommu = device_to_iommu(dev, NULL, NULL); + if (!iommu) + return ERR_PTR(-ENODEV); + + if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !ecap_nest(iommu->ecap)) + return ERR_PTR(-EOPNOTSUPP); + + domain = iommu_domain_alloc(dev->bus); + if (!domain) + domain = ERR_PTR(-ENOMEM); + return domain; +} + static void intel_iommu_domain_free(struct iommu_domain *domain) { if (domain != &si_domain->domain && domain != &blocking_domain) @@ -4807,6 +4826,7 @@ const struct iommu_ops intel_iommu_ops = { .capable = intel_iommu_capable, .hw_info = intel_iommu_hw_info, .domain_alloc = intel_iommu_domain_alloc, + .domain_alloc_user = intel_iommu_domain_alloc_user, .probe_device = intel_iommu_probe_device, .probe_finalize = intel_iommu_probe_finalize, .release_device = intel_iommu_release_device,