* Re: [LINUX KERNEL PATCH v5 1/2] virtio_pci: Add freeze_mode for virtio_pci_common_cfg
[not found] ` <20230919104607.2282248-2-Jiqian.Chen@amd.com>
@ 2023-10-03 13:20 ` Michael S. Tsirkin
0 siblings, 0 replies; only message in thread
From: Michael S. Tsirkin @ 2023-10-03 13:20 UTC (permalink / raw)
To: Jiqian Chen
Cc: qemu-devel, dri-devel, Gurchetan Singh, Huang Rui, Julia Zhang,
Xuan Zhuo, Stefano Stabellini, David Airlie, Chia-I Wu,
Juergen Gross, Honglei Huang, Stewart Hildebrand, Boris Ostrovsky,
virtualization, Robert Beckett, linux-kernel,
Oleksandr Tyshchenko, Marc-André Lureau, Xenia Ragiadakou,
Alex Deucher, Christian Koenig, Roger Pau Monné
On Tue, Sep 19, 2023 at 06:46:06PM +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, that 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, this patch add a new parameter named freeze_mode to
> struct virtio_pci_common_cfg, and when guest suspends, it can set
> freeze_mode to be FREEZE_S3, so that virtio devices can change their
> reset behavior on Qemu side according to that mode.
>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> drivers/virtio/virtio.c | 13 +++++++++++++
> drivers/virtio/virtio_pci_modern.c | 9 +++++++++
> drivers/virtio/virtio_pci_modern_dev.c | 16 ++++++++++++++++
> include/linux/virtio_config.h | 1 +
> include/linux/virtio_pci_modern.h | 2 ++
> include/uapi/linux/virtio_pci.h | 16 ++++++++++++++--
> 6 files changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index 3893dc29eb26..b4eb8369d5a1 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -7,6 +7,7 @@
> #include <linux/idr.h>
> #include <linux/of.h>
> #include <uapi/linux/virtio_ids.h>
> +#include <uapi/linux/virtio_pci.h>
>
> /* Unique numbering for virtio devices. */
> static DEFINE_IDA(virtio_index_ida);
> @@ -486,10 +487,20 @@ void unregister_virtio_device(struct virtio_device *dev)
> EXPORT_SYMBOL_GPL(unregister_virtio_device);
>
> #ifdef CONFIG_PM_SLEEP
> +static void virtio_set_freeze_mode(struct virtio_device *dev, u16 mode)
> +{
> + if (!dev->config->set_freeze_mode)
> + return;
> + might_sleep();
> + dev->config->set_freeze_mode(dev, mode);
> +}
> +
> int virtio_device_freeze(struct virtio_device *dev)
> {
> struct virtio_driver *drv = drv_to_virtio(dev->dev.driver);
>
> + virtio_set_freeze_mode(dev, VIRTIO_PCI_FREEZE_MODE_FREEZE_S3);
> +
> virtio_config_disable(dev);
>
> dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
> @@ -544,6 +555,8 @@ int virtio_device_restore(struct virtio_device *dev)
>
> virtio_config_enable(dev);
>
> + virtio_set_freeze_mode(dev, VIRTIO_PCI_FREEZE_MODE_UNFREEZE);
> +
> return 0;
>
> err:
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index d6bb68ba84e5..846b70919cbd 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -491,6 +491,13 @@ static bool vp_get_shm_region(struct virtio_device *vdev,
> return true;
> }
>
> +static void vp_set_freeze_mode(struct virtio_device *vdev, u16 mode)
> +{
> + struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> +
> + vp_modern_set_freeze_mode(&vp_dev->mdev, mode);
> +}
> +
> static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
> .get = NULL,
> .set = NULL,
> @@ -509,6 +516,7 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
> .get_shm_region = vp_get_shm_region,
> .disable_vq_and_reset = vp_modern_disable_vq_and_reset,
> .enable_vq_after_reset = vp_modern_enable_vq_after_reset,
> + .set_freeze_mode = vp_set_freeze_mode,
> };
>
> static const struct virtio_config_ops virtio_pci_config_ops = {
> @@ -529,6 +537,7 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
> .get_shm_region = vp_get_shm_region,
> .disable_vq_and_reset = vp_modern_disable_vq_and_reset,
> .enable_vq_after_reset = vp_modern_enable_vq_after_reset,
> + .set_freeze_mode = vp_set_freeze_mode,
> };
>
> /* the PCI probing function */
> diff --git a/drivers/virtio/virtio_pci_modern_dev.c b/drivers/virtio/virtio_pci_modern_dev.c
> index aad7d9296e77..4a6f7d130b6e 100644
> --- a/drivers/virtio/virtio_pci_modern_dev.c
> +++ b/drivers/virtio/virtio_pci_modern_dev.c
> @@ -203,6 +203,8 @@ static inline void check_offsets(void)
> offsetof(struct virtio_pci_common_cfg, queue_used_lo));
> BUILD_BUG_ON(VIRTIO_PCI_COMMON_Q_USEDHI !=
> offsetof(struct virtio_pci_common_cfg, queue_used_hi));
> + BUILD_BUG_ON(VIRTIO_PCI_COMMON_F_MODE !=
> + offsetof(struct virtio_pci_common_cfg, freeze_mode));
> }
>
> /*
> @@ -714,6 +716,20 @@ void __iomem *vp_modern_map_vq_notify(struct virtio_pci_modern_device *mdev,
> }
> EXPORT_SYMBOL_GPL(vp_modern_map_vq_notify);
>
> +/*
> + * vp_modern_set_freeze_mode - set freeze mode to device
> + * @mdev: the modern virtio-pci device
> + * @mode: the mode set to device
> + */
> +void vp_modern_set_freeze_mode(struct virtio_pci_modern_device *mdev,
> + u16 mode)
> +{
> + struct virtio_pci_common_cfg __iomem *cfg = mdev->common;
> +
> + vp_iowrite16(mode, &cfg->freeze_mode);
> +}
> +EXPORT_SYMBOL_GPL(vp_modern_set_freeze_mode);
> +
> MODULE_VERSION("0.1");
> MODULE_DESCRIPTION("Modern Virtio PCI Device");
> MODULE_AUTHOR("Jason Wang <jasowang@redhat.com>");
> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 2b3438de2c4d..2a7443ff7f12 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -120,6 +120,7 @@ struct virtio_config_ops {
> struct virtio_shm_region *region, u8 id);
> int (*disable_vq_and_reset)(struct virtqueue *vq);
> int (*enable_vq_after_reset)(struct virtqueue *vq);
> + void (*set_freeze_mode)(struct virtio_device *vdev, u16 mode);
> };
>
> /* If driver didn't advertise the feature, it will never appear. */
> diff --git a/include/linux/virtio_pci_modern.h b/include/linux/virtio_pci_modern.h
> index 067ac1d789bc..ba6eed216ded 100644
> --- a/include/linux/virtio_pci_modern.h
> +++ b/include/linux/virtio_pci_modern.h
> @@ -121,4 +121,6 @@ int vp_modern_probe(struct virtio_pci_modern_device *mdev);
> void vp_modern_remove(struct virtio_pci_modern_device *mdev);
> int vp_modern_get_queue_reset(struct virtio_pci_modern_device *mdev, u16 index);
> void vp_modern_set_queue_reset(struct virtio_pci_modern_device *mdev, u16 index);
> +void vp_modern_set_freeze_mode(struct virtio_pci_modern_device *mdev,
> + u16 mode);
> #endif
> diff --git a/include/uapi/linux/virtio_pci.h b/include/uapi/linux/virtio_pci.h
> index f703afc7ad31..725ace458a1b 100644
> --- a/include/uapi/linux/virtio_pci.h
> +++ b/include/uapi/linux/virtio_pci.h
> @@ -140,6 +140,15 @@ struct virtio_pci_notify_cap {
> __le32 notify_off_multiplier; /* Multiplier for queue_notify_off. */
> };
>
> +typedef enum {
> + VIRTIO_PCI_FREEZE_MODE_UNFREEZE = 0,
> + VIRTIO_PCI_FREEZE_MODE_FREEZE_S3 = 3,
> +} virtio_pci_freeze_mode_t;
we don't normally do typedefs.
> +
> +#define VIRTIO_PCI_FREEZE_MODE_MASK \
> + ((1 << VIRTIO_PCI_FREEZE_MODE_UNFREEZE) | \
> + (1 << VIRTIO_PCI_FREEZE_MODE_FREEZE_S3))
> +
not sure why is this useful generally.
> /* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */
> struct virtio_pci_common_cfg {
> /* About the whole device. */
> @@ -164,6 +173,8 @@ struct virtio_pci_common_cfg {
> __le32 queue_avail_hi; /* read-write */
> __le32 queue_used_lo; /* read-write */
> __le32 queue_used_hi; /* read-write */
> +
> + __le16 freeze_mode; /* read-write */
> };
>
Your patch will likely break uses of sizeof(struct virtio_pci_common_cfg)
on existing devices.
> /* Fields in VIRTIO_PCI_CAP_PCI_CFG: */
> @@ -202,8 +213,9 @@ struct virtio_pci_cfg_cap {
> #define VIRTIO_PCI_COMMON_Q_AVAILHI 44
> #define VIRTIO_PCI_COMMON_Q_USEDLO 48
> #define VIRTIO_PCI_COMMON_Q_USEDHI 52
> -#define VIRTIO_PCI_COMMON_Q_NDATA 56
> -#define VIRTIO_PCI_COMMON_Q_RESET 58
> +#define VIRTIO_PCI_COMMON_F_MODE 56
F_ here stands for freeze? Please don't abbreviate.
Q for queue is a pun that works, F for freeze doesn't.
> +#define VIRTIO_PCI_COMMON_Q_NDATA 58
> +#define VIRTIO_PCI_COMMON_Q_RESET 60
>
> #endif /* VIRTIO_PCI_NO_MODERN */
>
> --
> 2.34.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] only message in thread