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

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

On Fri, Sep 06, 2024 at 07:51:08AM +0800, Jason Wang wrote:
> On Thu, Sep 5, 2024 at 4:12 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Sep 05, 2024 at 03:12:41PM +0800, Zhu Lingshan wrote:
> > >
> > >
> > > On 9/5/2024 2:51 PM, Michael S. Tsirkin wrote:
> > > > On Wed, Sep 04, 2024 at 02:38:36PM +0800, Zhu Lingshan wrote:
> > > >>
> > > >> 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
> > > > If you mean reset, we have a problem.
> > > > Specifically, reset has to work before feature
> > > > negotiation. So we can not do as we would with
> > > > SUSPEND and use a feature bit to expose presence
> > > > of the indicator register.
> > > I guess the driver can reset the device even after DRIVER_OK.
> >
> > in fact, unlike suspend - at any point at all.
> >
> > > IMHO the indicator should work for all device_status transitions,
> > > not only for RESET or SUSPEND, it should also apply for FEATURE_OK
> > > and DRIVER_OK.
> >
> > I don't  know what kind of transition is there for DRIVER/DRIVER_OK.
> > FEATURES_OK brings with it more issues, e.g. it can fail.
> >
> >
> > > So we don't need a feature bit, we may just
> > > place it in the common config space as you proposed before to
> > > define common config space for all transport. We can do this for sure!
> >
> > I have to say, this project is really ballooning out ;)
> >
> > > Of course another approach is what Jason proposed, the existing
> > > msleep and poll, driver knows whether the device is in progress
> > > of status transitions because it is the driver writes device_status.
> > >
> > > Thanks
> >
> > or just a register for suspend transitions, worry about
> > extending it later/
> 
> Is this for PCI only?

It's best to add it to all transports. Not rocket science at all.

> Another question is that, if suspend needs that,
> reset would also want.

I don't know what does "reset would also want" means.
Unlike suspend, reset is not a special state, so it
does not really require a spcial register to track
that state.

> Or it doesn't justify itself as reset needs to
> take longer time than reset.
> 
> Thanks
> 

Don't know what this means, either.

> >
> > > >
> > > > Regrettably, reset will have to still be supported
> > > > through clearing status just because this is always there.
> > > >
> > > >
> > > >>>>> 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
> > > >>>
> >