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

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

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
>
>>> 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
>