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
