[v2] vfio-pci: Provide reviewers and acceptance criteria for vendor drivers

Commit Message

Alex Williamson March 14, 2022, 7:14 p.m. UTC

Vendor or device specific extensions for devices exposed to userspace
through the vfio-pci-core library open both new functionality and new
risks.  Here we attempt to provided formalized requirements and
expectations to ensure that future drivers both collaborate in their
interaction with existing host drivers, as well as receive additional
reviews from community members with experience in this area.

Cc: Jason Gunthorpe <[e-mail]>
Cc: Yishai Hadas <[e-mail]>
Cc: Shameer Kolothum <[e-mail]>
Cc: Kevin Tian <[e-mail]>
Signed-off-by: Alex Williamson <[e-mail]>
---

v2:

Added Yishai

v1:

Per the proposal here[1], I've collected those that volunteered and
those that I interpreted as showing interest (alpha by last name).  For
those on the reviewers list below, please R-b/A-b to keep your name as a
reviewer.  More volunteers are still welcome, please let me know
explicitly; R-b/A-b will not be used to automatically add reviewers but
are of course welcome.  Thanks,

Alex

[1]https://lore.kernel.org/all/[e-mail]/
 .../vfio/vfio-pci-vendor-driver-acceptance.rst     |   35 ++++++++++++++++++++
 MAINTAINERS                                        |   10 ++++++
 2 files changed, 45 insertions(+)
 create mode 100644 Documentation/vfio/vfio-pci-vendor-driver-acceptance.rst

Comments

Shameerali Kolothum Thodi March 14, 2022, 7:32 p.m. UTC | #1

> -----Original Message-----
> From: Alex Williamson [mailto:[e-mail]]
> Sent: 14 March 2022 19:15
> To: [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]; Shameerali Kolothum Thodi
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> Subject: [PATCH v2] vfio-pci: Provide reviewers and acceptance criteria for
> vendor drivers
> 
> Vendor or device specific extensions for devices exposed to userspace
> through the vfio-pci-core library open both new functionality and new
> risks.  Here we attempt to provided formalized requirements and
> expectations to ensure that future drivers both collaborate in their
> interaction with existing host drivers, as well as receive additional
> reviews from community members with experience in this area.
> 
> Cc: Jason Gunthorpe <[e-mail]>
> Cc: Yishai Hadas <[e-mail]>
> Cc: Shameer Kolothum <[e-mail]>
> Cc: Kevin Tian <[e-mail]>
> Signed-off-by: Alex Williamson <[e-mail]>

Acked-by: Shameer Kolothum <[e-mail]>

Thanks,
Shameer

> ---
> 
> v2:
> 
> Added Yishai
> 
> v1:
> 
> Per the proposal here[1], I've collected those that volunteered and
> those that I interpreted as showing interest (alpha by last name).  For
> those on the reviewers list below, please R-b/A-b to keep your name as a
> reviewer.  More volunteers are still welcome, please let me know
> explicitly; R-b/A-b will not be used to automatically add reviewers but
> are of course welcome.  Thanks,
> 
> Alex
> 
> [1]https://lore.kernel.org/all/20220310134954.0df4bb12.alex.williamson@red
> hat.com/
>  .../vfio/vfio-pci-vendor-driver-acceptance.rst     |   35
> ++++++++++++++++++++
>  MAINTAINERS                                        |   10 ++++++
>  2 files changed, 45 insertions(+)
>  create mode 100644
> Documentation/vfio/vfio-pci-vendor-driver-acceptance.rst
> 
> 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..3a108d748681
> --- /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/Acked-by/etc 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..7f6b14013412 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20314,6 +20314,16 @@ F:	drivers/vfio/mdev/
>  F:	include/linux/mdev.h
>  F:	samples/vfio-mdev/
> 
> +VFIO PCI VENDOR DRIVERS
> +R:	Jason Gunthorpe <[e-mail]>
> +R:	Yishai Hadas <[e-mail]>
> +R:	Shameer Kolothum <[e-mail]>
> +R:	Kevin Tian <[e-mail]>
> +L:	[e-mail]
> +S:	Maintained
> +P:	Documentation/vfio/vfio-pci-vendor-driver-acceptance.rst
> +F:	drivers/vfio/pci/*/
> +
>  VFIO PLATFORM DRIVER
>  M:	Eric Auger <[e-mail]>
>  L:	[e-mail]
> 
>

Jonathan Corbet March 14, 2022, 7:42 p.m. UTC | #2

Alex Williamson <[e-mail]> writes:

> Vendor or device specific extensions for devices exposed to userspace
> through the vfio-pci-core library open both new functionality and new
> risks.  Here we attempt to provided formalized requirements and
> expectations to ensure that future drivers both collaborate in their
> interaction with existing host drivers, as well as receive additional
> reviews from community members with experience in this area.
>
> Cc: Jason Gunthorpe <[e-mail]>
> Cc: Yishai Hadas <[e-mail]>
> Cc: Shameer Kolothum <[e-mail]>
> Cc: Kevin Tian <[e-mail]>
> Signed-off-by: Alex Williamson <[e-mail]>
> ---

One thing...

>  .../vfio/vfio-pci-vendor-driver-acceptance.rst     |   35 ++++++++++++++++++++
>  MAINTAINERS                                        |   10 ++++++
>  2 files changed, 45 insertions(+)

If you add a new RST file, you need to add it to an index.rst somewhere
so that it becomes part of the kernel docs build.

Also, though: can we avoid creating a new top-level documentation
directory for just this file?  It seems like it would logically be a
part of the maintainers guide (Documentation/maintainer) ... ?

Thanks,

jon

Alex Williamson March 14, 2022, 7:52 p.m. UTC | #3

Hi Jon,

On Mon, 14 Mar 2022 13:42:51 -0600
Jonathan Corbet <[e-mail]> wrote:

> Alex Williamson <[e-mail]> writes:
> 
> > Vendor or device specific extensions for devices exposed to userspace
> > through the vfio-pci-core library open both new functionality and new
> > risks.  Here we attempt to provided formalized requirements and
> > expectations to ensure that future drivers both collaborate in their
> > interaction with existing host drivers, as well as receive additional
> > reviews from community members with experience in this area.
> >
> > Cc: Jason Gunthorpe <[e-mail]>
> > Cc: Yishai Hadas <[e-mail]>
> > Cc: Shameer Kolothum <[e-mail]>
> > Cc: Kevin Tian <[e-mail]>
> > Signed-off-by: Alex Williamson <[e-mail]>
> > ---  
> 
> One thing...
> 
> >  .../vfio/vfio-pci-vendor-driver-acceptance.rst     |   35 ++++++++++++++++++++
> >  MAINTAINERS                                        |   10 ++++++
> >  2 files changed, 45 insertions(+)  
> 
> If you add a new RST file, you need to add it to an index.rst somewhere
> so that it becomes part of the kernel docs build.

Whoops

> Also, though: can we avoid creating a new top-level documentation
> directory for just this file?  It seems like it would logically be a
> part of the maintainers guide (Documentation/maintainer) ... ?

I'm not sure it's appropriate for Documentation/maintainer/ but it
would make sense to link it from maintainer-entry-profile.rst there.
What if I move it to Documentation/driver-api where there are a couple
other vfio docs?  Thanks,

Alex

Jonathan Corbet March 14, 2022, 7:58 p.m. UTC | #4

Alex Williamson <[e-mail]> writes:

>> Also, though: can we avoid creating a new top-level documentation
>> directory for just this file?  It seems like it would logically be a
>> part of the maintainers guide (Documentation/maintainer) ... ?
>
> I'm not sure it's appropriate for Documentation/maintainer/ but it
> would make sense to link it from maintainer-entry-profile.rst there.
> What if I move it to Documentation/driver-api where there are a couple
> other vfio docs?  Thanks,

Sure, that's fine.

Thanks,

jon

Patch

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..3a108d748681
--- /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/Acked-by/etc 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..7f6b14013412 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20314,6 +20314,16 @@  F:	drivers/vfio/mdev/
 F:	include/linux/mdev.h
 F:	samples/vfio-mdev/
 
+VFIO PCI VENDOR DRIVERS
+R:	Jason Gunthorpe <[e-mail]>
+R:	Yishai Hadas <[e-mail]>
+R:	Shameer Kolothum <[e-mail]>
+R:	Kevin Tian <[e-mail]>
+L:	[e-mail]
+S:	Maintained
+P:	Documentation/vfio/vfio-pci-vendor-driver-acceptance.rst
+F:	drivers/vfio/pci/*/
+
 VFIO PLATFORM DRIVER
 M:	Eric Auger <[e-mail]>
 L:	[e-mail]