qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern()


From: Cornelia Huck
Subject: Re: [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern()
Date: Fri, 12 Nov 2021 16:55:10 +0100
User-agent: Notmuch/0.33.1 (https://notmuchmail.org)

On Fri, Nov 12 2021, Halil Pasic <pasic@linux.ibm.com> wrote:

> On Fri, 29 Oct 2021 16:53:25 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Fri, Oct 29 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).
>> >
>> > 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         | 12 ++++++++++++
>> >  include/hw/virtio/virtio.h |  1 +
>> >  2 files changed, 13 insertions(+)
>> >
>> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> > index 3a1f6c520c..75aee0e098 100644
>> > --- a/hw/virtio/virtio.c
>> > +++ b/hw/virtio/virtio.c
>> > @@ -3281,6 +3281,18 @@ void virtio_init(VirtIODevice *vdev, const char 
>> > *name,
>> >      vdev->use_guest_notifier_mask = true;
>> >  }
>> >  
>> > +void  virtio_force_modern(VirtIODevice *vdev)  
>> 
>> <bikeshed> I'm not sure I like that name. We're not actually forcing the
>> device to be modern, we just set an early indication in the device
>> before proper feature negotiation has finished. Maybe
>> virtio_indicate_modern()? </bikeshed>
>
>
> I don't like virtio_indicate_modern(dev) form object orientation
> perspective. In an OO language one would write it like
> dev.virtio_indicate_modern()
> which would read like the device should indicate modern to somebody.

I think that is actually what happens: we indicate that it is a modern
device to the code making the endianness decisions.

>
> In my opinion what happens is that we want to disable the legacy
> interface if it is exposed by the device, or in other words instruct the
> device that should act (precisely and exclusively) according to the
> interface specification of the modern interface.

I don't see us disabling anything; the driver has already chosen what
they want, and we simply need to make sure that all code honours that
decision.

>
> Maybe we can find a better name than force_modern, but I don't think
> indicate_modern is a better name.
>
>> 
>> > +{
>> > +    /*
>> > +     * This takes care of the devices that implement config space access
>> > +     * in QEMU. For vhost-user and similar we need to make sure the 
>> > features
>> > +     * are actually propagated to the device implementing the config 
>> > space.
>> > +     *
>> > +     * A VirtioDeviceClass callback may be a good idea.
>> > +     */
>> > +    virtio_set_features(vdev, (1ULL << VIRTIO_F_VERSION_1));  
>> 
>> Do we really need/want to do the whole song-and-dance for setting
>> features, just for setting VERSION_1? 
>
> When doing the whole song-and-dance the chance is higher that the
> information will propagate to every place it needs to reach. For
> example to the acked_features of vhost_dev. I've just posted a v2 RFC.
> It should not be hard to see what I mean after examining that RFC.
>
>> Devices may modify some of their
>> behaviour or features, depending on what features they are called with,
>
> I believe, if this is the case, we want the behavior that corresponds to
> VERSION_1 set, i.e. 'modern'. So in my understanding this is rather good
> than bad.
>
>> and we will be calling this one again later with what is likely a
>> different feature set. 
>
> That is true, but the driver is allowed to set the features multiple
> times, and since transports only support piecemeal access to the
> features (32 bits at a time), I guess this is biz as usual.

Also see my comment in the v2: I'm not sure how well tested that
actually is.

>
>>Also, the return code is not checked.
>> 
>
> That is true! It might be a good idea to log an error. Unfortunately I
> don't think there is anything else we can sanely do.
>
>> Maybe introduce a new function that sets guest_features directly and
>> errors out if the features are not set in host_features? 
>
> See above.
>
>> If we try to
>> set VERSION_1 here despite the device not offering it, we are in a
>> pickle anyway, as we should not have gotten here if we did not offer it,
>> and we really should moan and fail in that case.
>
> I agree about the moan part. I'm not sure what is the best way to
> 'fail'. Maybe we should continue this discussion in the v2 thread.

Yeah, let's continue there, since that code is a bit different.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]