Re: [edk2] [PATCH 2/2] efinet: fix lost packets due to active MNP instances

[Andrei Borzenkov]

В Tue, 21 Apr 2015 14:12:54 +0800
Michael Chang <address@hidden> пишет:

> > >  
> > > +static grub_err_t
> > > +open_card (struct grub_net_card *dev)
> > > +{
> > > +  grub_efi_simple_network_t *net;
> > 
> > I'm not sure about adding null check for dev->efi_net here is necessary
> > or not, as we don't close it in close_card so that reopening a closed
> > interface would make open_protocol fail with EFI_ACCESS_DENIED and in
> > turn makes the entire open_card funtion fail with GRUB_ERR_BAD_DEVICE.
> > 

This is now implicit in new version - SNP remains open for the driver
lifetime.

> > > +
> > > +  /* FIXME do we need to close Simple Network Protocol here? */
> > 
> > The question from me is why not? :)
> > 
> > If we don't close it, the consequence might prevent other application
> > (for eg, the chainloaded one from grub) from using managed network
> > protocol or related one ?
> > 

That's not so simple. Opening SNP exclusively actually shuts down all
other drivers using it together with all protocols. So if you load e.g.
EFI Shell you cannot use ifconfig there anymore as protocols are no
more present. Of course it is probably possible to bind them again.

Probably this patch should be split in skipping extra devices and
opening SNP exclusively, as they are more or less independent.

> 
> 2. I tried to get it going by adding card open to
> grub_net_send_ip6_packet (), but experienced another problem as the link
> layer resolve timeout due to the network interface's hardware mac
> address unset (inf->hwaddress.mac). After some debugging I realized
> that's in my net_bootp6 patch, the network interface is added during
> handling of cached dhcpv6 reply packets using the hardware mac provided
> by the card instance, which is again not yet opened.:(
> 

I tried to avoid second open to avoid situation when those values
change between two open instances. I really miss the possibility to
promote open protocol instance to exclusive use. I now reverted changes
to grub_efinet_findcards and do second exclusive open in ->open.

> 
> 3. Even I can add the card open earler before hadling the dhcpv6
> packets, it will freeze at grub_efi_open_protocol if the option in use
> is GRUB_EFI_OPEN_PROTOCOL_BY_EXCLUSIVE. I was actually using
> GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL in the entire test and am running
> out of idea how to deal with that diversity.
> 

Somehow I cannot convince my QEMU to netboot over IPv6, so please test
updated version of patch.

From: Andrei Borzenkov <address@hidden>
Subject: [PATCH v2] efinet: fix lost packets due to active MNP instances
Cc: address@hidden
Cc: Michael Chang <address@hidden>
Cc: address@hidden
Cc: address@hidden

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 | 101 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 2 deletions(-)

diff --git a/grub-core/net/drivers/efi/efinet.c 
b/grub-core/net/drivers/efi/efinet.c
index f171f20..46e337a 100644
--- a/grub-core/net/drivers/efi/efinet.c
+++ b/grub-core/net/drivers/efi/efinet.c
@@ -142,9 +142,52 @@ 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;
+
+  /* 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
+         && 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;
+
+      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;
+    }
+
+  /* If it failed we just try to run as best as we can */
+  return GRUB_ERR_NONE;
+}
+
+static void
+close_card (struct grub_net_card *dev)
+{
+  efi_call_4 (grub_efi_system_table->boot_services->close_protocol,
+             dev->efi_net, &net_io_guid,
+             grub_efi_image_handle, dev->efi_handle);
+}
+
 static struct grub_net_card_driver efidriver =
   {
     .name = "efinet",
+    .open = open_card,
+    .close = close_card,
     .send = send_card_buffer,
     .recv = get_card_packet
   };
@@ -174,6 +217,29 @@ grub_efinet_findcards (void)
     {
       grub_efi_simple_network_t *net;
       struct grub_net_card *card;
+      grub_efi_device_path_t *dp, *parent = NULL, *child = NULL;
+
+      /* EDK2 UEFI PXE driver 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;
+      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;
 
       net = grub_efi_open_protocol (*handle, &net_io_guid,
                                    GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
@@ -251,7 +317,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 UEFI PXE driver 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)
@@ -278,6 +370,11 @@ GRUB_MOD_FINI(efinet)
 
   FOR_NET_CARDS_SAFE (card, next) 
     if (card->driver == &efidriver)
-      grub_net_card_unregister (card);
+      {
+       grub_net_card_unregister (card);
+       grub_free (card->txbuf);
+       grub_free (card->rcvbuf);
+       grub_free (card);
+      }
 }
 
-- 
tg: (c981fc7..) u/efinet/skip_ipv46 (depends on: e/efinet/dp_utils)