Re: [virtio-comment] [PATCH v3 1/1] Define a low power mode for devices

[Jason Wang <jasowang@redhat.com>]

On Thu, Dec 7, 2023 at 12:55âPM David Stevens <stevensd@chromium.org> wrote:
>
> On Thu, Dec 7, 2023 at 1:16âPM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Dec 6, 2023 at 6:17âPM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Dec 06, 2023 at 05:16:04PM +0800, Jason Wang wrote:
> > > > On Tue, Dec 5, 2023 at 6:58âPM David Stevens <stevensd@chromium.org> wrote:
> > > > >
> > > > > On Tue, Dec 5, 2023 at 1:18âPM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Mon, Dec 4, 2023 at 5:41âPM David Stevens <stevensd@chromium.org> wrote:
> > > > > > >
> > > > > > > Define a low power mode for virtio devices where the devices are
> > > > > > > expected to maintain their state. This gives drivers an option for power
> > > > > > > management besides simply resetting their device. In the virtualization
> > > > > > > use case, this allows the guest to be suspended even with stateful
> > > > > > > virtio devices like gpu and fs.
> > > > > > >
> > > > > > > Low power mode is primarily defined at the transport layer. The only
> > > > > > > part that depends on device-type specific details is whether a given
> > > > > > > virtqueue is device driven or driver driven.
> > > > > > >
> > > > > > > This change only defines the transport-specific implementation for
> > > > > > > Virtio over PCI.
> > > > > >
> > > > > > A dumb question, if this is only for PCI, can the device just
> > > > > > implement no_soft_reset via PMC?
> > > > >
> > > > > This is basically No_Soft_Reset, yes. If a change similar to [1] would
> > > > > be acceptable based only on the No_Soft_Reset bit even with no concept
> > > > > of power management in the virtio spec, then I personally don't have
> > > > > any problems with that.
> > > > >
> > > > > [1] https://lore.kernel.org/lkml/20231113055138.117392-1-stevensd@chromium.org/
> > > >
> > > > So if I read the code correctly, the current Qemu advertises PM but
> > > > without no_soft_reset.
> > > >
> > > > So this patch seems to break e.g virtio-net and doesn't fix virtio-GPU.
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > -David
> > > > >
> > >
> > > what is the breakage exactly?
> >
> > It looks to me we need:
> >
> > 1) Device side, if device can preserve state on d3hot, it may
> > advertise no_soft_reset
> > 2) Driver side, if driver sees no_soft_reset, it doesn't need to
> > manually suspend and resume. But in the above patch, it only checks
> > the existence of PM, it looks to me it needs to check no_soft_reset
> > instead.
>
> Right, my original reply was needlessly vague. Similar to the linked
> patch, but based on checking No_Soft_Reset instead of just pm_cap.

Right, so I think I'm fine if it works like this.

>
> That also raises the point that the proposed spec changes are also
> insufficient - the PCI section needs to say that setting No_Soft_Reset
> is required for low power mode.

Yes, but I think we probably don't need the spec patch.

Thanks

>
> -David
>