Message-ID: <ZyRlC-5V_NTKgzXh@dev-qz>
Date: Fri, 1 Nov 2024 13:20:11 +0800
From: Qiang Zhang <qiang4.zhang@...ux.intel.com>
To: Jason Wang <jasowang@...hat.com>
Cc: "Michael S. Tsirkin" <mst@...hat.com>,
	Paolo Bonzini <pbonzini@...hat.com>,
	Stefan Hajnoczi <stefanha@...hat.com>,
	Eugenio Pérez <eperezma@...hat.com>,
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
	Jens Axboe <axboe@...nel.dk>, Olivia Mackall <olivia@...enic.com>,
	Herbert Xu <herbert@...dor.apana.org.au>,
	Amit Shah <amit@...nel.org>, Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Gonglei <arei.gonglei@...wei.com>,
	"David S. Miller" <davem@...emloft.net>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	"Chen, Jian Jun" <jian.jun.chen@...el.com>,
	Andi Shyti <andi.shyti@...nel.org>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	"James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
	"Martin K. Petersen" <martin.petersen@...cle.com>,
	David Hildenbrand <david@...hat.com>,
	Gerd Hoffmann <kraxel@...hat.com>,
	Anton Yakovlev <anton.yakovlev@...nsynergy.com>,
	Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
	Qiang Zhang <qiang4.zhang@...el.com>,
	virtualization@...ts.linux.dev, linux-block@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
	linux-i2c@...r.kernel.org, netdev@...r.kernel.org,
	linux-scsi@...r.kernel.org, linux-sound@...r.kernel.org
Subject: Re: [PATCH v2] virtio: only reset device and restore status if
 needed in device resume

On Fri, Nov 01, 2024 at 10:11:11AM +0800, Jason Wang wrote:
> On Fri, Nov 1, 2024 at 9:54 AM <qiang4.zhang@...ux.intel.com> wrote:
> >
> > From: Qiang Zhang <qiang4.zhang@...el.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

Powered by blists - more mailing lists