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

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

On Mon, 12 Oct 2015 15:34:18 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

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

> > > >  \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?

Hm, yes, it might poll the configuration changed indicator.

Although I'm wondering: While 2.1.2 talks about "sending" a change
notification (which one might interpret as making the interrupt
pending), 3.2.3 explicitly talks about delivering/triggering an
interrupt. Should the wording in 3.2.3 be changed to match the wording
in 2.1.2, or is this really picking at words?

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

We still have NEEDS_RESET - the driver needs to be able to retrieve
this when the configuration changed indicator is set.

> > I'll prepare a v2.
>