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
mch...@suse.com
SUSE
------------------------------------------------------------------------------
BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT
Develop your own process in accordance with the BPMN 2 standard
Learn Process modeling best practices with Bonita BPM through live exercises
http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_
source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
