Re: [PATCH 1/2] virtio-scsi: Make max_target value configurable

["Alex Bennée" <alex.bennee@linaro.org>]

Karolina Stolarek <karolina.stolarek@oracle.com> writes:

> In virtiscsi_probe(), virtio-scsi enumerates all possible
> targets, up to VIRTIO_SCSI_MAX_TARGET, to see which of them
> are available. This means that during that scan, the initiator
> queues up INQUIRY commands to the targets that do not exist.
> Such inquires fail, returning a BAD_TARGET response.
>
> Currently, there is no way to limit the number of possible
> targets or to finish the scan earlier. Add "max_target"
> option for virtio-scsi and vhost-scsi devices to provide
> a hint on the number of targets available for scanning.
>
> Signed-off-by: Karolina Stolarek <karolina.stolarek@oracle.com>
> ---
>  hw/scsi/vhost-scsi.c            |  2 ++
>  hw/scsi/virtio-scsi.c           | 42 +++++++++++++++++++--------------
>  include/hw/virtio/virtio-scsi.h |  1 +
>  3 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index cdf405b0f8..1a4860a72f 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -353,6 +353,8 @@ static const Property vhost_scsi_properties[] = {
>                         128),
>      DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSICommon, conf.seg_max_adjust,
>                        true),
> +    DEFINE_PROP_UINT16("max_target", VirtIOSCSICommon, conf.max_target,
> +                        VIRTIO_SCSI_MAX_TARGET),
>      DEFINE_PROP_UINT32("max_sectors", VirtIOSCSICommon, conf.max_sectors,
>                         0xFFFF),
>      DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSICommon, conf.cmd_per_lun, 128),
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 93e87c459c..091f68090e 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -971,7 +971,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>      virtio_stl_p(vdev, &scsiconf->sense_size, s->sense_size);
>      virtio_stl_p(vdev, &scsiconf->cdb_size, s->cdb_size);
>      virtio_stw_p(vdev, &scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
> -    virtio_stw_p(vdev, &scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
> +    virtio_stw_p(vdev, &scsiconf->max_target, s->conf.max_target);
>      virtio_stl_p(vdev, &scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
>  }
>  
> @@ -1260,23 +1260,26 @@ static void virtio_scsi_drained_end(SCSIBus *bus)
>      }
>  }
>  
> -static struct SCSIBusInfo virtio_scsi_scsi_info = {
> -    .tcq = true,
> -    .max_channel = VIRTIO_SCSI_MAX_CHANNEL,
> -    .max_target = VIRTIO_SCSI_MAX_TARGET,
> -    .max_lun = VIRTIO_SCSI_MAX_LUN,

See bellow.

> -
> -    .complete = virtio_scsi_command_complete,
> -    .fail = virtio_scsi_command_failed,
> -    .cancel = virtio_scsi_request_cancelled,
> -    .change = virtio_scsi_change,
> -    .parse_cdb = virtio_scsi_parse_cdb,
> -    .get_sg_list = virtio_scsi_get_sg_list,
> -    .save_request = virtio_scsi_save_request,
> -    .load_request = virtio_scsi_load_request,
> -    .drained_begin = virtio_scsi_drained_begin,
> -    .drained_end = virtio_scsi_drained_end,
> -};
> +static struct SCSIBusInfo virtio_scsi_scsi_info;
> +

We can't have a static structure here otherwise all the VirtIO scsi
devices will share the same configuration.

> +static void virtio_scsi_init_scsi_info(struct VirtIOSCSIConf *conf)
> +{
> +    virtio_scsi_scsi_info.tcq = true;
> +    virtio_scsi_scsi_info.max_channel = VIRTIO_SCSI_MAX_CHANNEL;
> +    virtio_scsi_scsi_info.max_lun = VIRTIO_SCSI_MAX_LUN;
> +    virtio_scsi_scsi_info.max_target = conf->max_target;

I think we need to allocate a dynamic configuration block for this
stuff. Although given we have ops and config mixed in this structure I
suspect the cleaner option is to re-factor SCSIBusInfo into
SCSIBusConfig and SCSIBusOps so you can keep the static around. But this
would touch quite a bit of code.

> +
> +    virtio_scsi_scsi_info.complete = virtio_scsi_command_complete;
> +    virtio_scsi_scsi_info.fail = virtio_scsi_command_failed;
> +    virtio_scsi_scsi_info.cancel = virtio_scsi_request_cancelled;
> +    virtio_scsi_scsi_info.change = virtio_scsi_change;
> +    virtio_scsi_scsi_info.parse_cdb = virtio_scsi_parse_cdb;
> +    virtio_scsi_scsi_info.get_sg_list = virtio_scsi_get_sg_list;
> +    virtio_scsi_scsi_info.save_request = virtio_scsi_save_request;
> +    virtio_scsi_scsi_info.load_request = virtio_scsi_load_request;
> +    virtio_scsi_scsi_info.drained_begin = virtio_scsi_drained_begin;
> +    virtio_scsi_scsi_info.drained_end = virtio_scsi_drained_end;
> +}
>  
>  void virtio_scsi_common_realize(DeviceState *dev,
>                                  VirtIOHandleOutput ctrl,
> @@ -1289,6 +1292,7 @@ void virtio_scsi_common_realize(DeviceState *dev,
>      int i;
>  
>      virtio_init(vdev, VIRTIO_ID_SCSI, sizeof(VirtIOSCSIConfig));
> +    virtio_scsi_init_scsi_info(&s->conf);
>  
>      if (s->conf.num_queues == VIRTIO_SCSI_AUTO_NUM_QUEUES) {
>          s->conf.num_queues = 1;
> @@ -1379,6 +1383,8 @@ static const Property virtio_scsi_properties[] = {
>                                           parent_obj.conf.virtqueue_size, 256),
>      DEFINE_PROP_BOOL("seg_max_adjust", VirtIOSCSI,
>                        parent_obj.conf.seg_max_adjust, true),
> +    DEFINE_PROP_UINT16("max_target", VirtIOSCSICommon, conf.max_target,
> +                        VIRTIO_SCSI_MAX_TARGET),
>      DEFINE_PROP_UINT32("max_sectors", VirtIOSCSI, parent_obj.conf.max_sectors,
>                                                    0xFFFF),
>      DEFINE_PROP_UINT32("cmd_per_lun", VirtIOSCSI, parent_obj.conf.cmd_per_lun,
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index b6028bb5cd..3998b241f6 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -54,6 +54,7 @@ struct VirtIOSCSIConf {
>      uint32_t virtqueue_size;
>      bool worker_per_virtqueue;
>      bool seg_max_adjust;
> +    uint16_t max_target;
>      uint32_t max_sectors;
>      uint32_t cmd_per_lun;
>      char *vhostfd;

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro