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

[Michael S. Tsirkin]

On Fri, Nov 08, 2019 at 07:43:22AM +0000, Denis Plotnikov wrote:
> The 1st patch from the series seems to be useless. The patch extending 
> queue length by adding machine type may break vm-s which use seabios 
> with max queue size = 128.
> 
> Looks like only this patch doesn't break anything and helps to express 
> queue size and seg max dependency (the specification constraint) 
> explicitly. So, I would like to re-send this patch as a standalone one 
> and send other patches including the test later, when we all agree on 
> how exactly to deal with issues posted in the thread.

OK, and I think we should make it machine type dependent.

> Any objections are welcome.
> 
> Denis
> 
> On 06.11.2019 14:54, Michael S. Tsirkin wrote:
> > 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