Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Yuval Avnery <yuvalav@mellanox.com>
To:     Jakub Kicinski <jakub.kicinski@netronome.com>
CC:     Jiri Pirko <jiri@mellanox.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH net-next] netdevsim: Add max_vfs to bus_dev
Thread-Topic: [PATCH net-next] netdevsim: Add max_vfs to bus_dev
Thread-Index: AQHVr871nDBB5hrvy0ak2C7+9Ay1lae1Oa8AgAAB1gCAABOZgIAACO8QgAArtYCAAAxfkIAAC50AgABVJmA=
Date:   Thu, 12 Dec 2019 05:11:12 +0000
Message-ID: <AM6PR05MB51425B74E736C5D765356DC8C5550@AM6PR05MB5142.eurprd05.prod.outlook.com>
References: <1576033133-18845-1-git-send-email-yuvalav@mellanox.com>
        <20191211095854.6cd860f1@cakuba.netronome.com>
        <AM6PR05MB514244DC6D25DDD433C0E238C55A0@AM6PR05MB5142.eurprd05.prod.outlook.com>
        <20191211111537.416bf078@cakuba.netronome.com>
        <AM6PR05MB5142CCAB9A06DAC199F7100CC55A0@AM6PR05MB5142.eurprd05.prod.outlook.com>
        <20191211142401.742189cf@cakuba.netronome.com>
        <AM6PR05MB51423D365FB5A8DB22B1DE62C55A0@AM6PR05MB5142.eurprd05.prod.outlook.com>
 <20191211154952.50109494@cakuba.netronome.com>
In-Reply-To: <20191211154952.50109494@cakuba.netronome.com>
Accept-Language: he-IL, en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: mellanox.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 11b89d97-da03-4d1e-404a-08d77ec1b4fe
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Dec 2019 05:11:12.4784
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: NfZaMyi7wj5y6YBHgqWX1R7sx4nmBwYkSdL3hg/6f6fxu+GqrKFQ+NbvLC8sMka1w72W4eL2qn0Vsz3e80cSlA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM6PR05MB5443
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Wednesday, December 11, 2019 3:50 PM
> To: Yuval Avnery <yuvalav@mellanox.com>
> Cc: Jiri Pirko <jiri@mellanox.com>; davem@davemloft.net;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next] netdevsim: Add max_vfs to bus_dev
>=20
> On Wed, 11 Dec 2019 23:25:09 +0000, Yuval Avnery wrote:
> > > -----Original Message-----
> > > From: Jakub Kicinski <jakub.kicinski@netronome.com>
> > > Sent: Wednesday, December 11, 2019 2:24 PM
> > > To: Yuval Avnery <yuvalav@mellanox.com>
> > > Cc: Jiri Pirko <jiri@mellanox.com>; davem@davemloft.net;
> > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH net-next] netdevsim: Add max_vfs to bus_dev
> > >
> > > On Wed, 11 Dec 2019 19:57:34 +0000, Yuval Avnery wrote:
> > > > > -----Original Message-----
> > > > > From: Jakub Kicinski <jakub.kicinski@netronome.com>
> > > > > Sent: Wednesday, December 11, 2019 11:16 AM
> > > > > To: Yuval Avnery <yuvalav@mellanox.com>
> > > > > Cc: Jiri Pirko <jiri@mellanox.com>; davem@davemloft.net;
> > > > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> > > > > Subject: Re: [PATCH net-next] netdevsim: Add max_vfs to bus_dev
> > > > >
> > > > > On Wed, 11 Dec 2019 18:19:56 +0000, Yuval Avnery wrote:
> > > > > > > On Wed, 11 Dec 2019 04:58:53 +0200, Yuval Avnery wrote:
> > > > > > > > Currently there is no limit to the number of VFs netdevsim =
can
> enable.
> > > > > > > > In a real systems this value exist and used by driver.
> > > > > > > > Fore example, Some features might need to consider this
> > > > > > > > value when allocating memory.
> > > > > > >
> > > > > > > Thanks for the patch!
> > > > > > >
> > > > > > > Can you shed a little bit more light on where it pops up?
> > > > > > > Just for my
> > > > > curiosity?
> > > > > >
> > > > > > Yes, like we described in the subdev threads.
> > > > > > User should be able to configure some attributes before the VF
> > > > > > was
> > > > > enabled.
> > > > > > So all those (persistent) VF attributes should be available
> > > > > > for query and configuration before VF was enabled.
> > > > > > The driver can allocate an array according to max_vfs to hold
> > > > > > all that data, like we do here in" vfconfigs".
> > > > >
> > > > > I was after more practical reasoning, are you writing some tests
> > > > > for subdev stuff that will depend on this change? :)
> > > >
> > > > Yes we are writing tests for subdev with this.
> > >
> > > Okay, please post v2 together with the tests. We don't accept
> > > netdevsim features without tests any more.
> >
> > I think the only test I can currently write is the enable SR-IOV max_vf=
s
> enforcement.
> > Because subdev is not in yet.
> > Will that be good enough?
>=20
> It'd be good to test some netdev API rather than just the enforcement its=
elf
> which is entirely in netdevsim, I think.
>=20
> So max_vfs enforcement plus checking that ip link lists the correct numbe=
r of
> entries (and perhaps the entries are in reset state after
> enable) would do IMO.

Ok, but this is possible regardless of my patch (to enable vfs).

>=20
> > > > This is the way mlx5 works.. is that practical enough?
> > > >
> > > > > > > > Signed-off-by: Yuval Avnery <yuvalav@mellanox.com>
> > > > > > > > Acked-by: Jiri Pirko <jiri@mellanox.com>
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/netdevsim/bus.c
> > > > > > > > b/drivers/net/netdevsim/bus.c index
> > > > > > > > 6aeed0c600f8..f1a0171080cb
> > > > > > > > 100644
> > > > > > > > --- a/drivers/net/netdevsim/bus.c
> > > > > > > > +++ b/drivers/net/netdevsim/bus.c
> > > > > > > > @@ -26,9 +26,9 @@ static struct nsim_bus_dev
> > > > > > > > *to_nsim_bus_dev(struct device *dev)  static int
> > > > > > > > nsim_bus_dev_vfs_enable(struct nsim_bus_dev
> > > > > > > *nsim_bus_dev,
> > > > > > > >  				   unsigned int num_vfs)  {
> > > > > > > > -	nsim_bus_dev->vfconfigs =3D kcalloc(num_vfs,
> > > > > > > > -					  sizeof(struct
> > > nsim_vf_config),
> > > > > > > > -					  GFP_KERNEL);
> > > > > > >
> > > > > > > You're changing the semantics of the enable/disable as well n=
ow.
> > > > > > > The old values used to be wiped when SR-IOV is disabled, now
> > > > > > > they will be retained across disable/enable pair.
> > > > > > >
> > > > > > > I think it'd be better if that wasn't the case. Users may
> > > > > > > expect a system to be in the same state after they enable
> > > > > > > SR-IOV, regardless if someone else used SR-IOV since last reb=
oot.
> > > > > >
> > > > > > Right,
> > > > > > But some values should retain across enable/disable, for
> > > > > > example MAC
> > > > > address which is persistent.
> > > > > > So maybe we need to retain some values, while resetting others
> > > > > > on
> > > > > disable?
> > > > > > Would that work?
> > > > >
> > > > > Mmm. That is a good question. For all practical purposes SR-IOV
> > > > > used to be local to the host that enables it until Smart/middle
> > > > > box NICs
> > > emerged.
> > > > >
> > > > > Perhaps the best way forward would be to reset the config that
> > > > > was set via legacy APIs and keep only the MACs provisioned via
> > > > > persistent
> > > devlink API?
> > > > >
> > > > > So for now we'd memset, and once devlink API lands reset
> selectively?
> > > >
> > > > Legacy is also persistent.
> > > > Currently when you set mac address with "ip link vf set mac" it is
> > > > persistent
> > > (at least in mlx5).
> > >
> > > "Currently in mlx5" - maybe, but this is netdevsim. Currently it
> > > clears the config on re-enable which I believe to be preferable as
> explained before.
> > >
> > > > But ip link only exposes enabled VFS, so driver on VF has to
> > > > reload to
> > > acquire this MAC.
> > > > With devlink subdev it will be possible to set the MAC before VF
> > > > was
> > > enabled.
> > >
> > > Yup, sure. As I said, once subdev is implemented, we will treat the
> > > addresses set by it differently. Those are inherently persistent or
> > > rather their life time is independent of just the SR-IOV host.
> >
> > Ok, got it.
> > I am just wondering how this works when you have "ip link" and devlink
> setting the MAC independently.
> > Will they show the same MAC?
> > Or ip link will show the non-persistent MAC And devlink the persistent?
>=20
> My knee jerk reaction is that we should populate the values to those set =
via
> devlink upon SR-IOV enable, but then if user overwrites those values that=
's
> their problem.
>=20
> Sort of mirror how VF MAC addrs work, just a level deeper. The VF default=
s
> to the MAC addr provided by the PF after reset, but it can change it to
> something else (things may stop working because spoof check etc. will dro=
p
> all its frames, but nothing stops the VF in legacy HW from writing its MA=
C
> addr register).
>=20
> IOW the devlink addr is the default/provisioned addr, not necessarily the
> addr the PF has set _now_.
>=20
> Other options I guess are (a) reject the changes of the address from the =
PF
> once devlink has set a value; (b) provide some device->control CPU notifi=
er
> which can ack/reject a request from the PF to change devlink's value..?
>=20
> You guys posted the devlink patches a while ago, what was your
> implementation doing?

devlink simply calls the driver with set or get.
It is up to the vendor driver/HW if to make this address persistent or not.
The address is not saved in the devlink layer.
The MAC address in mlx5 is stored in the HW and
persistent (until PF reset) , whether it is set by devlink or ip link.

So from what I understand, we have the freedom to choose how netdevsim
behave in this case, which means non-persistent is ok.