All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR
@ 2023-07-04  3:34 Hawkins Jiawei
  2023-07-04  3:34 ` [PATCH v3 1/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mac() Hawkins Jiawei
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Hawkins Jiawei @ 2023-07-04  3:34 UTC (permalink / raw)
  To: jasowang, mst, eperezma; +Cc: qemu-stable, qemu-devel, yin31149, 18801353760

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mac()
  2023-07-04  3:34 [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR Hawkins Jiawei
@ 2023-07-04  3:34 ` Hawkins Jiawei
  2023-07-04  3:34 ` [PATCH v3 2/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mq() Hawkins Jiawei
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Hawkins Jiawei @ 2023-07-04  3:34 UTC (permalink / raw)
  To: jasowang, mst, eperezma; +Cc: qemu-stable, qemu-devel, yin31149, 18801353760

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_mac() 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 patch fixes this problem by returning -EIO when the device's
ack is not VIRTIO_NET_OK.

Fixes: f73c0c43ac ("vdpa: extract vhost_vdpa_net_load_mac from vhost_vdpa_net_load")
Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
---
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/07a1133d6c989394b342e35d8202257771e76769.1686746406.git.yin31149@gmail.com/

 net/vhost-vdpa.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index e19ab063fa..ee273c40ca 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -646,8 +646,9 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
         if (unlikely(dev_written < 0)) {
             return dev_written;
         }
-
-        return *s->status != VIRTIO_NET_OK;
+        if (*s->status != VIRTIO_NET_OK) {
+            return -EIO;
+        }
     }
 
     return 0;
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mq()
  2023-07-04  3:34 [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR Hawkins Jiawei
  2023-07-04  3:34 ` [PATCH v3 1/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mac() Hawkins Jiawei
@ 2023-07-04  3:34 ` Hawkins Jiawei
  2023-07-04  3:34 ` [PATCH v3 3/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_offloads() Hawkins Jiawei
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Hawkins Jiawei @ 2023-07-04  3:34 UTC (permalink / raw)
  To: jasowang, mst, eperezma; +Cc: qemu-stable, qemu-devel, yin31149, 18801353760

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_mq() 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 patch fixes this problem by returning -EIO when the device's
ack is not VIRTIO_NET_OK.

Fixes: f64c7cda69 ("vdpa: Add vhost_vdpa_net_load_mq")
Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
---
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/07a1133d6c989394b342e35d8202257771e76769.1686746406.git.yin31149@gmail.com/

 net/vhost-vdpa.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index ee273c40ca..03d87e85c8 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -671,8 +671,11 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
     if (unlikely(dev_written < 0)) {
         return dev_written;
     }
+    if (*s->status != VIRTIO_NET_OK) {
+        return -EIO;
+    }
 
-    return *s->status != VIRTIO_NET_OK;
+    return 0;
 }
 
 static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 3/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_offloads()
  2023-07-04  3:34 [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR Hawkins Jiawei
  2023-07-04  3:34 ` [PATCH v3 1/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mac() Hawkins Jiawei
  2023-07-04  3:34 ` [PATCH v3 2/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mq() Hawkins Jiawei
@ 2023-07-04  3:34 ` Hawkins Jiawei
  2023-07-05  7:59 ` [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR Lei Yang
  2023-08-05  6:15 ` Michael Tokarev
  4 siblings, 0 replies; 10+ messages in thread
From: Hawkins Jiawei @ 2023-07-04  3:34 UTC (permalink / raw)
  To: jasowang, mst, eperezma; +Cc: qemu-stable, qemu-devel, yin31149, 18801353760

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_offloads() 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 patch fixes this problem by returning -EIO when the device's
ack is not VIRTIO_NET_OK.

Fixes: 0b58d3686a ("vdpa: Add vhost_vdpa_net_load_offloads()")
Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
---
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/07a1133d6c989394b342e35d8202257771e76769.1686746406.git.yin31149@gmail.com/

 net/vhost-vdpa.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 03d87e85c8..36aa2d7f8c 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -712,8 +712,11 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
     if (unlikely(dev_written < 0)) {
         return dev_written;
     }
+    if (*s->status != VIRTIO_NET_OK) {
+        return -EIO;
+    }
 
-    return *s->status != VIRTIO_NET_OK;
+    return 0;
 }
 
 static int vhost_vdpa_net_load(NetClientState *nc)
-- 
2.25.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR
  2023-07-04  3:34 [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR Hawkins Jiawei
                   ` (2 preceding siblings ...)
  2023-07-04  3:34 ` [PATCH v3 3/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_offloads() Hawkins Jiawei
@ 2023-07-05  7:59 ` Lei Yang
  2023-07-05 11:03   ` Hawkins Jiawei
  2023-08-05  6:15 ` Michael Tokarev
  4 siblings, 1 reply; 10+ messages in thread
From: Lei Yang @ 2023-07-05  7:59 UTC (permalink / raw)
  To: Hawkins Jiawei
  Cc: jasowang, mst, eperezma, qemu-stable, qemu-devel, 18801353760

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?

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR
  2023-07-05  7:59 ` [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR Lei Yang
@ 2023-07-05 11:03   ` Hawkins Jiawei
  2023-07-06 11:03     ` Lei Yang
  0 siblings, 1 reply; 10+ messages in thread
From: Hawkins Jiawei @ 2023-07-05 11:03 UTC (permalink / raw)
  To: Lei Yang; +Cc: jasowang, mst, eperezma, qemu-stable, qemu-devel, 18801353760

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR
  2023-07-05 11:03   ` Hawkins Jiawei
@ 2023-07-06 11:03     ` Lei Yang
  0 siblings, 0 replies; 10+ messages in thread
From: Lei Yang @ 2023-07-06 11:03 UTC (permalink / raw)
  To: Hawkins Jiawei
  Cc: jasowang, mst, eperezma, qemu-stable, qemu-devel, 18801353760

On Wed, Jul 5, 2023 at 7:03 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> 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".

Based on the above steps, QE tests it without the above patch first,
it will not trigger any errors or warnings. Then QE manually applied
the above patch, boot guest again, it can trigger this warning:
qemu-system-x86_64: unable to start vhost net: 5: falling back on
userspace virtio, this is an expected result.

Tested-by: Lei Yang <leiyang@redhat.com>

BR
Lei

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR
  2023-07-04  3:34 [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR Hawkins Jiawei
                   ` (3 preceding siblings ...)
  2023-07-05  7:59 ` [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR Lei Yang
@ 2023-08-05  6:15 ` Michael Tokarev
  2023-08-05  9:28   ` Hawkins Jiawei
  4 siblings, 1 reply; 10+ messages in thread
From: Michael Tokarev @ 2023-08-05  6:15 UTC (permalink / raw)
  To: Hawkins Jiawei, jasowang, mst, eperezma
  Cc: qemu-stable, qemu-devel, 18801353760

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

Patch 6f34807116 also needs

b58d3686a0 vdpa: Add vhost_vdpa_net_load_offloads()

for 8.0.

Thanks,

/mjt


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR
  2023-08-05  6:15 ` Michael Tokarev
@ 2023-08-05  9:28   ` Hawkins Jiawei
  2023-08-05  9:53     ` Michael Tokarev
  0 siblings, 1 reply; 10+ messages in thread
From: Hawkins Jiawei @ 2023-08-05  9:28 UTC (permalink / raw)
  To: Michael Tokarev, jasowang, mst, eperezma
  Cc: qemu-stable, qemu-devel, 18801353760

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR
  2023-08-05  9:28   ` Hawkins Jiawei
@ 2023-08-05  9:53     ` Michael Tokarev
  0 siblings, 0 replies; 10+ messages in thread
From: Michael Tokarev @ 2023-08-05  9:53 UTC (permalink / raw)
  To: Hawkins Jiawei, jasowang, mst, eperezma
  Cc: qemu-stable, qemu-devel, 18801353760

05.08.2023 12:28, Hawkins Jiawei wrote:
..
>> 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.

Ok, this makes sense, thank you for making it clear.

> Sorry for not mentioning this information in the cover letter.

That's okay.  I too didn't pick it up in time, so fixing this now :

/mjt


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-08-05  9:54 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-04  3:34 [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR Hawkins Jiawei
2023-07-04  3:34 ` [PATCH v3 1/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mac() Hawkins Jiawei
2023-07-04  3:34 ` [PATCH v3 2/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_mq() Hawkins Jiawei
2023-07-04  3:34 ` [PATCH v3 3/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR in _load_offloads() Hawkins Jiawei
2023-07-05  7:59 ` [PATCH v3 0/3] vdpa: Return -EIO if device ack is VIRTIO_NET_ERR Lei Yang
2023-07-05 11:03   ` Hawkins Jiawei
2023-07-06 11:03     ` Lei Yang
2023-08-05  6:15 ` Michael Tokarev
2023-08-05  9:28   ` Hawkins Jiawei
2023-08-05  9:53     ` Michael Tokarev

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.