From: "Michael S. Tsirkin" <mst@redhat.com> To: Fam Zheng <famz@redhat.com> Cc: linux-kernel@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>, linux-scsi@vger.kernel.org, "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>, Jason Wang <jasowang@redhat.com>, "Martin K. Petersen" <martin.petersen@oracle.com>, stefanha@redhat.com, virtualization@lists.linux-foundation.org Subject: Re: [PATCH v2 2/2] virtio_scsi: Implement fc_host Date: Fri, 27 Jan 2017 00:06:27 +0200 [thread overview] Message-ID: <20170126234257-mutt-send-email-mst@kernel.org> (raw) In-Reply-To: <20170126034109.16144-3-famz@redhat.com> On Thu, Jan 26, 2017 at 11:41:09AM +0800, Fam Zheng wrote: > 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. Looks like there's active discussion on virtio tc mailing list. It's ok to post patches meanwhile but best as RFC, and repost after controversy is resolved. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > drivers/scsi/virtio_scsi.c | 60 +++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 59 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c > index ec91bd0..1bb330c 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,42 @@ 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; > + u8 node_name[8], port_name[8]; > + > + if (virtscsi_config_get(vdev, primary_active)) { > + virtio_cread_bytes(vdev, > + offsetof(struct virtio_scsi_config, primary_wwnn), > + &node_name, 8); > + virtio_cread_bytes(vdev, > + offsetof(struct virtio_scsi_config, primary_wwpn), > + &port_name, 8); > + } else { > + virtio_cread_bytes(vdev, > + offsetof(struct virtio_scsi_config, secondary_wwnn), > + &node_name, 8); > + virtio_cread_bytes(vdev, > + offsetof(struct virtio_scsi_config, secondary_wwpn), > + &port_name, 8); > + } This is racy, isn't it? You need to wrap this in a generation check otherwise read can race with primary_active changing. And you might want a wrapper to virtio_cread_bytes that does not include generation check. > + fc_host_node_name(shost) = wwn_to_u64(node_name); > + fc_host_port_name(shost) = wwn_to_u64(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 +1024,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 +1072,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 +1141,11 @@ static int virtscsi_restore(struct virtio_device *vdev) > } > #endif > > +static void virtscsi_config_changed(struct virtio_device *vdev) > +{ This is called unconditionally here, will access invalid config fields if feature is off. In fact this is wasting an MSIX vector when feature is not negotiated. No easy way not to, but best document this in a code comment. > + virtscsi_update_fc_host_attrs(vdev); > +} > + > static struct virtio_device_id id_table[] = { > { VIRTIO_ID_SCSI, VIRTIO_DEV_ANY_ID }, > { 0 }, > @@ -1109,6 +1157,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 +1172,20 @@ 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 +1233,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
WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com> To: Fam Zheng <famz@redhat.com> Cc: "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>, "Martin K. Petersen" <martin.petersen@oracle.com>, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, stefanha@redhat.com, Paolo Bonzini <pbonzini@redhat.com> Subject: Re: [PATCH v2 2/2] virtio_scsi: Implement fc_host Date: Fri, 27 Jan 2017 00:06:27 +0200 [thread overview] Message-ID: <20170126234257-mutt-send-email-mst@kernel.org> (raw) In-Reply-To: <20170126034109.16144-3-famz@redhat.com> On Thu, Jan 26, 2017 at 11:41:09AM +0800, Fam Zheng wrote: > 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. Looks like there's active discussion on virtio tc mailing list. It's ok to post patches meanwhile but best as RFC, and repost after controversy is resolved. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > drivers/scsi/virtio_scsi.c | 60 +++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 59 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c > index ec91bd0..1bb330c 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,42 @@ 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; > + u8 node_name[8], port_name[8]; > + > + if (virtscsi_config_get(vdev, primary_active)) { > + virtio_cread_bytes(vdev, > + offsetof(struct virtio_scsi_config, primary_wwnn), > + &node_name, 8); > + virtio_cread_bytes(vdev, > + offsetof(struct virtio_scsi_config, primary_wwpn), > + &port_name, 8); > + } else { > + virtio_cread_bytes(vdev, > + offsetof(struct virtio_scsi_config, secondary_wwnn), > + &node_name, 8); > + virtio_cread_bytes(vdev, > + offsetof(struct virtio_scsi_config, secondary_wwpn), > + &port_name, 8); > + } This is racy, isn't it? You need to wrap this in a generation check otherwise read can race with primary_active changing. And you might want a wrapper to virtio_cread_bytes that does not include generation check. > + fc_host_node_name(shost) = wwn_to_u64(node_name); > + fc_host_port_name(shost) = wwn_to_u64(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 +1024,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 +1072,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 +1141,11 @@ static int virtscsi_restore(struct virtio_device *vdev) > } > #endif > > +static void virtscsi_config_changed(struct virtio_device *vdev) > +{ This is called unconditionally here, will access invalid config fields if feature is off. In fact this is wasting an MSIX vector when feature is not negotiated. No easy way not to, but best document this in a code comment. > + virtscsi_update_fc_host_attrs(vdev); > +} > + > static struct virtio_device_id id_table[] = { > { VIRTIO_ID_SCSI, VIRTIO_DEV_ANY_ID }, > { 0 }, > @@ -1109,6 +1157,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 +1172,20 @@ 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 +1233,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
next prev parent reply other threads:[~2017-01-26 22:06 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-01-26 3:41 [PATCH v2 0/2] virtio-scsi: Implement FC_HOST feature Fam Zheng 2017-01-26 3:41 ` Fam Zheng 2017-01-26 3:41 ` [PATCH v2 1/2] virtio_scsi: Add fc_host definitions Fam Zheng 2017-01-26 3:41 ` Fam Zheng 2017-01-26 21:49 ` Michael S. Tsirkin 2017-01-26 21:49 ` Michael S. Tsirkin 2017-01-26 3:41 ` [PATCH v2 2/2] virtio_scsi: Implement fc_host Fam Zheng 2017-01-26 3:41 ` Fam Zheng 2017-01-26 8:34 ` Christoph Hellwig 2017-01-26 8:34 ` Christoph Hellwig 2017-01-26 8:55 ` kbuild test robot 2017-01-26 8:55 ` kbuild test robot 2017-01-26 22:06 ` Michael S. Tsirkin [this message] 2017-01-26 22:06 ` Michael S. Tsirkin 2017-01-27 8:43 ` Paolo Bonzini 2017-01-27 8:43 ` Paolo Bonzini
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170126234257-mutt-send-email-mst@kernel.org \ --to=mst@redhat.com \ --cc=famz@redhat.com \ --cc=jasowang@redhat.com \ --cc=jejb@linux.vnet.ibm.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-scsi@vger.kernel.org \ --cc=martin.petersen@oracle.com \ --cc=pbonzini@redhat.com \ --cc=stefanha@redhat.com \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.