Re: [RFC PATCH 1/5] virtio: introduce SUSPEND bit in device status

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

On 8/15/2023 8:37 AM, Jason Wang wrote:
> On Tue, Aug 15, 2023 at 8:26âAM Jason Wang <jasowang@redhat.com> wrote:
> > On Mon, Aug 14, 2023 at 7:29âPM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> > > This patch introudces a new status bit in the device status: SUSPEND.
> > >
> > > This SUSPEND bit can be used by the driver to suspend a device,
> > > in order to stablize the device states and virtqueue states.
> > >
> > > Its main use case is live migration.
> > I think patch 3 needs to be squashed into this one.
Patch 3 mentions Virtqueue State that is introduced in patch 2,
so I think patch 3 should placed after patch 2.

Thanks
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > ---
> > >   content.tex | 18 ++++++++++++++++++
> > >   1 file changed, 18 insertions(+)
> > >
> > > diff --git a/content.tex b/content.tex
> > > index 0a62dce..1bb4401 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -47,6 +47,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > >   \item[DRIVER_OK (4)] Indicates that the driver is set up and ready to
> > >     drive the device.
> > >
> > > +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> > > +  device has been suspended by the driver.
> > > +
> > >   \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> > >     an error from which it can't recover.
> > >   \end{description}
> > > @@ -73,6 +76,10 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > >   recover by issuing a reset.
> > >   \end{note}
> > >
> > > +The driver MUST NOT set SUSPEND if FEATURES_OK is not set.
> > > +
> > > +When set SUSPEND, the driver MUST re-read \field{device status} to ensure the SUSPEND bit is set.
> > > +
> > >   \devicenormative{\subsection}{Device Status Field}{Basic Facilities of a Virtio Device / Device Status Field}
> > >
> > >   The device MUST NOT consume buffers or send any used buffer
> > > @@ -82,6 +89,13 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > >   that a reset is needed.  If DRIVER_OK is set, after it sets DEVICE_NEEDS_RESET, the device
> > >   MUST send a device configuration change notification to the driver.
> > >
> > > +The device MUST ignore SUSPEND if FEATURES_OK is not set.
> > Is there any value to allow SUSPEND to be set without DRIVER_OK?
If live migration happens when the guest initializing the device,
maybe the guest driver has already configured some virtqueues or
modified the config space. At that point, We should allow to suspend
the device, because the hypervisor should not reset the device at
that moment.

SUSPEND requires FEATURES_OK because VIRTIO_F_SUSPEND has to be negotiated.
> > > +
> > > +The deivce MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
> > typo.
Yes
> > Thanks
> Btw, it's not clear to me if driver is allowed to clear this bit.
Patch 1 mentions that DRIVER_OK clears SUSPEND, so that the driver
can resume the device if LM fails.

Thanks
> Thanks
>
> > > +
> > > +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device MUST clear SUSPEND
> > > +and resumes operation upon DRIVER_OK.
> > > +
> > >   \section{Feature Bits}\label{sec:Basic Facilities of a Virtio Device / Feature Bits}
> > >
> > >   Each virtio device offers all the features it understands.  During
> > > @@ -872,6 +886,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(41)] This feature indicates that the driver can
> > > +   SUSPEND the device.
> > > +   See \ref{sec:Basic Facilities of a Virtio Device / Device Status Field}.
> > > +
> > >   \end{description}
> > >
> > >   \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature Bits}
> > > --
> > > 2.35.3