On Fri, Nov 13, 2015 at 1:07 AM, Josef Bacik <jba...@fb.com> wrote:
> The firmware bug I've been tracking down has been too extensive to work around
> effectively. Basically once we switch to EXCLUSIVE everything is completely
> horked. I can add a multicast filter but it's timing sensitive, so it has to
> be
> done at least 10 seconds after initialization for it to take place, and we
> have
> to continue to enable PROMISCUOUS_MULTICAST because otherwise we no longer get
> unicast traffic. I discovered however that if we do not make the EXCLUSIVE
> switch over everything works fine. So instead add a function that checks for
Does your firmware use MNP at all?
> this broken firmware and uses GET_PROTOCOL instead of EXCLUSIVE. This also
> keeps the original protocol we use when scanning the cards and leaves the
> initialization stuff for ->open. Thanks,
>
> Signed-off-by: Josef Bacik <jba...@fb.com>
> ---
> grub-core/net/drivers/efi/efinet.c | 99
> ++++++++++++++++++--------------------
> 1 file changed, 46 insertions(+), 53 deletions(-)
>
> diff --git a/grub-core/net/drivers/efi/efinet.c
> b/grub-core/net/drivers/efi/efinet.c
> index c8f80a1..5a207fd 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -156,59 +156,40 @@ get_card_packet (struct grub_net_card *dev)
> static grub_err_t
> open_card (struct grub_net_card *dev)
> {
> - grub_efi_simple_network_t *net;
> + grub_efi_simple_network_t *net = dev->efi_net;
>
> - /* Try to reopen SNP exlusively to close any active MNP protocol instance
> - that may compete for packet polling
> - */
> - net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid,
> - GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE);
> - if (net)
> + if (net->mode->state == GRUB_EFI_NETWORK_STOPPED)
> + return grub_error (GRUB_ERR_NET_NO_CARD, "%s: card stopped",
> + dev->name);
> +
> + if (net->mode->state == GRUB_EFI_NETWORK_STARTED
> + && efi_call_3 (net->initialize, net, 0, 0) != GRUB_EFI_SUCCESS)
> + return grub_error (GRUB_ERR_NET_NO_CARD, "%s: net initialize
> failed",
> + dev->name);
> +
> + /* Enable hardware receive filters if driver declares support for it.
> + We need unicast and broadcast and additionaly all nodes and
> + solicited multicast for IPv6. Solicited multicast is per-IPv6
> + address and we currently do not have API to do it so simply
> + try to enable receive of all multicast packets or evertyhing in
> + the worst case (i386 PXE driver always enables promiscuous too).
> +
> + This does trust firmware to do what it claims to do.
> + */
> + if (net->mode->receive_filter_mask)
> {
> - if (net->mode->state == GRUB_EFI_NETWORK_STOPPED
> - && efi_call_1 (net->start, net) != GRUB_EFI_SUCCESS)
> - return grub_error (GRUB_ERR_NET_NO_CARD, "%s: net start failed",
> - dev->name);
> -
> - if (net->mode->state == GRUB_EFI_NETWORK_STOPPED)
> - return grub_error (GRUB_ERR_NET_NO_CARD, "%s: card stopped",
> - dev->name);
> -
> - if (net->mode->state == GRUB_EFI_NETWORK_STARTED
> - && efi_call_3 (net->initialize, net, 0, 0) != GRUB_EFI_SUCCESS)
> - return grub_error (GRUB_ERR_NET_NO_CARD, "%s: net initialize failed",
> - dev->name);
> -
> - /* Enable hardware receive filters if driver declares support for it.
> - We need unicast and broadcast and additionaly all nodes and
> - solicited multicast for IPv6. Solicited multicast is per-IPv6
> - address and we currently do not have API to do it so simply
> - try to enable receive of all multicast packets or evertyhing in
> - the worst case (i386 PXE driver always enables promiscuous too).
> -
> - This does trust firmware to do what it claims to do.
> - */
> - if (net->mode->receive_filter_mask)
> - {
> - grub_uint32_t filters = GRUB_EFI_SIMPLE_NETWORK_RECEIVE_UNICAST |
> - GRUB_EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST |
> -
> GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST;
> -
> - filters &= net->mode->receive_filter_mask;
> - if (!(filters &
> GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST))
> - filters |= (net->mode->receive_filter_mask &
> - GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS);
> + grub_uint32_t filters = GRUB_EFI_SIMPLE_NETWORK_RECEIVE_UNICAST |
> + GRUB_EFI_SIMPLE_NETWORK_RECEIVE_BROADCAST |
> + GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST;
>
> - efi_call_6 (net->receive_filters, net, filters, 0, 0, 0, NULL);
> - }
> + filters &= net->mode->receive_filter_mask;
> + if (!(filters & GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST))
> + filters |= (net->mode->receive_filter_mask &
> + GRUB_EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS);
>
> - efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
> - dev->efi_net, &net_io_guid,
> - grub_efi_image_handle, dev->efi_handle);
> - dev->efi_net = net;
> + efi_call_6 (net->receive_filters, net, filters, 0, 0, 0, NULL);
> }
>
> - /* If it failed we just try to run as best as we can */
> return GRUB_ERR_NONE;
> }
>
> @@ -278,12 +259,26 @@ grub_efinet_is_mac_device (struct grub_net_card *card)
> return 0;
> }
>
> +static grub_efi_uint32_t
> +grub_snp_attributes (void)
Attributes? I'd say
use_snp ? GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE :
GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL
looks more readable.
> +{
> + /* Some firmware will completely screw up the transition to exclusive SNP,
> + doing things like not honoring receive filters or taking multiple
> seconds
> + to make the switch over. So don't bother using exclusive in this case.
> + */
> + if (!grub_memcmp(grub_efi_system_table->firmware_vendor, "A", 1) &&
> + grub_efi_system_table->firmware_revision == (grub_efi_uint32_t)262798)
Well, I'm not thrilled by this check ... at least we need to compare
full firmware_vendor then.
> + return GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL;
> + return GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE;
> +}
> +
> static void
> grub_efinet_findcards (void)
> {
> grub_efi_uintn_t num_handles;
> grub_efi_handle_t *handles;
> grub_efi_handle_t *handle;
> + grub_efi_uint32_t attributes;
> int i = 0;
>
> /* Find handles which support the disk io interface. */
> @@ -291,6 +286,9 @@ grub_efinet_findcards (void)
> 0, &num_handles);
> if (! handles)
> return;
> +
> + attributes = grub_snp_attributes();
> +
> for (handle = handles; num_handles--; handle++)
> {
> grub_efi_simple_network_t *net;
> @@ -319,8 +317,7 @@ grub_efinet_findcards (void)
> && GRUB_EFI_DEVICE_PATH_SUBTYPE (parent) ==
> GRUB_EFI_MAC_ADDRESS_DEVICE_PATH_SUBTYPE)
> continue;
>
> - net = grub_efi_open_protocol (*handle, &net_io_guid,
> - GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> + net = grub_efi_open_protocol (*handle, &net_io_guid, attributes);
No, we cannot open exclusively here, it will destroy autocnfiguration
information we need later. You need to add conditional in open_card.
> if (! net)
> /* This should not happen... Why? */
> continue;
> @@ -332,10 +329,6 @@ grub_efinet_findcards (void)
> if (net->mode->state == GRUB_EFI_NETWORK_STOPPED)
> continue;
>
> - if (net->mode->state == GRUB_EFI_NETWORK_STARTED
> - && efi_call_3 (net->initialize, net, 0, 0) != GRUB_EFI_SUCCESS)
> - continue;
> -
> card = grub_zalloc (sizeof (struct grub_net_card));
> if (!card)
> {
> --
> 1.8.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
