Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Date:   Tue, 12 Dec 2023 11:13:46 -0500
From:   "Michael S. Tsirkin" <mst@redhat.com>
To:     Jason Wang <jasowang@redhat.com>
Cc:     David Stevens <stevensd@chromium.org>,
        virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] virtio: Add support for no-reset virtio PCI PM
Message-ID: <20231212111323-mutt-send-email-mst@kernel.org>
References: <20231208070754.3132339-1-stevensd@chromium.org>
 <20231211113710-mutt-send-email-mst@kernel.org>
 <CACGkMEs_MajTFxGVcK5R8oqQzTxuL4Pm=uUnOonDczWzqaucsw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CACGkMEs_MajTFxGVcK5R8oqQzTxuL4Pm=uUnOonDczWzqaucsw@mail.gmail.com>
Precedence: bulk

On Tue, Dec 12, 2023 at 11:01:53AM +0800, Jason Wang wrote:
> On Tue, Dec 12, 2023 at 12:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Dec 08, 2023 at 04:07:54PM +0900, David Stevens wrote:
> > > If a virtio_pci_device supports native PCI power management and has the
> > > No_Soft_Reset bit set, then skip resetting and reinitializing the device
> > > when suspending and restoring the device. This allows system-wide low
> > > power states like s2idle to be used in systems with stateful virtio
> > > devices that can't simply be re-initialized (e.g. virtio-fs).
> > >
> > > Signed-off-by: David Stevens <stevensd@chromium.org>
> >
> > tagged, thanks!
> > I'm still debating with myself whether we can classify this
> > as a bugfix ... better not risk it I guess.
> 
> It might be suitable if there's a hypervisor that advertises
> no_soft_reset (but it seems Qemu doesn't).
> 
> Thanks

Yea... so a bugfix but no rush to merge it I think.

> >
> > > ---
> > > v1 -> v2:
> > >  - Check the No_Soft_Reset bit
> > >
> > >  drivers/virtio/virtio_pci_common.c | 34 +++++++++++++++++++++++++++++-
> > >  1 file changed, 33 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > > index c2524a7207cf..3a95ecaf12dc 100644
> > > --- a/drivers/virtio/virtio_pci_common.c
> > > +++ b/drivers/virtio/virtio_pci_common.c
> > > @@ -492,8 +492,40 @@ static int virtio_pci_restore(struct device *dev)
> > >       return virtio_device_restore(&vp_dev->vdev);
> > >  }
> > >
> > > +static bool vp_supports_pm_no_reset(struct device *dev)
> > > +{
> > > +     struct pci_dev *pci_dev = to_pci_dev(dev);
> > > +     u16 pmcsr;
> > > +
> > > +     if (!pci_dev->pm_cap)
> > > +             return false;
> > > +
> > > +     pci_read_config_word(pci_dev, pci_dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> > > +     if (PCI_POSSIBLE_ERROR(pmcsr)) {
> > > +             dev_err(dev, "Unable to query pmcsr");
> > > +             return false;
> > > +     }
> > > +
> > > +     return pmcsr & PCI_PM_CTRL_NO_SOFT_RESET;
> > > +}
> > > +
> > > +static int virtio_pci_suspend(struct device *dev)
> > > +{
> > > +     return vp_supports_pm_no_reset(dev) ? 0 : virtio_pci_freeze(dev);
> > > +}
> > > +
> > > +static int virtio_pci_resume(struct device *dev)
> > > +{
> > > +     return vp_supports_pm_no_reset(dev) ? 0 : virtio_pci_restore(dev);
> > > +}
> > > +
> > >  static const struct dev_pm_ops virtio_pci_pm_ops = {
> > > -     SET_SYSTEM_SLEEP_PM_OPS(virtio_pci_freeze, virtio_pci_restore)
> > > +     .suspend = virtio_pci_suspend,
> > > +     .resume = virtio_pci_resume,
> > > +     .freeze = virtio_pci_freeze,
> > > +     .thaw = virtio_pci_restore,
> > > +     .poweroff = virtio_pci_freeze,
> > > +     .restore = virtio_pci_restore,
> > >  };
> > >  #endif
> > >
> > > --
> > > 2.43.0.472.g3155946c3a-goog
> >