| Message ID | 20220302191417.1288145-1-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | DSA unicast filtering | expand |
Hi Vladimir, On 3/2/2022 11:14 AM, Vladimir Oltean wrote: > 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. Thanks a lot for submitting this, really happy to see a solution being brought upstream. I will be reviewing this in more details later on, but from where I left a few years ago, the two challenges that I had are outlined below, and I believe we have not quite addressed them yet: - for switches that implement global VLAN filtering, upper VLAN interfaces on top of standalone ports would require programming FDB and MDB entries with the appropriate VLAN ID, however there is no such tracking today AFAICT, so we are not yet solving those use cases yet, right? - what if the switch does not support FDB/MDB isolation, what would be our options here? As you might remember from a few months ago, the Broadcom roboswitch do not have any isolation, but what they can do is internally tag Ethernet frames with two VLAN tags, an that may be used as a form of isolation > > Vladimir Oltean (10): > net: dsa: remove workarounds for changing master promisc/allmulti only > while up > net: dsa: rename the host FDB and MDB methods to contain the "bridge" > namespace > net: dsa: install secondary unicast and multicast addresses as host > FDB/MDB > net: dsa: install the primary unicast MAC address as standalone port > host FDB > net: dsa: manage flooding on the CPU ports > net: dsa: felix: migrate host FDB and MDB entries when changing tag > proto > net: dsa: felix: migrate flood settings from NPI to tag_8021q CPU port > net: dsa: felix: start off with flooding disabled on the CPU port > net: dsa: felix: stop clearing CPU flooding in felix_setup_tag_8021q > net: mscc: ocelot: accept configuring bridge port flags on the NPI > port > > drivers/net/dsa/ocelot/felix.c | 241 ++++++++++++++++++++------ > drivers/net/ethernet/mscc/ocelot.c | 3 + > include/net/dsa.h | 7 + > net/dsa/dsa.c | 40 +++++ > net/dsa/dsa_priv.h | 53 +++++- > net/dsa/port.c | 160 +++++++++++++----- > net/dsa/slave.c | 261 +++++++++++++++++++++++------ > 7 files changed, 609 insertions(+), 156 deletions(-) >
Hi Florian, On Wed, Mar 02, 2022 at 11:30:41AM -0800, Florian Fainelli wrote: > Hi Vladimir, > > On 3/2/2022 11:14 AM, Vladimir Oltean wrote: > > 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. > > Thanks a lot for submitting this, really happy to see a solution being > brought upstream. I will be reviewing this in more details later on, but > from where I left a few years ago, the two challenges that I had are > outlined below, and I believe we have not quite addressed them yet: > > - for switches that implement global VLAN filtering, upper VLAN interfaces > on top of standalone ports would require programming FDB and MDB entries > with the appropriate VLAN ID, however there is no such tracking today > AFAICT, so we are not yet solving those use cases yet, right? Yes, here we're entering "brave" territory. With this patch set we have: static inline bool dsa_switch_supports_uc_filtering(struct dsa_switch *ds) { return ds->ops->port_fdb_add && ds->ops->port_fdb_del && ds->fdb_isolation && !ds->vlan_filtering_is_global && !ds->needs_standalone_vlan_filtering; } so yes, the use cases you mention are disqualified. To remove that restriction, every time our .ndo_vlan_rx_add_vid is called, we'd need to walk over our address lists (kept by struct net_device :: uc and mc), and replay them in the newly added VLAN. This is the easy part; the harder part is that we'd need to keep one more list of standalone VLANs added on a port, in order to do the reverse - every time dsa_slave_sync_uc() is called, we program that address not only in VID 0, but also in all VIDs from the list of standalone VLANs on that port. It might be possible to use vlan_for_each() for this. In any case it is incremental complexity which I've not yet added. This solution would install more host FDB entries than needed - the Cartesian product of RX VLANs and secondary MAC addresses. I'm ok with that, but there's also Ivan Khoronzhuk's patch series for Independent Virtual Device Filtering (IVDF), which basically annotates each MAC address from struct net_device with a VID. That one really seems excessive, at this stage, to me. > - what if the switch does not support FDB/MDB isolation, what would be our > options here? As you might remember from a few months ago, the Broadcom > roboswitch do not have any isolation, but what they can do is internally tag > Ethernet frames with two VLAN tags, an that may be used as a form of > isolation The central thing I had in mind is that DSA doesn't really enforce that packets are sent as 'control traffic'. The users of tag_8021q are the closest example to me - routing towards the correct egress port is done through a VID added to the packet, later stripped. The essential characteristic of a packet sent by a DSA tagger as a 'data packet' is that the FDB lookup is unavoidable. The reason why I added FDB isolation as a requirement is this: +--------------------+ | | | +------+ +------+ | | | swp0 | | swp1 | | +-+------+-+------+--+ | | +--------+ swp0 has 192.168.100.1/24, swp3 has 192.168.100.2/24, they are in different net namespaces or vrfs and want to ping each other. Without FDB isolation, the mantra is that DSA will keep the FDB devoid of swp0's and swp1's MAC addresses. Flooding is enabled on all ports and the CPU port, and learning is disabled. With FDB isolation and this patch series, DSA will install the MAC address of swp0 on the CPU port in swp0's private database, and the address of swp1 on the CPU port in swp1's private database. It is the driver's responsibility to then distinguish between those 2 databases the best it can. VID, FID, EFID, you name it. When swp0 sends a packet to swp1, it has swp0's MAC SA and swp1's MAC DA. If DSA doesn't know in the general sense that there will be no FDB lookup, then the best it can do is to ensure that swp0's and swp1's address are at least not in the same database. Because otherwise, the CPU port will see a packet with swp1's MAC DA, but the destination for that address is the CPU port => drop. Sadly I don't know if this scenario is hypothetical or not. With sja1105, it wasn't until I converted it to isolate the FDBs (packets are sent as 'data traffic', but every standalone port has a different pvid, and packets sent from the CPU follow the egress port's pvid). If there are more which I haven't caught (there are many DSA drivers which I don't know very well), this would break them. So I wanted to be on the conservative side, but I suppose I could be convinced otherwise. Please note that there is hope. On ocelot, standalone ports share the same pvid of 0, and yet it still declares FDB isolation. This is because traffic sent over the NPI port (using the "ocelot" tagger) bypasses the FDB lookup and it will always reach its intended destination. And using the "ocelot-8021q" tagger, packets hit a VCAP IS2 TCAM rule on the ingress of the CPU port that redirects them to the desired egress port, before the packet processing reaches the VLAN table. As for the Roboswitch discussion: https://lore.kernel.org/all/04d700b6-a4f7-b108-e711-d400ef01cc2e@bang-olufsen.dk/T/#mb2c185003d8a58c691918e056cf09fd0ef6d1bd0 that was mainly about TX forwarding offload, i.o.w. "how to use the FDB lookup of data plane packets to your benefit, instead of against you". In this context of FDB isolation I think it would be more beneficial to you to keep sending control packets, if those bypass the FDB. It is the bridge TX forwarding offload that I don't see how you could do. My takeaway from that discussion is that the IMP port processes the VID from the packet if VLAN awareness is turned on globally, and probably reverts to a single default VID otherwise. So even though you could arrange for each VLAN-unaware bridge to have a driver-private port pvid, and your tagger could wrap packets in an outer VLAN tag with this value, the fundamental problem seems to be that VLAN awareness is global in the first place. So either your user ports can offload a VLAN-unaware bridge, case in which the IMP can't see the VID from the packets, or the IMP can see the VID from the packets, but you can't offload a VLAN-unaware bridge. In this case and if I have understood all the information, I think what you can do is deny multiple bridges (regardless of VLAN awareness), configure standalone ports to use pvid 0 or 4095 (and different from the pvid of VLAN-unaware bridge ports) and just declare that you support FDB isolation.
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. 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. 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? 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. Kind regards, Alvin
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.
Hello: This series was applied to netdev/net-next.git (master) by David S. Miller <davem@davemloft.net>: On Wed, 2 Mar 2022 21:14:07 +0200 you wrote: > 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. > > [...] Here is the summary with links: - [net-next,01/10] net: dsa: remove workarounds for changing master promisc/allmulti only while up https://git.kernel.org/netdev/net-next/c/35aae5ab9121 - [net-next,02/10] net: dsa: rename the host FDB and MDB methods to contain the "bridge" namespace https://git.kernel.org/netdev/net-next/c/68d6d71eafd1 - [net-next,03/10] net: dsa: install secondary unicast and multicast addresses as host FDB/MDB https://git.kernel.org/netdev/net-next/c/5e8a1e03aa4d - [net-next,04/10] net: dsa: install the primary unicast MAC address as standalone port host FDB https://git.kernel.org/netdev/net-next/c/499aa9e1b332 - [net-next,05/10] net: dsa: manage flooding on the CPU ports https://git.kernel.org/netdev/net-next/c/7569459a52c9 - [net-next,06/10] net: dsa: felix: migrate host FDB and MDB entries when changing tag proto https://git.kernel.org/netdev/net-next/c/f9cef64fa23f - [net-next,07/10] net: dsa: felix: migrate flood settings from NPI to tag_8021q CPU port https://git.kernel.org/netdev/net-next/c/b903a6bd2e19 - [net-next,08/10] net: dsa: felix: start off with flooding disabled on the CPU port https://git.kernel.org/netdev/net-next/c/90897569beb1 - [net-next,09/10] net: dsa: felix: stop clearing CPU flooding in felix_setup_tag_8021q https://git.kernel.org/netdev/net-next/c/0cc369800e5f - [net-next,10/10] net: mscc: ocelot: accept configuring bridge port flags on the NPI port https://git.kernel.org/netdev/net-next/c/ac4552096023 You are awesome, thank you!
Vladimir Oltean <vladimir.oltean@nxp.com> writes: > 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. I think I'm not quite grokking what it means for a port to have its own database. Above you corrected me by stating that this patchset does not install FDB entries "on standalone ports", but on the CPU ports. The way I parse this is: you are adding an FDB entry to the database of the CPU port. But how would that help with the "line side to CPU side" direction? When a packet from the "line side" enters one of the (standalone) user ports, the switch would appeal to the forwarding database of that port, not the CPU port, no? So how does it help to install FDB entries on the CPU port(s)? Basically for a switch with two ports, swp0 and swp1, with MAC addresses foo and bar respectively, we want the following FDB entries to be installed when both ports are in standalone mode: MAC | VID | PORT ----+-----+------------- foo | 0 | cpu_port_num bar | 0 | cpu_port_num Such that, when swp0 receives a packet with MAC DA 'foo', it knows to forward that packet to cpu_port_num, i.e. towards the CPU port, instead of flooding/dropping/trapping it. And ditto for swp2 and 'bar'. Right? Now DSA assumes that each port has its own forwarding database, so let me annotate how I see that by adding another column "PORT DB": MAC | VID | PORT | PORT DB ----+-----+--------------+-------------- foo | 0 | cpu_port_num | swp0_port_num [*] bar | 0 | cpu_port_num | swp1_port_num [**] This codifies better that the entry for 'foo' should, ideally, only be used by the switch when it receives a packet from 'foo' on swp0, but not if it's received on swp1, etc. Internally this might be weakened to the same underlying FID, but as you already pointed out, this is acceptable. Overall this seems logical and OK to me, but I can't reconcile it with your statement that the FDB entries in question are installed "on the CPU port". Wouldn't that look like this? MAC | VID | PORT | PORT DB ----+-----+--------------+------------- foo | 0 | cpu_port_num | cpu_port_num bar | 0 | cpu_port_num | cpu_port_num ... which seems wrong. I'm suspecting/hoping that my above misunderstanding is just a matter of terminology. For the sake of emphasis, allow me to describe in plain English the FDB entries [*] and [**] above: [*] add an FDB entry on swp0 where [the host/station? with] MAC address 'foo' is behind the CPU port on VID 0 [**] add an FDB entry on swp1 where [the host/station? with] MAC address 'bar' is behind the CPU port on VID 0 Merely an attempt, and perhaps not idiomatic, but now you can tell me what's wrong in describing it that way :-) > > 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. Right, this is the scenario I had in mind. Fine by me if this is considered acceptable. I see how it is still better than flooding every packet. > 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 Yes, it does. > (a) you're not concerned about the hardware MAC DA filters being a bit > more relaxed than software expects Since they are dropped in software, it doesn't matter to me. > then I wouldn't worry too much about it, and keep using FID 0 for all > standalone ports. Great, thanks a lot for clarifying. Kind regards, Alvin
On Thu, Mar 03, 2022 at 02:20:29PM +0000, Alvin Šipraga wrote: > Vladimir Oltean <vladimir.oltean@nxp.com> writes: > > > 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. > > I think I'm not quite grokking what it means for a port to have its own > database. Above you corrected me by stating that this patchset does not > install FDB entries "on standalone ports", but on the CPU ports. The way > I parse this is: you are adding an FDB entry to the database of the > CPU port. No. I am adding an FDB entry _towards_ the CPU port. Perhaps I need to clarify what I mean by a "database". It is a construct used by DSA to denote "which subset of ports should match this entry". The entries from the database of a port should only be matched by that port when it operates as standalone. Not by other ports, not by bridged ports. The entries from the database of a bridge should only be matched by the ports in that bridge. Not by ports in other bridges, not by standalone ports. The entries from the database of a LAG should only be matched by the ports in that LAG. The FDB entries added here belong to the database of a standalone port. This is necessary, because classifying the packet to a FID is done on the ingress (=> standalone) port. But the destination is the CPU port. All shared (DSA and CPU) ports serve multiple databases. > But how would that help with the "line side to CPU side" > direction? When a packet from the "line side" enters one of the > (standalone) user ports, the switch would appeal to the forwarding > database of that port, not the CPU port, no? So how does it help to > install FDB entries on the CPU port(s)? See above. This is exactly why we pass the struct dsa_db as argument to .port_fdb_add(), and why you can't just deduce the database from the provided "port" argument. The CPU port must serve the databases of standalone ports. > Basically for a switch with two ports, swp0 and swp1, with MAC addresses > foo and bar respectively, we want the following FDB entries to be > installed when both ports are in standalone mode: > > MAC | VID | PORT > ----+-----+------------- > foo | 0 | cpu_port_num > bar | 0 | cpu_port_num > > Such that, when swp0 receives a packet with MAC DA 'foo', it knows to > forward that packet to cpu_port_num, i.e. towards the CPU port, instead > of flooding/dropping/trapping it. And ditto for swp2 and 'bar'. Right? > > Now DSA assumes that each port has its own forwarding database, so let > me annotate how I see that by adding another column "PORT DB": > > MAC | VID | PORT | PORT DB > ----+-----+--------------+-------------- > foo | 0 | cpu_port_num | swp0_port_num [*] > bar | 0 | cpu_port_num | swp1_port_num [**] > > This codifies better that the entry for 'foo' should, ideally, only be > used by the switch when it receives a packet from 'foo' on swp0, but not > if it's received on swp1, etc. Internally this might be weakened to the > same underlying FID, but as you already pointed out, this is acceptable. > > Overall this seems logical and OK to me, but I can't reconcile it with > your statement that the FDB entries in question are installed "on the > CPU port". Wouldn't that look like this? > > MAC | VID | PORT | PORT DB > ----+-----+--------------+------------- > foo | 0 | cpu_port_num | cpu_port_num > bar | 0 | cpu_port_num | cpu_port_num > > ... which seems wrong. See above. Basically, when I say "install a FDB entry on port X", I mean "install it and make it point towards port X". Maybe I should be more specific. > I'm suspecting/hoping that my above misunderstanding is just a matter > of terminology. For the sake of emphasis, allow me to describe in plain > English the FDB entries [*] and [**] above: > > [*] add an FDB entry on swp0 where [the host/station? with] MAC address > 'foo' is behind the CPU port on VID 0 > [**] add an FDB entry on swp1 where [the host/station? with] MAC address > 'bar' is behind the CPU port on VID 0 > > Merely an attempt, and perhaps not idiomatic, but now you can tell me > what's wrong in describing it that way :-) Perhaps: [*] add an FDB entry to the switch, with MAC address 'foo', which can only be matched for packets ingressing swp0 in standalone mode (i.e. is in swp0's port database), whose destination is the CPU port [**] same thing, just replace 'foo' with 'bar', and 'swp0' with 'swp1'
On Thu, Mar 03, 2022 at 04:35:29PM +0200, Vladimir Oltean wrote: > On Thu, Mar 03, 2022 at 02:20:29PM +0000, Alvin Šipraga wrote: > > Vladimir Oltean <vladimir.oltean@nxp.com> writes: > > > > > 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. > > > > I think I'm not quite grokking what it means for a port to have its own > > database. Above you corrected me by stating that this patchset does not > > install FDB entries "on standalone ports", but on the CPU ports. The way > > I parse this is: you are adding an FDB entry to the database of the > > CPU port. > > No. I am adding an FDB entry _towards_ the CPU port. > > Perhaps I need to clarify what I mean by a "database". It is a construct > used by DSA to denote "which subset of ports should match this entry". > The entries from the database of a port should only be matched by that > port when it operates as standalone. Not by other ports, not by bridged > ports. > The entries from the database of a bridge should only be matched by the > ports in that bridge. Not by ports in other bridges, not by standalone > ports. > The entries from the database of a LAG should only be matched by the > ports in that LAG. > > The FDB entries added here belong to the database of a standalone port. > This is necessary, because classifying the packet to a FID is done on > the ingress (=> standalone) port. But the destination is the CPU port. > > All shared (DSA and CPU) ports serve multiple databases. > > > But how would that help with the "line side to CPU side" > > direction? When a packet from the "line side" enters one of the > > (standalone) user ports, the switch would appeal to the forwarding > > database of that port, not the CPU port, no? So how does it help to > > install FDB entries on the CPU port(s)? > > See above. This is exactly why we pass the struct dsa_db as argument to > .port_fdb_add(), and why you can't just deduce the database from the > provided "port" argument. The CPU port must serve the databases of > standalone ports. > > > Basically for a switch with two ports, swp0 and swp1, with MAC addresses > > foo and bar respectively, we want the following FDB entries to be > > installed when both ports are in standalone mode: > > > > MAC | VID | PORT > > ----+-----+------------- > > foo | 0 | cpu_port_num > > bar | 0 | cpu_port_num > > > > Such that, when swp0 receives a packet with MAC DA 'foo', it knows to > > forward that packet to cpu_port_num, i.e. towards the CPU port, instead > > of flooding/dropping/trapping it. And ditto for swp2 and 'bar'. Right? > > > > Now DSA assumes that each port has its own forwarding database, so let > > me annotate how I see that by adding another column "PORT DB": > > > > MAC | VID | PORT | PORT DB > > ----+-----+--------------+-------------- > > foo | 0 | cpu_port_num | swp0_port_num [*] > > bar | 0 | cpu_port_num | swp1_port_num [**] > > > > This codifies better that the entry for 'foo' should, ideally, only be > > used by the switch when it receives a packet from 'foo' on swp0, but not > > if it's received on swp1, etc. Internally this might be weakened to the > > same underlying FID, but as you already pointed out, this is acceptable. > > > > Overall this seems logical and OK to me, but I can't reconcile it with > > your statement that the FDB entries in question are installed "on the > > CPU port". Wouldn't that look like this? > > > > MAC | VID | PORT | PORT DB > > ----+-----+--------------+------------- > > foo | 0 | cpu_port_num | cpu_port_num > > bar | 0 | cpu_port_num | cpu_port_num > > > > ... which seems wrong. > > See above. Basically, when I say "install a FDB entry on port X", I mean > "install it and make it point towards port X". Maybe I should be more > specific. > > > I'm suspecting/hoping that my above misunderstanding is just a matter > > of terminology. For the sake of emphasis, allow me to describe in plain > > English the FDB entries [*] and [**] above: > > > > [*] add an FDB entry on swp0 where [the host/station? with] MAC address > > 'foo' is behind the CPU port on VID 0 > > [**] add an FDB entry on swp1 where [the host/station? with] MAC address > > 'bar' is behind the CPU port on VID 0 > > > > Merely an attempt, and perhaps not idiomatic, but now you can tell me > > what's wrong in describing it that way :-) > > Perhaps: > > [*] add an FDB entry to the switch, with MAC address 'foo', which can > only be matched for packets ingressing swp0 in standalone mode > (i.e. is in swp0's port database), whose destination is the CPU > port > > [**] same thing, just replace 'foo' with 'bar', and 'swp0' with 'swp1' Let me give you a concrete example. The way in which sja1105 uses tag_8021q is that (assuming 4 ports), the following are configured as PVIDs in standalone mode: swp0 has PVID 3072 and MAC address 00:01:02:03:04:00 swp1 has PVID 3073 and MAC address 00:01:02:03:04:01 swp2 has PVID 3074 and MAC address 00:01:02:03:04:02 swp3 has PVID 3075 and MAC address 00:01:02:03:04:03 Port 4 is the CPU port. It doesn't have a PVID - all traffic ingressing it is VLAN-tagged, what isn't tagged is dropped. When DSA wants to program the unicast address of swp0 to hardware for unicast filtering, it calls .port_fdb_add(port 4, address 00:01:02:03:04:00, VID 0, DSA_DB_PORT with a "dp" of swp0). The sja1105 driver then translates the database of swp0 to VID/FID 3072, and installs MAC 00:01:02:03:04:00 & VID 3072 as a FDB entry towards port 4. This is what we want, because all packets received on swp0 in standalone mode are classified by the hardware to VID/FID 3072. They will therefore match the FDB entry and be routed to the CPU port. No other port will match on that entry, because every other port has a different PVID. DSA has no underlying knowledge of how the drivers handle their FIDs, it just passes the information they need such that they can make the necessary translation when installing FDB entries.
Vladimir Oltean <vladimir.oltean@nxp.com> writes: > On Thu, Mar 03, 2022 at 02:20:29PM +0000, Alvin Šipraga wrote: >> Vladimir Oltean <vladimir.oltean@nxp.com> writes: >> >> > 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. >> >> I think I'm not quite grokking what it means for a port to have its own >> database. Above you corrected me by stating that this patchset does not >> install FDB entries "on standalone ports", but on the CPU ports. The way >> I parse this is: you are adding an FDB entry to the database of the >> CPU port. > > No. I am adding an FDB entry _towards_ the CPU port. > > Perhaps I need to clarify what I mean by a "database". It is a construct > used by DSA to denote "which subset of ports should match this entry". > The entries from the database of a port should only be matched by that > port when it operates as standalone. Not by other ports, not by bridged > ports. > The entries from the database of a bridge should only be matched by the > ports in that bridge. Not by ports in other bridges, not by standalone > ports. > The entries from the database of a LAG should only be matched by the > ports in that LAG. > > The FDB entries added here belong to the database of a standalone port. > This is necessary, because classifying the packet to a FID is done on > the ingress (=> standalone) port. But the destination is the CPU port. > > All shared (DSA and CPU) ports serve multiple databases. > >> But how would that help with the "line side to CPU side" >> direction? When a packet from the "line side" enters one of the >> (standalone) user ports, the switch would appeal to the forwarding >> database of that port, not the CPU port, no? So how does it help to >> install FDB entries on the CPU port(s)? > > See above. This is exactly why we pass the struct dsa_db as argument to > .port_fdb_add(), and why you can't just deduce the database from the > provided "port" argument. The CPU port must serve the databases of > standalone ports. Right, this was what was I was missing. Now I follow. It helped to take another look at how sja1105 handles it: static int sja1105_fdb_add(struct dsa_switch *ds, int port, const unsigned char *addr, u16 vid, struct dsa_db db) { struct sja1105_private *priv = ds->priv; if (!vid) { switch (db.type) { case DSA_DB_PORT: vid = dsa_tag_8021q_standalone_vid(db.dp); break; case DSA_DB_BRIDGE: vid = dsa_tag_8021q_bridge_vid(db.bridge.num); break; default: return -EOPNOTSUPP; } } return priv->info->fdb_add_cmd(ds, port, addr, vid); } So, yes, we do call .port_fdb_add() "on" the CPU port (i.e. the 'port' argument refers to the port number of the CPU port). But this just means telling the switch to forward packets with MAC DA 'addr' to 'port'. The actual forwarding database - (standalone) port, or bridge, or LAG database - is determined by the 'db' parameter. It is up to the DSA driver to make sense of the info in db.{dp,bridge,lag} in order to ensure proper FDB isolation, such that the entry is only matched (on packet ingress) by the ports implied by the given database (a single standalone port, or a group of ports belonging to the same bridge, or a group of ports in a LAG). > >> Basically for a switch with two ports, swp0 and swp1, with MAC addresses >> foo and bar respectively, we want the following FDB entries to be >> installed when both ports are in standalone mode: >> >> MAC | VID | PORT >> ----+-----+------------- >> foo | 0 | cpu_port_num >> bar | 0 | cpu_port_num >> >> Such that, when swp0 receives a packet with MAC DA 'foo', it knows to >> forward that packet to cpu_port_num, i.e. towards the CPU port, instead >> of flooding/dropping/trapping it. And ditto for swp2 and 'bar'. Right? >> >> Now DSA assumes that each port has its own forwarding database, so let >> me annotate how I see that by adding another column "PORT DB": >> >> MAC | VID | PORT | PORT DB >> ----+-----+--------------+-------------- >> foo | 0 | cpu_port_num | swp0_port_num [*] >> bar | 0 | cpu_port_num | swp1_port_num [**] >> >> This codifies better that the entry for 'foo' should, ideally, only be >> used by the switch when it receives a packet from 'foo' on swp0, but not >> if it's received on swp1, etc. Internally this might be weakened to the >> same underlying FID, but as you already pointed out, this is acceptable. >> >> Overall this seems logical and OK to me, but I can't reconcile it with >> your statement that the FDB entries in question are installed "on the >> CPU port". Wouldn't that look like this? >> >> MAC | VID | PORT | PORT DB >> ----+-----+--------------+------------- >> foo | 0 | cpu_port_num | cpu_port_num >> bar | 0 | cpu_port_num | cpu_port_num >> >> ... which seems wrong. > > See above. Basically, when I say "install a FDB entry on port X", I mean > "install it and make it point towards port X". Maybe I should be more > specific. > >> I'm suspecting/hoping that my above misunderstanding is just a matter >> of terminology. For the sake of emphasis, allow me to describe in plain >> English the FDB entries [*] and [**] above: >> >> [*] add an FDB entry on swp0 where [the host/station? with] MAC address >> 'foo' is behind the CPU port on VID 0 >> [**] add an FDB entry on swp1 where [the host/station? with] MAC address >> 'bar' is behind the CPU port on VID 0 >> >> Merely an attempt, and perhaps not idiomatic, but now you can tell me >> what's wrong in describing it that way :-) > > Perhaps: > > [*] add an FDB entry to the switch, with MAC address 'foo', which can > only be matched for packets ingressing swp0 in standalone mode > (i.e. is in swp0's port database), whose destination is the CPU > port > > [**] same thing, just replace 'foo' with 'bar', and 'swp0' with 'swp1' Superb, thanks as always for the fine explanation. Kind regards, Alvin
On Thu, Mar 03, 2022 at 03:13:41PM +0000, Alvin Šipraga wrote: > So, yes, we do call .port_fdb_add() "on" the CPU port (i.e. the 'port' > argument refers to the port number of the CPU port). But this just means > telling the switch to forward packets with MAC DA 'addr' to 'port'. The > actual forwarding database - (standalone) port, or bridge, or LAG > database - is determined by the 'db' parameter. It is up to the DSA > driver to make sense of the info in db.{dp,bridge,lag} in order to > ensure proper FDB isolation, such that the entry is only matched (on > packet ingress) by the ports implied by the given database (a single > standalone port, or a group of ports belonging to the same bridge, or a > group of ports in a LAG). This is indeed what I was trying to say.
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. Vladimir Oltean (10): net: dsa: remove workarounds for changing master promisc/allmulti only while up net: dsa: rename the host FDB and MDB methods to contain the "bridge" namespace net: dsa: install secondary unicast and multicast addresses as host FDB/MDB net: dsa: install the primary unicast MAC address as standalone port host FDB net: dsa: manage flooding on the CPU ports net: dsa: felix: migrate host FDB and MDB entries when changing tag proto net: dsa: felix: migrate flood settings from NPI to tag_8021q CPU port net: dsa: felix: start off with flooding disabled on the CPU port net: dsa: felix: stop clearing CPU flooding in felix_setup_tag_8021q net: mscc: ocelot: accept configuring bridge port flags on the NPI port drivers/net/dsa/ocelot/felix.c | 241 ++++++++++++++++++++------ drivers/net/ethernet/mscc/ocelot.c | 3 + include/net/dsa.h | 7 + net/dsa/dsa.c | 40 +++++ net/dsa/dsa_priv.h | 53 +++++- net/dsa/port.c | 160 +++++++++++++----- net/dsa/slave.c | 261 +++++++++++++++++++++++------ 7 files changed, 609 insertions(+), 156 deletions(-)