lkml.org 
[lkml]   [2022]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v8 8/9] hisi_acc_vfio_pci: Add support for VFIO live migration
Date


> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: 10 March 2022 20:50
> To: Tian, Kevin <kevin.tian@intel.com>
> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Jason Gunthorpe <jgg@nvidia.com>; kvm@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-crypto@vger.kernel.org;
> linux-pci@vger.kernel.org; cohuck@redhat.com; mgurtovoy@nvidia.com;
> yishaih@nvidia.com; Linuxarm <linuxarm@huawei.com>; liulongfang
> <liulongfang@huawei.com>; Zengtao (B) <prime.zeng@hisilicon.com>;
> Jonathan Cameron <jonathan.cameron@huawei.com>; Wangzhou (B)
> <wangzhou1@hisilicon.com>; Xu Zaibo <xuzaibo@huawei.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@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,
>

Sure. Happy to help with reviews, verifications etc.

Thanks,
Shameer
\
 
 \ /
  Last update: 2022-03-11 14:23    [W:0.094 / U:2.532 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site