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

["Michael S. Tsirkin" <mst@redhat.com>]

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.



> >