lkml.org 
[lkml]   [2021]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 1/1] virtio: write back F_VERSION_1 before validate
Date
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?

\
 
 \ /
  Last update: 2021-10-07 17:33    [W:0.068 / U:0.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site