[Qemu-devel] [GIT PULL] tcm_vhost: Initial merge of vhost level target fabric driver

[Qemu-devel] [GIT PULL] tcm_vhost: Initial merge of vhost level target fabric driver

[Nicholas A. Bellinger]

Hi Linus,

Here is the PULL request for the initial merge of tcm_vhost based on
RFC-v5 code with MST's ACK appended to the initial merge commit.
As promised, the commit is available from two different branches for you
to consider merging as for-3.6 code.

The 'for-next-merge' branch based on mainline commit 7409a6657ae using
3.5-rc2 code contains two duplicates of pre-merge vhost patch
dependencies that have already been merged into mainline via net-next.
This commit is also in the 07302012 -next patchset, and available here:

  git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-next-merge

Or the 'for-linus' branch containing an -rc0 head @ commit bdc0077af57:

   Merge tag 'scsi-misc' of git://git.kernel.org/../jejb/scsi)

rebased up to the last commit in scsi-misc required for virtio-scsi
client LLD scanning logic to function properly with tcm_vhost fabric
ports, is available here:

  git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-linus

Both branches have gotten recent testing and have been running
over-night small block random I/O tests connected to raw block flash
backends.  The same diffstat below will result from pulling either
branch.

Also, the incremental patch to address MST's last round of post-merge
comments has been sent to the lists for feedback this afternoon.  This
will be included into the usual post -rc1 PULL via 3.6-rc-fixes, along
with any other bits that end up changing post-merge.

Please let us know if you have any concerns.

Thank you!

--nab

Nicholas Bellinger (1):
  tcm_vhost: Initial merge for vhost level target fabric driver

 drivers/vhost/Kconfig     |    3 +
 drivers/vhost/Kconfig.tcm |    6 +
 drivers/vhost/Makefile    |    2 +
 drivers/vhost/tcm_vhost.c | 1628 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/vhost/tcm_vhost.h |  101 +++
 5 files changed, 1740 insertions(+), 0 deletions(-)
 create mode 100644 drivers/vhost/Kconfig.tcm
 create mode 100644 drivers/vhost/tcm_vhost.c
 create mode 100644 drivers/vhost/tcm_vhost.h

Re: [Qemu-devel] [GIT PULL] tcm_vhost: Initial merge of vhost level target fabric driver

[Nicholas A. Bellinger]

Hi Linus,

Ping on the initial tcm_vhost merge for-3.6..?  I know it's been a
busier than usual merge window, but hopefully this one is still in your
PULL queue..

Otherwise if there is something else that you'd like to see different
from this PULL request, please let us know.

Thank you!

--nab

On Mon, 2012-07-30 at 18:19 -0700, Nicholas A. Bellinger wrote:
> Hi Linus,
> 
> Here is the PULL request for the initial merge of tcm_vhost based on
> RFC-v5 code with MST's ACK appended to the initial merge commit.
> As promised, the commit is available from two different branches for you
> to consider merging as for-3.6 code.
> 
> The 'for-next-merge' branch based on mainline commit 7409a6657ae using
> 3.5-rc2 code contains two duplicates of pre-merge vhost patch
> dependencies that have already been merged into mainline via net-next.
> This commit is also in the 07302012 -next patchset, and available here:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-next-merge
> 
> Or the 'for-linus' branch containing an -rc0 head @ commit bdc0077af57:
> 
>    Merge tag 'scsi-misc' of git://git.kernel.org/../jejb/scsi)
> 
> rebased up to the last commit in scsi-misc required for virtio-scsi
> client LLD scanning logic to function properly with tcm_vhost fabric
> ports, is available here:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/nab/target-pending.git for-linus
> 
> Both branches have gotten recent testing and have been running
> over-night small block random I/O tests connected to raw block flash
> backends.  The same diffstat below will result from pulling either
> branch.
> 
> Also, the incremental patch to address MST's last round of post-merge
> comments has been sent to the lists for feedback this afternoon.  This
> will be included into the usual post -rc1 PULL via 3.6-rc-fixes, along
> with any other bits that end up changing post-merge.
> 
> Please let us know if you have any concerns.
> 
> Thank you!
> 
> --nab
> 
> Nicholas Bellinger (1):
>   tcm_vhost: Initial merge for vhost level target fabric driver
> 
>  drivers/vhost/Kconfig     |    3 +
>  drivers/vhost/Kconfig.tcm |    6 +
>  drivers/vhost/Makefile    |    2 +
>  drivers/vhost/tcm_vhost.c | 1628 +++++++++++++++++++++++++++++++++++++++++++++
>  drivers/vhost/tcm_vhost.h |  101 +++
>  5 files changed, 1740 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/vhost/Kconfig.tcm
>  create mode 100644 drivers/vhost/tcm_vhost.c
>  create mode 100644 drivers/vhost/tcm_vhost.h
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [Qemu-devel] [GIT PULL] tcm_vhost: Initial merge of vhost level target fabric driver

[Michael S. Tsirkin]

Hi Nicholas,
I just noticed this problem in the interface:

+#include <linux/vhost.h>
+
+/*
+ * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
+ *
+ * ABI Rev 0: July 2012 version starting point for v3.6-rc merge
candidate +
+ *            RFC-v2 vhost-scsi userspace.  Add GET_ABI_VERSION ioctl
usage
+ */
+
+#define VHOST_SCSI_ABI_VERSION 0
+
+struct vhost_scsi_target {
+       int abi_version;
+       unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
+       unsigned short vhost_tpgt;
+};
+

Here TRANSPORT_IQN_LEN is 224, which is a multiple of 4.
Since vhost_tpgt is 2 bytes and abi_version is 4, the total size would
be 230.  But gcc needs struct size be aligned to first field size, which
is 4 bytes, so it pads the structure by extra 2 bytes to the total of
232.

This padding is very undesirable in an ABI:
- it can not be initialized easily
- it can not be checked easily
- it can leak information between kernel and userspace

Simplest solution is probably just to make the padding
explicit:

+struct vhost_scsi_target {
+       int abi_version;
+       unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
+       unsigned short vhost_tpgt;
+       unsigned short reserved;
+};
+

I think we should fix this buglet before it goes out to users.

-- 
MST

Re: [Qemu-devel] [GIT PULL] tcm_vhost: Initial merge of vhost level target fabric driver

[Nicholas A. Bellinger]

On Sat, 2012-08-18 at 23:04 +0300, Michael S. Tsirkin wrote:
> Hi Nicholas,
> I just noticed this problem in the interface:
> 
> +#include <linux/vhost.h>
> +
> +/*
> + * Used by QEMU userspace to ensure a consistent vhost-scsi ABI.
> + *
> + * ABI Rev 0: July 2012 version starting point for v3.6-rc merge
> candidate +
> + *            RFC-v2 vhost-scsi userspace.  Add GET_ABI_VERSION ioctl
> usage
> + */
> +
> +#define VHOST_SCSI_ABI_VERSION 0
> +
> +struct vhost_scsi_target {
> +       int abi_version;
> +       unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
> +       unsigned short vhost_tpgt;
> +};
> +
> 
> Here TRANSPORT_IQN_LEN is 224, which is a multiple of 4.
> Since vhost_tpgt is 2 bytes and abi_version is 4, the total size would
> be 230.  But gcc needs struct size be aligned to first field size, which
> is 4 bytes, so it pads the structure by extra 2 bytes to the total of
> 232.
> 
> This padding is very undesirable in an ABI:
> - it can not be initialized easily
> - it can not be checked easily
> - it can leak information between kernel and userspace
> 

Hmmmm, yes.  Very good reasons to avoid ABI ambiguity  ..

> Simplest solution is probably just to make the padding
> explicit:
> 
> +struct vhost_scsi_target {
> +       int abi_version;
> +       unsigned char vhost_wwpn[TRANSPORT_IQN_LEN];
> +       unsigned short vhost_tpgt;
> +       unsigned short reserved;
> +};
> +
> 
> I think we should fix this buglet before it goes out to users.
> 

<nod>, fixing this up in target-pending/master now w/ your reported-by
+signoff, and will change vhost-scsi's copy of these defs for next
week's RFC-v3 posting.

Thanks MST!

--nab