[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [PATCH v3] clarify device reset
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
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]