Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 29 Sep 2022 13:10:06 -0300
From:   Jason Gunthorpe <jgg@nvidia.com>
To:     Niklas Schnelle <schnelle@linux.ibm.com>
Cc:     Matthew Rosato <mjrosato@linux.ibm.com>,
        Pierre Morel <pmorel@linux.ibm.com>, iommu@lists.linux.dev,
        linux-s390@vger.kernel.org, borntraeger@linux.ibm.com,
        hca@linux.ibm.com, gor@linux.ibm.com,
        gerald.schaefer@linux.ibm.com, agordeev@linux.ibm.com,
        svens@linux.ibm.com, joro@8bytes.org, will@kernel.org,
        robin.murphy@arm.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/5] iommu/s390: Fix duplicate domain attachments
Message-ID: <YzXDXocEarCgJDSn@nvidia.com>
References: <20220929153302.3195115-1-schnelle@linux.ibm.com>
 <20220929153302.3195115-2-schnelle@linux.ibm.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220929153302.3195115-2-schnelle@linux.ibm.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?zPxD4QT5wfxnEWhkhQs4RmHte4Ds1o3rBfbJvPwvVArOL0ebXK0fPHrIFbHe?=
 =?us-ascii?Q?RXr1TLvENlsO/trOlhBykSJRc2TwdkWicmRoaH0b1SReCcB8MfQ2eRmtrWJ0?=
 =?us-ascii?Q?M3bJpUc7Htzk1NBtGVB0jdl8QJk97q7kDquHaRobUEGOg1kcG42me2i7tOF8?=
 =?us-ascii?Q?CEcRdxYsR6Hr+B23ajEjNkeHUJqAr9Je/eXXVsbEHf8ICWyMlcdmjlM8G7s4?=
 =?us-ascii?Q?LG+vomYot7ivIJHIcBiEdH049qBSnsCIVfrRzTe+33AwQ9PIhxoFauBtpMU+?=
 =?us-ascii?Q?oEBZOTBiyF6hXZ2TKPCxYanjH8qmQPX4UKNX7ZRfwC49zobXyBcfML5B0OOO?=
 =?us-ascii?Q?IfQWf8CzEL1V5RJ5YpYox4xVtM+epx7wbJTBa2dUpyHjpgr9Qva7PpbA2z2k?=
 =?us-ascii?Q?tcRjLht2nCvM670k1MsJ2elSgmlAfOKKYEBPEGtroE1/qKPjexlraDtr2ZHY?=
 =?us-ascii?Q?+b1cCvw9/qERfiShThRsOZUn22fAUSgwkrI4LoHVLX5Lk/Egl13qdhNtDgDp?=
 =?us-ascii?Q?8LfJD5l7tq4ectck8m4BvY1Jw/6AkmilDuvuToiuS++E6KOMUkrolFVlsDgi?=
 =?us-ascii?Q?e44/EAdNkpJh/oUp5fYUtIRjGk7IJrbrbwjeb6UT1/4CN5v4mZn9eNFMPp+h?=
 =?us-ascii?Q?FfW0oKKNfUnQ0651R/qKU8cQz9ND7MQR2W6zvrzf5IxEOG+u4yxgRH1hIww/?=
 =?us-ascii?Q?s/vGfnHAanNxcDalcSPOVRHVcCHdMF9xCrdQ/rGu+tR1b70u/TWr32Ky9ixB?=
 =?us-ascii?Q?qS77jbLaOcwbHud2pU45HF3E7XmqGBNDdpP0uKzgiuShELkEmJCO3zIpgX0Y?=
 =?us-ascii?Q?gyVGJF+6WaeXee7vWK4yypUNn4oDIbV3/1D3CuFhfXJ8ae8Q/WtFqE12lrX8?=
 =?us-ascii?Q?v3KT8GPS1gLBdXoLT/XJFVDwz2PEoExL0LlyTtAZUK5JF/sfsJe3dELgkngy?=
 =?us-ascii?Q?QDkFhIbqDeVKz29C1MgYsSFWotx52l84DIfgyqPOVnvLZNM4oeufLavYG4Wl?=
 =?us-ascii?Q?bVjp1ITWk/slepAng3xWyvoLHUEnhVvUjIXSXRbI/iRHoQpSSYWuE5J0aK1A?=
 =?us-ascii?Q?QVzCUybRuo+A+USeNSGuFWRfbQoRvCz80NXLAAwrbCrXL2b471Yt7fAmE8JA?=
 =?us-ascii?Q?Dxp3zYOWC6P45OX15GY5JG7rKLZzkyWa32nd5PxtI5WWT1R0kk/vsxjEOUcj?=
 =?us-ascii?Q?JCqKpMPOsh3aa7Xfc+AIZgki4aDnB/6gzvU4AUpXVkPIwBevgUtEeFYCVbR3?=
 =?us-ascii?Q?47LIR0i81644AbSeaRZt/DLblRm6YLa3fzGMAfY2nvHF/ay9FiB56qTpA/xa?=
 =?us-ascii?Q?97wpL5Us6VZ/QE2n7osiDBZcKfsBjOeX/5EMfGXmlS+kxM89pJz/rRDkQnPL?=
 =?us-ascii?Q?NTTSXCrqyX/ojV30J4z5mUPmF1O8BzsGjLTi6/pAWn3Gm65nv3E9PU4FABNW?=
 =?us-ascii?Q?CIY1MLIohf5uE/W7lUmZUCDAEvNbALzIjBU/CZGkee9h6oLvVu3a3nhv3J+u?=
 =?us-ascii?Q?8ErhIZaQbnO40uYlxSJT8HiBCY2T4tJr4qXQxF6et5hAbaNwzuO3GYA5KTyK?=
 =?us-ascii?Q?Wc4crrwBaml/GtSTEdw=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 46bc6158-79dc-4f60-5337-08daa2351366
X-MS-Exchange-CrossTenant-AuthSource: MN2PR12MB4192.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2022 16:10:07.0660
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: NwsnHvJMCVeoVjUSgvDO1qZDDVfYma7oeKyvkK0lw809ZwsTfFLDD8lvRYbPERaC
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB6861
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE
        autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Sep 29, 2022 at 05:32:58PM +0200, Niklas Schnelle wrote:
> Since commit fa7e9ecc5e1c ("iommu/s390: Tolerate repeat attach_dev
> calls") we can end up with duplicates in the list of devices attached to
> a domain. This is inefficient and confusing since only one domain can
> actually be in control of the IOMMU translations for a device. Fix this
> by detaching the device from the previous domain, if any, on attach.
> Add a WARN_ON() in case we still have attached devices on freeing the
> domain. While here remove the re-attach on failure dance as it was
> determined to be unlikely to help and may confuse debug and recovery.
> 
> Fixes: fa7e9ecc5e1c ("iommu/s390: Tolerate repeat attach_dev calls")
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
> Changes since v2:
> - Make __s390_iommu_detach_device() return void (Jason)
> - Remove superfluous locking when we're freeing anyway (Jason)
> - Remove the re-attach on failure dance as it is unlikely to help
>   and complicates debug and recovery (Jason)
> - Ignore attempts to detach from domain that the device is no longer
>   attached to.
> 
>  drivers/iommu/s390-iommu.c | 77 +++++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
> index c898bcbbce11..6fcb64e4b5e6 100644
> --- a/drivers/iommu/s390-iommu.c
> +++ b/drivers/iommu/s390-iommu.c
> @@ -79,10 +79,36 @@ static void s390_domain_free(struct iommu_domain *domain)
>  {
>  	struct s390_domain *s390_domain = to_s390_domain(domain);
>  
> +	WARN_ON(!list_empty(&s390_domain->devices));
>  	dma_cleanup_tables(s390_domain->dma_table);
>  	kfree(s390_domain);
>  }
>  
> +static void __s390_iommu_detach_device(struct s390_domain *s390_domain,
> +				       struct zpci_dev *zdev)
> +{
> +	struct s390_domain_device *domain_device, *tmp;
> +	unsigned long flags;
> +
> +	if (!zdev || zdev->s390_domain != s390_domain)

Please drop the s390_domain from this function, it is pointless..

Calling detach_device with a mismatched domain argument is a WARN_ON
offense, the correct recovery is still to remove the domain.

And zdev can already never be null due to the call chain

Also, s390_iommu_release_device() should call this new function since
we don't want to return back to the platform DMA when releasing.

So, like this:

diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 2bad24d6cfef59..e8333a9301ec95 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -75,12 +75,12 @@ static void s390_domain_free(struct iommu_domain *domain)
 	kfree(s390_domain);
 }
 
-static void __s390_iommu_detach_device(struct s390_domain *s390_domain,
-				       struct zpci_dev *zdev)
+static void __s390_iommu_detach_device(struct zpci_dev *zdev)
 {
+	struct s390_domain *s390_domain = zdev->s390_domain;
 	unsigned long flags;
 
-	if (!zdev || zdev->s390_domain != s390_domain)
+	if (!s390_domain)
 		return;
 
 	spin_lock_irqsave(&s390_domain->list_lock, flags);
@@ -108,7 +108,7 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 		return -EINVAL;
 
 	if (zdev->s390_domain)
-		__s390_iommu_detach_device(zdev->s390_domain, zdev);
+		__s390_iommu_detach_device(zdev);
 	else if (zdev->dma_table)
 		zpci_dma_exit_device(zdev);
 
@@ -130,10 +130,11 @@ static int s390_iommu_attach_device(struct iommu_domain *domain,
 static void s390_iommu_detach_device(struct iommu_domain *domain,
 				     struct device *dev)
 {
-	struct s390_domain *s390_domain = to_s390_domain(domain);
 	struct zpci_dev *zdev = to_zpci_dev(dev);
 
-	__s390_iommu_detach_device(s390_domain, zdev);
+	WARN_ON(zdev->s390_domain != to_s390_domain(domain));
+
+	__s390_iommu_detach_device(zdev);
 	zpci_dma_init_device(zdev);
 }
 
@@ -174,21 +175,11 @@ static void s390_iommu_release_device(struct device *dev)
 	struct iommu_domain *domain;
 
 	/*
-	 * This is a workaround for a scenario where the IOMMU API common code
-	 * "forgets" to call the detach_dev callback: After binding a device
-	 * to vfio-pci and completing the VFIO_SET_IOMMU ioctl (which triggers
-	 * the attach_dev), removing the device via
-	 * "echo 1 > /sys/bus/pci/devices/.../remove" won't trigger detach_dev,
-	 * only release_device will be called via the BUS_NOTIFY_REMOVED_DEVICE
-	 * notifier.
-	 *
-	 * So let's call detach_dev from here if it hasn't been called before.
+	 * release_device is expected to detach any domain currently attached
+	 * to the device, but keep it attached to other devices in the group.
 	 */
-	if (zdev && zdev->s390_domain) {
-		domain = iommu_get_domain_for_dev(dev);
-		if (domain)
-			s390_iommu_detach_device(domain, dev);
-	}
+	if (zdev)
+		__s390_iommu_detach_device(zdev);
 }
 
 static int s390_iommu_update_trans(struct s390_domain *s390_domain,