[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio] [PATCH 1/1] ccw: add CCW_CMD_READ_STATUS
On Mon, Oct 12, 2015 at 01:44:33PM +0200, Cornelia Huck wrote: > On Sun, 11 Oct 2015 18:41:52 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Fri, Sep 11, 2015 at 03:52:27PM +0200, Cornelia Huck wrote: > > > ccw currently allows the driver to update the status via the > > > CCW_CMD_WRITE_STATUS command; however, it does not allow the driver to > > > retrieve the current status at the device, which is needed to properly > > > support DEVICE_NEEDS_RESET. > > > > > > Therefore, provide a new command CCW_CMD_READ_STATUS allowing the driver > > > to retrieve the device status. Provide this command when revision 2 has > > > been negotiated. > > > > > > VIRTIO-117 > > > > Do you want this addressed in cs04? Deferred until 1.1? > > As mentioned in the cover letter: It's probably better to target 1.1, > if we consider -04 as the revision for getting feature parity with the > old spec, and I don't want to hold it up. OK, so if it's ready in time, we'll include it. > > When you think we are ready to vote on this, > > pls change status to open. > > Set Envoronment to reporter's name and email, > > and set fixed in version to version which should have this text. > > > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > > > --- > > > content.tex | 23 ++++++++++++++++++++++- > > > 1 file changed, 22 insertions(+), 1 deletion(-) > > > > > > diff --git a/content.tex b/content.tex > > > index d989d98..49d5829 100644 > > > --- a/content.tex > > > +++ b/content.tex > > > @@ -2505,6 +2505,7 @@ virtio: > > > #define CCW_CMD_WRITE_STATUS 0x31 > > > #define CCW_CMD_READ_VQ_CONF 0x32 > > > #define CCW_CMD_SET_VIRTIO_REV 0x83 > > > +#define CCW_CMD_READ_STATUS 0x72 > > > \end{lstlisting} > > > > > > \devicenormative{\subsubsection}{Basic Concepts}{Virtio Transport Options / Virtio over channel I/O / Basic Concepts} > > > @@ -2565,7 +2566,9 @@ The following values are supported: > > > \hline > > > 1 & 0 & <empty> & Virtio 1.0 \\ > > > \hline > > > -2-n & & & reserved for later revisions \\ > > > +2 & 0 & <empty> & CCW_CMD_READ_STATUS support \\ > > > +\hline > > > +3-n & & & reserved for later revisions \\ > > > \hline > > > \end{tabular} > > > > > > @@ -2683,18 +2686,36 @@ As described in > > > a device sometimes fails to set the \field{status} field: For example, it > > > might fail to accept the FEATURES_OK status bit during device initialization. > > > > > > +With revision 2, CCW_CMD_READ_STATUS is defined: It reads an 8 bit status > > > +value from the device and acts as a reverse operation to CCW_CMD_WRITE_STATUS. > > > + > > > \drivernormative{\paragraph}{Communicating Status Information}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Communicating Status Information} > > > > > > If the device posts a unit check with command reject in response to the > > > CCW_CMD_WRITE_STATUS command, the driver MUST assume that the device failed > > > to set the status and the \field{status} field retained its previous value. > > > > > > +If at least revision 2 has been negotiated, the driver MUST use the > > > > Why is this MUST here? > > The idea was that the device can rely on the driver having fetched the > new value and not needing to accommodate drivers that are not aware of > recent status changes. > > If we consider it the driver's responsibility to keep abreast of > changes (so that the device does not really need to care whether the > driver fetches all changes), we can change it to SHOULD. I this so - see below: > > > > > +CCW_CMD_READ_STATUS command to retrieve the \field{status} field after > > > +a configuration change interrupt has been retrieved. > > > > received? > > Maybe better "after a configuration change is detected". > > Hm... although I'm not sure how a driver may detect a configuration > change other than through an interrupt. It reads the indicator, isn't this right? > > What if interrupts are disabled? > > > > > It also SHOULD use > > > +the CCW_CMD_READ_STATUS command to verify the \field{status} field after it > > > +issued the CCW_CMD_WRITE_STATUS command. > > > > Seems to conflict with above logic which says > > assume it retained previous value. > > There are two parts to that: > - WRITE_STATUS failed -> status field is unchanged > - WRITE_STATUS did not fail -> status field has been modified; but use > READ_STATUS to verify that the device did not modify it itself > > I was thinking of the logic "write status with FEATURES_OK, then read > it to check whether the device kept FEATURES_OK". But it's probably ok > to keep that logic inside the generic sections; I'll remove this > sentence. My question is generally this: what's wrong with keeping the current logic where we require FEATURES_OK to fail if device doesn't set valid features? The result is that CCW_CMD_READ_STATUS is merely there as an option: for fatal errors or for drivers that don't want to maintain the status internally. > > > > > > + > > > +If not at least revision 2 has been negotiated, the driver SHOULD NOT attempt > > > +to issue the CCW_CMD_READ_STATUS command. > > > > Any reason it can do it? Maybe make this MUST NOT. > > Hm. It doesn't do harm (we have command reject), but as we're talking > about a compliant driver, MUST NOT is probably better. > > > > > > + > > > \devicenormative{\paragraph}{Communicating Status Information}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Communicating Status Information} > > > > > > If the device fails to set the \field{status} field to the value written by > > > the driver, the device MUST assure that the \field{status} field is left > > > unchanged and MUST post a unit check with command reject. > > > > > > +If at least revision 2 has been negotiated, the device MUST return the > > > +current \field{status} field if the CCW_CMD_READ_STATUS command is issued. > > > + > > > +If not at least revision 2 has been negotiated, the device SHOULD post a > > > +unit check with command reject to the CCW_CMD_READ_STATUS command. > > > > Why not MUST? Any reason it might make sense not to after all? > > See above; MUST is probably the better choice. > > > > > > + > > > \subsubsection{Handling Device Features}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features} > > > > > > Feature bits are arranged in an array of 32 bit values, making > > I'll prepare a v2.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]