Re: [PATCH v10 01/16] s390/vfio-ap: add version vfio_ap module

[Tony Krowiak]

On 8/27/20 6:32 AM, Cornelia Huck wrote:
> On Wed, 26 Aug 2020 10:49:47 -0400
> Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
>
> > On 8/25/20 6:04 AM, Cornelia Huck wrote:
> > > On Fri, 21 Aug 2020 15:56:01 -0400
> > > Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
> > >   
> > > > Let's set a version for the vfio_ap module so that automated regression
> > > > tests can determine whether dynamic configuration tests can be run or
> > > > not.
> > > >
> > > > Signed-off-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
> > > > ---
> > > >    drivers/s390/crypto/vfio_ap_drv.c | 2 ++
> > > >    1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> > > > index be2520cc010b..f4ceb380dd61 100644
> > > > --- a/drivers/s390/crypto/vfio_ap_drv.c
> > > > +++ b/drivers/s390/crypto/vfio_ap_drv.c
> > > > @@ -17,10 +17,12 @@
> > > >    
> > > >    #define VFIO_AP_ROOT_NAME "vfio_ap"
> > > >    #define VFIO_AP_DEV_NAME "matrix"
> > > > +#define VFIO_AP_MODULE_VERSION "1.2.0"
> > > >    
> > > >    MODULE_AUTHOR("IBM Corporation");
> > > >    MODULE_DESCRIPTION("VFIO AP device driver, Copyright IBM Corp. 2018");
> > > >    MODULE_LICENSE("GPL v2");
> > > > +MODULE_VERSION(VFIO_AP_MODULE_VERSION);
> > > >    
> > > >    static struct ap_driver vfio_ap_drv;
> > > >      
> > > Setting a version manually has some drawbacks:
> > > - tools wanting to check for capabilities need to keep track which
> > >     versions support which features
> > > - you need to remember to actually bump the version when adding a new,
> > >     visible feature
> > > (- selective downstream backports may get into a pickle, but that's
> > > arguably not your problem)
> > >
> > > Is there no way for a tool to figure out whether this is supported?
> > > E.g., via existence of a sysfs file, or via a known error that will
> > > occur. If not, it's maybe better to expose known capabilities via a
> > > generic interface.
> > This patch series introduces a new mediated device sysfs attribute,
> > guest_matrix, so the automated tests could check for the existence
> > of that interface. The problem I have with that is it will work for
> > this version of the vfio_ap device driver - which may be all that is
> > ever needed - but does not account for future enhancements
> > which may need to be detected by tooling or automated tests.
> > It seems to me that regardless of how a tool detects whether
> > a feature is supported or not, it will have to keep track of that
> > somehow.
> Which enhancements? If you change the interface in an incompatible way,
> you have a different problem anyway. If someone trying to use the
> enhanced version of the interface gets an error on a kernel providing
> an older version of the interface, that's a reasonable way to discover
> support.
>
> I think "discover device driver capabilities by probing" is less
> burdensome and error prone than trying to match up capabilities with a
> version number. If you expose a version number, a tool would still have
> to probe that version number, and then consult with a list of features
> per version, which can easily go out of sync.
>
> > Can you provide more details about this generic interface of
> > which you speak?
> If that is really needed, I'd probably do a driver sysfs attribute that
> exposes a list of documented capabilities (as integer values, or as a
> bit.) But since tools can simply check for guest_matrix to find out
> about support for this feature here, it seems like overkill to me --
> unless you have a multitude of features waiting in queue that need to
> be made discoverable.

Currently there are two tools that probably need to be aware of
the changes to these assignment interfaces:
* The hades test framework has tests that will fail if run against
   these patches that should be skipped if over-provisioning is
   allowed. There are also tests under development to test the
   function introduced by these patches that will fail if run against
   an older version of the driver. These tests should be skipped in
   that case.
* There is a tool under development for configuring AP matrix
   mediated devices that probably need to be aware of the change
   introduced by this series.

Since a tool would have to first determine whether a new sysfs
interface documenting facilities is available and it would only
expose one facility at this point, it seems reasonable for these tools
to check for the sysfs guest_matrix attribute to discern whether
over-provisioning is available or not. I'll go ahead and remove this
patch from the series.

>