Message ID | 20231219180858.120898-3-dtatulea@nvidia.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel+bounces-5775-ouuuleilei=gmail.com@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:7300:24d3:b0:fb:cd0c:d3e with SMTP id r19csp2126447dyi; Tue, 19 Dec 2023 10:10:55 -0800 (PST) X-Google-Smtp-Source: AGHT+IHJ+0vrYoPkzsCEyPWd15d+ZpEzOyyDpKUNrjS1X1iyEmuwFAE0evq8dBUiz6OEZNPlzBBu X-Received: by 2002:a05:6a00:a82:b0:6d0:875c:5d8d with SMTP id b2-20020a056a000a8200b006d0875c5d8dmr17934858pfl.41.1703009455558; Tue, 19 Dec 2023 10:10:55 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1703009455; cv=pass; d=google.com; s=arc-20160816; b=SrYMK2RQutuWEpoYW3wU0KmEGCDsfo55c78Gtl+bj0rCrv9l68GTCS7xIUTlywes3I nONbrfuDqaXJhTDiDO/NSNCdxUyp7x9LM8mawWS3f/zn6bzrQasgJQM3Uh1Kx8EnEeWL 3f86uFOVRBgcDVoI2LgECCPNvY5GO8RPllwhvrix29f9gF9VscMSiWrvozdrWtkt66lJ SohDIEH46JLRUgxseeMReC40xQEvBisTvedI7RfwiK/dvzMCWb/xKjmqL8yw5LE5oPLe qZQVuEWozbQO2BqfdeYdvkUek/cUY5fqfEFyboDnwmvctr/r+NhnsvE4ITLGjPr4XJ2c G/BQ== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-transfer-encoding:mime-version:list-unsubscribe :list-subscribe:list-id:precedence:references:in-reply-to:message-id :date:subject:cc:to:from:dkim-signature; bh=hFTHaY9PtUATO1cmPdLknCzEq/FUKk+/vuKSvCcAzAk=; fh=IC8CtaUsCHH+VLkdER9IbUix5yCQzPbBUzKtIdyo0ag=; b=CtvGpkFddi/5im6JSl2HhPmdXRT5oEMwCtFA/fKSKenZX7fD06dtFnXnEG07MXNv4o wNShRhcyyYU2ALcQgj2KVW+S9zCZO19+0SbWQ3QfMtbH/0jIGEzjf+Ar8K4tcHoR+EfZ dKDZkBPhLdS2YTyv1nUcd1sDrlHGcHVUHwGNCbvS+VJV5W3Af5/X16OcboxXYliJdIjv pLXZWf7HkTO7l5gV0cA0yC8OPi2CeZ4pQA+BuAE22wiVWzJn+DJhm4g3JhqRoeaPi5K0 3t4xpbNL5oPL1XniAElSvOu5lpmp45adxC2WiXlsydJp/lSYKQ88QghU8nQLxY4WQXxG aynw== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@Nvidia.com header.s=selector2 header.b=tkKuo4CE; arc=pass (i=1 spf=pass spfdomain=nvidia.com dmarc=pass fromdomain=nvidia.com); spf=pass (google.com: domain of linux-kernel+bounces-5775-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-5775-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=nvidia.com Received: from sv.mirrors.kernel.org (sv.mirrors.kernel.org. [2604:1380:45e3:2400::1]) by mx.google.com with ESMTPS id k3-20020aa79d03000000b006d30f25ddc5si6417445pfp.105.2023.12.19.10.10.55 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 19 Dec 2023 10:10:55 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel+bounces-5775-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1; Authentication-Results: mx.google.com; dkim=pass header.i=@Nvidia.com header.s=selector2 header.b=tkKuo4CE; arc=pass (i=1 spf=pass spfdomain=nvidia.com dmarc=pass fromdomain=nvidia.com); spf=pass (google.com: domain of linux-kernel+bounces-5775-ouuuleilei=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-5775-ouuuleilei=gmail.com@vger.kernel.org"; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=nvidia.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 41885285821 for <ouuuleilei@gmail.com>; Tue, 19 Dec 2023 18:10:55 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id D08563A8DB; Tue, 19 Dec 2023 18:09:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=Nvidia.com header.i=@Nvidia.com header.b="tkKuo4CE" X-Original-To: linux-kernel@vger.kernel.org Received: from NAM11-DM6-obe.outbound.protection.outlook.com (mail-dm6nam11on2089.outbound.protection.outlook.com [40.107.223.89]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5DDD139ADF; Tue, 19 Dec 2023 18:09:29 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=nvidia.com Authentication-Results: smtp.subspace.kernel.org; spf=fail smtp.mailfrom=nvidia.com ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=CXfc8hchL0+v6aNC9EUj8K/UCNAjztvhVN5wJ20zSc3x7wK6i5Ks+CE7SJy0QqXWjVgklI2zMeiaWnpY/vBFLFek8kkCaPa+MzM68Brf/oLhaasOkol+umFmDadK1jouGYnO45ftEn6qdLhDPwN78iX32EII7504rrUMDbruPqBKmDPkekOhAv3HlmwaZ8J/OOCXcyOFf0CO5BjvOwE5jzKIBpkGr4gjIX9LnkRzqatXx/XfC96L/hgKGWAr5nij2xMSZZxkTT2hNisdSGGTsH1pWuJoq9wXD5sV7ehm6FV7V1HsupRmlIor3AAsbZrrPiBgR0BbKWWRPO/+vJDfDw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=hFTHaY9PtUATO1cmPdLknCzEq/FUKk+/vuKSvCcAzAk=; b=C2YGIuDbIx/6hwdFS+bXK43xm3uK7bZn/1Mbm5JtyNG0Ku6sNpsC+rrvshp98eyhvBHre7TeSnGqUV9dLpfltsXGrgmLZpREWZ5JSwKhwYK7bhpyoMnRB8NrtOomoQNGLDAz6K7+CA3vUGQvOt1GnJGQfj4v55vdQxtp/A1oVOfMv6boNT1/hSCPC+ggHXe2FgTmHWI6ohfllj1f7P9sOflXCO4XMaWnkpp9hLEppNRUcWPHkH5SEBXRXkfJMyiicwZGJNOUpNyVh6oLu0werI6D7QsbQcNlTZyHZsfFpNd6vgZ6rjVPyw5+S9jRfOJx2o/ELRn6Hlut5BpaIRdovw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 216.228.117.161) smtp.rcpttodomain=redhat.com smtp.mailfrom=nvidia.com; dmarc=pass (p=reject sp=reject pct=100) action=none header.from=nvidia.com; dkim=none (message not signed); arc=none (0) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=hFTHaY9PtUATO1cmPdLknCzEq/FUKk+/vuKSvCcAzAk=; b=tkKuo4CEWQoCKg7aXdBG+kGYgXaWi1y9r4KzEkKg45WO08k4tcS8DbR7DtNEAQjk/BlsvCDlAeqCHEkUvHQ3x3a3Y1qvR7aoszCeIriqWCJEdjyD/t9Lq1ron9U+jWGPVmTRQf9+CH3OXzVbo3I5KqE2iX7Ov5S3jExcf6rLUM0cmtN24MyZ9XmYRjM5YghA52bJHmYTd0aBYFOPYasomBliHj7hqrEt1g1Y4p6UXJVWaIu2Ef6z2Mt9v1sc9AKnhWOOFggavl9/6FmgOQXksQNuxLBDVNJgVxRSxAp/f0pa3uj2uLK2W8M5RrwCe6NK46WaDZ4jxJoHliuiMdmSPA== Received: from BL1P221CA0003.NAMP221.PROD.OUTLOOK.COM (2603:10b6:208:2c5::9) by DM6PR12MB4546.namprd12.prod.outlook.com (2603:10b6:5:2ae::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7091.39; Tue, 19 Dec 2023 18:09:26 +0000 Received: from BL6PEPF0001AB72.namprd02.prod.outlook.com (2603:10b6:208:2c5:cafe::4c) by BL1P221CA0003.outlook.office365.com (2603:10b6:208:2c5::9) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7091.38 via Frontend Transport; Tue, 19 Dec 2023 18:09:26 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 216.228.117.161) smtp.mailfrom=nvidia.com; dkim=none (message not signed) header.d=none;dmarc=pass action=none header.from=nvidia.com; Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates 216.228.117.161 as permitted sender) receiver=protection.outlook.com; client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C Received: from mail.nvidia.com (216.228.117.161) by BL6PEPF0001AB72.mail.protection.outlook.com (10.167.242.165) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7113.14 via Frontend Transport; Tue, 19 Dec 2023 18:09:26 +0000 Received: from rnnvmail205.nvidia.com (10.129.68.10) by mail.nvidia.com (10.129.200.67) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.41; Tue, 19 Dec 2023 10:09:13 -0800 Received: from rnnvmail205.nvidia.com (10.129.68.10) by rnnvmail205.nvidia.com (10.129.68.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.41; Tue, 19 Dec 2023 10:09:13 -0800 Received: from c-237-113-220-225.mtl.labs.mlnx (10.127.8.12) by mail.nvidia.com (10.129.68.10) with Microsoft SMTP Server id 15.2.986.41 via Frontend Transport; Tue, 19 Dec 2023 10:09:10 -0800 From: Dragos Tatulea <dtatulea@nvidia.com> To: "Michael S . Tsirkin" <mst@redhat.com>, Jason Wang <jasowang@redhat.com>, Eugenio Perez Martin <eperezma@redhat.com>, Si-Wei Liu <si-wei.liu@oracle.com>, Saeed Mahameed <saeedm@nvidia.com>, Leon Romanovsky <leon@kernel.org>, <virtualization@lists.linux-foundation.org>, Gal Pressman <gal@nvidia.com> CC: Dragos Tatulea <dtatulea@nvidia.com>, <kvm@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Parav Pandit <parav@nvidia.com>, Xuan Zhuo <xuanzhuo@linux.alibaba.com> Subject: [PATCH vhost v4 02/15] vdpa: Add VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag Date: Tue, 19 Dec 2023 20:08:45 +0200 Message-ID: <20231219180858.120898-3-dtatulea@nvidia.com> X-Mailer: git-send-email 2.42.0 In-Reply-To: <20231219180858.120898-1-dtatulea@nvidia.com> References: <20231219180858.120898-1-dtatulea@nvidia.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: <linux-kernel.vger.kernel.org> List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-NV-OnPremToCloud: ExternallySecured X-EOPAttributedMessage: 0 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: BL6PEPF0001AB72:EE_|DM6PR12MB4546:EE_ X-MS-Office365-Filtering-Correlation-Id: d4e7cc82-c982-4f55-7be0-08dc00bda317 X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: onUa2fx0UvWyG6ApRLkSf4/fc/VAGqo959+wNRV0SvK/E9InpGxx+wDU8RT7/IykFPW7/GBBf1kvWIzqolrAnuLw/9SG01FyfDt1SDEsVFesjRPy++uB7nnosU7nKOgzWKGT/34ez8zMdTT29cvpAtFesPlkSid7D4T7Dh+RkDwrtWz4B99OE1YlOi6H801PKcK0Z9acUW+QhN8glp0z6PdWMNUbOtM2I+528Ev1DoswsmuWDZ5ay9XAJo9+qegqsjHNvO0AXz+ucbInZx9c7Cx5F6nU+kYDnfs8Ru9He03bJPdWEyYnCQEEvW1Ss9cFBWQ7wLq1gO8VJXSUT/pqj08IKt3PsXyf8utWg/f5pJTRXO4v1RrzaJ1SPXsB6guwSgkOhrUHAsPb7aFU27AM19MPAEvzM1mitlDhPrjjT58GwOn100vy+RUojX6ktlPLDaWFHVXQoOrIVovNmyU8m/fl6CtfRk+xQyB5fW30VRFqbrgkWEUsaRMhs1WJlG5XuNBYNQXtkb3u8o1pSKmDndLO60MszRrawmQH7Cxe1t5LToyoqLJqSvcdq5HGSNe6KXMXE7/qX3nX0lS5lIEkBW+XhNBJMFPp705mfv1oSe2TYtFTnSXMN9fhKCT2Y/UbMdM9Cc28dfs6vYstT+QGG3vwwTIE7u1XJ66vAU96gOFSVaw3KTfqqSc0NFq6BJQLV1Lif9YUErRz7VKFjexErwYVwwyZhnrkTXIljAtfirSJ1suLIScdUfvSyo1icmWa X-Forefront-Antispam-Report: CIP:216.228.117.161;CTRY:US;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:mail.nvidia.com;PTR:dc6edge2.nvidia.com;CAT:NONE;SFS:(13230031)(4636009)(376002)(396003)(136003)(346002)(39860400002)(230922051799003)(64100799003)(186009)(451199024)(1800799012)(82310400011)(40470700004)(36840700001)(46966006)(36860700001)(7636003)(356005)(40480700001)(47076005)(40460700003)(1076003)(336012)(426003)(83380400001)(2616005)(66574015)(26005)(36756003)(86362001)(82740400003)(6666004)(478600001)(6636002)(316002)(54906003)(110136005)(70586007)(70206006)(4744005)(4326008)(8676002)(8936002)(2906002)(5660300002)(41300700001);DIR:OUT;SFP:1101; X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Dec 2023 18:09:26.2936 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: d4e7cc82-c982-4f55-7be0-08dc00bda317 X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.161];Helo=[mail.nvidia.com] X-MS-Exchange-CrossTenant-AuthSource: BL6PEPF0001AB72.namprd02.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4546 X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1785734842761331742 X-GMAIL-MSGID: 1785734842761331742 |
Series |
vdpa/mlx5: Add support for resumable vqs
|
|
Commit Message
Dragos Tatulea
Dec. 19, 2023, 6:08 p.m. UTC
The virtio spec doesn't allow changing virtqueue addresses after
DRIVER_OK. Some devices do support this operation when the device is
suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag
advertises this support as a backend features.
Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
Suggested-by: Eugenio Pérez <eperezma@redhat.com>
---
include/uapi/linux/vhost_types.h | 4 ++++
1 file changed, 4 insertions(+)
Comments
On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > The virtio spec doesn't allow changing virtqueue addresses after > DRIVER_OK. Some devices do support this operation when the device is > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > advertises this support as a backend features. There's an ongoing effort in virtio spec to introduce the suspend state. So I wonder if it's better to just allow such behaviour? Thanks > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > Suggested-by: Eugenio Pérez <eperezma@redhat.com> > --- > include/uapi/linux/vhost_types.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h > index d7656908f730..aacd067afc89 100644 > --- a/include/uapi/linux/vhost_types.h > +++ b/include/uapi/linux/vhost_types.h > @@ -192,5 +192,9 @@ struct vhost_vdpa_iova_range { > #define VHOST_BACKEND_F_DESC_ASID 0x7 > /* IOTLB don't flush memory mapping across device reset */ > #define VHOST_BACKEND_F_IOTLB_PERSIST 0x8 > +/* Device supports changing virtqueue addresses when device is suspended > + * and is in state DRIVER_OK. > + */ > +#define VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND 0x9 > > #endif > -- > 2.43.0 >
On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > The virtio spec doesn't allow changing virtqueue addresses after > > DRIVER_OK. Some devices do support this operation when the device is > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > advertises this support as a backend features. > > There's an ongoing effort in virtio spec to introduce the suspend state. > > So I wonder if it's better to just allow such behaviour? Actually I mean, allow drivers to modify the parameters during suspend without a new feature. Thanks > > Thanks > >
On Wed, 2023-12-20 at 12:05 +0800, Jason Wang wrote: > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > DRIVER_OK. Some devices do support this operation when the device is > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > advertises this support as a backend features. > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > So I wonder if it's better to just allow such behaviour? > > Actually I mean, allow drivers to modify the parameters during suspend > without a new feature. > Fine by me. Less code is better than more code. The v2 of this series would be sufficient then. Thanks
On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > DRIVER_OK. Some devices do support this operation when the device is > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > advertises this support as a backend features. > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > So I wonder if it's better to just allow such behaviour? > > Actually I mean, allow drivers to modify the parameters during suspend > without a new feature. > That would be ideal, but how do userland checks if it can suspend + change properties + resume? The only way that comes to my mind is to make sure all parents return error if userland tries to do it, and then fallback in userland. I'm ok with that, but I'm not sure if the current master & previous kernel has a coherent behavior. Do they return error? Or return success without changing address / vq state?
On Tue, Dec 19, 2023 at 7:09 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > The virtio spec doesn't allow changing virtqueue addresses after > DRIVER_OK. Some devices do support this operation when the device is > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > advertises this support as a backend features. > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > Suggested-by: Eugenio Pérez <eperezma@redhat.com> > --- > include/uapi/linux/vhost_types.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h > index d7656908f730..aacd067afc89 100644 > --- a/include/uapi/linux/vhost_types.h > +++ b/include/uapi/linux/vhost_types.h > @@ -192,5 +192,9 @@ struct vhost_vdpa_iova_range { > #define VHOST_BACKEND_F_DESC_ASID 0x7 > /* IOTLB don't flush memory mapping across device reset */ > #define VHOST_BACKEND_F_IOTLB_PERSIST 0x8 > +/* Device supports changing virtqueue addresses when device is suspended > + * and is in state DRIVER_OK. > + */ > +#define VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND 0x9 > If we go by feature flag, Acked-by: Eugenio Pérez <eperezma@redhat.com> > #endif > -- > 2.43.0 >
On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > > DRIVER_OK. Some devices do support this operation when the device is > > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > > advertises this support as a backend features. > > > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > > > So I wonder if it's better to just allow such behaviour? > > > > Actually I mean, allow drivers to modify the parameters during suspend > > without a new feature. > > > > That would be ideal, but how do userland checks if it can suspend + > change properties + resume? As discussed, it looks to me the only device that supports suspend is simulator and it supports change properties. E.g: static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx, u64 desc_area, u64 driver_area, u64 device_area) { struct vdpasim *vdpasim = vdpa_to_sim(vdpa); struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; vq->desc_addr = desc_area; vq->driver_addr = driver_area; vq->device_addr = device_area; return 0; } > > The only way that comes to my mind is to make sure all parents return > error if userland tries to do it, and then fallback in userland. Yes. > I'm > ok with that, but I'm not sure if the current master & previous kernel > has a coherent behavior. Do they return error? Or return success > without changing address / vq state? We probably don't need to worry too much here, as e.g set_vq_address could fail even without suspend (just at uAPI level). Thanks >
On Thu, Dec 21, 2023 at 3:03 AM Jason Wang <jasowang@redhat.com> wrote: > > On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin > <eperezma@redhat.com> wrote: > > > > On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > > > DRIVER_OK. Some devices do support this operation when the device is > > > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > > > advertises this support as a backend features. > > > > > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > > > > > So I wonder if it's better to just allow such behaviour? > > > > > > Actually I mean, allow drivers to modify the parameters during suspend > > > without a new feature. > > > > > > > That would be ideal, but how do userland checks if it can suspend + > > change properties + resume? > > As discussed, it looks to me the only device that supports suspend is > simulator and it supports change properties. > > E.g: > > static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx, > u64 desc_area, u64 driver_area, > u64 device_area) > { > struct vdpasim *vdpasim = vdpa_to_sim(vdpa); > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > > vq->desc_addr = desc_area; > vq->driver_addr = driver_area; > vq->device_addr = device_area; > > return 0; > } > So in the current kernel master it is valid to set a different vq address while the device is suspended in vdpa_sim. But it is not valid in mlx5, as the FW will not be updated in resume (Dragos, please correct me if I'm wrong). Both of them return success. How can we know in the destination QEMU if it is valid to suspend & set address? Should we handle this as a bugfix and backport the change? > > > > The only way that comes to my mind is to make sure all parents return > > error if userland tries to do it, and then fallback in userland. > > Yes. > > > I'm > > ok with that, but I'm not sure if the current master & previous kernel > > has a coherent behavior. Do they return error? Or return success > > without changing address / vq state? > > We probably don't need to worry too much here, as e.g set_vq_address > could fail even without suspend (just at uAPI level). > I don't get this, sorry. I rephrased my point with an example earlier in the mail.
On Thu, 2023-12-21 at 08:46 +0100, Eugenio Perez Martin wrote: > On Thu, Dec 21, 2023 at 3:03 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin > > <eperezma@redhat.com> wrote: > > > > > > On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > > > > DRIVER_OK. Some devices do support this operation when the device is > > > > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > > > > advertises this support as a backend features. > > > > > > > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > > > > > > > So I wonder if it's better to just allow such behaviour? > > > > > > > > Actually I mean, allow drivers to modify the parameters during suspend > > > > without a new feature. > > > > > > > > > > That would be ideal, but how do userland checks if it can suspend + > > > change properties + resume? > > > > As discussed, it looks to me the only device that supports suspend is > > simulator and it supports change properties. > > > > E.g: > > > > static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx, > > u64 desc_area, u64 driver_area, > > u64 device_area) > > { > > struct vdpasim *vdpasim = vdpa_to_sim(vdpa); > > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > > > > vq->desc_addr = desc_area; > > vq->driver_addr = driver_area; > > vq->device_addr = device_area; > > > > return 0; > > } > > > > So in the current kernel master it is valid to set a different vq > address while the device is suspended in vdpa_sim. But it is not valid > in mlx5, as the FW will not be updated in resume (Dragos, please > correct me if I'm wrong). Both of them return success. > In the current state, there is no resume. HW Virtqueues will just get re-created with the new address. > How can we know in the destination QEMU if it is valid to suspend & > set address? Should we handle this as a bugfix and backport the > change? > > > > > > > The only way that comes to my mind is to make sure all parents return > > > error if userland tries to do it, and then fallback in userland. > > > > Yes. > > > > > I'm > > > ok with that, but I'm not sure if the current master & previous kernel > > > has a coherent behavior. Do they return error? Or return success > > > without changing address / vq state? > > > > We probably don't need to worry too much here, as e.g set_vq_address > > could fail even without suspend (just at uAPI level). > > > > I don't get this, sorry. I rephrased my point with an example earlier > in the mail. >
On Thu, Dec 21, 2023 at 12:52 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > On Thu, 2023-12-21 at 08:46 +0100, Eugenio Perez Martin wrote: > > On Thu, Dec 21, 2023 at 3:03 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin > > > <eperezma@redhat.com> wrote: > > > > > > > > On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > > > > > DRIVER_OK. Some devices do support this operation when the device is > > > > > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > > > > > advertises this support as a backend features. > > > > > > > > > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > > > > > > > > > So I wonder if it's better to just allow such behaviour? > > > > > > > > > > Actually I mean, allow drivers to modify the parameters during suspend > > > > > without a new feature. > > > > > > > > > > > > > That would be ideal, but how do userland checks if it can suspend + > > > > change properties + resume? > > > > > > As discussed, it looks to me the only device that supports suspend is > > > simulator and it supports change properties. > > > > > > E.g: > > > > > > static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx, > > > u64 desc_area, u64 driver_area, > > > u64 device_area) > > > { > > > struct vdpasim *vdpasim = vdpa_to_sim(vdpa); > > > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > > > > > > vq->desc_addr = desc_area; > > > vq->driver_addr = driver_area; > > > vq->device_addr = device_area; > > > > > > return 0; > > > } > > > > > > > So in the current kernel master it is valid to set a different vq > > address while the device is suspended in vdpa_sim. But it is not valid > > in mlx5, as the FW will not be updated in resume (Dragos, please > > correct me if I'm wrong). Both of them return success. > > > In the current state, there is no resume. HW Virtqueues will just get re-created > with the new address. > Oh, then all of this is effectively transparent to the userspace except for the time it takes? In that case you're right, we don't need feature flags. But I think it would be great to also move the error return in case userspace tries to modify vq parameters out of suspend state. Thanks! > > How can we know in the destination QEMU if it is valid to suspend & > > set address? Should we handle this as a bugfix and backport the > > change? > > > > > > > > > > The only way that comes to my mind is to make sure all parents return > > > > error if userland tries to do it, and then fallback in userland. > > > > > > Yes. > > > > > > > I'm > > > > ok with that, but I'm not sure if the current master & previous kernel > > > > has a coherent behavior. Do they return error? Or return success > > > > without changing address / vq state? > > > > > > We probably don't need to worry too much here, as e.g set_vq_address > > > could fail even without suspend (just at uAPI level). > > > > > > > I don't get this, sorry. I rephrased my point with an example earlier > > in the mail. > > >
On Thu, 2023-12-21 at 13:08 +0100, Eugenio Perez Martin wrote: > On Thu, Dec 21, 2023 at 12:52 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > On Thu, 2023-12-21 at 08:46 +0100, Eugenio Perez Martin wrote: > > > On Thu, Dec 21, 2023 at 3:03 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin > > > > <eperezma@redhat.com> wrote: > > > > > > > > > > On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > > > > > > DRIVER_OK. Some devices do support this operation when the device is > > > > > > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > > > > > > advertises this support as a backend features. > > > > > > > > > > > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > > > > > > > > > > > So I wonder if it's better to just allow such behaviour? > > > > > > > > > > > > Actually I mean, allow drivers to modify the parameters during suspend > > > > > > without a new feature. > > > > > > > > > > > > > > > > That would be ideal, but how do userland checks if it can suspend + > > > > > change properties + resume? > > > > > > > > As discussed, it looks to me the only device that supports suspend is > > > > simulator and it supports change properties. > > > > > > > > E.g: > > > > > > > > static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx, > > > > u64 desc_area, u64 driver_area, > > > > u64 device_area) > > > > { > > > > struct vdpasim *vdpasim = vdpa_to_sim(vdpa); > > > > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > > > > > > > > vq->desc_addr = desc_area; > > > > vq->driver_addr = driver_area; > > > > vq->device_addr = device_area; > > > > > > > > return 0; > > > > } > > > > > > > > > > So in the current kernel master it is valid to set a different vq > > > address while the device is suspended in vdpa_sim. But it is not valid > > > in mlx5, as the FW will not be updated in resume (Dragos, please > > > correct me if I'm wrong). Both of them return success. > > > > > In the current state, there is no resume. HW Virtqueues will just get re-created > > with the new address. > > > > Oh, then all of this is effectively transparent to the userspace > except for the time it takes? > Not quite: mlx5_vdpa_set_vq_address will save the vq address only on the SW vq representation. Only later will it will call into the FW to update the FW. Later means: - On DRIVER_OK state, when the VQs get created. - On .set_map when the VQs get re-created (before this series) / updated (after this series) - On .resume (after this series). So if the .set_vq_address is called when the VQ is in DRIVER_OK but not suspended those addresses will be set later for later. > In that case you're right, we don't need feature flags. But I think it > would be great to also move the error return in case userspace tries > to modify vq parameters out of suspend state. > On the driver side or on the core side? Thanks > Thanks! > > > > > How can we know in the destination QEMU if it is valid to suspend & > > > set address? Should we handle this as a bugfix and backport the > > > change? > > > > > > > > > > > > > The only way that comes to my mind is to make sure all parents return > > > > > error if userland tries to do it, and then fallback in userland. > > > > > > > > Yes. > > > > > > > > > I'm > > > > > ok with that, but I'm not sure if the current master & previous kernel > > > > > has a coherent behavior. Do they return error? Or return success > > > > > without changing address / vq state? > > > > > > > > We probably don't need to worry too much here, as e.g set_vq_address > > > > could fail even without suspend (just at uAPI level). > > > > > > > > > > I don't get this, sorry. I rephrased my point with an example earlier > > > in the mail. > > > > > >
On Thu, Dec 21, 2023 at 3:38 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > On Thu, 2023-12-21 at 13:08 +0100, Eugenio Perez Martin wrote: > > On Thu, Dec 21, 2023 at 12:52 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > On Thu, 2023-12-21 at 08:46 +0100, Eugenio Perez Martin wrote: > > > > On Thu, Dec 21, 2023 at 3:03 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin > > > > > <eperezma@redhat.com> wrote: > > > > > > > > > > > > On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > > > > > > > DRIVER_OK. Some devices do support this operation when the device is > > > > > > > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > > > > > > > advertises this support as a backend features. > > > > > > > > > > > > > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > > > > > > > > > > > > > So I wonder if it's better to just allow such behaviour? > > > > > > > > > > > > > > Actually I mean, allow drivers to modify the parameters during suspend > > > > > > > without a new feature. > > > > > > > > > > > > > > > > > > > That would be ideal, but how do userland checks if it can suspend + > > > > > > change properties + resume? > > > > > > > > > > As discussed, it looks to me the only device that supports suspend is > > > > > simulator and it supports change properties. > > > > > > > > > > E.g: > > > > > > > > > > static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx, > > > > > u64 desc_area, u64 driver_area, > > > > > u64 device_area) > > > > > { > > > > > struct vdpasim *vdpasim = vdpa_to_sim(vdpa); > > > > > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > > > > > > > > > > vq->desc_addr = desc_area; > > > > > vq->driver_addr = driver_area; > > > > > vq->device_addr = device_area; > > > > > > > > > > return 0; > > > > > } > > > > > > > > > > > > > So in the current kernel master it is valid to set a different vq > > > > address while the device is suspended in vdpa_sim. But it is not valid > > > > in mlx5, as the FW will not be updated in resume (Dragos, please > > > > correct me if I'm wrong). Both of them return success. > > > > > > > In the current state, there is no resume. HW Virtqueues will just get re-created > > > with the new address. > > > > > > > Oh, then all of this is effectively transparent to the userspace > > except for the time it takes? > > > Not quite: mlx5_vdpa_set_vq_address will save the vq address only on the SW vq > representation. Only later will it will call into the FW to update the FW. Later > means: > - On DRIVER_OK state, when the VQs get created. > - On .set_map when the VQs get re-created (before this series) / updated (after > this series) > - On .resume (after this series). > > So if the .set_vq_address is called when the VQ is in DRIVER_OK but not > suspended those addresses will be set later for later. > Ouch, that is more in the line of my thoughts :(. > > In that case you're right, we don't need feature flags. But I think it > > would be great to also move the error return in case userspace tries > > to modify vq parameters out of suspend state. > > > On the driver side or on the core side? > Core side. It does not have to be part of this series, I meant it can be proposed in a separate series and applied before the parent driver one. > Thanks > > Thanks! > > > > > > > > How can we know in the destination QEMU if it is valid to suspend & > > > > set address? Should we handle this as a bugfix and backport the > > > > change? > > > > > > > > > > > > > > > > The only way that comes to my mind is to make sure all parents return > > > > > > error if userland tries to do it, and then fallback in userland. > > > > > > > > > > Yes. > > > > > > > > > > > I'm > > > > > > ok with that, but I'm not sure if the current master & previous kernel > > > > > > has a coherent behavior. Do they return error? Or return success > > > > > > without changing address / vq state? > > > > > > > > > > We probably don't need to worry too much here, as e.g set_vq_address > > > > > could fail even without suspend (just at uAPI level). > > > > > > > > > > > > > I don't get this, sorry. I rephrased my point with an example earlier > > > > in the mail. > > > > > > > > > >
On Thu, 2023-12-21 at 15:55 +0100, Eugenio Perez Martin wrote: > On Thu, Dec 21, 2023 at 3:38 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > On Thu, 2023-12-21 at 13:08 +0100, Eugenio Perez Martin wrote: > > > On Thu, Dec 21, 2023 at 12:52 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > On Thu, 2023-12-21 at 08:46 +0100, Eugenio Perez Martin wrote: > > > > > On Thu, Dec 21, 2023 at 3:03 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin > > > > > > <eperezma@redhat.com> wrote: > > > > > > > > > > > > > > On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > > > > > > > > DRIVER_OK. Some devices do support this operation when the device is > > > > > > > > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > > > > > > > > advertises this support as a backend features. > > > > > > > > > > > > > > > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > > > > > > > > > > > > > > > So I wonder if it's better to just allow such behaviour? > > > > > > > > > > > > > > > > Actually I mean, allow drivers to modify the parameters during suspend > > > > > > > > without a new feature. > > > > > > > > > > > > > > > > > > > > > > That would be ideal, but how do userland checks if it can suspend + > > > > > > > change properties + resume? > > > > > > > > > > > > As discussed, it looks to me the only device that supports suspend is > > > > > > simulator and it supports change properties. > > > > > > > > > > > > E.g: > > > > > > > > > > > > static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx, > > > > > > u64 desc_area, u64 driver_area, > > > > > > u64 device_area) > > > > > > { > > > > > > struct vdpasim *vdpasim = vdpa_to_sim(vdpa); > > > > > > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > > > > > > > > > > > > vq->desc_addr = desc_area; > > > > > > vq->driver_addr = driver_area; > > > > > > vq->device_addr = device_area; > > > > > > > > > > > > return 0; > > > > > > } > > > > > > > > > > > > > > > > So in the current kernel master it is valid to set a different vq > > > > > address while the device is suspended in vdpa_sim. But it is not valid > > > > > in mlx5, as the FW will not be updated in resume (Dragos, please > > > > > correct me if I'm wrong). Both of them return success. > > > > > > > > > In the current state, there is no resume. HW Virtqueues will just get re-created > > > > with the new address. > > > > > > > > > > Oh, then all of this is effectively transparent to the userspace > > > except for the time it takes? > > > > > Not quite: mlx5_vdpa_set_vq_address will save the vq address only on the SW vq > > representation. Only later will it will call into the FW to update the FW. Later > > means: > > - On DRIVER_OK state, when the VQs get created. > > - On .set_map when the VQs get re-created (before this series) / updated (after > > this series) > > - On .resume (after this series). > > > > So if the .set_vq_address is called when the VQ is in DRIVER_OK but not > > suspended those addresses will be set later for later. > > > > Ouch, that is more in the line of my thoughts :(. > > > > In that case you're right, we don't need feature flags. But I think it > > > would be great to also move the error return in case userspace tries > > > to modify vq parameters out of suspend state. > > > > > On the driver side or on the core side? > > > > Core side. > Checking my understanding: instead of the feature flags there would be a check (for .set_vq_addr and .set_vq_state) to return an error if they are called under DRIVER_OK and not suspended state? > It does not have to be part of this series, I meant it can be proposed > in a separate series and applied before the parent driver one. > > > Thanks > > > Thanks! > > > > > > > > > > > How can we know in the destination QEMU if it is valid to suspend & > > > > > set address? Should we handle this as a bugfix and backport the > > > > > change? > > > > > > > > > > > > > > > > > > > The only way that comes to my mind is to make sure all parents return > > > > > > > error if userland tries to do it, and then fallback in userland. > > > > > > > > > > > > Yes. > > > > > > > > > > > > > I'm > > > > > > > ok with that, but I'm not sure if the current master & previous kernel > > > > > > > has a coherent behavior. Do they return error? Or return success > > > > > > > without changing address / vq state? > > > > > > > > > > > > We probably don't need to worry too much here, as e.g set_vq_address > > > > > > could fail even without suspend (just at uAPI level). > > > > > > > > > > > > > > > > I don't get this, sorry. I rephrased my point with an example earlier > > > > > in the mail. > > > > > > > > > > > > > > >
On Thu, Dec 21, 2023 at 3:47 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Thu, Dec 21, 2023 at 3:03 AM Jason Wang <jasowang@redhat.com> wrote: > > > > On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin > > <eperezma@redhat.com> wrote: > > > > > > On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > > > > DRIVER_OK. Some devices do support this operation when the device is > > > > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > > > > advertises this support as a backend features. > > > > > > > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > > > > > > > So I wonder if it's better to just allow such behaviour? > > > > > > > > Actually I mean, allow drivers to modify the parameters during suspend > > > > without a new feature. > > > > > > > > > > That would be ideal, but how do userland checks if it can suspend + > > > change properties + resume? > > > > As discussed, it looks to me the only device that supports suspend is > > simulator and it supports change properties. > > > > E.g: > > > > static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx, > > u64 desc_area, u64 driver_area, > > u64 device_area) > > { > > struct vdpasim *vdpasim = vdpa_to_sim(vdpa); > > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > > > > vq->desc_addr = desc_area; > > vq->driver_addr = driver_area; > > vq->device_addr = device_area; > > > > return 0; > > } > > > > So in the current kernel master it is valid to set a different vq > address while the device is suspended in vdpa_sim. But it is not valid > in mlx5, as the FW will not be updated in resume (Dragos, please > correct me if I'm wrong). Both of them return success. > > How can we know in the destination QEMU if it is valid to suspend & > set address? Should we handle this as a bugfix and backport the > change? Good point. We probably need to do backport, this seems to be the easiest way. Theoretically, userspace may assume this behavior (though I don't think there would be a user that depends on the simulator). > > > > > > > The only way that comes to my mind is to make sure all parents return > > > error if userland tries to do it, and then fallback in userland. > > > > Yes. > > > > > I'm > > > ok with that, but I'm not sure if the current master & previous kernel > > > has a coherent behavior. Do they return error? Or return success > > > without changing address / vq state? > > > > We probably don't need to worry too much here, as e.g set_vq_address > > could fail even without suspend (just at uAPI level). > > > > I don't get this, sorry. I rephrased my point with an example earlier > in the mail. I mean currently, VHOST_SET_VRING_ADDR can fail. So userspace should not assume it will always succeed. Thanks >
On Thu, Dec 21, 2023 at 4:07 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > On Thu, 2023-12-21 at 15:55 +0100, Eugenio Perez Martin wrote: > > On Thu, Dec 21, 2023 at 3:38 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > On Thu, 2023-12-21 at 13:08 +0100, Eugenio Perez Martin wrote: > > > > On Thu, Dec 21, 2023 at 12:52 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > On Thu, 2023-12-21 at 08:46 +0100, Eugenio Perez Martin wrote: > > > > > > On Thu, Dec 21, 2023 at 3:03 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > On Wed, Dec 20, 2023 at 9:32 PM Eugenio Perez Martin > > > > > > > <eperezma@redhat.com> wrote: > > > > > > > > > > > > > > > > On Wed, Dec 20, 2023 at 5:06 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > > > > > On Wed, Dec 20, 2023 at 11:46 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > > > > > > > > > > > > On Wed, Dec 20, 2023 at 2:09 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > > > > > The virtio spec doesn't allow changing virtqueue addresses after > > > > > > > > > > > DRIVER_OK. Some devices do support this operation when the device is > > > > > > > > > > > suspended. The VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND flag > > > > > > > > > > > advertises this support as a backend features. > > > > > > > > > > > > > > > > > > > > There's an ongoing effort in virtio spec to introduce the suspend state. > > > > > > > > > > > > > > > > > > > > So I wonder if it's better to just allow such behaviour? > > > > > > > > > > > > > > > > > > Actually I mean, allow drivers to modify the parameters during suspend > > > > > > > > > without a new feature. > > > > > > > > > > > > > > > > > > > > > > > > > That would be ideal, but how do userland checks if it can suspend + > > > > > > > > change properties + resume? > > > > > > > > > > > > > > As discussed, it looks to me the only device that supports suspend is > > > > > > > simulator and it supports change properties. > > > > > > > > > > > > > > E.g: > > > > > > > > > > > > > > static int vdpasim_set_vq_address(struct vdpa_device *vdpa, u16 idx, > > > > > > > u64 desc_area, u64 driver_area, > > > > > > > u64 device_area) > > > > > > > { > > > > > > > struct vdpasim *vdpasim = vdpa_to_sim(vdpa); > > > > > > > struct vdpasim_virtqueue *vq = &vdpasim->vqs[idx]; > > > > > > > > > > > > > > vq->desc_addr = desc_area; > > > > > > > vq->driver_addr = driver_area; > > > > > > > vq->device_addr = device_area; > > > > > > > > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > > > > > > So in the current kernel master it is valid to set a different vq > > > > > > address while the device is suspended in vdpa_sim. But it is not valid > > > > > > in mlx5, as the FW will not be updated in resume (Dragos, please > > > > > > correct me if I'm wrong). Both of them return success. > > > > > > > > > > > In the current state, there is no resume. HW Virtqueues will just get re-created > > > > > with the new address. > > > > > > > > > > > > > Oh, then all of this is effectively transparent to the userspace > > > > except for the time it takes? > > > > > > > Not quite: mlx5_vdpa_set_vq_address will save the vq address only on the SW vq > > > representation. Only later will it will call into the FW to update the FW. Later > > > means: > > > - On DRIVER_OK state, when the VQs get created. > > > - On .set_map when the VQs get re-created (before this series) / updated (after > > > this series) > > > - On .resume (after this series). > > > > > > So if the .set_vq_address is called when the VQ is in DRIVER_OK but not > > > suspended those addresses will be set later for later. > > > > > > > Ouch, that is more in the line of my thoughts :(. > > > > > > In that case you're right, we don't need feature flags. But I think it > > > > would be great to also move the error return in case userspace tries > > > > to modify vq parameters out of suspend state. > > > > > > > On the driver side or on the core side? > > > > > > > Core side. > > > Checking my understanding: instead of the feature flags there would be a check > (for .set_vq_addr and .set_vq_state) to return an error if they are called under > DRIVER_OK and not suspended state? > Yes, correct. Per Jason's message, it should be enough with two independent series: * Patches 6, 7 and 8 of this series, just checking for suspend state and not feature flags. * Your v2. Thanks! > > It does not have to be part of this series, I meant it can be proposed > > in a separate series and applied before the parent driver one. > > > > > Thanks > > > > Thanks! > > > > > > > > > > > > > > How can we know in the destination QEMU if it is valid to suspend & > > > > > > set address? Should we handle this as a bugfix and backport the > > > > > > change? > > > > > > > > > > > > > > > > > > > > > > The only way that comes to my mind is to make sure all parents return > > > > > > > > error if userland tries to do it, and then fallback in userland. > > > > > > > > > > > > > > Yes. > > > > > > > > > > > > > > > I'm > > > > > > > > ok with that, but I'm not sure if the current master & previous kernel > > > > > > > > has a coherent behavior. Do they return error? Or return success > > > > > > > > without changing address / vq state? > > > > > > > > > > > > > > We probably don't need to worry too much here, as e.g set_vq_address > > > > > > > could fail even without suspend (just at uAPI level). > > > > > > > > > > > > > > > > > > > I don't get this, sorry. I rephrased my point with an example earlier > > > > > > in the mail. > > > > > > > > > > > > > > > > > > > > >
On Thu, Dec 21, 2023 at 03:07:22PM +0000, Dragos Tatulea wrote: > > > > In that case you're right, we don't need feature flags. But I think it > > > > would be great to also move the error return in case userspace tries > > > > to modify vq parameters out of suspend state. > > > > > > > On the driver side or on the core side? > > > > > > > Core side. > > > Checking my understanding: instead of the feature flags there would be a check > (for .set_vq_addr and .set_vq_state) to return an error if they are called under > DRIVER_OK and not suspended state? Yea this looks much saner, if we start adding feature flags for each OPERATION_X_LEGAL_IN_STATE_Y then we will end up with N^2 feature bits which is not reasonable.
On Fri, 2023-12-22 at 03:29 -0500, Michael S. Tsirkin wrote: > On Thu, Dec 21, 2023 at 03:07:22PM +0000, Dragos Tatulea wrote: > > > > > In that case you're right, we don't need feature flags. But I think it > > > > > would be great to also move the error return in case userspace tries > > > > > to modify vq parameters out of suspend state. > > > > > > > > > On the driver side or on the core side? > > > > > > > > > > Core side. > > > > > Checking my understanding: instead of the feature flags there would be a check > > (for .set_vq_addr and .set_vq_state) to return an error if they are called under > > DRIVER_OK and not suspended state? > > Yea this looks much saner, if we start adding feature flags for > each OPERATION_X_LEGAL_IN_STATE_Y then we will end up with N^2 > feature bits which is not reasonable. > Ack. Is the v2 enough or should I respin a v5 with the updated Acked-by tags? I will prepare the core part as a different series without the flags. Thanks, Dragos
On Fri, 2023-12-22 at 11:51 +0100, Dragos Tatulea wrote: > On Fri, 2023-12-22 at 03:29 -0500, Michael S. Tsirkin wrote: > > On Thu, Dec 21, 2023 at 03:07:22PM +0000, Dragos Tatulea wrote: > > > > > > In that case you're right, we don't need feature flags. But I think it > > > > > > would be great to also move the error return in case userspace tries > > > > > > to modify vq parameters out of suspend state. > > > > > > > > > > > On the driver side or on the core side? > > > > > > > > > > > > > Core side. > > > > > > > Checking my understanding: instead of the feature flags there would be a check > > > (for .set_vq_addr and .set_vq_state) to return an error if they are called under > > > DRIVER_OK and not suspended state? > > > > Yea this looks much saner, if we start adding feature flags for > > each OPERATION_X_LEGAL_IN_STATE_Y then we will end up with N^2 > > feature bits which is not reasonable. > > > Ack. Is the v2 enough or should I respin a v5 with the updated Acked-by tags? > > I will prepare the core part as a different series without the flags. > Core part sent: https://lore.kernel.org/virtualization/20231225134210.151540-1-dtatulea@nvidia.com/T/#t I also have a v2 respin with extra Acked-by tags if necessary as a v5. Just let me know if it is needed. Thanks, Dragos
diff --git a/include/uapi/linux/vhost_types.h b/include/uapi/linux/vhost_types.h index d7656908f730..aacd067afc89 100644 --- a/include/uapi/linux/vhost_types.h +++ b/include/uapi/linux/vhost_types.h @@ -192,5 +192,9 @@ struct vhost_vdpa_iova_range { #define VHOST_BACKEND_F_DESC_ASID 0x7 /* IOTLB don't flush memory mapping across device reset */ #define VHOST_BACKEND_F_IOTLB_PERSIST 0x8 +/* Device supports changing virtqueue addresses when device is suspended + * and is in state DRIVER_OK. + */ +#define VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND 0x9 #endif