Re: [PATCH v1 2/4] virtio: make seg_max virtqueue size dependent

[Michael S. Tsirkin]

On Wed, Nov 06, 2019 at 10:07:02AM +0000, Denis Lunev wrote:
> On 11/5/19 9:51 PM, Michael S. Tsirkin wrote:
> > On Tue, Nov 05, 2019 at 07:11:03PM +0300, Denis Plotnikov wrote:
> >> seg_max has a restriction to be less or equal to virtqueue size
> >> according to Virtio 1.0 specification
> >>
> >> Although seg_max can't be set directly, it's worth to express this
> >> dependancy directly in the code for sanity purpose.
> >>
> >> Signed-off-by: Denis Plotnikov <address@hidden>
> > This is guest visible so needs to be machine type dependent, right?
> 
> we have discussed this verbally with Stefan and think that
> there is no need to add that to the machine type as:
> 
> - original default was 126, which matches 128 as queue
>   length in old machine types
> - queue length > 128 is not observed in the field as
>   SeaBios has quirk that asserts

Well that's just the SeaBios virtio driver. Not everyone's using that to
drive their devices.

> - if queue length will be set to something < 128 - linux
>   guest will crash

Again that's just one guest driver. Not everyone is using that either.


> If we really need to preserve original __buggy__ behavior -
> we can add boolean property, pls let us know.
> 
> Den

Looks like some drivers are buggy but I'm not sure it's
the same as saying the behavior is buggy.
So yes, I'd say it's preferable to be compatible.


> >> ---
> >>  hw/block/virtio-blk.c | 2 +-
> >>  hw/scsi/virtio-scsi.c | 2 +-
> >>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> >> index 06e57a4d39..21530304cf 100644
> >> --- a/hw/block/virtio-blk.c
> >> +++ b/hw/block/virtio-blk.c
> >> @@ -903,7 +903,7 @@ static void virtio_blk_update_config(VirtIODevice 
> >> *vdev, uint8_t *config)
> >>      blk_get_geometry(s->blk, &capacity);
> >>      memset(&blkcfg, 0, sizeof(blkcfg));
> >>      virtio_stq_p(vdev, &blkcfg.capacity, capacity);
> >> -    virtio_stl_p(vdev, &blkcfg.seg_max, 128 - 2);
> >> +    virtio_stl_p(vdev, &blkcfg.seg_max, s->conf.queue_size - 2);
> >>      virtio_stw_p(vdev, &blkcfg.geometry.cylinders, conf->cyls);
> >>      virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
> >>      virtio_stw_p(vdev, &blkcfg.min_io_size, conf->min_io_size / blk_size);
> >> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> >> index 839f120256..f7e5533cd5 100644
> >> --- a/hw/scsi/virtio-scsi.c
> >> +++ b/hw/scsi/virtio-scsi.c
> >> @@ -650,7 +650,7 @@ static void virtio_scsi_get_config(VirtIODevice *vdev,
> >>      VirtIOSCSICommon *s = VIRTIO_SCSI_COMMON(vdev);
> >>  
> >>      virtio_stl_p(vdev, &scsiconf->num_queues, s->conf.num_queues);
> >> -    virtio_stl_p(vdev, &scsiconf->seg_max, 128 - 2);
> >> +    virtio_stl_p(vdev, &scsiconf->seg_max, s->conf.virtqueue_size - 2);
> >>      virtio_stl_p(vdev, &scsiconf->max_sectors, s->conf.max_sectors);
> >>      virtio_stl_p(vdev, &scsiconf->cmd_per_lun, s->conf.cmd_per_lun);
> >>      virtio_stl_p(vdev, &scsiconf->event_info_size, 
> >> sizeof(VirtIOSCSIEvent));
> >> -- 
> >> 2.17.0
>