Re: [RFC v3 06/29] virtio-net: Honor VIRTIO_CONFIG_S_DEVICE_STOPPED

2021-06-02 Thread Jason Wang 



在 2021/6/1 下午3:13, Eugenio Perez Martin 写道:

On Wed, May 26, 2021 at 3:10 AM Jason Wang  wrote:


在 2021/5/26 上午9:06, Jason Wang 写道:

在 2021/5/20 上午12:28, Eugenio Pérez 写道:

So the guest can stop and start net device. It implements the RFC
https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html


To stop (as "pause") the device is required to migrate status and vring
addresses between device and SVQ.

This is a WIP commit: as with VIRTIO_F_QUEUE_STATE, is introduced in
virtio_config.h before of even proposing for the kernel, with no feature
flag, and, with no checking in the device. It also needs a modified
vp_vdpa driver that supports to set and retrieve status.

Signed-off-by: Eugenio Pérez 
---
   include/standard-headers/linux/virtio_config.h | 2 ++
   hw/net/virtio-net.c| 4 +++-
   2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/standard-headers/linux/virtio_config.h
b/include/standard-headers/linux/virtio_config.h
index 59fad3eb45..b3f6b1365d 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -40,6 +40,8 @@
   #define VIRTIO_CONFIG_S_DRIVER_OK4
   /* Driver has finished configuring features */
   #define VIRTIO_CONFIG_S_FEATURES_OK8
+/* Device is stopped */
+#define VIRTIO_CONFIG_S_DEVICE_STOPPED 32
   /* Device entered invalid state, driver must reset it */
   #define VIRTIO_CONFIG_S_NEEDS_RESET0x40
   /* We've given up on this device. */
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 96a3cc8357..2d3caea289 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -198,7 +198,9 @@ static bool virtio_net_started(VirtIONet *n,
uint8_t status)
   {
   VirtIODevice *vdev = VIRTIO_DEVICE(n);
   return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
-(n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
+(!(n->status & VIRTIO_CONFIG_S_DEVICE_STOPPED)) &&
+(n->status & VIRTIO_NET_S_LINK_UP) &&
+vdev->vm_running;
   }
 static void virtio_net_announce_notify(VirtIONet *net)


It looks to me this is only the part of pause.

For SVQ we need to switch vring addresses, and a full reset of the
device is required for that. Actually, the pause is just used to
recover

If you prefer this could be sent as a separate series where the full
pause/resume cycle is implemented, and then SVQ uses the pause part.
However there are no use for the resume part at the moment.



That would be fine if you can send it in another series.





And even for pause, I don't see anything that prevents rx/tx from being
executed? (E.g virtio_net_handle_tx_bh or virtio_net_handle_rx).


virtio_net_started is called from virtio_net_set_status. If
_S_DEVICE_STOPPED is true, the former return false, and variable
queue_started is false in the latter:
   queue_started =
 virtio_net_started(n, queue_status) && !n->vhost_started;

After that, it should work like a regular device reset or link down if
I'm not wrong, and the last part of virtio_net_set_status should
delete timer or cancel bh.



You are right.

Thanks





Thanks



We still need the resume?

Thanks

Re: [RFC v3 06/29] virtio-net: Honor VIRTIO_CONFIG_S_DEVICE_STOPPED

2021-06-01 Thread Eugenio Perez Martin 
On Wed, May 26, 2021 at 3:10 AM Jason Wang  wrote:
>
>
> 在 2021/5/26 上午9:06, Jason Wang 写道:
> >
> > 在 2021/5/20 上午12:28, Eugenio Pérez 写道:
> >> So the guest can stop and start net device. It implements the RFC
> >> https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html
> >>
> >>
> >> To stop (as "pause") the device is required to migrate status and vring
> >> addresses between device and SVQ.
> >>
> >> This is a WIP commit: as with VIRTIO_F_QUEUE_STATE, is introduced in
> >> virtio_config.h before of even proposing for the kernel, with no feature
> >> flag, and, with no checking in the device. It also needs a modified
> >> vp_vdpa driver that supports to set and retrieve status.
> >>
> >> Signed-off-by: Eugenio Pérez 
> >> ---
> >>   include/standard-headers/linux/virtio_config.h | 2 ++
> >>   hw/net/virtio-net.c| 4 +++-
> >>   2 files changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/standard-headers/linux/virtio_config.h
> >> b/include/standard-headers/linux/virtio_config.h
> >> index 59fad3eb45..b3f6b1365d 100644
> >> --- a/include/standard-headers/linux/virtio_config.h
> >> +++ b/include/standard-headers/linux/virtio_config.h
> >> @@ -40,6 +40,8 @@
> >>   #define VIRTIO_CONFIG_S_DRIVER_OK4
> >>   /* Driver has finished configuring features */
> >>   #define VIRTIO_CONFIG_S_FEATURES_OK8
> >> +/* Device is stopped */
> >> +#define VIRTIO_CONFIG_S_DEVICE_STOPPED 32
> >>   /* Device entered invalid state, driver must reset it */
> >>   #define VIRTIO_CONFIG_S_NEEDS_RESET0x40
> >>   /* We've given up on this device. */
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 96a3cc8357..2d3caea289 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -198,7 +198,9 @@ static bool virtio_net_started(VirtIONet *n,
> >> uint8_t status)
> >>   {
> >>   VirtIODevice *vdev = VIRTIO_DEVICE(n);
> >>   return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
> >> -(n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
> >> +(!(n->status & VIRTIO_CONFIG_S_DEVICE_STOPPED)) &&
> >> +(n->status & VIRTIO_NET_S_LINK_UP) &&
> >> +vdev->vm_running;
> >>   }
> >> static void virtio_net_announce_notify(VirtIONet *net)
> >
> >
> > It looks to me this is only the part of pause.
>

For SVQ we need to switch vring addresses, and a full reset of the
device is required for that. Actually, the pause is just used to
recover

If you prefer this could be sent as a separate series where the full
pause/resume cycle is implemented, and then SVQ uses the pause part.
However there are no use for the resume part at the moment.

>
> And even for pause, I don't see anything that prevents rx/tx from being
> executed? (E.g virtio_net_handle_tx_bh or virtio_net_handle_rx).
>

virtio_net_started is called from virtio_net_set_status. If
_S_DEVICE_STOPPED is true, the former return false, and variable
queue_started is false in the latter:
  queue_started =
virtio_net_started(n, queue_status) && !n->vhost_started;

After that, it should work like a regular device reset or link down if
I'm not wrong, and the last part of virtio_net_set_status should
delete timer or cancel bh.

> Thanks
>
>
> > We still need the resume?
> >
> > Thanks
> >
> >
>

Re: [RFC v3 06/29] virtio-net: Honor VIRTIO_CONFIG_S_DEVICE_STOPPED

2021-05-25 Thread Jason Wang 



在 2021/5/26 上午9:06, Jason Wang 写道:


在 2021/5/20 上午12:28, Eugenio Pérez 写道:

So the guest can stop and start net device. It implements the RFC
https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html 



To stop (as "pause") the device is required to migrate status and vring
addresses between device and SVQ.

This is a WIP commit: as with VIRTIO_F_QUEUE_STATE, is introduced in
virtio_config.h before of even proposing for the kernel, with no feature
flag, and, with no checking in the device. It also needs a modified
vp_vdpa driver that supports to set and retrieve status.

Signed-off-by: Eugenio Pérez 
---
  include/standard-headers/linux/virtio_config.h | 2 ++
  hw/net/virtio-net.c    | 4 +++-
  2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/standard-headers/linux/virtio_config.h 
b/include/standard-headers/linux/virtio_config.h

index 59fad3eb45..b3f6b1365d 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -40,6 +40,8 @@
  #define VIRTIO_CONFIG_S_DRIVER_OK    4
  /* Driver has finished configuring features */
  #define VIRTIO_CONFIG_S_FEATURES_OK    8
+/* Device is stopped */
+#define VIRTIO_CONFIG_S_DEVICE_STOPPED 32
  /* Device entered invalid state, driver must reset it */
  #define VIRTIO_CONFIG_S_NEEDS_RESET    0x40
  /* We've given up on this device. */
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 96a3cc8357..2d3caea289 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -198,7 +198,9 @@ static bool virtio_net_started(VirtIONet *n, 
uint8_t status)

  {
  VirtIODevice *vdev = VIRTIO_DEVICE(n);
  return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
-    (n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
+    (!(n->status & VIRTIO_CONFIG_S_DEVICE_STOPPED)) &&
+    (n->status & VIRTIO_NET_S_LINK_UP) &&
+    vdev->vm_running;
  }
    static void virtio_net_announce_notify(VirtIONet *net)



It looks to me this is only the part of pause. 



And even for pause, I don't see anything that prevents rx/tx from being 
executed? (E.g virtio_net_handle_tx_bh or virtio_net_handle_rx).


Thanks



We still need the resume?

Thanks

Re: [RFC v3 06/29] virtio-net: Honor VIRTIO_CONFIG_S_DEVICE_STOPPED

2021-05-25 Thread Jason Wang 



在 2021/5/20 上午12:28, Eugenio Pérez 写道:

So the guest can stop and start net device. It implements the RFC
https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html

To stop (as "pause") the device is required to migrate status and vring
addresses between device and SVQ.

This is a WIP commit: as with VIRTIO_F_QUEUE_STATE, is introduced in
virtio_config.h before of even proposing for the kernel, with no feature
flag, and, with no checking in the device. It also needs a modified
vp_vdpa driver that supports to set and retrieve status.

Signed-off-by: Eugenio Pérez 
---
  include/standard-headers/linux/virtio_config.h | 2 ++
  hw/net/virtio-net.c| 4 +++-
  2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/standard-headers/linux/virtio_config.h 
b/include/standard-headers/linux/virtio_config.h
index 59fad3eb45..b3f6b1365d 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -40,6 +40,8 @@
  #define VIRTIO_CONFIG_S_DRIVER_OK 4
  /* Driver has finished configuring features */
  #define VIRTIO_CONFIG_S_FEATURES_OK   8
+/* Device is stopped */
+#define VIRTIO_CONFIG_S_DEVICE_STOPPED 32
  /* Device entered invalid state, driver must reset it */
  #define VIRTIO_CONFIG_S_NEEDS_RESET   0x40
  /* We've given up on this device. */
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 96a3cc8357..2d3caea289 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -198,7 +198,9 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
  {
  VirtIODevice *vdev = VIRTIO_DEVICE(n);
  return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
-(n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
+(!(n->status & VIRTIO_CONFIG_S_DEVICE_STOPPED)) &&
+(n->status & VIRTIO_NET_S_LINK_UP) &&
+vdev->vm_running;
  }
  
  static void virtio_net_announce_notify(VirtIONet *net)



It looks to me this is only the part of pause. We still need the resume?

Thanks

[RFC v3 06/29] virtio-net: Honor VIRTIO_CONFIG_S_DEVICE_STOPPED

2021-05-19 Thread Eugenio Pérez 
So the guest can stop and start net device. It implements the RFC
https://lists.oasis-open.org/archives/virtio-comment/202012/msg00027.html

To stop (as "pause") the device is required to migrate status and vring
addresses between device and SVQ.

This is a WIP commit: as with VIRTIO_F_QUEUE_STATE, is introduced in
virtio_config.h before of even proposing for the kernel, with no feature
flag, and, with no checking in the device. It also needs a modified
vp_vdpa driver that supports to set and retrieve status.

Signed-off-by: Eugenio Pérez 
---
 include/standard-headers/linux/virtio_config.h | 2 ++
 hw/net/virtio-net.c| 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/standard-headers/linux/virtio_config.h 
b/include/standard-headers/linux/virtio_config.h
index 59fad3eb45..b3f6b1365d 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -40,6 +40,8 @@
 #define VIRTIO_CONFIG_S_DRIVER_OK  4
 /* Driver has finished configuring features */
 #define VIRTIO_CONFIG_S_FEATURES_OK8
+/* Device is stopped */
+#define VIRTIO_CONFIG_S_DEVICE_STOPPED 32
 /* Device entered invalid state, driver must reset it */
 #define VIRTIO_CONFIG_S_NEEDS_RESET0x40
 /* We've given up on this device. */
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 96a3cc8357..2d3caea289 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -198,7 +198,9 @@ static bool virtio_net_started(VirtIONet *n, uint8_t status)
 {
 VirtIODevice *vdev = VIRTIO_DEVICE(n);
 return (status & VIRTIO_CONFIG_S_DRIVER_OK) &&
-(n->status & VIRTIO_NET_S_LINK_UP) && vdev->vm_running;
+(!(n->status & VIRTIO_CONFIG_S_DEVICE_STOPPED)) &&
+(n->status & VIRTIO_NET_S_LINK_UP) &&
+vdev->vm_running;
 }
 
 static void virtio_net_announce_notify(VirtIONet *net)
-- 
2.27.0