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

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

On 9/5/2024 3:17 PM, Michael S. Tsirkin wrote:
> On Thu, Sep 05, 2024 at 03:14:45PM +0800, Zhu Lingshan wrote:
>>
>> 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 
> The spec should not assume devices are virtual, there are baremetal
> deployments of virtio. In such setups, there is no one to trap or
> emulate: driver must do everything itself.
sure thing, I am also confused why VM_EXIT & VM_ENTRY and is a problem here...
>
>
>>> Thanks.