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

[Hawkins Jiawei]

On 2023/7/5 15:59, Lei Yang wrote:
> Hello Hawkins
>
> QE can help test this series before  it is merged into master, I would
> like to know what test steps can cover this series related scenario?
>

Hi, I would like to suggest the following steps to test this patch series:

1.  Modify the QEMU source code to make the device return a
VIRTIO_NET_ERR for the CVQ command. Please apply the patch
provided below:

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 373609216f..58ade6d4e0 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -642,7 +642,7 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState
*s, const VirtIONet *n)
      if (virtio_vdev_has_feature(&n->parent_obj,
VIRTIO_NET_F_CTRL_MAC_ADDR)) {
          ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
VIRTIO_NET_CTRL_MAC,

VIRTIO_NET_CTRL_MAC_ADDR_SET,
-                                                  n->mac, sizeof(n->mac));
+                                                  n->mac,
sizeof(n->mac) - 1);
          if (unlikely(dev_written < 0)) {
              return dev_written;
          }

2. Start QEMU with the vdpa device in default state.
Without the patch series, QEMU should not trigger any errors or warnings.
With the series applied, QEMU should trigger the warning like
"qemu-system-x86_64: unable to start vhost net: 5: falling back on
userspace virtio".

Thanks!


> Thanks
> Lei
>
> On Tue, Jul 4, 2023 at 11:36 AM Hawkins Jiawei <yin31149@gmail.com> 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()
>>
>>   net/vhost-vdpa.c | 15 +++++++++++----
>>   1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> --
>> 2.25.1
>>
>>
>