From:   Parav Pandit <parav@nvidia.com>
To:     Si-Wei Liu <si-wei.liu@oracle.com>,
        "mst@redhat.com" <mst@redhat.com>,
        "jasowang@redhat.com" <jasowang@redhat.com>,
        Eli Cohen <elic@nvidia.com>
CC:     "virtualization@lists.linux-foundation.org" 
        <virtualization@lists.linux-foundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2 3/7] vdpa: validate provisioned device features against
 specified attribute
Thread-Topic: [PATCH v2 3/7] vdpa: validate provisioned device features
 against specified attribute
Thread-Index: AQHZNcrul/O2ZyBovE6TfvS9k66n+q67G64Q
Date:   Thu, 2 Feb 2023 05:05:46 +0000
Message-ID: <PH0PR12MB5481759D5CA6EB5C9D63C517DCD69@PH0PR12MB5481.namprd12.prod.outlook.com>
References: <1675207345-22328-1-git-send-email-si-wei.liu@oracle.com>
 <1675207345-22328-4-git-send-email-si-wei.liu@oracle.com>
In-Reply-To: <1675207345-22328-4-git-send-email-si-wei.liu@oracle.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?sK1XMug5whuk5SoPCaSTZV3mz0vYl2dcA+plVA7AGxSmdVSOdYQA4ikQeP25?=
 =?us-ascii?Q?/MMSZdABpESP7sZPXkFiW5SvVzjdGOtH9TgDACxB/+NlVTysiX66XCPtBgTb?=
 =?us-ascii?Q?jaYmEwQsXfJ26dqnUZORDfkGxy+MPPq/rRQEfsThY0DcbtQDv+4uJ50SAmyd?=
 =?us-ascii?Q?N00tpbY0qaYp9lG1zVVJ2m8EdQx8mNgrAQ3hu8rBiLGIsyRVQB2lZmUYBArr?=
 =?us-ascii?Q?IwPtKpCSI9BsL2/I6dykjgiiR1daqYLHCzrfvzFqfow7vF2g7u5azcnJnHzW?=
 =?us-ascii?Q?R4YtqtT+bGbXbYoqfAUMgvHyNWGx6RS5cs3qj+i4GkktuJ7sgyJsJysxj/x9?=
 =?us-ascii?Q?p4jB5lZ3y388lagrWTPRIq6NxXAxR0b5rJAFpx+cEDAU94JWq0b/X+8VZVrJ?=
 =?us-ascii?Q?V5wgdo4/DwIJqaOtbYtpCK5WTe3ih3+pX8KlG9KTt8iYpA9GLORuME9nE83f?=
 =?us-ascii?Q?VElAsv7P3Iqg4HkHl6AygJamF1IFBCbv2MruMcW00BZ6a9zB/lnTN75r/4FI?=
 =?us-ascii?Q?uetyPUvuJ2iB5HQt2cW8AwTtsuKsQC1yef17uOfT6TxJ5uOQlmU0k7rwCygx?=
 =?us-ascii?Q?WCWdh9oRN0TI/rZJB+Fykzce2ezgqruiNXtDvAodzYZrPpWf9HgYxcRdWSAD?=
 =?us-ascii?Q?bMnHiWxgFW9HoXuK4veLKH3JKHJ/qr5s9cwghBx+A3Xf+4tr/YjsZ/UqDkHM?=
 =?us-ascii?Q?ergBklaNvV7AyvmgxqZJ4bq5/jP9OCZpYHr9tE4qhH37EGqT+rV+AGVnhfn4?=
 =?us-ascii?Q?dMammGStFBzTtihQFMDG4/uW1pyGAvvaWm89pMqNo0Y5pdLYbMUOXt5yRSaB?=
 =?us-ascii?Q?Dyug8x5c1Kdpgx9gRGX4cK4IOij6jyU3JT4WcIyKtJhxJrUlb9N7XTOSegKG?=
 =?us-ascii?Q?+salOOe9iURnH2zaxWK4P4rpFjSLF0y5t0bnhHDhopXa/hPJWUbTgp8K447E?=
 =?us-ascii?Q?OLAc25mfcorKr48QODnAUs+rvGZy8cjdl2+JgKjYzxjp7Wsqdp8yFAKI9U8A?=
 =?us-ascii?Q?FR72K+hzoH//sGieSJZktDDSYmqFh4CYlGY/nVZVRuPhh9Ea0ii5h4q3mMuT?=
 =?us-ascii?Q?A1ytc9ZhaO2JWik5FDHEDPPdVwnCm7uKuwskBKcAytcEoC0aiiEj9gjosyAC?=
 =?us-ascii?Q?6uTwszI7qcY5ruNekp14XggcuCnedYaQN1kboWEj2ffeTRk4MEiRvMCaKWqY?=
 =?us-ascii?Q?gJ52VJmHkwUn1YeVtwPzan3Xet+lpq1YA374QnxQ0ZgLLg4hNPNvWkKOo+TM?=
 =?us-ascii?Q?9swo4xiiAjbS5IaTqOsGWDWIRzEnvNXgOYwD7CDRRyw50nXxmykDy667Vo8v?=
 =?us-ascii?Q?pJW86YfQEft8cEqKoTZVRwtFdBNXyL1n+dIQiaNKKtVwS+Bfm2HV2ID/evrZ?=
 =?us-ascii?Q?Cz1hThCpgt5d/3BHgC1+bkxysR9+B13d0xi+kkQgq3ZAL7IsCfamZdsQQjTL?=
 =?us-ascii?Q?DD7VR/HCXxV1eDA59krwRHrbJqRq8xssQelQkRrnZFzpiB1cnQ18iRnmlfUM?=
 =?us-ascii?Q?s9tVOBXRC1VVuGJVqoSrOIkccUNYvb1gWXCplsJnaX6hmADHqbgDTZQbekzL?=
 =?us-ascii?Q?8Vl3CP4pmX88m8fkX48=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR12MB5481.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 838f1d95-568c-4d9c-5b4e-08db04db2521
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Feb 2023 05:05:47.0193
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 9+KG/D3Zfwxv+et+aVppr2o1lrsDuGnUYHCWMRNs8F4+n1X9G13fGI5AXfV+7IESnPeQxyx0AKoCd5dfnel5Gg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR12MB4542
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Tuesday, January 31, 2023 6:22 PM
>=20
> With device feature provisioning, there's a chance for misconfiguration t=
hat the
> vdpa feature attribute supplied in 'vdpa dev add' command doesn't get
> selected on the device_features to be provisioned. For instance, when a @=
mac
> attribute is specified, the corresponding feature bit _F_MAC in device_fe=
atures
> should be set for consistency. If there's conflict on provisioned feature=
s against
> the attribute, it should be treated as an error to fail the ambiguous com=
mand.
> Noted the opposite is not necessarily true, for e.g. it's okay to have _F=
_MAC set
> in device_features without providing a corresponding @mac attribute, in w=
hich
> case the vdpa vendor driver could load certain default value for attribut=
e that is
> not explicitly specified.
>=20
> Generalize this check in vdpa core so that there's no duplicate code in e=
ach
> vendor driver.
>=20
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
>  drivers/vdpa/vdpa.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>=20
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index 21c8aa3..1eb=
a978
> 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -601,8 +601,26 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct
> sk_buff *skb, struct genl_info *i
>  		config.mask |=3D
> BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
>  	}
>  	if (nl_attrs[VDPA_ATTR_DEV_FEATURES]) {
> +		u64 missing =3D 0x0ULL;
> +
>  		config.device_features =3D
>  			nla_get_u64(nl_attrs[VDPA_ATTR_DEV_FEATURES]);
> +		if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR] &&
> +		    !(config.device_features & BIT_ULL(VIRTIO_NET_F_MAC)))
> +			missing |=3D BIT_ULL(VIRTIO_NET_F_MAC);
> +		if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU] &&
> +		    !(config.device_features & BIT_ULL(VIRTIO_NET_F_MTU)))
> +			missing |=3D BIT_ULL(VIRTIO_NET_F_MTU);
> +		if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP] &&
> +		    config.net.max_vq_pairs > 1 &&
> +		    !(config.device_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> +			missing |=3D BIT_ULL(VIRTIO_NET_F_MQ);
> +		if (missing) {
> +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +					       "Missing features 0x%llx for
> provided attributes",
> +					       missing);
> +			return -EINVAL;
> +		}
>  		config.mask |=3D BIT_ULL(VDPA_ATTR_DEV_FEATURES);
>  	}
>=20
> --
> 1.8.3.1
Vdpa this layer can likely derive the feature bits for the supplied config =
fields so that user doesn't need to keep track of both.
Only those feature bits which are unrelated to any config, is what user sho=
uld be setting.