[PATCH] net/iavf: simplify mailbox message generation for mac

[PATCH] net/iavf: simplify mailbox message generation for mac

[David Marchand]

Caught by code review.
The mailbox message maximum size is larger than the biggest message
to update mac addresses.
Remove the double loop and add some build / assert checks.

This cleanup also fixes a (never hit) issue where multiple mac addresses
would be marked as VIRTCHNL_ETHER_ADDR_PRIMARY if multiple messages had
been effectively sent.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/iavf/iavf_vchnl.c | 86 ++++++++++++++---------------------
 1 file changed, 34 insertions(+), 52 deletions(-)

diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index 065ab3594c..e5113605ac 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -1397,62 +1397,44 @@ void
 iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
 {
 	struct virtchnl_ether_addr_list *list;
+	char buf[sizeof(*list) + sizeof(struct virtchnl_ether_addr) * IAVF_NUM_MACADDR_MAX];
 	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
-	struct rte_ether_addr *addr;
 	struct iavf_cmd_info args;
-	int len, err, i, j;
-	int next_begin = 0;
-	int begin = 0;
-
-	do {
-		j = 0;
-		len = sizeof(struct virtchnl_ether_addr_list);
-		for (i = begin; i < IAVF_NUM_MACADDR_MAX; i++, next_begin++) {
-			addr = &adapter->dev_data->mac_addrs[i];
-			if (rte_is_zero_ether_addr(addr))
-				continue;
-			len += sizeof(struct virtchnl_ether_addr);
-			if (len >= IAVF_AQ_BUF_SZ) {
-				next_begin = i + 1;
-				break;
-			}
-		}
+	int len, i;
 
-		list = rte_zmalloc("iavf_del_mac_buffer", len, 0);
-		if (!list) {
-			PMD_DRV_LOG(ERR, "fail to allocate memory");
-			return;
-		}
+	RTE_BUILD_BUG_ON(sizeof(buf) > IAVF_AQ_BUF_SZ);
+	list = (struct virtchnl_ether_addr_list *)buf;
 
-		for (i = begin; i < next_begin; i++) {
-			addr = &adapter->dev_data->mac_addrs[i];
-			if (rte_is_zero_ether_addr(addr))
-				continue;
-			rte_memcpy(list->list[j].addr, addr->addr_bytes,
-				   sizeof(addr->addr_bytes));
-			list->list[j].type = (j == 0 ?
-					      VIRTCHNL_ETHER_ADDR_PRIMARY :
-					      VIRTCHNL_ETHER_ADDR_EXTRA);
-			PMD_DRV_LOG(DEBUG, "add/rm mac:" RTE_ETHER_ADDR_PRT_FMT,
-				    RTE_ETHER_ADDR_BYTES(addr));
-			j++;
-		}
-		list->vsi_id = vf->vsi_res->vsi_id;
-		list->num_elements = j;
-		args.ops = add ? VIRTCHNL_OP_ADD_ETH_ADDR :
-			   VIRTCHNL_OP_DEL_ETH_ADDR;
-		args.in_args = (uint8_t *)list;
-		args.in_args_size = len;
-		args.out_buffer = vf->aq_resp;
-		args.out_size = IAVF_AQ_BUF_SZ;
-		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
-		if (err)
-			PMD_DRV_LOG(ERR, "fail to execute command %s",
-				    add ? "OP_ADD_ETHER_ADDRESS" :
-				    "OP_DEL_ETHER_ADDRESS");
-		rte_free(list);
-		begin = next_begin;
-	} while (begin < IAVF_NUM_MACADDR_MAX);
+	len = sizeof(struct virtchnl_ether_addr_list);
+	for (i = 0; i < IAVF_NUM_MACADDR_MAX; i++) {
+		struct rte_ether_addr *addr;
+
+		addr = &adapter->dev_data->mac_addrs[i];
+		if (rte_is_zero_ether_addr(addr))
+			continue;
+		len += sizeof(struct virtchnl_ether_addr);
+		assert(len <= IAVF_AQ_BUF_SZ);
+
+		rte_memcpy(list->list[i].addr, addr->addr_bytes,
+			   sizeof(addr->addr_bytes));
+		list->list[i].type = (i == 0 ?
+				      VIRTCHNL_ETHER_ADDR_PRIMARY :
+				      VIRTCHNL_ETHER_ADDR_EXTRA);
+		PMD_DRV_LOG(DEBUG, "add/rm mac:" RTE_ETHER_ADDR_PRT_FMT,
+			    RTE_ETHER_ADDR_BYTES(addr));
+	}
+
+	list->vsi_id = vf->vsi_res->vsi_id;
+	list->num_elements = i;
+	args.ops = add ? VIRTCHNL_OP_ADD_ETH_ADDR : VIRTCHNL_OP_DEL_ETH_ADDR;
+	args.in_args = (uint8_t *)list;
+	args.in_args_size = len;
+	args.out_buffer = vf->aq_resp;
+	args.out_size = IAVF_AQ_BUF_SZ;
+
+	if (iavf_execute_vf_cmd_safe(adapter, &args, 0))
+		PMD_DRV_LOG(ERR, "fail to execute command %s",
+			    add ? "OP_ADD_ETHER_ADDRESS" : "OP_DEL_ETHER_ADDRESS");
 }
 
 int
-- 
2.46.2

Re: [PATCH] net/iavf: simplify mailbox message generation for mac

[Bruce Richardson]

On Tue, Oct 01, 2024 at 11:15:07AM +0200, David Marchand wrote:
> Caught by code review.
> The mailbox message maximum size is larger than the biggest message
> to update mac addresses.
> Remove the double loop and add some build / assert checks.
> 
> This cleanup also fixes a (never hit) issue where multiple mac addresses
> would be marked as VIRTCHNL_ETHER_ADDR_PRIMARY if multiple messages had
> been effectively sent.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Thanks David. The overall approach looks good. While not familiar (yet)
with the iavf to pf protocol and messaging, some review comments inline
below.

/Bruce

>  drivers/net/iavf/iavf_vchnl.c | 86 ++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
> index 065ab3594c..e5113605ac 100644
> --- a/drivers/net/iavf/iavf_vchnl.c
> +++ b/drivers/net/iavf/iavf_vchnl.c
> @@ -1397,62 +1397,44 @@ void
>  iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
>  {
>  	struct virtchnl_ether_addr_list *list;
> +	char buf[sizeof(*list) + sizeof(struct virtchnl_ether_addr) * IAVF_NUM_MACADDR_MAX];

The struct virtchnl_ether_addr_list variable includes a virtchnl_ether_addr
element in it, so I think we may have an off-by-one error in a number of
places here. For example, should the multiplication not be
sizeof(...) * (IAVF_NUM_MACADDR_MAX - 1).

The overall logic I think would be more sane if the 'list' element of the
list structure was made an undimensioned array rather than being size 1,
but that's a more significant change.

>  	struct iavf_info *vf = IAVF_DEV_PRIVATE_TO_VF(adapter);
> -	struct rte_ether_addr *addr;
>  	struct iavf_cmd_info args;
> -	int len, err, i, j;
> -	int next_begin = 0;
> -	int begin = 0;
> -
> -	do {
> -		j = 0;
> -		len = sizeof(struct virtchnl_ether_addr_list);
> -		for (i = begin; i < IAVF_NUM_MACADDR_MAX; i++, next_begin++) {
> -			addr = &adapter->dev_data->mac_addrs[i];
> -			if (rte_is_zero_ether_addr(addr))
> -				continue;
> -			len += sizeof(struct virtchnl_ether_addr);
> -			if (len >= IAVF_AQ_BUF_SZ) {
> -				next_begin = i + 1;
> -				break;
> -			}
> -		}
> +	int len, i;
>  
> -		list = rte_zmalloc("iavf_del_mac_buffer", len, 0);
> -		if (!list) {
> -			PMD_DRV_LOG(ERR, "fail to allocate memory");
> -			return;
> -		}
> +	RTE_BUILD_BUG_ON(sizeof(buf) > IAVF_AQ_BUF_SZ);
> +	list = (struct virtchnl_ether_addr_list *)buf;

Minor nit - I'd tend towards putting the definition of "buf" first in the
function, so you could collapse this line into the definition of list on
the second line of the function. That allows the BUILD_BUG_ON to stand
alone and more visible.
>  
> -		for (i = begin; i < next_begin; i++) {
> -			addr = &adapter->dev_data->mac_addrs[i];
> -			if (rte_is_zero_ether_addr(addr))
> -				continue;
> -			rte_memcpy(list->list[j].addr, addr->addr_bytes,
> -				   sizeof(addr->addr_bytes));
> -			list->list[j].type = (j == 0 ?
> -					      VIRTCHNL_ETHER_ADDR_PRIMARY :
> -					      VIRTCHNL_ETHER_ADDR_EXTRA);
> -			PMD_DRV_LOG(DEBUG, "add/rm mac:" RTE_ETHER_ADDR_PRT_FMT,
> -				    RTE_ETHER_ADDR_BYTES(addr));
> -			j++;
> -		}
> -		list->vsi_id = vf->vsi_res->vsi_id;
> -		list->num_elements = j;
> -		args.ops = add ? VIRTCHNL_OP_ADD_ETH_ADDR :
> -			   VIRTCHNL_OP_DEL_ETH_ADDR;
> -		args.in_args = (uint8_t *)list;
> -		args.in_args_size = len;
> -		args.out_buffer = vf->aq_resp;
> -		args.out_size = IAVF_AQ_BUF_SZ;
> -		err = iavf_execute_vf_cmd_safe(adapter, &args, 0);
> -		if (err)
> -			PMD_DRV_LOG(ERR, "fail to execute command %s",
> -				    add ? "OP_ADD_ETHER_ADDRESS" :
> -				    "OP_DEL_ETHER_ADDRESS");
> -		rte_free(list);
> -		begin = next_begin;
> -	} while (begin < IAVF_NUM_MACADDR_MAX);
> +	len = sizeof(struct virtchnl_ether_addr_list);
> +	for (i = 0; i < IAVF_NUM_MACADDR_MAX; i++) {
> +		struct rte_ether_addr *addr;
> +
> +		addr = &adapter->dev_data->mac_addrs[i];
> +		if (rte_is_zero_ether_addr(addr))
> +			continue;
> +		len += sizeof(struct virtchnl_ether_addr);

Where this is the first address you find, you should not increment the
length since list[0] is already included in the initial structure size.

[I'm assuming here that the message is not meant to be terminated with a
special null, or zero address, in which case we need to zero the buffer at
the start].

Is it worthwhile splitting this main loop into two - one to find the first
element and set it to ADDR_PRIMARY, and then a second loop to add all the
other addresses, setting them to ADDR_EXTRA and increasing the length for
each one?

> +		assert(len <= IAVF_AQ_BUF_SZ);
> +
> +		rte_memcpy(list->list[i].addr, addr->addr_bytes,
> +			   sizeof(addr->addr_bytes));
> +		list->list[i].type = (i == 0 ?
> +				      VIRTCHNL_ETHER_ADDR_PRIMARY :
> +				      VIRTCHNL_ETHER_ADDR_EXTRA);
> +		PMD_DRV_LOG(DEBUG, "add/rm mac:" RTE_ETHER_ADDR_PRT_FMT,
> +			    RTE_ETHER_ADDR_BYTES(addr));
> +	}
> +
> +	list->vsi_id = vf->vsi_res->vsi_id;
> +	list->num_elements = i;
> +	args.ops = add ? VIRTCHNL_OP_ADD_ETH_ADDR : VIRTCHNL_OP_DEL_ETH_ADDR;
> +	args.in_args = (uint8_t *)list;

If you use "buf" here, you can avoid the cast, I suspect.

> +	args.in_args_size = len;
> +	args.out_buffer = vf->aq_resp;
> +	args.out_size = IAVF_AQ_BUF_SZ;
> +
> +	if (iavf_execute_vf_cmd_safe(adapter, &args, 0))
> +		PMD_DRV_LOG(ERR, "fail to execute command %s",
> +			    add ? "OP_ADD_ETHER_ADDRESS" : "OP_DEL_ETHER_ADDRESS");
>  }
>  
>  int
> -- 
> 2.46.2
> 

Re: [PATCH] net/iavf: simplify mailbox message generation for mac

[David Marchand]

On Tue, Oct 15, 2024 at 6:23 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Oct 01, 2024 at 11:15:07AM +0200, David Marchand wrote:
> > Caught by code review.
> > The mailbox message maximum size is larger than the biggest message
> > to update mac addresses.
> > Remove the double loop and add some build / assert checks.
> >
> > This cleanup also fixes a (never hit) issue where multiple mac addresses
> > would be marked as VIRTCHNL_ETHER_ADDR_PRIMARY if multiple messages had
> > been effectively sent.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
>
> Thanks David. The overall approach looks good. While not familiar (yet)
> with the iavf to pf protocol and messaging, some review comments inline
> below.
>
> /Bruce
>
> >  drivers/net/iavf/iavf_vchnl.c | 86 ++++++++++++++---------------------
> >  1 file changed, 34 insertions(+), 52 deletions(-)
> >
> > diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
> > index 065ab3594c..e5113605ac 100644
> > --- a/drivers/net/iavf/iavf_vchnl.c
> > +++ b/drivers/net/iavf/iavf_vchnl.c
> > @@ -1397,62 +1397,44 @@ void
> >  iavf_add_del_all_mac_addr(struct iavf_adapter *adapter, bool add)
> >  {
> >       struct virtchnl_ether_addr_list *list;
> > +     char buf[sizeof(*list) + sizeof(struct virtchnl_ether_addr) * IAVF_NUM_MACADDR_MAX];
>
> The struct virtchnl_ether_addr_list variable includes a virtchnl_ether_addr
> element in it, so I think we may have an off-by-one error in a number of
> places here. For example, should the multiplication not be
> sizeof(...) * (IAVF_NUM_MACADDR_MAX - 1).

I think the previous code had the same issue.
IIUC, it should not be a problem: the message would be larger than
actual content, but the header contains the exact number of
transported macs (virtchnl_ether_addr_list::num_elements).


>
> The overall logic I think would be more sane if the 'list' element of the
> list structure was made an undimensioned array rather than being size 1,
> but that's a more significant change.

Yes, that would be the cleaner approach.
Note that this struct is not the only one in virtchnl.h which has such issue.

And now, as I was diffing dpdk and kernel virtchnl.h, I can see they
went through the same approach to fix this, not that long ago.
5e7f59fa07f8 ("virtchnl: fix fake 1-elem arrays in structures
allocated as `nents + 1`")

It would be worth cleaning all of those, but I prefer to postpone this
for a separate patch.

I'll relook at your other comments but they look ok to me, I'll handle
those for v2 after rc1.


-- 
David Marchand