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 05/10] iommufd: Add replace support in
 iommufd_access_set_ioas()
Thread-Topic: [PATCH v2 05/10] iommufd: Add replace support in
 iommufd_access_set_ioas()
Thread-Index: AQHZOzoOetEnDUz6KU+y7zqlV4tnmq7F8F9A
Date:   Thu, 9 Feb 2023 03:13:08 +0000
Message-ID: <BN9PR11MB5276AE27E37866B82A6E03608CD99@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <cover.1675802050.git.nicolinc@nvidia.com>
 <931be169ff4a1f4d4f1ed060d722c2dc17ce6667.1675802050.git.nicolinc@nvidia.com>
In-Reply-To: <931be169ff4a1f4d4f1ed060d722c2dc17ce6667.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?ukGjc64Gc1e/YbMLtLvm8q2fTP7M3BdnX029TBOE2ev+ah8pJbAp5FLk6PWf?=
 =?us-ascii?Q?Aqou1d5HOKMp9nZUBKz+mweGtHvrBIeRN7BnBwriOO4wbmpyZ3jjXPrd0X6A?=
 =?us-ascii?Q?0MT6ErLPb0H9QlMYfJdvNLc/raHiXDY6IZOWqrOxiLri7US9SU10VAycr6uD?=
 =?us-ascii?Q?616+M6kVZfz4imKyFHfldhSsONdyOU+2frxng+xTFZwrjTbx9xQH8Llj40i0?=
 =?us-ascii?Q?Y4jkXht4xVoZZvRA/zfgDyrYR5RJosgbdPrNjkC/oTRu5BOLdR4VqPw17Sp+?=
 =?us-ascii?Q?bRS3qa/FaRE6m3s4CnmKMZv2ri7dCubzyKr49k0/I93WU6uUjnnOErx7gdmF?=
 =?us-ascii?Q?r5y5E4ESkeG2zUS3M9+wVkcB1JnOjAtpvKbEGUv5/kILuL07Eyl7p4aV62R4?=
 =?us-ascii?Q?eC9sz3aMEu3QHuPViyoKwetphu9jL7gm4Xyp5klBrtH0UCy4/oS6qXpOwY2a?=
 =?us-ascii?Q?Adta9grfUQhlEhqFkmNSlySEnEeU3zqSUJDq7FUSdKlvmQre7WBF5Ed5kMxD?=
 =?us-ascii?Q?m6r9p1JUcU/DFvwoTt5pD9rPtlRDHbj0Ot+Vg0/8DfkJl7kqC/p6FF5Z6NzF?=
 =?us-ascii?Q?j1Nv8JaLeNlFbNXY64qImEP7scHP5Z3aXlTWoRyn0jSGq2hHhi4bXEcbvDOM?=
 =?us-ascii?Q?zR4FKK7ibHgyhBr0TFj21m6BOP5in2fgkNC60cz2CvfTpSLWQMD5I0tMItuA?=
 =?us-ascii?Q?7mzea00zmVZk3fESxnct/1XrGUHefjjiplSHbuFKENbwU3glKximnunwsChM?=
 =?us-ascii?Q?dmM2i/UobViURaNQhuCggOmGiOvWG8qJCcXJnlZDRShLCuNNyCIb4cZ9rPIQ?=
 =?us-ascii?Q?LI1cAzEG2VIDGIuRBYYWHpl4FZ7ukZS+dizVkpHCGRLcCAwKz1lv3TYzi0w+?=
 =?us-ascii?Q?j7oDuah4qKblaU2uPGlfEXXAnH97jykbtlXxZ1hy5YrLCCaWk1f1loyzpSJQ?=
 =?us-ascii?Q?A7GjYloe5ALzoOWEAkKxFaudqOd/RrZBA12adgKF7+jvgwV+9gmmyHJtemGR?=
 =?us-ascii?Q?zefTjJF5F64NOXtEDTc0Nja853eoLxP9vXt8T7r+witKaVWQsKjyjHxJO49i?=
 =?us-ascii?Q?w1P7eVrA57u8fsaw6mBhK3KGkueioQZeQ/sCTA66ME+L7XRSALl3zm/6GwcF?=
 =?us-ascii?Q?iqdX3PDs0fGXyRs6PNjMBsKKeIkNa5/eS3QhFGKulI/xwxOfCwqhfdWyDdjg?=
 =?us-ascii?Q?v3eYsw/zBS4p3b8czqQndEb+4O1qCQrDEc29SgYDgUqa6MQU74bEezpIRCog?=
 =?us-ascii?Q?n03o2QSp4bjgmLkMlj8mGNhz5w9VREDq20C7eC3cjuK+ytIXOiqZe9F9GzTC?=
 =?us-ascii?Q?CaR+V/Cz6XKmb+2H4FhoGVReUWyGWPksiB7dzZj8s0V8IrjUVB6+HJ5Ph45R?=
 =?us-ascii?Q?KeXMjs81SUhUWxSsLmc3rFut7/N4ll3f7t6QrleJYaAWo7UAYpYrViefzKJH?=
 =?us-ascii?Q?HSHl5RKgB4A7xm/m4UlTKhuEUdxVJFQA9GEEHaeimbA9LPup4tHaopRHKEfx?=
 =?us-ascii?Q?Wm3vjChPylS1QDZrC90Zc3oz0pPs/yAxJOrUtNzvkjLb2eDbel7ZtGFmXCjg?=
 =?us-ascii?Q?YWm7CZc5hQQ0ZSd26LUP7zOU0/dOKkv2QAOA6olw?=
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: fdc264f3-a263-46d1-ac1d-08db0a4b91a7
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Feb 2023 03:13:08.5410
 (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: 9f4aDWO7QMwbXTZYuJoKNuZJqA8NjAScxFrQ4notNgNdTge+LsLS+VL1xRzg7TbPGoCxt7X0zoUDRvQGOwpSFw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB7103
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
> Support an access->ioas replacement in iommufd_access_set_ioas(), which
> sets the access->ioas to NULL provisionally so that any further incoming
> iommufd_access_pin_pages() callback can be blocked.
>=20
> Then, call access->ops->unmap() to clean up the entire iopt. To allow an
> iommufd_access_unpin_pages() callback to happen via this unmap() call,
> add an ioas_unpin pointer so the unpin routine won't be affected by the
> "access->ioas =3D NULL" trick above.
>=20
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>

move patch10 before this one.

> ---
>  drivers/iommu/iommufd/device.c          | 16 ++++++++++++++--
>  drivers/iommu/iommufd/iommufd_private.h |  1 +
>  2 files changed, 15 insertions(+), 2 deletions(-)
>=20
> diff --git a/drivers/iommu/iommufd/device.c
> b/drivers/iommu/iommufd/device.c
> index f4bd6f532a90..10ce47484ffa 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -509,11 +509,23 @@ int iommufd_access_set_ioas(struct
> iommufd_access *access, u32 ioas_id)
>  		iommufd_ref_to_users(obj);
>  	}
>=20
> +	/*
> +	 * Set ioas to NULL to block any further iommufd_access_pin_pages().
> +	 * iommufd_access_unpin_pages() can continue using access-
> >ioas_unpin.
> +	 */
> +	access->ioas =3D NULL;
> +
>  	if (cur_ioas) {
> +		if (new_ioas) {
> +			mutex_unlock(&access->ioas_lock);
> +			access->ops->unmap(access->data, 0, ULONG_MAX);
> +			mutex_lock(&access->ioas_lock);
> +		}

why does above only apply to a valid new_ioas? this is the cleanup on
cur_ioas then required even when new_ioas=3DNULL.

Instead here the check should be "if (access->ops->unmap)". with
patch10 the cleanup is required only for driver which is allowed to
pin pages.