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
Signed-off-by: Fam Zheng <famz at 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
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 at 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
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
Christoph Hellwig
2017-Jan-16 17:34 UTC
[PATCH 0/2] virtio-scsi: Implement FC_HOST feature
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?
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 hostper 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 at 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)
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 24765 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20170117/8d86ef7a/attachment-0001.bin>
> 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
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 18632 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20170117/7f998e62/attachment-0001.bin>
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 at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html