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;
From:   "Tian, Kevin" <kevin.tian@intel.com>
To:     Jason Gunthorpe <jgg@nvidia.com>,
        Lu Baolu <baolu.lu@linux.intel.com>
CC:     "Liu, Yi L" <yi.l.liu@intel.com>,
        "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
        "hch@lst.de" <hch@lst.de>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        "joro@8bytes.org" <joro@8bytes.org>,
        "jean-philippe@linaro.org" <jean-philippe@linaro.org>,
        "parav@mellanox.com" <parav@mellanox.com>,
        "lkml@metux.net" <lkml@metux.net>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "lushenming@huawei.com" <lushenming@huawei.com>,
        "eric.auger@redhat.com" <eric.auger@redhat.com>,
        "corbet@lwn.net" <corbet@lwn.net>,
        "Raj, Ashok" <ashok.raj@intel.com>,
        "yi.l.liu@linux.intel.com" <yi.l.liu@linux.intel.com>,
        "Tian, Jun J" <jun.j.tian@intel.com>, "Wu, Hao" <hao.wu@intel.com>,
        "Jiang, Dave" <dave.jiang@intel.com>,
        "jacob.jun.pan@linux.intel.com" <jacob.jun.pan@linux.intel.com>,
        "kwankhede@nvidia.com" <kwankhede@nvidia.com>,
        "robin.murphy@arm.com" <robin.murphy@arm.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "iommu@lists.linux-foundation.org" <iommu@lists.linux-foundation.org>,
        "dwmw2@infradead.org" <dwmw2@infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "david@gibson.dropbear.id.au" <david@gibson.dropbear.id.au>,
        "nicolinc@nvidia.com" <nicolinc@nvidia.com>
Subject: RE: [RFC 06/20] iommu: Add iommu_device_init[exit]_user_dma
 interfaces
Thread-Topic: [RFC 06/20] iommu: Add iommu_device_init[exit]_user_dma
 interfaces
Thread-Index: AQHXrSF9WDj+Z+DU+UqMJ7tQK2EdmauuvK2AgACKnbCAALw6gIAHYVYAgACkNwCAAQ1OQIAAT36AgAAbKoCAAAj5AIAAq5rg
Date:   Wed, 29 Sep 2021 00:38:35 +0000
Message-ID: <BN9PR11MB5433B8CB2F1EA1A2D06586588CA99@BN9PR11MB5433.namprd11.prod.outlook.com>
References: <20210919063848.1476776-1-yi.l.liu@intel.com>
 <20210919063848.1476776-7-yi.l.liu@intel.com>
 <20210921170943.GS327412@nvidia.com>
 <BN9PR11MB5433DA330D4583387B59AA7F8CA29@BN9PR11MB5433.namprd11.prod.outlook.com>
 <20210922123931.GI327412@nvidia.com>
 <BN9PR11MB5433CE19425E85E7F52093278CA79@BN9PR11MB5433.namprd11.prod.outlook.com>
 <20210927150928.GA1517957@nvidia.com>
 <BN9PR11MB54337B7F65B98C2335B806938CA89@BN9PR11MB5433.namprd11.prod.outlook.com>
 <20210928115751.GK964074@nvidia.com>
 <9a314095-3db9-30fc-2ed9-4e46d385036d@linux.intel.com>
 <20210928140712.GL964074@nvidia.com>
In-Reply-To: <20210928140712.GL964074@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?hNPLYmX8pc2maH5J8/GDcXypaBAR1RyCCpGCxp94kDnQjF7Tc9eD3QFVmgWd?=
 =?us-ascii?Q?tP+uxs0V4wbAKv/v+UTtLyPp8lZ3QHVTQbjkEfkNlEyqRsJRbBRdaC/Uhak8?=
 =?us-ascii?Q?Md2YNlXbG9kGi4caGGNsYR6xJb1VfDkLev960Wmi4iG5UzXHiNIOIkm+E/rJ?=
 =?us-ascii?Q?QZROcf/Rfw3+n0RoI/ef0M0uiJfoFuy2Y4pU0TSARb+auc5XMCVEpRSG3Mmw?=
 =?us-ascii?Q?Mmc17qA7WzY+QWsGpXqH9jV5R9fiaNdU5K5vZdweFAc78jNNOxKBRs5gndo3?=
 =?us-ascii?Q?crGyecKx7tUdHRWzGNwY2yynfXPEGHPTgPtBQSeXxzBJ/2g0pKGAXyKQVF1o?=
 =?us-ascii?Q?Lqq7WT9YyRVIsYrg6PhJZSysC5F3e5hJcx3Goq3S1AanpBC1yqvaraV/O59Q?=
 =?us-ascii?Q?FjNSFFD6qLJu4P1Ahm8Q26ol69KEULnXegUXfhtpLeqzVuC2gPsVqHyItBbF?=
 =?us-ascii?Q?8IMjrTAzWVg1mMW5XlkJrBIJDeohCQO41RZsdMXauVcUR88OHeQsoPFq03v/?=
 =?us-ascii?Q?LtaLSX7uStzoPFS0Mjetylig2VgKStvs+DwkUlLuUruX+UwF72UFESVkCac+?=
 =?us-ascii?Q?RhmMgzo6xgK2oOzaC3yUt6KdcaMpwZEbvGlfNED095+iqPHixC5vxyS8C30f?=
 =?us-ascii?Q?SEWJJNQsaoHBVYCRqbt0/hvvFapImFLBNn6FUMsEtfM+4xDaiAN+5yCgA65k?=
 =?us-ascii?Q?hSWfm/DtK61GBrvrq+JAE9gkd44vaUZuakkPA6Szk1Dv5JUrKJruOzX55wUC?=
 =?us-ascii?Q?uMT0H/uiMGqVaEqJjmfbz+VD73gGuEkpA54BlP1Or/oI1cDP7gRPIUZrIJ/M?=
 =?us-ascii?Q?IaMHst7opwPyJ0SFfL1e4B2ccBAODC4kc+E9nKnIPeQGPkFLIPyehe6t+UfV?=
 =?us-ascii?Q?3Nj1IKMciUrYQr4z4UNlIVluL0SKBZi5KfSnP1C/V4HdjvJrMxCkWgRC1VNo?=
 =?us-ascii?Q?sE9dGIgxFsoU4MZKPYgVJJhwAlPnlInWGEm46yk5Ks6o+oUZ9H1jBufufD0g?=
 =?us-ascii?Q?ikjVCPcNf1soEamb3cSsHo/0uQv2d9qF47TTdVKPmnNU9eMKhgIEbLUSD5uH?=
 =?us-ascii?Q?oR1j5zp2YvjD4jOGcRTMsvKq3Up+POVvLxsU3u68jEjO56OthKrWPfhz0Sig?=
 =?us-ascii?Q?4A6NSEyYAwQM8rzH3DQ2PZoQcC+QeExX93mbf7uQY3egg0a2j+KmYe6dD+m/?=
 =?us-ascii?Q?wnBlKwNCcHmVGurD/Lux/xtXTyoIuHVc+BR3c4VIp6vmBhI+h95GqFNesgg1?=
 =?us-ascii?Q?Wc8KEJ4kWE5Jm1RZJ/kAWmtuktG5eYonqiCcmdFadGUP6nO1d30pebCPrJY8?=
 =?us-ascii?Q?Nw8=3D?=
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: BN9PR11MB5433.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8be6209a-eb75-46e9-994a-08d982e178e5
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Sep 2021 00:38:35.4971
 (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: NK8UXPO3bGuUCUPH8IZxRd2GveczNXFrmGcYyv9KRkGv3FwMU5T7AbJUhnHv88T0ZlLSwvvEPXW+5r0zzoHk9Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5434
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: Tuesday, September 28, 2021 10:07 PM
>=20
> On Tue, Sep 28, 2021 at 09:35:05PM +0800, Lu Baolu wrote:
> > Another issue is, when putting a device into user-dma mode, all devices
> > belonging to the same iommu group shouldn't be bound with a kernel-dma
> > driver. Kevin's prototype checks this by READ_ONCE(dev->driver). This i=
s
> > not lock safe as discussed below,
> >
> > https://lore.kernel.org/linux-
> iommu/20210927130935.GZ964074@nvidia.com/
> >
> > Any guidance on this?
>=20
> Something like this?
>=20
>=20

yes, with this group level atomics we don't need loop every dev->driver
respectively.

> int iommu_set_device_dma_owner(struct device *dev, enum
> device_dma_owner mode,
> 			       struct file *user_owner)
> {
> 	struct iommu_group *group =3D group_from_dev(dev);
>=20
> 	spin_lock(&iommu_group->dma_owner_lock);
> 	switch (mode) {
> 		case DMA_OWNER_KERNEL:
> 			if (iommu_group-
> >dma_users[DMA_OWNER_USERSPACE])
> 				return -EBUSY;
> 			break;
> 		case DMA_OWNER_SHARED:
> 			break;
> 		case DMA_OWNER_USERSPACE:
> 			if (iommu_group-
> >dma_users[DMA_OWNER_KERNEL])
> 				return -EBUSY;
> 			if (iommu_group->dma_owner_file !=3D user_owner) {
> 				if (iommu_group-
> >dma_users[DMA_OWNER_USERSPACE])
> 					return -EPERM;
> 				get_file(user_owner);
> 				iommu_group->dma_owner_file =3D
> user_owner;
> 			}
> 			break;
> 		default:
> 			spin_unlock(&iommu_group->dma_owner_lock);
> 			return -EINVAL;
> 	}
> 	iommu_group->dma_users[mode]++;
> 	spin_unlock(&iommu_group->dma_owner_lock);
> 	return 0;
> }
>=20
> int iommu_release_device_dma_owner(struct device *dev,
> 				   enum device_dma_owner mode)
> {
> 	struct iommu_group *group =3D group_from_dev(dev);
>=20
> 	spin_lock(&iommu_group->dma_owner_lock);
> 	if (WARN_ON(!iommu_group->dma_users[mode]))
> 		goto err_unlock;
> 	if (!iommu_group->dma_users[mode]--) {
> 		if (mode =3D=3D DMA_OWNER_USERSPACE) {
> 			fput(iommu_group->dma_owner_file);
> 			iommu_group->dma_owner_file =3D NULL;
> 		}
> 	}
> err_unlock:
> 	spin_unlock(&iommu_group->dma_owner_lock);
> }
>=20
>=20
> Where, the driver core does before probe:
>=20
>    iommu_set_device_dma_owner(dev, DMA_OWNER_KERNEL, NULL)
>=20
> pci_stub/etc does in their probe func:
>=20
>    iommu_set_device_dma_owner(dev, DMA_OWNER_SHARED, NULL)
>=20
> And vfio/iommfd does when a struct vfio_device FD is attached:
>=20
>    iommu_set_device_dma_owner(dev, DMA_OWNER_USERSPACE,
> group_file/iommu_file)
>=20

Just a nit. Per your comment in previous mail:

/* If set the driver must call iommu_XX as the first action in probe() */
 bool suppress_dma_owner:1;

Following above logic userspace drivers won't call iommu_XX in probe().
Just want to double confirm whether you see any issue here with this
relaxed behavior. If no problem:

/* If set the driver must call iommu_XX as the first action in probe() or
  * before it attempts to do DMA
  */
 bool suppress_dma_owner:1;

Thanks
Kevin