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 <dplotni...@virtuozzo.com> > >>> 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