linux-kernel - Re: [VIRTIO PCI PATCH v5 1/1] transport-pci: Add freeze_mode to virtio_pci_common

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BL1PR12MB58498D05C8C2F9E7DBCF0978E7F9A@BL1PR12MB5849.namprd12.prod.outlook.com>
Date:   Wed, 20 Sep 2023 04:56:34 +0000
From:   "Chen, Jiqian" <Jiqian.Chen@....com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
CC:     Gerd Hoffmann <kraxel@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
        David Airlie <airlied@...hat.com>,
        Gurchetan Singh <gurchetansingh@...omium.org>,
        Chia-I Wu <olvaffe@...il.com>,
        Marc-André Lureau <marcandre.lureau@...il.com>,
        Robert Beckett <bob.beckett@...labora.com>,
        Mikhail Golubev-Ciuchea <Mikhail.Golubev-Ciuchea@...nsynergy.com>,
        Parav Pandit <parav@...dia.com>,
        "virtio-comment@...ts.oasis-open.org" 
        <virtio-comment@...ts.oasis-open.org>,
        "virtio-dev@...ts.oasis-open.org" <virtio-dev@...ts.oasis-open.org>,
        "qemu-devel@...gnu.org" <qemu-devel@...gnu.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Stefano Stabellini <sstabellini@...nel.org>,
        Roger Pau Monné <roger.pau@...rix.com>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Koenig, Christian" <Christian.Koenig@....com>,
        "Hildebrand, Stewart" <Stewart.Hildebrand@....com>,
        Xenia Ragiadakou <burzalodowa@...il.com>,
        "Huang, Honglei1" <Honglei1.Huang@....com>,
        "Zhang, Julia" <Julia.Zhang@....com>,
        "Huang, Ray" <Ray.Huang@....com>,
        "Chen, Jiqian" <Jiqian.Chen@....com>
Subject: Re: [VIRTIO PCI PATCH v5 1/1] transport-pci: Add freeze_mode to
 virtio_pci_common_cfg

Hi Michael S. Tsirkin,

On 2023/9/19 20:31, 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.
>>
>> Signed-off-by: Jiqian Chen <Jiqian.Chen@....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.
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?
I add the offset for freeze_mode(VIRTIO_PCI_COMMON_F_MODE), and change the offset of Q_NDATA and Q_RESET
-#define VIRTIO_PCI_COMMON_Q_NDATA      56
-#define VIRTIO_PCI_COMMON_Q_RESET      58
+#define VIRTIO_PCI_COMMON_F_MODE       56
+#define VIRTIO_PCI_COMMON_Q_NDATA      58
+#define VIRTIO_PCI_COMMON_Q_RESET      60

> 
>   
>> @@ -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.
Thanks, I will add the values.

> 
> we also need
> - feature bit to detect support for S3
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?

> - conformance statements documenting behavious under S3
Sorry, I am not very sure. Do you mean when freeze_mode is set FREEZE_S3, what operations should driver and device to do? Can you elaborate on it, or give an example?

> 
> 
>>  \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
> 

-- 
Best regards,
Jiqian Chen.