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

[Denis Plotnikov]

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