Hi maintainers, Are there any other concerns I should address for this patch? On Mon, Sep 9, 2024 at 4:52 PM Philip Chen <philipchen@xxxxxxxxxxxx> wrote: > > 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 > > > > > > > >
