qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Cornelia Huck <cohuck@redhat.com>
To: Halil Pasic <pasic@linux.ibm.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Halil Pasic <pasic@linux.ibm.com>,
	qemu-s390x@nongnu.org, qemu-devel@nongnu.org
Cc: Christian Borntraeger <borntraeger@de.ibm.com>,
	Thomas Huth <thuth@redhat.com>,
	Richard Henderson <richard.henderson@linaro.org>,
	David Hildenbrand <david@redhat.com>
Subject: Re: [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern()
Date: Fri, 29 Oct 2021 16:53:25 +0200	[thread overview]
Message-ID: <87tugzc26y.fsf@redhat.com> (raw)
In-Reply-To: <20211028220017.930806-2-pasic@linux.ibm.com>

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>

> +{
> +    /*
> +     * 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? Devices may modify some of their
behaviour or features, depending on what features they are called with,
and we will be calling this one again later with what is likely a
different feature set. Also, the return code is not checked.

Maybe introduce a new function that sets guest_features directly and
errors out if the features are not set in host_features? 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.

> +}
> +
>  /*
>   * Only devices that have already been around prior to defining the virtio
>   * standard support legacy mode; this includes devices not specified in the

  reply	other threads:[~2021-10-29 14:56 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-28 22:00 [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio Halil Pasic
2021-10-28 22:00 ` [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern() Halil Pasic
2021-10-29 14:53   ` Cornelia Huck [this message]
2021-11-12 15:42     ` Halil Pasic
2021-11-12 15:55       ` Cornelia Huck
2021-11-12 16:36         ` Halil Pasic
2021-10-28 22:00 ` [RFC PATCH v1 2/3] virtio-ccw: use virtio_force_modern Halil Pasic
2021-10-28 22:00 ` [RFC PATCH v1 3/3] virtio-pci: use virtio_force_modern() Halil Pasic
2021-11-05  7:42 ` [RFC PATCH v1 0/3] virtio: early detect 'modern' virtio Michael S. Tsirkin
2021-11-09 10:12   ` Halil Pasic

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87tugzc26y.fsf@redhat.com \
    --to=cohuck@redhat.com \
    --cc=borntraeger@de.ibm.com \
    --cc=david@redhat.com \
    --cc=mst@redhat.com \
    --cc=pasic@linux.ibm.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-s390x@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=thuth@redhat.com \
    --subject='Re: [RFC PATCH v1 1/3] virtio: introduce virtio_force_modern()' \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).