| Message ID | 781b5bddb195bffa2ba80e5a04cb10336db74c03.1728491453.git.nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cover-letter: iommufd: Add vIOMMU infrastructure (Part-1) | expand |
On Wed, Oct 09, 2024 at 09:38:05AM -0700, Nicolin Chen wrote: > With a viommu object wrapping a potentially shareable S2 domain, a nested > domain should be allocated by associating to a viommu instead. > > For drivers without a viommu support, keep the parent domain input, which > should be just viommu->hwpt->common.domain otherwise. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > include/linux/iommu.h | 1 + > drivers/iommu/amd/iommu.c | 1 + > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + > drivers/iommu/intel/iommu.c | 1 + > drivers/iommu/iommufd/hw_pagetable.c | 5 +++-- > drivers/iommu/iommufd/selftest.c | 1 + > 6 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > index 3a50f57b0861..9105478bdbcd 100644 > --- a/include/linux/iommu.h > +++ b/include/linux/iommu.h > @@ -573,6 +573,7 @@ struct iommu_ops { > struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type); > struct iommu_domain *(*domain_alloc_user)( > struct device *dev, u32 flags, struct iommu_domain *parent, > + struct iommufd_viommu *viommu, > const struct iommu_user_data *user_data); This re-enforces my feeling we should have made a domain_alloc_nested().. > struct iommu_domain *(*domain_alloc_paging)(struct device *dev); > struct iommu_domain *(*domain_alloc_sva)(struct device *dev, > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > index 8364cd6fa47d..3100ddcaf62e 100644 > --- a/drivers/iommu/amd/iommu.c > +++ b/drivers/iommu/amd/iommu.c > @@ -2394,6 +2394,7 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type) > static struct iommu_domain * > amd_iommu_domain_alloc_user(struct device *dev, u32 flags, > struct iommu_domain *parent, > + struct iommufd_viommu *viommu, > const struct iommu_user_data *user_data) > > { Do these all need to check for NULL viommu now too? Something is needed at least, only valid viommus should be accepted here. Like you can't pass an ARM viommu to AMD or something silly. Should drivers accept the default viommu? Jason
On Thu, Oct 17, 2024 at 01:51:51PM -0300, Jason Gunthorpe wrote: > On Wed, Oct 09, 2024 at 09:38:05AM -0700, Nicolin Chen wrote: > > With a viommu object wrapping a potentially shareable S2 domain, a nested > > domain should be allocated by associating to a viommu instead. > > > > For drivers without a viommu support, keep the parent domain input, which > > should be just viommu->hwpt->common.domain otherwise. > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > --- > > include/linux/iommu.h | 1 + > > drivers/iommu/amd/iommu.c | 1 + > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + > > drivers/iommu/intel/iommu.c | 1 + > > drivers/iommu/iommufd/hw_pagetable.c | 5 +++-- > > drivers/iommu/iommufd/selftest.c | 1 + > > 6 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > index 3a50f57b0861..9105478bdbcd 100644 > > --- a/include/linux/iommu.h > > +++ b/include/linux/iommu.h > > @@ -573,6 +573,7 @@ struct iommu_ops { > > struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type); > > struct iommu_domain *(*domain_alloc_user)( > > struct device *dev, u32 flags, struct iommu_domain *parent, > > + struct iommufd_viommu *viommu, > > const struct iommu_user_data *user_data); > > This re-enforces my feeling we should have made a > domain_alloc_nested().. That could make these changes slightly cleaner. Maybe adding a small series prior to your initial nesting, and get it merged quickly? Assuming these nesting series can make it to this cycle, that would be in the late rc stage anyway. > > struct iommu_domain *(*domain_alloc_paging)(struct device *dev); > > struct iommu_domain *(*domain_alloc_sva)(struct device *dev, > > diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c > > index 8364cd6fa47d..3100ddcaf62e 100644 > > --- a/drivers/iommu/amd/iommu.c > > +++ b/drivers/iommu/amd/iommu.c > > @@ -2394,6 +2394,7 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type) > > static struct iommu_domain * > > amd_iommu_domain_alloc_user(struct device *dev, u32 flags, > > struct iommu_domain *parent, > > + struct iommufd_viommu *viommu, > > const struct iommu_user_data *user_data) > > > > { > > Do these all need to check for NULL viommu now too? Something is > needed at least, only valid viommus should be accepted here. Like you > can't pass an ARM viommu to AMD or something silly. These drivers don't have iommu_ops->viommu_alloc, so a viommu (!TYPE_DEFAULT) object won't reach to here. Perhaps we should block IOMMU_VIOMMU_TYPE_DEFAULT allocations if !iommu_ops->default_viommu_ops, in the core. > Should drivers accept the default viommu? Yes, driver can use default_viommu_ops. Thanks Nicolin
On Thu, Oct 17, 2024 at 10:21:31AM -0700, Nicolin Chen wrote: > On Thu, Oct 17, 2024 at 01:51:51PM -0300, Jason Gunthorpe wrote: > > On Wed, Oct 09, 2024 at 09:38:05AM -0700, Nicolin Chen wrote: > > > With a viommu object wrapping a potentially shareable S2 domain, a nested > > > domain should be allocated by associating to a viommu instead. > > > > > > For drivers without a viommu support, keep the parent domain input, which > > > should be just viommu->hwpt->common.domain otherwise. > > > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > > --- > > > include/linux/iommu.h | 1 + > > > drivers/iommu/amd/iommu.c | 1 + > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + > > > drivers/iommu/intel/iommu.c | 1 + > > > drivers/iommu/iommufd/hw_pagetable.c | 5 +++-- > > > drivers/iommu/iommufd/selftest.c | 1 + > > > 6 files changed, 8 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > > index 3a50f57b0861..9105478bdbcd 100644 > > > --- a/include/linux/iommu.h > > > +++ b/include/linux/iommu.h > > > @@ -573,6 +573,7 @@ struct iommu_ops { > > > struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type); > > > struct iommu_domain *(*domain_alloc_user)( > > > struct device *dev, u32 flags, struct iommu_domain *parent, > > > + struct iommufd_viommu *viommu, > > > const struct iommu_user_data *user_data); > > > > This re-enforces my feeling we should have made a > > domain_alloc_nested().. > > That could make these changes slightly cleaner. Maybe adding a > small series prior to your initial nesting, and get it merged > quickly? Maybe we should put an op on the viommu_ops to allocate a nested domain? It make some sense and resolves my worries about checking for driver ownership. Jason
On Thu, Oct 17, 2024 at 02:38:55PM -0300, Jason Gunthorpe wrote: > On Thu, Oct 17, 2024 at 10:21:31AM -0700, Nicolin Chen wrote: > > On Thu, Oct 17, 2024 at 01:51:51PM -0300, Jason Gunthorpe wrote: > > > On Wed, Oct 09, 2024 at 09:38:05AM -0700, Nicolin Chen wrote: > > > > With a viommu object wrapping a potentially shareable S2 domain, a nested > > > > domain should be allocated by associating to a viommu instead. > > > > > > > > For drivers without a viommu support, keep the parent domain input, which > > > > should be just viommu->hwpt->common.domain otherwise. > > > > > > > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > > > > --- > > > > include/linux/iommu.h | 1 + > > > > drivers/iommu/amd/iommu.c | 1 + > > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + > > > > drivers/iommu/intel/iommu.c | 1 + > > > > drivers/iommu/iommufd/hw_pagetable.c | 5 +++-- > > > > drivers/iommu/iommufd/selftest.c | 1 + > > > > 6 files changed, 8 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/linux/iommu.h b/include/linux/iommu.h > > > > index 3a50f57b0861..9105478bdbcd 100644 > > > > --- a/include/linux/iommu.h > > > > +++ b/include/linux/iommu.h > > > > @@ -573,6 +573,7 @@ struct iommu_ops { > > > > struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type); > > > > struct iommu_domain *(*domain_alloc_user)( > > > > struct device *dev, u32 flags, struct iommu_domain *parent, > > > > + struct iommufd_viommu *viommu, > > > > const struct iommu_user_data *user_data); > > > > > > This re-enforces my feeling we should have made a > > > domain_alloc_nested().. > > > > That could make these changes slightly cleaner. Maybe adding a > > small series prior to your initial nesting, and get it merged > > quickly? > > Maybe we should put an op on the viommu_ops to allocate a nested > domain? > > It make some sense and resolves my worries about checking for > driver ownership. Yea, that sounds a smart move to me! Will give it a try. Nicolin
diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 3a50f57b0861..9105478bdbcd 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -573,6 +573,7 @@ struct iommu_ops { struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type); struct iommu_domain *(*domain_alloc_user)( struct device *dev, u32 flags, struct iommu_domain *parent, + struct iommufd_viommu *viommu, const struct iommu_user_data *user_data); struct iommu_domain *(*domain_alloc_paging)(struct device *dev); struct iommu_domain *(*domain_alloc_sva)(struct device *dev, diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c index 8364cd6fa47d..3100ddcaf62e 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -2394,6 +2394,7 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type) static struct iommu_domain * amd_iommu_domain_alloc_user(struct device *dev, u32 flags, struct iommu_domain *parent, + struct iommufd_viommu *viommu, const struct iommu_user_data *user_data) { diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 4e559e025149..4b836a5e9fde 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -3117,6 +3117,7 @@ static struct iommu_domain arm_smmu_blocked_domain = { static struct iommu_domain * arm_smmu_domain_alloc_user(struct device *dev, u32 flags, struct iommu_domain *parent, + struct iommufd_viommu *viommu, const struct iommu_user_data *user_data) { struct arm_smmu_master *master = dev_iommu_priv_get(dev); diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 9f6b0780f2ef..a8a66a954c27 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -3522,6 +3522,7 @@ static struct iommu_domain *intel_iommu_domain_alloc(unsigned type) static struct iommu_domain * intel_iommu_domain_alloc_user(struct device *dev, u32 flags, struct iommu_domain *parent, + struct iommufd_viommu *viommu, const struct iommu_user_data *user_data) { struct device_domain_info *info = dev_iommu_priv_get(dev); diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c index d06bf6e6c19f..77a1d30031d2 100644 --- a/drivers/iommu/iommufd/hw_pagetable.c +++ b/drivers/iommu/iommufd/hw_pagetable.c @@ -137,7 +137,7 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas, if (ops->domain_alloc_user) { hwpt->domain = ops->domain_alloc_user(idev->dev, flags, NULL, - user_data); + NULL, user_data); if (IS_ERR(hwpt->domain)) { rc = PTR_ERR(hwpt->domain); hwpt->domain = NULL; @@ -240,7 +240,8 @@ iommufd_hwpt_nested_alloc(struct iommufd_ctx *ictx, hwpt->domain = ops->domain_alloc_user(idev->dev, flags & ~IOMMU_HWPT_FAULT_ID_VALID, - parent->common.domain, user_data); + parent->common.domain, + NULL, user_data); if (IS_ERR(hwpt->domain)) { rc = PTR_ERR(hwpt->domain); hwpt->domain = NULL; diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index 540437be168a..f4be87b49447 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -319,6 +319,7 @@ __mock_domain_alloc_nested(struct mock_iommu_domain *mock_parent, static struct iommu_domain * mock_domain_alloc_user(struct device *dev, u32 flags, struct iommu_domain *parent, + struct iommufd_viommu *viommu, const struct iommu_user_data *user_data) { struct mock_iommu_domain *mock_parent;
With a viommu object wrapping a potentially shareable S2 domain, a nested domain should be allocated by associating to a viommu instead. For drivers without a viommu support, keep the parent domain input, which should be just viommu->hwpt->common.domain otherwise. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- include/linux/iommu.h | 1 + drivers/iommu/amd/iommu.c | 1 + drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 + drivers/iommu/intel/iommu.c | 1 + drivers/iommu/iommufd/hw_pagetable.c | 5 +++-- drivers/iommu/iommufd/selftest.c | 1 + 6 files changed, 8 insertions(+), 2 deletions(-)