> From: Alex Williamson <[email protected]>
> Sent: Wednesday, March 9, 2022 3:33 AM
>
> On Tue, 8 Mar 2022 08:11:11 +0000
> "Tian, Kevin" <[email protected]> wrote:
>
> > > From: Alex Williamson <[email protected]>
> > > 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...
Thanks
Kevin
On Wed, 9 Mar 2022 10:11:06 +0000
"Tian, Kevin" <[email protected]> wrote:
> > From: Alex Williamson <[email protected]>
> > Sent: Wednesday, March 9, 2022 3:33 AM
> >
> > On Tue, 8 Mar 2022 08:11:11 +0000
> > "Tian, Kevin" <[email protected]> wrote:
> >
> > > > From: Alex Williamson <[email protected]>
> > > > 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 <[email protected]>
+L: [email protected]
+S: Maintained
+P: Documentation/vfio/vfio-pci-vendor-driver-acceptance.rst
+F: drivers/vfio/pci/*/
+
VFIO PLATFORM DRIVER
M: Eric Auger <[email protected]>
L: [email protected]
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
> -----Original Message-----
> From: Alex Williamson [mailto:[email protected]]
> Sent: 10 March 2022 20:50
> To: Tian, Kevin <[email protected]>
> Cc: Shameerali Kolothum Thodi <[email protected]>;
> Jason Gunthorpe <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Linuxarm <[email protected]>; liulongfang
> <[email protected]>; Zengtao (B) <[email protected]>;
> Jonathan Cameron <[email protected]>; Wangzhou (B)
> <[email protected]>; Xu Zaibo <[email protected]>
> 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" <[email protected]> wrote:
>
> > > From: Alex Williamson <[email protected]>
> > > Sent: Wednesday, March 9, 2022 3:33 AM
> > >
> > > On Tue, 8 Mar 2022 08:11:11 +0000
> > > "Tian, Kevin" <[email protected]> wrote:
> > >
> > > > > From: Alex Williamson <[email protected]>
> > > > > 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 <[email protected]>
> +L: [email protected]
> +S: Maintained
> +P: Documentation/vfio/vfio-pci-vendor-driver-acceptance.rst
> +F: drivers/vfio/pci/*/
> +
> VFIO PLATFORM DRIVER
> M: Eric Auger <[email protected]>
> L: [email protected]
>
> 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