Re: [virtio] [PATCH 3/4] ccw: virtio revision vs. virtio feature bits

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

On Mon, Feb 09, 2015 at 09:38:12AM +0100, Cornelia Huck wrote:
> On Sun, 8 Feb 2015 11:21:18 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Oct 31, 2014 at 03:26:18PM +0100, Cornelia Huck wrote:
> > > We don't want the VERSION_1 feature bit unless at least revision 1
> > > has been negotiated. Make this a requirement.
> > > 
> > > VIRTIO-119
> > > 
> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > 
> > Sorry about the delay in response.
> > I think I disagree with this one.
> > 
> > I think this discusses how transitional devices work when
> > using the legacy interface?
> > In that case, this needs to go into a separate section,
> > explicitly labelled "legacy".
> 
> Hmm... but a legacy device does not actually know about VERSION_1, does
> it? Or am I misunderstanding what you mean?

you are describing here behaviour where negotiated
revision is 0, is that right?
If so this is transitional device/driver behaviour, isn't it?
We say so in
4.3.2.1 Setting the Virtio Revision
And the spec says:
	Requirements pertaining to transitional devices and drivers is contained
	in sections named ’Legacy Interface’




> > Looking into this deeper, how do devices "not accept"
> > bits when using a legacy interface?
> 
> A legacy device can't accept the VERSION_1 bit, as it only has 32
> feature bits. It will fail the ccw for the second half of the feature
> bits.

So are we talking about drivers which set guest feature bits
which are not set in the host feature bit mask?

We have this text:
	2.2.2 Device Requirements: Feature Bits
	The device MUST NOT offer a feature which requires another feature which
	was not offered. The device
	SHOULD accept any valid subset of features the driver accepts, otherwise
	it MUST fail to set the FEA-
	TURES_OK device status bit when the driver writes it.



> > 
> > Also, the wording here seems wrong to me.
> 
> Could you elaborate?

"if not at least" seems confusing. do you mean
"negotiated revision is 0"?
negociated isn't in my dictionary.

> > 
> 
> > > @@ -2656,6 +2656,14 @@ For communicating its supported features to the device, the driver
> > >  uses the CCW_CMD_WRITE_FEAT command, denoting a \field{features}/\field{index}
> > >  combination.
> > >  
> > > +\devicenormative{\paragraph}{Handling Device Features}{Virtio Transport Options / Virtio over channel I/O / Device Initialization / Handling Device Features}
> > > +
> > > +The device MUST NOT offer VIRTIO_F_VERSION_1 if not at least revision 1 has
> > > +been negociated.
> > > +
> > > +The device MUST NOT accept VIRTIO_F_VERSION_1 if not at least revision 1 has
> > > +been negociated.
> 
> I think this all boils down to whether revision handling is enough to
> designate a device as non-legacy, no?

It's not clear to me from this text.
4.3.2.1 Setting the Virtio Revision
says rev 0 is legacy interface, but there
is not conformance statement.
Would you like to add text that says negotiating rev 0
MUST force legacy interface?
Such text should be in a separate section marked "Legacy Interface".

We can add text that says drivers must not accept VIRTIO_F_VERSION_1
in this mode, and devices must not offer VIRTIO_F_VERSION_1.
Seems generic, non ccw specific, does it not?
Such text should also be in a separate section marked "Legacy Interface".


> > > +
> > >  \subsubsection{Device Configuration}\label{sec:Virtio Transport Options / Virtio over channel I/O / Device Initialization / Device Configuration}
> > >  
> > >  The device's configuration space is located in host memory.