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

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

On 9/3/2024 5:36 PM, Michael S. Tsirkin wrote:
> On Tue, Sep 03, 2024 at 04:51:18PM +0800, Zhu Lingshan wrote:
>>
>> 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
> There is a difference between SUSPEND and RESET.
> RESET is not a state. Thus a single bit is enough to
> signal "reset in progress".
>
> I don't really see any other transitions that can take
> a long time. We can start with just suspend, and
> extend it later if appropriate.
Yes, that is what I mean, the register should not only work for SUSPEND.
To be more specific, the definition should be:

// The device_status is still in a status transition
#define DEVICE_STATUS_TRANSITION_IN_PROGRESS     0
// device status transition is done
#define DEVICE_STATUS_TRANSITION_DONE                     1

They should not be defined as:
#define DEVICE_STATUS_SUSPEND_IN_PROGRESS     0

Thanks

>
>
>