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

[Cornelia Huck <cohuck@redhat.com>]

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.