Message ID | 7d26e897780bdc009b11bc0c0ddc7b755a769b24.1678348754.git.nicolinc@nvidia.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Add Nested Translation Support for SMMUv3 | expand |
Hi Nicolin, On 3/9/23 11:53, Nicolin Chen wrote: > The arm_smmu_domain_alloc_user callback function is used for userspace to > allocate iommu_domains, such as standalone stage-1 domain, nested stage-1 > domain, and nested stage-2 domain. The input user_data is in the type of > struct iommu_hwpt_arm_smmuv3 that contains the configurations of a nested > stage-1 or a nested stage-2 iommu_domain. A NULL user_data will just opt > in a standalone stage-1 domain allocation. > > Add a constitutive function __arm_smmu_domain_alloc to support that. > > Since ops->domain_alloc_user has a valid dev pointer, the master pointer > is available when calling __arm_smmu_domain_alloc() in this case, meaning > that arm_smmu_domain_finalise() can be done at the allocation stage. This > allows IOMMUFD to initialize the hw_pagetable for the domain. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 95 ++++++++++++++------- > 1 file changed, 65 insertions(+), 30 deletions(-) > > 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 2d29f7320570..5ff74edfbd68 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2053,36 +2053,6 @@ static void *arm_smmu_hw_info(struct device *dev, u32 *length) > return info; > } > > -static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) > -{ > - struct arm_smmu_domain *smmu_domain; > - > - if (type == IOMMU_DOMAIN_SVA) > - return arm_smmu_sva_domain_alloc(); > - > - if (type != IOMMU_DOMAIN_UNMANAGED && > - type != IOMMU_DOMAIN_DMA && > - type != IOMMU_DOMAIN_DMA_FQ && > - type != IOMMU_DOMAIN_IDENTITY) > - return NULL; > - > - /* > - * Allocate the domain and initialise some of its data structures. > - * We can't really do anything meaningful until we've added a > - * master. > - */ > - smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL); > - if (!smmu_domain) > - return NULL; > - > - mutex_init(&smmu_domain->init_mutex); > - INIT_LIST_HEAD(&smmu_domain->devices); > - spin_lock_init(&smmu_domain->devices_lock); > - INIT_LIST_HEAD(&smmu_domain->mmu_notifiers); > - > - return &smmu_domain->domain; > -} > - > static struct iommu_domain *arm_smmu_get_unmanaged_domain(struct device *dev) > { > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > @@ -2893,10 +2863,75 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid) > arm_smmu_sva_remove_dev_pasid(domain, dev, pasid); > } > > +static struct iommu_domain * > +__arm_smmu_domain_alloc(unsigned type, > + struct arm_smmu_domain *s2, > + struct arm_smmu_master *master, > + const struct iommu_hwpt_arm_smmuv3 *user_cfg) > +{ > + struct arm_smmu_domain *smmu_domain; > + struct iommu_domain *domain; > + int ret = 0; > + > + if (type == IOMMU_DOMAIN_SVA) > + return arm_smmu_sva_domain_alloc(); > + > + if (type != IOMMU_DOMAIN_UNMANAGED && > + type != IOMMU_DOMAIN_DMA && > + type != IOMMU_DOMAIN_DMA_FQ && > + type != IOMMU_DOMAIN_IDENTITY) > + return NULL; > + > + /* > + * Allocate the domain and initialise some of its data structures. > + * We can't really finalise the domain unless a master is given. > + */ > + smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL); > + if (!smmu_domain) > + return NULL; > + domain = &smmu_domain->domain; > + > + domain->type = type; > + domain->ops = arm_smmu_ops.default_domain_ops; Compared to the original code, that's something new. Please can you explain why this is added in this patch? > + > + mutex_init(&smmu_domain->init_mutex); > + INIT_LIST_HEAD(&smmu_domain->devices); > + spin_lock_init(&smmu_domain->devices_lock); > + INIT_LIST_HEAD(&smmu_domain->mmu_notifiers); > + > + if (master) { > + smmu_domain->smmu = master->smmu; > + ret = arm_smmu_domain_finalise(domain, master, user_cfg); > + if (ret) { > + kfree(smmu_domain); > + return NULL; > + } > + } > + > + return domain; > +} > + > +static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) > +{ > + return __arm_smmu_domain_alloc(type, NULL, NULL, NULL); > +} > + > +static struct iommu_domain * > +arm_smmu_domain_alloc_user(struct device *dev, struct iommu_domain *parent, > + const void *user_data) > +{ > + const struct iommu_hwpt_arm_smmuv3 *user_cfg = user_data; > + struct arm_smmu_master *master = dev_iommu_priv_get(dev); > + unsigned type = IOMMU_DOMAIN_UNMANAGED; is there any guarantee that master is non null? Don't we want to check? > + > + return __arm_smmu_domain_alloc(type, NULL, master, user_cfg); > +} > + > static struct iommu_ops arm_smmu_ops = { > .capable = arm_smmu_capable, > .hw_info = arm_smmu_hw_info, > .domain_alloc = arm_smmu_domain_alloc, > + .domain_alloc_user = arm_smmu_domain_alloc_user, > .get_unmanaged_domain = arm_smmu_get_unmanaged_domain, > .probe_device = arm_smmu_probe_device, > .release_device = arm_smmu_release_device, Thanks Eric
On 3/9/23 11:53, Nicolin Chen wrote: > The arm_smmu_domain_alloc_user callback function is used for userspace to > allocate iommu_domains, such as standalone stage-1 domain, nested stage-1 > domain, and nested stage-2 domain. The input user_data is in the type of > struct iommu_hwpt_arm_smmuv3 that contains the configurations of a nested > stage-1 or a nested stage-2 iommu_domain. A NULL user_data will just opt > in a standalone stage-1 domain allocation. > > Add a constitutive function __arm_smmu_domain_alloc to support that. > > Since ops->domain_alloc_user has a valid dev pointer, the master pointer > is available when calling __arm_smmu_domain_alloc() in this case, meaning > that arm_smmu_domain_finalise() can be done at the allocation stage. This > allows IOMMUFD to initialize the hw_pagetable for the domain. > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 95 ++++++++++++++------- > 1 file changed, 65 insertions(+), 30 deletions(-) > > 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 2d29f7320570..5ff74edfbd68 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2053,36 +2053,6 @@ static void *arm_smmu_hw_info(struct device *dev, u32 *length) > return info; > } > > -static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) > -{ > - struct arm_smmu_domain *smmu_domain; > - > - if (type == IOMMU_DOMAIN_SVA) > - return arm_smmu_sva_domain_alloc(); > - > - if (type != IOMMU_DOMAIN_UNMANAGED && > - type != IOMMU_DOMAIN_DMA && > - type != IOMMU_DOMAIN_DMA_FQ && > - type != IOMMU_DOMAIN_IDENTITY) > - return NULL; > - > - /* > - * Allocate the domain and initialise some of its data structures. > - * We can't really do anything meaningful until we've added a > - * master. > - */ > - smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL); > - if (!smmu_domain) > - return NULL; > - > - mutex_init(&smmu_domain->init_mutex); > - INIT_LIST_HEAD(&smmu_domain->devices); > - spin_lock_init(&smmu_domain->devices_lock); > - INIT_LIST_HEAD(&smmu_domain->mmu_notifiers); > - > - return &smmu_domain->domain; > -} > - > static struct iommu_domain *arm_smmu_get_unmanaged_domain(struct device *dev) > { > struct arm_smmu_master *master = dev_iommu_priv_get(dev); > @@ -2893,10 +2863,75 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid) > arm_smmu_sva_remove_dev_pasid(domain, dev, pasid); > } > > +static struct iommu_domain * > +__arm_smmu_domain_alloc(unsigned type, > + struct arm_smmu_domain *s2, I think you should rather introduce s2 param in "iommu/arm-smmu-v3: Support IOMMU_DOMAIN_NESTED type of allocations" because it is not use at all in this patch nor really explained in the commit msg Thanks Eric > + struct arm_smmu_master *master, > + const struct iommu_hwpt_arm_smmuv3 *user_cfg) > +{ > + struct arm_smmu_domain *smmu_domain; > + struct iommu_domain *domain; > + int ret = 0; > + > + if (type == IOMMU_DOMAIN_SVA) > + return arm_smmu_sva_domain_alloc(); > + > + if (type != IOMMU_DOMAIN_UNMANAGED && > + type != IOMMU_DOMAIN_DMA && > + type != IOMMU_DOMAIN_DMA_FQ && > + type != IOMMU_DOMAIN_IDENTITY) > + return NULL; > + > + /* > + * Allocate the domain and initialise some of its data structures. > + * We can't really finalise the domain unless a master is given. > + */ > + smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL); > + if (!smmu_domain) > + return NULL; > + domain = &smmu_domain->domain; > + > + domain->type = type; > + domain->ops = arm_smmu_ops.default_domain_ops; > + > + mutex_init(&smmu_domain->init_mutex); > + INIT_LIST_HEAD(&smmu_domain->devices); > + spin_lock_init(&smmu_domain->devices_lock); > + INIT_LIST_HEAD(&smmu_domain->mmu_notifiers); > + > + if (master) { > + smmu_domain->smmu = master->smmu; > + ret = arm_smmu_domain_finalise(domain, master, user_cfg); > + if (ret) { > + kfree(smmu_domain); > + return NULL; > + } > + } > + > + return domain; > +} > + > +static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) > +{ > + return __arm_smmu_domain_alloc(type, NULL, NULL, NULL); > +} > + > +static struct iommu_domain * > +arm_smmu_domain_alloc_user(struct device *dev, struct iommu_domain *parent, > + const void *user_data) > +{ > + const struct iommu_hwpt_arm_smmuv3 *user_cfg = user_data; > + struct arm_smmu_master *master = dev_iommu_priv_get(dev); > + unsigned type = IOMMU_DOMAIN_UNMANAGED; > + > + return __arm_smmu_domain_alloc(type, NULL, master, user_cfg); > +} > + > static struct iommu_ops arm_smmu_ops = { > .capable = arm_smmu_capable, > .hw_info = arm_smmu_hw_info, > .domain_alloc = arm_smmu_domain_alloc, > + .domain_alloc_user = arm_smmu_domain_alloc_user, > .get_unmanaged_domain = arm_smmu_get_unmanaged_domain, > .probe_device = arm_smmu_probe_device, > .release_device = arm_smmu_release_device,
Hi Eirc, Thanks for the review. On Fri, Mar 24, 2023 at 04:28:26PM +0100, Eric Auger wrote: > > +static struct iommu_domain * > > +__arm_smmu_domain_alloc(unsigned type, > > + struct arm_smmu_domain *s2, > > + struct arm_smmu_master *master, > > + const struct iommu_hwpt_arm_smmuv3 *user_cfg) > > +{ > > + struct arm_smmu_domain *smmu_domain; > > + struct iommu_domain *domain; > > + int ret = 0; > > + > > + if (type == IOMMU_DOMAIN_SVA) > > + return arm_smmu_sva_domain_alloc(); > > + > > + if (type != IOMMU_DOMAIN_UNMANAGED && > > + type != IOMMU_DOMAIN_DMA && > > + type != IOMMU_DOMAIN_DMA_FQ && > > + type != IOMMU_DOMAIN_IDENTITY) > > + return NULL; > > + > > + /* > > + * Allocate the domain and initialise some of its data structures. > > + * We can't really finalise the domain unless a master is given. > > + */ > > + smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL); > > + if (!smmu_domain) > > + return NULL; > > + domain = &smmu_domain->domain; > > + > > + domain->type = type; > > + domain->ops = arm_smmu_ops.default_domain_ops; > Compared to the original code, that's something new. Please can you > explain why this is added in this patch? Yea, I probably should have mentioned in the commit message that this function via ops->domain_alloc_user() is called by IOMMUFD directly without a wrapper, v.s. the other callers all go with the __iommu_domain_alloc() helper in the iommu core where an ops pointer gets setup. So, this is something new, in order to work with IOMMUFD. Thanks Nicolin
On Fri, Mar 24, 2023 at 04:33:31PM +0100, Eric Auger wrote: > > @@ -2893,10 +2863,75 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid) > > arm_smmu_sva_remove_dev_pasid(domain, dev, pasid); > > } > > > > +static struct iommu_domain * > > +__arm_smmu_domain_alloc(unsigned type, > > + struct arm_smmu_domain *s2, > I think you should rather introduce s2 param in "iommu/arm-smmu-v3: > Support IOMMU_DOMAIN_NESTED type of allocations" because it is not use > at all in this patch nor really explained in the commit msg OK. I will move it. Thanks! Nicolin
On Fri, Mar 24, 2023 at 10:40:46AM -0700, Nicolin Chen wrote: > Hi Eirc, > > Thanks for the review. > > On Fri, Mar 24, 2023 at 04:28:26PM +0100, Eric Auger wrote: > > > > +static struct iommu_domain * > > > +__arm_smmu_domain_alloc(unsigned type, > > > + struct arm_smmu_domain *s2, > > > + struct arm_smmu_master *master, > > > + const struct iommu_hwpt_arm_smmuv3 *user_cfg) > > > +{ > > > + struct arm_smmu_domain *smmu_domain; > > > + struct iommu_domain *domain; > > > + int ret = 0; > > > + > > > + if (type == IOMMU_DOMAIN_SVA) > > > + return arm_smmu_sva_domain_alloc(); > > > + > > > + if (type != IOMMU_DOMAIN_UNMANAGED && > > > + type != IOMMU_DOMAIN_DMA && > > > + type != IOMMU_DOMAIN_DMA_FQ && > > > + type != IOMMU_DOMAIN_IDENTITY) > > > + return NULL; > > > + > > > + /* > > > + * Allocate the domain and initialise some of its data structures. > > > + * We can't really finalise the domain unless a master is given. > > > + */ > > > + smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL); > > > + if (!smmu_domain) > > > + return NULL; > > > + domain = &smmu_domain->domain; > > > + > > > + domain->type = type; > > > + domain->ops = arm_smmu_ops.default_domain_ops; > > Compared to the original code, that's something new. Please can you > > explain why this is added in this patch? > > Yea, I probably should have mentioned in the commit message that > this function via ops->domain_alloc_user() is called by IOMMUFD > directly without a wrapper, v.s. the other callers all go with > the __iommu_domain_alloc() helper in the iommu core where an ops > pointer gets setup. > > So, this is something new, in order to work with IOMMUFD. But using default_domain_ops is not great, the ops should be set based on the domain type being created and the various different flavours should have their own types and ops. So name the existing ops something logical like 'unmanaged_domain_ops' and move it out of the inline initializer. Make another ops for identity like shown here to get the ball rolling: https://lore.kernel.org/r/20230324111127.221640-1-steven.price@arm.com There is a whole bunch of tidying here to make things follow the op per type design. Jason
On Fri, Mar 24, 2023 at 02:50:42PM -0300, Jason Gunthorpe wrote: > On Fri, Mar 24, 2023 at 10:40:46AM -0700, Nicolin Chen wrote: > > Hi Eirc, > > > > Thanks for the review. > > > > On Fri, Mar 24, 2023 at 04:28:26PM +0100, Eric Auger wrote: > > > > > > +static struct iommu_domain * > > > > +__arm_smmu_domain_alloc(unsigned type, > > > > + struct arm_smmu_domain *s2, > > > > + struct arm_smmu_master *master, > > > > + const struct iommu_hwpt_arm_smmuv3 *user_cfg) > > > > +{ > > > > + struct arm_smmu_domain *smmu_domain; > > > > + struct iommu_domain *domain; > > > > + int ret = 0; > > > > + > > > > + if (type == IOMMU_DOMAIN_SVA) > > > > + return arm_smmu_sva_domain_alloc(); > > > > + > > > > + if (type != IOMMU_DOMAIN_UNMANAGED && > > > > + type != IOMMU_DOMAIN_DMA && > > > > + type != IOMMU_DOMAIN_DMA_FQ && > > > > + type != IOMMU_DOMAIN_IDENTITY) > > > > + return NULL; > > > > + > > > > + /* > > > > + * Allocate the domain and initialise some of its data structures. > > > > + * We can't really finalise the domain unless a master is given. > > > > + */ > > > > + smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL); > > > > + if (!smmu_domain) > > > > + return NULL; > > > > + domain = &smmu_domain->domain; > > > > + > > > > + domain->type = type; > > > > + domain->ops = arm_smmu_ops.default_domain_ops; > > > Compared to the original code, that's something new. Please can you > > > explain why this is added in this patch? > > > > Yea, I probably should have mentioned in the commit message that > > this function via ops->domain_alloc_user() is called by IOMMUFD > > directly without a wrapper, v.s. the other callers all go with > > the __iommu_domain_alloc() helper in the iommu core where an ops > > pointer gets setup. > > > > So, this is something new, in order to work with IOMMUFD. > > But using default_domain_ops is not great, the ops should be set based > on the domain type being created and the various different flavours > should have their own types and ops. > > So name the existing ops something logical like 'unmanaged_domain_ops' > and move it out of the inline initializer. > > Make another ops for identity like shown here to get the ball rolling: > > https://lore.kernel.org/r/20230324111127.221640-1-steven.price@arm.com > > There is a whole bunch of tidying here to make things follow the op > per type design. Thanks for the suggestion. Will add a patch doing that in v2. Nicolin
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 2d29f7320570..5ff74edfbd68 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2053,36 +2053,6 @@ static void *arm_smmu_hw_info(struct device *dev, u32 *length) return info; } -static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) -{ - struct arm_smmu_domain *smmu_domain; - - if (type == IOMMU_DOMAIN_SVA) - return arm_smmu_sva_domain_alloc(); - - if (type != IOMMU_DOMAIN_UNMANAGED && - type != IOMMU_DOMAIN_DMA && - type != IOMMU_DOMAIN_DMA_FQ && - type != IOMMU_DOMAIN_IDENTITY) - return NULL; - - /* - * Allocate the domain and initialise some of its data structures. - * We can't really do anything meaningful until we've added a - * master. - */ - smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL); - if (!smmu_domain) - return NULL; - - mutex_init(&smmu_domain->init_mutex); - INIT_LIST_HEAD(&smmu_domain->devices); - spin_lock_init(&smmu_domain->devices_lock); - INIT_LIST_HEAD(&smmu_domain->mmu_notifiers); - - return &smmu_domain->domain; -} - static struct iommu_domain *arm_smmu_get_unmanaged_domain(struct device *dev) { struct arm_smmu_master *master = dev_iommu_priv_get(dev); @@ -2893,10 +2863,75 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid) arm_smmu_sva_remove_dev_pasid(domain, dev, pasid); } +static struct iommu_domain * +__arm_smmu_domain_alloc(unsigned type, + struct arm_smmu_domain *s2, + struct arm_smmu_master *master, + const struct iommu_hwpt_arm_smmuv3 *user_cfg) +{ + struct arm_smmu_domain *smmu_domain; + struct iommu_domain *domain; + int ret = 0; + + if (type == IOMMU_DOMAIN_SVA) + return arm_smmu_sva_domain_alloc(); + + if (type != IOMMU_DOMAIN_UNMANAGED && + type != IOMMU_DOMAIN_DMA && + type != IOMMU_DOMAIN_DMA_FQ && + type != IOMMU_DOMAIN_IDENTITY) + return NULL; + + /* + * Allocate the domain and initialise some of its data structures. + * We can't really finalise the domain unless a master is given. + */ + smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL); + if (!smmu_domain) + return NULL; + domain = &smmu_domain->domain; + + domain->type = type; + domain->ops = arm_smmu_ops.default_domain_ops; + + mutex_init(&smmu_domain->init_mutex); + INIT_LIST_HEAD(&smmu_domain->devices); + spin_lock_init(&smmu_domain->devices_lock); + INIT_LIST_HEAD(&smmu_domain->mmu_notifiers); + + if (master) { + smmu_domain->smmu = master->smmu; + ret = arm_smmu_domain_finalise(domain, master, user_cfg); + if (ret) { + kfree(smmu_domain); + return NULL; + } + } + + return domain; +} + +static struct iommu_domain *arm_smmu_domain_alloc(unsigned type) +{ + return __arm_smmu_domain_alloc(type, NULL, NULL, NULL); +} + +static struct iommu_domain * +arm_smmu_domain_alloc_user(struct device *dev, struct iommu_domain *parent, + const void *user_data) +{ + const struct iommu_hwpt_arm_smmuv3 *user_cfg = user_data; + struct arm_smmu_master *master = dev_iommu_priv_get(dev); + unsigned type = IOMMU_DOMAIN_UNMANAGED; + + return __arm_smmu_domain_alloc(type, NULL, master, user_cfg); +} + static struct iommu_ops arm_smmu_ops = { .capable = arm_smmu_capable, .hw_info = arm_smmu_hw_info, .domain_alloc = arm_smmu_domain_alloc, + .domain_alloc_user = arm_smmu_domain_alloc_user, .get_unmanaged_domain = arm_smmu_get_unmanaged_domain, .probe_device = arm_smmu_probe_device, .release_device = arm_smmu_release_device,
The arm_smmu_domain_alloc_user callback function is used for userspace to allocate iommu_domains, such as standalone stage-1 domain, nested stage-1 domain, and nested stage-2 domain. The input user_data is in the type of struct iommu_hwpt_arm_smmuv3 that contains the configurations of a nested stage-1 or a nested stage-2 iommu_domain. A NULL user_data will just opt in a standalone stage-1 domain allocation. Add a constitutive function __arm_smmu_domain_alloc to support that. Since ops->domain_alloc_user has a valid dev pointer, the master pointer is available when calling __arm_smmu_domain_alloc() in this case, meaning that arm_smmu_domain_finalise() can be done at the allocation stage. This allows IOMMUFD to initialize the hw_pagetable for the domain. Signed-off-by: Nicolin Chen <nicolinc@nvidia.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 95 ++++++++++++++------- 1 file changed, 65 insertions(+), 30 deletions(-)