Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <649cf77d-849b-1ed1-e804-3abab9e339d1@oracle.com>
Date:   Fri, 23 Sep 2022 13:11:29 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.0
Subject: Re: [PATCH V2 3/3] vp_vdpa: support feature provisioning
Content-Language: en-US
To:     Jason Wang <jasowang@redhat.com>, mst@redhat.com
Cc:     elic@nvidia.com, parav@nvidia.com, wuzongyong@linux.alibaba.com,
        virtualization@lists.linux-foundation.org,
        linux-kernel@vger.kernel.org, eperezma@redhat.com,
        lingshan.zhu@intel.com, gdawar@xilinx.com, lulu@redhat.com,
        xieyongji@bytedance.com
References: <20220922024305.1718-1-jasowang@redhat.com>
 <20220922024305.1718-4-jasowang@redhat.com>
From:   Si-Wei Liu <si-wei.liu@oracle.com>
Organization: Oracle Corporation
In-Reply-To: <20220922024305.1718-4-jasowang@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?K3lyTzZQNTVDb0cvbGZQampINGxjemJqVTlaSSt6bnlHNmt0ZkZJMDIvZUlq?=
 =?utf-8?B?K3JHUlhFSWxrUjloUW5aSkk3UXk1LzBJOWs3QzlBTGdzZzFsc0UydEE4TEhY?=
 =?utf-8?B?N1JRUzBpR0QvbFZGMVAxYms2cXc5Qnp1STVveWc5aHlVR2I5b2FkeVZ3Q3h2?=
 =?utf-8?B?bHRPdC9hTFZZamo2SUhIcTNiNnFpRXZyem81QUQrbzk1MGtVY1NCYzhNRlNP?=
 =?utf-8?B?K09ySk9mNTN2TU9WN1BBUm1FMzQyeHVGeldVOXpaa3NaRW9jWjNTNE9FT2hN?=
 =?utf-8?B?RHAzMzR3WWRSNXdIR2Zhem5zOUtTN2IrcmZ5VWxMSHBxTG9JUXl1ZXVFUzFj?=
 =?utf-8?B?Q3dJVytSYWd1ZWQzOHlWZDJCT2xLV01yc2xuOFNRc3MvdHJZZ2FlUTRYbnNN?=
 =?utf-8?B?NEFPMEJNVFk3NzdOQTcwWk95b1VrUnlMelB4WWJIUnFIUngrVklWemFzRmxq?=
 =?utf-8?B?OWFrN094Z0IxaENsMys4MVlrcjZZTGhOOGc4cGQ2eWpITkZnSWtSeGxHcURi?=
 =?utf-8?B?V3FON2dBYXB3M2ZkL1J6d1hzTUtzRWc3UDZCMHA1UkNWcWhPN0w3VWpoMXRo?=
 =?utf-8?B?VDFqNHlMdmpNTTAxRk1nbjByRVg1YzlJK0hhRC80SXg1YkpkUlZrVTZvaWdP?=
 =?utf-8?B?K1F4eDhNYTZncWgrS1dmUW8wZUtjRkJVZVIzMnNnaGxpTEhtbzdUaVp0b2pF?=
 =?utf-8?B?QWVKdEpIZFkyRExrT3lpdW9BUko0dFRtMDA1RzVLamxUT3Jzb1Z2dnVubndX?=
 =?utf-8?B?b2pTVnNzaU9kd2RTb1NVa2dUaDNXUmR4VkorR2FBMmFRZ2hqb2RrcXE4dXpT?=
 =?utf-8?B?SEtDL3QwelRQL2RRK2J0SWNCajVadFVicmQ3ODlpUmpnaTRMTHlxbmU1WFNt?=
 =?utf-8?B?R3RNdHhQN1dzenI3ZmdIL3BXVEZpcWhRdWNsd1FDSUNWSVA0U01mY0FzekJh?=
 =?utf-8?B?SndERkJCZlJ1Tk9yM3RRRXJ2TExFYm1JME14R045bHBiU2pWUk14ZGhoZG1X?=
 =?utf-8?B?Y0xZckpvckZZWmYwZytqNjBPSytSbHpGZ2NKR28welJQSnpUaTZmWFQ4Qzhs?=
 =?utf-8?B?U3ZiZ255QlcyVStSNXI1RXpHbnRxZGtJeHVXSW9tS2ZoM2E0aFRDNUQ5ZVJi?=
 =?utf-8?B?YVJiUVBpS2JZbU15N1VJbnRyQ0ZhOFRiNmJjZm91enlKMXMzUHJoc3liT2dm?=
 =?utf-8?B?VDJYNWFzYWZzQ3RnZDQ3ZGRId3prVHRTdXF0eXZyMS9mL0xNMTJVRU5aeG96?=
 =?utf-8?B?bnZrelo1YU40RS9WMVArRVc2RFN6UHkxWiszNDVSQXprVEVDVG8vWEZaaUJY?=
 =?utf-8?B?c3lYdzlyQ1k4UDM1Y1gvS28rVFY0OCtrMlpMenlmYnVCZ1RobWt4eXR6K3Bw?=
 =?utf-8?B?aFFyZjl2TXNGYlNuRFdSZGRZdjhlYUxxdmtxWVpBU1ZUQWc2ZVpYNlhOYWhO?=
 =?utf-8?B?dEU2T3EvTXhLb25GV1dPWTJsZ3pGU1VmRHB4L1VKWnQ4ejRBMTl5SUtUQkJq?=
 =?utf-8?B?N0xqVGdDSWs4dzB2cWlUNHBMSG91bDdKb01XWUxhcjV6dklGd0U0OVFqYTY5?=
 =?utf-8?B?bUJROWNRWmFGZmJJd29Wc2hNNVFkMTRJSDBza1k4cXZ0TG1QRlNJUkdXL0p1?=
 =?utf-8?B?RithRElHS3dxL1ZuYnlPYjE5eVhubjVVT1pOeUoyaDVNS01Ed0lZZGdoRHFV?=
 =?utf-8?B?dlFMTzRFdHM2MWk3VlY3TWhkcTRQMjNtN0RZOGZDTTMrL2xYSkVjNkx4WjNC?=
 =?utf-8?B?R3gzWnZva1pLa2VqS3p0K2swdEd0TTVSZW1XRjYzbEN3bTh5Wld0TFFvdVZj?=
 =?utf-8?B?ZXA5aDNEdU5ZS3FJVlRBNEJwRUMxMmdWbWFFclVDUk5zb1JBS2RiWVQ2TUVV?=
 =?utf-8?B?TzBpVnZDeDRJUzRVaCs5TTNJeDR1TlZqRGFLMDJpWDc3WmpJNFdFM2lQZnNW?=
 =?utf-8?B?QW1PVmkwMW9MS2NuMGNRcSsxOVZ6MWpEYkRtU1cyM2RjZ1FXbTJDOEZMVFJS?=
 =?utf-8?B?YUUwMDc3alhlNzVrQjJJeU1hQ2VpY255bVFZeXZxNkxZb0ZpVCtCTHgwYldp?=
 =?utf-8?B?dGpRQ29EMXkyaWpFdUx2dlg3SzM5SDFLNWZPdWRORUVvVWV6NmhhNXYvcWlQ?=
 =?utf-8?Q?bvfjR7I3OqKkEgigQzqS28x7w?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e4528130-cd0d-4449-5904-08da9d9fcfe9
X-MS-Exchange-CrossTenant-AuthSource: MW4PR10MB6535.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2022 20:11:34.2085
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: EfNmzvNjIZUCXUgTu1GVytfkn8gas+5dR5rTH3LiarNR74bEsIZwrUS5zYmQ1pld0a2kXjcSNoqacRoWYFToWw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4672
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-23_08,2022-09-22_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 phishscore=0 mlxlogscore=999
 bulkscore=0 suspectscore=0 adultscore=0 spamscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2209130000
 definitions=main-2209230131
X-Proofpoint-GUID: zRJMBaWZKJjWGBgZ8_25vxaqclZGqEVc
X-Proofpoint-ORIG-GUID: zRJMBaWZKJjWGBgZ8_25vxaqclZGqEVc
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 9/21/2022 7:43 PM, Jason Wang wrote:
> This patch allows the device features to be provisioned via
> netlink. This is done by:
>
> 1) validating the provisioned features to be a subset of the parent
>     features.
> 2) clearing the features that is not wanted by the userspace
>
> For example:
>
> # vdpa mgmtdev show
> pci/0000:02:00.0:
>    supported_classes net
>    max_supported_vqs 3
>    dev_features CSUM GUEST_CSUM CTRL_GUEST_OFFLOADS MAC GUEST_TSO4
>    GUEST_TSO6 GUEST_ECN GUEST_UFO HOST_TSO4 HOST_TSO6 HOST_ECN HOST_UFO
>    MRG_RXBUF STATUS CTRL_VQ CTRL_RX CTRL_VLAN CTRL_RX_EXTRA
>    GUEST_ANNOUNCE CTRL_MAC_ADDR RING_INDIRECT_DESC RING_EVENT_IDX
>    VERSION_1 ACCESS_PLATFORM
>
> 1) provision vDPA device with all features that are supported by the virtio-pci
>
> # vdpa dev add name dev1 mgmtdev pci/0000:02:00.0
> # vdpa dev config show
> dev1: mac 52:54:00:12:34:56 link up link_announce false mtu 65535
>    negotiated_features CSUM GUEST_CSUM CTRL_GUEST_OFFLOADS MAC
>    GUEST_TSO4 GUEST_TSO6 GUEST_ECN GUEST_UFO HOST_TSO4 HOST_TSO6
>    HOST_ECN HOST_UFO MRG_RXBUF STATUS CTRL_VQ CTRL_RX CTRL_VLAN
>    GUEST_ANNOUNCE CTRL_MAC_ADDR RING_INDIRECT_DESC RING_EVENT_IDX
>    VERSION_1 ACCESS_PLATFORM
>
> 2) provision vDPA device with a subset of the features
>
> # vdpa dev add name dev1 mgmtdev pci/0000:02:00.0 device_features 0x300020000
> # dev1: mac 52:54:00:12:34:56 link up link_announce false mtu 65535
>    negotiated_features CTRL_VQ VERSION_1 ACCESS_PLATFORM
>
> Reviewed-by: Eli Cohen <elic@nvidia.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   drivers/vdpa/virtio_pci/vp_vdpa.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c b/drivers/vdpa/virtio_pci/vp_vdpa.c
> index 04522077735b..4b28e0c95ba2 100644
> --- a/drivers/vdpa/virtio_pci/vp_vdpa.c
> +++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
> @@ -17,6 +17,7 @@
>   #include <linux/virtio_ring.h>
>   #include <linux/virtio_pci.h>
>   #include <linux/virtio_pci_modern.h>
> +#include <uapi/linux/vdpa.h>
>   
>   #define VP_VDPA_QUEUE_MAX 256
>   #define VP_VDPA_DRIVER_NAME "vp_vdpa"
> @@ -35,6 +36,7 @@ struct vp_vdpa {
>   	struct virtio_pci_modern_device *mdev;
>   	struct vp_vring *vring;
>   	struct vdpa_callback config_cb;
> +	u64 device_features;
>   	char msix_name[VP_VDPA_NAME_SIZE];
>   	int config_irq;
>   	int queues;
> @@ -66,9 +68,9 @@ static struct virtio_pci_modern_device *vp_vdpa_to_mdev(struct vp_vdpa *vp_vdpa)
>   
>   static u64 vp_vdpa_get_device_features(struct vdpa_device *vdpa)
>   {
> -	struct virtio_pci_modern_device *mdev = vdpa_to_mdev(vdpa);
> +	struct vp_vdpa *vp_vdpa = vdpa_to_vp(vdpa);
>   
> -	return vp_modern_get_features(mdev);
> +	return vp_vdpa->device_features;
>   }
>   
>   static int vp_vdpa_set_driver_features(struct vdpa_device *vdpa, u64 features)
> @@ -475,6 +477,7 @@ static int vp_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   	struct pci_dev *pdev = mdev->pci_dev;
>   	struct device *dev = &pdev->dev;
>   	struct vp_vdpa *vp_vdpa = NULL;
> +	u64 device_features;
>   	int ret, i;
>   
>   	vp_vdpa = vdpa_alloc_device(struct vp_vdpa, vdpa,
> @@ -491,6 +494,14 @@ static int vp_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   	vp_vdpa->queues = vp_modern_get_num_queues(mdev);
>   	vp_vdpa->mdev = mdev;
>   
> +	device_features = vp_modern_get_features(mdev);
> +	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) {
> +		if (add_config->device_features & ~device_features)
> +			return -EINVAL;
Should use "goto err" rather than direct return. Also, would be the best 
to tell user the reason why device creation is failing. In the other 
places of the same function, dev_err() or dev_warn() is used.

-Siwei

> +		device_features &= add_config->device_features;
> +	}
> +	vp_vdpa->device_features = device_features;
> +
>   	ret = devm_add_action_or_reset(dev, vp_vdpa_free_irq_vectors, pdev);
>   	if (ret) {
>   		dev_err(&pdev->dev,
> @@ -599,6 +610,7 @@ static int vp_vdpa_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   	mgtdev->id_table = mdev_id;
>   	mgtdev->max_supported_vqs = vp_modern_get_num_queues(mdev);
>   	mgtdev->supported_features = vp_modern_get_features(mdev);
> +	mgtdev->config_attr_mask = (1 << VDPA_ATTR_DEV_FEATURES);
>   	pci_set_master(pdev);
>   	pci_set_drvdata(pdev, vp_vdpa_mgtdev);
>