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

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

On 2024/1/12 17:44, Parav Pandit wrote:
> 
> 
>> From: Chen, Jiqian <Jiqian.Chen@amd.com>
>> Sent: Friday, January 12, 2024 2:55 PM
>>
>> On 2024/1/12 16:47, Parav Pandit wrote:
>>>
>>>
>>>> From: Chen, Jiqian <Jiqian.Chen@amd.com>
>>>> Sent: Friday, January 12, 2024 1:55 PM
>>>>
>>>>
>>>> On 2024/1/12 16:02, Parav Pandit wrote:
>>>>> Hi Jiqian,
>>>>>
>>>>>> From: Chen, Jiqian <Jiqian.Chen@amd.com>
>>>>>> Sent: Friday, January 12, 2024 1:11 PM
>>>>>
>>>>>>
>>>>>> Hi all,
>>>>>> Sorry to reply late.
>>>>>> I don't know if you still remember this problem, let me briefly descript
>> it.
>>>>>> I am working to implement virtgpu S3 function on Xen.
>>>>>> Currently on Xen, if we start a guest through qemu with enabling
>>>>>> virtgpu, and then suspend and resume guest. We can find that the
>>>>>> guest kernel comes back, but the display doesn't. It just shown a
>>>>>> black
>>>> screen.
>>>>>> That is because during suspending, guest called into qemu and qemu
>>>>>> destroyed all display resources and reset renderer. This made the
>>>>>> display gone after guest resumed.
>>>>>> So, I add a new command for virtio-gpu that when guest is
>>>>>> suspending, it will notify qemu and set
>>>>>> parameter(preserver_resource) to 1 and then qemu will preserve
>>>>>> resources, and when resuming, guest will notify qemu to set
>>>>>> parameter to 0, and then qemu will keep the normal actions. That can
>> help guest's display come back.
>>>>>> When I upstream above implementation, Parav and MST suggest me to
>>>> use
>>>>>> the PM capability to fix this problem instead of adding a new
>>>>>> command or state bit.
>>>>>> Now, I have tried the PM capability of virtio-gpu, it can't be used
>>>>>> to solve this problem.
>>>>>> The reason is:
>>>>>> during guest suspending, it will write D3 state through PM cap,
>>>>>> then I can save the resources of virtio-gpu on Qemu side(set
>>>>>> preserver_resource to 1), but during the process of resuming, the
>>>>>> state of PM cap will be cleared by qemu
>>>>>> resetting(qemu_system_wakeup-> qemu_devices_reset->
>>>>>> virtio_vga_base_reset-> virtio_gpu_gl_reset), it causes that when
>>>>>> guest reads state from PM cap, it will find the virtio-gpu has
>>>>>> already been D0 state,
>>>>> This behavior needs to be fixed. As the spec listed out, " This
>>>>> 2-bit field is
>>>> used both to determine the current power state of a Function"
>>>> Do you mean it is wrong to reset PM cap when vritio_gpu reset in
>>>> current qemu code? Why?
>>> Because PM is implementing the support from D3->d0 transition and if it
>> device in D3, the device needs to respond that it is in D3 to match the PCI
>> spec.
>>>
>>>> Shouldn't all device states, including PM registers, be reset during
>>>> the process of virtio-gpu reset?
>>> If No_Soft_Reset== 0, no device context to be preserved.
>>> If No_Soft_Reset == 1, device to preserve minimal registers and other
>> things listed in pci spec.
>>>
>>>>
>>>>>
>>>>> So device needs to return D3 in the PowerState field.  This is must.
>>>> But current behavior is a kind of soft reset, I
>>>> think(!PCI_PM_CTRL_NO_SOFT_RESET). The pm cap is reset by qemu is
>>>> reasonable.
>>> What you described is, you need to do No_Soft_Reset=1, so please set the
>> cap accordingly to achieve the restore.
>>>
>>> Also in your case the if the QEMU knows that it will have to resume the
>> device soon.
>>> Hence, it can well prepare the gpu context before resuming the vcpus.
>>> In such case, the extra register wouldnât be necessary.
>>> Having PM control register is anyway needed to drive the device properly.
>> Even as you said, the reset behavior of PM in the current QEMU code is
>> incorrect, and even if it is modified, it also cannot fix this problem with S3.
> You meant D3?
> 
>> Because there is twice device reset during resuming.
>> First is when we trigger a resume command to qemu, qemu will reset
>> device(qemu_system_wakeup-> qemu_devices_reset->
>> virtio_vga_base_reset-> virtio_gpu_gl_reset).
> A device implementation that supports PM should not reset the device. Please fix it.
Do you mean the devices_reset behavior of qemu shouldn't happen when a device has PM cap?
Or just the state of PM cap shouldn't be reset?

> 
>> After the first time, then second time happens in guest kernel,
>> virtio_device_restore-> virtio_reset_device. But the PM state changes (D3 to
>> D0) happens between those two, so the display resources will be still
>> destroyed by the second time of device reset.
> The driver that understands that device supports PM, will skip the reset steps.
But it happens for now.
In current qemu codes, the bit PCI_PM_CTRL_NO_SOFT_RESET of virtio-pci devices isn't set, is it a bug? It means the no_soft_reset==0.

> Hence the 2nd reset will also not occur.
> Therefore, device state will be restored.

-- 
Best regards,
Jiqian Chen.