Re: [QEMU PATCH 0/1]

[Chen, Jiqian]

Hi all,

Modifications on kernel end is:
https://lore.kernel.org/lkml/20230608063857.1677973-2-jiqian.c...@amd.com/T/#u


On 2023/6/8 10:56, Jiqian Chen wrote:
> Hi all,
> 
> I am working to implement virtgpu S3 function on Xen.
> 
> Currently on Xen, if we start a guest who enables virtgpu, and then
> run "echo mem > /sys/power/state" to suspend guest. And run
> "sudo xl trigger  s3resume" to resume guest. We can find that
> the guest kernel comes back, but the display doesn't. It just shown a
> black screen.
> 
> Through reading codes, I founded that when guest was during suspending,
> it called into Qemu to call virtio_gpu_gl_reset. In virtio_gpu_gl_reset,
> it destroyed all resources and reset renderer. This made the display
> gone after guest resumed.
> 
> I think we should keep resources or prevent they being destroyed when
> guest is suspending. So, I add a new status named freezing to virtgpu,
> and add a new ctrl message VIRTIO_GPU_CMD_STATUS_FREEZING to get
> notification from guest. If freezing is set to true, and then Qemu will
> realize that guest is suspending, it will not destroy resources and will
> not reset renderer. If freezing is set to false, Qemu will do its origin
> actions, and has no other impaction.
> 
> And now, display can come back and applications can continue their
> status after guest resumes.
> 
> Jiqian Chen (1):
>   virtgpu: do not destroy resources when guest suspend
> 
>  hw/display/virtio-gpu-gl.c  |  9 ++-
>  hw/display/virtio-gpu-virgl.c   |  3 +++
>  hw/display/virtio-gpu.c | 26 +++--
>  include/hw/virtio/virtio-gpu.h  |  3 +++
>  include/standard-headers/linux/virtio_gpu.h |  9 +++
>  5 files changed, 47 insertions(+), 3 deletions(-)
> 

-- 
Best regards,
Chen, Jiqian.

Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Chen, Jiqian]

On 2023/6/9 00:41, Robert Beckett wrote:
> 
> On 08/06/2023 03:56, Jiqian Chen wrote:
>> After suspending and resuming guest VM, you will get
>> a black screen, and the display can't come back.
>>
>> This is because when guest did suspending, it called
>> into qemu to call virtio_gpu_gl_reset. In function
>> virtio_gpu_gl_reset, it destroyed resources and reset
>> renderer, which were used for display. As a result,
>> guest's screen can't come back to the time when it was
>> suspended and only showed black.
>>
>> So, this patch adds a new ctrl message
>> VIRTIO_GPU_CMD_STATUS_FREEZING to get notification from
>> guest. If guest is during suspending, it sets freezing
>> status of virtgpu to true, this will prevent destroying
>> resources and resetting renderer when guest calls into
>> virtio_gpu_gl_reset. If guest is during resuming, it sets
>> freezing to false, and then virtio_gpu_gl_reset will keep
>> its origin actions and has no other impaction.
>>
>> Signed-off-by: Jiqian Chen 
>> ---
>>   hw/display/virtio-gpu-gl.c  |  9 ++-
>>   hw/display/virtio-gpu-virgl.c   |  3 +++
>>   hw/display/virtio-gpu.c | 26 +++--
>>   include/hw/virtio/virtio-gpu.h  |  3 +++
>>   include/standard-headers/linux/virtio_gpu.h |  9 +++
>>   5 files changed, 47 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>> index e06be60dfb..e11ad233eb 100644
>> --- a/hw/display/virtio-gpu-gl.c
>> +++ b/hw/display/virtio-gpu-gl.c
>> @@ -100,7 +100,14 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
>>    */
>>   if (gl->renderer_inited && !gl->renderer_reset) {
>>   virtio_gpu_virgl_reset_scanout(g);
>> -    gl->renderer_reset = true;
>> +    /*
>> + * If guest is suspending, we shouldn't reset renderer,
>> + * otherwise, the display can't come back to the time when
>> + * it was suspended after guest resumed.
>> + */
>> +    if (!g->freezing) {
>> +    gl->renderer_reset = true;
>> +    }
>>   }
>>   }
>>   diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index 73cb92c8d5..183ec92d53 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -464,6 +464,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>>   case VIRTIO_GPU_CMD_GET_EDID:
>>   virtio_gpu_get_edid(g, cmd);
>>   break;
>> +    case VIRTIO_GPU_CMD_STATUS_FREEZING:
>> +    virtio_gpu_cmd_status_freezing(g, cmd);
>> +    break;
>>   default:
>>   cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>   break;
>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>> index 5e15c79b94..8f235d7848 100644
>> --- a/hw/display/virtio-gpu.c
>> +++ b/hw/display/virtio-gpu.c
>> @@ -373,6 +373,16 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU 
>> *g,
>>   QTAILQ_INSERT_HEAD(&g->reslist, res, next);
>>   }
>>   +void virtio_gpu_cmd_status_freezing(VirtIOGPU *g,
>> + struct virtio_gpu_ctrl_command *cmd)
>> +{
>> +    struct virtio_gpu_status_freezing sf;
>> +
>> +    VIRTIO_GPU_FILL_CMD(sf);
>> +    virtio_gpu_bswap_32(&sf, sizeof(sf));
>> +    g->freezing = sf.freezing;
>> +}
>> +
>>   static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
>>   {
>>   struct virtio_gpu_scanout *scanout = 
>> &g->parent_obj.scanout[scanout_id];
>> @@ -986,6 +996,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>>   case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
>>   virtio_gpu_resource_detach_backing(g, cmd);
>>   break;
>> +    case VIRTIO_GPU_CMD_STATUS_FREEZING:
>> +    virtio_gpu_cmd_status_freezing(g, cmd);
>> +    break;
>>   default:
>>   cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>   break;
>> @@ -1344,6 +1357,8 @@ void virtio_gpu_device_realize(DeviceState *qdev, 
>> Error **errp)
>>   QTAILQ_INIT(&g->reslist);
>>   QTAILQ_INIT(&g->cmdq);
>>   QTAILQ_INIT(&g->fenceq);
>> +
>> +    g->freezing = false;
>>   }
>>     void virtio_gpu_reset(VirtIODevice *vdev)
>> @@ -1352,8 +1367,15 @@ void virtio_gpu_reset(VirtIODevice *vdev)
>>   struct virtio_gpu_simple_resource *res, *tmp;
>>   struct virtio_gpu_ctrl_command *cmd;
>>   -    QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
>> -    virtio_gpu_resource_destroy(g, res);
>> +    /*
>> + * If guest is suspending, we shouldn't destroy resources,
>> + * otherwise, the display can't come back to the time when
>> + * it was suspended after guest resumed.
>> + */
> 
> 
> What should happen if qemu is torn down while the guest is suspended.
> Currently there is no other place where the resources will be destroyed. 
> While it is likely viable to rely on process auto tear down of mem and files 
> from the host cleanup point of view, it feels bad to rely on that.
> Perhaps an inverted co

Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Chen, Jiqian]

Hi Marc-André Lureau

On 2023/6/12 20:42, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jun 8, 2023 at 6:26 AM Jiqian Chen  > wrote:
> 
> After suspending and resuming guest VM, you will get
> a black screen, and the display can't come back.
> 
> This is because when guest did suspending, it called
> into qemu to call virtio_gpu_gl_reset. In function
> virtio_gpu_gl_reset, it destroyed resources and reset
> renderer, which were used for display. As a result,
> guest's screen can't come back to the time when it was
> suspended and only showed black.
> 
> So, this patch adds a new ctrl message
> VIRTIO_GPU_CMD_STATUS_FREEZING to get notification from
> guest. If guest is during suspending, it sets freezing
> status of virtgpu to true, this will prevent destroying
> resources and resetting renderer when guest calls into
> virtio_gpu_gl_reset. If guest is during resuming, it sets
> freezing to false, and then virtio_gpu_gl_reset will keep
> its origin actions and has no other impaction.
> 
> Signed-off-by: Jiqian Chen  >
> ---
>  hw/display/virtio-gpu-gl.c                  |  9 ++-
>  hw/display/virtio-gpu-virgl.c               |  3 +++
>  hw/display/virtio-gpu.c                     | 26 +++--
>  include/hw/virtio/virtio-gpu.h              |  3 +++
>  include/standard-headers/linux/virtio_gpu.h |  9 +++
>  5 files changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
> index e06be60dfb..e11ad233eb 100644
> --- a/hw/display/virtio-gpu-gl.c
> +++ b/hw/display/virtio-gpu-gl.c
> @@ -100,7 +100,14 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
>       */
>      if (gl->renderer_inited && !gl->renderer_reset) {
>          virtio_gpu_virgl_reset_scanout(g);
> -        gl->renderer_reset = true;
> +        /*
> +         * If guest is suspending, we shouldn't reset renderer,
> +         * otherwise, the display can't come back to the time when
> +         * it was suspended after guest resumed.
> +         */
> +        if (!g->freezing) {
> +            gl->renderer_reset = true;
> +        }
>      }
>  }
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 73cb92c8d5..183ec92d53 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -464,6 +464,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>      case VIRTIO_GPU_CMD_GET_EDID:
>          virtio_gpu_get_edid(g, cmd);
>          break;
> +    case VIRTIO_GPU_CMD_STATUS_FREEZING:
> +        virtio_gpu_cmd_status_freezing(g, cmd);
> +        break;
>      default:
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          break;
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 5e15c79b94..8f235d7848 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -373,6 +373,16 @@ static void 
> virtio_gpu_resource_create_blob(VirtIOGPU *g,
>      QTAILQ_INSERT_HEAD(&g->reslist, res, next);
>  }
> 
> +void virtio_gpu_cmd_status_freezing(VirtIOGPU *g,
> +                         struct virtio_gpu_ctrl_command *cmd)
> +{
> +    struct virtio_gpu_status_freezing sf;
> +
> +    VIRTIO_GPU_FILL_CMD(sf);
> +    virtio_gpu_bswap_32(&sf, sizeof(sf));
> +    g->freezing = sf.freezing;
> +}
> +
>  static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
>  {
>      struct virtio_gpu_scanout *scanout = 
> &g->parent_obj.scanout[scanout_id];
> @@ -986,6 +996,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>      case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
>          virtio_gpu_resource_detach_backing(g, cmd);
>          break;
> +    case VIRTIO_GPU_CMD_STATUS_FREEZING:
> +        virtio_gpu_cmd_status_freezing(g, cmd);
> +        break;
>      default:
>          cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>          break;
> @@ -1344,6 +1357,8 @@ void virtio_gpu_device_realize(DeviceState *qdev, 
> Error **errp)
>      QTAILQ_INIT(&g->reslist);
>      QTAILQ_INIT(&g->cmdq);
>      QTAILQ_INIT(&g->fenceq);
> +
> +    g->freezing = false;
>  }
> 
>  void virtio_gpu_reset(VirtIODevice *vdev)
> @@ -1352,8 +1367,15 @@ void virtio_gpu_reset(VirtIODevice *vdev)
>      struct virtio_gpu_simple_resource *res, *tmp;
>      struct virtio_gpu_ctrl_command *cmd;
> 
> -    QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
> -        virtio_gpu_resource_destroy(g, res);
> +    /*
> +     * If guest is suspending, we shouldn't destroy resources,
> +     * otherwise, the display can't come back to the time when
> + 

Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Chen, Jiqian]

On 2023/6/9 00:41, Robert Beckett wrote:
> 
> On 08/06/2023 03:56, Jiqian Chen wrote:
>> After suspending and resuming guest VM, you will get
>> a black screen, and the display can't come back.
>>
>> This is because when guest did suspending, it called
>> into qemu to call virtio_gpu_gl_reset. In function
>> virtio_gpu_gl_reset, it destroyed resources and reset
>> renderer, which were used for display. As a result,
>> guest's screen can't come back to the time when it was
>> suspended and only showed black.
>>
>> So, this patch adds a new ctrl message
>> VIRTIO_GPU_CMD_STATUS_FREEZING to get notification from
>> guest. If guest is during suspending, it sets freezing
>> status of virtgpu to true, this will prevent destroying
>> resources and resetting renderer when guest calls into
>> virtio_gpu_gl_reset. If guest is during resuming, it sets
>> freezing to false, and then virtio_gpu_gl_reset will keep
>> its origin actions and has no other impaction.
>>
>> Signed-off-by: Jiqian Chen 
>> ---
>>   hw/display/virtio-gpu-gl.c  |  9 ++-
>>   hw/display/virtio-gpu-virgl.c   |  3 +++
>>   hw/display/virtio-gpu.c | 26 +++--
>>   include/hw/virtio/virtio-gpu.h  |  3 +++
>>   include/standard-headers/linux/virtio_gpu.h |  9 +++
>>   5 files changed, 47 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>> index e06be60dfb..e11ad233eb 100644
>> --- a/hw/display/virtio-gpu-gl.c
>> +++ b/hw/display/virtio-gpu-gl.c
>> @@ -100,7 +100,14 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
>>    */
>>   if (gl->renderer_inited && !gl->renderer_reset) {
>>   virtio_gpu_virgl_reset_scanout(g);
>> -    gl->renderer_reset = true;
>> +    /*
>> + * If guest is suspending, we shouldn't reset renderer,
>> + * otherwise, the display can't come back to the time when
>> + * it was suspended after guest resumed.
>> + */
>> +    if (!g->freezing) {
>> +    gl->renderer_reset = true;
>> +    }
>>   }
>>   }
>>   diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index 73cb92c8d5..183ec92d53 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -464,6 +464,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>>   case VIRTIO_GPU_CMD_GET_EDID:
>>   virtio_gpu_get_edid(g, cmd);
>>   break;
>> +    case VIRTIO_GPU_CMD_STATUS_FREEZING:
>> +    virtio_gpu_cmd_status_freezing(g, cmd);
>> +    break;
>>   default:
>>   cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>   break;
>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>> index 5e15c79b94..8f235d7848 100644
>> --- a/hw/display/virtio-gpu.c
>> +++ b/hw/display/virtio-gpu.c
>> @@ -373,6 +373,16 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU 
>> *g,
>>   QTAILQ_INSERT_HEAD(&g->reslist, res, next);
>>   }
>>   +void virtio_gpu_cmd_status_freezing(VirtIOGPU *g,
>> + struct virtio_gpu_ctrl_command *cmd)
>> +{
>> +    struct virtio_gpu_status_freezing sf;
>> +
>> +    VIRTIO_GPU_FILL_CMD(sf);
>> +    virtio_gpu_bswap_32(&sf, sizeof(sf));
>> +    g->freezing = sf.freezing;
>> +}
>> +
>>   static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
>>   {
>>   struct virtio_gpu_scanout *scanout = 
>> &g->parent_obj.scanout[scanout_id];
>> @@ -986,6 +996,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>>   case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
>>   virtio_gpu_resource_detach_backing(g, cmd);
>>   break;
>> +    case VIRTIO_GPU_CMD_STATUS_FREEZING:
>> +    virtio_gpu_cmd_status_freezing(g, cmd);
>> +    break;
>>   default:
>>   cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>   break;
>> @@ -1344,6 +1357,8 @@ void virtio_gpu_device_realize(DeviceState *qdev, 
>> Error **errp)
>>   QTAILQ_INIT(&g->reslist);
>>   QTAILQ_INIT(&g->cmdq);
>>   QTAILQ_INIT(&g->fenceq);
>> +
>> +    g->freezing = false;
>>   }
>>     void virtio_gpu_reset(VirtIODevice *vdev)
>> @@ -1352,8 +1367,15 @@ void virtio_gpu_reset(VirtIODevice *vdev)
>>   struct virtio_gpu_simple_resource *res, *tmp;
>>   struct virtio_gpu_ctrl_command *cmd;
>>   -    QTAILQ_FOREACH_SAFE(res, &g->reslist, next, tmp) {
>> -    virtio_gpu_resource_destroy(g, res);
>> +    /*
>> + * If guest is suspending, we shouldn't destroy resources,
>> + * otherwise, the display can't come back to the time when
>> + * it was suspended after guest resumed.
>> + */
> 
> 
> What should happen if qemu is torn down while the guest is suspended.

I don't know if there is a place to reclaim all memory in Qemu. If not, in the 
case you said, the memory of resources may leak.
But I ran "sudo xl destroy " to destroy Qemu without suspending 
guest, and then I found it didn't destroy resources t

Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Chen, Jiqian]

Hi Gerd Hoffmann

On 2023/6/19 20:51, Gerd Hoffmann wrote:
>   Hi, 
>> Adding a new command requires new feature flag (and maybe it should be in
>> the <0x1000 range instead)
>>
>> But I am not sure we need a new message at the virtio-gpu level. Gerd, wdyt?
>>
>> Maybe it's not a good place to reset all GPU resources during QEMU reset()
>> after all, if it's called during s3 and there is no mechanism to restore
>> it. Damien?
> 
> The guest driver should be able to restore resources after resume.

Thank you for your suggestion!
As far as I know, resources are created on host side and guest has no backup, 
if resources are destroyed, guest can't restore them.
Or do you mean guest driver need to send commands to re-create resources after 
resume?
If so, I have some questions. Can guest re-create resources by using 
object(virtio_vpu_object) or others? Can the new resources replace the 
destroyed resources to continue the suspended display tasks after resume? I 
think those will help me improve my implementation, thank you!

> 
> take care,
>   Gerd
> 

-- 
Best regards,
Jiqian Chen.

Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Chen, Jiqian]

Hi Dongwon,

On 2023/6/30 00:53, Kim, Dongwon wrote:
> This method - letting QEMU not remove resources would work on S3 case but 
> with S4, the QEMU would lose all the resources anyway as the process will be 
> terminated. So objects restoring was only option for us as
My patch is for S3 function on Xen. I haven't tried S4 before, I will try S4 
later.

> 
> in [RFC PATCH 2/2] drm/virtio: restore virtio_gpu_objects upon suspend and 
> resume (lists.freedesktop.org) 
> 
> 
> But I only considered and tested cases with scanout blob resources, so this 
> may not cover other resource types...
I tried your patch, but I can't success to resume guest and guest crashed.

> 
> On 6/7/2023 7:56 PM, Jiqian Chen wrote:
>> After suspending and resuming guest VM, you will get
>> a black screen, and the display can't come back.
>>
>> This is because when guest did suspending, it called
>> into qemu to call virtio_gpu_gl_reset. In function
>> virtio_gpu_gl_reset, it destroyed resources and reset
>> renderer, which were used for display. As a result,
>> guest's screen can't come back to the time when it was
>> suspended and only showed black.
>>
>> So, this patch adds a new ctrl message
>> VIRTIO_GPU_CMD_STATUS_FREEZING to get notification from
>> guest. If guest is during suspending, it sets freezing
>> status of virtgpu to true, this will prevent destroying
>> resources and resetting renderer when guest calls into
>> virtio_gpu_gl_reset. If guest is during resuming, it sets
>> freezing to false, and then virtio_gpu_gl_reset will keep
>> its origin actions and has no other impaction.
>>
>> Signed-off-by: Jiqian Chen 
>> ---
>>   hw/display/virtio-gpu-gl.c  |  9 ++-
>>   hw/display/virtio-gpu-virgl.c   |  3 +++
>>   hw/display/virtio-gpu.c | 26 +++--
>>   include/hw/virtio/virtio-gpu.h  |  3 +++
>>   include/standard-headers/linux/virtio_gpu.h |  9 +++
>>   5 files changed, 47 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>> index e06be60dfb..e11ad233eb 100644
>> --- a/hw/display/virtio-gpu-gl.c
>> +++ b/hw/display/virtio-gpu-gl.c
>> @@ -100,7 +100,14 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
>>    */
>>   if (gl->renderer_inited && !gl->renderer_reset) {
>>   virtio_gpu_virgl_reset_scanout(g);
>> -    gl->renderer_reset = true;
>> +    /*
>> + * If guest is suspending, we shouldn't reset renderer,
>> + * otherwise, the display can't come back to the time when
>> + * it was suspended after guest resumed.
>> + */
>> +    if (!g->freezing) {
>> +    gl->renderer_reset = true;
>> +    }
>>   }
>>   }
>>   diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index 73cb92c8d5..183ec92d53 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -464,6 +464,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>>   case VIRTIO_GPU_CMD_GET_EDID:
>>   virtio_gpu_get_edid(g, cmd);
>>   break;
>> +    case VIRTIO_GPU_CMD_STATUS_FREEZING:
>> +    virtio_gpu_cmd_status_freezing(g, cmd);
>> +    break;
>>   default:
>>   cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>   break;
>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>> index 5e15c79b94..8f235d7848 100644
>> --- a/hw/display/virtio-gpu.c
>> +++ b/hw/display/virtio-gpu.c
>> @@ -373,6 +373,16 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU 
>> *g,
>>   QTAILQ_INSERT_HEAD(&g->reslist, res, next);
>>   }
>>   +void virtio_gpu_cmd_status_freezing(VirtIOGPU *g,
>> + struct virtio_gpu_ctrl_command *cmd)
>> +{
>> +    struct virtio_gpu_status_freezing sf;
>> +
>> +    VIRTIO_GPU_FILL_CMD(sf);
>> +    virtio_gpu_bswap_32(&sf, sizeof(sf));
>> +    g->freezing = sf.freezing;
>> +}
>> +
>>   static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
>>   {
>>   struct virtio_gpu_scanout *scanout = 
>> &g->parent_obj.scanout[scanout_id];
>> @@ -986,6 +996,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>>   case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
>>   virtio_gpu_resource_detach_backing(g, cmd);
>>   break;
>> +    case VIRTIO_GPU_CMD_STATUS_FREEZING:
>> +    virtio_gpu_cmd_status_freezing(g, cmd);
>> +    break;
>>   default:
>>   cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>   break;
>> @@ -1344,6 +1357,8 @@ void virtio_gpu_device_realize(DeviceState *qdev, 
>> Error **errp)
>>   QTAILQ_INIT(&g->reslist);
>>   QTAILQ_INIT(&g->cmdq);
>>   QTAILQ_INIT(&g->fenceq);
>> +
>> +    g->freezing = false;
>>   }
>>     void virtio_gpu_reset(VirtIODevice *vdev)
>> @@ -1352,8 +1367,15 @@ void virtio_gpu_reset(VirtIODevice *vdev)
>>   struct virtio_gpu_simple_r

Re: [QEMU PATCH v2 0/1] S3 support

[Chen, Jiqian]

Hi all,

V2 patch of kernel is 
https://lore.kernel.org/lkml/20230630073448.842767-1-jiqian.c...@amd.com/T/#t.

On 2023/6/30 15:00, Jiqian Chen wrote:
> v2:
> 
> Hi all,
> 
> Thanks to Marc-André Lureau, Robert Beckett and Gerd Hoffmann for
> their advice and guidance. V2 makes below changes:
> 
> * Change VIRTIO_CPU_CMD_STATUS_FREEZING to 0x0400 (<0x1000)
> * Add virtio_gpu_device_unrealize to destroy resources to solve
>   potential memory leak problem. This also needs hot-plug support.
> * Add a new feature flag VIRTIO_GPU_F_FREEZING, so that guest and
>   host can negotiate whenever freezing is supported or not.
> 
> Best regards,
> Jiqian Chen.
> 
> v1:
> 
> link: 
> https://lore.kernel.org/qemu-devel/20230608025655.1674357-1-jiqian.c...@amd.com/
> 
> Hi all,
> 
> I am working to implement virtgpu S3 function on Xen.
> 
> Currently on Xen, if we start a guest who enables virtgpu, and then
> run "echo mem > /sys/power/state" to suspend guest. And run
> "sudo xl trigger  s3resume" to resume guest. We can find that
> the guest kernel comes back, but the display doesn't. It just shown a
> black screen.
> 
> Through reading codes, I founded that when guest was during suspending,
> it called into Qemu to call virtio_gpu_gl_reset. In virtio_gpu_gl_reset,
> it destroyed all resources and reset renderer. This made the display
> gone after guest resumed.
> 
> I think we should keep resources or prevent they being destroyed when
> guest is suspending. So, I add a new status named freezing to virtgpu,
> and add a new ctrl message VIRTIO_GPU_CMD_STATUS_FREEZING to get
> notification from guest. If freezing is set to true, and then Qemu will
> realize that guest is suspending, it will not destroy resources and will
> not reset renderer. If freezing is set to false, Qemu will do its origin
> actions, and has no other impaction.
> 
> And now, display can come back and applications can continue their
> status after guest resumes.
> 
> Jiqian Chen (1):
>   virtgpu: do not destroy resources when guest suspend
> 
>  hw/display/virtio-gpu-base.c|  3 ++
>  hw/display/virtio-gpu-gl.c  |  9 +++-
>  hw/display/virtio-gpu-virgl.c   |  7 +++
>  hw/display/virtio-gpu.c | 52 -
>  hw/virtio/virtio.c  |  3 ++
>  include/hw/virtio/virtio-gpu.h  |  6 +++
>  include/standard-headers/linux/virtio_gpu.h | 15 ++
>  7 files changed, 92 insertions(+), 3 deletions(-)
> 

-- 
Best regards,
Jiqian Chen.

Re: [QEMU PATCH v2 1/1] virtgpu: do not destroy resources when guest suspend

[Chen, Jiqian]

Hi,

On 2023/7/11 04:28, Michael S. Tsirkin wrote:
> On Fri, Jun 30, 2023 at 03:00:16PM +0800, Jiqian Chen wrote:
>> After suspending and resuming guest VM, you will get
>> a black screen, and the display can't come back.
>>
>> This is because when guest did suspending, it called
>> into qemu to call virtio_gpu_gl_reset. In function
>> virtio_gpu_gl_reset, it destroyed resources and reset
>> renderer, which were used for display. As a result,
>> guest's screen can't come back to the time when it was
>> suspended and only showed black.
>>
>> So, this patch adds a new ctrl message
>> VIRTIO_GPU_CMD_STATUS_FREEZING to get notification from
>> guest. If guest is during suspending, it sets freezing
>> status of virtgpu to true, this will prevent destroying
>> resources and resetting renderer when guest calls into
>> virtio_gpu_gl_reset. If guest is during resuming, it sets
>> freezing to false, and then virtio_gpu_gl_reset will keep
>> its origin actions and has no other impaction.
>>
>> Due to this implemention needs cooperation with guest, so
>> it added a new feature flag VIRTIO_GPU_F_FREEZING, so
>> that guest and host can negotiate whenever freezing is
>> supported or not.
>>
>> Signed-off-by: Jiqian Chen 
>> ---
>>  hw/display/virtio-gpu-base.c|  3 ++
>>  hw/display/virtio-gpu-gl.c  |  9 +++-
>>  hw/display/virtio-gpu-virgl.c   |  7 +++
>>  hw/display/virtio-gpu.c | 52 -
>>  hw/virtio/virtio.c  |  3 ++
>>  include/hw/virtio/virtio-gpu.h  |  6 +++
>>  include/standard-headers/linux/virtio_gpu.h | 15 ++
>>  7 files changed, 92 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
>> index a29f191aa8..d55dc8fdfc 100644
>> --- a/hw/display/virtio-gpu-base.c
>> +++ b/hw/display/virtio-gpu-base.c
>> @@ -215,6 +215,9 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, 
>> uint64_t features,
>>  if (virtio_gpu_blob_enabled(g->conf)) {
>>  features |= (1 << VIRTIO_GPU_F_RESOURCE_BLOB);
>>  }
>> +if (virtio_gpu_freezing_enabled(g->conf)) {
>> +features |= (1 << VIRTIO_GPU_F_FREEZING);
>> +}
>>  
>>  return features;
>>  }
>> diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
>> index e06be60dfb..de108f1502 100644
>> --- a/hw/display/virtio-gpu-gl.c
>> +++ b/hw/display/virtio-gpu-gl.c
>> @@ -100,7 +100,14 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
>>   */
>>  if (gl->renderer_inited && !gl->renderer_reset) {
>>  virtio_gpu_virgl_reset_scanout(g);
>> -gl->renderer_reset = true;
>> +/*
>> + * If guest is suspending, we shouldn't reset renderer,
>> + * otherwise, the display can't come back to the time when
>> + * it was suspended after guest resumed.
>> + */
>> +if (!virtio_gpu_freezing_enabled(g->parent_obj.conf) || 
>> !g->freezing) {
>> +gl->renderer_reset = true;
>> +}
>>  }
>>  }
>>  
>> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> index 73cb92c8d5..547c4d98ad 100644
>> --- a/hw/display/virtio-gpu-virgl.c
>> +++ b/hw/display/virtio-gpu-virgl.c
>> @@ -464,6 +464,13 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
>>  case VIRTIO_GPU_CMD_GET_EDID:
>>  virtio_gpu_get_edid(g, cmd);
>>  break;
>> +case VIRTIO_GPU_CMD_STATUS_FREEZING:
>> +if (virtio_gpu_freezing_enabled(g->parent_obj.conf)) {
>> +virtio_gpu_cmd_status_freezing(g, cmd);
>> +} else {
>> +cmd->error = VIRTIO_GPU_RESP_ERR_INVALID_PARAMETER;
>> +}
>> +break;
>>  default:
>>  cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
>>  break;
>> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
>> index 5e15c79b94..54a5e2e57c 100644
>> --- a/hw/display/virtio-gpu.c
>> +++ b/hw/display/virtio-gpu.c
>> @@ -373,6 +373,16 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU 
>> *g,
>>  QTAILQ_INSERT_HEAD(&g->reslist, res, next);
>>  }
>>  
>> +void virtio_gpu_cmd_status_freezing(VirtIOGPU *g,
>> + struct virtio_gpu_ctrl_command *cmd)
>> +{
>> +struct virtio_gpu_status_freezing sf;
>> +
>> +VIRTIO_GPU_FILL_CMD(sf);
>> +virtio_gpu_bswap_32(&sf, sizeof(sf));
>> +g->freezing = sf.freezing;
>> +}
>> +
>>  static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
>>  {
>>  struct virtio_gpu_scanout *scanout = &g->parent_obj.scanout[scanout_id];
>> @@ -986,6 +996,13 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
>>  case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
>>  virtio_gpu_resource_detach_backing(g, cmd);
>>  break;
>> +case VIRTIO_GPU_CMD_STATUS_FREEZING:
>> +if (virtio_gpu_freezing_enabled(g->parent_obj.conf)) {
>> +virtio_gpu_cmd_status_freezing(g, cmd);
>> +} else {
>> +   

Re: [virtio-dev] [RESEND VIRTIO GPU PATCH v3 1/1] virtio-gpu: Add new feature flag VIRTIO_GPU_F_FREEZE_S3

[Chen, Jiqian]

Hi Mikhail Golubev-Ciuchea,

I have improved this implementation to the virtio pci level. Could you please 
try my patches and see if them are useful in your scene?
My new series:
V5 of Qemu patch:
https://lore.kernel.org/qemu-devel/20230919110225.2282914-1-jiqian.c...@amd.com/T/#t
V5 of kernel patch:
https://lore.kernel.org/lkml/20230919104607.2282248-1-jiqian.c...@amd.com/T/#t
V5 of virtio-spec patch:
https://lists.oasis-open.org/archives/virtio-comment/202309/msg00245.html


On 2023/9/12 00:33, Mikhail Golubev-Ciuchea wrote:
> Hi Jiqian,
> 
> Thanks for the proposal.
> 
> Some time ago I was working on the same issue with suspending the gpu device
> (on arm).  Additionally, I had troubles with virtio-video device as well, see
> https://lore.kernel.org/lkml/20211215172739.ga77...@opensynergy.com/T/ for
> details.
> 
> In your case, the
> VIRTIO_GPU_FREEZE_MODE_FREEZE_S3/VIRTIO_GPU_FREEZE_MODE_UNFREEZE do influence
> how reset is being handled by Qemu, is this correct?  Since multiple devices
> can benefit from the same mechanism, would it be possible to: a) have a more
> generic, non ACPI-based name, b) make the feature generic, applicable to other
> devices as well?
> 
> 
> Best wishes,
> Mikhail Golubev-Ciuchea
> 
> 
> 
> --
> 
> Mikhail Golubev-Ciuchea
> 
> 
> OpenSynergy GmbH
> 
> Rotherstr. 20, 10245 Berlin
> 
> Telefon: +49 (30) 60 98 54 0 - 903
> 
> EMail:   mikhail.golu...@opensynergy.com
> 
> www.opensynergy.com
> 
> Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B
> 
> Geschäftsführer/Managing Director: Regis Adjamah
> 
> 
> 
> From: virtio-...@lists.oasis-open.org  on 
> behalf of Jiqian Chen 
> Sent: Monday, September 11, 2023 12:04 PM
> To: Gerd Hoffmann; Marc-André Lureau; Robert Beckett; 
> virtio-comm...@lists.oasis-open.org; virtio-...@lists.oasis-open.org
> Cc: qemu-devel@nongnu.org; linux-ker...@vger.kernel.org; Stefano Stabellini; 
> Roger Pau Monné; Alex Deucher; Christian Koenig; Stewart Hildebrand; Xenia 
> Ragiadakou; Honglei Huang; Julia Zhang; Huang Rui; Jiqian Chen
> Subject: [virtio-dev] [RESEND VIRTIO GPU PATCH v3 1/1] virtio-gpu: Add new 
> feature flag VIRTIO_GPU_F_FREEZE_S3
> 
> When we suspend/resume guest on Xen, the display can't come back.
> This is because when guest suspended, it called into Qemu. Then
> Qemu destroyed all resources which is used for display. So that
> guest's display can't come back to the time when it was suspended.
> 
> To solve above problem, I added a new mechanism that when guest is
> suspending, it will notify Qemu, and then Qemu will not destroy
> resourcesi which are created by using commands
> VIRTIO_GPU_CMD_RESOURCE_CREATE_*.
> 
> Due to that mechanism needs cooperation between guest and host,
> I need to add a new feature flag, so that guest and host can
> negotiate whenever freeze_S3 is supported or not.
> 
> Signed-off-by: Jiqian Chen 
> ---
>  device-types/gpu/description.tex | 42 
>  1 file changed, 42 insertions(+)
> 
> diff --git a/device-types/gpu/description.tex 
> b/device-types/gpu/description.tex
> index 4435248..1a137e7 100644
> --- a/device-types/gpu/description.tex
> +++ b/device-types/gpu/description.tex
> @@ -37,6 +37,8 @@ \subsection{Feature bits}\label{sec:Device Types / GPU 
> Device / Feature bits}
>resources is supported.
>  \item[VIRTIO_GPU_F_CONTEXT_INIT (4)] multiple context types and
>synchronization timelines supported.  Requires VIRTIO_GPU_F_VIRGL.
> +\item[VIRTIO_GPU_F_FREEZE_S3 (5)] freezing virtio-gpu and keeping resources
> +  alive is supported.
>  \end{description}
> 
>  \subsection{Device configuration layout}\label{sec:Device Types / GPU Device 
> / Device configuration layout}
> @@ -228,6 +230,9 @@ \subsubsection{Device Operation: Request 
> header}\label{sec:Device Types / GPU De
>  VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
>  VIRTIO_GPU_CMD_MOVE_CURSOR,
> 
> +/* freeze mode */
> +VIRTIO_GPU_CMD_SET_FREEZE_MODE = 0x0400,
> +
>  /* success responses */
>  VIRTIO_GPU_RESP_OK_NODATA = 0x1100,
>  VIRTIO_GPU_RESP_OK_DISPLAY_INFO,
> @@ -838,6 +843,43 @@ \subsubsection{Device Operation: 
> cursorq}\label{sec:Device Types / GPU Device /
> 
>  \end{description}
> 
> +\subsubsection{Device Operation: freeze_mode}\label{sec:Device Types / GPU 
> Device / Device Operation / Device Operation: freeze_mode}
> +
> +\begin{lstlisting}
> +typedef enum {
> +  VIRTIO_GPU_FREEZE_MODE_UNFREEZE = 0,
> +  VIRTIO_GPU_FREEZE_MODE_FREEZE_S3 = 3,
> +} virtio_gpu_freeze_mode_t;
> +
> +struct virtio_gpu_set_freeze_mode {
> +  struct virtio_gpu_ctrl_hdr hdr;
> +  virtio_gpu_freeze_mode_t freeze_mode;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +
> +\item[VIRTIO_GPU_CMD_SET_FREEZE_MODE]
> +Notify freeze mode through controlq.
> +Request data is \field{struct virtio_gpu_set_freeze_mode}.
> +Response type is VIRTIO_GPU_RESP_OK_NODATA.
> +
> +This is added for S3 func

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

[Chen, Jiqian]

Hi Parav,

On 2023/9/19 20:10, Parav Pandit wrote:
> Hi Jiqian,
> 
>> From: Jiqian Chen 
>> 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 
>> ---
>>  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.  

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

[Chen, Jiqian]

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

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 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-33c449b55...@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 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.
Can you send me your patch link on kernel and qemu side? I will take a deep 
look.

> 
> Thanks,
> Zhu Lingshan
>>>
>>> Signed-off-by: Jiqian Chen 
>>> ---
>>>   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-subscr...@lists.oasis-open.org
>> Unsubscribe: virtio-comment-unsubscr...@lists.oasis-open.org
>> List help: virtio-comment-h...@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
>> Committ

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-33c449b55...@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 
>>>>> ---
>>>>>    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 */
>>>>>    l

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,
It seems you reply to the wrong email thread. They are not related to my patch.

On 2023/9/20 15:06, Zhu, Lingshan wrote:
> 
> 
> On 9/20/2023 2:58 PM, Parav Pandit wrote:
>>> From: Chen, Jiqian 
>>> Sent: Wednesday, September 20, 2023 12:03 PM
>>> If driver write 0 to reset device, can the SUSPEND bit be cleared?
>> It must as reset operation, resets everything else and so the suspend too.
>>
>>> (pci_pm_resume->virtio_pci_restore->virtio_device_restore-
>>>> virtio_reset_device)
>>> 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.
>> Reset should not be influenced by suspend.
>> Suspend should do the work of suspend and reset to do the reset.
>>
>> The problem to overcome in [1] is, resume operation needs to be synchronous 
>> as it involves large part of context to resume back, and hence just 
>> asynchronously setting DRIVER_OK is not enough.
>> The sw must verify back that device has resumed the operation and ready to 
>> answer requests.
> this is not live migration, all device status and other information still 
> stay in the device, no need to "resume" context, just resume running.
> 
> Like resume from a failed LM.
>>
>> This is slightly different flow than setting the DRIVER_OK for the first 
>> time device initialization sequence as it does not involve large restoration.
>>
>> So, to merge two ideas, instead of doing DRIVER_OK to resume, the driver 
>> should clear the SUSPEND bit and verify that it is out of SUSPEND.
>>
>> Because driver is still in _OK_ driving the device flipping the SUSPEND bit.
> Please read the spec, it says:
> The driver MUST NOT clear a device status bit
> 
> 

-- 
Best regards,
Jiqian Chen.

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,
Please reply to your own email thread, below are not related to my patches. 
Thanks a lot.

On 2023/9/20 15:47, Zhu, Lingshan wrote:
> 
> 
> On 9/20/2023 3:35 PM, Parav Pandit wrote:
>>> From: Zhu, Lingshan 
>>> Sent: Wednesday, September 20, 2023 1:00 PM
>>>
>>> On 9/20/2023 3:24 PM, Chen, Jiqian wrote:
>>>> Hi Lingshan,
>>>> It seems you reply to the wrong email thread. They are not related to my
>>> patch.
>>> These reply to Parva's comments.
>>> @Parva, if you want to discuss more about live migration, please reply in my
>>> thread, lets don't flood here.
>> You made the point that "this is not live migration".
>> I am not discussing live migration in this thread.
>>
>> I replied for the point that device restoration from suspend for physical 
>> and hypevisor based device is not a 40nsec worth of work to restore by just 
>> doing a register write.
>> This is not live or device migration. This is restoring the device context 
>> initiated by the driver owning the device.
> restore the device context should be done by the hypervisor before setting 
> DRIVER_OK and waking up the guest, not a concern here, out of spec
> 

-- 
Best regards,
Jiqian Chen.

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]

On 2023/9/20 15:56, Parav Pandit wrote:
> Hi Jiquian,
> 
>> From: Chen, Jiqian 
>> Sent: Wednesday, September 20, 2023 1:24 PM
>>
>> Hi Lingshan,
>> Please reply to your own email thread, below are not related to my patches.
>> Thanks a lot.
> 
> They are related to your patch.
>  Both the patches have overlapping functionalities.
But Lingshan's patch is not meet my need. It clears the SUSPEND bit during 
reset.

> 
> You probably missed my previous response explaining the same at [1].
> 
> [1] https://lists.oasis-open.org/archives/virtio-comment/202309/msg00255.html
Yes, I didn't receive this response. 
After reading your responses, I think your concerns are below:
> The point is when driver tells to freeze, it is freeze command and not reset.
> So resume() should not invoke device_reset() when FREEZE+RESUME supported.
The modifications in my Qemu and kernel patches, I just set freeze_mode to be 
S3, and then when guest resume I can change the reset behavior according the S3 
mode, see below callstack:
pci_pm_resume
virtio_pci_restore
virtio_device_restore
virtio_reset_device(set 0 the device status and then 
call virtio_reset to reset device)
drv->restore(virtio_gpu_restore)
So, reset will be done during the restore process(resume invoke the reset), and 
I want to affect the reset behavior during the restore process.

> 

-- 
Best regards,
Jiqian Chen.

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

[Chen, Jiqian]

Hi Jason,

On 2023/9/21 12:22, Jason Wang wrote:
> On Tue, Sep 19, 2023 at 7:43 PM 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.
> 
> 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 
>> ---
>>  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: virtio-dev-unsubscr...@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
> 

-- 
Best regards,
Jiqian Chen.

Re: [QEMU PATCH v4 0/1] S3 support

[Chen, Jiqian]

Hi all,
Please forgive me for asking. Do you have any other comments on my latest 
version patches about virtio-gpu S3 on Xen? Looking forward to your reply.


On 2023/7/20 20:08, Jiqian Chen wrote:
> v4:
> 
> Hi all,
> Thanks for Gerd Hoffmann's advice. V4 makes below changes:
> * Use enum for freeze mode, so this can be extended with more
>   modes in the future.
> * Rename functions and paratemers with "_S3" postfix.
> And no functional changes.
> 
> latest version on kernel side:
> https://lore.kernel.org/lkml/20230720115805.8206-1-jiqian.c...@amd.com/T/#t
> 
> Best regards,
> Jiqian Chen.
> 
> 
> v3:
> link,
> https://lore.kernel.org/qemu-devel/20230719074726.1613088-1-jiqian.c...@amd.com/T/#t
> 
> Hi all,
> Thanks for Michael S. Tsirkin's advice. V3 makes below changes:
> * Remove changes in file include/standard-headers/linux/virtio_gpu.h
>   I am not supposed to edit this file and it will be imported after
>   the patches of linux kernel was merged.
> 
> 
> v2:
> link,
> https://lore.kernel.org/qemu-devel/20230630070016.841459-1-jiqian.c...@amd.com/T/#t
> 
> Hi all,
> Thanks to Marc-André Lureau, Robert Beckett and Gerd Hoffmann for
> their advice and guidance. V2 makes below changes:
> 
> * Change VIRTIO_CPU_CMD_STATUS_FREEZING to 0x0400 (<0x1000)
> * Add virtio_gpu_device_unrealize to destroy resources to solve
>   potential memory leak problem. This also needs hot-plug support.
> * Add a new feature flag VIRTIO_GPU_F_FREEZING, so that guest and
>   host can negotiate whenever freezing is supported or not.
> 
> v1:
> link,
> https://lore.kernel.org/qemu-devel/20230608025655.1674357-1-jiqian.c...@amd.com/
> 
> Hi all,
> I am working to implement virtgpu S3 function on Xen.
> 
> Currently on Xen, if we start a guest who enables virtgpu, and then
> run "echo mem > /sys/power/state" to suspend guest. And run
> "sudo xl trigger  s3resume" to resume guest. We can find that
> the guest kernel comes back, but the display doesn't. It just shown a
> black screen.
> 
> Through reading codes, I founded that when guest was during suspending,
> it called into Qemu to call virtio_gpu_gl_reset. In virtio_gpu_gl_reset,
> it destroyed all resources and reset renderer. This made the display
> gone after guest resumed.
> 
> I think we should keep resources or prevent they being destroyed when
> guest is suspending. So, I add a new status named freezing to virtgpu,
> and add a new ctrl message VIRTIO_GPU_CMD_STATUS_FREEZING to get
> notification from guest. If freezing is set to true, and then Qemu will
> realize that guest is suspending, it will not destroy resources and will
> not reset renderer. If freezing is set to false, Qemu will do its origin
> actions, and has no other impaction.
> 
> And now, display can come back and applications can continue their
> status after guest resumes.
> 
> Jiqian Chen (1):
>   virtgpu: do not destroy resources when guest suspend
> 
>  hw/display/virtio-gpu-base.c   |  3 ++
>  hw/display/virtio-gpu-gl.c | 10 ++-
>  hw/display/virtio-gpu-virgl.c  |  7 +
>  hw/display/virtio-gpu.c| 55 --
>  hw/virtio/virtio.c |  3 ++
>  include/hw/virtio/virtio-gpu.h |  6 
>  6 files changed, 81 insertions(+), 3 deletions(-)
> 

-- 
Best regards,
Jiqian Chen.

Re: [virtio-dev] [RESEND VIRTIO GPU PATCH v3 1/1] virtio-gpu: Add new feature flag VIRTIO_GPU_F_FREEZE_S3

[Chen, Jiqian]

Hi Mikhail Golubev-Ciuchea,

On 2023/9/12 00:33, Mikhail Golubev-Ciuchea wrote:
> Hi Jiqian,
> 
> Thanks for the proposal.
> 
> Some time ago I was working on the same issue with suspending the gpu device
> (on arm).  Additionally, I had troubles with virtio-video device as well, see
> https://lore.kernel.org/lkml/20211215172739.ga77...@opensynergy.com/T/ for
> details.
> 
> In your case, the
> VIRTIO_GPU_FREEZE_MODE_FREEZE_S3/VIRTIO_GPU_FREEZE_MODE_UNFREEZE do influence
> how reset is being handled by Qemu, is this correct?  Since multiple devices
Yes, it can affect the reset behavior of Qemu.

> can benefit from the same mechanism, would it be possible to: a) have a more
> generic, non ACPI-based name, b) make the feature generic, applicable to other
> devices as well?
It seems your opinion is the same as Parav Pandit's, both hope this feature can 
be more generic. 
If that's the case, its role and scope of influence will become quite large, 
and I'm not sure if that's appropriate.
I will try to modify the code to improve the level of this feature, if I can 
make it work, I will send a new series of patch.
And during this period, I am still looking forward to receiving more 
suggestions and guidance from everyone. Thank you!

> 
> 
> Best wishes,
> Mikhail Golubev-Ciuchea
> 
> 
> 
> --
> 
> Mikhail Golubev-Ciuchea
> 
> 
> OpenSynergy GmbH
> 
> Rotherstr. 20, 10245 Berlin
> 
> Telefon: +49 (30) 60 98 54 0 - 903
> 
> EMail:   mikhail.golu...@opensynergy.com
> 
> www.opensynergy.com
> 
> Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B
> 
> Geschäftsführer/Managing Director: Regis Adjamah
> 
> 
> 
> From: virtio-...@lists.oasis-open.org  on 
> behalf of Jiqian Chen 
> Sent: Monday, September 11, 2023 12:04 PM
> To: Gerd Hoffmann; Marc-André Lureau; Robert Beckett; 
> virtio-comm...@lists.oasis-open.org; virtio-...@lists.oasis-open.org
> Cc: qemu-devel@nongnu.org; linux-ker...@vger.kernel.org; Stefano Stabellini; 
> Roger Pau Monné; Alex Deucher; Christian Koenig; Stewart Hildebrand; Xenia 
> Ragiadakou; Honglei Huang; Julia Zhang; Huang Rui; Jiqian Chen
> Subject: [virtio-dev] [RESEND VIRTIO GPU PATCH v3 1/1] virtio-gpu: Add new 
> feature flag VIRTIO_GPU_F_FREEZE_S3
> 
> When we suspend/resume guest on Xen, the display can't come back.
> This is because when guest suspended, it called into Qemu. Then
> Qemu destroyed all resources which is used for display. So that
> guest's display can't come back to the time when it was suspended.
> 
> To solve above problem, I added a new mechanism that when guest is
> suspending, it will notify Qemu, and then Qemu will not destroy
> resourcesi which are created by using commands
> VIRTIO_GPU_CMD_RESOURCE_CREATE_*.
> 
> Due to that mechanism needs cooperation between guest and host,
> I need to add a new feature flag, so that guest and host can
> negotiate whenever freeze_S3 is supported or not.
> 
> Signed-off-by: Jiqian Chen 
> ---
>  device-types/gpu/description.tex | 42 
>  1 file changed, 42 insertions(+)
> 
> diff --git a/device-types/gpu/description.tex 
> b/device-types/gpu/description.tex
> index 4435248..1a137e7 100644
> --- a/device-types/gpu/description.tex
> +++ b/device-types/gpu/description.tex
> @@ -37,6 +37,8 @@ \subsection{Feature bits}\label{sec:Device Types / GPU 
> Device / Feature bits}
>resources is supported.
>  \item[VIRTIO_GPU_F_CONTEXT_INIT (4)] multiple context types and
>synchronization timelines supported.  Requires VIRTIO_GPU_F_VIRGL.
> +\item[VIRTIO_GPU_F_FREEZE_S3 (5)] freezing virtio-gpu and keeping resources
> +  alive is supported.
>  \end{description}
> 
>  \subsection{Device configuration layout}\label{sec:Device Types / GPU Device 
> / Device configuration layout}
> @@ -228,6 +230,9 @@ \subsubsection{Device Operation: Request 
> header}\label{sec:Device Types / GPU De
>  VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
>  VIRTIO_GPU_CMD_MOVE_CURSOR,
> 
> +/* freeze mode */
> +VIRTIO_GPU_CMD_SET_FREEZE_MODE = 0x0400,
> +
>  /* success responses */
>  VIRTIO_GPU_RESP_OK_NODATA = 0x1100,
>  VIRTIO_GPU_RESP_OK_DISPLAY_INFO,
> @@ -838,6 +843,43 @@ \subsubsection{Device Operation: 
> cursorq}\label{sec:Device Types / GPU Device /
> 
>  \end{description}
> 
> +\subsubsection{Device Operation: freeze_mode}\label{sec:Device Types / GPU 
> Device / Device Operation / Device Operation: freeze_mode}
> +
> +\begin{lstlisting}
> +typedef enum {
> +  VIRTIO_GPU_FREEZE_MODE_UNFREEZE = 0,
> +  VIRTIO_GPU_FREEZE_MODE_FREEZE_S3 = 3,
> +} virtio_gpu_freeze_mode_t;
> +
> +struct virtio_gpu_set_freeze_mode {
> +  struct virtio_gpu_ctrl_hdr hdr;
> +  virtio_gpu_freeze_mode_t freeze_mode;
> +};
> +\end{lstlisting}
> +
> +\begin{description}
> +
> +\item[VIRTIO_GPU_CMD_SET_FREEZE_MODE]
> +Notify freeze mode through controlq.
> +Request data is \field{struct virtio_gpu_set_freeze_mode}.
> +Response type is

Re: [RFC QEMU PATCH v3 1/1] xen: Use gsi instead of irq for mapping pirq

[Chen, Jiqian]

On 2023/12/11 23:33, Roger Pau Monné wrote:
> On Mon, Dec 11, 2023 at 12:52:40AM +0800, Jiqian Chen wrote:
>> In PVH dom0, it uses the linux local interrupt mechanism,
>> when it allocs irq for a gsi, it is dynamic, and follow
>> the principle of applying first, distributing first. And
>> the irq number is alloced from small to large, but the
>> applying gsi number is not, may gsi 38 comes before gsi
>> 28, that causes the irq number is not equal with the gsi
>> number. And when passthrough a device, qemu wants to use
>> gsi to map pirq, xen_pt_realize->xc_physdev_map_pirq, but
>> the gsi number is got from file
>> /sys/bus/pci/devices//irq in current code, so it
>> will fail when mapping.
>>
>> Use real gsi number read from gsi sysfs.
>>
>> Co-developed-by: Huang Rui 
>> Signed-off-by: Jiqian Chen 
>> ---
>>  hw/xen/xen-host-pci-device.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/xen/xen-host-pci-device.c b/hw/xen/xen-host-pci-device.c
>> index 8c6e9a1716..e270ac2631 100644
>> --- a/hw/xen/xen-host-pci-device.c
>> +++ b/hw/xen/xen-host-pci-device.c
>> @@ -364,7 +364,7 @@ void xen_host_pci_device_get(XenHostPCIDevice *d, 
>> uint16_t domain,
>>  }
>>  d->device_id = v;
>>  
>> -xen_host_pci_get_dec_value(d, "irq", &v, errp);
>> +xen_host_pci_get_dec_value(d, "gsi", &v, errp);
> 
> Don't you need to fallthrough to use the irq number on failure?
> Otherwise passthrough won't work on older Linux versions that don't
> expose the gsi node.
You are right, I will use the irq if there isn't a gsi sysfs, in next version. 
Thank you.

> 
> Thanks, Roger.

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v9 1/2] virtio-pci: only reset pm state during resetting

[Chen, Jiqian]

Hi,

On 2024/4/16 15:01, Jiqian Chen wrote:
> Fix bug imported by 27ce0f3afc9dd25d21b43bbce505157afd93d111
> (fix Power Management Control Register for PCI Express virtio devices)
> 
> Only state of PM_CTRL is writable.
> Only when flag VIRTIO_PCI_FLAG_INIT_PM is set, need to reset state.
> 
> Signed-off-by: Jiqian Chen 
> ---
>  hw/virtio/virtio-pci.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index cb159fd0785c..a1b61308e7a0 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2300,10 +2300,16 @@ static void virtio_pci_bus_reset_hold(Object *obj)
>  virtio_pci_reset(qdev);
>  
>  if (pci_is_express(dev)) {
> +VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
> +
>  pcie_cap_deverr_reset(dev);
>  pcie_cap_lnkctl_reset(dev);
>  
> -pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0);
> +if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
> +pci_word_test_and_clear_mask(
> +dev->config + dev->exp.pm_cap + PCI_PM_CTRL,
> +PCI_PM_CTRL_STATE_MASK);
> +}
>  }
>  }
>  

Do you have any other doubts about this patch?
And another patch of this series?

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v9 1/2] virtio-pci: only reset pm state during resetting

[Chen, Jiqian]

On 2024/5/10 18:18, Michael S. Tsirkin wrote:
> On Tue, Apr 16, 2024 at 03:01:26PM +0800, Jiqian Chen wrote:
>> Fix bug imported by 27ce0f3afc9dd25d21b43bbce505157afd93d111
>> (fix Power Management Control Register for PCI Express virtio devices)
> 
> 
> should be:
> 
> 27ce0f3afc9dd ("fix Power Management Control Register for PCI Express virtio 
> devices"
> 
> Pls include a bit more info about the bug: after this change, we observe 
> .. .
OK, I will modify in next version.
How about the other patch of this series?
Do you have any doubts?

> 
>> Only state of PM_CTRL is writable.
>> Only when flag VIRTIO_PCI_FLAG_INIT_PM is set, need to reset state.
> 
> 
> 
> and here, add:
> 
> Fixes: 27ce0f3afc9dd ("fix Power Management Control Register for PCI Express 
> virtio devices"
> 
>> Signed-off-by: Jiqian Chen 
>> ---
>>  hw/virtio/virtio-pci.c | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index cb159fd0785c..a1b61308e7a0 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2300,10 +2300,16 @@ static void virtio_pci_bus_reset_hold(Object *obj)
>>  virtio_pci_reset(qdev);
>>  
>>  if (pci_is_express(dev)) {
>> +VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
>> +
>>  pcie_cap_deverr_reset(dev);
>>  pcie_cap_lnkctl_reset(dev);
>>  
>> -pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0);
>> +if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
>> +pci_word_test_and_clear_mask(
>> +dev->config + dev->exp.pm_cap + PCI_PM_CTRL,
>> +PCI_PM_CTRL_STATE_MASK);
>> +}
>>  }
>>  }
>>  
>> -- 
>> 2.34.1
> 

-- 
Best regards,
Jiqian Chen.

Re: [QEMU PATCH v6 1/1] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

Hi Michael S. Tsirkin,
Do you have any comments on this patch?

On 2024/2/22 11:40, Jiqian Chen wrote:
> In current code, when guest does S3, virtio devices are reset due to
> the bit No_Soft_Reset is not set. After resetting, the display resources
> of virtio-gpu are destroyed, then the display can't come back and only
> show blank after resuming.
> 
> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check
> this bit, if this bit is set, the devices resetting will not be done, and
> then the display can work after resuming.
> 
> Signed-off-by: Jiqian Chen 
> ---
>  hw/virtio/virtio-pci.c | 37 +-
>  include/hw/virtio/virtio-pci.h |  5 +
>  2 files changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 1a7039fb0c68..da5312010345 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2197,6 +2197,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
> Error **errp)
>  pcie_cap_lnkctl_init(pci_dev);
>  }
>  
> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) {
> +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL,
> + PCI_PM_CTRL_NO_SOFT_RESET);
> +}
> +
>  if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
>  /* Init Power Management Control Register */
>  pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL,
> @@ -2259,18 +2264,46 @@ static void virtio_pci_reset(DeviceState *qdev)
>  }
>  }
>  
> +static bool device_no_need_reset(PCIDevice *dev)
> +{
> +if (pci_is_express(dev)) {
> +uint16_t pmcsr;
> +
> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL);
> +/*
> + * When No_Soft_Reset bit is set and device
> + * is in D3hot state, can't reset device
> + */
> +if ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) &&
> +(pmcsr & PCI_PM_CTRL_STATE_MASK) == 3)
> +return true;
> +}
> +
> +return false;
> +}
> +
>  static void virtio_pci_bus_reset_hold(Object *obj)
>  {
>  PCIDevice *dev = PCI_DEVICE(obj);
>  DeviceState *qdev = DEVICE(obj);
>  
> +if (device_no_need_reset(dev))
> +return;
> +
>  virtio_pci_reset(qdev);
>  
>  if (pci_is_express(dev)) {
> +uint16_t pmcsr;
> +uint16_t val = 0;
> +
>  pcie_cap_deverr_reset(dev);
>  pcie_cap_lnkctl_reset(dev);
>  
> -pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0);
> +/* don't reset the RO bits */
> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL);
> +val = val | (pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) |
> +(pmcsr & PCI_PM_CTRL_DATA_SCALE_MASK);
> +pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val);
>  }
>  }
>  
> @@ -2297,6 +2330,8 @@ static Property virtio_pci_properties[] = {
>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, true),
>  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>  VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>  DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
> index 59d88018c16a..9e67ba38c748 100644
> --- a/include/hw/virtio/virtio-pci.h
> +++ b/include/hw/virtio/virtio-pci.h
> @@ -43,6 +43,7 @@ enum {
>  VIRTIO_PCI_FLAG_INIT_FLR_BIT,
>  VIRTIO_PCI_FLAG_AER_BIT,
>  VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT,
> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT,
>  };
>  
>  /* Need to activate work-arounds for buggy guests at vmstate load. */
> @@ -79,6 +80,10 @@ enum {
>  /* Init Power Management */
>  #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT)
>  
> +/* Init The No_Soft_Reset bit of Power Management */
> +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \
> +  (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT)
> +
>  /* Init Function Level Reset capability */
>  #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT)
>  

-- 
Best regards,
Jiqian Chen.

Re: [QEMU PATCH v6 1/1] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/3/18 16:04, Michael S. Tsirkin wrote:
> On Thu, Feb 22, 2024 at 11:40:10AM +0800, Jiqian Chen wrote:
>> In current code, when guest does S3, virtio devices are reset due to
>> the bit No_Soft_Reset is not set. After resetting, the display resources
>> of virtio-gpu are destroyed, then the display can't come back and only
>> show blank after resuming.
>>
>> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check
>> this bit, if this bit is set, the devices resetting will not be done, and
>> then the display can work after resuming.
>>
>> Signed-off-by: Jiqian Chen 
>> ---
>>  hw/virtio/virtio-pci.c | 37 +-
>>  include/hw/virtio/virtio-pci.h |  5 +
>>  2 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 1a7039fb0c68..da5312010345 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2197,6 +2197,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
>> Error **errp)
>>  pcie_cap_lnkctl_init(pci_dev);
>>  }
>>  
>> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) {
>> +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL,
>> + PCI_PM_CTRL_NO_SOFT_RESET);
>> +}
>> +
>>  if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
>>  /* Init Power Management Control Register */
>>  pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL,
> 
> 
> Don't we need compat machinery to avoid breaking migration for
> existing machine types?
Could you elaborate on it? I am sorry I don't know which machine types to be 
compatible with, and how to be compatible with them.
Can I simply set the default value of x-pcie-pm-no-soft-reset to false? If 
someone need this bit, they can set x-pcie-pm-no-soft-reset=true in the config 
parameters for Qemu. So that will not affect exiting machine types?

> 
>> @@ -2259,18 +2264,46 @@ static void virtio_pci_reset(DeviceState *qdev)
>>  }
>>  }
>>  
>> +static bool device_no_need_reset(PCIDevice *dev)
>> +{
>> +if (pci_is_express(dev)) {
>> +uint16_t pmcsr;
>> +
>> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL);
>> +/*
>> + * When No_Soft_Reset bit is set and device
> 
> the device
Will change in next version.
> 
>> + * is in D3hot state, can't reset device
> 
> can't? or don't?
Will change to "don't" in next version.

> 
>> + */
>> +if ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) &&
>> +(pmcsr & PCI_PM_CTRL_STATE_MASK) == 3)
>> +return true;
> 
> coding style violation
Will add braces {} in next version.

> 
>> +}
>> +
>> +return false;
>> +}
>> +
>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>  {
>>  PCIDevice *dev = PCI_DEVICE(obj);
>>  DeviceState *qdev = DEVICE(obj);
>>  
>> +if (device_no_need_reset(dev))
>> +return;
>> +
>>  virtio_pci_reset(qdev);
>>  
>>  if (pci_is_express(dev)) {
>> +uint16_t pmcsr;
>> +uint16_t val = 0;
>> +
>>  pcie_cap_deverr_reset(dev);
>>  pcie_cap_lnkctl_reset(dev);
>>  
>> -pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0);
>> +/* don't reset the RO bits */
>> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL);
>> +val = val | (pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) |
>> +(pmcsr & PCI_PM_CTRL_DATA_SCALE_MASK);
>> +pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val);
> 
> First, we have test and clear for this.
> 
> Second, this has to be conditional on the flag, no?
Before resetting, I first read the value of config, its function is equivalent 
to checking the label, right?
If the flag is set, the value of No_Soft_Reset is 1, the bit of "val" is also 1.
If the flag is not set, the value of No_Soft_Reset is 0, "val" is also 0.

> Without the flag don't we reset everything?
We need reset everything include the RO bit, right?
If so, that is my fault, then I will change to check the flag of proxy in next 
version.

> Or you can reuse wmask for this, also an option.
> 
> Also what's going on with PCI_PM_CTRL_DATA_SCALE_MASK?
> Where does that come from?
I read from PCI spec, the bit DATA_SCALE and No_Soft_Reset are both RO bit, so 
I thought we shouldn't reset them.
It is something wrong with my understanding, I will remove 
PCI_PM_CTRL_DATA_SCALE_MASK in next version. Sorry.

> 
>>  }
>>  }
>>  
>> @@ -2297,6 +2330,8 @@ static Property virtio_pci_properties[] = {
>>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
>> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, true),
>>  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCI

Re: [PATCH v11 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

Hi MST,

On 2024/6/6 18:22, Jiqian Chen wrote:
> In current code, when guest does S3, virtio-gpu are reset due to the
> bit No_Soft_Reset is not set. After resetting, the display resources
> of virtio-gpu are destroyed, then the display can't come back and only
> show blank after resuming.
> 
> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check
> this bit, if this bit is set, the devices resetting will not be done, and
> then the display can work after resuming.
> 
> No_Soft_Reset bit is implemented for all virtio devices, and was tested
> only on virtio-gpu device. Set it false by default for safety.
> 
> Signed-off-by: Jiqian Chen 
> ---
>  hw/core/machine.c  |  1 +
>  hw/virtio/virtio-pci.c | 29 +
>  include/hw/virtio/virtio-pci.h |  5 +
>  3 files changed, 35 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 77a356f232f5..b6af94edcd0a 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -36,6 +36,7 @@
>  GlobalProperty hw_compat_9_0[] = {
>  {"arm-cpu", "backcompat-cntfrq", "true" },
>  {"vfio-pci", "skip-vsc-check", "false" },
> +{ "virtio-pci", "x-pcie-pm-no-soft-reset", "off" },
>  };
>  const size_t hw_compat_9_0_len = G_N_ELEMENTS(hw_compat_9_0);
>  
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 1b63bcb3f15c..c881f853253c 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2230,6 +2230,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
> Error **errp)
>  pcie_cap_lnkctl_init(pci_dev);
>  }
>  
> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) {
> +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL,
> + PCI_PM_CTRL_NO_SOFT_RESET);
> +}
> +
>  if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
>  /* Init Power Management Control Register */
>  pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL,
> @@ -2292,11 +2297,33 @@ static void virtio_pci_reset(DeviceState *qdev)
>  }
>  }
>  
> +static bool virtio_pci_no_soft_reset(PCIDevice *dev)
> +{
> +uint16_t pmcsr;
> +
> +if (!pci_is_express(dev) || !dev->exp.pm_cap) {
> +return false;
> +}
> +
> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL);
> +
> +/*
> + * When No_Soft_Reset bit is set and the device
> + * is in D3hot state, don't reset device
> + */
> +return (pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) &&
> +   (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3;
> +}
> +
>  static void virtio_pci_bus_reset_hold(Object *obj, ResetType type)
>  {
>  PCIDevice *dev = PCI_DEVICE(obj);
>  DeviceState *qdev = DEVICE(obj);
>  
> +if (virtio_pci_no_soft_reset(dev)) {
> +return;
> +}
> +
>  virtio_pci_reset(qdev);
>  
>  if (pci_is_express(dev)) {
> @@ -2336,6 +2363,8 @@ static Property virtio_pci_properties[] = {
>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
>  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>  VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>  DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
> index 59d88018c16a..9e67ba38c748 100644
> --- a/include/hw/virtio/virtio-pci.h
> +++ b/include/hw/virtio/virtio-pci.h
> @@ -43,6 +43,7 @@ enum {
>  VIRTIO_PCI_FLAG_INIT_FLR_BIT,
>  VIRTIO_PCI_FLAG_AER_BIT,
>  VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT,
> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT,
>  };
>  
>  /* Need to activate work-arounds for buggy guests at vmstate load. */
> @@ -79,6 +80,10 @@ enum {
>  /* Init Power Management */
>  #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT)
>  
> +/* Init The No_Soft_Reset bit of Power Management */
> +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \
> +  (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT)
> +
>  /* Init Function Level Reset capability */
>  #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT)
>  
I have added compatibility for old machine.
Do you have any other concerns about this patch?

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v11 1/2] virtio-pci: only reset pm state during resetting

[Chen, Jiqian]

Hi MST,

On 2024/6/6 18:22, Jiqian Chen wrote:
> Fix bug imported by 27ce0f3afc9dd ("fix Power Management Control Register for 
> PCI Express virtio devices"
> After this change, observe that QEMU may erroneously clear the power status 
> of the device,
> or may erroneously clear non writable registers, such as NO_SOFT_RESET, etc.
> 
> Only state of PM_CTRL is writable.
> Only when flag VIRTIO_PCI_FLAG_INIT_PM is set, need to reset state.
> 
> Fixes: 27ce0f3afc9dd ("fix Power Management Control Register for PCI Express 
> virtio devices"
> 
> Signed-off-by: Jiqian Chen 
> ---
>  hw/virtio/virtio-pci.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index b1d02f4b3de0..1b63bcb3f15c 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2300,10 +2300,16 @@ static void virtio_pci_bus_reset_hold(Object *obj, 
> ResetType type)
>  virtio_pci_reset(qdev);
>  
>  if (pci_is_express(dev)) {
> +VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
> +
>  pcie_cap_deverr_reset(dev);
>  pcie_cap_lnkctl_reset(dev);
>  
> -pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0);
> +if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
> +pci_word_test_and_clear_mask(
> +dev->config + dev->exp.pm_cap + PCI_PM_CTRL,
> +PCI_PM_CTRL_STATE_MASK);
> +}
>  }
>  }
>  
I noticed that you merged this patch into the staging before, but then reverted 
it. Do you still have any concerns?

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v11 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/6/21 17:20, Chen, Jiqian wrote:
> Hi MST,
> 
> On 2024/6/6 18:22, Jiqian Chen wrote:
>> In current code, when guest does S3, virtio-gpu are reset due to the
>> bit No_Soft_Reset is not set. After resetting, the display resources
>> of virtio-gpu are destroyed, then the display can't come back and only
>> show blank after resuming.
>>
>> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check
>> this bit, if this bit is set, the devices resetting will not be done, and
>> then the display can work after resuming.
>>
>> No_Soft_Reset bit is implemented for all virtio devices, and was tested
>> only on virtio-gpu device. Set it false by default for safety.
>>
>> Signed-off-by: Jiqian Chen 
>> ---
>>  hw/core/machine.c  |  1 +
>>  hw/virtio/virtio-pci.c | 29 +
>>  include/hw/virtio/virtio-pci.h |  5 +
>>  3 files changed, 35 insertions(+)
>>
>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>> index 77a356f232f5..b6af94edcd0a 100644
>> --- a/hw/core/machine.c
>> +++ b/hw/core/machine.c
>> @@ -36,6 +36,7 @@
>>  GlobalProperty hw_compat_9_0[] = {
>>  {"arm-cpu", "backcompat-cntfrq", "true" },
>>  {"vfio-pci", "skip-vsc-check", "false" },
>> +{ "virtio-pci", "x-pcie-pm-no-soft-reset", "off" },
>>  };
>>  const size_t hw_compat_9_0_len = G_N_ELEMENTS(hw_compat_9_0);
>>  
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 1b63bcb3f15c..c881f853253c 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2230,6 +2230,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
>> Error **errp)
>>  pcie_cap_lnkctl_init(pci_dev);
>>  }
>>  
>> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) {
>> +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL,
>> + PCI_PM_CTRL_NO_SOFT_RESET);
>> +}
>> +
>>  if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
>>  /* Init Power Management Control Register */
>>  pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL,
>> @@ -2292,11 +2297,33 @@ static void virtio_pci_reset(DeviceState *qdev)
>>  }
>>  }
>>  
>> +static bool virtio_pci_no_soft_reset(PCIDevice *dev)
>> +{
>> +uint16_t pmcsr;
>> +
>> +if (!pci_is_express(dev) || !dev->exp.pm_cap) {
>> +return false;
>> +}
>> +
>> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL);
>> +
>> +/*
>> + * When No_Soft_Reset bit is set and the device
>> + * is in D3hot state, don't reset device
>> + */
>> +return (pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) &&
>> +   (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3;
>> +}
>> +
>>  static void virtio_pci_bus_reset_hold(Object *obj, ResetType type)
>>  {
>>  PCIDevice *dev = PCI_DEVICE(obj);
>>  DeviceState *qdev = DEVICE(obj);
>>  
>> +if (virtio_pci_no_soft_reset(dev)) {
>> +return;
>> +}
>> +
>>  virtio_pci_reset(qdev);
>>  
>>  if (pci_is_express(dev)) {
>> @@ -2336,6 +2363,8 @@ static Property virtio_pci_properties[] = {
>>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
>> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
>>  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>>  VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>>  DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
>> index 59d88018c16a..9e67ba38c748 100644
>> --- a/include/hw/virtio/virtio-pci.h
>> +++ b/include/hw/virtio/virtio-pci.h
>> @@ -43,6 +43,7 @@ enum {
>>  VIRTIO_PCI_FLAG_INIT_FLR_BIT,
>>  VIRTIO_PCI_FLAG_AER_BIT,
>>  VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT,
>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT,
>>  };
>>  
>>  /* Need to activate work-arounds for buggy guests at vmstate load. */
>> @@ -79,6 +80,10 @@ enum {
>>  /* Init Power Management */
>>  #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT)
>>  
>> +/* Init The No_Soft_Reset bit of Power Management */
>> +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \
>> +  (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT)
>> +
>>  /* Init Function Level Reset capability */
>>  #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT)
>>  
> I have added compatibility for old machine.
> Do you have any other concerns about this patch?
> 
If you don't have other concerns. May I get your Review-by?

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v7 1/1] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

Hi MST,
I modified this patch according to your comments. Do you have any other 
suggestions?

On 2024/3/25 15:07, Jiqian Chen wrote:
> In current code, when guest does S3, virtio devices are reset due to
> the bit No_Soft_Reset is not set. After resetting, the display resources
> of virtio-gpu are destroyed, then the display can't come back and only
> show blank after resuming.
> 
> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check
> this bit, if this bit is set, the devices resetting will not be done, and
> then the display can work after resuming.
> 
> Signed-off-by: Jiqian Chen 
> ---
>  hw/virtio/virtio-pci.c | 38 +-
>  include/hw/virtio/virtio-pci.h |  5 +
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 1a7039fb0c68..daafda315f8c 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2197,6 +2197,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
> Error **errp)
>  pcie_cap_lnkctl_init(pci_dev);
>  }
>  
> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) {
> +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL,
> + PCI_PM_CTRL_NO_SOFT_RESET);
> +}
> +
>  if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
>  /* Init Power Management Control Register */
>  pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL,
> @@ -2259,18 +2264,47 @@ static void virtio_pci_reset(DeviceState *qdev)
>  }
>  }
>  
> +static bool device_no_need_reset(PCIDevice *dev)
> +{
> +if (pci_is_express(dev)) {
> +uint16_t pmcsr;
> +
> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL);
> +/*
> + * When No_Soft_Reset bit is set and the device
> + * is in D3hot state, don't reset device
> + */
> +if ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) &&
> +(pmcsr & PCI_PM_CTRL_STATE_MASK) == 3) {
> +return true;
> +}
> +}
> +
> +return false;
> +}
> +
>  static void virtio_pci_bus_reset_hold(Object *obj)
>  {
>  PCIDevice *dev = PCI_DEVICE(obj);
>  DeviceState *qdev = DEVICE(obj);
>  
> +if (device_no_need_reset(dev)) {
> +return;
> +}
> +
>  virtio_pci_reset(qdev);
>  
>  if (pci_is_express(dev)) {
> +uint16_t val = 0;
> +VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
> +
>  pcie_cap_deverr_reset(dev);
>  pcie_cap_lnkctl_reset(dev);
>  
> -pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0);
> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) {
> +val |= PCI_PM_CTRL_NO_SOFT_RESET;
> +}
> +pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val);
>  }
>  }
>  
> @@ -2297,6 +2331,8 @@ static Property virtio_pci_properties[] = {
>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
>  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>  VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>  DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
> index 59d88018c16a..9e67ba38c748 100644
> --- a/include/hw/virtio/virtio-pci.h
> +++ b/include/hw/virtio/virtio-pci.h
> @@ -43,6 +43,7 @@ enum {
>  VIRTIO_PCI_FLAG_INIT_FLR_BIT,
>  VIRTIO_PCI_FLAG_AER_BIT,
>  VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT,
> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT,
>  };
>  
>  /* Need to activate work-arounds for buggy guests at vmstate load. */
> @@ -79,6 +80,10 @@ enum {
>  /* Init Power Management */
>  #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT)
>  
> +/* Init The No_Soft_Reset bit of Power Management */
> +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \
> +  (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT)
> +
>  /* Init Function Level Reset capability */
>  #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT)
>  

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v7 1/1] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/3/28 16:11, Michael S. Tsirkin wrote:
> 
>> +VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);
>> +
>>  pcie_cap_deverr_reset(dev);
>>  pcie_cap_lnkctl_reset(dev);
>>  
>> -pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, 0);
>> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) {
>> +val |= PCI_PM_CTRL_NO_SOFT_RESET;
>> +}
>> +pci_set_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL, val);
> 
> 
> There is no need to do it like this - only state is writeable
> anyway. So simply
>   pci_word_test_and_clear_mask(dev->config + dev->exp.pm_cap + 
> PCI_PM_CTRL, PCI_PM_CTRL_STATE_MASK)
> 
> 
> maybe we should actually check here:
>if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM)
> there's a chance commit 27ce0f3afc9 broke things for old machines
> and we never noticed. If so that should be a separate bugfix patch though.
Make sense. It is actually a bug imported by 27ce0f3afc9.
According to your comments, I think here should be a separate patch, like:
   if (pci_is_express(dev)) {
VirtIOPCIProxy *proxy = VIRTIO_PCI(dev);

pcie_cap_deverr_reset(dev);
pcie_cap_lnkctl_reset(dev);

if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
pci_word_test_and_clear_mask(
dev->config + dev->exp.pm_cap + PCI_PM_CTRL,
PCI_PM_CTRL_STATE_MASK);
}
}
Right?

> 
> 

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/3/28 18:57, Michael S. Tsirkin wrote:
> On Thu, Mar 28, 2024 at 06:39:03PM +0800, Jiqian Chen wrote:
>> In current code, when guest does S3, virtio devices are reset due to
>> the bit No_Soft_Reset is not set. After resetting, the display resources
>> of virtio-gpu are destroyed, then the display can't come back and only
>> show blank after resuming.
>>
>> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check
>> this bit, if this bit is set, the devices resetting will not be done, and
>> then the display can work after resuming.
>>
>> Signed-off-by: Jiqian Chen 
>> ---
>>  hw/virtio/virtio-pci.c | 29 +
>>  include/hw/virtio/virtio-pci.h |  5 +
>>  2 files changed, 34 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 05dd03758d9f..8d9fab855c7d 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2378,6 +2378,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
>> Error **errp)
>>  pcie_cap_lnkctl_init(pci_dev);
>>  }
>>  
>> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) {
>> +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL,
>> + PCI_PM_CTRL_NO_SOFT_RESET);
>> +}
>> +
>>  if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
>>  /* Init Power Management Control Register */
>>  pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL,
>> @@ -2440,11 +2445,33 @@ static void virtio_pci_reset(DeviceState *qdev)
>>  }
>>  }
>>  
>> +static bool virtio_pci_no_soft_reset(PCIDevice *dev)
>> +{
>> +uint16_t pmcsr;
>> +
>> +if (!pci_is_express(dev) || !dev->exp.pm_cap) {
>> +return false;
>> +}
>> +
>> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL);
>> +
>> +/*
>> + * When No_Soft_Reset bit is set and the device
>> + * is in D3hot state, don't reset device
>> + */
>> +return ((pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) &&
>> +(pmcsr & PCI_PM_CTRL_STATE_MASK) == 3);
> 
> 
> No need for () around return value.
Ok, will delete in next version.

> 
>> +}
>> +
>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>  {
>>  PCIDevice *dev = PCI_DEVICE(obj);
>>  DeviceState *qdev = DEVICE(obj);
>>  
>> +if (virtio_pci_no_soft_reset(dev)) {
>> +return;
>> +}
>> +
>>  virtio_pci_reset(qdev);
>>  
>>  if (pci_is_express(dev)) {
>> @@ -2484,6 +2511,8 @@ static Property virtio_pci_properties[] = {
>>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
>> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
>>  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>>  VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>>  DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
> 
> I am a bit confused about this part.
> Do you want to make this software controllable?
Yes, because even the real hardware, this bit is not always set.

> Or should this be set to true by default and then
> changed to false for old machine types?
How can I do so?
Do you mean set this to true by default, and if old machine types don't need 
this bit, they can pass false config to qemu when running qemu?

> 
> 
>> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
>> index 4d57a9c75130..c758eb738234 100644
>> --- a/include/hw/virtio/virtio-pci.h
>> +++ b/include/hw/virtio/virtio-pci.h
>> @@ -44,6 +44,7 @@ enum {
>>  VIRTIO_PCI_FLAG_AER_BIT,
>>  VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT,
>>  VIRTIO_PCI_FLAG_VDPA_BIT,
>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT,
>>  };
>>  
>>  /* Need to activate work-arounds for buggy guests at vmstate load. */
>> @@ -80,6 +81,10 @@ enum {
>>  /* Init Power Management */
>>  #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT)
>>  
>> +/* Init The No_Soft_Reset bit of Power Management */
>> +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \
>> +  (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT)
>> +
>>  /* Init Function Level Reset capability */
>>  #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT)
>>  
>> -- 
>> 2.34.1
> 

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/3/28 20:36, Michael S. Tsirkin wrote:
 +}
 +
  static void virtio_pci_bus_reset_hold(Object *obj)
  {
  PCIDevice *dev = PCI_DEVICE(obj);
  DeviceState *qdev = DEVICE(obj);
  
 +if (virtio_pci_no_soft_reset(dev)) {
 +return;
 +}
 +
  virtio_pci_reset(qdev);
  
  if (pci_is_express(dev)) {
 @@ -2484,6 +2511,8 @@ static Property virtio_pci_properties[] = {
  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
 +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
 +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
  VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
  DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>>>
>>> I am a bit confused about this part.
>>> Do you want to make this software controllable?
>> Yes, because even the real hardware, this bit is not always set.
> 
> So which virtio devices should and which should not set this bit?
This depends on the scenario the virtio-device is used, if we want to trigger 
an internal soft reset for the virtio-device during S3, this bit shouldn't be 
set.
In my use case on my environment, I don't want to reset virtio-gpu during S3,
because once the display resources are destroyed, there are not enough 
information to re-create them, so this bit should be set.
Making this bit software controllable is convenient for users to take their own 
choices.

> 
>>> Or should this be set to true by default and then
>>> changed to false for old machine types?
>> How can I do so?
>> Do you mean set this to true by default, and if old machine types don't need 
>> this bit, they can pass false config to qemu when running qemu?
> 
> No, you would use compat machinery. See how is x-pcie-flr-init handled.
> 
> 

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/3/29 15:20, Jason Wang wrote:
> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian  wrote:
>>
>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
>>>>>> +}
>>>>>> +
>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>>>>>  {
>>>>>>  PCIDevice *dev = PCI_DEVICE(obj);
>>>>>>  DeviceState *qdev = DEVICE(obj);
>>>>>>
>>>>>> +if (virtio_pci_no_soft_reset(dev)) {
>>>>>> +return;
>>>>>> +}
>>>>>> +
>>>>>>  virtio_pci_reset(qdev);
>>>>>>
>>>>>>  if (pci_is_express(dev)) {
>>>>>> @@ -2484,6 +2511,8 @@ static Property virtio_pci_properties[] = {
>>>>>>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>>>>>>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>>>>>>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
>>>>>> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
>>>>>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
> 
> Why does it come with an x prefix?
Sorry, it's my misunderstanding of this prefix, if No_Soft_Reset doesn't need 
this prefix, I will delete it in next version.
Does x prefix means compat machinery? Or other meanings?

> 
>>>>>>  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>>>>>>  VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>>>>>>  DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>>>>>
>>>>> I am a bit confused about this part.
>>>>> Do you want to make this software controllable?
>>>> Yes, because even the real hardware, this bit is not always set.
> 
> We are talking about emulated devices here.
Yes, I just gave an example. It actually this bit is not always set. What's 
your opinion about when to set this bit or which virtio-device should set this 
bit?

> 
>>>
>>> So which virtio devices should and which should not set this bit?
>> This depends on the scenario the virtio-device is used, if we want to 
>> trigger an internal soft reset for the virtio-device during S3, this bit 
>> shouldn't be set.
> 
> If the device doesn't need reset, why bother the driver for this?
I don't know what you mean.
If the device doesn't need reset, we can config true to set this bit, then on 
the driver side, driver finds this bit is set, then driver will not trigger a 
soft reset.

> 
> Btw, no_soft_reset is insufficient for some cases, 
May I know which cases?

> there's a proposal for the virtio-spec. I think we need to wait until it is 
> done.
Can you share the proposal?

> 
>> In my use case on my environment, I don't want to reset virtio-gpu during S3,
>> because once the display resources are destroyed, there are not enough 
>> information to re-create them, so this bit should be set.
>> Making this bit software controllable is convenient for users to take their 
>> own choices.
> 
> Thanks
> 
>>
>>>
>>>>> Or should this be set to true by default and then
>>>>> changed to false for old machine types?
>>>> How can I do so?
>>>> Do you mean set this to true by default, and if old machine types don't 
>>>> need this bit, they can pass false config to qemu when running qemu?
>>>
>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
>>>
>>>
>>
>> --
>> Best regards,
>> Jiqian Chen.
> 

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/3/29 18:38, Jason Wang wrote:
> On Fri, Mar 29, 2024 at 4:00 PM Chen, Jiqian  wrote:
>>
>> On 2024/3/29 15:20, Jason Wang wrote:
>>> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian  wrote:
>>>>
>>>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>>>>>>>  {
>>>>>>>>  PCIDevice *dev = PCI_DEVICE(obj);
>>>>>>>>  DeviceState *qdev = DEVICE(obj);
>>>>>>>>
>>>>>>>> +if (virtio_pci_no_soft_reset(dev)) {
>>>>>>>> +return;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  virtio_pci_reset(qdev);
>>>>>>>>
>>>>>>>>  if (pci_is_express(dev)) {
>>>>>>>> @@ -2484,6 +2511,8 @@ static Property virtio_pci_properties[] = {
>>>>>>>>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>>>>>>>>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>>>>>>>>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
>>>>>>>> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
>>>>>>>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
>>>
>>> Why does it come with an x prefix?
>> Sorry, it's my misunderstanding of this prefix, if No_Soft_Reset doesn't 
>> need this prefix, I will delete it in next version.
>> Does x prefix means compat machinery? Or other meanings?
>>
>>>
>>>>>>>>  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>>>>>>>>  VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>>>>>>>>  DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>>>>>>>
>>>>>>> I am a bit confused about this part.
>>>>>>> Do you want to make this software controllable?
>>>>>> Yes, because even the real hardware, this bit is not always set.
>>>
>>> We are talking about emulated devices here.
>> Yes, I just gave an example. It actually this bit is not always set. What's 
>> your opinion about when to set this bit or which virtio-device should set 
>> this bit?
> 
> If the implementation of Qemu is correct, we should set it unless we
> need compatibility.
Ok, I will set it default to true in next version.
If we need compatibility, what's your opinion about which machine types should 
we compatible with? Does the same as x-pcie-pm-init?
> 
>>
>>>
>>>>>
>>>>> So which virtio devices should and which should not set this bit?
>>>> This depends on the scenario the virtio-device is used, if we want to 
>>>> trigger an internal soft reset for the virtio-device during S3, this bit 
>>>> shouldn't be set.
>>>
>>> If the device doesn't need reset, why bother the driver for this?
>> I don't know what you mean.
>> If the device doesn't need reset, we can config true to set this bit, then 
>> on the driver side, driver finds this bit is set, then driver will not 
>> trigger a soft reset.
> 
> I mean if the device can suspend without reset, we don't need to
> bother the driver to save and load states.
> 
>>
>>>
>>> Btw, no_soft_reset is insufficient for some cases,
>> May I know which cases?
>>
>>> there's a proposal for the virtio-spec. I think we need to wait until it is 
>>> done.
>> Can you share the proposal?
> 
> See this
> 
> https://lore.kernel.org/all/20240227015345.3614965-1-steve...@chromium.org/T/
I looked the detail of this proposal.
What the proposal does is to add a new status to trigger device to 
suspend/resume.
But this patch is to add No_Soft_Reset bit, it decides if the device should be 
reset. This patch doesn't depend on the proposal.
If you mean that the proposal says "A device MUST maintain its state while 
suspended", I think it needs new patch to implement it after the proposal is 
done.

> 
> Thanks
> 
>>
>>>
>>>> In my use case on my environment, I don't want to reset virtio-gpu during 
>>>> S3,
>>>> because once the display resources are destroyed, there are not enough 
>>>> information to re-create them, so this bit should be set.
>>>> Making this bit software controllable is convenient for users to take 
>>>> their own choices.
>>>
>>> Thanks
>>>
>>>>
>>>>>
>>>>>>> Or should this be set to true by default and then
>>>>>>> changed to false for old machine types?
>>>>>> How can I do so?
>>>>>> Do you mean set this to true by default, and if old machine types don't 
>>>>>> need this bit, they can pass false config to qemu when running qemu?
>>>>>
>>>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
>>>>>
>>>>>
>>>>
>>>> --
>>>> Best regards,
>>>> Jiqian Chen.
>>>
>>
>> --
>> Best regards,
>> Jiqian Chen.
> 

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/3/29 18:44, Michael S. Tsirkin wrote:
> On Fri, Mar 29, 2024 at 03:20:59PM +0800, Jason Wang wrote:
>> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian  wrote:
>>>
>>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
>>>>>>> +}
>>>>>>> +
>>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>>>>>>  {
>>>>>>>  PCIDevice *dev = PCI_DEVICE(obj);
>>>>>>>  DeviceState *qdev = DEVICE(obj);
>>>>>>>
>>>>>>> +if (virtio_pci_no_soft_reset(dev)) {
>>>>>>> +return;
>>>>>>> +}
>>>>>>> +
>>>>>>>  virtio_pci_reset(qdev);
>>>>>>>
>>>>>>>  if (pci_is_express(dev)) {
>>>>>>> @@ -2484,6 +2511,8 @@ static Property virtio_pci_properties[] = {
>>>>>>>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>>>>>>>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>>>>>>>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
>>>>>>> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
>>>>>>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
>>
>> Why does it come with an x prefix?
>>
>>>>>>>  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>>>>>>>  VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>>>>>>>  DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>>>>>>
>>>>>> I am a bit confused about this part.
>>>>>> Do you want to make this software controllable?
>>>>> Yes, because even the real hardware, this bit is not always set.
>>
>> We are talking about emulated devices here.
>>
>>>>
>>>> So which virtio devices should and which should not set this bit?
>>> This depends on the scenario the virtio-device is used, if we want to 
>>> trigger an internal soft reset for the virtio-device during S3, this bit 
>>> shouldn't be set.
>>
>> If the device doesn't need reset, why bother the driver for this?
>>
>> Btw, no_soft_reset is insufficient for some cases, there's a proposal
>> for the virtio-spec. I think we need to wait until it is done.
> 
> That seems orthogonal or did I miss something?
Yes, I looked the detail of the proposal, I also think they are unrelated.
I will set the default value of No_Soft_Reset bit to true in next version 
according to your opinion.
About the compatibility of old machine types, which types should I consider? 
Does the same as x-pcie-pm-init(hw_compat_2_8)?
Forgive me for not knowing much about compatibility.
> 
>>> In my use case on my environment, I don't want to reset virtio-gpu during 
>>> S3,
>>> because once the display resources are destroyed, there are not enough 
>>> information to re-create them, so this bit should be set.
>>> Making this bit software controllable is convenient for users to take their 
>>> own choices.
>>
>> Thanks
>>
>>>
>>>>
>>>>>> Or should this be set to true by default and then
>>>>>> changed to false for old machine types?
>>>>> How can I do so?
>>>>> Do you mean set this to true by default, and if old machine types don't 
>>>>> need this bit, they can pass false config to qemu when running qemu?
>>>>
>>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
>>>>
>>>>
>>>
>>> --
>>> Best regards,
>>> Jiqian Chen.
> 

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/4/7 11:20, Jason Wang wrote:
> On Tue, Apr 2, 2024 at 11:03 AM Chen, Jiqian  wrote:
>>
>> On 2024/3/29 18:44, Michael S. Tsirkin wrote:
>>> On Fri, Mar 29, 2024 at 03:20:59PM +0800, Jason Wang wrote:
>>>> On Fri, Mar 29, 2024 at 3:07 PM Chen, Jiqian  wrote:
>>>>>
>>>>> On 2024/3/28 20:36, Michael S. Tsirkin wrote:
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>>>>>>>>  {
>>>>>>>>>  PCIDevice *dev = PCI_DEVICE(obj);
>>>>>>>>>  DeviceState *qdev = DEVICE(obj);
>>>>>>>>>
>>>>>>>>> +if (virtio_pci_no_soft_reset(dev)) {
>>>>>>>>> +return;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>  virtio_pci_reset(qdev);
>>>>>>>>>
>>>>>>>>>  if (pci_is_express(dev)) {
>>>>>>>>> @@ -2484,6 +2511,8 @@ static Property virtio_pci_properties[] = {
>>>>>>>>>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>>>>>>>>>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>>>>>>>>>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
>>>>>>>>> +DEFINE_PROP_BIT("x-pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
>>>>>>>>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
>>>>
>>>> Why does it come with an x prefix?
>>>>
>>>>>>>>>  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>>>>>>>>>  VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>>>>>>>>>  DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>>>>>>>>
>>>>>>>> I am a bit confused about this part.
>>>>>>>> Do you want to make this software controllable?
>>>>>>> Yes, because even the real hardware, this bit is not always set.
>>>>
>>>> We are talking about emulated devices here.
>>>>
>>>>>>
>>>>>> So which virtio devices should and which should not set this bit?
>>>>> This depends on the scenario the virtio-device is used, if we want to 
>>>>> trigger an internal soft reset for the virtio-device during S3, this bit 
>>>>> shouldn't be set.
>>>>
>>>> If the device doesn't need reset, why bother the driver for this?
>>>>
>>>> Btw, no_soft_reset is insufficient for some cases, there's a proposal
>>>> for the virtio-spec. I think we need to wait until it is done.
>>>
>>> That seems orthogonal or did I miss something?
>> Yes, I looked the detail of the proposal, I also think they are unrelated.
> 
> The point is the proposal said
> 
> """
> Without a mechanism to
> suspend/resume virtio devices when the driver is suspended/resumed in
> the early phase of suspend/late phase of resume, there is a window where
> interrupts can be lost.
> """
> 
> It looks safe to enable it with the suspend bit. Or if you think it's
> wrong, please comment on the virtio spec patch.
If I understand the proposal correctly.
Only need to check the SUSPEND bit when virtio_pci_bus_reset_hold is called.
It seems the proposal won't block this patch to upstream.
In next version, I will add comments to note the SUSPEND bit that need to be 
considered once it is accepted.

> 
>> I will set the default value of No_Soft_Reset bit to true in next version 
>> according to your opinion.
>> About the compatibility of old machine types, which types should I consider? 
>> Does the same as x-pcie-pm-init(hw_compat_2_8)?
>> Forgive me for not knowing much about compatibility.
> 
> "x" means no compatibility at all, please drop the "x" prefix. And it
Thanks to explain.
So it seems the prefix "x" of "x-pcie-pm-init" is also wrong? Because it 
considered the hw_compat_2_8. Also "x-pcie-flr-init".
Back to No_Soft_Reset, do you know which old machines should I consider to 
compatible with?

> looks more safe to start as "false" by default.
> 
> Thanks
> 
>>>
>>>>> In my use case on my environment, I don't want to reset virtio-gpu during 
>>>>> S3,
>>>>> because once the display resources are destroyed, there are not enough 
>>>>> information to re-create them, so this bit should be set.
>>>>> Making this bit software controllable is convenient for users to take 
>>>>> their own choices.
>>>>
>>>> Thanks
>>>>
>>>>>
>>>>>>
>>>>>>>> Or should this be set to true by default and then
>>>>>>>> changed to false for old machine types?
>>>>>>> How can I do so?
>>>>>>> Do you mean set this to true by default, and if old machine types don't 
>>>>>>> need this bit, they can pass false config to qemu when running qemu?
>>>>>>
>>>>>> No, you would use compat machinery. See how is x-pcie-flr-init handled.
>>>>>>
>>>>>>
>>>>>
>>>>> --
>>>>> Best regards,
>>>>> Jiqian Chen.
>>>
>>
>> --
>> Best regards,
>> Jiqian Chen.
> 

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/4/7 19:49, Michael S. Tsirkin wrote:
>>> I will set the default value of No_Soft_Reset bit to true in next version 
>>> according to your opinion.
>>> About the compatibility of old machine types, which types should I 
>>> consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
>>> Forgive me for not knowing much about compatibility.
>>
>> "x" means no compatibility at all, please drop the "x" prefix. And it
>> looks more safe to start as "false" by default.
>>
>> Thanks
> 
> 
> Not sure I agree. External flags are for when users want to tweak them.
> When would users want it to be off?
> What is done here is I feel sane, just add machine compat machinery
> to change to off for old machine types.
Do you know which old machines should I consider to compatible with?
Or which guys should I add to "CC" and can get answer from them?
I have less knowledge about compatibility.

> 

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/4/8 12:56, Jason Wang wrote:
 I will set the default value of No_Soft_Reset bit to true in next version 
 according to your opinion.
 About the compatibility of old machine types, which types should I 
 consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
 Forgive me for not knowing much about compatibility.
>>>
>>> "x" means no compatibility at all, please drop the "x" prefix. And it
>>> looks more safe to start as "false" by default.
>>>
>>> Thanks
>>
>>
>> Not sure I agree. External flags are for when users want to tweak them.
>> When would users want it to be off?
> 
> If I understand the suspending status proposal correctly, there are at
> least 1 device that is not safe. And this series does not mention
> which device it has tested.
I only tested virtio-gpu with my patch, I will mention this in "commit message" 
in next version.

> 
> It means if we enable it unconditionally, guests may break.
Make sense, will keep " No_Soft_Reset bit " false by default. And add some 
comments in the codes to describe what you considered.

> 
> Thanks
> 
>> What is done here is I feel sane, just add machine compat machinery
>> to change to off for old machine types.

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v8 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/4/12 14:41, Jason Wang wrote:
> On Fri, Apr 12, 2024 at 2:05 PM Chen, Jiqian  wrote:
>>
>> On 2024/4/7 19:49, Michael S. Tsirkin wrote:
>>>>> I will set the default value of No_Soft_Reset bit to true in next version 
>>>>> according to your opinion.
>>>>> About the compatibility of old machine types, which types should I 
>>>>> consider? Does the same as x-pcie-pm-init(hw_compat_2_8)?
>>>>> Forgive me for not knowing much about compatibility.
>>>>
>>>> "x" means no compatibility at all, please drop the "x" prefix. And it
>>>> looks more safe to start as "false" by default.
>>>>
>>>> Thanks
>>>
>>>
>>> Not sure I agree. External flags are for when users want to tweak them.
>>> When would users want it to be off?
>>> What is done here is I feel sane, just add machine compat machinery
>>> to change to off for old machine types.
>> Do you know which old machines should I consider to compatible with?
>> Or which guys should I add to "CC" and can get answer from them?
>> I have less knowledge about compatibility.
> 
> If you make it off by default, you don't need otherwise, it's one
> release before.
Ok, thanks. In next version, I will follow the result of discussion and remove 
"x", while adding some comments in codes.

> 
> Thanks
> 
>>
>>>
>>
>> --
>> Best regards,
>> Jiqian Chen.
> 

-- 
Best regards,
Jiqian Chen.

Re: [PATCH v10 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/6/2 22:33, Michael S. Tsirkin wrote:
> On Wed, May 15, 2024 at 03:35:26PM +0800, Jiqian Chen wrote:
>> In current code, when guest does S3, virtio-gpu are reset due to the
>> bit No_Soft_Reset is not set. After resetting, the display resources
>> of virtio-gpu are destroyed, then the display can't come back and only
>> show blank after resuming.
>>
>> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check
>> this bit, if this bit is set, the devices resetting will not be done, and
>> then the display can work after resuming.
>>
>> No_Soft_Reset bit is implemented for all virtio devices, and was tested
>> only on virtio-gpu device. Set it false by default for safety.
>>
>> Signed-off-by: Jiqian Chen 
>> ---
>>  hw/virtio/virtio-pci.c | 37 ++
>>  include/hw/virtio/virtio-pci.h |  5 +
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index 1b63bcb3f15c..3052528c0730 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2230,6 +2230,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
>> Error **errp)
>>  pcie_cap_lnkctl_init(pci_dev);
>>  }
>>  
>> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) {
>> +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL,
>> + PCI_PM_CTRL_NO_SOFT_RESET);
>> +}
>> +
>>  if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
>>  /* Init Power Management Control Register */
>>  pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL,
>> @@ -2292,11 +2297,37 @@ static void virtio_pci_reset(DeviceState *qdev)
>>  }
>>  }
>>  
>> +static bool virtio_pci_no_soft_reset(PCIDevice *dev)
>> +{
>> +uint16_t pmcsr;
>> +
>> +if (!pci_is_express(dev) || !dev->exp.pm_cap) {
>> +return false;
>> +}
>> +
>> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL);
>> +
>> +/*
>> + * When No_Soft_Reset bit is set and the device
>> + * is in D3hot state, don't reset device
>> + */
>> +return (pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) &&
>> +   (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3;
>> +}
>> +
>>  static void virtio_pci_bus_reset_hold(Object *obj, ResetType type)
>>  {
>>  PCIDevice *dev = PCI_DEVICE(obj);
>>  DeviceState *qdev = DEVICE(obj);
>>  
>> +/*
>> + * Note that: a proposal to add SUSPEND bit is being discussed,
>> + * may need to consider the state of SUSPEND bit in future
>> + */
>> +if (virtio_pci_no_soft_reset(dev)) {
>> +return;
>> +}
>> +
>>  virtio_pci_reset(qdev);
>>  
>>  if (pci_is_express(dev)) {
>> @@ -2336,6 +2367,12 @@ static Property virtio_pci_properties[] = {
>>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
>> +/*
>> + * For safety, set this false by default, if change it to true,
>> + * need to consider compatible for old machine
>> + */
> 
> I think you should do exactly that, and make the name start with
> "x-". It's not reasonable to tell users "set it to true, conduct your own QE 
> testing". Neither do we want to double the amount of QE work with
> each bugfix.
OK, so you still insist on needing to consider the compatibility for old 
machine.
Can you give me some tips on which old machines do not require this 
feature(No_Soft_Reset bit)? That will be helpful.

> 
> If for some reason you want this true only for the gpu, you can do that too.
> 
> 
>> +DEFINE_PROP_BIT("pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT, false),
>>  DEFINE_PROP_BIT("x-pcie-flr-init", VirtIOPCIProxy, flags,
>>  VIRTIO_PCI_FLAG_INIT_FLR_BIT, true),
>>  DEFINE_PROP_BIT("aer", VirtIOPCIProxy, flags,
>> diff --git a/include/hw/virtio/virtio-pci.h b/include/hw/virtio/virtio-pci.h
>> index 59d88018c16a..9e67ba38c748 100644
>> --- a/include/hw/virtio/virtio-pci.h
>> +++ b/include/hw/virtio/virtio-pci.h
>> @@ -43,6 +43,7 @@ enum {
>>  VIRTIO_PCI_FLAG_INIT_FLR_BIT,
>>  VIRTIO_PCI_FLAG_AER_BIT,
>>  VIRTIO_PCI_FLAG_ATS_PAGE_ALIGNED_BIT,
>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT,
>>  };
>>  
>>  /* Need to activate work-arounds for buggy guests at vmstate load. */
>> @@ -79,6 +80,10 @@ enum {
>>  /* Init Power Management */
>>  #define VIRTIO_PCI_FLAG_INIT_PM (1 << VIRTIO_PCI_FLAG_INIT_PM_BIT)
>>  
>> +/* Init The No_Soft_Reset bit of Power Management */
>> +#define VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET \
>> +  (1 << VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET_BIT)
>> +
>>  /* Init Function Level Reset capability */
>>  #define VIRTIO_PCI_FLAG_INIT_FLR (1 << VIRTIO_PCI_FLAG_INIT_FLR_BIT)
>>  
>> -- 
>> 2.34.1
> 

-- 
Best regards,
Jiqian Chen.

Re: [RFC QEMU PATCH v9 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/4/16 23:45, Igor Mammedov wrote:
> On Tue, 16 Apr 2024 15:01:27 +0800
> Jiqian Chen  wrote:
> 
>> In current code, when guest does S3, virtio-gpu are reset due to the
>> bit No_Soft_Reset is not set. After resetting, the display resources
>> of virtio-gpu are destroyed, then the display can't come back and only
>> show blank after resuming.
> 
> Just a high level question.
> Typically when system goes into S3 all devices (modulo RAM) loose context
> (aka powered off), and then it's upto device driver to recover whatever
> was lost.
No, what you said is just one situation that when system goes into S3 devices 
loose context and fully reinitialized and be D0-uninitialized state.
The other situation is the context must be maintained so that the devices can 
quickly be D0-active state after resuming.
There are some descriptions in PCIe specification in Chapter 5.3.1.4 D3 state:
" Functional context is required to be maintained by Functions in the D3Hot 
state if the No_Soft_Reset field in the PMCSR is
Set. In this case, System Software is not required to re-initialize the 
Function after a transition from D3Hot to D0 (the
Function will be in the D0active state). If the No_Soft_Reset bit is Clear, 
functional context is not required to be
maintained by the Function in the D3Hot state, however it is not guaranteed 
that functional context will be cleared and
software must not depend on such behavior. As a result, in this case System 
Software is required to fully re-initialize the
Function after a transition to D0 as the Function will be in the 
D0uninitialized state."

What's more, not all virtio devices' context can be recovered by driver after 
resuming, like virtio-gpu, we have not enough information to re-create all
display resources, that is discussed in my previous version email thread.

> So why should we implement hw(qemu) workaround for a driver problem?
I think this is not a workaround, No_Soft_Reset bit is also described in PCIe 
specification, it can be set or not.
And we can set this bit for the specific device which we want to maintain 
context during S3.

> 
>>
>> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check
>> this bit, if this bit is set, the devices resetting will not be done, and
>> then the display can work after resuming.
>>
>> No_Soft_Reset bit is implemented for all virtio devices, and was tested
>> only on virtio-gpu device. Set it false by default for safety.
>>
>> Signed-off-by: Jiqian Chen 
>> ---
>>  hw/virtio/virtio-pci.c | 37 ++
>>  include/hw/virtio/virtio-pci.h |  5 +
>>  2 files changed, 42 insertions(+)
>>
>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>> index a1b61308e7a0..82fa4defe5cd 100644
>> --- a/hw/virtio/virtio-pci.c
>> +++ b/hw/virtio/virtio-pci.c
>> @@ -2230,6 +2230,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
>> Error **errp)
>>  pcie_cap_lnkctl_init(pci_dev);
>>  }
>>  
>> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) {
>> +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL,
>> + PCI_PM_CTRL_NO_SOFT_RESET);
>> +}
>> +
>>  if (proxy->flags & VIRTIO_PCI_FLAG_INIT_PM) {
>>  /* Init Power Management Control Register */
>>  pci_set_word(pci_dev->wmask + pos + PCI_PM_CTRL,
>> @@ -2292,11 +2297,37 @@ static void virtio_pci_reset(DeviceState *qdev)
>>  }
>>  }
>>  
>> +static bool virtio_pci_no_soft_reset(PCIDevice *dev)
>> +{
>> +uint16_t pmcsr;
>> +
>> +if (!pci_is_express(dev) || !dev->exp.pm_cap) {
>> +return false;
>> +}
>> +
>> +pmcsr = pci_get_word(dev->config + dev->exp.pm_cap + PCI_PM_CTRL);
>> +
>> +/*
>> + * When No_Soft_Reset bit is set and the device
>> + * is in D3hot state, don't reset device
>> + */
>> +return (pmcsr & PCI_PM_CTRL_NO_SOFT_RESET) &&
>> +   (pmcsr & PCI_PM_CTRL_STATE_MASK) == 3;
>> +}
>> +
>>  static void virtio_pci_bus_reset_hold(Object *obj)
>>  {
>>  PCIDevice *dev = PCI_DEVICE(obj);
>>  DeviceState *qdev = DEVICE(obj);
>>  
>> +/*
>> + * Note that: a proposal to add SUSPEND bit is being discussed,
>> + * may need to consider the state of SUSPEND bit in future
>> + */
>> +if (virtio_pci_no_soft_reset(dev)) {
>> +return;
>> +}
>> +
>>  virtio_pci_reset(qdev);
>>  
>>  if (pci_is_express(dev)) {
>> @@ -2336,6 +2367,12 @@ static Property virtio_pci_properties[] = {
>>  VIRTIO_PCI_FLAG_INIT_LNKCTL_BIT, true),
>>  DEFINE_PROP_BIT("x-pcie-pm-init", VirtIOPCIProxy, flags,
>>  VIRTIO_PCI_FLAG_INIT_PM_BIT, true),
>> +/*
>> + * for safety, set this false by default, if change it to true,
>> + * need to consider compatible for old machine
>> + */
>> +DEFINE_PROP_BIT("pcie-pm-no-soft-reset", VirtIOPCIProxy, flags,
>> +VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET

Re: [RFC QEMU PATCH v9 2/2] virtio-pci: implement No_Soft_Reset bit

[Chen, Jiqian]

On 2024/4/17 13:33, Yan Vugenfirer wrote:
> On Wed, Apr 17, 2024 at 6:13 AM Chen, Jiqian  wrote:
>>
>> On 2024/4/16 23:45, Igor Mammedov wrote:
>>> On Tue, 16 Apr 2024 15:01:27 +0800
>>> Jiqian Chen  wrote:
>>>
>>>> In current code, when guest does S3, virtio-gpu are reset due to the
>>>> bit No_Soft_Reset is not set. After resetting, the display resources
>>>> of virtio-gpu are destroyed, then the display can't come back and only
>>>> show blank after resuming.
>>>
>>> Just a high level question.
>>> Typically when system goes into S3 all devices (modulo RAM) loose context
>>> (aka powered off), and then it's upto device driver to recover whatever
>>> was lost.
>> No, what you said is just one situation that when system goes into S3 
>> devices loose context and fully reinitialized and be D0-uninitialized state.
>> The other situation is the context must be maintained so that the devices 
>> can quickly be D0-active state after resuming.
>> There are some descriptions in PCIe specification in Chapter 5.3.1.4 D3 
>> state:
>> " Functional context is required to be maintained by Functions in the D3Hot 
>> state if the No_Soft_Reset field in the PMCSR is
>> Set. In this case, System Software is not required to re-initialize the 
>> Function after a transition from D3Hot to D0 (the
>> Function will be in the D0active state). If the No_Soft_Reset bit is Clear, 
>> functional context is not required to be
>> maintained by the Function in the D3Hot state, however it is not guaranteed 
>> that functional context will be cleared and
>> software must not depend on such behavior. As a result, in this case System 
>> Software is required to fully re-initialize the
>> Function after a transition to D0 as the Function will be in the 
>> D0uninitialized state."
>>
> BTW: according to Windows documentation D3 Hot state implementation is
> a must for real HW devices.
> https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/device-sleeping-states
> "Device power states D1, D2, and D3 are the device low-power states.
> Starting with Windows 8, D3 is divided into two substates, D3hot and
> D3cold.
> 
> D1 and D2 are intermediate low-power states. Many classes of devices
> do not define these states. All devices must define D3hot."
Yes, I know, it is also described in PCIe specification. But this is not a 
conflict.
When set No_Soft_Reset didn’t mean the device can't enter D3hot state. It means 
when device enter D3hot, the context of device must be maintained.

> 
> Best regards,
> Yan.
> 
> 
>> What's more, not all virtio devices' context can be recovered by driver 
>> after resuming, like virtio-gpu, we have not enough information to re-create 
>> all
>> display resources, that is discussed in my previous version email thread.
>>
>>> So why should we implement hw(qemu) workaround for a driver problem?
>> I think this is not a workaround, No_Soft_Reset bit is also described in 
>> PCIe specification, it can be set or not.
>> And we can set this bit for the specific device which we want to maintain 
>> context during S3.
>>
>>>
>>>>
>>>> Implement No_Soft_Reset bit of PCI_PM_CTRL register, then guest can check
>>>> this bit, if this bit is set, the devices resetting will not be done, and
>>>> then the display can work after resuming.
>>>>
>>>> No_Soft_Reset bit is implemented for all virtio devices, and was tested
>>>> only on virtio-gpu device. Set it false by default for safety.
>>>>
>>>> Signed-off-by: Jiqian Chen 
>>>> ---
>>>>  hw/virtio/virtio-pci.c | 37 ++
>>>>  include/hw/virtio/virtio-pci.h |  5 +
>>>>  2 files changed, 42 insertions(+)
>>>>
>>>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
>>>> index a1b61308e7a0..82fa4defe5cd 100644
>>>> --- a/hw/virtio/virtio-pci.c
>>>> +++ b/hw/virtio/virtio-pci.c
>>>> @@ -2230,6 +2230,11 @@ static void virtio_pci_realize(PCIDevice *pci_dev, 
>>>> Error **errp)
>>>>  pcie_cap_lnkctl_init(pci_dev);
>>>>  }
>>>>
>>>> +if (proxy->flags & VIRTIO_PCI_FLAG_PM_NO_SOFT_RESET) {
>>>> +pci_set_word(pci_dev->config + pos + PCI_PM_CTRL,
>>>> + PCI_PM_CTRL_NO_SOFT_RESET);
>>>> +}
>>>> +
>>