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

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

On 9/4/2024 2:46 PM, Parav Pandit wrote:
>> From: Zhu Lingshan <lingshan.zhu@amd.com>
>> Sent: Wednesday, September 4, 2024 12:09 PM
>>
>> 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.
>>
> Sounds good to me as long as new functionality does not force the device to react in 50nsec.
> In the current state it forces the device so please upgrade the proposal as Michael guided.
Trap and emulate is very often in virtualization, of course the sooner the better for emulation.
But there are no 50nsec assumption and no one can force the hypervisor to finish any emulations within
a certain time 
> Thanks.