Re: [PATCH net-next v2 1/2] hinic: add generating mailbox random index support

[Jakub Kicinski]

On Fri, 31 Jul 2020 09:56:41 +0800 Luo bin wrote:
> add support to generate mailbox random id of VF to ensure that
> mailbox messages PF received are from the correct VF.
> 
> Signed-off-by: Luo bin <luobin9@xxxxxxxxxx>

> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c b/drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c
> index 47c93f946b94..c72aa8e8bce8 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_hw_mbox.c
> @@ -486,6 +486,111 @@ static void recv_mbox_handler(struct hinic_mbox_func_to_func *func_to_func,
>  	kfree(rcv_mbox_temp);
>  }
>  
> +static int set_vf_mbox_random_id(struct hinic_hwdev *hwdev, u16 func_id)
> +{
> +	struct hinic_mbox_func_to_func *func_to_func = hwdev->func_to_func;
> +	struct hinic_set_random_id rand_info = {0};
> +	u16 out_size = sizeof(rand_info);
> +	struct hinic_pfhwdev *pfhwdev;
> +	int ret;
> +
> +	pfhwdev = container_of(hwdev, struct hinic_pfhwdev, hwdev);
> +
> +	rand_info.version = HINIC_CMD_VER_FUNC_ID;
> +	rand_info.func_idx = func_id;
> +	rand_info.vf_in_pf = (u8)(func_id - hinic_glb_pf_vf_offset(hwdev->hwif));

this cast is unnecessary

> +	get_random_bytes(&rand_info.random_id, sizeof(u32));

get_random_u32()

> +
> +	func_to_func->vf_mbx_rand_id[func_id] = rand_info.random_id;
> +
> +	ret = hinic_msg_to_mgmt(&pfhwdev->pf_to_mgmt, HINIC_MOD_COMM,
> +				HINIC_MGMT_CMD_SET_VF_RANDOM_ID,
> +				&rand_info, sizeof(rand_info),
> +				&rand_info, &out_size, HINIC_MGMT_MSG_SYNC);
> +	if ((rand_info.status != HINIC_MGMT_CMD_UNSUPPORTED &&
> +	     rand_info.status) || !out_size || ret) {
> +		dev_err(&hwdev->hwif->pdev->dev, "Set VF random id failed, err: %d, status: 0x%x, out size: 0x%x\n",
> +			ret, rand_info.status, out_size);
> +		return -EIO;
> +	}
> +
> +	if (rand_info.status == HINIC_MGMT_CMD_UNSUPPORTED)
> +		return rand_info.status;
> +
> +	func_to_func->vf_mbx_old_rand_id[func_id] =
> +				func_to_func->vf_mbx_rand_id[func_id];
> +
> +	return 0;
> +}

> +static bool check_vf_mbox_random_id(struct hinic_mbox_func_to_func *func_to_func,
> +				    u8 *header)
> +{
> +	struct hinic_hwdev *hwdev = func_to_func->hwdev;
> +	struct hinic_mbox_work *mbox_work = NULL;
> +	u64 mbox_header = *((u64 *)header);
> +	u16 offset, src;
> +	u32 random_id;
> +	int vf_in_pf;
> +
> +	src = HINIC_MBOX_HEADER_GET(mbox_header, SRC_GLB_FUNC_IDX);
> +
> +	if (IS_PF_OR_PPF_SRC(src) || !func_to_func->support_vf_random)
> +		return true;
> +
> +	if (!HINIC_IS_PPF(hwdev->hwif)) {
> +		offset = hinic_glb_pf_vf_offset(hwdev->hwif);
> +		vf_in_pf = src - offset;
> +
> +		if (vf_in_pf < 1 || vf_in_pf > hwdev->nic_cap.max_vf) {
> +			dev_warn(&hwdev->hwif->pdev->dev,
> +				 "Receive vf id(0x%x) is invalid, vf id should be from 0x%x to 0x%x\n",
> +				 src, offset + 1,
> +				 hwdev->nic_cap.max_vf + offset);
> +			return false;
> +		}
> +	}
> +
> +	random_id = be32_to_cpu(*(u32 *)(header + MBOX_SEG_LEN +
> +					 MBOX_HEADER_SZ));
> +
> +	if (random_id == func_to_func->vf_mbx_rand_id[src] ||
> +	    random_id == func_to_func->vf_mbx_old_rand_id[src])

What guarantees src < MAX_FUNCTION_NUM ?

> +		return true;
> +
> +	dev_warn(&hwdev->hwif->pdev->dev,
> +		 "The mailbox random id(0x%x) of func_id(0x%x) doesn't match with pf reservation(0x%x)\n",
> +		 random_id, src, func_to_func->vf_mbx_rand_id[src]);
> +
> +	mbox_work = kzalloc(sizeof(*mbox_work), GFP_KERNEL);
> +	if (!mbox_work)
> +		return false;
> +
> +	mbox_work->func_to_func = func_to_func;
> +	mbox_work->src_func_idx = src;
> +
> +	INIT_WORK(&mbox_work->work, update_random_id_work_handler);
> +	queue_work(func_to_func->workq, &mbox_work->work);
> +
> +	return false;
> +}

> +int hinic_vf_mbox_random_id_init(struct hinic_hwdev *hwdev)
> +{
> +	u8 vf_in_pf;
> +	int err = 0;
> +
> +	if (HINIC_IS_VF(hwdev->hwif))
> +		return 0;
> +
> +	for (vf_in_pf = 1; vf_in_pf <= hwdev->nic_cap.max_vf; vf_in_pf++) {
> +		err = set_vf_mbox_random_id(hwdev, hinic_glb_pf_vf_offset
> +					    (hwdev->hwif) + vf_in_pf);

Parenthesis around hwdev->hwif not necessary

> +		if (err)
> +			break;
> +	}
> +
> +	if (err == HINIC_MGMT_CMD_UNSUPPORTED) {
> +		hwdev->func_to_func->support_vf_random = false;

So all VFs need to support the feature for it to be used?

> +		err = 0;
> +		dev_warn(&hwdev->hwif->pdev->dev, "Mgmt is unsupported to set VF%d random id\n",
> +			 vf_in_pf - 1);
> +	} else if (!err) {
> +		hwdev->func_to_func->support_vf_random = true;
> +		dev_info(&hwdev->hwif->pdev->dev, "PF Set VF random id success\n");

Is this info message really necessary?

> +	}