From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754492AbdA0Io1 (ORCPT ); Fri, 27 Jan 2017 03:44:27 -0500 Received: from mx3-phx2.redhat.com ([209.132.183.24]:37163 "EHLO mx3-phx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754354AbdA0IoD (ORCPT ); Fri, 27 Jan 2017 03:44:03 -0500 Date: Fri, 27 Jan 2017 03:43:57 -0500 (EST) From: Paolo Bonzini To: "Michael S. Tsirkin" Cc: Fam Zheng , linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org, "James E.J. Bottomley" , Jason Wang , "Martin K. Petersen" , stefanha@redhat.com, virtualization@lists.linux-foundation.org Message-ID: <395840854.13088421.1485506637639.JavaMail.zimbra@redhat.com> In-Reply-To: <20170126234257-mutt-send-email-mst@kernel.org> References: <20170126034109.16144-1-famz@redhat.com> <20170126034109.16144-3-famz@redhat.com> <20170126234257-mutt-send-email-mst@kernel.org> Subject: Re: [PATCH v2 2/2] virtio_scsi: Implement fc_host MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [10.4.164.1, 10.5.100.50] X-Mailer: Zimbra 8.0.6_GA_5922 (ZimbraWebClient - FF50 (Linux)/8.0.6_GA_5922) Thread-Topic: virtio_scsi: Implement fc_host Thread-Index: 9GfOESBdhkIv9tjT4oZyDwBBPUGtIw== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- Original Message ----- > From: "Michael S. Tsirkin" > To: "Fam Zheng" > Cc: linux-kernel@vger.kernel.org, "Paolo Bonzini" , linux-scsi@vger.kernel.org, "James E.J. > Bottomley" , "Jason Wang" , "Martin K. Petersen" > , stefanha@redhat.com, virtualization@lists.linux-foundation.org > Sent: Thursday, January 26, 2017 11:06:27 PM > Subject: Re: [PATCH v2 2/2] virtio_scsi: Implement fc_host > > 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. Discussion on the TC mailing list was not about the merit of the feature, only about the timing of the vote. Paolo > > > > > Signed-off-by: Fam Zheng > > --- > > 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 > > #include > > #include > > +#include > > #include > > > > #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 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v2 2/2] virtio_scsi: Implement fc_host Date: Fri, 27 Jan 2017 03:43:57 -0500 (EST) Message-ID: <395840854.13088421.1485506637639.JavaMail.zimbra@redhat.com> References: <20170126034109.16144-1-famz@redhat.com> <20170126034109.16144-3-famz@redhat.com> <20170126234257-mutt-send-email-mst@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170126234257-mutt-send-email-mst@kernel.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org To: "Michael S. Tsirkin" Cc: "James E.J. Bottomley" , "Martin K. Petersen" , linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, stefanha@redhat.com List-Id: linux-scsi@vger.kernel.org ----- Original Message ----- > From: "Michael S. Tsirkin" > To: "Fam Zheng" > Cc: linux-kernel@vger.kernel.org, "Paolo Bonzini" , linux-scsi@vger.kernel.org, "James E.J. > Bottomley" , "Jason Wang" , "Martin K. Petersen" > , stefanha@redhat.com, virtualization@lists.linux-foundation.org > Sent: Thursday, January 26, 2017 11:06:27 PM > Subject: Re: [PATCH v2 2/2] virtio_scsi: Implement fc_host > > 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. Discussion on the TC mailing list was not about the merit of the feature, only about the timing of the vote. Paolo > > > > > Signed-off-by: Fam Zheng > > --- > > 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 > > #include > > #include > > +#include > > #include > > > > #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 >