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

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

On Thu, Sep 05, 2024 at 03:27:25PM +0800, Zhu Lingshan wrote:
> 
> 
> On 9/3/2024 5:36 PM, Michael S. Tsirkin wrote:
> > 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.
> Yes, that is what I mean, the register should not only work for SUSPEND.
> To be more specific, the definition should be:
> 
> // The device_status is still in a status transition
> #define DEVICE_STATUS_TRANSITION_IN_PROGRESS     0
> // device status transition is done
> #define DEVICE_STATUS_TRANSITION_DONE                     1
> 
> They should not be defined as:
> #define DEVICE_STATUS_SUSPEND_IN_PROGRESS     0
> 
> Thanks
> 


in case we add more commands down the line? ok, sure.

> >
> >