[V3] Implementing the receive buffer for ofnet

[V3] Implementing the receive buffer for ofnet

[Stanislav Kholmanskikh]

Hi!

This is V3 of the series:

http://lists.gnu.org/archive/html/grub-devel/2016-04/msg00015.html

Changes since V2:

http://lists.gnu.org/archive/html/grub-devel/2016-12/msg00005.html

- Removed the patch which adds a check for error for grub_netbuff_reserve(nb, 2)
- Keep 'return 0' in search_net_devices() if we fail to allocate
  memory for txbuf or rcvbuf
- Removed the patch which frees memory on module unload.
- Put the ofnet_alloc_netbuf() wrapper into the patch enabling
  the receive buffer.

Thanks.

[PATCH V3 1/2] ofnet: move the allocation of the transmit buffer into a function

[Stanislav Kholmanskikh]

In the current code search_net_devices() uses the "alloc-mem" command
from the IEEE1275 User Interface for allocation of the transmit buffer
for the case when GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set.

I don't have hardware where this flag is set to verify if this
workaround is still needed. However, further changes to ofnet will
require to execute this workaround one more time. Therefore, to
avoid possible duplication of code I'm moving this piece of
code into a function.

Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
---
 grub-core/net/drivers/ieee1275/ofnet.c |   69 ++++++++++++++++++++------------
 1 files changed, 43 insertions(+), 26 deletions(-)

diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 6bd3b92..cec0ccc 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -294,6 +294,48 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
   }
 }
 
+/* Allocate memory with alloc-mem */
+static void *
+grub_ieee1275_alloc_mem (grub_size_t len)
+{
+  struct alloc_args
+  {
+    struct grub_ieee1275_common_hdr common;
+    grub_ieee1275_cell_t method;
+    grub_ieee1275_cell_t len;
+    grub_ieee1275_cell_t catch;
+    grub_ieee1275_cell_t result;
+  }
+  args;
+
+  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INTERPRET))
+    {
+      grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_("interpret is not supported"));
+      return NULL;
+    }
+
+  INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
+  args.len = len;
+  args.method = (grub_ieee1275_cell_t) "alloc-mem";
+
+  if (IEEE1275_CALL_ENTRY_FN (&args) == -1 || args.catch)
+    {
+      grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed"));
+      return NULL;
+    }
+  else
+    return (void *)args.result;
+}
+
+static void *
+ofnet_alloc_netbuf (grub_size_t len)
+{
+  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
+    return grub_ieee1275_alloc_mem (len);
+  else
+    return grub_zalloc (len);
+}
+
 static int
 search_net_devices (struct grub_ieee1275_devalias *alias)
 {
@@ -410,32 +452,7 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
 
   card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
 
-  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
-    {
-      struct alloc_args
-      {
-	struct grub_ieee1275_common_hdr common;
-	grub_ieee1275_cell_t method;
-	grub_ieee1275_cell_t len;
-	grub_ieee1275_cell_t catch;
-	grub_ieee1275_cell_t result;
-      }
-      args;
-      INIT_IEEE1275_COMMON (&args.common, "interpret", 2, 2);
-      args.len = card->txbufsize;
-      args.method = (grub_ieee1275_cell_t) "alloc-mem";
-
-      if (IEEE1275_CALL_ENTRY_FN (&args) == -1
-	  || args.catch)
-	{
-	  card->txbuf = 0;
-	  grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
-	}
-      else
-	card->txbuf = (void *) args.result;
-    }
-  else
-    card->txbuf = grub_zalloc (card->txbufsize);
+  card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
   if (!card->txbuf)
     {
       grub_free (ofdata->path);
-- 
1.7.1

[PATCH V3 2/2] ofnet: implement the receive buffer

[Stanislav Kholmanskikh]

get_card_packet() from ofnet.c allocates a netbuff based on the device's MTU:

 nb = grub_netbuff_alloc (dev->mtu + 64 + 2);

In the case when the MTU is large, and the received packet is
relatively small, this leads to allocation of significantly more memory,
than it's required. An example could be transmission of TFTP packets
with 0x400 blksize via a network card with 0x10000 MTU.

This patch implements a per-card receive buffer in a way similar to efinet.c,
and makes get_card_packet() allocate a netbuff of the received data size.

Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
---
 grub-core/net/drivers/ieee1275/ofnet.c |   90 ++++++++++++++++++++++++++-----
 1 files changed, 75 insertions(+), 15 deletions(-)

diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index cec0ccc..d7a3ada 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -85,24 +85,30 @@ get_card_packet (struct grub_net_card *dev)
   grub_uint64_t start_time;
   struct grub_net_buff *nb;
 
-  nb = grub_netbuff_alloc (dev->mtu + 64 + 2);
+  start_time = grub_get_time_ms ();
+  do
+    rc = grub_ieee1275_read (data->handle, dev->rcvbuf, dev->rcvbufsize, &actual);
+  while ((actual <= 0 || rc < 0) && (grub_get_time_ms () - start_time < 200));
+
+  if (actual <= 0)
+    return NULL;
+  
+  nb = grub_netbuff_alloc (actual + 2);
   if (!nb)
     return NULL;
   /* Reserve 2 bytes so that 2 + 14/18 bytes of ethernet header is divisible
      by 4. So that IP header is aligned on 4 bytes. */
   grub_netbuff_reserve (nb, 2);
 
-  start_time = grub_get_time_ms ();
-  do
-    rc = grub_ieee1275_read (data->handle, nb->data, dev->mtu + 64, &actual);
-  while ((actual <= 0 || rc < 0) && (grub_get_time_ms () - start_time < 200));
-  if (actual > 0)
+  grub_memcpy (nb->data, dev->rcvbuf, actual);
+
+  if (grub_netbuff_put (nb, actual))
     {
-      grub_netbuff_put (nb, actual);
-      return nb;
+      grub_netbuff_free (nb);
+      return NULL;
     }
-  grub_netbuff_free (nb);
-  return NULL;
+  
+  return nb;
 }
 
 static struct grub_net_card_driver ofdriver =
@@ -327,6 +333,40 @@ grub_ieee1275_alloc_mem (grub_size_t len)
     return (void *)args.result;
 }
 
+/* Free memory allocated by alloc-mem */
+static grub_err_t
+grub_ieee1275_free_mem (void *addr, grub_size_t len)
+{
+  struct free_args
+  {
+    struct grub_ieee1275_common_hdr common;
+    grub_ieee1275_cell_t method;
+    grub_ieee1275_cell_t len;
+    grub_ieee1275_cell_t addr;
+    grub_ieee1275_cell_t catch;
+  }
+  args;
+
+  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INTERPRET))
+    {
+      grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_("interpret is not supported"));
+      return grub_errno;
+    }
+
+  INIT_IEEE1275_COMMON (&args.common, "interpret", 3, 1);
+  args.addr = (grub_ieee1275_cell_t)addr;
+  args.len = len;
+  args.method = (grub_ieee1275_cell_t) "free-mem";
+
+  if (IEEE1275_CALL_ENTRY_FN(&args) == -1 || args.catch)
+    {
+      grub_error (GRUB_ERR_INVALID_COMMAND, N_("free-mem failed"));
+      return grub_errno;
+    }
+
+  return GRUB_ERR_NONE;
+}
+
 static void *
 ofnet_alloc_netbuf (grub_size_t len)
 {
@@ -336,6 +376,15 @@ ofnet_alloc_netbuf (grub_size_t len)
     return grub_zalloc (len);
 }
 
+static void
+ofnet_free_netbuf (void *addr, grub_size_t len)
+{
+  if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN))
+    grub_ieee1275_free_mem (addr, len);
+  else
+    grub_free (addr);
+}
+
 static int
 search_net_devices (struct grub_ieee1275_devalias *alias)
 {
@@ -451,15 +500,19 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
   card->default_address = lla;
 
   card->txbufsize = ALIGN_UP (card->mtu, 64) + 256;
+  card->rcvbufsize = ALIGN_UP (card->mtu, 64) + 256;
 
   card->txbuf = ofnet_alloc_netbuf (card->txbufsize);
   if (!card->txbuf)
+    goto fail_netbuf;
+
+  card->rcvbuf = ofnet_alloc_netbuf (card->rcvbufsize);
+  if (!card->rcvbuf)
     {
-      grub_free (ofdata->path);
-      grub_free (ofdata);
-      grub_free (card);
-      grub_print_error ();
-      return 0;
+      grub_error_push ();
+      ofnet_free_netbuf (card->txbuf, card->txbufsize);
+      grub_error_pop ();
+      goto fail_netbuf;
     }
   card->driver = NULL;
   card->data = ofdata;
@@ -472,6 +525,13 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
   card->driver = &ofdriver;
   grub_net_card_register (card);
   return 0;
+
+fail_netbuf:
+  grub_free (ofdata->path);
+  grub_free (ofdata);
+  grub_free (card);
+  grub_print_error ();
+  return 0;
 }
 
 static void
-- 
1.7.1

Re: [PATCH V3 1/2] ofnet: move the allocation of the transmit buffer into a function

[Daniel Kiper]

On Mon, Dec 12, 2016 at 06:03:38PM +0300, Stanislav Kholmanskikh wrote:
> In the current code search_net_devices() uses the "alloc-mem" command
> from the IEEE1275 User Interface for allocation of the transmit buffer
> for the case when GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set.
>
> I don't have hardware where this flag is set to verify if this
> workaround is still needed. However, further changes to ofnet will
> require to execute this workaround one more time. Therefore, to
> avoid possible duplication of code I'm moving this piece of
> code into a function.
>
> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [PATCH V3 2/2] ofnet: implement the receive buffer

[Daniel Kiper]

On Mon, Dec 12, 2016 at 06:03:39PM +0300, Stanislav Kholmanskikh wrote:
> get_card_packet() from ofnet.c allocates a netbuff based on the device's MTU:
>
>  nb = grub_netbuff_alloc (dev->mtu + 64 + 2);
>
> In the case when the MTU is large, and the received packet is
> relatively small, this leads to allocation of significantly more memory,
> than it's required. An example could be transmission of TFTP packets
> with 0x400 blksize via a network card with 0x10000 MTU.
>
> This patch implements a per-card receive buffer in a way similar to efinet.c,
> and makes get_card_packet() allocate a netbuff of the received data size.
>
> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

Re: [V3] Implementing the receive buffer for ofnet

[Daniel Kiper]

On Mon, Dec 12, 2016 at 06:03:37PM +0300, Stanislav Kholmanskikh wrote:
> Hi!
>
> This is V3 of the series:
>
> http://lists.gnu.org/archive/html/grub-devel/2016-04/msg00015.html
>
> Changes since V2:
>
> http://lists.gnu.org/archive/html/grub-devel/2016-12/msg00005.html
>
> - Removed the patch which adds a check for error for grub_netbuff_reserve(nb, 2)
> - Keep 'return 0' in search_net_devices() if we fail to allocate
>   memory for txbuf or rcvbuf
> - Removed the patch which frees memory on module unload.
> - Put the ofnet_alloc_netbuf() wrapper into the patch enabling
>   the receive buffer.

If there are no more objections I will apply this patch series in 2-3 days.

Daniel

Re: [V3] Implementing the receive buffer for ofnet

[Daniel Kiper]

On Mon, Dec 12, 2016 at 05:42:56PM +0100, Daniel Kiper wrote:
> On Mon, Dec 12, 2016 at 06:03:37PM +0300, Stanislav Kholmanskikh wrote:
> > Hi!
> >
> > This is V3 of the series:
> >
> > http://lists.gnu.org/archive/html/grub-devel/2016-04/msg00015.html
> >
> > Changes since V2:
> >
> > http://lists.gnu.org/archive/html/grub-devel/2016-12/msg00005.html
> >
> > - Removed the patch which adds a check for error for grub_netbuff_reserve(nb, 2)
> > - Keep 'return 0' in search_net_devices() if we fail to allocate
> >   memory for txbuf or rcvbuf
> > - Removed the patch which frees memory on module unload.
> > - Put the ofnet_alloc_netbuf() wrapper into the patch enabling
> >   the receive buffer.
>
> If there are no more objections I will apply this patch series in 2-3 days.

Applied! Thanks a lot for doing the work!

Daniel