To ease the for-driver iommufd APIs, get_viommu_size and viommu_init ops
are introduced.
Sanitize the inputs and report the size of struct arm_vsmmu on success, in
arm_smmu_get_viommu_size().
Place the type sanity at the last, becase there will be soon an impl level
get_viommu_size op, which will require the same sanity tests prior. It can
simply insert a piece of code in front of the IOMMU_VIOMMU_TYPE_ARM_SMMUV3
sanity.
The core will ensure the viommu_type is set to the core vIOMMU object, and
pass in the same dev pointer, so arm_vsmmu_init() won't need to repeat the
same sanity tests but to simply init the arm_vsmmu struct.
Remove the arm_vsmmu_alloc, completing the replacement.
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 11 +++--
.../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 46 ++++++++++---------
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 +-
3 files changed, 32 insertions(+), 28 deletions(-)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index ea41d790463e..bb39af84e6b0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -1034,18 +1034,19 @@ struct arm_vsmmu {
#if IS_ENABLED(CONFIG_ARM_SMMU_V3_IOMMUFD)
void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type);
-struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
- struct iommu_domain *parent,
- struct iommufd_ctx *ictx,
- unsigned int viommu_type);
+size_t arm_smmu_get_viommu_size(struct device *dev,
+ enum iommu_viommu_type viommu_type);
+int arm_vsmmu_init(struct iommufd_viommu *viommu,
+ struct iommu_domain *parent_domain);
int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
struct arm_smmu_nested_domain *nested_domain);
void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state);
void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master);
int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt);
#else
+#define arm_smmu_get_viommu_size NULL
#define arm_smmu_hw_info NULL
-#define arm_vsmmu_alloc NULL
+#define arm_vsmmu_init NULL
static inline int
arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
index e4fd8d522af8..9f59c95a254c 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
@@ -382,25 +382,14 @@ static const struct iommufd_viommu_ops arm_vsmmu_ops = {
.cache_invalidate = arm_vsmmu_cache_invalidate,
};
-struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
- struct iommu_domain *parent,
- struct iommufd_ctx *ictx,
- unsigned int viommu_type)
+size_t arm_smmu_get_viommu_size(struct device *dev,
+ enum iommu_viommu_type viommu_type)
{
- struct arm_smmu_device *smmu =
- iommu_get_iommu_dev(dev, struct arm_smmu_device, iommu);
struct arm_smmu_master *master = dev_iommu_priv_get(dev);
- struct arm_smmu_domain *s2_parent = to_smmu_domain(parent);
- struct arm_vsmmu *vsmmu;
-
- if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
- return ERR_PTR(-EOPNOTSUPP);
+ struct arm_smmu_device *smmu = master->smmu;
if (!(smmu->features & ARM_SMMU_FEAT_NESTING))
- return ERR_PTR(-EOPNOTSUPP);
-
- if (s2_parent->smmu != master->smmu)
- return ERR_PTR(-EINVAL);
+ return 0;
/*
* FORCE_SYNC is not set with FEAT_NESTING. Some study of the exact HW
@@ -408,7 +397,7 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
* any change to remove this.
*/
if (WARN_ON(smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
- return ERR_PTR(-EOPNOTSUPP);
+ return 0;
/*
* Must support some way to prevent the VM from bypassing the cache
@@ -420,19 +409,32 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
*/
if (!arm_smmu_master_canwbs(master) &&
!(smmu->features & ARM_SMMU_FEAT_S2FWB))
- return ERR_PTR(-EOPNOTSUPP);
+ return 0;
- vsmmu = iommufd_viommu_alloc(ictx, struct arm_vsmmu, core,
- &arm_vsmmu_ops);
- if (IS_ERR(vsmmu))
- return ERR_CAST(vsmmu);
+ if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
+ return 0;
+
+ return VIOMMU_STRUCT_SIZE(struct arm_vsmmu, core);
+}
+
+int arm_vsmmu_init(struct iommufd_viommu *viommu,
+ struct iommu_domain *parent_domain)
+{
+ struct arm_vsmmu *vsmmu = container_of(viommu, struct arm_vsmmu, core);
+ struct arm_smmu_device *smmu =
+ container_of(viommu->iommu_dev, struct arm_smmu_device, iommu);
+ struct arm_smmu_domain *s2_parent = to_smmu_domain(parent_domain);
+
+ if (s2_parent->smmu != smmu)
+ return -EINVAL;
vsmmu->smmu = smmu;
vsmmu->s2_parent = s2_parent;
/* FIXME Move VMID allocation from the S2 domain allocation to here */
vsmmu->vmid = s2_parent->s2_cfg.vmid;
- return &vsmmu->core;
+ viommu->ops = &arm_vsmmu_ops;
+ return 0;
}
int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt)
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 10cc6dc26b7b..181d07bc1a9d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3688,7 +3688,8 @@ static struct iommu_ops arm_smmu_ops = {
.get_resv_regions = arm_smmu_get_resv_regions,
.page_response = arm_smmu_page_response,
.def_domain_type = arm_smmu_def_domain_type,
- .viommu_alloc = arm_vsmmu_alloc,
+ .get_viommu_size = arm_smmu_get_viommu_size,
+ .viommu_init = arm_vsmmu_init,
.user_pasid_table = 1,
.pgsize_bitmap = -1UL, /* Restricted during device attach */
.owner = THIS_MODULE,
--
2.43.0On Fri, Jun 13, 2025 at 11:35:22PM -0700, Nicolin Chen wrote:
> To ease the for-driver iommufd APIs, get_viommu_size and viommu_init ops
> are introduced.
>
> Sanitize the inputs and report the size of struct arm_vsmmu on success, in
> arm_smmu_get_viommu_size().
>
> Place the type sanity at the last, becase there will be soon an impl level
> get_viommu_size op, which will require the same sanity tests prior. It can
> simply insert a piece of code in front of the IOMMU_VIOMMU_TYPE_ARM_SMMUV3
> sanity.
>
That's what I was wondering, so we plan to replace the impl->vsmmu_alloc
op as well?
> The core will ensure the viommu_type is set to the core vIOMMU object, and
> pass in the same dev pointer, so arm_vsmmu_init() won't need to repeat the
> same sanity tests but to simply init the arm_vsmmu struct.
>
> Remove the arm_vsmmu_alloc, completing the replacement.
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 11 +++--
> .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 46 ++++++++++---------
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 +-
> 3 files changed, 32 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index ea41d790463e..bb39af84e6b0 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -1034,18 +1034,19 @@ struct arm_vsmmu {
>
> #if IS_ENABLED(CONFIG_ARM_SMMU_V3_IOMMUFD)
> void *arm_smmu_hw_info(struct device *dev, u32 *length, u32 *type);
> -struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
> - struct iommu_domain *parent,
> - struct iommufd_ctx *ictx,
> - unsigned int viommu_type);
> +size_t arm_smmu_get_viommu_size(struct device *dev,
> + enum iommu_viommu_type viommu_type);
> +int arm_vsmmu_init(struct iommufd_viommu *viommu,
> + struct iommu_domain *parent_domain);
> int arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
> struct arm_smmu_nested_domain *nested_domain);
> void arm_smmu_attach_commit_vmaster(struct arm_smmu_attach_state *state);
> void arm_smmu_master_clear_vmaster(struct arm_smmu_master *master);
> int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt);
> #else
> +#define arm_smmu_get_viommu_size NULL
> #define arm_smmu_hw_info NULL
> -#define arm_vsmmu_alloc NULL
> +#define arm_vsmmu_init NULL
>
> static inline int
> arm_smmu_attach_prepare_vmaster(struct arm_smmu_attach_state *state,
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> index e4fd8d522af8..9f59c95a254c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-iommufd.c
> @@ -382,25 +382,14 @@ static const struct iommufd_viommu_ops arm_vsmmu_ops = {
> .cache_invalidate = arm_vsmmu_cache_invalidate,
> };
>
> -struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
> - struct iommu_domain *parent,
> - struct iommufd_ctx *ictx,
> - unsigned int viommu_type)
> +size_t arm_smmu_get_viommu_size(struct device *dev,
> + enum iommu_viommu_type viommu_type)
> {
> - struct arm_smmu_device *smmu =
> - iommu_get_iommu_dev(dev, struct arm_smmu_device, iommu);
> struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> - struct arm_smmu_domain *s2_parent = to_smmu_domain(parent);
> - struct arm_vsmmu *vsmmu;
> -
> - if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
> - return ERR_PTR(-EOPNOTSUPP);
> + struct arm_smmu_device *smmu = master->smmu;
>
> if (!(smmu->features & ARM_SMMU_FEAT_NESTING))
> - return ERR_PTR(-EOPNOTSUPP);
> -
> - if (s2_parent->smmu != master->smmu)
> - return ERR_PTR(-EINVAL);
> + return 0;
>
> /*
> * FORCE_SYNC is not set with FEAT_NESTING. Some study of the exact HW
> @@ -408,7 +397,7 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
> * any change to remove this.
> */
> if (WARN_ON(smmu->options & ARM_SMMU_OPT_CMDQ_FORCE_SYNC))
> - return ERR_PTR(-EOPNOTSUPP);
> + return 0;
>
> /*
> * Must support some way to prevent the VM from bypassing the cache
> @@ -420,19 +409,32 @@ struct iommufd_viommu *arm_vsmmu_alloc(struct device *dev,
> */
> if (!arm_smmu_master_canwbs(master) &&
> !(smmu->features & ARM_SMMU_FEAT_S2FWB))
> - return ERR_PTR(-EOPNOTSUPP);
> + return 0;
>
> - vsmmu = iommufd_viommu_alloc(ictx, struct arm_vsmmu, core,
> - &arm_vsmmu_ops);
> - if (IS_ERR(vsmmu))
> - return ERR_CAST(vsmmu);
> + if (viommu_type != IOMMU_VIOMMU_TYPE_ARM_SMMUV3)
> + return 0;
> +
> + return VIOMMU_STRUCT_SIZE(struct arm_vsmmu, core);
> +}
> +
> +int arm_vsmmu_init(struct iommufd_viommu *viommu,
> + struct iommu_domain *parent_domain)
> +{
> + struct arm_vsmmu *vsmmu = container_of(viommu, struct arm_vsmmu, core);
> + struct arm_smmu_device *smmu =
> + container_of(viommu->iommu_dev, struct arm_smmu_device, iommu);
> + struct arm_smmu_domain *s2_parent = to_smmu_domain(parent_domain);
> +
> + if (s2_parent->smmu != smmu)
> + return -EINVAL;
>
> vsmmu->smmu = smmu;
> vsmmu->s2_parent = s2_parent;
> /* FIXME Move VMID allocation from the S2 domain allocation to here */
> vsmmu->vmid = s2_parent->s2_cfg.vmid;
>
> - return &vsmmu->core;
> + viommu->ops = &arm_vsmmu_ops;
> + return 0;
> }
Seems much better now that the driver doesn't need to callback to the
core for allocating viommu. One quick question though I see we've
removed the following too:
if (master->smmu->impl_ops &&master->smmu->impl_ops->vsmmu_alloc)
vsmmu = master->smmu->impl_ops->vsmmu_alloc(
master->smmu, s2_parent,
ictx, viommu_type,
user_data);
Not sure why don't I see that in the diffs.. do we plan to split this
into an impl-specific size and init too?
>
> int arm_vmaster_report_event(struct arm_smmu_vmaster *vmaster, u64 *evt)
> 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 10cc6dc26b7b..181d07bc1a9d 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3688,7 +3688,8 @@ static struct iommu_ops arm_smmu_ops = {
> .get_resv_regions = arm_smmu_get_resv_regions,
> .page_response = arm_smmu_page_response,
> .def_domain_type = arm_smmu_def_domain_type,
> - .viommu_alloc = arm_vsmmu_alloc,
> + .get_viommu_size = arm_smmu_get_viommu_size,
> + .viommu_init = arm_vsmmu_init,
> .user_pasid_table = 1,
> .pgsize_bitmap = -1UL, /* Restricted during device attach */
> .owner = THIS_MODULE,
> --
> 2.43.0
>
With that,
Reviewed-by: Pranjal Shrivastava <praan@google.com>
On Mon, Jun 16, 2025 at 10:43:22PM +0000, Pranjal Shrivastava wrote: > On Fri, Jun 13, 2025 at 11:35:22PM -0700, Nicolin Chen wrote: > > To ease the for-driver iommufd APIs, get_viommu_size and viommu_init ops > > are introduced. > > > > Sanitize the inputs and report the size of struct arm_vsmmu on success, in > > arm_smmu_get_viommu_size(). > > > > Place the type sanity at the last, becase there will be soon an impl level > > get_viommu_size op, which will require the same sanity tests prior. It can > > simply insert a piece of code in front of the IOMMU_VIOMMU_TYPE_ARM_SMMUV3 > > sanity. > > > > That's what I was wondering, so we plan to replace the impl->vsmmu_alloc > op as well? There is no such op in v6.16-rc1. > > - return &vsmmu->core; > > + viommu->ops = &arm_vsmmu_ops; > > + return 0; > > } > > Seems much better now that the driver doesn't need to callback to the > core for allocating viommu. One quick question though I see we've > removed the following too: > > if (master->smmu->impl_ops &&master->smmu->impl_ops->vsmmu_alloc) > vsmmu = master->smmu->impl_ops->vsmmu_alloc( > master->smmu, s2_parent, > ictx, viommu_type, > user_data); > > Not sure why don't I see that in the diffs.. do we plan to split this > into an impl-specific size and init too? Because there is no vsmmu_alloc in v6.16-rc1. I guess you are referring to older versions of HW queue (vCMDQ) series that was not merged? Thanks Nicolin
On Mon, Jun 16, 2025 at 07:15:19PM -0700, Nicolin Chen wrote: > On Mon, Jun 16, 2025 at 10:43:22PM +0000, Pranjal Shrivastava wrote: > > On Fri, Jun 13, 2025 at 11:35:22PM -0700, Nicolin Chen wrote: > > > To ease the for-driver iommufd APIs, get_viommu_size and viommu_init ops > > > are introduced. > > > > > > Sanitize the inputs and report the size of struct arm_vsmmu on success, in > > > arm_smmu_get_viommu_size(). > > > > > > Place the type sanity at the last, becase there will be soon an impl level > > > get_viommu_size op, which will require the same sanity tests prior. It can > > > simply insert a piece of code in front of the IOMMU_VIOMMU_TYPE_ARM_SMMUV3 > > > sanity. > > > > > > > That's what I was wondering, so we plan to replace the impl->vsmmu_alloc > > op as well? > > There is no such op in v6.16-rc1. > > > > - return &vsmmu->core; > > > + viommu->ops = &arm_vsmmu_ops; > > > + return 0; > > > } > > > > Seems much better now that the driver doesn't need to callback to the > > core for allocating viommu. One quick question though I see we've > > removed the following too: > > > > if (master->smmu->impl_ops &&master->smmu->impl_ops->vsmmu_alloc) > > vsmmu = master->smmu->impl_ops->vsmmu_alloc( > > master->smmu, s2_parent, > > ictx, viommu_type, > > user_data); > > > > Not sure why don't I see that in the diffs.. do we plan to split this > > into an impl-specific size and init too? > > Because there is no vsmmu_alloc in v6.16-rc1. I guess you are > referring to older versions of HW queue (vCMDQ) series that > was not merged? That's right, my bad. I rebased the older series while pulling, apologies for the confusion! Reviewed-by: Pranjal Shrivastava <praan@google.com> > > Thanks > Nicolin Thanks!
On Fri, Jun 13, 2025 at 11:35:22PM -0700, Nicolin Chen wrote: > To ease the for-driver iommufd APIs, get_viommu_size and viommu_init ops > are introduced. > > Sanitize the inputs and report the size of struct arm_vsmmu on success, in > arm_smmu_get_viommu_size(). > > Place the type sanity at the last, becase there will be soon an impl level > get_viommu_size op, which will require the same sanity tests prior. It can > simply insert a piece of code in front of the IOMMU_VIOMMU_TYPE_ARM_SMMUV3 > sanity. > > The core will ensure the viommu_type is set to the core vIOMMU object, and > pass in the same dev pointer, so arm_vsmmu_init() won't need to repeat the > same sanity tests but to simply init the arm_vsmmu struct. > > Remove the arm_vsmmu_alloc, completing the replacement. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 11 +++-- > .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 46 ++++++++++--------- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 +- > 3 files changed, 32 insertions(+), 28 deletions(-) Reviewed-by: Jason Gunthorpe <jgg@nvidia.com> Jason
On Fri, Jun 13, 2025 at 11:35:22PM -0700, Nicolin Chen wrote: > To ease the for-driver iommufd APIs, get_viommu_size and viommu_init ops > are introduced. > > Sanitize the inputs and report the size of struct arm_vsmmu on success, in > arm_smmu_get_viommu_size(). > > Place the type sanity at the last, becase there will be soon an impl level > get_viommu_size op, which will require the same sanity tests prior. It can > simply insert a piece of code in front of the IOMMU_VIOMMU_TYPE_ARM_SMMUV3 > sanity. > > The core will ensure the viommu_type is set to the core vIOMMU object, and > pass in the same dev pointer, so arm_vsmmu_init() won't need to repeat the > same sanity tests but to simply init the arm_vsmmu struct. > > Remove the arm_vsmmu_alloc, completing the replacement. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 11 +++-- > .../arm/arm-smmu-v3/arm-smmu-v3-iommufd.c | 46 ++++++++++--------- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 +- > 3 files changed, 32 insertions(+), 28 deletions(-) Acked-by: Will Deacon <will@kernel.org> Will
© 2016 - 2025 Red Hat, Inc.