Date:   Thu, 12 Dec 2019 10:25:17 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Yuval Avnery <yuvalav@...lanox.com>
Cc:     Jiri Pirko <jiri@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [PATCH net-next] netdevsim: Add max_vfs to bus_dev

On Thu, 12 Dec 2019 05:11:12 +0000, Yuval Avnery wrote:
> > > > 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_vfs  
> > > enforcement. Because subdev is not in yet.
> > > Will that be good enough?  
> > 
> > It'd be good to test some netdev API rather than just the enforcement itself
> > which is entirely in netdevsim, I think.
> > 
> > So max_vfs enforcement plus checking that ip link lists the correct number 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).

I was being lenient :) Your patch is only really needed when the
devlink API lands, since devlink will display all max VFs not enabled.

> > 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.
> > 
> > Sort of mirror how VF MAC addrs work, just a level deeper. The VF defaults
> > 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 drop
> > all its frames, but nothing stops the VF in legacy HW from writing its MAC
> > addr register).
> > 
> > IOW the devlink addr is the default/provisioned addr, not necessarily the
> > addr the PF has set _now_.
> > 
> > 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 notifier
> > which can ack/reject a request from the PF to change devlink's value..?
> > 
> > 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.

It'd be preferable for the behaviour of the kernel API to not be vendor
specific. That defeats the purpose of having an operating system as a
HW abstraction layer. SR-IOV devices of today are so FW heavy we can
make them behave whatever way we choose makes most sense.

> The MAC address in mlx5 is stored in the HW and
> persistent (until PF reset) , whether it is set by devlink or ip link.

Okay, let's see if I understand. The devlink and ip link interfaces
basically do the same thing but one reaches from control CPU and the
other one from the SR-IOV host? And on SR-IOV host reset the addresses
go back to 00:00.. i.e. any?

What happens if the SR-IOV host changes the MAC? Is it used by HW or is
the MAC provisioned by the control CPU used for things like spoof check?

Does the control CPU get a notification for SR-IOV host reset? In that
case the control CPU driver could restore the MAC addr.

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

To be clear - by persistent I meant that it survives the SR-IOV host's
resets, not necessarily written to NVRAM of any sort.

I'd like to see netdevsim to also serve as sort of a reference model
for device behaviour. Vendors who are not first to implement a feature
always complain that there is no documentation on how things should
work.

Powered by blists - more mailing lists