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 19:10, Parav Pandit wrote:
> 
>> From: Chen, Jiqian <Jiqian.Chen@amd.com>
>> Sent: Monday, January 15, 2024 4:37 PM
>>
>> On 2024/1/15 18:52, Parav Pandit wrote:
>>>
>>>> From: Chen, Jiqian <Jiqian.Chen@amd.com>
>>>> Sent: Monday, January 15, 2024 4:17 PM
>>>>
>>>> On 2024/1/15 17:46, Parav Pandit wrote:
>>>>>
>>>>>> From: Chen, Jiqian <Jiqian.Chen@amd.com>
>>>>>> Sent: Monday, January 15, 2024 3:10 PM
>>>>>
>>>>>>> Display resources should be only restored when following
>>>>>>> conditions are
>>>>>> met.
>>>>>>> 1. PCI PM cap is reported.
>>>>>>> 2. PCI PM cap has non_soft_reset=1 3. virtio guest driver do not
>>>>>>> perform reset when transport offers a restore
>>>>>> capability using #1 and #2.
>>>>>>>
>>>>>>> Do you agree? Yes?
>>>>>> Yes, I think this problem (display resources are destroyed during
>>>>>> S3) can be sorted to two situations:
>>>>>> First is what you said above, in this situation, the devices_reset
>>>>>> of qemu is unreasonable, if a device has PM cap and
>>>>>> non_soft_reset=1, qemu should not do resetting.
>>>>>
>>>>>> Second is without #1 or #2, the reset behavior is fine. My patch is
>>>>>> to fix this situation, that sending a new virtio-gpu command to
>>>>>> notify qemu to prevent destroying display resources during S3.
>>>>>
>>>>> I still have hard time following "My patch is to fix this situation...".
>>>>>
>>>>> When #1 and #2 is not done, there is nothing to restore.
>>>>> Driver should not send some new virtio specific command when #1 and
>>>>> #2 is
>>>> not there.
>>>>> Instead, if the device wants to restore the context, all #1, #2 and
>>>>> #3 should
>>>> be done to implement the restore functionality.
>>>> When #1 and #2 is done. The "device reset behavior" should not
>>>> happen. And then the display resources are not destroyed.
>>>> I didnât say send command to restore context.
>>>
>>>> I mean when #1 and #2 is not done. Driver and qemu both reset devices
>>>> when resuming, at that time,
>>> Above behavior is as per the spec guidelines. Hence it is fine.
>>>
>>>> we need a method to prevent the display resources from destroying.
>>> I disagree to above as it is not as per the spec guidelines.
>>> Just follow #1, #2 and #3 to not destroy the display resource destroying.
>> I agree that follow #1, #2 and #3 will not destroy display resource.
>> So the question goes back: If there is no #2 and you say that the reset
>> behavior complies with the spec guidelines, how can I make the virtio gpu
>> work properly with display resources destroyed?
> Implement #2. :)
We can't simply implement #2 if a device doesn't have #2, see Section 7.5.2.2 in PCI Express Spec, about No_Soft_Reset:
" If a VF implements the Power Management Capability, the VF's value of this field must be identical to the associated PF's value. "
What's more, a device doesn't have #2 is allowed, we should consider how to solve the two situations(No_Soft_Reset=1 and No_Soft_Reset=0), rather than simply considering implementing #2 for the device when it does not have #2.
Also in PCI Express Spec:
" 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."
This description corresponds to the two situations we are discussing.
First is a device has #2(No_Soft_Reset=1), the device resetting will not happen, then the display resources of virtio-gpu problem will not happen.
But in Second, a device doesn't have #2(No_Soft_Reset=0), it is fine for system to do device resetting, at this time, the display resources of virtio-gpu problem will happen, we should also consider a method to solve that problem, but not to implement #2.

> 
>> What my patch do is not to prevent resetting, is to prevent destroying
>> resources during resetting. Gerd Hoffmann has agreed to this point in my
>> qemu patch reviewing.
>> Even if I use PM cap, I still need to prevent destroying resources during
>> resetting.
> Do you mean when virtio device is reset, you want to prevent?
I want to protect display resources, not to prevent the whole process of device resetting.
Also in PCI Express Spec:
"Note that a Function's software driver participates in the process of transitioning the Function from D0 to D3Hot. It contributes to the process by saving any functional state that would otherwise be lost with removal of main power, and otherwise preparing the Function for the transition to D3Hot."
I let guest driver to notify qemu to protect the display resources that will be loss during S3, it is also compliant with Spec.

> If so, that is yet additional virtio spec hack that must be avoided as reset flow should be one.
> Please do #3 to avoid resetting the device.
The same reason as above. According to the regulations of the spec, we should allow for the occurrence of two situations(No_Soft_Reset=1 and No_Soft_Reset=0) and provide solutions for each situation, rather than simply categorizing them into one.

-- 
Best regards,
Jiqian Chen.