diff mbox series

[v1,12/14] iommu/arm-smmu-v3: Support IOMMU_DOMAIN_NESTED type of allocations

Message ID b01b2bad6d0d34908812d964eba118a9cc1e89ab.1678348754.git.nicolinc@nvidia.com
State New
Headers
Series Add Nested Translation Support for SMMUv3 |

Commit Message

Nicolin Chen March 9, 2023, 10:53 a.m. UTC 
  Add domain allocation support for IOMMU_DOMAIN_NESTED type. This includes
the "finalise" part to log in the user space Stream Table Entry info.

Co-developed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 38 +++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

Comments

Robin Murphy March 9, 2023, 1:20 p.m. UTC | #1 
On 2023-03-09 10:53, Nicolin Chen wrote:
> Add domain allocation support for IOMMU_DOMAIN_NESTED type. This includes
> the "finalise" part to log in the user space Stream Table Entry info.
> 
> Co-developed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 38 +++++++++++++++++++--
>   1 file changed, 36 insertions(+), 2 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 5ff74edfbd68..1f318b5e0921 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2214,6 +2214,19 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain,
>   		return 0;
>   	}
>   
> +	if (domain->type == IOMMU_DOMAIN_NESTED) {
> +		if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1) ||
> +		    !(smmu->features & ARM_SMMU_FEAT_TRANS_S2)) {
> +			dev_dbg(smmu->dev, "does not implement two stages\n");
> +			return -EINVAL;
> +		}
> +		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> +		smmu_domain->s1_cfg.s1fmt = user_cfg->s1fmt;
> +		smmu_domain->s1_cfg.s1cdmax = user_cfg->s1cdmax;
> +		smmu_domain->s1_cfg.cdcfg.cdtab_dma = user_cfg->s1ctxptr;
> +		return 0;

How's that going to work? If the caller's asked for something we can't 
provide, returning something else and hoping it fails later is not 
sensible, we should just fail right here. It's even more worrying if 
there's a chance it *won't* fail later, and a guest ends up with 
"nested" translation giving it full access to host PA space :/

Thanks,
Robin.

> +	}
> +
>   	if (user_cfg_s2 && !(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
>   		return -EINVAL;
>   	if (user_cfg_s2)
> @@ -2863,6 +2876,11 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>   	arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
>   }
>   
> +static const struct iommu_domain_ops arm_smmu_nested_domain_ops = {
> +	.attach_dev		= arm_smmu_attach_dev,
> +	.free			= arm_smmu_domain_free,
> +};
> +
>   static struct iommu_domain *
>   __arm_smmu_domain_alloc(unsigned type,
>   			struct arm_smmu_domain *s2,
> @@ -2877,11 +2895,15 @@ __arm_smmu_domain_alloc(unsigned type,
>   		return arm_smmu_sva_domain_alloc();
>   
>   	if (type != IOMMU_DOMAIN_UNMANAGED &&
> +	    type != IOMMU_DOMAIN_NESTED &&
>   	    type != IOMMU_DOMAIN_DMA &&
>   	    type != IOMMU_DOMAIN_DMA_FQ &&
>   	    type != IOMMU_DOMAIN_IDENTITY)
>   		return NULL;
>   
> +	if (s2 && s2->stage != ARM_SMMU_DOMAIN_S2)
> +		return NULL;
> +
>   	/*
>   	 * Allocate the domain and initialise some of its data structures.
>   	 * We can't really finalise the domain unless a master is given.
> @@ -2889,10 +2911,14 @@ __arm_smmu_domain_alloc(unsigned type,
>   	smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL);
>   	if (!smmu_domain)
>   		return NULL;
> +	smmu_domain->s2 = s2;
>   	domain = &smmu_domain->domain;
>   
>   	domain->type = type;
> -	domain->ops = arm_smmu_ops.default_domain_ops;
> +	if (s2)
> +		domain->ops = &arm_smmu_nested_domain_ops;
> +	else
> +		domain->ops = arm_smmu_ops.default_domain_ops;
>   
>   	mutex_init(&smmu_domain->init_mutex);
>   	INIT_LIST_HEAD(&smmu_domain->devices);
> @@ -2923,8 +2949,16 @@ arm_smmu_domain_alloc_user(struct device *dev, struct iommu_domain *parent,
>   	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;
> +	struct arm_smmu_domain *s2 = NULL;
> +
> +	if (parent) {
> +		if (parent->ops != arm_smmu_ops.default_domain_ops)
> +			return NULL;
> +		type = IOMMU_DOMAIN_NESTED;
> +		s2 = to_smmu_domain(parent);
> +	}
>   
> -	return __arm_smmu_domain_alloc(type, NULL, master, user_cfg);
> +	return __arm_smmu_domain_alloc(type, s2, master, user_cfg);
>   }
>   
>   static struct iommu_ops arm_smmu_ops = {
Robin Murphy March 9, 2023, 2:28 p.m. UTC | #2 
On 2023-03-09 13:20, Robin Murphy wrote:
> On 2023-03-09 10:53, Nicolin Chen wrote:
>> Add domain allocation support for IOMMU_DOMAIN_NESTED type. This includes
>> the "finalise" part to log in the user space Stream Table Entry info.
>>
>> Co-developed-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 38 +++++++++++++++++++--
>>   1 file changed, 36 insertions(+), 2 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 5ff74edfbd68..1f318b5e0921 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -2214,6 +2214,19 @@ static int arm_smmu_domain_finalise(struct 
>> iommu_domain *domain,
>>           return 0;
>>       }
>> +    if (domain->type == IOMMU_DOMAIN_NESTED) {
>> +        if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1) ||
>> +            !(smmu->features & ARM_SMMU_FEAT_TRANS_S2)) {
>> +            dev_dbg(smmu->dev, "does not implement two stages\n");
>> +            return -EINVAL;
>> +        }
>> +        smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
>> +        smmu_domain->s1_cfg.s1fmt = user_cfg->s1fmt;
>> +        smmu_domain->s1_cfg.s1cdmax = user_cfg->s1cdmax;
>> +        smmu_domain->s1_cfg.cdcfg.cdtab_dma = user_cfg->s1ctxptr;
>> +        return 0;
> 
> How's that going to work? If the caller's asked for something we can't 
> provide, returning something else and hoping it fails later is not 
> sensible, we should just fail right here. It's even more worrying if 
> there's a chance it *won't* fail later, and a guest ends up with 
> "nested" translation giving it full access to host PA space :/

Oops, apologies - in part thanks to the confusing indentation, I managed 
to miss the early return and misread this all being under the if 
condition for nesting not being supported. Sorry for the confusion :(

Thanks,
Robin.

>> +    }
>> +
>>       if (user_cfg_s2 && !(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
>>           return -EINVAL;
>>       if (user_cfg_s2)
>> @@ -2863,6 +2876,11 @@ static void arm_smmu_remove_dev_pasid(struct 
>> device *dev, ioasid_t pasid)
>>       arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
>>   }
>> +static const struct iommu_domain_ops arm_smmu_nested_domain_ops = {
>> +    .attach_dev        = arm_smmu_attach_dev,
>> +    .free            = arm_smmu_domain_free,
>> +};
>> +
>>   static struct iommu_domain *
>>   __arm_smmu_domain_alloc(unsigned type,
>>               struct arm_smmu_domain *s2,
>> @@ -2877,11 +2895,15 @@ __arm_smmu_domain_alloc(unsigned type,
>>           return arm_smmu_sva_domain_alloc();
>>       if (type != IOMMU_DOMAIN_UNMANAGED &&
>> +        type != IOMMU_DOMAIN_NESTED &&
>>           type != IOMMU_DOMAIN_DMA &&
>>           type != IOMMU_DOMAIN_DMA_FQ &&
>>           type != IOMMU_DOMAIN_IDENTITY)
>>           return NULL;
>> +    if (s2 && s2->stage != ARM_SMMU_DOMAIN_S2)
>> +        return NULL;
>> +
>>       /*
>>        * Allocate the domain and initialise some of its data structures.
>>        * We can't really finalise the domain unless a master is given.
>> @@ -2889,10 +2911,14 @@ __arm_smmu_domain_alloc(unsigned type,
>>       smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL);
>>       if (!smmu_domain)
>>           return NULL;
>> +    smmu_domain->s2 = s2;
>>       domain = &smmu_domain->domain;
>>       domain->type = type;
>> -    domain->ops = arm_smmu_ops.default_domain_ops;
>> +    if (s2)
>> +        domain->ops = &arm_smmu_nested_domain_ops;
>> +    else
>> +        domain->ops = arm_smmu_ops.default_domain_ops;
>>       mutex_init(&smmu_domain->init_mutex);
>>       INIT_LIST_HEAD(&smmu_domain->devices);
>> @@ -2923,8 +2949,16 @@ arm_smmu_domain_alloc_user(struct device *dev, 
>> struct iommu_domain *parent,
>>       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;
>> +    struct arm_smmu_domain *s2 = NULL;
>> +
>> +    if (parent) {
>> +        if (parent->ops != arm_smmu_ops.default_domain_ops)
>> +            return NULL;
>> +        type = IOMMU_DOMAIN_NESTED;
>> +        s2 = to_smmu_domain(parent);
>> +    }
>> -    return __arm_smmu_domain_alloc(type, NULL, master, user_cfg);
>> +    return __arm_smmu_domain_alloc(type, s2, master, user_cfg);
>>   }
>>   static struct iommu_ops arm_smmu_ops = {
>
Nicolin Chen March 10, 2023, 1:34 a.m. UTC | #3 
On Thu, Mar 09, 2023 at 02:28:09PM +0000, Robin Murphy wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2023-03-09 13:20, Robin Murphy wrote:
> > On 2023-03-09 10:53, Nicolin Chen wrote:
> > > Add domain allocation support for IOMMU_DOMAIN_NESTED type. This includes
> > > the "finalise" part to log in the user space Stream Table Entry info.
> > > 
> > > Co-developed-by: Eric Auger <eric.auger@redhat.com>
> > > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> > > ---
> > >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 38 +++++++++++++++++++--
> > >   1 file changed, 36 insertions(+), 2 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 5ff74edfbd68..1f318b5e0921 100644
> > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > @@ -2214,6 +2214,19 @@ static int arm_smmu_domain_finalise(struct
> > > iommu_domain *domain,
> > >           return 0;
> > >       }
> > > +    if (domain->type == IOMMU_DOMAIN_NESTED) {
> > > +        if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1) ||
> > > +            !(smmu->features & ARM_SMMU_FEAT_TRANS_S2)) {
> > > +            dev_dbg(smmu->dev, "does not implement two stages\n");
> > > +            return -EINVAL;
> > > +        }
> > > +        smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> > > +        smmu_domain->s1_cfg.s1fmt = user_cfg->s1fmt;
> > > +        smmu_domain->s1_cfg.s1cdmax = user_cfg->s1cdmax;
> > > +        smmu_domain->s1_cfg.cdcfg.cdtab_dma = user_cfg->s1ctxptr;
> > > +        return 0;
> > 
> > How's that going to work? If the caller's asked for something we can't
> > provide, returning something else and hoping it fails later is not
> > sensible, we should just fail right here. It's even more worrying if
> > there's a chance it *won't* fail later, and a guest ends up with
> > "nested" translation giving it full access to host PA space :/
> 
> Oops, apologies - in part thanks to the confusing indentation, I managed
> to miss the early return and misread this all being under the if
> condition for nesting not being supported. Sorry for the confusion :(

Perhaps this can help readability, considering that we have
multiple places checking the TRANS_S1 and TRANS_S2 features:

	bool feat_has_s1 smmu->features & ARM_SMMU_FEAT_TRANS_S1;
	bool feat_has_s2 smmu->features & ARM_SMMU_FEAT_TRANS_S2;

	if (domain->type == IOMMU_DOMAIN_NESTED) {
		if (!feat_has_s1 || !feat_has_s2) {
			dev_dbg(smmu->dev, "does not implement two stages\n");
			return -EINVAL;
		}
		...
		return 0;
	}

	if (user_cfg_s2 && !feat_has_s2)
		return -EINVAL;
	...
	if (!feat_has_s1)
		smmu_domain->stage = ARM_SMMU_DOMAIN_S2;
	if (!feat_has_s2)
		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;

Would you like this?

Thanks
Nic
Eric Auger March 24, 2023, 3:44 p.m. UTC | #4 
Hi Nicolin,

On 3/9/23 11:53, Nicolin Chen wrote:
> Add domain allocation support for IOMMU_DOMAIN_NESTED type. This includes
> the "finalise" part to log in the user space Stream Table Entry info.

Please explain the domain ops specialization.
>
> Co-developed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 38 +++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 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 5ff74edfbd68..1f318b5e0921 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2214,6 +2214,19 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain,
>  		return 0;
>  	}
>  
> +	if (domain->type == IOMMU_DOMAIN_NESTED) {
> +		if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1) ||
> +		    !(smmu->features & ARM_SMMU_FEAT_TRANS_S2)) {
> +			dev_dbg(smmu->dev, "does not implement two stages\n");
> +			return -EINVAL;
> +		}
> +		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> +		smmu_domain->s1_cfg.s1fmt = user_cfg->s1fmt;
> +		smmu_domain->s1_cfg.s1cdmax = user_cfg->s1cdmax;
> +		smmu_domain->s1_cfg.cdcfg.cdtab_dma = user_cfg->s1ctxptr;
> +		return 0;
> +	}
> +
>  	if (user_cfg_s2 && !(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
>  		return -EINVAL;
>  	if (user_cfg_s2)
> @@ -2863,6 +2876,11 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>  	arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
>  }
>  
> +static const struct iommu_domain_ops arm_smmu_nested_domain_ops = {
> +	.attach_dev		= arm_smmu_attach_dev,
> +	.free			= arm_smmu_domain_free,
> +};
> +
>  static struct iommu_domain *
>  __arm_smmu_domain_alloc(unsigned type,
>  			struct arm_smmu_domain *s2,
> @@ -2877,11 +2895,15 @@ __arm_smmu_domain_alloc(unsigned type,
>  		return arm_smmu_sva_domain_alloc();
>  
>  	if (type != IOMMU_DOMAIN_UNMANAGED &&
> +	    type != IOMMU_DOMAIN_NESTED &&
>  	    type != IOMMU_DOMAIN_DMA &&
>  	    type != IOMMU_DOMAIN_DMA_FQ &&
>  	    type != IOMMU_DOMAIN_IDENTITY)
>  		return NULL;
>  
> +	if (s2 && s2->stage != ARM_SMMU_DOMAIN_S2)
> +		return NULL;
> +
>  	/*
>  	 * Allocate the domain and initialise some of its data structures.
>  	 * We can't really finalise the domain unless a master is given.
> @@ -2889,10 +2911,14 @@ __arm_smmu_domain_alloc(unsigned type,
>  	smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL);
>  	if (!smmu_domain)
>  		return NULL;
> +	smmu_domain->s2 = s2;
>  	domain = &smmu_domain->domain;
>  
>  	domain->type = type;
> -	domain->ops = arm_smmu_ops.default_domain_ops;
> +	if (s2)
> +		domain->ops = &arm_smmu_nested_domain_ops;
> +	else
> +		domain->ops = arm_smmu_ops.default_domain_ops;
>  
>  	mutex_init(&smmu_domain->init_mutex);
>  	INIT_LIST_HEAD(&smmu_domain->devices);
> @@ -2923,8 +2949,16 @@ arm_smmu_domain_alloc_user(struct device *dev, struct iommu_domain *parent,
>  	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;
> +	struct arm_smmu_domain *s2 = NULL;
> +
> +	if (parent) {
> +		if (parent->ops != arm_smmu_ops.default_domain_ops)
> +			return NULL;
> +		type = IOMMU_DOMAIN_NESTED;
> +		s2 = to_smmu_domain(parent);
> +	}
Please can you explain the (use) case where !parent. This creates an
unmanaged S1?

Thanks

Eric
>  
> -	return __arm_smmu_domain_alloc(type, NULL, master, user_cfg);
> +	return __arm_smmu_domain_alloc(type, s2, master, user_cfg);
>  }
>  
>  static struct iommu_ops arm_smmu_ops = {
Jason Gunthorpe March 24, 2023, 4:30 p.m. UTC | #5 
On Fri, Mar 24, 2023 at 04:44:58PM +0100, Eric Auger wrote:

> Please can you explain the (use) case where !parent. This creates an
> unmanaged S1?

If parent is not specified then userspace can force the IOPTE format
to be S1 or S2 of a normal unmanaged domain.

Not sure there is a usecase, but it seems reasonable to support. It
would be useful if there is further parameterization of the S1 like
limiting the number of address bits or something.

Jason
Nicolin Chen March 24, 2023, 5:50 p.m. UTC | #6 
On Fri, Mar 24, 2023 at 04:44:58PM +0100, Eric Auger wrote:
> > @@ -2923,8 +2949,16 @@ arm_smmu_domain_alloc_user(struct device *dev, struct iommu_domain *parent,
> >       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;
> > +     struct arm_smmu_domain *s2 = NULL;
> > +
> > +     if (parent) {
> > +             if (parent->ops != arm_smmu_ops.default_domain_ops)
> > +                     return NULL;
> > +             type = IOMMU_DOMAIN_NESTED;
> > +             s2 = to_smmu_domain(parent);
> > +     }
> Please can you explain the (use) case where !parent. This creates an
> unmanaged S1?

It creates an unmanaged type of a domain. The decision to mark
it as an unmanaged S1 or an unmanaged S2 domain, is done in the
finalise() function that it checks the S2 flag and set a stage
accordingly.

I think that I could add a few lines of comments inline or at
the top of the function to ease the readability.

Thanks
Nicolin
Jason Gunthorpe March 24, 2023, 5:51 p.m. UTC | #7 
On Fri, Mar 24, 2023 at 10:50:34AM -0700, Nicolin Chen wrote:
> On Fri, Mar 24, 2023 at 04:44:58PM +0100, Eric Auger wrote:
> > > @@ -2923,8 +2949,16 @@ arm_smmu_domain_alloc_user(struct device *dev, struct iommu_domain *parent,
> > >       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;
> > > +     struct arm_smmu_domain *s2 = NULL;
> > > +
> > > +     if (parent) {
> > > +             if (parent->ops != arm_smmu_ops.default_domain_ops)
> > > +                     return NULL;
> > > +             type = IOMMU_DOMAIN_NESTED;
> > > +             s2 = to_smmu_domain(parent);
> > > +     }
> > Please can you explain the (use) case where !parent. This creates an
> > unmanaged S1?
> 
> It creates an unmanaged type of a domain. The decision to mark
> it as an unmanaged S1 or an unmanaged S2 domain, is done in the
> finalise() function that it checks the S2 flag and set a stage
> accordingly.

This also needs to be fixed up, the alloc_user should not return
incompletely initialized domains.

Jason
Nicolin Chen March 24, 2023, 5:55 p.m. UTC | #8 
On Fri, Mar 24, 2023 at 02:51:45PM -0300, Jason Gunthorpe wrote:
> On Fri, Mar 24, 2023 at 10:50:34AM -0700, Nicolin Chen wrote:
> > On Fri, Mar 24, 2023 at 04:44:58PM +0100, Eric Auger wrote:
> > > > @@ -2923,8 +2949,16 @@ arm_smmu_domain_alloc_user(struct device *dev, struct iommu_domain *parent,
> > > >       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;
> > > > +     struct arm_smmu_domain *s2 = NULL;
> > > > +
> > > > +     if (parent) {
> > > > +             if (parent->ops != arm_smmu_ops.default_domain_ops)
> > > > +                     return NULL;
> > > > +             type = IOMMU_DOMAIN_NESTED;
> > > > +             s2 = to_smmu_domain(parent);
> > > > +     }
> > > Please can you explain the (use) case where !parent. This creates an
> > > unmanaged S1?
> > 
> > It creates an unmanaged type of a domain. The decision to mark
> > it as an unmanaged S1 or an unmanaged S2 domain, is done in the
> > finalise() function that it checks the S2 flag and set a stage
> > accordingly.
> 
> This also needs to be fixed up, the alloc_user should not return
> incompletely initialized domains.

The finalise() is called at the end of __arm_smmu_domain_alloc()
so alloc_user passing a dev pointer completes the initialization
actually.

Thanks
Nicolin
diff mbox series

Patch

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 5ff74edfbd68..1f318b5e0921 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2214,6 +2214,19 @@  static int arm_smmu_domain_finalise(struct iommu_domain *domain,
 		return 0;
 	}
 
+	if (domain->type == IOMMU_DOMAIN_NESTED) {
+		if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1) ||
+		    !(smmu->features & ARM_SMMU_FEAT_TRANS_S2)) {
+			dev_dbg(smmu->dev, "does not implement two stages\n");
+			return -EINVAL;
+		}
+		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
+		smmu_domain->s1_cfg.s1fmt = user_cfg->s1fmt;
+		smmu_domain->s1_cfg.s1cdmax = user_cfg->s1cdmax;
+		smmu_domain->s1_cfg.cdcfg.cdtab_dma = user_cfg->s1ctxptr;
+		return 0;
+	}
+
 	if (user_cfg_s2 && !(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
 		return -EINVAL;
 	if (user_cfg_s2)
@@ -2863,6 +2876,11 @@  static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 	arm_smmu_sva_remove_dev_pasid(domain, dev, pasid);
 }
 
+static const struct iommu_domain_ops arm_smmu_nested_domain_ops = {
+	.attach_dev		= arm_smmu_attach_dev,
+	.free			= arm_smmu_domain_free,
+};
+
 static struct iommu_domain *
 __arm_smmu_domain_alloc(unsigned type,
 			struct arm_smmu_domain *s2,
@@ -2877,11 +2895,15 @@  __arm_smmu_domain_alloc(unsigned type,
 		return arm_smmu_sva_domain_alloc();
 
 	if (type != IOMMU_DOMAIN_UNMANAGED &&
+	    type != IOMMU_DOMAIN_NESTED &&
 	    type != IOMMU_DOMAIN_DMA &&
 	    type != IOMMU_DOMAIN_DMA_FQ &&
 	    type != IOMMU_DOMAIN_IDENTITY)
 		return NULL;
 
+	if (s2 && s2->stage != ARM_SMMU_DOMAIN_S2)
+		return NULL;
+
 	/*
 	 * Allocate the domain and initialise some of its data structures.
 	 * We can't really finalise the domain unless a master is given.
@@ -2889,10 +2911,14 @@  __arm_smmu_domain_alloc(unsigned type,
 	smmu_domain = kzalloc(sizeof(*smmu_domain), GFP_KERNEL);
 	if (!smmu_domain)
 		return NULL;
+	smmu_domain->s2 = s2;
 	domain = &smmu_domain->domain;
 
 	domain->type = type;
-	domain->ops = arm_smmu_ops.default_domain_ops;
+	if (s2)
+		domain->ops = &arm_smmu_nested_domain_ops;
+	else
+		domain->ops = arm_smmu_ops.default_domain_ops;
 
 	mutex_init(&smmu_domain->init_mutex);
 	INIT_LIST_HEAD(&smmu_domain->devices);
@@ -2923,8 +2949,16 @@  arm_smmu_domain_alloc_user(struct device *dev, struct iommu_domain *parent,
 	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;
+	struct arm_smmu_domain *s2 = NULL;
+
+	if (parent) {
+		if (parent->ops != arm_smmu_ops.default_domain_ops)
+			return NULL;
+		type = IOMMU_DOMAIN_NESTED;
+		s2 = to_smmu_domain(parent);
+	}
 
-	return __arm_smmu_domain_alloc(type, NULL, master, user_cfg);
+	return __arm_smmu_domain_alloc(type, s2, master, user_cfg);
 }
 
 static struct iommu_ops arm_smmu_ops = {