RE: [PATCH v8 8/9] hisi_acc_vfio_pci: Add support for VFIO live migration

["Tian, Kevin" <kevin.tian@intel.com>]

> From: Alex Williamson <alex.williamson@redhat.com>
> Sent: Friday, March 11, 2022 4:50 AM
> 
> 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
> > >
> > > On Tue, 8 Mar 2022 08:11:11 +0000
> > > "Tian, Kevin" <kevin.tian@intel.com> wrote:
> > >
> > > > > From: Alex Williamson <alex.williamson@redhat.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@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,
> 

Yes, this works for me. Moving forward the kdoc may be expanded
to include certain template/example to demonstrate necessary 
information to be provided by vendor drivers if common review
gaps are repeatedly identified from practice.

Thanks
Kevin