[Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Nicholas A. Bellinger]

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)
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Greg Kroah-Hartman]

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

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Nicholas A. Bellinger]

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

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Nicholas A. Bellinger]

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

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Stefan Hajnoczi]

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

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Nicholas A. Bellinger]

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.

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Avi Kivity]

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.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Nicholas A. Bellinger]

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.

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Michael S. Tsirkin]

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.


-- 
MST

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Avi Kivity]

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


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Avi Kivity]

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.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Nicholas A. Bellinger]

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

Re: [Qemu-devel] [PATCH] tcm_vhost: Expose ABI version via VHOST_SCSI_GET_ABI_VERSION

[Michael S. Tsirkin]

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?

-- 
MST