[virtio-comment] [PATCH RFC v2] clarify device reset

[virtio-comment] [PATCH RFC v2] 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 -> RFC v2:
  - moved reset spec to basic facilities

---
 conformance.tex |  1 +
 content.tex     | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/conformance.tex b/conformance.tex
index eb3324053080..3be499ae3c5e 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -271,6 +271,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..782ddf3ed78d 100644
--- a/content.tex
+++ b/content.tex
@@ -193,6 +193,19 @@ \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 receiving a reset.
+
 \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 RFC v2] clarify device reset

[Dr. David Alan Gilbert]

* 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 -> RFC v2:
>   - moved reset spec to basic facilities
> 
> ---
>  conformance.tex |  1 +
>  content.tex     | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/conformance.tex b/conformance.tex
> index eb3324053080..3be499ae3c5e 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -271,6 +271,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..782ddf3ed78d 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -193,6 +193,19 @@ \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 receiving a reset.
> +

This feels like a bit of a race in the description;  a Device may have
just sent a notification at the point that it receives a reset.
When a driver initiates a reset, how does the driver know that the
device has received it?

Dave

>  \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/
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK


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 RFC v2] clarify device reset

[Cornelia Huck]

On Mon, 18 Jan 2021 16:41:32 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * 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 -> RFC v2:
> >   - moved reset spec to basic facilities
> > 
> > ---
> >  conformance.tex |  1 +
> >  content.tex     | 13 +++++++++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/conformance.tex b/conformance.tex
> > index eb3324053080..3be499ae3c5e 100644
> > --- a/conformance.tex
> > +++ b/conformance.tex
> > @@ -271,6 +271,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..782ddf3ed78d 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -193,6 +193,19 @@ \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 receiving a reset.
> > +  
> 
> This feels like a bit of a race in the description;  a Device may have
> just sent a notification at the point that it receives a reset.
> When a driver initiates a reset, how does the driver know that the
> device has received it?

I'm actually not 100% sure about that requirement... we can also drop
it.


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 RFC v2] clarify device reset

[Halil Pasic]

On Mon, 18 Jan 2021 16:41:32 +0000
"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:

> * 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 -> RFC v2:
> >   - moved reset spec to basic facilities
> > 
> > ---
> >  conformance.tex |  1 +
> >  content.tex     | 13 +++++++++++++
> >  2 files changed, 14 insertions(+)
> > 
> > diff --git a/conformance.tex b/conformance.tex
> > index eb3324053080..3be499ae3c5e 100644
> > --- a/conformance.tex
> > +++ b/conformance.tex
> > @@ -271,6 +271,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..782ddf3ed78d 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -193,6 +193,19 @@ \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 receiving a reset.
> > +

s/after receiving a reset/after presenting a 0 status, that indicates
the reset is done/  
> 
> This feels like a bit of a race in the description;  a Device may have
> just sent a notification at the point that it receives a reset.
> When a driver initiates a reset, how does the driver know that the
> device has received it?

I agree, but with the proposed modification not any more. 

To answer your question: PCI has the following driver normative (which I
believe needs to be generalized so we have something similar for each
transport, and thus the same semantics):
"After writing 0 to device_status, the driver MUST wait for a read of
device_status to return 0 before reinitializing the device." 
(4.1.4.3.2 Driver Requirements: Common configuration structure layout,
https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-1090004)

In general, after asking for a reset, the driver should/must ensure that
the reset was performed by the device by reading a 0 status. If the
status is non-zero, the reset at the device may still be in progress.
IMHO we need another driver normative for that.

Of course tying this kind of sequencing (happens-before) to the
receiving of a reset does not work. I agree with that to the fullest.

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/

Re: [virtio-comment] [PATCH RFC v2] clarify device reset

[Halil Pasic]

On Mon, 18 Jan 2021 17:48:42 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, 18 Jan 2021 16:41:32 +0000
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * 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 -> RFC v2:
> > >   - moved reset spec to basic facilities
> > > 
> > > ---
> > >  conformance.tex |  1 +
> > >  content.tex     | 13 +++++++++++++
> > >  2 files changed, 14 insertions(+)
> > > 
> > > diff --git a/conformance.tex b/conformance.tex
> > > index eb3324053080..3be499ae3c5e 100644
> > > --- a/conformance.tex
> > > +++ b/conformance.tex
> > > @@ -271,6 +271,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..782ddf3ed78d 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -193,6 +193,19 @@ \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 receiving a reset.
> > > +    
> > 
> > This feels like a bit of a race in the description;  a Device may have
> > just sent a notification at the point that it receives a reset.
> > When a driver initiates a reset, how does the driver know that the
> > device has received it?  
> 
> I'm actually not 100% sure about that requirement... we can also drop
> it.
> 

Please see my response to Dave's mail. In short, I don't we need to drop
it , I think we need to improve and expand.

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/

Re: [virtio-comment] [PATCH RFC v2] clarify device reset

[Cornelia Huck]

On Tue, 19 Jan 2021 03:40:08 +0100
Halil Pasic <pasic@linux.ibm.com> wrote:

> On Mon, 18 Jan 2021 16:41:32 +0000
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> 
> > * 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 -> RFC v2:
> > >   - moved reset spec to basic facilities
> > > 
> > > ---
> > >  conformance.tex |  1 +
> > >  content.tex     | 13 +++++++++++++
> > >  2 files changed, 14 insertions(+)
> > > 
> > > diff --git a/conformance.tex b/conformance.tex
> > > index eb3324053080..3be499ae3c5e 100644
> > > --- a/conformance.tex
> > > +++ b/conformance.tex
> > > @@ -271,6 +271,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..782ddf3ed78d 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -193,6 +193,19 @@ \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 receiving a reset.
> > > +  
> 
> s/after receiving a reset/after presenting a 0 status, that indicates
> the reset is done/  

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

?

> > 
> > This feels like a bit of a race in the description;  a Device may have
> > just sent a notification at the point that it receives a reset.
> > When a driver initiates a reset, how does the driver know that the
> > device has received it?  
> 
> I agree, but with the proposed modification not any more. 
> 
> To answer your question: PCI has the following driver normative (which I
> believe needs to be generalized so we have something similar for each
> transport, and thus the same semantics):
> "After writing 0 to device_status, the driver MUST wait for a read of
> device_status to return 0 before reinitializing the device." 
> (4.1.4.3.2 Driver Requirements: Common configuration structure layout,
> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-1090004)
> 
> In general, after asking for a reset, the driver should/must ensure that
> the reset was performed by the device by reading a 0 status. If the
> status is non-zero, the reset at the device may still be in progress.
> IMHO we need another driver normative for that.

"After the driver has initiated a reset of the device, it MUST NOT
consider the reset to be completed if the device status is not 0."

?

Maybe without the double negation.

(We could consider the reset for ccw devices done once we get final
status for the reset ccw. Would save the round trip for a read status
ccw, but would also be different from the other transports.)


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 RFC v2] clarify device reset

[Halil Pasic]

On Tue, 19 Jan 2021 18:45:06 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 19 Jan 2021 03:40:08 +0100
> Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Mon, 18 Jan 2021 16:41:32 +0000
> > "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >   
> > > * 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 -> RFC v2:
> > > >   - moved reset spec to basic facilities
> > > > 
> > > > ---
> > > >  conformance.tex |  1 +
> > > >  content.tex     | 13 +++++++++++++
> > > >  2 files changed, 14 insertions(+)
> > > > 
> > > > diff --git a/conformance.tex b/conformance.tex
> > > > index eb3324053080..3be499ae3c5e 100644
> > > > --- a/conformance.tex
> > > > +++ b/conformance.tex
> > > > @@ -271,6 +271,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..782ddf3ed78d 100644
> > > > --- a/content.tex
> > > > +++ b/content.tex
> > > > @@ -193,6 +193,19 @@ \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 receiving a reset.
> > > > +    
> > 
> > s/after receiving a reset/after presenting a 0 status, that indicates
> > the reset is done/    
> 
> "A device MUST NOT send notifications after indicating completion of
> the reset by reinitializing the device status to 0."
> 
> ?

Works with me. I tried to align my wording with the pci wording.

> 
> > > 
> > > This feels like a bit of a race in the description;  a Device may have
> > > just sent a notification at the point that it receives a reset.
> > > When a driver initiates a reset, how does the driver know that the
> > > device has received it?    
> > 
> > I agree, but with the proposed modification not any more. 
> > 
> > To answer your question: PCI has the following driver normative (which I
> > believe needs to be generalized so we have something similar for each
> > transport, and thus the same semantics):
> > "After writing 0 to device_status, the driver MUST wait for a read of
> > device_status to return 0 before reinitializing the device." 
> > (4.1.4.3.2 Driver Requirements: Common configuration structure layout,
> > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-1090004)
> > 
> > In general, after asking for a reset, the driver should/must ensure that
> > the reset was performed by the device by reading a 0 status. If the
> > status is non-zero, the reset at the device may still be in progress.
> > IMHO we need another driver normative for that.  
> 
> "After the driver has initiated a reset of the device, it MUST NOT
> consider the reset to be completed if the device status is not 0."
> 
> ?

", before it reads status 0."

My point is, that usually when I do an assignment to a memory location
with a single instruction, and the instruction completes successfully,
for me (on my CPU), that memory location is 0.

PCI is however not like this: the device can delay or reject the write,
apparently. Jason taught me that. So I think we should insist on the
read.

> 
> Maybe without the double negation.
> 
> (We could consider the reset for ccw devices done once we get final
> status for the reset ccw. Would save the round trip for a read status
> ccw, but would also be different from the other transports.)

We could work around that by making a positive statement. Not telling,
when the driver MUST NOT consider the reset completed, but tell when the
driver SHOULD consider the reset completed.

The MUST NOT does not buy much to the driver. It knows, what is
certainly wrong, but it still does not know what is right. What the
driver needs is a criterion when the reset is certainly completed (so
it can free up resources for example).

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/

Re: [virtio-comment] [PATCH RFC v2] clarify device reset

[Jason Wang]

On 2021/1/20 上午2:52, Halil Pasic wrote:
> On Tue, 19 Jan 2021 18:45:06 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Tue, 19 Jan 2021 03:40:08 +0100
>> Halil Pasic <pasic@linux.ibm.com> wrote:
>>
>>> On Mon, 18 Jan 2021 16:41:32 +0000
>>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
>>>    
>>>> * 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 -> RFC v2:
>>>>>    - moved reset spec to basic facilities
>>>>>
>>>>> ---
>>>>>   conformance.tex |  1 +
>>>>>   content.tex     | 13 +++++++++++++
>>>>>   2 files changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/conformance.tex b/conformance.tex
>>>>> index eb3324053080..3be499ae3c5e 100644
>>>>> --- a/conformance.tex
>>>>> +++ b/conformance.tex
>>>>> @@ -271,6 +271,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..782ddf3ed78d 100644
>>>>> --- a/content.tex
>>>>> +++ b/content.tex
>>>>> @@ -193,6 +193,19 @@ \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 receiving a reset.
>>>>> +
>>> s/after receiving a reset/after presenting a 0 status, that indicates
>>> the reset is done/
>> "A device MUST NOT send notifications after indicating completion of
>> the reset by reinitializing the device status to 0."
>>
>> ?
> Works with me. I tried to align my wording with the pci wording.
>
>>>> This feels like a bit of a race in the description;  a Device may have
>>>> just sent a notification at the point that it receives a reset.
>>>> When a driver initiates a reset, how does the driver know that the
>>>> device has received it?
>>> I agree, but with the proposed modification not any more.
>>>
>>> To answer your question: PCI has the following driver normative (which I
>>> believe needs to be generalized so we have something similar for each
>>> transport, and thus the same semantics):
>>> "After writing 0 to device_status, the driver MUST wait for a read of
>>> device_status to return 0 before reinitializing the device."
>>> (4.1.4.3.2 Driver Requirements: Common configuration structure layout,
>>> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-1090004)
>>>
>>> In general, after asking for a reset, the driver should/must ensure that
>>> the reset was performed by the device by reading a 0 status. If the
>>> status is non-zero, the reset at the device may still be in progress.
>>> IMHO we need another driver normative for that.
>> "After the driver has initiated a reset of the device, it MUST NOT
>> consider the reset to be completed if the device status is not 0."
>>
>> ?
> ", before it reads status 0."
>
> My point is, that usually when I do an assignment to a memory location
> with a single instruction, and the instruction completes successfully,
> for me (on my CPU), that memory location is 0.
>
> PCI is however not like this: the device can delay or reject the write,
> apparently. Jason taught me that. So I think we should insist on the
> read.


Yes. For PCI the status is implemented via registers, there's no 
guarantee a read is 0 after write 0 to that.


>
>> Maybe without the double negation.
>>
>> (We could consider the reset for ccw devices done once we get final
>> status for the reset ccw. Would save the round trip for a read status
>> ccw, but would also be different from the other transports.)


I think it's probably not a problem since we don't care about the 
performance of reset.

Thanks


> We could work around that by making a positive statement. Not telling,
> when the driver MUST NOT consider the reset completed, but tell when the
> driver SHOULD consider the reset completed.
>
> The MUST NOT does not buy much to the driver. It knows, what is
> certainly wrong, but it still does not know what is right. What the
> driver needs is a criterion when the reset is certainly completed (so
> it can free up resources for example).
>
> 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/
>


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 RFC v2] clarify device reset

[Cornelia Huck]

On Wed, 20 Jan 2021 11:13:37 +0800
Jason Wang <jasowang@redhat.com> wrote:

> On 2021/1/20 上午2:52, Halil Pasic wrote:
> > On Tue, 19 Jan 2021 18:45:06 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >  
> >> On Tue, 19 Jan 2021 03:40:08 +0100
> >> Halil Pasic <pasic@linux.ibm.com> wrote:
> >>  
> >>> On Mon, 18 Jan 2021 16:41:32 +0000
> >>> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> >>>      
> >>>> * 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 -> RFC v2:
> >>>>>    - moved reset spec to basic facilities
> >>>>>
> >>>>> ---
> >>>>>   conformance.tex |  1 +
> >>>>>   content.tex     | 13 +++++++++++++
> >>>>>   2 files changed, 14 insertions(+)
> >>>>>
> >>>>> diff --git a/conformance.tex b/conformance.tex
> >>>>> index eb3324053080..3be499ae3c5e 100644
> >>>>> --- a/conformance.tex
> >>>>> +++ b/conformance.tex
> >>>>> @@ -271,6 +271,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..782ddf3ed78d 100644
> >>>>> --- a/content.tex
> >>>>> +++ b/content.tex
> >>>>> @@ -193,6 +193,19 @@ \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 receiving a reset.
> >>>>> +  
> >>> s/after receiving a reset/after presenting a 0 status, that indicates
> >>> the reset is done/  
> >> "A device MUST NOT send notifications after indicating completion of
> >> the reset by reinitializing the device status to 0."
> >>
> >> ?  
> > Works with me. I tried to align my wording with the pci wording.
> >  
> >>>> This feels like a bit of a race in the description;  a Device may have
> >>>> just sent a notification at the point that it receives a reset.
> >>>> When a driver initiates a reset, how does the driver know that the
> >>>> device has received it?  
> >>> I agree, but with the proposed modification not any more.
> >>>
> >>> To answer your question: PCI has the following driver normative (which I
> >>> believe needs to be generalized so we have something similar for each
> >>> transport, and thus the same semantics):
> >>> "After writing 0 to device_status, the driver MUST wait for a read of
> >>> device_status to return 0 before reinitializing the device."
> >>> (4.1.4.3.2 Driver Requirements: Common configuration structure layout,
> >>> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-1090004)
> >>>
> >>> In general, after asking for a reset, the driver should/must ensure that
> >>> the reset was performed by the device by reading a 0 status. If the
> >>> status is non-zero, the reset at the device may still be in progress.
> >>> IMHO we need another driver normative for that.  
> >> "After the driver has initiated a reset of the device, it MUST NOT
> >> consider the reset to be completed if the device status is not 0."
> >>
> >> ?  
> > ", before it reads status 0."
> >
> > My point is, that usually when I do an assignment to a memory location
> > with a single instruction, and the instruction completes successfully,
> > for me (on my CPU), that memory location is 0.
> >
> > PCI is however not like this: the device can delay or reject the write,
> > apparently. Jason taught me that. So I think we should insist on the
> > read.  
> 
> 
> Yes. For PCI the status is implemented via registers, there's no 
> guarantee a read is 0 after write 0 to that.

I thought it was clear that the driver cannot know the device status
without doing a read, but we certainly can make that explicit.

> 
> 
> >  
> >> Maybe without the double negation.
> >>
> >> (We could consider the reset for ccw devices done once we get final
> >> status for the reset ccw. Would save the round trip for a read status
> >> ccw, but would also be different from the other transports.)  
> 
> 
> I think it's probably not a problem since we don't care about the 
> performance of reset.

I was less concerned about the performance, more about the complexity
of the implementation. Sending an extra command is not that bad, though.

> 
> Thanks
> 
> 
> > We could work around that by making a positive statement. Not telling,
> > when the driver MUST NOT consider the reset completed, but tell when the
> > driver SHOULD consider the reset completed.

That could be

"The driver SHOULD consider a driver-initiated reset complete when it
reads the device status as 0."

> >
> > The MUST NOT does not buy much to the driver. It knows, what is
> > certainly wrong, but it still does not know what is right. What the
> > driver needs is a criterion when the reset is certainly completed (so
> > it can free up resources for example).

Any further comments before I put together a v3 (without the RFC)?
I also need to open a github issue for this.


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 RFC v2] clarify device reset

[Halil Pasic]

On Fri, 22 Jan 2021 11:42:12 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> > > We could work around that by making a positive statement. Not telling,
> > > when the driver MUST NOT consider the reset completed, but tell when the
> > > driver SHOULD consider the reset completed.  
> 
> That could be
> 
> "The driver SHOULD consider a driver-initiated reset complete when it
> reads the device status as 0."

Sounds good to me!

> 
> > >
> > > The MUST NOT does not buy much to the driver. It knows, what is
> > > certainly wrong, but it still does not know what is right. What the
> > > driver needs is a criterion when the reset is certainly completed (so
> > > it can free up resources for example).  
> 
> Any further comments before I put together a v3 (without the RFC)?
> I also need to open a github issue for this.

No further comments. I think we are heading in the right direction.

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/