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

[Halil Pasic]

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.

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

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