Hi On Wed, Sep 4, 2024 at 10:54 AM Ira Weiny <ira.weiny@xxxxxxxxx> wrote: > > Philip Chen wrote: > > Hi maintainers, > > > > Can anyone let me know if this patch makes sense? > > Any comment/feedback is appreciated. > > Thanks in advance! > > I'm not an expert on virtio but the code looks ok on the surface. I've > discussed this with Dan a bit and virtio-pmem is not heavily tested. Thanks for your comments. I think this specific patch is not heavily involved with virtio spec details. This patch simply provides the basic freeze/restore PM callbacks for virtio_pmem, like people already did for the other virtio drivers. > > Based on our discussion [1] I wonder if there is a way we can recreate this > with QEMU to incorporate into our testing? Yes, these are how I test on crosvm, but I believe the same steps can be applied to QEMU: (1) Set pm_test_level to TEST_PLATFORM (build time change) (2) Write something to pmem (3) Make the device go through a freeze/restore cycle by writing `disk` to `/sys/power/state` (4) Validate the data written to pmem in (2) is still preserved Note: (a) The freeze/restore PM routines are sometimes called as the backup for suspend/resume PM routines in a suspend/resume cycle. In this case, we can also test freeze/restore PM routines with suspend/resume: i.e. skip (1) and write `mem` to `sys/power/state` in (3). (b) I also tried to set up QEMU for testing. But QEMU crashes when I try to freeze the device even without applying this patch. Since the issue seems to be irrelevant to pmem and most likely a QEMU setup problem on my end, I didn't spend more time enabling QEMU. > > Ira > > [1] https://lore.kernel.org/lkml/CA+cxXhnb2i5O7_BiOfKLth5Zwp5T62d6F6c39vnuT53cUkU_uw@xxxxxxxxxxxxxx/ > > > > > On Wed, Aug 14, 2024 at 5:46 PM Philip Chen <philipchen@xxxxxxxxxxxx> wrote: > > > > > > Add basic freeze/restore PM callbacks to support hibernation (S4): > > > - On freeze, delete vq and quiesce the device to prepare for > > > snapshotting. > > > - On restore, re-init vq and mark DRIVER_OK. > > > > > > Signed-off-by: Philip Chen <philipchen@xxxxxxxxxxxx> > > > --- > > > drivers/nvdimm/virtio_pmem.c | 24 ++++++++++++++++++++++++ > > > 1 file changed, 24 insertions(+) > > > > > > diff --git a/drivers/nvdimm/virtio_pmem.c b/drivers/nvdimm/virtio_pmem.c > > > index c9b97aeabf85..2396d19ce549 100644 > > > --- a/drivers/nvdimm/virtio_pmem.c > > > +++ b/drivers/nvdimm/virtio_pmem.c > > > @@ -143,6 +143,28 @@ static void virtio_pmem_remove(struct virtio_device *vdev) > > > virtio_reset_device(vdev); > > > } > > > > > > +static int virtio_pmem_freeze(struct virtio_device *vdev) > > > +{ > > > + vdev->config->del_vqs(vdev); > > > + virtio_reset_device(vdev); > > > + > > > + return 0; > > > +} > > > + > > > +static int virtio_pmem_restore(struct virtio_device *vdev) > > > +{ > > > + int ret; > > > + > > > + ret = init_vq(vdev->priv); > > > + if (ret) { > > > + dev_err(&vdev->dev, "failed to initialize virtio pmem's vq\n"); > > > + return ret; > > > + } > > > + virtio_device_ready(vdev); > > > + > > > + return 0; > > > +} > > > + > > > static unsigned int features[] = { > > > VIRTIO_PMEM_F_SHMEM_REGION, > > > }; > > > @@ -155,6 +177,8 @@ static struct virtio_driver virtio_pmem_driver = { > > > .validate = virtio_pmem_validate, > > > .probe = virtio_pmem_probe, > > > .remove = virtio_pmem_remove, > > > + .freeze = virtio_pmem_freeze, > > > + .restore = virtio_pmem_restore, > > > }; > > > > > > module_virtio_driver(virtio_pmem_driver); > > > -- > > > 2.46.0.76.ge559c4bf1a-goog > > > > >
