Re: [PATCH 1/1] virtio: write back F_VERSION_1 before validate

[Cornelia Huck <cohuck@redhat.com>]

On Thu, Oct 07 2021, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Thu, 07 Oct 2021 13:52:24 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Wed, Oct 06 2021, Halil Pasic <pasic@linux.ibm.com> wrote:
>> 
>> > The virtio specification virtio-v1.1-cs01 states: Transitional devices
>> > MUST detect Legacy drivers by detecting that VIRTIO_F_VERSION_1 has not
>> > been acknowledged by the driver.  This is exactly what QEMU as of 6.1
>> > has done relying solely on VIRTIO_F_VERSION_1 for detecting that.
>> >
>> > However, the specification also says: driver MAY read (but MUST NOT
>> > write) the device-specific configuration fields to check that it can
>> > support the device before setting FEATURES_OK.  
>> 
>> Suggest to put the citations from the spec into quotes, so that they are
>> distinguishable from the rest of the text.
>
> For the record: I basically took Michael's description, the one which you
> said you prefer, with some minor changes.

Well I did look at what the text said, not the details in the formatting...

>
> This is one of the changes, which renders this a paraphrase and not a
> quote. Michael didn't use quotation marks so I was not sure it is was
> a word by word quote anyway. It was. But the spec depends on "During this
> step" which does not make any sense without the context. That is why I made
> the end of step explicit.

I still think that would be nicer while using some quotation marks, even
if you are just doing a partial quote.

In the first paragraph, however, we really should mark the quote
properly. It gave me a stop when I first read it.

>
> I think we are fine without quotation marks. Those who care can read the
> spec.
>
>> 
>> >
>> > In that case, any transitional device relying solely on
>> > VIRTIO_F_VERSION_1 for detecting legacy drivers will return data in
>> > legacy format.  In particular, this implies that it is in big endian
>> > format for big endian guests. This naturally confuses the driver which
>> > expects little endian in the modern mode.
>> >
>> > It is probably a good idea to amend the spec to clarify that
>> > VIRTIO_F_VERSION_1 can only be relied on after the feature negotiation
>> > is complete. However, we already have regression so let's try to address  
>> 
>> s/regression/a regression/
>> 
>
> Yes. Was like this in the original. Will change
>
>> > it.  
>> 
>> Maybe mention what the regression is?
>
> How about the following?
>
> The regressions affect the VIRTIO_NET_F_MTU feature of virtio-net and the
> VIRTIO_BLK_F_BLK_SIZE feature of virtio-blk for BE guests when virtio
> 1.0 is used on both sides. The latter renders virtio-blk unusable with
> DASD backing, because things simply don't work with the default.

Sounds good to me.

>
>> 
>> Also mention that we use this workaround for modern on BE only?
>
> We have that already, don't we. The sentence that starts with "In
> particular". The regression description should reinforce that
> sufficiently IMHO.

No strong opinion here. Anyone else?

>
>> 
>> >
>> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
>> > Fixes: 82e89ea077b9 ("virtio-blk: Add validation for block size in config space")
>> > Fixes: fe36cbe0671e ("virtio_net: clear MTU when out of range")
>> > Reported-by: markver@us.ibm.com
>> > ---
>> >  drivers/virtio/virtio.c | 10 ++++++++++
>> >  1 file changed, 10 insertions(+)
>> >
>> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> > index 0a5b54034d4b..494cfecd3376 100644
>> > --- a/drivers/virtio/virtio.c
>> > +++ b/drivers/virtio/virtio.c
>> > @@ -239,6 +239,16 @@ static int virtio_dev_probe(struct device *_d)
>> >  		driver_features_legacy = driver_features;
>> >  	}
>> >  
>> > +	/*
>> > +	 * Some devices detect legacy solely via F_VERSION_1. Write
>> > +	 * F_VERSION_1 to force LE for these when needed.  
>> 
>> "...to force LE config space accesses before FEATURES_OK for these when
>> needed (BE)."
>> 
>> ?
>
> Can do, but I would rather omit the (BE) at the end. All the conditions
> are necessary:
> * have validate callback
> * device offered VERSION_1
> * virtio legacy is be
>

Ok, let's use that without the trailing BE.

>> 
>> > +	 */
>> > +	if (drv->validate && !virtio_legacy_is_little_endian()
>> > +			  && BIT_ULL(VIRTIO_F_VERSION_1) & device_features) {  
>> 
>> Nit: putting device_features first would read more naturally to me.
>> 
>
> Can do.
>
>> > +		dev->features = BIT_ULL(VIRTIO_F_VERSION_1);
>> > +		dev->config->finalize_features(dev);
>> > +	}
>> > +
>> >  	if (device_features & (1ULL << VIRTIO_F_VERSION_1))
>> >  		dev->features = driver_features & device_features;
>> >  	else  
>> 
>> Patch LGTM.
>> 
>> 
>
> Thanks for having a look. If you are fine with the proposed solution
> please tell me, so I can send out a v2.

No further comments other than what I wrote above, but maybe others have
comments as well?