From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Nicolin Chen <nicolinc@nvidia.com>,
        "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>
CC:     "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+Nzg
Date:   Thu, 9 Feb 2023 04:00:52 +0000
Message-ID: <BN9PR11MB5276C1807B710CAD3E5820D78CD99@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <cover.1675802050.git.nicolinc@nvidia.com>
 <4653f009c3dacae8ebf3a4865aaa944aa9c7cc7e.1675802050.git.nicolinc@nvidia.com>
In-Reply-To: <4653f009c3dacae8ebf3a4865aaa944aa9c7cc7e.1675802050.git.nicolinc@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?m+vNmmdPtfC6rt/j9ncFQtW0sz5czAMMzUUXeH0j0Kn82gINKYHRdy3s4Q4f?=
 =?us-ascii?Q?l7/lT9klzVr7uNfGbevOCQ2dUFhrdu40zCfnKzDhMgLaYY2ApzKcgOzxyJh/?=
 =?us-ascii?Q?WCS1w4+bAkoDMOT4WNm2jZzLKOfje29UjGdVeIqKRkwK4rHae3mDOXYRKlKa?=
 =?us-ascii?Q?QGe5SsqbQPDMtQy5MNu/TgY7umxQk0G+e65WgXHs2ozVQ2TTL7uwUVg8luRV?=
 =?us-ascii?Q?21dm3ImzGdr9gOfF7O8mZetu8+nWnRO+/FmSHo+fxVpi69qkMGkfrC4F+pv9?=
 =?us-ascii?Q?va9vw9keRTuHeW2gwZHyOyC/YjkHX+OiHPwNw5dF+5OUp3V1wUsxSsbGVZe4?=
 =?us-ascii?Q?UJhsu7rzY1cfmV8ziR7xuZGf79IwnGwwcfliwn++K9Qcr9MbmybnZhUqXbjy?=
 =?us-ascii?Q?XmORnwzzRTe9iJgeUXyGODfJpgfe6sAV2/lYwrWsjYWEwkTyI+6RoLo+nNks?=
 =?us-ascii?Q?cVrQh1AlUigshfjB8J9DI9ksgruwsSt2kUdK8W0/EbiiuatqbhSoROFSBpYM?=
 =?us-ascii?Q?4aP6tn7PKS4SQrgkHwXMgZb2N23hdBvsa4pzvHgzk6DQpHsQZMpxf7C5jcZS?=
 =?us-ascii?Q?WMks2gtLnQLbQth5nHkYDkHhmJEBEzb3F8QCnu0gOUsyWklKPR3GbkRWGuTU?=
 =?us-ascii?Q?wC+l9mKVNw25RHzHWJykmcM7M2fZ+CajcIJ9QgjlJmTVO2VGD1mfqfelUsm6?=
 =?us-ascii?Q?9Pn55PnOmi4w5hMrjO3IFqMSA8Zbs+rZ9XDHCdkJoeMmh/NlHT/njNSy4PXp?=
 =?us-ascii?Q?1KVclz3oKyVkcqQr4uo+k0ccGZkJiBrIIGCl0YO6U8jTDntYUZTxsTtC6oao?=
 =?us-ascii?Q?jRc2wNUIqVwO4HWZZ6VY9Uh5QTNMr9FgR9gZ6/7zWGgbVXS+Vtw+uB7KalOd?=
 =?us-ascii?Q?2kBM8yHHmpA6dGhYw5vX7V+DOLC7GEIvPgRDwWZMVl0jZuTR6KcUef4Dvaj8?=
 =?us-ascii?Q?DTRsB2WPolzsr+EM2SESfQLO0FrmjkVjPvAV2GFAdeHuqJlNumWGQN+ik1cB?=
 =?us-ascii?Q?Dy1UCz7zbxCwH6gfH3K5VPL8OCTd8Xq+i9N6q7DrrKvY5aNERye+L7J1xRGU?=
 =?us-ascii?Q?W61lCpAtIG4vOx5pGU03bneD4Epq9g56p177cp/k9fLFILGsgPMkEw5fyXwp?=
 =?us-ascii?Q?g6NsXUNGfdrL6Di5AgB5Xf601GtPzpUQh72OvHZ+1SH+ed3B6B/GHHCVkXrs?=
 =?us-ascii?Q?TTjxSVO64aM/tm6Dvkdq3C4gGg26x+ve1vMgg0DPvH0wnCRXlbXjXxRy3E3I?=
 =?us-ascii?Q?6g0fYwtfHK79pc+jzJ6rKYyxiOMBZaOqci6Raf6MPMCH6Jn73nP+G9q2qGwV?=
 =?us-ascii?Q?4C6eGGrfwbUZYkFbPC//AWwRJgUvTMFD0COKnwrcMmyHz0catCX62UAJSoxy?=
 =?us-ascii?Q?vnHQ9NttQKjAzSf55TrBi5PDZlymGisuv30lgugU7wdXUuCfahxy6E3pEl1B?=
 =?us-ascii?Q?maD8Y04FrKrcNCIWdK0TUx0/opm+Dm3+/OnaFbapImNv/M0mglmAmrv603yz?=
 =?us-ascii?Q?9druF9fMs+DmL2Q6w5kifMpIRo8Nbgx1CloIe/JqAx01XLMz1myng9ED5434?=
 =?us-ascii?Q?HLT5MhmmQ6qaEsN8JGwdJWJjQGcukkcexp5iSFUj?=
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: 4677f5da-c290-44e7-4976-08db0a523cbe
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Feb 2023 04:00:52.5288
 (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: AkKrMifcskQCogfBDtYRlbfm3s+Ub1L66zNaePWfRijYpIb8s9KMUzH81AY/HoxvBwq6SNjpWzp/BjFap/bbjA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7437
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: Wednesday, February 8, 2023 5:18 AM
>=20
> iommu_group_replace_domain() is introduced to support use cases where
> an
> iommu_group can be attached to a new domain without getting detached
> from
> the old one. This replacement feature will be useful, for cases such as:
> 1) vPASID mode, when a guest wants to replace a single pasid (PASID=3D0)
>    table with a larger table (PASID=3DN)
> 2) Nesting mode, when switching the attaching device from an S2 domain
>    to an S1 domain, or when switching between relevant S1 domains.
> as it allows these cases to switch seamlessly without a DMA disruption.
>=20
> So, call iommu_group_replace_domain() in the iommufd_device_do_attach().
> And add a __iommmufd_device_detach helper to allow the replace routine
> to
> do a partial detach on the current hwpt that's being replaced. Though the
> updated locking logic is overcomplicated, it will be eased, once those
> iopt_table_add/remove_ioas and list_add/del calls are moved to hwpt's
> allocation/destroy() functions in the coming nesting series, as that'll
> depend on a new ->domain_alloc_user op in the iommu core.

then why not moving those changes into this series to make it simple?

>=20
> Also, block replace operations that are from/to auto_domains, i.e. only
> user-allocated hw_pagetables can be replaced or replaced with.

where does this restriction come from? iommu_group_replace_domain()
can switch between any two UNMANAGED domains. What is the extra
problem in iommufd to support from/to auto domains?

>=20
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> ---
>  drivers/iommu/iommufd/device.c          | 101 +++++++++++++++++-------
>  drivers/iommu/iommufd/iommufd_private.h |   2 +
>  2 files changed, 76 insertions(+), 27 deletions(-)
>=20
> diff --git a/drivers/iommu/iommufd/device.c
> b/drivers/iommu/iommufd/device.c
> index b8c3e3baccb5..8a9834fc129a 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -9,6 +9,8 @@
>  #include "io_pagetable.h"
>  #include "iommufd_private.h"
>=20
> +MODULE_IMPORT_NS(IOMMUFD_INTERNAL);
> +
>  static bool allow_unsafe_interrupts;
>  module_param(allow_unsafe_interrupts, bool, S_IRUGO | S_IWUSR);
>  MODULE_PARM_DESC(
> @@ -194,9 +196,61 @@ static bool
> iommufd_hw_pagetable_has_group(struct iommufd_hw_pagetable *hwpt,
>  	return false;
>  }
>=20
> +/**
> + * __iommmufd_device_detach - Detach a device from idev->hwpt to
> new_hwpt

'from ... to ...' means a replace semantics. then this should be called
iommufd_device_replace_hwpt().

> + * @idev: device to detach
> + * @new_hwpt: new hw_pagetable to attach (pass in NULL for a simple
> detach)
> + * @detach_group: flag to call iommu_detach_group
> + *
> + * This is a cleanup helper shared by the replace and detach routines.
> Comparing
> + * to a detach routine, a replace routine only needs a partial detach
> procedure:
> + * it does not need the iommu_detach_group(); it will attach the device =
to a
> new
> + * hw_pagetable after a partial detach from the currently attached
> hw_pagetable,
> + * so certain steps can be skipped if two hw_pagetables have the same IO=
AS.
> + */
> +static void __iommmufd_device_detach(struct iommufd_device *idev,
> +				     struct iommufd_hw_pagetable
> *new_hwpt,
> +				     bool detach_group)
> +{
> +	struct iommufd_hw_pagetable *hwpt =3D idev->hwpt;
> +	struct iommufd_ioas *new_ioas =3D NULL;
> +
> +	if (new_hwpt)
> +		new_ioas =3D new_hwpt->ioas;
> +
> +	mutex_lock(&hwpt->devices_lock);
> +	list_del(&idev->devices_item);
> +	if (hwpt->ioas !=3D new_ioas)
> +		mutex_lock(&hwpt->ioas->mutex);

I think new_ioas->mutext was meant here.

> +	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.