Re: [PATCH net-next 00/10] DSA unicast filtering

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: "Alvin ┼áipraga" <ALSI@bang-olufsen.dk>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Jakub Kicinski <kuba@kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Vivien Didelot <vivien.didelot@gmail.com>,
	Vladimir Oltean <olteanv@gmail.com>,
	Ido Schimmel <idosch@nvidia.com>,
	Tobias Waldekranz <tobias@waldekranz.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	"UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>
Subject: Re: [PATCH net-next 00/10] DSA unicast filtering
Date: Thu, 3 Mar 2022 13:18:21 +0000	[thread overview]
Message-ID: <20220303131820.4qzrcrdtx7tlhaei@skbuf> (raw)
In-Reply-To: <87wnhbdyee.fsf@bang-olufsen.dk>

Hi Alvin,

On Thu, Mar 03, 2022 at 12:16:26PM +0000, Alvin ┼áipraga wrote:
> Hi Vladimir,
> 
> Vladimir Oltean <vladimir.oltean@nxp.com> writes:
> 
> > This series doesn't attempt anything extremely brave, it just changes
> > the way in which standalone ports which support FDB isolation work.
> >
> > Up until now, DSA has recommended that switch drivers configure
> > standalone ports in a separate VID/FID with learning disabled, and with
> > the CPU port as the only destination, reached trivially via flooding.
> > That works, except that standalone ports will deliver all packets to the
> > CPU. We can leverage the hardware FDB as a MAC DA filter, and disable
> > flooding towards the CPU port, to force the dropping of packets with
> > unknown MAC DA.
> >
> > We handle port promiscuity by re-enabling flooding towards the CPU port.
> > This is relevant because the bridge puts its automatic (learning +
> > flooding) ports in promiscuous mode, and this makes some things work
> > automagically, like for example bridging with a foreign interface.
> > We don't delve yet into the territory of managing CPU flooding more
> > aggressively while under a bridge.
> >
> > The only switch driver that benefits from this work right now is the
> > NXP LS1028A switch (felix). The others need to implement FDB isolation
> > first, before DSA is going to install entries to the port's standalone
> > database. Otherwise, these entries might collide with bridge FDB/MDB
> > entries.
> >
> > This work was done mainly to have all the required features in place
> > before somebody starts seriously architecting DSA support for multiple
> > CPU ports. Otherwise it is much more difficult to bolt these features on
> > top of multiple CPU ports.
> 
> So, previously FDB entries were only installed on bridged ports. Now you
> also want to install FDB entries on standalone ports so that flooding
> can be disabled on standalone ports for the reasons stated in your cover
> letter.

Not "on standalone ports", but on the CPU ports, for the standalone
ports' addresses. Otherwise, yes.

> To implement FDB isolation in a DSA driver, a typical approach might be
> to use a filter ID (FID) for the FDB entries that is unique per
> bridge. That is, since FDB entries were only added on bridged ports
> (through learning or static entries added by software), the DSA driver
> could readily use the bridge_num of the bridge that is being offloaded
> to select the FID. The same bridge_num/FID would be used by the hardware
> for lookup/learning on the given port.

Yes and no. "FID" is a double-edged sword, it will actually depend upon
hardware implementation. FID in general is a mechanism for FDB
partitioning. If the VID->FID translation is keyed only by VID, then the
only intended use case for that is to support shared VLAN learning,
where all VIDs use the same FID (=> the same database for addresses).
This isn't very interesting for us.
If the VID->FID translation is keyed by {port, VID}, it is then possible
to make VIDs on different ports (part of different bridges) use
different FIDs, and that is what is interesting.

> If the above general statements are correct-ish, then my question here
> is: what should be the FID - or other equivalent unique identifier used
> by the hardware for FDB isolation - when the port is not offloading a
> bridge, but is standalone? If FDB isolation is implemented in hardware
> with something like FIDs, then do all standalone ports need to have a
> unique FID?

Not necessarily, although as far as the DSA core is concerned, we treat
drivers supporting FDB isolation as though each port has its own database.
For example, the sja1105 driver really has unique VIDs (=> FIDs) per
standalone port, so if we didn't treat it that way, it wouldn't work.

It is important to visualize that there are only 2 directions for a
packet to travel in a standalone port's FID: either from the line side
to the CPU, or from the CPU to the line side.

If the "CPU to line side" direction is unimpeded by FDB lookups, then
only the direction towards the CPU matters.

Further assuming that there is a single CPU port (you didn't ask about
multiple CPU ports, so I won't tell), the only practical implication of
standalone ports sharing the same FID is that they will, in effect,
share the same MAC DA filters. So, if you have 4 slave interfaces, each
with its own unicast and multicast address lists, what will happen is
that each port will accept the union of all others too. It's not
perfect, in the sense that unnecessary packets would still reach the
CPU, but they would still be dropped there.
For the felix/ocelot driver, I was ok with that => that driver uses the
same VID/FID of 0 for all standalone ports.

> For some context: I have been working on implementing offload features
> for the rtl8365mb driver and I can also support FDB isolation between
> bridged ports. The number of offloaded bridges is bounded by the number
> of FIDs available, which is 8. For standalone ports I use a reserved
> FID=0 which currently would never match any entries in the FDB, because
> learning is disabled on standalone ports and Linux does not install any
> FDB entries. When placed in a bridge, the FID of that port is then set
> to bridge_num, which - rather conveniently - is indexed by 1.
> 
> Your change seems to introduce a more generic concept of per-port
> FDB. How should one model the per-port FDB in hardware which uses FIDs?
> Should I ensure that all ports - standalone by default - start with a
> unique FID? That will be OK for switches with up to 8 ports, but for
> switches with more ports, I'm a but puzzled as to what I can do. Do I
> then have to declare that FDB isolation is unsupported
> (fdb_isolation=0)?
> 
> Hope the question makes sense.

As long as
(a) tag_rtl8_4.c sends packets to standalone ports in a way in which FDB
    lookups are bypassed
(a) you're not concerned about the hardware MAC DA filters being a bit
    more relaxed than software expects
then I wouldn't worry too much about it, and keep using FID 0 for all
standalone ports.