* [virtio-dev] Re: [PATCH] virtio: introduce SUSPEND bit in device status
@ 2024-02-20 5:09 ` David Stevens
0 siblings, 0 replies; 24+ messages in thread
From: David Stevens @ 2024-02-20 5:09 UTC (permalink / raw)
To: Zhu, Lingshan
Cc: Michael S. Tsirkin, jasowang, eperezma, cohuck, stefanha,
virtio-comment, virtio-dev
On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> On 2/19/2024 2:46 PM, David Stevens wrote:
> > On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
> >>> This commit allows the driver to suspend the device by
> >>> introducing a new status bit SUSPEND in device_status.
> >>>
> >>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
> >>> which indicating whether the device support SUSPEND.
> >>>
> >>> This SUSPEND bit is transport-independent.
> >>>
> >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >> Could we get some kind of dscription how this has taken into
> >> consideration the proposal from David Stevens?
> >>
> >> I find it really tiring when there are competing patches with authors
> >> ignoring each other's work and leaving it up to reviewers to
> >> figure out how do the patches compare.
> > This patch looks like it could be used to implement my use case.
> > However, parts of it are a bit vague and imprecise, so it's hard to
> > actually say whether my use case would actually be covered by a
> > specific implementation of this proposal.
> I am on vacation till this Friday. Shall we co-work on this and
> post something new together?
That works for me.
> >
> > To give specifics on what I'm trying to do, I need to allow a guest
> > with virtio-pci devices (including stateful devices like virtio-fs and
> > virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
> > to wake up the guest (currently via an ACPI GPE, but eventually via a
> > native PCI PME). This is all done on a consumer device, so there is no
> > need for snapshotting or for live migration.
> Suspending is not dedicated only for live migration.
> For your use case, shall we add a new PCI section like saying: when entering
> SUSPEND, the device should get into S1/S3 and other PCI specific steps
> in PCI transport charter.
>
> That is because PCI is a transport layer of virtio, controlling virtio
> states by PCI sounds like a layer violation.
I don't know if it's really necessary to mandate that in the spec.
Sure, most systems are probably going to want to put the virtio-pci
device into S1/S3 after suspending virtio operation. But nothing
really breaks or goes wrong if we don't do that.
> >
> >>> ---
> >>> content.tex | 34 ++++++++++++++++++++++++++++++++--
> >>> 1 file changed, 32 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/content.tex b/content.tex
> >>> index 0a62dce..3d656b5 100644
> >>> --- a/content.tex
> >>> +++ b/content.tex
> >>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> >>>
> >>> \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> >>> an error from which it can't recover.
> >>> +
> >>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> >>> + device has been suspended by the driver.
> >>> \end{description}
> >>>
> >>> The \field{device status} field starts out as 0, and is reinitialized to 0 by
> >>> @@ -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 SHOULD NOT set SUSPEND if FEATURES_OK is not set.
> >>> +
> >>> +When setting 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,25 @@ \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.
> >>> +
> >>> +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
> >>> +
> >>> +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
> >>> +
> >>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
> >>> +and resumes operation upon DRIVER_OK.
> >>> +
> >>> +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
> >>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
> >>> +
> >>> +\begin{itemize}
> >>> +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
> >>> +\item Wait until all descriptors that being processed to finish and mark them as used.
> >>> +\item Flush all used buffer and send used buffer notifications to the driver.
> > What's the difference between the previous three lines? They might be
> > trying to say different things, but there is a lot of overlap in how
> > they're phrased. That makes it hard to figure out exactly what they're
> > mandating. Is it something like: "stop processing new buffers",
> > "finish processing any buffers that are currently in flight", "mark
> > all finished buffers as used and send a used buffer notification"?
> sure, the "mark used" parts can merge.
> >
> > Also, what does this mean for devices where the driver places empty
> > buffers into the virtqueue that the device then holds on to for an
> > indeterminate period of time (e.g. network receiveq, balloon statsq,
> > etc)?
> The device doesn't process them.
> Oh, I should add: The driver should not make any more buffers available
> to the device.
> >
> >>> +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> > What does "Pause its operation except device status" mean? The word
> > "operation" is vague, what exactly does it include? Also what does
> > "operate its device status" mean?
> Because we need to resume the device after suspending it by setting
> DRIVER_OK, so we need the device status
> filed keel alive.
This doesn't answer what "Pause its operation" means. If the
specification is not precise, then the best that can be implemented is
for a specific device to be compatible with a specific driver. If we
actually want portable, spec-compliant devices, we need explicitly
defined MUST/MUST NOT requirements. Presumably we want something like:
A device MUST not send notifications while suspended.
A device MUST ignore writes to its device configuration while
suspended, except for writes to the device status byte.
A driver MUST NOT write to a suspended device's configuration space,
except for the device status byte.
We also need to specify what a suspended device is allowed to do with
buffers it owns. For my use case of just suspending/resuming a VM, I
don't think it particularly matters if a suspended device writes to
buffers or descriptors while it is suspended. But based on what you
wrote about finishing any in-flight processing of buffers and not
processing any empty buffers, presumably you would prefer that the
device not be allowed to access buffer/descriptors while it is
suspended? That approach is probably cleaner from a pure specification
standpoint, but it's also more restrictive for devices and thus harder
to properly implement.
-David
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 24+ messages in thread* [virtio-comment] Re: [PATCH] virtio: introduce SUSPEND bit in device status
2024-02-20 5:09 ` [virtio-dev] " David Stevens
@ 2024-02-20 7:48 ` Michael S. Tsirkin
-1 siblings, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2024-02-20 7:48 UTC (permalink / raw)
To: David Stevens
Cc: Zhu, Lingshan, jasowang, eperezma, cohuck, stefanha,
virtio-comment, virtio-dev
On Tue, Feb 20, 2024 at 02:09:18PM +0900, David Stevens wrote:
> On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> > On 2/19/2024 2:46 PM, David Stevens wrote:
> > > On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
> > >>> This commit allows the driver to suspend the device by
> > >>> introducing a new status bit SUSPEND in device_status.
> > >>>
> > >>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
> > >>> which indicating whether the device support SUSPEND.
> > >>>
> > >>> This SUSPEND bit is transport-independent.
> > >>>
> > >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > >> Could we get some kind of dscription how this has taken into
> > >> consideration the proposal from David Stevens?
> > >>
> > >> I find it really tiring when there are competing patches with authors
> > >> ignoring each other's work and leaving it up to reviewers to
> > >> figure out how do the patches compare.
> > > This patch looks like it could be used to implement my use case.
> > > However, parts of it are a bit vague and imprecise, so it's hard to
> > > actually say whether my use case would actually be covered by a
> > > specific implementation of this proposal.
> > I am on vacation till this Friday. Shall we co-work on this and
> > post something new together?
>
> That works for me.
>
> > >
> > > To give specifics on what I'm trying to do, I need to allow a guest
> > > with virtio-pci devices (including stateful devices like virtio-fs and
> > > virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
> > > to wake up the guest (currently via an ACPI GPE, but eventually via a
> > > native PCI PME). This is all done on a consumer device, so there is no
> > > need for snapshotting or for live migration.
> > Suspending is not dedicated only for live migration.
> > For your use case, shall we add a new PCI section like saying: when entering
> > SUSPEND, the device should get into S1/S3 and other PCI specific steps
> > in PCI transport charter.
> >
> > That is because PCI is a transport layer of virtio, controlling virtio
> > states by PCI sounds like a layer violation.
>
> I don't know if it's really necessary to mandate that in the spec.
> Sure, most systems are probably going to want to put the virtio-pci
> device into S1/S3 after suspending virtio operation. But nothing
> really breaks or goes wrong if we don't do that.
What I feel is worth mentioning in virtio spec is that just setting the
status bit does not reduce power by itself, standard pci
control is required for that.
> > >
> > >>> ---
> > >>> content.tex | 34 ++++++++++++++++++++++++++++++++--
> > >>> 1 file changed, 32 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/content.tex b/content.tex
> > >>> index 0a62dce..3d656b5 100644
> > >>> --- a/content.tex
> > >>> +++ b/content.tex
> > >>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > >>>
> > >>> \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> > >>> an error from which it can't recover.
> > >>> +
> > >>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> > >>> + device has been suspended by the driver.
> > >>> \end{description}
> > >>>
> > >>> The \field{device status} field starts out as 0, and is reinitialized to 0 by
> > >>> @@ -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 SHOULD NOT set SUSPEND if FEATURES_OK is not set.
> > >>> +
> > >>> +When setting 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,25 @@ \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.
> > >>> +
> > >>> +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
> > >>> +
> > >>> +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
> > >>> +
> > >>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
> > >>> +and resumes operation upon DRIVER_OK.
> > >>> +
> > >>> +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
> > >>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
> > >>> +
> > >>> +\begin{itemize}
> > >>> +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
> > >>> +\item Wait until all descriptors that being processed to finish and mark them as used.
> > >>> +\item Flush all used buffer and send used buffer notifications to the driver.
> > > What's the difference between the previous three lines? They might be
> > > trying to say different things, but there is a lot of overlap in how
> > > they're phrased. That makes it hard to figure out exactly what they're
> > > mandating. Is it something like: "stop processing new buffers",
> > > "finish processing any buffers that are currently in flight", "mark
> > > all finished buffers as used and send a used buffer notification"?
> > sure, the "mark used" parts can merge.
> > >
> > > Also, what does this mean for devices where the driver places empty
> > > buffers into the virtqueue that the device then holds on to for an
> > > indeterminate period of time (e.g. network receiveq, balloon statsq,
> > > etc)?
> > The device doesn't process them.
> > Oh, I should add: The driver should not make any more buffers available
> > to the device.
> > >
> > >>> +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> > > What does "Pause its operation except device status" mean? The word
> > > "operation" is vague, what exactly does it include? Also what does
> > > "operate its device status" mean?
> > Because we need to resume the device after suspending it by setting
> > DRIVER_OK, so we need the device status
> > filed keel alive.
>
> This doesn't answer what "Pause its operation" means. If the
> specification is not precise, then the best that can be implemented is
> for a specific device to be compatible with a specific driver. If we
> actually want portable, spec-compliant devices, we need explicitly
> defined MUST/MUST NOT requirements. Presumably we want something like:
>
> A device MUST not send notifications while suspended.
> A device MUST ignore writes to its device configuration while
> suspended, except for writes to the device status byte.
> A driver MUST NOT write to a suspended device's configuration space,
> except for the device status byte.
>
> We also need to specify what a suspended device is allowed to do with
> buffers it owns. For my use case of just suspending/resuming a VM, I
> don't think it particularly matters if a suspended device writes to
> buffers or descriptors while it is suspended. But based on what you
> wrote about finishing any in-flight processing of buffers and not
> processing any empty buffers, presumably you would prefer that the
> device not be allowed to access buffer/descriptors while it is
> suspended? That approach is probably cleaner from a pure specification
> standpoint, but it's also more restrictive for devices and thus harder
> to properly implement.
>
> -David
Of course it's a bit harder. It does not sound too restrictive
to me: as driver will not be adding new buffers, if device is
still processing queues just defer setting the bit until it does not.
This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.
In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.
Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/
^ permalink raw reply [flat|nested] 24+ messages in thread* [virtio-dev] Re: [PATCH] virtio: introduce SUSPEND bit in device status
@ 2024-02-20 7:48 ` Michael S. Tsirkin
0 siblings, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2024-02-20 7:48 UTC (permalink / raw)
To: David Stevens
Cc: Zhu, Lingshan, jasowang, eperezma, cohuck, stefanha,
virtio-comment, virtio-dev
On Tue, Feb 20, 2024 at 02:09:18PM +0900, David Stevens wrote:
> On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> > On 2/19/2024 2:46 PM, David Stevens wrote:
> > > On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
> > >>> This commit allows the driver to suspend the device by
> > >>> introducing a new status bit SUSPEND in device_status.
> > >>>
> > >>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
> > >>> which indicating whether the device support SUSPEND.
> > >>>
> > >>> This SUSPEND bit is transport-independent.
> > >>>
> > >>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > >>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > >> Could we get some kind of dscription how this has taken into
> > >> consideration the proposal from David Stevens?
> > >>
> > >> I find it really tiring when there are competing patches with authors
> > >> ignoring each other's work and leaving it up to reviewers to
> > >> figure out how do the patches compare.
> > > This patch looks like it could be used to implement my use case.
> > > However, parts of it are a bit vague and imprecise, so it's hard to
> > > actually say whether my use case would actually be covered by a
> > > specific implementation of this proposal.
> > I am on vacation till this Friday. Shall we co-work on this and
> > post something new together?
>
> That works for me.
>
> > >
> > > To give specifics on what I'm trying to do, I need to allow a guest
> > > with virtio-pci devices (including stateful devices like virtio-fs and
> > > virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
> > > to wake up the guest (currently via an ACPI GPE, but eventually via a
> > > native PCI PME). This is all done on a consumer device, so there is no
> > > need for snapshotting or for live migration.
> > Suspending is not dedicated only for live migration.
> > For your use case, shall we add a new PCI section like saying: when entering
> > SUSPEND, the device should get into S1/S3 and other PCI specific steps
> > in PCI transport charter.
> >
> > That is because PCI is a transport layer of virtio, controlling virtio
> > states by PCI sounds like a layer violation.
>
> I don't know if it's really necessary to mandate that in the spec.
> Sure, most systems are probably going to want to put the virtio-pci
> device into S1/S3 after suspending virtio operation. But nothing
> really breaks or goes wrong if we don't do that.
What I feel is worth mentioning in virtio spec is that just setting the
status bit does not reduce power by itself, standard pci
control is required for that.
> > >
> > >>> ---
> > >>> content.tex | 34 ++++++++++++++++++++++++++++++++--
> > >>> 1 file changed, 32 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/content.tex b/content.tex
> > >>> index 0a62dce..3d656b5 100644
> > >>> --- a/content.tex
> > >>> +++ b/content.tex
> > >>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > >>>
> > >>> \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> > >>> an error from which it can't recover.
> > >>> +
> > >>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> > >>> + device has been suspended by the driver.
> > >>> \end{description}
> > >>>
> > >>> The \field{device status} field starts out as 0, and is reinitialized to 0 by
> > >>> @@ -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 SHOULD NOT set SUSPEND if FEATURES_OK is not set.
> > >>> +
> > >>> +When setting 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,25 @@ \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.
> > >>> +
> > >>> +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
> > >>> +
> > >>> +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
> > >>> +
> > >>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
> > >>> +and resumes operation upon DRIVER_OK.
> > >>> +
> > >>> +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
> > >>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
> > >>> +
> > >>> +\begin{itemize}
> > >>> +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
> > >>> +\item Wait until all descriptors that being processed to finish and mark them as used.
> > >>> +\item Flush all used buffer and send used buffer notifications to the driver.
> > > What's the difference between the previous three lines? They might be
> > > trying to say different things, but there is a lot of overlap in how
> > > they're phrased. That makes it hard to figure out exactly what they're
> > > mandating. Is it something like: "stop processing new buffers",
> > > "finish processing any buffers that are currently in flight", "mark
> > > all finished buffers as used and send a used buffer notification"?
> > sure, the "mark used" parts can merge.
> > >
> > > Also, what does this mean for devices where the driver places empty
> > > buffers into the virtqueue that the device then holds on to for an
> > > indeterminate period of time (e.g. network receiveq, balloon statsq,
> > > etc)?
> > The device doesn't process them.
> > Oh, I should add: The driver should not make any more buffers available
> > to the device.
> > >
> > >>> +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> > > What does "Pause its operation except device status" mean? The word
> > > "operation" is vague, what exactly does it include? Also what does
> > > "operate its device status" mean?
> > Because we need to resume the device after suspending it by setting
> > DRIVER_OK, so we need the device status
> > filed keel alive.
>
> This doesn't answer what "Pause its operation" means. If the
> specification is not precise, then the best that can be implemented is
> for a specific device to be compatible with a specific driver. If we
> actually want portable, spec-compliant devices, we need explicitly
> defined MUST/MUST NOT requirements. Presumably we want something like:
>
> A device MUST not send notifications while suspended.
> A device MUST ignore writes to its device configuration while
> suspended, except for writes to the device status byte.
> A driver MUST NOT write to a suspended device's configuration space,
> except for the device status byte.
>
> We also need to specify what a suspended device is allowed to do with
> buffers it owns. For my use case of just suspending/resuming a VM, I
> don't think it particularly matters if a suspended device writes to
> buffers or descriptors while it is suspended. But based on what you
> wrote about finishing any in-flight processing of buffers and not
> processing any empty buffers, presumably you would prefer that the
> device not be allowed to access buffer/descriptors while it is
> suspended? That approach is probably cleaner from a pure specification
> standpoint, but it's also more restrictive for devices and thus harder
> to properly implement.
>
> -David
Of course it's a bit harder. It does not sound too restrictive
to me: as driver will not be adding new buffers, if device is
still processing queues just defer setting the bit until it does not.
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 24+ messages in thread* [virtio-comment] Re: [PATCH] virtio: introduce SUSPEND bit in device status
2024-02-20 7:48 ` [virtio-dev] " Michael S. Tsirkin
@ 2024-02-23 7:46 ` Zhu, Lingshan
-1 siblings, 0 replies; 24+ messages in thread
From: Zhu, Lingshan @ 2024-02-23 7:46 UTC (permalink / raw)
To: Michael S. Tsirkin, David Stevens
Cc: jasowang, eperezma, cohuck, stefanha, virtio-comment, virtio-dev
On 2/20/2024 3:48 PM, Michael S. Tsirkin wrote:
> On Tue, Feb 20, 2024 at 02:09:18PM +0900, David Stevens wrote:
>> On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>> On 2/19/2024 2:46 PM, David Stevens wrote:
>>>> On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
>>>>>> This commit allows the driver to suspend the device by
>>>>>> introducing a new status bit SUSPEND in device_status.
>>>>>>
>>>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
>>>>>> which indicating whether the device support SUSPEND.
>>>>>>
>>>>>> This SUSPEND bit is transport-independent.
>>>>>>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>> Could we get some kind of dscription how this has taken into
>>>>> consideration the proposal from David Stevens?
>>>>>
>>>>> I find it really tiring when there are competing patches with authors
>>>>> ignoring each other's work and leaving it up to reviewers to
>>>>> figure out how do the patches compare.
>>>> This patch looks like it could be used to implement my use case.
>>>> However, parts of it are a bit vague and imprecise, so it's hard to
>>>> actually say whether my use case would actually be covered by a
>>>> specific implementation of this proposal.
>>> I am on vacation till this Friday. Shall we co-work on this and
>>> post something new together?
>> That works for me.
>>
>>>> To give specifics on what I'm trying to do, I need to allow a guest
>>>> with virtio-pci devices (including stateful devices like virtio-fs and
>>>> virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
>>>> to wake up the guest (currently via an ACPI GPE, but eventually via a
>>>> native PCI PME). This is all done on a consumer device, so there is no
>>>> need for snapshotting or for live migration.
>>> Suspending is not dedicated only for live migration.
>>> For your use case, shall we add a new PCI section like saying: when entering
>>> SUSPEND, the device should get into S1/S3 and other PCI specific steps
>>> in PCI transport charter.
>>>
>>> That is because PCI is a transport layer of virtio, controlling virtio
>>> states by PCI sounds like a layer violation.
>> I don't know if it's really necessary to mandate that in the spec.
>> Sure, most systems are probably going to want to put the virtio-pci
>> device into S1/S3 after suspending virtio operation. But nothing
>> really breaks or goes wrong if we don't do that.
> What I feel is worth mentioning in virtio spec is that just setting the
> status bit does not reduce power by itself, standard pci
> control is required for that.
I agree, only mention power management in a separate PCI transport patch in
a new collaborated series
>
>>>>>> ---
>>>>>> content.tex | 34 ++++++++++++++++++++++++++++++++--
>>>>>> 1 file changed, 32 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/content.tex b/content.tex
>>>>>> index 0a62dce..3d656b5 100644
>>>>>> --- a/content.tex
>>>>>> +++ b/content.tex
>>>>>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>>
>>>>>> \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>>>> an error from which it can't recover.
>>>>>> +
>>>>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>>>>> + device has been suspended by the driver.
>>>>>> \end{description}
>>>>>>
>>>>>> The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>>>>> @@ -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 SHOULD NOT set SUSPEND if FEATURES_OK is not set.
>>>>>> +
>>>>>> +When setting 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,25 @@ \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.
>>>>>> +
>>>>>> +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>>>>>> +
>>>>>> +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
>>>>>> +
>>>>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
>>>>>> +and resumes operation upon DRIVER_OK.
>>>>>> +
>>>>>> +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
>>>>>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
>>>>>> +
>>>>>> +\begin{itemize}
>>>>>> +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
>>>>>> +\item Wait until all descriptors that being processed to finish and mark them as used.
>>>>>> +\item Flush all used buffer and send used buffer notifications to the driver.
>>>> What's the difference between the previous three lines? They might be
>>>> trying to say different things, but there is a lot of overlap in how
>>>> they're phrased. That makes it hard to figure out exactly what they're
>>>> mandating. Is it something like: "stop processing new buffers",
>>>> "finish processing any buffers that are currently in flight", "mark
>>>> all finished buffers as used and send a used buffer notification"?
>>> sure, the "mark used" parts can merge.
>>>> Also, what does this mean for devices where the driver places empty
>>>> buffers into the virtqueue that the device then holds on to for an
>>>> indeterminate period of time (e.g. network receiveq, balloon statsq,
>>>> etc)?
>>> The device doesn't process them.
>>> Oh, I should add: The driver should not make any more buffers available
>>> to the device.
>>>>>> +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>>>> What does "Pause its operation except device status" mean? The word
>>>> "operation" is vague, what exactly does it include? Also what does
>>>> "operate its device status" mean?
>>> Because we need to resume the device after suspending it by setting
>>> DRIVER_OK, so we need the device status
>>> filed keel alive.
>> This doesn't answer what "Pause its operation" means. If the
>> specification is not precise, then the best that can be implemented is
>> for a specific device to be compatible with a specific driver. If we
>> actually want portable, spec-compliant devices, we need explicitly
>> defined MUST/MUST NOT requirements. Presumably we want something like:
>>
>> A device MUST not send notifications while suspended.
>> A device MUST ignore writes to its device configuration while
>> suspended, except for writes to the device status byte.
>> A driver MUST NOT write to a suspended device's configuration space,
>> except for the device status byte.
>>
>> We also need to specify what a suspended device is allowed to do with
>> buffers it owns. For my use case of just suspending/resuming a VM, I
>> don't think it particularly matters if a suspended device writes to
>> buffers or descriptors while it is suspended. But based on what you
>> wrote about finishing any in-flight processing of buffers and not
>> processing any empty buffers, presumably you would prefer that the
>> device not be allowed to access buffer/descriptors while it is
>> suspended? That approach is probably cleaner from a pure specification
>> standpoint, but it's also more restrictive for devices and thus harder
>> to properly implement.
>>
>> -David
> Of course it's a bit harder. It does not sound too restrictive
> to me: as driver will not be adding new buffers, if device is
> still processing queues just defer setting the bit until it does not.
Yes, I agree
Thanks
Zhu Lingshan
>
This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.
In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.
Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/
^ permalink raw reply [flat|nested] 24+ messages in thread* [virtio-dev] Re: [PATCH] virtio: introduce SUSPEND bit in device status
@ 2024-02-23 7:46 ` Zhu, Lingshan
0 siblings, 0 replies; 24+ messages in thread
From: Zhu, Lingshan @ 2024-02-23 7:46 UTC (permalink / raw)
To: Michael S. Tsirkin, David Stevens
Cc: jasowang, eperezma, cohuck, stefanha, virtio-comment, virtio-dev
On 2/20/2024 3:48 PM, Michael S. Tsirkin wrote:
> On Tue, Feb 20, 2024 at 02:09:18PM +0900, David Stevens wrote:
>> On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>> On 2/19/2024 2:46 PM, David Stevens wrote:
>>>> On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
>>>>>> This commit allows the driver to suspend the device by
>>>>>> introducing a new status bit SUSPEND in device_status.
>>>>>>
>>>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
>>>>>> which indicating whether the device support SUSPEND.
>>>>>>
>>>>>> This SUSPEND bit is transport-independent.
>>>>>>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>> Could we get some kind of dscription how this has taken into
>>>>> consideration the proposal from David Stevens?
>>>>>
>>>>> I find it really tiring when there are competing patches with authors
>>>>> ignoring each other's work and leaving it up to reviewers to
>>>>> figure out how do the patches compare.
>>>> This patch looks like it could be used to implement my use case.
>>>> However, parts of it are a bit vague and imprecise, so it's hard to
>>>> actually say whether my use case would actually be covered by a
>>>> specific implementation of this proposal.
>>> I am on vacation till this Friday. Shall we co-work on this and
>>> post something new together?
>> That works for me.
>>
>>>> To give specifics on what I'm trying to do, I need to allow a guest
>>>> with virtio-pci devices (including stateful devices like virtio-fs and
>>>> virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
>>>> to wake up the guest (currently via an ACPI GPE, but eventually via a
>>>> native PCI PME). This is all done on a consumer device, so there is no
>>>> need for snapshotting or for live migration.
>>> Suspending is not dedicated only for live migration.
>>> For your use case, shall we add a new PCI section like saying: when entering
>>> SUSPEND, the device should get into S1/S3 and other PCI specific steps
>>> in PCI transport charter.
>>>
>>> That is because PCI is a transport layer of virtio, controlling virtio
>>> states by PCI sounds like a layer violation.
>> I don't know if it's really necessary to mandate that in the spec.
>> Sure, most systems are probably going to want to put the virtio-pci
>> device into S1/S3 after suspending virtio operation. But nothing
>> really breaks or goes wrong if we don't do that.
> What I feel is worth mentioning in virtio spec is that just setting the
> status bit does not reduce power by itself, standard pci
> control is required for that.
I agree, only mention power management in a separate PCI transport patch in
a new collaborated series
>
>>>>>> ---
>>>>>> content.tex | 34 ++++++++++++++++++++++++++++++++--
>>>>>> 1 file changed, 32 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/content.tex b/content.tex
>>>>>> index 0a62dce..3d656b5 100644
>>>>>> --- a/content.tex
>>>>>> +++ b/content.tex
>>>>>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>>
>>>>>> \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>>>> an error from which it can't recover.
>>>>>> +
>>>>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>>>>> + device has been suspended by the driver.
>>>>>> \end{description}
>>>>>>
>>>>>> The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>>>>> @@ -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 SHOULD NOT set SUSPEND if FEATURES_OK is not set.
>>>>>> +
>>>>>> +When setting 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,25 @@ \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.
>>>>>> +
>>>>>> +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>>>>>> +
>>>>>> +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
>>>>>> +
>>>>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
>>>>>> +and resumes operation upon DRIVER_OK.
>>>>>> +
>>>>>> +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
>>>>>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
>>>>>> +
>>>>>> +\begin{itemize}
>>>>>> +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
>>>>>> +\item Wait until all descriptors that being processed to finish and mark them as used.
>>>>>> +\item Flush all used buffer and send used buffer notifications to the driver.
>>>> What's the difference between the previous three lines? They might be
>>>> trying to say different things, but there is a lot of overlap in how
>>>> they're phrased. That makes it hard to figure out exactly what they're
>>>> mandating. Is it something like: "stop processing new buffers",
>>>> "finish processing any buffers that are currently in flight", "mark
>>>> all finished buffers as used and send a used buffer notification"?
>>> sure, the "mark used" parts can merge.
>>>> Also, what does this mean for devices where the driver places empty
>>>> buffers into the virtqueue that the device then holds on to for an
>>>> indeterminate period of time (e.g. network receiveq, balloon statsq,
>>>> etc)?
>>> The device doesn't process them.
>>> Oh, I should add: The driver should not make any more buffers available
>>> to the device.
>>>>>> +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>>>> What does "Pause its operation except device status" mean? The word
>>>> "operation" is vague, what exactly does it include? Also what does
>>>> "operate its device status" mean?
>>> Because we need to resume the device after suspending it by setting
>>> DRIVER_OK, so we need the device status
>>> filed keel alive.
>> This doesn't answer what "Pause its operation" means. If the
>> specification is not precise, then the best that can be implemented is
>> for a specific device to be compatible with a specific driver. If we
>> actually want portable, spec-compliant devices, we need explicitly
>> defined MUST/MUST NOT requirements. Presumably we want something like:
>>
>> A device MUST not send notifications while suspended.
>> A device MUST ignore writes to its device configuration while
>> suspended, except for writes to the device status byte.
>> A driver MUST NOT write to a suspended device's configuration space,
>> except for the device status byte.
>>
>> We also need to specify what a suspended device is allowed to do with
>> buffers it owns. For my use case of just suspending/resuming a VM, I
>> don't think it particularly matters if a suspended device writes to
>> buffers or descriptors while it is suspended. But based on what you
>> wrote about finishing any in-flight processing of buffers and not
>> processing any empty buffers, presumably you would prefer that the
>> device not be allowed to access buffer/descriptors while it is
>> suspended? That approach is probably cleaner from a pure specification
>> standpoint, but it's also more restrictive for devices and thus harder
>> to properly implement.
>>
>> -David
> Of course it's a bit harder. It does not sound too restrictive
> to me: as driver will not be adding new buffers, if device is
> still processing queues just defer setting the bit until it does not.
Yes, I agree
Thanks
Zhu Lingshan
>
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 24+ messages in thread
* [virtio-comment] Re: [PATCH] virtio: introduce SUSPEND bit in device status
2024-02-20 5:09 ` [virtio-dev] " David Stevens
@ 2024-02-23 7:44 ` Zhu, Lingshan
-1 siblings, 0 replies; 24+ messages in thread
From: Zhu, Lingshan @ 2024-02-23 7:44 UTC (permalink / raw)
To: David Stevens
Cc: Michael S. Tsirkin, jasowang, eperezma, cohuck, stefanha,
virtio-comment, virtio-dev
On 2/20/2024 1:09 PM, David Stevens wrote:
> On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>> On 2/19/2024 2:46 PM, David Stevens wrote:
>>> On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
>>>>> This commit allows the driver to suspend the device by
>>>>> introducing a new status bit SUSPEND in device_status.
>>>>>
>>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
>>>>> which indicating whether the device support SUSPEND.
>>>>>
>>>>> This SUSPEND bit is transport-independent.
>>>>>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> Could we get some kind of dscription how this has taken into
>>>> consideration the proposal from David Stevens?
>>>>
>>>> I find it really tiring when there are competing patches with authors
>>>> ignoring each other's work and leaving it up to reviewers to
>>>> figure out how do the patches compare.
>>> This patch looks like it could be used to implement my use case.
>>> However, parts of it are a bit vague and imprecise, so it's hard to
>>> actually say whether my use case would actually be covered by a
>>> specific implementation of this proposal.
>> I am on vacation till this Friday. Shall we co-work on this and
>> post something new together?
> That works for me.
Cool! How about two patches in a new series:
1) a general virtio suspending patch describing suspending behaviors for all
virtio devices from me, an updating version of this patch
2) a PCI transport suspending patch from you, describing PCI specific
behaviors.
Dose this sound good to you?
>
>>> To give specifics on what I'm trying to do, I need to allow a guest
>>> with virtio-pci devices (including stateful devices like virtio-fs and
>>> virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
>>> to wake up the guest (currently via an ACPI GPE, but eventually via a
>>> native PCI PME). This is all done on a consumer device, so there is no
>>> need for snapshotting or for live migration.
>> Suspending is not dedicated only for live migration.
>> For your use case, shall we add a new PCI section like saying: when entering
>> SUSPEND, the device should get into S1/S3 and other PCI specific steps
>> in PCI transport charter.
>>
>> That is because PCI is a transport layer of virtio, controlling virtio
>> states by PCI sounds like a layer violation.
> I don't know if it's really necessary to mandate that in the spec.
> Sure, most systems are probably going to want to put the virtio-pci
> device into S1/S3 after suspending virtio operation. But nothing
> really breaks or goes wrong if we don't do that.
Yes, maybe not S1/S3, I remember you want to manage power in PCI
transport when suspend in a cap, S1/S3 is just an example.
>
>>>>> ---
>>>>> content.tex | 34 ++++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 32 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/content.tex b/content.tex
>>>>> index 0a62dce..3d656b5 100644
>>>>> --- a/content.tex
>>>>> +++ b/content.tex
>>>>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>
>>>>> \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>>> an error from which it can't recover.
>>>>> +
>>>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>>>> + device has been suspended by the driver.
>>>>> \end{description}
>>>>>
>>>>> The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>>>> @@ -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 SHOULD NOT set SUSPEND if FEATURES_OK is not set.
>>>>> +
>>>>> +When setting 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,25 @@ \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.
>>>>> +
>>>>> +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>>>>> +
>>>>> +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
>>>>> +
>>>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
>>>>> +and resumes operation upon DRIVER_OK.
>>>>> +
>>>>> +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
>>>>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
>>>>> +
>>>>> +\begin{itemize}
>>>>> +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
>>>>> +\item Wait until all descriptors that being processed to finish and mark them as used.
>>>>> +\item Flush all used buffer and send used buffer notifications to the driver.
>>> What's the difference between the previous three lines? They might be
>>> trying to say different things, but there is a lot of overlap in how
>>> they're phrased. That makes it hard to figure out exactly what they're
>>> mandating. Is it something like: "stop processing new buffers",
>>> "finish processing any buffers that are currently in flight", "mark
>>> all finished buffers as used and send a used buffer notification"?
>> sure, the "mark used" parts can merge.
>>> Also, what does this mean for devices where the driver places empty
>>> buffers into the virtqueue that the device then holds on to for an
>>> indeterminate period of time (e.g. network receiveq, balloon statsq,
>>> etc)?
>> The device doesn't process them.
>> Oh, I should add: The driver should not make any more buffers available
>> to the device.
>>>>> +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>>> What does "Pause its operation except device status" mean? The word
>>> "operation" is vague, what exactly does it include? Also what does
>>> "operate its device status" mean?
>> Because we need to resume the device after suspending it by setting
>> DRIVER_OK, so we need the device status
>> filed keel alive.
> This doesn't answer what "Pause its operation" means. If the
> specification is not precise, then the best that can be implemented is
> for a specific device to be compatible with a specific driver. If we
> actually want portable, spec-compliant devices, we need explicitly
> defined MUST/MUST NOT requirements. Presumably we want something like:
>
> A device MUST not send notifications while suspended.
The device should send a last used buffer notification if
it has buffers under processing before presenting SUSPEND
in the device status, but the device should not process
any new buffers. These are in devicenormative section,
but they may be vague, I can add more details to make
it precise, thanks for the tips
> A device MUST ignore writes to its device configuration while
> suspended, except for writes to the device status byte.
> A driver MUST NOT write to a suspended device's configuration space,
> except for the device status byte.
sure, we can make it more precise than only pausing.
>
> We also need to specify what a suspended device is allowed to do with
> buffers it owns. For my use case of just suspending/resuming a VM, I
> don't think it particularly matters if a suspended device writes to
> buffers or descriptors while it is suspended. But based on what you
> wrote about finishing any in-flight processing of buffers and not
> processing any empty buffers, presumably you would prefer that the
> device not be allowed to access buffer/descriptors while it is
> suspended? That approach is probably cleaner from a pure specification
> standpoint, but it's also more restrictive for devices and thus harder
> to properly implement.
When the driver sets SUSPEND, the device should finished all buffers that
already under processing, include the in-flight buffers(or it may lose
the buffers).
And the device should not touch the buffers once it present SUSPEND in the
device status field, because once it presents SUSPEND, the device state
should
be stable, people may use SUSPEND to debug the device.
The device still can process the buffer before it presenting SUSPEND.
Thanks
Zhu Lingshan
>
> -David
This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.
In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.
Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/
^ permalink raw reply [flat|nested] 24+ messages in thread* [virtio-dev] Re: [PATCH] virtio: introduce SUSPEND bit in device status
@ 2024-02-23 7:44 ` Zhu, Lingshan
0 siblings, 0 replies; 24+ messages in thread
From: Zhu, Lingshan @ 2024-02-23 7:44 UTC (permalink / raw)
To: David Stevens
Cc: Michael S. Tsirkin, jasowang, eperezma, cohuck, stefanha,
virtio-comment, virtio-dev
On 2/20/2024 1:09 PM, David Stevens wrote:
> On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>> On 2/19/2024 2:46 PM, David Stevens wrote:
>>> On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
>>>>> This commit allows the driver to suspend the device by
>>>>> introducing a new status bit SUSPEND in device_status.
>>>>>
>>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
>>>>> which indicating whether the device support SUSPEND.
>>>>>
>>>>> This SUSPEND bit is transport-independent.
>>>>>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> Could we get some kind of dscription how this has taken into
>>>> consideration the proposal from David Stevens?
>>>>
>>>> I find it really tiring when there are competing patches with authors
>>>> ignoring each other's work and leaving it up to reviewers to
>>>> figure out how do the patches compare.
>>> This patch looks like it could be used to implement my use case.
>>> However, parts of it are a bit vague and imprecise, so it's hard to
>>> actually say whether my use case would actually be covered by a
>>> specific implementation of this proposal.
>> I am on vacation till this Friday. Shall we co-work on this and
>> post something new together?
> That works for me.
Cool! How about two patches in a new series:
1) a general virtio suspending patch describing suspending behaviors for all
virtio devices from me, an updating version of this patch
2) a PCI transport suspending patch from you, describing PCI specific
behaviors.
Dose this sound good to you?
>
>>> To give specifics on what I'm trying to do, I need to allow a guest
>>> with virtio-pci devices (including stateful devices like virtio-fs and
>>> virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
>>> to wake up the guest (currently via an ACPI GPE, but eventually via a
>>> native PCI PME). This is all done on a consumer device, so there is no
>>> need for snapshotting or for live migration.
>> Suspending is not dedicated only for live migration.
>> For your use case, shall we add a new PCI section like saying: when entering
>> SUSPEND, the device should get into S1/S3 and other PCI specific steps
>> in PCI transport charter.
>>
>> That is because PCI is a transport layer of virtio, controlling virtio
>> states by PCI sounds like a layer violation.
> I don't know if it's really necessary to mandate that in the spec.
> Sure, most systems are probably going to want to put the virtio-pci
> device into S1/S3 after suspending virtio operation. But nothing
> really breaks or goes wrong if we don't do that.
Yes, maybe not S1/S3, I remember you want to manage power in PCI
transport when suspend in a cap, S1/S3 is just an example.
>
>>>>> ---
>>>>> content.tex | 34 ++++++++++++++++++++++++++++++++--
>>>>> 1 file changed, 32 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/content.tex b/content.tex
>>>>> index 0a62dce..3d656b5 100644
>>>>> --- a/content.tex
>>>>> +++ b/content.tex
>>>>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>
>>>>> \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>>> an error from which it can't recover.
>>>>> +
>>>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>>>> + device has been suspended by the driver.
>>>>> \end{description}
>>>>>
>>>>> The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>>>> @@ -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 SHOULD NOT set SUSPEND if FEATURES_OK is not set.
>>>>> +
>>>>> +When setting 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,25 @@ \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.
>>>>> +
>>>>> +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>>>>> +
>>>>> +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
>>>>> +
>>>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
>>>>> +and resumes operation upon DRIVER_OK.
>>>>> +
>>>>> +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
>>>>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
>>>>> +
>>>>> +\begin{itemize}
>>>>> +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
>>>>> +\item Wait until all descriptors that being processed to finish and mark them as used.
>>>>> +\item Flush all used buffer and send used buffer notifications to the driver.
>>> What's the difference between the previous three lines? They might be
>>> trying to say different things, but there is a lot of overlap in how
>>> they're phrased. That makes it hard to figure out exactly what they're
>>> mandating. Is it something like: "stop processing new buffers",
>>> "finish processing any buffers that are currently in flight", "mark
>>> all finished buffers as used and send a used buffer notification"?
>> sure, the "mark used" parts can merge.
>>> Also, what does this mean for devices where the driver places empty
>>> buffers into the virtqueue that the device then holds on to for an
>>> indeterminate period of time (e.g. network receiveq, balloon statsq,
>>> etc)?
>> The device doesn't process them.
>> Oh, I should add: The driver should not make any more buffers available
>> to the device.
>>>>> +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>>> What does "Pause its operation except device status" mean? The word
>>> "operation" is vague, what exactly does it include? Also what does
>>> "operate its device status" mean?
>> Because we need to resume the device after suspending it by setting
>> DRIVER_OK, so we need the device status
>> filed keel alive.
> This doesn't answer what "Pause its operation" means. If the
> specification is not precise, then the best that can be implemented is
> for a specific device to be compatible with a specific driver. If we
> actually want portable, spec-compliant devices, we need explicitly
> defined MUST/MUST NOT requirements. Presumably we want something like:
>
> A device MUST not send notifications while suspended.
The device should send a last used buffer notification if
it has buffers under processing before presenting SUSPEND
in the device status, but the device should not process
any new buffers. These are in devicenormative section,
but they may be vague, I can add more details to make
it precise, thanks for the tips
> A device MUST ignore writes to its device configuration while
> suspended, except for writes to the device status byte.
> A driver MUST NOT write to a suspended device's configuration space,
> except for the device status byte.
sure, we can make it more precise than only pausing.
>
> We also need to specify what a suspended device is allowed to do with
> buffers it owns. For my use case of just suspending/resuming a VM, I
> don't think it particularly matters if a suspended device writes to
> buffers or descriptors while it is suspended. But based on what you
> wrote about finishing any in-flight processing of buffers and not
> processing any empty buffers, presumably you would prefer that the
> device not be allowed to access buffer/descriptors while it is
> suspended? That approach is probably cleaner from a pure specification
> standpoint, but it's also more restrictive for devices and thus harder
> to properly implement.
When the driver sets SUSPEND, the device should finished all buffers that
already under processing, include the in-flight buffers(or it may lose
the buffers).
And the device should not touch the buffers once it present SUSPEND in the
device status field, because once it presents SUSPEND, the device state
should
be stable, people may use SUSPEND to debug the device.
The device still can process the buffer before it presenting SUSPEND.
Thanks
Zhu Lingshan
>
> -David
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 24+ messages in thread* [virtio-comment] Re: [PATCH] virtio: introduce SUSPEND bit in device status
2024-02-23 7:44 ` [virtio-dev] " Zhu, Lingshan
@ 2024-02-25 8:52 ` Michael S. Tsirkin
-1 siblings, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2024-02-25 8:52 UTC (permalink / raw)
To: Zhu, Lingshan
Cc: David Stevens, jasowang, eperezma, cohuck, stefanha,
virtio-comment, virtio-dev
On Fri, Feb 23, 2024 at 03:44:41PM +0800, Zhu, Lingshan wrote:
>
>
> On 2/20/2024 1:09 PM, David Stevens wrote:
> > On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> > > On 2/19/2024 2:46 PM, David Stevens wrote:
> > > > On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
> > > > > > This commit allows the driver to suspend the device by
> > > > > > introducing a new status bit SUSPEND in device_status.
> > > > > >
> > > > > > This commit also introduce a new feature bit VIRTIO_F_SUSPEND
> > > > > > which indicating whether the device support SUSPEND.
> > > > > >
> > > > > > This SUSPEND bit is transport-independent.
> > > > > >
> > > > > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > Could we get some kind of dscription how this has taken into
> > > > > consideration the proposal from David Stevens?
> > > > >
> > > > > I find it really tiring when there are competing patches with authors
> > > > > ignoring each other's work and leaving it up to reviewers to
> > > > > figure out how do the patches compare.
> > > > This patch looks like it could be used to implement my use case.
> > > > However, parts of it are a bit vague and imprecise, so it's hard to
> > > > actually say whether my use case would actually be covered by a
> > > > specific implementation of this proposal.
> > > I am on vacation till this Friday. Shall we co-work on this and
> > > post something new together?
> > That works for me.
> Cool! How about two patches in a new series:
> 1) a general virtio suspending patch describing suspending behaviors for all
> virtio devices from me, an updating version of this patch
> 2) a PCI transport suspending patch from you, describing PCI specific
> behaviors.
>
> Dose this sound good to you?
Well David's patch was more precise than yours, and also better worded.
So think if there's an agreement you guys would really start with that,
move the functionality to the status bit and make other changes as needed.
> >
> > > > To give specifics on what I'm trying to do, I need to allow a guest
> > > > with virtio-pci devices (including stateful devices like virtio-fs and
> > > > virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
> > > > to wake up the guest (currently via an ACPI GPE, but eventually via a
> > > > native PCI PME). This is all done on a consumer device, so there is no
> > > > need for snapshotting or for live migration.
> > > Suspending is not dedicated only for live migration.
> > > For your use case, shall we add a new PCI section like saying: when entering
> > > SUSPEND, the device should get into S1/S3 and other PCI specific steps
> > > in PCI transport charter.
> > >
> > > That is because PCI is a transport layer of virtio, controlling virtio
> > > states by PCI sounds like a layer violation.
> > I don't know if it's really necessary to mandate that in the spec.
> > Sure, most systems are probably going to want to put the virtio-pci
> > device into S1/S3 after suspending virtio operation. But nothing
> > really breaks or goes wrong if we don't do that.
> Yes, maybe not S1/S3, I remember you want to manage power in PCI
> transport when suspend in a cap, S1/S3 is just an example.
>
> >
> > > > > > ---
> > > > > > content.tex | 34 ++++++++++++++++++++++++++++++++--
> > > > > > 1 file changed, 32 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/content.tex b/content.tex
> > > > > > index 0a62dce..3d656b5 100644
> > > > > > --- a/content.tex
> > > > > > +++ b/content.tex
> > > > > > @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > > > > >
> > > > > > \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> > > > > > an error from which it can't recover.
> > > > > > +
> > > > > > +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> > > > > > + device has been suspended by the driver.
> > > > > > \end{description}
> > > > > >
> > > > > > The \field{device status} field starts out as 0, and is reinitialized to 0 by
> > > > > > @@ -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 SHOULD NOT set SUSPEND if FEATURES_OK is not set.
> > > > > > +
> > > > > > +When setting 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,25 @@ \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.
> > > > > > +
> > > > > > +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
> > > > > > +
> > > > > > +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
> > > > > > +
> > > > > > +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
> > > > > > +and resumes operation upon DRIVER_OK.
> > > > > > +
> > > > > > +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
> > > > > > +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
> > > > > > +
> > > > > > +\begin{itemize}
> > > > > > +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
> > > > > > +\item Wait until all descriptors that being processed to finish and mark them as used.
> > > > > > +\item Flush all used buffer and send used buffer notifications to the driver.
> > > > What's the difference between the previous three lines? They might be
> > > > trying to say different things, but there is a lot of overlap in how
> > > > they're phrased. That makes it hard to figure out exactly what they're
> > > > mandating. Is it something like: "stop processing new buffers",
> > > > "finish processing any buffers that are currently in flight", "mark
> > > > all finished buffers as used and send a used buffer notification"?
> > > sure, the "mark used" parts can merge.
> > > > Also, what does this mean for devices where the driver places empty
> > > > buffers into the virtqueue that the device then holds on to for an
> > > > indeterminate period of time (e.g. network receiveq, balloon statsq,
> > > > etc)?
> > > The device doesn't process them.
> > > Oh, I should add: The driver should not make any more buffers available
> > > to the device.
> > > > > > +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> > > > What does "Pause its operation except device status" mean? The word
> > > > "operation" is vague, what exactly does it include? Also what does
> > > > "operate its device status" mean?
> > > Because we need to resume the device after suspending it by setting
> > > DRIVER_OK, so we need the device status
> > > filed keel alive.
> > This doesn't answer what "Pause its operation" means. If the
> > specification is not precise, then the best that can be implemented is
> > for a specific device to be compatible with a specific driver. If we
> > actually want portable, spec-compliant devices, we need explicitly
> > defined MUST/MUST NOT requirements. Presumably we want something like:
> >
> > A device MUST not send notifications while suspended.
> The device should send a last used buffer notification if
> it has buffers under processing before presenting SUSPEND
> in the device status, but the device should not process
> any new buffers. These are in devicenormative section,
> but they may be vague, I can add more details to make
> it precise, thanks for the tips
> > A device MUST ignore writes to its device configuration while
> > suspended, except for writes to the device status byte.
> > A driver MUST NOT write to a suspended device's configuration space,
> > except for the device status byte.
> sure, we can make it more precise than only pausing.
> >
> > We also need to specify what a suspended device is allowed to do with
> > buffers it owns. For my use case of just suspending/resuming a VM, I
> > don't think it particularly matters if a suspended device writes to
> > buffers or descriptors while it is suspended. But based on what you
> > wrote about finishing any in-flight processing of buffers and not
> > processing any empty buffers, presumably you would prefer that the
> > device not be allowed to access buffer/descriptors while it is
> > suspended? That approach is probably cleaner from a pure specification
> > standpoint, but it's also more restrictive for devices and thus harder
> > to properly implement.
> When the driver sets SUSPEND, the device should finished all buffers that
> already under processing, include the in-flight buffers(or it may lose the
> buffers).
> And the device should not touch the buffers once it present SUSPEND in the
> device status field, because once it presents SUSPEND, the device state
> should
> be stable, people may use SUSPEND to debug the device.
>
> The device still can process the buffer before it presenting SUSPEND.
>
> Thanks
> Zhu Lingshan
> >
> > -David
This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.
In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.
Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/
^ permalink raw reply [flat|nested] 24+ messages in thread* [virtio-dev] Re: [PATCH] virtio: introduce SUSPEND bit in device status
@ 2024-02-25 8:52 ` Michael S. Tsirkin
0 siblings, 0 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2024-02-25 8:52 UTC (permalink / raw)
To: Zhu, Lingshan
Cc: David Stevens, jasowang, eperezma, cohuck, stefanha,
virtio-comment, virtio-dev
On Fri, Feb 23, 2024 at 03:44:41PM +0800, Zhu, Lingshan wrote:
>
>
> On 2/20/2024 1:09 PM, David Stevens wrote:
> > On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> > > On 2/19/2024 2:46 PM, David Stevens wrote:
> > > > On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
> > > > > > This commit allows the driver to suspend the device by
> > > > > > introducing a new status bit SUSPEND in device_status.
> > > > > >
> > > > > > This commit also introduce a new feature bit VIRTIO_F_SUSPEND
> > > > > > which indicating whether the device support SUSPEND.
> > > > > >
> > > > > > This SUSPEND bit is transport-independent.
> > > > > >
> > > > > > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > Could we get some kind of dscription how this has taken into
> > > > > consideration the proposal from David Stevens?
> > > > >
> > > > > I find it really tiring when there are competing patches with authors
> > > > > ignoring each other's work and leaving it up to reviewers to
> > > > > figure out how do the patches compare.
> > > > This patch looks like it could be used to implement my use case.
> > > > However, parts of it are a bit vague and imprecise, so it's hard to
> > > > actually say whether my use case would actually be covered by a
> > > > specific implementation of this proposal.
> > > I am on vacation till this Friday. Shall we co-work on this and
> > > post something new together?
> > That works for me.
> Cool! How about two patches in a new series:
> 1) a general virtio suspending patch describing suspending behaviors for all
> virtio devices from me, an updating version of this patch
> 2) a PCI transport suspending patch from you, describing PCI specific
> behaviors.
>
> Dose this sound good to you?
Well David's patch was more precise than yours, and also better worded.
So think if there's an agreement you guys would really start with that,
move the functionality to the status bit and make other changes as needed.
> >
> > > > To give specifics on what I'm trying to do, I need to allow a guest
> > > > with virtio-pci devices (including stateful devices like virtio-fs and
> > > > virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
> > > > to wake up the guest (currently via an ACPI GPE, but eventually via a
> > > > native PCI PME). This is all done on a consumer device, so there is no
> > > > need for snapshotting or for live migration.
> > > Suspending is not dedicated only for live migration.
> > > For your use case, shall we add a new PCI section like saying: when entering
> > > SUSPEND, the device should get into S1/S3 and other PCI specific steps
> > > in PCI transport charter.
> > >
> > > That is because PCI is a transport layer of virtio, controlling virtio
> > > states by PCI sounds like a layer violation.
> > I don't know if it's really necessary to mandate that in the spec.
> > Sure, most systems are probably going to want to put the virtio-pci
> > device into S1/S3 after suspending virtio operation. But nothing
> > really breaks or goes wrong if we don't do that.
> Yes, maybe not S1/S3, I remember you want to manage power in PCI
> transport when suspend in a cap, S1/S3 is just an example.
>
> >
> > > > > > ---
> > > > > > content.tex | 34 ++++++++++++++++++++++++++++++++--
> > > > > > 1 file changed, 32 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/content.tex b/content.tex
> > > > > > index 0a62dce..3d656b5 100644
> > > > > > --- a/content.tex
> > > > > > +++ b/content.tex
> > > > > > @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
> > > > > >
> > > > > > \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
> > > > > > an error from which it can't recover.
> > > > > > +
> > > > > > +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
> > > > > > + device has been suspended by the driver.
> > > > > > \end{description}
> > > > > >
> > > > > > The \field{device status} field starts out as 0, and is reinitialized to 0 by
> > > > > > @@ -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 SHOULD NOT set SUSPEND if FEATURES_OK is not set.
> > > > > > +
> > > > > > +When setting 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,25 @@ \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.
> > > > > > +
> > > > > > +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
> > > > > > +
> > > > > > +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
> > > > > > +
> > > > > > +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
> > > > > > +and resumes operation upon DRIVER_OK.
> > > > > > +
> > > > > > +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
> > > > > > +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
> > > > > > +
> > > > > > +\begin{itemize}
> > > > > > +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
> > > > > > +\item Wait until all descriptors that being processed to finish and mark them as used.
> > > > > > +\item Flush all used buffer and send used buffer notifications to the driver.
> > > > What's the difference between the previous three lines? They might be
> > > > trying to say different things, but there is a lot of overlap in how
> > > > they're phrased. That makes it hard to figure out exactly what they're
> > > > mandating. Is it something like: "stop processing new buffers",
> > > > "finish processing any buffers that are currently in flight", "mark
> > > > all finished buffers as used and send a used buffer notification"?
> > > sure, the "mark used" parts can merge.
> > > > Also, what does this mean for devices where the driver places empty
> > > > buffers into the virtqueue that the device then holds on to for an
> > > > indeterminate period of time (e.g. network receiveq, balloon statsq,
> > > > etc)?
> > > The device doesn't process them.
> > > Oh, I should add: The driver should not make any more buffers available
> > > to the device.
> > > > > > +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> > > > What does "Pause its operation except device status" mean? The word
> > > > "operation" is vague, what exactly does it include? Also what does
> > > > "operate its device status" mean?
> > > Because we need to resume the device after suspending it by setting
> > > DRIVER_OK, so we need the device status
> > > filed keel alive.
> > This doesn't answer what "Pause its operation" means. If the
> > specification is not precise, then the best that can be implemented is
> > for a specific device to be compatible with a specific driver. If we
> > actually want portable, spec-compliant devices, we need explicitly
> > defined MUST/MUST NOT requirements. Presumably we want something like:
> >
> > A device MUST not send notifications while suspended.
> The device should send a last used buffer notification if
> it has buffers under processing before presenting SUSPEND
> in the device status, but the device should not process
> any new buffers. These are in devicenormative section,
> but they may be vague, I can add more details to make
> it precise, thanks for the tips
> > A device MUST ignore writes to its device configuration while
> > suspended, except for writes to the device status byte.
> > A driver MUST NOT write to a suspended device's configuration space,
> > except for the device status byte.
> sure, we can make it more precise than only pausing.
> >
> > We also need to specify what a suspended device is allowed to do with
> > buffers it owns. For my use case of just suspending/resuming a VM, I
> > don't think it particularly matters if a suspended device writes to
> > buffers or descriptors while it is suspended. But based on what you
> > wrote about finishing any in-flight processing of buffers and not
> > processing any empty buffers, presumably you would prefer that the
> > device not be allowed to access buffer/descriptors while it is
> > suspended? That approach is probably cleaner from a pure specification
> > standpoint, but it's also more restrictive for devices and thus harder
> > to properly implement.
> When the driver sets SUSPEND, the device should finished all buffers that
> already under processing, include the in-flight buffers(or it may lose the
> buffers).
> And the device should not touch the buffers once it present SUSPEND in the
> device status field, because once it presents SUSPEND, the device state
> should
> be stable, people may use SUSPEND to debug the device.
>
> The device still can process the buffer before it presenting SUSPEND.
>
> Thanks
> Zhu Lingshan
> >
> > -David
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 24+ messages in thread* [virtio-comment] Re: [PATCH] virtio: introduce SUSPEND bit in device status
2024-02-25 8:52 ` [virtio-dev] " Michael S. Tsirkin
@ 2024-02-26 1:36 ` Zhu, Lingshan
-1 siblings, 0 replies; 24+ messages in thread
From: Zhu, Lingshan @ 2024-02-26 1:36 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Stevens, jasowang, eperezma, cohuck, stefanha,
virtio-comment, virtio-dev
On 2/25/2024 4:52 PM, Michael S. Tsirkin wrote:
> On Fri, Feb 23, 2024 at 03:44:41PM +0800, Zhu, Lingshan wrote:
>>
>> On 2/20/2024 1:09 PM, David Stevens wrote:
>>> On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>> On 2/19/2024 2:46 PM, David Stevens wrote:
>>>>> On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
>>>>>>> This commit allows the driver to suspend the device by
>>>>>>> introducing a new status bit SUSPEND in device_status.
>>>>>>>
>>>>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
>>>>>>> which indicating whether the device support SUSPEND.
>>>>>>>
>>>>>>> This SUSPEND bit is transport-independent.
>>>>>>>
>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>> Could we get some kind of dscription how this has taken into
>>>>>> consideration the proposal from David Stevens?
>>>>>>
>>>>>> I find it really tiring when there are competing patches with authors
>>>>>> ignoring each other's work and leaving it up to reviewers to
>>>>>> figure out how do the patches compare.
>>>>> This patch looks like it could be used to implement my use case.
>>>>> However, parts of it are a bit vague and imprecise, so it's hard to
>>>>> actually say whether my use case would actually be covered by a
>>>>> specific implementation of this proposal.
>>>> I am on vacation till this Friday. Shall we co-work on this and
>>>> post something new together?
>>> That works for me.
>> Cool! How about two patches in a new series:
>> 1) a general virtio suspending patch describing suspending behaviors for all
>> virtio devices from me, an updating version of this patch
>> 2) a PCI transport suspending patch from you, describing PCI specific
>> behaviors.
>>
>> Dose this sound good to you?
> Well David's patch was more precise than yours, and also better worded.
> So think if there's an agreement you guys would really start with that,
> move the functionality to the status bit and make other changes as needed.
I think we plan to cooperate on a new series including both the status
bit and
PCI transport, collaborations on both of our patches.
Let's see whether this looks good to David
Thanks
>
>
>
>>>>> To give specifics on what I'm trying to do, I need to allow a guest
>>>>> with virtio-pci devices (including stateful devices like virtio-fs and
>>>>> virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
>>>>> to wake up the guest (currently via an ACPI GPE, but eventually via a
>>>>> native PCI PME). This is all done on a consumer device, so there is no
>>>>> need for snapshotting or for live migration.
>>>> Suspending is not dedicated only for live migration.
>>>> For your use case, shall we add a new PCI section like saying: when entering
>>>> SUSPEND, the device should get into S1/S3 and other PCI specific steps
>>>> in PCI transport charter.
>>>>
>>>> That is because PCI is a transport layer of virtio, controlling virtio
>>>> states by PCI sounds like a layer violation.
>>> I don't know if it's really necessary to mandate that in the spec.
>>> Sure, most systems are probably going to want to put the virtio-pci
>>> device into S1/S3 after suspending virtio operation. But nothing
>>> really breaks or goes wrong if we don't do that.
>> Yes, maybe not S1/S3, I remember you want to manage power in PCI
>> transport when suspend in a cap, S1/S3 is just an example.
>>
>>>>>>> ---
>>>>>>> content.tex | 34 ++++++++++++++++++++++++++++++++--
>>>>>>> 1 file changed, 32 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/content.tex b/content.tex
>>>>>>> index 0a62dce..3d656b5 100644
>>>>>>> --- a/content.tex
>>>>>>> +++ b/content.tex
>>>>>>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>>>
>>>>>>> \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>>>>> an error from which it can't recover.
>>>>>>> +
>>>>>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>>>>>> + device has been suspended by the driver.
>>>>>>> \end{description}
>>>>>>>
>>>>>>> The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>>>>>> @@ -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 SHOULD NOT set SUSPEND if FEATURES_OK is not set.
>>>>>>> +
>>>>>>> +When setting 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,25 @@ \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.
>>>>>>> +
>>>>>>> +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>>>>>>> +
>>>>>>> +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
>>>>>>> +
>>>>>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
>>>>>>> +and resumes operation upon DRIVER_OK.
>>>>>>> +
>>>>>>> +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
>>>>>>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
>>>>>>> +
>>>>>>> +\begin{itemize}
>>>>>>> +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
>>>>>>> +\item Wait until all descriptors that being processed to finish and mark them as used.
>>>>>>> +\item Flush all used buffer and send used buffer notifications to the driver.
>>>>> What's the difference between the previous three lines? They might be
>>>>> trying to say different things, but there is a lot of overlap in how
>>>>> they're phrased. That makes it hard to figure out exactly what they're
>>>>> mandating. Is it something like: "stop processing new buffers",
>>>>> "finish processing any buffers that are currently in flight", "mark
>>>>> all finished buffers as used and send a used buffer notification"?
>>>> sure, the "mark used" parts can merge.
>>>>> Also, what does this mean for devices where the driver places empty
>>>>> buffers into the virtqueue that the device then holds on to for an
>>>>> indeterminate period of time (e.g. network receiveq, balloon statsq,
>>>>> etc)?
>>>> The device doesn't process them.
>>>> Oh, I should add: The driver should not make any more buffers available
>>>> to the device.
>>>>>>> +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>>>>> What does "Pause its operation except device status" mean? The word
>>>>> "operation" is vague, what exactly does it include? Also what does
>>>>> "operate its device status" mean?
>>>> Because we need to resume the device after suspending it by setting
>>>> DRIVER_OK, so we need the device status
>>>> filed keel alive.
>>> This doesn't answer what "Pause its operation" means. If the
>>> specification is not precise, then the best that can be implemented is
>>> for a specific device to be compatible with a specific driver. If we
>>> actually want portable, spec-compliant devices, we need explicitly
>>> defined MUST/MUST NOT requirements. Presumably we want something like:
>>>
>>> A device MUST not send notifications while suspended.
>> The device should send a last used buffer notification if
>> it has buffers under processing before presenting SUSPEND
>> in the device status, but the device should not process
>> any new buffers. These are in devicenormative section,
>> but they may be vague, I can add more details to make
>> it precise, thanks for the tips
>>> A device MUST ignore writes to its device configuration while
>>> suspended, except for writes to the device status byte.
>>> A driver MUST NOT write to a suspended device's configuration space,
>>> except for the device status byte.
>> sure, we can make it more precise than only pausing.
>>> We also need to specify what a suspended device is allowed to do with
>>> buffers it owns. For my use case of just suspending/resuming a VM, I
>>> don't think it particularly matters if a suspended device writes to
>>> buffers or descriptors while it is suspended. But based on what you
>>> wrote about finishing any in-flight processing of buffers and not
>>> processing any empty buffers, presumably you would prefer that the
>>> device not be allowed to access buffer/descriptors while it is
>>> suspended? That approach is probably cleaner from a pure specification
>>> standpoint, but it's also more restrictive for devices and thus harder
>>> to properly implement.
>> When the driver sets SUSPEND, the device should finished all buffers that
>> already under processing, include the in-flight buffers(or it may lose the
>> buffers).
>> And the device should not touch the buffers once it present SUSPEND in the
>> device status field, because once it presents SUSPEND, the device state
>> should
>> be stable, people may use SUSPEND to debug the device.
>>
>> The device still can process the buffer before it presenting SUSPEND.
>>
>> Thanks
>> Zhu Lingshan
>>> -David
This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.
In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.
Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/
^ permalink raw reply [flat|nested] 24+ messages in thread* [virtio-dev] Re: [PATCH] virtio: introduce SUSPEND bit in device status
@ 2024-02-26 1:36 ` Zhu, Lingshan
0 siblings, 0 replies; 24+ messages in thread
From: Zhu, Lingshan @ 2024-02-26 1:36 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: David Stevens, jasowang, eperezma, cohuck, stefanha,
virtio-comment, virtio-dev
On 2/25/2024 4:52 PM, Michael S. Tsirkin wrote:
> On Fri, Feb 23, 2024 at 03:44:41PM +0800, Zhu, Lingshan wrote:
>>
>> On 2/20/2024 1:09 PM, David Stevens wrote:
>>> On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>> On 2/19/2024 2:46 PM, David Stevens wrote:
>>>>> On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
>>>>>>> This commit allows the driver to suspend the device by
>>>>>>> introducing a new status bit SUSPEND in device_status.
>>>>>>>
>>>>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
>>>>>>> which indicating whether the device support SUSPEND.
>>>>>>>
>>>>>>> This SUSPEND bit is transport-independent.
>>>>>>>
>>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>> Could we get some kind of dscription how this has taken into
>>>>>> consideration the proposal from David Stevens?
>>>>>>
>>>>>> I find it really tiring when there are competing patches with authors
>>>>>> ignoring each other's work and leaving it up to reviewers to
>>>>>> figure out how do the patches compare.
>>>>> This patch looks like it could be used to implement my use case.
>>>>> However, parts of it are a bit vague and imprecise, so it's hard to
>>>>> actually say whether my use case would actually be covered by a
>>>>> specific implementation of this proposal.
>>>> I am on vacation till this Friday. Shall we co-work on this and
>>>> post something new together?
>>> That works for me.
>> Cool! How about two patches in a new series:
>> 1) a general virtio suspending patch describing suspending behaviors for all
>> virtio devices from me, an updating version of this patch
>> 2) a PCI transport suspending patch from you, describing PCI specific
>> behaviors.
>>
>> Dose this sound good to you?
> Well David's patch was more precise than yours, and also better worded.
> So think if there's an agreement you guys would really start with that,
> move the functionality to the status bit and make other changes as needed.
I think we plan to cooperate on a new series including both the status
bit and
PCI transport, collaborations on both of our patches.
Let's see whether this looks good to David
Thanks
>
>
>
>>>>> To give specifics on what I'm trying to do, I need to allow a guest
>>>>> with virtio-pci devices (including stateful devices like virtio-fs and
>>>>> virtio-gpu) to be put into S1/S3, and to allow the virtio-pci devices
>>>>> to wake up the guest (currently via an ACPI GPE, but eventually via a
>>>>> native PCI PME). This is all done on a consumer device, so there is no
>>>>> need for snapshotting or for live migration.
>>>> Suspending is not dedicated only for live migration.
>>>> For your use case, shall we add a new PCI section like saying: when entering
>>>> SUSPEND, the device should get into S1/S3 and other PCI specific steps
>>>> in PCI transport charter.
>>>>
>>>> That is because PCI is a transport layer of virtio, controlling virtio
>>>> states by PCI sounds like a layer violation.
>>> I don't know if it's really necessary to mandate that in the spec.
>>> Sure, most systems are probably going to want to put the virtio-pci
>>> device into S1/S3 after suspending virtio operation. But nothing
>>> really breaks or goes wrong if we don't do that.
>> Yes, maybe not S1/S3, I remember you want to manage power in PCI
>> transport when suspend in a cap, S1/S3 is just an example.
>>
>>>>>>> ---
>>>>>>> content.tex | 34 ++++++++++++++++++++++++++++++++--
>>>>>>> 1 file changed, 32 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/content.tex b/content.tex
>>>>>>> index 0a62dce..3d656b5 100644
>>>>>>> --- a/content.tex
>>>>>>> +++ b/content.tex
>>>>>>> @@ -49,6 +49,9 @@ \section{\field{Device Status} Field}\label{sec:Basic Facilities of a Virtio Dev
>>>>>>>
>>>>>>> \item[DEVICE_NEEDS_RESET (64)] Indicates that the device has experienced
>>>>>>> an error from which it can't recover.
>>>>>>> +
>>>>>>> +\item[SUSPEND (16)] When VIRTIO_F_SUSPEND is negotiated, indicates that the
>>>>>>> + device has been suspended by the driver.
>>>>>>> \end{description}
>>>>>>>
>>>>>>> The \field{device status} field starts out as 0, and is reinitialized to 0 by
>>>>>>> @@ -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 SHOULD NOT set SUSPEND if FEATURES_OK is not set.
>>>>>>> +
>>>>>>> +When setting 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,25 @@ \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.
>>>>>>> +
>>>>>>> +The device MUST ignore SUSPEND if VIRTIO_F_SUSPEND is not negotiated.
>>>>>>> +
>>>>>>> +The device SHOULD allow settings to \field{device status} even when SUSPEND is set.
>>>>>>> +
>>>>>>> +If VIRTIO_F_SUSPEND is negotiated and SUSPEND is set, the device SHOULD clear SUSPEND
>>>>>>> +and resumes operation upon DRIVER_OK.
>>>>>>> +
>>>>>>> +If VIRTIO_F_SUSPEND is negotiated, when the driver sets SUSPEND,
>>>>>>> +the device SHOULD perform the following actions before presenting SUSPEND bit in the \field{device status}:
>>>>>>> +
>>>>>>> +\begin{itemize}
>>>>>>> +\item Stop consuming buffers of any virtqueues and mark all finished descriptors as used.
>>>>>>> +\item Wait until all descriptors that being processed to finish and mark them as used.
>>>>>>> +\item Flush all used buffer and send used buffer notifications to the driver.
>>>>> What's the difference between the previous three lines? They might be
>>>>> trying to say different things, but there is a lot of overlap in how
>>>>> they're phrased. That makes it hard to figure out exactly what they're
>>>>> mandating. Is it something like: "stop processing new buffers",
>>>>> "finish processing any buffers that are currently in flight", "mark
>>>>> all finished buffers as used and send a used buffer notification"?
>>>> sure, the "mark used" parts can merge.
>>>>> Also, what does this mean for devices where the driver places empty
>>>>> buffers into the virtqueue that the device then holds on to for an
>>>>> indeterminate period of time (e.g. network receiveq, balloon statsq,
>>>>> etc)?
>>>> The device doesn't process them.
>>>> Oh, I should add: The driver should not make any more buffers available
>>>> to the device.
>>>>>>> +\item Pause its operation except \field{device status} and preserve configurations in its Device Configuration Space, see \ref{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>>>>> What does "Pause its operation except device status" mean? The word
>>>>> "operation" is vague, what exactly does it include? Also what does
>>>>> "operate its device status" mean?
>>>> Because we need to resume the device after suspending it by setting
>>>> DRIVER_OK, so we need the device status
>>>> filed keel alive.
>>> This doesn't answer what "Pause its operation" means. If the
>>> specification is not precise, then the best that can be implemented is
>>> for a specific device to be compatible with a specific driver. If we
>>> actually want portable, spec-compliant devices, we need explicitly
>>> defined MUST/MUST NOT requirements. Presumably we want something like:
>>>
>>> A device MUST not send notifications while suspended.
>> The device should send a last used buffer notification if
>> it has buffers under processing before presenting SUSPEND
>> in the device status, but the device should not process
>> any new buffers. These are in devicenormative section,
>> but they may be vague, I can add more details to make
>> it precise, thanks for the tips
>>> A device MUST ignore writes to its device configuration while
>>> suspended, except for writes to the device status byte.
>>> A driver MUST NOT write to a suspended device's configuration space,
>>> except for the device status byte.
>> sure, we can make it more precise than only pausing.
>>> We also need to specify what a suspended device is allowed to do with
>>> buffers it owns. For my use case of just suspending/resuming a VM, I
>>> don't think it particularly matters if a suspended device writes to
>>> buffers or descriptors while it is suspended. But based on what you
>>> wrote about finishing any in-flight processing of buffers and not
>>> processing any empty buffers, presumably you would prefer that the
>>> device not be allowed to access buffer/descriptors while it is
>>> suspended? That approach is probably cleaner from a pure specification
>>> standpoint, but it's also more restrictive for devices and thus harder
>>> to properly implement.
>> When the driver sets SUSPEND, the device should finished all buffers that
>> already under processing, include the in-flight buffers(or it may lose the
>> buffers).
>> And the device should not touch the buffers once it present SUSPEND in the
>> device status field, because once it presents SUSPEND, the device state
>> should
>> be stable, people may use SUSPEND to debug the device.
>>
>> The device still can process the buffer before it presenting SUSPEND.
>>
>> Thanks
>> Zhu Lingshan
>>> -David
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 24+ messages in thread* [virtio-comment] Re: [PATCH] virtio: introduce SUSPEND bit in device status
2024-02-26 1:36 ` [virtio-dev] " Zhu, Lingshan
@ 2024-02-26 2:43 ` David Stevens
-1 siblings, 0 replies; 24+ messages in thread
From: David Stevens @ 2024-02-26 2:43 UTC (permalink / raw)
To: Zhu, Lingshan
Cc: Michael S. Tsirkin, jasowang, eperezma, cohuck, stefanha,
virtio-comment, virtio-dev
On Mon, Feb 26, 2024 at 10:36 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> On 2/25/2024 4:52 PM, Michael S. Tsirkin wrote:
> > On Fri, Feb 23, 2024 at 03:44:41PM +0800, Zhu, Lingshan wrote:
> >>
> >> On 2/20/2024 1:09 PM, David Stevens wrote:
> >>> On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>>> On 2/19/2024 2:46 PM, David Stevens wrote:
> >>>>> On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
> >>>>>>> This commit allows the driver to suspend the device by
> >>>>>>> introducing a new status bit SUSPEND in device_status.
> >>>>>>>
> >>>>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
> >>>>>>> which indicating whether the device support SUSPEND.
> >>>>>>>
> >>>>>>> This SUSPEND bit is transport-independent.
> >>>>>>>
> >>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>>>> Could we get some kind of dscription how this has taken into
> >>>>>> consideration the proposal from David Stevens?
> >>>>>>
> >>>>>> I find it really tiring when there are competing patches with authors
> >>>>>> ignoring each other's work and leaving it up to reviewers to
> >>>>>> figure out how do the patches compare.
> >>>>> This patch looks like it could be used to implement my use case.
> >>>>> However, parts of it are a bit vague and imprecise, so it's hard to
> >>>>> actually say whether my use case would actually be covered by a
> >>>>> specific implementation of this proposal.
> >>>> I am on vacation till this Friday. Shall we co-work on this and
> >>>> post something new together?
> >>> That works for me.
> >> Cool! How about two patches in a new series:
> >> 1) a general virtio suspending patch describing suspending behaviors for all
> >> virtio devices from me, an updating version of this patch
> >> 2) a PCI transport suspending patch from you, describing PCI specific
> >> behaviors.
> >>
> >> Dose this sound good to you?
> > Well David's patch was more precise than yours, and also better worded.
> > So think if there's an agreement you guys would really start with that,
> > move the functionality to the status bit and make other changes as needed.
> I think we plan to cooperate on a new series including both the status
> bit and
> PCI transport, collaborations on both of our patches.
>
> Let's see whether this looks good to David
That works for me. I can incorporate the feedback on both proposals
and send out a combined version in the next day or so.
Thanks,
David
This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.
In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.
Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/
^ permalink raw reply [flat|nested] 24+ messages in thread
* [virtio-dev] Re: [PATCH] virtio: introduce SUSPEND bit in device status
@ 2024-02-26 2:43 ` David Stevens
0 siblings, 0 replies; 24+ messages in thread
From: David Stevens @ 2024-02-26 2:43 UTC (permalink / raw)
To: Zhu, Lingshan
Cc: Michael S. Tsirkin, jasowang, eperezma, cohuck, stefanha,
virtio-comment, virtio-dev
On Mon, Feb 26, 2024 at 10:36 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> On 2/25/2024 4:52 PM, Michael S. Tsirkin wrote:
> > On Fri, Feb 23, 2024 at 03:44:41PM +0800, Zhu, Lingshan wrote:
> >>
> >> On 2/20/2024 1:09 PM, David Stevens wrote:
> >>> On Tue, Feb 20, 2024 at 1:06 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>>> On 2/19/2024 2:46 PM, David Stevens wrote:
> >>>>> On Sun, Feb 18, 2024 at 11:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>> On Sun, Feb 18, 2024 at 09:23:06PM +0800, Zhu Lingshan wrote:
> >>>>>>> This commit allows the driver to suspend the device by
> >>>>>>> introducing a new status bit SUSPEND in device_status.
> >>>>>>>
> >>>>>>> This commit also introduce a new feature bit VIRTIO_F_SUSPEND
> >>>>>>> which indicating whether the device support SUSPEND.
> >>>>>>>
> >>>>>>> This SUSPEND bit is transport-independent.
> >>>>>>>
> >>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>>>> Could we get some kind of dscription how this has taken into
> >>>>>> consideration the proposal from David Stevens?
> >>>>>>
> >>>>>> I find it really tiring when there are competing patches with authors
> >>>>>> ignoring each other's work and leaving it up to reviewers to
> >>>>>> figure out how do the patches compare.
> >>>>> This patch looks like it could be used to implement my use case.
> >>>>> However, parts of it are a bit vague and imprecise, so it's hard to
> >>>>> actually say whether my use case would actually be covered by a
> >>>>> specific implementation of this proposal.
> >>>> I am on vacation till this Friday. Shall we co-work on this and
> >>>> post something new together?
> >>> That works for me.
> >> Cool! How about two patches in a new series:
> >> 1) a general virtio suspending patch describing suspending behaviors for all
> >> virtio devices from me, an updating version of this patch
> >> 2) a PCI transport suspending patch from you, describing PCI specific
> >> behaviors.
> >>
> >> Dose this sound good to you?
> > Well David's patch was more precise than yours, and also better worded.
> > So think if there's an agreement you guys would really start with that,
> > move the functionality to the status bit and make other changes as needed.
> I think we plan to cooperate on a new series including both the status
> bit and
> PCI transport, collaborations on both of our patches.
>
> Let's see whether this looks good to David
That works for me. I can incorporate the feedback on both proposals
and send out a combined version in the next day or so.
Thanks,
David
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 24+ messages in thread