From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1d8dGb-0003VI-TT for mharc-grub-devel@gnu.org; Wed, 10 May 2017 21:51:45 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54875) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d8dGY-0003Uk-Sm for grub-devel@gnu.org; Wed, 10 May 2017 21:51:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d8dGX-0002WP-7y for grub-devel@gnu.org; Wed, 10 May 2017 21:51:42 -0400 Received: from mail-yb0-x242.google.com ([2607:f8b0:4002:c09::242]:35009) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1d8dGX-0002WG-2N for grub-devel@gnu.org; Wed, 10 May 2017 21:51:41 -0400 Received: by mail-yb0-x242.google.com with SMTP id 19so465402ybl.2 for ; Wed, 10 May 2017 18:51:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=g/JMjpnjY03rhz3tJD8eUTL8Rlaxy9cMY7Kc+CMUZUE=; b=OQFunHrIuhk9jLOCrG4g23W9aNflEu5WNZ6YVFdQdc5XjwsNiibqDNMsBzmyNFwKKn gvumq7TJtrct3Jr33oZkQ/5j8vstALQ2/ebjAnMq48uSoXGVS56tSlSVV4uuKDJItFu5 Hurnf6gMfRx/fHD333rkQlLsKLF7WFj4kpY4R8IVT0LAv47EeCxXuUA36U4QEq/jbzj4 7kFtdIDfuI0s07HEEQg/UYVFVoRvT4nX2A2jz8nnAotxQFpKG+C69rwF2SgwP5wwKq54 KBPMnNW5V0TCFuNPYFgyGzpFucVbvs8kVyEkWq01XOZ3F/rpOWlTMBmVVDNQGxlWefvX vchw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=g/JMjpnjY03rhz3tJD8eUTL8Rlaxy9cMY7Kc+CMUZUE=; b=ivIeklknt2ObQElxuXTnKcD3Es2Tk9qV+sG+enqXDUy+TpB+y2MCFcUYAajvFUVmGR OybhMI+Iuh2wEXTs2zi5XJWk1B+Omsxnfc0TMRe5OMFSem0WwmfjJHsKmCnxhUvzt/zN XBWQh4ZZsmu3+mMLcM7K3CUWMwuipLAainSywK3yQUgyyjEFvo/227uXMGX2klbAoUk9 EW3YyokaRGGVVcIeplEHPl8/0e7TNlYJTM7hnGaBrEtWMpYVCwZ6HH80MtM0m2skXy5u mil2ohOvbPFt67kHldnLHCF0Mmc3mM5PFoCIuZjUyrB/FhZLWCBGeIwRGInfsyCidimf 54RQ== X-Gm-Message-State: AODbwcDFekX8lBXrTfgLT0wGpEaJCtfkGghYpVoIGBTNbYQkcqu9CWCH v7k9LM0vQmjNRSrJFt6ogiOr3afzMQ== X-Received: by 10.37.196.69 with SMTP id u66mr7675422ybf.11.1494467500356; Wed, 10 May 2017 18:51:40 -0700 (PDT) MIME-Version: 1.0 References: <1460464796-24738-1-git-send-email-stanislav.kholmanskikh@oracle.com> <20161115212243.GH16470@router-fw-old.local.net-space.pl> <582EF4EB.1040305@oracle.com> In-Reply-To: <582EF4EB.1040305@oracle.com> From: "Vladimir 'phcoder' Serbinenko" Date: Thu, 11 May 2017 01:51:29 +0000 Message-ID: Subject: Re: [PATCH 1/2] ieee1275: alloc-mem and free-mem To: The development of GRUB 2 Cc: vasily.isaenko@oracle.com Content-Type: multipart/alternative; boundary=94eb2c054ae47cba8f054f35d443 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4002:c09::242 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 May 2017 01:51:44 -0000 --94eb2c054ae47cba8f054f35d443 Content-Type: text/plain; charset=UTF-8 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 > --94eb2c054ae47cba8f054f35d443 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On Fri, Nov 18, 2016, 1= 3: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? Ther= e 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 o= ptions 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<= br> 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 th= e
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 loca= tion 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 funct= ions.

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@oracl= e.com>
>> ---
>>=C2=A0 grub-core/kern/ieee1275/openfw.c |=C2=A0 =C2=A068 ++++++++++= ++++++++++++++++++++++++++++
>>=C2=A0 include/grub/ieee1275/ieee1275.h |=C2=A0 =C2=A0 2 +
>>=C2=A0 2 files changed, 70 insertions(+), 0 deletions(-)
>>
>> diff --git a/grub-core/kern/ieee1275/openfw.c b/grub-core/kern/iee= e1275/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 cha= r *path)
>>=C2=A0 =C2=A0 return NULL;
>>=C2=A0 }
>>
>> +/* Allocate memory with alloc-mem */
>> +void *
>> +grub_ieee1275_alloc_mem (grub_size_t len)
>> +{
>> +=C2=A0 struct alloc_args
>> +=C2=A0 {
>> +=C2=A0 =C2=A0 struct grub_ieee1275_common_hdr common;
>> +=C2=A0 =C2=A0 grub_ieee1275_cell_t method;
>> +=C2=A0 =C2=A0 grub_ieee1275_cell_t len;
>> +=C2=A0 =C2=A0 grub_ieee1275_cell_t catch;
>> +=C2=A0 =C2=A0 grub_ieee1275_cell_t result;
>> +=C2=A0 }
>> +=C2=A0 args;
>> +
>> +=C2=A0 if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INT= ERPRET))
>> +=C2=A0 =C2=A0 {
>> +=C2=A0 =C2=A0 =C2=A0 grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_(&qu= ot;interpret is not supported"));
>> +=C2=A0 =C2=A0 =C2=A0 return NULL;
>> +=C2=A0 =C2=A0 }
>> +
>> +=C2=A0 INIT_IEEE1275_COMMON (&args.common, "interpret&qu= ot;, 2, 2);
>> +=C2=A0 args.len =3D len;
>> +=C2=A0 args.method =3D (grub_ieee1275_cell_t) "alloc-mem&quo= t;;
>> +
>> +=C2=A0 if (IEEE1275_CALL_ENTRY_FN (&args) =3D=3D -1
>> +=C2=A0 =C2=A0 =C2=A0 || args.catch)
>
> I think that this can be in one line.

Ok.

>
>> +=C2=A0 =C2=A0 {
>> +=C2=A0 =C2=A0 =C2=A0 grub_error (GRUB_ERR_INVALID_COMMAND, N_(&qu= ot;alloc-mem failed"));
>> +=C2=A0 =C2=A0 =C2=A0 return NULL;
>> +=C2=A0 =C2=A0 }
>> +=C2=A0 else
>> +=C2=A0 =C2=A0 return (void *)args.result;
>> +}
>> +
>> +/* Free memory allocated by alloc-mem */
>> +grub_err_t
>> +grub_ieee1275_free_mem (void *addr, grub_size_t len)
>> +{
>> +=C2=A0 struct free_args
>> +=C2=A0 {
>> +=C2=A0 =C2=A0 struct grub_ieee1275_common_hdr common;
>> +=C2=A0 =C2=A0 grub_ieee1275_cell_t method;
>> +=C2=A0 =C2=A0 grub_ieee1275_cell_t len;
>> +=C2=A0 =C2=A0 grub_ieee1275_cell_t addr;
>> +=C2=A0 =C2=A0 grub_ieee1275_cell_t catch;
>> +=C2=A0 }
>> +=C2=A0 args;
>> +
>> +=C2=A0 if (grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_CANNOT_INT= ERPRET))
>> +=C2=A0 =C2=A0 {
>> +=C2=A0 =C2=A0 =C2=A0 grub_error (GRUB_ERR_UNKNOWN_COMMAND, N_(&qu= ot;interpret is not supported"));
>> +=C2=A0 =C2=A0 =C2=A0 return grub_errno;
>> +=C2=A0 =C2=A0 }
>> +
>> +=C2=A0 INIT_IEEE1275_COMMON (&args.common, "interpret&qu= ot;, 3, 1);
>> +=C2=A0 args.addr =3D (grub_ieee1275_cell_t)addr;
>> +=C2=A0 args.len =3D len;
>> +=C2=A0 args.method =3D (grub_ieee1275_cell_t) "free-mem"= ;;
>> +
>> +=C2=A0 if (IEEE1275_CALL_ENTRY_FN(&args) =3D=3D -1
>> +=C2=A0 =C2=A0 =C2=A0 || args.catch)
>
> Ditto.

Ok.

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

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org<= /a>
https://lists.gnu.org/mailman/listinfo/grub-devel
--94eb2c054ae47cba8f054f35d443--