| Message ID | 7-v2-621370057090+91fec-smmuv3_nesting_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Initial support for SMMUv3 nested translation | expand |
On Tue, Aug 27, 2024 at 12:51:37PM -0300, Jason Gunthorpe wrote: > For SMMUv3 the parent must be a S2 domain, which can be composed > into a IOMMU_DOMAIN_NESTED. > > In future the S2 parent will also need a VMID linked to the VIOMMU and > even to KVM. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Tuesday, August 27, 2024 11:52 PM > > For SMMUv3 the parent must be a S2 domain, which can be composed > into a IOMMU_DOMAIN_NESTED. > > In future the S2 parent will also need a VMID linked to the VIOMMU and > even to KVM. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with a nit: > @@ -3103,7 +3103,8 @@ arm_smmu_domain_alloc_user(struct device *dev, > u32 flags, > const struct iommu_user_data *user_data) > { > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > - const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > + const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | > + IOMMU_HWPT_ALLOC_NEST_PARENT; lowercase for variable name.
On Fri, Aug 30, 2024 at 07:58:09AM +0000, Tian, Kevin wrote: > > @@ -3103,7 +3103,8 @@ arm_smmu_domain_alloc_user(struct device *dev, > > u32 flags, > > const struct iommu_user_data *user_data) > > { > > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > > - const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > > + const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | > > + IOMMU_HWPT_ALLOC_NEST_PARENT; > > lowercase for variable name. Ah, but it is constant :) I have no idea if there is a style consensus here Thanks, Jason
Hi Jason, On Tue, Aug 27, 2024 at 12:51:37PM -0300, Jason Gunthorpe wrote: > For SMMUv3 the parent must be a S2 domain, which can be composed > into a IOMMU_DOMAIN_NESTED. > > In future the S2 parent will also need a VMID linked to the VIOMMU and > even to KVM. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > 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 ec2fcdd4523a26..8db3db6328f8b7 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -3103,7 +3103,8 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, > const struct iommu_user_data *user_data) > { > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > - const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > + const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | > + IOMMU_HWPT_ALLOC_NEST_PARENT; > struct arm_smmu_domain *smmu_domain; > int ret; > > @@ -3116,6 +3117,14 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, > if (!smmu_domain) > return ERR_PTR(-ENOMEM); > > + if (flags & IOMMU_HWPT_ALLOC_NEST_PARENT) { > + if (!(master->smmu->features & ARM_SMMU_FEAT_NESTING)) { > + ret = -EOPNOTSUPP; I think that should be: ret = ERR_PTR(-EOPNOTSUPP); Thanks, Mostafa > + goto err_free; > + } > + smmu_domain->stage = ARM_SMMU_DOMAIN_S2; > + } > + > smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED; > smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops; > ret = arm_smmu_domain_finalise(smmu_domain, master->smmu, flags); > -- > 2.46.0 >
On Fri, Aug 30, 2024 at 03:27:00PM +0000, Mostafa Saleh wrote: > Hi Jason, > > On Tue, Aug 27, 2024 at 12:51:37PM -0300, Jason Gunthorpe wrote: > > For SMMUv3 the parent must be a S2 domain, which can be composed > > into a IOMMU_DOMAIN_NESTED. > > > > In future the S2 parent will also need a VMID linked to the VIOMMU and > > even to KVM. > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 ++++++++++- > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > 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 ec2fcdd4523a26..8db3db6328f8b7 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > @@ -3103,7 +3103,8 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, > > const struct iommu_user_data *user_data) > > { > > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > > - const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > > + const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | > > + IOMMU_HWPT_ALLOC_NEST_PARENT; > > struct arm_smmu_domain *smmu_domain; > > int ret; > > > > @@ -3116,6 +3117,14 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, > > if (!smmu_domain) > > return ERR_PTR(-ENOMEM); > > > > + if (flags & IOMMU_HWPT_ALLOC_NEST_PARENT) { > > + if (!(master->smmu->features & ARM_SMMU_FEAT_NESTING)) { > > + ret = -EOPNOTSUPP; > I think that should be: > ret = ERR_PTR(-EOPNOTSUPP); Read again :) static struct iommu_domain * arm_smmu_domain_alloc_user(struct device *dev, u32 flags, struct iommu_domain *parent, const struct iommu_user_data *user_data) { struct arm_smmu_master *master = dev_iommu_priv_get(dev); const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | IOMMU_HWPT_ALLOC_NEST_PARENT; struct arm_smmu_domain *smmu_domain; int ret; ^^^^^^^^^^^^^^ err_free: kfree(smmu_domain); return ERR_PTR(ret); ^^^^^^^^^^^^^^^^^^ Jason
On Fri, Aug 30, 2024 at 02:18:17PM -0300, Jason Gunthorpe wrote: > On Fri, Aug 30, 2024 at 03:27:00PM +0000, Mostafa Saleh wrote: > > Hi Jason, > > > > On Tue, Aug 27, 2024 at 12:51:37PM -0300, Jason Gunthorpe wrote: > > > For SMMUv3 the parent must be a S2 domain, which can be composed > > > into a IOMMU_DOMAIN_NESTED. > > > > > > In future the S2 parent will also need a VMID linked to the VIOMMU and > > > even to KVM. > > > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > --- > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 ++++++++++- > > > 1 file changed, 10 insertions(+), 1 deletion(-) > > > > > > 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 ec2fcdd4523a26..8db3db6328f8b7 100644 > > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > > @@ -3103,7 +3103,8 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, > > > const struct iommu_user_data *user_data) > > > { > > > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > > > - const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; > > > + const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | > > > + IOMMU_HWPT_ALLOC_NEST_PARENT; > > > struct arm_smmu_domain *smmu_domain; > > > int ret; > > > > > > @@ -3116,6 +3117,14 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, > > > if (!smmu_domain) > > > return ERR_PTR(-ENOMEM); > > > > > > + if (flags & IOMMU_HWPT_ALLOC_NEST_PARENT) { > > > + if (!(master->smmu->features & ARM_SMMU_FEAT_NESTING)) { > > > + ret = -EOPNOTSUPP; > > I think that should be: > > ret = ERR_PTR(-EOPNOTSUPP); > > Read again :) Oops, sorry about the noise. Thanks, Mostafa > > static struct iommu_domain * > arm_smmu_domain_alloc_user(struct device *dev, u32 flags, > struct iommu_domain *parent, > const struct iommu_user_data *user_data) > { > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | > IOMMU_HWPT_ALLOC_NEST_PARENT; > struct arm_smmu_domain *smmu_domain; > int ret; > ^^^^^^^^^^^^^^ > > err_free: > kfree(smmu_domain); > return ERR_PTR(ret); > ^^^^^^^^^^^^^^^^^^ > > Jason
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 ec2fcdd4523a26..8db3db6328f8b7 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -3103,7 +3103,8 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, const struct iommu_user_data *user_data) { struct arm_smmu_master *master = dev_iommu_priv_get(dev); - const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING; + const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING | + IOMMU_HWPT_ALLOC_NEST_PARENT; struct arm_smmu_domain *smmu_domain; int ret; @@ -3116,6 +3117,14 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags, if (!smmu_domain) return ERR_PTR(-ENOMEM); + if (flags & IOMMU_HWPT_ALLOC_NEST_PARENT) { + if (!(master->smmu->features & ARM_SMMU_FEAT_NESTING)) { + ret = -EOPNOTSUPP; + goto err_free; + } + smmu_domain->stage = ARM_SMMU_DOMAIN_S2; + } + smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED; smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops; ret = arm_smmu_domain_finalise(smmu_domain, master->smmu, flags);
For SMMUv3 the parent must be a S2 domain, which can be composed into a IOMMU_DOMAIN_NESTED. In future the S2 parent will also need a VMID linked to the VIOMMU and even to KVM. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)