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>
CC:     "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        "kwankhede@nvidia.com" <kwankhede@nvidia.com>,
        "hch@lst.de" <hch@lst.de>,
        "jean-philippe@linaro.org" <jean-philippe@linaro.org>,
        "Jiang, Dave" <dave.jiang@intel.com>,
        "Raj, Ashok" <ashok.raj@intel.com>,
        "corbet@lwn.net" <corbet@lwn.net>,
        "parav@mellanox.com" <parav@mellanox.com>,
        "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
        "lkml@metux.net" <lkml@metux.net>,
        "david@gibson.dropbear.id.au" <david@gibson.dropbear.id.au>,
        "dwmw2@infradead.org" <dwmw2@infradead.org>,
        "Tian, Jun J" <jun.j.tian@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "lushenming@huawei.com" <lushenming@huawei.com>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "robin.murphy@arm.com" <robin.murphy@arm.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+UqMJ7tQK2EdmauuvK2AgACKnbCAALw6gIAHYVYAgABthQCAABACQIAABTKAgAAAdqA=
Date:   Mon, 27 Sep 2021 13:32:34 +0000
Message-ID: <BN9PR11MB543327D25DCE242919F7909A8CA79@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>
 <20210927115342.GW964074@nvidia.com>
 <BN9PR11MB5433502FEF11940984774F278CA79@BN9PR11MB5433.namprd11.prod.outlook.com>
 <20210927130935.GZ964074@nvidia.com>
In-Reply-To: <20210927130935.GZ964074@nvidia.com>
Accept-Language: en-US
Content-Language: en-US
dlp-version: 11.6.200.16
dlp-product: dlpe-windows
dlp-reaction: no-action
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?7x/ugAdGWWctv4j/dOWSqpSHMl7f0fRzZ8LwdScvZe4daRZ8qxWXF8YOuAPa?=
 =?us-ascii?Q?rPX9jzeOCzoHq56PSWVrui0Erm/d5mV//p1+btHhEKhVbPipo+8VCA0UfybN?=
 =?us-ascii?Q?4OfI/ErahzPM58cHGUaH2lG2qKQUOf9iH6vgkJRynxueKftiew5MECex+Mhr?=
 =?us-ascii?Q?PATBZ/PYNt11iBBhegOmqBWYq8lL1V1X6rFiKhEIV4tEXWBf+3d47FMyamvb?=
 =?us-ascii?Q?6J9brnb8aD67Sb3zWbnHI+WH9olb+9Bl38Z/YGJaJSa3iGb6tyXxEh91//Qe?=
 =?us-ascii?Q?cX375BF8c+TB05WHy9vAtqq1a9At2iz+LuXu4koT8ZQg+gmGvVoT6RBDAl1L?=
 =?us-ascii?Q?2BCFh2V4MpgWOEoZEhHpceY3vtz4jcu2LrBi3Vor12Qkl+yZ9GjOTnhnFKwI?=
 =?us-ascii?Q?KZmwpvMCkiU5Ptob4DCl1xyA8+Ap25crzPYG6onyfh/wOFMuqKBbTlBOROWV?=
 =?us-ascii?Q?LX2G8keaDuKCmthLTSmPLllDMr0y8VnKClkkA/7aw9+KKroOL9dNmf2nj6hk?=
 =?us-ascii?Q?+o+E2iWwlsB4gHWhxqLSAD2UcnvDMto3XrbcDZsm1PpVbSQBz3TJhZ7aETzQ?=
 =?us-ascii?Q?Cr0vr2LuRfdZsXkVkt9rjZdoXRz2oBU8kknzZQMqik6v7sK81fyVbLYlX+g2?=
 =?us-ascii?Q?qXO73QT8oI8klbnBbz5S1j9YXvzVGM/yvw7Qa27sRtwCwK09dCiQGc1zD/+1?=
 =?us-ascii?Q?T9Mg7POwG3Eiw5D2nVv7KaEpS9hFw1s1aESeXMO4cz5PyzWFKF2xhTbF8fpK?=
 =?us-ascii?Q?i+8xHKUVBOJofl6aVvtHaC3L3wfm3TvcAHwirSxYqNQB+xO9tqCq1QVVWLvW?=
 =?us-ascii?Q?k6bXApXKzDOe8kgjiOYGwmNPNOl39+vL+eNvFaO5G5dGY7qqn8Hw6jdc0lOM?=
 =?us-ascii?Q?jqLN1K+BnaWt9mnn5ruf1w5nEI/fjEbYe4rDjPT4WgfZdpvD7EyQmtE+O7bi?=
 =?us-ascii?Q?G5JYP8KHW2jfAq9844YQmV6VeOJg0M+Q/BW0j0UZJyqRvjZe+xrU3R0PQnXB?=
 =?us-ascii?Q?R1dBUqhrkyfq8E6wMnVdjzss/J4Zhb3W+xrThzUmy3m++gqKS1DrWgAucByn?=
 =?us-ascii?Q?ghFG/ZUDHBRQ6UaDuLIgtV+tvvr96SVyAThodu8q5j11q9Q5xlJtYb90MFrW?=
 =?us-ascii?Q?ciWFouFyi5RHgdk/XjdWxELNoS+Uk0ezRftpvHPtzVbtk7a0dnQKu8ocUi5A?=
 =?us-ascii?Q?SASJSWYeNt5K+IopjPHMa91rQKUIc/MD8M9J8qd9U3eaQBEZxDQ2dPZ/JjHm?=
 =?us-ascii?Q?7NI5OQMfOxPZy4yzh3hSb/NVSLaWnBE0y0NKN9QK1EfOwQKAOQK1Ci+Yt+3V?=
 =?us-ascii?Q?yuceAuiAcPs1e2OnOowfFOwt?=
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: d6a00164-17f5-4d4e-375b-08d981bb4394
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Sep 2021 13:32:34.1205
 (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: K5wcpbFK5YMNzlR7sIE90eqDTHFdkFd4Dg1n/jGTo1C0DbJzZHuVSayBv4qBDj4jQLjYKsJefFdS3JyQg9BFJQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR1101MB2148
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Jason Gunthorpe
> Sent: Monday, September 27, 2021 9:10 PM
>=20
> On Mon, Sep 27, 2021 at 01:00:08PM +0000, Tian, Kevin wrote:
>=20
> > > I think for such a narrow usage you should not change the struct
> > > device_driver. Just have pci_stub call a function to flip back to use=
r
> > > mode.
> >
> > Here we want to ensure that kernel dma should be blocked
> > if the group is already marked for user-dma. If we just blindly
> > do it for any driver at this point (as you commented earlier):
> >
> > +       ret =3D iommu_set_kernel_ownership(dev);
> > +       if (ret)
> > +               return ret;
> >
> > how would pci-stub reach its function to indicate that it doesn't
> > do dma and flip back?
>=20
> > Do you envision a simpler policy that no driver can be bound
> > to the group if it's already set for user-dma? what about vfio-pci
> > itself?
>=20
> Yes.. I'm not sure there is a good use case to allow the stub drivers
> to load/unload while a VFIO is running. At least, not a strong enough
> one to justify a global change to the driver core..

I'm fine with not loading pci-stub. From the very 1st commit msg
looks pci-stub was introduced before vfio to prevent host driver=20
loading when doing device assignment with KVM. I'm not sure=20
whether other usages are built on pci-stub later, but in general it's=20
not good to position devices in a same group into different usages.

but I'm little worried that even vfio-pci itself cannot be bound now,
which implies that all devices in a group which are intended to be
used by the user must be bound to vfio-pci in a breath before the=20
user attempts to open any of them, i.e. late-binding and device-
hotplug is disallowed after the initial open. I'm not sure how=20
important such an usage would be, but it does cause user-tangible
semantics change.

Alex?

>=20
> > > > +static int iommu_dev_viable(struct device *dev, void *data)
> > > > +{
> > > > +	enum dma_hint hint =3D *data;
> > > > +	struct device_driver *drv =3D READ_ONCE(dev->driver);
> > >
> > > Especially since this isn't locked properly or safe.
> >
> > I have the same worry when copying from vfio. Not sure how
> > vfio gets safe with this approach...
>=20
> Fixing the locking in vfio_dev_viable is part of deleting the unbound
> list. Once it properly uses the device_lock and doesn't race with the
> driver core like this things are much better. Don't copy this stuff
> into the iommu core without fixing it.

sure. Above was just a quickly-baked sample code to match your=20
thought.

>=20
> https://github.com/jgunthorpe/linux/commit/fa6abb318ccca114da12c0b5b1
> 23c99131ace926
> https://github.com/jgunthorpe/linux/commit/45980bd90b023d1eea56df70d
> 1c395bdf4cc7cf1
>=20
> I can't remember if the above is contingent on some of the mdev
> cleanups or not.. Have to get back to it.
>=20

my home network has some problem to access above links. Will check it
tomorrow and follow the fix when working on the formal change in=20
iommu core.

Thanks
Kevin