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

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

On Thu, Sep 05, 2024 at 03:14:45PM +0800, Zhu Lingshan wrote:
> 
> 
> On 9/4/2024 2:46 PM, Parav Pandit wrote:
> >> From: Zhu Lingshan <lingshan.zhu@amd.com>
> >> Sent: Wednesday, September 4, 2024 12:09 PM
> >>
> >> 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.
> >>
> > Sounds good to me as long as new functionality does not force the device to react in 50nsec.
> > In the current state it forces the device so please upgrade the proposal as Michael guided.
> Trap and emulate is very often in virtualization, of course the sooner the better for emulation.
> But there are no 50nsec assumption and no one can force the hypervisor to finish any emulations within
> a certain time 

The spec should not assume devices are virtual, there are baremetal
deployments of virtio. In such setups, there is no one to trap or
emulate: driver must do everything itself.


> > Thanks.