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

Commit Message

Alex Williamson March 14, 2022, 8:24 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: Kevin Tian <[e-mail]>
Acked-by: Shameer Kolothum <[e-mail]>
Signed-off-by: Alex Williamson <[e-mail]>
---

v3:

Relocate to Documentation/driver-api/
Include index.rst reference
Cross link from maintainer-entry-profile
Add Shameer's Ack

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]/

 Documentation/driver-api/index.rst                 |    1 +
 .../vfio-pci-vendor-driver-acceptance.rst          |   35 ++++++++++++++++++++
 .../maintainer/maintainer-entry-profile.rst        |    1 +
 MAINTAINERS                                        |   10 ++++++
 4 files changed, 47 insertions(+)
 create mode 100644 Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst

Comments

Tian, Kevin March 15, 2022, 2:14 a.m. UTC | #1

> From: Alex Williamson <[e-mail]>
> Sent: Tuesday, March 15, 2022 4:25 AM
> 
> 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: Kevin Tian <[e-mail]>
> Acked-by: Shameer Kolothum <[e-mail]>
> Signed-off-by: Alex Williamson <[e-mail]>

Acked-by: Kevin Tian <[e-mail]>

> ---
> 
> v3:
> 
> Relocate to Documentation/driver-api/
> Include index.rst reference
> Cross link from maintainer-entry-profile
> Add Shameer's Ack
> 
> 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@re
> dhat.com/
> 
>  Documentation/driver-api/index.rst                 |    1 +
>  .../vfio-pci-vendor-driver-acceptance.rst          |   35 ++++++++++++++++++++
>  .../maintainer/maintainer-entry-profile.rst        |    1 +
>  MAINTAINERS                                        |   10 ++++++
>  4 files changed, 47 insertions(+)
>  create mode 100644 Documentation/driver-api/vfio-pci-vendor-driver-
> acceptance.rst
> 
> diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-
> api/index.rst
> index c57c609ad2eb..da1372c8ec3d 100644
> --- a/Documentation/driver-api/index.rst
> +++ b/Documentation/driver-api/index.rst
> @@ -103,6 +103,7 @@ available subsections can be seen below.
>     sync_file
>     vfio-mediated-device
>     vfio
> +   vfio-pci-vendor-driver-acceptance
>     xilinx/index
>     xillybus
>     zorro
> diff --git a/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst
> b/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst
> new file mode 100644
> index 000000000000..3a108d748681
> --- /dev/null
> +++ b/Documentation/driver-api/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/Documentation/maintainer/maintainer-entry-profile.rst
> b/Documentation/maintainer/maintainer-entry-profile.rst
> index 5d5cc3acdf85..8b4971c7e3fa 100644
> --- a/Documentation/maintainer/maintainer-entry-profile.rst
> +++ b/Documentation/maintainer/maintainer-entry-profile.rst
> @@ -103,3 +103,4 @@ to do something different in the near future.
>     ../nvdimm/maintainer-entry-profile
>     ../riscv/patch-acceptance
>     ../driver-api/media/maintainer-entry-profile
> +   ../driver-api/vfio-pci-vendor-driver-acceptance
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4322b5321891..fd17d1891216 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/driver-api/vfio-pci-vendor-driver-acceptance.rst
> +F:	drivers/vfio/pci/*/
> +
>  VFIO PLATFORM DRIVER
>  M:	Eric Auger <[e-mail]>
>  L:	[e-mail]
>

Yishai Hadas March 15, 2022, 7:23 a.m. UTC | #2

On 3/14/2022 10:24 PM, Alex Williamson wrote:
> 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: Kevin Tian <[e-mail]>
> Acked-by: Shameer Kolothum <[e-mail]>
> Signed-off-by: Alex Williamson <[e-mail]>

Reviewed-by: Yishai Hadas <[e-mail]>

> ---
>
> v3:
>
> Relocate to Documentation/driver-api/
> Include index.rst reference
> Cross link from maintainer-entry-profile
> Add Shameer's Ack
>
> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220310134954.0df4bb12.alex.williamson%40redhat.com%2F&amp;data=04%7C01%7Cyishaih%40nvidia.com%7Cf97af54aedc341708d6f08da05f8b34b%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637828862983240522%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=aQQY6NWJ65p%2B6nyzMXu9VUpOFi1ZClrSW0fQZbLWuoE%3D&amp;reserved=0
>
>   Documentation/driver-api/index.rst                 |    1 +
>   .../vfio-pci-vendor-driver-acceptance.rst          |   35 ++++++++++++++++++++
>   .../maintainer/maintainer-entry-profile.rst        |    1 +
>   MAINTAINERS                                        |   10 ++++++
>   4 files changed, 47 insertions(+)
>   create mode 100644 Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst
>
> diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
> index c57c609ad2eb..da1372c8ec3d 100644
> --- a/Documentation/driver-api/index.rst
> +++ b/Documentation/driver-api/index.rst
> @@ -103,6 +103,7 @@ available subsections can be seen below.
>      sync_file
>      vfio-mediated-device
>      vfio
> +   vfio-pci-vendor-driver-acceptance
>      xilinx/index
>      xillybus
>      zorro
> diff --git a/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst b/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst
> new file mode 100644
> index 000000000000..3a108d748681
> --- /dev/null
> +++ b/Documentation/driver-api/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/Documentation/maintainer/maintainer-entry-profile.rst b/Documentation/maintainer/maintainer-entry-profile.rst
> index 5d5cc3acdf85..8b4971c7e3fa 100644
> --- a/Documentation/maintainer/maintainer-entry-profile.rst
> +++ b/Documentation/maintainer/maintainer-entry-profile.rst
> @@ -103,3 +103,4 @@ to do something different in the near future.
>      ../nvdimm/maintainer-entry-profile
>      ../riscv/patch-acceptance
>      ../driver-api/media/maintainer-entry-profile
> +   ../driver-api/vfio-pci-vendor-driver-acceptance
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4322b5321891..fd17d1891216 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/driver-api/vfio-pci-vendor-driver-acceptance.rst
> +F:	drivers/vfio/pci/*/
> +
>   VFIO PLATFORM DRIVER
>   M:	Eric Auger <[e-mail]>
>   L:	[e-mail]
>
>

Cornelia Huck March 15, 2022, 9:26 a.m. UTC | #3

On Mon, Mar 14 2022, Alex Williamson <[e-mail]> wrote:

> 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: Kevin Tian <[e-mail]>
> Acked-by: Shameer Kolothum <[e-mail]>
> Signed-off-by: Alex Williamson <[e-mail]>
> ---

(...)

> diff --git a/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst b/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst
> new file mode 100644
> index 000000000000..3a108d748681
> --- /dev/null
> +++ b/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst

What about Christoph's request to drop the "vendor" name?
vfio-pci-device-specific-driver-acceptance.rst would match the actual
title of the document, and the only drawback I see is that it is a bit
longer.

> @@ -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.

s/Sign-off/Reviewed-by/ ?

I would not generally expect the reviewers listed to sign off on other
people's patches.

> +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/Documentation/maintainer/maintainer-entry-profile.rst b/Documentation/maintainer/maintainer-entry-profile.rst
> index 5d5cc3acdf85..8b4971c7e3fa 100644
> --- a/Documentation/maintainer/maintainer-entry-profile.rst
> +++ b/Documentation/maintainer/maintainer-entry-profile.rst
> @@ -103,3 +103,4 @@ to do something different in the near future.
>     ../nvdimm/maintainer-entry-profile
>     ../riscv/patch-acceptance
>     ../driver-api/media/maintainer-entry-profile
> +   ../driver-api/vfio-pci-vendor-driver-acceptance
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4322b5321891..fd17d1891216 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

VFIO PCI DEVICE SPECIFIC 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/driver-api/vfio-pci-vendor-driver-acceptance.rst
> +F:	drivers/vfio/pci/*/
> +
>  VFIO PLATFORM DRIVER
>  M:	Eric Auger <[e-mail]>
>  L:	[e-mail]

Other than that, looks good to me (and thanks to the people volunteering
for review!)

Jason Gunthorpe March 15, 2022, 3:53 p.m. UTC | #4

On Tue, Mar 15, 2022 at 10:26:17AM +0100, Cornelia Huck wrote:
> On Mon, Mar 14 2022, Alex Williamson <[e-mail]> wrote:
> 
> > 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: Kevin Tian <[e-mail]>
> > Acked-by: Shameer Kolothum <[e-mail]>
> > Signed-off-by: Alex Williamson <[e-mail]>
> 
> (...)
> 
> > diff --git a/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst b/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst
> > new file mode 100644
> > index 000000000000..3a108d748681
> > +++ b/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst
> 
> What about Christoph's request to drop the "vendor" name?
> vfio-pci-device-specific-driver-acceptance.rst would match the actual
> title of the document, and the only drawback I see is that it is a bit
> longer.

I agree we should not use the vendor name

In general I wonder if this is a bit too specific to PCI, really this
is just review criteria for any driver making a struct vfio_device_ops
implementation, and we have some specific guidance for migration here
as well.

Like if IBM makes s390 migration drivers all of this applies just as
well even though they are not PCI.

> > +New driver submissions are therefore requested to have approval via
> > +Sign-off/Acked-by/etc for any interactions with parent drivers.
> 
> s/Sign-off/Reviewed-by/ ?
> 
> I would not generally expect the reviewers listed to sign off on other
> people's patches.

It happens quite a lot when those people help write the patches too :)

Jason

Alex Williamson March 15, 2022, 4:22 p.m. UTC | #5

On Tue, 15 Mar 2022 12:53:04 -0300
Jason Gunthorpe <[e-mail]> wrote:

> On Tue, Mar 15, 2022 at 10:26:17AM +0100, Cornelia Huck wrote:
> > On Mon, Mar 14 2022, Alex Williamson <[e-mail]> wrote:
> >   
> > > 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: Kevin Tian <[e-mail]>
> > > Acked-by: Shameer Kolothum <[e-mail]>
> > > Signed-off-by: Alex Williamson <[e-mail]>  
> > 
> > (...)
> >   
> > > diff --git a/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst b/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst
> > > new file mode 100644
> > > index 000000000000..3a108d748681
> > > +++ b/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst  
> > 
> > What about Christoph's request to drop the "vendor" name?
> > vfio-pci-device-specific-driver-acceptance.rst would match the actual
> > title of the document, and the only drawback I see is that it is a bit
> > longer.  
> 
> I agree we should not use the vendor name
> 
> In general I wonder if this is a bit too specific to PCI, really this
> is just review criteria for any driver making a struct vfio_device_ops
> implementation, and we have some specific guidance for migration here
> as well.
> 
> Like if IBM makes s390 migration drivers all of this applies just as
> well even though they are not PCI.

Are you volunteering to be a reviewer under drivers/vfio/?  Careful,
I'll add you ;)

What you're saying is true of course and it could be argued that this
sort of criteria is true for any new driver, I think the unique thing
here that raises it to a point where we want to formalize the breadth
of reviews is how significantly lower the bar is to create a device
specific driver now that we have a vfio-pci-core library.  Shameer's
stub driver is 100 LoC.  I also expect that the pool of people willing
to volunteer to be reviewers for PCI related device specific drivers is
large than we might see for arbitrary drivers.

> > > +New driver submissions are therefore requested to have approval via
> > > +Sign-off/Acked-by/etc for any interactions with parent drivers.  
> > 
> > s/Sign-off/Reviewed-by/ ?
> > 
> > I would not generally expect the reviewers listed to sign off on other
> > people's patches.  
> 
> It happens quite a lot when those people help write the patches too :)

This is what "etc" is for, the owners are involved and have endorsed it
in some way, that's all we care about.  Thanks,

Alex

Cornelia Huck March 15, 2022, 5:32 p.m. UTC | #6

On Tue, Mar 15 2022, Alex Williamson <[e-mail]> wrote:

> On Tue, 15 Mar 2022 12:53:04 -0300
> Jason Gunthorpe <[e-mail]> wrote:
>
>> On Tue, Mar 15, 2022 at 10:26:17AM +0100, Cornelia Huck wrote:
>> > On Mon, Mar 14 2022, Alex Williamson <[e-mail]> wrote:

>> In general I wonder if this is a bit too specific to PCI, really this
>> is just review criteria for any driver making a struct vfio_device_ops
>> implementation, and we have some specific guidance for migration here
>> as well.
>> 
>> Like if IBM makes s390 migration drivers all of this applies just as
>> well even though they are not PCI.
>
> Are you volunteering to be a reviewer under drivers/vfio/?  Careful,
> I'll add you ;)
>
> What you're saying is true of course and it could be argued that this
> sort of criteria is true for any new driver, I think the unique thing
> here that raises it to a point where we want to formalize the breadth
> of reviews is how significantly lower the bar is to create a device
> specific driver now that we have a vfio-pci-core library.  Shameer's
> stub driver is 100 LoC.  I also expect that the pool of people willing
> to volunteer to be reviewers for PCI related device specific drivers is
> large than we might see for arbitrary drivers.

Yes. Also, I expect that more people understand how a PCI driver works
than how an s390 channel subsystem driver works :)

I think we'll just have to hope that attempts to add e.g. migration
support to a driver outside of vfio-pci show up on the correct mailing
lists and that the right people notice it or can be pointed towards it.

>
>> > > +New driver submissions are therefore requested to have approval via
>> > > +Sign-off/Acked-by/etc for any interactions with parent drivers.  
>> > 
>> > s/Sign-off/Reviewed-by/ ?
>> > 
>> > I would not generally expect the reviewers listed to sign off on other
>> > people's patches.  
>> 
>> It happens quite a lot when those people help write the patches too :)
>
> This is what "etc" is for, the owners are involved and have endorsed it
> in some way, that's all we care about.

Fair enough.

Jason Gunthorpe March 17, 2022, 7:41 p.m. UTC | #7

On Tue, Mar 15, 2022 at 10:22:00AM -0600, Alex Williamson wrote:
> On Tue, 15 Mar 2022 12:53:04 -0300

> > I agree we should not use the vendor name
> > 
> > In general I wonder if this is a bit too specific to PCI, really this
> > is just review criteria for any driver making a struct vfio_device_ops
> > implementation, and we have some specific guidance for migration here
> > as well.
> > 
> > Like if IBM makes s390 migration drivers all of this applies just as
> > well even though they are not PCI.
> 
> Are you volunteering to be a reviewer under drivers/vfio/?  Careful,
> I'll add you ;)

Haha, sure you can do that if it helps

We still have a quite a ways to go before all the iommu features are
exposed and we get dirty tracking done.

Thanks,
Jason

Patch

diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
index c57c609ad2eb..da1372c8ec3d 100644
--- a/Documentation/driver-api/index.rst
+++ b/Documentation/driver-api/index.rst
@@ -103,6 +103,7 @@  available subsections can be seen below.
    sync_file
    vfio-mediated-device
    vfio
+   vfio-pci-vendor-driver-acceptance
    xilinx/index
    xillybus
    zorro
diff --git a/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst b/Documentation/driver-api/vfio-pci-vendor-driver-acceptance.rst
new file mode 100644
index 000000000000..3a108d748681
--- /dev/null
+++ b/Documentation/driver-api/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/Documentation/maintainer/maintainer-entry-profile.rst b/Documentation/maintainer/maintainer-entry-profile.rst
index 5d5cc3acdf85..8b4971c7e3fa 100644
--- a/Documentation/maintainer/maintainer-entry-profile.rst
+++ b/Documentation/maintainer/maintainer-entry-profile.rst
@@ -103,3 +103,4 @@  to do something different in the near future.
    ../nvdimm/maintainer-entry-profile
    ../riscv/patch-acceptance
    ../driver-api/media/maintainer-entry-profile
+   ../driver-api/vfio-pci-vendor-driver-acceptance
diff --git a/MAINTAINERS b/MAINTAINERS
index 4322b5321891..fd17d1891216 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/driver-api/vfio-pci-vendor-driver-acceptance.rst
+F:	drivers/vfio/pci/*/
+
 VFIO PLATFORM DRIVER
 M:	Eric Auger <[e-mail]>
 L:	[e-mail]