Re: [PATCH v6 1/1] content: Add new feature VIRTIO_F_PRESERVE_RESOURCES

["Chen, Jiqian" <Jiqian.Chen@amd.com>]

On 2023/10/23 14:00, Parav Pandit wrote:
> Hi Jiqian,
> 
>> From: Jiqian Chen <Jiqian.Chen@amd.com>
>> Sent: Saturday, October 21, 2023 9:21 AM
>> To: Michael S . Tsirkin <mst@redhat.com>; Gerd Hoffmann
>> <kraxel@redhat.com>; Parav Pandit <parav@nvidia.com>; Jason Wang
>> <jasowang@redhat.com>; Xuan Zhuo <xuanzhuo@linux.alibaba.com>; David
>> Airlie <airlied@redhat.com>; Gurchetan Singh
>> <gurchetansingh@chromium.org>; Chia-I Wu <olvaffe@gmail.com>; Marc-
>> Andrà Lureau <marcandre.lureau@gmail.com>; Robert Beckett
>> <bob.beckett@collabora.com>; Mikhail Golubev-Ciuchea <Mikhail.Golubev-
>> Ciuchea@opensynergy.com>; virtio-comment@lists.oasis-open.org; virtio-
>> dev@lists.oasis-open.org
>> Cc: Honglei Huang <Honglei1.Huang@amd.com>; Julia Zhang
>> <Julia.Zhang@amd.com>; Huang Rui <Ray.Huang@amd.com>; Jiqian Chen
>> <Jiqian.Chen@amd.com>
>> Subject: [PATCH v6 1/1] content: Add new feature
>> VIRTIO_F_PRESERVE_RESOURCES
>>
>> In some scenes, Qemu may reset or destroy resources of virtio device, but some
>> of them can't be re-created, so that causes some problems.
>>
> It can be re-created. It is just that the guest driver lost the previous resource values.
> So a better wording for the motivation is combining with below line to me is,
Yes, guest driver hasn't enough information or values of resources to re-create them. I will change my description in next version.

> 
> Currently guest drivers destroy and re-create virtio resources during power management suspend/resume sequence respectively.
> For example, for a PCI transport, even if the device offers D3 to d0 state transition, a virtio guest driver has no way to know if 
> the device can store the state during D0->D3 transition and same state can be restored on D3->D0 transition.
Thanks, will change in next version.

> 
>> For example, when we do S3 for guest, guest will set device_status to 0, it
>> causes Qemu to reset virtioi-gpu device, and then all render resources of virtio-
>> gpu will be destroyed. As a result, after guest resuming, the display can't come
>> back, and we only see a black screen.
>>
> To make guest drivers aware of above device capability, introduce a new feature bit to indicate such device capability.
> This patch adds...
Thanks, will change in next version.

> 
>> In order to deal with the above scene, we need a mechanism that allows guest
>> and Qemu to negotiate their behaviors for resources. So, this patch adds a new
>> feature named VIRTIO_F_PRESERVE_RESOURCES. It allows guest to tell Qemu
>> when there is a need to preserve resources, guest must preserve resources until
>> 0 is set.
>>
> I think this can be done without introducing the new register.
> Can you please check if the PM register itself can serve the purpose instead of new virtio level register?
Do you mean the system PM register? I think it is unreasonable to let virtio-device listen the PM state of Guest system. It's more suitable that each device gets notifications from driver, and then do preserving resources operation.

> 
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
>> ---
>>  conformance.tex   |  2 ++
>>  content.tex       | 25 +++++++++++++++++++++++++
>>  transport-pci.tex |  6 ++++++
>>  3 files changed, 33 insertions(+)
>>
>> diff --git a/conformance.tex b/conformance.tex index dc00e84..60cc0b1
>> 100644
>> --- a/conformance.tex
>> +++ b/conformance.tex
>> @@ -91,6 +91,7 @@ \section{Conformance Targets}\label{sec:Conformance /
>> Conformance Targets}  \item \ref{drivernormative:Basic Facilities of a Virtio
>> Device / Packed Virtqueues / The Virtqueue Descriptor Table / Indirect
>> Descriptors}  \item \ref{drivernormative:Basic Facilities of a Virtio Device /
>> Packed Virtqueues / Supplying Buffers to The Device / Updating flags}  \item
>> \ref{drivernormative:Basic Facilities of a Virtio Device / Packed Virtqueues /
>> Supplying Buffers to The Device / Sending Available Buffer Notifications}
>> +\item \ref{drivernormative:Basic Facilities of a Virtio Device /
>> +Preserve Resources}
>>  \item \ref{drivernormative:General Initialization And Device Operation /
>> Device Initialization}  \item \ref{drivernormative:General Initialization And
>> Device Operation / Device Cleanup}  \item \ref{drivernormative:Reserved
>> Feature Bits} @@ -172,6 +173,7 @@ \section{Conformance
>> Targets}\label{sec:Conformance / Conformance Targets}  \item
>> \ref{devicenormative:Basic Facilities of a Virtio Device / Packed Virtqueues /
>> The Virtqueue Descriptor Table}  \item \ref{devicenormative:Basic Facilities of
>> a Virtio Device / Packed Virtqueues / Scatter-Gather Support}  \item
>> \ref{devicenormative:Basic Facilities of a Virtio Device / Shared Memory
>> Regions}
>> +\item \ref{devicenormative:Basic Facilities of a Virtio Device /
>> +Preserve Resources}
>>  \item \ref{devicenormative:Reserved Feature Bits}  \end{itemize}
>>
>> diff --git a/content.tex b/content.tex
>> index 0a62dce..b6b1859 100644
>> --- a/content.tex
>> +++ b/content.tex
>> @@ -502,6 +502,27 @@ \section{Exporting Objects}\label{sec:Basic Facilities
>> of a Virtio Device / Expo  types. It is RECOMMENDED that devices generate
>> version 4  UUIDs as specified by \hyperref[intro:rfc4122]{[RFC4122]}.
>>
>> +\section{Preserve Resources}\label{sec:Basic Facilities of a Virtio
>> +Device / Preserve Resources}
>> +
>> +As virtio devices are paravirtualization devices by design.
> This is not true and not relevant for the spec. Please remove this line.
OK, will remove this line in next version.

> 
>> +There are various devices resources created by sending commands from
>> +frontend and stored in backend.
>> +
>> +In some scenes, resources may be destroyed or reset, some of them can
>> +be re-created since frontend has enough information , but some can't.
>> +At this case, we can set \field{Preserve Resources} to 1 by specific
>> +transport, to prevent resources being destroyed.
>> +
>> +Which kind of resources need to be preserved and how to preserve
>> +resources depend on specific devices.
> s/on specific devices/on specific device type/
Thanks, will change in next version.

> 
>> +
>> +\drivernormative{\subsection}{Preserve Resources}{Basic Facilities of a
>> +Virtio Device / Preserve resources} A driver SHOULD set \field{Preserve
>> +Resources} to 1 when there is a need to preserve resources.
>> +
>> +\devicenormative{\subsection}{Preserve Resources}{Basic Facilities of a
>> +Virtio Device / Preserve resources} A device MUST NOT destroy resources until
>> \field{Preserve Resources} is 0.
>> +
>>  \input{admin.tex}
>>
>>  \chapter{General Initialization And Device Operation}\label{sec:General
>> Initialization And Device Operation} @@ -872,6 +893,10 @@
>> \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>>  	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits}
>> for
>>  	handling features reserved for future use.
>>
>> +  \item[VIRTIO_F_PRESERVE_RESOURCES(42)] This feature indicates  that
>> + the device need to preserve resources.
>> +  See \ref{sec:Basic Facilities of a Virtio Device / Preserve Resources}.
>> +
>>  \end{description}
>>
>>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits} diff --
>> git a/transport-pci.tex b/transport-pci.tex index a5c6719..f6eea65 100644
>> --- a/transport-pci.tex
>> +++ b/transport-pci.tex
>> @@ -325,6 +325,7 @@ \subsubsection{Common configuration structure
>> layout}\label{sec:Virtio Transport
>>          /* About the administration virtqueue. */
>>          le16 admin_queue_index;         /* read-only for driver */
>>          le16 admin_queue_num;         /* read-only for driver */
>> +        le16 preserve_resources;        /* read-write */
> Preserving these resources in the device implementation takes finite amount of time.
> Possibly more than 40nsec (time of PCIe write TLP).
> Hence this register must be a polling register to indicate that preservation_done.
> This will tell the guest when the preservation is done, and when restoration is done, so that it can resume upper layers.
> 
> Please refer to queue_reset definition to learn more about such register definition.
Thanks, I will refer to "queue_reset". So, I need three values, driver write 1 to let device do preserving resources, driver write 2 to let device do restoring resources, device write 0 to tell driver that preserving or restoring done, am I right?

> 
> Lets please make sure that PCIe PM level registers are sufficient/not-sufficient to decide the addition of this register.
But if the device is not a PCIe device, it doesn't have PM capability, then this will not work. Actually in my local environment, pci_is_express() return false in Qemu, they are not PCIe device.

> 
>>  };
>>  \end{lstlisting}
>>
>> @@ -428,6 +429,11 @@ \subsubsection{Common configuration structure
>> layout}\label{sec:Virtio Transport
>>  	The value 0 indicates no supported administration virtqueues.
>>  	This field is valid only if VIRTIO_F_ADMIN_VQ has been
>>  	negotiated.
>> +
>> +\item[\field{preserve_resources}]
>> +        The driver writes this to let device preserve resources whenever driver
>> has demands.
>> +        1 - device need to preserve resources which can't be re-created, until 0 is
>> set.
>> +        0 - all resources can be destroyed.
>>  \end{description}
>>
>>  \devicenormative{\paragraph}{Common configuration structure layout}{Virtio
>> Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common
>> configuration structure layout}
>> --
>> 2.34.1
> 

-- 
Best regards,
Jiqian Chen.