Message ID | 20231205104609.876194-5-dtatulea@nvidia.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp3342397vqy; Tue, 5 Dec 2023 02:47:27 -0800 (PST) X-Google-Smtp-Source: AGHT+IHaYuQVG407nJTHmRNs5I3QTbWKJH1NfZmEaP6Al6U9nHfKbWSfQYIqzZByWCtEY++VFvGM X-Received: by 2002:a05:6a20:a19b:b0:18f:97c:927e with SMTP id r27-20020a056a20a19b00b0018f097c927emr2301752pzk.99.1701773247107; Tue, 05 Dec 2023 02:47:27 -0800 (PST) ARC-Seal: i=2; a=rsa-sha256; t=1701773247; cv=pass; d=google.com; s=arc-20160816; b=yUq77W25EvYyqXEtoCr4Q4wKokF+AV5UrtMRPZ7XMCLE5EtrzLsRpwZrtdXUepbb67 qDg+sXQQzaylJabVZFSoTUpFQ73pghw5p+axnXmnwV2mKBDiu9LyN4XqRkjVLKwIPnQE hWf5HpzAj/J00uFcR0Zr6WNHRJmpYiKKma3aMoedSfaQQZP7LsZa524j6wzzFP8wmyFj hPeu3yWAIpbKYmL1Ztp4rOksAEztGouGRiy96CElIZBrvoMkcu0O0VJMV5D7TH2Bwk+y cwCG5kbYvY/DRhYtNjRts9IxGoOh4dCHrdikMuiHmFUUvotMz1B5dKYV1dmi21YhVzkx ScWQ== ARC-Message-Signature: i=2; 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=CGyLeP1cFq3pBUJz5P3wMAmQBwNy91SdqMFyXqiZ1nM=; fh=IC8CtaUsCHH+VLkdER9IbUix5yCQzPbBUzKtIdyo0ag=; b=j8fsTWNiTGnNO+j9meodfHZDUshSBySAhOVAYMOCLArx8W60kwTC8IZ39ecLljRdJJ O6miEXeK3iTQZIPw+wI119GYslinhp6aJtNxh2mibZQo76f1npRrxY6MciJhhcnws4em 94Jbyb7adbKSdQ/bLpwHmo3Ryxx2PeXQhezRXUjDyK3YJ/0tjZTWJwjlZNZkg55tkl5j hyhfMMMKSm9NAc6HfZ6uJcL2R/IXrcKYOuzbQshZIMUCR+WQ0EcpHZnw/pRIsd/emxHp Xq2wf8FIZML7rOAbQwyOBhbttx4rzhdlLdY3D6h088fm2L323UKVsDHsEy82VyePtzXU bo6Q== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@Nvidia.com header.s=selector2 header.b=S5znUFSZ; arc=pass (i=1 spf=pass spfdomain=nvidia.com dmarc=pass fromdomain=nvidia.com); spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=nvidia.com Received: from lipwig.vger.email (lipwig.vger.email. [23.128.96.33]) by mx.google.com with ESMTPS id c19-20020a056a000ad300b006baae7ecfacsi9596422pfl.308.2023.12.05.02.47.26 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Dec 2023 02:47:27 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33; Authentication-Results: mx.google.com; dkim=pass header.i=@Nvidia.com header.s=selector2 header.b=S5znUFSZ; arc=pass (i=1 spf=pass spfdomain=nvidia.com dmarc=pass fromdomain=nvidia.com); spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=nvidia.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by lipwig.vger.email (Postfix) with ESMTP id 1E30F80403B1; Tue, 5 Dec 2023 02:47:22 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346727AbjLEKq4 (ORCPT <rfc822;chrisfriedt@gmail.com> + 99 others); Tue, 5 Dec 2023 05:46:56 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33844 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1346605AbjLEKqs (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 5 Dec 2023 05:46:48 -0500 Received: from NAM04-BN8-obe.outbound.protection.outlook.com (mail-bn8nam04on2059.outbound.protection.outlook.com [40.107.100.59]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3815E116; Tue, 5 Dec 2023 02:46:51 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Nd/QRdGUnQ8xPL+rTevk8YIRcku05uvXs/pA8STLkTdMdrMLPKx4YtzETSVUhxluMPOflgFcV5z/vaJaHs7ugMAJLp2LuzWJ8F/FtevMGSTPf3cysjYEiMlE5kaZL1cmpfamtJA3jitJVCqPk36oD+e0baIVusF2xz3TnRATeczkdIl+H4MEJ37EueF3kfQa3qkGH0Ipk3Z9C4vaVPh8WSXxMaiurzm5WOPkEIQ6snsQVbiXPjAoJKsXNmVOJdSfQpP2hWCz41kCtjGcJhaGBlPjbDq5lOgNQOYapnaYkPV4uyl62MDHcF0gSjqqv/JFMTzMOp6fFoVo3qksC5ivaw== 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=CGyLeP1cFq3pBUJz5P3wMAmQBwNy91SdqMFyXqiZ1nM=; b=gEI6R/2J8QsIeczPvJuycVTvl4R0lUNsOL+CSG6Lu2/dRGiSsXrCRLv0P4FH9fOPaRW/X3h5656+Z1KeQ8cEtW9Tfs9igTXVd/IFF1h58DfY2n28TZJs0tCufezTC320Gp34MSsT6mV3VV1x3g01mqpQUZEO3+Z86mE+FtikkmFKXg11Ti1u+Oo6QVPSyPgpgpVNLODcBT9gzIVyDLFeMUkTCQpAdyNaoYg868FNLmDg5KDOJzSfg8wbb30OtauQZbSSClc9U0ypJtcsuvUFPOBSMzmtSoBG7ise2Hf8Pg6TH86zV+ZpytasNywW2Zto6UBT+64pHhClBUmeG5zvRA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 216.228.117.160) 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=CGyLeP1cFq3pBUJz5P3wMAmQBwNy91SdqMFyXqiZ1nM=; b=S5znUFSZ/SWyNvsvKeHgYthaBbub6BHP/9ttLGhwrMp9LX1YMfTQybApujyBfnfEpPczPXkW4NNTNts1M27Ger27da5vaoFCyDKb5L/mPQ8XPgyN5XEmkDLWXNkMPEJcQVdO//z3spb73WykVoJyufp2IHhO8ByDZwVI5bIYmxoiTlD4Gc7HwOWjaNY9xcFzXRK2Hvn+L+0ZdxXf7H2AuxsHTkluNC7mQbdAMrT0byiVRUkUKqRWKF80+y4EsIkM6/Go4VA2TEWcGsU7G9g+Jfvx2Utzst1aYQlLzLl2OOJMy7e/dynHbBJTUHkWpLfEt4sjkEpWrC3bYcNQtfC6OQ== Received: from PR0P264CA0168.FRAP264.PROD.OUTLOOK.COM (2603:10a6:100:1b::36) by DM4PR12MB5358.namprd12.prod.outlook.com (2603:10b6:5:39c::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7046.34; Tue, 5 Dec 2023 10:46:48 +0000 Received: from SA2PEPF000015C8.namprd03.prod.outlook.com (2603:10a6:100:1b:cafe::86) by PR0P264CA0168.outlook.office365.com (2603:10a6:100:1b::36) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7046.34 via Frontend Transport; Tue, 5 Dec 2023 10:46:47 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 216.228.117.160) 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.160 as permitted sender) receiver=protection.outlook.com; client-ip=216.228.117.160; helo=mail.nvidia.com; pr=C Received: from mail.nvidia.com (216.228.117.160) by SA2PEPF000015C8.mail.protection.outlook.com (10.167.241.198) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7068.20 via Frontend Transport; Tue, 5 Dec 2023 10:46:47 +0000 Received: from rnnvmail201.nvidia.com (10.129.68.8) by mail.nvidia.com (10.129.200.66) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.41; Tue, 5 Dec 2023 02:46:35 -0800 Received: from rnnvmail201.nvidia.com (10.129.68.8) by rnnvmail201.nvidia.com (10.129.68.8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.986.41; Tue, 5 Dec 2023 02:46:34 -0800 Received: from c-237-113-220-225.mtl.labs.mlnx (10.127.8.12) by mail.nvidia.com (10.129.68.8) with Microsoft SMTP Server id 15.2.986.41 via Frontend Transport; Tue, 5 Dec 2023 02:46:31 -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 v2 4/8] vdpa/mlx5: Mark vq addrs for modification in hw vq Date: Tue, 5 Dec 2023 12:46:05 +0200 Message-ID: <20231205104609.876194-5-dtatulea@nvidia.com> X-Mailer: git-send-email 2.42.0 In-Reply-To: <20231205104609.876194-1-dtatulea@nvidia.com> References: <20231205104609.876194-1-dtatulea@nvidia.com> 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: SA2PEPF000015C8:EE_|DM4PR12MB5358:EE_ X-MS-Office365-Filtering-Correlation-Id: 2308df19-1e91-44aa-fea8-08dbf57f7afc X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: SIOSg1xefXfBdhNPxegUmam1vABP2JvBQiKBvdrNplFN7FYWGNgM5QCuf7HJWjXnsiEBUuAw983kaW97FWVwDNUVTs6JMUyFea4LxBLWE7gIaqb15a/Uo9/+M8iy6/HRs4xkkoksm0FmsQYPpNk1DUy1v2XnRZlK821qWDdWgYaMsEeRFZ18QsT5KcitKqGQBlKHngvrWeBgmxtHUGFnHxa11vkvHdH6Ch3Y3f3e//54qe6jAcmn72TpUqJm7ttEAqoMjde81uB+7Fii+vczywoiLF6YFHyWG780tx7EZYU7nZd7b5t8RKkyJeddsLmoGhgFn7wQOzPFFJ4ZMZjdg7EUbtv/sUKgA0Y3pG7lAGCLkich8UlIei65M9G2Uy5ox18N0xyrowtGagUfWul7XNJDBLvL3mwlD8xGdnjOCwTx0L56HEJrCGD1sd/p/SFZk1+raDw0SHCBcphMX9Pji7UBZtyXvLkiuHsGNlURYkbDDVOJ9T2KRtD1Q5uzKCtjQPeXFIIZS6zxzOENCYd/+m1l/QFwOrB40nPwNp9aukbCTKBAffmDND6ItBTShHVZ1T6k0akrmNz6By5OgU3sCiu86nIULzcN0xy50l4b42V2zH1VX0J1H2uA2mAnefhlgwXFc6k5MPIkzb8KCv8W15XAKFRMhqUJaeEB0l97yWYvRjjjWQu7AlhbcE3lBCwHCZux8YMAitKmeRsvU7U1VS0MJpBOYEYHbnjo/Y+vQNccupHsj69XECqUOAFlr2pN X-Forefront-Antispam-Report: CIP:216.228.117.160;CTRY:US;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:mail.nvidia.com;PTR:dc6edge1.nvidia.com;CAT:NONE;SFS:(13230031)(4636009)(136003)(346002)(376002)(396003)(39860400002)(230922051799003)(451199024)(82310400011)(186009)(1800799012)(64100799003)(40470700004)(46966006)(36840700001)(40460700003)(54906003)(316002)(86362001)(478600001)(8936002)(8676002)(6636002)(4326008)(70586007)(110136005)(70206006)(41300700001)(36756003)(5660300002)(2906002)(36860700001)(356005)(7636003)(47076005)(2616005)(26005)(1076003)(6666004)(82740400003)(83380400001)(336012)(426003)(40480700001);DIR:OUT;SFP:1101; X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Dec 2023 10:46:47.4703 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: 2308df19-1e91-44aa-fea8-08dbf57f7afc 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.160];Helo=[mail.nvidia.com] X-MS-Exchange-CrossTenant-AuthSource: SA2PEPF000015C8.namprd03.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5358 X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Tue, 05 Dec 2023 02:47:22 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784438584574306047 X-GMAIL-MSGID: 1784438584574306047 |
Series |
vdpa/mlx5: Add support for resumable vqs
|
|
Commit Message
Dragos Tatulea
Dec. 5, 2023, 10:46 a.m. UTC
Addresses get set by .set_vq_address. hw vq addresses will be updated on next modify_virtqueue. Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> Reviewed-by: Gal Pressman <gal@nvidia.com> Acked-by: Eugenio Pérez <eperezma@redhat.com> --- drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + 2 files changed, 10 insertions(+)
Comments
On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > next modify_virtqueue. > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > Reviewed-by: Gal Pressman <gal@nvidia.com> > Acked-by: Eugenio Pérez <eperezma@redhat.com> I'm kind of ok with this patch and the next one about state, but I didn't ack them in the previous series. My main concern is that it is not valid to change the vq address after DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to change at this moment. I'm not sure about vq state in vDPA, but vhost forbids changing it with an active backend. Suspend is not defined in VirtIO at this moment though, so maybe it is ok to decide that all of these parameters may change during suspend. Maybe the best thing is to protect this with a vDPA feature flag. Jason, what do you think? Thanks! > --- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index f8f088cced50..80e066de0866 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > bool state_change = false; > void *obj_context; > void *cmd_hdr; > + void *vq_ctx; > void *in; > int err; > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > state_change = true; > } > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > + } > + > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > if (err) > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > mvq->desc_addr = desc_area; > mvq->device_addr = device_area; > mvq->driver_addr = driver_area; > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > return 0; > } > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > index b86d51a855f6..9594ac405740 100644 > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > @@ -145,6 +145,7 @@ enum { > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > }; > > -- > 2.42.0 >
On Tue, 2023-12-12 at 20:21 +0100, Eugenio Perez Martin wrote: > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > next modify_virtqueue. > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > I'm kind of ok with this patch and the next one about state, but I > didn't ack them in the previous series. > Sorry about the Ack misplacement. I got confused. > My main concern is that it is not valid to change the vq address after > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > change at this moment. I'm not sure about vq state in vDPA, but vhost > forbids changing it with an active backend. > > Suspend is not defined in VirtIO at this moment though, so maybe it is > ok to decide that all of these parameters may change during suspend. > Maybe the best thing is to protect this with a vDPA feature flag. > > Jason, what do you think? > > Thanks! > > > --- > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > 2 files changed, 10 insertions(+) > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > index f8f088cced50..80e066de0866 100644 > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > bool state_change = false; > > void *obj_context; > > void *cmd_hdr; > > + void *vq_ctx; > > void *in; > > int err; > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > state_change = true; > > } > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > + } > > + > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > if (err) > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > mvq->desc_addr = desc_area; > > mvq->device_addr = device_area; > > mvq->driver_addr = driver_area; > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > return 0; > > } > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > index b86d51a855f6..9594ac405740 100644 > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > @@ -145,6 +145,7 @@ enum { > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > }; > > > > -- > > 2.42.0 > > >
On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: >> Addresses get set by .set_vq_address. hw vq addresses will be updated on >> next modify_virtqueue. >> >> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> >> Reviewed-by: Gal Pressman <gal@nvidia.com> >> Acked-by: Eugenio Pérez <eperezma@redhat.com> > I'm kind of ok with this patch and the next one about state, but I > didn't ack them in the previous series. > > My main concern is that it is not valid to change the vq address after > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > change at this moment. I'm not sure about vq state in vDPA, but vhost > forbids changing it with an active backend. > > Suspend is not defined in VirtIO at this moment though, so maybe it is > ok to decide that all of these parameters may change during suspend. > Maybe the best thing is to protect this with a vDPA feature flag. I think protect with vDPA feature flag could work, while on the other hand vDPA means vendor specific optimization is possible around suspend and resume (in case it helps performance), which doesn't have to be backed by virtio spec. Same applies to vhost user backend features, variations there were not backed by spec either. Of course, we should try best to make the default behavior backward compatible with virtio-based backend, but that circles back to no suspend definition in the current virtio spec, for which I hope we don't cease development on vDPA indefinitely. After all, the virtio based vdap backend can well define its own feature flag to describe (minor difference in) the suspend behavior based on the later spec once it is formed in future. Regards, -Siwei > > Jason, what do you think? > > Thanks! > >> --- >> drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ >> include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + >> 2 files changed, 10 insertions(+) >> >> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c >> index f8f088cced50..80e066de0866 100644 >> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c >> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c >> @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, >> bool state_change = false; >> void *obj_context; >> void *cmd_hdr; >> + void *vq_ctx; >> void *in; >> int err; >> >> @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, >> MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); >> >> obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); >> + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); >> >> if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { >> if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { >> @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, >> state_change = true; >> } >> >> + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { >> + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); >> + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); >> + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); >> + } >> + >> MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); >> err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); >> if (err) >> @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ >> mvq->desc_addr = desc_area; >> mvq->device_addr = device_area; >> mvq->driver_addr = driver_area; >> + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; >> return 0; >> } >> >> diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h >> index b86d51a855f6..9594ac405740 100644 >> --- a/include/linux/mlx5/mlx5_ifc_vdpa.h >> +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h >> @@ -145,6 +145,7 @@ enum { >> MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, >> MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, >> MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, >> + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, >> MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, >> }; >> >> -- >> 2.42.0 >>
On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > next modify_virtqueue. > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > I'm kind of ok with this patch and the next one about state, but I > > didn't ack them in the previous series. > > > > My main concern is that it is not valid to change the vq address after > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > forbids changing it with an active backend. > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > ok to decide that all of these parameters may change during suspend. > > Maybe the best thing is to protect this with a vDPA feature flag. > I think protect with vDPA feature flag could work, while on the other > hand vDPA means vendor specific optimization is possible around suspend > and resume (in case it helps performance), which doesn't have to be > backed by virtio spec. Same applies to vhost user backend features, > variations there were not backed by spec either. Of course, we should > try best to make the default behavior backward compatible with > virtio-based backend, but that circles back to no suspend definition in > the current virtio spec, for which I hope we don't cease development on > vDPA indefinitely. After all, the virtio based vdap backend can well > define its own feature flag to describe (minor difference in) the > suspend behavior based on the later spec once it is formed in future. > So what is the way forward here? From what I understand the options are: 1) Add a vdpa feature flag for changing device properties while suspended. 2) Drop these 2 patches from the series for now. Not sure if this makes sense as this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this code won't work anymore. This means the series would be less well tested. Are there other possible options? What do you think? [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip Thanks, Dragos > Regards, > -Siwei > > > > > > > Jason, what do you think? > > > > Thanks! > > > > > --- > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > > 2 files changed, 10 insertions(+) > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > index f8f088cced50..80e066de0866 100644 > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > bool state_change = false; > > > void *obj_context; > > > void *cmd_hdr; > > > + void *vq_ctx; > > > void *in; > > > int err; > > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > state_change = true; > > > } > > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > + } > > > + > > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > > if (err) > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > > mvq->desc_addr = desc_area; > > > mvq->device_addr = device_area; > > > mvq->driver_addr = driver_area; > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > > return 0; > > > } > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > index b86d51a855f6..9594ac405740 100644 > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > @@ -145,6 +145,7 @@ enum { > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > > }; > > > > > > -- > > > 2.42.0 > > > >
On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > next modify_virtqueue. > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > I'm kind of ok with this patch and the next one about state, but I > > > didn't ack them in the previous series. > > > > > > My main concern is that it is not valid to change the vq address after > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > forbids changing it with an active backend. > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > ok to decide that all of these parameters may change during suspend. > > > Maybe the best thing is to protect this with a vDPA feature flag. > > I think protect with vDPA feature flag could work, while on the other > > hand vDPA means vendor specific optimization is possible around suspend > > and resume (in case it helps performance), which doesn't have to be > > backed by virtio spec. Same applies to vhost user backend features, > > variations there were not backed by spec either. Of course, we should > > try best to make the default behavior backward compatible with > > virtio-based backend, but that circles back to no suspend definition in > > the current virtio spec, for which I hope we don't cease development on > > vDPA indefinitely. After all, the virtio based vdap backend can well > > define its own feature flag to describe (minor difference in) the > > suspend behavior based on the later spec once it is formed in future. > > > So what is the way forward here? From what I understand the options are: > > 1) Add a vdpa feature flag for changing device properties while suspended. > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > code won't work anymore. This means the series would be less well tested. > > Are there other possible options? What do you think? > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip I am fine with either of these. > Thanks, > Dragos > > > Regards, > > -Siwei > > > > > > > > > > > > Jason, what do you think? > > > > > > Thanks! > > > > > > > --- > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > > > 2 files changed, 10 insertions(+) > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > index f8f088cced50..80e066de0866 100644 > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > bool state_change = false; > > > > void *obj_context; > > > > void *cmd_hdr; > > > > + void *vq_ctx; > > > > void *in; > > > > int err; > > > > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > state_change = true; > > > > } > > > > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > > + } > > > > + > > > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > > > if (err) > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > > > mvq->desc_addr = desc_area; > > > > mvq->device_addr = device_area; > > > > mvq->driver_addr = driver_area; > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > > > return 0; > > > > } > > > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > index b86d51a855f6..9594ac405740 100644 > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > @@ -145,6 +145,7 @@ enum { > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > > > }; > > > > > > > > -- > > > > 2.42.0 > > > > > > >
On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > next modify_virtqueue. > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > I'm kind of ok with this patch and the next one about state, but I > > > > didn't ack them in the previous series. > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > forbids changing it with an active backend. > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > ok to decide that all of these parameters may change during suspend. > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > I think protect with vDPA feature flag could work, while on the other > > > hand vDPA means vendor specific optimization is possible around suspend > > > and resume (in case it helps performance), which doesn't have to be > > > backed by virtio spec. Same applies to vhost user backend features, > > > variations there were not backed by spec either. Of course, we should > > > try best to make the default behavior backward compatible with > > > virtio-based backend, but that circles back to no suspend definition in > > > the current virtio spec, for which I hope we don't cease development on > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > define its own feature flag to describe (minor difference in) the > > > suspend behavior based on the later spec once it is formed in future. > > > > > So what is the way forward here? From what I understand the options are: > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > code won't work anymore. This means the series would be less well tested. > > > > Are there other possible options? What do you think? > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > I am fine with either of these. > How about allowing the change only under the following conditions: vhost_vdpa_can_suspend && vhost_vdpa_can_resume && VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set ? Thanks, Dragos > > Thanks, > > Dragos > > > > > Regards, > > > -Siwei > > > > > > > > > > > > > > > > > Jason, what do you think? > > > > > > > > Thanks! > > > > > > > > > --- > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > index f8f088cced50..80e066de0866 100644 > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > bool state_change = false; > > > > > void *obj_context; > > > > > void *cmd_hdr; > > > > > + void *vq_ctx; > > > > > void *in; > > > > > int err; > > > > > > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > > > > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > > > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > state_change = true; > > > > > } > > > > > > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > > > + } > > > > > + > > > > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > > > > if (err) > > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > > > > mvq->desc_addr = desc_area; > > > > > mvq->device_addr = device_area; > > > > > mvq->driver_addr = driver_area; > > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > > > > return 0; > > > > > } > > > > > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > index b86d51a855f6..9594ac405740 100644 > > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > @@ -145,6 +145,7 @@ enum { > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > > > > }; > > > > > > > > > > -- > > > > > 2.42.0 > > > > > > > > > > >
On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > next modify_virtqueue. > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > didn't ack them in the previous series. > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > forbids changing it with an active backend. > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > ok to decide that all of these parameters may change during suspend. > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > I think protect with vDPA feature flag could work, while on the other > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > and resume (in case it helps performance), which doesn't have to be > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > variations there were not backed by spec either. Of course, we should > > > > try best to make the default behavior backward compatible with > > > > virtio-based backend, but that circles back to no suspend definition in > > > > the current virtio spec, for which I hope we don't cease development on > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > define its own feature flag to describe (minor difference in) the > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > code won't work anymore. This means the series would be less well tested. > > > > > > Are there other possible options? What do you think? > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > I am fine with either of these. > > > How about allowing the change only under the following conditions: > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > ? I think the best option by far is 1, as there is no hint in the combination of these 3 indicating that you can change device properties in the suspended state. > > Thanks, > Dragos > > > > Thanks, > > > Dragos > > > > > > > Regards, > > > > -Siwei > > > > > > > > > > > > > > > > > > > > > > Jason, what do you think? > > > > > > > > > > Thanks! > > > > > > > > > > > --- > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > index f8f088cced50..80e066de0866 100644 > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > bool state_change = false; > > > > > > void *obj_context; > > > > > > void *cmd_hdr; > > > > > > + void *vq_ctx; > > > > > > void *in; > > > > > > int err; > > > > > > > > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > > > > > > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > > > > > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > state_change = true; > > > > > > } > > > > > > > > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > > > > + } > > > > > > + > > > > > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > > > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > > > > > if (err) > > > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > > > > > mvq->desc_addr = desc_area; > > > > > > mvq->device_addr = device_area; > > > > > > mvq->driver_addr = driver_area; > > > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > > > > > return 0; > > > > > > } > > > > > > > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > index b86d51a855f6..9594ac405740 100644 > > > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > @@ -145,6 +145,7 @@ enum { > > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > > > > > }; > > > > > > > > > > > > -- > > > > > > 2.42.0 > > > > > > > > > > > > > > > >
On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > variations there were not backed by spec either. Of course, we should > > > > > try best to make the default behavior backward compatible with > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > define its own feature flag to describe (minor difference in) the > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > Are there other possible options? What do you think? > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > I am fine with either of these. > > > > > How about allowing the change only under the following conditions: > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > ? > > I think the best option by far is 1, as there is no hint in the > combination of these 3 indicating that you can change device > properties in the suspended state. > Sure. Will respin a v3 without these two patches. Another series can implement option 2 and add these 2 patches on top. > > > > Thanks, > > Dragos > > > > > > Thanks, > > > > Dragos > > > > > > > > > Regards, > > > > > -Siwei > > > > > > > > > > > > > > > > > > > > > > > > > > > Jason, what do you think? > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > --- > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > index f8f088cced50..80e066de0866 100644 > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > bool state_change = false; > > > > > > > void *obj_context; > > > > > > > void *cmd_hdr; > > > > > > > + void *vq_ctx; > > > > > > > void *in; > > > > > > > int err; > > > > > > > > > > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > > > > > > > > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > > > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > > > > > > > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > > > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > > > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > state_change = true; > > > > > > > } > > > > > > > > > > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > > > > > + } > > > > > > > + > > > > > > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > > > > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > > > > > > if (err) > > > > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > > > > > > mvq->desc_addr = desc_area; > > > > > > > mvq->device_addr = device_area; > > > > > > > mvq->driver_addr = driver_area; > > > > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > index b86d51a855f6..9594ac405740 100644 > > > > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > @@ -145,6 +145,7 @@ enum { > > > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > > > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > > > > > > }; > > > > > > > > > > > > > > -- > > > > > > > 2.42.0 > > > > > > > > > > > > > > > > > > > > > >
On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > > variations there were not backed by spec either. Of course, we should > > > > > > try best to make the default behavior backward compatible with > > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > > define its own feature flag to describe (minor difference in) the > > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > > > Are there other possible options? What do you think? > > > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > I am fine with either of these. > > > > > > > How about allowing the change only under the following conditions: > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > ? > > > > I think the best option by far is 1, as there is no hint in the > > combination of these 3 indicating that you can change device > > properties in the suspended state. > > > Sure. Will respin a v3 without these two patches. > > Another series can implement option 2 and add these 2 patches on top. Hmm...I misunderstood your statement and sent a erroneus v3. You said that having a feature flag is the best option. Will add a feature flag in v4: is this similar to the VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? Thanks, Dragos > > > Thanks, > > > Dragos > > > > > > > > Thanks, > > > > > Dragos > > > > > > > > > > > Regards, > > > > > > -Siwei > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Jason, what do you think? > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > --- > > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > > > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > index f8f088cced50..80e066de0866 100644 > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > bool state_change = false; > > > > > > > > void *obj_context; > > > > > > > > void *cmd_hdr; > > > > > > > > + void *vq_ctx; > > > > > > > > void *in; > > > > > > > > int err; > > > > > > > > > > > > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > > > > > > > > > > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > > > > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > > > > > > > > > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > > > > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > > > > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > state_change = true; > > > > > > > > } > > > > > > > > > > > > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > > > > > > + } > > > > > > > > + > > > > > > > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > > > > > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > > > > > > > if (err) > > > > > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > > > > > > > mvq->desc_addr = desc_area; > > > > > > > > mvq->device_addr = device_area; > > > > > > > > mvq->driver_addr = driver_area; > > > > > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > > > > > > > return 0; > > > > > > > > } > > > > > > > > > > > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > index b86d51a855f6..9594ac405740 100644 > > > > > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > @@ -145,6 +145,7 @@ enum { > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > > > > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > > > > > > > }; > > > > > > > > > > > > > > > > -- > > > > > > > > 2.42.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > > > variations there were not backed by spec either. Of course, we should > > > > > > > try best to make the default behavior backward compatible with > > > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > > > define its own feature flag to describe (minor difference in) the > > > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > > > > > Are there other possible options? What do you think? > > > > > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > > > I am fine with either of these. > > > > > > > > > How about allowing the change only under the following conditions: > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > > > ? > > > > > > I think the best option by far is 1, as there is no hint in the > > > combination of these 3 indicating that you can change device > > > properties in the suspended state. > > > > > Sure. Will respin a v3 without these two patches. > > > > Another series can implement option 2 and add these 2 patches on top. > Hmm...I misunderstood your statement and sent a erroneus v3. You said that > having a feature flag is the best option. > > Will add a feature flag in v4: is this similar to the > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? > Right, it should be easy to return it from .get_backend_features op if the FW returns that capability, isn't it? > Thanks, > Dragos > > > > > Thanks, > > > > Dragos > > > > > > > > > > Thanks, > > > > > > Dragos > > > > > > > > > > > > > Regards, > > > > > > > -Siwei > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Jason, what do you think? > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > --- > > > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > > > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > > > > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > index f8f088cced50..80e066de0866 100644 > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > bool state_change = false; > > > > > > > > > void *obj_context; > > > > > > > > > void *cmd_hdr; > > > > > > > > > + void *vq_ctx; > > > > > > > > > void *in; > > > > > > > > > int err; > > > > > > > > > > > > > > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > > > > > > > > > > > > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > > > > > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > > > > > > > > > > > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > > > > > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > > > > > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > state_change = true; > > > > > > > > > } > > > > > > > > > > > > > > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > > > > > > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > > > > > > > > if (err) > > > > > > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > > > > > > > > mvq->desc_addr = desc_area; > > > > > > > > > mvq->device_addr = device_area; > > > > > > > > > mvq->driver_addr = driver_area; > > > > > > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > > > > > > > > return 0; > > > > > > > > > } > > > > > > > > > > > > > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > index b86d51a855f6..9594ac405740 100644 > > > > > > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > @@ -145,6 +145,7 @@ enum { > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > > > > > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > > > > > > > > }; > > > > > > > > > > > > > > > > > > -- > > > > > > > > > 2.42.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Fri, 2023-12-15 at 18:56 +0100, Eugenio Perez Martin wrote: > On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > > > > variations there were not backed by spec either. Of course, we should > > > > > > > > try best to make the default behavior backward compatible with > > > > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > > > > define its own feature flag to describe (minor difference in) the > > > > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > > > > > > > Are there other possible options? What do you think? > > > > > > > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > > > > > I am fine with either of these. > > > > > > > > > > > How about allowing the change only under the following conditions: > > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > > > > > ? > > > > > > > > I think the best option by far is 1, as there is no hint in the > > > > combination of these 3 indicating that you can change device > > > > properties in the suspended state. > > > > > > > Sure. Will respin a v3 without these two patches. > > > > > > Another series can implement option 2 and add these 2 patches on top. > > Hmm...I misunderstood your statement and sent a erroneus v3. You said that > > having a feature flag is the best option. > > > > Will add a feature flag in v4: is this similar to the > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? > > > > Right, it should be easy to return it from .get_backend_features op if > the FW returns that capability, isn't it? > Yes, that's easy. But I wonder if we need one feature bit for each type of change: - VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND - VHOST_BACKEND_F_CHANGEABLE_VQ_STATE_IN_SUSPEND Or would a big one VHOST_BACKEND_F_CAN_RECONFIG_VQ_IN_SUSPEND suffice? To me having individual feature bits makes sense. But it could also takes too many bits if more changes are required. Thanks, Dragos > > Thanks, > > Dragos > > > > > > > Thanks, > > > > > Dragos > > > > > > > > > > > > Thanks, > > > > > > > Dragos > > > > > > > > > > > > > > > Regards, > > > > > > > > -Siwei > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Jason, what do you think? > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > > > > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > > > > > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > index f8f088cced50..80e066de0866 100644 > > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > bool state_change = false; > > > > > > > > > > void *obj_context; > > > > > > > > > > void *cmd_hdr; > > > > > > > > > > + void *vq_ctx; > > > > > > > > > > void *in; > > > > > > > > > > int err; > > > > > > > > > > > > > > > > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > > > > > > > > > > > > > > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > > > > > > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > > > > > > > > > > > > > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > > > > > > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > > > > > > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > state_change = true; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > > > > > > > > + } > > > > > > > > > > + > > > > > > > > > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > > > > > > > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > > > > > > > > > if (err) > > > > > > > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > > > > > > > > > mvq->desc_addr = desc_area; > > > > > > > > > > mvq->device_addr = device_area; > > > > > > > > > > mvq->driver_addr = driver_area; > > > > > > > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > > > > > > > > > return 0; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > index b86d51a855f6..9594ac405740 100644 > > > > > > > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > @@ -145,6 +145,7 @@ enum { > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > > > > > > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > 2.42.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Sat, Dec 16, 2023 at 12:03 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > On Fri, 2023-12-15 at 18:56 +0100, Eugenio Perez Martin wrote: > > On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > > > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > > > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > > > > > variations there were not backed by spec either. Of course, we should > > > > > > > > > try best to make the default behavior backward compatible with > > > > > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > > > > > define its own feature flag to describe (minor difference in) the > > > > > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > > > > > > > > > Are there other possible options? What do you think? > > > > > > > > > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > > > > > > > I am fine with either of these. > > > > > > > > > > > > > How about allowing the change only under the following conditions: > > > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > > > > > > > ? > > > > > > > > > > I think the best option by far is 1, as there is no hint in the > > > > > combination of these 3 indicating that you can change device > > > > > properties in the suspended state. > > > > > > > > > Sure. Will respin a v3 without these two patches. > > > > > > > > Another series can implement option 2 and add these 2 patches on top. > > > Hmm...I misunderstood your statement and sent a erroneus v3. You said that > > > having a feature flag is the best option. > > > > > > Will add a feature flag in v4: is this similar to the > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? > > > > > > > Right, it should be easy to return it from .get_backend_features op if > > the FW returns that capability, isn't it? > > > Yes, that's easy. But I wonder if we need one feature bit for each type of > change: > - VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND > - VHOST_BACKEND_F_CHANGEABLE_VQ_STATE_IN_SUSPEND > I'd say yes. Although we could configure SVQ initial state in userland as different than 0 for this first step, it would be needed in the long term. > Or would a big one VHOST_BACKEND_F_CAN_RECONFIG_VQ_IN_SUSPEND suffice? > I'd say "reconfig vq" is not valid as mlx driver doesn't allow changing queue sizes, for example, isn't it? To define that it is valid to change "all parameters" seems very confident. > To me having individual feature bits makes sense. But it could also takes too > many bits if more changes are required. > Yes, that's a good point. Maybe it is valid to define a subset of features that can be changed., but I think it is way clearer to just check for individual feature bits. > Thanks, > Dragos > > > > Thanks, > > > Dragos > > > > > > > > > Thanks, > > > > > > Dragos > > > > > > > > > > > > > > Thanks, > > > > > > > > Dragos > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > -Siwei > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Jason, what do you think? > > > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > > > > > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > > > > > > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > > index f8f088cced50..80e066de0866 100644 > > > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > > bool state_change = false; > > > > > > > > > > > void *obj_context; > > > > > > > > > > > void *cmd_hdr; > > > > > > > > > > > + void *vq_ctx; > > > > > > > > > > > void *in; > > > > > > > > > > > int err; > > > > > > > > > > > > > > > > > > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > > > > > > > > > > > > > > > > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > > > > > > > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > > > > > > > > > > > > > > > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > > > > > > > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > > > > > > > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > > state_change = true; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > > > > > > > > > + } > > > > > > > > > > > + > > > > > > > > > > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > > > > > > > > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > > > > > > > > > > if (err) > > > > > > > > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > > > > > > > > > > mvq->desc_addr = desc_area; > > > > > > > > > > > mvq->device_addr = device_area; > > > > > > > > > > > mvq->driver_addr = driver_area; > > > > > > > > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > > > > > > > > > > return 0; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > > index b86d51a855f6..9594ac405740 100644 > > > > > > > > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > > @@ -145,6 +145,7 @@ enum { > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > > > > > > > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > 2.42.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Mon, 2023-12-18 at 11:16 +0100, Eugenio Perez Martin wrote: > On Sat, Dec 16, 2023 at 12:03 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > On Fri, 2023-12-15 at 18:56 +0100, Eugenio Perez Martin wrote: > > > On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > > > > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > > > > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > > > > > > variations there were not backed by spec either. Of course, we should > > > > > > > > > > try best to make the default behavior backward compatible with > > > > > > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > > > > > > define its own feature flag to describe (minor difference in) the > > > > > > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > > > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > > > > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > > > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > > > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > > > > > > > > > > > Are there other possible options? What do you think? > > > > > > > > > > > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > > > > > > > > > I am fine with either of these. > > > > > > > > > > > > > > > How about allowing the change only under the following conditions: > > > > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > > > > > > > > > ? > > > > > > > > > > > > I think the best option by far is 1, as there is no hint in the > > > > > > combination of these 3 indicating that you can change device > > > > > > properties in the suspended state. > > > > > > > > > > > Sure. Will respin a v3 without these two patches. > > > > > > > > > > Another series can implement option 2 and add these 2 patches on top. > > > > Hmm...I misunderstood your statement and sent a erroneus v3. You said that > > > > having a feature flag is the best option. > > > > > > > > Will add a feature flag in v4: is this similar to the > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? > > > > > > > > > > Right, it should be easy to return it from .get_backend_features op if > > > the FW returns that capability, isn't it? > > > > > Yes, that's easy. But I wonder if we need one feature bit for each type of > > change: > > - VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND > > - VHOST_BACKEND_F_CHANGEABLE_VQ_STATE_IN_SUSPEND > > > > I'd say yes. Although we could configure SVQ initial state in userland > as different than 0 for this first step, it would be needed in the > long term. > > > Or would a big one VHOST_BACKEND_F_CAN_RECONFIG_VQ_IN_SUSPEND suffice? > > > > I'd say "reconfig vq" is not valid as mlx driver doesn't allow > changing queue sizes, for example, isn't it? > Modifying the queue size for a vq is indeed not supported by the mlx device. > To define that it is > valid to change "all parameters" seems very confident. > Ack > > To me having individual feature bits makes sense. But it could also takes too > > many bits if more changes are required. > > > > Yes, that's a good point. Maybe it is valid to define a subset of > features that can be changed., but I think it is way clearer to just > check for individual feature bits. > I will prepare extra patches with the 2 feature bits approach. Is it necessary to add checks in the vdpa core that block changing these properties if the state is driver ok and the device doesn't support the feature? > > Thanks, > > Dragos > > > > > > Thanks, > > > > Dragos > > > > > > > > > > > Thanks, > > > > > > > Dragos > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > Dragos > > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > -Siwei > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Jason, what do you think? > > > > > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > > > > > > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > > > > > > > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > > > index f8f088cced50..80e066de0866 100644 > > > > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > > > bool state_change = false; > > > > > > > > > > > > void *obj_context; > > > > > > > > > > > > void *cmd_hdr; > > > > > > > > > > > > + void *vq_ctx; > > > > > > > > > > > > void *in; > > > > > > > > > > > > int err; > > > > > > > > > > > > > > > > > > > > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > > > > > > > > > > > > > > > > > > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > > > > > > > > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > > > > > > > > > > > > > > > > > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > > > > > > > > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > > > > > > > > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > > > state_change = true; > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > > > > > > > > > > + } > > > > > > > > > > > > + > > > > > > > > > > > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > > > > > > > > > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > > > > > > > > > > > if (err) > > > > > > > > > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > > > > > > > > > > > mvq->desc_addr = desc_area; > > > > > > > > > > > > mvq->device_addr = device_area; > > > > > > > > > > > > mvq->driver_addr = driver_area; > > > > > > > > > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > > > > > > > > > > > return 0; > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > > > index b86d51a855f6..9594ac405740 100644 > > > > > > > > > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > > > @@ -145,6 +145,7 @@ enum { > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > > > > > > > > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > 2.42.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Mon, Dec 18, 2023 at 11:52 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > On Mon, 2023-12-18 at 11:16 +0100, Eugenio Perez Martin wrote: > > On Sat, Dec 16, 2023 at 12:03 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > On Fri, 2023-12-15 at 18:56 +0100, Eugenio Perez Martin wrote: > > > > On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > > > > > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > > > > > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > > > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > > > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > > > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > > > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > > > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > > > > > > > variations there were not backed by spec either. Of course, we should > > > > > > > > > > > try best to make the default behavior backward compatible with > > > > > > > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > > > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > > > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > > > > > > > define its own feature flag to describe (minor difference in) the > > > > > > > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > > > > > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > > > > > > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > > > > > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > > > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > > > > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > > > > > > > > > > > > > Are there other possible options? What do you think? > > > > > > > > > > > > > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > > > > > > > > > > > I am fine with either of these. > > > > > > > > > > > > > > > > > How about allowing the change only under the following conditions: > > > > > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > I think the best option by far is 1, as there is no hint in the > > > > > > > combination of these 3 indicating that you can change device > > > > > > > properties in the suspended state. > > > > > > > > > > > > > Sure. Will respin a v3 without these two patches. > > > > > > > > > > > > Another series can implement option 2 and add these 2 patches on top. > > > > > Hmm...I misunderstood your statement and sent a erroneus v3. You said that > > > > > having a feature flag is the best option. > > > > > > > > > > Will add a feature flag in v4: is this similar to the > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? > > > > > > > > > > > > > Right, it should be easy to return it from .get_backend_features op if > > > > the FW returns that capability, isn't it? > > > > > > > Yes, that's easy. But I wonder if we need one feature bit for each type of > > > change: > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_STATE_IN_SUSPEND > > > > > > > I'd say yes. Although we could configure SVQ initial state in userland > > as different than 0 for this first step, it would be needed in the > > long term. > > > > > Or would a big one VHOST_BACKEND_F_CAN_RECONFIG_VQ_IN_SUSPEND suffice? > > > > > > > I'd say "reconfig vq" is not valid as mlx driver doesn't allow > > changing queue sizes, for example, isn't it? > > > Modifying the queue size for a vq is indeed not supported by the mlx device. > > > To define that it is > > valid to change "all parameters" seems very confident. > > > Ack > > > > To me having individual feature bits makes sense. But it could also takes too > > > many bits if more changes are required. > > > > > > > Yes, that's a good point. Maybe it is valid to define a subset of > > features that can be changed., but I think it is way clearer to just > > check for individual feature bits. > > > I will prepare extra patches with the 2 feature bits approach. > > Is it necessary to add checks in the vdpa core that block changing these > properties if the state is driver ok and the device doesn't support the feature? > Yes, I think it is better to protect for changes in vdpa core. > > > Thanks, > > > Dragos > > > > > > > > Thanks, > > > > > Dragos > > > > > > > > > > > > > Thanks, > > > > > > > > Dragos > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Dragos > > > > > > > > > > > > > > > > > > > > > Regards, > > > > > > > > > > > -Siwei > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Jason, what do you think? > > > > > > > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++ > > > > > > > > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 + > > > > > > > > > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > > > > index f8f088cced50..80e066de0866 100644 > > > > > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > > > > bool state_change = false; > > > > > > > > > > > > > void *obj_context; > > > > > > > > > > > > > void *cmd_hdr; > > > > > > > > > > > > > + void *vq_ctx; > > > > > > > > > > > > > void *in; > > > > > > > > > > > > > int err; > > > > > > > > > > > > > > > > > > > > > > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); > > > > > > > > > > > > > > > > > > > > > > > > > > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); > > > > > > > > > > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); > > > > > > > > > > > > > > > > > > > > > > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { > > > > > > > > > > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { > > > > > > > > > > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > > > > > state_change = true; > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { > > > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); > > > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); > > > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); > > > > > > > > > > > > > + } > > > > > > > > > > > > > + > > > > > > > > > > > > > MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); > > > > > > > > > > > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); > > > > > > > > > > > > > if (err) > > > > > > > > > > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ > > > > > > > > > > > > > mvq->desc_addr = desc_area; > > > > > > > > > > > > > mvq->device_addr = device_area; > > > > > > > > > > > > > mvq->driver_addr = driver_area; > > > > > > > > > > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; > > > > > > > > > > > > > return 0; > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > > > > index b86d51a855f6..9594ac405740 100644 > > > > > > > > > > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h > > > > > > > > > > > > > @@ -145,6 +145,7 @@ enum { > > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, > > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, > > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, > > > > > > > > > > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, > > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, > > > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > 2.42.0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Mon, 2023-12-18 at 13:06 +0100, Eugenio Perez Martin wrote: > On Mon, Dec 18, 2023 at 11:52 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > On Mon, 2023-12-18 at 11:16 +0100, Eugenio Perez Martin wrote: > > > On Sat, Dec 16, 2023 at 12:03 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > On Fri, 2023-12-15 at 18:56 +0100, Eugenio Perez Martin wrote: > > > > > On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > > > > > > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > > > > > > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > > > > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > > > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > > > > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > > > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > > > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > > > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > > > > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > > > > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > > > > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > > > > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > > > > > > > > variations there were not backed by spec either. Of course, we should > > > > > > > > > > > > try best to make the default behavior backward compatible with > > > > > > > > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > > > > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > > > > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > > > > > > > > define its own feature flag to describe (minor difference in) the > > > > > > > > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > > > > > > > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > > > > > > > > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > > > > > > > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > > > > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > > > > > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > > > > > > > > > > > > > > > Are there other possible options? What do you think? > > > > > > > > > > > > > > > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > > > > > > > > > > > > > I am fine with either of these. > > > > > > > > > > > > > > > > > > > How about allowing the change only under the following conditions: > > > > > > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > > > I think the best option by far is 1, as there is no hint in the > > > > > > > > combination of these 3 indicating that you can change device > > > > > > > > properties in the suspended state. > > > > > > > > > > > > > > > Sure. Will respin a v3 without these two patches. > > > > > > > > > > > > > > Another series can implement option 2 and add these 2 patches on top. > > > > > > Hmm...I misunderstood your statement and sent a erroneus v3. You said that > > > > > > having a feature flag is the best option. > > > > > > > > > > > > Will add a feature flag in v4: is this similar to the > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? > > > > > > > > > > > > > > > > Right, it should be easy to return it from .get_backend_features op if > > > > > the FW returns that capability, isn't it? > > > > > > > > > Yes, that's easy. But I wonder if we need one feature bit for each type of > > > > change: > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_STATE_IN_SUSPEND > > > > > > > > > > I'd say yes. Although we could configure SVQ initial state in userland > > > as different than 0 for this first step, it would be needed in the > > > long term. > > > > > > > Or would a big one VHOST_BACKEND_F_CAN_RECONFIG_VQ_IN_SUSPEND suffice? > > > > > > > > > > I'd say "reconfig vq" is not valid as mlx driver doesn't allow > > > changing queue sizes, for example, isn't it? > > > > > Modifying the queue size for a vq is indeed not supported by the mlx device. > > > > > To define that it is > > > valid to change "all parameters" seems very confident. > > > > > Ack > > > > > > To me having individual feature bits makes sense. But it could also takes too > > > > many bits if more changes are required. > > > > > > > > > > Yes, that's a good point. Maybe it is valid to define a subset of > > > features that can be changed., but I think it is way clearer to just > > > check for individual feature bits. > > > > > I will prepare extra patches with the 2 feature bits approach. > > > > Is it necessary to add checks in the vdpa core that block changing these > > properties if the state is driver ok and the device doesn't support the feature? > > > > Yes, I think it is better to protect for changes in vdpa core. > Hmmm... there is no suspended state available. I would only add checks for the DRIVER_OK state of the device because adding a is_suspended state/op seems out of scope for this series. Any thoughts?
On Mon, Dec 18, 2023 at 2:58 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > On Mon, 2023-12-18 at 13:06 +0100, Eugenio Perez Martin wrote: > > On Mon, Dec 18, 2023 at 11:52 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > On Mon, 2023-12-18 at 11:16 +0100, Eugenio Perez Martin wrote: > > > > On Sat, Dec 16, 2023 at 12:03 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > On Fri, 2023-12-15 at 18:56 +0100, Eugenio Perez Martin wrote: > > > > > > On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > > > > > > > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > > > > > > > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > > > > > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > > > > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > > > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > > > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > > > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > > > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > > > > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > > > > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > > > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > > > > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > > > > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > > > > > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > > > > > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > > > > > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > > > > > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > > > > > > > > > variations there were not backed by spec either. Of course, we should > > > > > > > > > > > > > try best to make the default behavior backward compatible with > > > > > > > > > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > > > > > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > > > > > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > > > > > > > > > define its own feature flag to describe (minor difference in) the > > > > > > > > > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > > > > > > > > > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > > > > > > > > > > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > > > > > > > > > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > > > > > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > > > > > > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > > > > > > > > > > > > > > > > > Are there other possible options? What do you think? > > > > > > > > > > > > > > > > > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > > > > > > > > > > > > > > > I am fine with either of these. > > > > > > > > > > > > > > > > > > > > > How about allowing the change only under the following conditions: > > > > > > > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > > > > > I think the best option by far is 1, as there is no hint in the > > > > > > > > > combination of these 3 indicating that you can change device > > > > > > > > > properties in the suspended state. > > > > > > > > > > > > > > > > > Sure. Will respin a v3 without these two patches. > > > > > > > > > > > > > > > > Another series can implement option 2 and add these 2 patches on top. > > > > > > > Hmm...I misunderstood your statement and sent a erroneus v3. You said that > > > > > > > having a feature flag is the best option. > > > > > > > > > > > > > > Will add a feature flag in v4: is this similar to the > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? > > > > > > > > > > > > > > > > > > > Right, it should be easy to return it from .get_backend_features op if > > > > > > the FW returns that capability, isn't it? > > > > > > > > > > > Yes, that's easy. But I wonder if we need one feature bit for each type of > > > > > change: > > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND > > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_STATE_IN_SUSPEND > > > > > > > > > > > > > I'd say yes. Although we could configure SVQ initial state in userland > > > > as different than 0 for this first step, it would be needed in the > > > > long term. > > > > > > > > > Or would a big one VHOST_BACKEND_F_CAN_RECONFIG_VQ_IN_SUSPEND suffice? > > > > > > > > > > > > > I'd say "reconfig vq" is not valid as mlx driver doesn't allow > > > > changing queue sizes, for example, isn't it? > > > > > > > Modifying the queue size for a vq is indeed not supported by the mlx device. > > > > > > > To define that it is > > > > valid to change "all parameters" seems very confident. > > > > > > > Ack > > > > > > > > To me having individual feature bits makes sense. But it could also takes too > > > > > many bits if more changes are required. > > > > > > > > > > > > > Yes, that's a good point. Maybe it is valid to define a subset of > > > > features that can be changed., but I think it is way clearer to just > > > > check for individual feature bits. > > > > > > > I will prepare extra patches with the 2 feature bits approach. > > > > > > Is it necessary to add checks in the vdpa core that block changing these > > > properties if the state is driver ok and the device doesn't support the feature? > > > > > > > Yes, I think it is better to protect for changes in vdpa core. > > > Hmmm... there is no suspended state available. I would only add checks for the > DRIVER_OK state of the device because adding a is_suspended state/op seems out > of scope for this series. Any thoughts? > I can develop it so you can include it in your series for sure, I will send it ASAP. Thanks!
On Tue, 2023-12-19 at 08:24 +0100, Eugenio Perez Martin wrote: > On Mon, Dec 18, 2023 at 2:58 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > On Mon, 2023-12-18 at 13:06 +0100, Eugenio Perez Martin wrote: > > > On Mon, Dec 18, 2023 at 11:52 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > On Mon, 2023-12-18 at 11:16 +0100, Eugenio Perez Martin wrote: > > > > > On Sat, Dec 16, 2023 at 12:03 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > On Fri, 2023-12-15 at 18:56 +0100, Eugenio Perez Martin wrote: > > > > > > > On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > > > > > > > > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > > > > > > > > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > > > > > > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > > > > > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > > > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > > > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > > > > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > > > > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > > > > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > > > > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > > > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > > > > > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > > > > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > > > > > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > > > > > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > > > > > > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > > > > > > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > > > > > > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > > > > > > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > > > > > > > > > > variations there were not backed by spec either. Of course, we should > > > > > > > > > > > > > > try best to make the default behavior backward compatible with > > > > > > > > > > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > > > > > > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > > > > > > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > > > > > > > > > > define its own feature flag to describe (minor difference in) the > > > > > > > > > > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > > > > > > > > > > > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > > > > > > > > > > > > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > > > > > > > > > > > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > > > > > > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > > > > > > > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > > > > > > > > > > > > > > > > > > > Are there other possible options? What do you think? > > > > > > > > > > > > > > > > > > > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > > > > > > > > > > > > > > > > > I am fine with either of these. > > > > > > > > > > > > > > > > > > > > > > > How about allowing the change only under the following conditions: > > > > > > > > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > > > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > > > > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > > > > > > > I think the best option by far is 1, as there is no hint in the > > > > > > > > > > combination of these 3 indicating that you can change device > > > > > > > > > > properties in the suspended state. > > > > > > > > > > > > > > > > > > > Sure. Will respin a v3 without these two patches. > > > > > > > > > > > > > > > > > > Another series can implement option 2 and add these 2 patches on top. > > > > > > > > Hmm...I misunderstood your statement and sent a erroneus v3. You said that > > > > > > > > having a feature flag is the best option. > > > > > > > > > > > > > > > > Will add a feature flag in v4: is this similar to the > > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? > > > > > > > > > > > > > > > > > > > > > > Right, it should be easy to return it from .get_backend_features op if > > > > > > > the FW returns that capability, isn't it? > > > > > > > > > > > > > Yes, that's easy. But I wonder if we need one feature bit for each type of > > > > > > change: > > > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND > > > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_STATE_IN_SUSPEND > > > > > > > > > > > > > > > > I'd say yes. Although we could configure SVQ initial state in userland > > > > > as different than 0 for this first step, it would be needed in the > > > > > long term. > > > > > > > > > > > Or would a big one VHOST_BACKEND_F_CAN_RECONFIG_VQ_IN_SUSPEND suffice? > > > > > > > > > > > > > > > > I'd say "reconfig vq" is not valid as mlx driver doesn't allow > > > > > changing queue sizes, for example, isn't it? > > > > > > > > > Modifying the queue size for a vq is indeed not supported by the mlx device. > > > > > > > > > To define that it is > > > > > valid to change "all parameters" seems very confident. > > > > > > > > > Ack > > > > > > > > > > To me having individual feature bits makes sense. But it could also takes too > > > > > > many bits if more changes are required. > > > > > > > > > > > > > > > > Yes, that's a good point. Maybe it is valid to define a subset of > > > > > features that can be changed., but I think it is way clearer to just > > > > > check for individual feature bits. > > > > > > > > > I will prepare extra patches with the 2 feature bits approach. > > > > > > > > Is it necessary to add checks in the vdpa core that block changing these > > > > properties if the state is driver ok and the device doesn't support the feature? > > > > > > > > > > Yes, I think it is better to protect for changes in vdpa core. > > > > > Hmmm... there is no suspended state available. I would only add checks for the > > DRIVER_OK state of the device because adding a is_suspended state/op seems out > > of scope for this series. Any thoughts? > > > > I can develop it so you can include it in your series for sure, I will > send it ASAP. > If it's a matter of: - Adding a suspended state to struct vhost_vdpa. - Setting it to true on successful device suspend. - Clearing it on successful device resume and device reset. I can add this patch. I'm just not sure about the locking part. But maybe I can send it and we can debate on the code. Thanks, Dragos
On Tue, Dec 19, 2023 at 12:16 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > On Tue, 2023-12-19 at 08:24 +0100, Eugenio Perez Martin wrote: > > On Mon, Dec 18, 2023 at 2:58 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > On Mon, 2023-12-18 at 13:06 +0100, Eugenio Perez Martin wrote: > > > > On Mon, Dec 18, 2023 at 11:52 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > On Mon, 2023-12-18 at 11:16 +0100, Eugenio Perez Martin wrote: > > > > > > On Sat, Dec 16, 2023 at 12:03 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > On Fri, 2023-12-15 at 18:56 +0100, Eugenio Perez Martin wrote: > > > > > > > > On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > > > > > > > > > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > > > > > > > > > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > > > > > > > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > > > > > > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > > > > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > > > > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > > > > > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > > > > > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > > > > > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > > > > > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > > > > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > > > > > > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > > > > > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > > > > > > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > > > > > > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > > > > > > > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > > > > > > > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > > > > > > > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > > > > > > > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > > > > > > > > > > > variations there were not backed by spec either. Of course, we should > > > > > > > > > > > > > > > try best to make the default behavior backward compatible with > > > > > > > > > > > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > > > > > > > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > > > > > > > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > > > > > > > > > > > define its own feature flag to describe (minor difference in) the > > > > > > > > > > > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > > > > > > > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > > > > > > > > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Are there other possible options? What do you think? > > > > > > > > > > > > > > > > > > > > > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > > > > > > > > > > > > > > > > > > > I am fine with either of these. > > > > > > > > > > > > > > > > > > > > > > > > > How about allowing the change only under the following conditions: > > > > > > > > > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > > > > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > > > > > > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > > > > > > > > > I think the best option by far is 1, as there is no hint in the > > > > > > > > > > > combination of these 3 indicating that you can change device > > > > > > > > > > > properties in the suspended state. > > > > > > > > > > > > > > > > > > > > > Sure. Will respin a v3 without these two patches. > > > > > > > > > > > > > > > > > > > > Another series can implement option 2 and add these 2 patches on top. > > > > > > > > > Hmm...I misunderstood your statement and sent a erroneus v3. You said that > > > > > > > > > having a feature flag is the best option. > > > > > > > > > > > > > > > > > > Will add a feature flag in v4: is this similar to the > > > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? > > > > > > > > > > > > > > > > > > > > > > > > > Right, it should be easy to return it from .get_backend_features op if > > > > > > > > the FW returns that capability, isn't it? > > > > > > > > > > > > > > > Yes, that's easy. But I wonder if we need one feature bit for each type of > > > > > > > change: > > > > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND > > > > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_STATE_IN_SUSPEND > > > > > > > > > > > > > > > > > > > I'd say yes. Although we could configure SVQ initial state in userland > > > > > > as different than 0 for this first step, it would be needed in the > > > > > > long term. > > > > > > > > > > > > > Or would a big one VHOST_BACKEND_F_CAN_RECONFIG_VQ_IN_SUSPEND suffice? > > > > > > > > > > > > > > > > > > > I'd say "reconfig vq" is not valid as mlx driver doesn't allow > > > > > > changing queue sizes, for example, isn't it? > > > > > > > > > > > Modifying the queue size for a vq is indeed not supported by the mlx device. > > > > > > > > > > > To define that it is > > > > > > valid to change "all parameters" seems very confident. > > > > > > > > > > > Ack > > > > > > > > > > > > To me having individual feature bits makes sense. But it could also takes too > > > > > > > many bits if more changes are required. > > > > > > > > > > > > > > > > > > > Yes, that's a good point. Maybe it is valid to define a subset of > > > > > > features that can be changed., but I think it is way clearer to just > > > > > > check for individual feature bits. > > > > > > > > > > > I will prepare extra patches with the 2 feature bits approach. > > > > > > > > > > Is it necessary to add checks in the vdpa core that block changing these > > > > > properties if the state is driver ok and the device doesn't support the feature? > > > > > > > > > > > > > Yes, I think it is better to protect for changes in vdpa core. > > > > > > > Hmmm... there is no suspended state available. I would only add checks for the > > > DRIVER_OK state of the device because adding a is_suspended state/op seems out > > > of scope for this series. Any thoughts? > > > > > > > I can develop it so you can include it in your series for sure, I will > > send it ASAP. > > > If it's a matter of: > - Adding a suspended state to struct vhost_vdpa. > - Setting it to true on successful device suspend. > - Clearing it on successful device resume and device reset. > > I can add this patch. I'm just not sure about the locking part. But maybe I can > send it and we can debate on the code. > Yes, that was the plan basically. I think vhost_vdpa_suspend / vhost_vdpa_resume are already called from vhost_vdpa_unlocked_ioctl with the lock acquired, is that what you mean? Thanks!
On Tue, 2023-12-19 at 15:02 +0100, Eugenio Perez Martin wrote: > On Tue, Dec 19, 2023 at 12:16 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > On Tue, 2023-12-19 at 08:24 +0100, Eugenio Perez Martin wrote: > > > On Mon, Dec 18, 2023 at 2:58 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > On Mon, 2023-12-18 at 13:06 +0100, Eugenio Perez Martin wrote: > > > > > On Mon, Dec 18, 2023 at 11:52 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > On Mon, 2023-12-18 at 11:16 +0100, Eugenio Perez Martin wrote: > > > > > > > On Sat, Dec 16, 2023 at 12:03 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > On Fri, 2023-12-15 at 18:56 +0100, Eugenio Perez Martin wrote: > > > > > > > > > On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote: > > > > > > > > > > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote: > > > > > > > > > > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote: > > > > > > > > > > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote: > > > > > > > > > > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote: > > > > > > > > > > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote: > > > > > > > > > > > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on > > > > > > > > > > > > > > > > > > next modify_virtqueue. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com> > > > > > > > > > > > > > > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com> > > > > > > > > > > > > > > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com> > > > > > > > > > > > > > > > > > I'm kind of ok with this patch and the next one about state, but I > > > > > > > > > > > > > > > > > didn't ack them in the previous series. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > My main concern is that it is not valid to change the vq address after > > > > > > > > > > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to > > > > > > > > > > > > > > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost > > > > > > > > > > > > > > > > > forbids changing it with an active backend. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is > > > > > > > > > > > > > > > > > ok to decide that all of these parameters may change during suspend. > > > > > > > > > > > > > > > > > Maybe the best thing is to protect this with a vDPA feature flag. > > > > > > > > > > > > > > > > I think protect with vDPA feature flag could work, while on the other > > > > > > > > > > > > > > > > hand vDPA means vendor specific optimization is possible around suspend > > > > > > > > > > > > > > > > and resume (in case it helps performance), which doesn't have to be > > > > > > > > > > > > > > > > backed by virtio spec. Same applies to vhost user backend features, > > > > > > > > > > > > > > > > variations there were not backed by spec either. Of course, we should > > > > > > > > > > > > > > > > try best to make the default behavior backward compatible with > > > > > > > > > > > > > > > > virtio-based backend, but that circles back to no suspend definition in > > > > > > > > > > > > > > > > the current virtio spec, for which I hope we don't cease development on > > > > > > > > > > > > > > > > vDPA indefinitely. After all, the virtio based vdap backend can well > > > > > > > > > > > > > > > > define its own feature flag to describe (minor difference in) the > > > > > > > > > > > > > > > > suspend behavior based on the later spec once it is formed in future. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > So what is the way forward here? From what I understand the options are: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 1) Add a vdpa feature flag for changing device properties while suspended. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as > > > > > > > > > > > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this > > > > > > > > > > > > > > > code won't work anymore. This means the series would be less well tested. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Are there other possible options? What do you think? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip > > > > > > > > > > > > > > > > > > > > > > > > > > > > I am fine with either of these. > > > > > > > > > > > > > > > > > > > > > > > > > > > How about allowing the change only under the following conditions: > > > > > > > > > > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume && > > > > > > > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set > > > > > > > > > > > > > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > > > > > > > > > > > I think the best option by far is 1, as there is no hint in the > > > > > > > > > > > > combination of these 3 indicating that you can change device > > > > > > > > > > > > properties in the suspended state. > > > > > > > > > > > > > > > > > > > > > > > Sure. Will respin a v3 without these two patches. > > > > > > > > > > > > > > > > > > > > > > Another series can implement option 2 and add these 2 patches on top. > > > > > > > > > > Hmm...I misunderstood your statement and sent a erroneus v3. You said that > > > > > > > > > > having a feature flag is the best option. > > > > > > > > > > > > > > > > > > > > Will add a feature flag in v4: is this similar to the > > > > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag? > > > > > > > > > > > > > > > > > > > > > > > > > > > > Right, it should be easy to return it from .get_backend_features op if > > > > > > > > > the FW returns that capability, isn't it? > > > > > > > > > > > > > > > > > Yes, that's easy. But I wonder if we need one feature bit for each type of > > > > > > > > change: > > > > > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND > > > > > > > > - VHOST_BACKEND_F_CHANGEABLE_VQ_STATE_IN_SUSPEND > > > > > > > > > > > > > > > > > > > > > > I'd say yes. Although we could configure SVQ initial state in userland > > > > > > > as different than 0 for this first step, it would be needed in the > > > > > > > long term. > > > > > > > > > > > > > > > Or would a big one VHOST_BACKEND_F_CAN_RECONFIG_VQ_IN_SUSPEND suffice? > > > > > > > > > > > > > > > > > > > > > > I'd say "reconfig vq" is not valid as mlx driver doesn't allow > > > > > > > changing queue sizes, for example, isn't it? > > > > > > > > > > > > > Modifying the queue size for a vq is indeed not supported by the mlx device. > > > > > > > > > > > > > To define that it is > > > > > > > valid to change "all parameters" seems very confident. > > > > > > > > > > > > > Ack > > > > > > > > > > > > > > To me having individual feature bits makes sense. But it could also takes too > > > > > > > > many bits if more changes are required. > > > > > > > > > > > > > > > > > > > > > > Yes, that's a good point. Maybe it is valid to define a subset of > > > > > > > features that can be changed., but I think it is way clearer to just > > > > > > > check for individual feature bits. > > > > > > > > > > > > > I will prepare extra patches with the 2 feature bits approach. > > > > > > > > > > > > Is it necessary to add checks in the vdpa core that block changing these > > > > > > properties if the state is driver ok and the device doesn't support the feature? > > > > > > > > > > > > > > > > Yes, I think it is better to protect for changes in vdpa core. > > > > > > > > > Hmmm... there is no suspended state available. I would only add checks for the > > > > DRIVER_OK state of the device because adding a is_suspended state/op seems out > > > > of scope for this series. Any thoughts? > > > > > > > > > > I can develop it so you can include it in your series for sure, I will > > > send it ASAP. > > > > > If it's a matter of: > > - Adding a suspended state to struct vhost_vdpa. > > - Setting it to true on successful device suspend. > > - Clearing it on successful device resume and device reset. > > > > I can add this patch. I'm just not sure about the locking part. But maybe I can > > send it and we can debate on the code. > > > > Yes, that was the plan basically. > > I think vhost_vdpa_suspend / vhost_vdpa_resume are already called from > vhost_vdpa_unlocked_ioctl with the lock acquired, is that what you > mean? > Yes, that's what I wanted to make sure that is correct. I will send the v4 soon. Thanks, Dragos
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index f8f088cced50..80e066de0866 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, bool state_change = false; void *obj_context; void *cmd_hdr; + void *vq_ctx; void *in; int err; @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid); obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context); + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) { if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) { @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, state_change = true; } + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) { + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr); + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr); + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr); + } + MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields); err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out)); if (err) @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_ mvq->desc_addr = desc_area; mvq->device_addr = device_area; mvq->driver_addr = driver_area; + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS; return 0; } diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h index b86d51a855f6..9594ac405740 100644 --- a/include/linux/mlx5/mlx5_ifc_vdpa.h +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h @@ -145,6 +145,7 @@ enum { MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0, MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3, MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4, + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6, MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14, };