From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Nicolin Chen <nicolinc@nvidia.com>
CC:     "jgg@nvidia.com" <jgg@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 08/10] iommufd/device: Use iommu_group_replace_domain()
Thread-Topic: [PATCH v2 08/10] iommufd/device: Use
 iommu_group_replace_domain()
Thread-Index: AQHZOzoV+UAbwLQwnEuO17sJd5b+0K7F+NzggAEnS4CAAE2/cA==
Date:   Fri, 10 Feb 2023 02:11:23 +0000
Message-ID: <BN9PR11MB5276268D3ED0360913A05C368CDE9@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <cover.1675802050.git.nicolinc@nvidia.com>
 <4653f009c3dacae8ebf3a4865aaa944aa9c7cc7e.1675802050.git.nicolinc@nvidia.com>
 <BN9PR11MB5276C1807B710CAD3E5820D78CD99@BN9PR11MB5276.namprd11.prod.outlook.com>
 <Y+Vh479cDD7LX2x/@Asurada-Nvidia>
In-Reply-To: <Y+Vh479cDD7LX2x/@Asurada-Nvidia>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?8urzy1hyN1wYeYuA2NjY5gwcOQO2D+YMGQ2sQjDuZusSzkMOsPN+YeiHKTXV?=
 =?us-ascii?Q?4VkJmE5halb47v5ZdojNyWMRK+opXPAVslWkdbws3nic36Qiptg8RktB3vrc?=
 =?us-ascii?Q?15C9hInxTgIF0RjRsyg2t/L4m2Jw9Fe1U5XYJQPqeUVuyft21lFgypQU2ahL?=
 =?us-ascii?Q?HKhHhG691lHmS1R0yvOkXua5COwJZAt964WnO7TVFJk3nDhktmIqT3MCSyLa?=
 =?us-ascii?Q?SxAaRtAJkyPxWLuG6rouvwgcoTnXqBIKrldgoaPjc6vZcwizLeYM5eC01PCd?=
 =?us-ascii?Q?aQKQaPP9vsWWbqnAoDzWOswa+y7EmPcVe/Gf8qsw67+2wHUiFH7o1vkbxIOE?=
 =?us-ascii?Q?OB9K6PAURdXbFnIOs/oRTC8dJZmXWz0XAeEAXeupl7oXYptlL6XsI05S8T7p?=
 =?us-ascii?Q?laMYRsAgq4TSjdNDZUHsTg7GNHGe74PROxdZfzfUJhuepIZCYac64/hBMi+L?=
 =?us-ascii?Q?1Yn6b90BPdWjjTbFI2hBCNHs7NWt7IH7Ou5cL3IMRloMfUaByssUxCIGruf7?=
 =?us-ascii?Q?ApBkPifL3LW92ZaUPNG3/v95l6pTR6xjzlJOGJ4CKqF2f45G5yc5OXSU8qo1?=
 =?us-ascii?Q?Uo9ue56tDQ/fFX/isHjVpsEbrY8Sh0QrNn9amYag05ZL9ZB8QBonR83Ewpps?=
 =?us-ascii?Q?OR+OHnvG7aotVM4er6eNxGkCMfryONVXDwc2EyDx0iHZ+YIaehDNNx1jl4ER?=
 =?us-ascii?Q?By65KjkTE82A5Cq8XPqesMY2znhl5EfZdmT8pt2ke0SGAugEYuKsXm3Zemx/?=
 =?us-ascii?Q?mvEw/ylLC39XCvcbr4mB6gPlIzBGm8v9oFfS5oKndWtZgvRgDOVNFg+dVMxy?=
 =?us-ascii?Q?2pDcxW64hq05x0A0SHVnsrtTpAi8LJxOzNq6yWH3gazxdOcJ7GHaoWilERXB?=
 =?us-ascii?Q?m/S9obbWAkKT3odM4vdA4tekOfau/0tNmA0Wp1lakpUcY82BE1U9vapjIs9n?=
 =?us-ascii?Q?sit2m/1b6lwKLxJWSDrVLZPsyqq2mEgn3mvXfjlF/UkXn3b/dJUgObuz++tf?=
 =?us-ascii?Q?apEWE+H97AOyDP8GfouoFBncHlXxeSfQbOl7zySIgBNDjoyqbRN4QEHjdIdL?=
 =?us-ascii?Q?hjk+wH8FAJzjOD7lMZbFZgFtLgqqec72zKr7BJxF8ftTFWsW/K/eDyokLLBi?=
 =?us-ascii?Q?nc06+P5y1fa2jbw9LuI+J98HLSdmTNfNfdn3QALR8/myVcjV3X7sPqajM9go?=
 =?us-ascii?Q?OYfwBrW3/sCeGqFw9NeajnyPk7fpoovtvS1N4s+bZUJBIqeRtH+C/yOor2vy?=
 =?us-ascii?Q?MFZEj13fGKLmhADdG9w32m6/Ow1GG3CHOpp7iQFSqMR2iLKLIh8e3VHDEY/K?=
 =?us-ascii?Q?B7M15F4Ni7YavyGr/Q4MuRowtL4BiS+gvd7I6v3tspNprt4ZNy+OhaUV2P09?=
 =?us-ascii?Q?+D3J37xjQnrYXt1usxwuEjrzWGOfiX/DvN49hG4xfbqv11hEOP7nFaH706uJ?=
 =?us-ascii?Q?+rBPWYPUOJum+aLK2HU9pxCxTn61IngMJDQKIe0+f7r7JVbGtrxAO3mecRjb?=
 =?us-ascii?Q?lXCjslxJGylMG9LujIC+FuShPzaPfKdkZO86Nm553rsXNkht4FshSFPxm8KD?=
 =?us-ascii?Q?OQep4DwaxfFeyRtvpduaDmIJtXqtLZsuxOGRtSTl?=
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: 35745af9-602b-4349-4f88-08db0b0c1b79
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Feb 2023 02:11:23.1349
 (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: G91B+jct4/NgjzsBWkM5k/BonhGtdmob0d7c37rY6XqbcAqLE1wmYvUPpu243h5pG33nuyjOTs9HjcVgouHCbQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB7183
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Friday, February 10, 2023 5:13 AM
>=20
> > > +     if (!iommufd_hw_pagetable_has_group(hwpt, idev->group)) {
> > > +             if (list_empty(&hwpt->devices)) {
> > > +                     iopt_table_remove_domain(&hwpt->ioas->iopt,
> > > +                                              hwpt->domain);
> > > +                     list_del(&hwpt->hwpt_item);
> > > +             }
> >
> > I'm not sure how this can be fully shared between detach and replace.
> > Here some work e.g. above needs to be done before calling
> > iommu_group_replace_domain() while others can be done afterwards.
>=20
> This iopt_table_remove_domain/list_del is supposed to be done in
> the hwpt's destroy() actually. We couldn't move it because it'd
> need the new domain_alloc_user op and its implementation in ARM
> driver. Overall, I think it should be safe to put it behind the
> iommu_group_replace_domain().
>=20

My confusion is that we have different flows between detach/attach
and replace.

today with separate detach+attach we have following flow:

	Remove device from current hwpt;
	if (last_device in hwpt) {
		Remove hwpt domain from current iopt;
		if (last_device in group)
			detach group from hwpt domain;
	}

	if (first device in group) {
		attach group to new hwpt domain;
		if (first_device in hwpt)
			Add hwpt domain to new iopt;
	Add device to new hwpt;

but replace flow is different on the detach part:

	if (first device in group) {
		replace group's domain from current hwpt to new hwpt;
		if (first_device in hwpt)
			Add hwpt domain to new iopt;
	}

	Remove device from old hwpt;
	if (last_device in old hwpt)
		Remove hwpt domain from old iopt;

	Add device to new hwpt;

I'm yet to figure out whether we have sufficient lock protection to
prevent other paths from using old iopt/hwpt to find the device
which is already attached to a different domain.