[RFC,v1,1/5] hw/scsi: Override the max_sectors value for virtio-scsi

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 <farman@linux.vnet.ibm.com>
---
 hw/scsi/virtio-scsi.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

On 26/04/2017 16:46, 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.

Have you tried looking at the block limits VPD page in the firmware?
QEMU is passing BLKSECTGET down by patching that VPD page.  Your
solution would not work with hotplug.

Thanks,

Paolo

> Signed-off-by: Eric Farman <farman@linux.vnet.ibm.com>
> ---
>  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,
>

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 <farman@linux.vnet.ibm.com>

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