2023-09-19 14:09:50

by Jiqian Chen

[permalink] [raw]
Subject: [VIRTIO PCI PATCH v5 1/1] transport-pci: Add freeze_mode to virtio_pci_common_cfg

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.
For excample, Qemu calls virtio_reset->virtio_gpu_gl_reset when guest
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.

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_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, 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 <[email protected]>
---
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 */

@@ -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-pci enters S3 suspension;
+ Other values are reserved for future use, like S4, etc.
+
\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


2023-09-19 16:16:06

by Parav Pandit

[permalink] [raw]
Subject: RE: [VIRTIO PCI PATCH v5 1/1] transport-pci: Add freeze_mode to virtio_pci_common_cfg

Hi Jiqian,

> From: Jiqian Chen <[email protected]>
> 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.
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 <[email protected]>
> ---
> 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/[email protected]/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

2023-09-20 02:26:32

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [VIRTIO PCI PATCH v5 1/1] transport-pci: Add freeze_mode to virtio_pci_common_cfg

On Tue, Sep 19, 2023 at 12:10:29PM +0000, Parav Pandit wrote:
> Hi Jiqian,
>
> > From: Jiqian Chen <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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/[email protected]/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

2023-09-21 04:56:57

by Jiqian Chen

[permalink] [raw]
Subject: Re: [virtio-dev] RE: [VIRTIO PCI PATCH v5 1/1] transport-pci: Add freeze_mode to virtio_pci_common_cfg

Hi Parav,

On 2023/9/19 20:10, Parav Pandit wrote:
> Hi Jiqian,
>
>> From: Jiqian Chen <[email protected]>
>> 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.
Thanks, I will update it.

>
>> 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.
> Explaining the use case is good.
Thanks, I will also add it to cover letter.

>
>>
>> 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.
But in my virtio-gpu scene, I want to prevent Qemu destroying resources when Guest do resuming(pci_pm_resume-> virtio_pci_restore-> virtio_device_restore-> virtio_reset_device-> vp_modern_set_status->Qemu virtio_pci_reset->virtio_gpu_gl_reset-> virtio_gpu_reset). And I add check in virtio_gpu_gl_reset and virtio_gpu_reset, if freeze_mode was set to FREEZE_S3 during Guest suspending, Qemu will not destroy resources. So the reason why I add this mechanism is to affect the reset behavior. And I think this also can help other virtio devices to affect their behavior, like the issue of virtio-video which Mikhail Golubev-Ciuchea encountered.

>
>> 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.
The same reason as above.

>
>> 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 <[email protected]>
>> ---
>> 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.
I have confused about this. I found in latest kernel code(master branch):
struct virtio_pci_common_cfg {
/* About the whole device. */
__le32 device_feature_select; /* read-write */
__le32 device_feature; /* read-only */
__le32 guest_feature_select; /* read-write */
__le32 guest_feature; /* read-write */
__le16 msix_config; /* read-write */
__le16 num_queues; /* read-only */
__u8 device_status; /* read-write */
__u8 config_generation; /* read-only */

/* About a specific virtqueue. */
__le16 queue_select; /* read-write */
__le16 queue_size; /* read-write, power of 2. */
__le16 queue_msix_vector; /* read-write */
__le16 queue_enable; /* read-write */
__le16 queue_notify_off; /* read-only */
__le32 queue_desc_lo; /* read-write */
__le32 queue_desc_hi; /* read-write */
__le32 queue_avail_lo; /* read-write */
__le32 queue_avail_hi; /* read-write */
__le32 queue_used_lo; /* read-write */
__le32 queue_used_hi; /* read-write */

__le16 freeze_mode; /* read-write */
};
There is no queue_notif_config_data or queue_reset, and freeze_mode I added is at the end. Why is it different from virtio-spec?

>
>> @@ -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.
Ok, I will add them.

>
>> 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.
I think the freeze functionality itself has not many problems. My patches just want to tell Qemu that the reset request is from the process of guest resuming not other scene, and write a status into freeze_mode, then we can change the reset behavior during guest resuming.

>
> 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.
Thanks, I will add them.

>
> Also please fix the description to not talk about guest VM. Largely it only exists in theory of operation etc text.
Thanks, I will change it.

>
> 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/[email protected]/T/#u
Thank you very much for your suggestion. I will refer to your link and then modify my description.

>
> you are missing the feature bit to indicate to the driver that device supports this functionality.
> Please add one.
Do I need to add feature bit to DEFINE_VIRTIO_COMMON_FEATURES? And each time when I write freeze_mode filed on kernel driver side, I need to check this bit?

>
>> \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
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>

--
Best regards,
Jiqian Chen.

2023-09-21 17:23:28

by Jason Wang

[permalink] [raw]
Subject: Re: [VIRTIO PCI PATCH v5 1/1] transport-pci: Add freeze_mode to virtio_pci_common_cfg

On Tue, Sep 19, 2023 at 7:43 PM Jiqian Chen <[email protected]> wrote:
>
> 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.
> For excample, Qemu calls virtio_reset->virtio_gpu_gl_reset when guest
> 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.
>
> 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_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, freeze_mode can be used for all virtio
> devices to affect the behavior of Qemu, not just virtio gpu device.

A simple question, why is this issue specific to pci?

Thanks


>
> Signed-off-by: Jiqian Chen <[email protected]>
> ---
> 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 */
>
> @@ -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-pci enters S3 suspension;
> + Other values are reserved for future use, like S4, etc.
> +
> \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
>

2023-09-22 01:17:28

by Jiqian Chen

[permalink] [raw]
Subject: Re: [virtio-dev] Re: [VIRTIO PCI PATCH v5 1/1] transport-pci: Add freeze_mode to virtio_pci_common_cfg

Hi Jason,

On 2023/9/21 12:22, Jason Wang wrote:
> On Tue, Sep 19, 2023 at 7:43 PM Jiqian Chen <[email protected]> wrote:
>>
>> 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.
>> For excample, Qemu calls virtio_reset->virtio_gpu_gl_reset when guest
>> 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.
>>
>> 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_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, freeze_mode can be used for all virtio
>> devices to affect the behavior of Qemu, not just virtio gpu device.
>
> A simple question, why is this issue specific to pci?
I thought you possibly missed the previous version patches. At the beginning, I just wanted to add a new feature flag VIRTIO_GPU_F_FREEZE_S3 for virtio-gpu since I encountered virtio-gpu issue during guest S3, so that the guest and qemu can negotiate and change the reset behavior during S3. But Parav and Mikhail hoped me can improve the feature to a pci level, then other virtio devices can also benefit from it. Although I am not sure if expanding its influence is appropriate, I have not received any feedback from others, so I change it to the pci level and made this version.
If you are interested, please see the previous version: https://lists.oasis-open.org/archives/virtio-comment/202307/msg00209.html, thank you.

>
> Thanks
>
>
>>
>> Signed-off-by: Jiqian Chen <[email protected]>
>> ---
>> 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 */
>>
>> @@ -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-pci enters S3 suspension;
>> + Other values are reserved for future use, like S4, etc.
>> +
>> \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
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>

--
Best regards,
Jiqian Chen.

2023-09-22 07:26:39

by Jason Wang

[permalink] [raw]
Subject: Re: [virtio-dev] Re: [VIRTIO PCI PATCH v5 1/1] transport-pci: Add freeze_mode to virtio_pci_common_cfg

On Thu, Sep 21, 2023 at 2:28 PM Chen, Jiqian <[email protected]> wrote:
>
> Hi Jason,
>
> On 2023/9/21 12:22, Jason Wang wrote:
> > On Tue, Sep 19, 2023 at 7:43 PM Jiqian Chen <[email protected]> wrote:
> >>
> >> 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.
> >> For excample, Qemu calls virtio_reset->virtio_gpu_gl_reset when guest
> >> 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.
> >>
> >> 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_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, freeze_mode can be used for all virtio
> >> devices to affect the behavior of Qemu, not just virtio gpu device.
> >
> > A simple question, why is this issue specific to pci?
> I thought you possibly missed the previous version patches. At the beginning, I just wanted to add a new feature flag VIRTIO_GPU_F_FREEZE_S3 for virtio-gpu since I encountered virtio-gpu issue during guest S3, so that the guest and qemu can negotiate and change the reset behavior during S3. But Parav and Mikhail hoped me can improve the feature to a pci level, then other virtio devices can also benefit from it. Although I am not sure if expanding its influence is appropriate, I have not received any feedback from others, so I change it to the pci level and made this version.
> If you are interested, please see the previous version: https://lists.oasis-open.org/archives/virtio-comment/202307/msg00209.html, thank you.

This is not a good answer. Let me ask you differently, why don't you
see it in other forms of transport like virtio-gpu-mmio?

Thanks

>
> >
> > Thanks
> >
> >
> >>
> >> Signed-off-by: Jiqian Chen <[email protected]>
> >> ---
> >> 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 */
> >>
> >> @@ -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-pci enters S3 suspension;
> >> + Other values are reserved for future use, like S4, etc.
> >> +
> >> \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
> >>
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [email protected]
> > For additional commands, e-mail: [email protected]
> >
>
> --
> Best regards,
> Jiqian Chen.