Virtualization - Jan 2015 - [RFC] virtio-mmio: Update the device to OASIS spec version

On Mon, 2015-01-19 at 18:36 +0000, Michael S. Tsirkin
wrote:
> > > > Well, not quite: as of now I've got legacy block device
driver happily
> > > > working on top of compliant (so v2 in MMIO speech) MMIO
device - the
> > > > transport if completely transparent here.
> > > 
> > > Spec says explicitly it's an illegal configuration.
> > 
> > What part of the spec exactly? The closest I can think of are 2.2.3,
6.2
> > and 6.3. Both legacy and spec-complaint MMIO transports will satisfy
all
> > requirements from those paragraphs, whether VIRTIO_F_VERSION_1 is
> > negotiated or not.
> 
> The parts that say VIRTIO_F_VERSION_1 MUST be set.
> I'll look up the chapter and verse later if you like.
That would be:

"6.1 Driver Requirements: Reserved Feature Bits
A driver MUST accept VIRTIO_F_VERSION_1 if it is offered. A driver MAY
fail to operate further if VIRTIO_F_VERSION_1 is not offered."

"6.2 Device Requirements: Reserved Feature Bits
A device MUST offer VIRTIO_F_VERSION_1. A device MAY fail to operate
further if VIRTIO_F_VERSION_1 is not accepted."

Both are perfectly clear and reasonable for me. And the MMIO transport
in both versions (pre-spec and the spec one) will meet those
requirements, as it was able to pass more than 32 features bits from the
very beginning.
> Can you please also add a comment?
> E.g. 
>      /*
>       * MMIO uses ID 0 to mean device not present.  Avoid probing
>       * the device further: it's sure to fail anyway.
>       */
There is a comment:
> On Fri, 2014-12-19 at 18:38 +0000, Pawel Moll wrote: 
> > +		/*
> > +		 * ID 0 means a dummy (placeholder) device, skip quietly
> > +		 * (as in: no error) with no further actions
> > +		 */
> 
But I'm can use your wording if you find it better.

Pawel

On Tue, Jan 20, 2015 at 05:18:23PM +0000, Pawel Moll
wrote:
> On Mon, 2015-01-19 at 18:36 +0000, Michael S. Tsirkin wrote:
> > > > > Well, not quite: as of now I've got legacy block
device driver happily
> > > > > working on top of compliant (so v2 in MMIO speech) MMIO
device - the
> > > > > transport if completely transparent here.
> > > > 
> > > > Spec says explicitly it's an illegal configuration.
> > > 
> > > What part of the spec exactly? The closest I can think of are
2.2.3, 6.2
> > > and 6.3. Both legacy and spec-complaint MMIO transports will
satisfy all
> > > requirements from those paragraphs, whether VIRTIO_F_VERSION_1 is
> > > negotiated or not.
> > 
> > The parts that say VIRTIO_F_VERSION_1 MUST be set.
> > I'll look up the chapter and verse later if you like.
> 
> That would be:
> 
> "6.1 Driver Requirements: Reserved Feature Bits
> A driver MUST accept VIRTIO_F_VERSION_1 if it is offered. A driver MAY
> fail to operate further if VIRTIO_F_VERSION_1 is not offered."
> 
> "6.2 Device Requirements: Reserved Feature Bits
> A device MUST offer VIRTIO_F_VERSION_1. A device MAY fail to operate
> further if VIRTIO_F_VERSION_1 is not accepted."
That, and there are a bunch of other changes we made in devices
as compared to virtio 0.9.
> 
> Both are perfectly clear and reasonable for me. And the MMIO transport
> in both versions (pre-spec and the spec one) will meet those
> requirements, as it was able to pass more than 32 features bits from the
> very beginning.
Good. But your current code will also try to support a mix: device that
uses the new transport underneath, but old layout and semantics on top.

This has no value: the spec does *not* define such devices
and it also does not match any hosts, so this just adds to support matrix.
And if linux supports it, someone *will* ship such a
device and we'll be stuck with it forever.

So please, detect out of spec devices and fail gracefully.
pci and s390 do this already.

> > Can you please also add a comment?
> > E.g. 
> >      /*
> >       * MMIO uses ID 0 to mean device not present.  Avoid probing
> >       * the device further: it's sure to fail anyway.
> >       */
> 
> There is a comment:
> 
> > On Fri, 2014-12-19 at 18:38 +0000, Pawel Moll wrote: 
> > > +		/*
> > > +		 * ID 0 means a dummy (placeholder) device, skip quietly
> > > +		 * (as in: no error) with no further actions
> > > +		 */
> > 
> But I'm can use your wording if you find it better.
> 
> Pawel
Either that or combine them in some way.

On Tue, 2015-01-20 at 17:44 +0000, Michael S. Tsirkin
wrote:
> Good. But your current code will also try to support a mix: device that
> uses the new transport underneath, but old layout and semantics on top.
It will not try to support the mix, but rather will not stop it from
working.
> This has no value: the spec does *not* define such devices
> and it also does not match any hosts, so this just adds to support matrix.
> And if linux supports it, someone *will* ship such a
> device and we'll be stuck with it forever.
>
> So please, detect out of spec devices and fail gracefully.
> pci and s390 do this already.
Ok, let it be. Will send v2 in a second.

Pawel