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

[David Marchand <david.marchand@redhat.com>]

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