From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Jason Gunthorpe <jgg@nvidia.com>
CC:     Nicolin Chen <nicolinc@nvidia.com>,
        "joro@8bytes.org" <joro@8bytes.org>,
        "will@kernel.org" <will@kernel.org>,
        "robin.murphy@arm.com" <robin.murphy@arm.com>,
        "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
        "shuah@kernel.org" <shuah@kernel.org>,
        "Liu, Yi L" <yi.l.liu@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>,
        "baolu.lu@linux.intel.com" <baolu.lu@linux.intel.com>
Subject: RE: [PATCH v2 07/10] iommufd/device: Make hwpt_list list_add/del
 symmetric
Thread-Topic: [PATCH v2 07/10] iommufd/device: Make hwpt_list list_add/del
 symmetric
Thread-Index: AQHZOzoSQMv1v33l6Eyp7LBz8BGoIq7F9D6AgACo6ACAAM2SAA==
Date:   Fri, 10 Feb 2023 01:46:18 +0000
Message-ID: <BN9PR11MB5276CA32940B6886F8238FA38CDE9@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <cover.1675802050.git.nicolinc@nvidia.com>
 <9d0d4f935c4972771f5aed4b4837d8ae35814e06.1675802050.git.nicolinc@nvidia.com>
 <BN9PR11MB5276F36D2F07209AC7155BAB8CD99@BN9PR11MB5276.namprd11.prod.outlook.com>
 <Y+Tz/kkZ68e9QWR/@nvidia.com>
In-Reply-To: <Y+Tz/kkZ68e9QWR/@nvidia.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?oGzvTtHX2GI23rYB/7O5yLhOLj3DnQ/xLUZjYHIvVb/B/xYyCuepNhWa05eq?=
 =?us-ascii?Q?C0+ncMbPk1B4Nt20DgXwxcphYV5JeeQTq/X3A9aIkfA3EuPHNA9n9J+e2Rfp?=
 =?us-ascii?Q?7rLfgHVbU5/+2ZAVPKYPPiN1xKg6mF4Q44CVGykSbLedcngGSNS3n5870VUQ?=
 =?us-ascii?Q?wJHvSK2M4Lot+Mmup0d1balhXM/HK7EVuYe2xwDMf3PyUGKRGqN8j/9lBm1K?=
 =?us-ascii?Q?UgLSD3v3L69puv/U6qexq80ZDdd6FFeb2HVt/RRnWF3Cxo8JxPh0GxCOj/lx?=
 =?us-ascii?Q?RN8GXyfGDt1gEv9TBrA3oI2n6lexA8gnNt4Z3M2x88OoVpkTQz3WeG64yZdB?=
 =?us-ascii?Q?yK42NmBPZIZTz8S1rn4q/ZZFgVb2O+dJKDxy5O+UFlrJdSa/R0GO6NP5S8p8?=
 =?us-ascii?Q?tSoEctRPg2IznIhoi1sv3jq5FiZBV/PgncM+8cLOQ9VvTSgWOiRWXnUPkAuD?=
 =?us-ascii?Q?IdiNcMJkP+/G5DTELndJmhIGDTkvHuJ+DURBieTtr0HN139E+NrXi7dkKzRf?=
 =?us-ascii?Q?VtfnR6tbmHLVHP2mkCzP1IHqbpci/IWyqwenTkummywQxvtW6LDk12jacwMK?=
 =?us-ascii?Q?SO6tGRW5Q/LEe1pTiB0Vo7HK4aH4dbwMSvC00EeAElicpgCjqnWpwhrv8hi/?=
 =?us-ascii?Q?GZTFR96pHiq6dzHaIahjloml1okD+Yek6pwDhub7JvvdQhY6s5pH/+Fer37d?=
 =?us-ascii?Q?gY2acWZNYLQ1wzM3dwEF7ypzLFqX+uawJUkgIAWFIeQ2WfLnLWbVgq9qY6zF?=
 =?us-ascii?Q?T7uMB058gXyhirfD43GbcS40wgP8QfbL9MqOVlvPfXkVRhYrYlpku90dZQne?=
 =?us-ascii?Q?gdNddLqmKKGp1pXkARmTMkXV+5IVjAgTkIMQ/P/ICZSePbTS6++ZK4hsrdGx?=
 =?us-ascii?Q?HcaHgnS4WiOO5JnUJmj3z/bPktf2I61mtdq0N3ef16YtKd7TlrVCvKZXC1Br?=
 =?us-ascii?Q?GaQQwzaN8URok9CwctX2XHb7idTN2YXltpfHlsX8lB+cgMMcfz3lp8QMhNCO?=
 =?us-ascii?Q?osV6JAe7ipfK5Y6W+u69ivAvpDjzYH+/R2D9XU7M8MtHsBnnfoS+AcI3BRIZ?=
 =?us-ascii?Q?yJptX0fZ+dvtqjOYzQpjpwNK7s3RqKqDTEd1GKcKAJTMUxrGgDbt9TT0pO0/?=
 =?us-ascii?Q?cQAejhP/0/tpo6SR0G6VOhVaea/gMP5rZAwZGo6Mbsz1usq6LS5GPbfGqYvG?=
 =?us-ascii?Q?2EcMc9QQuJtDNpmxorJtT+3kiG8trcDXNBDobUro9CY8OH9sSP8xGpOSIw4Z?=
 =?us-ascii?Q?C2/13vUuHFdhG8TFrwH9muRGLemDaRWbEMzzfIc9ru7t96EeRtxhiWUT97qm?=
 =?us-ascii?Q?JCHen5Aj5ycRgAUGt0ElzXRV38QeqLTsRDTrSWmYyBCrewFNyNFhHGvFFqu7?=
 =?us-ascii?Q?Ztv+yxqYH2cjp1+tZKS+f5wkP91l7XOzMGLROIXqYMZuNmH3NO8h0G18dmLi?=
 =?us-ascii?Q?HRA4sVi8VGXvNQ47DhEjnr9fWvNX80sPpv22pdPIuzJvmW7oELMpHGx8dlS4?=
 =?us-ascii?Q?KE1nSzu9ast5Z9VdqTT2lT5aREoGlTa+QFgP9juKgqeLqU8ixXUHEWij5/oL?=
 =?us-ascii?Q?pQLyiWQvPHQkTYA/AtEpqS2tYXKeJSJRULDVaqH0?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6c8904d9-b458-412a-cfb0-08db0b089ac6
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Feb 2023 01:46:18.6952
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: mk9fkh5CO1PDRWf8z7yNq0Jfo+uIt9RWQwjXrcKA5bNKxGu+DZebI5VAGcDbva8efAgpG5HFx0p2V8hE4REzrQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6074
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Thursday, February 9, 2023 9:24 PM
>=20
> On Thu, Feb 09, 2023 at 03:23:47AM +0000, Tian, Kevin wrote:
> > > From: Nicolin Chen <nicolinc@nvidia.com>
> > > Sent: Wednesday, February 8, 2023 5:18 AM
> > >
> > > Because list_del() is together with iopt_table_remove_domain(), it ma=
kes
> > > sense to have list_add_tail() together with iopt_table_add_domain().
> > >
> > > Also place the mutex outside the iommufd_device_do_attach() call,
> similar
> > > to what's in the iommufd_device_auto_get_domain() function.
> > >
> > > Co-developed-by: Yi Liu <yi.l.liu@intel.com>
> > > Signed-off-by: Yi Liu <yi.l.liu@intel.com>
> > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> > > Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> >
> > shouldn't this be a separate bug fix and backported? double adding a
> > list item would certainly clobber the list...
>=20
> AFAIK there is no bug, this is just reorganizing things
>=20

there is semantics change.

here is the current code:

	case IOMMUFD_OBJ_HW_PAGETABLE: {
		struct iommufd_hw_pagetable *hwpt =3D
			container_of(pt_obj, struct iommufd_hw_pagetable, obj);

		rc =3D iommufd_device_do_attach(idev, hwpt);
		if (rc)
			goto out_put_pt_obj;

		mutex_lock(&hwpt->ioas->mutex);
		list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
		mutex_unlock(&hwpt->ioas->mutex);
		break;
	}

Above means every attach to hwpt will try to add the hwpt to the
list tail. Isn't it a bug?

with this patch the hwpt is added to the list only when it's attached
by the first device, i.e. when iopt_table_add_domain() is called.

	if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) {
		rc =3D iommu_attach_group(hwpt->domain, idev->group);
		if (rc)
			goto out_iova;

		if (list_empty(&hwpt->devices)) {
			rc =3D iopt_table_add_domain(&hwpt->ioas->iopt,
						hwpt->domain);
			if (rc)
				goto out_detach;
			list_add_tail(&hwpt->hwpt_item, &hwpt->ioas->hwpt_list);
		}
	}

so it's actually a bug fix.