| Message ID | 20191105161105.19016-3-dplotnikov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: fix IO request length in virtio SCSI/block | expand |
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 <dplotnikov@virtuozzo.com> This is guest visible so needs to be machine type dependent, right? > --- > 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
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 <dplotnikov@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 - if queue length will be set to something < 128 - linux guest will crash If we really need to preserve original __buggy__ behavior - we can add boolean property, pls let us know. Den > >> --- >> 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
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 <dplotnikov@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 >
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. 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 <dplotnikov@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
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 <dplotnikov@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
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));
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 <dplotnikov@virtuozzo.com> --- hw/block/virtio-blk.c | 2 +- hw/scsi/virtio-scsi.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)