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, Alex
