Re: [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR

[Hawkins Jiawei <yin31149@gmail.com>]

On 2023/8/5 14:15, Michael Tokarev wrote:
> 04.07.2023 06:34, Hawkins Jiawei wrote:
>> According to VirtIO standard, "The class, command and
>> command-specific-data are set by the driver,
>> and the device sets the ack byte.
>> There is little it can do except issue a diagnostic
>> if ack is not VIRTIO_NET_OK."
>>
>> Therefore, QEMU should stop sending the queued SVQ commands and
>> cancel the device startup if the device's ack is not VIRTIO_NET_OK.
>>
>> Yet the problem is that, vhost_vdpa_net_load_x() returns 1 based on
>> `*s->status != VIRTIO_NET_OK` when the device's ack is VIRTIO_NET_ERR.
>> As a result, net->nc->info->load() also returns 1, this makes
>> vhost_net_start_one() incorrectly assume the device state is
>> successfully loaded by vhost_vdpa_net_load() and return 0, instead of
>> goto `fail` label to cancel the device startup, as vhost_net_start_one()
>> only cancels the device startup when net->nc->info->load() returns a
>> negative value.
>>
>> This patchset fixes this problem by returning -EIO when the device's
>> ack is not VIRTIO_NET_OK.
>>
>> Changelog
>> =========
>> v3:
>>   - split the fixes suggested by Eugenio
>>   - return -EIO suggested by Michael
>>
>> v2:
>> https://lore.kernel.org/all/69010e9ebb5e3729aef595ed92840f43e48e53e5.1687875592.git.yin31149@gmail.com/
>>   - fix the same bug in vhost_vdpa_net_load_offloads()
>>
>> v1: https://lore.kernel.org/all/cover.1686746406.git.yin31149@gmail.com/
>>
>> Hawkins Jiawei (3):
>>    vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mac()
>>    vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mq()
>>    vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_offloads()
>
> Hi!
>
> I don't remember why, but this patch series is marked as "check later" in
> my qemu-stable-to-apply email folder.  Does it make sense to back-port this
> series to stable-8.0?
>
> 6f34807116 vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in
> _load_offloads()
> f45fd95ec9 vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mq()
> b479bc3c9d vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mac()
>

Hi Michael,

Yes, this bug exists in stable-8.0, so it makes sense to back-port this
series.

Commit f45fd95ec9("vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in
_load_mq()") and
commit b479bc3c9d("vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in
_load_mac()") can be back-ported directly.

> Patch 6f34807116 also needs
>
> b58d3686a0 vdpa: Add vhost_vdpa_net_load_offloads()

As you point out, patch 6f34807116("vdpa: Return -EIO if device ack is
VIRTIO_NET_ERR in _load_offloads()") is a fix to the commit
b58d3686a0("vdpa: Add vhost_vdpa_net_load_offloads()"), which was
introduced by patch series "Vhost-vdpa Shadow Virtqueue Offloads
support" at [1].

This mentioned patch series introduces a new feature for QEMU and
has not been merged into stable-8.0 yet, so I think we do not need to
apply the
patch 6f34807116("vdpa: Return -EIO if device ack is
VIRTIO_NET_ERR in _load_offloads()") to stable-8.0.

Sorry for not mentioning this information in the cover letter.

Thanks!

[1]. https://lore.kernel.org/all/cover.1685704856.git.yin31149@gmail.com/

>
> for 8.0.
>
> Thanks,
>
> /mjt