On Tue, Sep 19, 2023 at 12:10:29PM +0000, Parav Pandit wrote:
> Hi Jiqian,
>
> > From: Jiqian Chen <jiqian.c...@amd.com>
> > Sent: Tuesday, September 19, 2023 5:13 PM
> >
> > When guest vm does S3, Qemu will reset and clear some things of virtio
> > devices, but guest can't aware that, so that may cause some problems.
> It is not true that guest VM is not aware of it.
> As you show in your kernel patch, it is freeze/unfreeze in the guest VM PCI
> PM driver callback. So please update the commit log.
>
> > For excample, Qemu calls virtio_reset->virtio_gpu_gl_reset when guest
> s/excample/example
>
> > resume, that function will destroy render resources of virtio-gpu. As a
> > result,
> > after guest resume, the display can't come back and we only saw a black
> > screen. Due to guest can't re-create all the resources, so we need to let
> > Qemu
> > not to destroy them when S3.
> Above QEMU specific details to go in cover letter, instead of commit log, but
> no strong opinion.
i feel it does not matter much.
> Explaining the use case is good.
>
> >
> > For above purpose, we need a mechanism that allows guests and QEMU to
> > negotiate their reset behavior. So this patch add a new parameter named
> Freeze != reset. :)
> Please fix it to say freeze or suspend.
>
> > freeze_mode to struct virtio_pci_common_cfg. And when guest suspends, it can
> > write freeze_mode to be FREEZE_S3, and then virtio devices can change their
> > reset behavior on Qemu side according to freeze_mode. What's more,
> Not reset, but suspend behavior.
>
> > freeze_mode can be used for all virtio devices to affect the behavior of
> > Qemu,
> > not just virtio gpu device.
> >
> > Signed-off-by: Jiqian Chen <jiqian.c...@amd.com>
> > ---
> > transport-pci.tex | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/transport-pci.tex b/transport-pci.tex index a5c6719..2543536
> > 100644
> > --- a/transport-pci.tex
> > +++ b/transport-pci.tex
> > @@ -319,6 +319,7 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport
> > le64 queue_desc; /* read-write */
> > le64 queue_driver; /* read-write */
> > le64 queue_device; /* read-write */
> > + le16 freeze_mode; /* read-write */
> > le16 queue_notif_config_data; /* read-only for driver */
> > le16 queue_reset; /* read-write */
> >
> The new field cannot be in the middle of the structure.
> Otherwise, the location of the queue_notif_config_data depends on completely
> unrelated feature bit, breaking the backward compatibility.
> So please move it at the end.
>
> > @@ -393,6 +394,12 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport \item[\field{queue_device}]
> > The driver writes the physical address of Device Area here. See
> > section
> > \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}.
> >
> > +\item[\field{freeze_mode}]
> > + The driver writes this to set the freeze mode of virtio pci.
> > + VIRTIO_PCI_FREEZE_MODE_UNFREEZE - virtio-pci is running;
> > + VIRTIO_PCI_FREEZE_MODE_FREEZE_S3 - guest vm is doing S3, and
> > virtio-
> For above names, please define the actual values in the spec.
>
> > pci enters S3 suspension;
> > + Other values are reserved for future use, like S4, etc.
> > +
> It cannot be just one way communication from driver to device as freezing the
> device of few hundred MB to GB of gpu memory or other device memory can take
> several msec.
> Hence driver must poll to get the acknowledgement from the device that freeze
> functionality is completed.
>
> Please refer to queue_reset register definition for achieving such scheme and
> reframe the wording for it.
>
> Also kindly add the device and driver normative on how/when this register is
> accessed.
>
> Also please fix the description to not talk about guest VM. Largely it only
> exists in theory of operation etc text.
>
> You need to describe what exactly should happen in the device when its freeze.
> Please refer to my series where infrastructure is added for device migration
> where the FREEZE mode behavior is defined.
> It is similar to what you define, but its management plane operation
> controlled outside of the guest VM.
> But it is good direction in terms of what to define in spec language.
> https://lore.kernel.org/virtio-comment/20230909142911.524407-7-pa...@nvidia.com/T/#u
>
> you are missing the feature bit to indicate to the driver that device
> supports this functionality.
> Please add one.
>
> > \item[\field{queue_notif_config_data}]
> > This field exists only if VIRTIO_F_NOTIF_CONFIG_DATA has been
> > negotiated.
> > The driver will use this value when driver sends available buffer
> > --
> > 2.34.1
