genet: wire up GenetSimpleNetworkReceiveFilters

[andreiw]

Today this function does nothing (it is not allowed to fail at least within MnpDxe, so if it fails, there's no networking). We have promisc mode being enabled in Initialize() callback instead, and this needs to be moved into GenetSimpleNetworkReceiveFilters even if we don't bother supporting multicast.

(having promisc always on in a busy network just taxes the L2 stack)

[ardbiesheuvel]

so why do we need to enable this in the first place?

[andreiw]

it's part of the initialization for the network interface - it can't fail (MnpStart->MnpConfigReceiveFilters->Snp->ReceiveFilters or NetLibDetectMedia->Snp->ReceiveFilters).

The way it's documented we could treat everything not unicast as being "upgraded" to promisc, but that toggling of promisc state should happen in GenetSimpleNetworkReceiveFilters, not in the Initialize() Snp callback. I.e. we don't need to go all OCD on this and add multicast filters.

[ardbiesheuvel]

Fair enough. But we can just ignore multicast for the time being, no? MNP will check MaxMCastFilterCount, and only set the EFI_SIMPLE_NETWORK_RECEIVE_UNICAST, EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST or EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS bits, and we only care about the last one.

[ardbiesheuvel]

Something like the below perhaps?

diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c
index e3d015dd0820..a6102421cc26 100644
--- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c
+++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/DriverBinding.c
@@ -154,11 +154,10 @@ GenetDriverBindingStart (
   Genet->SnpMode.NvRamSize              = 0;
   Genet->SnpMode.NvRamAccessSize        = 0;
   Genet->SnpMode.ReceiveFilterMask      = EFI_SIMPLE_NETWORK_RECEIVE_UNICAST |
-                                          EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST |
                                           EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST |
-                                          EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS |
-                                          EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST;
-  Genet->SnpMode.ReceiveFilterSetting   = Genet->SnpMode.ReceiveFilterMask;
+                                          EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS;
+  Genet->SnpMode.ReceiveFilterSetting   = EFI_SIMPLE_NETWORK_RECEIVE_UNICAST |
+                                          EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST;
   Genet->SnpMode.MaxMCastFilterCount    = 0;
   Genet->SnpMode.MCastFilterCount       = 0;
   Genet->SnpMode.IfType                 = NET_IFTYPE_ETHERNET;
diff --git a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c
index bf28448445d1..a99edeff625d 100644
--- a/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c
+++ b/Silicon/Broadcom/Drivers/Net/BcmGenetDxe/SimpleNetwork.c
@@ -148,10 +148,6 @@ GenetSimpleNetworkInitialize (
   }
 
   GenetSetMacAddress (Genet, &Genet->SnpMode.CurrentAddress);
-  /*
-   * TODO: this belongs in GenetSimpleNetworkReceiveFilters, not here.
-   */
-  GenetSetPromisc (Genet, TRUE);
 
   GenetDmaInitRings (Genet);
 
@@ -313,13 +309,7 @@ GenetSimpleNetworkReceiveFilters (
     return EFI_DEVICE_ERROR;
   }
 
-  //
-  // Can't actually return EFI_UNSUPPORTED, so just ignore the filters
-  // (we set promiscuous mode ON inside GenetSimpleNetworkInitialize).
-  //
-  // TODO: move promisc handling here.
-  // TODO 2: support multicast/broadcast.
-  //
+  GenetSetPromisc (Genet, Enable & EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS);
 
   return EFI_SUCCESS;
 }

[andreiw]

Yes, I think we can "promote" multicast/broadcast to promiscuous, though.

The UEFI spec says this:

Note: Some network drivers and/or devices will automatically promote receive filter settings if the requested setting can not be honored. For example, if a request for four multicast addresses is made and the underlying hardware only supports two multicast addresses the driver might set the promiscuous or promiscuous multicast receive filters instead. The receiving software is responsible for discarding any extra packets that get through the hardware receive filters.

Also before any of that, "Enable" needs to be sanitized.

Spec says:

The receive filter change is broken down into three steps: • The filter mask bits that are set (ON) in the Enable parameter are added to the current receive filter settings. • The filter mask bits that are set (ON) in the Disable parameter are subtracted from the updated receive filter settings. • If the resulting receive filter setting is not supported by the hardware a more liberal setting is selected. If the same bits are set in the Enable and Disable parameters, then the bits in the Disable parameter takes precedence.

[ardbiesheuvel]

OK, so this should do the trick then.

  GenetSetPromisc (Genet,
    Enable & ~Disable & EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS);

If we expose the SNP mode with MaxMCastFilterCount == 0, I don't think we need to care about or sanitize the other arguments, except maybe for an ASSERT() in DEBUG builds that multicast is not being enabled.

[andreiw]

Can we promote “broadcast” to “promisc” as well?