Messages in this thread | | | Date | Tue, 1 Feb 2022 19:55:27 +0200 | From | Vladimir Oltean <> | Subject | Re: [PATCH net-next 4/5] net: dsa: mv88e6xxx: Improve multichip isolation of standalone ports |
| |
On Mon, Jan 31, 2022 at 04:46:54PM +0100, Tobias Waldekranz wrote: > Given that standalone ports are now configured to bypass the ATU and > forward all frames towards the upstream port, extend the ATU bypass to > multichip systems. > > Load VID 0 (standalone) into the VTU with the policy bit set. Since > VID 4095 (bridged) is already loaded, we now know that all VIDs in use > are always available in all VTUs. Therefore, we can safely enable > 802.1Q on DSA ports. > > Setting the DSA ports' VTU policy to TRAP means that all incoming > frames on VID 0 will be classified as MGMT - as a result, the ATU is > bypassed on all subsequent switches. > > With this isolation in place, we are able to support configurations > that are simultaneously very quirky and very useful. Quirky because it > involves looping cables between local switchports like in this > example: > > CPU > | .------. > .---0---. | .----0----. > | sw0 | | | sw1 | > '-1-2-3-' | '-1-2-3-4-' > $ @ '---' $ @ % % > > We have three physically looped pairs ($, @, and %). > > This is very useful because it allows us to run the kernel's > kselftests for the bridge on mv88e6xxx hardware. > > Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> > --- > drivers/net/dsa/mv88e6xxx/chip.c | 63 ++++++++++++++++++++++---------- > 1 file changed, 44 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index 8896709b9103..d0d766354669 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -1630,21 +1630,11 @@ static int mv88e6xxx_fid_map_vlan(struct mv88e6xxx_chip *chip, > > int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *fid_bitmap) > { > - int i, err; > - u16 fid; > - > bitmap_zero(fid_bitmap, MV88E6XXX_N_FID); > > - /* Set every FID bit used by the (un)bridged ports */ > - for (i = 0; i < mv88e6xxx_num_ports(chip); ++i) { > - err = mv88e6xxx_port_get_fid(chip, i, &fid); > - if (err) > - return err; > - > - set_bit(fid, fid_bitmap); > - } > - > - /* Set every FID bit used by the VLAN entries */ > + /* Every FID has an associated VID, so walking the VTU > + * will discover the full set of FIDs in use. > + */
So practically, regardless of whether the switch supports VTU policy or not, we still load VID 0 in the VTU, and this simplifies the driver a bit. Could we also simplify mv88e6xxx_port_db_dump() by deleting the mv88e6xxx_port_get_fid() from there (and then delete this function altogether)?
I think the mv88e6xxx_port_set_fid() call is now useless too?
> return mv88e6xxx_vtu_walk(chip, mv88e6xxx_fid_map_vlan, fid_bitmap); > } > > @@ -1657,10 +1647,7 @@ static int mv88e6xxx_atu_new(struct mv88e6xxx_chip *chip, u16 *fid) > if (err) > return err; > > - /* The reset value 0x000 is used to indicate that multiple address > - * databases are not needed. Return the next positive available. > - */ > - *fid = find_next_zero_bit(fid_bitmap, MV88E6XXX_N_FID, 1); > + *fid = find_first_zero_bit(fid_bitmap, MV88E6XXX_N_FID); > if (unlikely(*fid >= mv88e6xxx_num_databases(chip))) > return -ENOSPC; > > @@ -2152,6 +2139,9 @@ static int mv88e6xxx_port_vlan_join(struct mv88e6xxx_chip *chip, int port, > if (!vlan.valid) { > memset(&vlan, 0, sizeof(vlan)); > > + if (vid == MV88E6XXX_VID_STANDALONE) > + vlan.policy = true; > + > err = mv88e6xxx_atu_new(chip, &vlan.fid); > if (err) > return err; > @@ -2949,8 +2939,43 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) > if (err) > return err; > > + /* On chips that support it, set all DSA ports' VLAN policy to > + * TRAP. In combination with loading MV88E6XXX_VID_STANDALONE > + * as a policy entry in the VTU, this provides a better > + * isolation barrier between standalone ports, as the ATU is > + * bypassed on any intermediate switches between the incoming > + * port and the CPU. > + */ > + if (!dsa_is_user_port(ds, port) && chip->info->ops->port_set_policy) {
Will this not also affect FWD frames sent on behalf of VLAN-unaware bridges as they are received on CPU ports and upstream-facing DSA ports? Somehow I think you intend to make this match only on downstream-facing DSA ports.
> + err = chip->info->ops->port_set_policy(chip, port, > + MV88E6XXX_POLICY_MAPPING_VTU, > + MV88E6XXX_POLICY_ACTION_TRAP); > + if (err) > + return err; > + } > + > + /* User ports start out in standalone mode and 802.1Q is > + * therefore disabled. On DSA ports, all valid VIDs are always > + * loaded in the VTU - therefore, enable 802.1Q in order to take > + * advantage of VLAN policy on chips that supports it. > + */
Is this really needed? I thought cascade ports parse the VID from the DSA header regardless of 802.1Q mode.
> err = mv88e6xxx_port_set_8021q_mode(chip, port, > - MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED); > + dsa_is_user_port(ds, port) ? > + MV88E6XXX_PORT_CTL2_8021Q_MODE_DISABLED : > + MV88E6XXX_PORT_CTL2_8021Q_MODE_SECURE); > + if (err) > + return err; > + > + /* Bind MV88E6XXX_VID_STANDALONE to MV88E6XXX_FID_STANDALONE by > + * virtue of the fact that mv88e6xxx_atu_new() will pick it as > + * the first free FID. This will be used as the private PVID for > + * unbridged ports. Shared (DSA and CPU) ports must also be > + * members of this VID, in order to trap all frames assigned to > + * it to the CPU. > + */ > + err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_STANDALONE, > + MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED, > + false); > if (err) > return err; > > @@ -2963,7 +2988,7 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_chip *chip, int port) > * relying on their port default FID. > */ > err = mv88e6xxx_port_vlan_join(chip, port, MV88E6XXX_VID_BRIDGED, > - MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNTAGGED, > + MV88E6XXX_G1_VTU_DATA_MEMBER_TAG_UNMODIFIED,
I think the idea with UNTAGGED here was that packets sent by tag_dsa.c with TX forwarding offload on behalf of a VLAN-unaware bridge have VID 4095. By setting the port as untagged, that VID is stripped on egress. If you make it UNMODIFIED, the outside world will see it. Or am I wrong?
> false); > if (err) > return err; > -- > 2.25.1 >
| |