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

[Chen, Jiqian]

Hi Lingshan,

On 2023/9/20 14:58, Zhu, Lingshan wrote:
> 
> 
> On 9/20/2023 2:33 PM, Chen, Jiqian wrote:
>> Hi Lingshan,
>>
>> On 2023/9/20 13:59, Zhu, Lingshan wrote:
>>>
>>> On 9/19/2023 8:31 PM, Michael S. Tsirkin wrote:
>>>> On Tue, Sep 19, 2023 at 07:42:42PM +0800, Jiqian Chen 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.
>>> Hi Jiqian,
>>>
>>> Have you seen this series: [PATCH 0/5] virtio: introduce SUSPEND bit and vq 
>>> state
>>> https://lore.kernel.org/all/3f4cbf84-010c-cffa-0b70-33c449b5561b@intel.com/T/
>>>
>>> We introduced a bit in the device status SUSPEND, when VIRTIO_F_SUSPEND is
>>> negotiated, the driver can set SUSPEND in the device status to suspend the
>>> device.
>>>
>>> When SUSPEND, the device should pause its operations and preserve its 
>>> configurations
>>> in its configuration space.
>>>
>>> The driver re-write DRIVER_OK to clear SUSPEND, so the device resumes 
>>> running.
>>>
>>> This is originally to serve live migration, but I think it can also meet 
>>> your needs.
>> I noticed your series, but I am not sure they are also meet my needs.
>> If driver write 0 to reset device, can the SUSPEND bit be cleared? 
>> (pci_pm_resume->virtio_pci_restore->virtio_device_restore->virtio_reset_device)
> if the driver writes 0, it resets all virtio functionalities. So SUSPEND is 
> cleared.
Then your patches are not meet my needs. In my scene, it needs to keep the 
SUSPEND bit util the resume process is complete.
Because in my virtio-gpu scene, when guest resume, it call virtio_reset_device 
to clear all device status bits, and then reset virtio-gpu in Qemu, and then 
destroy render resources, I don't want the resources are destroyed during the 
resume process. So, I add freeze_mode to tell Qemu that guest is doing S3 and 
resources need to be kept.

> device reset can also be used to recover the device from fatal errors, so it 
> should reset everything in virtio.
>> If SUSPEND is cleared, then during the reset process in Qemu, I can't judge 
>> if the reset request is from guest restore process or not, and then I can't 
>> change the reset behavior.
> I think when enter S3, the hypervisor/driver should set SUSPEND to the 
> device. And when resume from S3, the hypervisor/driver should
> re-write DRIVER_OK to clear SUSPEND, then the device resume running.
>> Can you send me your patch link on kernel and qemu side? I will take a deep 
>> look.
> There are no patches for qemu/kernel yet, spec first.
>>
>>> Thanks,
>>> Zhu Lingshan
>>>>> Signed-off-by: Jiqian Chen <Jiqian.Chen@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 */
>>>>>
>>>> we can't add fields in the middle of the structure like this -
>>>> offset of queue_notif_config_data and queue_reset changes.
>>>>
>>>>   
>>>>> @@ -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.
>>>>> +
>>>> we need to specify these values then.
>>>>
>>>> we also need
>>>> - feature bit to detect support for S3
>>>> - conformance statements documenting behavious under S3
>>>>
>>>>
>>>>>    \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
>>>> This publicly archived list offers a means to provide input to the
>>>> OASIS Virtual I/O Device (VIRTIO) TC.
>>>>
>>>> In order to verify user consent to the Feedback License terms and
>>>> to minimize spam in the list archive, subscription is required
>>>> before posting.
>>>>
>>>> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
>>>> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
>>>> List help: virtio-comment-help@lists.oasis-open.org
>>>> List archive: https://lists.oasis-open.org/archives/virtio-comment/
>>>> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
>>>> List Guidelines: 
>>>> https://www.oasis-open.org/policies-guidelines/mailing-lists
>>>> Committee: https://www.oasis-open.org/committees/virtio/
>>>> Join OASIS: https://www.oasis-open.org/join/
>>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>>
> 
> 
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
> 
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
> 
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
> 

-- 
Best regards,
Jiqian Chen.