[RFC,v1,2/2] vfio/iommu-type1: set only stage 2 translation

[Robin Murphy]

On 21/10/16 15:27, Alex Williamson wrote:
> On Fri, 21 Oct 2016 12:39:24 +0800
> Rick Song <songwenjun at huawei.com> wrote:
> 
>> Normally, VFIO should use only stage 2 translation of
>> iommu, to create the address mapping. If nesting translation
>> is disabled from VFIO core, enable iommu domain only stage 2
>> attribute, otherwise, enable iommu domain nesting attribute.
>>
>> Signed-off-by: Rick Song <songwenjun at huawei.com>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index 2ba1942..c0265fe 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -741,7 +741,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>  	struct vfio_group *group, *g;
>>  	struct vfio_domain *domain, *d;
>>  	struct bus_type *bus = NULL;
>> -	int ret;
>> +	int attr, ret;
>>  
>>  	mutex_lock(&iommu->lock);
>>  
>> @@ -775,13 +775,22 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>  		goto out_free;
>>  	}
>>  
>> +	/*
>> +	 * Set iommu nesting domain attribute if nesting translation
>> +	 * is enabled from iommu vfio, otherwise set iommu only stage
>> +	 * 2 domain attribute.
>> +	 */
>> +	attr = 1;
>>  	if (iommu->nesting) {
>> -		int attr = 1;
>> -
>>  		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_NESTING,
>>  					    &attr);
>>  		if (ret)
>>  			goto out_domain;
>> +	} else {
>> +		ret = iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_S2,
>> +					    &attr);
>> +		if (ret)
>> +			goto out_domain;
>>  	}
> 
> This attribute is not relevant to the majority of current users, why
> would we assume that we need to call it for all non-nesting cases?  Why
> do we need to set the attribute at all, what benefit does it provide?
> If this is the normal case for an IOMMU API domain, why is there an
> option for it at all?  Maybe this should be the default and S1
> (whatever that means) should be the alternate option.  Thanks,

Indeed, it should be fairly straightforward to make
arm_smmu_domain_finalise() prefer stage 1/stage 2 based on domain->type
in the case that both stages are implemented. That would be preferable
to changing core VFIO code for something that really is SMMU-specific.

To echo Alex, though, what's the motivation for this? Could it be
addressed by simply implementing a force_stage parameter like the SMMUv2
driver has?

Robin.

> 
> Alex
> 
>>  
>>  	ret = iommu_attach_group(domain->domain, iommu_group);
>