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__)
>