| Message ID | 1343093180-5598-1-git-send-email-nab@linux-iscsi.org |
|---|---|
| State | New |
| Headers | show |
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
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
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
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.
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
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
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
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.
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.
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.
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?
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.
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
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)