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

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

On 9/5/2024 4:12 PM, Michael S. Tsirkin wrote:
> On Thu, Sep 05, 2024 at 03:12:41PM +0800, Zhu Lingshan wrote:
>>
>> On 9/5/2024 2:51 PM, Michael S. Tsirkin wrote:
>>> On Wed, Sep 04, 2024 at 02:38:36PM +0800, Zhu Lingshan wrote:
>>>> On 9/4/2024 2:31 PM, Jason Wang wrote:
>>>>> On Wed, Sep 4, 2024 at 12:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> On Wed, Sep 04, 2024 at 11:07:25AM +0800, Jason Wang wrote:
>>>>>>> On Tue, Sep 3, 2024 at 6:37 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>> On Tue, Sep 03, 2024 at 06:35:59AM -0400, Michael S. Tsirkin wrote:
>>>>>>>>>>> But I don't like it that looking at the registers, one does not know the device
>>>>>>>>>>> state. Hidden state is bad for debuggability.
>>>>>>>>>>> We have 4 states:
>>>>>>>>>>> suspending->suspended->resuming->resumed
>>>>>>>>>>> so we need a register with at least 2 bits.
>>>>>>>>>>>
>>>>>>>>>>> we could steal 2 bits from status but it seems a bit much.
>>>>>>>>>>>
>>>>>>>>>> This is why letting the status tell the status and control register to control thing is elegant.
>>>>>>>>> No argument here.
>>>>>>>> Or, to be more precise, our status is driver status.
>>>>>>> It looks like the device actually otherwise there's no need for
>>>>>>> re-read or poll for things like reset and others.
>>>>>> The need is there for complex device state transitions, which
>>>>>> can not reasonably block a read response.
>>>>>> Another standard approach with PCI is to specify the time
>>>>>> transitions can take. I consider that less elegant -
>>>>>> is this what you are advocating? The advantage is that
>>>>>> driver does not load the pci bus with constant re-polling.
>>>>>> The disadvantage is that it is hard to pick a universal
>>>>>> number.  A combination of these approaches might work,
>>>>>> e.g. a recommended timeout then poll.
>>>>> We've already had msleep() for vp_reset(), anyhow we can increase the
>>>>> sleep time, if it can overload the pci:
>>>>>
>>>>>         while (vp_modern_get_status(mdev))
>>>>>                 msleep(1);
>>>>>
>>>>> We can do the same for suspending.
>>>>>
>>>>> The main blocker for timeout is that it may break migration and
>>>>> complicate the hardening. Another proposal in the past is to have a
>>>>> notification.
>>>>>
>>>>> But what I don't understand here is that suspend/resume should be
>>>>> lighter than reset. If we can afford a reset, so did the
>>>>> suspending/resume. If we want to have something new, that's fine but
>>>>> it should be orthogonal to a specific new status bit?
>>>> I agree, if we want new status indicator, then the new indicator should not be
>>>> specific to SUSPEND.
>>>>
>>>> Thanks
>>>> Zhu Lingshan
>>> If you mean reset, we have a problem.
>>> Specifically, reset has to work before feature
>>> negotiation. So we can not do as we would with
>>> SUSPEND and use a feature bit to expose presence
>>> of the indicator register.
>> I guess the driver can reset the device even after DRIVER_OK.
> in fact, unlike suspend - at any point at all.
>
>> IMHO the indicator should work for all device_status transitions,
>> not only for RESET or SUSPEND, it should also apply for FEATURE_OK
>> and DRIVER_OK.
> I don't  know what kind of transition is there for DRIVER/DRIVER_OK.
> FEATURES_OK brings with it more issues, e.g. it can fail.
>
>
>> So we don't need a feature bit, we may just
>> place it in the common config space as you proposed before to
>> define common config space for all transport. We can do this for sure!
> I have to say, this project is really ballooning out ;)
>
>> Of course another approach is what Jason proposed, the existing
>> msleep and poll, driver knows whether the device is in progress
>> of status transitions because it is the driver writes device_status.
>>
>> Thanks
> or just a register for suspend transitions, worry about
> extending it later/
OK, will implement it in V8
>
>>> Regrettably, reset will have to still be supported
>>> through clearing status just because this is always there.
>>>
>>>
>>>>>>> Anyhow driver know
>>>>>>> its own status.
>>>>>>>
>>>>>>> Thanks
>>>>>> indeed, the status register is there to inform the device about
>>>>>> the driver status.
>>>>>>
>>>>>>
>>>>>>
>>>>>>>> If we need
>>>>>>>> to reflect and control device status, we need something else.
>>>>>>>>
>>>>> Thanks
>>>>>