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

Robin Murphy
2024-02-15T19:02:20Z

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
>>