* [PATCH] net/iavf: simplify mailbox message generation for mac @ 2024-10-01 9:15 David Marchand 2024-10-15 16:22 ` Bruce Richardson 0 siblings, 1 reply; 3+ messages in thread From: David Marchand @ 2024-10-01 9:15 UTC (permalink / raw) To: dev; +Cc: bruce.richardson, Jingjing Wu 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] net/iavf: simplify mailbox message generation for mac 2024-10-01 9:15 [PATCH] net/iavf: simplify mailbox message generation for mac David Marchand @ 2024-10-15 16:22 ` Bruce Richardson 2024-10-16 10:16 ` David Marchand 0 siblings, 1 reply; 3+ messages in thread From: Bruce Richardson @ 2024-10-15 16:22 UTC (permalink / raw) To: David Marchand; +Cc: dev, Jingjing Wu 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 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] net/iavf: simplify mailbox message generation for mac 2024-10-15 16:22 ` Bruce Richardson @ 2024-10-16 10:16 ` David Marchand 0 siblings, 0 replies; 3+ messages in thread From: David Marchand @ 2024-10-16 10:16 UTC (permalink / raw) To: Bruce Richardson; +Cc: dev, Jingjing Wu 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-10-16 10:16 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-10-01 9:15 [PATCH] net/iavf: simplify mailbox message generation for mac David Marchand 2024-10-15 16:22 ` Bruce Richardson 2024-10-16 10:16 ` David Marchand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).