diff mbox series

[v3,5/6] vdpa/mlx5: conditionally show MTU and STATUS in config space

Message ID 1675400523-12519-6-git-send-email-si-wei.liu@oracle.com
State New
Headers
Series features provisioning fixes and mlx5_vdpa support |

Commit Message

Si-Wei Liu Feb. 3, 2023, 5:02 a.m. UTC 
  The spec says:
    mtu only exists if VIRTIO_NET_F_MTU is set
    status only exists if VIRTIO_NET_F_STATUS is set

We should only show MTU and STATUS conditionally depending on
the feature bits.

Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Comments

Eli Cohen Feb. 5, 2023, 9:36 a.m. UTC | #1 
On 03/02/2023 7:02, Si-Wei Liu wrote:
> The spec says:
>      mtu only exists if VIRTIO_NET_F_MTU is set
>      status only exists if VIRTIO_NET_F_STATUS is set
>
> We should only show MTU and STATUS conditionally depending on
> the feature bits.
>
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 3a6dbbc6..867ac18 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -3009,6 +3009,8 @@ static int event_handler(struct notifier_block *nb, unsigned long event, void *p
>   	struct mlx5_vdpa_wq_ent *wqent;
>   
>   	if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
> +		if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
> +			return NOTIFY_DONE;
>   		switch (eqe->sub_type) {
>   		case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
>   		case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> @@ -3118,16 +3120,20 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>   			goto err_alloc;
>   	}
>   
> -	err = query_mtu(mdev, &mtu);
> -	if (err)
> -		goto err_alloc;
> +	if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_MTU)) {

VIRTIO_NET_F_MTU is offered by the device. So conditional is always true. We are not done with feature negotiation at this stage so you may still set a value to device mtu if MTU won't be negotiated eventually. But that is not a problem because the spec says:

  VIRTIO_NET_F_MTU(3) Device maximum MTU reporting is supported. If 
offered by the device, device
advises driver about the value of its maximum MTU. If negotiated, the 
driver uses mtu as the maximum

MTU value.

So the driver will use whatever value is there only if negotiated.

> +		err = query_mtu(mdev, &mtu);
> +		if (err)
> +			goto err_alloc;
>   
> -	ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
> +		ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
> +	}
>   
> -	if (get_link_state(mvdev))
> -		ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
> -	else
> -		ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
> +	if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
> +		if (get_link_state(mvdev))
> +			ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
> +		else
> +			ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
> +	}
Same thing here. Feature negotiation is not complete yet and if

VIRTIO_NET_F_STATUS ends up not being negotiated, the driver will ignore this value.

>   
>   	if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
>   		memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);
Si-Wei Liu Feb. 6, 2023, 4:53 a.m. UTC | #2 
On 2/5/2023 1:36 AM, Eli Cohen wrote:
>
> On 03/02/2023 7:02, Si-Wei Liu wrote:
>> The spec says:
>>      mtu only exists if VIRTIO_NET_F_MTU is set
>>      status only exists if VIRTIO_NET_F_STATUS is set
>>
>> We should only show MTU and STATUS conditionally depending on
>> the feature bits.
>>
>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>> ---
>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c 
>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index 3a6dbbc6..867ac18 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -3009,6 +3009,8 @@ static int event_handler(struct notifier_block 
>> *nb, unsigned long event, void *p
>>       struct mlx5_vdpa_wq_ent *wqent;
>>         if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
>> +        if (!(ndev->mvdev.actual_features & 
>> BIT_ULL(VIRTIO_NET_F_STATUS)))
>> +            return NOTIFY_DONE;
>>           switch (eqe->sub_type) {
>>           case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
>>           case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
>> @@ -3118,16 +3120,20 @@ static int mlx5_vdpa_dev_add(struct 
>> vdpa_mgmt_dev *v_mdev, const char *name,
>>               goto err_alloc;
>>       }
>>   -    err = query_mtu(mdev, &mtu);
>> -    if (err)
>> -        goto err_alloc;
>> +    if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
>
> VIRTIO_NET_F_MTU is offered by the device. So conditional is always true. 
With the next patch in series that selectively provisions device 
features to mlx5_vdpa, this conditional will not be always true. That 
was the reason why I made patch 5 and 6 in a single commit, as this 
conditional will only be needed until feature provisioning is supported. 
Basically patch 5 and 6 are logically connected and technically should 
be separated out. I'm now puzzled, what was your thought then the change 
in patch 5 shouldn't be part of patch 6?

> We are not done with feature negotiation at this stage so you
'You' are who? device, driver or guest user?

> may still set a value to device mtu if MTU won't be negotiated 
> eventually. But that is not a problem because the spec says:
>
>  VIRTIO_NET_F_MTU(3) Device maximum MTU reporting is supported. If 
> offered by the device, device
> advises driver about the value of its maximum MTU. If negotiated, the 
> driver uses mtu as the maximum
>
> MTU value.
>
> So the driver will use whatever value is there only if negotiated.
My understanding is that 'vdpa dev config' now displays user provisioned 
config, or default config value advertised by the device, rather than 
what config driver will actually use.

>
>> +        err = query_mtu(mdev, &mtu);
>> +        if (err)
>> +            goto err_alloc;
>>   -    ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
>> +        ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
>> +    }
>>   -    if (get_link_state(mvdev))
>> -        ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, 
>> VIRTIO_NET_S_LINK_UP);
>> -    else
>> -        ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, 
>> ~VIRTIO_NET_S_LINK_UP);
>> +    if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
>> +        if (get_link_state(mvdev))
>> +            ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, 
>> VIRTIO_NET_S_LINK_UP);
>> +        else
>> +            ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, 
>> ~VIRTIO_NET_S_LINK_UP);
>> +    }
> Same thing here. Feature negotiation is not complete yet and if
>
> VIRTIO_NET_F_STATUS ends up not being negotiated, the driver will 
> ignore this value.
See above. With feature provisioning, whether the VIRTIO_NET_F_STATUS 
feature is advertised by device is subject to the device_features value 
provisioned by the host admin users.

-Siwei

>
>>         if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
>>           memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);
Eli Cohen Feb. 6, 2023, 6:52 a.m. UTC | #3 
On 06/02/2023 6:53, Si-Wei Liu wrote:
>
>
> On 2/5/2023 1:36 AM, Eli Cohen wrote:
>>
>> On 03/02/2023 7:02, Si-Wei Liu wrote:
>>> The spec says:
>>>      mtu only exists if VIRTIO_NET_F_MTU is set
>>>      status only exists if VIRTIO_NET_F_STATUS is set
>>>
>>> We should only show MTU and STATUS conditionally depending on
>>> the feature bits.
>>>
>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>> ---
>>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++--------
>>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c 
>>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index 3a6dbbc6..867ac18 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -3009,6 +3009,8 @@ static int event_handler(struct notifier_block 
>>> *nb, unsigned long event, void *p
>>>       struct mlx5_vdpa_wq_ent *wqent;
>>>         if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
>>> +        if (!(ndev->mvdev.actual_features & 
>>> BIT_ULL(VIRTIO_NET_F_STATUS)))
>>> +            return NOTIFY_DONE;
>>>           switch (eqe->sub_type) {
>>>           case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
>>>           case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
>>> @@ -3118,16 +3120,20 @@ static int mlx5_vdpa_dev_add(struct 
>>> vdpa_mgmt_dev *v_mdev, const char *name,
>>>               goto err_alloc;
>>>       }
>>>   -    err = query_mtu(mdev, &mtu);
>>> -    if (err)
>>> -        goto err_alloc;
>>> +    if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
>>
>> VIRTIO_NET_F_MTU is offered by the device. So conditional is always 
>> true. 
> With the next patch in series that selectively provisions device 
> features to mlx5_vdpa, this conditional will not be always true. That 
> was the reason why I made patch 5 and 6 in a single commit, as this 
> conditional will only be needed until feature provisioning is 
> supported. Basically patch 5 and 6 are logically connected and 
> technically should be separated out. I'm now puzzled, what was your 
> thought then the change in patch 5 shouldn't be part of patch 6?

No, I think breaking into two patches is the right way to go.

I missed the fact the for setting MTU and MAC you need the device to 
*offer* the feature and does not depend on negotiation.

>
>> We are not done with feature negotiation at this stage so you
> 'You' are who? device, driver or guest user?
>
>> may still set a value to device mtu if MTU won't be negotiated 
>> eventually. But that is not a problem because the spec says:
>>
>>  VIRTIO_NET_F_MTU(3) Device maximum MTU reporting is supported. If 
>> offered by the device, device
>> advises driver about the value of its maximum MTU. If negotiated, the 
>> driver uses mtu as the maximum
>>
>> MTU value.
>>
>> So the driver will use whatever value is there only if negotiated.
> My understanding is that 'vdpa dev config' now displays user 
> provisioned config, or default config value advertised by the device, 
> rather than what config driver will actually use.
Reviewed-by: Eli Cohen <elic@nvidia.com>
>
>>
>>> +        err = query_mtu(mdev, &mtu);
>>> +        if (err)
>>> +            goto err_alloc;
>>>   -    ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
>>> +        ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
>>> +    }
>>>   -    if (get_link_state(mvdev))
>>> -        ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, 
>>> VIRTIO_NET_S_LINK_UP);
>>> -    else
>>> -        ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, 
>>> ~VIRTIO_NET_S_LINK_UP);
>>> +    if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
>>> +        if (get_link_state(mvdev))
>>> +            ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, 
>>> VIRTIO_NET_S_LINK_UP);
>>> +        else
>>> +            ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, 
>>> ~VIRTIO_NET_S_LINK_UP);
>>> +    }
>> Same thing here. Feature negotiation is not complete yet and if
>>
>> VIRTIO_NET_F_STATUS ends up not being negotiated, the driver will 
>> ignore this value.
> See above. With feature provisioning, whether the VIRTIO_NET_F_STATUS 
> feature is advertised by device is subject to the device_features 
> value provisioned by the host admin users.
>
> -Siwei
>
>>
>>>         if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
>>>           memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);
>
diff mbox series

Patch

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 3a6dbbc6..867ac18 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -3009,6 +3009,8 @@  static int event_handler(struct notifier_block *nb, unsigned long event, void *p
 	struct mlx5_vdpa_wq_ent *wqent;
 
 	if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
+		if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
+			return NOTIFY_DONE;
 		switch (eqe->sub_type) {
 		case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
 		case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
@@ -3118,16 +3120,20 @@  static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 			goto err_alloc;
 	}
 
-	err = query_mtu(mdev, &mtu);
-	if (err)
-		goto err_alloc;
+	if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
+		err = query_mtu(mdev, &mtu);
+		if (err)
+			goto err_alloc;
 
-	ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
+		ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
+	}
 
-	if (get_link_state(mvdev))
-		ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
-	else
-		ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
+	if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
+		if (get_link_state(mvdev))
+			ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
+		else
+			ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
+	}
 
 	if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
 		memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);