All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andrei Borzenkov <arvidjaar@gmail.com>
To: grub-devel@gnu.org
Cc: mmazur@kernel.pl, mchang@suse.com,
	edk2-devel@lists.sourceforge.net, msalter@redhat.com
Subject: [PATCH 2/2] efinet: fix lost packets due to active MNP instances
Date: Sun, 19 Apr 2015 11:01:11 +0300	[thread overview]
Message-ID: <1429430471-7229-2-git-send-email-arvidjaar@gmail.com> (raw)
In-Reply-To: <1429430471-7229-1-git-send-email-arvidjaar@gmail.com>

EDK2 network stack is based on Managed Network Protocol which is layered
on top of Simple Management Protocol and does background polling. This
polling races with grub for received (and probably trasmitted) packets
which causes either serious slowdown or complete failure to load files.

Additionaly PXE driver creates two child devices - IPv4 and IPv6 - with
bound SNP instance as well. This means we get three cards for every
physical adapter when enumerating. Not only is this confusing, this
may result in grub ignoring packets that come in via the "wrong" card.

Example of device hierarchy is

 Ctrl[91] PciRoot(0x0)/Pci(0x3,0x0)
   Ctrl[95] PciRoot(0x0)/Pci(0x3,0x0)/MAC(525400123456,0x1)
     Ctrl[B4] PciRoot(0x0)/Pci(0x3,0x0)/MAC(525400123456,0x1)/IPv4(0.0.0.0)
     Ctrl[BC] PciRoot(0x0)/Pci(0x3,0x0)/MAC(525400123456,0x1)/IPv6(0000:0000:0000:0000:0000:0000:0000:0000)

Skip PXE created virtual devices and open SNP on base device exclusively.
This destroys all child MNP instances and stops background polling.  We
cannot do it for virtual devices anyway as PXE would probably attempt
to destroy them when stopped and current OVMF crashes when we try to do it.

Exclusive open cannot be done when enumerating cards, as it would destroy
PXE information we need to autoconfigure interface; and it cannot be done
during autoconfiguration as we need to do it for non-PXE boot as well. So
move SNP open to card ->open method and add matching ->close to clean up.

References: https://savannah.gnu.org/bugs/?41731
---
 grub-core/net/drivers/efi/efinet.c | 147 ++++++++++++++++++++++++++++---------
 1 file changed, 112 insertions(+), 35 deletions(-)

diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
index f171f20..5777016 100644
--- a/grub-core/net/drivers/efi/efinet.c
+++ b/grub-core/net/drivers/efi/efinet.c
@@ -142,9 +142,72 @@ get_card_packet (struct grub_net_card *dev)
   return nb;
 }
 
+static grub_err_t
+open_card (struct grub_net_card *dev)
+{
+  grub_efi_simple_network_t *net;
+
+  net = grub_efi_open_protocol (dev->efi_handle, &net_io_guid,
+				GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE);
+  if (! net)
+    /* This should not happen... Why?  */
+    return GRUB_ERR_BAD_DEVICE;
+
+  if (net->mode->state == GRUB_EFI_NETWORK_STOPPED
+      && efi_call_1 (net->start, net) != GRUB_EFI_SUCCESS)
+    return GRUB_ERR_BAD_DEVICE;
+
+  if (net->mode->state == GRUB_EFI_NETWORK_STOPPED)
+    return GRUB_ERR_BAD_DEVICE;
+
+  if (net->mode->state == GRUB_EFI_NETWORK_STARTED
+      && efi_call_3 (net->initialize, net, 0, 0) != GRUB_EFI_SUCCESS)
+    return GRUB_ERR_BAD_DEVICE;
+
+  dev->mtu = net->mode->max_packet_size;
+
+  dev->txbufsize = ALIGN_UP (dev->mtu, 64) + 256;
+  dev->txbuf = grub_zalloc (dev->txbufsize);
+  if (!dev->txbuf)
+    {
+      grub_print_error ();
+      return GRUB_ERR_BAD_DEVICE;
+    }
+  dev->txbusy = 0;
+
+  dev->rcvbufsize = ALIGN_UP (dev->mtu, 64) + 256;
+
+  grub_memcpy (dev->default_address.mac,
+	       net->mode->current_address,
+	       sizeof (dev->default_address.mac));
+
+  dev->efi_net = net;
+  return GRUB_ERR_NONE;
+}
+
+static void
+close_card (struct grub_net_card *dev)
+{
+  if (dev->txbuf)
+    {
+      grub_free (dev->txbuf);
+      dev->txbuf = NULL;
+    }
+
+  if (dev->rcvbuf)
+    {
+      grub_free (dev->rcvbuf);
+      dev->rcvbuf = NULL;
+    }
+
+  /* FIXME do we need to close Simple Network Protocol here? */
+}
+
 static struct grub_net_card_driver efidriver =
   {
     .name = "efinet",
+    .open = open_card,
+    .close = close_card,
     .send = send_card_buffer,
     .recv = get_card_packet
   };
@@ -172,24 +235,29 @@ grub_efinet_findcards (void)
     return;
   for (handle = handles; num_handles--; handle++)
     {
-      grub_efi_simple_network_t *net;
       struct grub_net_card *card;
-
-      net = grub_efi_open_protocol (*handle, &net_io_guid,
-				    GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
-      if (! net)
-	/* This should not happen... Why?  */
+      grub_efi_device_path_t *dp, *parent = NULL, *child = NULL;
+
+      /* EDK2 UEFI PXE drivers creates IPv4 and IPv6 messaging devices as
+	 children of main MAC messaging device. We only need one device with
+	 bound SNP per physical card, otherwise they compete with each other
+	 when polling for incoming packets.
+       */
+      dp = grub_efi_get_device_path (*handle);
+      if (!dp)
 	continue;
-
-      if (net->mode->state == GRUB_EFI_NETWORK_STOPPED
-	  && efi_call_1 (net->start, net) != GRUB_EFI_SUCCESS)
-	continue;
-
-      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)
+      for (; ! GRUB_EFI_END_ENTIRE_DEVICE_PATH (dp); dp = GRUB_EFI_NEXT_DEVICE_PATH (dp))
+	{
+	  parent = child;
+	  child = dp;
+	}
+      if (child
+	  && GRUB_EFI_DEVICE_PATH_TYPE (child) == GRUB_EFI_MESSAGING_DEVICE_PATH_TYPE
+	  && (GRUB_EFI_DEVICE_PATH_SUBTYPE (child) == GRUB_EFI_IPV4_DEVICE_PATH_SUBTYPE
+	      || GRUB_EFI_DEVICE_PATH_SUBTYPE (child) == GRUB_EFI_IPV6_DEVICE_PATH_SUBTYPE)
+	  && parent
+	  && GRUB_EFI_DEVICE_PATH_TYPE (parent) == GRUB_EFI_MESSAGING_DEVICE_PATH_TYPE
+	  && GRUB_EFI_DEVICE_PATH_SUBTYPE (parent) == GRUB_EFI_MAC_ADDRESS_DEVICE_PATH_SUBTYPE)
 	continue;
 
       card = grub_zalloc (sizeof (struct grub_net_card));
@@ -200,28 +268,11 @@ grub_efinet_findcards (void)
 	  return;
 	}
 
-      card->mtu = net->mode->max_packet_size;
-      card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
-      card->txbuf = grub_zalloc (card->txbufsize);
-      if (!card->txbuf)
-	{
-	  grub_print_error ();
-	  grub_free (handles);
-	  grub_free (card);
-	  return;
-	}
-      card->txbusy = 0;
-
-      card->rcvbufsize = ALIGN_UP (card->mtu, 64) + 256;
-
       card->name = grub_xasprintf ("efinet%d", i++);
       card->driver = &efidriver;
       card->flags = 0;
       card->default_address.type = GRUB_NET_LINK_LEVEL_PROTOCOL_ETHERNET;
-      grub_memcpy (card->default_address.mac,
-		   net->mode->current_address,
-		   sizeof (card->default_address.mac));
-      card->efi_net = net;
+      card->efi_net = NULL;
       card->efi_handle = *handle;
 
       grub_net_card_register (card);
@@ -251,7 +302,33 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char **device,
     if (! cdp)
       continue;
     if (grub_efi_compare_device_paths (dp, cdp) != 0)
-      continue;
+      {
+	grub_efi_device_path_t *ldp, *dup_dp, *dup_ldp;
+	int match;
+
+	/* EDK2 PXE implementation creates pseudo devices with type IPv4/IPv6
+	   as children of Ethernet card and binds PXE and Load File protocols
+	   to it. Loaded Image Device Path protocol will point to these pseudo
+	   devices. We skip them when enumerating cards, so here we need to
+	   find matching MAC device.
+         */
+	ldp = grub_efi_find_last_device_path (dp);
+	if (GRUB_EFI_DEVICE_PATH_TYPE (ldp) != GRUB_EFI_MESSAGING_DEVICE_PATH_TYPE
+	    || (GRUB_EFI_DEVICE_PATH_SUBTYPE (ldp) != GRUB_EFI_IPV4_DEVICE_PATH_SUBTYPE
+		&& GRUB_EFI_DEVICE_PATH_SUBTYPE (ldp) != GRUB_EFI_IPV6_DEVICE_PATH_SUBTYPE))
+	  continue;
+	dup_dp = grub_efi_duplicate_device_path (dp);
+	if (!dup_dp)
+	  continue;
+	dup_ldp = grub_efi_find_last_device_path (dup_dp);
+	dup_ldp->type = GRUB_EFI_END_DEVICE_PATH_TYPE;
+	dup_ldp->subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
+	dup_ldp->length = sizeof (*dup_ldp);
+	match = grub_efi_compare_device_paths (dup_dp, cdp) == 0;
+	grub_free (dup_dp);
+	if (!match)
+	  continue;
+      }
     pxe = grub_efi_open_protocol (hnd, &pxe_io_guid,
 				  GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
     if (! pxe)
-- 
2.1.4



  reply	other threads:[~2015-04-19  8:01 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-04-19  8:01 [PATCH 1/2] efidisk: move device path helpers in core for efinet Andrei Borzenkov
2015-04-19  8:01 ` Andrei Borzenkov [this message]
2015-04-20  6:30   ` [PATCH 2/2] efinet: fix lost packets due to active MNP instances Michael Chang
2015-04-21  6:12     ` [edk2] " Michael Chang
2015-04-21  6:49       ` Michael Chang
2015-04-21 12:30         ` Laszlo Ersek
2015-04-22  9:57           ` Michael Chang
2015-04-22 10:50             ` Laszlo Ersek
2015-04-25 14:12       ` Andrei Borzenkov
2015-04-27  6:42         ` Michael Chang
2015-04-29 13:47         ` Vladimir 'φ-coder/phcoder' Serbinenko
2015-05-07 17:39           ` Andrei Borzenkov
2015-04-26  6:42       ` Andrei Borzenkov
2015-04-26 10:44         ` Andrei Borzenkov
2015-04-27  6:47         ` Michael Chang
2015-04-28  6:03           ` Michael Chang
2015-04-28  8:45             ` Michael Chang
2015-04-29 15:42 ` [PATCH 1/2] efidisk: move device path helpers in core for efinet Vladimir 'phcoder' Serbinenko
2015-05-07 14:53 ` Vladimir 'φ-coder/phcoder' Serbinenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1429430471-7229-2-git-send-email-arvidjaar@gmail.com \
    --to=arvidjaar@gmail.com \
    --cc=edk2-devel@lists.sourceforge.net \
    --cc=grub-devel@gnu.org \
    --cc=mchang@suse.com \
    --cc=mmazur@kernel.pl \
    --cc=msalter@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.