grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael Chang
Subject: Re: [edk2] [PATCH 2/2] efinet: fix lost packets due to active MNP instances
Date: Mon, 27 Apr 2015 14:42:26 +0800
User-agent: Mutt/1.5.21 (2010-09-15)

On Sat, Apr 25, 2015 at 05:12:33PM +0300, Andrei Borzenkov wrote:
> В 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.

I'm fine with that, as they are also somewhat related as your intention
is to exlusive open "on right the device" (ie skipping extra/wrong
devices).

> 
> > 
> > 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.

I confirmed that the updated patch fixes the problem for me. In more
details this is what I've tested.

1. IPv4 PXE booting from firmware boot menu, grub2 booted in normal mode
and after selecting the boot entry, kernel and initrd were loaded and
booted. During the process it did not suffer any slowness problem.

2. IPv6 PXE booting from firmware boot menu, grub2 booted in normal mode
and after selecting the boot entry, kernel and initrd were loaded and
booted. I don't have to modify the source to use no exclusive in the
card_open() this time .. 

3. Tested with net_bootp6 command and correct efinet card device was
used and configured.

For #2, I'm not really sure why I don't have to use no exclusive this
time compared to your last patch. But from your initial comment I
believed it's also no surprised it can work.

"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."

So I'm happy to acknowlegde the patch's with my test result.

Tested-by Michael Chang <address@hidden>

Thanks,
Michael

> 
> 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)
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]