Re: [PATCH V7 v7] virtio: introduce SUSPEND bit in device status

[Zhu Lingshan <lingshan.zhu@amd.com>]

On 8/30/2024 11:10 AM, Parav Pandit wrote:
>
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Friday, August 30, 2024 8:07 AM
>>
>>
>> On 8/15/2024 11:07 PM, Michael S. Tsirkin wrote:
>>> On Thu, Aug 15, 2024 at 10:59:45AM +0000, Parav Pandit wrote:
>>>>> From: Michael S. Tsirkin <mst@redhat.com>
>>>>> Sent: Thursday, August 15, 2024 4:23 PM
>>>>>
>>>>> On Tue, Aug 13, 2024 at 06:55:04AM +0000, Parav Pandit wrote:
>>>>>> That means, PCI HW needs to return suspend=0, until the device is
>>>>>> not
>>>>> suspended.
>>>>>> In this example, the device cannot build special circuitry to
>>>>>> answer
>>>>> suspend=true within 50nsec, or in other words building special
>>>>> circuitry to return suspend=false is too complex for the slow operation.
>>>>>> If this understanding of burden is clear,
>>>>>>
>>>>>> The proposal is, can you please extend the interface such that,
>>>>>>
>>>>>> 1. driver writes suspend command.
>>>>>> 2. driver reads suspend_status, and receives not_completed=(false).
>>>>>> This is
>>>>> the default value.
>>>>>> 3. When the device completes suspend, it changes the polarity of
>>>>> suspend_status=true.
>>>>>> This has two main benefits:
>>>>>> [A] This will enable software-based devices to write data to slow
>>>>>> files and
>>>>> does not have to force VM_EXITs.
>>>>>> [B] It also enables hw based devices to not build special circuitry
>>>>>> to answer
>>>>> within 50nsec, which can get very complicated for tens or hundreds
>>>>> of PCI PFs.
>>>>>
>>>>> I read this several times, and I don't understand what is proposed.
>>>>> A special register for suspend/resume? Is this the difference?
>>>>>
>>>> Yes, a command register for suspend/resume operation.
>>>> And device_status new bit that Lingshan defined returns the status of this
>> operation.
>>>
>>> Ugh, it's all quite messy IMHO.
>>> We have 4 states:
>>> - operational (resumed)
>>> - suspend in progress
>>> - suspended
>>> - resume in progress
>>>
>>> What I'd do then is a two bit register.
>>> To suspend:
>>> - write suspend in progress
>>> - re-read, waiting until suspended
>>> To resume
>>> - write resume in progress
>>> - re-read, waiting until operational (resumed)
>>>
>>> How does this sound?
>> This can work for sure. but is it a must?
>> I mean, the driver has its own knowledge of how it operate the device.
>> When device presents SUSPEND == 0, It know whether the device is in
>> normal operational state or in the progress of SUSPENDING.
>>
>> But if you think we should add a new register which applying for all
>> device_status transitions, NOT only for SUSPEND. we can surely do that.
>>
>> Thanks
> New register beyond suspend+resume can be useful too.
> For sure it will simplify the suspend + resume flow.
There should be no difference in how the driver handles SUSPEND
and other device status like RESET.

If we want to add a new register, then it is not only for SUSPEND,
but for all status transitions. 
We need Michael to confirm we should implement this new register
that apply to all device_status transitions for common interests.

Thanks
>