[PATCH 1/2] ieee1275: alloc-mem and free-mem

[PATCH 1/2] ieee1275: alloc-mem and free-mem

[Stanislav Kholmanskikh]

Add wrappers for memory allocation using
alloc-mem and free-mem commands from the User Interface.

Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
---
 grub-core/kern/ieee1275/openfw.c |   68 ++++++++++++++++++++++++++++++++++++++
 include/grub/ieee1275/ieee1275.h |    2 +
 2 files changed, 70 insertions(+), 0 deletions(-)

diff --git a/grub-core/kern/ieee1275/openfw.c b/grub-core/kern/ieee1275/openfw.c
index ddb7783..35225ec 100644
--- a/grub-core/kern/ieee1275/openfw.c
+++ b/grub-core/kern/ieee1275/openfw.c
@@ -561,3 +561,71 @@ grub_ieee1275_canonicalise_devname (const char *path)
   return NULL;
 }
 
+/* Allocate memory with alloc-mem */
+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;
+}
+
+/* Free memory allocated by alloc-mem */
+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;
+}
diff --git a/include/grub/ieee1275/ieee1275.h b/include/grub/ieee1275/ieee1275.h
index 8e42513..91510b3 100644
--- a/include/grub/ieee1275/ieee1275.h
+++ b/include/grub/ieee1275/ieee1275.h
@@ -234,6 +234,8 @@ int EXPORT_FUNC(grub_ieee1275_devalias_next) (struct grub_ieee1275_devalias *ali
 void EXPORT_FUNC(grub_ieee1275_children_peer) (struct grub_ieee1275_devalias *alias);
 void EXPORT_FUNC(grub_ieee1275_children_first) (const char *devpath,
 						struct grub_ieee1275_devalias *alias);
+void *EXPORT_FUNC(grub_ieee1275_alloc_mem) (grub_size_t len);
+grub_err_t EXPORT_FUNC(grub_ieee1275_free_mem) (void * addr, grub_size_t len);
 
 #define FOR_IEEE1275_DEVALIASES(alias) for (grub_ieee1275_devalias_init_iterator (&(alias)); grub_ieee1275_devalias_next (&(alias));)
 
-- 
1.7.1

[PATCH 2/2] ofnet: implement a 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 |  100 ++++++++++++++++++-------------
 1 files changed, 58 insertions(+), 42 deletions(-)

diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
index 6bd3b92..09ec77e 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -85,24 +85,35 @@ 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);
+  if (grub_netbuff_reserve (nb, 2))
+    {
+      grub_netbuff_free (nb);
+      return NULL;
+    }
 
-  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 =
@@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
   }
 }
 
+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_malloc (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)
 {
@@ -409,41 +438,21 @@ 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;
 
-  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)
+    goto fail;
+
+  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;
     }
+
   card->driver = NULL;
   card->data = ofdata;
   card->flags = 0;
@@ -455,6 +464,13 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
   card->driver = &ofdriver;
   grub_net_card_register (card);
   return 0;
+
+ fail:
+  grub_free (ofdata->path);
+  grub_free (ofdata);
+  grub_free (card);
+  grub_print_error ();
+  return 1;
 }
 
 static void
-- 
1.7.1

Re: [PATCH 2/2] ofnet: implement a receive buffer

[Stanislav Kholmanskikh]

Hi.

Could anybody have a look at this series, please?

Thanks.

On 04/12/2016 03:39 PM, 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>
> ---
>   grub-core/net/drivers/ieee1275/ofnet.c |  100 ++++++++++++++++++-------------
>   1 files changed, 58 insertions(+), 42 deletions(-)
>
> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
> index 6bd3b92..09ec77e 100644
> --- a/grub-core/net/drivers/ieee1275/ofnet.c
> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
> @@ -85,24 +85,35 @@ 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);
> +  if (grub_netbuff_reserve (nb, 2))
> +    {
> +      grub_netbuff_free (nb);
> +      return NULL;
> +    }
>
> -  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 =
> @@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
>     }
>   }
>
> +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_malloc (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)
>   {
> @@ -409,41 +438,21 @@ 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;
>
> -  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)
> +    goto fail;
> +
> +  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;
>       }
> +
>     card->driver = NULL;
>     card->data = ofdata;
>     card->flags = 0;
> @@ -455,6 +464,13 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
>     card->driver = &ofdriver;
>     grub_net_card_register (card);
>     return 0;
> +
> + fail:
> +  grub_free (ofdata->path);
> +  grub_free (ofdata);
> +  grub_free (card);
> +  grub_print_error ();
> +  return 1;
>   }
>
>   static void
>

Re: [PATCH 2/2] ofnet: implement a receive buffer

[Stanislav Kholmanskikh]

Hi.

On 05/10/2016 01:45 PM, Stanislav Kholmanskikh wrote:
> Hi.
>
> Could anybody have a look at this series, please?

Ping.

>
> Thanks.
>
> On 04/12/2016 03:39 PM, 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>
>> ---
>>   grub-core/net/drivers/ieee1275/ofnet.c |  100
>> ++++++++++++++++++-------------
>>   1 files changed, 58 insertions(+), 42 deletions(-)
>>
>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c
>> b/grub-core/net/drivers/ieee1275/ofnet.c
>> index 6bd3b92..09ec77e 100644
>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>> @@ -85,24 +85,35 @@ 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);
>> +  if (grub_netbuff_reserve (nb, 2))
>> +    {
>> +      grub_netbuff_free (nb);
>> +      return NULL;
>> +    }
>>
>> -  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 =
>> @@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char
>> *devpath, char **device, char **path,
>>     }
>>   }
>>
>> +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_malloc (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)
>>   {
>> @@ -409,41 +438,21 @@ 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;
>>
>> -  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)
>> +    goto fail;
>> +
>> +  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;
>>       }
>> +
>>     card->driver = NULL;
>>     card->data = ofdata;
>>     card->flags = 0;
>> @@ -455,6 +464,13 @@ search_net_devices (struct grub_ieee1275_devalias
>> *alias)
>>     card->driver = &ofdriver;
>>     grub_net_card_register (card);
>>     return 0;
>> +
>> + fail:
>> +  grub_free (ofdata->path);
>> +  grub_free (ofdata);
>> +  grub_free (card);
>> +  grub_print_error ();
>> +  return 1;
>>   }
>>
>>   static void
>>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH 2/2] ofnet: implement a receive buffer

[Stanislav Kholmanskikh]

Hi.

I still believe this series may be useful for the community. So here is
one more ping.

Thanks.

On 07/13/2016 05:35 PM, Stanislav Kholmanskikh wrote:
> Hi.
> 
> On 05/10/2016 01:45 PM, Stanislav Kholmanskikh wrote:
>> Hi.
>>
>> Could anybody have a look at this series, please?
> 
> Ping.
> 
>>
>> Thanks.
>>
>> On 04/12/2016 03:39 PM, 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>
>>> ---
>>>   grub-core/net/drivers/ieee1275/ofnet.c |  100
>>> ++++++++++++++++++-------------
>>>   1 files changed, 58 insertions(+), 42 deletions(-)
>>>
>>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c
>>> b/grub-core/net/drivers/ieee1275/ofnet.c
>>> index 6bd3b92..09ec77e 100644
>>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>>> @@ -85,24 +85,35 @@ 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);
>>> +  if (grub_netbuff_reserve (nb, 2))
>>> +    {
>>> +      grub_netbuff_free (nb);
>>> +      return NULL;
>>> +    }
>>>
>>> -  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 =
>>> @@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char
>>> *devpath, char **device, char **path,
>>>     }
>>>   }
>>>
>>> +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_malloc (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)
>>>   {
>>> @@ -409,41 +438,21 @@ 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;
>>>
>>> -  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)
>>> +    goto fail;
>>> +
>>> +  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;
>>>       }
>>> +
>>>     card->driver = NULL;
>>>     card->data = ofdata;
>>>     card->flags = 0;
>>> @@ -455,6 +464,13 @@ search_net_devices (struct grub_ieee1275_devalias
>>> *alias)
>>>     card->driver = &ofdriver;
>>>     grub_net_card_register (card);
>>>     return 0;
>>> +
>>> + fail:
>>> +  grub_free (ofdata->path);
>>> +  grub_free (ofdata);
>>> +  grub_free (card);
>>> +  grub_print_error ();
>>> +  return 1;
>>>   }
>>>
>>>   static void
>>>
>>
>> _______________________________________________
>> 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

Re: [PATCH 2/2] ofnet: implement a receive buffer

[Daniel Kiper]

On Tue, Apr 12, 2016 at 03:39:56PM +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.

Have you checked performance impact of this patch? It should not be
meaningful but it is good to know.

> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
> ---
>  grub-core/net/drivers/ieee1275/ofnet.c |  100 ++++++++++++++++++-------------
>  1 files changed, 58 insertions(+), 42 deletions(-)
>
> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
> index 6bd3b92..09ec77e 100644
> --- a/grub-core/net/drivers/ieee1275/ofnet.c
> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
> @@ -85,24 +85,35 @@ 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));

Why 200? Please avoid using plain numbers if possible. Use constants. If it does
not make sense then put comment which explains why this figure not another.

Additionally, are we sure that whole packet can be always stored in dev->rcvbuf?

> +  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);
> +  if (grub_netbuff_reserve (nb, 2))
> +    {
> +      grub_netbuff_free (nb);
> +      return NULL;
> +    }

This smells like separate fix not belonging to this patch.

> -  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;
>      }

Why not...

  if (!grub_netbuff_put (nb, actual))
    return nb;

> -  grub_netbuff_free (nb);
> -  return NULL;
> +
> +  return nb;

...then you do not need these changes too...

>  }

It looks that everything below belongs to patch #1...

>  static struct grub_net_card_driver ofdriver =
> @@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
>    }
>  }
>
> +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_malloc (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)
>  {
> @@ -409,41 +438,21 @@ 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;
>
> -  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)
> +    goto fail;
> +
> +  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;
>      }
> +
>    card->driver = NULL;
>    card->data = ofdata;
>    card->flags = 0;
> @@ -455,6 +464,13 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
>    card->driver = &ofdriver;
>    grub_net_card_register (card);
>    return 0;
> +
> + fail:
> +  grub_free (ofdata->path);
> +  grub_free (ofdata);
> +  grub_free (card);
> +  grub_print_error ();
> +  return 1;

...and without full explanation why in #1 commit message it is
not obvious for what this change is really needed...

Daniel

Re: [PATCH 2/2] ofnet: implement a receive buffer

[Stanislav Kholmanskikh]

On 11/16/2016 01:34 AM, Daniel Kiper wrote:
> On Tue, Apr 12, 2016 at 03:39:56PM +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.
> 
> Have you checked performance impact of this patch? It should not be
> meaningful but it is good to know.

No. I didn't do performance testing.

> 
>> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
>> ---
>>  grub-core/net/drivers/ieee1275/ofnet.c |  100 ++++++++++++++++++-------------
>>  1 files changed, 58 insertions(+), 42 deletions(-)
>>
>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
>> index 6bd3b92..09ec77e 100644
>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>> @@ -85,24 +85,35 @@ 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));
> 
> Why 200? Please avoid using plain numbers if possible. Use constants. If it does
> not make sense then put comment which explains why this figure not another.

The whole 'do while' construction is from the existing code, I only
modify the destination for the grub_ieee1275_read() call.

> 
> Additionally, are we sure that whole packet can be always stored in dev->rcvbuf?

Code in search_net_devices() allocates the buffer to be of size:

ALIGN_UP (card->mtu, 64) + 256;

so, yes, it's capable to handle any valid packet size.

> 
>> +  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);
>> +  if (grub_netbuff_reserve (nb, 2))
>> +    {
>> +      grub_netbuff_free (nb);
>> +      return NULL;
>> +    }
> 
> This smells like separate fix not belonging to this patch.

Ok. I can move this change into a separate patch.

> 
>> -  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;
>>      }
> 
> Why not...

Ok.

> 
>   if (!grub_netbuff_put (nb, actual))
>     return nb;
> 
>> -  grub_netbuff_free (nb);
>> -  return NULL;
>> +
>> +  return nb;
> 
> ...then you do not need these changes too...
> 
>>  }
> 
> It looks that everything below belongs to patch #1...

No. Patch 1 is about two supplementary funcions for "alloc-mem",
"free-mem". The changes below setup the transmit/receive buffers for a
network card. The changes above use this receive buffer. So, in my
opinion, this all is logically coupled and should be in one patch.

> 
>>  static struct grub_net_card_driver ofdriver =
>> @@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
>>    }
>>  }
>>
>> +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_malloc (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)
>>  {
>> @@ -409,41 +438,21 @@ 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;
>>
>> -  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)
>> +    goto fail;
>> +
>> +  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;
>>      }
>> +
>>    card->driver = NULL;
>>    card->data = ofdata;
>>    card->flags = 0;
>> @@ -455,6 +464,13 @@ search_net_devices (struct grub_ieee1275_devalias *alias)
>>    card->driver = &ofdriver;
>>    grub_net_card_register (card);
>>    return 0;
>> +
>> + fail:
>> +  grub_free (ofdata->path);
>> +  grub_free (ofdata);
>> +  grub_free (card);
>> +  grub_print_error ();
>> +  return 1;
> 
> ...and without full explanation why in #1 commit message it is
> not obvious for what this change is really needed...
> 
> Daniel
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 

Re: [PATCH 2/2] ofnet: implement a receive buffer

[Daniel Kiper]

On Fri, Nov 18, 2016 at 04:29:24PM +0300, Stanislav Kholmanskikh wrote:
> On 11/16/2016 01:34 AM, Daniel Kiper wrote:
> > On Tue, Apr 12, 2016 at 03:39:56PM +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.
> >
> > Have you checked performance impact of this patch? It should not be
> > meaningful but it is good to know.
>
> No. I didn't do performance testing.

Please do.

> >> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
> >> ---
> >>  grub-core/net/drivers/ieee1275/ofnet.c |  100 ++++++++++++++++++-------------
> >>  1 files changed, 58 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
> >> index 6bd3b92..09ec77e 100644
> >> --- a/grub-core/net/drivers/ieee1275/ofnet.c
> >> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
> >> @@ -85,24 +85,35 @@ 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));
> >
> > Why 200? Please avoid using plain numbers if possible. Use constants. If it does
> > not make sense then put comment which explains why this figure not another.
>
> The whole 'do while' construction is from the existing code, I only
> modify the destination for the grub_ieee1275_read() call.

OK but if you move such code around anyway do not leave it unreadable. Improve it
by constants or comments.

> > Additionally, are we sure that whole packet can be always stored in dev->rcvbuf?
>
> Code in search_net_devices() allocates the buffer to be of size:
>
> ALIGN_UP (card->mtu, 64) + 256;
>
> so, yes, it's capable to handle any valid packet size.

Great but why this numbers?

> >> +  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);
> >> +  if (grub_netbuff_reserve (nb, 2))
> >> +    {
> >> +      grub_netbuff_free (nb);
> >> +      return NULL;
> >> +    }
> >
> > This smells like separate fix not belonging to this patch.
>
> Ok. I can move this change into a separate patch.

Thanks a lot!

> >> -  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;
> >>      }
> >
> > Why not...
>
> Ok.
>
> >
> >   if (!grub_netbuff_put (nb, actual))
> >     return nb;
> >
> >> -  grub_netbuff_free (nb);
> >> -  return NULL;
> >> +
> >> +  return nb;
> >
> > ...then you do not need these changes too...
> >
> >>  }
> >
> > It looks that everything below belongs to patch #1...
>
> No. Patch 1 is about two supplementary funcions for "alloc-mem",
> "free-mem". The changes below setup the transmit/receive buffers for a
> network card. The changes above use this receive buffer. So, in my
> opinion, this all is logically coupled and should be in one patch.

I have checked code below once again. First of all I think that
grub_ieee1275_alloc_mem() and grub_ieee1275_free_mem() have to live
in this file not in grub-core/kern/ieee1275/openfw.c. I see no
other callers for both of them. Additionally, patch #1 should
introduce grub_ieee1275_alloc_mem(), ofnet_alloc_netbuf() and
search_net_devices() should call ofnet_alloc_netbuf(). No more
no less. Do not forget to mention in commit message why patch #1
is needed. Then patch #2 should introduce rest of the code below.

> >>  static struct grub_net_card_driver ofdriver =
> >> @@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
> >>    }
> >>  }
> >>
> >> +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_malloc (len);

It looks that it should be grub_zalloc() instead of grub_malloc() here.

> >> +}
> >> +
> >> +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)
> >>  {
> >> @@ -409,41 +438,21 @@ 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;
> >>
> >> -  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)
> >> +    goto fail;
> >> +
> >> +  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;
> >>      }

Should not we free card->rcvbuf and/or card->txbuf if module is
unloaded or something like that?

Daniel

Re: [PATCH 2/2] ofnet: implement a receive buffer

[Stanislav Kholmanskikh]

On 11/22/2016 12:48 AM, Daniel Kiper wrote:
> On Fri, Nov 18, 2016 at 04:29:24PM +0300, Stanislav Kholmanskikh wrote:
>> On 11/16/2016 01:34 AM, Daniel Kiper wrote:
>>> On Tue, Apr 12, 2016 at 03:39:56PM +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.
>>>
>>> Have you checked performance impact of this patch? It should not be
>>> meaningful but it is good to know.
>>
>> No. I didn't do performance testing.
> 
> Please do.

Ok. I'll check what I can do here.

> 
>>>> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
>>>> ---
>>>>  grub-core/net/drivers/ieee1275/ofnet.c |  100 ++++++++++++++++++-------------
>>>>  1 files changed, 58 insertions(+), 42 deletions(-)
>>>>
>>>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
>>>> index 6bd3b92..09ec77e 100644
>>>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>>>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>>>> @@ -85,24 +85,35 @@ 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));
>>>
>>> Why 200? Please avoid using plain numbers if possible. Use constants. If it does
>>> not make sense then put comment which explains why this figure not another.
>>
>> The whole 'do while' construction is from the existing code, I only
>> modify the destination for the grub_ieee1275_read() call.
> 
> OK but if you move such code around anyway do not leave it unreadable. Improve it
> by constants or comments.

May I use a macro for this

#define READ_TIMEOUT 200

?

> 
>>> Additionally, are we sure that whole packet can be always stored in dev->rcvbuf?
>>
>> Code in search_net_devices() allocates the buffer to be of size:
>>
>> ALIGN_UP (card->mtu, 64) + 256;
>>
>> so, yes, it's capable to handle any valid packet size.
> 
> Great but why this numbers?

I have to admit that I can't answer to your question. :( I copied this
stuff from efi (for the receive buffer). The transmit buffer was already
of this size.

> 
>>>> +  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);
>>>> +  if (grub_netbuff_reserve (nb, 2))
>>>> +    {
>>>> +      grub_netbuff_free (nb);
>>>> +      return NULL;
>>>> +    }
>>>
>>> This smells like separate fix not belonging to this patch.
>>
>> Ok. I can move this change into a separate patch.
> 
> Thanks a lot!
> 
>>>> -  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;
>>>>      }
>>>
>>> Why not...
>>
>> Ok.
>>
>>>
>>>   if (!grub_netbuff_put (nb, actual))
>>>     return nb;
>>>
>>>> -  grub_netbuff_free (nb);
>>>> -  return NULL;
>>>> +
>>>> +  return nb;
>>>
>>> ...then you do not need these changes too...
>>>
>>>>  }
>>>
>>> It looks that everything below belongs to patch #1...
>>
>> No. Patch 1 is about two supplementary funcions for "alloc-mem",
>> "free-mem". The changes below setup the transmit/receive buffers for a
>> network card. The changes above use this receive buffer. So, in my
>> opinion, this all is logically coupled and should be in one patch.
> 
> I have checked code below once again. First of all I think that
> grub_ieee1275_alloc_mem() and grub_ieee1275_free_mem() have to live
> in this file not in grub-core/kern/ieee1275/openfw.c. I see no
> other callers for both of them. Additionally, patch #1 should
> introduce grub_ieee1275_alloc_mem(), ofnet_alloc_netbuf() and
> search_net_devices() should call ofnet_alloc_netbuf(). No more
> no less. Do not forget to mention in commit message why patch #1
> is needed. Then patch #2 should introduce rest of the code below.

Ok.

> 
>>>>  static struct grub_net_card_driver ofdriver =
>>>> @@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
>>>>    }
>>>>  }
>>>>
>>>> +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_malloc (len);
> 
> It looks that it should be grub_zalloc() instead of grub_malloc() here.

I have two reasons why I don't use grub_zalloc() here:

1. The buffer allocated with this function is written/read many times
while grub is working. We write some amount of bytes to the buffer, and
then read this amount of bytes. So I don't see why zeroing the buffer on
allocation should matter.

2. In IEEE1275-1994 I do not see an explicit notice that memory
allocated with alloc-mem is zeroed. So for consistence of
ofnet_alloc_netbuf() I do not call grub_zalloc() there.

> 
>>>> +}
>>>> +
>>>> +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)
>>>>  {
>>>> @@ -409,41 +438,21 @@ 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;
>>>>
>>>> -  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)
>>>> +    goto fail;
>>>> +
>>>> +  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;
>>>>      }
> 
> Should not we free card->rcvbuf and/or card->txbuf if module is
> unloaded or something like that?

Yes, I think so. Thanks for pointing at this.

It's interesting that none of uboot, efi, ieee1275 drivers seems to care
of freeing the card data structure on module unload. All they do is
similar to:

FOR_NET_CARDS_SAFE (card, next)
  if (the card is handled by us)
    grub_net_card_unregister (card);

whereas from grub-core/net/net.c I don't see that
grub_net_card_unregister() frees memory.

It seems that the job of freeing card's memory is expected to be handled
in drivers and none of the drivers care about it, excluding pxe, where
'grub_pxe_card' is statically allocated. Or am I missing something?

As for ieee1275 I'm thinking about something like:

diff --git a/grub-core/net/drivers/ieee1275/ofnet.c
b/grub-core/net/drivers/ieee1275/ofnet.c
index 6bd3b92..12a4289 100644
--- a/grub-core/net/drivers/ieee1275/ofnet.c
+++ b/grub-core/net/drivers/ieee1275/ofnet.c
@@ -473,9 +473,28 @@ GRUB_MOD_INIT(ofnet)
 GRUB_MOD_FINI(ofnet)
 {
   struct grub_net_card *card, *next;
+  struct grub_ofnetcard_data *ofdata;

   FOR_NET_CARDS_SAFE (card, next)
     if (card->driver && grub_strcmp (card->driver->name, "ofnet") == 0)
-      grub_net_card_unregister (card);
+      {
+       grub_net_card_unregister (card);
+
+       /*
+        * The fact that we are here means the card was successfully
+        * initialized in the past, so all the below pointers are valid,
+        * and we may free associated memory without checks.
+        */
+
+       ofdata = (struct grub_ofnetcard_data *) card->data;
+       grub_free (ofdata->path);
+       grub_free (ofdata->suffix);
+       grub_free (ofdata);
+
+       ofnet_free_netbuf (card->txbuf, card->txbufsize);
+       ofnet_free_netbuf (card->rcvbuf, card->rcvbufsize);
+
+       grub_free (card);
+      }
   grub_ieee1275_net_config = 0;
 }

(not tested)

I think it deserves a separate patch. In one patch we are adding the
receive buffer, and in another we are making the ieee1275 driver to free
all card resources on unload.

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

Re: [PATCH 2/2] ofnet: implement a receive buffer

[Daniel Kiper]

On Tue, Nov 22, 2016 at 05:08:25PM +0300, Stanislav Kholmanskikh wrote:
> On 11/22/2016 12:48 AM, Daniel Kiper wrote:
> > On Fri, Nov 18, 2016 at 04:29:24PM +0300, Stanislav Kholmanskikh wrote:
> >> On 11/16/2016 01:34 AM, Daniel Kiper wrote:
> >>> On Tue, Apr 12, 2016 at 03:39:56PM +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.
> >>>
> >>> Have you checked performance impact of this patch? It should not be
> >>> meaningful but it is good to know.
> >>
> >> No. I didn't do performance testing.
> >
> > Please do.
>
> Ok. I'll check what I can do here.

Great! Thnaks!

> >>>> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
> >>>> ---
> >>>>  grub-core/net/drivers/ieee1275/ofnet.c |  100 ++++++++++++++++++-------------
> >>>>  1 files changed, 58 insertions(+), 42 deletions(-)
> >>>>
> >>>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
> >>>> index 6bd3b92..09ec77e 100644
> >>>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
> >>>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
> >>>> @@ -85,24 +85,35 @@ 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));
> >>>
> >>> Why 200? Please avoid using plain numbers if possible. Use constants. If it does
> >>> not make sense then put comment which explains why this figure not another.
> >>
> >> The whole 'do while' construction is from the existing code, I only
> >> modify the destination for the grub_ieee1275_read() call.
> >
> > OK but if you move such code around anyway do not leave it unreadable. Improve it
> > by constants or comments.
>
> May I use a macro for this
>
> #define READ_TIMEOUT 200
>
> ?

It seems to me that it appears in one place, so, comment would be better here.
Unfortunately, it looks that there is no explanation for that value in commit
message... Ehhh... Could you check mail archive? If there is no such thing there
then let's leave it as it. Though I do not like it.

> >>> Additionally, are we sure that whole packet can be always stored in dev->rcvbuf?
> >>
> >> Code in search_net_devices() allocates the buffer to be of size:
> >>
> >> ALIGN_UP (card->mtu, 64) + 256;
> >>
> >> so, yes, it's capable to handle any valid packet size.
> >
> > Great but why this numbers?
>
> I have to admit that I can't answer to your question. :( I copied this
> stuff from efi (for the receive buffer). The transmit buffer was already
> of this size.

Ugh... Same as above...

[...]

> >>>>  static struct grub_net_card_driver ofdriver =
> >>>> @@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
> >>>>    }
> >>>>  }
> >>>>
> >>>> +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_malloc (len);
> >
> > It looks that it should be grub_zalloc() instead of grub_malloc() here.
>
> I have two reasons why I don't use grub_zalloc() here:
>
> 1. The buffer allocated with this function is written/read many times
> while grub is working. We write some amount of bytes to the buffer, and
> then read this amount of bytes. So I don't see why zeroing the buffer on

I suppose that "this" == "same" here...

> allocation should matter.
>
> 2. In IEEE1275-1994 I do not see an explicit notice that memory
> allocated with alloc-mem is zeroed. So for consistence of
> ofnet_alloc_netbuf() I do not call grub_zalloc() there.

Yep, I am aware of that. However, I am asking because you change
currently exiting code behavior which uses grub_zalloc(). Maybe
we should leave it as is (I am thinking about grub_zalloc()) but
it is not very strong must. If you choose grub_malloc() please
explain in commit message why you did it and why it is safe here.

> >>>> +}
> >>>> +
> >>>> +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)
> >>>>  {
> >>>> @@ -409,41 +438,21 @@ 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;
> >>>>
> >>>> -  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)
> >>>> +    goto fail;
> >>>> +
> >>>> +  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;
> >>>>      }
> >
> > Should not we free card->rcvbuf and/or card->txbuf if module is
> > unloaded or something like that?
>
> Yes, I think so. Thanks for pointing at this.
>
> It's interesting that none of uboot, efi, ieee1275 drivers seems to care
> of freeing the card data structure on module unload. All they do is
> similar to:
>
> FOR_NET_CARDS_SAFE (card, next)
>   if (the card is handled by us)
>     grub_net_card_unregister (card);
>
> whereas from grub-core/net/net.c I don't see that
> grub_net_card_unregister() frees memory.
>
> It seems that the job of freeing card's memory is expected to be handled
> in drivers and none of the drivers care about it, excluding pxe, where
> 'grub_pxe_card' is statically allocated. Or am I missing something?
>
> As for ieee1275 I'm thinking about something like:
>
> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c
> b/grub-core/net/drivers/ieee1275/ofnet.c
> index 6bd3b92..12a4289 100644
> --- a/grub-core/net/drivers/ieee1275/ofnet.c
> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
> @@ -473,9 +473,28 @@ GRUB_MOD_INIT(ofnet)
>  GRUB_MOD_FINI(ofnet)
>  {
>    struct grub_net_card *card, *next;
> +  struct grub_ofnetcard_data *ofdata;
>
>    FOR_NET_CARDS_SAFE (card, next)
>      if (card->driver && grub_strcmp (card->driver->name, "ofnet") == 0)
> -      grub_net_card_unregister (card);
> +      {
> +       grub_net_card_unregister (card);
> +
> +       /*
> +        * The fact that we are here means the card was successfully
> +        * initialized in the past, so all the below pointers are valid,
> +        * and we may free associated memory without checks.
> +        */
> +
> +       ofdata = (struct grub_ofnetcard_data *) card->data;
> +       grub_free (ofdata->path);
> +       grub_free (ofdata->suffix);
> +       grub_free (ofdata);
> +
> +       ofnet_free_netbuf (card->txbuf, card->txbufsize);
> +       ofnet_free_netbuf (card->rcvbuf, card->rcvbufsize);
> +
> +       grub_free (card);
> +      }
>    grub_ieee1275_net_config = 0;
>  }
>
> (not tested)
>
> I think it deserves a separate patch. In one patch we are adding the
> receive buffer, and in another we are making the ieee1275 driver to free
> all card resources on unload.

Make sense for me. Could you do the same thing for other modules (at
least for those mentioned by you) too? Of course in separate patches.

Daniel

Re: [PATCH 2/2] ofnet: implement a receive buffer

[Stanislav Kholmanskikh]

On 11/23/2016 02:16 PM, Daniel Kiper wrote:
> On Tue, Nov 22, 2016 at 05:08:25PM +0300, Stanislav Kholmanskikh wrote:
>> On 11/22/2016 12:48 AM, Daniel Kiper wrote:
>>> On Fri, Nov 18, 2016 at 04:29:24PM +0300, Stanislav Kholmanskikh wrote:
>>>> On 11/16/2016 01:34 AM, Daniel Kiper wrote:
>>>>> On Tue, Apr 12, 2016 at 03:39:56PM +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.
>>>>>
>>>>> Have you checked performance impact of this patch? It should not be
>>>>> meaningful but it is good to know.
>>>>
>>>> No. I didn't do performance testing.
>>>
>>> Please do.
>>
>> Ok. I'll check what I can do here.
> 
> Great! Thnaks!
> 
>>>>>> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
>>>>>> ---
>>>>>>  grub-core/net/drivers/ieee1275/ofnet.c |  100 ++++++++++++++++++-------------
>>>>>>  1 files changed, 58 insertions(+), 42 deletions(-)
>>>>>>
>>>>>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
>>>>>> index 6bd3b92..09ec77e 100644
>>>>>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>>>>>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>>>>>> @@ -85,24 +85,35 @@ 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));
>>>>>
>>>>> Why 200? Please avoid using plain numbers if possible. Use constants. If it does
>>>>> not make sense then put comment which explains why this figure not another.
>>>>
>>>> The whole 'do while' construction is from the existing code, I only
>>>> modify the destination for the grub_ieee1275_read() call.
>>>
>>> OK but if you move such code around anyway do not leave it unreadable. Improve it
>>> by constants or comments.
>>
>> May I use a macro for this
>>
>> #define READ_TIMEOUT 200
>>
>> ?
> 
> It seems to me that it appears in one place, so, comment would be better here.
> Unfortunately, it looks that there is no explanation for that value in commit
> message... Ehhh... Could you check mail archive? If there is no such thing there
> then let's leave it as it. Though I do not like it.

Ok. I'll check the mail archive as well.

> 
>>>>> Additionally, are we sure that whole packet can be always stored in dev->rcvbuf?
>>>>
>>>> Code in search_net_devices() allocates the buffer to be of size:
>>>>
>>>> ALIGN_UP (card->mtu, 64) + 256;
>>>>
>>>> so, yes, it's capable to handle any valid packet size.
>>>
>>> Great but why this numbers?
>>
>> I have to admit that I can't answer to your question. :( I copied this
>> stuff from efi (for the receive buffer). The transmit buffer was already
>> of this size.
> 
> Ugh... Same as above...
> 
> [...]
> 
>>>>>>  static struct grub_net_card_driver ofdriver =
>>>>>> @@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
>>>>>>    }
>>>>>>  }
>>>>>>
>>>>>> +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_malloc (len);
>>>
>>> It looks that it should be grub_zalloc() instead of grub_malloc() here.
>>
>> I have two reasons why I don't use grub_zalloc() here:
>>
>> 1. The buffer allocated with this function is written/read many times
>> while grub is working. We write some amount of bytes to the buffer, and
>> then read this amount of bytes. So I don't see why zeroing the buffer on
> 
> I suppose that "this" == "same" here...
> 
>> allocation should matter.
>>
>> 2. In IEEE1275-1994 I do not see an explicit notice that memory
>> allocated with alloc-mem is zeroed. So for consistence of
>> ofnet_alloc_netbuf() I do not call grub_zalloc() there.
> 
> Yep, I am aware of that. However, I am asking because you change
> currently exiting code behavior which uses grub_zalloc(). Maybe
> we should leave it as is (I am thinking about grub_zalloc()) but
> it is not very strong must. If you choose grub_malloc() please
> explain in commit message why you did it and why it is safe here.

Well, let it be grub_zalloc() then.

> 
>>>>>> +}
>>>>>> +
>>>>>> +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)
>>>>>>  {
>>>>>> @@ -409,41 +438,21 @@ 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;
>>>>>>
>>>>>> -  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)
>>>>>> +    goto fail;
>>>>>> +
>>>>>> +  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;
>>>>>>      }
>>>
>>> Should not we free card->rcvbuf and/or card->txbuf if module is
>>> unloaded or something like that?
>>
>> Yes, I think so. Thanks for pointing at this.
>>
>> It's interesting that none of uboot, efi, ieee1275 drivers seems to care
>> of freeing the card data structure on module unload. All they do is
>> similar to:
>>
>> FOR_NET_CARDS_SAFE (card, next)
>>   if (the card is handled by us)
>>     grub_net_card_unregister (card);
>>
>> whereas from grub-core/net/net.c I don't see that
>> grub_net_card_unregister() frees memory.
>>
>> It seems that the job of freeing card's memory is expected to be handled
>> in drivers and none of the drivers care about it, excluding pxe, where
>> 'grub_pxe_card' is statically allocated. Or am I missing something?
>>
>> As for ieee1275 I'm thinking about something like:
>>
>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c
>> b/grub-core/net/drivers/ieee1275/ofnet.c
>> index 6bd3b92..12a4289 100644
>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>> @@ -473,9 +473,28 @@ GRUB_MOD_INIT(ofnet)
>>  GRUB_MOD_FINI(ofnet)
>>  {
>>    struct grub_net_card *card, *next;
>> +  struct grub_ofnetcard_data *ofdata;
>>
>>    FOR_NET_CARDS_SAFE (card, next)
>>      if (card->driver && grub_strcmp (card->driver->name, "ofnet") == 0)
>> -      grub_net_card_unregister (card);
>> +      {
>> +       grub_net_card_unregister (card);
>> +
>> +       /*
>> +        * The fact that we are here means the card was successfully
>> +        * initialized in the past, so all the below pointers are valid,
>> +        * and we may free associated memory without checks.
>> +        */
>> +
>> +       ofdata = (struct grub_ofnetcard_data *) card->data;
>> +       grub_free (ofdata->path);
>> +       grub_free (ofdata->suffix);
>> +       grub_free (ofdata);
>> +
>> +       ofnet_free_netbuf (card->txbuf, card->txbufsize);
>> +       ofnet_free_netbuf (card->rcvbuf, card->rcvbufsize);
>> +
>> +       grub_free (card);
>> +      }
>>    grub_ieee1275_net_config = 0;
>>  }
>>
>> (not tested)
>>
>> I think it deserves a separate patch. In one patch we are adding the
>> receive buffer, and in another we are making the ieee1275 driver to free
>> all card resources on unload.
> 
> Make sense for me. Could you do the same thing for other modules (at
> least for those mentioned by you) too? Of course in separate patches.

I'll do this for ieee1275. As for efi/uboot, I think I can also do this,
but later, since testing this may take some time. I'd prefer to play
with efi/uboot after finishing with this series :)

Regarding this series. It seems we have all questions answered and the
ball is on my side. I'll try to come up with V2 someday next week.

Thank you for your time reviewing this!

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

Re: [PATCH 2/2] ofnet: implement a receive buffer

[Daniel Kiper]

On Wed, Nov 23, 2016 at 06:09:52PM +0300, Stanislav Kholmanskikh wrote:

[...]

> I'll do this for ieee1275. As for efi/uboot, I think I can also do this,
> but later, since testing this may take some time. I'd prefer to play
> with efi/uboot after finishing with this series :)

No problem.

> Regarding this series. It seems we have all questions answered and the
> ball is on my side. I'll try to come up with V2 someday next week.

Great!

> Thank you for your time reviewing this!

You are welcome!

Daniel

Re: [PATCH 2/2] ofnet: implement a receive buffer

[Stanislav Kholmanskikh]

On 11/23/2016 06:09 PM, Stanislav Kholmanskikh wrote:
> 
> 
> On 11/23/2016 02:16 PM, Daniel Kiper wrote:
>> On Tue, Nov 22, 2016 at 05:08:25PM +0300, Stanislav Kholmanskikh wrote:
>>> On 11/22/2016 12:48 AM, Daniel Kiper wrote:
>>>> On Fri, Nov 18, 2016 at 04:29:24PM +0300, Stanislav Kholmanskikh wrote:
>>>>> On 11/16/2016 01:34 AM, Daniel Kiper wrote:
>>>>>> On Tue, Apr 12, 2016 at 03:39:56PM +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.
>>>>>>
>>>>>> Have you checked performance impact of this patch? It should not be
>>>>>> meaningful but it is good to know.
>>>>>
>>>>> No. I didn't do performance testing.
>>>>
>>>> Please do.
>>>
>>> Ok. I'll check what I can do here.
>>
>> Great! Thnaks!
>>
>>>>>>> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
>>>>>>> ---
>>>>>>>  grub-core/net/drivers/ieee1275/ofnet.c |  100 ++++++++++++++++++-------------
>>>>>>>  1 files changed, 58 insertions(+), 42 deletions(-)
>>>>>>>
>>>>>>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c b/grub-core/net/drivers/ieee1275/ofnet.c
>>>>>>> index 6bd3b92..09ec77e 100644
>>>>>>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>>>>>>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>>>>>>> @@ -85,24 +85,35 @@ 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));
>>>>>>
>>>>>> Why 200? Please avoid using plain numbers if possible. Use constants. If it does
>>>>>> not make sense then put comment which explains why this figure not another.
>>>>>
>>>>> The whole 'do while' construction is from the existing code, I only
>>>>> modify the destination for the grub_ieee1275_read() call.
>>>>
>>>> OK but if you move such code around anyway do not leave it unreadable. Improve it
>>>> by constants or comments.
>>>
>>> May I use a macro for this
>>>
>>> #define READ_TIMEOUT 200
>>>
>>> ?
>>
>> It seems to me that it appears in one place, so, comment would be better here.
>> Unfortunately, it looks that there is no explanation for that value in commit
>> message... Ehhh... Could you check mail archive? If there is no such thing there
>> then let's leave it as it. Though I do not like it.
> 
> Ok. I'll check the mail archive as well.

What I found is that this timeout of 200 ms was introduced by commit:

commit 0f231af8ae44b6e4efe6b25794db21fbfd270718
Author: Manoel Rebelo Abranches <mrabran@br.ibm.com>
Date:   Tue May 10 09:50:18 2011 -0300

    Implement timeout when receiving packets.

It seems this commit has roots here:

http://lists.gnu.org/archive/html/grub-devel/2011-05/msg00041.html

where my search stops.


> 
>>
>>>>>> Additionally, are we sure that whole packet can be always stored in dev->rcvbuf?
>>>>>
>>>>> Code in search_net_devices() allocates the buffer to be of size:
>>>>>
>>>>> ALIGN_UP (card->mtu, 64) + 256;
>>>>>
>>>>> so, yes, it's capable to handle any valid packet size.
>>>>
>>>> Great but why this numbers?
>>>
>>> I have to admit that I can't answer to your question. :( I copied this
>>> stuff from efi (for the receive buffer). The transmit buffer was already
>>> of this size.
>>
>> Ugh... Same as above...

It seems that commit:

commit 3e7472395127dc231c0f7139600b0465f68d0095
Author: Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
Date:   Sat Jun 9 11:00:18 2012 +0200

        Keep TX and RX buffers on EFI rather than always allocate new ones.

        * include/grub/net.h (grub_net_card_driver): Allow driver to modify
        card. All users updated.
        (grub_net_card): New members txbuf, rcvbuf, rcvbufsize and txbusy.
        * grub-core/net/drivers/efi/efinet.c (send_card_buffer): Reuse
buffer.
        (get_card_packet): Likewise.
        (grub_efinet_findcards): Init new fields.


was the first one to use ALIGN_UP (card->mtu, 64) + 256 for the receive
buffer. Before that the buffer size was hard coded to 1536.

I could not find an email message describing this change...


>>
>> [...]
>>
>>>>>>>  static struct grub_net_card_driver ofdriver =
>>>>>>> @@ -294,6 +305,24 @@ grub_ieee1275_net_config_real (const char *devpath, char **device, char **path,
>>>>>>>    }
>>>>>>>  }
>>>>>>>
>>>>>>> +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_malloc (len);
>>>>
>>>> It looks that it should be grub_zalloc() instead of grub_malloc() here.
>>>
>>> I have two reasons why I don't use grub_zalloc() here:
>>>
>>> 1. The buffer allocated with this function is written/read many times
>>> while grub is working. We write some amount of bytes to the buffer, and
>>> then read this amount of bytes. So I don't see why zeroing the buffer on
>>
>> I suppose that "this" == "same" here...
>>
>>> allocation should matter.
>>>
>>> 2. In IEEE1275-1994 I do not see an explicit notice that memory
>>> allocated with alloc-mem is zeroed. So for consistence of
>>> ofnet_alloc_netbuf() I do not call grub_zalloc() there.
>>
>> Yep, I am aware of that. However, I am asking because you change
>> currently exiting code behavior which uses grub_zalloc(). Maybe
>> we should leave it as is (I am thinking about grub_zalloc()) but
>> it is not very strong must. If you choose grub_malloc() please
>> explain in commit message why you did it and why it is safe here.
> 
> Well, let it be grub_zalloc() then.
> 
>>
>>>>>>> +}
>>>>>>> +
>>>>>>> +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)
>>>>>>>  {
>>>>>>> @@ -409,41 +438,21 @@ 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;
>>>>>>>
>>>>>>> -  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)
>>>>>>> +    goto fail;
>>>>>>> +
>>>>>>> +  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;
>>>>>>>      }
>>>>
>>>> Should not we free card->rcvbuf and/or card->txbuf if module is
>>>> unloaded or something like that?
>>>
>>> Yes, I think so. Thanks for pointing at this.
>>>
>>> It's interesting that none of uboot, efi, ieee1275 drivers seems to care
>>> of freeing the card data structure on module unload. All they do is
>>> similar to:
>>>
>>> FOR_NET_CARDS_SAFE (card, next)
>>>   if (the card is handled by us)
>>>     grub_net_card_unregister (card);
>>>
>>> whereas from grub-core/net/net.c I don't see that
>>> grub_net_card_unregister() frees memory.
>>>
>>> It seems that the job of freeing card's memory is expected to be handled
>>> in drivers and none of the drivers care about it, excluding pxe, where
>>> 'grub_pxe_card' is statically allocated. Or am I missing something?
>>>
>>> As for ieee1275 I'm thinking about something like:
>>>
>>> diff --git a/grub-core/net/drivers/ieee1275/ofnet.c
>>> b/grub-core/net/drivers/ieee1275/ofnet.c
>>> index 6bd3b92..12a4289 100644
>>> --- a/grub-core/net/drivers/ieee1275/ofnet.c
>>> +++ b/grub-core/net/drivers/ieee1275/ofnet.c
>>> @@ -473,9 +473,28 @@ GRUB_MOD_INIT(ofnet)
>>>  GRUB_MOD_FINI(ofnet)
>>>  {
>>>    struct grub_net_card *card, *next;
>>> +  struct grub_ofnetcard_data *ofdata;
>>>
>>>    FOR_NET_CARDS_SAFE (card, next)
>>>      if (card->driver && grub_strcmp (card->driver->name, "ofnet") == 0)
>>> -      grub_net_card_unregister (card);
>>> +      {
>>> +       grub_net_card_unregister (card);
>>> +
>>> +       /*
>>> +        * The fact that we are here means the card was successfully
>>> +        * initialized in the past, so all the below pointers are valid,
>>> +        * and we may free associated memory without checks.
>>> +        */
>>> +
>>> +       ofdata = (struct grub_ofnetcard_data *) card->data;
>>> +       grub_free (ofdata->path);
>>> +       grub_free (ofdata->suffix);
>>> +       grub_free (ofdata);
>>> +
>>> +       ofnet_free_netbuf (card->txbuf, card->txbufsize);
>>> +       ofnet_free_netbuf (card->rcvbuf, card->rcvbufsize);
>>> +
>>> +       grub_free (card);
>>> +      }
>>>    grub_ieee1275_net_config = 0;
>>>  }
>>>
>>> (not tested)
>>>
>>> I think it deserves a separate patch. In one patch we are adding the
>>> receive buffer, and in another we are making the ieee1275 driver to free
>>> all card resources on unload.
>>
>> Make sense for me. Could you do the same thing for other modules (at
>> least for those mentioned by you) too? Of course in separate patches.
> 
> I'll do this for ieee1275. As for efi/uboot, I think I can also do this,
> but later, since testing this may take some time. I'd prefer to play
> with efi/uboot after finishing with this series :)
> 
> Regarding this series. It seems we have all questions answered and the
> ball is on my side. I'll try to come up with V2 someday next week.
> 
> Thank you for your time reviewing this!
> 
>>
>> 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
> 

Re: [PATCH 1/2] ieee1275: alloc-mem and free-mem

[Daniel Kiper]

On Tue, Apr 12, 2016 at 03:39:55PM +0300, Stanislav Kholmanskikh wrote:
> Add wrappers for memory allocation using
> alloc-mem and free-mem commands from the User Interface.

Please tell why it is needed. Additionally, please forgive me if it is stupid
question, why are you using command line to allocate/free memory? There is
a lack of better API in IEEE 1275?

> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
> ---
>  grub-core/kern/ieee1275/openfw.c |   68 ++++++++++++++++++++++++++++++++++++++
>  include/grub/ieee1275/ieee1275.h |    2 +
>  2 files changed, 70 insertions(+), 0 deletions(-)
>
> diff --git a/grub-core/kern/ieee1275/openfw.c b/grub-core/kern/ieee1275/openfw.c
> index ddb7783..35225ec 100644
> --- a/grub-core/kern/ieee1275/openfw.c
> +++ b/grub-core/kern/ieee1275/openfw.c
> @@ -561,3 +561,71 @@ grub_ieee1275_canonicalise_devname (const char *path)
>    return NULL;
>  }
>
> +/* Allocate memory with alloc-mem */
> +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)

I think that this can be in one line.

> +    {
> +      grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed"));
> +      return NULL;
> +    }
> +  else
> +    return (void *)args.result;
> +}
> +
> +/* Free memory allocated by alloc-mem */
> +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)

Ditto.

Daniel

Re: [PATCH 1/2] ieee1275: alloc-mem and free-mem

[Stanislav Kholmanskikh]

Hi, Daniel.

Thank you for review.

My comments are below.

On 11/16/2016 12:22 AM, Daniel Kiper wrote:
> On Tue, Apr 12, 2016 at 03:39:55PM +0300, Stanislav Kholmanskikh wrote:
>> Add wrappers for memory allocation using
>> alloc-mem and free-mem commands from the User Interface.
> 
> Please tell why it is needed. Additionally, please forgive me if it is stupid
> question, why are you using command line to allocate/free memory? There is
> a lack of better API in IEEE 1275?

In the current git code in grub-core/net/drivers/ieee1275/ofnet.c the
search_net_devices() function uses the "alloc-mem" command for
allocation of the transmit buffer for the case when
GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set.

Yes, besides "alloc-mem", "free-mem", there are other options for
allocating memory. But, to be honest, I don't know why grub_malloc()
cannot be used for the receive buffer on those systems. From the flag's
name it seems those systems don't have a MMU. Ok, but grub_malloc() is
called from many places in grub, and, presumably, grub works on those
systems. I don't have such a MacRISC system to try grub_malloc() for the
buffers (grub-core/kern/ieee1275/cmain.c).

In patch 2 I'm implementing a receive buffer, so I decided to keep this
case as is, but move "alloc-mem", "free-mem" into functions.

Does it answer your questions? If yes, I'll update the commit message
for V2 with a more detailed explanation why these functions are needed.

> 
>> Signed-off-by: Stanislav Kholmanskikh <stanislav.kholmanskikh@oracle.com>
>> ---
>>  grub-core/kern/ieee1275/openfw.c |   68 ++++++++++++++++++++++++++++++++++++++
>>  include/grub/ieee1275/ieee1275.h |    2 +
>>  2 files changed, 70 insertions(+), 0 deletions(-)
>>
>> diff --git a/grub-core/kern/ieee1275/openfw.c b/grub-core/kern/ieee1275/openfw.c
>> index ddb7783..35225ec 100644
>> --- a/grub-core/kern/ieee1275/openfw.c
>> +++ b/grub-core/kern/ieee1275/openfw.c
>> @@ -561,3 +561,71 @@ grub_ieee1275_canonicalise_devname (const char *path)
>>    return NULL;
>>  }
>>
>> +/* Allocate memory with alloc-mem */
>> +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)
> 
> I think that this can be in one line.

Ok.

> 
>> +    {
>> +      grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed"));
>> +      return NULL;
>> +    }
>> +  else
>> +    return (void *)args.result;
>> +}
>> +
>> +/* Free memory allocated by alloc-mem */
>> +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)
> 
> Ditto.

Ok.

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

Re: [PATCH 1/2] ieee1275: alloc-mem and free-mem

[Vladimir 'phcoder' Serbinenko]

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

On Fri, Nov 18, 2016, 13:33 Stanislav Kholmanskikh <
stanislav.kholmanskikh@oracle.com> wrote:

> Hi, Daniel.
>
> Thank you for review.
>
> My comments are below.
>
> On 11/16/2016 12:22 AM, Daniel Kiper wrote:
> > On Tue, Apr 12, 2016 at 03:39:55PM +0300, Stanislav Kholmanskikh wrote:
> >> Add wrappers for memory allocation using
> >> alloc-mem and free-mem commands from the User Interface.
> >
> > Please tell why it is needed. Additionally, please forgive me if it is
> stupid
> > question, why are you using command line to allocate/free memory? There
> is
> > a lack of better API in IEEE 1275?
>
> In the current git code in grub-core/net/drivers/ieee1275/ofnet.c the
> search_net_devices() function uses the "alloc-mem" command for
> allocation of the transmit buffer for the case when
> GRUB_IEEE1275_FLAG_VIRT_TO_REAL_BROKEN is set.
>
> Yes, besides "alloc-mem", "free-mem", there are other options for
> allocating memory. But, to be honest, I don't know why grub_malloc()
> cannot be used for the receive buffer on those systems. From the flag's
> name it seems those systems don't have a MMU. Ok, but grub_malloc() is
> called from many places in grub, and, presumably, grub works on those
> systems. I don't have such a MacRISC system to try grub_malloc() for the
> buffers (grub-core/kern/ieee1275/cmain.c).
>
On my test system if I try to use grub_malloc, then the network driver
tries to do some dubious pointer arithmetic and puts data in a completely
wrong location unless alloc-mem was used

>
> In patch 2 I'm implementing a receive buffer, so I decided to keep this
> case as is, but move "alloc-mem", "free-mem" into functions.
>
> Does it answer your questions? If yes, I'll update the commit message
> for V2 with a more detailed explanation why these functions are needed.
>
> >
> >> Signed-off-by: Stanislav Kholmanskikh <
> stanislav.kholmanskikh@oracle.com>
> >> ---
> >>  grub-core/kern/ieee1275/openfw.c |   68
> ++++++++++++++++++++++++++++++++++++++
> >>  include/grub/ieee1275/ieee1275.h |    2 +
> >>  2 files changed, 70 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/grub-core/kern/ieee1275/openfw.c
> b/grub-core/kern/ieee1275/openfw.c
> >> index ddb7783..35225ec 100644
> >> --- a/grub-core/kern/ieee1275/openfw.c
> >> +++ b/grub-core/kern/ieee1275/openfw.c
> >> @@ -561,3 +561,71 @@ grub_ieee1275_canonicalise_devname (const char
> *path)
> >>    return NULL;
> >>  }
> >>
> >> +/* Allocate memory with alloc-mem */
> >> +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)
> >
> > I think that this can be in one line.
>
> Ok.
>
> >
> >> +    {
> >> +      grub_error (GRUB_ERR_INVALID_COMMAND, N_("alloc-mem failed"));
> >> +      return NULL;
> >> +    }
> >> +  else
> >> +    return (void *)args.result;
> >> +}
> >> +
> >> +/* Free memory allocated by alloc-mem */
> >> +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)
> >
> > Ditto.
>
> Ok.
>
> >
> > 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: Type: text/html, Size: 6691 bytes --]