[virtio-comment] [PATCH v3] clarify device reset

[virtio-comment] [PATCH v3] clarify device reset

[Cornelia Huck]

Properly specify that the method for the driver to request a
device reset is transport specific, and some action the device
has to take.

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---

RFC v2 -> v3:
  - re-worded the "must not send notifications" clause to avoid guessing
  - added a driver conformance clause on how a driver should find out
    when reset is complete
RFC -> RFC v2:
  - moved reset spec to basic facilities

---
 conformance.tex |  2 ++
 content.tex     | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/conformance.tex b/conformance.tex
index eb3324053080..21fe89ccd937 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -60,6 +60,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \begin{itemize}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Status Field}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}
+\item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Reset}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Configuration Space}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
 \item \ref{drivernormative:Basic Facilities of a Virtio Device / Message Framing}
@@ -271,6 +272,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \begin{itemize}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Status Field}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits}
+\item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Reset}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Configuration Space}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Message Framing}
 \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
diff --git a/content.tex b/content.tex
index 620c0e28c9a7..9cdefe16509e 100644
--- a/content.tex
+++ b/content.tex
@@ -193,6 +193,25 @@ \section{Notifications}\label{sec:Basic Facilities of a Virtio Device
 terminology. Occasionally, the term event is used to refer to
 a notification or a receipt of a notification.
 
+\section{Device Reset}\label{sec:Basic Facilities of a Virtio Device / Device Reset}
+
+The driver may initiate a device reset at various times; notably, during
+device initialization and device cleanup.
+
+The mechanism used by the driver to initiate the reset is transport specific.
+
+\devicenormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
+
+A device MUST reinitialize device status to 0 after receiving a reset.
+
+A device MUST NOT send notifications after indicating completion of
+the reset by reinitializing the device status to 0.
+
+\drivernormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
+
+The driver SHOULD consider a driver-initiated reset complete when it
+reads the device status as 0.
+
 \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
 
 Device configuration space is generally used for rarely-changing or
-- 
2.26.2


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/

Re: [virtio-comment] [PATCH v3] clarify device reset

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]

On Mon, Jan 25, 2021 at 12:08:23PM +0100, Cornelia Huck wrote:
> Properly specify that the method for the driver to request a
> device reset is transport specific, and some action the device
> has to take.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> RFC v2 -> v3:
>   - re-worded the "must not send notifications" clause to avoid guessing
>   - added a driver conformance clause on how a driver should find out
>     when reset is complete
> RFC -> RFC v2:
>   - moved reset spec to basic facilities
> 
> ---
>  conformance.tex |  2 ++
>  content.tex     | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [virtio-comment] [PATCH v3] clarify device reset

[Jason Wang]

On 2021/1/25 下午7:08, Cornelia Huck wrote:
> Properly specify that the method for the driver to request a
> device reset is transport specific, and some action the device
> has to take.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>
> RFC v2 -> v3:
>    - re-worded the "must not send notifications" clause to avoid guessing
>    - added a driver conformance clause on how a driver should find out
>      when reset is complete
> RFC -> RFC v2:
>    - moved reset spec to basic facilities
>
> ---
>   conformance.tex |  2 ++
>   content.tex     | 19 +++++++++++++++++++
>   2 files changed, 21 insertions(+)
>
> diff --git a/conformance.tex b/conformance.tex
> index eb3324053080..21fe89ccd937 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -60,6 +60,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>   \begin{itemize}
>   \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Status Field}
>   \item \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}
> +\item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Reset}
>   \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Configuration Space}
>   \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
>   \item \ref{drivernormative:Basic Facilities of a Virtio Device / Message Framing}
> @@ -271,6 +272,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>   \begin{itemize}
>   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Status Field}
>   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits}
> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Reset}
>   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Configuration Space}
>   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Message Framing}
>   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
> diff --git a/content.tex b/content.tex
> index 620c0e28c9a7..9cdefe16509e 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -193,6 +193,25 @@ \section{Notifications}\label{sec:Basic Facilities of a Virtio Device
>   terminology. Occasionally, the term event is used to refer to
>   a notification or a receipt of a notification.
>   
> +\section{Device Reset}\label{sec:Basic Facilities of a Virtio Device / Device Reset}
> +
> +The driver may initiate a device reset at various times; notably, during
> +device initialization and device cleanup.
> +
> +The mechanism used by the driver to initiate the reset is transport specific.
> +
> +\devicenormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
> +
> +A device MUST reinitialize device status to 0 after receiving a reset.
> +


We had similar description in "2.1.2 Device Requirements: Device Status 
Field":

"The device MUST initialize device status to 0 upon reset."

Consider we had a dedicated part for reset, maybe we can remove that one.

Thanks


> +A device MUST NOT send notifications after indicating completion of
> +the reset by reinitializing the device status to 0.
> +
> +\drivernormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
> +
> +The driver SHOULD consider a driver-initiated reset complete when it
> +reads the device status as 0.
> +
>   \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>   
>   Device configuration space is generally used for rarely-changing or


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/

[virtio-comment] Re: [PATCH v3] clarify device reset

[Halil Pasic]

On Mon, 25 Jan 2021 12:08:23 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> Properly specify that the method for the driver to request a
> device reset is transport specific, and some action the device
> has to take.
> 
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> 
> RFC v2 -> v3:
>   - re-worded the "must not send notifications" clause to avoid guessing
>   - added a driver conformance clause on how a driver should find out
>     when reset is complete
> RFC -> RFC v2:
>   - moved reset spec to basic facilities
> 
> ---
>  conformance.tex |  2 ++
>  content.tex     | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/conformance.tex b/conformance.tex
> index eb3324053080..21fe89ccd937 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -60,6 +60,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \begin{itemize}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Status Field}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}
> +\item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Reset}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Configuration Space}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
>  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Message Framing}
> @@ -271,6 +272,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \begin{itemize}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Status Field}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits}
> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Reset}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Configuration Space}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Message Framing}
>  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
> diff --git a/content.tex b/content.tex
> index 620c0e28c9a7..9cdefe16509e 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -193,6 +193,25 @@ \section{Notifications}\label{sec:Basic Facilities of a Virtio Device
>  terminology. Occasionally, the term event is used to refer to
>  a notification or a receipt of a notification.
>  
> +\section{Device Reset}\label{sec:Basic Facilities of a Virtio Device / Device Reset}
> +
> +The driver may initiate a device reset at various times; notably, during
> +device initialization and device cleanup.

Are there times the driver may not initiate a device reset? What are we
trying to tell with this sentence?

If we are just looking for an introductionary sentence, I would probably
go with something like "A device reset be either initiated by the
driver, or by a system reset, or other external controls."

> +
> +The mechanism used by the driver to initiate the reset is transport specific.
> +
> +\devicenormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
> +
> +A device MUST reinitialize device status to 0 after receiving a reset.
> +

What is the intent behind this sentence?

One way I can read this it, that the device reset is not allowed to
fail. I.e. device must reinitialize status to after receiving a reset
and status 0 indicating the reset is done implies, that a reset must
succeed eventually (provided the device did receive).

The other way i can read it is, what I would rather have worded as:
"The device MUST indicate the completion of the reset by reinitializing
the device status to 0".


> +A device MUST NOT send notifications after indicating completion of
> +the reset by reinitializing the device status to 0.

I've just realized, that 'after' is very open to the right. Of course
the device may send notifications again after the driver re-initialized
the device as per 3.1.1.

Maybe it is wiser to tie this restriction to the current status value
(without any after or before), and do it where the notifications are
described. 

Also a device that has status 0 may not poke the virtqueues. If we
are explicit about the notifications, we should be explicit about
the virtqueues as well.

> +
> +\drivernormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
> +
> +The driver SHOULD consider a driver-initiated reset complete when it
> +reads the device status as 0.
> +
>  \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>  
>  Device configuration space is generally used for rarely-changing or


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/

Re: [virtio-comment] [PATCH v3] clarify device reset

[Cornelia Huck]

On Wed, 27 Jan 2021 11:07:53 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2021/1/25 下午7:08, Cornelia Huck wrote:
> > Properly specify that the method for the driver to request a
> > device reset is transport specific, and some action the device
> > has to take.
> >
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> >
> > RFC v2 -> v3:
> >    - re-worded the "must not send notifications" clause to avoid guessing
> >    - added a driver conformance clause on how a driver should find out
> >      when reset is complete
> > RFC -> RFC v2:
> >    - moved reset spec to basic facilities
> >
> > ---
> >   conformance.tex |  2 ++
> >   content.tex     | 19 +++++++++++++++++++
> >   2 files changed, 21 insertions(+)
> >
> > diff --git a/conformance.tex b/conformance.tex
> > index eb3324053080..21fe89ccd937 100644
> > --- a/conformance.tex
> > +++ b/conformance.tex
> > @@ -60,6 +60,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> >   \begin{itemize}
> >   \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Status Field}
> >   \item \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}
> > +\item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Reset}
> >   \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Configuration Space}
> >   \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
> >   \item \ref{drivernormative:Basic Facilities of a Virtio Device / Message Framing}
> > @@ -271,6 +272,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> >   \begin{itemize}
> >   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Status Field}
> >   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits}
> > +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Reset}
> >   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Configuration Space}
> >   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Message Framing}
> >   \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
> > diff --git a/content.tex b/content.tex
> > index 620c0e28c9a7..9cdefe16509e 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -193,6 +193,25 @@ \section{Notifications}\label{sec:Basic Facilities of a Virtio Device
> >   terminology. Occasionally, the term event is used to refer to
> >   a notification or a receipt of a notification.
> >   
> > +\section{Device Reset}\label{sec:Basic Facilities of a Virtio Device / Device Reset}
> > +
> > +The driver may initiate a device reset at various times; notably, during
> > +device initialization and device cleanup.
> > +
> > +The mechanism used by the driver to initiate the reset is transport specific.
> > +
> > +\devicenormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
> > +
> > +A device MUST reinitialize device status to 0 after receiving a reset.
> > +  
> 
> 
> We had similar description in "2.1.2 Device Requirements: Device Status 
> Field":
> 
> "The device MUST initialize device status to 0 upon reset."
> 
> Consider we had a dedicated part for reset, maybe we can remove that one.

Maybe we can replace the normative statement with an informal
description in the device status section? I.e. something like

"The device status field starts out as 0, and is reinitialized to 0 by
the device during reset."

> 
> Thanks
> 
> 
> > +A device MUST NOT send notifications after indicating completion of
> > +the reset by reinitializing the device status to 0.
> > +
> > +\drivernormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
> > +
> > +The driver SHOULD consider a driver-initiated reset complete when it
> > +reads the device status as 0.
> > +
> >   \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> >   
> >   Device configuration space is generally used for rarely-changing or  


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/

[virtio-comment] Re: [PATCH v3] clarify device reset

[Cornelia Huck]

On Wed, 27 Jan 2021 10:19:04 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Mon, 25 Jan 2021 12:08:23 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > Properly specify that the method for the driver to request a
> > device reset is transport specific, and some action the device
> > has to take.
> > 
> > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > ---
> > 
> > RFC v2 -> v3:
> >   - re-worded the "must not send notifications" clause to avoid guessing
> >   - added a driver conformance clause on how a driver should find out
> >     when reset is complete
> > RFC -> RFC v2:
> >   - moved reset spec to basic facilities
> > 
> > ---
> >  conformance.tex |  2 ++
> >  content.tex     | 19 +++++++++++++++++++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/conformance.tex b/conformance.tex
> > index eb3324053080..21fe89ccd937 100644
> > --- a/conformance.tex
> > +++ b/conformance.tex
> > @@ -60,6 +60,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> >  \begin{itemize}
> >  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Status Field}
> >  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}
> > +\item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Reset}
> >  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Configuration Space}
> >  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
> >  \item \ref{drivernormative:Basic Facilities of a Virtio Device / Message Framing}
> > @@ -271,6 +272,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> >  \begin{itemize}
> >  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Status Field}
> >  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits}
> > +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Reset}
> >  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Configuration Space}
> >  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Message Framing}
> >  \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
> > diff --git a/content.tex b/content.tex
> > index 620c0e28c9a7..9cdefe16509e 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -193,6 +193,25 @@ \section{Notifications}\label{sec:Basic Facilities of a Virtio Device
> >  terminology. Occasionally, the term event is used to refer to
> >  a notification or a receipt of a notification.
> >  
> > +\section{Device Reset}\label{sec:Basic Facilities of a Virtio Device / Device Reset}
> > +
> > +The driver may initiate a device reset at various times; notably, during
> > +device initialization and device cleanup.  
> 
> Are there times the driver may not initiate a device reset? What are we
> trying to tell with this sentence?

s/may initiate/may want to initiate/

> 
> If we are just looking for an introductionary sentence, I would probably
> go with something like "A device reset be either initiated by the
> driver, or by a system reset, or other external controls."

It was mostly supposed to be a lead-in.

I'm not sure we want to spell out things like system resets. They are
platform-specific, and virtio devices are basically just a device
there. Of course, the actual implementation will share backend code to
reinitialize to a clean state for driver-initiated resets, system
resets, and whatever else there might be, but I consider that an
implementation detail and not something that needs to be specified.

We do want to spell out, however, what a driver can expect and a device
needs to do for a driver-initiated reset, and that's what this update
aims to do.

> 
> > +
> > +The mechanism used by the driver to initiate the reset is transport specific.
> > +
> > +\devicenormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
> > +
> > +A device MUST reinitialize device status to 0 after receiving a reset.
> > +  
> 
> What is the intent behind this sentence?
> 
> One way I can read this it, that the device reset is not allowed to
> fail. I.e. device must reinitialize status to after receiving a reset
> and status 0 indicating the reset is done implies, that a reset must
> succeed eventually (provided the device did receive).

Yes, I don't think it makes sense to reject a reset.

> 
> The other way i can read it is, what I would rather have worded as:
> "The device MUST indicate the completion of the reset by reinitializing
> the device status to 0".

That's also true, but I think it is already covered?

> 
> 
> > +A device MUST NOT send notifications after indicating completion of
> > +the reset by reinitializing the device status to 0.  
> 
> I've just realized, that 'after' is very open to the right. Of course
> the device may send notifications again after the driver re-initialized
> the device as per 3.1.1.

Append "until the driver re-initializes the device."?

> 
> Maybe it is wiser to tie this restriction to the current status value
> (without any after or before), and do it where the notifications are
> described. 
> 
> Also a device that has status 0 may not poke the virtqueues. If we
> are explicit about the notifications, we should be explicit about
> the virtqueues as well.

What about

"A device MUST NOT send notifications or process queue buffers after
indication completion of the reset by reinitializing the device status
to 0, until the driver re-initializes the device."

?

> 
> > +
> > +\drivernormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
> > +
> > +The driver SHOULD consider a driver-initiated reset complete when it
> > +reads the device status as 0.
> > +
> >  \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
> >  
> >  Device configuration space is generally used for rarely-changing or  
> 


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/

[virtio-comment] Re: [PATCH v3] clarify device reset

[Halil Pasic]

On Wed, 27 Jan 2021 12:44:03 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 27 Jan 2021 10:19:04 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Mon, 25 Jan 2021 12:08:23 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > Properly specify that the method for the driver to request a
> > > device reset is transport specific, and some action the device
> > > has to take.
> > > 
> > > Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> > > ---
> > > 
> > > RFC v2 -> v3:
> > >   - re-worded the "must not send notifications" clause to avoid
> > > guessing
> > >   - added a driver conformance clause on how a driver should find
> > > out when reset is complete
> > > RFC -> RFC v2:
> > >   - moved reset spec to basic facilities
> > > 
> > > ---
> > >  conformance.tex |  2 ++
> > >  content.tex     | 19 +++++++++++++++++++
> > >  2 files changed, 21 insertions(+)
> > > 
> > > diff --git a/conformance.tex b/conformance.tex
> > > index eb3324053080..21fe89ccd937 100644
> > > --- a/conformance.tex
> > > +++ b/conformance.tex
> > > @@ -60,6 +60,7 @@ \section{Conformance
> > > Targets}\label{sec:Conformance / Conformance Targets}
> > > \begin{itemize} \item \ref{drivernormative:Basic Facilities of a
> > > Virtio Device / Device Status Field} \item
> > > \ref{drivernormative:Basic Facilities of a Virtio Device / Feature
> > > Bits} +\item \ref{drivernormative:Basic Facilities of a Virtio
> > > Device / Device Reset} \item \ref{drivernormative:Basic Facilities
> > > of a Virtio Device / Device Configuration Space} \item
> > > \ref{drivernormative:Basic Facilities of a Virtio Device /
> > > Virtqueues} \item \ref{drivernormative:Basic Facilities of a
> > > Virtio Device / Message Framing} @@ -271,6 +272,7 @@
> > > \section{Conformance Targets}\label{sec:Conformance / Conformance
> > > Targets} \begin{itemize} \item \ref{devicenormative:Basic
> > > Facilities of a Virtio Device / Device Status Field} \item
> > > \ref{devicenormative:Basic Facilities of a Virtio Device / Feature
> > > Bits} +\item \ref{devicenormative:Basic Facilities of a Virtio
> > > Device / Device Reset} \item \ref{devicenormative:Basic Facilities
> > > of a Virtio Device / Device Configuration Space} \item
> > > \ref{devicenormative:Basic Facilities of a Virtio Device / Message
> > > Framing} \item \ref{devicenormative:Basic Facilities of a Virtio
> > > Device / Virtqueues / The Virtqueue Descriptor Table} diff --git
> > > a/content.tex b/content.tex index 620c0e28c9a7..9cdefe16509e
> > > 100644 --- a/content.tex +++ b/content.tex @@ -193,6 +193,25 @@
> > > \section{Notifications}\label{sec:Basic Facilities of a Virtio
> > > Device terminology. Occasionally, the term event is used to refer
> > > to a notification or a receipt of a notification. +\section{Device
> > > Reset}\label{sec:Basic Facilities of a Virtio Device / Device
> > > Reset} + +The driver may initiate a device reset at various times;
> > > notably, during +device initialization and device cleanup.    
> > 
> > Are there times the driver may not initiate a device reset? What are
> > we trying to tell with this sentence?  
> 
> s/may initiate/may want to initiate/

Definitively better. I would still prefer avoiding the discussion
on when the driver may want to reset the device. But I can live
with this.

> 
> > 
> > If we are just looking for an introductionary sentence, I would
> > probably go with something like "A device reset be either initiated
> > by the

s/be/can be/

> > driver, or by a system reset, or other external controls."  
> 
> It was mostly supposed to be a lead-in.
> 
> I'm not sure we want to spell out things like system resets. They are
> platform-specific, and virtio devices are basically just a device
> there.

Hm. AFAIR the only way to make the device non-live, i.e. illegal for it 
to e.g. access a queue, which is in guest memory is device reset. Now if
I want to perform a reboot, I do want to stop the devices from poking
such memory. But I think, in linux we don't reset the device form the
driver, before reboot, and I believe we don't have to because we are
guaranteed that the device will get reset. Or is my memory failing me?

I think it is of value to clarify that a virtio reset can possibly be
initiated by other means than explicitly specified by the virtio spec,
and that when a 'proxy-device' is reset, the 'virtio-device' is to be
reset as well. We do mention system reset once, in a normative section
at that (2.2.2).

> Of course, the actual implementation will share backend code to
> reinitialize to a clean state for driver-initiated resets, system
> resets, and whatever else there might be, but I consider that an
> implementation detail and not something that needs to be specified.
> 
> We do want to spell out, however, what a driver can expect and a device
> needs to do for a driver-initiated reset, and that's what this update
> aims to do.
> 
> >   
> > > +
> > > +The mechanism used by the driver to initiate the reset is
> > > transport specific. +
> > > +\devicenormative{\subsection}{Device Reset}{Basic Facilities of a
> > > Virtio Device / Device Reset} +
> > > +A device MUST reinitialize device status to 0 after receiving a
> > > reset.
> > > +    
> > 
> > What is the intent behind this sentence?
> > 
> > One way I can read this it, that the device reset is not allowed to
> > fail. I.e. device must reinitialize status to after receiving a reset
> > and status 0 indicating the reset is done implies, that a reset must
> > succeed eventually (provided the device did receive).  
> 
> Yes, I don't think it makes sense to reject a reset.
> 

I don't know. And if that is what we want to say, I would certainly
prefer a SHOULD over a must. E.g. what if the device looses power
between receiving the reset and being able to finish it. Also if
we want to express that a reset should not fail we should say that
in a more straight forward way.

How do we expect the device and the driver to deal with a device
error that ain't recoverable even by a device reset? AFAIR all
the device can do is set DEVICE_NEEDS_RESET to indicate "that the device
has experienced an error from which it can’t recover". On the other
hand, I think the most reasonable way for a driver to meet
DEVICE_NEEDS_RESET
is to reset it and make an attempt to re-initialize it...

> > 
> > The other way i can read it is, what I would rather have worded as:
> > "The device MUST indicate the completion of the reset by
> > reinitializing the device status to 0".  
> 
> That's also true, but I think it is already covered?
> 

It is IMHO implicitly covered by the driver normative.

> > 
> >   
> > > +A device MUST NOT send notifications after indicating completion
> > > of +the reset by reinitializing the device status to 0.    
> > 
> > I've just realized, that 'after' is very open to the right. Of course
> > the device may send notifications again after the driver
> > re-initialized the device as per 3.1.1.  
> 
> Append "until the driver re-initializes the device."?
> 

That is another solution.

> > 
> > Maybe it is wiser to tie this restriction to the current status value
> > (without any after or before), and do it where the notifications are
> > described. 
> > 
> > Also a device that has status 0 may not poke the virtqueues. If we
> > are explicit about the notifications, we should be explicit about
> > the virtqueues as well.  
> 
> What about
> 
> "A device MUST NOT send notifications or process queue buffers after
> indication completion of the reset by reinitializing the device status
> to 0, until the driver re-initializes the device."
> 
> ?

Getting better, but IMHO not perfect. Process queue buffers is a bit
vague IMHO. Even examining (reading) the ring(s) is taboo in my opinion,
and I'm not sure that is unambiguously covered by 'process queue
buffers'. In my opinion after a successful reset, the driver is
entitled to even hot-unplug the ram that used to host the queues,
notifier bits, etc.

All this said, I believe the v3 is already a significant improvement,
and the virtio spec ain't the most formal and painfully precise
specification anyway. So if you don't feel like polishing this even
further.

Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

Regards,
Halil

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/

[virtio-comment] Re: [PATCH v3] clarify device reset

[Cornelia Huck]

On Wed, 27 Jan 2021 14:48:57 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 27 Jan 2021 12:44:03 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Wed, 27 Jan 2021 10:19:04 +0100
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> > > On Mon, 25 Jan 2021 12:08:23 +0100
> > > Cornelia Huck <cohuck@redhat.com> wrote:

> > > > +\section{Device
> > > > Reset}\label{sec:Basic Facilities of a Virtio Device / Device
> > > > Reset} + +The driver may initiate a device reset at various times;
> > > > notably, during +device initialization and device cleanup.      
> > > 
> > > Are there times the driver may not initiate a device reset? What are
> > > we trying to tell with this sentence?    
> > 
> > s/may initiate/may want to initiate/  
> 
> Definitively better. I would still prefer avoiding the discussion
> on when the driver may want to reset the device. But I can live
> with this.

The driver is free to reset the device whenever it wants to, even if it
does not make sense. Device initialization and cleanup are examples for
when it definitely will do so.

> 
> >   
> > > 
> > > If we are just looking for an introductionary sentence, I would
> > > probably go with something like "A device reset be either initiated
> > > by the  
> 
> s/be/can be/
> 
> > > driver, or by a system reset, or other external controls."    
> > 
> > It was mostly supposed to be a lead-in.
> > 
> > I'm not sure we want to spell out things like system resets. They are
> > platform-specific, and virtio devices are basically just a device
> > there.  
> 
> Hm. AFAIR the only way to make the device non-live, i.e. illegal for it 
> to e.g. access a queue, which is in guest memory is device reset. Now if
> I want to perform a reboot, I do want to stop the devices from poking
> such memory. But I think, in linux we don't reset the device form the
> driver, before reboot, and I believe we don't have to because we are
> guaranteed that the device will get reset. Or is my memory failing me?
> 
> I think it is of value to clarify that a virtio reset can possibly be
> initiated by other means than explicitly specified by the virtio spec,
> and that when a 'proxy-device' is reset, the 'virtio-device' is to be
> reset as well. We do mention system reset once, in a normative section
> at that (2.2.2).

That section looks a bit questionable (what is 'resuming from suspend'?).

Platforms may have different definitions of what a system reset does
(and even different types of reset). I think it is reasonable to assume
that any platform/system reset that brings devices into some kind of
initial state also brings virtio devices into that initial state (and
especially that bringing the proxy device into an initial state also
covers the virtio-specific parts.)

> 
> > Of course, the actual implementation will share backend code to
> > reinitialize to a clean state for driver-initiated resets, system
> > resets, and whatever else there might be, but I consider that an
> > implementation detail and not something that needs to be specified.
> > 
> > We do want to spell out, however, what a driver can expect and a device
> > needs to do for a driver-initiated reset, and that's what this update
> > aims to do.
> >   
> > >     
> > > > +
> > > > +The mechanism used by the driver to initiate the reset is
> > > > transport specific. +
> > > > +\devicenormative{\subsection}{Device Reset}{Basic Facilities of a
> > > > Virtio Device / Device Reset} +
> > > > +A device MUST reinitialize device status to 0 after receiving a
> > > > reset.
> > > > +      
> > > 
> > > What is the intent behind this sentence?
> > > 
> > > One way I can read this it, that the device reset is not allowed to
> > > fail. I.e. device must reinitialize status to after receiving a reset
> > > and status 0 indicating the reset is done implies, that a reset must
> > > succeed eventually (provided the device did receive).    
> > 
> > Yes, I don't think it makes sense to reject a reset.
> >   
> 
> I don't know. And if that is what we want to say, I would certainly
> prefer a SHOULD over a must. E.g. what if the device looses power
> between receiving the reset and being able to finish it. Also if
> we want to express that a reset should not fail we should say that
> in a more straight forward way.

If a device loses power, we cannot really mandate anything; it depends
on the platform whether the device can continue with whatever has been
in progress or whether it ends up in some kind of initial state. IOW, I
think this is outside the scope of this standard.

> 
> How do we expect the device and the driver to deal with a device
> error that ain't recoverable even by a device reset? AFAIR all
> the device can do is set DEVICE_NEEDS_RESET to indicate "that the device
> has experienced an error from which it can’t recover". On the other
> hand, I think the most reasonable way for a driver to meet
> DEVICE_NEEDS_RESET
> is to reset it and make an attempt to re-initialize it...

If the device is not recoverable at all, I'd expect some
platform-specific error when trying to access it in any way (write
error, cc 3 on ssch, ...), as that implies to me that it is not
operational anymore.

> 
> > > 
> > > The other way i can read it is, what I would rather have worded as:
> > > "The device MUST indicate the completion of the reset by
> > > reinitializing the device status to 0".    
> > 
> > That's also true, but I think it is already covered?
> >   
> 
> It is IMHO implicitly covered by the driver normative.

Yes.

> 
> > > 
> > >     
> > > > +A device MUST NOT send notifications after indicating completion
> > > > of +the reset by reinitializing the device status to 0.      
> > > 
> > > I've just realized, that 'after' is very open to the right. Of course
> > > the device may send notifications again after the driver
> > > re-initialized the device as per 3.1.1.    
> > 
> > Append "until the driver re-initializes the device."?
> >   
> 
> That is another solution.
> 
> > > 
> > > Maybe it is wiser to tie this restriction to the current status value
> > > (without any after or before), and do it where the notifications are
> > > described. 
> > > 
> > > Also a device that has status 0 may not poke the virtqueues. If we
> > > are explicit about the notifications, we should be explicit about
> > > the virtqueues as well.    
> > 
> > What about
> > 
> > "A device MUST NOT send notifications or process queue buffers after
> > indication completion of the reset by reinitializing the device status

s/indication/indicating/

> > to 0, until the driver re-initializes the device."
> > 
> > ?  
> 
> Getting better, but IMHO not perfect. Process queue buffers is a bit
> vague IMHO. Even examining (reading) the ring(s) is taboo in my opinion,
> and I'm not sure that is unambiguously covered by 'process queue
> buffers'. In my opinion after a successful reset, the driver is
> entitled to even hot-unplug the ram that used to host the queues,
> notifier bits, etc.

"interact with the queues", maybe?

> 
> All this said, I believe the v3 is already a significant improvement,
> and the virtio spec ain't the most formal and painfully precise
> specification anyway. So if you don't feel like polishing this even
> further.
> 
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>

Thanks!

I guess I'll send a v4 containing the changes outlined above.

@all: anything else I should consider?


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/

Re: [virtio-comment] [PATCH v3] clarify device reset

[Jason Wang]

On 2021/1/27 下午7:11, Cornelia Huck wrote:
> On Wed, 27 Jan 2021 11:07:53 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
>> On 2021/1/25 下午7:08, Cornelia Huck wrote:
>>> Properly specify that the method for the driver to request a
>>> device reset is transport specific, and some action the device
>>> has to take.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>
>>> RFC v2 -> v3:
>>>     - re-worded the "must not send notifications" clause to avoid guessing
>>>     - added a driver conformance clause on how a driver should find out
>>>       when reset is complete
>>> RFC -> RFC v2:
>>>     - moved reset spec to basic facilities
>>>
>>> ---
>>>    conformance.tex |  2 ++
>>>    content.tex     | 19 +++++++++++++++++++
>>>    2 files changed, 21 insertions(+)
>>>
>>> diff --git a/conformance.tex b/conformance.tex
>>> index eb3324053080..21fe89ccd937 100644
>>> --- a/conformance.tex
>>> +++ b/conformance.tex
>>> @@ -60,6 +60,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>>    \begin{itemize}
>>>    \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Status Field}
>>>    \item \ref{drivernormative:Basic Facilities of a Virtio Device / Feature Bits}
>>> +\item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Reset}
>>>    \item \ref{drivernormative:Basic Facilities of a Virtio Device / Device Configuration Space}
>>>    \item \ref{drivernormative:Basic Facilities of a Virtio Device / Virtqueues}
>>>    \item \ref{drivernormative:Basic Facilities of a Virtio Device / Message Framing}
>>> @@ -271,6 +272,7 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>>>    \begin{itemize}
>>>    \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Status Field}
>>>    \item \ref{devicenormative:Basic Facilities of a Virtio Device / Feature Bits}
>>> +\item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Reset}
>>>    \item \ref{devicenormative:Basic Facilities of a Virtio Device / Device Configuration Space}
>>>    \item \ref{devicenormative:Basic Facilities of a Virtio Device / Message Framing}
>>>    \item \ref{devicenormative:Basic Facilities of a Virtio Device / Virtqueues / The Virtqueue Descriptor Table}
>>> diff --git a/content.tex b/content.tex
>>> index 620c0e28c9a7..9cdefe16509e 100644
>>> --- a/content.tex
>>> +++ b/content.tex
>>> @@ -193,6 +193,25 @@ \section{Notifications}\label{sec:Basic Facilities of a Virtio Device
>>>    terminology. Occasionally, the term event is used to refer to
>>>    a notification or a receipt of a notification.
>>>    
>>> +\section{Device Reset}\label{sec:Basic Facilities of a Virtio Device / Device Reset}
>>> +
>>> +The driver may initiate a device reset at various times; notably, during
>>> +device initialization and device cleanup.
>>> +
>>> +The mechanism used by the driver to initiate the reset is transport specific.
>>> +
>>> +\devicenormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
>>> +
>>> +A device MUST reinitialize device status to 0 after receiving a reset.
>>> +
>>
>> We had similar description in "2.1.2 Device Requirements: Device Status
>> Field":
>>
>> "The device MUST initialize device status to 0 upon reset."
>>
>> Consider we had a dedicated part for reset, maybe we can remove that one.
> Maybe we can replace the normative statement with an informal
> description in the device status section? I.e. something like
>
> "The device status field starts out as 0, and is reinitialized to 0 by
> the device during reset."


Fine with me.

Thanks


>
>> Thanks
>>
>>
>>> +A device MUST NOT send notifications after indicating completion of
>>> +the reset by reinitializing the device status to 0.
>>> +
>>> +\drivernormative{\subsection}{Device Reset}{Basic Facilities of a Virtio Device / Device Reset}
>>> +
>>> +The driver SHOULD consider a driver-initiated reset complete when it
>>> +reads the device status as 0.
>>> +
>>>    \section{Device Configuration Space}\label{sec:Basic Facilities of a Virtio Device / Device Configuration Space}
>>>    
>>>    Device configuration space is generally used for rarely-changing or
>
> 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/
>


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/

[virtio-comment] Re: [PATCH v3] clarify device reset

[Halil Pasic]

On Wed, 27 Jan 2021 18:14:11 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 27 Jan 2021 14:48:57 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Wed, 27 Jan 2021 12:44:03 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >   
> > > On Wed, 27 Jan 2021 10:19:04 +0100
> > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > >     
> > > > On Mon, 25 Jan 2021 12:08:23 +0100
> > > > Cornelia Huck <cohuck@redhat.com> wrote:  
> 
> > > > > +\section{Device
> > > > > Reset}\label{sec:Basic Facilities of a Virtio Device / Device
> > > > > Reset} + +The driver may initiate a device reset at various times;
> > > > > notably, during +device initialization and device cleanup.        
> > > > 
> > > > Are there times the driver may not initiate a device reset? What are
> > > > we trying to tell with this sentence?      
> > > 
> > > s/may initiate/may want to initiate/    
> > 
> > Definitively better. I would still prefer avoiding the discussion
> > on when the driver may want to reset the device. But I can live
> > with this.  
> 
> The driver is free to reset the device whenever it wants to, even if it
> does not make sense. Device initialization and cleanup are examples for
> when it definitely will do so.
> 

For device initialization and for cleanup is actually a MUST,
and not a simple may want to. And that is properly specified in 3.1.1
and 3.3.1 respectively.

I would actually advice against resetting the device when device status
is still 0, and especially if we expect it to change to ACKNOWLEDGE,
because we just asked the device to set that bit. Why? Because I assume
we could read the old 0 and assume the reset was performed.
 

> >   
> > >     
> > > > 
> > > > If we are just looking for an introductionary sentence, I would
> > > > probably go with something like "A device reset be either initiated
> > > > by the    
> > 
> > s/be/can be/
> >   
> > > > driver, or by a system reset, or other external controls."      
> > > 
> > > It was mostly supposed to be a lead-in.
> > > 
> > > I'm not sure we want to spell out things like system resets. They are
> > > platform-specific, and virtio devices are basically just a device
> > > there.    
> > 
> > Hm. AFAIR the only way to make the device non-live, i.e. illegal for it 
> > to e.g. access a queue, which is in guest memory is device reset. Now if
> > I want to perform a reboot, I do want to stop the devices from poking
> > such memory. But I think, in linux we don't reset the device form the
> > driver, before reboot, and I believe we don't have to because we are
> > guaranteed that the device will get reset. Or is my memory failing me?
> > 
> > I think it is of value to clarify that a virtio reset can possibly be
> > initiated by other means than explicitly specified by the virtio spec,
> > and that when a 'proxy-device' is reset, the 'virtio-device' is to be
> > reset as well. We do mention system reset once, in a normative section
> > at that (2.2.2).  
> 
> That section looks a bit questionable (what is 'resuming from suspend'?).
> 

I have no problems with that sections, and I think it is very
reasonable. You can think of 'resuming form suspend' like 'resuming from
suspend-to-disk' or from 'virsh save'. I.e. a situation where the device
has (probably) lost power and thus needs a (complete) re-initialization,
but we are supposed to preserve some continuity (e.g. if ACCESS_PLATFORM
was negotiable, it should still be negotiable).

> Platforms may have different definitions of what a system reset does
> (and even different types of reset). I think it is reasonable to assume
> that any platform/system reset that brings devices into some kind of
> initial state also brings virtio devices into that initial state (and
> especially that bringing the proxy device into an initial state also
> covers the virtio-specific parts.)
> 

In an ideal world, after reading the spec, one should have to ponder if
something is reasonable to assume or not.

I agree, platforms (e.g. s390, ppc) may have different definitions and
different flavors of resets, and the situation is similar for the
transports (e.g. PCI, CCW). And the very same device may be used on
different platforms. 

For PCIe there is something called a Function Level Reset. 

The relationship between the different resets ain't trivial to me. E.g.
when we say 'reset the device' we actually mean 'reset the virtio
aspect of the device', and not the 'entire device'. E.g.
CCW_CMD_VDEV_RESET does not reset the ccw-device.

[..]

> > > > 
> > > > Maybe it is wiser to tie this restriction to the current status value
> > > > (without any after or before), and do it where the notifications are
> > > > described. 
> > > > 
> > > > Also a device that has status 0 may not poke the virtqueues. If we
> > > > are explicit about the notifications, we should be explicit about
> > > > the virtqueues as well.      
> > > 
> > > What about
> > > 
> > > "A device MUST NOT send notifications or process queue buffers after
> > > indication completion of the reset by reinitializing the device status  
> 
> s/indication/indicating/
> 
> > > to 0, until the driver re-initializes the device."
> > > 
> > > ?    
> > 
> > Getting better, but IMHO not perfect. Process queue buffers is a bit
> > vague IMHO. Even examining (reading) the ring(s) is taboo in my opinion,
> > and I'm not sure that is unambiguously covered by 'process queue
> > buffers'. In my opinion after a successful reset, the driver is
> > entitled to even hot-unplug the ram that used to host the queues,
> > notifier bits, etc.  
> 
> "interact with the queues", maybe?
>

Yes even better. Another thing to consider is behavior during the reset.
I think it could be beneficial to say that.

The device SHOULD NOT interact with the queues while performing the
reset.

If I'm not wrong there was some problem with secure execution, but I
don't remember what did we do about it. What I remember is that
virtio-blk interacted with the queue during the reset, which happened,
after the protection/encryption status of the memory backing the queues
changed.
 
> > 
> > All this said, I believe the v3 is already a significant improvement,
> > and the virtio spec ain't the most formal and painfully precise
> > specification anyway. So if you don't feel like polishing this even
> > further.
> > 
> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com>  
> 
> Thanks!
> 
> I guess I'll send a v4 containing the changes outlined above.
> 
> @all: anything else I should consider?
> 



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/

[virtio-comment] Re: [PATCH v3] clarify device reset

[Cornelia Huck]

On Thu, 28 Jan 2021 09:06:32 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Wed, 27 Jan 2021 18:14:11 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Wed, 27 Jan 2021 14:48:57 +0100
> > Halil Pasic <pasic@linux.ibm.com> wrote:
> >   
> > > On Wed, 27 Jan 2021 12:44:03 +0100
> > > Cornelia Huck <cohuck@redhat.com> wrote:
> > >     
> > > > On Wed, 27 Jan 2021 10:19:04 +0100
> > > > Halil Pasic <pasic@linux.ibm.com> wrote:
> > > >       
> > > > > On Mon, 25 Jan 2021 12:08:23 +0100
> > > > > Cornelia Huck <cohuck@redhat.com> wrote:    
> >   
> > > > > > +\section{Device
> > > > > > Reset}\label{sec:Basic Facilities of a Virtio Device / Device
> > > > > > Reset} + +The driver may initiate a device reset at various times;
> > > > > > notably, during +device initialization and device cleanup.          
> > > > > 
> > > > > Are there times the driver may not initiate a device reset? What are
> > > > > we trying to tell with this sentence?        
> > > > 
> > > > s/may initiate/may want to initiate/      
> > > 
> > > Definitively better. I would still prefer avoiding the discussion
> > > on when the driver may want to reset the device. But I can live
> > > with this.    
> > 
> > The driver is free to reset the device whenever it wants to, even if it
> > does not make sense. Device initialization and cleanup are examples for
> > when it definitely will do so.
> >   
> 
> For device initialization and for cleanup is actually a MUST,
> and not a simple may want to. And that is properly specified in 3.1.1
> and 3.3.1 respectively.
> 
> I would actually advice against resetting the device when device status
> is still 0, and especially if we expect it to change to ACKNOWLEDGE,
> because we just asked the device to set that bit. Why? Because I assume
> we could read the old 0 and assume the reset was performed.

I don't think we need to point out every little questionable thing a
driver might do.

>  
> 
> > >     
> > > >       
> > > > > 
> > > > > If we are just looking for an introductionary sentence, I would
> > > > > probably go with something like "A device reset be either initiated
> > > > > by the      
> > > 
> > > s/be/can be/
> > >     
> > > > > driver, or by a system reset, or other external controls."        
> > > > 
> > > > It was mostly supposed to be a lead-in.
> > > > 
> > > > I'm not sure we want to spell out things like system resets. They are
> > > > platform-specific, and virtio devices are basically just a device
> > > > there.      
> > > 
> > > Hm. AFAIR the only way to make the device non-live, i.e. illegal for it 
> > > to e.g. access a queue, which is in guest memory is device reset. Now if
> > > I want to perform a reboot, I do want to stop the devices from poking
> > > such memory. But I think, in linux we don't reset the device form the
> > > driver, before reboot, and I believe we don't have to because we are
> > > guaranteed that the device will get reset. Or is my memory failing me?
> > > 
> > > I think it is of value to clarify that a virtio reset can possibly be
> > > initiated by other means than explicitly specified by the virtio spec,
> > > and that when a 'proxy-device' is reset, the 'virtio-device' is to be
> > > reset as well. We do mention system reset once, in a normative section
> > > at that (2.2.2).    
> > 
> > That section looks a bit questionable (what is 'resuming from suspend'?).
> >   
> 
> I have no problems with that sections, and I think it is very
> reasonable. You can think of 'resuming form suspend' like 'resuming from
> suspend-to-disk' or from 'virsh save'. I.e. a situation where the device
> has (probably) lost power and thus needs a (complete) re-initialization,
> but we are supposed to preserve some continuity (e.g. if ACCESS_PLATFORM
> was negotiable, it should still be negotiable).

Maybe leave that for a different discussion?

> 
> > Platforms may have different definitions of what a system reset does
> > (and even different types of reset). I think it is reasonable to assume
> > that any platform/system reset that brings devices into some kind of
> > initial state also brings virtio devices into that initial state (and
> > especially that bringing the proxy device into an initial state also
> > covers the virtio-specific parts.)
> >   
> 
> In an ideal world, after reading the spec, one should have to ponder if
> something is reasonable to assume or not.
> 
> I agree, platforms (e.g. s390, ppc) may have different definitions and
> different flavors of resets, and the situation is similar for the
> transports (e.g. PCI, CCW). And the very same device may be used on
> different platforms. 
> 
> For PCIe there is something called a Function Level Reset. 
> 
> The relationship between the different resets ain't trivial to me. E.g.
> when we say 'reset the device' we actually mean 'reset the virtio
> aspect of the device', and not the 'entire device'. E.g.
> CCW_CMD_VDEV_RESET does not reset the ccw-device.

This spec is dealing with virtio and the proxy devices. Attempting to
use a virtio mechanism to reset a generic pci or ccw entity is not
something I'd extract from the spec. Resetting the virtio and proxy
device parts by resetting the generic device is something that does not
look any different from any other device type, so I really consider it
out of scope. IOW, I'd rather not go down that rabbit hole.

> 
> [..]
> 
> > > > > 
> > > > > Maybe it is wiser to tie this restriction to the current status value
> > > > > (without any after or before), and do it where the notifications are
> > > > > described. 
> > > > > 
> > > > > Also a device that has status 0 may not poke the virtqueues. If we
> > > > > are explicit about the notifications, we should be explicit about
> > > > > the virtqueues as well.        
> > > > 
> > > > What about
> > > > 
> > > > "A device MUST NOT send notifications or process queue buffers after
> > > > indication completion of the reset by reinitializing the device status    
> > 
> > s/indication/indicating/
> >   
> > > > to 0, until the driver re-initializes the device."
> > > > 
> > > > ?      
> > > 
> > > Getting better, but IMHO not perfect. Process queue buffers is a bit
> > > vague IMHO. Even examining (reading) the ring(s) is taboo in my opinion,
> > > and I'm not sure that is unambiguously covered by 'process queue
> > > buffers'. In my opinion after a successful reset, the driver is
> > > entitled to even hot-unplug the ram that used to host the queues,
> > > notifier bits, etc.    
> > 
> > "interact with the queues", maybe?
> >  
> 
> Yes even better. Another thing to consider is behavior during the reset.
> I think it could be beneficial to say that.
> 
> The device SHOULD NOT interact with the queues while performing the
> reset.
> 
> If I'm not wrong there was some problem with secure execution, but I
> don't remember what did we do about it. What I remember is that
> virtio-blk interacted with the queue during the reset, which happened,
> after the protection/encryption status of the memory backing the queues
> changed.

I don't think we should actually specify that: we never specified it
before, so the driver cannot make a fair assumption anyway.


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/