Re: [RFC PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM

[Michael S. Tsirkin]

On Mon, Feb 07, 2022 at 04:05:16PM +0100, Halil Pasic wrote:
> On Mon, 07 Feb 2022 14:41:58 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Mon, Feb 07 2022, Daniel Henrique Barboza <danielhb413@gmail.com> wrote:
> > 
> > > On 2/3/22 13:45, Halil Pasic wrote:  
> > >> Unlike most virtio features ACCESS_PATFORM is considered mandatory, i.e. 
> > >>  
> > 
> > s/ACCESS_PATFORM/ACCESS_PLATFORM/
> 
> Will fix.
> 
> > 
> > >> the driver must accept it if offered by the device. The virtio
> > >> specification says that the driver SHOULD accept the ACCESS_PLATFORM
> > >> feature if offered, and that the device MAY fail to operate if
> > >> ACCESS_PLATFORM was offered but not negotiated.
> > >> 
> > >> While a SHOULD ain't exactly a MUST, we are certainly allowed to fail
> > >> the device when the driver fences ACCESS_PLATFORM. With commit  
> > >
> > >
> > > I believe a link to the virtio specification where this is being 
> > > mentioned would
> > > be good to have in the commit message.  
> > 
> > It's in section 6.1 "Driver Requirements: Reserved Feature Bits": "A
> > driver SHOULD accept VIRTIO_F_ACCESS_PLATFORM if it is offered" and
> > section 6.2 "Device Requirements: Reserved Feature Bits": "A device MAY
> > fail to operate further if VIRTIO_F_ACCESS_PLATFORM is not accepted."
> > 
> > That said, I'm not sure the wording in the spec translates to
> > "mandatory"... if the driver fails to accept the bit, the device can
> > choose to not work with the driver, but it's not forced to.
> 
> I didn't mean to claim that the spec makes this feature "mandatory", and
> this is why I paraphrased the spec. IMHO it is QEMU that considers it
> mandatory.

this:
A device MAY
fail to operate further if VIRTIO_F_ACCESS_PLATFORM is not accepted

is the hint here.

> > There are
> > other instances where the device may reject FEATURES_OK (e.g. when the
> > driver does not accept a feature that is a pre-req for another feature),
> > I'd say it is up to the device whether something is mandatory or not. If
> > the device/setup cannot work without it, it certainly is mandatory, but
> > the driver only knows when FEATURES_OK is rejected without the feature.
> 
> Right but for the guys that write the drivers it is of interest to know
> what level of interoperability can  one can keep if certain
>       features are
> not implemented. Usually it is safe to fence delay implementing
> features, as long as the support for the features is implemented in the
> order mandated by the spec.
> 
> > 
> > OTOH, the decision to make it mandatory is certainly sound, and covered
> > by the spec. As the driver must be prepared for the device failing to
> > accept FEATURES_OK, we can make it mandatory here -- we should just not
> > say that it is considered mandatory from a spec standpoint. The spec
> > allows to make it mandatory, and we make it mandatory in our
> > implementation.
> 
> Right. Was never my intention to say that it is considered mandatory
> by the spec. I guess the spec considers it less optional than the
> run of the mill features.

It would be nice to have a security considerations section.

The point is that within guest, with ACCESS_PLATFORM it should be safe
to assume that device can be passed through to nested guests or
userspace.



> Should I change the first sentence to something like "Unlike most virtio
> features ACCESS_PATFORM is considered mandatory by QEMU, i.e. the driver
> must accept it if offered by the device."
> 
> [..]
> 
> Regards,
> Halil