From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id CE12645B45; Wed, 16 Oct 2024 12:16:29 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B716040144; Wed, 16 Oct 2024 12:16:29 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id 8A597400D6 for ; Wed, 16 Oct 2024 12:16:28 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1729073788; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=5FAPT7KkwxHiZGv4f7s3FA43k1IrQTJHKgq4G0uhMGY=; b=LCNBow86ulRgfsKk4JLvcS3dHv4f/pdepKmAfTLWIKupGxoj2UArDzJUrzeLE6f4EOYCxs PIV3HyytK49Uotboj9EWlDGYdhEboxUvlxkKzAuG/01Mw3B8eAgbEgEl1EA/yhO+gCmUwY Mcr6cwrXPsPWTPg68voH7FE8pp5xWdQ= Received: from mail-lf1-f71.google.com (mail-lf1-f71.google.com [209.85.167.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-527-bxmVzlM7PBKS3NmyCXb7Pw-1; Wed, 16 Oct 2024 06:16:26 -0400 X-MC-Unique: bxmVzlM7PBKS3NmyCXb7Pw-1 Received: by mail-lf1-f71.google.com with SMTP id 2adb3069b0e04-539ea3d778dso3084503e87.1 for ; Wed, 16 Oct 2024 03:16:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729073785; x=1729678585; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=5FAPT7KkwxHiZGv4f7s3FA43k1IrQTJHKgq4G0uhMGY=; b=ApRs3rcIlKnQioo3yKteJWQWpNXqZz1p9tRLvKuc0Au1flEm2ZUiaOWl3vHAC5wLcB 1eDeLcKgscsCJY5q5tCqW2y5Cpchb3jZ2WY4eV1T9c4Cf8jVVkJ+nzN4xsKmf+oRr8G3 kH50Kz3q76NcCWW4+fVCuGOZoEyM5kVEy7DAoJLl0ffXtqk504zhB7DtPMxEqewkSQW+ 5wNxjMVmdflzpCQt96PczleYzvdDZUrNSCEYh9gQ5j0z+62pd+s2UUGyGwlDnlAUqsw7 sxhJ0OujyqBMaksqdwWMqhsK4TFqLUX9gBm/FfNhOJFydUps56kNc2c6K2bbSTaf/wX/ fsUw== X-Gm-Message-State: AOJu0Yx65cPhGlYl9G2sJegwmddDBWpyRlOJtSFF3zPRub/wM03zcTAq 8ZG34dFcla5eXI31gy2Bh4BW+K9Qxl/tjCwQTBV6NCnMh2Ys1uE16Z+sysE9T74h/cSP3Mdch4i 3+5vPiXAl8KtSkcBIisMWi0dZ2emab4Zeli3EY/oxGmfs1bVOspBnGR7rBUOoYhoZhG4Gk3nuzP drmM6Lbi9ZzGwPQkk= X-Received: by 2002:a05:6512:33cd:b0:539:905c:15ab with SMTP id 2adb3069b0e04-539e55187aemr7023747e87.32.1729073785228; Wed, 16 Oct 2024 03:16:25 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHBUJ3nP1XPpAd3WNG22KnBFUaZrC7uUehjGDtZqAvonMUls9MNK3cO9gB/MfCrP350anRTKQlFEDelEMOfPWM= X-Received: by 2002:a05:6512:33cd:b0:539:905c:15ab with SMTP id 2adb3069b0e04-539e55187aemr7023732e87.32.1729073784812; Wed, 16 Oct 2024 03:16:24 -0700 (PDT) MIME-Version: 1.0 References: <20241001091507.98785-1-david.marchand@redhat.com> In-Reply-To: From: David Marchand Date: Wed, 16 Oct 2024 12:16:13 +0200 Message-ID: Subject: Re: [PATCH] net/iavf: simplify mailbox message generation for mac To: Bruce Richardson Cc: dev@dpdk.org, Jingjing Wu X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, Oct 15, 2024 at 6:23=E2=80=AFPM Bruce Richardson 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 addresse= s > > would be marked as VIRTCHNL_ETHER_ADDR_PRIMARY if multiple messages had > > been effectively sent. > > > > Signed-off-by: David Marchand > > --- > > 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_vchn= l.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) * IAV= F_NUM_MACADDR_MAX]; > > The struct virtchnl_ether_addr_list variable includes a virtchnl_ether_ad= dr > 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 issu= e. 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. --=20 David Marchand