| Message ID | 20200821195616.13554-2-akrowiak@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390/vfio-ap: dynamic configuration support | expand |
On Fri, 21 Aug 2020 15:56:01 -0400 Tony Krowiak <akrowiak@linux.ibm.com> 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@linux.ibm.com> > --- > 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.
On 8/25/20 6:04 AM, Cornelia Huck wrote: > On Fri, 21 Aug 2020 15:56:01 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> 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@linux.ibm.com> >> --- >> 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. Can you provide more details about this generic interface of which you speak? >
On Wed, 26 Aug 2020 10:49:47 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > On 8/25/20 6:04 AM, Cornelia Huck wrote: > > On Fri, 21 Aug 2020 15:56:01 -0400 > > Tony Krowiak <akrowiak@linux.ibm.com> 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@linux.ibm.com> > >> --- > >> 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.
On 8/27/20 6:32 AM, Cornelia Huck wrote: > On Wed, 26 Aug 2020 10:49:47 -0400 > Tony Krowiak <akrowiak@linux.ibm.com> wrote: > >> On 8/25/20 6:04 AM, Cornelia Huck wrote: >>> On Fri, 21 Aug 2020 15:56:01 -0400 >>> Tony Krowiak <akrowiak@linux.ibm.com> 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@linux.ibm.com> >>>> --- >>>> 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. >
On Thu, 27 Aug 2020 10:39:07 -0400 Tony Krowiak <akrowiak@linux.ibm.com> wrote: > 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. Thanks for the explanation, that seems reasonable to me.
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;
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@linux.ibm.com> --- drivers/s390/crypto/vfio_ap_drv.c | 2 ++ 1 file changed, 2 insertions(+)