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

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

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/

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