Message ID | 20221110141335.62171-1-sgarzare@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp174203wru; Thu, 10 Nov 2022 06:18:01 -0800 (PST) X-Google-Smtp-Source: AA0mqf69bIewsq2S4PHlVRx8rVSJxBw50qzJmzDIxAh+NubvPlXoaiGyEvg2Gvw1HQQCHznY8bWm X-Received: by 2002:a17:907:72c4:b0:7ae:ae80:b9d6 with SMTP id du4-20020a17090772c400b007aeae80b9d6mr921893ejc.597.1668089881285; Thu, 10 Nov 2022 06:18:01 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668089881; cv=none; d=google.com; s=arc-20160816; b=NtTLP4Npevv8Old2G/bHWqNSiq7k1AlobLMEsC3wdJbfgAKSotUgHhIhBoQ4ek0aPb bZ1rNWMgS47RI+V+jDFI3h24TCKDrijS50kVaVp9VA159J3dviwoalqlgzT6kibac62u dAjcoNc3xEuDN0Qha8uf/xzzZZsLjvE/089rPEpRhP6ZZdZOpM/J9WX0QvTpQVMBIC9P ZTnzsDG0TZLKAozUGltn4b85SJQI+pzsZPT61ykquKqy1Q7hyiXhRaPDy6B/HsudV0hi Dym74DDWRMnOSNYpRRA4YdoRb1ebupF8UfpoU04Ux0Q6W13QY8kPppyHD8JS/yIRZhE+ ii7A== 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 :message-id:date:subject:cc:to:from:dkim-signature; bh=EGdeBqZHdvW6TGtz0bzuryVKHsywKrh20LlxuiZOelI=; b=feSI9N7d2ZRE5eshSiHplfuh8DbIHddRbnxqXimm6m9J5tn6qbRl0s/U6XmAPvhKVF fsotyuDf06Jrbz/z9RrYWBWyR8QoRWxPcXSvADyd8YKnV9RPYsnauQ0iMUfmO5740kve rkeyn50Dn/V/U2ORyv8f7Axd8ejAiPJQ9zjO95uCAC/BdqP58p41So022HXxSbBtA3nS u88yBLjLYVbj6HIBM7WTupbd2iGjnoSvqunFvlo2vBnd69gS2r6RQclrXw4+C8OR/J+Q ySP69MbcffmN2u9aJfy/tx0HMYnvPQ0rjl4+J+6MfiazlEKtGqJ7Jax6+R5Iq/e2ndyD Hbhg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=b8dwHhiM; 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 bu23-20020a170906a15700b007ac60b82ea5si6510551ejb.96.2022.11.10.06.17.36; Thu, 10 Nov 2022 06:18:01 -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=b8dwHhiM; 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 S230347AbiKJOPW (ORCPT <rfc822;winker.wchi@gmail.com> + 99 others); Thu, 10 Nov 2022 09:15:22 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46736 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229561AbiKJOOz (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 10 Nov 2022 09:14:55 -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 710E2FD15 for <linux-kernel@vger.kernel.org>; Thu, 10 Nov 2022 06:13:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1668089631; 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; bh=EGdeBqZHdvW6TGtz0bzuryVKHsywKrh20LlxuiZOelI=; b=b8dwHhiMkkgH0Ibj+9bDLY8AihyPeEXpAjNnQ5J44dGQ7EFjkv/V3vrDdDZvIXmJETfp4P CIKBpDNBV2fs/cuf4X1cPje8cCkp+Y5XQUQCT8elmvVn4oK2Y+0uFbStc369AVdmRnlc3U jgMOFrdPrhiVYhyuEN9UnXdi4ZnyI+o= Received: from mail-pj1-f71.google.com (mail-pj1-f71.google.com [209.85.216.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-196-I9jTOKIpPQuvOR7sX9yOog-1; Thu, 10 Nov 2022 09:13:50 -0500 X-MC-Unique: I9jTOKIpPQuvOR7sX9yOog-1 Received: by mail-pj1-f71.google.com with SMTP id n4-20020a17090a2fc400b002132adb9485so1190654pjm.0 for <linux-kernel@vger.kernel.org>; Thu, 10 Nov 2022 06:13:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=EGdeBqZHdvW6TGtz0bzuryVKHsywKrh20LlxuiZOelI=; b=bZnMr4GN6Nz2AuoMyd8+SP7SVAafrKedIcZHUDGwRnmIcO1o5w2Pu8X6rxhlgEeNiJ qS9GI8pkdcv5wcJOyl21eXIypd1tjjqWn/z7+DO/0ZKS8AxN2uuMyxvCHSw88jPdiP7r SW36UMJD2pSd9B6TlYUBJy3yzsmjepgEr9+BXI24kZiaT26Wsy3a7F7y6Iy/7H6Zq2wJ Worv5uEMsPoqpqNpMTcIWzKGbAl9/Ca0zV8i9QnK1UijyI1dg5rWIqv3zgJ/L4y4I4mm THuXrxn669Av50KeK81AD3jDZTmgzhVeRnBrPaA5B8xhGiE27dNGbsuFgTFMdwbbG9UW LuHQ== X-Gm-Message-State: ACrzQf0+FiipXrfmU1kGbs0JTZ+VR0JaXUtB4W+fhTBghgsPgRcnZ7WW Gy9dhVgnFR9DBCOiN+g+FYAoj1rt3412rGdZwgdyks2DUJRUiKexHSVSeXpVWbzB642mVCOUl0z NlLRqpyOSweg/RsvFiULYU4ko X-Received: by 2002:a17:902:ccc2:b0:178:29f9:5c5e with SMTP id z2-20020a170902ccc200b0017829f95c5emr62089268ple.21.1668089628823; Thu, 10 Nov 2022 06:13:48 -0800 (PST) X-Received: by 2002:a17:902:ccc2:b0:178:29f9:5c5e with SMTP id z2-20020a170902ccc200b0017829f95c5emr62089242ple.21.1668089628429; Thu, 10 Nov 2022 06:13:48 -0800 (PST) Received: from step1.redhat.com (host-82-53-134-234.retail.telecomitalia.it. [82.53.134.234]) by smtp.gmail.com with ESMTPSA id o24-20020aa79798000000b0056a7486da77sm10503973pfp.13.2022.11.10.06.13.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Nov 2022 06:13:47 -0800 (PST) From: Stefano Garzarella <sgarzare@redhat.com> To: virtualization@lists.linux-foundation.org Cc: "Michael S. Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, linux-kernel@vger.kernel.org, Eugenio Perez Martin <eperezma@redhat.com>, Stefano Garzarella <sgarzare@redhat.com> Subject: [PATCH] vdpa_sim: fix vringh initialization in vdpasim_queue_ready() Date: Thu, 10 Nov 2022 15:13:35 +0100 Message-Id: <20221110141335.62171-1-sgarzare@redhat.com> X-Mailer: git-send-email 2.38.1 MIME-Version: 1.0 Content-type: text/plain Content-Transfer-Encoding: 8bit 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?1749119015306391489?= X-GMAIL-MSGID: =?utf-8?q?1749119015306391489?= |
Series |
vdpa_sim: fix vringh initialization in vdpasim_queue_ready()
|
|
Commit Message
Stefano Garzarella
Nov. 10, 2022, 2:13 p.m. UTC
When we initialize vringh, we should pass the features and the
number of elements in the virtqueue negotiated with the driver,
otherwise operations with vringh may fail.
This was discovered in a case where the driver sets a number of
elements in the virtqueue different from the value returned by
.get_vq_num_max().
In vdpasim_vq_reset() is safe to initialize the vringh with
default values, since the virtqueue will not be used until
vdpasim_queue_ready() is called again.
Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator")
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Comments
On Thu, Nov 10, 2022 at 10:13 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > When we initialize vringh, we should pass the features and the > number of elements in the virtqueue negotiated with the driver, > otherwise operations with vringh may fail. > > This was discovered in a case where the driver sets a number of > elements in the virtqueue different from the value returned by > .get_vq_num_max(). > > In vdpasim_vq_reset() is safe to initialize the vringh with > default values, since the virtqueue will not be used until > vdpasim_queue_ready() is called again. > > Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator") > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> Acked-by: Jason Wang <jasowang@redhat.com> Thanks > --- > drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c > index b071f0d842fb..b20689f8fe89 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c > @@ -67,8 +67,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx) > { > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > > - vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features, > - VDPASIM_QUEUE_MAX, false, > + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false, > (struct vring_desc *)(uintptr_t)vq->desc_addr, > (struct vring_avail *) > (uintptr_t)vq->driver_addr, > -- > 2.38.1 >
On Thu, Nov 10, 2022 at 3:13 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > When we initialize vringh, we should pass the features and the > number of elements in the virtqueue negotiated with the driver, > otherwise operations with vringh may fail. > > This was discovered in a case where the driver sets a number of > elements in the virtqueue different from the value returned by > .get_vq_num_max(). > > In vdpasim_vq_reset() is safe to initialize the vringh with > default values, since the virtqueue will not be used until > vdpasim_queue_ready() is called again. > > Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator") > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > --- > drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c > index b071f0d842fb..b20689f8fe89 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c > @@ -67,8 +67,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx) > { > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > > - vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features, > - VDPASIM_QUEUE_MAX, false, > + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false, > (struct vring_desc *)(uintptr_t)vq->desc_addr, > (struct vring_avail *) > (uintptr_t)vq->driver_addr, > -- > 2.38.1 > I think this is definitely an improvement, but I'd say we should go a step further and rename VDPASIM_QUEUE_MAX to VDPASIM_QUEUE_DEFAULT. As you point out in the patch message it is not a max anymore. Another thing to note is that we don't have a way to report that userspace indicated a bad value for queue length. With the current code vringh will not initialize at all if I'm not wrong, so we should prevent userspace to put a bad num. Ideally, we should repeat the tests of vring_init_kern at vdpasim_set_vq_num. We could either call it with NULL vring addresses to check for -EINVAL, or simply repeat the conditional (!num || num > 0xffff || (num & (num - 1))). I'd say the first one is better to not go out of sync. All of that can be done on top anyway, so for this patch: Acked-by: Eugenio Pérez <eperezma@redhat.com>
On Fri, Nov 11, 2022 at 04:40:33PM +0100, Eugenio Perez Martin wrote: >On Thu, Nov 10, 2022 at 3:13 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> When we initialize vringh, we should pass the features and the >> number of elements in the virtqueue negotiated with the driver, >> otherwise operations with vringh may fail. >> >> This was discovered in a case where the driver sets a number of >> elements in the virtqueue different from the value returned by >> .get_vq_num_max(). >> >> In vdpasim_vq_reset() is safe to initialize the vringh with >> default values, since the virtqueue will not be used until >> vdpasim_queue_ready() is called again. >> >> Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator") >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> --- >> drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c >> index b071f0d842fb..b20689f8fe89 100644 >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c >> @@ -67,8 +67,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx) >> { >> struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; >> >> - vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features, >> - VDPASIM_QUEUE_MAX, false, >> + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false, >> (struct vring_desc *)(uintptr_t)vq->desc_addr, >> (struct vring_avail *) >> (uintptr_t)vq->driver_addr, >> -- >> 2.38.1 >> > >I think this is definitely an improvement, but I'd say we should go a >step further and rename VDPASIM_QUEUE_MAX to VDPASIM_QUEUE_DEFAULT. As >you point out in the patch message it is not a max anymore. I'm not sure about renaming since it is the value returned by vdpasim_get_vq_num_max, so IMHO the _MAX suffix is fine. But I admit that initially I didn't understand whether it's the maximum number of queues or elements, so maybe VDPASIM_VQ_NUM_MAX is better. > >Another thing to note is that we don't have a way to report that >userspace indicated a bad value for queue length. With the current >code vringh will not initialize at all if I'm not wrong, so we should >prevent userspace to put a bad num. Right! > >Ideally, we should repeat the tests of vring_init_kern at >vdpasim_set_vq_num. We could either call it with NULL vring addresses >to check for -EINVAL, or simply repeat the conditional (!num || num > >0xffff || (num & (num - 1))). I'd say the first one is better to not >go out of sync. Or we could do the check in vdpasim_set_vq_ready() and set it not ready if the vq_num is wrong. > >All of that can be done on top anyway, so for this patch: > >Acked-by: Eugenio Pérez <eperezma@redhat.com> > Thanks for the review, Stefano
On Fri, Nov 11, 2022 at 5:30 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Fri, Nov 11, 2022 at 04:40:33PM +0100, Eugenio Perez Martin wrote: > >On Thu, Nov 10, 2022 at 3:13 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > >> > >> When we initialize vringh, we should pass the features and the > >> number of elements in the virtqueue negotiated with the driver, > >> otherwise operations with vringh may fail. > >> > >> This was discovered in a case where the driver sets a number of > >> elements in the virtqueue different from the value returned by > >> .get_vq_num_max(). > >> > >> In vdpasim_vq_reset() is safe to initialize the vringh with > >> default values, since the virtqueue will not be used until > >> vdpasim_queue_ready() is called again. > >> > >> Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator") > >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > >> --- > >> drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c > >> index b071f0d842fb..b20689f8fe89 100644 > >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c > >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c > >> @@ -67,8 +67,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx) > >> { > >> struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > >> > >> - vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features, > >> - VDPASIM_QUEUE_MAX, false, > >> + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false, > >> (struct vring_desc *)(uintptr_t)vq->desc_addr, > >> (struct vring_avail *) > >> (uintptr_t)vq->driver_addr, > >> -- > >> 2.38.1 > >> > > > >I think this is definitely an improvement, but I'd say we should go a > >step further and rename VDPASIM_QUEUE_MAX to VDPASIM_QUEUE_DEFAULT. As > >you point out in the patch message it is not a max anymore. > > I'm not sure about renaming since it is the value returned by > vdpasim_get_vq_num_max, so IMHO the _MAX suffix is fine. Oh that's a very good point. But then I guess a conformant driver should never set more descriptors than that. Would it be convenient to make the default queue size of 32768 and let the guest specify less descriptors than that? Default configuration will consume more memory then. > But I admit that initially I didn't understand whether it's the maximum > number of queues or elements, so maybe VDPASIM_VQ_NUM_MAX is better. > > > > >Another thing to note is that we don't have a way to report that > >userspace indicated a bad value for queue length. With the current > >code vringh will not initialize at all if I'm not wrong, so we should > >prevent userspace to put a bad num. > > Right! > > > > >Ideally, we should repeat the tests of vring_init_kern at > >vdpasim_set_vq_num. We could either call it with NULL vring addresses > >to check for -EINVAL, or simply repeat the conditional (!num || num > > >0xffff || (num & (num - 1))). I'd say the first one is better to not > >go out of sync. > > Or we could do the check in vdpasim_set_vq_ready() and set it not ready > if the vq_num is wrong. > Maybe it is the right place to do it, but the device is initiated at that point so the driver needs to perform a full reset. As a reference, qemu will retain the last valid size set to a vq, or the default. This is because it ignores the bad values systematically. Not sure what is more conformant actually :). > > > >All of that can be done on top anyway, so for this patch: > > > >Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > Thanks for the review, > Stefano >
On Mon, Nov 14, 2022 at 10:13:51AM +0100, Eugenio Perez Martin wrote: >On Fri, Nov 11, 2022 at 5:30 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> On Fri, Nov 11, 2022 at 04:40:33PM +0100, Eugenio Perez Martin wrote: >> >On Thu, Nov 10, 2022 at 3:13 PM Stefano Garzarella <sgarzare@redhat.com> wrote: >> >> >> >> When we initialize vringh, we should pass the features and the >> >> number of elements in the virtqueue negotiated with the driver, >> >> otherwise operations with vringh may fail. >> >> >> >> This was discovered in a case where the driver sets a number of >> >> elements in the virtqueue different from the value returned by >> >> .get_vq_num_max(). >> >> >> >> In vdpasim_vq_reset() is safe to initialize the vringh with >> >> default values, since the virtqueue will not be used until >> >> vdpasim_queue_ready() is called again. >> >> >> >> Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator") >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> >> >> --- >> >> drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 +-- >> >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> >> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c >> >> index b071f0d842fb..b20689f8fe89 100644 >> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c >> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c >> >> @@ -67,8 +67,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx) >> >> { >> >> struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; >> >> >> >> - vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features, >> >> - VDPASIM_QUEUE_MAX, false, >> >> + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false, >> >> (struct vring_desc *)(uintptr_t)vq->desc_addr, >> >> (struct vring_avail *) >> >> (uintptr_t)vq->driver_addr, >> >> -- >> >> 2.38.1 >> >> >> > >> >I think this is definitely an improvement, but I'd say we should go a >> >step further and rename VDPASIM_QUEUE_MAX to VDPASIM_QUEUE_DEFAULT. As >> >you point out in the patch message it is not a max anymore. >> >> I'm not sure about renaming since it is the value returned by >> vdpasim_get_vq_num_max, so IMHO the _MAX suffix is fine. > >Oh that's a very good point. But then I guess a conformant driver >should never set more descriptors than that. Yep, right! > >Would it be convenient to make the default queue size of 32768 and let Yep, I think it makes sense. >the guest specify less descriptors than that? Default configuration >will consume more memory then. Do you mean for the driver point of view? Because IIUC in vringh we don't allocate anything related to the queue size. > >> But I admit that initially I didn't understand whether it's the maximum >> number of queues or elements, so maybe VDPASIM_VQ_NUM_MAX is better. >> >> > >> >Another thing to note is that we don't have a way to report that >> >userspace indicated a bad value for queue length. With the current >> >code vringh will not initialize at all if I'm not wrong, so we should >> >prevent userspace to put a bad num. >> >> Right! >> >> > >> >Ideally, we should repeat the tests of vring_init_kern at >> >vdpasim_set_vq_num. We could either call it with NULL vring addresses >> >to check for -EINVAL, or simply repeat the conditional (!num || num > >> >0xffff || (num & (num - 1))). I'd say the first one is better to not >> >go out of sync. >> >> Or we could do the check in vdpasim_set_vq_ready() and set it not ready >> if the vq_num is wrong. >> > >Maybe it is the right place to do it, but the device is initiated at >that point so the driver needs to perform a full reset. > Yes, but the driver is misbehaving, so it might be okay to request a full reset. >As a reference, qemu will retain the last valid size set to a vq, or >the default. This is because it ignores the bad values systematically. >Not sure what is more conformant actually :). > Me too :-) Thanks, Stefano
On Mon, Nov 14, 2022 at 4:11 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > > On Mon, Nov 14, 2022 at 10:13:51AM +0100, Eugenio Perez Martin wrote: > >On Fri, Nov 11, 2022 at 5:30 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > >> > >> On Fri, Nov 11, 2022 at 04:40:33PM +0100, Eugenio Perez Martin wrote: > >> >On Thu, Nov 10, 2022 at 3:13 PM Stefano Garzarella <sgarzare@redhat.com> wrote: > >> >> > >> >> When we initialize vringh, we should pass the features and the > >> >> number of elements in the virtqueue negotiated with the driver, > >> >> otherwise operations with vringh may fail. > >> >> > >> >> This was discovered in a case where the driver sets a number of > >> >> elements in the virtqueue different from the value returned by > >> >> .get_vq_num_max(). > >> >> > >> >> In vdpasim_vq_reset() is safe to initialize the vringh with > >> >> default values, since the virtqueue will not be used until > >> >> vdpasim_queue_ready() is called again. > >> >> > >> >> Fixes: 2c53d0f64c06 ("vdpasim: vDPA device simulator") > >> >> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com> > >> >> --- > >> >> drivers/vdpa/vdpa_sim/vdpa_sim.c | 3 +-- > >> >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> >> > >> >> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c > >> >> index b071f0d842fb..b20689f8fe89 100644 > >> >> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c > >> >> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c > >> >> @@ -67,8 +67,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx) > >> >> { > >> >> struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > >> >> > >> >> - vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features, > >> >> - VDPASIM_QUEUE_MAX, false, > >> >> + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false, > >> >> (struct vring_desc *)(uintptr_t)vq->desc_addr, > >> >> (struct vring_avail *) > >> >> (uintptr_t)vq->driver_addr, > >> >> -- > >> >> 2.38.1 > >> >> > >> > > >> >I think this is definitely an improvement, but I'd say we should go a > >> >step further and rename VDPASIM_QUEUE_MAX to VDPASIM_QUEUE_DEFAULT. As > >> >you point out in the patch message it is not a max anymore. > >> > >> I'm not sure about renaming since it is the value returned by > >> vdpasim_get_vq_num_max, so IMHO the _MAX suffix is fine. > > > >Oh that's a very good point. But then I guess a conformant driver > >should never set more descriptors than that. > > Yep, right! > > > > >Would it be convenient to make the default queue size of 32768 and let > > Yep, I think it makes sense. > > >the guest specify less descriptors than that? Default configuration > >will consume more memory then. > > Do you mean for the driver point of view? > > Because IIUC in vringh we don't allocate anything related to the queue > size. > Right, I mean the driver that does not override the vring size will start allocating bigger vrings by default. But I don't think that's a problem actually, given that it is the simulator, just pointing it out. > > > >> But I admit that initially I didn't understand whether it's the maximum > >> number of queues or elements, so maybe VDPASIM_VQ_NUM_MAX is better. > >> > >> > > >> >Another thing to note is that we don't have a way to report that > >> >userspace indicated a bad value for queue length. With the current > >> >code vringh will not initialize at all if I'm not wrong, so we should > >> >prevent userspace to put a bad num. > >> > >> Right! > >> > >> > > >> >Ideally, we should repeat the tests of vring_init_kern at > >> >vdpasim_set_vq_num. We could either call it with NULL vring addresses > >> >to check for -EINVAL, or simply repeat the conditional (!num || num > > >> >0xffff || (num & (num - 1))). I'd say the first one is better to not > >> >go out of sync. > >> > >> Or we could do the check in vdpasim_set_vq_ready() and set it not ready > >> if the vq_num is wrong. > >> > > > >Maybe it is the right place to do it, but the device is initiated at > >that point so the driver needs to perform a full reset. > > > > Yes, but the driver is misbehaving, so it might be okay to request a > full reset. > Setting DEVICE_NEEDS_RESET in that case, right? Thanks! > >As a reference, qemu will retain the last valid size set to a vq, or > >the default. This is because it ignores the bad values systematically. > >Not sure what is more conformant actually :). > > > > Me too :-) > > Thanks, > Stefano >
diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c index b071f0d842fb..b20689f8fe89 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c @@ -67,8 +67,7 @@ static void vdpasim_queue_ready(struct vdpasim *vdpasim, unsigned int idx) { struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; - vringh_init_iotlb(&vq->vring, vdpasim->dev_attr.supported_features, - VDPASIM_QUEUE_MAX, false, + vringh_init_iotlb(&vq->vring, vdpasim->features, vq->num, false, (struct vring_desc *)(uintptr_t)vq->desc_addr, (struct vring_avail *) (uintptr_t)vq->driver_addr,