Message-ID: <20220310134954.0df4bb12.alex.williamson@redhat.com>
Date:   Thu, 10 Mar 2022 13:49:54 -0700
From:   Alex Williamson <alex.williamson@...hat.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>
Cc:     Shameerali Kolothum Thodi <shameerali.kolothum.thodi@...wei.com>,
        "Jason Gunthorpe" <jgg@...dia.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "cohuck@...hat.com" <cohuck@...hat.com>,
        "mgurtovoy@...dia.com" <mgurtovoy@...dia.com>,
        "yishaih@...dia.com" <yishaih@...dia.com>,
        Linuxarm <linuxarm@...wei.com>,
        liulongfang <liulongfang@...wei.com>,
        "Zengtao (B)" <prime.zeng@...ilicon.com>,
        Jonathan Cameron <jonathan.cameron@...wei.com>,
        "Wangzhou (B)" <wangzhou1@...ilicon.com>,
        Xu Zaibo <xuzaibo@...wei.com>
Subject: Re: [PATCH v8 8/9] hisi_acc_vfio_pci: Add support for VFIO live
 migration

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

> > From: Alex Williamson <alex.williamson@...hat.com>
> > Sent: Wednesday, March 9, 2022 3:33 AM
> > 
> > On Tue, 8 Mar 2022 08:11:11 +0000
> > "Tian, Kevin" <kevin.tian@...el.com> wrote:
> >   
> > > > From: Alex Williamson <alex.williamson@...hat.com>
> > > > Sent: Tuesday, March 8, 2022 3:53 AM  
> > > > >  
> > > > > > I think we still require acks from Bjorn and Zaibo for select patches
> > > > > > in this series.  
> > > > >
> > > > > I checked with Ziabo. He moved projects and is no longer looking into  
> > > > crypto stuff.  
> > > > > Wangzhou and LiuLongfang now take care of this. Received acks from  
> > > > Wangzhou  
> > > > > already and I will request Longfang to provide his. Hope that's ok.  
> > > >
> > > > Maybe a good time to have them update MAINTAINERS as well.  Thanks,
> > > >  
> > >
> > > I have one question here (similar to what we discussed for mdev before).
> > >
> > > Now we are adding vendor specific drivers under /drivers/vfio. Two drivers
> > > 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  
> > should  
> > > provide enough background for vfio community to understand how it  
> > works  
> > > or as done here just rely on the PF driver owner to cover device specific
> > > 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  
> > who  
> > > 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 owner is
> > > sufficient. But if the sub-system maintainer wants to review detail change
> > > 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 above
> > > does be some puzzle occurred in my review. 😊  
> > 
> > Hi Kevin,
> > 
> > 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.
> > 
> > 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.
> > 
> > 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,
> >   
> 
> Yes, and the criteria is the hard part. In the end it largely depend on 
> the expectations of the reviewers.  
> 
> If the unaffiliated reviewer only cares about the usage of shared 
> 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 
> and doesn't require detail device knowledge.
> 
> On the other hand if the reviewer wants to do a full functional
> review of how migration is actually supported for such device, 
> whatever information (patch description, code comment, kdoc,
> etc.) necessary to build a standalone migration story would be
> appreciated, e.g.:
> 
>   - 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;
>   - ...
> 
> I guess we don't want to force one model over the other. Just
> from my impression the more information the driver can 
> 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.
> 
> 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 
> 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/Documentation/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
+================================================================
+
+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/
 
+VFIO PCI VENDOR DRIVERS
+R:	Your Name <your.name@...e.com>
+L:	kvm@...r.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@...hat.com>
 L:	kvm@...r.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

Powered by blists - more mailing lists