[PATCH] virtio: add support for SIZE_MAX & SEG_MAX features

[Christian Pötzsch]

Hi all,

On 4/8/26 9:12 PM, Tom Rini wrote:
> On Thu, Apr 02, 2026 at 01:41:11PM -0600, Simon Glass wrote:
>> Hi Christian,
>>
>> On 2026-03-27T08:18:58, Christian Pötzsch
>> <christian.poetzsch at kernkonzept.com> wrote:
>>> virtio: add support for SIZE_MAX & SEG_MAX features
>>> virtio: add support for SIZE_MAX & SEG_MAX features
>>>
>>> Some virtio implementations may forward the virtio requests directly to
>>> the underlying hw. The hw may have some restrictions in how many and how
>>> big the requests can be. Therefore, the corresponding virtio device will
>>> announce this limitations with the SIZE_MAX & SEG_MAX feature.
>>>
>>> Add support for those features. Split an io request into multiple virtio
>>> requests if more than seg_max segments would be used. Also split a
>>> single buffer request into multiple segments if the buffer is bigger
>>> then size_max.
>>>
>>> Signed-off-by: Christian Pötzsch <christian.poetzsch at kernkonzept.com>
>>> Signed-off-by: Adam Lackorzynski <adam at l4re.org>
>>
>>> diff --git a/drivers/virtio/virtio_blk.c b/drivers/virtio/virtio_blk.c
>>> @@ -119,7 +145,38 @@ static ulong virtio_blk_do_req(...)
>>> +err_free:
>>> +     for (i = 0; i < seg_cnt + 2; ++i)
>>> +             free(sgs[i]);
>>> +     free(sgs);
>>> +
>>> +     return status == VIRTIO_BLK_S_OK ? blkcnt : -EIO;
>>
>> The err_free label is reached from both the success path (fall-through
>> after virtqueue_get_buf()) and error paths (goto from malloc()
>> failure, virtqueue_add() failure, or unknown type). On error paths,
>> status is uninitialised, so the final check has undefined behaviour.
>> Please can you separate the success path from the error cleanup, e.g.
>> with an explicit return before the err_free label.
Right, the error case is not well defined. However, I can't return before the label, because than the two segments are leaked. At this point the virtio operation has finished, so the segment pointers need to be freed. In v2, I did initialize the status to error instead. Now, the error case is well defined too.

>>
>>> diff --git a/drivers/virtio/virtio_blk.c b/drivers/virtio/virtio_blk.c
>>> @@ -145,38 +145,38 @@ static ulong virtio_blk_do_req(...)
>>> +     while (i < blkcnt) {
>>> +             u32 blk_per_sg = min(blkcnt - i, seg_sec_cnt * priv->seg_max);
>>> +
>>> +             ret = virtio_blk_do_single_req(dev, sector + i, blk_per_sg,
>>> +                                            buffer + i * 512, type);
>>> +             if (!ret)
>>> +                     return ret;
>>
>> The error check is inverted. virtio_blk_do_single_req() returns blkcnt
>> (positive) on success and -EIO on failure, so if (!ret) only triggers
>> when ret is zero. I suspect you want if (ret < 0) here.
Correct, fixed it in v2.

>>
>> BTW please add a function comment for virtio_blk_do_single_req() so
>> all this is clearer.
Done in v2.

> 
> This is all true, and useful feedback.
> 
>>> diff --git a/drivers/virtio/virtio_blk.c b/drivers/virtio/virtio_blk.c
>>> @@ -67,50 +74,69 @@ static void virtio_blk_init_data_sg(...)
>>> +     // The virtio device may have constrains on the maximum segment size.
>>
>> U-Boot follows the Linux kernel coding style which prefers /* */
>> comments over //. Please can you convert these.
> 
> This hasn't been true for a number of years, Linux Kernel nor here.
> We've accepted C99-style comments (when not needing to be kernel-doc)
> for a long time now.
>
Thanks for the info Tom, I still converted the comments in v2.

Thanks
Chris