Re: [virtio-comment] RE: [PATCH v6 1/1] content: Add new feature VIRTIO_F_PRESERVE_RESOURCES

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

On 2024/1/15 15:37, Parav Pandit wrote:
> 
>> From: Chen, Jiqian <Jiqian.Chen@amd.com>
>> Sent: Monday, January 15, 2024 1:03 PM
>>
>> 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?
> Right. Device reset should not happen on PM capable device that offers no_soft_reset==1.
But current qemu code, no_soft_reset==0.

> 
>> 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.
> This should be enhanced in the guest driver.
> 
>> 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.
>>
> It should be set when/if the device can store and restore the device context on d3->d0 transition.
I don't know why it doesn't be set in current qemu code.
What do you mean "the device can store and restore the device context on d3->d0 transition"? The real physical device or the simulated devices on qemu side?
How do I confirm it?

> 
>>> Hence the 2nd reset will also not occur.
>>> Therefore, device state will be restored.
>>
>> --
>> Best regards,
>> Jiqian Chen.

-- 
Best regards,
Jiqian Chen.