Adds NetIf functions for filtering MAC addresses

[evpopov]

Description

Thanks you @htibosch for suggesting this functionality be placed in a PR of it's own, so here it is... In summary, this PR adds functionality that can be used on it's own and is also the basis of #1019 There are 3 parts to this PR

1. Adds ipconfigMULTICAST_MAC_FILTERING. When this is enabled, NetworkInterface_t gets two new function pointers. Those are used to control which multicast MAC addresses should be received by the EMAC hardware.
2. Adds functions to the SAME70 network driver that allow modification of what multicast MAC addresses are being received by the hardware. This implementation utilizes the 64bit hash match register that is present in the SAME70/V71 microcontrollers. Those changes to the SAME70 driver can serve as a primer for implementing the MAC filtering feature on other platforms. Please read the comments and try to contribute support for other platforms. I can only work on the SAME70 port.
3. Moves the registering of the solicited-node multicast address from the network driver to vIPNetworkUpCalls(). This again applies to the SAME70 driver. Other network drivers need to be updated to NOT register the solicited node MAC address.

More details about bullet point 3 above..... IPv6 end-points that have a link-local or global address must be able to receive the so-called solicited node address. This is a multicast IPv6 address that corresponds to the end-point's own IP address. As far as I understand things, being able to receive this special multicast address allows detection of duplicate addresses. I believe the same duplicate IP detection is also used in DHCPv6 as well. In any case, the solicited-node address can change any time the end-point's IPv6 address changes and because of this we cannot "set and forget" the solicited-node MAC address in the network driver's init function. In this PR, I have added some code to the vIPNetworkUpCalls() Any time an IPv6 end-point goes UP, I calculate the solicited node multicast MAC address and instruct the network driver to begin receiving it. I have not added the complementary code that stops receiving the "old" multicast MAC. This is in my todo list.

If this PR is received well, I'm hoping to base #1019 onto this PR because #1019 depends on this code. Hopefully, we can expand this PR with more network drivers. That would in turn ensure that #1019 also supports as many hardware platforms as possible.

p.s. I'm leaving this a work-in-progress for now and as usual, let me know what you think.

Questions 1: According to the coding guidelines, which one of these is considered better?

#if ( ipconfigMULTICAST_MAC_FILTERING == ipconfigENABLE )

or

#if ( ipconfigMULTICAST_MAC_FILTERING == 1 )

or something else maybe?

Question 2: I have a feeling that hiding the changes of this PR behind ipconfigMULTICAST_MAC_FILTERING is kind of useless. I think eventually, all platforms should support this functionality and I don't thnk ipconfigMULTICAST_MAC_FILTERING should be optional and disabled by default. Multicasts are highly needed especially with IPv6 and it feels like I'm adding a useless defines and bloating the code. Another reason I think we don't need ipconfigMULTICAST_MAC_FILTERING is because even if a platform doesn't support the two functions that this PR adds, NetworkInterface_t is always memset to zero which causes the pointers to the functions to be NULL and every code in this PR and in #1019 checks those pointers before calling them. In essence, nothing bad will happen if the function pointers are just added without an ipconfig define. Thoughts?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[evpopov]

Update: I modified the prvAddAllowedMACAddress() and prvRemoveAllowedMACAddress() functions and added a bunch of notes and documentation. The reason for the extensive comments is in case those functions get used as templates for other ports. I can of course get rid of them if the team thinks they are excessive. I also modified prvGMACInit() to clean up all specific match and hash match registers and then iterate over all end-points of the network interface and call prvAddAllowedMACAddress() for their MAC address. That last change means that we now just have to properly fill-out our end-points and the network driver will handle the MAC addresses whether all end-points use the same MAC or unique MAC addresses.

Let me know what you all think

p.s. @htibosch is this close to how you were envisioning these filtering functions?

[evpopov]

Thanks for the input @htibosch I completely rewrote the comment describing the two filter functions. I tried striking a balance between what you wanted to say and what I wanted to say. I also removed the ipconfigMAC_FILTERING altogether. Just to reiterate, All calls to the new functions are checked for null before the call is made, so adding the function pointers to NetworkInterface_t is safe even for ports that don't implement the filter functions yet. Having a define like ipconfigMAC_FILTERING just adds more clutter and because of this, I believe this PR is better of without it.

I believe this PR is ready for review. I am not able to implement any CI testing and I'd be very thankful if someone can help with that.

[htibosch]

In FreeRTOS_IP.c and in FreeRTOS_IP_Utils.c you are assuming that ipconfigUSE_IPv6 is always enabled, please make it conditional. The variable xAddressType shall only be define when ipconfigUSE_IPv6 is enabled.

FreeRTOS_IP.c

void vIPNetworkUpCalls( struct xNetworkEndPoint * pxEndPoint )
{
-        IPv6_Type_t xAddressType;
+    #if ( ipconfigUSE_IPv6 != 0 )
         if( pxEndPoint->bits.bIPv6 == pdTRUE_UNSIGNED )
         {
             /* Now that the network is up, pxEndPoint->ipv6_settings should hold the actual address of this
              * end-point. For unicast addresses, generate the respective solicited-node multicast address.
              * Note that the check below guards against the loopback address, the unspecified address,
              * and against the weird scenario of someone assigning a multicast address to the end-point. */
-            xAddressType = xIPv6_GetIPType( &( pxEndPoint->ipv6_settings.xIPAddress ) );
+            IPv6_Type_t xAddressType = xIPv6_GetIPType( &( pxEndPoint->ipv6_settings.xIPAddress ) );
             ...
         }
+    #endif /* ( ipconfigUSE_IPv6 != 0 ) */

FreeRTOS_IP_Utils.c

-    IPv6_Type_t xAddressType;
+    #if( ipconfigUSE_IPv6 != 0 )
         if( pxEndPoint->bits.bIPv6 == pdTRUE_UNSIGNED )
         {
-            xAddressType = xIPv6_GetIPType( &( pxEndPoint->ipv6_settings.xIPAddress ) );
+            IPv6_Type_t xAddressType = xIPv6_GetIPType( &( pxEndPoint->ipv6_settings.xIPAddress ) );
             ...
         }
+    #endif /* ( ipconfigUSE_IPv6 != 0 ) */

Unit testing: after the above changes, the UT's will run well. We do have to look at the coverage scores though:

[evpopov]

Got it. I'm currently working on ipv4/ipv6 separation in the multicast PR and just forgot to do it here. Thanks for reminding me. Hopefully my changes make it better. I also added some more comments to the else cases with the hope that it helps with the unit tests.

[htibosch]

@evpopov , here are changes needed to satisfy unit-testing and to reach a coverage of 100%:

Unit-testing_PR_1065.zip

[tony-josi-aws]

Thanks @htibosch for the unit test fix. @evpopov can you help in updating the PR.

[evpopov]

Updated xIndex to uxIndex Moved vManageSolicitedNodeAddress() to FreeRTOS_IPv6_Utils.c I'm ready for the next round of nit-picking.

[HTRamsey]

I am implementing the stm32 network interface multicast stuff as you go, and there are a couple things that can be used across Network Interfaces. I know this is really trivial but can we get the ucIsMulticast = pucMacAddress[ 0 ] & 1U; as a helper somewhere? Making pcLOCAL_ALL_NODES_MULTICAST_MAC accessible was another but you already got that. Maybe providing an implementation of the CRC calculation as well?

[evpopov]

@HTRamsey We sure can. Just to make sure I understand you correctly, do you mean something like this?

#define isMulticastMAC( MacBytes )     ( ( MacBytes[ 0 ] & 1U ) != 0U ) 
#define isUnicastMAC( MacBytes )       ( ( MacBytes[ 0 ] & 1U ) == 0U ) 

If this is what you mean, I think the best place would be FreeRTOS_IP_Common.h or FreeRTOS_IP_Utils.h.... or maybe NetworkInterface.h

Now that you brought that up, I think I will also benefit from such a macro in a future PR that I'm working on. Thoughts anyone?

[evpopov]

....Maybe providing an implementation of the CRC calculation as well?

Are you asking about the protocol checksum calculation or are you referring to the crc function that is used calculating the hash match register bit?

[HTRamsey]

....Maybe providing an implementation of the CRC calculation as well?

Are you asking about the protocol checksum calculation or are you referring to the crc function that is used calculating the hash match register bit?

The hash match. I'm gonna show my naivety here but is there any reason all of the interfaces wouldn't just use CRC32 with 0x04C11DB7 as the poly? Why is DriverSam using CRC16?

[evpopov]

@HTRamsey I'm afraid we can't expect that every EMAC hardware will use the same function. Here's the thing, the hash function is something defined by the EMAC hardware. Initially I coded my own hash function based on the SAME70 datasheet. Later on I switched to the current SAME70 driver and in there I found the prvGenerateCRC16 function. I don't know if it really implements a CRC16 algorithm but it's result matches the hash function described in the SAME70 datasheet. Also if you look at the end of the function you will see that the result is trimmed down to just 6 bits, so if we have to be honest, that function isn't technically doing CRC16. And the reason the output of the function is a 6 bit number is because that result represents a bit number in a 64 bit register and 6 bits hold a value between 0 and 63. I would not expect any other hardware to use the exact same hash function.... Maybe they do, maybe they don't. I hope the above also answers your question why the hardware doesn't use CRC32 function. If you are working on the STM32 driver, can you just look at the datasheet and see what it says about it's EMAC and if it supports hash matching?

@htibosch Do you have any insight on the above?

[htibosch]

@HTRamsey wrote:

The hash match. I'm gonna show my naivety here but is there any reason all of the interfaces wouldn't just use CRC32 with 0x04C11DB7 as the poly? Why is DriverSam using CRC16?

Here is a third one: LPC18xx, which uses a 32-bit CRC but with a different poly (0xEDB88320). STM32 indeed uses:

/* IEEE 802.3 CRC32 polynomial - 0x04C11DB7 */

So I think that the hash/CRC calculations should stay in their networkinterface.c

[HTRamsey]

@htibosch I believe that is the same as the rest, just manually writing the bit reversal. The current stm32h7 interface does it like this instead prvRevBits32( 0x04C11DB7 ). If they are all different in general then yeah I agree leaving it in the network interface is best but if it's like a 99% use the standard 0x04C11DB7 then I figured a helper would be nice.

[evpopov]

I added a MAC unicast/multicast check macro to NetworkInterface.h as requested by @HTRamsey I decided to put it there because NetworkInterface.h is the lowest level I could find. Everything else is IP and up. NetworkInterface.h is also pretty empty,

While I was at it, I added a bunch of macros and defines to the SAME70 driver in order to greatly improve the readability of all hash register manipulation code.

@AniruddhaKanhere and @htibosch let me know if you disagree with my changes to NetworkInterface.h or if you feel they need improvement.

[HTRamsey]

@evpopov One more suggestion, I think probably NetworkInterface_t * pxInterface should be a parameter to prvAddAllowedMACAddress and prvRemoveAllowedMACAddress as well like the other interface functions have. I like the idea of storing an interface's data in a struct in its pvArgument, so having the interface as a parameter could let you retrieve that data there if necessary (like the counters and index and such).

[evpopov]

@HTRamsey, I don't think this is needed. These are static functions inside NetworkInterface.c and they are only exposed to the outside world through pointers inside the NetworkInterface_t struct. Therefore, if you somehow have a pointer to the network interface, you have pointers to those functions. Hopefully I understood you correctly. If not, please give me some more details.

[HTRamsey]

@evpopov It's just for the purpose of being able to access pvArgument inside of the NetworkInterface struct. Take a look at the Zynq NetworkInterface and you will see why this is necessary. It has multiple instances of the same interface, and stores the index in pvArgument (Although it probably should have been implemented with a struct pointer stored in pvArgument instead, especially if we assume there can be multiple types of interfaces, which would require an index per type of interface rather than a general one). So to get the particular counters, register, etc for that particular interface instance you need access to pvArgument from the interface struct.

[htibosch]

@HTRamsey wrote:

Take a look at the Zynq NetworkInterface and you will see why this is necessary.

I agree that it is useful to pass a pointer to NetworkInterface_t to the MAC filtering functions.

typedef void ( * NetworkInterfaceMACFilterFunction_t )
(
    NetworkBufferDescriptor_t * const pxNetworkBuffer,
    const uint8_t * pucMacAddressBytes
);

Some devices like Xilinx have multiple independent EMAC's, each with their own MAC filter rules.

[evpopov]

@HTRamsey I see. I must have been tired last night but now I understand exactly what you mean. @htibosch I think you mean to say:

typedef void ( * NetworkInterfaceMACFilterFunction_t ) ( struct xNetworkInterface * pxDescriptor,
                                                         const uint8_t * pucMacAddressBytes );

but I get the idea. @HTRamsey I agree with calling the parameter pxInterface, but for consistency with the other functions, I will stick to pxDescriptor.

Gentlemen, since this PR adds functions to the TCP stack's API, I believe it is of critical importance to get things right the first time. If we don't fix these details now we will have to deal with them as backwards compatibility when we fix them later. I greatly appreciate this type of input.

[evpopov]

@htibosch I hate to bother you, but when you have a moment, would you please take a look at the unit tests. I updated the function definitions and declarations, but I have no idea what I'm doing. Thanks in advance!

[HTRamsey]

@evpopov you check for overflow when incrementing the hash counters but not the match counters.

[evpopov]

@HTRamsey Nice find! Thanks!

[amazonKamath]

@evpopov and everyone active on this PR, we are just wrapping up a round of testing on the main HEAD and will be pushing out a v4.1.0 either this week or early next week. Till then we will close the merge window. We are excluding this PR out of v4.1.0 and will club it potentially in a next release together with your multicast PR #1019 and some other contributions and features/bug fixes. Thank you.

[evpopov]

Did you check the changes when using a network interface that leaves pfAddAllowedMAC and prvRemoveAllowedMACAddress to NULL?

This PR only calls pfAddAllowedMAC() and pfRemoveAllowedMAC() from within vManageSolicitedNodeAddress() and both pointers get checked for NULL before making the call.

1019 makes a lot more use of these functions and there as well, the pointers are checked before every call.

Let me know if you found a place where I missed the check.

[evpopov]

@tony-josi-aws sorry it took me two weeks to address your latest comments. I have been busy with other tasks lately. Let me know if there is anything else that I can change to improve this PR.

[evpopov]

/bot run formatting