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

[Karolina Stolarek <karolina.stolarek@oracle.com>]

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