| Message ID | 20241101015101.98111-1-qiang4.zhang@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v2] virtio: only reset device and restore status if needed in device resume | expand |
On Fri, Nov 1, 2024 at 9:54 AM <qiang4.zhang@linux.intel.com> wrote: > > From: Qiang Zhang <qiang4.zhang@intel.com> > > Virtio core unconditionally reset and restore status for all virtio > devices before calling restore method. This breaks some virtio drivers > which don't need to do anything in suspend and resume because they > just want to keep device state retained. The challenge is how can driver know device doesn't need rest. For example, PCI has no_soft_reset which has been done in the commit "virtio: Add support for no-reset virtio PCI PM". And there's a ongoing long discussion of adding suspend support in the virtio spec, then driver know it's safe to suspend/resume without reset. > > Virtio GPIO is a typical example. GPIO states should be kept unchanged > after suspend and resume (e.g. output pins keep driving the output) and > Virtio GPIO driver does nothing in freeze and restore methods. But the > reset operation in virtio_device_restore breaks this. Is this mandated by GPIO or virtio spec? If yes, let's quote the revelant part. > > Since some devices need reset in suspend and resume while some needn't, > create a new helper function for the original reset and status restore > logic so that virtio drivers can invoke it in their restore method > if necessary. How are those drivers classified? > > Signed-off-by: Qiang Zhang <qiang4.zhang@intel.com> Thanks
On Fri, Nov 01, 2024 at 10:11:11AM +0800, Jason Wang wrote: > On Fri, Nov 1, 2024 at 9:54 AM <qiang4.zhang@linux.intel.com> wrote: > > > > From: Qiang Zhang <qiang4.zhang@intel.com> > > > > Virtio core unconditionally reset and restore status for all virtio > > devices before calling restore method. This breaks some virtio drivers > > which don't need to do anything in suspend and resume because they > > just want to keep device state retained. > > The challenge is how can driver know device doesn't need rest. Hi, Per my understanding to PM, in the suspend flow, device drivers need to 1. First manage/stop accesses from upper level software and 2. Store the volatile context into in-memory data structures. 3. Put devices into some low power (suspended) state. The resume process does the reverse. If a device context won't loose after entering some low power state (optional), it's OK to skip step 2. For virtio devices, spec doesn't define whether their states will lost after platform entering suspended state. So to work with different hypervisors, virtio drivers typically trigger a reset in suspend/resume flow. This works fine for virtio devices if following conditions are met: - Device state can be totally recoverable. - There isn't any working behaviour expected in suspended state, i.e. the suspended state should be sub-state of reset. However, the first point may be hard to implement from driver side for some devices. The second point may be unacceptable for some kind of devices. For your question, for devices whose suspended state is alike reset state, the hypervisor have the flexibility to retain its state or not, kernel driver can unconditionally reset it with proper re-initialization to accomplish better compatibility. For others, hypervisor *must* retain device state and driver just keeps using it. > > For example, PCI has no_soft_reset which has been done in the commit > "virtio: Add support for no-reset virtio PCI PM". > > And there's a ongoing long discussion of adding suspend support in the > virtio spec, then driver know it's safe to suspend/resume without > reset. That's great! Hopefully it can fill the gap. Currently, I think we can safely move the reset to drivers' freeze methods, virtio core has no reason to take it as a common action required by all devices. And the reset operation can be optional skipped if driver have hints from device that it can retain state. > > > > > Virtio GPIO is a typical example. GPIO states should be kept unchanged > > after suspend and resume (e.g. output pins keep driving the output) and > > Virtio GPIO driver does nothing in freeze and restore methods. But the > > reset operation in virtio_device_restore breaks this. > > Is this mandated by GPIO or virtio spec? If yes, let's quote the revelant part. No. But in actual hardware design (e.g. Intel PCH GPIO), or from the requirement perspective, GPIO pin state can be (should support) retained in suspended state. If Virtio GPIO is used to let VM operate such physical GPIO chip indirectly, it can't be reset in suspend and resume. Meanwhile the hypervisor will retain pin states after suspension. > > > > > Since some devices need reset in suspend and resume while some needn't, > > create a new helper function for the original reset and status restore > > logic so that virtio drivers can invoke it in their restore method > > if necessary. > > How are those drivers classified? I think this depends whether hypervisor will keep devices state in platform suspend process. I think hypervisor should because suspend and reset are conceptually two different things. Thanks Qiang
On Fri, Nov 1, 2024 at 1:23 PM Qiang Zhang <qiang4.zhang@linux.intel.com> wrote: > > On Fri, Nov 01, 2024 at 10:11:11AM +0800, Jason Wang wrote: > > On Fri, Nov 1, 2024 at 9:54 AM <qiang4.zhang@linux.intel.com> wrote: > > > > > > From: Qiang Zhang <qiang4.zhang@intel.com> > > > > > > Virtio core unconditionally reset and restore status for all virtio > > > devices before calling restore method. This breaks some virtio drivers > > > which don't need to do anything in suspend and resume because they > > > just want to keep device state retained. > > > > The challenge is how can driver know device doesn't need rest. > > Hi, > > Per my understanding to PM, in the suspend flow, device drivers need to > 1. First manage/stop accesses from upper level software and > 2. Store the volatile context into in-memory data structures. > 3. Put devices into some low power (suspended) state. > The resume process does the reverse. > If a device context won't loose after entering some low power state > (optional), it's OK to skip step 2. > > For virtio devices, spec doesn't define whether their states will lost > after platform entering suspended state. This is exactly what suspend patch tries to define. > So to work with different > hypervisors, virtio drivers typically trigger a reset in suspend/resume > flow. This works fine for virtio devices if following conditions are met: > - Device state can be totally recoverable. > - There isn't any working behaviour expected in suspended state, i.e. the > suspended state should be sub-state of reset. > However, the first point may be hard to implement from driver side for some > devices. The second point may be unacceptable for some kind of devices. > > For your question, for devices whose suspended state is alike reset state, > the hypervisor have the flexibility to retain its state or not, kernel > driver can unconditionally reset it with proper re-initialization to > accomplish better compatibility. For others, hypervisor *must* retain > device state and driver just keeps using it. Right, so my question is how did the driver know the behaviour of a device? We usually do that via a feature bit. Note that the thing that matters here is the migration compatibility. > > > > > For example, PCI has no_soft_reset which has been done in the commit > > "virtio: Add support for no-reset virtio PCI PM". > > > > And there's a ongoing long discussion of adding suspend support in the > > virtio spec, then driver know it's safe to suspend/resume without > > reset. > > That's great! Hopefully it can fill the gap. > Currently, I think we can safely move the reset to drivers' freeze methods, > virtio core has no reason to take it as a common action required by all > devices. And the reset operation can be optional skipped if driver have > hints from device that it can retain state. The problem here is whether the device can be resumed without "soft reset" seems a general feature which could be either the knowledge of 1) virtio core (a feature bit or not) or 2) transport layer (like PCI) > > > > > > > > > Virtio GPIO is a typical example. GPIO states should be kept unchanged > > > after suspend and resume (e.g. output pins keep driving the output) and > > > Virtio GPIO driver does nothing in freeze and restore methods. But the > > > reset operation in virtio_device_restore breaks this. > > > > Is this mandated by GPIO or virtio spec? If yes, let's quote the revelant part. > > No. But in actual hardware design (e.g. Intel PCH GPIO), or from the > requirement perspective, GPIO pin state can be (should support) retained > in suspended state. > If Virtio GPIO is used to let VM operate such physical GPIO chip indirectly, > it can't be reset in suspend and resume. Meanwhile the hypervisor will > retain pin states after suspension. > > > > > > > > > Since some devices need reset in suspend and resume while some needn't, > > > create a new helper function for the original reset and status restore > > > logic so that virtio drivers can invoke it in their restore method > > > if necessary. > > > > How are those drivers classified? > > I think this depends whether hypervisor will keep devices state in platform > suspend process. So the problem is that the actual implementation (hypervisor, physical device or mediation) is transparent to the driver. Driver needs a general way to know whether it's safe (or not) to reset during the suspend/resume. > I think hypervisor should because suspend and reset are > conceptually two different things. Probably, but rest is and doing software state load/save is common practice for devices that will lose their state during PM. Thanks > > > Thanks > Qiang >
On Fri, Nov 01, 2024 at 10:11:11AM +0800, Jason Wang wrote: > On Fri, Nov 1, 2024 at 9:54 AM <qiang4.zhang@linux.intel.com> wrote: > > > > From: Qiang Zhang <qiang4.zhang@intel.com> > > > > Virtio core unconditionally reset and restore status for all virtio > > devices before calling restore method. This breaks some virtio drivers > > which don't need to do anything in suspend and resume because they > > just want to keep device state retained. > > The challenge is how can driver know device doesn't need rest. I actually don't remember why do we do reset on restore. Do you? > For example, PCI has no_soft_reset which has been done in the commit > "virtio: Add support for no-reset virtio PCI PM". > > And there's a ongoing long discussion of adding suspend support in the > virtio spec, then driver know it's safe to suspend/resume without > reset. > > > > > Virtio GPIO is a typical example. GPIO states should be kept unchanged > > after suspend and resume (e.g. output pins keep driving the output) and > > Virtio GPIO driver does nothing in freeze and restore methods. But the > > reset operation in virtio_device_restore breaks this. > > Is this mandated by GPIO or virtio spec? If yes, let's quote the revelant part. > > > > > Since some devices need reset in suspend and resume while some needn't, > > create a new helper function for the original reset and status restore > > logic so that virtio drivers can invoke it in their restore method > > if necessary. > > How are those drivers classified? > > > > > Signed-off-by: Qiang Zhang <qiang4.zhang@intel.com> > > Thanks
On Wed, Nov 6, 2024 at 5:29 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Nov 01, 2024 at 10:11:11AM +0800, Jason Wang wrote: > > On Fri, Nov 1, 2024 at 9:54 AM <qiang4.zhang@linux.intel.com> wrote: > > > > > > From: Qiang Zhang <qiang4.zhang@intel.com> > > > > > > Virtio core unconditionally reset and restore status for all virtio > > > devices before calling restore method. This breaks some virtio drivers > > > which don't need to do anything in suspend and resume because they > > > just want to keep device state retained. > > > > The challenge is how can driver know device doesn't need rest. > > I actually don't remember why do we do reset on restore. Do you? > Because the driver doesn't know if the device can keep its state, so it chooses to start from that. Thanks
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 194417abc105..bba7148bd219 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -1612,6 +1612,10 @@ static int virtblk_restore(struct virtio_device *vdev) struct virtio_blk *vblk = vdev->priv; int ret; + ret = virtio_device_reset_and_restore_status(vdev); + if (ret) + return ret; + ret = init_vq(vdev->priv); if (ret) return ret; diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c index dd998f4fe4f2..954280514f5a 100644 --- a/drivers/char/hw_random/virtio-rng.c +++ b/drivers/char/hw_random/virtio-rng.c @@ -218,6 +218,10 @@ static int virtrng_restore(struct virtio_device *vdev) { int err; + err = virtio_device_reset_and_restore_status(vdev); + if (err) + return err; + err = probe_common(vdev); if (!err) { struct virtrng_info *vi = vdev->priv; diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index c62b208b42f1..c431e4d5cd29 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -2132,6 +2132,10 @@ static int virtcons_restore(struct virtio_device *vdev) portdev = vdev->priv; + ret = virtio_device_reset_and_restore_status(vdev); + if (ret) + return ret; + ret = init_vqs(portdev); if (ret) return ret; diff --git a/drivers/crypto/virtio/virtio_crypto_core.c b/drivers/crypto/virtio/virtio_crypto_core.c index d0278eb568b9..b3363dd80448 100644 --- a/drivers/crypto/virtio/virtio_crypto_core.c +++ b/drivers/crypto/virtio/virtio_crypto_core.c @@ -536,6 +536,10 @@ static int virtcrypto_restore(struct virtio_device *vdev) struct virtio_crypto *vcrypto = vdev->priv; int err; + err = virtio_device_reset_and_restore_status(vdev); + if (err) + return err; + err = virtcrypto_init_vqs(vcrypto); if (err) return err; diff --git a/drivers/i2c/busses/i2c-virtio.c b/drivers/i2c/busses/i2c-virtio.c index 2a351f961b89..ce6493d6fe8e 100644 --- a/drivers/i2c/busses/i2c-virtio.c +++ b/drivers/i2c/busses/i2c-virtio.c @@ -251,6 +251,12 @@ static int virtio_i2c_freeze(struct virtio_device *vdev) static int virtio_i2c_restore(struct virtio_device *vdev) { + int ret; + + ret = virtio_device_reset_and_restore_status(vdev); + if (ret) + return ret; + return virtio_i2c_setup_vqs(vdev->priv); } diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 792e9eadbfc3..5be2a5f68f68 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -6693,6 +6693,10 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev) struct virtnet_info *vi = vdev->priv; int err; + err = virtio_device_reset_and_restore_status(vdev); + if (err) + return err; + err = virtnet_restore_up(vdev); if (err) return err; diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c index 8471f38b730e..3aeddf6331d3 100644 --- a/drivers/scsi/virtio_scsi.c +++ b/drivers/scsi/virtio_scsi.c @@ -1016,6 +1016,10 @@ static int virtscsi_restore(struct virtio_device *vdev) struct virtio_scsi *vscsi = shost_priv(sh); int err; + err = virtio_device_reset_and_restore_status(vdev); + if (err) + return err; + err = virtscsi_init(vdev, vscsi); if (err) return err; diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index b9095751e43b..55f80a61fb85 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -527,6 +527,41 @@ void unregister_virtio_device(struct virtio_device *dev) } EXPORT_SYMBOL_GPL(unregister_virtio_device); +int virtio_device_reset_and_restore_status(struct virtio_device *dev) +{ + struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); + int ret; + + /* We always start by resetting the device, in case a previous + * driver messed it up. */ + virtio_reset_device(dev); + + /* Acknowledge that we've seen the device. */ + virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); + + /* Maybe driver failed before freeze. + * Restore the failed status, for debugging. */ + if (dev->failed) + virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); + + if (!drv) + return 0; + + /* We have a driver! */ + virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); + + ret = dev->config->finalize_features(dev); + if (ret) + return ret; + + ret = virtio_features_ok(dev); + if (ret) + return ret; + + return 0; +} +EXPORT_SYMBOL_GPL(virtio_device_reset_and_restore_status); + #ifdef CONFIG_PM_SLEEP int virtio_device_freeze(struct virtio_device *dev) { @@ -554,44 +589,20 @@ int virtio_device_restore(struct virtio_device *dev) struct virtio_driver *drv = drv_to_virtio(dev->dev.driver); int ret; - /* We always start by resetting the device, in case a previous - * driver messed it up. */ - virtio_reset_device(dev); - - /* Acknowledge that we've seen the device. */ - virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE); - - /* Maybe driver failed before freeze. - * Restore the failed status, for debugging. */ - if (dev->failed) - virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED); - - if (!drv) - return 0; - - /* We have a driver! */ - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER); - - ret = dev->config->finalize_features(dev); - if (ret) - goto err; - - ret = virtio_features_ok(dev); - if (ret) - goto err; - - if (drv->restore) { - ret = drv->restore(dev); - if (ret) - goto err; + if (drv) { + if (drv->restore) { + ret = drv->restore(dev); + if (ret) + goto err; + } + + /* If restore didn't do it, mark device DRIVER_OK ourselves. */ + if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK)) + virtio_device_ready(dev); + + virtio_config_core_enable(dev); } - /* If restore didn't do it, mark device DRIVER_OK ourselves. */ - if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK)) - virtio_device_ready(dev); - - virtio_config_core_enable(dev); - return 0; err: diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index b36d2803674e..ba92b3e56391 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -1163,6 +1163,10 @@ static int virtballoon_restore(struct virtio_device *vdev) struct virtio_balloon *vb = vdev->priv; int ret; + ret = virtio_device_reset_and_restore_status(vdev); + if (ret) + return ret; + ret = init_vqs(vdev->priv); if (ret) return ret; diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c index a5d63269f20b..81c2ffd0e76e 100644 --- a/drivers/virtio/virtio_input.c +++ b/drivers/virtio/virtio_input.c @@ -374,6 +374,10 @@ static int virtinput_restore(struct virtio_device *vdev) struct virtio_input *vi = vdev->priv; int err; + err = virtio_device_reset_and_restore_status(vdev); + if (err) + return err; + err = virtinput_init_vqs(vi); if (err) return err; diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c index b0b871441578..47c23aa43c20 100644 --- a/drivers/virtio/virtio_mem.c +++ b/drivers/virtio/virtio_mem.c @@ -3018,6 +3018,10 @@ static int virtio_mem_restore(struct virtio_device *vdev) struct virtio_mem *vm = vdev->priv; int ret; + ret = virtio_device_reset_and_restore_status(vdev); + if (ret) + return ret; + ret = virtio_mem_init_vq(vm); if (ret) return ret; diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 306137a15d07..bce26597b8fc 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -178,6 +178,7 @@ int virtio_device_freeze(struct virtio_device *dev); int virtio_device_restore(struct virtio_device *dev); #endif void virtio_reset_device(struct virtio_device *dev); +int virtio_device_reset_and_restore_status(struct virtio_device *dev); size_t virtio_max_dma_size(const struct virtio_device *vdev); diff --git a/sound/virtio/virtio_card.c b/sound/virtio/virtio_card.c index 965209e1d872..3c7a6d76c46c 100644 --- a/sound/virtio/virtio_card.c +++ b/sound/virtio/virtio_card.c @@ -412,6 +412,10 @@ static int virtsnd_restore(struct virtio_device *vdev) struct virtio_snd *snd = vdev->priv; int rc; + rc = virtio_device_reset_and_restore_status(vdev); + if (rc) + return rc; + rc = virtsnd_find_vqs(snd); if (rc) return rc;