[v7,7/8] iommu/vt-d: Add nested domain allocation

Message ID	20231024151412.50046-8-yi.l.liu@intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <kvm-owner@vger.kernel.org> 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 v7 7/8] iommu/vt-d: Add nested domain allocation Date: Tue, 24 Oct 2023 08:14:11 -0700 Message-Id: <20231024151412.50046-8-yi.l.liu@intel.com> In-Reply-To: <20231024151412.50046-1-yi.l.liu@intel.com> References: <20231024151412.50046-1-yi.l.liu@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	Add Intel VT-d nested translation (part 1/2) \| expand [v7,0/8] Add Intel VT-d nested translation (part 1/2) [v7,1/8] iommufd: Add data structure for Intel VT-d stage-1 domain allocation [v7,2/8] iommu/vt-d: Extend dmar_domain to support nested domain [v7,3/8] iommu/vt-d: Add helper for nested domain allocation [v7,4/8] iommu/vt-d: Add helper to setup pasid nested translation [v7,5/8] iommu/vt-d: Make domain attach helpers to be extern [v7,6/8] iommu/vt-d: Set the nested domain to a device [v7,7/8] iommu/vt-d: Add nested domain allocation [v7,8/8] iommu/vt-d: Disallow read-only mappings to nest parent domain

Message ID

20231024151412.50046-8-yi.l.liu@intel.com (mailing list archive)

State

New, archived

Headers

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 v7 7/8] iommu/vt-d: Add nested domain allocation
Date: Tue, 24 Oct 2023 08:14:11 -0700
Message-Id: <20231024151412.50046-8-yi.l.liu@intel.com>
In-Reply-To: <20231024151412.50046-1-yi.l.liu@intel.com>
References: <20231024151412.50046-1-yi.l.liu@intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Series

Add Intel VT-d nested translation (part 1/2) | expand

Commit Message

Yi Liu Oct. 24, 2023, 3:14 p.m. UTC

From: Lu Baolu <baolu.lu@linux.intel.com>

This adds the support for IOMMU_HWPT_DATA_VTD_S1 type. And 'nested_parent'
is added to mark the nested parent domain to sanitize the input parent domain.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c | 67 +++++++++++++++++++++++++------------
 drivers/iommu/intel/iommu.h |  1 +
 2 files changed, 46 insertions(+), 22 deletions(-)

Comments

Jason Gunthorpe Oct. 24, 2023, 11:03 p.m. UTC | #1

On Tue, Oct 24, 2023 at 08:14:11AM -0700, Yi Liu wrote:
> From: Lu Baolu <baolu.lu@linux.intel.com>
> 
> This adds the support for IOMMU_HWPT_DATA_VTD_S1 type. And 'nested_parent'
> is added to mark the nested parent domain to sanitize the input parent domain.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 67 +++++++++++++++++++++++++------------
>  drivers/iommu/intel/iommu.h |  1 +
>  2 files changed, 46 insertions(+), 22 deletions(-)

I think it should be written like this:

@@ -4077,38 +4082,39 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
 			      struct iommu_domain *parent,
 			      const struct iommu_user_data *user_data)
 {
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	bool dirty_tracking = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
+	bool nested_parent = flags & IOMMU_HWPT_ALLOC_NEST_PARENT;
+	struct intel_iommu *iommu = info->iommu;
 	struct iommu_domain *domain;
-	struct intel_iommu *iommu;
-	bool dirty_tracking;
+
+	/* Must be NESTING domain */
+	if (parent) {
+		if (!nested_supported(iommu) || flags)
+			return ERR_PTR(-EOPNOTSUPP);
+		return intel_nested_domain_alloc(parent, user_data);
+	}
 
 	if (flags &
 	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
 		return ERR_PTR(-EOPNOTSUPP);
-
-	if (parent || user_data)
+	if (nested_parent && !nested_supported(iommu))
 		return ERR_PTR(-EOPNOTSUPP);
-
-	iommu = device_to_iommu(dev, NULL, NULL);
-	if (!iommu)
-		return ERR_PTR(-ENODEV);
-
-	if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !nested_supported(iommu))
-		return ERR_PTR(-EOPNOTSUPP);
-
-	dirty_tracking = (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING);
-	if (dirty_tracking && !ssads_supported(iommu))
+	if (user_data || (dirty_tracking && !ssads_supported(iommu)))
 		return ERR_PTR(-EOPNOTSUPP);
 
 	/*
-	 * domain_alloc_user op needs to fully initialize a domain
-	 * before return, so uses iommu_domain_alloc() here for
-	 * simple.
+	 * domain_alloc_user op needs to fully initialize a domain before
+	 * return, so uses iommu_domain_alloc() here for simple.
 	 */
 	domain = iommu_domain_alloc(dev->bus);
 	if (!domain)
-		domain = ERR_PTR(-ENOMEM);
+		return ERR_PTR(-ENOMEM);
 
-	if (!IS_ERR(domain) && dirty_tracking) {
+	if (nested_parent)
+		to_dmar_domain(domain)->nested_parent = true;
+
+	if (dirty_tracking) {
 		if (to_dmar_domain(domain)->use_first_level) {
 			iommu_domain_free(domain);
 			return ERR_PTR(-EOPNOTSUPP);
@@ -4849,6 +4855,7 @@ static void *intel_iommu_hw_info(struct device *dev, u32 *length, u32 *type)
 	if (!vtd)
 		return ERR_PTR(-ENOMEM);
 
+	vtd->flags = IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17;
 	vtd->cap_reg = iommu->cap;
 	vtd->ecap_reg = iommu->ecap;
 	*length = sizeof(*vtd);
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index d9197dd72748b1..b5a5563ab32c6b 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -78,13 +78,21 @@ static const struct iommu_domain_ops intel_nested_domain_ops = {
 	.free			= intel_nested_domain_free,
 };
 
-struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *s2_domain,
+struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
 					       const struct iommu_user_data *user_data)
 {
+	struct dmar_domain *s2_domain = to_dmar_domain(parent);
 	struct iommu_hwpt_vtd_s1 vtd;
 	struct dmar_domain *domain;
 	int ret;
 
+	/* Must be nested domain */
+	if (user_data->type != IOMMU_HWPT_DATA_VTD_S1)
+		return ERR_PTR(-EOPNOTSUPP);
+	if (parent->ops != intel_iommu_ops.default_domain_ops ||
+	    !s2_domain->nested_parent)
+		return ERR_PTR(-EINVAL);
+
 	ret = iommu_copy_struct_from_user(&vtd, user_data,
 					  IOMMU_HWPT_DATA_VTD_S1, __reserved);
 	if (ret)
@@ -95,7 +103,7 @@ struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *s2_domain,
 		return ERR_PTR(-ENOMEM);
 
 	domain->use_first_level = true;
-	domain->s2_domain = to_dmar_domain(s2_domain);
+	domain->s2_domain = s2_domain;
 	domain->s1_pgtbl = vtd.pgtbl_addr;
 	domain->s1_cfg = vtd;
 	domain->domain.ops = &intel_nested_domain_ops;

Tian, Kevin Oct. 25, 2023, 7:34 a.m. UTC | #2

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, October 25, 2023 7:03 AM
>
> @@ -4849,6 +4855,7 @@ static void *intel_iommu_hw_info(struct device
> *dev, u32 *length, u32 *type)
>  	if (!vtd)
>  		return ERR_PTR(-ENOMEM);
> 
> +	vtd->flags = IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17;

this doesn't belong to this patch. otherwise looks good:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Yi Liu Oct. 25, 2023, 9:02 a.m. UTC | #3

On 2023/10/25 07:03, Jason Gunthorpe wrote:
> On Tue, Oct 24, 2023 at 08:14:11AM -0700, Yi Liu wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>
>> This adds the support for IOMMU_HWPT_DATA_VTD_S1 type. And 'nested_parent'
>> is added to mark the nested parent domain to sanitize the input parent domain.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
>> ---
>>   drivers/iommu/intel/iommu.c | 67 +++++++++++++++++++++++++------------
>>   drivers/iommu/intel/iommu.h |  1 +
>>   2 files changed, 46 insertions(+), 22 deletions(-)
> 
> I think it should be written like this:
> 
> @@ -4077,38 +4082,39 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
>   			      struct iommu_domain *parent,
>   			      const struct iommu_user_data *user_data)
>   {
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	bool dirty_tracking = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> +	bool nested_parent = flags & IOMMU_HWPT_ALLOC_NEST_PARENT;
> +	struct intel_iommu *iommu = info->iommu;
>   	struct iommu_domain *domain;
> -	struct intel_iommu *iommu;
> -	bool dirty_tracking;
> +
> +	/* Must be NESTING domain */
> +	if (parent) {
> +		if (!nested_supported(iommu) || flags)
> +			return ERR_PTR(-EOPNOTSUPP);
> +		return intel_nested_domain_alloc(parent, user_data);
> +	}
>   
>   	if (flags &
>   	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
>   		return ERR_PTR(-EOPNOTSUPP);
> -
> -	if (parent || user_data)
> +	if (nested_parent && !nested_supported(iommu))
>   		return ERR_PTR(-EOPNOTSUPP);
> -
> -	iommu = device_to_iommu(dev, NULL, NULL);
> -	if (!iommu)
> -		return ERR_PTR(-ENODEV);
> -
> -	if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !nested_supported(iommu))
> -		return ERR_PTR(-EOPNOTSUPP);
> -
> -	dirty_tracking = (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING);
> -	if (dirty_tracking && !ssads_supported(iommu))
> +	if (user_data || (dirty_tracking && !ssads_supported(iommu)))
>   		return ERR_PTR(-EOPNOTSUPP);
>   
>   	/*
> -	 * domain_alloc_user op needs to fully initialize a domain
> -	 * before return, so uses iommu_domain_alloc() here for
> -	 * simple.
> +	 * domain_alloc_user op needs to fully initialize a domain before
> +	 * return, so uses iommu_domain_alloc() here for simple.
>   	 */
>   	domain = iommu_domain_alloc(dev->bus);
>   	if (!domain)
> -		domain = ERR_PTR(-ENOMEM);
> +		return ERR_PTR(-ENOMEM);
>   
> -	if (!IS_ERR(domain) && dirty_tracking) {
> +	if (nested_parent)
> +		to_dmar_domain(domain)->nested_parent = true;
> +
> +	if (dirty_tracking) {
>   		if (to_dmar_domain(domain)->use_first_level) {
>   			iommu_domain_free(domain);
>   			return ERR_PTR(-EOPNOTSUPP);
> @@ -4849,6 +4855,7 @@ static void *intel_iommu_hw_info(struct device *dev, u32 *length, u32 *type)
>   	if (!vtd)
>   		return ERR_PTR(-ENOMEM);
>   
> +	vtd->flags = IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17;

this flag is introduced in [8/8]. still make sense to keep it there?

>   	vtd->cap_reg = iommu->cap;
>   	vtd->ecap_reg = iommu->ecap;
>   	*length = sizeof(*vtd);
> diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
> index d9197dd72748b1..b5a5563ab32c6b 100644
> --- a/drivers/iommu/intel/nested.c
> +++ b/drivers/iommu/intel/nested.c
> @@ -78,13 +78,21 @@ static const struct iommu_domain_ops intel_nested_domain_ops = {
>   	.free			= intel_nested_domain_free,
>   };
>   
> -struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *s2_domain,
> +struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
>   					       const struct iommu_user_data *user_data)
>   {
> +	struct dmar_domain *s2_domain = to_dmar_domain(parent);
>   	struct iommu_hwpt_vtd_s1 vtd;
>   	struct dmar_domain *domain;
>   	int ret;
>   
> +	/* Must be nested domain */
> +	if (user_data->type != IOMMU_HWPT_DATA_VTD_S1)
> +		return ERR_PTR(-EOPNOTSUPP);
> +	if (parent->ops != intel_iommu_ops.default_domain_ops ||
> +	    !s2_domain->nested_parent)
> +		return ERR_PTR(-EINVAL);
> +
>   	ret = iommu_copy_struct_from_user(&vtd, user_data,
>   					  IOMMU_HWPT_DATA_VTD_S1, __reserved);
>   	if (ret)
> @@ -95,7 +103,7 @@ struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *s2_domain,
>   		return ERR_PTR(-ENOMEM);
>   
>   	domain->use_first_level = true;
> -	domain->s2_domain = to_dmar_domain(s2_domain);
> +	domain->s2_domain = s2_domain;
>   	domain->s1_pgtbl = vtd.pgtbl_addr;
>   	domain->s1_cfg = vtd;
>   	domain->domain.ops = &intel_nested_domain_ops;

Jason Gunthorpe Oct. 25, 2023, 11:49 a.m. UTC | #4

On Wed, Oct 25, 2023 at 07:34:20AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > Sent: Wednesday, October 25, 2023 7:03 AM
> >
> > @@ -4849,6 +4855,7 @@ static void *intel_iommu_hw_info(struct device
> > *dev, u32 *length, u32 *type)
> >  	if (!vtd)
> >  		return ERR_PTR(-ENOMEM);
> > 
> > +	vtd->flags = IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17;
> 
> this doesn't belong to this patch. otherwise looks good:

Ah.. It seems to be some artifact of how I make this diff, it is was
fine in the actual commit.

Thanks,
Jason

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 292baa64188b..85366862fb5e 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4077,46 +4077,69 @@  intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
 			      struct iommu_domain *parent,
 			      const struct iommu_user_data *user_data)
 {
-	struct iommu_domain *domain;
 	struct intel_iommu *iommu;
 	bool dirty_tracking;
+	bool nested_parent;
 
 	if (flags &
 	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
 		return ERR_PTR(-EOPNOTSUPP);
 
-	if (parent || user_data)
-		return ERR_PTR(-EOPNOTSUPP);
-
 	iommu = device_to_iommu(dev, NULL, NULL);
 	if (!iommu)
 		return ERR_PTR(-ENODEV);
 
-	if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !nested_supported(iommu))
-		return ERR_PTR(-EOPNOTSUPP);
-
+	nested_parent = flags & IOMMU_HWPT_ALLOC_NEST_PARENT;
 	dirty_tracking = (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING);
-	if (dirty_tracking && !ssads_supported(iommu))
-		return ERR_PTR(-EOPNOTSUPP);
 
-	/*
-	 * domain_alloc_user op needs to fully initialize a domain
-	 * before return, so uses iommu_domain_alloc() here for
-	 * simple.
-	 */
-	domain = iommu_domain_alloc(dev->bus);
-	if (!domain)
-		domain = ERR_PTR(-ENOMEM);
+	if (!user_data) { /* Must be PAGING domain */
+		struct iommu_domain *domain;
 
-	if (!IS_ERR(domain) && dirty_tracking) {
-		if (to_dmar_domain(domain)->use_first_level) {
-			iommu_domain_free(domain);
+		if (nested_parent && !nested_supported(iommu))
 			return ERR_PTR(-EOPNOTSUPP);
+		if (dirty_tracking && !ssads_supported(iommu))
+			return ERR_PTR(-EOPNOTSUPP);
+		if (parent)
+			return ERR_PTR(-EINVAL);
+
+		/*
+		 * domain_alloc_user op needs to fully initialize a domain
+		 * before return, so uses iommu_domain_alloc() here for
+		 * simple.
+		 */
+		domain = iommu_domain_alloc(dev->bus);
+		if (!domain)
+			return ERR_PTR(-ENOMEM);
+
+		if (nested_parent)
+			to_dmar_domain(domain)->nested_parent = true;
+
+		if (dirty_tracking) {
+			if (to_dmar_domain(domain)->use_first_level) {
+				iommu_domain_free(domain);
+				return ERR_PTR(-EOPNOTSUPP);
+			}
+			domain->dirty_ops = &intel_dirty_ops;
 		}
-		domain->dirty_ops = &intel_dirty_ops;
+
+		return domain;
 	}
 
-	return domain;
+	/* Must be nested domain */
+	if (user_data->type != IOMMU_HWPT_DATA_VTD_S1)
+		return ERR_PTR(-EOPNOTSUPP);
+	if (!nested_supported(iommu))
+		return ERR_PTR(-EOPNOTSUPP);
+	if (!parent || parent->ops != intel_iommu_ops.default_domain_ops)
+		return ERR_PTR(-EINVAL);
+	if (!to_dmar_domain(parent)->nested_parent)
+		return ERR_PTR(-EINVAL);
+	if (nested_parent)
+		return ERR_PTR(-EINVAL);
+	if (dirty_tracking)
+		return ERR_PTR(-EINVAL);
+
+	return intel_nested_domain_alloc(parent, user_data);
 }
 
 static void intel_iommu_domain_free(struct iommu_domain *domain)
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index d5aaaedf2094..65e660eb1f47 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -601,6 +601,7 @@  struct dmar_domain {
 					 * level.
 					 */
 	u8 dirty_tracking:1;		/* Dirty tracking is enabled */
+	u8 nested_parent:1;		/* Has other domains nested on it */
 
 	spinlock_t lock;		/* Protect device tracking lists */
 	struct list_head devices;	/* all devices' list */

[v7,7/8] iommu/vt-d: Add nested domain allocation

Commit Message

Comments

Patch