Patchwork tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

login
register
mail settings
Submitter Nicholas A. Bellinger
Date July 24, 2012, 1:26 a.m.
Message ID <1343093180-5598-1-git-send-email-nab@linux-iscsi.org>
Download mbox | patch
Permalink /patch/172769/
State New
Headers show

Comments

Nicholas A. Bellinger - July 24, 2012, 1:26 a.m.
From: Nicholas Bellinger <nab@linux-iscsi.org>

As requested by Anthony, here is a patch against target-pending/for-next-merge
to expose an ABI version to userspace via a new VHOST_SCSI_GET_ABI_VERSION
ioctl operation.

As mentioned in the comment, ABI Rev 0 is for pre 2012 out-of-tree code, and
ABI Rev 1 (the current rev) is for current WIP v3.6 kernel merge candiate code.

I think this is what you had in mind, and hopefully it will make MST happy too.
The incremental vhost-scsi patches against Zhi's QEMU are going out shortly ahead
of cutting a new vhost-scsi RFC over the next days.

Please have a look and let me know if you have any concerns here.

Thanks!

Reported-by: Anthony Liguori <aliguori@us.ibm.com>
Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/vhost/tcm_vhost.c |    9 +++++++++
 drivers/vhost/tcm_vhost.h |   11 +++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)
Greg KH - July 24, 2012, 1:56 a.m.
On Tue, Jul 24, 2012 at 01:26:20AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> As requested by Anthony, here is a patch against target-pending/for-next-merge
> to expose an ABI version to userspace via a new VHOST_SCSI_GET_ABI_VERSION
> ioctl operation.
> 
> As mentioned in the comment, ABI Rev 0 is for pre 2012 out-of-tree code, and
> ABI Rev 1 (the current rev) is for current WIP v3.6 kernel merge candiate code.
> 
> I think this is what you had in mind, and hopefully it will make MST happy too.
> The incremental vhost-scsi patches against Zhi's QEMU are going out shortly ahead
> of cutting a new vhost-scsi RFC over the next days.
> 
> Please have a look and let me know if you have any concerns here.
> 
> Thanks!
> 
> Reported-by: Anthony Liguori <aliguori@us.ibm.com>
> Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/vhost/tcm_vhost.c |    9 +++++++++
>  drivers/vhost/tcm_vhost.h |   11 +++++++++++
>  2 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> index dc7e024..3f04169 100644
> --- a/drivers/vhost/tcm_vhost.c
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -977,6 +977,15 @@ static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl,
>  			return -EFAULT;
>  
>  		return vhost_scsi_clear_endpoint(vs, &backend);
> +	case VHOST_SCSI_GET_ABI_VERSION:
> +		if (copy_from_user(&backend, argp, sizeof backend))
> +			return -EFAULT;
> +
> +		backend.abi_version = VHOST_SCSI_ABI_VERSION;
> +
> +		if (copy_to_user(argp, &backend, sizeof backend))
> +			return -EFAULT;
> +		return 0;
>  	case VHOST_GET_FEATURES:
>  		features = VHOST_FEATURES;
>  		if (copy_to_user(featurep, &features, sizeof features))
> diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> index e942df9..3d5378f 100644
> --- a/drivers/vhost/tcm_vhost.h
> +++ b/drivers/vhost/tcm_vhost.h
> @@ -80,7 +80,17 @@ struct tcm_vhost_tport {
>  
>  #include <linux/vhost.h>
>  
> +/*
> + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> + *
> + * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
> + * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
> + */
> +
> +#define VHOST_SCSI_ABI_VERSION	1
> +
>  struct vhost_scsi_target {
> +	int abi_version;
>  	unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
>  	unsigned short vhost_tpgt;
>  };
> @@ -88,3 +98,4 @@ struct vhost_scsi_target {
>  /* VHOST_SCSI specific defines */
>  #define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
>  #define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
> +#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct vhost_scsi_target)

No, you just broke the ABI for version "0" here, that's not how you do
this at all.

greg k-h
Nicholas A. Bellinger - July 24, 2012, 8:45 p.m.
On Mon, 2012-07-23 at 18:56 -0700, Greg Kroah-Hartman wrote:
> On Tue, Jul 24, 2012 at 01:26:20AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > As requested by Anthony, here is a patch against target-pending/for-next-merge
> > to expose an ABI version to userspace via a new VHOST_SCSI_GET_ABI_VERSION
> > ioctl operation.
> > 
> > As mentioned in the comment, ABI Rev 0 is for pre 2012 out-of-tree code, and
> > ABI Rev 1 (the current rev) is for current WIP v3.6 kernel merge candiate code.
> > 
> > I think this is what you had in mind, and hopefully it will make MST happy too.
> > The incremental vhost-scsi patches against Zhi's QEMU are going out shortly ahead
> > of cutting a new vhost-scsi RFC over the next days.
> > 
> > Please have a look and let me know if you have any concerns here.
> > 
> > Thanks!
> > 
> > Reported-by: Anthony Liguori <aliguori@us.ibm.com>
> > Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >  drivers/vhost/tcm_vhost.c |    9 +++++++++
> >  drivers/vhost/tcm_vhost.h |   11 +++++++++++
> >  2 files changed, 20 insertions(+), 0 deletions(-)
> > 

<SNIP>

> > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> > index e942df9..3d5378f 100644
> > --- a/drivers/vhost/tcm_vhost.h
> > +++ b/drivers/vhost/tcm_vhost.h
> > @@ -80,7 +80,17 @@ struct tcm_vhost_tport {
> >  
> >  #include <linux/vhost.h>
> >  
> > +/*
> > + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> > + *
> > + * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
> > + * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
> > + */
> > +
> > +#define VHOST_SCSI_ABI_VERSION	1
> > +
> >  struct vhost_scsi_target {
> > +	int abi_version;
> >  	unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
> >  	unsigned short vhost_tpgt;
> >  };
> > @@ -88,3 +98,4 @@ struct vhost_scsi_target {
> >  /* VHOST_SCSI specific defines */
> >  #define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
> >  #define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
> > +#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct vhost_scsi_target)
> 
> No, you just broke the ABI for version "0" here, that's not how you do
> this at all.
> 

The intention of this patch is use ABI=1 as a starting point for
tcm_vhost moving forward, with no back-wards compat for the ABI=0
prototype userspace code because:

- It's based on a slightly older version of QEMU (updating the QEMU series now)
- It does not have an GET_ABI_VERSION ioctl cmd (that starts with ABI=1)
- It has a small user-base of target + virtio-scsi developers

So I did consider just starting from ABI=0, but figured this would help
reduce the confusion for QEMU userspace wrt to the vhost-scsi code
that's been floating around out-of-tree for the last 2 years.

Stefan & Co folks, do you have a preference for a starting point..?

Thanks,

--nab
Stefan Hajnoczi - July 25, 2012, 11:55 a.m.
On Tue, Jul 24, 2012 at 01:45:24PM -0700, Nicholas A. Bellinger wrote:
> On Mon, 2012-07-23 at 18:56 -0700, Greg Kroah-Hartman wrote:
> > On Tue, Jul 24, 2012 at 01:26:20AM +0000, Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > 
> > > As requested by Anthony, here is a patch against target-pending/for-next-merge
> > > to expose an ABI version to userspace via a new VHOST_SCSI_GET_ABI_VERSION
> > > ioctl operation.
> > > 
> > > As mentioned in the comment, ABI Rev 0 is for pre 2012 out-of-tree code, and
> > > ABI Rev 1 (the current rev) is for current WIP v3.6 kernel merge candiate code.
> > > 
> > > I think this is what you had in mind, and hopefully it will make MST happy too.
> > > The incremental vhost-scsi patches against Zhi's QEMU are going out shortly ahead
> > > of cutting a new vhost-scsi RFC over the next days.
> > > 
> > > Please have a look and let me know if you have any concerns here.
> > > 
> > > Thanks!
> > > 
> > > Reported-by: Anthony Liguori <aliguori@us.ibm.com>
> > > Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com>
> > > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > > ---
> > >  drivers/vhost/tcm_vhost.c |    9 +++++++++
> > >  drivers/vhost/tcm_vhost.h |   11 +++++++++++
> > >  2 files changed, 20 insertions(+), 0 deletions(-)
> > > 
> 
> <SNIP>
> 
> > > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> > > index e942df9..3d5378f 100644
> > > --- a/drivers/vhost/tcm_vhost.h
> > > +++ b/drivers/vhost/tcm_vhost.h
> > > @@ -80,7 +80,17 @@ struct tcm_vhost_tport {
> > >  
> > >  #include <linux/vhost.h>
> > >  
> > > +/*
> > > + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> > > + *
> > > + * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
> > > + * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
> > > + */
> > > +
> > > +#define VHOST_SCSI_ABI_VERSION	1
> > > +
> > >  struct vhost_scsi_target {
> > > +	int abi_version;
> > >  	unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
> > >  	unsigned short vhost_tpgt;
> > >  };
> > > @@ -88,3 +98,4 @@ struct vhost_scsi_target {
> > >  /* VHOST_SCSI specific defines */
> > >  #define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
> > >  #define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
> > > +#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct vhost_scsi_target)
> > 
> > No, you just broke the ABI for version "0" here, that's not how you do
> > this at all.
> > 
> 
> The intention of this patch is use ABI=1 as a starting point for
> tcm_vhost moving forward, with no back-wards compat for the ABI=0
> prototype userspace code because:
> 
> - It's based on a slightly older version of QEMU (updating the QEMU series now)
> - It does not have an GET_ABI_VERSION ioctl cmd (that starts with ABI=1)
> - It has a small user-base of target + virtio-scsi developers
> 
> So I did consider just starting from ABI=0, but figured this would help
> reduce the confusion for QEMU userspace wrt to the vhost-scsi code
> that's been floating around out-of-tree for the last 2 years.

There is no real user base beyond the handful of people who have hacked
on this.  Adding the GET_ABI_VERSION ioctl() at this stage is fine,
especially considering that the userspace code that talks to tcm_vhost
isn't in mainline in userspace yet either.

Stefan
Avi Kivity - July 25, 2012, 1:10 p.m.
On 07/24/2012 11:45 PM, Nicholas A. Bellinger wrote:

>> > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
>> > index e942df9..3d5378f 100644
>> > --- a/drivers/vhost/tcm_vhost.h
>> > +++ b/drivers/vhost/tcm_vhost.h
>> > @@ -80,7 +80,17 @@ struct tcm_vhost_tport {
>> >  
>> >  #include <linux/vhost.h>
>> >  
>> > +/*
>> > + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
>> > + *
>> > + * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
>> > + * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
>> > + */


If it's out of tree, why consider it at all?  Put a stable ABI in tree
and extend it in compatible ways.
Nicholas A. Bellinger - July 25, 2012, 9:10 p.m.
On Wed, 2012-07-25 at 16:10 +0300, Avi Kivity wrote:
> On 07/24/2012 11:45 PM, Nicholas A. Bellinger wrote:
> 
> >> > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> >> > index e942df9..3d5378f 100644
> >> > --- a/drivers/vhost/tcm_vhost.h
> >> > +++ b/drivers/vhost/tcm_vhost.h
> >> > @@ -80,7 +80,17 @@ struct tcm_vhost_tport {
> >> >  
> >> >  #include <linux/vhost.h>
> >> >  
> >> > +/*
> >> > + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> >> > + *
> >> > + * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
> >> > + * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
> >> > + */
> 
> 
> If it's out of tree, why consider it at all?  Put a stable ABI in tree
> and extend it in compatible ways.
> 
> 

This comment was supposed to convey that ABI=0 vhost-scsi userspace code
is not supported with tcm_vhost mainline code.

But obviously that was not clear enough here.  Updating the comment to
reflect to make this clear.

So the main question here was if it's fine to start with ABI=1, and
require >= ABI=1 for all vhost-scsi userspace code to function with
tcm_vhost.

The idea was to avoid confusion for the ABI=0 vhost-scsi code that's
been floating around for the last 2 years.

--nab
Nicholas A. Bellinger - July 25, 2012, 9:14 p.m.
On Wed, 2012-07-25 at 12:55 +0100, Stefan Hajnoczi wrote:
> On Tue, Jul 24, 2012 at 01:45:24PM -0700, Nicholas A. Bellinger wrote:
> > On Mon, 2012-07-23 at 18:56 -0700, Greg Kroah-Hartman wrote:
> > > On Tue, Jul 24, 2012 at 01:26:20AM +0000, Nicholas A. Bellinger wrote:
> > > > From: Nicholas Bellinger <nab@linux-iscsi.org>

<SNIP>

> > 
> > > > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> > > > index e942df9..3d5378f 100644
> > > > --- a/drivers/vhost/tcm_vhost.h
> > > > +++ b/drivers/vhost/tcm_vhost.h
> > > > @@ -80,7 +80,17 @@ struct tcm_vhost_tport {
> > > >  
> > > >  #include <linux/vhost.h>
> > > >  
> > > > +/*
> > > > + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> > > > + *
> > > > + * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
> > > > + * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
> > > > + */
> > > > +
> > > > +#define VHOST_SCSI_ABI_VERSION	1
> > > > +
> > > >  struct vhost_scsi_target {
> > > > +	int abi_version;
> > > >  	unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
> > > >  	unsigned short vhost_tpgt;
> > > >  };
> > > > @@ -88,3 +98,4 @@ struct vhost_scsi_target {
> > > >  /* VHOST_SCSI specific defines */
> > > >  #define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
> > > >  #define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
> > > > +#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct vhost_scsi_target)
> > > 
> > > No, you just broke the ABI for version "0" here, that's not how you do
> > > this at all.
> > > 
> > 
> > The intention of this patch is use ABI=1 as a starting point for
> > tcm_vhost moving forward, with no back-wards compat for the ABI=0
> > prototype userspace code because:
> > 
> > - It's based on a slightly older version of QEMU (updating the QEMU series now)
> > - It does not have an GET_ABI_VERSION ioctl cmd (that starts with ABI=1)
> > - It has a small user-base of target + virtio-scsi developers
> > 
> > So I did consider just starting from ABI=0, but figured this would help
> > reduce the confusion for QEMU userspace wrt to the vhost-scsi code
> > that's been floating around out-of-tree for the last 2 years.
> 
> There is no real user base beyond the handful of people who have hacked
> on this.  Adding the GET_ABI_VERSION ioctl() at this stage is fine,
> especially considering that the userspace code that talks to tcm_vhost
> isn't in mainline in userspace yet either.


Do you have a preference for a VHOST_SCSI_ABI_VERSION starting point
here..?

I thought that v1 would be helpful to avoid confusion with the older
userspace code, but don't really have a strong opinion either way..

Let me know what you'd prefer here, and I'll make the changes to
tcm_vhost + vhost-scsi patch series accordingly.

Thanks!

--nab
Stefan Hajnoczi - July 25, 2012, 10:35 p.m.
On Wed, Jul 25, 2012 at 02:14:50PM -0700, Nicholas A. Bellinger wrote:
> On Wed, 2012-07-25 at 12:55 +0100, Stefan Hajnoczi wrote:
> > On Tue, Jul 24, 2012 at 01:45:24PM -0700, Nicholas A. Bellinger wrote:
> > > On Mon, 2012-07-23 at 18:56 -0700, Greg Kroah-Hartman wrote:
> > > > On Tue, Jul 24, 2012 at 01:26:20AM +0000, Nicholas A. Bellinger wrote:
> > > > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> <SNIP>
> 
> > > 
> > > > > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> > > > > index e942df9..3d5378f 100644
> > > > > --- a/drivers/vhost/tcm_vhost.h
> > > > > +++ b/drivers/vhost/tcm_vhost.h
> > > > > @@ -80,7 +80,17 @@ struct tcm_vhost_tport {
> > > > >  
> > > > >  #include <linux/vhost.h>
> > > > >  
> > > > > +/*
> > > > > + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> > > > > + *
> > > > > + * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
> > > > > + * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
> > > > > + */
> > > > > +
> > > > > +#define VHOST_SCSI_ABI_VERSION	1
> > > > > +
> > > > >  struct vhost_scsi_target {
> > > > > +	int abi_version;
> > > > >  	unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
> > > > >  	unsigned short vhost_tpgt;
> > > > >  };
> > > > > @@ -88,3 +98,4 @@ struct vhost_scsi_target {
> > > > >  /* VHOST_SCSI specific defines */
> > > > >  #define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
> > > > >  #define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
> > > > > +#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct vhost_scsi_target)
> > > > 
> > > > No, you just broke the ABI for version "0" here, that's not how you do
> > > > this at all.
> > > > 
> > > 
> > > The intention of this patch is use ABI=1 as a starting point for
> > > tcm_vhost moving forward, with no back-wards compat for the ABI=0
> > > prototype userspace code because:
> > > 
> > > - It's based on a slightly older version of QEMU (updating the QEMU series now)
> > > - It does not have an GET_ABI_VERSION ioctl cmd (that starts with ABI=1)
> > > - It has a small user-base of target + virtio-scsi developers
> > > 
> > > So I did consider just starting from ABI=0, but figured this would help
> > > reduce the confusion for QEMU userspace wrt to the vhost-scsi code
> > > that's been floating around out-of-tree for the last 2 years.
> > 
> > There is no real user base beyond the handful of people who have hacked
> > on this.  Adding the GET_ABI_VERSION ioctl() at this stage is fine,
> > especially considering that the userspace code that talks to tcm_vhost
> > isn't in mainline in userspace yet either.
> 
> 
> Do you have a preference for a VHOST_SCSI_ABI_VERSION starting point
> here..?
> 
> I thought that v1 would be helpful to avoid confusion with the older
> userspace code, but don't really have a strong opinion either way..
> 
> Let me know what you'd prefer here, and I'll make the changes to
> tcm_vhost + vhost-scsi patch series accordingly.

I don't think 0 for out-of-tree is needed.  I'd start at 0 but either
way is okay.

The main thing I would like to confirm is that this only versions the
tcm_vhost ioctls?  In that case a single version number works.

Stefan
Nicholas A. Bellinger - July 26, 2012, 2:34 a.m.
On Wed, 2012-07-25 at 23:35 +0100, Stefan Hajnoczi wrote:
> On Wed, Jul 25, 2012 at 02:14:50PM -0700, Nicholas A. Bellinger wrote:
> > On Wed, 2012-07-25 at 12:55 +0100, Stefan Hajnoczi wrote:
> > > On Tue, Jul 24, 2012 at 01:45:24PM -0700, Nicholas A. Bellinger wrote:
> > > > On Mon, 2012-07-23 at 18:56 -0700, Greg Kroah-Hartman wrote:
> > > > > On Tue, Jul 24, 2012 at 01:26:20AM +0000, Nicholas A. Bellinger wrote:
> > > > > > From: Nicholas Bellinger <nab@linux-iscsi.org>

<SNIP>

> > > > The intention of this patch is use ABI=1 as a starting point for
> > > > tcm_vhost moving forward, with no back-wards compat for the ABI=0
> > > > prototype userspace code because:
> > > > 
> > > > - It's based on a slightly older version of QEMU (updating the QEMU series now)
> > > > - It does not have an GET_ABI_VERSION ioctl cmd (that starts with ABI=1)
> > > > - It has a small user-base of target + virtio-scsi developers
> > > > 
> > > > So I did consider just starting from ABI=0, but figured this would help
> > > > reduce the confusion for QEMU userspace wrt to the vhost-scsi code
> > > > that's been floating around out-of-tree for the last 2 years.
> > > 
> > > There is no real user base beyond the handful of people who have hacked
> > > on this.  Adding the GET_ABI_VERSION ioctl() at this stage is fine,
> > > especially considering that the userspace code that talks to tcm_vhost
> > > isn't in mainline in userspace yet either.
> > 
> > 
> > Do you have a preference for a VHOST_SCSI_ABI_VERSION starting point
> > here..?
> > 
> > I thought that v1 would be helpful to avoid confusion with the older
> > userspace code, but don't really have a strong opinion either way..
> > 
> > Let me know what you'd prefer here, and I'll make the changes to
> > tcm_vhost + vhost-scsi patch series accordingly.
> 
> I don't think 0 for out-of-tree is needed.  I'd start at 0 but either
> way is okay.
> 

<nod>

In that case, respinning a -v5 for tcm_vhost to start from ABI=0 and
will post an updated patch shortly.

> The main thing I would like to confirm is that this only versions the
> tcm_vhost ioctls?  In that case a single version number works.
> 

Correct, the GET_ABI_VERSION call is only intended to identify the
changing of tcm_vhost ioctls.
Avi Kivity - July 26, 2012, 8:58 a.m.
On 07/26/2012 05:34 AM, Nicholas A. Bellinger wrote:
> 
> In that case, respinning a -v5 for tcm_vhost to start from ABI=0 and
> will post an updated patch shortly.
> 
>> The main thing I would like to confirm is that this only versions the
>> tcm_vhost ioctls?  In that case a single version number works.
>> 
> 
> Correct, the GET_ABI_VERSION call is only intended to identify the
> changing of tcm_vhost ioctls.

Why use version numbers at all?

Feature maps are better, because you can get very fine granularity.
Nicholas A. Bellinger - July 26, 2012, 8:15 p.m.
On Thu, 2012-07-26 at 11:58 +0300, Avi Kivity wrote:
> On 07/26/2012 05:34 AM, Nicholas A. Bellinger wrote:
> > 
> > In that case, respinning a -v5 for tcm_vhost to start from ABI=0 and
> > will post an updated patch shortly.
> > 
> >> The main thing I would like to confirm is that this only versions the
> >> tcm_vhost ioctls?  In that case a single version number works.
> >> 
> > 
> > Correct, the GET_ABI_VERSION call is only intended to identify the
> > changing of tcm_vhost ioctls.
> 
> Why use version numbers at all?
> 

This was a request by QEMU folks.. 


> Feature maps are better, because you can get very fine granularity.
> 
> 

Example..?  If there is a better way to handle ioctl compat I'd
certainly like to hear about it.
Michael S. Tsirkin - July 29, 2012, 12:27 p.m.
On Wed, Jul 25, 2012 at 02:10:48PM -0700, Nicholas A. Bellinger wrote:
> On Wed, 2012-07-25 at 16:10 +0300, Avi Kivity wrote:
> > On 07/24/2012 11:45 PM, Nicholas A. Bellinger wrote:
> > 
> > >> > diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> > >> > index e942df9..3d5378f 100644
> > >> > --- a/drivers/vhost/tcm_vhost.h
> > >> > +++ b/drivers/vhost/tcm_vhost.h
> > >> > @@ -80,7 +80,17 @@ struct tcm_vhost_tport {
> > >> >  
> > >> >  #include <linux/vhost.h>
> > >> >  
> > >> > +/*
> > >> > + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> > >> > + *
> > >> > + * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
> > >> > + * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
> > >> > + */
> > 
> > 
> > If it's out of tree, why consider it at all?  Put a stable ABI in tree
> > and extend it in compatible ways.
> > 
> > 
> 
> This comment was supposed to convey that ABI=0 vhost-scsi userspace code
> is not supported with tcm_vhost mainline code.
> 
> But obviously that was not clear enough here.  Updating the comment to
> reflect to make this clear.
> 
> So the main question here was if it's fine to start with ABI=1, and
> require >= ABI=1 for all vhost-scsi userspace code to function with
> tcm_vhost.
> 
> The idea was to avoid confusion for the ABI=0 vhost-scsi code that's
> been floating around for the last 2 years.
> 
> --nab
> 
> 
> 
> 
> 
> 

Yes but I am guessing that code does not invoke the ABI ioctl?
Michael S. Tsirkin - July 29, 2012, 12:35 p.m.
On Thu, Jul 26, 2012 at 01:15:26PM -0700, Nicholas A. Bellinger wrote:
>  If there is a better way to handle ioctl compat I'd
> certainly like to hear about it.


You basically don't touch exiting commands, always add new ones.
For example if you want to simply stop some ioctl from working
for old userspace, the simplest way is to change ioctl number.
Avi Kivity - July 29, 2012, 12:50 p.m.
On 07/26/2012 11:15 PM, Nicholas A. Bellinger wrote:
>> 
> 
> Example..?  If there is a better way to handle ioctl compat I'd
> certainly like to hear about it.
> 

r = ioctl(fd, KVM_CHECK_EXTENSION, KVM_CAP_ASSIGN_DEV_IRQ);
if (r == -1)
    ...


if (r)
    // ioctl(fd, KVM_ASSIGN_DEV_IRQ, ...) is available
else
   not available

Patch

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index dc7e024..3f04169 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -977,6 +977,15 @@  static long vhost_scsi_ioctl(struct file *f, unsigned int ioctl,
 			return -EFAULT;
 
 		return vhost_scsi_clear_endpoint(vs, &backend);
+	case VHOST_SCSI_GET_ABI_VERSION:
+		if (copy_from_user(&backend, argp, sizeof backend))
+			return -EFAULT;
+
+		backend.abi_version = VHOST_SCSI_ABI_VERSION;
+
+		if (copy_to_user(argp, &backend, sizeof backend))
+			return -EFAULT;
+		return 0;
 	case VHOST_GET_FEATURES:
 		features = VHOST_FEATURES;
 		if (copy_to_user(featurep, &features, sizeof features))
diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
index e942df9..3d5378f 100644
--- a/drivers/vhost/tcm_vhost.h
+++ b/drivers/vhost/tcm_vhost.h
@@ -80,7 +80,17 @@  struct tcm_vhost_tport {
 
 #include <linux/vhost.h>
 
+/*
+ * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
+ *
+ * ABI Rev 0: All pre 2012 revisions used by prototype out-of-tree code
+ * ABI Rev 1: 2012 version for v3.6 kernel merge candiate
+ */
+
+#define VHOST_SCSI_ABI_VERSION	1
+
 struct vhost_scsi_target {
+	int abi_version;
 	unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
 	unsigned short vhost_tpgt;
 };
@@ -88,3 +98,4 @@  struct vhost_scsi_target {
 /* VHOST_SCSI specific defines */
 #define VHOST_SCSI_SET_ENDPOINT _IOW(VHOST_VIRTIO, 0x40, struct vhost_scsi_target)
 #define VHOST_SCSI_CLEAR_ENDPOINT _IOW(VHOST_VIRTIO, 0x41, struct vhost_scsi_target)
+#define VHOST_SCSI_GET_ABI_VERSION _IOW(VHOST_VIRTIO, 0x42, struct vhost_scsi_target)