> From: Alex Williamson <alex.williamson@xxxxxxxxxx> > Sent: Friday, March 11, 2022 4:50 AM > > On Wed, 9 Mar 2022 10:11:06 +0000 > "Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote: > > > > From: Alex Williamson <alex.williamson@xxxxxxxxxx> > > > Sent: Wednesday, March 9, 2022 3:33 AM > > > > > > On Tue, 8 Mar 2022 08:11:11 +0000 > > > "Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote: > > > > > > > > From: Alex Williamson <alex.williamson@xxxxxxxxxx> > > > > > 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@xxxxxxxx> > +L: kvm@xxxxxxxxxxxxxxx > +S: Maintained > +P: Documentation/vfio/vfio-pci-vendor-driver-acceptance.rst > +F: drivers/vfio/pci/*/ > + > VFIO PLATFORM DRIVER > M: Eric Auger <eric.auger@xxxxxxxxxx> > L: kvm@xxxxxxxxxxxxxxx > > 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
