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

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

On Thu, Sep 12, 2024 at 10:05:02AM +0800, Jason Wang wrote:
> On Wed, Sep 11, 2024 at 6:20 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > 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.
> 
> I'm asking since I wonder if the "issue" exists only for PCI.
> If yes,
> solving the PCI transport (registers) issue at the basic facility
> level seems like a layer violation.


I don't think so. Suspend is fundamentally a slow operation for
any transport.


> And I still don't see why
> introducing a single new bit in the status brings any new troubles
> compared with the existing reset and other state transitions.

reset is not a state.

> As
> mentioned before, re-read has been used for both FEAUTRES_OK and
> reset.

FEAUTRES_OK is driver state, so can take place immediately

> >
> > > Another question is that, if suspend needs that,
> > > reset would also want.
> >
> > I don't know what does "reset would also want" means.
> 
> I meant we have already had a state transition like reset.
> 
> For example, you worries about:
> 
> suspending->suspended->resuming->resumed
> 
> But we've already had
> 
> setting_features -> feature_ok(or not) -> resetting -> reseted


features ok just has to validate the bitmap, so it
can take place immediately. no intermediate state.

> > Unlike suspend, reset is not a special state,
> 
> It depends but we had a dedicated chapter in the basic facility to
> describe the reset state.

this is two different meanings of the same word.
reset is not a special state, device operates normally.


> > so it
> > does not really require a spcial register to track
> > that state.
> 
> This seems to be PCI specific. Note that ccw has a dedicated command for reset:
> 
> #define CCW_CMD_VDEV_RESET 0x33
> ...
> #define CCW_CMD_WRITE_STATUS 0x31


maybe reusing state 0 to reset was a bad idea.


> >
> > > Or it doesn't justify itself as reset needs to
> > > take longer time than reset.
> > >
> > > Thanks
> > >
> >
> > Don't know what this means, either.
> 
> I meant
> 
> 1) driver knows its own status and it can read device status
> 2) reset may take much longer time than suspend

that is why driver re-reads status to check for reset.


> 
> If we really need a new status register (I don't see why so far), we
> should not only suspend.
> 
> Thanks

it's not a new status register, it's a suspend register used for PM.


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