Re: [Qemu-devel] [RFC PATCH v1 1/5] hw/scsi: Override the max_sectors value for virtio-scsi

[Christian Borntraeger]

On 04/26/2017 04:46 PM, Eric Farman wrote:
> The virtio spec states that the max_sectors field is
> "a hint to the driver for the maximum transfer size"
> that would be used for a virtio-scsi request.
> 
> It's currently hardcoded to xFFFF unless one is established
> by the max_sectors parameter on the command line, but let's
> roll through the associated devices and set it to anything
> lower if one is set for the underlying block device and
> retrieved by the BLKSECTGET ioctl.
> 
> Signed-off-by: Eric Farman <address@hidden>

So since nobody else commented, I looked into this (not knowing anything 
about scsi before)

Your patch basically limits the HBA limit to the lowest size of all child 
devices.
This will certainly work, as guests will use the minimum of per-device size
and hba size.
Now: I think this might have some impact on cases where a HBA will have 
different
scsi disks or even non disk type scsi devices (what does blk_get_max_transfer 
return for a scsi cdrom or old scsi-1 devices?)

So in essence I think what could be a better solution is to enhance the 
ccw bios to actually read the per-device config in addition to the hba config.

Linux seems to use the Block Limits VPD any maybe the ccw bios should do the
same. So basically keep patch 4 and add an additional call to read the block VPD
and use the minimum of HBA and device value?

> ---
>  hw/scsi/virtio-scsi.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 46a3e3f..bca9461 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -640,7 +640,11 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>                                     uint8_t *config)
>  {
>      VirtIOSCSIConfig *scsiconf = (VirtIOSCSIConfig *)config;
> +    VirtIOSCSI *vs = VIRTIO_SCSI(vdev);
>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
> +    SCSIDevice *d;
> +    BusChild *kid;
> +    unsigned int max_transfer;
> 
>      virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
>      virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
> @@ -652,6 +656,14 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
>      virtio_stw_p(vdev, &scsiconf->max_channel, VIRTIO_SCSI_MAX_CHANNEL);
>      virtio_stw_p(vdev, &scsiconf->max_target, VIRTIO_SCSI_MAX_TARGET);
>      virtio_stl_p(vdev, &scsiconf->max_lun, VIRTIO_SCSI_MAX_LUN);
> +
> +    QTAILQ_FOREACH(kid, &vs->bus.qbus.children, sibling) {
> +        d = SCSI_DEVICE(kid->child);
> +        max_transfer = blk_get_max_transfer(d->conf.blk) / d->blocksize;
> +        virtio_stl_p(vdev,
> +                     &scsiconf->max_sectors,
> +                     MIN_NON_ZERO(max_transfer, scsiconf->max_sectors));
> +    }
>  }
> 
>  static void virtio_scsi_set_config(VirtIODevice *vdev,
>