diff mbox series

[v5,08/17] iommu/arm-smmu-v3: Do not change the STE twice during arm_smmu_attach_dev()

Message ID 8-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 
This was needed because the STE code required the STE to be in
ABORT/BYPASS inorder to program a cdtable or S2 STE. Now that the STE code
can automatically handle all transitions we can remove this step
from the attach_dev flow.

A few small bugs exist because of this:

1) If the core code does BLOCKED -> UNMANAGED with disable_bypass=false
   then there will be a moment where the STE points at BYPASS. Since
   this can be done by VFIO/IOMMUFD it is a small security race.

2) If the core code does IDENTITY -> DMA then any IOMMU_RESV_DIRECT
   regions will temporarily become BLOCKED. We'd like drivers to
   work in a way that allows IOMMU_RESV_DIRECT to be continuously
   functional during these transitions.

Make arm_smmu_release_device() put the STE back to the correct
ABORT/BYPASS setting. Fix a bug where a IOMMU_RESV_DIRECT was ignored on
this path.

As noted before the reordering of the linked list/STE/CD changes is OK
against concurrent arm_smmu_share_asid() because of the
arm_smmu_asid_lock.

Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Tested-by: Nicolin Chen <nicolinc@nvidia.com>
Tested-by: Moritz Fischer <moritzf@google.com>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Mostafa Saleh Feb. 13, 2024, 3:40 p.m. UTC | #1 
Hi Jason,

On Tue, Feb 06, 2024 at 11:12:45AM -0400, Jason Gunthorpe wrote:
> This was needed because the STE code required the STE to be in
> ABORT/BYPASS inorder to program a cdtable or S2 STE. Now that the STE code
> can automatically handle all transitions we can remove this step
> from the attach_dev flow.
> 
> A few small bugs exist because of this:
> 
> 1) If the core code does BLOCKED -> UNMANAGED with disable_bypass=false
>    then there will be a moment where the STE points at BYPASS. Since
>    this can be done by VFIO/IOMMUFD it is a small security race.
> 
> 2) If the core code does IDENTITY -> DMA then any IOMMU_RESV_DIRECT
>    regions will temporarily become BLOCKED. We'd like drivers to
>    work in a way that allows IOMMU_RESV_DIRECT to be continuously
>    functional during these transitions.
> 
> Make arm_smmu_release_device() put the STE back to the correct
> ABORT/BYPASS setting. Fix a bug where a IOMMU_RESV_DIRECT was ignored on
> this path.
> 
> As noted before the reordering of the linked list/STE/CD changes is OK
> against concurrent arm_smmu_share_asid() because of the
> arm_smmu_asid_lock.
> 
> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Moritz Fischer <moritzf@google.com>
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 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 1138e868c4d73e..340f3dc82c9ce0 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2562,7 +2562,6 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
>  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>  {
>  	unsigned long flags;
> -	struct arm_smmu_ste target;
>  	struct arm_smmu_domain *smmu_domain = master->domain;
>  
>  	if (!smmu_domain)
> @@ -2576,11 +2575,6 @@ static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>  
>  	master->domain = NULL;
>  	master->ats_enabled = false;
> -	if (disable_bypass)
> -		arm_smmu_make_abort_ste(&target);
> -	else
> -		arm_smmu_make_bypass_ste(&target);
> -	arm_smmu_install_ste_for_dev(master, &target);
>  	/*
>  	 * Clearing the CD entry isn't strictly required to detach the domain
>  	 * since the table is uninstalled anyway, but it helps avoid confusion
> @@ -2928,9 +2922,18 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
>  static void arm_smmu_release_device(struct device *dev)
>  {
>  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> +	struct arm_smmu_ste target;
>  
>  	if (WARN_ON(arm_smmu_master_sva_enabled(master)))
>  		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> +
> +	/* Put the STE back to what arm_smmu_init_strtab() sets */
> +	if (disable_bypass && !dev->iommu->require_direct)
> +		arm_smmu_make_abort_ste(&target);
> +	else
> +		arm_smmu_make_bypass_ste(&target);
> +	arm_smmu_install_ste_for_dev(master, &target);
> +
>  	arm_smmu_detach_dev(master);
>  	arm_smmu_disable_pasid(master);
>  	arm_smmu_remove_master(master);
> -- 
> 2.43.0
> 
I am still reviewing patch-1 and the hitless machinery (also I think -or hope-
this can be simplified), with the assumption that
arm_smmu_install_ste_for_dev()/arm_smmu_write_ste() will do the right thing,
that good looks good to me.

However, as it changes the current behavior of the driver where disable_bypass
used to override require_direct, I am not sure if this would break any existing setups.


Thanks,
Mostafa
Jason Gunthorpe Feb. 13, 2024, 4:26 p.m. UTC | #2 
On Tue, Feb 13, 2024 at 03:40:34PM +0000, Mostafa Saleh wrote:
> > @@ -2928,9 +2922,18 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
> >  static void arm_smmu_release_device(struct device *dev)
> >  {
> >  	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> > +	struct arm_smmu_ste target;
> >  
> >  	if (WARN_ON(arm_smmu_master_sva_enabled(master)))
> >  		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
> > +
> > +	/* Put the STE back to what arm_smmu_init_strtab() sets */
> > +	if (disable_bypass && !dev->iommu->require_direct)
> > +		arm_smmu_make_abort_ste(&target);
> > +	else
> > +		arm_smmu_make_bypass_ste(&target);
> > +	arm_smmu_install_ste_for_dev(master, &target);
> > +
> >  	arm_smmu_detach_dev(master);
> >  	arm_smmu_disable_pasid(master);
> >  	arm_smmu_remove_master(master);

> I am still reviewing patch-1 and the hitless machinery (also I think -or hope-
> this can be simplified), with the assumption that
> arm_smmu_install_ste_for_dev()/arm_smmu_write_ste() will do the right thing,
> that good looks good to me.

I'm interested if you can come up with something. Let me know if you
want to bounce some ideas.

> However, as it changes the current behavior of the driver where
> disable_bypass used to override require_direct, I am not sure if
> this would break any existing setups.

Yes, the commit message explains this. It is a little bug.

require_direct takes precedence when building the initial STE, release
should restore the STE back to how it was before probe.

I don't imagine a case where a system was fine with the STE during
boot but doesn't like that same STE during devuce hot unplug???

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 1138e868c4d73e..340f3dc82c9ce0 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2562,7 +2562,6 @@  static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
 static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 {
 	unsigned long flags;
-	struct arm_smmu_ste target;
 	struct arm_smmu_domain *smmu_domain = master->domain;
 
 	if (!smmu_domain)
@@ -2576,11 +2575,6 @@  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 
 	master->domain = NULL;
 	master->ats_enabled = false;
-	if (disable_bypass)
-		arm_smmu_make_abort_ste(&target);
-	else
-		arm_smmu_make_bypass_ste(&target);
-	arm_smmu_install_ste_for_dev(master, &target);
 	/*
 	 * Clearing the CD entry isn't strictly required to detach the domain
 	 * since the table is uninstalled anyway, but it helps avoid confusion
@@ -2928,9 +2922,18 @@  static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 static void arm_smmu_release_device(struct device *dev)
 {
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
+	struct arm_smmu_ste target;
 
 	if (WARN_ON(arm_smmu_master_sva_enabled(master)))
 		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
+
+	/* Put the STE back to what arm_smmu_init_strtab() sets */
+	if (disable_bypass && !dev->iommu->require_direct)
+		arm_smmu_make_abort_ste(&target);
+	else
+		arm_smmu_make_bypass_ste(&target);
+	arm_smmu_install_ste_for_dev(master, &target);
+
 	arm_smmu_detach_dev(master);
 	arm_smmu_disable_pasid(master);
 	arm_smmu_remove_master(master);