[PATCH 0/2] virtio-scsi: Implement FC_HOST feature

[PATCH 0/2] virtio-scsi: Implement FC_HOST feature

[Fam Zheng]

This series implements the proposed fc_host feature of virtio-scsi.

The first patch updates the data structure changes according to the spec
proposal; the second patch actually implements the operations.

Fam Zheng (2):
  virtio_scsi: Add fc_host definitions
  virtio_scsi: Implement fc_host

 drivers/scsi/virtio_scsi.c       | 55 +++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/virtio_scsi.h |  6 +++++
 2 files changed, 60 insertions(+), 1 deletion(-)

-- 
2.9.3

[PATCH 1/2] virtio_scsi: Add fc_host definitions

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/uapi/linux/virtio_scsi.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/uapi/linux/virtio_scsi.h b/include/uapi/linux/virtio_scsi.h
index cc18ef8..a26fb31 100644
--- a/include/uapi/linux/virtio_scsi.h
+++ b/include/uapi/linux/virtio_scsi.h
@@ -113,6 +113,11 @@ struct virtio_scsi_config {
 	__u16 max_channel;
 	__u16 max_target;
 	__u32 max_lun;
+	__u8  primary_wwpn[8];
+	__u8  primary_wwnn[8];
+	__u8  secondary_wwpn[8];
+	__u8  secondary_wwnn[8];
+	__u8  primary_active;
 } __attribute__((packed));
 
 /* Feature Bits */
@@ -120,6 +125,7 @@ struct virtio_scsi_config {
 #define VIRTIO_SCSI_F_HOTPLUG                  1
 #define VIRTIO_SCSI_F_CHANGE                   2
 #define VIRTIO_SCSI_F_T10_PI                   3
+#define VIRTIO_SCSI_F_FC_HOST                  4
 
 /* Response codes */
 #define VIRTIO_SCSI_S_OK                       0
-- 
2.9.3

[PATCH 1/2] virtio_scsi: Add fc_host definitions

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/uapi/linux/virtio_scsi.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/uapi/linux/virtio_scsi.h b/include/uapi/linux/virtio_scsi.h
index cc18ef8..a26fb31 100644
--- a/include/uapi/linux/virtio_scsi.h
+++ b/include/uapi/linux/virtio_scsi.h
@@ -113,6 +113,11 @@ struct virtio_scsi_config {
 	__u16 max_channel;
 	__u16 max_target;
 	__u32 max_lun;
+	__u8  primary_wwpn[8];
+	__u8  primary_wwnn[8];
+	__u8  secondary_wwpn[8];
+	__u8  secondary_wwnn[8];
+	__u8  primary_active;
 } __attribute__((packed));
 
 /* Feature Bits */
@@ -120,6 +125,7 @@ struct virtio_scsi_config {
 #define VIRTIO_SCSI_F_HOTPLUG                  1
 #define VIRTIO_SCSI_F_CHANGE                   2
 #define VIRTIO_SCSI_F_T10_PI                   3
+#define VIRTIO_SCSI_F_FC_HOST                  4
 
 /* Response codes */
 #define VIRTIO_SCSI_S_OK                       0
-- 
2.9.3

[PATCH 2/2] virtio_scsi: Implement fc_host

This implements the VIRTIO_SCSI_F_FC_HOST feature by reading the config
fields and presenting them as sysfs fc_host attributes. The config
change handler is added here because primary_active will toggle during
migration.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 drivers/scsi/virtio_scsi.c | 55 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index ec91bd0..9e92db9 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -28,6 +28,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_tcq.h>
+#include <scsi/scsi_transport_fc.h>
 #include <linux/seqlock.h>
 
 #define VIRTIO_SCSI_MEMPOOL_SZ 64
@@ -795,6 +796,15 @@ static struct scsi_host_template virtscsi_host_template_multi = {
 	.track_queue_depth = 1,
 };
 
+static struct fc_function_template virtscsi_fc_template = {
+	.show_host_node_name = 1,
+	.show_host_port_name = 1,
+	.show_host_port_type = 1,
+	.show_host_port_state = 1,
+};
+
+static struct scsi_transport_template *virtscsi_fc_transport_template;
+
 #define virtscsi_config_get(vdev, fld) \
 	({ \
 		typeof(((struct virtio_scsi_config *)0)->fld) __val; \
@@ -956,15 +966,38 @@ static int virtscsi_init(struct virtio_device *vdev,
 	return err;
 }
 
+static void virtscsi_update_fc_host_attrs(struct virtio_device *vdev)
+{
+	struct Scsi_Host *shost = vdev->priv;
+	u64 node_name, port_name;
+
+	if (virtscsi_config_get(vdev, primary_active)) {
+		node_name = virtio_cread64(vdev,
+			offsetof(struct virtio_scsi_config, primary_wwnn));
+		port_name = virtio_cread64(vdev,
+			offsetof(struct virtio_scsi_config, primary_wwpn));
+	} else {
+		node_name = virtio_cread64(vdev,
+			offsetof(struct virtio_scsi_config, secondary_wwnn));
+		port_name = virtio_cread64(vdev,
+			offsetof(struct virtio_scsi_config, secondary_wwpn));
+	}
+	fc_host_node_name(shost) = node_name;
+	fc_host_port_name(shost) = port_name;
+	fc_host_port_type(shost) = FC_PORTTYPE_NPORT;
+	fc_host_port_state(shost) = FC_PORTSTATE_ONLINE;
+}
+
 static int virtscsi_probe(struct virtio_device *vdev)
 {
-	struct Scsi_Host *shost;
+	struct Scsi_Host *shost = NULL;
 	struct virtio_scsi *vscsi;
 	int err;
 	u32 sg_elems, num_targets;
 	u32 cmd_per_lun;
 	u32 num_queues;
 	struct scsi_host_template *hostt;
+	bool fc_host_enabled;
 
 	if (!vdev->config->get) {
 		dev_err(&vdev->dev, "%s failure: config access disabled\n",
@@ -987,6 +1020,9 @@ static int virtscsi_probe(struct virtio_device *vdev)
 	if (!shost)
 		return -ENOMEM;
 
+	fc_host_enabled = virtio_has_feature(vdev, VIRTIO_SCSI_F_FC_HOST);
+	if (fc_host_enabled)
+		shost->transportt = virtscsi_fc_transport_template;
 	sg_elems = virtscsi_config_get(vdev, seg_max) ?: 1;
 	shost->sg_tablesize = sg_elems;
 	vscsi = shost_priv(shost);
@@ -1032,6 +1068,9 @@ static int virtscsi_probe(struct virtio_device *vdev)
 	if (err)
 		goto scsi_add_host_failed;
 
+	if (fc_host_enabled)
+		virtscsi_update_fc_host_attrs(vdev);
+
 	virtio_device_ready(vdev);
 
 	if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
@@ -1098,6 +1137,11 @@ static int virtscsi_restore(struct virtio_device *vdev)
 }
 #endif
 
+static void virtscsi_config_changed(struct virtio_device *vdev)
+{
+	virtscsi_update_fc_host_attrs(vdev);
+}
+
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_SCSI, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -1109,6 +1153,7 @@ static unsigned int features[] = {
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 	VIRTIO_SCSI_F_T10_PI,
 #endif
+	VIRTIO_SCSI_F_FC_HOST,
 };
 
 static struct virtio_driver virtio_scsi_driver = {
@@ -1123,12 +1168,19 @@ static struct virtio_driver virtio_scsi_driver = {
 	.restore = virtscsi_restore,
 #endif
 	.remove = virtscsi_remove,
+	.config_changed = virtscsi_config_changed,
 };
 
 static int __init init(void)
 {
 	int ret = -ENOMEM;
 
+	virtscsi_fc_transport_template = fc_attach_transport(&virtscsi_fc_template);
+	if (!virtscsi_fc_transport_template) {
+		pr_err("fc_attach_transport() failed\n");
+		goto error;
+	}
+
 	virtscsi_cmd_cache = KMEM_CACHE(virtio_scsi_cmd, 0);
 	if (!virtscsi_cmd_cache) {
 		pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
@@ -1176,6 +1228,7 @@ static int __init init(void)
 
 static void __exit fini(void)
 {
+	fc_release_transport(virtscsi_fc_transport_template);
 	unregister_virtio_driver(&virtio_scsi_driver);
 	cpuhp_remove_multi_state(virtioscsi_online);
 	cpuhp_remove_multi_state(CPUHP_VIRT_SCSI_DEAD);
-- 
2.9.3

[PATCH 2/2] virtio_scsi: Implement fc_host

[Fam Zheng]

This implements the VIRTIO_SCSI_F_FC_HOST feature by reading the config
fields and presenting them as sysfs fc_host attributes. The config
change handler is added here because primary_active will toggle during
migration.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 drivers/scsi/virtio_scsi.c | 55 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
index ec91bd0..9e92db9 100644
--- a/drivers/scsi/virtio_scsi.c
+++ b/drivers/scsi/virtio_scsi.c
@@ -28,6 +28,7 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_tcq.h>
+#include <scsi/scsi_transport_fc.h>
 #include <linux/seqlock.h>
 
 #define VIRTIO_SCSI_MEMPOOL_SZ 64
@@ -795,6 +796,15 @@ static struct scsi_host_template virtscsi_host_template_multi = {
 	.track_queue_depth = 1,
 };
 
+static struct fc_function_template virtscsi_fc_template = {
+	.show_host_node_name = 1,
+	.show_host_port_name = 1,
+	.show_host_port_type = 1,
+	.show_host_port_state = 1,
+};
+
+static struct scsi_transport_template *virtscsi_fc_transport_template;
+
 #define virtscsi_config_get(vdev, fld) \
 	({ \
 		typeof(((struct virtio_scsi_config *)0)->fld) __val; \
@@ -956,15 +966,38 @@ static int virtscsi_init(struct virtio_device *vdev,
 	return err;
 }
 
+static void virtscsi_update_fc_host_attrs(struct virtio_device *vdev)
+{
+	struct Scsi_Host *shost = vdev->priv;
+	u64 node_name, port_name;
+
+	if (virtscsi_config_get(vdev, primary_active)) {
+		node_name = virtio_cread64(vdev,
+			offsetof(struct virtio_scsi_config, primary_wwnn));
+		port_name = virtio_cread64(vdev,
+			offsetof(struct virtio_scsi_config, primary_wwpn));
+	} else {
+		node_name = virtio_cread64(vdev,
+			offsetof(struct virtio_scsi_config, secondary_wwnn));
+		port_name = virtio_cread64(vdev,
+			offsetof(struct virtio_scsi_config, secondary_wwpn));
+	}
+	fc_host_node_name(shost) = node_name;
+	fc_host_port_name(shost) = port_name;
+	fc_host_port_type(shost) = FC_PORTTYPE_NPORT;
+	fc_host_port_state(shost) = FC_PORTSTATE_ONLINE;
+}
+
 static int virtscsi_probe(struct virtio_device *vdev)
 {
-	struct Scsi_Host *shost;
+	struct Scsi_Host *shost = NULL;
 	struct virtio_scsi *vscsi;
 	int err;
 	u32 sg_elems, num_targets;
 	u32 cmd_per_lun;
 	u32 num_queues;
 	struct scsi_host_template *hostt;
+	bool fc_host_enabled;
 
 	if (!vdev->config->get) {
 		dev_err(&vdev->dev, "%s failure: config access disabled\n",
@@ -987,6 +1020,9 @@ static int virtscsi_probe(struct virtio_device *vdev)
 	if (!shost)
 		return -ENOMEM;
 
+	fc_host_enabled = virtio_has_feature(vdev, VIRTIO_SCSI_F_FC_HOST);
+	if (fc_host_enabled)
+		shost->transportt = virtscsi_fc_transport_template;
 	sg_elems = virtscsi_config_get(vdev, seg_max) ?: 1;
 	shost->sg_tablesize = sg_elems;
 	vscsi = shost_priv(shost);
@@ -1032,6 +1068,9 @@ static int virtscsi_probe(struct virtio_device *vdev)
 	if (err)
 		goto scsi_add_host_failed;
 
+	if (fc_host_enabled)
+		virtscsi_update_fc_host_attrs(vdev);
+
 	virtio_device_ready(vdev);
 
 	if (virtio_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG))
@@ -1098,6 +1137,11 @@ static int virtscsi_restore(struct virtio_device *vdev)
 }
 #endif
 
+static void virtscsi_config_changed(struct virtio_device *vdev)
+{
+	virtscsi_update_fc_host_attrs(vdev);
+}
+
 static struct virtio_device_id id_table[] = {
 	{ VIRTIO_ID_SCSI, VIRTIO_DEV_ANY_ID },
 	{ 0 },
@@ -1109,6 +1153,7 @@ static unsigned int features[] = {
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 	VIRTIO_SCSI_F_T10_PI,
 #endif
+	VIRTIO_SCSI_F_FC_HOST,
 };
 
 static struct virtio_driver virtio_scsi_driver = {
@@ -1123,12 +1168,19 @@ static struct virtio_driver virtio_scsi_driver = {
 	.restore = virtscsi_restore,
 #endif
 	.remove = virtscsi_remove,
+	.config_changed = virtscsi_config_changed,
 };
 
 static int __init init(void)
 {
 	int ret = -ENOMEM;
 
+	virtscsi_fc_transport_template = fc_attach_transport(&virtscsi_fc_template);
+	if (!virtscsi_fc_transport_template) {
+		pr_err("fc_attach_transport() failed\n");
+		goto error;
+	}
+
 	virtscsi_cmd_cache = KMEM_CACHE(virtio_scsi_cmd, 0);
 	if (!virtscsi_cmd_cache) {
 		pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
@@ -1176,6 +1228,7 @@ static int __init init(void)
 
 static void __exit fini(void)
 {
+	fc_release_transport(virtscsi_fc_transport_template);
 	unregister_virtio_driver(&virtio_scsi_driver);
 	cpuhp_remove_multi_state(virtioscsi_online);
 	cpuhp_remove_multi_state(CPUHP_VIRT_SCSI_DEAD);
-- 
2.9.3

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Paolo Bonzini]

On 16/01/2017 17:04, Fam Zheng wrote:
> +		node_name = virtio_cread64(vdev,
> +			offsetof(struct virtio_scsi_config, primary_wwnn));
> +		port_name = virtio_cread64(vdev,
> +			offsetof(struct virtio_scsi_config, primary_wwpn));
> +	} else {
> +		node_name = virtio_cread64(vdev,
> +			offsetof(struct virtio_scsi_config, secondary_wwnn));
> +		port_name = virtio_cread64(vdev,
> +			offsetof(struct virtio_scsi_config, secondary_wwpn));

Is the endianness correct for big-endian host here?

Thanks,

Paolo

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Paolo Bonzini]

On 16/01/2017 17:04, Fam Zheng wrote:
> +		node_name = virtio_cread64(vdev,
> +			offsetof(struct virtio_scsi_config, primary_wwnn));
> +		port_name = virtio_cread64(vdev,
> +			offsetof(struct virtio_scsi_config, primary_wwpn));
> +	} else {
> +		node_name = virtio_cread64(vdev,
> +			offsetof(struct virtio_scsi_config, secondary_wwnn));
> +		port_name = virtio_cread64(vdev,
> +			offsetof(struct virtio_scsi_config, secondary_wwpn));

Is the endianness correct for big-endian host here?

Thanks,

Paolo

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Fam Zheng]

On Mon, 01/16 17:45, Paolo Bonzini wrote:
> 
> 
> On 16/01/2017 17:04, Fam Zheng wrote:
> > +		node_name = virtio_cread64(vdev,
> > +			offsetof(struct virtio_scsi_config, primary_wwnn));
> > +		port_name = virtio_cread64(vdev,
> > +			offsetof(struct virtio_scsi_config, primary_wwpn));
> > +	} else {
> > +		node_name = virtio_cread64(vdev,
> > +			offsetof(struct virtio_scsi_config, secondary_wwnn));
> > +		port_name = virtio_cread64(vdev,
> > +			offsetof(struct virtio_scsi_config, secondary_wwpn));
> 
> Is the endianness correct for big-endian host here?

I think so. The fc_host sysfs uses u64 to represent port_name and node_name,
this patch does the same, so using virtio_* helpers for these fields should
handle the endianness correctly.

Maybe we should use u64 in struct virtio_scsi_config as well?

Fam

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Fam Zheng]

On Mon, 01/16 17:45, Paolo Bonzini wrote:
> 
> 
> On 16/01/2017 17:04, Fam Zheng wrote:
> > +		node_name = virtio_cread64(vdev,
> > +			offsetof(struct virtio_scsi_config, primary_wwnn));
> > +		port_name = virtio_cread64(vdev,
> > +			offsetof(struct virtio_scsi_config, primary_wwpn));
> > +	} else {
> > +		node_name = virtio_cread64(vdev,
> > +			offsetof(struct virtio_scsi_config, secondary_wwnn));
> > +		port_name = virtio_cread64(vdev,
> > +			offsetof(struct virtio_scsi_config, secondary_wwpn));
> 
> Is the endianness correct for big-endian host here?

I think so. The fc_host sysfs uses u64 to represent port_name and node_name,
this patch does the same, so using virtio_* helpers for these fields should
handle the endianness correctly.

Maybe we should use u64 in struct virtio_scsi_config as well?

Fam

Re: [PATCH 0/2] virtio-scsi: Implement FC_HOST feature

[Christoph Hellwig]

On Tue, Jan 17, 2017 at 12:04:28AM +0800, Fam Zheng wrote:
> This series implements the proposed fc_host feature of virtio-scsi.

Yikes.  Why?

Re: [PATCH 0/2] virtio-scsi: Implement FC_HOST feature

[Christoph Hellwig]

On Tue, Jan 17, 2017 at 12:04:28AM +0800, Fam Zheng wrote:
> This series implements the proposed fc_host feature of virtio-scsi.

Yikes.  Why?

Re: [PATCH 0/2] virtio-scsi: Implement FC_HOST feature

[Hannes Reinecke]

On 01/16/2017 05:04 PM, Fam Zheng wrote:
> This series implements the proposed fc_host feature of virtio-scsi.
> 
> The first patch updates the data structure changes according to the spec
> proposal; the second patch actually implements the operations.
> 
> Fam Zheng (2):
>   virtio_scsi: Add fc_host definitions
>   virtio_scsi: Implement fc_host
> 
>  drivers/scsi/virtio_scsi.c       | 55 +++++++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/virtio_scsi.h |  6 +++++
>  2 files changed, 60 insertions(+), 1 deletion(-)
> 
Hmm.

How it this supposed to work?
You do export the WWPN/WWNN of the associated host to the guest (nb:
will get interesting for non NPIV setups ...), but virtio scsi will
still do a LUN remapping.
IE the LUNs you see on the host will be different from the LUNs
presented to the guest.

This makes reliable operations very tricky.
Plus you don't _actually_ expose the FC host, but rather the WWPN of the
host presenting the LUN.
So how do you handle LUNs from different FC hosts on the guest?
>From what I've seen virtio will either presenting you with with one host
per LUN (so you'll end up with different SCSI hosts on the guest each
having the _same_ WWPN/WWNN), or a single host presenting all LUNs,
making the WWPN setting ... interesting.

Overall, I'm not overly happy with this approach.
You already added WWPN ids to the virtio transport, so why didn't you
update the LUN field, too, to avoid this ominous LUN remapping?
And we really should make sure to have a single FC host in the guest
presenting all LUNs.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH 0/2] virtio-scsi: Implement FC_HOST feature

[Hannes Reinecke]

On 01/16/2017 05:04 PM, Fam Zheng wrote:
> This series implements the proposed fc_host feature of virtio-scsi.
> 
> The first patch updates the data structure changes according to the spec
> proposal; the second patch actually implements the operations.
> 
> Fam Zheng (2):
>   virtio_scsi: Add fc_host definitions
>   virtio_scsi: Implement fc_host
> 
>  drivers/scsi/virtio_scsi.c       | 55 +++++++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/virtio_scsi.h |  6 +++++
>  2 files changed, 60 insertions(+), 1 deletion(-)
> 
Hmm.

How it this supposed to work?
You do export the WWPN/WWNN of the associated host to the guest (nb:
will get interesting for non NPIV setups ...), but virtio scsi will
still do a LUN remapping.
IE the LUNs you see on the host will be different from the LUNs
presented to the guest.

This makes reliable operations very tricky.
Plus you don't _actually_ expose the FC host, but rather the WWPN of the
host presenting the LUN.
So how do you handle LUNs from different FC hosts on the guest?
>From what I've seen virtio will either presenting you with with one host
per LUN (so you'll end up with different SCSI hosts on the guest each
having the _same_ WWPN/WWNN), or a single host presenting all LUNs,
making the WWPN setting ... interesting.

Overall, I'm not overly happy with this approach.
You already added WWPN ids to the virtio transport, so why didn't you
update the LUN field, too, to avoid this ominous LUN remapping?
And we really should make sure to have a single FC host in the guest
presenting all LUNs.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH 0/2] virtio-scsi: Implement FC_HOST feature

[Hannes Reinecke]

On 01/16/2017 05:04 PM, Fam Zheng wrote:
> This series implements the proposed fc_host feature of virtio-scsi.
> 
> The first patch updates the data structure changes according to the spec
> proposal; the second patch actually implements the operations.
> 
> Fam Zheng (2):
>   virtio_scsi: Add fc_host definitions
>   virtio_scsi: Implement fc_host
> 
>  drivers/scsi/virtio_scsi.c       | 55 +++++++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/virtio_scsi.h |  6 +++++
>  2 files changed, 60 insertions(+), 1 deletion(-)
> 
Hmm.

How it this supposed to work?
You do export the WWPN/WWNN of the associated host to the guest (nb:
will get interesting for non NPIV setups ...), but virtio scsi will
still do a LUN remapping.
IE the LUNs you see on the host will be different from the LUNs
presented to the guest.

This makes reliable operations very tricky.
Plus you don't _actually_ expose the FC host, but rather the WWPN of the
host presenting the LUN.
So how do you handle LUNs from different FC hosts on the guest?
From what I've seen virtio will either presenting you with with one host
per LUN (so you'll end up with different SCSI hosts on the guest each
having the _same_ WWPN/WWNN), or a single host presenting all LUNs,
making the WWPN setting ... interesting.

Overall, I'm not overly happy with this approach.
You already added WWPN ids to the virtio transport, so why didn't you
update the LUN field, too, to avoid this ominous LUN remapping?
And we really should make sure to have a single FC host in the guest
presenting all LUNs.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 963 bytes --]

Hi Fam,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc4 next-20170116]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Fam-Zheng/virtio-scsi-Implement-FC_HOST-feature/20170117-011950
config: i386-randconfig-r0-201703 (attached as .config)
compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `init':
>> virtio_scsi.c:(.init.text+0x10d52): undefined reference to `fc_attach_transport'
   drivers/built-in.o: In function `fini':
>> virtio_scsi.c:(.exit.text+0x14a4): undefined reference to `fc_release_transport'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24765 bytes --]

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 963 bytes --]

Hi Fam,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc4 next-20170116]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Fam-Zheng/virtio-scsi-Implement-FC_HOST-feature/20170117-011950
config: i386-randconfig-r0-201703 (attached as .config)
compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `init':
>> virtio_scsi.c:(.init.text+0x10d52): undefined reference to `fc_attach_transport'
   drivers/built-in.o: In function `fini':
>> virtio_scsi.c:(.exit.text+0x14a4): undefined reference to `fc_release_transport'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24765 bytes --]

[-- Attachment #3: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/2] virtio-scsi: Implement FC_HOST feature

[Paolo Bonzini]

> How it this supposed to work?
> You do export the WWPN/WWNN of the associated host to the guest (nb:
> will get interesting for non NPIV setups ...), but virtio scsi will
> still do a LUN remapping.
> IE the LUNs you see on the host will be different from the LUNs
> presented to the guest.

This is taken care of in the host by presenting to the host all LUNs from
a host's NPIV vHBA.  (Libvirt probably would be the one taking care of this,
because QEMU may not have enough permissions).

> Plus you don't _actually_ expose the FC host, but rather the WWPN of the
> host presenting the LUN.
> So how do you handle LUNs from different FC hosts on the guest?

I'm not sure I understand.

Neither I nor Fam know this stuff very well, but we are trying to do the same
as Hyper-V (and other proprietary hypervisors too).

> Overall, I'm not overly happy with this approach.
> You already added WWPN ids to the virtio transport, so why didn't you
> update the LUN field, too, to avoid this ominous LUN remapping?

Is this your old idea of adding a separate target field to commands,
in order to support 64-bit LUNs?  That is separate, and most FC drivers
only default to 16-bit LUNs anyway.

> And we really should make sure to have a single FC host in the guest
> presenting all LUNs.

Yes, of course.

Paolo

Re: [PATCH 0/2] virtio-scsi: Implement FC_HOST feature

[Paolo Bonzini]

> How it this supposed to work?
> You do export the WWPN/WWNN of the associated host to the guest (nb:
> will get interesting for non NPIV setups ...), but virtio scsi will
> still do a LUN remapping.
> IE the LUNs you see on the host will be different from the LUNs
> presented to the guest.

This is taken care of in the host by presenting to the host all LUNs from
a host's NPIV vHBA.  (Libvirt probably would be the one taking care of this,
because QEMU may not have enough permissions).

> Plus you don't _actually_ expose the FC host, but rather the WWPN of the
> host presenting the LUN.
> So how do you handle LUNs from different FC hosts on the guest?

I'm not sure I understand.

Neither I nor Fam know this stuff very well, but we are trying to do the same
as Hyper-V (and other proprietary hypervisors too).

> Overall, I'm not overly happy with this approach.
> You already added WWPN ids to the virtio transport, so why didn't you
> update the LUN field, too, to avoid this ominous LUN remapping?

Is this your old idea of adding a separate target field to commands,
in order to support 64-bit LUNs?  That is separate, and most FC drivers
only default to 16-bit LUNs anyway.

> And we really should make sure to have a single FC host in the guest
> presenting all LUNs.

Yes, of course.

Paolo

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3187 bytes --]

Hi Fam,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc4 next-20170116]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Fam-Zheng/virtio-scsi-Implement-FC_HOST-feature/20170117-011950
config: arm-vexpress_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `init':
>> drivers/scsi/virtio_scsi.c:1178: undefined reference to `fc_attach_transport'
   drivers/built-in.o: In function `fini':
>> drivers/scsi/virtio_scsi.c:1231: undefined reference to `fc_release_transport'

vim +1178 drivers/scsi/virtio_scsi.c

  1172	};
  1173	
  1174	static int __init init(void)
  1175	{
  1176		int ret = -ENOMEM;
  1177	
> 1178		virtscsi_fc_transport_template = fc_attach_transport(&virtscsi_fc_template);
  1179		if (!virtscsi_fc_transport_template) {
  1180			pr_err("fc_attach_transport() failed\n");
  1181			goto error;
  1182		}
  1183	
  1184		virtscsi_cmd_cache = KMEM_CACHE(virtio_scsi_cmd, 0);
  1185		if (!virtscsi_cmd_cache) {
  1186			pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
  1187			goto error;
  1188		}
  1189	
  1190	
  1191		virtscsi_cmd_pool =
  1192			mempool_create_slab_pool(VIRTIO_SCSI_MEMPOOL_SZ,
  1193						 virtscsi_cmd_cache);
  1194		if (!virtscsi_cmd_pool) {
  1195			pr_err("mempool_create() for virtscsi_cmd_pool failed\n");
  1196			goto error;
  1197		}
  1198		ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
  1199					      "scsi/virtio:online",
  1200					      virtscsi_cpu_online, NULL);
  1201		if (ret < 0)
  1202			goto error;
  1203		virtioscsi_online = ret;
  1204		ret = cpuhp_setup_state_multi(CPUHP_VIRT_SCSI_DEAD, "scsi/virtio:dead",
  1205					      NULL, virtscsi_cpu_online);
  1206		if (ret)
  1207			goto error;
  1208		ret = register_virtio_driver(&virtio_scsi_driver);
  1209		if (ret < 0)
  1210			goto error;
  1211	
  1212		return 0;
  1213	
  1214	error:
  1215		if (virtscsi_cmd_pool) {
  1216			mempool_destroy(virtscsi_cmd_pool);
  1217			virtscsi_cmd_pool = NULL;
  1218		}
  1219		if (virtscsi_cmd_cache) {
  1220			kmem_cache_destroy(virtscsi_cmd_cache);
  1221			virtscsi_cmd_cache = NULL;
  1222		}
  1223		if (virtioscsi_online)
  1224			cpuhp_remove_multi_state(virtioscsi_online);
  1225		cpuhp_remove_multi_state(CPUHP_VIRT_SCSI_DEAD);
  1226		return ret;
  1227	}
  1228	
  1229	static void __exit fini(void)
  1230	{
> 1231		fc_release_transport(virtscsi_fc_transport_template);
  1232		unregister_virtio_driver(&virtio_scsi_driver);
  1233		cpuhp_remove_multi_state(virtioscsi_online);
  1234		cpuhp_remove_multi_state(CPUHP_VIRT_SCSI_DEAD);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 18632 bytes --]

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3187 bytes --]

Hi Fam,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc4 next-20170116]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Fam-Zheng/virtio-scsi-Implement-FC_HOST-feature/20170117-011950
config: arm-vexpress_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `init':
>> drivers/scsi/virtio_scsi.c:1178: undefined reference to `fc_attach_transport'
   drivers/built-in.o: In function `fini':
>> drivers/scsi/virtio_scsi.c:1231: undefined reference to `fc_release_transport'

vim +1178 drivers/scsi/virtio_scsi.c

  1172	};
  1173	
  1174	static int __init init(void)
  1175	{
  1176		int ret = -ENOMEM;
  1177	
> 1178		virtscsi_fc_transport_template = fc_attach_transport(&virtscsi_fc_template);
  1179		if (!virtscsi_fc_transport_template) {
  1180			pr_err("fc_attach_transport() failed\n");
  1181			goto error;
  1182		}
  1183	
  1184		virtscsi_cmd_cache = KMEM_CACHE(virtio_scsi_cmd, 0);
  1185		if (!virtscsi_cmd_cache) {
  1186			pr_err("kmem_cache_create() for virtscsi_cmd_cache failed\n");
  1187			goto error;
  1188		}
  1189	
  1190	
  1191		virtscsi_cmd_pool =
  1192			mempool_create_slab_pool(VIRTIO_SCSI_MEMPOOL_SZ,
  1193						 virtscsi_cmd_cache);
  1194		if (!virtscsi_cmd_pool) {
  1195			pr_err("mempool_create() for virtscsi_cmd_pool failed\n");
  1196			goto error;
  1197		}
  1198		ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
  1199					      "scsi/virtio:online",
  1200					      virtscsi_cpu_online, NULL);
  1201		if (ret < 0)
  1202			goto error;
  1203		virtioscsi_online = ret;
  1204		ret = cpuhp_setup_state_multi(CPUHP_VIRT_SCSI_DEAD, "scsi/virtio:dead",
  1205					      NULL, virtscsi_cpu_online);
  1206		if (ret)
  1207			goto error;
  1208		ret = register_virtio_driver(&virtio_scsi_driver);
  1209		if (ret < 0)
  1210			goto error;
  1211	
  1212		return 0;
  1213	
  1214	error:
  1215		if (virtscsi_cmd_pool) {
  1216			mempool_destroy(virtscsi_cmd_pool);
  1217			virtscsi_cmd_pool = NULL;
  1218		}
  1219		if (virtscsi_cmd_cache) {
  1220			kmem_cache_destroy(virtscsi_cmd_cache);
  1221			virtscsi_cmd_cache = NULL;
  1222		}
  1223		if (virtioscsi_online)
  1224			cpuhp_remove_multi_state(virtioscsi_online);
  1225		cpuhp_remove_multi_state(CPUHP_VIRT_SCSI_DEAD);
  1226		return ret;
  1227	}
  1228	
  1229	static void __exit fini(void)
  1230	{
> 1231		fc_release_transport(virtscsi_fc_transport_template);
  1232		unregister_virtio_driver(&virtio_scsi_driver);
  1233		cpuhp_remove_multi_state(virtioscsi_online);
  1234		cpuhp_remove_multi_state(CPUHP_VIRT_SCSI_DEAD);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 18632 bytes --]

[-- Attachment #3: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/2] virtio-scsi: Implement FC_HOST feature

[Fam Zheng]

On Mon, 01/16 09:34, Christoph Hellwig wrote:
> On Tue, Jan 17, 2017 at 12:04:28AM +0800, Fam Zheng wrote:
> > This series implements the proposed fc_host feature of virtio-scsi.
> 
> Yikes.  Why?

Hi Christoph, this is mostly to "passthrough" a host NPIV vport (leaving libvirt
doing the heavy lifting to watch and attach all visible LUNs to guest).

Fam

> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] virtio-scsi: Implement FC_HOST feature

[Fam Zheng]

On Mon, 01/16 09:34, Christoph Hellwig wrote:
> On Tue, Jan 17, 2017 at 12:04:28AM +0800, Fam Zheng wrote:
> > This series implements the proposed fc_host feature of virtio-scsi.
> 
> Yikes.  Why?

Hi Christoph, this is mostly to "passthrough" a host NPIV vport (leaving libvirt
doing the heavy lifting to watch and attach all visible LUNs to guest).

Fam

> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Fam Zheng]

On Tue, 01/17 03:02, kbuild test robot wrote:
> Hi Fam,
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.10-rc4 next-20170116]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Fam-Zheng/virtio-scsi-Implement-FC_HOST-feature/20170117-011950
> config: i386-randconfig-r0-201703 (attached as .config)
> compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/built-in.o: In function `init':
> >> virtio_scsi.c:(.init.text+0x10d52): undefined reference to `fc_attach_transport'
>    drivers/built-in.o: In function `fini':
> >> virtio_scsi.c:(.exit.text+0x14a4): undefined reference to `fc_release_transport'

This is good catch.

Looks like virtio-scsi will now have to depend on fc, or we wrap the fc code
with #ifdef's.

Fam

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Fam Zheng]

On Tue, 01/17 03:02, kbuild test robot wrote:
> Hi Fam,
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.10-rc4 next-20170116]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Fam-Zheng/virtio-scsi-Implement-FC_HOST-feature/20170117-011950
> config: i386-randconfig-r0-201703 (attached as .config)
> compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>    drivers/built-in.o: In function `init':
> >> virtio_scsi.c:(.init.text+0x10d52): undefined reference to `fc_attach_transport'
>    drivers/built-in.o: In function `fini':
> >> virtio_scsi.c:(.exit.text+0x14a4): undefined reference to `fc_release_transport'

This is good catch.

Looks like virtio-scsi will now have to depend on fc, or we wrap the fc code
with #ifdef's.

Fam

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Paolo Bonzini]

On 16/01/2017 18:26, Fam Zheng wrote:
>> Is the endianness correct for big-endian host here?
>
> I think so. The fc_host sysfs uses u64 to represent port_name and node_name,
> this patch does the same, so using virtio_* helpers for these fields should
> handle the endianness correctly.

I was suspicious about it because they are defined as "u8 x[8]" in the
virtio_scsi_config struct.  So you would need to read with
virtio_cread_bytes and pass the result to wwn_to_u64.

For example, if you have 0x500123456789abcd this would be

	0x50 0x01 0x23 0x45 0x67 0x89 0xab 0cd

in virtio_scsi_config, and then virtio_cread64 would read it as a
little-endian u64, 0xcdab896745230150.  Maybe your QEMU patch is also
writing things as little-endian 64-bit integers, rather than 8-element
arrays of bytes?

Paolo

> Maybe we should use u64 in struct virtio_scsi_config as well?

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Paolo Bonzini]

On 16/01/2017 18:26, Fam Zheng wrote:
>> Is the endianness correct for big-endian host here?
>
> I think so. The fc_host sysfs uses u64 to represent port_name and node_name,
> this patch does the same, so using virtio_* helpers for these fields should
> handle the endianness correctly.

I was suspicious about it because they are defined as "u8 x[8]" in the
virtio_scsi_config struct.  So you would need to read with
virtio_cread_bytes and pass the result to wwn_to_u64.

For example, if you have 0x500123456789abcd this would be

	0x50 0x01 0x23 0x45 0x67 0x89 0xab 0cd

in virtio_scsi_config, and then virtio_cread64 would read it as a
little-endian u64, 0xcdab896745230150.  Maybe your QEMU patch is also
writing things as little-endian 64-bit integers, rather than 8-element
arrays of bytes?

Paolo

> Maybe we should use u64 in struct virtio_scsi_config as well?

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Fam Zheng]

On Tue, 01/17 14:17, Paolo Bonzini wrote:
> 
> 
> On 16/01/2017 18:26, Fam Zheng wrote:
> >> Is the endianness correct for big-endian host here?
> >
> > I think so. The fc_host sysfs uses u64 to represent port_name and node_name,
> > this patch does the same, so using virtio_* helpers for these fields should
> > handle the endianness correctly.
> 
> I was suspicious about it because they are defined as "u8 x[8]" in the
> virtio_scsi_config struct.  So you would need to read with
> virtio_cread_bytes and pass the result to wwn_to_u64.
> 
> For example, if you have 0x500123456789abcd this would be
> 
> 	0x50 0x01 0x23 0x45 0x67 0x89 0xab 0cd
> 
> in virtio_scsi_config, and then virtio_cread64 would read it as a
> little-endian u64, 0xcdab896745230150.  Maybe your QEMU patch is also
> writing things as little-endian 64-bit integers, rather than 8-element
> arrays of bytes?

Yes, they all used 64-bit integers in a "less surprising" endian. I think there
is an endianness conecpt to WWN, as in 0x500123456789abcd; and there is an
native endianness to virtio, which is little-endian. If we use a "u8 x[8]" type
in the spec and want the WWN's MSB, namely the 0x50 stuff, to be the first byte,
is it worth to explicitly document that to avoid confusion?

Fam

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Fam Zheng]

On Tue, 01/17 14:17, Paolo Bonzini wrote:
> 
> 
> On 16/01/2017 18:26, Fam Zheng wrote:
> >> Is the endianness correct for big-endian host here?
> >
> > I think so. The fc_host sysfs uses u64 to represent port_name and node_name,
> > this patch does the same, so using virtio_* helpers for these fields should
> > handle the endianness correctly.
> 
> I was suspicious about it because they are defined as "u8 x[8]" in the
> virtio_scsi_config struct.  So you would need to read with
> virtio_cread_bytes and pass the result to wwn_to_u64.
> 
> For example, if you have 0x500123456789abcd this would be
> 
> 	0x50 0x01 0x23 0x45 0x67 0x89 0xab 0cd
> 
> in virtio_scsi_config, and then virtio_cread64 would read it as a
> little-endian u64, 0xcdab896745230150.  Maybe your QEMU patch is also
> writing things as little-endian 64-bit integers, rather than 8-element
> arrays of bytes?

Yes, they all used 64-bit integers in a "less surprising" endian. I think there
is an endianness conecpt to WWN, as in 0x500123456789abcd; and there is an
native endianness to virtio, which is little-endian. If we use a "u8 x[8]" type
in the spec and want the WWN's MSB, namely the 0x50 stuff, to be the first byte,
is it worth to explicitly document that to avoid confusion?

Fam

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Michael S. Tsirkin]

On Tue, Jan 17, 2017 at 10:05:00PM +0800, Fam Zheng wrote:
> On Tue, 01/17 14:17, Paolo Bonzini wrote:
> > 
> > 
> > On 16/01/2017 18:26, Fam Zheng wrote:
> > >> Is the endianness correct for big-endian host here?
> > >
> > > I think so. The fc_host sysfs uses u64 to represent port_name and node_name,
> > > this patch does the same, so using virtio_* helpers for these fields should
> > > handle the endianness correctly.
> > 
> > I was suspicious about it because they are defined as "u8 x[8]" in the
> > virtio_scsi_config struct.  So you would need to read with
> > virtio_cread_bytes and pass the result to wwn_to_u64.
> > 
> > For example, if you have 0x500123456789abcd this would be
> > 
> > 	0x50 0x01 0x23 0x45 0x67 0x89 0xab 0cd
> > 
> > in virtio_scsi_config, and then virtio_cread64 would read it as a
> > little-endian u64, 0xcdab896745230150.  Maybe your QEMU patch is also
> > writing things as little-endian 64-bit integers, rather than 8-element
> > arrays of bytes?
> 
> Yes, they all used 64-bit integers in a "less surprising" endian. I think there
> is an endianness conecpt to WWN, as in 0x500123456789abcd; and there is an
> native endianness to virtio, which is little-endian. If we use a "u8 x[8]" type
> in the spec and want the WWN's MSB, namely the 0x50 stuff, to be the first byte,
> is it worth to explicitly document that to avoid confusion?
> 
> Fam

Can't hurt, for sure.

Re: [PATCH 2/2] virtio_scsi: Implement fc_host

[Michael S. Tsirkin]

On Tue, Jan 17, 2017 at 10:05:00PM +0800, Fam Zheng wrote:
> On Tue, 01/17 14:17, Paolo Bonzini wrote:
> > 
> > 
> > On 16/01/2017 18:26, Fam Zheng wrote:
> > >> Is the endianness correct for big-endian host here?
> > >
> > > I think so. The fc_host sysfs uses u64 to represent port_name and node_name,
> > > this patch does the same, so using virtio_* helpers for these fields should
> > > handle the endianness correctly.
> > 
> > I was suspicious about it because they are defined as "u8 x[8]" in the
> > virtio_scsi_config struct.  So you would need to read with
> > virtio_cread_bytes and pass the result to wwn_to_u64.
> > 
> > For example, if you have 0x500123456789abcd this would be
> > 
> > 	0x50 0x01 0x23 0x45 0x67 0x89 0xab 0cd
> > 
> > in virtio_scsi_config, and then virtio_cread64 would read it as a
> > little-endian u64, 0xcdab896745230150.  Maybe your QEMU patch is also
> > writing things as little-endian 64-bit integers, rather than 8-element
> > arrays of bytes?
> 
> Yes, they all used 64-bit integers in a "less surprising" endian. I think there
> is an endianness conecpt to WWN, as in 0x500123456789abcd; and there is an
> native endianness to virtio, which is little-endian. If we use a "u8 x[8]" type
> in the spec and want the WWN's MSB, namely the 0x50 stuff, to be the first byte,
> is it worth to explicitly document that to avoid confusion?
> 
> Fam

Can't hurt, for sure.