OS-6769 want i40e HW VLAN support

[joyent-automation]

OS-6769 want i40e HW VLAN support

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/6587/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@rmustacc commented at 2019-07-11T00:04:04

Patch Set 1:

(6 comments)

I tried to audit all the places that we relied on groups having mgi_addmac as valid. I think I caught most of them, but I'm not certain. All in all this seems pretty good, though there are a few edge cases I think we need to consider still.

@rzezeski commented at 2019-07-12T14:46:13

Patch Set 1:

(6 comments)

I'm a bit stuck on how to proceed in relation to aggrs. But wanted to add some comments to show you were my head is at for the moment.

Patch Set 1 code comments

usr/src/uts/common/io/i40e/i40e_gld.c#81 @rmustacc

See the notes on add. I think we still need to pass I40E_AQC_MACVLAN_DEL_IGNORE_VLAN when we are passed MAC_VLAN_UNTAGGED.

usr/src/uts/common/io/i40e/i40e_gld.c#81 @rzezeski

I addressed this under the other comment.

usr/src/uts/common/io/i40e/i40e_gld.c#153 @rmustacc

If we have MAC_VLAN_UNTAGGED don't we need to actually need to set I40E_AQC_MACVLAN_ADD_PERFECT_MATCH? Just like ixgbe does the logical equivalent in that case? Isn't that the distinction between having vid be zero and the flag?

usr/src/uts/common/io/i40e/i40e_gld.c#153 @rzezeski

I assume you meant to ask if we still need I40E_AQC_MACVLAN_ADD_IGNORE_VLAN here. Not only do we no longer need it, I don't think we ever should have enabled it. That flag indicates that we literally don't care what the VID value in the L2 frame is, just as long as the MAC matches. That is, the value could be anything and it'll be accepted as long as the MAC address matches. I think this means we have a bug into the current i40e driver: a client will see all traffic for a given MAC address, not just the untagged/VLAN it is interested in.

In any case, we want to be explicit about the VLANs we accept. And in order to accept untagged traffic the VID must be set to zero. This is covered in section 7.4.9.5.9.1 of the PG.

usr/src/uts/common/io/mac/mac.c#1973 @rmustacc

I think that we need to update this function to know to either call mac_group_addmac or to call the mac/vlan combo here. It may make more sense to change mac_group_addmac() to call the underlying endpoint with the undefined VLAN bit rather than to try and bubble everything up. I suspect we'll need to come back and visit how that intersects with aggrs, the only caller of this interface.

usr/src/uts/common/io/mac/mac.c#1973 @rzezeski

Yea, so we have a problem here. As I mentioned in the other comment, we don't want to set the "ignore VLAN" flag for i40e. And even if we had a way to set that flag only when dealing with aggrs we'd have the problem where all traffic (tagged and untagged) is being sent to the client's group. I think the only way to rectify this, without some kind of major filtering API overhaul, is to implement the new mgi_add_macvlan API for aggr, and then it will determine which API to use for its underlying port. So for an ixgbe port it would call the mgi_addmac + mgi_addvlan APIs, and for i40e it would call mgi_add_macvlan. I think this would work but I haven't thought it through completely.

usr/src/uts/common/io/mac/mac.c#5449 @rmustacc

Right now we don't check or care if a driver has both interfaces or not. Should we prefer the HW_MACVLAN if it is set over just the HW_VLAN?

usr/src/uts/common/io/mac/mac.c#5449 @rzezeski

Yes, we should prefer it.

usr/src/uts/common/io/mac/mac.c#5587 @rmustacc

It the above order is changed this will need to too. Maybe we should indicate what type of hw filter we've done in the map structure just to make life easier when tearing down so it's a little less implicit?

usr/src/uts/common/io/mac/mac.c#5587 @rzezeski

There should be no order because it should always be one or the other. This should be enforced at register to make it absolutely clear.

usr/src/uts/common/sys/mac_provider.h#473 @rmustacc

I don't believe we're actually enforcing that the add/rem_macvlan are exclusive with everything else. I don't know that we need to either. Just wanted to mention it.

usr/src/uts/common/sys/mac_provider.h#473 @rzezeski

We probably should enforce it to make it clear that it's one or the other.

[sjorge]

Wasn't this or part of this merged upstream?

[rmustacc]

On 3/11/20 1:53 PM, Jorge Schrauwen wrote: Wasn't this or part of this merged upstream?

No, it wasn't.

[sjorge]

@rmustacc most have had it confused with one of the other network bits... also github is drunk as I cannot unassigne people from this project :/