Re: [PATCH v5 03/17] iommu/arm-smmu-v3: Move arm_smmu_rmr_install_bypass_ste()

[Robin Murphy <robin.murphy@arm.com>]

On 13/02/2024 3:37 pm, Mostafa Saleh wrote:
> Hi Jason,
> 
> On Tue, Feb 06, 2024 at 11:12:40AM -0400, Jason Gunthorpe wrote:
>> Logically arm_smmu_init_strtab() is the function that allocates and
>> populates the stream table with the initial value of the STEs. After this
>> function returns the stream table should be fully ready.
>>
>> arm_smmu_rmr_install_bypass_ste() adjusts the initial stream table to force
>> any SIDs that the FW says have IOMMU_RESV_DIRECT to use bypass. This
>> ensures there is no disruption to the identity mapping during boot.
>>
>> Put arm_smmu_rmr_install_bypass_ste() into arm_smmu_init_strtab(), it
>> already executes immediately after arm_smmu_init_strtab().
>>
>> No functional change intended.
> 
> I think arm_smmu_init_strtab is quite low level to abstract FW configuration in it.
> For example in KVM[1] we'd re-use a big part of this driver and rely on similar
> low-level functions. But no strong opinion.

Right, the fact that RMR handling is currently based on bypass STEs is 
an implementation detail; if we ever get round to doing the strict 
version with full-on temporary pagetables, that would obviously not 
belong in init_strtab, thus I would prefer to leave the "handle RMRs" 
step in its appropriate place in the higher-level flow regardless of how 
it happens to be named and implemented today.

Thanks,
Robin.

> 
> [1] https://lore.kernel.org/kvmarm/20230201125328.2186498-1-jean-philippe@linaro.org/
> 
> 
> Thanks,
> Mostafa
> 
>> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
>> Tested-by: Moritz Fischer <moritzf@google.com>
>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 8 +++++---
>>   1 file changed, 5 insertions(+), 3 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 6123e5ad95822c..2ab36dcf7c61f5 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -101,6 +101,8 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
>>   	{ 0, NULL},
>>   };
>>   
>> +static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu);
>> +
>>   static void parse_driver_options(struct arm_smmu_device *smmu)
>>   {
>>   	int i = 0;
>> @@ -3256,6 +3258,7 @@ static int arm_smmu_init_strtab_linear(struct arm_smmu_device *smmu)
>>   	cfg->strtab_base_cfg = reg;
>>   
>>   	arm_smmu_init_bypass_stes(strtab, cfg->num_l1_ents);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -3279,6 +3282,8 @@ static int arm_smmu_init_strtab(struct arm_smmu_device *smmu)
>>   
>>   	ida_init(&smmu->vmid_map);
>>   
>> +	/* Check for RMRs and install bypass STEs if any */
>> +	arm_smmu_rmr_install_bypass_ste(smmu);
>>   	return 0;
>>   }
>>   
>> @@ -4073,9 +4078,6 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>   	/* Record our private device structure */
>>   	platform_set_drvdata(pdev, smmu);
>>   
>> -	/* Check for RMRs and install bypass STEs if any */
>> -	arm_smmu_rmr_install_bypass_ste(smmu);
>> -
>>   	/* Reset the device */
>>   	ret = arm_smmu_device_reset(smmu, bypass);
>>   	if (ret)
>> -- 
>> 2.43.0
>>