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

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

On Mon, 9 Feb 2015 15:49:36 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:

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

But a legacy device/driver can't negotiate version 0, as it doesn't
know the respective command.

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

But here you were asking about legacy devices, weren't you? And these
simply don't have the second half of feature bits, and of course don't
know of the FEATURE_OK bit.

And this section is talking about feature bit dependencies, isn't 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"?

"if not at least revision 1 has been negotiated" == "no revision or
revision 0 has been negotiated" (so revision 0, as this is not about
legacy devices/drivers)

I don't know what's confusing about "at least"?

> negociated isn't in my dictionary.

Sigh, that's an obvious typo.

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

Hm, I meant "standard compliant device/driver using the legacy
interface". In practice, revision 0 is unlikely to be ever negotiated.

This all boils down to what a legacy device/driver is supposed to be.
For me, it is a device/driver not knowing about the standard.
Negotiating any revision (including 0) indicates it cares about the
standard. Talking about revisions in the legacy sections therefore
sounds wrong to me.

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

It is specific to ccw revisions, unless we describe a general revision
mechanism with specific implementations.

> Such text should also be in a separate section marked "Legacy Interface".

Can we somehow withdraw the ballot? It seems this whole topic needs
more discussion, and I'm currently in the middle of a move and don't
have a head for in-depth discussions.