lkml.org 
[lkml]   [2022]   [Mar]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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@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,

Alex

\
 
 \ /
  Last update: 2022-03-10 21:51    [W:0.117 / U:0.724 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site