[PATCH] efinet: disable MNP background polling

[PATCH] efinet: disable MNP background polling

[HATAYAMA Daisuke]

Currently, as of the commit f348aee7b33dd85e7da62b497a96a7319a0bf9dd,
SNP is exclusively reopened to avoid slowdown or complete failure to
load files due to races between MNP background polling and grub's
receiving and transmitting packets.

This exclusive SNP reopen affects some EFI applications/drivers that
use SNP and causes PXE boot on such systems to fail. Hardware filter
issue fixed by the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73 is
one example.

The difficulty here is that effects of the exclusive SNP reopen
differs from system to system depending their UEFI firmware
implementations. One system works well with the commit
f348aee7b33dd85e7da62b497a96a7319a0bf9dd only, another system still
needs the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73, and there
could be other systems where PXE boot still fails.

Actually, PXE boot fails on Fujitsu PRIMEQUEST 2000 series now.

Thus, it seems to me that the exclusive SNP reopen makes grub
maintenance difficult by affecting a variety of systems differently.

Instead, the idea of this patch is to disable MNP background polling
temporarily while grub uses SNP. Then, the race issue must be removed.

I think origianal MNP configuration should be recovered before grub
terminates, but this version doesn't do that because I didn't find a
good place to do so.

Before applying this patch, please revert the following 2 commits
related to the exclusive SNP reopen:

  f348aee7b33dd85e7da62b497a96a7319a0bf9dd
  49426e9fd2e562c73a4f1206f32eff9e424a1a73

I would highly appreciate if someone help testing on your environment
because the origianal slow down issue doesn't occur on Fujitsu
PRIMEQUEST 2000 seriees.
---
 grub-core/net/drivers/efi/efinet.c | 14 ++++++++++++++
 include/grub/efi/api.h             | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
index c9af01c..a506ab3 100644
--- a/grub-core/net/drivers/efi/efinet.c
+++ b/grub-core/net/drivers/efi/efinet.c
@@ -29,6 +29,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
 /* GUID.  */
 static grub_efi_guid_t net_io_guid = GRUB_EFI_SIMPLE_NETWORK_GUID;
 static grub_efi_guid_t pxe_io_guid = GRUB_EFI_PXE_GUID;
+static grub_efi_guid_t mnp_io_guid = GRUB_EFI_MANAGED_NETWORK_GUID;
 
 static grub_err_t
 send_card_buffer (struct grub_net_card *dev,
@@ -269,6 +270,9 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char **device,
     grub_efi_device_path_t *cdp;
     struct grub_efi_pxe *pxe;
     struct grub_efi_pxe_mode *pxe_mode;
+    grub_efi_managed_network_t *mnp;
+    struct grub_efi_managed_network_config_data m;
+    struct grub_efi_simple_network_mode s;
     if (card->driver != &efidriver)
       continue;
     cdp = grub_efi_get_device_path (card->efi_handle);
@@ -312,6 +316,16 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char **device,
 				    &pxe_mode->dhcp_ack,
 				    sizeof (pxe_mode->dhcp_ack),
 				    1, device, path);
+
+    mnp = grub_efi_open_protocol (card->efi_handle, &mnp_io_guid,
+				  GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+
+    if (mnp
+	&& efi_call_3 (mnp->get_mode_data, mnp, &m, &s) == GRUB_EFI_SUCCESS) {
+      m.disable_background_polling = 1;
+      efi_call_2 (mnp->configure, mnp, &m);
+    }
+
     return;
   }
 }
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index e5dd543..e2baba2 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -286,6 +286,11 @@
       { 0x8B, 0x8C, 0xE2, 0x1B, 0x01, 0xAE, 0xF2, 0xB7 } \
   }
 
+#define GRUB_EFI_MANAGED_NETWORK_GUID \
+  { 0x7ab33a91, 0xace5, 0x4326, \
+      { 0xb5, 0x72, 0xe7, 0xee, 0x33, 0xd3, 0x9f, 0x16} \
+  }
+
 struct grub_efi_sal_system_table
 {
   grub_uint32_t signature;
@@ -1559,6 +1564,36 @@ struct grub_efi_block_io
 };
 typedef struct grub_efi_block_io grub_efi_block_io_t;
 
+struct grub_efi_managed_network_config_data
+{
+  grub_uint32_t received_queue_timeout_value;
+  grub_uint32_t transmit_queue_timeout_value;
+  grub_uint16_t protocol_type_filter;
+  grub_efi_boolean_t enable_unicast_receive;
+  grub_efi_boolean_t enable_multicast_receive;
+  grub_efi_boolean_t enable_broadcast_receive;
+  grub_efi_boolean_t enable_promiscuous_receive;
+  grub_efi_boolean_t flush_queues_on_reset;
+  grub_efi_boolean_t enable_receive_timestamps;
+  grub_efi_boolean_t disable_background_polling;
+};
+
+struct grub_efi_managed_network
+{
+  grub_efi_status_t (*get_mode_data) (struct grub_efi_managed_network *this,
+				      struct grub_efi_managed_network_config_data *mnp_config,
+				      struct grub_efi_simple_network_mode *snp_mode);
+  grub_efi_status_t (*configure) (struct grub_efi_managed_network *this,
+				  struct grub_efi_managed_network_config_data *mnp_config);
+  void (*mcast_ip_to_mac) (void);
+  void (*groups) (void);
+  void (*transmit) (void);
+  void (*receive) (void);
+  void (*cancel) (void);
+  void (*poll) (void);
+};
+typedef struct grub_efi_managed_network grub_efi_managed_network_t;
+
 #if (GRUB_TARGET_SIZEOF_VOID_P == 4) || defined (__ia64__) \
   || defined (__aarch64__) || defined (__MINGW64__) || defined (__CYGWIN__)

Re: [grub PATCH] efinet: disable MNP background polling

[Laszlo Ersek]

CC'ing Mark Salter, and edk2-devel, also updating the subject slightly
for better context.

On 10/01/15 11:26, HATAYAMA Daisuke wrote:
> Currently, as of the commit f348aee7b33dd85e7da62b497a96a7319a0bf9dd,
> SNP is exclusively reopened to avoid slowdown or complete failure to
> load files due to races between MNP background polling and grub's
> receiving and transmitting packets.
>
> This exclusive SNP reopen affects some EFI applications/drivers that
> use SNP and causes PXE boot on such systems to fail. Hardware filter
> issue fixed by the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73 is
> one example.

I think the above two commit references are in inverse order. That is,
commit 49426e9f is the one responsible for the (needed) exclusive open,
and commit f348aee7 fixes up the former by reconfiguring the receive
filters.

In other words, the first two paragraphs here seem accurate, just please
reorder the commit hashes.

> The difficulty here is that effects of the exclusive SNP reopen
> differs from system to system depending their UEFI firmware
> implementations. One system works well with the commit
> f348aee7b33dd85e7da62b497a96a7319a0bf9dd only, another system still
> needs the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73, and there
> could be other systems where PXE boot still fails.

Here too I think the commit hashes should be switched around.

>
> Actually, PXE boot fails on Fujitsu PRIMEQUEST 2000 series now.
>
> Thus, it seems to me that the exclusive SNP reopen makes grub
> maintenance difficult by affecting a variety of systems differently.

(Admittedly, this is going to be a bit of speculation:) I think the
difference in behavior is not due to SNP implementations, but due to
differences in higher level protocol implementations -- i.e., the
behavior depends on what those drivers do to the underlying SNP that are
*closed*.

According to the spec of OpenProtocol(), in case of a successful
exclusive open, the other BY_DRIVER reference is kicked off with
DisconnectController(), which in turn calls the other driver's
EFI_DRIVER_BINDING_PROTOCOL.Stop() function.

So, the question is what that *other* (higher level) driver does in its
Stop() function, when it unbinds the handle with the underlying SNP
interface. Does it mess with SNP's receive filters? And so on.

>
> Instead, the idea of this patch is to disable MNP background polling
> temporarily while grub uses SNP. Then, the race issue must be removed.

This is not the right approach in my opinion.

The original problem is that GRUB's direct access to SNP races with
*several* MNP instances to the same SNP. The MNP instances are correctly
arbitrated between each other (see more on this later), but the SNP
accesses from GRUB are not.

SNP is meant as an exclusive-access interface to the NIC, so those
parallel clients won't work.

One way to solve that is to kick out those other SNP accessors, by way
of exclusive open. This is correct from a UEFI driver model perspective,
but -- unless GRUB does full reconfiguration on the SNP level -- brittle
in practice, as you've experienced; apparently different implementations
of higher level protocols leave the SNP in different states when they go
away. (It's perfectly possible that some of those driver binding Stop()
functions have bugs.)

However, the other way (because there is another way, yes) is different
from interfering with existing MNP instances (note: plural). MNP (and a
bunch of other networking related protocols) don't work like your usual
UEFI device drivers; they are child protocols (= protocol interfaces on
child handles) that are created *as needed* with the help of Service
Binding protocol instances.

Please read the following sections of the UEFI spec (v2.5) carefully:

- 2.5.8 EFI Services Binding
- 10.6 EFI Service Binding Protocol
- 24.1 EFI Managed Network Protocol
  EFI_MANAGED_NETWORK_SERVICE_BINDING_PROTOCOL

> This chapter defines the EFI Managed Network Protocol. It is split
> into the following two main sections:
>
> * Managed Network Service Binding Protocol (MNSBP)
> * Managed Network Protocol (MNP)
>
> The MNP provides raw (unformatted) asynchronous network packet I/O
> services. These services make it possible for multiple-event-driven
> drivers and applications to access and use the system network
> interfaces at the same time.

The idea is that exactly one MNP *Service Binding* protocol sits on top
of SNP. Then this protocol can be used by several clients to create
client-private MNP instances -- i.e. to bind the networking *service*,
not the hardware controller handle. Those MNP instances can be then used
in parallel by different clients, and the MNPSB driver will
automatically arbitrate the exclusive SNP access between them.

In other words,
- think of the SNP as an exclusive resource,
- think of MNPSB as a *proxy* (or multiplex) that *owns* SNP,
- and if you need async packet access, go to the MNPSB, and ask it to
  give you your dedicated MNP instance,
- send and receive using your private MNP instance,
- MNPSB will nicely serialize / arbitrate that against other (=
  sibling) MNP instances.

And now it should be clear why this patch is not correct -- looking up
an existent MNP instance (or any other protocol instance that was
created with a Service Binding protocol's CreateChild() function) is
*never* correct. Such a protocol instance always belongs to *another*
client (unless you happen to find the MNP instance your own self created
earlier), and you have no business messing with that.

Therefore, if the current forced-exclusive SNP access doesn't work
reliably in GRUB, then I can see only the following method:

- identify NIC handles the same way you do now (I guess by enumerating
  SNP interfaces)
- for each handle found, look for MNPSB *first*.
- If there is no MNPSB, then stick with the current strategy -- open
  the SNP with exclusive access
- However, if there *is* an MNPSB, call the CreateChild() function to
  extract a new MNP instance
- Use this MNP instance to send and receive packets. This MNP instance
  will peacefully coexist with other (sibling) MNP clients.

Here's an example. Boot OVMF, with a virtio-net-pci device enabled in
QEMU, and make sure that the NICs ROM BAR does not contain the iPXE
driver (-device virtio-net-pci,rombar=0). This will allow OVMF's builtin
VirtioNetDxe to bind the device (which I want to use now for
illustrating the question). Then, enter the UEFI shell, and issue the
following command:

> Shell> dh -d -v -p SimpleNetwork
> A0: UnknownDevice
>     LoadFile
>     PXEBaseCode
>     TCPv4ServiceBinding
>     MTFTPv4ServiceBinding
>     DHCPv4ServiceBinding
>     UDPv4ServiceBinding
>     IPv4Config2
>     IPv4ServiceBinding
>     ARPServiceBinding
>     UnknownDevice
>     ManagedNetworkServiceBinding
>     VlanConfig
>     DevicePath PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)
>     SimpleNetwork
>
>    Controller Name    : Virtio Network Device
>    Device Path        : PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)
>    Controller Type    : BUS
>    Configuration      : NO
>    Diagnostics        : NO
>    Managed by         :
>      Drv[6F]          : MNP Network Service Driver
>      Drv[70]          : VLAN Configuration Driver
>      Drv[73]          : IP4 Network Service Driver
>    Parent Controllers :
>      Parent[8C]       : Virtio Network Device
>    Child Controllers  :
>      Child[A2]        : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x806, VlanId=0)
>      Child[A3]        : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x800, VlanId=0)
>      Child[A1]        : PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)/VenHw(D79DF6B0-EF44-43BD-9797-43E93BCF5FA8)
>      Child[A4]        : PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)/VenHw(9FB1A1F3-3B71-4324-B39A-745CBB015FFF)

There is only one handle with an SNP instance on it in the system
(because I configured only one virtio-net NIC for the VM). The
underlying hardware controller handle (marked as A0 above) has a bunch
of other protocol interfaces installed on it. Note the many
___ServiceBinding protocols!

(
For example, the UEFI spec says about UDPv4ServiceBinding:

    The EFI UDPv4 Service Binding Protocol is used to locate
    communication devices that are supported by an EFI UDPv4 Protocol
    driver and to create and destroy instances of the EFI UDPv4 Protocol
    child protocol driver that can use the underlying communications
    device.

Multiple consumers could call UDPv4ServiceBinding.CreateChild(), and
then use their private UDPv4 protocol interfaces to transmit and receive
in parallel.
)

Also, note that there is *no* MNP instance directly installed on the
handle.

Anyway, among other things, this handle is open by "MNP Network Service
Driver" (which directly provides MNPSB), marked as [6F]. And,
apparently, there have been two requests to MNPSB for children: see [A2]
and [A3] above.

We can investigate those as well:

> Shell> dh -d -v A2
> A2: 7EF49B58
> ManagedNetwork
>    Controller Name    : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x806, VlanId=0)
>    Device Path        : <None>
>    Controller Type    : BUS
>    Configuration      : NO
>    Diagnostics        : NO
>    Managed by         :
>      Drv[71]          : ARP Network Service Driver
>    Parent Controllers :
>      Parent[A0]       : Virtio Network Device
>    Child Controllers  :
>      Child[AB]        : PXE Controller

The first MNP child has been extracted / requested, from MNPSB, by the
ARP (SB) driver.

> Shell> dh -d -v A3
> A3: 7EF56AD8
> ManagedNetwork
>    Controller Name    : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x800, VlanId=0)
>    Device Path        : <None>
>    Controller Type    : BUS
>    Configuration      : NO
>    Diagnostics        : NO
>    Managed by         :
>      Drv[73]          : IP4 Network Service Driver
>    Parent Controllers :
>      Parent[A0]       : Virtio Network Device
>    Child Controllers  :
>      Child[A5]        : IPv4 (SrcIP=0.0.0.0)
>      Child[A6]        : IPv4 (SrcIP=0.0.0.0)
>      Child[A8]        : IPv4 (Not started)
>      Child[AA]        : IPv4 (SrcIP=0.0.0.0)
>      Child[AD]        : PXE Controller
>      Child[AE]        : IPv4 (Not started)
>      Child[B1]        : IPv4 (Not started)
>      Child[B3]        : IPv4 (Not started)
>      Child[B7]        : IPv4 (Not started)

The other MNP child has been extracted / requested, from MNPSB, by the
IPv4 (SB) driver.

Now, assuming GRUB wants Ethernet packet level access, this is exactly
what GRUB should do too: locate MNPSB, and ask it for another MNP
instance. That MNP instance will be dedicated to GRUB, and the MNP
Network Service Driver will multiplex it with other users (ARP Network
Service Driver, IP4 Network Service Driver).

Alternatively, GRUB could look for higher level service binding
protocols as well, on EFI systems (see the list near A0 above), but I
doubt that would fit well into GRUB's net framework (which is supposed
to run on "legacy" BIOS systems too).

To summarize:
- identify the level / abstraction of the network protocol that GRUB
  needs,
- assuming it is "ethernet packet", look for MNPSB first, and if it's
  there, call it to get a private-use MNP instance, in order to transmit
  and receive,
- if MNPSB is not there, open SNP in exclusive mode, same as now.

Or else,
- stick with the current exclusive SNP reopen, but make sure that all
  aspects are reconfigured from the ground up.

... I'm not a GRUB contributor, and ideas are cheap :), so I'll leave it
to you how much importance you give to the above. But, since you CC'd me
on the patch, I thought I'd offer an opinion.

Thanks
Laszlo

On 10/01/15 11:26, HATAYAMA Daisuke wrote:
> I think origianal MNP configuration should be recovered before grub
> terminates, but this version doesn't do that because I didn't find a
> good place to do so.
>
> Before applying this patch, please revert the following 2 commits
> related to the exclusive SNP reopen:
>
>   f348aee7b33dd85e7da62b497a96a7319a0bf9dd
>   49426e9fd2e562c73a4f1206f32eff9e424a1a73
>
> I would highly appreciate if someone help testing on your environment
> because the origianal slow down issue doesn't occur on Fujitsu
> PRIMEQUEST 2000 seriees.
> ---
>  grub-core/net/drivers/efi/efinet.c | 14 ++++++++++++++
>  include/grub/efi/api.h             | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
>
> diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
> index c9af01c..a506ab3 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -29,6 +29,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  /* GUID.  */
>  static grub_efi_guid_t net_io_guid = GRUB_EFI_SIMPLE_NETWORK_GUID;
>  static grub_efi_guid_t pxe_io_guid = GRUB_EFI_PXE_GUID;
> +static grub_efi_guid_t mnp_io_guid = GRUB_EFI_MANAGED_NETWORK_GUID;
>
>  static grub_err_t
>  send_card_buffer (struct grub_net_card *dev,
> @@ -269,6 +270,9 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char **device,
>      grub_efi_device_path_t *cdp;
>      struct grub_efi_pxe *pxe;
>      struct grub_efi_pxe_mode *pxe_mode;
> +    grub_efi_managed_network_t *mnp;
> +    struct grub_efi_managed_network_config_data m;
> +    struct grub_efi_simple_network_mode s;
>      if (card->driver != &efidriver)
>        continue;
>      cdp = grub_efi_get_device_path (card->efi_handle);
> @@ -312,6 +316,16 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char **device,
>  				    &pxe_mode->dhcp_ack,
>  				    sizeof (pxe_mode->dhcp_ack),
>  				    1, device, path);
> +
> +    mnp = grub_efi_open_protocol (card->efi_handle, &mnp_io_guid,
> +				  GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +
> +    if (mnp
> +	&& efi_call_3 (mnp->get_mode_data, mnp, &m, &s) == GRUB_EFI_SUCCESS) {
> +      m.disable_background_polling = 1;
> +      efi_call_2 (mnp->configure, mnp, &m);
> +    }
> +
>      return;
>    }
>  }
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index e5dd543..e2baba2 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -286,6 +286,11 @@
>        { 0x8B, 0x8C, 0xE2, 0x1B, 0x01, 0xAE, 0xF2, 0xB7 } \
>    }
>
> +#define GRUB_EFI_MANAGED_NETWORK_GUID \
> +  { 0x7ab33a91, 0xace5, 0x4326, \
> +      { 0xb5, 0x72, 0xe7, 0xee, 0x33, 0xd3, 0x9f, 0x16} \
> +  }
> +
>  struct grub_efi_sal_system_table
>  {
>    grub_uint32_t signature;
> @@ -1559,6 +1564,36 @@ struct grub_efi_block_io
>  };
>  typedef struct grub_efi_block_io grub_efi_block_io_t;
>
> +struct grub_efi_managed_network_config_data
> +{
> +  grub_uint32_t received_queue_timeout_value;
> +  grub_uint32_t transmit_queue_timeout_value;
> +  grub_uint16_t protocol_type_filter;
> +  grub_efi_boolean_t enable_unicast_receive;
> +  grub_efi_boolean_t enable_multicast_receive;
> +  grub_efi_boolean_t enable_broadcast_receive;
> +  grub_efi_boolean_t enable_promiscuous_receive;
> +  grub_efi_boolean_t flush_queues_on_reset;
> +  grub_efi_boolean_t enable_receive_timestamps;
> +  grub_efi_boolean_t disable_background_polling;
> +};
> +
> +struct grub_efi_managed_network
> +{
> +  grub_efi_status_t (*get_mode_data) (struct grub_efi_managed_network *this,
> +				      struct grub_efi_managed_network_config_data *mnp_config,
> +				      struct grub_efi_simple_network_mode *snp_mode);
> +  grub_efi_status_t (*configure) (struct grub_efi_managed_network *this,
> +				  struct grub_efi_managed_network_config_data *mnp_config);
> +  void (*mcast_ip_to_mac) (void);
> +  void (*groups) (void);
> +  void (*transmit) (void);
> +  void (*receive) (void);
> +  void (*cancel) (void);
> +  void (*poll) (void);
> +};
> +typedef struct grub_efi_managed_network grub_efi_managed_network_t;
> +
>  #if (GRUB_TARGET_SIZEOF_VOID_P == 4) || defined (__ia64__) \
>    || defined (__aarch64__) || defined (__MINGW64__) || defined (__CYGWIN__)
>

Re: [grub PATCH] efinet: disable MNP background polling

[Andrei Borzenkov]

01.10.2015 14:50, Laszlo Ersek пишет:
> - assuming it is "ethernet packet", look for MNPSB first, and if it's
>    there, call it to get a private-use MNP instance, in order to transmit
>    and receive,
> - if MNPSB is not there, open SNP in exclusive mode, same as now.
>
> Or else,
> - stick with the current exclusive SNP reopen, but make sure that all
>    aspects are reconfigured from the ground up.
>

I completely agree; the tiny insignificant missing piece here is actual 
code :) Of course it also means new can of worms and new unknown 
firmware bugs ...

Hatayama-san, would you consider implementing MNP-based driver for GRUB? 
Having at least proof of concept available for testing would be good.

Re: [grub PATCH] efinet: disable MNP background polling

[Yinghai Lu]

On Thu, Oct 1, 2015 at 10:53 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>
> Hatayama-san, would you consider implementing MNP-based driver for GRUB?
> Having at least proof of concept available for testing would be good.

There is one solaris grub2 that does support MNP.

Thanks

Yinghai

Re: [grub PATCH] efinet: disable MNP background polling

[Andrei Borzenkov]

02.10.2015 01:04, Yinghai Lu пишет:
> On Thu, Oct 1, 2015 at 10:53 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
>>
>> Hatayama-san, would you consider implementing MNP-based driver for GRUB?
>> Having at least proof of concept available for testing would be good.
>
> There is one solaris grub2 that does support MNP.
>

Unfortunately it does not really help unless someone is going to submit 
it here.

Re: [grub PATCH] efinet: disable MNP background polling

[HATAYAMA Daisuke]

Sorry for delayed response.

From: Andrei Borzenkov <arvidjaar@gmail.com>
Subject: Re: [grub PATCH] efinet: disable MNP background polling
Date: Thu, 1 Oct 2015 20:53:44 +0300

> 01.10.2015 14:50, Laszlo Ersek пишет:
>> - assuming it is "ethernet packet", look for MNPSB first, and if it's
>>    there, call it to get a private-use MNP instance, in order to transmit
>>    and receive,
>> - if MNPSB is not there, open SNP in exclusive mode, same as now.
>>
>> Or else,
>> - stick with the current exclusive SNP reopen, but make sure that all
>>    aspects are reconfigured from the ground up.
>>
> 
> I completely agree; the tiny insignificant missing piece here is
> actual code :) Of course it also means new can of worms and new
> unknown firmware bugs ...
> 
> Hatayama-san, would you consider implementing MNP-based driver for
> GRUB? Having at least proof of concept available for testing would be
> good.
>

Yes, I think now MNP-based implementaion is necessary. I'll try that
but I need some time to investigate the implementation. Please wait.

--
Thanks.
HATAYAMA, Daisuke

Re: [grub PATCH] efinet: disable MNP background polling

[Daniel Kiper]

On Thu, Oct 01, 2015 at 08:53:44PM +0300, Andrei Borzenkov wrote:
> 01.10.2015 14:50, Laszlo Ersek ??????????:
> >- assuming it is "ethernet packet", look for MNPSB first, and if it's
> >   there, call it to get a private-use MNP instance, in order to transmit
> >   and receive,
> >- if MNPSB is not there, open SNP in exclusive mode, same as now.
> >
> >Or else,
> >- stick with the current exclusive SNP reopen, but make sure that all
> >   aspects are reconfigured from the ground up.
> >
>
> I completely agree; the tiny insignificant missing piece here is actual
> code :) Of course it also means new can of worms and new unknown
> firmware bugs ...
>
> Hatayama-san, would you consider implementing MNP-based driver for GRUB?
> Having at least proof of concept available for testing would be good.

Are we sure that we want to migrate to MNP? As I saw others (e.g. PXELINUX,
iPXE) use SNP and it work for them.

If yes then I can post my unfinished rebase work for Solaris GRUB2 MNP patches.
I did that for our internal needs, however, I did not finish it because usptream
GRUB2 solved our problems. Well, patches are very raw, based on very old GRUB2
(Apr 2012) and do not build. However, maybe they could be a good starting point
for further work.

Daniel

Re: [grub PATCH] efinet: disable MNP background polling

[Laszlo Ersek]

On 10/14/15 00:11, Daniel Kiper wrote:
> On Thu, Oct 01, 2015 at 08:53:44PM +0300, Andrei Borzenkov wrote:
>> 01.10.2015 14:50, Laszlo Ersek ??????????:
>>> - assuming it is "ethernet packet", look for MNPSB first, and if it's
>>>   there, call it to get a private-use MNP instance, in order to transmit
>>>   and receive,
>>> - if MNPSB is not there, open SNP in exclusive mode, same as now.
>>>
>>> Or else,
>>> - stick with the current exclusive SNP reopen, but make sure that all
>>>   aspects are reconfigured from the ground up.
>>>
>>
>> I completely agree; the tiny insignificant missing piece here is actual
>> code :) Of course it also means new can of worms and new unknown
>> firmware bugs ...
>>
>> Hatayama-san, would you consider implementing MNP-based driver for GRUB?
>> Having at least proof of concept available for testing would be good.
> 
> Are we sure that we want to migrate to MNP? As I saw others (e.g. PXELINUX,
> iPXE) use SNP and it work for them.

Hatayama-san, can you perhaps test PXELINUX and iPXE on those machines
where the current grub code exposes problems?

Thank you,
Laszlo

> 
> If yes then I can post my unfinished rebase work for Solaris GRUB2 MNP patches.
> I did that for our internal needs, however, I did not finish it because usptream
> GRUB2 solved our problems. Well, patches are very raw, based on very old GRUB2
> (Apr 2012) and do not build. However, maybe they could be a good starting point
> for further work.
> 
> Daniel
> 

Re: [grub PATCH] efinet: disable MNP background polling

[Yinghai Lu]

On Tue, Oct 13, 2015 at 3:11 PM, Daniel Kiper <dkiper@net-space.pl> wrote:
> On Thu, Oct 01, 2015 at 08:53:44PM +0300, Andrei Borzenkov wrote:
>> Hatayama-san, would you consider implementing MNP-based driver for GRUB?
>> Having at least proof of concept available for testing would be good.
>
> Are we sure that we want to migrate to MNP? As I saw others (e.g. PXELINUX,
> iPXE) use SNP and it work for them.
>
> If yes then I can post my unfinished rebase work for Solaris GRUB2 MNP patches.
> I did that for our internal needs, however, I did not finish it because usptream
> GRUB2 solved our problems. Well, patches are very raw, based on very old GRUB2
> (Apr 2012) and do not build. However, maybe they could be a good starting point
> for further work.

I ported that on top of grub2 upstream this January. It is based on
upstream of 2015-01-28.
so need to revert 4fe8e6d f348aee c52ae40 49426e9 cf2b4a3 from current upstream.

We (x86 system group) have used it internally for a while. It solved
all the uefi pxe booting
problems that we met with grub2, like 1g intel supper slow, or 10g
intel does not even start.

Thanks

Yinghai

Re: [grub PATCH] efinet: disable MNP background polling

[Andrei Borzenkov]

14.10.2015 04:01, Yinghai Lu пишет:
> On Tue, Oct 13, 2015 at 3:11 PM, Daniel Kiper <dkiper@net-space.pl> wrote:
>> On Thu, Oct 01, 2015 at 08:53:44PM +0300, Andrei Borzenkov wrote:
>>> Hatayama-san, would you consider implementing MNP-based driver for GRUB?
>>> Having at least proof of concept available for testing would be good.
>>
>> Are we sure that we want to migrate to MNP? As I saw others (e.g. PXELINUX,
>> iPXE) use SNP and it work for them.
>>
>> If yes then I can post my unfinished rebase work for Solaris GRUB2 MNP patches.
>> I did that for our internal needs, however, I did not finish it because usptream
>> GRUB2 solved our problems. Well, patches are very raw, based on very old GRUB2
>> (Apr 2012) and do not build. However, maybe they could be a good starting point
>> for further work.
>
> I ported that on top of grub2 upstream this January. It is based on
> upstream of 2015-01-28.
> so need to revert 4fe8e6d f348aee c52ae40 49426e9 cf2b4a3 from current upstream.
>

Do you also observe problems with current upstream?

> We (x86 system group) have used it internally for a while. It solved
> all the uefi pxe booting
> problems that we met with grub2, like 1g intel supper slow, or 10g
> intel does not even start.
>
> Thanks
>
> Yinghai
>

Re: [grub PATCH] efinet: disable MNP background polling

[HATAYAMA Daisuke]

Sorry for delayed response.

From: Laszlo Ersek <lersek@redhat.com>
Subject: Re: [grub PATCH] efinet: disable MNP background polling
Date: Thu, 1 Oct 2015 13:50:31 +0200

> CC'ing Mark Salter, and edk2-devel, also updating the subject slightly
> for better context.
> 
> On 10/01/15 11:26, HATAYAMA Daisuke wrote:
>> Currently, as of the commit f348aee7b33dd85e7da62b497a96a7319a0bf9dd,
>> SNP is exclusively reopened to avoid slowdown or complete failure to
>> load files due to races between MNP background polling and grub's
>> receiving and transmitting packets.
>>
>> This exclusive SNP reopen affects some EFI applications/drivers that
>> use SNP and causes PXE boot on such systems to fail. Hardware filter
>> issue fixed by the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73 is
>> one example.
> 
> I think the above two commit references are in inverse order. That is,
> commit 49426e9f is the one responsible for the (needed) exclusive open,
> and commit f348aee7 fixes up the former by reconfiguring the receive
> filters.
> 
> In other words, the first two paragraphs here seem accurate, just please
> reorder the commit hashes.
>
>> The difficulty here is that effects of the exclusive SNP reopen
>> differs from system to system depending their UEFI firmware
>> implementations. One system works well with the commit
>> f348aee7b33dd85e7da62b497a96a7319a0bf9dd only, another system still
>> needs the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73, and there
>> could be other systems where PXE boot still fails.
> 
> Here too I think the commit hashes should be switched around.
>

Sorry, I'll fix this in next path if I mention them.

>>
>> Actually, PXE boot fails on Fujitsu PRIMEQUEST 2000 series now.
>>
>> Thus, it seems to me that the exclusive SNP reopen makes grub
>> maintenance difficult by affecting a variety of systems differently.
> 
> (Admittedly, this is going to be a bit of speculation:) I think the
> difference in behavior is not due to SNP implementations, but due to
> differences in higher level protocol implementations -- i.e., the
> behavior depends on what those drivers do to the underlying SNP that are
> *closed*.
> 
> According to the spec of OpenProtocol(), in case of a successful
> exclusive open, the other BY_DRIVER reference is kicked off with
> DisconnectController(), which in turn calls the other driver's
> EFI_DRIVER_BINDING_PROTOCOL.Stop() function.
> 
> So, the question is what that *other* (higher level) driver does in its
> Stop() function, when it unbinds the handle with the underlying SNP
> interface. Does it mess with SNP's receive filters? And so on.
> 
>>
>> Instead, the idea of this patch is to disable MNP background polling
>> temporarily while grub uses SNP. Then, the race issue must be removed.
> 
> This is not the right approach in my opinion.
> 
> The original problem is that GRUB's direct access to SNP races with
> *several* MNP instances to the same SNP. The MNP instances are correctly
> arbitrated between each other (see more on this later), but the SNP
> accesses from GRUB are not.
> 
> SNP is meant as an exclusive-access interface to the NIC, so those
> parallel clients won't work.
> 
> One way to solve that is to kick out those other SNP accessors, by way
> of exclusive open. This is correct from a UEFI driver model perspective,
> but -- unless GRUB does full reconfiguration on the SNP level -- brittle
> in practice, as you've experienced; apparently different implementations
> of higher level protocols leave the SNP in different states when they go
> away. (It's perfectly possible that some of those driver binding Stop()
> functions have bugs.)
> 
> However, the other way (because there is another way, yes) is different
> from interfering with existing MNP instances (note: plural). MNP (and a
> bunch of other networking related protocols) don't work like your usual
> UEFI device drivers; they are child protocols (= protocol interfaces on
> child handles) that are created *as needed* with the help of Service
> Binding protocol instances.
> 
> Please read the following sections of the UEFI spec (v2.5) carefully:
> 
> - 2.5.8 EFI Services Binding
> - 10.6 EFI Service Binding Protocol
> - 24.1 EFI Managed Network Protocol
>   EFI_MANAGED_NETWORK_SERVICE_BINDING_PROTOCOL
> 
>> This chapter defines the EFI Managed Network Protocol. It is split
>> into the following two main sections:
>>
>> * Managed Network Service Binding Protocol (MNSBP)
>> * Managed Network Protocol (MNP)
>>
>> The MNP provides raw (unformatted) asynchronous network packet I/O
>> services. These services make it possible for multiple-event-driven
>> drivers and applications to access and use the system network
>> interfaces at the same time.
> 
> The idea is that exactly one MNP *Service Binding* protocol sits on top
> of SNP. Then this protocol can be used by several clients to create
> client-private MNP instances -- i.e. to bind the networking *service*,
> not the hardware controller handle. Those MNP instances can be then used
> in parallel by different clients, and the MNPSB driver will
> automatically arbitrate the exclusive SNP access between them.
> 
> In other words,
> - think of the SNP as an exclusive resource,
> - think of MNPSB as a *proxy* (or multiplex) that *owns* SNP,
> - and if you need async packet access, go to the MNPSB, and ask it to
>   give you your dedicated MNP instance,
> - send and receive using your private MNP instance,
> - MNPSB will nicely serialize / arbitrate that against other (=
>   sibling) MNP instances.
> 
> And now it should be clear why this patch is not correct -- looking up
> an existent MNP instance (or any other protocol instance that was
> created with a Service Binding protocol's CreateChild() function) is
> *never* correct. Such a protocol instance always belongs to *another*
> client (unless you happen to find the MNP instance your own self created
> earlier), and you have no business messing with that.
> 
> Therefore, if the current forced-exclusive SNP access doesn't work
> reliably in GRUB, then I can see only the following method:
> 
> - identify NIC handles the same way you do now (I guess by enumerating
>   SNP interfaces)
> - for each handle found, look for MNPSB *first*.
> - If there is no MNPSB, then stick with the current strategy -- open
>   the SNP with exclusive access
> - However, if there *is* an MNPSB, call the CreateChild() function to
>   extract a new MNP instance
> - Use this MNP instance to send and receive packets. This MNP instance
>   will peacefully coexist with other (sibling) MNP clients.
> 
> Here's an example. Boot OVMF, with a virtio-net-pci device enabled in
> QEMU, and make sure that the NICs ROM BAR does not contain the iPXE
> driver (-device virtio-net-pci,rombar=0). This will allow OVMF's builtin
> VirtioNetDxe to bind the device (which I want to use now for
> illustrating the question). Then, enter the UEFI shell, and issue the
> following command:
> 
>> Shell> dh -d -v -p SimpleNetwork
>> A0: UnknownDevice
>>     LoadFile
>>     PXEBaseCode
>>     TCPv4ServiceBinding
>>     MTFTPv4ServiceBinding
>>     DHCPv4ServiceBinding
>>     UDPv4ServiceBinding
>>     IPv4Config2
>>     IPv4ServiceBinding
>>     ARPServiceBinding
>>     UnknownDevice
>>     ManagedNetworkServiceBinding
>>     VlanConfig
>>     DevicePath PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)
>>     SimpleNetwork
>>
>>    Controller Name    : Virtio Network Device
>>    Device Path        : PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)
>>    Controller Type    : BUS
>>    Configuration      : NO
>>    Diagnostics        : NO
>>    Managed by         :
>>      Drv[6F]          : MNP Network Service Driver
>>      Drv[70]          : VLAN Configuration Driver
>>      Drv[73]          : IP4 Network Service Driver
>>    Parent Controllers :
>>      Parent[8C]       : Virtio Network Device
>>    Child Controllers  :
>>      Child[A2]        : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x806, VlanId=0)
>>      Child[A3]        : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x800, VlanId=0)
>>      Child[A1]        : PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)/VenHw(D79DF6B0-EF44-43BD-9797-43E93BCF5FA8)
>>      Child[A4]        : PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)/VenHw(9FB1A1F3-3B71-4324-B39A-745CBB015FFF)
> 
> There is only one handle with an SNP instance on it in the system
> (because I configured only one virtio-net NIC for the VM). The
> underlying hardware controller handle (marked as A0 above) has a bunch
> of other protocol interfaces installed on it. Note the many
> ___ServiceBinding protocols!
> 
> (
> For example, the UEFI spec says about UDPv4ServiceBinding:
> 
>     The EFI UDPv4 Service Binding Protocol is used to locate
>     communication devices that are supported by an EFI UDPv4 Protocol
>     driver and to create and destroy instances of the EFI UDPv4 Protocol
>     child protocol driver that can use the underlying communications
>     device.
> 
> Multiple consumers could call UDPv4ServiceBinding.CreateChild(), and
> then use their private UDPv4 protocol interfaces to transmit and receive
> in parallel.
> )
> 
> Also, note that there is *no* MNP instance directly installed on the
> handle.
> 
> Anyway, among other things, this handle is open by "MNP Network Service
> Driver" (which directly provides MNPSB), marked as [6F]. And,
> apparently, there have been two requests to MNPSB for children: see [A2]
> and [A3] above.
> 
> We can investigate those as well:
> 
>> Shell> dh -d -v A2
>> A2: 7EF49B58
>> ManagedNetwork
>>    Controller Name    : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x806, VlanId=0)
>>    Device Path        : <None>
>>    Controller Type    : BUS
>>    Configuration      : NO
>>    Diagnostics        : NO
>>    Managed by         :
>>      Drv[71]          : ARP Network Service Driver
>>    Parent Controllers :
>>      Parent[A0]       : Virtio Network Device
>>    Child Controllers  :
>>      Child[AB]        : PXE Controller
> 
> The first MNP child has been extracted / requested, from MNPSB, by the
> ARP (SB) driver.
> 
>> Shell> dh -d -v A3
>> A3: 7EF56AD8
>> ManagedNetwork
>>    Controller Name    : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x800, VlanId=0)
>>    Device Path        : <None>
>>    Controller Type    : BUS
>>    Configuration      : NO
>>    Diagnostics        : NO
>>    Managed by         :
>>      Drv[73]          : IP4 Network Service Driver
>>    Parent Controllers :
>>      Parent[A0]       : Virtio Network Device
>>    Child Controllers  :
>>      Child[A5]        : IPv4 (SrcIP=0.0.0.0)
>>      Child[A6]        : IPv4 (SrcIP=0.0.0.0)
>>      Child[A8]        : IPv4 (Not started)
>>      Child[AA]        : IPv4 (SrcIP=0.0.0.0)
>>      Child[AD]        : PXE Controller
>>      Child[AE]        : IPv4 (Not started)
>>      Child[B1]        : IPv4 (Not started)
>>      Child[B3]        : IPv4 (Not started)
>>      Child[B7]        : IPv4 (Not started)
> 
> The other MNP child has been extracted / requested, from MNPSB, by the
> IPv4 (SB) driver.
> 
> Now, assuming GRUB wants Ethernet packet level access, this is exactly
> what GRUB should do too: locate MNPSB, and ask it for another MNP
> instance. That MNP instance will be dedicated to GRUB, and the MNP
> Network Service Driver will multiplex it with other users (ARP Network
> Service Driver, IP4 Network Service Driver).
> 
> Alternatively, GRUB could look for higher level service binding
> protocols as well, on EFI systems (see the list near A0 above), but I
> doubt that would fit well into GRUB's net framework (which is supposed
> to run on "legacy" BIOS systems too).
> 
> To summarize:
> - identify the level / abstraction of the network protocol that GRUB
>   needs,
> - assuming it is "ethernet packet", look for MNPSB first, and if it's
>   there, call it to get a private-use MNP instance, in order to transmit
>   and receive,
> - if MNPSB is not there, open SNP in exclusive mode, same as now.
> 
> Or else,
> - stick with the current exclusive SNP reopen, but make sure that all
>   aspects are reconfigured from the ground up.
> 
> ... I'm not a GRUB contributor, and ideas are cheap :), so I'll leave it
> to you how much importance you give to the above. But, since you CC'd me
> on the patch, I thought I'd offer an opinion.
> 
> Thanks
> Laszlo
>

Thanks for detailed explanation. I'm beginner for UEFI field so this
is very helpful to understand the details around this issue.

Now I think I need to do this work and I want sample codes that uses
NMP and MNPSB protocols if exists. Could you tell me such ones if you
know? In EDK2 source code?

--
Thanks.
HATAYAMA, Daisuke

Re: [grub PATCH] efinet: disable MNP background polling

[Laszlo Ersek]

On 10/09/15 12:10, HATAYAMA Daisuke wrote:
> Sorry for delayed response.
> 
> From: Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [grub PATCH] efinet: disable MNP background polling
> Date: Thu, 1 Oct 2015 13:50:31 +0200
> 

[snip]

>> Here's an example. Boot OVMF, with a virtio-net-pci device enabled in
>> QEMU, and make sure that the NICs ROM BAR does not contain the iPXE
>> driver (-device virtio-net-pci,rombar=0). This will allow OVMF's builtin
>> VirtioNetDxe to bind the device (which I want to use now for
>> illustrating the question). Then, enter the UEFI shell, and issue the
>> following command:
>>
>>> Shell> dh -d -v -p SimpleNetwork
>>> A0: UnknownDevice
>>>     LoadFile
>>>     PXEBaseCode
>>>     TCPv4ServiceBinding
>>>     MTFTPv4ServiceBinding
>>>     DHCPv4ServiceBinding
>>>     UDPv4ServiceBinding
>>>     IPv4Config2
>>>     IPv4ServiceBinding
>>>     ARPServiceBinding
>>>     UnknownDevice
>>>     ManagedNetworkServiceBinding
>>>     VlanConfig
>>>     DevicePath PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)
>>>     SimpleNetwork
>>>
>>>    Controller Name    : Virtio Network Device
>>>    Device Path        : PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)
>>>    Controller Type    : BUS
>>>    Configuration      : NO
>>>    Diagnostics        : NO
>>>    Managed by         :
>>>      Drv[6F]          : MNP Network Service Driver
>>>      Drv[70]          : VLAN Configuration Driver
>>>      Drv[73]          : IP4 Network Service Driver
>>>    Parent Controllers :
>>>      Parent[8C]       : Virtio Network Device
>>>    Child Controllers  :
>>>      Child[A2]        : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x806, VlanId=0)
>>>      Child[A3]        : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x800, VlanId=0)
>>>      Child[A1]        : PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)/VenHw(D79DF6B0-EF44-43BD-9797-43E93BCF5FA8)
>>>      Child[A4]        : PciRoot(0x0)/Pci(0x3,0x0)/MAC(52540036A382,0x1)/VenHw(9FB1A1F3-3B71-4324-B39A-745CBB015FFF)
>>
>> There is only one handle with an SNP instance on it in the system
>> (because I configured only one virtio-net NIC for the VM). The
>> underlying hardware controller handle (marked as A0 above) has a bunch
>> of other protocol interfaces installed on it. Note the many
>> ___ServiceBinding protocols!
>>
>> (
>> For example, the UEFI spec says about UDPv4ServiceBinding:
>>
>>     The EFI UDPv4 Service Binding Protocol is used to locate
>>     communication devices that are supported by an EFI UDPv4 Protocol
>>     driver and to create and destroy instances of the EFI UDPv4 Protocol
>>     child protocol driver that can use the underlying communications
>>     device.
>>
>> Multiple consumers could call UDPv4ServiceBinding.CreateChild(), and
>> then use their private UDPv4 protocol interfaces to transmit and receive
>> in parallel.
>> )
>>
>> Also, note that there is *no* MNP instance directly installed on the
>> handle.
>>
>> Anyway, among other things, this handle is open by "MNP Network Service
>> Driver" (which directly provides MNPSB), marked as [6F]. And,
>> apparently, there have been two requests to MNPSB for children: see [A2]
>> and [A3] above.
>>
>> We can investigate those as well:
>>
>>> Shell> dh -d -v A2
>>> A2: 7EF49B58
>>> ManagedNetwork
>>>    Controller Name    : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x806, VlanId=0)
>>>    Device Path        : <None>
>>>    Controller Type    : BUS
>>>    Configuration      : NO
>>>    Diagnostics        : NO
>>>    Managed by         :
>>>      Drv[71]          : ARP Network Service Driver
>>>    Parent Controllers :
>>>      Parent[A0]       : Virtio Network Device
>>>    Child Controllers  :
>>>      Child[AB]        : PXE Controller
>>
>> The first MNP child has been extracted / requested, from MNPSB, by the
>> ARP (SB) driver.
>>
>>> Shell> dh -d -v A3
>>> A3: 7EF56AD8
>>> ManagedNetwork
>>>    Controller Name    : MNP (MAC=52-54-00-36-A3-82, ProtocolType=0x800, VlanId=0)
>>>    Device Path        : <None>
>>>    Controller Type    : BUS
>>>    Configuration      : NO
>>>    Diagnostics        : NO
>>>    Managed by         :
>>>      Drv[73]          : IP4 Network Service Driver
>>>    Parent Controllers :
>>>      Parent[A0]       : Virtio Network Device
>>>    Child Controllers  :
>>>      Child[A5]        : IPv4 (SrcIP=0.0.0.0)
>>>      Child[A6]        : IPv4 (SrcIP=0.0.0.0)
>>>      Child[A8]        : IPv4 (Not started)
>>>      Child[AA]        : IPv4 (SrcIP=0.0.0.0)
>>>      Child[AD]        : PXE Controller
>>>      Child[AE]        : IPv4 (Not started)
>>>      Child[B1]        : IPv4 (Not started)
>>>      Child[B3]        : IPv4 (Not started)
>>>      Child[B7]        : IPv4 (Not started)
>>
>> The other MNP child has been extracted / requested, from MNPSB, by the
>> IPv4 (SB) driver.
>>
>> Now, assuming GRUB wants Ethernet packet level access, this is exactly
>> what GRUB should do too: locate MNPSB, and ask it for another MNP
>> instance. That MNP instance will be dedicated to GRUB, and the MNP
>> Network Service Driver will multiplex it with other users (ARP Network
>> Service Driver, IP4 Network Service Driver).
>>
>> Alternatively, GRUB could look for higher level service binding
>> protocols as well, on EFI systems (see the list near A0 above), but I
>> doubt that would fit well into GRUB's net framework (which is supposed
>> to run on "legacy" BIOS systems too).
>>
>> To summarize:
>> - identify the level / abstraction of the network protocol that GRUB
>>   needs,
>> - assuming it is "ethernet packet", look for MNPSB first, and if it's
>>   there, call it to get a private-use MNP instance, in order to transmit
>>   and receive,
>> - if MNPSB is not there, open SNP in exclusive mode, same as now.
>>
>> Or else,
>> - stick with the current exclusive SNP reopen, but make sure that all
>>   aspects are reconfigured from the ground up.
>>
>> ... I'm not a GRUB contributor, and ideas are cheap :), so I'll leave it
>> to you how much importance you give to the above. But, since you CC'd me
>> on the patch, I thought I'd offer an opinion.
>>
>> Thanks
>> Laszlo
>>
> 
> Thanks for detailed explanation. I'm beginner for UEFI field so this
> is very helpful to understand the details around this issue.
> 
> Now I think I need to do this work and I want sample codes that uses
> NMP and MNPSB protocols if exists. Could you tell me such ones if you
> know? In EDK2 source code?

Sure. As I mentioned in the above example, you can look at "ARP Network
Service Driver" and "IP4 Network Service Driver" for examples. The
relevant source files in edk2 are:

- MdeModulePkg/Universal/Network/ArpDxe/ArpDriver.c

  See function ArpCreateService():

  //
  // Create a MNP child instance.
  //
  Status = NetLibCreateServiceChild (
             ControllerHandle,
             ImageHandle,
             &gEfiManagedNetworkServiceBindingProtocolGuid,
             &ArpService->MnpChildHandle
             );
  if (EFI_ERROR (Status)) {
    return Status;
  }


- MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Driver.c

  See function Ip4CreateService():

  Status = NetLibCreateServiceChild (
             Controller,
             ImageHandle,
             &gEfiManagedNetworkServiceBindingProtocolGuid,
             &IpSb->MnpChildHandle
             );

  if (EFI_ERROR (Status)) {
    goto ON_ERROR;
  }

- In both cases, the private MNP instance is extracted from MNPSB with
  the NetLibCreateServiceChild() utility function. It is implemented in
  "MdeModulePkg/Library/DxeNetLib/DxeNetLib.c".

  NetLibCreateServiceChild() is a simple function that looks up the
  requested service binding protocol first, then calls its
  CreateChild() member function to extract a private handle for the
  caller.

  See also the parallel function NetLibDestroyServiceChild().

Once you have an MNP instance that is private to GRUB, please consult
the UEFI spec (chapter 24.1, in UEFI v2.5) for the right member
functions, for configuration / transmitting / receiving. Before the
member function reference sections start, the chapter gives a good
guide-level description.

Thanks
Laszlo

Re: [grub PATCH] efinet: disable MNP background polling

[Daniel Kiper]

Hi Laszlo,

First of all, thanks a lot for very nice explanation!

On Thu, Oct 01, 2015 at 01:50:31PM +0200, Laszlo Ersek wrote:
> CC'ing Mark Salter, and edk2-devel, also updating the subject slightly
> for better context.
>
> On 10/01/15 11:26, HATAYAMA Daisuke wrote:
> > Currently, as of the commit f348aee7b33dd85e7da62b497a96a7319a0bf9dd,
> > SNP is exclusively reopened to avoid slowdown or complete failure to
> > load files due to races between MNP background polling and grub's
> > receiving and transmitting packets.
> >
> > This exclusive SNP reopen affects some EFI applications/drivers that
> > use SNP and causes PXE boot on such systems to fail. Hardware filter
> > issue fixed by the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73 is
> > one example.
>
> I think the above two commit references are in inverse order. That is,
> commit 49426e9f is the one responsible for the (needed) exclusive open,
> and commit f348aee7 fixes up the former by reconfiguring the receive
> filters.
>
> In other words, the first two paragraphs here seem accurate, just please
> reorder the commit hashes.
>
> > The difficulty here is that effects of the exclusive SNP reopen
> > differs from system to system depending their UEFI firmware
> > implementations. One system works well with the commit
> > f348aee7b33dd85e7da62b497a96a7319a0bf9dd only, another system still
> > needs the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73, and there
> > could be other systems where PXE boot still fails.
>
> Here too I think the commit hashes should be switched around.
>
> >
> > Actually, PXE boot fails on Fujitsu PRIMEQUEST 2000 series now.
> >
> > Thus, it seems to me that the exclusive SNP reopen makes grub
> > maintenance difficult by affecting a variety of systems differently.
>
> (Admittedly, this is going to be a bit of speculation:) I think the
> difference in behavior is not due to SNP implementations, but due to
> differences in higher level protocol implementations -- i.e., the
> behavior depends on what those drivers do to the underlying SNP that are
> *closed*.
>
> According to the spec of OpenProtocol(), in case of a successful
> exclusive open, the other BY_DRIVER reference is kicked off with
> DisconnectController(), which in turn calls the other driver's
> EFI_DRIVER_BINDING_PROTOCOL.Stop() function.
>
> So, the question is what that *other* (higher level) driver does in its
> Stop() function, when it unbinds the handle with the underlying SNP
> interface. Does it mess with SNP's receive filters? And so on.
>
> >
> > Instead, the idea of this patch is to disable MNP background polling
> > temporarily while grub uses SNP. Then, the race issue must be removed.
>
> This is not the right approach in my opinion.
>
> The original problem is that GRUB's direct access to SNP races with
> *several* MNP instances to the same SNP. The MNP instances are correctly
> arbitrated between each other (see more on this later), but the SNP
> accesses from GRUB are not.
>
> SNP is meant as an exclusive-access interface to the NIC, so those
> parallel clients won't work.
>
> One way to solve that is to kick out those other SNP accessors, by way
> of exclusive open. This is correct from a UEFI driver model perspective,
> but -- unless GRUB does full reconfiguration on the SNP level -- brittle
> in practice, as you've experienced; apparently different implementations
> of higher level protocols leave the SNP in different states when they go
> away. (It's perfectly possible that some of those driver binding Stop()
> functions have bugs.)
>
> However, the other way (because there is another way, yes) is different
> from interfering with existing MNP instances (note: plural). MNP (and a
> bunch of other networking related protocols) don't work like your usual
> UEFI device drivers; they are child protocols (= protocol interfaces on
> child handles) that are created *as needed* with the help of Service
> Binding protocol instances.
>
> Please read the following sections of the UEFI spec (v2.5) carefully:
>
> - 2.5.8 EFI Services Binding
> - 10.6 EFI Service Binding Protocol
> - 24.1 EFI Managed Network Protocol
>   EFI_MANAGED_NETWORK_SERVICE_BINDING_PROTOCOL

Taking into account above and sentences in UEFI spec (v2.5) like "Once the
remote image is successfully loaded, it may utilize the EFI_PXE_BASE_CODE_PROTOCOL
interfaces, or even the EFI_SIMPLE_NETWORK_PROTOCOL interfaces, to continue
the remote process." I have a feeling that UEFI spec is very imprecise how
to use SNP. I have not seen any single word saying that there are any constraints
on SNP usage (am I missing something?). I heard about that in our internal
discussions but I was not convinced that it is true until I have seen your
email with all details. So, now I think that UEFI spec should have special
paragraph saying how to play with SNP. What do you think about that?

By the way, I saw that other boot loaders like PXELINUX or iPXE use SNP.
Do you suggest that all of them should be rewritten to avoid issues with
this protocol?

Daniel

Re: [grub PATCH] efinet: disable MNP background polling

[Laszlo Ersek]

On 10/13/15 23:49, Daniel Kiper wrote:
> Hi Laszlo,
> 
> First of all, thanks a lot for very nice explanation!
> 
> On Thu, Oct 01, 2015 at 01:50:31PM +0200, Laszlo Ersek wrote:
>> CC'ing Mark Salter, and edk2-devel, also updating the subject slightly
>> for better context.
>>
>> On 10/01/15 11:26, HATAYAMA Daisuke wrote:
>>> Currently, as of the commit f348aee7b33dd85e7da62b497a96a7319a0bf9dd,
>>> SNP is exclusively reopened to avoid slowdown or complete failure to
>>> load files due to races between MNP background polling and grub's
>>> receiving and transmitting packets.
>>>
>>> This exclusive SNP reopen affects some EFI applications/drivers that
>>> use SNP and causes PXE boot on such systems to fail. Hardware filter
>>> issue fixed by the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73 is
>>> one example.
>>
>> I think the above two commit references are in inverse order. That is,
>> commit 49426e9f is the one responsible for the (needed) exclusive open,
>> and commit f348aee7 fixes up the former by reconfiguring the receive
>> filters.
>>
>> In other words, the first two paragraphs here seem accurate, just please
>> reorder the commit hashes.
>>
>>> The difficulty here is that effects of the exclusive SNP reopen
>>> differs from system to system depending their UEFI firmware
>>> implementations. One system works well with the commit
>>> f348aee7b33dd85e7da62b497a96a7319a0bf9dd only, another system still
>>> needs the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73, and there
>>> could be other systems where PXE boot still fails.
>>
>> Here too I think the commit hashes should be switched around.
>>
>>>
>>> Actually, PXE boot fails on Fujitsu PRIMEQUEST 2000 series now.
>>>
>>> Thus, it seems to me that the exclusive SNP reopen makes grub
>>> maintenance difficult by affecting a variety of systems differently.
>>
>> (Admittedly, this is going to be a bit of speculation:) I think the
>> difference in behavior is not due to SNP implementations, but due to
>> differences in higher level protocol implementations -- i.e., the
>> behavior depends on what those drivers do to the underlying SNP that are
>> *closed*.
>>
>> According to the spec of OpenProtocol(), in case of a successful
>> exclusive open, the other BY_DRIVER reference is kicked off with
>> DisconnectController(), which in turn calls the other driver's
>> EFI_DRIVER_BINDING_PROTOCOL.Stop() function.
>>
>> So, the question is what that *other* (higher level) driver does in its
>> Stop() function, when it unbinds the handle with the underlying SNP
>> interface. Does it mess with SNP's receive filters? And so on.
>>
>>>
>>> Instead, the idea of this patch is to disable MNP background polling
>>> temporarily while grub uses SNP. Then, the race issue must be removed.
>>
>> This is not the right approach in my opinion.
>>
>> The original problem is that GRUB's direct access to SNP races with
>> *several* MNP instances to the same SNP. The MNP instances are correctly
>> arbitrated between each other (see more on this later), but the SNP
>> accesses from GRUB are not.
>>
>> SNP is meant as an exclusive-access interface to the NIC, so those
>> parallel clients won't work.
>>
>> One way to solve that is to kick out those other SNP accessors, by way
>> of exclusive open. This is correct from a UEFI driver model perspective,
>> but -- unless GRUB does full reconfiguration on the SNP level -- brittle
>> in practice, as you've experienced; apparently different implementations
>> of higher level protocols leave the SNP in different states when they go
>> away. (It's perfectly possible that some of those driver binding Stop()
>> functions have bugs.)
>>
>> However, the other way (because there is another way, yes) is different
>> from interfering with existing MNP instances (note: plural). MNP (and a
>> bunch of other networking related protocols) don't work like your usual
>> UEFI device drivers; they are child protocols (= protocol interfaces on
>> child handles) that are created *as needed* with the help of Service
>> Binding protocol instances.
>>
>> Please read the following sections of the UEFI spec (v2.5) carefully:
>>
>> - 2.5.8 EFI Services Binding
>> - 10.6 EFI Service Binding Protocol
>> - 24.1 EFI Managed Network Protocol
>>   EFI_MANAGED_NETWORK_SERVICE_BINDING_PROTOCOL
> 
> Taking into account above and sentences in UEFI spec (v2.5) like "Once the
> remote image is successfully loaded, it may utilize the EFI_PXE_BASE_CODE_PROTOCOL
> interfaces, or even the EFI_SIMPLE_NETWORK_PROTOCOL interfaces, to continue
> the remote process." I have a feeling that UEFI spec is very imprecise how
> to use SNP. I have not seen any single word saying that there are any constraints
> on SNP usage (am I missing something?). I heard about that in our internal
> discussions but I was not convinced that it is true until I have seen your
> email with all details. So, now I think that UEFI spec should have special
> paragraph saying how to play with SNP. What do you think about that?

I think that a request for clarification should be filed with the USWG,
or at least raised on edk2-devel :)

> By the way, I saw that other boot loaders like PXELINUX or iPXE use SNP.
> Do you suggest that all of them should be rewritten to avoid issues with
> this protocol?

I don't know these projects overly well :), but I wouldn't suggest such
an indiscriminate rewrite for each. The main incentive for thinking
about MNP at all is that the exclusive open of SNP, which *should* work,
hangs various proprietary UEFI implementations.

When Mark Salter pinged me on IRC quite a few months back about the
original grub problem, the first thing that I recommended (although
clearly not immediately -- I had to research the spec) was the exclusive
open. Mark went ahead and implemented that, and grub started working on
the UEFI platform we had. (Then he contributed the patch(es) to upstream.)

The exclusive open is a valid (and simple) thing to do, according to the
UEFI spec. So the alternative to the elaborate MNP(SB) patch is to debug
and fix the UEFI implementations on which the current (otherwise
spec-conformant) grub code -- the exclusive open -- exposes a hang /
crash etc.

The people who are likely most interested in either fixing those
problematic UEFI implementations, *or* in converting the various open
source projects to MNP(SB), should be those that want to run the latter
on the former. :) Hatayama-san is certainly at liberty to solve the
issue either way.

Thanks!
Laszlo

> 
> Daniel
> 

Re: [grub PATCH] efinet: disable MNP background polling

[Daniel Kiper]

On Wed, Oct 14, 2015 at 12:21:29AM +0200, Laszlo Ersek wrote:
> On 10/13/15 23:49, Daniel Kiper wrote:

[...]

> > Taking into account above and sentences in UEFI spec (v2.5) like "Once the
> > remote image is successfully loaded, it may utilize the EFI_PXE_BASE_CODE_PROTOCOL
> > interfaces, or even the EFI_SIMPLE_NETWORK_PROTOCOL interfaces, to continue
> > the remote process." I have a feeling that UEFI spec is very imprecise how
> > to use SNP. I have not seen any single word saying that there are any constraints
> > on SNP usage (am I missing something?). I heard about that in our internal
> > discussions but I was not convinced that it is true until I have seen your
> > email with all details. So, now I think that UEFI spec should have special
> > paragraph saying how to play with SNP. What do you think about that?
>
> I think that a request for clarification should be filed with the USWG,
> or at least raised on edk2-devel :)

I will take care of it. Hmmm... If firmware do not conform to spec then
firmware should be fixed not spec. Anyway, I will take a look and we
will see what (if any) should be done.

> > By the way, I saw that other boot loaders like PXELINUX or iPXE use SNP.
> > Do you suggest that all of them should be rewritten to avoid issues with
> > this protocol?
>
> I don't know these projects overly well :), but I wouldn't suggest such
> an indiscriminate rewrite for each. The main incentive for thinking
> about MNP at all is that the exclusive open of SNP, which *should* work,
> hangs various proprietary UEFI implementations.
>
> When Mark Salter pinged me on IRC quite a few months back about the
> original grub problem, the first thing that I recommended (although
> clearly not immediately -- I had to research the spec) was the exclusive
> open. Mark went ahead and implemented that, and grub started working on
> the UEFI platform we had. (Then he contributed the patch(es) to upstream.)
>
> The exclusive open is a valid (and simple) thing to do, according to the
> UEFI spec. So the alternative to the elaborate MNP(SB) patch is to debug

I have the same feeling since I started work on this issue.

> and fix the UEFI implementations on which the current (otherwise

This is firmware guys work. I think that we can try to convince
some of them to fix what should be fixed.

> spec-conformant) grub code -- the exclusive open -- exposes a hang /
> crash etc.
>
> The people who are likely most interested in either fixing those
> problematic UEFI implementations, *or* in converting the various open
> source projects to MNP(SB), should be those that want to run the latter
> on the former. :) Hatayama-san is certainly at liberty to solve the
> issue either way.

I think that we should push people who write firmware which do not
conform UEFI spec. Otherwise they will not care more and more and we
will have more and more problems (not only related to network stack).

Daniel

Re: [grub PATCH] efinet: disable MNP background polling

[Seth Goldberg]

> On Oct 13, 2015, at 3:21 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 10/13/15 23:49, Daniel Kiper wrote:
>> Hi Laszlo,
>> 
>> First of all, thanks a lot for very nice explanation!
>> 
>>> On Thu, Oct 01, 2015 at 01:50:31PM +0200, Laszlo Ersek wrote:
>>> CC'ing Mark Salter, and edk2-devel, also updating the subject slightly
>>> for better context.
>>> 
>>>> On 10/01/15 11:26, HATAYAMA Daisuke wrote:
>>>> Currently, as of the commit f348aee7b33dd85e7da62b497a96a7319a0bf9dd,
>>>> SNP is exclusively reopened to avoid slowdown or complete failure to
>>>> load files due to races between MNP background polling and grub's
>>>> receiving and transmitting packets.
>>>> 
>>>> This exclusive SNP reopen affects some EFI applications/drivers that
>>>> use SNP and causes PXE boot on such systems to fail. Hardware filter
>>>> issue fixed by the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73 is
>>>> one example.
>>> 
>>> I think the above two commit references are in inverse order. That is,
>>> commit 49426e9f is the one responsible for the (needed) exclusive open,
>>> and commit f348aee7 fixes up the former by reconfiguring the receive
>>> filters.
>>> 
>>> In other words, the first two paragraphs here seem accurate, just please
>>> reorder the commit hashes.
>>> 
>>>> The difficulty here is that effects of the exclusive SNP reopen
>>>> differs from system to system depending their UEFI firmware
>>>> implementations. One system works well with the commit
>>>> f348aee7b33dd85e7da62b497a96a7319a0bf9dd only, another system still
>>>> needs the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73, and there
>>>> could be other systems where PXE boot still fails.
>>> 
>>> Here too I think the commit hashes should be switched around.
>>> 
>>>> 
>>>> Actually, PXE boot fails on Fujitsu PRIMEQUEST 2000 series now.
>>>> 
>>>> Thus, it seems to me that the exclusive SNP reopen makes grub
>>>> maintenance difficult by affecting a variety of systems differently.
>>> 
>>> (Admittedly, this is going to be a bit of speculation:) I think the
>>> difference in behavior is not due to SNP implementations, but due to
>>> differences in higher level protocol implementations -- i.e., the
>>> behavior depends on what those drivers do to the underlying SNP that are
>>> *closed*.
>>> 
>>> According to the spec of OpenProtocol(), in case of a successful
>>> exclusive open, the other BY_DRIVER reference is kicked off with
>>> DisconnectController(), which in turn calls the other driver's
>>> EFI_DRIVER_BINDING_PROTOCOL.Stop() function.
>>> 
>>> So, the question is what that *other* (higher level) driver does in its
>>> Stop() function, when it unbinds the handle with the underlying SNP
>>> interface. Does it mess with SNP's receive filters? And so on.
>>> 
>>>> 
>>>> Instead, the idea of this patch is to disable MNP background polling
>>>> temporarily while grub uses SNP. Then, the race issue must be removed.
>>> 
>>> This is not the right approach in my opinion.
>>> 
>>> The original problem is that GRUB's direct access to SNP races with
>>> *several* MNP instances to the same SNP. The MNP instances are correctly
>>> arbitrated between each other (see more on this later), but the SNP
>>> accesses from GRUB are not.
>>> 
>>> SNP is meant as an exclusive-access interface to the NIC, so those
>>> parallel clients won't work.
>>> 
>>> One way to solve that is to kick out those other SNP accessors, by way
>>> of exclusive open. This is correct from a UEFI driver model perspective,
>>> but -- unless GRUB does full reconfiguration on the SNP level -- brittle
>>> in practice, as you've experienced; apparently different implementations
>>> of higher level protocols leave the SNP in different states when they go
>>> away. (It's perfectly possible that some of those driver binding Stop()
>>> functions have bugs.)
>>> 
>>> However, the other way (because there is another way, yes) is different
>>> from interfering with existing MNP instances (note: plural). MNP (and a
>>> bunch of other networking related protocols) don't work like your usual
>>> UEFI device drivers; they are child protocols (= protocol interfaces on
>>> child handles) that are created *as needed* with the help of Service
>>> Binding protocol instances.
>>> 
>>> Please read the following sections of the UEFI spec (v2.5) carefully:
>>> 
>>> - 2.5.8 EFI Services Binding
>>> - 10.6 EFI Service Binding Protocol
>>> - 24.1 EFI Managed Network Protocol
>>>  EFI_MANAGED_NETWORK_SERVICE_BINDING_PROTOCOL
>> 
>> Taking into account above and sentences in UEFI spec (v2.5) like "Once the
>> remote image is successfully loaded, it may utilize the EFI_PXE_BASE_CODE_PROTOCOL
>> interfaces, or even the EFI_SIMPLE_NETWORK_PROTOCOL interfaces, to continue
>> the remote process." I have a feeling that UEFI spec is very imprecise how
>> to use SNP. I have not seen any single word saying that there are any constraints
>> on SNP usage (am I missing something?). I heard about that in our internal
>> discussions but I was not convinced that it is true until I have seen your
>> email with all details. So, now I think that UEFI spec should have special
>> paragraph saying how to play with SNP. What do you think about that?
> 
> I think that a request for clarification should be filed with the USWG,
> or at least raised on edk2-devel :)
> 
>> By the way, I saw that other boot loaders like PXELINUX or iPXE use SNP.
>> Do you suggest that all of them should be rewritten to avoid issues with
>> this protocol?
> 
> I don't know these projects overly well :), but I wouldn't suggest such
> an indiscriminate rewrite for each. The main incentive for thinking
> about MNP at all is that the exclusive open of SNP, which *should* work,
> hangs various proprietary UEFI implementations.
> 
> When Mark Salter pinged me on IRC quite a few months back about the
> original grub problem, the first thing that I recommended (although
> clearly not immediately -- I had to research the spec) was the exclusive
> open. Mark went ahead and implemented that, and grub started working on
> the UEFI platform we had. (Then he contributed the patch(es) to upstream.)
> 
> The exclusive open is a valid (and simple) thing to do, according to the
> UEFI spec. So the alternative to the elaborate MNP(SB) patch is to debug
> and fix the UEFI implementations on which the current (otherwise
> spec-conformant) grub code -- the exclusive open -- exposes a hang /
> crash etc.
> 
> The people who are likely most interested in either fixing those
> problematic UEFI implementations, *or* in converting the various open
> source projects to MNP(SB), should be those that want to run the latter
> on the former. :) Hatayama-san is certainly at liberty to solve the
> issue either way.

  The folks I've talked to at Intel say that the bug is that the snp exclusive open should fail if any other consumer has already opened snp, and that is the case with existing implementations (that is, mnp has already opened snp by the time grub is loaded and running).  If that bug were fixed, the grub snp driver would always fail which would make it pretty obvious that mnp is the way to go. 

  --S



> 
> Thanks!
> Laszlo
> 
>> 
>> Daniel
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

RE: [edk2] [grub PATCH] efinet: disable MNP background polling

[Ye, Ting]

Hi all,

If I understand the issue correctly, I don't quite agree that UEFI spec is imprecise about SNP constraints described as following.
The "constraint" described here is that the grub should use attribute "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol(). 

EXCLUSIVE		Used by applications to gain exclusive access to a protocol interface. 
			If any drivers have the protocol interface opened with an attribute of BY_DRIVER, 
			then an attempt will be made to remove them by calling the driver's Stop() function.

The grub code should not assume that the SNP is not occupied by other drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to be more precise, use OpenProtocolInformation() to check whether SNP is already opened by other driver, then decide whether need to use EXCLUSIVE to disconnect the other drivers. This is the typical usage for all UEFI protocol, not particular constraints to SNP protocol.


> Taking into account above and sentences in UEFI spec (v2.5) like "Once the
> remote image is successfully loaded, it may utilize the EFI_PXE_BASE_CODE_PROTOCOL
> interfaces, or even the EFI_SIMPLE_NETWORK_PROTOCOL interfaces, to continue
> the remote process." I have a feeling that UEFI spec is very imprecise how
> to use SNP. I have not seen any single word saying that there are any constraints
> on SNP usage (am I missing something?). I heard about that in our internal
> discussions but I was not convinced that it is true until I have seen your
> email with all details. So, now I think that UEFI spec should have special
> paragraph saying how to play with SNP. What do you think about that?

I think that a request for clarification should be filed with the USWG,
or at least raised on edk2-devel :)


Best Regards,
Ye Ting

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Wednesday, October 14, 2015 6:21 AM
To: daniel.kiper@oracle.com; grub-devel@gnu.org
Cc: konrad.wilk@oracle.com; arvidjaar@gmail.com; edk2-devel-01; glin@suse.com; seth.goldberg@oracle.com; Mark Salter
Subject: Re: [edk2] [grub PATCH] efinet: disable MNP background polling

On 10/13/15 23:49, Daniel Kiper wrote:
> Hi Laszlo,
> 
> First of all, thanks a lot for very nice explanation!
> 
> On Thu, Oct 01, 2015 at 01:50:31PM +0200, Laszlo Ersek wrote:
>> CC'ing Mark Salter, and edk2-devel, also updating the subject slightly
>> for better context.
>>
>> On 10/01/15 11:26, HATAYAMA Daisuke wrote:
>>> Currently, as of the commit f348aee7b33dd85e7da62b497a96a7319a0bf9dd,
>>> SNP is exclusively reopened to avoid slowdown or complete failure to
>>> load files due to races between MNP background polling and grub's
>>> receiving and transmitting packets.
>>>
>>> This exclusive SNP reopen affects some EFI applications/drivers that
>>> use SNP and causes PXE boot on such systems to fail. Hardware filter
>>> issue fixed by the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73 is
>>> one example.
>>
>> I think the above two commit references are in inverse order. That is,
>> commit 49426e9f is the one responsible for the (needed) exclusive open,
>> and commit f348aee7 fixes up the former by reconfiguring the receive
>> filters.
>>
>> In other words, the first two paragraphs here seem accurate, just please
>> reorder the commit hashes.
>>
>>> The difficulty here is that effects of the exclusive SNP reopen
>>> differs from system to system depending their UEFI firmware
>>> implementations. One system works well with the commit
>>> f348aee7b33dd85e7da62b497a96a7319a0bf9dd only, another system still
>>> needs the commit 49426e9fd2e562c73a4f1206f32eff9e424a1a73, and there
>>> could be other systems where PXE boot still fails.
>>
>> Here too I think the commit hashes should be switched around.
>>
>>>
>>> Actually, PXE boot fails on Fujitsu PRIMEQUEST 2000 series now.
>>>
>>> Thus, it seems to me that the exclusive SNP reopen makes grub
>>> maintenance difficult by affecting a variety of systems differently.
>>
>> (Admittedly, this is going to be a bit of speculation:) I think the
>> difference in behavior is not due to SNP implementations, but due to
>> differences in higher level protocol implementations -- i.e., the
>> behavior depends on what those drivers do to the underlying SNP that are
>> *closed*.
>>
>> According to the spec of OpenProtocol(), in case of a successful
>> exclusive open, the other BY_DRIVER reference is kicked off with
>> DisconnectController(), which in turn calls the other driver's
>> EFI_DRIVER_BINDING_PROTOCOL.Stop() function.
>>
>> So, the question is what that *other* (higher level) driver does in its
>> Stop() function, when it unbinds the handle with the underlying SNP
>> interface. Does it mess with SNP's receive filters? And so on.
>>
>>>
>>> Instead, the idea of this patch is to disable MNP background polling
>>> temporarily while grub uses SNP. Then, the race issue must be removed.
>>
>> This is not the right approach in my opinion.
>>
>> The original problem is that GRUB's direct access to SNP races with
>> *several* MNP instances to the same SNP. The MNP instances are correctly
>> arbitrated between each other (see more on this later), but the SNP
>> accesses from GRUB are not.
>>
>> SNP is meant as an exclusive-access interface to the NIC, so those
>> parallel clients won't work.
>>
>> One way to solve that is to kick out those other SNP accessors, by way
>> of exclusive open. This is correct from a UEFI driver model perspective,
>> but -- unless GRUB does full reconfiguration on the SNP level -- brittle
>> in practice, as you've experienced; apparently different implementations
>> of higher level protocols leave the SNP in different states when they go
>> away. (It's perfectly possible that some of those driver binding Stop()
>> functions have bugs.)
>>
>> However, the other way (because there is another way, yes) is different
>> from interfering with existing MNP instances (note: plural). MNP (and a
>> bunch of other networking related protocols) don't work like your usual
>> UEFI device drivers; they are child protocols (= protocol interfaces on
>> child handles) that are created *as needed* with the help of Service
>> Binding protocol instances.
>>
>> Please read the following sections of the UEFI spec (v2.5) carefully:
>>
>> - 2.5.8 EFI Services Binding
>> - 10.6 EFI Service Binding Protocol
>> - 24.1 EFI Managed Network Protocol
>>   EFI_MANAGED_NETWORK_SERVICE_BINDING_PROTOCOL
> 
> Taking into account above and sentences in UEFI spec (v2.5) like "Once the
> remote image is successfully loaded, it may utilize the EFI_PXE_BASE_CODE_PROTOCOL
> interfaces, or even the EFI_SIMPLE_NETWORK_PROTOCOL interfaces, to continue
> the remote process." I have a feeling that UEFI spec is very imprecise how
> to use SNP. I have not seen any single word saying that there are any constraints
> on SNP usage (am I missing something?). I heard about that in our internal
> discussions but I was not convinced that it is true until I have seen your
> email with all details. So, now I think that UEFI spec should have special
> paragraph saying how to play with SNP. What do you think about that?

I think that a request for clarification should be filed with the USWG,
or at least raised on edk2-devel :)

> By the way, I saw that other boot loaders like PXELINUX or iPXE use SNP.
> Do you suggest that all of them should be rewritten to avoid issues with
> this protocol?

I don't know these projects overly well :), but I wouldn't suggest such
an indiscriminate rewrite for each. The main incentive for thinking
about MNP at all is that the exclusive open of SNP, which *should* work,
hangs various proprietary UEFI implementations.

When Mark Salter pinged me on IRC quite a few months back about the
original grub problem, the first thing that I recommended (although
clearly not immediately -- I had to research the spec) was the exclusive
open. Mark went ahead and implemented that, and grub started working on
the UEFI platform we had. (Then he contributed the patch(es) to upstream.)

The exclusive open is a valid (and simple) thing to do, according to the
UEFI spec. So the alternative to the elaborate MNP(SB) patch is to debug
and fix the UEFI implementations on which the current (otherwise
spec-conformant) grub code -- the exclusive open -- exposes a hang /
crash etc.

The people who are likely most interested in either fixing those
problematic UEFI implementations, *or* in converting the various open
source projects to MNP(SB), should be those that want to run the latter
on the former. :) Hatayama-san is certainly at liberty to solve the
issue either way.

Thanks!
Laszlo

> 
> Daniel
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [grub PATCH] efinet: disable MNP background polling

[Andrei Borzenkov]

14.10.2015 08:19, Ye, Ting пишет:
> Hi all,
>
> If I understand the issue correctly, I don't quite agree that UEFI spec is imprecise about SNP constraints described as following.
> The "constraint" described here is that the grub should use attribute "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>
> EXCLUSIVE		Used by applications to gain exclusive access to a protocol interface.
> 			If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
> 			then an attempt will be made to remove them by calling the driver's Stop() function.
>
> The grub code should not assume that the SNP is not occupied by other drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to be more precise, use OpenProtocolInformation() to check whether SNP is already opened by other driver, then decide whether need to use EXCLUSIVE to disconnect the other drivers. This is the typical usage for all UEFI protocol, not particular constraints to SNP protocol.
>

That is exactly what grub currently does - it opens SNP exclusively. 
Apparently it is causing problems in some cases.

RE: [edk2] [grub PATCH] efinet: disable MNP background polling

[Ye, Ting]

May I know the details what problems it causes in some cases? 

Thanks,
Ye Ting

-----Original Message-----
From: Andrei Borzenkov [mailto:arvidjaar@gmail.com] 
Sent: Wednesday, October 14, 2015 1:58 PM
To: Ye, Ting; Laszlo Ersek; daniel.kiper@oracle.com; grub-devel@gnu.org
Cc: konrad.wilk@oracle.com; edk2-devel-01; glin@suse.com; seth.goldberg@oracle.com; Mark Salter
Subject: Re: [edk2] [grub PATCH] efinet: disable MNP background polling

14.10.2015 08:19, Ye, Ting пишет:
> Hi all,
>
> If I understand the issue correctly, I don't quite agree that UEFI spec is imprecise about SNP constraints described as following.
> The "constraint" described here is that the grub should use attribute "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>
> EXCLUSIVE		Used by applications to gain exclusive access to a protocol interface.
> 			If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
> 			then an attempt will be made to remove them by calling the driver's Stop() function.
>
> The grub code should not assume that the SNP is not occupied by other drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to be more precise, use OpenProtocolInformation() to check whether SNP is already opened by other driver, then decide whether need to use EXCLUSIVE to disconnect the other drivers. This is the typical usage for all UEFI protocol, not particular constraints to SNP protocol.
>

That is exactly what grub currently does - it opens SNP exclusively. 
Apparently it is causing problems in some cases.

Re: [edk2] [grub PATCH] efinet: disable MNP background polling

[Andrei Borzenkov]

14.10.2015 09:15, Ye, Ting пишет:
> May I know the details what problems it causes in some cases?
>

One is being discussed in this thread:

http://lists.gnu.org/archive/html/grub-devel/2015-10/msg00013.html
http://lists.gnu.org/archive/html/grub-devel/2015-10/msg00068.html

Another was reported recently:

http://lists.gnu.org/archive/html/help-grub/2015-09/msg00033.html
http://lists.gnu.org/archive/html/grub-devel/2015-10/msg00071.html


> Thanks,
> Ye Ting
>
> -----Original Message-----
> From: Andrei Borzenkov [mailto:arvidjaar@gmail.com]
> Sent: Wednesday, October 14, 2015 1:58 PM
> To: Ye, Ting; Laszlo Ersek; daniel.kiper@oracle.com; grub-devel@gnu.org
> Cc: konrad.wilk@oracle.com; edk2-devel-01; glin@suse.com; seth.goldberg@oracle.com; Mark Salter
> Subject: Re: [edk2] [grub PATCH] efinet: disable MNP background polling
>
> 14.10.2015 08:19, Ye, Ting пишет:
>> Hi all,
>>
>> If I understand the issue correctly, I don't quite agree that UEFI spec is imprecise about SNP constraints described as following.
>> The "constraint" described here is that the grub should use attribute "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>>
>> EXCLUSIVE		Used by applications to gain exclusive access to a protocol interface.
>> 			If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
>> 			then an attempt will be made to remove them by calling the driver's Stop() function.
>>
>> The grub code should not assume that the SNP is not occupied by other drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to be more precise, use OpenProtocolInformation() to check whether SNP is already opened by other driver, then decide whether need to use EXCLUSIVE to disconnect the other drivers. This is the typical usage for all UEFI protocol, not particular constraints to SNP protocol.
>>
>
> That is exactly what grub currently does - it opens SNP exclusively.
> Apparently it is causing problems in some cases.
>

RE: [edk2] [grub PATCH] efinet: disable MNP background polling

[Ye, Ting]

Could you please describe the details how does GRUB use UEFI network protocols?

I see the thread says that EXCLUSIVE open SNP causes PXE boot fail. 

If you read below description about 'EXCLUSIVE' in UEFI specification, you will find when GRUB exclusive opens SNP protocol, the UEFI will remove any drivers that opened SNP with BY_DRIVER by calling the driver's Stop() function. In UEFI network stack, MNP driver will open SNP 'BY_DRIVER'. So if GRUB/iPXE exclusive opens SNP, MNP will uninstall itself and the whole UEFI network stack is disconnected except SNP and UNDI. Hence, the UEFI PXE no longer work.


>> EXCLUSIVE		Used by applications to gain exclusive access to a protocol interface.
>> 			If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
>> 			then an attempt will be made to remove them by calling the driver's Stop() function.


If GRUB would like to call SNP protocol only, it should use EXCLUSIVE open to gain the exclusive access and disconnect UEFI network stack since MNP. Then the MNP background polling is also disabled.
If GRUB still needs UEFI PXE capability, it should utilize PXE base code protocol to continue the process, rather than calling SNP, as SNP is already consumed by UEFI network stack.

Thanks,
Ye Ting


-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Andrei Borzenkov
Sent: Wednesday, October 14, 2015 2:58 PM
To: Ye, Ting; Laszlo Ersek; daniel.kiper@oracle.com; grub-devel@gnu.org
Cc: edk2-devel-01; Mark Salter; glin@suse.com; seth.goldberg@oracle.com; konrad.wilk@oracle.com
Subject: Re: [edk2] [grub PATCH] efinet: disable MNP background polling

14.10.2015 09:15, Ye, Ting пишет:
> May I know the details what problems it causes in some cases?
>

One is being discussed in this thread:

http://lists.gnu.org/archive/html/grub-devel/2015-10/msg00013.html
http://lists.gnu.org/archive/html/grub-devel/2015-10/msg00068.html

Another was reported recently:

http://lists.gnu.org/archive/html/help-grub/2015-09/msg00033.html
http://lists.gnu.org/archive/html/grub-devel/2015-10/msg00071.html


> Thanks,
> Ye Ting
>
> -----Original Message-----
> From: Andrei Borzenkov [mailto:arvidjaar@gmail.com]
> Sent: Wednesday, October 14, 2015 1:58 PM
> To: Ye, Ting; Laszlo Ersek; daniel.kiper@oracle.com; grub-devel@gnu.org
> Cc: konrad.wilk@oracle.com; edk2-devel-01; glin@suse.com; seth.goldberg@oracle.com; Mark Salter
> Subject: Re: [edk2] [grub PATCH] efinet: disable MNP background polling
>
> 14.10.2015 08:19, Ye, Ting пишет:
>> Hi all,
>>
>> If I understand the issue correctly, I don't quite agree that UEFI spec is imprecise about SNP constraints described as following.
>> The "constraint" described here is that the grub should use attribute "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>>
>> EXCLUSIVE		Used by applications to gain exclusive access to a protocol interface.
>> 			If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
>> 			then an attempt will be made to remove them by calling the driver's Stop() function.
>>
>> The grub code should not assume that the SNP is not occupied by other drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to be more precise, use OpenProtocolInformation() to check whether SNP is already opened by other driver, then decide whether need to use EXCLUSIVE to disconnect the other drivers. This is the typical usage for all UEFI protocol, not particular constraints to SNP protocol.
>>
>
> That is exactly what grub currently does - it opens SNP exclusively.
> Apparently it is causing problems in some cases.
>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [grub PATCH] efinet: disable MNP background polling

[Andrei Borzenkov]

14.10.2015 11:00, Ye, Ting пишет:
> Could you please describe the details how does GRUB use UEFI network protocols?
>

When efinet driver is loaded it enumerates handles with SNP; these 
handles represent network cards for grub. If driver is part of initial 
core.img (default for network boot image) it additionally queries loaded 
image handle for PXE for DhcpAck using EFI_PXE_BASE_CODE_PROTOCOL and is 
using it for autoconfiguration. Before the first send/receive request on 
network card SNP on associated handle is opened exclusively and SNP 
Transmit/Receive are used.

> I see the thread says that EXCLUSIVE open SNP causes PXE boot fail.
>
> If you read below description about 'EXCLUSIVE' in UEFI specification, you will find when GRUB exclusive opens SNP protocol, the UEFI will remove any drivers that opened SNP with BY_DRIVER by calling the driver's Stop() function. In UEFI network stack, MNP driver will open SNP 'BY_DRIVER'. So if GRUB/iPXE exclusive opens SNP, MNP will uninstall itself and the whole UEFI network stack is disconnected except SNP and UNDI. Hence, the UEFI PXE no longer work.
>
>
>>> EXCLUSIVE		Used by applications to gain exclusive access to a protocol interface.
>>> 			If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
>>> 			then an attempt will be made to remove them by calling the driver's Stop() function.
>
>
> If GRUB would like to call SNP protocol only, it should use EXCLUSIVE open to gain the exclusive access and disconnect UEFI network stack since MNP. Then the MNP background polling is also disabled.

It does it already.

> If GRUB still needs UEFI PXE capability, it should utilize PXE base code protocol to continue the process, rather than calling SNP, as SNP is already consumed by UEFI network stack.
>

It does not use PXE after initial autoconfiguration as described above 
nor does it need it. All data transmission is performed using SNP only.

> Thanks,
> Ye Ting
>
>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Andrei Borzenkov
> Sent: Wednesday, October 14, 2015 2:58 PM
> To: Ye, Ting; Laszlo Ersek; daniel.kiper@oracle.com; grub-devel@gnu.org
> Cc: edk2-devel-01; Mark Salter; glin@suse.com; seth.goldberg@oracle.com; konrad.wilk@oracle.com
> Subject: Re: [edk2] [grub PATCH] efinet: disable MNP background polling
>
> 14.10.2015 09:15, Ye, Ting пишет:
>> May I know the details what problems it causes in some cases?
>>
>
> One is being discussed in this thread:
>
> http://lists.gnu.org/archive/html/grub-devel/2015-10/msg00013.html
> http://lists.gnu.org/archive/html/grub-devel/2015-10/msg00068.html
>
> Another was reported recently:
>
> http://lists.gnu.org/archive/html/help-grub/2015-09/msg00033.html
> http://lists.gnu.org/archive/html/grub-devel/2015-10/msg00071.html
>
>
>> Thanks,
>> Ye Ting
>>
>> -----Original Message-----
>> From: Andrei Borzenkov [mailto:arvidjaar@gmail.com]
>> Sent: Wednesday, October 14, 2015 1:58 PM
>> To: Ye, Ting; Laszlo Ersek; daniel.kiper@oracle.com; grub-devel@gnu.org
>> Cc: konrad.wilk@oracle.com; edk2-devel-01; glin@suse.com; seth.goldberg@oracle.com; Mark Salter
>> Subject: Re: [edk2] [grub PATCH] efinet: disable MNP background polling
>>
>> 14.10.2015 08:19, Ye, Ting пишет:
>>> Hi all,
>>>
>>> If I understand the issue correctly, I don't quite agree that UEFI spec is imprecise about SNP constraints described as following.
>>> The "constraint" described here is that the grub should use attribute "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>>>
>>> EXCLUSIVE		Used by applications to gain exclusive access to a protocol interface.
>>> 			If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
>>> 			then an attempt will be made to remove them by calling the driver's Stop() function.
>>>
>>> The grub code should not assume that the SNP is not occupied by other drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to be more precise, use OpenProtocolInformation() to check whether SNP is already opened by other driver, then decide whether need to use EXCLUSIVE to disconnect the other drivers. This is the typical usage for all UEFI protocol, not particular constraints to SNP protocol.
>>>
>>
>> That is exactly what grub currently does - it opens SNP exclusively.
>> Apparently it is causing problems in some cases.
>>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>

Re: [edk2] [grub PATCH] efinet: disable MNP background polling

[Daniel Kiper]

On Wed, Oct 14, 2015 at 05:19:32AM +0000, Ye, Ting wrote:
> Hi all,
>
> If I understand the issue correctly, I don't quite agree that UEFI
> spec is imprecise about SNP constraints described as following.
> The "constraint" described here is that the grub should use attribute
> "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage
> is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>
> EXCLUSIVE		Used by applications to gain exclusive access to a protocol interface.
> 			If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
> 			then an attempt will be made to remove them by calling the driver's Stop() function.
>
> The grub code should not assume that the SNP is not occupied by other
> drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to
> be more precise, use OpenProtocolInformation() to check whether SNP is
> already opened by other driver, then decide whether need to use EXCLUSIVE
> to disconnect the other drivers. This is the typical usage for all UEFI
> protocol, not particular constraints to SNP protocol.

Looks good! Great! However, it looks that we still have a problem if somebody
opens SNP in EXCLUSIVE mode. Then GRUB2 SNP open will fail according to UEFI spec.
Sadly we do not have a control on other stuff and one day our approach may fail
because somebody decided to open SNP in EXCLUSIVE mode in e.g. a driver. Does
it mean that migration to MNP is one sensible solution for our problems? As I know
this is huge overhaul, so, we should think twice before choosing that way.

Daniel

Re: [edk2] [grub PATCH] efinet: disable MNP background polling

[Seth Goldberg]

> On Oct 14, 2015, at 4:08 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
>> On Wed, Oct 14, 2015 at 05:19:32AM +0000, Ye, Ting wrote:
>> Hi all,
>> 
>> If I understand the issue correctly, I don't quite agree that UEFI
>> spec is imprecise about SNP constraints described as following.
>> The "constraint" described here is that the grub should use attribute
>> "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage
>> is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>> 
>> EXCLUSIVE        Used by applications to gain exclusive access to a protocol interface.
>>            If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
>>            then an attempt will be made to remove them by calling the driver's Stop() function.
>> 
>> The grub code should not assume that the SNP is not occupied by other
>> drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to
>> be more precise, use OpenProtocolInformation() to check whether SNP is
>> already opened by other driver, then decide whether need to use EXCLUSIVE
>> to disconnect the other drivers. This is the typical usage for all UEFI
>> protocol, not particular constraints to SNP protocol.
> 
> Looks good! Great! However, it looks that we still have a problem if somebody
> opens SNP in EXCLUSIVE mode. Then GRUB2 SNP open will fail according to UEFI spec.
> Sadly we do not have a control on other stuff and one day our approach may fail
> because somebody decided to open SNP in EXCLUSIVE mode in e.g. a driver. Does
> it mean that migration to MNP is one sensible solution for our problems? As I know
> this is huge overhaul, so, we should think twice before choosing that way.


   Then it is fortunate that when I wrote the MNP implementation that we ship with Oracle Solaris 11.2, that I tested it on many thousands of systems as well as on new UEFI implementations at the UEFI Plugfest ;).

  --S



> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

RE: [edk2] [grub PATCH] efinet: disable MNP background polling

[Ye, Ting]

So the current problem is:
 GRUB wants to EXCLUSIVE open SNP, though if other application/driver already opens SNP with EXCLUSIVE attribute, the GRUB would fail. According to UEFI spec V2.5 page 182, 
	If Attributes is BY_DRIVER , BY_DRIVER|EXCLUSIVE, or EXCLUSIVE, and there are any items on the open list of the protocol interface with an attribute of EXCLUSIVE or BY_DRIVER|EXCLUSIVE, then EFI_ACCESS_DENIED is returned.

 My question is: who will EXCLUSIVE open SNP before GRUB? Why it EXCLUSIVE opens SNP and NOT close SNP protocol before handover to GRUB? 
	 For information, the MNP driver in UEFI network stack will open SNP with attribute 'BY_DRIVER', without EXCLUSIVE.

In my opinion, if it is a bug in other stuff GRUB can't handle, and GRUB needs to EXCLUSIVE open SNP, one alternative is the GRUG uses OpenProtocolInformation() to retrieve the list of agents that currently EXCLUSIVE opened SNP, then calls CloseProtocol() to close the opened protocol. If it is the case that GRUB and 'other stuff' both need the network operation and would like to keep EXCLUSIVE open SNP by intention, a MNP solution should be involved since SNP can't support multiple access to use the network interface at the same time.


Best Regards,
Ye Ting


-----Original Message-----
From: Seth Goldberg [mailto:seth.goldberg@oracle.com] 
Sent: Wednesday, October 14, 2015 11:39 PM
To: The development of GNU GRUB
Cc: Ye, Ting; arvidjaar@gmail.com; edk2-devel-01; glin@suse.com; Mark Salter; Laszlo Ersek
Subject: Re: [edk2] [grub PATCH] efinet: disable MNP background polling



> On Oct 14, 2015, at 4:08 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> 
>> On Wed, Oct 14, 2015 at 05:19:32AM +0000, Ye, Ting wrote:
>> Hi all,
>> 
>> If I understand the issue correctly, I don't quite agree that UEFI
>> spec is imprecise about SNP constraints described as following.
>> The "constraint" described here is that the grub should use attribute
>> "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage
>> is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>> 
>> EXCLUSIVE        Used by applications to gain exclusive access to a protocol interface.
>>            If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
>>            then an attempt will be made to remove them by calling the driver's Stop() function.
>> 
>> The grub code should not assume that the SNP is not occupied by other
>> drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to
>> be more precise, use OpenProtocolInformation() to check whether SNP is
>> already opened by other driver, then decide whether need to use EXCLUSIVE
>> to disconnect the other drivers. This is the typical usage for all UEFI
>> protocol, not particular constraints to SNP protocol.
> 
> Looks good! Great! However, it looks that we still have a problem if somebody
> opens SNP in EXCLUSIVE mode. Then GRUB2 SNP open will fail according to UEFI spec.
> Sadly we do not have a control on other stuff and one day our approach may fail
> because somebody decided to open SNP in EXCLUSIVE mode in e.g. a driver. Does
> it mean that migration to MNP is one sensible solution for our problems? As I know
> this is huge overhaul, so, we should think twice before choosing that way.


   Then it is fortunate that when I wrote the MNP implementation that we ship with Oracle Solaris 11.2, that I tested it on many thousands of systems as well as on new UEFI implementations at the UEFI Plugfest ;).

  --S



> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [edk2] [grub PATCH] efinet: disable MNP background polling

[Laszlo Ersek]

On 10/15/15 04:11, Ye, Ting wrote:
> So the current problem is:
>  GRUB wants to EXCLUSIVE open SNP, though if other application/driver already opens SNP with EXCLUSIVE attribute, the GRUB would fail. According to UEFI spec V2.5 page 182, 
> 	If Attributes is BY_DRIVER , BY_DRIVER|EXCLUSIVE, or EXCLUSIVE, and there are any items on the open list of the protocol interface with an attribute of EXCLUSIVE or BY_DRIVER|EXCLUSIVE, then EFI_ACCESS_DENIED is returned.
> 
>  My question is: who will EXCLUSIVE open SNP before GRUB? Why it EXCLUSIVE opens SNP and NOT close SNP protocol before handover to GRUB?

Right; when an app is done using the SNP instance and intends to pass
control to another app for good, it should close the protocol first --
same as it is expected to release memory.

... I wonder if these problems are rooted in an "outdated" pre-OS view
of system resources. I assume that before UEFI, pre-OS applications used
to think to own all of those resources, and no real life-cycle
management was done. I don't know if that's the case, but if it is, it's
not compatible with UEFI. With UEFI in the picture, there are resources
that need to be tracked and handled cooperatively between unrelated /
independent applications. Each single app needs to be prudent about
resource management.

Laszlo

> 	 For information, the MNP driver in UEFI network stack will open SNP with attribute 'BY_DRIVER', without EXCLUSIVE.
> 
> In my opinion, if it is a bug in other stuff GRUB can't handle, and
> GRUB needs to EXCLUSIVE open SNP, one alternative is the GRUG uses
> OpenProtocolInformation() to retrieve the list of agents that
> currently EXCLUSIVE opened SNP, then calls CloseProtocol() to close
> the opened protocol. If it is the case that GRUB and 'other stuff'
> both need the network operation and would like to keep EXCLUSIVE open
> SNP by intention, a MNP solution should be involved since SNP can't
> support multiple access to use the network interface at the same
> time.

> 
> 
> Best Regards,
> Ye Ting
> 
> 
> -----Original Message-----
> From: Seth Goldberg [mailto:seth.goldberg@oracle.com] 
> Sent: Wednesday, October 14, 2015 11:39 PM
> To: The development of GNU GRUB
> Cc: Ye, Ting; arvidjaar@gmail.com; edk2-devel-01; glin@suse.com; Mark Salter; Laszlo Ersek
> Subject: Re: [edk2] [grub PATCH] efinet: disable MNP background polling
> 
> 
> 
>> On Oct 14, 2015, at 4:08 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>
>>> On Wed, Oct 14, 2015 at 05:19:32AM +0000, Ye, Ting wrote:
>>> Hi all,
>>>
>>> If I understand the issue correctly, I don't quite agree that UEFI
>>> spec is imprecise about SNP constraints described as following.
>>> The "constraint" described here is that the grub should use attribute
>>> "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage
>>> is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>>>
>>> EXCLUSIVE        Used by applications to gain exclusive access to a protocol interface.
>>>            If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
>>>            then an attempt will be made to remove them by calling the driver's Stop() function.
>>>
>>> The grub code should not assume that the SNP is not occupied by other
>>> drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to
>>> be more precise, use OpenProtocolInformation() to check whether SNP is
>>> already opened by other driver, then decide whether need to use EXCLUSIVE
>>> to disconnect the other drivers. This is the typical usage for all UEFI
>>> protocol, not particular constraints to SNP protocol.
>>
>> Looks good! Great! However, it looks that we still have a problem if somebody
>> opens SNP in EXCLUSIVE mode. Then GRUB2 SNP open will fail according to UEFI spec.
>> Sadly we do not have a control on other stuff and one day our approach may fail
>> because somebody decided to open SNP in EXCLUSIVE mode in e.g. a driver. Does
>> it mean that migration to MNP is one sensible solution for our problems? As I know
>> this is huge overhaul, so, we should think twice before choosing that way.
> 
> 
>    Then it is fortunate that when I wrote the MNP implementation that we ship with Oracle Solaris 11.2, that I tested it on many thousands of systems as well as on new UEFI implementations at the UEFI Plugfest ;).
> 
>   --S
> 
> 
> 
>>
>> Daniel
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [edk2] [grub PATCH] efinet: disable MNP background polling

[Andrew Fish]

> On Oct 15, 2015, at 11:14 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 10/15/15 04:11, Ye, Ting wrote:
>> So the current problem is:
>> GRUB wants to EXCLUSIVE open SNP, though if other application/driver already opens SNP with EXCLUSIVE attribute, the GRUB would fail. According to UEFI spec V2.5 page 182, 
>> 	If Attributes is BY_DRIVER , BY_DRIVER|EXCLUSIVE, or EXCLUSIVE, and there are any items on the open list of the protocol interface with an attribute of EXCLUSIVE or BY_DRIVER|EXCLUSIVE, then EFI_ACCESS_DENIED is returned.
>> 
>> My question is: who will EXCLUSIVE open SNP before GRUB? Why it EXCLUSIVE opens SNP and NOT close SNP protocol before handover to GRUB?
> 
> Right; when an app is done using the SNP instance and intends to pass
> control to another app for good, it should close the protocol first --
> same as it is expected to release memory.
> 
> ... I wonder if these problems are rooted in an "outdated" pre-OS view
> of system resources. I assume that before UEFI, pre-OS applications used
> to think to own all of those resources, and no real life-cycle
> management was done. I don't know if that's the case, but if it is, it's
> not compatible with UEFI. With UEFI in the picture, there are resources
> that need to be tracked and handled cooperatively between unrelated /
> independent applications. Each single app needs to be prudent about
> resource management.
> 

From an EFI history point of view EXCLUSIVE was not intended for “every day” use.  EXCLUSIVE exists to solve the problem of how do you format a hard drive, or run diagnostics on a hard drive that could break the file system driver etc. 

The EFI Driver Model, lets you connect and disconnect, drivers as needed. The EFI networking stack supports the EFI Manged Network Protocol to help manage the network stack configuration. This is what was intended for normal operation. 

I’m just guessing but the iPXE developers have to deal with multiple environments (Legacy BIOS, EFI, …) and probably picked a path that allowed all these “worlds” to have a similar code flow. So the EFI OpenProtocol  EXCLUSIVE probably mapped into the other flows where iPXE just takes over. If you are maintaining a complex networking stack (iPXE) trying to make it as common as possible in all its various ports does not seem like an unreasonable goal, but you might not end up with the ideal implementation for each environment. 

Thanks,

Andrew Fish

> Laszlo
> 
>> 	 For information, the MNP driver in UEFI network stack will open SNP with attribute 'BY_DRIVER', without EXCLUSIVE.
>> 
>> In my opinion, if it is a bug in other stuff GRUB can't handle, and
>> GRUB needs to EXCLUSIVE open SNP, one alternative is the GRUG uses
>> OpenProtocolInformation() to retrieve the list of agents that
>> currently EXCLUSIVE opened SNP, then calls CloseProtocol() to close
>> the opened protocol. If it is the case that GRUB and 'other stuff'
>> both need the network operation and would like to keep EXCLUSIVE open
>> SNP by intention, a MNP solution should be involved since SNP can't
>> support multiple access to use the network interface at the same
>> time.
> 
>> 
>> 
>> Best Regards,
>> Ye Ting
>> 
>> 
>> -----Original Message-----
>> From: Seth Goldberg [mailto:seth.goldberg@oracle.com] 
>> Sent: Wednesday, October 14, 2015 11:39 PM
>> To: The development of GNU GRUB
>> Cc: Ye, Ting; arvidjaar@gmail.com; edk2-devel-01; glin@suse.com; Mark Salter; Laszlo Ersek
>> Subject: Re: [edk2] [grub PATCH] efinet: disable MNP background polling
>> 
>> 
>> 
>>> On Oct 14, 2015, at 4:08 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>> 
>>>> On Wed, Oct 14, 2015 at 05:19:32AM +0000, Ye, Ting wrote:
>>>> Hi all,
>>>> 
>>>> If I understand the issue correctly, I don't quite agree that UEFI
>>>> spec is imprecise about SNP constraints described as following.
>>>> The "constraint" described here is that the grub should use attribute
>>>> "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage
>>>> is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>>>> 
>>>> EXCLUSIVE        Used by applications to gain exclusive access to a protocol interface.
>>>>           If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
>>>>           then an attempt will be made to remove them by calling the driver's Stop() function.
>>>> 
>>>> The grub code should not assume that the SNP is not occupied by other
>>>> drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to
>>>> be more precise, use OpenProtocolInformation() to check whether SNP is
>>>> already opened by other driver, then decide whether need to use EXCLUSIVE
>>>> to disconnect the other drivers. This is the typical usage for all UEFI
>>>> protocol, not particular constraints to SNP protocol.
>>> 
>>> Looks good! Great! However, it looks that we still have a problem if somebody
>>> opens SNP in EXCLUSIVE mode. Then GRUB2 SNP open will fail according to UEFI spec.
>>> Sadly we do not have a control on other stuff and one day our approach may fail
>>> because somebody decided to open SNP in EXCLUSIVE mode in e.g. a driver. Does
>>> it mean that migration to MNP is one sensible solution for our problems? As I know
>>> this is huge overhaul, so, we should think twice before choosing that way.
>> 
>> 
>>   Then it is fortunate that when I wrote the MNP implementation that we ship with Oracle Solaris 11.2, that I tested it on many thousands of systems as well as on new UEFI implementations at the UEFI Plugfest ;).
>> 
>>  --S
>> 
>> 
>> 
>>> 
>>> Daniel
>>> 
>>> _______________________________________________
>>> Grub-devel mailing list
>>> Grub-devel@gnu.org
>>> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [grub PATCH] efinet: disable MNP background polling

[Michael Brown]

On 15/10/15 23:33, Andrew Fish wrote:
> The EFI Driver Model, lets you connect and disconnect, drivers as needed. The EFI networking stack supports the EFI Manged Network Protocol to help manage the network stack configuration. This is what was intended for normal operation.
>
> I’m just guessing but the iPXE developers have to deal with multiple environments (Legacy BIOS, EFI, …) and probably picked a path that allowed all these “worlds” to have a similar code flow. So the EFI OpenProtocol  EXCLUSIVE probably mapped into the other flows where iPXE just takes over. If you are maintaining a complex networking stack (iPXE) trying to make it as common as possible in all its various ports does not seem like an unreasonable goal, but you might not end up with the ideal implementation for each environment.

That's part of it.  Other important reasons are:

1. We want to minimise our exposure to buggy firmware implementations, 
by using as few of the firmware-provided facilities as possible.  If we 
have a native iPXE driver for the NIC hardware then we will explicitly 
rip out any attached MNP, SNP and NII drivers, plug ourselves in (via 
the UEFI driver model) as the driver for the underlying PCI device, and 
control the hardware directly.  If we don't have a native driver (e.g. 
in snponly.efi) then we'll fall back to ripping out just MNP and SNP and 
taking exclusive ownership of the NII device.

2. We actually care about performance.  A "good" MNP-attached driver 
might be able to achieve 10Mbps on a 1000Mbps NIC.  We expect to achieve 
the full 1000Mbps in iPXE.

However, the fact that iPXE takes exclusive ownership of the underlying 
device should not matter, because we always expose a new device handle 
onto which we install SNP, NII, PxeBc, LoadFile, etc.  Whatever we load 
will be given this new handle as the DeviceHandle.

Michael

Re: [edk2] [grub PATCH] efinet: disable MNP background polling

[Laszlo Ersek]

On 10/16/15 00:33, Andrew Fish wrote:
> 
>> On Oct 15, 2015, at 11:14 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 10/15/15 04:11, Ye, Ting wrote:
>>> So the current problem is:
>>> GRUB wants to EXCLUSIVE open SNP, though if other application/driver already opens SNP with EXCLUSIVE attribute, the GRUB would fail. According to UEFI spec V2.5 page 182, 
>>> 	If Attributes is BY_DRIVER , BY_DRIVER|EXCLUSIVE, or EXCLUSIVE, and there are any items on the open list of the protocol interface with an attribute of EXCLUSIVE or BY_DRIVER|EXCLUSIVE, then EFI_ACCESS_DENIED is returned.
>>>
>>> My question is: who will EXCLUSIVE open SNP before GRUB? Why it EXCLUSIVE opens SNP and NOT close SNP protocol before handover to GRUB?
>>
>> Right; when an app is done using the SNP instance and intends to pass
>> control to another app for good, it should close the protocol first --
>> same as it is expected to release memory.
>>
>> ... I wonder if these problems are rooted in an "outdated" pre-OS view
>> of system resources. I assume that before UEFI, pre-OS applications used
>> to think to own all of those resources, and no real life-cycle
>> management was done. I don't know if that's the case, but if it is, it's
>> not compatible with UEFI. With UEFI in the picture, there are resources
>> that need to be tracked and handled cooperatively between unrelated /
>> independent applications. Each single app needs to be prudent about
>> resource management.
>>
> 
> From an EFI history point of view EXCLUSIVE was not intended for
> “every day” use.  EXCLUSIVE exists to solve the problem of how do you
> format a hard drive, or run diagnostics on a hard drive that could
> break the file system driver etc.
> 
> The EFI Driver Model, lets you connect and disconnect, drivers as
> needed. The EFI networking stack supports the EFI Manged Network
> Protocol to help manage the network stack configuration. This is what
> was intended for normal operation.
> 
> I’m just guessing but the iPXE developers have to deal with multiple
> environments (Legacy BIOS, EFI, …) and probably picked a path that
> allowed all these “worlds” to have a similar code flow. So the EFI
> OpenProtocol  EXCLUSIVE probably mapped into the other flows where
> iPXE just takes over. If you are maintaining a complex networking
> stack (iPXE) trying to make it as common as possible in all its
> various ports does not seem like an unreasonable goal, but you might
> not end up with the ideal implementation for each environment.

This exactly has been my thinking about GRUB, and to an extent, iPXE.
They have their internal networking abstractions.

Thanks
Laszlo

> 
> Thanks,
> 
> Andrew Fish
> 
>> Laszlo
>>
>>> 	 For information, the MNP driver in UEFI network stack will open SNP with attribute 'BY_DRIVER', without EXCLUSIVE.
>>>
>>> In my opinion, if it is a bug in other stuff GRUB can't handle, and
>>> GRUB needs to EXCLUSIVE open SNP, one alternative is the GRUG uses
>>> OpenProtocolInformation() to retrieve the list of agents that
>>> currently EXCLUSIVE opened SNP, then calls CloseProtocol() to close
>>> the opened protocol. If it is the case that GRUB and 'other stuff'
>>> both need the network operation and would like to keep EXCLUSIVE open
>>> SNP by intention, a MNP solution should be involved since SNP can't
>>> support multiple access to use the network interface at the same
>>> time.
>>
>>>
>>>
>>> Best Regards,
>>> Ye Ting
>>>
>>>
>>> -----Original Message-----
>>> From: Seth Goldberg [mailto:seth.goldberg@oracle.com] 
>>> Sent: Wednesday, October 14, 2015 11:39 PM
>>> To: The development of GNU GRUB
>>> Cc: Ye, Ting; arvidjaar@gmail.com; edk2-devel-01; glin@suse.com; Mark Salter; Laszlo Ersek
>>> Subject: Re: [edk2] [grub PATCH] efinet: disable MNP background polling
>>>
>>>
>>>
>>>> On Oct 14, 2015, at 4:08 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>>>
>>>>> On Wed, Oct 14, 2015 at 05:19:32AM +0000, Ye, Ting wrote:
>>>>> Hi all,
>>>>>
>>>>> If I understand the issue correctly, I don't quite agree that UEFI
>>>>> spec is imprecise about SNP constraints described as following.
>>>>> The "constraint" described here is that the grub should use attribute
>>>>> "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage
>>>>> is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>>>>>
>>>>> EXCLUSIVE        Used by applications to gain exclusive access to a protocol interface.
>>>>>           If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
>>>>>           then an attempt will be made to remove them by calling the driver's Stop() function.
>>>>>
>>>>> The grub code should not assume that the SNP is not occupied by other
>>>>> drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to
>>>>> be more precise, use OpenProtocolInformation() to check whether SNP is
>>>>> already opened by other driver, then decide whether need to use EXCLUSIVE
>>>>> to disconnect the other drivers. This is the typical usage for all UEFI
>>>>> protocol, not particular constraints to SNP protocol.
>>>>
>>>> Looks good! Great! However, it looks that we still have a problem if somebody
>>>> opens SNP in EXCLUSIVE mode. Then GRUB2 SNP open will fail according to UEFI spec.
>>>> Sadly we do not have a control on other stuff and one day our approach may fail
>>>> because somebody decided to open SNP in EXCLUSIVE mode in e.g. a driver. Does
>>>> it mean that migration to MNP is one sensible solution for our problems? As I know
>>>> this is huge overhaul, so, we should think twice before choosing that way.
>>>
>>>
>>>   Then it is fortunate that when I wrote the MNP implementation that we ship with Oracle Solaris 11.2, that I tested it on many thousands of systems as well as on new UEFI implementations at the UEFI Plugfest ;).
>>>
>>>  --S
>>>
>>>
>>>
>>>>
>>>> Daniel
>>>>
>>>> _______________________________________________
>>>> Grub-devel mailing list
>>>> Grub-devel@gnu.org
>>>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [edk2] [grub PATCH] efinet: disable MNP background polling

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]

On 14.10.2015 17:39, Seth Goldberg wrote:
> 
> 
>> On Oct 14, 2015, at 4:08 AM, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>>
>>> On Wed, Oct 14, 2015 at 05:19:32AM +0000, Ye, Ting wrote:
>>> Hi all,
>>>
>>> If I understand the issue correctly, I don't quite agree that UEFI
>>> spec is imprecise about SNP constraints described as following.
>>> The "constraint" described here is that the grub should use attribute
>>> "EXCLUSIVE" to open SNP protocol to gain exclusive access. This usage
>>> is clearly described in page 184, chapter 6.3 EFI_BOOT_SERVICES.OpenProtocol().
>>>
>>> EXCLUSIVE        Used by applications to gain exclusive access to a protocol interface.
>>>            If any drivers have the protocol interface opened with an attribute of BY_DRIVER,
>>>            then an attempt will be made to remove them by calling the driver's Stop() function.
>>>
>>> The grub code should not assume that the SNP is not occupied by other
>>> drivers, instead, it should use EXCLUSIVE to open SNP protocol, or to
>>> be more precise, use OpenProtocolInformation() to check whether SNP is
>>> already opened by other driver, then decide whether need to use EXCLUSIVE
>>> to disconnect the other drivers. This is the typical usage for all UEFI
>>> protocol, not particular constraints to SNP protocol.
>>
>> Looks good! Great! However, it looks that we still have a problem if somebody
>> opens SNP in EXCLUSIVE mode. Then GRUB2 SNP open will fail according to UEFI spec.
>> Sadly we do not have a control on other stuff and one day our approach may fail
>> because somebody decided to open SNP in EXCLUSIVE mode in e.g. a driver. Does
>> it mean that migration to MNP is one sensible solution for our problems? As I know
>> this is huge overhaul, so, we should think twice before choosing that way.
> 
> 
>    Then it is fortunate that when I wrote the MNP implementation that we ship with Oracle Solaris 11.2, that I tested it on many thousands of systems as well as on new UEFI implementations at the UEFI Plugfest ;).
> 
Can you please point to the patch you used?
I think the only sane solution judging from what I have read so far is
to use MNP as far as possible and fallback to current code if MNP fails
>   --S
> 
> 
> 
>>
>> Daniel
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> .
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [PATCH] efinet: disable MNP background polling

[Andrei Borzenkov]

01.10.2015 12:26, HATAYAMA Daisuke пишет:
>
> Actually, PXE boot fails on Fujitsu PRIMEQUEST 2000 series now.
>

Could you give more details what fails here? GRUB is not loaded or GRUB 
fails to load its files or image chainloaded by GRUB fails?

>
> diff --git a/grub-core/net/drivers/efi/efinet.c b/grub-core/net/drivers/efi/efinet.c
> index c9af01c..a506ab3 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -29,6 +29,7 @@ GRUB_MOD_LICENSE ("GPLv3+");
>   /* GUID.  */
>   static grub_efi_guid_t net_io_guid = GRUB_EFI_SIMPLE_NETWORK_GUID;
>   static grub_efi_guid_t pxe_io_guid = GRUB_EFI_PXE_GUID;
> +static grub_efi_guid_t mnp_io_guid = GRUB_EFI_MANAGED_NETWORK_GUID;
>
>   static grub_err_t
>   send_card_buffer (struct grub_net_card *dev,
> @@ -269,6 +270,9 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char **device,
>       grub_efi_device_path_t *cdp;
>       struct grub_efi_pxe *pxe;
>       struct grub_efi_pxe_mode *pxe_mode;
> +    grub_efi_managed_network_t *mnp;
> +    struct grub_efi_managed_network_config_data m;
> +    struct grub_efi_simple_network_mode s;
>       if (card->driver != &efidriver)
>         continue;
>       cdp = grub_efi_get_device_path (card->efi_handle);
> @@ -312,6 +316,16 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char **device,
>   				    &pxe_mode->dhcp_ack,
>   				    sizeof (pxe_mode->dhcp_ack),
>   				    1, device, path);
> +
> +    mnp = grub_efi_open_protocol (card->efi_handle, &mnp_io_guid,
> +				  GRUB_EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +
> +    if (mnp
> +	&& efi_call_3 (mnp->get_mode_data, mnp, &m, &s) == GRUB_EFI_SUCCESS) {
> +      m.disable_background_polling = 1;
> +      efi_call_2 (mnp->configure, mnp, &m);
> +    }
> +

That's not going to work, sorry. As Laszlo already explained, base card 
does not have MNP protocol as all. MNP is bound to child nodes and 
configuration is per-child so you would need to iterate through all of them.

Re: [PATCH] efinet: disable MNP background polling

[HATAYAMA Daisuke]

From: Andrei Borzenkov <arvidjaar@gmail.com>
Subject: Re: [PATCH] efinet: disable MNP background polling
Date: Thu, 1 Oct 2015 20:40:24 +0300

> 01.10.2015 12:26, HATAYAMA Daisuke пишет:
>>
>> Actually, PXE boot fails on Fujitsu PRIMEQUEST 2000 series now.
>>
> 
> Could you give more details what fails here? GRUB is not loaded or
> GRUB fails to load its files or image chainloaded by GRUB fails?
>

Behaviour changes according to firmware versions. For some version,
grub succeeds, but for another version, grub fails. In failure cases,
grub always hangs in UEFI side during OpenProtocol with
BY_EXCLUSIVE. I don't know how it hangs in UEFI side; I'm now waiting
for responce from firmware people.

On the other hand, in distribution version of grub2, I see network
failrue dropping into grub command prompt. I confirmed that the
network failure is fixed by receive filter patch.

--
Thanks.
HATAYAMA, Daisuke