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

[Michael Chang]

On Sun, Apr 19, 2015 at 11:01:11AM +0300, Andrei Borzenkov wrote:
> 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;

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.

> +
> +  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? */

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 ?

The rest of the patch looks good to me and a lot better than my
workaround patch. Thanks for you work on this.

I can give this patch a test to see if it fixed the slowness issue I
have also experienced in OVMF for IPv4 networking and also together with
my net_bootp6 patch.

Thanks,
Michael

> +}
> +
>  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
> 

-- 
Michael Chang
Software Engineer
Rm. B, 26F, No.216, Tun Hwa S. Rd., Sec.2
Taipei 106, Taiwan, R.O.C
+886223760030
address@hidden
SUSE