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

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

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.

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
> >