[PATCH] efinet: add efinet_multicast_filter command

[PATCH] efinet: add efinet_multicast_filter command

[Josef Bacik]

We have some hardware that doesn't honor
EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST properly so we aren't getting
RA's that are multicasted properly (our switches respond to solicitations with a
multicast rather than a unicast).  I don't want to add this filtering by
default, so add a new command to allow a user to specify a multicast receive
filter.  We use it like this

efinet_multicast_filter efinet0 33:33:0:0:0:1

to get ipv6 multicasts which allows us to receive the router advertisements.
Thanks,

Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 grub-core/kern/efi/efi.c           | 12 ++++-----
 grub-core/net/drivers/efi/efinet.c | 51 ++++++++++++++++++++++++++++++++++++++
 grub-core/net/net.c                | 43 ++++++++++++++++++++++++++++++++
 include/grub/efi/api.h             |  5 +++-
 include/grub/net.h                 |  3 +++
 5 files changed, 107 insertions(+), 7 deletions(-)

diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 2e77834..efc3d33 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -652,12 +652,12 @@ grub_efi_print_device_path (grub_efi_device_path_t *dp)
 		grub_efi_mac_address_device_path_t *mac
 		  = (grub_efi_mac_address_device_path_t *) dp;
 		grub_printf ("/MacAddr(%02x:%02x:%02x:%02x:%02x:%02x,%x)",
-			     (unsigned) mac->mac_address[0],
-			     (unsigned) mac->mac_address[1],
-			     (unsigned) mac->mac_address[2],
-			     (unsigned) mac->mac_address[3],
-			     (unsigned) mac->mac_address[4],
-			     (unsigned) mac->mac_address[5],
+			     (unsigned) mac->mac_address.addr[0],
+			     (unsigned) mac->mac_address.addr[1],
+			     (unsigned) mac->mac_address.addr[2],
+			     (unsigned) mac->mac_address.addr[3],
+			     (unsigned) mac->mac_address.addr[4],
+			     (unsigned) mac->mac_address.addr[5],
 			     (unsigned) mac->if_type);
 	      }
 	      break;
diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
index c8f80a1..8c2c4f8 100644
--- a/grub-core/net/drivers/efi/efinet.c
+++ b/grub-core/net/drivers/efi/efinet.c
@@ -23,6 +23,7 @@
 #include <grub/efi/api.h>
 #include <grub/efi/efi.h>
 #include <grub/i18n.h>
+#include <grub/command.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -451,9 +452,58 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char **device,
   }
 }
 
+static grub_err_t
+grub_cmd_multicast_filter (struct grub_command *cmd __attribute__ ((unused)),
+			   int argc, char **args)
+{
+  struct grub_net_card *card;
+  grub_efi_simple_network_t *net;
+  grub_net_link_level_address_t hwaddr;
+  grub_efi_mac_address_t filter_mac[1];
+  grub_efi_status_t st;
+
+  if (argc != 2)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
+
+  FOR_NET_CARDS (card)
+    if (grub_strcmp (card->name, args[0]) == 0)
+      break;
+
+  if (card == NULL)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("card not found"));
+  if (card->driver != &efidriver)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("card not an efi card"));
+
+  if (grub_net_str_to_hwaddr (args[1], &hwaddr) == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("couldn't parse hw address"));
+  grub_memset (filter_mac, 0, sizeof(filter_mac));
+  grub_memcpy (filter_mac, hwaddr.mac, 6);
+
+  net = card->efi_net;
+  if (!(net->mode->receive_filter_mask &
+	GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST))
+    return grub_error (GRUB_ERR_IO,
+		       N_("device doesn't support multicast filtering"));
+
+  st = efi_call_6 (net->receive_filters, net,
+		   GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST, 0, 0, 1,
+		   filter_mac);
+  if (st != GRUB_EFI_SUCCESS)
+    return grub_error (GRUB_ERR_IO,
+		       N_("could not set multicast filter address"));
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_command_t cmd_multicast_filter;
+
 GRUB_MOD_INIT(efinet)
 {
   grub_efinet_findcards ();
+  cmd_multicast_filter = grub_register_command ("efinet_multicast_filter",
+						grub_cmd_multicast_filter,
+						N_("CARD HWADDRESS"),
+						N_("Add a multicast filter"));
   grub_efi_net_config = grub_efi_net_config_real;
 }
 
@@ -464,5 +514,6 @@ GRUB_MOD_FINI(efinet)
   FOR_NET_CARDS_SAFE (card, next) 
     if (card->driver == &efidriver)
       grub_net_card_unregister (card);
+  grub_unregister_command(cmd_multicast_filter);
 }
 
diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 65bea28..3a69f63 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -480,6 +480,49 @@ parse_ip6 (const char *val, grub_uint64_t *ip, const char **rest)
   return 1;
 }
 
+int
+grub_net_str_to_hwaddr (const char *val, grub_net_link_level_address_t *hwaddr)
+{
+  grub_uint8_t newmac[6];
+  const char *ptr = val;
+  int word;
+
+  if (ptr[0] == ':' && ptr[1] != ':')
+    return 0;
+  if (ptr[0] == ':')
+    ptr++;
+
+  for (word = 0; word < 6; word++)
+    {
+      unsigned long t;
+      if (*ptr == ':')
+	{
+	  word--;
+	  ptr++;
+	  continue;
+	}
+      t = grub_strtoul (ptr, (char **) &ptr, 16);
+      if (grub_errno)
+	{
+	  grub_errno = GRUB_ERR_NONE;
+	  break;
+	}
+      if (t & ~0xff)
+	return 0;
+      newmac[word] = t;
+      if (*ptr != ':')
+	break;
+      ptr++;
+    }
+  if (*ptr != 0)
+	  return grub_error (GRUB_ERR_NET_BAD_ADDRESS,
+			     N_("unrecognized link level address '%s'"),
+			     val);
+  hwaddr->type = GRUB_NET_LINK_LEVEL_PROTOCOL_ETHERNET;
+  grub_memcpy (hwaddr->mac, newmac, 6);
+  return 1;
+}
+
 static int
 match_net (const grub_net_network_level_netaddress_t *net,
 	   const grub_net_network_level_address_t *addr)
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index 9a73976..48940be 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -523,7 +523,10 @@ typedef void *grub_efi_handle_t;
 typedef void *grub_efi_event_t;
 typedef grub_efi_uint64_t grub_efi_lba_t;
 typedef grub_efi_uintn_t grub_efi_tpl_t;
-typedef grub_uint8_t grub_efi_mac_address_t[32];
+typedef struct {
+	grub_uint8_t addr[32];
+} grub_efi_mac_address_t;
+
 typedef grub_uint8_t grub_efi_ipv4_address_t[4];
 typedef grub_uint16_t grub_efi_ipv6_address_t[8];
 typedef grub_uint8_t grub_efi_ip_address_t[8] __attribute__ ((aligned(4)));
diff --git a/include/grub/net.h b/include/grub/net.h
index a1ff519..12cc1db 100644
--- a/include/grub/net.h
+++ b/include/grub/net.h
@@ -522,6 +522,9 @@ grub_net_addr_to_str (const grub_net_network_level_address_t *target,
 		      char *buf);
 void
 grub_net_hwaddr_to_str (const grub_net_link_level_address_t *addr, char *str);
+int
+grub_net_str_to_hwaddr (const char *val,
+			grub_net_link_level_address_t *hwaddr);
 
 grub_err_t
 grub_env_set_net_property (const char *intername, const char *suffix,
-- 
1.8.1

Re: [PATCH] efinet: add efinet_multicast_filter command

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 8439 bytes --]

I don't have EFI spec under my hand now. Can we get away with making it a
default or at least for the case when no interface overrides mac address.
Extra config to workaround firmware bugs is usually harmful
Le 5 nov. 2015 9:17 PM, "Josef Bacik" <jbacik@fb.com> a écrit :

> We have some hardware that doesn't honor
> EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST properly so we aren't
> getting
> RA's that are multicasted properly (our switches respond to solicitations
> with a
> multicast rather than a unicast).  I don't want to add this filtering by
> default, so add a new command to allow a user to specify a multicast
> receive
> filter.  We use it like this
>
> efinet_multicast_filter efinet0 33:33:0:0:0:1
>
> to get ipv6 multicasts which allows us to receive the router
> advertisements.
> Thanks,
>
> Signed-off-by: Josef Bacik <jbacik@fb.com>
> ---
>  grub-core/kern/efi/efi.c           | 12 ++++-----
>  grub-core/net/drivers/efi/efinet.c | 51
> ++++++++++++++++++++++++++++++++++++++
>  grub-core/net/net.c                | 43 ++++++++++++++++++++++++++++++++
>  include/grub/efi/api.h             |  5 +++-
>  include/grub/net.h                 |  3 +++
>  5 files changed, 107 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index 2e77834..efc3d33 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -652,12 +652,12 @@ grub_efi_print_device_path (grub_efi_device_path_t
> *dp)
>                 grub_efi_mac_address_device_path_t *mac
>                   = (grub_efi_mac_address_device_path_t *) dp;
>                 grub_printf ("/MacAddr(%02x:%02x:%02x:%02x:%02x:%02x,%x)",
> -                            (unsigned) mac->mac_address[0],
> -                            (unsigned) mac->mac_address[1],
> -                            (unsigned) mac->mac_address[2],
> -                            (unsigned) mac->mac_address[3],
> -                            (unsigned) mac->mac_address[4],
> -                            (unsigned) mac->mac_address[5],
> +                            (unsigned) mac->mac_address.addr[0],
> +                            (unsigned) mac->mac_address.addr[1],
> +                            (unsigned) mac->mac_address.addr[2],
> +                            (unsigned) mac->mac_address.addr[3],
> +                            (unsigned) mac->mac_address.addr[4],
> +                            (unsigned) mac->mac_address.addr[5],
>                              (unsigned) mac->if_type);
>               }
>               break;
> diff --git a/grub-core/net/drivers/efi/efinet.c
> b/grub-core/net/drivers/efi/efinet.c
> index c8f80a1..8c2c4f8 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -23,6 +23,7 @@
>  #include <grub/efi/api.h>
>  #include <grub/efi/efi.h>
>  #include <grub/i18n.h>
> +#include <grub/command.h>
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -451,9 +452,58 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char
> **device,
>    }
>  }
>
> +static grub_err_t
> +grub_cmd_multicast_filter (struct grub_command *cmd __attribute__
> ((unused)),
> +                          int argc, char **args)
> +{
> +  struct grub_net_card *card;
> +  grub_efi_simple_network_t *net;
> +  grub_net_link_level_address_t hwaddr;
> +  grub_efi_mac_address_t filter_mac[1];
> +  grub_efi_status_t st;
> +
> +  if (argc != 2)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments
> expected"));
> +
> +  FOR_NET_CARDS (card)
> +    if (grub_strcmp (card->name, args[0]) == 0)
> +      break;
> +
> +  if (card == NULL)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("card not found"));
> +  if (card->driver != &efidriver)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("card not an efi card"));
> +
> +  if (grub_net_str_to_hwaddr (args[1], &hwaddr) == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("couldn't parse hw
> address"));
> +  grub_memset (filter_mac, 0, sizeof(filter_mac));
> +  grub_memcpy (filter_mac, hwaddr.mac, 6);
> +
> +  net = card->efi_net;
> +  if (!(net->mode->receive_filter_mask &
> +       GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST))
> +    return grub_error (GRUB_ERR_IO,
> +                      N_("device doesn't support multicast filtering"));
> +
> +  st = efi_call_6 (net->receive_filters, net,
> +                  GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST, 0, 0, 1,
> +                  filter_mac);
> +  if (st != GRUB_EFI_SUCCESS)
> +    return grub_error (GRUB_ERR_IO,
> +                      N_("could not set multicast filter address"));
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_command_t cmd_multicast_filter;
> +
>  GRUB_MOD_INIT(efinet)
>  {
>    grub_efinet_findcards ();
> +  cmd_multicast_filter = grub_register_command ("efinet_multicast_filter",
> +                                               grub_cmd_multicast_filter,
> +                                               N_("CARD HWADDRESS"),
> +                                               N_("Add a multicast
> filter"));
>    grub_efi_net_config = grub_efi_net_config_real;
>  }
>
> @@ -464,5 +514,6 @@ GRUB_MOD_FINI(efinet)
>    FOR_NET_CARDS_SAFE (card, next)
>      if (card->driver == &efidriver)
>        grub_net_card_unregister (card);
> +  grub_unregister_command(cmd_multicast_filter);
>  }
>
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 65bea28..3a69f63 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -480,6 +480,49 @@ parse_ip6 (const char *val, grub_uint64_t *ip, const
> char **rest)
>    return 1;
>  }
>
> +int
> +grub_net_str_to_hwaddr (const char *val, grub_net_link_level_address_t
> *hwaddr)
> +{
> +  grub_uint8_t newmac[6];
> +  const char *ptr = val;
> +  int word;
> +
> +  if (ptr[0] == ':' && ptr[1] != ':')
> +    return 0;
> +  if (ptr[0] == ':')
> +    ptr++;
> +
> +  for (word = 0; word < 6; word++)
> +    {
> +      unsigned long t;
> +      if (*ptr == ':')
> +       {
> +         word--;
> +         ptr++;
> +         continue;
> +       }
> +      t = grub_strtoul (ptr, (char **) &ptr, 16);
> +      if (grub_errno)
> +       {
> +         grub_errno = GRUB_ERR_NONE;
> +         break;
> +       }
> +      if (t & ~0xff)
> +       return 0;
> +      newmac[word] = t;
> +      if (*ptr != ':')
> +       break;
> +      ptr++;
> +    }
> +  if (*ptr != 0)
> +         return grub_error (GRUB_ERR_NET_BAD_ADDRESS,
> +                            N_("unrecognized link level address '%s'"),
> +                            val);
> +  hwaddr->type = GRUB_NET_LINK_LEVEL_PROTOCOL_ETHERNET;
> +  grub_memcpy (hwaddr->mac, newmac, 6);
> +  return 1;
> +}
> +
>  static int
>  match_net (const grub_net_network_level_netaddress_t *net,
>            const grub_net_network_level_address_t *addr)
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index 9a73976..48940be 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -523,7 +523,10 @@ typedef void *grub_efi_handle_t;
>  typedef void *grub_efi_event_t;
>  typedef grub_efi_uint64_t grub_efi_lba_t;
>  typedef grub_efi_uintn_t grub_efi_tpl_t;
> -typedef grub_uint8_t grub_efi_mac_address_t[32];
> +typedef struct {
> +       grub_uint8_t addr[32];
> +} grub_efi_mac_address_t;
> +
>  typedef grub_uint8_t grub_efi_ipv4_address_t[4];
>  typedef grub_uint16_t grub_efi_ipv6_address_t[8];
>  typedef grub_uint8_t grub_efi_ip_address_t[8] __attribute__
> ((aligned(4)));
> diff --git a/include/grub/net.h b/include/grub/net.h
> index a1ff519..12cc1db 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -522,6 +522,9 @@ grub_net_addr_to_str (const
> grub_net_network_level_address_t *target,
>                       char *buf);
>  void
>  grub_net_hwaddr_to_str (const grub_net_link_level_address_t *addr, char
> *str);
> +int
> +grub_net_str_to_hwaddr (const char *val,
> +                       grub_net_link_level_address_t *hwaddr);
>
>  grub_err_t
>  grub_env_set_net_property (const char *intername, const char *suffix,
> --
> 1.8.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 10183 bytes --]

Re: [PATCH] efinet: add efinet_multicast_filter command

[Josef Bacik]

On 11/05/2015 03:28 PM, Vladimir 'phcoder' Serbinenko wrote:
> I don't have EFI spec under my hand now. Can we get away with making it
> a default or at least for the case when no interface overrides mac
> address. Extra config to workaround firmware bugs is usually harmful
>

http://wiki.phoenix.com/wiki/index.php/EFI_SIMPLE_NETWORK_PROTOCOL

That is what I've been using.  The thing I worry about is that this is 
just the multicast address for ipv6.  There's a wikipedia page that 
lists about 15 addresses for different things.  Now how many do we care 
about for grub?  I have no idea.  I _think_ that we only really care 
about multicast traffic for ipv6 router advertisements, I can't think of 
when else we'd want to use it.  So in that sense I think it would be ok 
to do this.  But I have no way of knowing what would break in current 
working configurations which is why I went with the config option.  I 
can test with the variety of hardware I have here and see how it does 
with it set by default.  It's up to you, I'm good either way.  Thanks,

Josef

Re: [PATCH] efinet: add efinet_multicast_filter command

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1540 bytes --]

If we can't figure out which ones we need to filter out, then it's
unrealistic to expect our users to be able to configure. We need to go with
multicast filter per default and switch to promiscuous only when we
actually need it: when we override mac address. You can add function to go
to promiscuous mode to network interface structure
On 11/05/2015 03:28 PM, Vladimir 'phcoder' Serbinenko wrote:

> I don't have EFI spec under my hand now. Can we get away with making it
> a default or at least for the case when no interface overrides mac
> address. Extra config to workaround firmware bugs is usually harmful
>
>
http://wiki.phoenix.com/wiki/index.php/EFI_SIMPLE_NETWORK_PROTOCOL

That is what I've been using.  The thing I worry about is that this is just
the multicast address for ipv6.  There's a wikipedia page that lists about
15 addresses for different things.  Now how many do we care about for
grub?  I have no idea.  I _think_ that we only really care about multicast
traffic for ipv6 router advertisements, I can't think of when else we'd
want to use it.  So in that sense I think it would be ok to do this.  But I
have no way of knowing what would break in current working configurations
which is why I went with the config option.  I can test with the variety of
hardware I have here and see how it does with it set by default.  It's up
to you, I'm good either way.  Thanks,

Josef

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 2192 bytes --]

Re: [PATCH] efinet: add efinet_multicast_filter command

[Andrei Borzenkov]

06.11.2015 02:47, Vladimir 'phcoder' Serbinenko пишет:
> If we can't figure out which ones we need to filter out, then it's
> unrealistic to expect our users to be able to configure. We need to go with
> multicast filter per default and switch to promiscuous only when we
> actually need it: when we override mac address. You can add function to go
> to promiscuous mode to network interface structure

I'm afraid if we cannot trust firmware the only solution is to simply 
use promiscuous mode. This is not as bad as it sounds, as with moder 
switches we won't get significantly more traffic anyway unless someone 
floods network with video broadcast, in which case grub is probably not 
the right place to fix it.

Also firmware is free to implement filter using promiscuous mode, so it 
may well be what we end up doing anyway.

And i386 did it all the time. I do not know how wide testing it 
received, but nobody complained about problems due to promiscuous.

Re: [PATCH] efinet: add efinet_multicast_filter command

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

Le 6 nov. 2015 7:43 PM, "Andrei Borzenkov" <arvidjaar@gmail.com> a écrit :
>
> 06.11.2015 02:47, Vladimir 'phcoder' Serbinenko пишет:
>
>> If we can't figure out which ones we need to filter out, then it's
>> unrealistic to expect our users to be able to configure. We need to go
with
>> multicast filter per default and switch to promiscuous only when we
>> actually need it: when we override mac address. You can add function to
go
>> to promiscuous mode to network interface structure
>
>
> I'm afraid if we cannot trust firmware the only solution is to simply use
promiscuous mode. This is not as bad as it sounds, as with moder switches
we won't get significantly more traffic anyway unless someone floods
network with video broadcast, in which case grub is probably not the right
place to fix it.
>
> Also firmware is free to implement filter using promiscuous mode, so it
may well be what we end up doing anyway.
>
> And i386 did it all the time. I do not know how wide testing it received,
but nobody complained about problems due to promiscuous.
>
Josef's firmware apparently doesn't work with promiscuous. Or do you mean
some kind of hybrid mode when we set both promiscuous and filters?
>
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: Type: text/html, Size: 1788 bytes --]

Re: [PATCH] efinet: add efinet_multicast_filter command

[Andrei Borzenkov]

06.11.2015 21:47, Vladimir 'phcoder' Serbinenko пишет:
> Le 6 nov. 2015 7:43 PM, "Andrei Borzenkov" <arvidjaar@gmail.com> a écrit :
>>
>> 06.11.2015 02:47, Vladimir 'phcoder' Serbinenko пишет:
>>
>>> If we can't figure out which ones we need to filter out, then it's
>>> unrealistic to expect our users to be able to configure. We need to go
> with
>>> multicast filter per default and switch to promiscuous only when we
>>> actually need it: when we override mac address. You can add function to
> go
>>> to promiscuous mode to network interface structure
>>
>>
>> I'm afraid if we cannot trust firmware the only solution is to simply use
> promiscuous mode. This is not as bad as it sounds, as with moder switches
> we won't get significantly more traffic anyway unless someone floods
> network with video broadcast, in which case grub is probably not the right
> place to fix it.
>>
>> Also firmware is free to implement filter using promiscuous mode, so it
> may well be what we end up doing anyway.
>>
>> And i386 did it all the time. I do not know how wide testing it received,
> but nobody complained about problems due to promiscuous.
>>
> Josef's firmware apparently doesn't work with promiscuous. Or do you mean
> some kind of hybrid mode when we set both promiscuous and filters?

I mean full promiscuous, not multicast promiscuous - 
EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS.

Re: [PATCH] efinet: add efinet_multicast_filter command

[Josef Bacik]

On 11/05/2015 03:28 PM, Vladimir 'phcoder' Serbinenko wrote:
> I don't have EFI spec under my hand now. Can we get away with making it
> a default or at least for the case when no interface overrides mac
> address. Extra config to workaround firmware bugs is usually harmful
>

So I made us start setting the mac filter at open time and the problem 
still happened.  I"ve been running down what is going on since then and 
it seems there is a timing window between setting the receive filters 
and the card actually honoring the settings.  I've now gotten to the 
point that it works if I set the receive filters and then reset the 
interface and then set the mac filter.  This is horrible, and I may be 
able to get it to work less horribly but it feels more and more like my 
option is the least painful way.  I'll get it working as cleanly as 
possible and send that patch out and you can decide which approach you 
hate the least.  Thanks,

Josef

Re: [PATCH] efinet: add efinet_multicast_filter command

[Andrei Borzenkov]

05.11.2015 22:23, Josef Bacik пишет:
> We have some hardware that doesn't honor
> EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST properly so we aren't getting

You mean that driver advertises promiscuous multicast support but does 
not implement it? Can you add debugging to efi_call_6 
(net->receive_filters, net, filters, 0, 0, 0, NULL); whether it fails. 
May be need set each one separately and fall back to promiscuous.

> RA's that are multicasted properly (our switches respond to solicitations with a
> multicast rather than a unicast).

Could you send packet trace?

>  I don't want to add this filtering by
> default, so add a new command to allow a user to specify a multicast receive
> filter.  We use it like this
>

I do not think we need any IPv4 multicasts; for IPv6 we need all nodes 
and solicited address.

But I would like to understand first whether receive_filters fail and we 
ignore it or it succeeds.

Re: [PATCH] efinet: add efinet_multicast_filter command

[Josef Bacik]

On 11/05/2015 11:15 PM, Andrei Borzenkov wrote:
> 05.11.2015 22:23, Josef Bacik пишет:
>> We have some hardware that doesn't honor
>> EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST properly so we aren't
>> getting
>
> You mean that driver advertises promiscuous multicast support but does
> not implement it? Can you add debugging to efi_call_6
> (net->receive_filters, net, filters, 0, 0, 0, NULL); whether it fails.
> May be need set each one separately and fall back to promiscuous.
>

I spent a week debugging this before I declared it a firmware bug.  The 
receive_filters succeeds when setting it to promiscuous, and 
net->mode->receive_filter_settings has all the appropriate bits set. 
It's all done right, the firmware just isn't working.

I'll also note we noticed this with ipxe first (we haven't finished our 
grub2 rollout yet) and we had to do a similar approach there.

>> RA's that are multicasted properly (our switches respond to
>> solicitations with a
>> multicast rather than a unicast).
>
> Could you send packet trace?
>

Can't sorry, this is in our secure network.  But I can tell you we see 
the solicit from grub properly, and then the switches respond with a 
multicast RA and grub doesn't do anything with it.  These are from our 
FBOSS switches, we are fixing them to do unicast responses to 
solicitations as well because of this but a multicast response is also 
allowed.

>>  I don't want to add this filtering by
>> default, so add a new command to allow a user to specify a multicast
>> receive
>> filter.  We use it like this
>>
>
> I do not think we need any IPv4 multicasts; for IPv6 we need all nodes
> and solicited address.
>
> But I would like to understand first whether receive_filters fail and we
> ignore it or it succeeds.
>

I agree, I'll do what Vladimir is suggesting.  However I'm going to 
leave an option to use promiscuous.  We are going to use that to test 
new hardware coming in so we stop buying crap that doesn't behave 
according to spec.  Thanks,

Josef