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