Message ID | 20230118164359.1523760-3-eperezma@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:eb09:0:0:0:0:0 with SMTP id s9csp2443147wrn; Wed, 18 Jan 2023 08:50:30 -0800 (PST) X-Google-Smtp-Source: AMrXdXuCAXN9odfXcOM4argHUrcL8/WQtEXAkjWCIL4o+3rOGIASj5Cq4gg4NsQ/x53g/KhEoqvA X-Received: by 2002:a17:90b:1209:b0:229:472d:af43 with SMTP id gl9-20020a17090b120900b00229472daf43mr7836726pjb.44.1674060630549; Wed, 18 Jan 2023 08:50:30 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1674060630; cv=none; d=google.com; s=arc-20160816; b=mVa23kIv9sQv9vZwR86VNJF29gXHd1l3XjQCF/OJCpGXINjA12H7RukxYPrxehv5Dh 2/61ofmhz5ShLeOA62wnFtnAW4YGB1/0i+xigJwKN8JR8ZQdTnFl5B+mmqxiFmnJ57Z4 fXVE6LJd5sud8B5Rpawq9Nxem9bV4zG45Q8mfX5TsmsCIwS9Mf7PbateKxI7YVHMiIxS KybrbFb+gwflRD+/KuJVPaRGdsmzon9jPOUEcTpIvDN97SfZZWo9NsrcncrhdTcFoT1i SX2dhhlH3ir5Kz8z++kHhTIDqWqTScODP0V4jMEPydef4SNFobbMgzHeKkyKND036T+K 83XQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=wHqIpzQSkuEl19TQY/3XES0k5HPtR7PzJw91HkmdGrE=; b=BLktwu/IlRTQGPiNA1cg+K7NS4HV7X2x8W8qPS4DDhUNU+us5F7FZklNBXFj8ITn+q BQMygxxYfClA3kDQMTefpEGViXxUw38ZEPlUjjlZ++2EktFdaCBsR4sMepfsPHrgEPnt tci3x4kdeSTdZUNToz5IvGwc2CKUrj/HVDcIk2Jx0QptUkGJo7WRaeZb3oQBB7ECt2gO 31/0Esf5j4fZXqRlA4E7c3rX6ivHz6FQn/vcjhUpLvXfNoX5krXAtAAZ5W303e83puvR cRi4ROmqck6gIR+eRFRoS6737JsFNRn7JroFmOnqkk4aDHN/IUvKneWVZOvHg2j4iSZi FPQA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=UK3L84TP; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id br3-20020a17090b0f0300b002265fe84432si2614925pjb.44.2023.01.18.08.50.18; Wed, 18 Jan 2023 08:50:30 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=UK3L84TP; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229917AbjARQqA (ORCPT <rfc822;pfffrao@gmail.com> + 99 others); Wed, 18 Jan 2023 11:46:00 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48764 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229986AbjARQpf (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 18 Jan 2023 11:45:35 -0500 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6685D582AA for <linux-kernel@vger.kernel.org>; Wed, 18 Jan 2023 08:44:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1674060253; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wHqIpzQSkuEl19TQY/3XES0k5HPtR7PzJw91HkmdGrE=; b=UK3L84TPIeY/Ia5ttYrHvRvXpk44Ep/TNNwL1+4BqEOdGMXerU3ce6WjGRI0lTHbgmZAxb KxKAYBprKc8q6tTnxd76l26gQdoxF5xRGH2t2CvJSXCwhLDRdSG7VRldN63zAV39JwYpF0 b6DlrpbkZevbOF+rscsaKqzlovpnrmM= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-264-qnHa-CyiOPyF5XEt1aLMuQ-1; Wed, 18 Jan 2023 11:44:12 -0500 X-MC-Unique: qnHa-CyiOPyF5XEt1aLMuQ-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 979113804527; Wed, 18 Jan 2023 16:44:08 +0000 (UTC) Received: from eperezma.remote.csb (unknown [10.39.192.141]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6EE30140EBF6; Wed, 18 Jan 2023 16:44:06 +0000 (UTC) From: =?utf-8?q?Eugenio_P=C3=A9rez?= <eperezma@redhat.com> To: mst@redhat.com Cc: leiyang@redhat.com, Laurent Vivier <lvivier@redhat.com>, sgarzare@redhat.com, jasowang@redhat.com, Zhu Lingshan <lingshan.zhu@intel.com>, virtualization@lists.linux-foundation.org, si-wei.liu@oracle.com, linux-kernel@vger.kernel.org, lulu@redhat.com, Gautam Dawar <gdawar@xilinx.com>, alvaro.karsz@solid-run.com Subject: [PATCH 2/2] vringh: fetch used_idx from vring at vringh_init_iotlb Date: Wed, 18 Jan 2023 17:43:59 +0100 Message-Id: <20230118164359.1523760-3-eperezma@redhat.com> In-Reply-To: <20230118164359.1523760-1-eperezma@redhat.com> References: <20230118164359.1523760-1-eperezma@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1755379799988378420?= X-GMAIL-MSGID: =?utf-8?q?1755379799988378420?= |
Series |
Fix expected set_vq_state behavior on vdpa_sim
|
|
Commit Message
Eugenio Perez Martin
Jan. 18, 2023, 4:43 p.m. UTC
Starting from an used_idx different than 0 is needed in use cases like
virtual machine migration. Not doing so and letting the caller set an
avail idx different than 0 causes destination device to try to use old
buffers that source driver already recover and are not available
anymore.
While callers like vdpa_sim set avail_idx directly it does not set
used_idx. Instead of let the caller do the assignment, fetch it from
the guest at initialization like vhost-kernel do.
To perform the same at vring_kernel_init and vring_user_init is left for
the future.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
drivers/vhost/vringh.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
Comments
On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote: > > Starting from an used_idx different than 0 is needed in use cases like > virtual machine migration. Not doing so and letting the caller set an > avail idx different than 0 causes destination device to try to use old > buffers that source driver already recover and are not available > anymore. > > While callers like vdpa_sim set avail_idx directly it does not set > used_idx. Instead of let the caller do the assignment, fetch it from > the guest at initialization like vhost-kernel do. > > To perform the same at vring_kernel_init and vring_user_init is left for > the future. > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > drivers/vhost/vringh.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > index 33eb941fcf15..0eed825197f2 100644 > --- a/drivers/vhost/vringh.c > +++ b/drivers/vhost/vringh.c > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh, > return 0; > } > > +/** > + * vringh_update_used_idx - fetch used idx from driver's used split vring > + * @vrh: The vring. > + * > + * Returns -errno or 0. > + */ > +static inline int vringh_update_used_idx(struct vringh *vrh) > +{ > + return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx); > +} > + > /** > * vringh_init_iotlb - initialize a vringh for a ring with IOTLB. > * @vrh: the vringh to initialize. > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features, > struct vring_avail *avail, > struct vring_used *used) > { While at this, I wonder if it's better to have a dedicated parameter for last_avail_idx? > - return vringh_init_kern(vrh, features, num, weak_barriers, > - desc, avail, used); > + int r = vringh_init_kern(vrh, features, num, weak_barriers, desc, > + avail, used); > + > + if (r != 0) > + return r; > + > + /* Consider the ring not initialized */ > + if ((void *)desc == used) > + return 0; I don't understand when we can get this (actually it should be a bug of the caller). Thanks > + > + return vringh_update_used_idx(vrh); > + > } > EXPORT_SYMBOL(vringh_init_iotlb); > > -- > 2.31.1 >
On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote: > > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > Starting from an used_idx different than 0 is needed in use cases like > > virtual machine migration. Not doing so and letting the caller set an > > avail idx different than 0 causes destination device to try to use old > > buffers that source driver already recover and are not available > > anymore. > > > > While callers like vdpa_sim set avail_idx directly it does not set > > used_idx. Instead of let the caller do the assignment, fetch it from > > the guest at initialization like vhost-kernel do. > > > > To perform the same at vring_kernel_init and vring_user_init is left for > > the future. > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > drivers/vhost/vringh.c | 25 +++++++++++++++++++++++-- > > 1 file changed, 23 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > > index 33eb941fcf15..0eed825197f2 100644 > > --- a/drivers/vhost/vringh.c > > +++ b/drivers/vhost/vringh.c > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh, > > return 0; > > } > > > > +/** > > + * vringh_update_used_idx - fetch used idx from driver's used split vring > > + * @vrh: The vring. > > + * > > + * Returns -errno or 0. > > + */ > > +static inline int vringh_update_used_idx(struct vringh *vrh) > > +{ > > + return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx); > > +} > > + > > /** > > * vringh_init_iotlb - initialize a vringh for a ring with IOTLB. > > * @vrh: the vringh to initialize. > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features, > > struct vring_avail *avail, > > struct vring_used *used) > > { > > While at this, I wonder if it's better to have a dedicated parameter > for last_avail_idx? > I also had that thought. To directly assign last_avail_idx is not a specially elegant API IMO. Maybe expose a way to fetch used_idx from device vring and pass used_idx as parameter too? > > - return vringh_init_kern(vrh, features, num, weak_barriers, > > - desc, avail, used); > > + int r = vringh_init_kern(vrh, features, num, weak_barriers, desc, > > + avail, used); > > + > > + if (r != 0) > > + return r; > > + > > + /* Consider the ring not initialized */ > > + if ((void *)desc == used) > > + return 0; > > I don't understand when we can get this (actually it should be a bug > of the caller). > You can see it in vdpasim_vq_reset. Note that to consider desc == 0 to be an uninitialized ring is a bug IMO. QEMU considers it that way also, but the standard does not forbid any ring to be at address 0. Especially if we use vIOMMU. So I think the best way to know if we can use the vringh is either this way, or provide an explicit "initialized" boolean attribute. Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to add new attributes. Thanks! > Thanks > > > + > > + return vringh_update_used_idx(vrh); > > + > > } > > EXPORT_SYMBOL(vringh_init_iotlb); > > > > -- > > 2.31.1 > > >
On Thu, Jan 19, 2023 at 4:11 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > > > Starting from an used_idx different than 0 is needed in use cases like > > > virtual machine migration. Not doing so and letting the caller set an > > > avail idx different than 0 causes destination device to try to use old > > > buffers that source driver already recover and are not available > > > anymore. > > > > > > While callers like vdpa_sim set avail_idx directly it does not set > > > used_idx. Instead of let the caller do the assignment, fetch it from > > > the guest at initialization like vhost-kernel do. > > > > > > To perform the same at vring_kernel_init and vring_user_init is left for > > > the future. > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > --- > > > drivers/vhost/vringh.c | 25 +++++++++++++++++++++++-- > > > 1 file changed, 23 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > > > index 33eb941fcf15..0eed825197f2 100644 > > > --- a/drivers/vhost/vringh.c > > > +++ b/drivers/vhost/vringh.c > > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh, > > > return 0; > > > } > > > > > > +/** > > > + * vringh_update_used_idx - fetch used idx from driver's used split vring > > > + * @vrh: The vring. > > > + * > > > + * Returns -errno or 0. > > > + */ > > > +static inline int vringh_update_used_idx(struct vringh *vrh) > > > +{ > > > + return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx); > > > +} > > > + > > > /** > > > * vringh_init_iotlb - initialize a vringh for a ring with IOTLB. > > > * @vrh: the vringh to initialize. > > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features, > > > struct vring_avail *avail, > > > struct vring_used *used) > > > { > > > > While at this, I wonder if it's better to have a dedicated parameter > > for last_avail_idx? > > > > I also had that thought. To directly assign last_avail_idx is not a > specially elegant API IMO. > > Maybe expose a way to fetch used_idx from device vring and pass > used_idx as parameter too? If I was not wrong, we can start from last_avail_idx, for used_idx it is only needed for inflight descriptors which might require other APIs? (All the current vDPA user of vringh is doing in order processing) > > > > - return vringh_init_kern(vrh, features, num, weak_barriers, > > > - desc, avail, used); > > > + int r = vringh_init_kern(vrh, features, num, weak_barriers, desc, > > > + avail, used); > > > + > > > + if (r != 0) > > > + return r; > > > + > > > + /* Consider the ring not initialized */ > > > + if ((void *)desc == used) > > > + return 0; > > > > I don't understand when we can get this (actually it should be a bug > > of the caller). > > > > You can see it in vdpasim_vq_reset. > > Note that to consider desc == 0 to be an uninitialized ring is a bug > IMO. QEMU considers it that way also, but the standard does not forbid > any ring to be at address 0. Especially if we use vIOMMU. > > So I think the best way to know if we can use the vringh is either > this way, or provide an explicit "initialized" boolean attribute. > Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to > add new attributes. I wonder if we can avoid this in the simulator level instead of the vringh (anyhow it only exposes a vringh_init_xxx() helper now). Thanks > > Thanks! > > > Thanks > > > > > + > > > + return vringh_update_used_idx(vrh); > > > + > > > } > > > EXPORT_SYMBOL(vringh_init_iotlb); > > > > > > -- > > > 2.31.1 > > > > > >
On Sun, Jan 29, 2023 at 7:01 AM Jason Wang <jasowang@redhat.com> wrote: > > On Thu, Jan 19, 2023 at 4:11 PM Eugenio Perez Martin > <eperezma@redhat.com> wrote: > > > > On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > > > > > Starting from an used_idx different than 0 is needed in use cases like > > > > virtual machine migration. Not doing so and letting the caller set an > > > > avail idx different than 0 causes destination device to try to use old > > > > buffers that source driver already recover and are not available > > > > anymore. > > > > > > > > While callers like vdpa_sim set avail_idx directly it does not set > > > > used_idx. Instead of let the caller do the assignment, fetch it from > > > > the guest at initialization like vhost-kernel do. > > > > > > > > To perform the same at vring_kernel_init and vring_user_init is left for > > > > the future. > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > --- > > > > drivers/vhost/vringh.c | 25 +++++++++++++++++++++++-- > > > > 1 file changed, 23 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > > > > index 33eb941fcf15..0eed825197f2 100644 > > > > --- a/drivers/vhost/vringh.c > > > > +++ b/drivers/vhost/vringh.c > > > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh, > > > > return 0; > > > > } > > > > > > > > +/** > > > > + * vringh_update_used_idx - fetch used idx from driver's used split vring > > > > + * @vrh: The vring. > > > > + * > > > > + * Returns -errno or 0. > > > > + */ > > > > +static inline int vringh_update_used_idx(struct vringh *vrh) > > > > +{ > > > > + return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx); > > > > +} > > > > + > > > > /** > > > > * vringh_init_iotlb - initialize a vringh for a ring with IOTLB. > > > > * @vrh: the vringh to initialize. > > > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features, > > > > struct vring_avail *avail, > > > > struct vring_used *used) > > > > { > > > > > > While at this, I wonder if it's better to have a dedicated parameter > > > for last_avail_idx? > > > > > > > I also had that thought. To directly assign last_avail_idx is not a > > specially elegant API IMO. > > > > Maybe expose a way to fetch used_idx from device vring and pass > > used_idx as parameter too? > > If I was not wrong, we can start from last_avail_idx, for used_idx it > is only needed for inflight descriptors which might require other > APIs? > > (All the current vDPA user of vringh is doing in order processing) > That was actually my first attempt and it works equally well for the moment, but it diverges from vhost-kernel behavior for little benefit. To assign both values at set_vring_base mean that if vDPA introduces an (hypothetical) VHOST_VDPA_F_INFLIGHT backend feature in the future, the initialization process would vary a lot: * Without that feature, the used_idx starts with 0, and the avail one is 0 or whatever value the user set with vring_set_base. * With that feature, the device will read guest's used_idx as vhost-kernel? We would enable a new ioctl to set it, or expand set_base to include used_idx, effectively diverting from vhost-kernel? To me the wisest option is to move this with vhost-kernel. Maybe we need to add a feature bit to know that the hypervisor can trust the device will do "the right thing" (VHOST_VDPA_F_FETCH_USED_AT_ENABLE?), but we should keep it orthogonal to inflight descriptor migration in my opinion. Having said that, I'm totally ok to do it otherwise (or to expand the patch message if needed). > > > > > > - return vringh_init_kern(vrh, features, num, weak_barriers, > > > > - desc, avail, used); > > > > + int r = vringh_init_kern(vrh, features, num, weak_barriers, desc, > > > > + avail, used); > > > > + > > > > + if (r != 0) > > > > + return r; > > > > + > > > > + /* Consider the ring not initialized */ > > > > + if ((void *)desc == used) > > > > + return 0; > > > > > > I don't understand when we can get this (actually it should be a bug > > > of the caller). > > > > > > > You can see it in vdpasim_vq_reset. > > > > Note that to consider desc == 0 to be an uninitialized ring is a bug > > IMO. QEMU considers it that way also, but the standard does not forbid > > any ring to be at address 0. Especially if we use vIOMMU. > > > > So I think the best way to know if we can use the vringh is either > > this way, or provide an explicit "initialized" boolean attribute. > > Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to > > add new attributes. > > I wonder if we can avoid this in the simulator level instead of the > vringh (anyhow it only exposes a vringh_init_xxx() helper now). > In my opinion that is a mistake if other drivers will use it to implement the emulated control virtqueue. And it requires more changes. But it is doable for sure. Thanks!
On Tue, Jan 31, 2023 at 12:39 AM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Sun, Jan 29, 2023 at 7:01 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Thu, Jan 19, 2023 at 4:11 PM Eugenio Perez Martin > > <eperezma@redhat.com> wrote: > > > > > > On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > > > > > > > Starting from an used_idx different than 0 is needed in use cases like > > > > > virtual machine migration. Not doing so and letting the caller set an > > > > > avail idx different than 0 causes destination device to try to use old > > > > > buffers that source driver already recover and are not available > > > > > anymore. > > > > > > > > > > While callers like vdpa_sim set avail_idx directly it does not set > > > > > used_idx. Instead of let the caller do the assignment, fetch it from > > > > > the guest at initialization like vhost-kernel do. > > > > > > > > > > To perform the same at vring_kernel_init and vring_user_init is left for > > > > > the future. > > > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > > --- > > > > > drivers/vhost/vringh.c | 25 +++++++++++++++++++++++-- > > > > > 1 file changed, 23 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > > > > > index 33eb941fcf15..0eed825197f2 100644 > > > > > --- a/drivers/vhost/vringh.c > > > > > +++ b/drivers/vhost/vringh.c > > > > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh, > > > > > return 0; > > > > > } > > > > > > > > > > +/** > > > > > + * vringh_update_used_idx - fetch used idx from driver's used split vring > > > > > + * @vrh: The vring. > > > > > + * > > > > > + * Returns -errno or 0. > > > > > + */ > > > > > +static inline int vringh_update_used_idx(struct vringh *vrh) > > > > > +{ > > > > > + return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx); > > > > > +} > > > > > + > > > > > /** > > > > > * vringh_init_iotlb - initialize a vringh for a ring with IOTLB. > > > > > * @vrh: the vringh to initialize. > > > > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features, > > > > > struct vring_avail *avail, > > > > > struct vring_used *used) > > > > > { > > > > > > > > While at this, I wonder if it's better to have a dedicated parameter > > > > for last_avail_idx? > > > > > > > > > > I also had that thought. To directly assign last_avail_idx is not a > > > specially elegant API IMO. > > > > > > Maybe expose a way to fetch used_idx from device vring and pass > > > used_idx as parameter too? > > > > If I was not wrong, we can start from last_avail_idx, for used_idx it > > is only needed for inflight descriptors which might require other > > APIs? > > > > (All the current vDPA user of vringh is doing in order processing) > > > > That was actually my first attempt and it works equally well for the > moment, but it diverges from vhost-kernel behavior for little benefit. > > To assign both values at set_vring_base mean that if vDPA introduces > an (hypothetical) VHOST_VDPA_F_INFLIGHT backend feature in the future, > the initialization process would vary a lot: > * Without that feature, the used_idx starts with 0, and the avail one > is 0 or whatever value the user set with vring_set_base. > * With that feature, the device will read guest's used_idx as > vhost-kernel? We would enable a new ioctl to set it, or expand > set_base to include used_idx, effectively diverting from vhost-kernel? Adding Longpeng who is looking at this. We can leave this to the caller to decide. Btw, a question, at which case the device used index does not equal to the used index in the vring when the device is suspended? I think the correct way is to flush the pending used indexes before suspending. Otherwise we need an API to get pending used indices? > > To me the wisest option is to move this with vhost-kernel. Maybe we > need to add a feature bit to know that the hypervisor can trust the > device will do "the right thing" (VHOST_VDPA_F_FETCH_USED_AT_ENABLE?), > but we should keep it orthogonal to inflight descriptor migration in > my opinion. I think we need to understand if there are any other possible use cases for setting used idx other than inflight stuff. > > Having said that, I'm totally ok to do it otherwise (or to expand the > patch message if needed). I tend to do that in another series (not mix with the fixes). > > > > > > > > > - return vringh_init_kern(vrh, features, num, weak_barriers, > > > > > - desc, avail, used); > > > > > + int r = vringh_init_kern(vrh, features, num, weak_barriers, desc, > > > > > + avail, used); > > > > > + > > > > > + if (r != 0) > > > > > + return r; > > > > > + > > > > > + /* Consider the ring not initialized */ > > > > > + if ((void *)desc == used) > > > > > + return 0; > > > > > > > > I don't understand when we can get this (actually it should be a bug > > > > of the caller). > > > > > > > > > > You can see it in vdpasim_vq_reset. > > > > > > Note that to consider desc == 0 to be an uninitialized ring is a bug > > > IMO. QEMU considers it that way also, but the standard does not forbid > > > any ring to be at address 0. Especially if we use vIOMMU. > > > > > > So I think the best way to know if we can use the vringh is either > > > this way, or provide an explicit "initialized" boolean attribute. > > > Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to > > > add new attributes. > > > > I wonder if we can avoid this in the simulator level instead of the > > vringh (anyhow it only exposes a vringh_init_xxx() helper now). > > > > In my opinion that is a mistake if other drivers will use it to > implement the emulated control virtqueue. And it requires more > changes. But it is doable for sure. The problem is, there's no reset API in vringh, that's why you need to do if ((void *)desc == used) which depends on behaviour of the vringh user. So I think we should either: 1) move that check in vdpa_sim (since it's not guaranteed that all the vringh users will make desc equal to used during reset) or 2) introduce a vringh_reset_xxx() 1) seems a good step for -stable. Thanks > > Thanks! >
On Tue, Jan 31, 2023 at 4:16 AM Jason Wang <jasowang@redhat.com> wrote: > > On Tue, Jan 31, 2023 at 12:39 AM Eugenio Perez Martin > <eperezma@redhat.com> wrote: > > > > On Sun, Jan 29, 2023 at 7:01 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Thu, Jan 19, 2023 at 4:11 PM Eugenio Perez Martin > > > <eperezma@redhat.com> wrote: > > > > > > > > On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > > > > > > > > > Starting from an used_idx different than 0 is needed in use cases like > > > > > > virtual machine migration. Not doing so and letting the caller set an > > > > > > avail idx different than 0 causes destination device to try to use old > > > > > > buffers that source driver already recover and are not available > > > > > > anymore. > > > > > > > > > > > > While callers like vdpa_sim set avail_idx directly it does not set > > > > > > used_idx. Instead of let the caller do the assignment, fetch it from > > > > > > the guest at initialization like vhost-kernel do. > > > > > > > > > > > > To perform the same at vring_kernel_init and vring_user_init is left for > > > > > > the future. > > > > > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > --- > > > > > > drivers/vhost/vringh.c | 25 +++++++++++++++++++++++-- > > > > > > 1 file changed, 23 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > > > > > > index 33eb941fcf15..0eed825197f2 100644 > > > > > > --- a/drivers/vhost/vringh.c > > > > > > +++ b/drivers/vhost/vringh.c > > > > > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh, > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +/** > > > > > > + * vringh_update_used_idx - fetch used idx from driver's used split vring > > > > > > + * @vrh: The vring. > > > > > > + * > > > > > > + * Returns -errno or 0. > > > > > > + */ > > > > > > +static inline int vringh_update_used_idx(struct vringh *vrh) > > > > > > +{ > > > > > > + return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx); > > > > > > +} > > > > > > + > > > > > > /** > > > > > > * vringh_init_iotlb - initialize a vringh for a ring with IOTLB. > > > > > > * @vrh: the vringh to initialize. > > > > > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features, > > > > > > struct vring_avail *avail, > > > > > > struct vring_used *used) > > > > > > { > > > > > > > > > > While at this, I wonder if it's better to have a dedicated parameter > > > > > for last_avail_idx? > > > > > > > > > > > > > I also had that thought. To directly assign last_avail_idx is not a > > > > specially elegant API IMO. > > > > > > > > Maybe expose a way to fetch used_idx from device vring and pass > > > > used_idx as parameter too? > > > > > > If I was not wrong, we can start from last_avail_idx, for used_idx it > > > is only needed for inflight descriptors which might require other > > > APIs? > > > > > > (All the current vDPA user of vringh is doing in order processing) > > > > > > > That was actually my first attempt and it works equally well for the > > moment, but it diverges from vhost-kernel behavior for little benefit. > > > > To assign both values at set_vring_base mean that if vDPA introduces > > an (hypothetical) VHOST_VDPA_F_INFLIGHT backend feature in the future, > > the initialization process would vary a lot: > > * Without that feature, the used_idx starts with 0, and the avail one > > is 0 or whatever value the user set with vring_set_base. > > * With that feature, the device will read guest's used_idx as > > vhost-kernel? We would enable a new ioctl to set it, or expand > > set_base to include used_idx, effectively diverting from vhost-kernel? > > Adding Longpeng who is looking at this. > Sorry, I'll CC longpeng2@huawei.com in future series like this one. > We can leave this to the caller to decide. > > Btw, a question, at which case the device used index does not equal to > the used index in the vring when the device is suspended? I think the > correct way is to flush the pending used indexes before suspending. > Otherwise we need an API to get pending used indices? > > > > > To me the wisest option is to move this with vhost-kernel. Maybe we > > need to add a feature bit to know that the hypervisor can trust the > > device will do "the right thing" (VHOST_VDPA_F_FETCH_USED_AT_ENABLE?), > > but we should keep it orthogonal to inflight descriptor migration in > > my opinion. > > I think we need to understand if there are any other possible use > cases for setting used idx other than inflight stuff. > Answering this and the previous comment, I cannot think in any case outside of inflight migration. I'm just trying to avoid different behavior between vhost-kernel and vhost-vdpa, and to make features as orthogonal as possible. > > > > Having said that, I'm totally ok to do it otherwise (or to expand the > > patch message if needed). > > I tend to do that in another series (not mix with the fixes). > > > > > > > > > > > > > - return vringh_init_kern(vrh, features, num, weak_barriers, > > > > > > - desc, avail, used); > > > > > > + int r = vringh_init_kern(vrh, features, num, weak_barriers, desc, > > > > > > + avail, used); > > > > > > + > > > > > > + if (r != 0) > > > > > > + return r; > > > > > > + > > > > > > + /* Consider the ring not initialized */ > > > > > > + if ((void *)desc == used) > > > > > > + return 0; > > > > > > > > > > I don't understand when we can get this (actually it should be a bug > > > > > of the caller). > > > > > > > > > > > > > You can see it in vdpasim_vq_reset. > > > > > > > > Note that to consider desc == 0 to be an uninitialized ring is a bug > > > > IMO. QEMU considers it that way also, but the standard does not forbid > > > > any ring to be at address 0. Especially if we use vIOMMU. > > > > > > > > So I think the best way to know if we can use the vringh is either > > > > this way, or provide an explicit "initialized" boolean attribute. > > > > Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to > > > > add new attributes. > > > > > > I wonder if we can avoid this in the simulator level instead of the > > > vringh (anyhow it only exposes a vringh_init_xxx() helper now). > > > > > > > In my opinion that is a mistake if other drivers will use it to > > implement the emulated control virtqueue. And it requires more > > changes. But it is doable for sure. > > The problem is, there's no reset API in vringh, that's why you need to > do if ((void *)desc == used) which depends on behaviour of the vringh > user. > That's a very good point indeed. > So I think we should either: > > 1) move that check in vdpa_sim (since it's not guaranteed that all the > vringh users will make desc equal to used during reset) > > or > > 2) introduce a vringh_reset_xxx() > > 1) seems a good step for -stable. > We can go to 1 for sure. So let's set last_used_idx at vdpasim_set_vq_state, same value as last_avail_idx, and stash it at vdpasim_queue_ready. Do I need to resend the previous patch in this series? Do we need to offer a new feature flag indicating we will set used_idx with avail_idx? Thanks!
On Wed, Jan 18, 2023 at 05:43:59PM +0100, Eugenio Pérez wrote: > Starting from an used_idx different than 0 is needed in use cases like > virtual machine migration. Not doing so and letting the caller set an > avail idx different than 0 causes destination device to try to use old > buffers that source driver already recover and are not available > anymore. > > While callers like vdpa_sim set avail_idx directly it does not set > used_idx. Instead of let the caller do the assignment, fetch it from > the guest at initialization like vhost-kernel do. > > To perform the same at vring_kernel_init and vring_user_init is left for > the future. > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> So I applied 1/2 and dropped 2/2 for now, right? > --- > drivers/vhost/vringh.c | 25 +++++++++++++++++++++++-- > 1 file changed, 23 insertions(+), 2 deletions(-) > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > index 33eb941fcf15..0eed825197f2 100644 > --- a/drivers/vhost/vringh.c > +++ b/drivers/vhost/vringh.c > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh, > return 0; > } > > +/** > + * vringh_update_used_idx - fetch used idx from driver's used split vring > + * @vrh: The vring. > + * > + * Returns -errno or 0. > + */ > +static inline int vringh_update_used_idx(struct vringh *vrh) > +{ > + return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx); > +} > + > /** > * vringh_init_iotlb - initialize a vringh for a ring with IOTLB. > * @vrh: the vringh to initialize. > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features, > struct vring_avail *avail, > struct vring_used *used) > { > - return vringh_init_kern(vrh, features, num, weak_barriers, > - desc, avail, used); > + int r = vringh_init_kern(vrh, features, num, weak_barriers, desc, > + avail, used); > + > + if (r != 0) > + return r; > + > + /* Consider the ring not initialized */ > + if ((void *)desc == used) > + return 0; > + > + return vringh_update_used_idx(vrh); > + > } > EXPORT_SYMBOL(vringh_init_iotlb); > > -- > 2.31.1
On Wed, Feb 1, 2023 at 5:11 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Wed, Jan 18, 2023 at 05:43:59PM +0100, Eugenio Pérez wrote: > > Starting from an used_idx different than 0 is needed in use cases like > > virtual machine migration. Not doing so and letting the caller set an > > avail idx different than 0 causes destination device to try to use old > > buffers that source driver already recover and are not available > > anymore. > > > > While callers like vdpa_sim set avail_idx directly it does not set > > used_idx. Instead of let the caller do the assignment, fetch it from > > the guest at initialization like vhost-kernel do. > > > > To perform the same at vring_kernel_init and vring_user_init is left for > > the future. > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > So I applied 1/2 and dropped 2/2 for now, right? > Yes, please. 2/2 needs tweaking, I'll address them ASAP. Thanks! > > --- > > drivers/vhost/vringh.c | 25 +++++++++++++++++++++++-- > > 1 file changed, 23 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > > index 33eb941fcf15..0eed825197f2 100644 > > --- a/drivers/vhost/vringh.c > > +++ b/drivers/vhost/vringh.c > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh, > > return 0; > > } > > > > +/** > > + * vringh_update_used_idx - fetch used idx from driver's used split vring > > + * @vrh: The vring. > > + * > > + * Returns -errno or 0. > > + */ > > +static inline int vringh_update_used_idx(struct vringh *vrh) > > +{ > > + return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx); > > +} > > + > > /** > > * vringh_init_iotlb - initialize a vringh for a ring with IOTLB. > > * @vrh: the vringh to initialize. > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features, > > struct vring_avail *avail, > > struct vring_used *used) > > { > > - return vringh_init_kern(vrh, features, num, weak_barriers, > > - desc, avail, used); > > + int r = vringh_init_kern(vrh, features, num, weak_barriers, desc, > > + avail, used); > > + > > + if (r != 0) > > + return r; > > + > > + /* Consider the ring not initialized */ > > + if ((void *)desc == used) > > + return 0; > > + > > + return vringh_update_used_idx(vrh); > > + > > } > > EXPORT_SYMBOL(vringh_init_iotlb); > > > > -- > > 2.31.1 >
On Tue, Jan 31, 2023 at 08:58:37AM +0100, Eugenio Perez Martin wrote: > On Tue, Jan 31, 2023 at 4:16 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Tue, Jan 31, 2023 at 12:39 AM Eugenio Perez Martin > > <eperezma@redhat.com> wrote: > > > > > > On Sun, Jan 29, 2023 at 7:01 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > On Thu, Jan 19, 2023 at 4:11 PM Eugenio Perez Martin > > > > <eperezma@redhat.com> wrote: > > > > > > > > > > On Thu, Jan 19, 2023 at 4:20 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > On Thu, Jan 19, 2023 at 12:44 AM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > > > > > > > > > > > Starting from an used_idx different than 0 is needed in use cases like > > > > > > > virtual machine migration. Not doing so and letting the caller set an > > > > > > > avail idx different than 0 causes destination device to try to use old > > > > > > > buffers that source driver already recover and are not available > > > > > > > anymore. > > > > > > > > > > > > > > While callers like vdpa_sim set avail_idx directly it does not set > > > > > > > used_idx. Instead of let the caller do the assignment, fetch it from > > > > > > > the guest at initialization like vhost-kernel do. > > > > > > > > > > > > > > To perform the same at vring_kernel_init and vring_user_init is left for > > > > > > > the future. > > > > > > > > > > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > --- > > > > > > > drivers/vhost/vringh.c | 25 +++++++++++++++++++++++-- > > > > > > > 1 file changed, 23 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c > > > > > > > index 33eb941fcf15..0eed825197f2 100644 > > > > > > > --- a/drivers/vhost/vringh.c > > > > > > > +++ b/drivers/vhost/vringh.c > > > > > > > @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh, > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > +/** > > > > > > > + * vringh_update_used_idx - fetch used idx from driver's used split vring > > > > > > > + * @vrh: The vring. > > > > > > > + * > > > > > > > + * Returns -errno or 0. > > > > > > > + */ > > > > > > > +static inline int vringh_update_used_idx(struct vringh *vrh) > > > > > > > +{ > > > > > > > + return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx); > > > > > > > +} > > > > > > > + > > > > > > > /** > > > > > > > * vringh_init_iotlb - initialize a vringh for a ring with IOTLB. > > > > > > > * @vrh: the vringh to initialize. > > > > > > > @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features, > > > > > > > struct vring_avail *avail, > > > > > > > struct vring_used *used) > > > > > > > { > > > > > > > > > > > > While at this, I wonder if it's better to have a dedicated parameter > > > > > > for last_avail_idx? > > > > > > > > > > > > > > > > I also had that thought. To directly assign last_avail_idx is not a > > > > > specially elegant API IMO. > > > > > > > > > > Maybe expose a way to fetch used_idx from device vring and pass > > > > > used_idx as parameter too? > > > > > > > > If I was not wrong, we can start from last_avail_idx, for used_idx it > > > > is only needed for inflight descriptors which might require other > > > > APIs? > > > > > > > > (All the current vDPA user of vringh is doing in order processing) > > > > > > > > > > That was actually my first attempt and it works equally well for the > > > moment, but it diverges from vhost-kernel behavior for little benefit. > > > > > > To assign both values at set_vring_base mean that if vDPA introduces > > > an (hypothetical) VHOST_VDPA_F_INFLIGHT backend feature in the future, > > > the initialization process would vary a lot: > > > * Without that feature, the used_idx starts with 0, and the avail one > > > is 0 or whatever value the user set with vring_set_base. > > > * With that feature, the device will read guest's used_idx as > > > vhost-kernel? We would enable a new ioctl to set it, or expand > > > set_base to include used_idx, effectively diverting from vhost-kernel? > > > > Adding Longpeng who is looking at this. > > > > Sorry, I'll CC longpeng2@huawei.com in future series like this one. > > > We can leave this to the caller to decide. > > > > Btw, a question, at which case the device used index does not equal to > > the used index in the vring when the device is suspended? I think the > > correct way is to flush the pending used indexes before suspending. > > Otherwise we need an API to get pending used indices? > > > > > > > > To me the wisest option is to move this with vhost-kernel. Maybe we > > > need to add a feature bit to know that the hypervisor can trust the > > > device will do "the right thing" (VHOST_VDPA_F_FETCH_USED_AT_ENABLE?), > > > but we should keep it orthogonal to inflight descriptor migration in > > > my opinion. > > > > I think we need to understand if there are any other possible use > > cases for setting used idx other than inflight stuff. > > > > Answering this and the previous comment, I cannot think in any case > outside of inflight migration. I'm just trying to avoid different > behavior between vhost-kernel and vhost-vdpa, and to make features as > orthogonal as possible. > > > > > > > Having said that, I'm totally ok to do it otherwise (or to expand the > > > patch message if needed). > > > > I tend to do that in another series (not mix with the fixes). > > > > > > > > > > > > > > > > > - return vringh_init_kern(vrh, features, num, weak_barriers, > > > > > > > - desc, avail, used); > > > > > > > + int r = vringh_init_kern(vrh, features, num, weak_barriers, desc, > > > > > > > + avail, used); > > > > > > > + > > > > > > > + if (r != 0) > > > > > > > + return r; > > > > > > > + > > > > > > > + /* Consider the ring not initialized */ > > > > > > > + if ((void *)desc == used) > > > > > > > + return 0; > > > > > > > > > > > > I don't understand when we can get this (actually it should be a bug > > > > > > of the caller). > > > > > > > > > > > > > > > > You can see it in vdpasim_vq_reset. > > > > > > > > > > Note that to consider desc == 0 to be an uninitialized ring is a bug > > > > > IMO. QEMU considers it that way also, but the standard does not forbid > > > > > any ring to be at address 0. Especially if we use vIOMMU. > > > > > > > > > > So I think the best way to know if we can use the vringh is either > > > > > this way, or provide an explicit "initialized" boolean attribute. > > > > > Maybe a new "bool is_initialized(vrh)" is enough, if we don't want to > > > > > add new attributes. > > > > > > > > I wonder if we can avoid this in the simulator level instead of the > > > > vringh (anyhow it only exposes a vringh_init_xxx() helper now). > > > > > > > > > > In my opinion that is a mistake if other drivers will use it to > > > implement the emulated control virtqueue. And it requires more > > > changes. But it is doable for sure. > > > > The problem is, there's no reset API in vringh, that's why you need to > > do if ((void *)desc == used) which depends on behaviour of the vringh > > user. > > > > That's a very good point indeed. > > > So I think we should either: > > > > 1) move that check in vdpa_sim (since it's not guaranteed that all the > > vringh users will make desc equal to used during reset) > > > > or > > > > 2) introduce a vringh_reset_xxx() > > > > 1) seems a good step for -stable. > > > > We can go to 1 for sure. So let's set last_used_idx at > vdpasim_set_vq_state, same value as last_avail_idx, and stash it at > vdpasim_queue_ready. > > Do I need to resend the previous patch in this series? > > Do we need to offer a new feature flag indicating we will set used_idx > with avail_idx? > > Thanks! Jason did you forget to answer or did I miss it?
diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c index 33eb941fcf15..0eed825197f2 100644 --- a/drivers/vhost/vringh.c +++ b/drivers/vhost/vringh.c @@ -1301,6 +1301,17 @@ static inline int putused_iotlb(const struct vringh *vrh, return 0; } +/** + * vringh_update_used_idx - fetch used idx from driver's used split vring + * @vrh: The vring. + * + * Returns -errno or 0. + */ +static inline int vringh_update_used_idx(struct vringh *vrh) +{ + return getu16_iotlb(vrh, &vrh->last_used_idx, &vrh->vring.used->idx); +} + /** * vringh_init_iotlb - initialize a vringh for a ring with IOTLB. * @vrh: the vringh to initialize. @@ -1319,8 +1330,18 @@ int vringh_init_iotlb(struct vringh *vrh, u64 features, struct vring_avail *avail, struct vring_used *used) { - return vringh_init_kern(vrh, features, num, weak_barriers, - desc, avail, used); + int r = vringh_init_kern(vrh, features, num, weak_barriers, desc, + avail, used); + + if (r != 0) + return r; + + /* Consider the ring not initialized */ + if ((void *)desc == used) + return 0; + + return vringh_update_used_idx(vrh); + } EXPORT_SYMBOL(vringh_init_iotlb);