Re: [RFC PATCH v2 1/5] virtio: introduce virtio_force_modern()

[Halil Pasic]

On Mon, 15 Nov 2021 17:57:28 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Mon, Nov 15 2021, Halil Pasic <pasic@linux.ibm.com> wrote:
> 
> > On Fri, 12 Nov 2021 16:37:20 +0100
> > Cornelia Huck <cohuck@redhat.com> wrote:
> >  
> >> On Fri, Nov 12 2021, Halil Pasic <pasic@linux.ibm.com> wrote:
> >>   
> >> > Legacy vs modern should be detected via transport specific means. We
> >> > can't wait till feature negotiation is done. Let us introduce
> >> > virtio_force_modern() as a means for the transport code to signal
> >> > that the device should operate in modern mode (because a modern driver
> >> > was detected).
> >> >
> >> > A new callback is added for the situations where the device needs
> >> > to do more than just setting the VIRTIO_F_VERSION_1 feature bit. For
> >> > example, when vhost is involved, we may need to propagate the features
> >> > to the vhost device.
> >> >
> >> > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> >> > ---
> >> >
> >> > I'm still struggling with how to deal with vhost-user and co. The
> >> > problem is that I'm not very familiar with the life-cycle of, let us
> >> > say, a vhost_user device.
> >> >
> >> > Looks to me like the vhost part might be just an implementation detail,
> >> > and could even become a hot swappable thing.
> >> >
> >> > Another thing is, that vhost processes set_features differently. It
> >> > might or might not be a good idea to change this.
> >> >
> >> > Does anybody know why don't we propagate the features on features_set,
> >> > but under a set of different conditions, one of which is the vhost
> >> > device is started?
> >> > ---
> >> >  hw/virtio/virtio.c         | 13 +++++++++++++
> >> >  include/hw/virtio/virtio.h |  2 ++
> >> >  2 files changed, 15 insertions(+)
> >> >    
> >> 
> >> Did you see my feedback in
> >> https://lore.kernel.org/qemu-devel/87tugzc26y.fsf@redhat.com/? I think
> >> at least some of it still applies.
> >>   
> >
> > Sure. My idea was to send out a v2 first which helps us think about the
> > bigger picture, and then answer that mail. Now I realize I should have
> > sent the response first, and then the v2 immediately afterwards.
> >  
> >> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> > index 3a1f6c520c..26db1b31e6 100644
> >> > --- a/hw/virtio/virtio.c
> >> > +++ b/hw/virtio/virtio.c
> >> > @@ -3281,6 +3281,19 @@ void virtio_init(VirtIODevice *vdev, const char 
> >> > *name,
> >> >      vdev->use_guest_notifier_mask = true;
> >> >  }
> >> >  
> >> > +void  virtio_force_modern(VirtIODevice *vdev)    
> >> 
> >> I'd still prefer to call this virtio_indicate_modern: we don't really
> >> force anything; the driver has simply already decided that it will use
> >> the modern interface and we provide an early indication in the features
> >> so that code looking at them makes the right decisions.  
> >
> > I tried to explain my dislike for virtio_indicate_modern in my response
> > to that email. In somewhat different words: IMHO indication is about an
> > external observer and has a symbolic dimension to it. Please see
> > https://www.oxfordlearnersdictionaries.com/definition/english/indicate
> > This function is about changing the behavior of the device. Its
> > post-condition is: the device acts compliant to virtio 1.0 or higher.  
> 
> My personal preference is "indicate", I don't like "force". I don't want
> a semantics discussion; I'll leave the decision to the virtio
> maintainers.

I can't really follow your train of thought, but I'm OK with the outcome.

> 
> >  
> >>   
> >> > +{
> >> > +    VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >> > +
> >> > +    virtio_add_feature(&vdev->guest_features, VIRTIO_F_VERSION_1);
> >> > +    /* Let the device do it's normal thing. */
> >> > +    virtio_set_features(vdev, vdev->guest_features);    
> >> 
> >> I don't think this is substantially different from setting VERSION_1
> >> only: At least the callers you introduce call this during reset,
> >> i.e. when guest_features is 0 anyway.   
> >
> > I agree. Just wanted to be conservative, and preserve whatever is there.
> >
> >  
> >> We still have the whole processing
> >> that is done after feature setting that may have effects different from
> >> what the ultimate feature setting will give us.  
> >
> > Yes, this is an intermediate state. As I pointed out, intermediate states
> > are necessary.  
> 
> Why? We just want VERSION_1 so that the checks work, why do we need to
> fiddle with other settings? We only need to propagate it to e.g. vhost.
>

Intermediate states are necessary, because transports can not set
features as an atomic operation. That is, if both feature bits > 31 and
<= 31 need to be set, an intermediate state is necessary.

I think virtio_set_features() is what computer science calls a "mutator
method" (for reference see:
https://en.wikipedia.org/wiki/Mutator_method). Do we agree that
virtio_set_features() is a mutator?
 

Not using the mutator method is considered bad practice AFAIK. Just to
get this perfectly clear you want me to not use virtio_set_features()
here, right?

> >  
> >> While I don't think
> >> calling set_features twice is forbidden, that sequence is likely quite
> >> untested, and I'm not sure we can exclude side effects.  
> >
> > I can't disagree with that. But IMHO we can just say: such problems, if
> > any, are bugs that need to be fixed.  
> 
> Well, what about first fixing the endianness bugs, before we potentially
> open up a can of worms?

That can of worms is already open, because the driver can set features
several times. I'm not exactly clear on what exactly is your concern
here. So let us have a look at what virtio_set_features() does:

(1) Makes sure the device is in a state when features can be set
(!VIRTIO_CONFIG_S_FEATURES_OK)
(2) Does basic validation and filtering (accepts only features that are
indeed offered).
(3) Calls the callback of the device class ->set_features() if any.
(4) If VIRTIO_RING_F_EVENT_IDX does virtio_init_region_cache() for
relevant queues.
(5) Manages start_on_kick for legacy.

In my opinion points (1) and (2) are desirable and could help catching
bugs. Points (4) and (5) are irrelevant but conditional and obviously do
not hurt.

So I assume your problem is with the call to
VirtioDeviceClass.set_features(), right?


> 
> >
> > I think not doing the whole song-and-dance is conceptually more
> > problematic because it is more likely to lead to inconsistent internal
> > state. For example check out: vhost acked_features <-> guest_features.  
> 
> What is wrong with verifying with one single feature?
> 

I don't think verifying with one single features is wrong. I don't think
I ever said it is wrong. And I'm not sure what you mean by this.

What is in my opinion not nice, and arguably an encapsulation and
layering violation is tweaking the features without calling
VirtioDeviceClass.set_features(). In my opinion, a device implementation
can reasonably assume that the features didn't change if the
implementation provided a set_features() callback, and this callback was
not called. IMHO the callback is there, so that the device implementation
can notified about feature changes. Or do you disagree?

For something like vhost, when parts of the device don't live in the
qemu process and have no access to vdev->guest_features, this is very
reasonable: the QEMU external part will "cache" the relevant features,
but it ain't prohibited in the general case either.

What needs to be done and what may be done by the device on setting the
features isnot over-specified. In the sense of virtio 1.0 or higher the
changes have to take effect when feature negotiation is completed. That
is when FEATURES_OK is set. For pre 1.0 virtio, it is for me hard to
tell. I believe "feature negotiation completed" is tightly related to the
QEMU concept of "started", although we also have "DRIVER_OK" which also
plays a role.

The validation of the features also takes place when FEATURES_OK is set
and not before.

I really can't put my finger on what are you concerned about. In practice
guest_features goes from 0 to VIRTIO_F_VERSION_1. So even if the
->set_features callback were to do some funky feature enabling and
disabling, it would only enable the functionality it associates with
VIRTIO_F_VERSION_1 a.k.a. "modern", which is exactly what we want, and
disable nothing, because no bit went from 1 to 0. Right?

Yes, adding an unsolicited change to the feature set when transport
detects a modern driver that is trying to drive a modern device is
something unprecedented. But that is inherent. We want a transitional
device to behave according to "features == 0" (legacy) -> "features ==
VIRTIO_F_VERSION_1" (modern) -> "features negotiated and final" (modern
and features negotiated).

Michael, can you please help us with this dispute?

Regards,
Halil