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

["Michael S. Tsirkin" <mst@redhat.com>]

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.