diff mbox series

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

Message ID 3-v5-cd1be8dd9c71+3fa-smmuv3_newapi_p1_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series [v5,01/17] iommu/arm-smmu-v3: Make STE programming independent of the callers | expand

Commit Message

Jason Gunthorpe Feb. 6, 2024, 3:12 p.m. UTC 
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.

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

Comments

Mostafa Saleh Feb. 13, 2024, 3:37 p.m. UTC | #1 
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.

[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
>
Jason Gunthorpe Feb. 13, 2024, 4:16 p.m. UTC | #2 
On Tue, Feb 13, 2024 at 03:37:43PM +0000, 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.

I'm happy to drop this patch, if no strong opinion I will leave it

> [1] https://lore.kernel.org/kvmarm/20230201125328.2186498-1-jean-philippe@linaro.org/

I saw this but I didn't try to figure out too much what it is
doing.. It looks like a new iommu driver that re-uses alot of the
datastructures of SMMU but has a different HW facing API based on
hypercalls?

It seems interesting, but my knee jerk reaction would be that a new
iommu driver proposal needs to implement the new iommu core APIs, not
the old stuff. IMHO, when this progresses past a RFC it needs to come
as a proper submission of just the iommu driver, in the normal way.

I also wonder about the wisdom of sharing so much code. Code sharing
is not unconditionally good if it doesn't have a robust abstraction..

Jason
Mostafa Saleh Feb. 13, 2024, 4:46 p.m. UTC | #3 
On Tue, Feb 13, 2024 at 12:16:01PM -0400, Jason Gunthorpe wrote:
> On Tue, Feb 13, 2024 at 03:37:43PM +0000, 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.
> 
> I'm happy to drop this patch, if no strong opinion I will leave it
> 
> > [1] https://lore.kernel.org/kvmarm/20230201125328.2186498-1-jean-philippe@linaro.org/
> 
> I saw this but I didn't try to figure out too much what it is
> doing.. It looks like a new iommu driver that re-uses alot of the
> datastructures of SMMU but has a different HW facing API based on
> hypercalls?

Yes, this is an implementation for the SMMUv3 driver in EL2 in KVM that is needed
for DMA isolation for Protected KVM(pKVM) as the host is untrusted in this case and
can attack the hypervisor through DMA.

This is similar to the kernel driver but with a bunch of tricks such as page table
allocation, power management and more.

Also we have some plans to extend it to KVM guests, (Initially I was considering
KVM-VFIO device but now with the new iommufd stuff, it might fit in there)

> It seems interesting, but my knee jerk reaction would be that a new
> iommu driver proposal needs to implement the new iommu core APIs, not
> the old stuff. IMHO, when this progresses past a RFC it needs to come
> as a proper submission of just the iommu driver, in the normal way.
> 
> I also wonder about the wisdom of sharing so much code. Code sharing
> is not unconditionally good if it doesn't have a robust abstraction..
>

The idea is that as the hardware is common and most of the dt binding also,
we shouldn’t really reinvent the wheel, so this series moves the common code
for the page-table library to be shared with EL1/EL2 and for HW/FW probe and
init to be shared between the kernel drivers and adds a lot of the infrastructure
for IOMMUs in the hypervisor.

Thanks,
Mostafa
Robin Murphy Feb. 15, 2024, 7:01 p.m. UTC | #4 
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
>>
Jason Gunthorpe Feb. 15, 2024, 9:18 p.m. UTC | #5 
On Thu, Feb 15, 2024 at 07:01:59PM +0000, Robin Murphy wrote:
> 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.

I will drop this patch

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