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

Karolina Stolarek posted 2 patches 2 months, 2 weeks ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>, "Michael S. Tsirkin" <mst@redhat.com>, Stefano Garzarella <sgarzare@redhat.com>
[PATCH 1/2] virtio-scsi: Make max_target value configurable
Posted by Karolina Stolarek 2 months, 2 weeks ago
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,
-
-    .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;
+
+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;
+
+    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;
-- 
2.47.1
Re: [PATCH 1/2] virtio-scsi: Make max_target value configurable
Posted by Alex Bennée 2 months ago
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
Re: [PATCH 1/2] virtio-scsi: Make max_target value configurable
Posted by Karolina Stolarek 2 months ago
On 08/12/2025 14:54, Alex Bennée wrote:
> Karolina Stolarek <karolina.stolarek@oracle.com> writes:
> 
>> -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.

Ah, that is true, it escaped me.

> 
>> +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.

I understand, that sounds like a better solution. It might take some 
time to finalize this, but I'd like to try it for v2.

Many thanks for your review, really appreciate it.

All the best,
Karolina

> 
>> +
>> +    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;
>