diff mbox series

[06/17] iommufd/hw_pagetable: Use domain_alloc_user op for domain allocation

Message ID 20230209043153.14964-7-yi.l.liu@intel.com (mailing list archive)
State New
Headers show
Series Add Intel VT-d nested translation | expand

Commit Message

Yi Liu Feb. 9, 2023, 4:31 a.m. UTC 
This converts iommufd to use iommu_domain_alloc_user() for iommu_domain
creation.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommufd/hw_pagetable.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Matthew Rosato Feb. 9, 2023, 5:59 p.m. UTC | #1 
On 2/8/23 11:31 PM, Yi Liu wrote:
> This converts iommufd to use iommu_domain_alloc_user() for iommu_domain
> creation.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> ---
>  drivers/iommu/iommufd/hw_pagetable.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
> index 43d473989a06..08d963ee38c7 100644
> --- a/drivers/iommu/iommufd/hw_pagetable.c
> +++ b/drivers/iommu/iommufd/hw_pagetable.c
> @@ -30,6 +30,7 @@ struct iommufd_hw_pagetable *
>  iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
>  			   struct device *dev)
>  {
> +	const struct iommu_ops *ops;
>  	struct iommufd_hw_pagetable *hwpt;
>  	int rc;
>  
> @@ -37,7 +38,13 @@ iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
>  	if (IS_ERR(hwpt))
>  		return hwpt;
>  
> -	hwpt->domain = iommu_domain_alloc(dev->bus);
> +	ops = dev_iommu_ops(dev);
> +	if (!ops || !ops->domain_alloc_user) {
> +		rc = -EOPNOTSUPP;
> +		goto out_abort;
> +	}

Hi Yi,

This seems to break the iommufd vfio container support for any iommu that hasn't implemented domain_alloc_user yet.

I noticed it using vfio-pci on s390 with

CONFIG_IOMMUFD=m
CONFIG_IOMMUFD_VFIO_CONTAINER=y
CONFIG_VFIO_GROUP=y

Not sure if the intent is to make domain_alloc_user a hard requirement for using iommufd (if so then the commit description really should highlight that).  Otherwise, conditionally calling iommu_domain_alloc(dev->bus) when !ops->domain_alloc_user (instead of returning -EOPNOTSUPP) seems to restore the prior functionality for me.

Thanks,
Matt
Jason Gunthorpe Feb. 9, 2023, 6:36 p.m. UTC | #2 
On Thu, Feb 09, 2023 at 12:59:58PM -0500, Matthew Rosato wrote:
> really should highlight that).  Otherwise, conditionally calling
> iommu_domain_alloc(dev->bus) when !ops->domain_alloc_user (instead
> of returning -EOPNOTSUPP) seems to restore the prior functionality
> for me.

Yes, that is right if the input user data is 0 length or full of 0s
then we should call the normal driver function

Jason
Nicolin Chen Feb. 9, 2023, 7:51 p.m. UTC | #3 
On Thu, Feb 09, 2023 at 02:36:49PM -0400, Jason Gunthorpe wrote:
> On Thu, Feb 09, 2023 at 12:59:58PM -0500, Matthew Rosato wrote:
> > really should highlight that).  Otherwise, conditionally calling
> > iommu_domain_alloc(dev->bus) when !ops->domain_alloc_user (instead
> > of returning -EOPNOTSUPP) seems to restore the prior functionality
> > for me.
> 
> Yes, that is right if the input user data is 0 length or full of 0s
> then we should call the normal driver function

Maybe I am wrong, yet I recall that doing ops->domain_alloc_user
without a fallback was intentional to rule out drivers that don't
support IOMMUFD?

To be backward-compatible and concern about SMMU, we can opt in
ops->domain_alloc_user upon availability and then add a fallback:

	if ((!ops || !ops->domain_alloc_user) && user_data) {
		rc = -EOPNOTSUPP;
		goto out_abort;
	}

	if (ops->domain_alloc_user)
		hwpt->domain = ops->domain_alloc_user(dev, NULL, NULL);
	else
		hwpt->domain = iommu_domain_alloc(dev->bus);
	if (!hwpt->domain) {
		rc = -ENOMEM;
		goto out_abort;
	}

Yet, even by doing so, this series having the PATCH 07/17 that
moves iopt_table_add_domain() would temporally break IOMMUFD on
ARM platforms, until we add the ops->domain_alloc_user to SMMU
drivers.

Thanks
Nic
Jason Gunthorpe Feb. 9, 2023, 8:39 p.m. UTC | #4 
On Thu, Feb 09, 2023 at 11:51:31AM -0800, Nicolin Chen wrote:
> On Thu, Feb 09, 2023 at 02:36:49PM -0400, Jason Gunthorpe wrote:
> > On Thu, Feb 09, 2023 at 12:59:58PM -0500, Matthew Rosato wrote:
> > > really should highlight that).  Otherwise, conditionally calling
> > > iommu_domain_alloc(dev->bus) when !ops->domain_alloc_user (instead
> > > of returning -EOPNOTSUPP) seems to restore the prior functionality
> > > for me.
> > 
> > Yes, that is right if the input user data is 0 length or full of 0s
> > then we should call the normal driver function
> 
> Maybe I am wrong, yet I recall that doing ops->domain_alloc_user
> without a fallback was intentional to rule out drivers that don't
> support IOMMUFD?

I think we moved away from that to the idea of using the dma_domain
patch I suggested..

> To be backward-compatible and concern about SMMU, we can opt in
> ops->domain_alloc_user upon availability and then add a fallback:
> 
> 	if ((!ops || !ops->domain_alloc_user) && user_data) {
> 		rc = -EOPNOTSUPP;
> 		goto out_abort;
> 	}
> 
> 	if (ops->domain_alloc_user)
> 		hwpt->domain = ops->domain_alloc_user(dev, NULL, NULL);
> 	else
> 		hwpt->domain = iommu_domain_alloc(dev->bus);
> 	if (!hwpt->domain) {
> 		rc = -ENOMEM;
> 		goto out_abort;
> 	}
> 
> Yet, even by doing so, this series having the PATCH 07/17 that
> moves iopt_table_add_domain() would temporally break IOMMUFD on
> ARM platforms, until we add the ops->domain_alloc_user to SMMU
> drivers.

Drop patch 7 and 8

Change patch 12 so it has a unique flow to allocate and IOAS map a
HWPT that does not try to share so much code with the existing flow.

The HWPT flow should always just call allocate and then map with no
effort to attach first. This will fail on ARM SMMU at this point, and
that is fine.

All the existing code should work exactly as it is now and not have
any need to be changed.

Where things when wrong was trying to share
"__iommufd_hw_pagetable_alloc", I think.

Don't try to consolidate at this point. Once all the drivers are
updated then we could try to consolidate things.

Jason
Nicolin Chen Feb. 9, 2023, 10:22 p.m. UTC | #5 
On Thu, Feb 09, 2023 at 04:39:37PM -0400, Jason Gunthorpe wrote:
> On Thu, Feb 09, 2023 at 11:51:31AM -0800, Nicolin Chen wrote:
> > On Thu, Feb 09, 2023 at 02:36:49PM -0400, Jason Gunthorpe wrote:
> > > On Thu, Feb 09, 2023 at 12:59:58PM -0500, Matthew Rosato wrote:
> > > > really should highlight that).  Otherwise, conditionally calling
> > > > iommu_domain_alloc(dev->bus) when !ops->domain_alloc_user (instead
> > > > of returning -EOPNOTSUPP) seems to restore the prior functionality
> > > > for me.
> > > 
> > > Yes, that is right if the input user data is 0 length or full of 0s
> > > then we should call the normal driver function
> > 
> > Maybe I am wrong, yet I recall that doing ops->domain_alloc_user
> > without a fallback was intentional to rule out drivers that don't
> > support IOMMUFD?
> 
> I think we moved away from that to the idea of using the dma_domain
> patch I suggested..
> 
> > To be backward-compatible and concern about SMMU, we can opt in
> > ops->domain_alloc_user upon availability and then add a fallback:
> > 
> > 	if ((!ops || !ops->domain_alloc_user) && user_data) {
> > 		rc = -EOPNOTSUPP;
> > 		goto out_abort;
> > 	}
> > 
> > 	if (ops->domain_alloc_user)
> > 		hwpt->domain = ops->domain_alloc_user(dev, NULL, NULL);
> > 	else
> > 		hwpt->domain = iommu_domain_alloc(dev->bus);
> > 	if (!hwpt->domain) {
> > 		rc = -ENOMEM;
> > 		goto out_abort;
> > 	}
> > 
> > Yet, even by doing so, this series having the PATCH 07/17 that
> > moves iopt_table_add_domain() would temporally break IOMMUFD on
> > ARM platforms, until we add the ops->domain_alloc_user to SMMU
> > drivers.
> 
> Drop patch 7 and 8
> 
> Change patch 12 so it has a unique flow to allocate and IOAS map a
> HWPT that does not try to share so much code with the existing flow.
> 
> The HWPT flow should always just call allocate and then map with no
> effort to attach first. This will fail on ARM SMMU at this point, and
> that is fine.
> 
> All the existing code should work exactly as it is now and not have
> any need to be changed.
> 
> Where things when wrong was trying to share
> "__iommufd_hw_pagetable_alloc", I think.
> 
> Don't try to consolidate at this point. Once all the drivers are
> updated then we could try to consolidate things.

Yea, I think that's the only way out for now. Though I am not
sure about other drivers yet, hopefully the SMMU driver(s) is
the last one that we need to update...

Thanks
Nic
Jason Gunthorpe Feb. 9, 2023, 11:59 p.m. UTC | #6 
On Thu, Feb 09, 2023 at 02:22:26PM -0800, Nicolin Chen wrote:

> Yea, I think that's the only way out for now. Though I am not
> sure about other drivers yet, hopefully the SMMU driver(s) is
> the last one that we need to update...

I noticed apple dart had copy and pasted this from smmu, but I see
that it needed it.

Jason
Yi Liu Feb. 10, 2023, 10:50 a.m. UTC | #7 
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, February 10, 2023 6:22 AM
> 
> On Thu, Feb 09, 2023 at 04:39:37PM -0400, Jason Gunthorpe wrote:
> > On Thu, Feb 09, 2023 at 11:51:31AM -0800, Nicolin Chen wrote:
> > > On Thu, Feb 09, 2023 at 02:36:49PM -0400, Jason Gunthorpe wrote:
> > > > On Thu, Feb 09, 2023 at 12:59:58PM -0500, Matthew Rosato wrote:
> > > > > really should highlight that).  Otherwise, conditionally calling
> > > > > iommu_domain_alloc(dev->bus) when !ops->domain_alloc_user
> (instead
> > > > > of returning -EOPNOTSUPP) seems to restore the prior functionality
> > > > > for me.
> > > >
> > > > Yes, that is right if the input user data is 0 length or full of 0s
> > > > then we should call the normal driver function
> > >
> > > Maybe I am wrong, yet I recall that doing ops->domain_alloc_user
> > > without a fallback was intentional to rule out drivers that don't
> > > support IOMMUFD?
> >
> > I think we moved away from that to the idea of using the dma_domain
> > patch I suggested..
> >
> > > To be backward-compatible and concern about SMMU, we can opt in
> > > ops->domain_alloc_user upon availability and then add a fallback:
> > >
> > > 	if ((!ops || !ops->domain_alloc_user) && user_data) {
> > > 		rc = -EOPNOTSUPP;
> > > 		goto out_abort;
> > > 	}
> > >
> > > 	if (ops->domain_alloc_user)
> > > 		hwpt->domain = ops->domain_alloc_user(dev, NULL, NULL);
> > > 	else
> > > 		hwpt->domain = iommu_domain_alloc(dev->bus);
> > > 	if (!hwpt->domain) {
> > > 		rc = -ENOMEM;
> > > 		goto out_abort;
> > > 	}
> > >
> > > Yet, even by doing so, this series having the PATCH 07/17 that
> > > moves iopt_table_add_domain() would temporally break IOMMUFD on
> > > ARM platforms, until we add the ops->domain_alloc_user to SMMU
> > > drivers.
> >
> > Drop patch 7 and 8
> >
> > Change patch 12 so it has a unique flow to allocate and IOAS map a
> > HWPT that does not try to share so much code with the existing flow.
> >
> > The HWPT flow should always just call allocate and then map with no
> > effort to attach first. This will fail on ARM SMMU at this point, and
> > that is fine.

Ok. this sounds iopt_table_add_domain() should still be called
right after user hwpt is allocated instead of first device attached.

> >
> > All the existing code should work exactly as it is now and not have
> > any need to be changed.
> >
> > Where things when wrong was trying to share
> > "__iommufd_hw_pagetable_alloc", I think.
> >

Sure. 

> > Don't try to consolidate at this point. Once all the drivers are
> > updated then we could try to consolidate things.
> 
> Yea, I think that's the only way out for now. Though I am not
> sure about other drivers yet, hopefully the SMMU driver(s) is
> the last one that we need to update...
diff mbox series

Patch

diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index 43d473989a06..08d963ee38c7 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -30,6 +30,7 @@  struct iommufd_hw_pagetable *
 iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 			   struct device *dev)
 {
+	const struct iommu_ops *ops;
 	struct iommufd_hw_pagetable *hwpt;
 	int rc;
 
@@ -37,7 +38,13 @@  iommufd_hw_pagetable_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 	if (IS_ERR(hwpt))
 		return hwpt;
 
-	hwpt->domain = iommu_domain_alloc(dev->bus);
+	ops = dev_iommu_ops(dev);
+	if (!ops || !ops->domain_alloc_user) {
+		rc = -EOPNOTSUPP;
+		goto out_abort;
+	}
+
+	hwpt->domain = ops->domain_alloc_user(dev, NULL, NULL);
 	if (!hwpt->domain) {
 		rc = -ENOMEM;
 		goto out_abort;