Re: [virtio] [PATCH 1/1] ccw: add CCW_CMD_READ_STATUS

[Cornelia Huck <cornelia.huck@de.ibm.com>]

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.

> 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.

> 
> > +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.

> 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.

> 
> > +
> > +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.