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

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

On Thu, Aug 15, 2024 at 05:12:23PM +0800, Zhu Lingshan wrote:
> 
> 
> On 8/13/2024 4:01 PM, Michael S. Tsirkin wrote:
> > On Thu, Aug 01, 2024 at 07:35:16PM +0800, Zhu Lingshan wrote:
> >> +\drivernormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> >> +
> >> +The driver MUST NOT set SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> > Actually, it has no effect before DRIVER_OK, no? I would forbid that
> > then.
> There can be two cases:
> 1) debugging the device before DRIVER_OK
> 2) VM migration right after boot, and before DRIVER_OK. So it needs to migrate the device before DRIVER_OK


Sorry I don't understand. The only effect of SUSPEND as you specified
it is not consuming buffers. Buffers are not consumed before DRIVER_OK,
thus there is no point to set SUSPEND before DRIVER_OK.

> >
> >> +Once the driver sets SUSPEND to \field{device status} of the device:
> >> +\begin{itemize}
> >> +\item The driver MUST re-read \field{device status} to verify whether the SUSPEND bit is set.
> >
> > This is still vague, I commented on this several times.
> > I think what you mean is that until it reads status with
> > SUSPEND as 1, it does not consider SUSPEND set.
> >
> >
> > But, what happens if driver clears SUSPEND before it reads it as set?
> > It would seem that should cancel suspend (which is a useful thing to
> > support)? But this creates a problem as it breaks read/modify/write that
> > some hypervisors assumed to be safe. I guess we need an extra
> > SUSPEND_IN_PROGRESS bit then? A little too much for status, at this
> > stage - maybe we need an extra register for this.
> if the driver reads SUSPEND bit == 0, with the knowledge of its own operations,
> it knows the device is either in a) normal operation 2). suspending in progress
> 
> Cancelling a SUSPEND right after setting SUSPEND is actually a RESUME.
> The ideal case is the driver waits until SUSPEND == 1, then clear it.
> But there are various drivers, not all drivers follow our expectation,
> so there can be a corner case that how the driver know whether
> the device is resumed after clearing the SUSPEND bit or not finish suspending.
> 
> I think adding a new resister can surely fix this issue, however as you
> said, add a new status bit is overkill and where to place this new register
> is a new question.

Place it after existing registers.


> 
> So, to make things easier, how about suspend & resume the device sequentially:
> 
> If the driver has suspended the device by setting the SUSPEND bit to 1, it MUST NOT
> clear the SUSPEND bit before the device presenting the SUSPEND bit set as 1.


I'm worried that it's fragile - one can no longer deduce
the device state by reading the status.

> >
> >
> >> +\item The driver MUST NOT make any more buffers available to the device.
> >> +\item The driver MUST NOT access any virtqueues or send notifications for any virtqueues.
> >> +\item The driver MUST NOT access Device Configuration Space.
> >> +\end{itemize}
> >> +
> >> +\devicenormative{\subsection}{Device Suspend}{General Initialization And Device Operation / Device Suspend}
> >> +
> >> +The device MUST ignore SUSPEND if FEATURES_OK is not set or VIRTIO_F_SUSPEND is not negotiated.
> >> +
> >> +The device MUST ignore all access to its Configuration Space while
> >> +suspended, except for \field{device status} if it is part of the Configuration Space.
> >> +
> >> +A device MUST NOT send any notifications for any virtqeuues,
> >> +access any virtqueues, or modify any fields in
> >> +its Configuration Space while suspended.
> >> +
> >> +If changes occur in the Configuration Space while the SUSPEND bit is set,
> >> +the device MUST NOT send any configuration change notifications.
> >> +Instead, the device MUST send the notification after the SUSPEND bit has been cleared.
> >> +
> >> +When the driver sets SUSPEND, the device MUST either suspend itself or set DEVICE_NEEDS_RESET if failed to suspend.
> >> +
> >> +If SUSPEND is set in \field{device status}, when the driver clears SUSPEND,
> >> +the device MUST either resume normal operation or set DEVICE_NEEDS_RESET.
> >> +
> >> +When the driver sets SUSPEND,
> >> +the device SHOULD perform the following actions before presenting that the SUSPEND bit is set to 1 in the \field{device status}:
> > what does "before presenting" mean? does it return SUSPEND as 0
> > after driver wrote 1 there and before it completed these
> > actions?
> Yes, it should return 0 if the suspending  operation is still in progress,
> It should not change SUSPEND bit value before finish.
> Do you suggest we add:
> The device MUST present SUSPEND bit set to 1 in \field{device status} once it has been suspended
> 
> It says: presenting that the SUSPEND bit is set to 1.
> Not a native speaker, but this looks clear to me.

I don't think this is sufficient, you need to document all assumptions
on both device and driver.


> >
> >> +
> >> +\begin{itemize}
> >> +\item Stop processing more buffers of any virtqueues
> >> +\item Wait until all buffers that are being processed have been used.
> >> +\item Send used buffer notifications to the driver.
> >> +\end{itemize}
> >> +
> >>  \chapter{Virtio Transport Options}\label{sec:Virtio Transport Options}
> >>  
> >>  Virtio can use various different buses, thus the standard is split
> >> @@ -872,6 +923,10 @@ \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
> >>  	\ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits} for
> >>  	handling features reserved for future use.
> >>  
> >> +  \item[VIRTIO_F_SUSPEND(42)] This feature indicates that the driver can
> >> +   trigger suspending the device via the SUSPEND flag
> >> +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> >> +
> >>  \end{description}
> >>  
> >>  \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> >> -- 
> >> 2.45.2