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

[Vladimir 'phcoder' Serbinenko]

On Fri, Nov 18, 2016, 13:33 Stanislav Kholmanskikh <address@hidden> 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 <address@hidden>
>> ---
>>  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
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

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