Re: [virtio-dev] [PATCH 4/4] ccw: allow WRITE_STATUS to fail

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

On Wed, 4 Feb 2015 00:12:06 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Oct 31, 2014 at 03:26:19PM +0100, Cornelia Huck wrote:
> > We want to be able to reject a status (e.g. FEATURES_OK if the device
> > can't work with the features negotiated).
> 
> I guess the reason it's needed is because ccw doesn't have
> READ_STATUS?

Yes, because we need to introduce channel commands for things that
pci/mmio just can do out of the box by reading/writing some memory
area. And allowing failures is the least invasive way to get the
effects we want.

> 
> 
> > The easiest way to do that
> > is to allow the device to fail the WRITE_STATUS command.
> > 
> > VIRTIO-116
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>

> > diff --git a/content.tex b/content.tex
> > index 38ae49e..490823e 100644
> > --- a/content.tex
> > +++ b/content.tex
> > @@ -2627,6 +2627,21 @@ The driver changes the status of a device via the
> >  CCW_CMD_WRITE_STATUS command, which transmits an 8 bit status
> >  value.
> >  
> > +The device is allowed to reject a status: For example, it might fail to accept
> > +the FEATURES_OK status bit during device initialization.
> 
> I kind of dislike it that we are adding this new concept of
> "rejecting a status". The generic spec says:
> 
> 	The device SHOULD accept any valid subset
> 	of features the driver accepts, otherwise it MUST fail to set the
> 	FEATURES_OK \field{device status} bit when the driver writes it.
> 
> so devices accept features, not status, I think applying concept of
> accept/reject to status is confusing.

It basically comes from the concept of "rejecting a ccw with unit
check".

> Let's say that device fails to set status to requested value.
> For example:
> 
> 
> 
> 	As described in <cross reference> devices sometimes fail to set
> 	the \field{device status} to value written by the driver.

No objection from me, it basically ends up meaning the same thing.

> 
> > +
> > +\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
> 
> add:         setting device failed to set the status and that
> 
> > +the status
> > +field
> > +retained its previous value.
> > +
> > +\devicenormative{\paragraph}{Communicating Status Information}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Communicating Status Information}
> > +
> 
> > +If the driver attempts to set a status that the device does not accept, 
> 
> instead:
> 	If the device fails to set the \field{device status} field
> 	to the value written by driver
> 
> 
> > + the
> > +device MUST leave the status field unchanged and MUST post a unit check
> > +with command reject.
> > +
> >  \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

Your suggested changes look OK to me. I'll prepare an updated patch.