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

[Halil Pasic]

On Thu, 07 Oct 2021 17:25:52 +0200
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:

> On Thu, Oct 07 2021, Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:
> 
> > On Thu, 07 Oct 2021 13:52:24 +0200
> > Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> >  
> >> On Wed, Oct 06 2021, Halil Pasic <pasic@xxxxxxxxxxxxx> 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've added in some quotation marks and ellipsis marks. Does that look
good for you?

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

Will add it to the end.

> 
> >  
> >> 
> >> 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@xxxxxxxxxxxxx>
> >> > 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@xxxxxxxxxx
> >> > ---
> >> >  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.
>

Nod.
 
> >>   
> >> > +	 */
> >> > +	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?
> 
> 

I will wait then till end of day before sending out a v2.

Thank you very much!

Regards,
Halil