Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 10 Mar 2022 13:49:54 -0700
From:   Alex Williamson <alex.williamson@redhat.com>
To:     "Tian, Kevin" <kevin.tian@intel.com>
Cc:     Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>,
        "Jason Gunthorpe" <jgg@nvidia.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "cohuck@redhat.com" <cohuck@redhat.com>,
        "mgurtovoy@nvidia.com" <mgurtovoy@nvidia.com>,
        "yishaih@nvidia.com" <yishaih@nvidia.com>,
        Linuxarm <linuxarm@huawei.com>,
        liulongfang <liulongfang@huawei.com>,
        "Zengtao (B)" <prime.zeng@hisilicon.com>,
        Jonathan Cameron <jonathan.cameron@huawei.com>,
        "Wangzhou (B)" <wangzhou1@hisilicon.com>,
        Xu Zaibo <xuzaibo@huawei.com>
Subject: Re: [PATCH v8 8/9] hisi_acc_vfio_pci: Add support for VFIO live
 migration
Message-ID: <20220310134954.0df4bb12.alex.williamson@redhat.com>
In-Reply-To: <BN9PR11MB527634CCF86829E0680E5E678C0A9@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20220303230131.2103-1-shameerali.kolothum.thodi@huawei.com>
        <20220303230131.2103-9-shameerali.kolothum.thodi@huawei.com>
        <20220304205720.GE219866@nvidia.com>
        <20220307120513.74743f17.alex.williamson@redhat.com>
        <aac9a26dc27140d9a1ce56ebdec393a6@huawei.com>
        <20220307125239.7261c97d.alex.williamson@redhat.com>
        <BN9PR11MB5276EBE887402EBE22630BAB8C099@BN9PR11MB5276.namprd11.prod.outlook.com>
        <20220308123312.1f4ba768.alex.williamson@redhat.com>
        <BN9PR11MB527634CCF86829E0680E5E678C0A9@BN9PR11MB5276.namprd11.prod.outlook.com>
Organization: Red Hat
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, 9 Mar 2022 10:11:06 +0000
"Tian, Kevin" <kevin.tian@intel.com> wrote:

> > From: Alex Williamson <alex.williamson@redhat.com>
> > Sent: Wednesday, March 9, 2022 3:33 AM
> >=20
> > On Tue, 8 Mar 2022 08:11:11 +0000
> > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> >  =20
> > > > From: Alex Williamson <alex.williamson@redhat.com>
> > > > Sent: Tuesday, March 8, 2022 3:53 AM =20
> > > > > =20
> > > > > > I think we still require acks from Bjorn and Zaibo for select p=
atches
> > > > > > in this series. =20
> > > > >
> > > > > I checked with Ziabo. He moved projects and is no longer looking =
into =20
> > > > crypto stuff. =20
> > > > > Wangzhou and LiuLongfang now take care of this. Received acks fro=
m =20
> > > > Wangzhou =20
> > > > > already and I will request Longfang to provide his. Hope that's o=
k. =20
> > > >
> > > > Maybe a good time to have them update MAINTAINERS as well.  Thanks,
> > > > =20
> > >
> > > I have one question here (similar to what we discussed for mdev befor=
e).
> > >
> > > Now we are adding vendor specific drivers under /drivers/vfio. Two dr=
ivers
> > > on radar and more will come. Then what would be the criteria for
> > > accepting such a driver? Do we prefer to a model in which the author =
=20
> > should =20
> > > provide enough background for vfio community to understand how it =20
> > works =20
> > > or as done here just rely on the PF driver owner to cover device spec=
ific
> > > code?
> > >
> > > If the former we may need document some process for what information
> > > is necessary and also need secure increased review bandwidth from key
> > > reviewers in vfio community.
> > >
> > > If the latter then how can we guarantee no corner case overlooked by =
both
> > > sides (i.e. how to know the coverage of total reviews)? Another open =
is =20
> > who =20
> > > from the PF driver sub-system should be considered as the one to give=
 the
> > > green signal. If the sub-system maintainer trusts the PF driver owner=
 and
> > > just pulls commits from him then having the r-b from the PF driver ow=
ner is
> > > sufficient. But if the sub-system maintainer wants to review detail c=
hange
> > > in every underlying driver then we probably also want to get the ack =
from
> > > the maintainer.
> > >
> > > Overall I didn't mean to slow down the progress of this series. But a=
bove
> > > does be some puzzle occurred in my review. =F0=9F=98=8A =20
> >=20
> > Hi Kevin,
> >=20
> > Good questions, I'd like a better understanding of expectations as
> > well.  I think the intentions are the same as any other sub-system, the
> > drivers make use of shared interfaces and extensions and the role of
> > the sub-system should be to make sure those interfaces are used
> > correctly and extensions fit well within the overall design.  However,
> > just as the network maintainer isn't expected to fully understand every
> > NIC driver, I think/hope we have the same expectations here.  It's
> > certainly a benefit to the community and perceived trustworthiness if
> > each driver outlines its operating model and security nuances, but
> > those are only ever going to be the nuances identified by the people
> > who have the access and energy to evaluate the device.
> >=20
> > It's going to be up to the community to try to determine that any new
> > drivers are seriously considering security and not opening any new gaps
> > relative to behavior using the base vfio-pci driver.  For the driver
> > examples we have, this seems a bit easier than evaluating an entire
> > mdev device because they're largely providing direct access to the
> > device rather than trying to multiplex a shared physical device.  We
> > can therefore focus on incremental functionality, as both drivers have
> > done, implementing a boilerplate vendor driver, then adding migration
> > support.  I imagine this won't always be the case though and some
> > drivers will re-implement much of the core to support further emulation
> > and shared resources.
> >=20
> > So how do we as a community want to handle this?  I wouldn't mind, I'd
> > actually welcome, some sort of review requirement for new vfio vendor
> > driver variants.  Is that reasonable?  What would be the criteria?
> > Approval from the PF driver owner, if different/necessary, and at least
> > one unaffiliated reviewer (preferably an active vfio reviewer or
> > existing vfio variant driver owner/contributor)?  Ideas welcome.
> > Thanks,
> >  =20
>=20
> Yes, and the criteria is the hard part. In the end it largely depend on=20
> the expectations of the reviewers. =20
>=20
> If the unaffiliated reviewer only cares about the usage of shared=20
> interfaces or extensions as you said then what this series does is
> just fine. Such type of review can be easily done via reading code=20
> and doesn't require detail device knowledge.
>=20
> On the other hand if the reviewer wants to do a full functional
> review of how migration is actually supported for such device,=20
> whatever information (patch description, code comment, kdoc,
> etc.) necessary to build a standalone migration story would be
> appreciated, e.g.:
>=20
>   - What composes the device state?
>   - Which portion of the device state is exposed to and managed
>     by the user and which is hidden from the user (i.e. controlled
>     by the PF driver)?
>   - Interface between the vfio driver and the device (and/or PF
>     driver) to manage the device state;
>   - Rich functional-level comments for the reviewer to dive into
>     the migration flow;
>   - ...
>=20
> I guess we don't want to force one model over the other. Just
> from my impression the more information the driver can=20
> provide the more time I'd like to spend on the review. Otherwise
> it has to trend to the minimal form i.e. the first model.
>=20
> and currently I don't have a concrete idea how the 2nd model will
> work. maybe it will get clear only when a future driver attracts=20
> people to do thorough review...

Do you think we should go so far as to formalize this via a MAINTAINERS
entry, for example:

diff --git a/Documentation/vfio/vfio-pci-vendor-driver-acceptance.rst b/Doc=
umentation/vfio/vfio-pci-vendor-driver-acceptance.rst
new file mode 100644
index 000000000000..54ebafcdd735
--- /dev/null
+++ b/Documentation/vfio/vfio-pci-vendor-driver-acceptance.rst
@@ -0,0 +1,35 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+Acceptance criteria for vfio-pci device specific driver variants
+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
+
+Overview
+--------
+The vfio-pci driver exists as a device agnostic driver using the
+system IOMMU and relying on the robustness of platform fault
+handling to provide isolated device access to userspace.  While the
+vfio-pci driver does include some device specific support, further
+extensions for yet more advanced device specific features are not
+sustainable.  The vfio-pci driver has therefore split out
+vfio-pci-core as a library that may be reused to implement features
+requiring device specific knowledge, ex. saving and loading device
+state for the purposes of supporting migration.
+
+In support of such features, it's expected that some device specific
+variants may interact with parent devices (ex. SR-IOV PF in support of
+a user assigned VF) or other extensions that may not be otherwise
+accessible via the vfio-pci base driver.  Authors of such drivers
+should be diligent not to create exploitable interfaces via such
+interactions or allow unchecked userspace data to have an effect
+beyond the scope of the assigned device.
+
+New driver submissions are therefore requested to have approval via
+Sign-off for any interactions with parent drivers.  Additionally,
+drivers should make an attempt to provide sufficient documentation
+for reviewers to understand the device specific extensions, for
+example in the case of migration data, how is the device state
+composed and consumed, which portions are not otherwise available to
+the user via vfio-pci, what safeguards exist to validate the data,
+etc.  To that extent, authors should additionally expect to require
+reviews from at least one of the listed reviewers, in addition to the
+overall vfio maintainer.
diff --git a/MAINTAINERS b/MAINTAINERS
index 4322b5321891..4f7d26f9aac6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20314,6 +20314,13 @@ F:	drivers/vfio/mdev/
 F:	include/linux/mdev.h
 F:	samples/vfio-mdev/
=20
+VFIO PCI VENDOR DRIVERS
+R:	Your Name <your.name@here.com>
+L:	kvm@vger.kernel.org
+S:	Maintained
+P:	Documentation/vfio/vfio-pci-vendor-driver-acceptance.rst
+F:	drivers/vfio/pci/*/
+
 VFIO PLATFORM DRIVER
 M:	Eric Auger <eric.auger@redhat.com>
 L:	kvm@vger.kernel.org

Ideally we'd have at least Yishai, Shameer, Jason, and yourself listed
as reviewers (Connie and I are included via the higher level entry).
Thoughts from anyone?  Volunteers for reviewers if we want to press
forward with this as formal acceptance criteria?  Thanks,

Alex