Re: [PATCH] arm64: Fix the bug in fdt module
Hi Vladimir
Please correct me :-)
Can I do this :
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 2ef10d1..ea5831f 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1665,7 +1665,6 @@ module = {
ia64_efi = loader/ia64/efi/linux.c;
arm = loader/arm/linux.c;
arm64 = loader/arm64/linux.c;
- fdt = lib/fdt.c;
common = loader/linux.c;
common = lib/cmdline.c;
enable = noemu;
@@ -1674,7 +1673,9 @@ module = {
module = {
name = fdt;
arm64 = loader/arm64/fdt.c;
- enable = arm64;
+ arm = loader/arm/fdt.c;
+ common = lib/fdt.c;
+ enable = fdt;
};
module = {
On 4 November 2015 at 19:16, Vladimir 'phcoder' Serbinenko
wrote:
>
> Le 4 nov. 2015 12:12 PM, "Vladimir 'phcoder' Serbinenko"
> a écrit :
>>
>>
>> Le 4 nov. 2015 12:09 PM, "Fu Wei" a écrit :
>> >
>> > Hi Vladimir,
>> >
>> > On 4 November 2015 at 19:06, Fu Wei wrote:
>> > > Hi Vladimir,
>> > >
>> > > On 4 November 2015 at 18:47, Vladimir 'phcoder' Serbinenko
>> > > wrote:
>> > >>
>> > >> Le 4 nov. 2015 10:48 AM, "Fu Wei" a écrit :
>> > >>>
>> > >>> Hi Vladimir,
>> > >>>
>> > >>> Great thanks for your help :-)
>> > >>>
>> > >>>
>> > >>> On 4 November 2015 at 02:07, Vladimir 'phcoder' Serbinenko
>> > >>> wrote:
>> > >>> >
>> > >>> > Le 3 nov. 2015 9:56 AM, a écrit :
>> > >>> >>
>> > >>> >> From: Fu Wei
>> > >>> >>
>> > >>> >> This patch goes with commit:
>> > >>> >> 4d0cb755387d6f109b901386ed4d3d475df239fe
>> > >>> >> arm64: Move FDT functions to separate module
>> > >>> >>
>> > >>> >> linux and xen_boot modules can't work without this patch.
>> > >>> >>
>> > >>> >> Signed-off-by: Fu Wei
>> > >>> >> ---
>> > >>> >> grub-core/Makefile.core.def | 1 +
>> > >>> >> grub-core/loader/arm64/fdt.c | 5 +
>> > >>> >> 2 files changed, 6 insertions(+)
>> > >>> >>
>> > >>> >> diff --git a/grub-core/Makefile.core.def
>> > >>> >> b/grub-core/Makefile.core.def
>> > >>> >> index 2ef10d1..3ea4e49 100644
>> > >>> >> --- a/grub-core/Makefile.core.def
>> > >>> >> +++ b/grub-core/Makefile.core.def
>> > >>> >> @@ -1674,6 +1674,7 @@ module = {
>> > >>> >> module = {
>> > >>> >>name = fdt;
>> > >>> >>arm64 = loader/arm64/fdt.c;
>> > >>> >> + fdt = lib/fdt.c;
>> > >>> >>enable = arm64;
>> > >>> >> };
>> > >>> >>
>> > >>> > Please don't add same file to 2 different modules. Remove it from
>> > >>> > Linux
>> > >>> > module
>> > >>>
>> > >>> AFAIK, for now , only arm and arm64 are using lib/fdt.c
>> > >>> So please allow me to separate all the fdt code from
>> > >>> loader/arm/linux.c; just like arm64.
>> > >>>
>> > >> I don't think it's necessary at this point. Is xen_boot going to be
>> > >> available for 32-bit arm as well?
>> >
>> > And I would like to help to sort out the code for your new fdt module
>> > , If you don't mind.
>> >
>> > But according to my tests, I think we need "fdt = lib/fdt.c;" in
>> > module. If we don't separate fdt code from arm, we also need "fdt =
>> > lib/fdt.c;" in linux module.
>> >
>> > Please correct me if I misunderstand something, thanks
>> >
>> We need arm = lib/fdt.c in linux if we don't restructure. There is no
>> reason to include lib/fdt.c in 2 different modules on arm64. Fdt in first
>> part just mean arm and arm64 in this context
>>
> As an alternative: enable fdt module on all fdt platforms (enable = fdt) and
> put lib/fdt.c
>
>> > >>> This patch may become a patchset :-)
>> > >>>
>> > >>> >> diff --git a/grub-core/loader/arm64/fdt.c
>> > >>> >> b/grub-core/loader/arm64/fdt.c
>> > >>> >> index 5202c14..d160ca0 100644
>> > >>> >> --- a/grub-core/loader/arm64/fdt.c
>> > >>> >> +++ b/grub-core/loader/arm64/fdt.c
>> > >>> >> @@ -25,6 +25,10 @@
>> > >>> >> #include
>> > >>> >> #include
>> > >>> >>
>> > >>> >> +GRUB_MOD_LICENSE ("GPLv3+");
>> > >>> >> +
>> > >>> >> +static grub_dl_t my_mod;
>> > >>> >> +
>> > >>> > What's the reason for my_mod?
>> > >>>
>> > >>> this is for grub_dl_unref and grub_dl_ref. but I forgot to check
>> > >>> this
>> > >>> again for this, sorry,
>> > >>> will add this later
>> > >>>
>> > >> Forget dl_ref and dl_unref. Linux module having a function reference
>> > >> is
>> > >> already good enough
>> > >>> >> static void *loaded_fdt;
>> > >>> >> static void *fdt;
>> > >>> >>
>> > >>> >> @@ -177,6 +181,7 @@ GRUB_MOD_INIT (fdt)
>> > >>> >>cmd_devicetree =
>> > >>> >> grub_register_command ("devicetree", grub_cmd_devicetree, 0,
>> > >>> >>N_("Load DTB file."));
>> > >>> >> + my_mod = mod;
>> > >>> >> }
>> > >>> >>
>> > >>> >> GRUB_MOD_FINI (fdt)
>> > >>> >> --
>> > >>> >> 2.4.3
>> > >>> >>
>> > >>>
>> > >>>
>> > >>>
>> > >>> --
>> > >>> Best regards,
>> > >>>
>> > >>> Fu Wei
>> > >>> Software Engineer
>> > >>> Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
>> > >>> Ph: +86 21 61221326(direct)
>> > >>> Ph: +86 186 2020 4684 (mobile)
>> > >>> Room 1512, Regus One Corporate Avenue,Level 15,
>> > >>> One Corporate Avenue,222 Hubin Road,Huangpu District,
>> > >>> Shan
Re: [PATCH] arm64: Fix the bug in fdt module
Le 5 nov. 2015 9:51 AM, "Fu Wei" a écrit :
>
> Hi Vladimir
>
> Please correct me :-)
> Can I do this :
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 2ef10d1..ea5831f 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1665,7 +1665,6 @@ module = {
>ia64_efi = loader/ia64/efi/linux.c;
>arm = loader/arm/linux.c;
>arm64 = loader/arm64/linux.c;
> - fdt = lib/fdt.c;
>common = loader/linux.c;
>common = lib/cmdline.c;
>enable = noemu;
> @@ -1674,7 +1673,9 @@ module = {
> module = {
>name = fdt;
>arm64 = loader/arm64/fdt.c;
> - enable = arm64;
> + arm = loader/arm/fdt.c;
Where does this file come from?
> + common = lib/fdt.c;
> + enable = fdt;
> };
>
> module = {
>
> On 4 November 2015 at 19:16, Vladimir 'phcoder' Serbinenko
> wrote:
> >
> > Le 4 nov. 2015 12:12 PM, "Vladimir 'phcoder' Serbinenko" <
phco...@gmail.com>
> > a écrit :
> >>
> >>
> >> Le 4 nov. 2015 12:09 PM, "Fu Wei" a écrit :
> >> >
> >> > Hi Vladimir,
> >> >
> >> > On 4 November 2015 at 19:06, Fu Wei wrote:
> >> > > Hi Vladimir,
> >> > >
> >> > > On 4 November 2015 at 18:47, Vladimir 'phcoder' Serbinenko
> >> > > wrote:
> >> > >>
> >> > >> Le 4 nov. 2015 10:48 AM, "Fu Wei" a écrit :
> >> > >>>
> >> > >>> Hi Vladimir,
> >> > >>>
> >> > >>> Great thanks for your help :-)
> >> > >>>
> >> > >>>
> >> > >>> On 4 November 2015 at 02:07, Vladimir 'phcoder' Serbinenko
> >> > >>> wrote:
> >> > >>> >
> >> > >>> > Le 3 nov. 2015 9:56 AM, a écrit :
> >> > >>> >>
> >> > >>> >> From: Fu Wei
> >> > >>> >>
> >> > >>> >> This patch goes with commit:
> >> > >>> >> 4d0cb755387d6f109b901386ed4d3d475df239fe
> >> > >>> >> arm64: Move FDT functions to separate module
> >> > >>> >>
> >> > >>> >> linux and xen_boot modules can't work without this patch.
> >> > >>> >>
> >> > >>> >> Signed-off-by: Fu Wei
> >> > >>> >> ---
> >> > >>> >> grub-core/Makefile.core.def | 1 +
> >> > >>> >> grub-core/loader/arm64/fdt.c | 5 +
> >> > >>> >> 2 files changed, 6 insertions(+)
> >> > >>> >>
> >> > >>> >> diff --git a/grub-core/Makefile.core.def
> >> > >>> >> b/grub-core/Makefile.core.def
> >> > >>> >> index 2ef10d1..3ea4e49 100644
> >> > >>> >> --- a/grub-core/Makefile.core.def
> >> > >>> >> +++ b/grub-core/Makefile.core.def
> >> > >>> >> @@ -1674,6 +1674,7 @@ module = {
> >> > >>> >> module = {
> >> > >>> >>name = fdt;
> >> > >>> >>arm64 = loader/arm64/fdt.c;
> >> > >>> >> + fdt = lib/fdt.c;
> >> > >>> >>enable = arm64;
> >> > >>> >> };
> >> > >>> >>
> >> > >>> > Please don't add same file to 2 different modules. Remove it
from
> >> > >>> > Linux
> >> > >>> > module
> >> > >>>
> >> > >>> AFAIK, for now , only arm and arm64 are using lib/fdt.c
> >> > >>> So please allow me to separate all the fdt code from
> >> > >>> loader/arm/linux.c; just like arm64.
> >> > >>>
> >> > >> I don't think it's necessary at this point. Is xen_boot going to
be
> >> > >> available for 32-bit arm as well?
> >> >
> >> > And I would like to help to sort out the code for your new fdt module
> >> > , If you don't mind.
> >> >
> >> > But according to my tests, I think we need "fdt = lib/fdt.c;" in
> >> > module. If we don't separate fdt code from arm, we also need "fdt =
> >> > lib/fdt.c;" in linux module.
> >> >
> >> > Please correct me if I misunderstand something, thanks
> >> >
> >> We need arm = lib/fdt.c in linux if we don't restructure. There is no
> >> reason to include lib/fdt.c in 2 different modules on arm64. Fdt in
first
> >> part just mean arm and arm64 in this context
> >>
> > As an alternative: enable fdt module on all fdt platforms (enable =
fdt) and
> > put lib/fdt.c
> >
> >> > >>> This patch may become a patchset :-)
> >> > >>>
> >> > >>> >> diff --git a/grub-core/loader/arm64/fdt.c
> >> > >>> >> b/grub-core/loader/arm64/fdt.c
> >> > >>> >> index 5202c14..d160ca0 100644
> >> > >>> >> --- a/grub-core/loader/arm64/fdt.c
> >> > >>> >> +++ b/grub-core/loader/arm64/fdt.c
> >> > >>> >> @@ -25,6 +25,10 @@
> >> > >>> >> #include
> >> > >>> >> #include
> >> > >>> >>
> >> > >>> >> +GRUB_MOD_LICENSE ("GPLv3+");
> >> > >>> >> +
> >> > >>> >> +static grub_dl_t my_mod;
> >> > >>> >> +
> >> > >>> > What's the reason for my_mod?
> >> > >>>
> >> > >>> this is for grub_dl_unref and grub_dl_ref. but I forgot to check
> >> > >>> this
> >> > >>> again for this, sorry,
> >> > >>> will add this later
> >> > >>>
> >> > >> Forget dl_ref and dl_unref. Linux module having a function
reference
> >> > >> is
> >> > >> already good enough
> >> > >>> >> static void *loaded_fdt;
> >> > >>> >> static void *fdt;
> >> > >>> >>
> >> > >>> >> @@ -177,6 +181,7 @@ GRUB_MOD_INIT (fdt)
> >> > >>> >>cmd_devicetree =
> >> > >>> >> grub_register_command ("devicetree",
grub_cmd_devicetree, 0,
> >> > >>> >>N_("Load DTB file."));
> >> > >>> >> + my_mod = mod;
> >> > >>> >> }
> >> > >>> >>
> >> > >>> >> GRUB_MOD_FINI (fdt)
> >> > >>> >> --
> >> > >>> >> 2.4.3
>
Re: [PATCH v3 2/4] arm64: Add xen_boot module file
Hi Ian, On 3 November 2015 at 23:22, Ian Campbell wrote: > On Tue, 2015-11-03 at 22:57 +0800, Fu Wei wrote: >> Hi Vladimir, >> >> After discussing with Ian Campbell, Since we already can load all >> the necessary binaries for Xen boot on arm64 for now, we don't really >> need "xen_module" command now. >> But maybe someday , xen need a new type of binary in boot time, then >> we still need this support. > > You mean support for "--type" passed to the xen_module command, right? I > thought the xen_module stuff had been applied. Or am I misunderstanding > which bits have been applied? Actually, I mean: xen-module command is for "--type" support. If we don't need "--type" now, we can delete xen-module code(which has been deleted by Vladimir from my patch, so now, the upstream grub has not --type support). Vladimir has applied most of my patch, except xen-module command code. > >> So I will submit a "xen_module" command patch soon, in case we need >> it. > > Just to clarify, my suggestion was to repost the bits which were omitted > from the prior patches just so that they are available in the ML archives > etc should anyone ever want to resurrect them in the future. yes, that is what I am gonna do. > > Ian. > -- Best regards, Fu Wei Software Engineer Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch Ph: +86 21 61221326(direct) Ph: +86 186 2020 4684 (mobile) Room 1512, Regus One Corporate Avenue,Level 15, One Corporate Avenue,222 Hubin Road,Huangpu District, Shanghai,China 200021 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] arm64: Fix the bug in fdt module
Hi Vladimir
On 5 November 2015 at 17:45, Vladimir 'phcoder' Serbinenko
wrote:
>
> Le 5 nov. 2015 9:51 AM, "Fu Wei" a écrit :
>>
>> Hi Vladimir
>>
>> Please correct me :-)
>> Can I do this :
>>
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 2ef10d1..ea5831f 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -1665,7 +1665,6 @@ module = {
>>ia64_efi = loader/ia64/efi/linux.c;
>>arm = loader/arm/linux.c;
>>arm64 = loader/arm64/linux.c;
>> - fdt = lib/fdt.c;
>>common = loader/linux.c;
>>common = lib/cmdline.c;
>>enable = noemu;
>> @@ -1674,7 +1673,9 @@ module = {
>> module = {
>>name = fdt;
>>arm64 = loader/arm64/fdt.c;
>> - enable = arm64;
>> + arm = loader/arm/fdt.c;
> Where does this file come from?
>
that is the file I am working on
i just tried to let you know my thought.
Is this way Ok for you? Please correct me if I misunderstand you
>
>> + common = lib/fdt.c;
>> + enable = fdt;
>> };
>>
>> module = {
>>
>> On 4 November 2015 at 19:16, Vladimir 'phcoder' Serbinenko
>> wrote:
>> >
>> > Le 4 nov. 2015 12:12 PM, "Vladimir 'phcoder' Serbinenko"
>> >
>> > a écrit :
>> >>
>> >>
>> >> Le 4 nov. 2015 12:09 PM, "Fu Wei" a écrit :
>> >> >
>> >> > Hi Vladimir,
>> >> >
>> >> > On 4 November 2015 at 19:06, Fu Wei wrote:
>> >> > > Hi Vladimir,
>> >> > >
>> >> > > On 4 November 2015 at 18:47, Vladimir 'phcoder' Serbinenko
>> >> > > wrote:
>> >> > >>
>> >> > >> Le 4 nov. 2015 10:48 AM, "Fu Wei" a écrit :
>> >> > >>>
>> >> > >>> Hi Vladimir,
>> >> > >>>
>> >> > >>> Great thanks for your help :-)
>> >> > >>>
>> >> > >>>
>> >> > >>> On 4 November 2015 at 02:07, Vladimir 'phcoder' Serbinenko
>> >> > >>> wrote:
>> >> > >>> >
>> >> > >>> > Le 3 nov. 2015 9:56 AM, a écrit :
>> >> > >>> >>
>> >> > >>> >> From: Fu Wei
>> >> > >>> >>
>> >> > >>> >> This patch goes with commit:
>> >> > >>> >> 4d0cb755387d6f109b901386ed4d3d475df239fe
>> >> > >>> >> arm64: Move FDT functions to separate module
>> >> > >>> >>
>> >> > >>> >> linux and xen_boot modules can't work without this patch.
>> >> > >>> >>
>> >> > >>> >> Signed-off-by: Fu Wei
>> >> > >>> >> ---
>> >> > >>> >> grub-core/Makefile.core.def | 1 +
>> >> > >>> >> grub-core/loader/arm64/fdt.c | 5 +
>> >> > >>> >> 2 files changed, 6 insertions(+)
>> >> > >>> >>
>> >> > >>> >> diff --git a/grub-core/Makefile.core.def
>> >> > >>> >> b/grub-core/Makefile.core.def
>> >> > >>> >> index 2ef10d1..3ea4e49 100644
>> >> > >>> >> --- a/grub-core/Makefile.core.def
>> >> > >>> >> +++ b/grub-core/Makefile.core.def
>> >> > >>> >> @@ -1674,6 +1674,7 @@ module = {
>> >> > >>> >> module = {
>> >> > >>> >>name = fdt;
>> >> > >>> >>arm64 = loader/arm64/fdt.c;
>> >> > >>> >> + fdt = lib/fdt.c;
>> >> > >>> >>enable = arm64;
>> >> > >>> >> };
>> >> > >>> >>
>> >> > >>> > Please don't add same file to 2 different modules. Remove it
>> >> > >>> > from
>> >> > >>> > Linux
>> >> > >>> > module
>> >> > >>>
>> >> > >>> AFAIK, for now , only arm and arm64 are using lib/fdt.c
>> >> > >>> So please allow me to separate all the fdt code from
>> >> > >>> loader/arm/linux.c; just like arm64.
>> >> > >>>
>> >> > >> I don't think it's necessary at this point. Is xen_boot going to
>> >> > >> be
>> >> > >> available for 32-bit arm as well?
>> >> >
>> >> > And I would like to help to sort out the code for your new fdt module
>> >> > , If you don't mind.
>> >> >
>> >> > But according to my tests, I think we need "fdt = lib/fdt.c;" in
>> >> > module. If we don't separate fdt code from arm, we also need "fdt =
>> >> > lib/fdt.c;" in linux module.
>> >> >
>> >> > Please correct me if I misunderstand something, thanks
>> >> >
>> >> We need arm = lib/fdt.c in linux if we don't restructure. There is no
>> >> reason to include lib/fdt.c in 2 different modules on arm64. Fdt in
>> >> first
>> >> part just mean arm and arm64 in this context
>> >>
>> > As an alternative: enable fdt module on all fdt platforms (enable = fdt)
>> > and
>> > put lib/fdt.c
>> >
>> >> > >>> This patch may become a patchset :-)
>> >> > >>>
>> >> > >>> >> diff --git a/grub-core/loader/arm64/fdt.c
>> >> > >>> >> b/grub-core/loader/arm64/fdt.c
>> >> > >>> >> index 5202c14..d160ca0 100644
>> >> > >>> >> --- a/grub-core/loader/arm64/fdt.c
>> >> > >>> >> +++ b/grub-core/loader/arm64/fdt.c
>> >> > >>> >> @@ -25,6 +25,10 @@
>> >> > >>> >> #include
>> >> > >>> >> #include
>> >> > >>> >>
>> >> > >>> >> +GRUB_MOD_LICENSE ("GPLv3+");
>> >> > >>> >> +
>> >> > >>> >> +static grub_dl_t my_mod;
>> >> > >>> >> +
>> >> > >>> > What's the reason for my_mod?
>> >> > >>>
>> >> > >>> this is for grub_dl_unref and grub_dl_ref. but I forgot to check
>> >> > >>> this
>> >> > >>> again for this, sorry,
>> >> > >>> will add this later
>> >> > >>>
>> >> > >> Forget dl_ref and dl_unref. Linux module having a function
>> >> > >> reference
>> >> > >> is
>> >> > >> already good enough
>> >> > >>> >> static void *loaded_fdt;
>> >> > >>
Re: [PATCH] arm64: Fix the bug in fdt module
Le 5 nov. 2015 10:48 AM, "Fu Wei" a écrit :
>
> Hi Vladimir
>
> On 5 November 2015 at 17:45, Vladimir 'phcoder' Serbinenko
> wrote:
> >
> > Le 5 nov. 2015 9:51 AM, "Fu Wei" a écrit :
> >>
> >> Hi Vladimir
> >>
> >> Please correct me :-)
> >> Can I do this :
> >>
> >> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> >> index 2ef10d1..ea5831f 100644
> >> --- a/grub-core/Makefile.core.def
> >> +++ b/grub-core/Makefile.core.def
> >> @@ -1665,7 +1665,6 @@ module = {
> >>ia64_efi = loader/ia64/efi/linux.c;
> >>arm = loader/arm/linux.c;
> >>arm64 = loader/arm64/linux.c;
> >> - fdt = lib/fdt.c;
> >>common = loader/linux.c;
> >>common = lib/cmdline.c;
> >>enable = noemu;
> >> @@ -1674,7 +1673,9 @@ module = {
> >> module = {
> >>name = fdt;
> >>arm64 = loader/arm64/fdt.c;
> >> - enable = arm64;
> >> + arm = loader/arm/fdt.c;
> > Where does this file come from?
> >
>
> that is the file I am working on
>
> i just tried to let you know my thought.
>
> Is this way Ok for you? Please correct me if I misunderstand you
>
What is the reason to reshuffle fdt code for 32-bit arm? Does it merge any
code with arm64? Will this code be used by anything besides linux module?
>
> >
> >> + common = lib/fdt.c;
> >> + enable = fdt;
> >> };
> >>
> >> module = {
> >>
> >> On 4 November 2015 at 19:16, Vladimir 'phcoder' Serbinenko
> >> wrote:
> >> >
> >> > Le 4 nov. 2015 12:12 PM, "Vladimir 'phcoder' Serbinenko"
> >> >
> >> > a écrit :
> >> >>
> >> >>
> >> >> Le 4 nov. 2015 12:09 PM, "Fu Wei" a écrit :
> >> >> >
> >> >> > Hi Vladimir,
> >> >> >
> >> >> > On 4 November 2015 at 19:06, Fu Wei wrote:
> >> >> > > Hi Vladimir,
> >> >> > >
> >> >> > > On 4 November 2015 at 18:47, Vladimir 'phcoder' Serbinenko
> >> >> > > wrote:
> >> >> > >>
> >> >> > >> Le 4 nov. 2015 10:48 AM, "Fu Wei" a écrit :
> >> >> > >>>
> >> >> > >>> Hi Vladimir,
> >> >> > >>>
> >> >> > >>> Great thanks for your help :-)
> >> >> > >>>
> >> >> > >>>
> >> >> > >>> On 4 November 2015 at 02:07, Vladimir 'phcoder' Serbinenko
> >> >> > >>> wrote:
> >> >> > >>> >
> >> >> > >>> > Le 3 nov. 2015 9:56 AM, a écrit :
> >> >> > >>> >>
> >> >> > >>> >> From: Fu Wei
> >> >> > >>> >>
> >> >> > >>> >> This patch goes with commit:
> >> >> > >>> >> 4d0cb755387d6f109b901386ed4d3d475df239fe
> >> >> > >>> >> arm64: Move FDT functions to separate module
> >> >> > >>> >>
> >> >> > >>> >> linux and xen_boot modules can't work without this patch.
> >> >> > >>> >>
> >> >> > >>> >> Signed-off-by: Fu Wei
> >> >> > >>> >> ---
> >> >> > >>> >> grub-core/Makefile.core.def | 1 +
> >> >> > >>> >> grub-core/loader/arm64/fdt.c | 5 +
> >> >> > >>> >> 2 files changed, 6 insertions(+)
> >> >> > >>> >>
> >> >> > >>> >> diff --git a/grub-core/Makefile.core.def
> >> >> > >>> >> b/grub-core/Makefile.core.def
> >> >> > >>> >> index 2ef10d1..3ea4e49 100644
> >> >> > >>> >> --- a/grub-core/Makefile.core.def
> >> >> > >>> >> +++ b/grub-core/Makefile.core.def
> >> >> > >>> >> @@ -1674,6 +1674,7 @@ module = {
> >> >> > >>> >> module = {
> >> >> > >>> >>name = fdt;
> >> >> > >>> >>arm64 = loader/arm64/fdt.c;
> >> >> > >>> >> + fdt = lib/fdt.c;
> >> >> > >>> >>enable = arm64;
> >> >> > >>> >> };
> >> >> > >>> >>
> >> >> > >>> > Please don't add same file to 2 different modules. Remove it
> >> >> > >>> > from
> >> >> > >>> > Linux
> >> >> > >>> > module
> >> >> > >>>
> >> >> > >>> AFAIK, for now , only arm and arm64 are using lib/fdt.c
> >> >> > >>> So please allow me to separate all the fdt code from
> >> >> > >>> loader/arm/linux.c; just like arm64.
> >> >> > >>>
> >> >> > >> I don't think it's necessary at this point. Is xen_boot going
to
> >> >> > >> be
> >> >> > >> available for 32-bit arm as well?
> >> >> >
> >> >> > And I would like to help to sort out the code for your new fdt
module
> >> >> > , If you don't mind.
> >> >> >
> >> >> > But according to my tests, I think we need "fdt = lib/fdt.c;" in
> >> >> > module. If we don't separate fdt code from arm, we also need "fdt
=
> >> >> > lib/fdt.c;" in linux module.
> >> >> >
> >> >> > Please correct me if I misunderstand something, thanks
> >> >> >
> >> >> We need arm = lib/fdt.c in linux if we don't restructure. There is
no
> >> >> reason to include lib/fdt.c in 2 different modules on arm64. Fdt in
> >> >> first
> >> >> part just mean arm and arm64 in this context
> >> >>
> >> > As an alternative: enable fdt module on all fdt platforms (enable =
fdt)
> >> > and
> >> > put lib/fdt.c
> >> >
> >> >> > >>> This patch may become a patchset :-)
> >> >> > >>>
> >> >> > >>> >> diff --git a/grub-core/loader/arm64/fdt.c
> >> >> > >>> >> b/grub-core/loader/arm64/fdt.c
> >> >> > >>> >> index 5202c14..d160ca0 100644
> >> >> > >>> >> --- a/grub-core/loader/arm64/fdt.c
> >> >> > >>> >> +++ b/grub-core/loader/arm64/fdt.c
> >> >> > >>> >> @@ -25,6 +25,10 @@
> >> >> > >>> >> #include
> >> >> > >>> >> #include
> >> >> > >>> >>
> >> >> > >>> >> +GRUB_MOD_LICENSE ("GPLv3+");
> >> >>
Re: [PATCH] arm64: Fix the bug in fdt module
Hi Vladimir,
On 5 November 2015 at 18:00, Vladimir 'phcoder' Serbinenko
wrote:
>
> Le 5 nov. 2015 10:48 AM, "Fu Wei" a écrit :
>>
>> Hi Vladimir
>>
>> On 5 November 2015 at 17:45, Vladimir 'phcoder' Serbinenko
>> wrote:
>> >
>> > Le 5 nov. 2015 9:51 AM, "Fu Wei" a écrit :
>> >>
>> >> Hi Vladimir
>> >>
>> >> Please correct me :-)
>> >> Can I do this :
>> >>
>> >> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> >> index 2ef10d1..ea5831f 100644
>> >> --- a/grub-core/Makefile.core.def
>> >> +++ b/grub-core/Makefile.core.def
>> >> @@ -1665,7 +1665,6 @@ module = {
>> >>ia64_efi = loader/ia64/efi/linux.c;
>> >>arm = loader/arm/linux.c;
>> >>arm64 = loader/arm64/linux.c;
>> >> - fdt = lib/fdt.c;
>> >>common = loader/linux.c;
>> >>common = lib/cmdline.c;
>> >>enable = noemu;
>> >> @@ -1674,7 +1673,9 @@ module = {
>> >> module = {
>> >>name = fdt;
>> >>arm64 = loader/arm64/fdt.c;
>> >> - enable = arm64;
>> >> + arm = loader/arm/fdt.c;
>> > Where does this file come from?
>> >
>>
>> that is the file I am working on
>>
>> i just tried to let you know my thought.
>>
>> Is this way Ok for you? Please correct me if I misunderstand you
>>
> What is the reason to reshuffle fdt code for 32-bit arm?
reason:
we can delete fdt = lib/fdt.c; in linux module,
make this only in fdt module.
> Does it merge any code with arm64?
No, because arm64 uses uefi runtime service, but arm doesn't
> Will this code be used by anything besides linux module?
For arm64, as you know, yes, by xen_boot,
but for arm, NO
So maybe we can do this:
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 3ea4e49..db36a68 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1664,8 +1664,8 @@ module = {
sparc64_ieee1275 = loader/sparc64/ieee1275/linux.c;
ia64_efi = loader/ia64/efi/linux.c;
arm = loader/arm/linux.c;
+ arm = lib/fdt.c;
arm64 = loader/arm64/linux.c;
- fdt = lib/fdt.c;
common = loader/linux.c;
common = lib/cmdline.c;
enable = noemu;
@@ -1674,7 +1674,7 @@ module = {
module = {
name = fdt;
arm64 = loader/arm64/fdt.c;
- fdt = lib/fdt.c;
+ common = lib/fdt.c;
enable = arm64;
};
>
>>
>> >
>> >> + common = lib/fdt.c;
>> >> + enable = fdt;
>> >> };
>> >>
>> >> module = {
>> >>
>> >> On 4 November 2015 at 19:16, Vladimir 'phcoder' Serbinenko
>> >> wrote:
>> >> >
>> >> > Le 4 nov. 2015 12:12 PM, "Vladimir 'phcoder' Serbinenko"
>> >> >
>> >> > a écrit :
>> >> >>
>> >> >>
>> >> >> Le 4 nov. 2015 12:09 PM, "Fu Wei" a écrit :
>> >> >> >
>> >> >> > Hi Vladimir,
>> >> >> >
>> >> >> > On 4 November 2015 at 19:06, Fu Wei wrote:
>> >> >> > > Hi Vladimir,
>> >> >> > >
>> >> >> > > On 4 November 2015 at 18:47, Vladimir 'phcoder' Serbinenko
>> >> >> > > wrote:
>> >> >> > >>
>> >> >> > >> Le 4 nov. 2015 10:48 AM, "Fu Wei" a écrit :
>> >> >> > >>>
>> >> >> > >>> Hi Vladimir,
>> >> >> > >>>
>> >> >> > >>> Great thanks for your help :-)
>> >> >> > >>>
>> >> >> > >>>
>> >> >> > >>> On 4 November 2015 at 02:07, Vladimir 'phcoder' Serbinenko
>> >> >> > >>> wrote:
>> >> >> > >>> >
>> >> >> > >>> > Le 3 nov. 2015 9:56 AM, a écrit :
>> >> >> > >>> >>
>> >> >> > >>> >> From: Fu Wei
>> >> >> > >>> >>
>> >> >> > >>> >> This patch goes with commit:
>> >> >> > >>> >> 4d0cb755387d6f109b901386ed4d3d475df239fe
>> >> >> > >>> >> arm64: Move FDT functions to separate module
>> >> >> > >>> >>
>> >> >> > >>> >> linux and xen_boot modules can't work without this patch.
>> >> >> > >>> >>
>> >> >> > >>> >> Signed-off-by: Fu Wei
>> >> >> > >>> >> ---
>> >> >> > >>> >> grub-core/Makefile.core.def | 1 +
>> >> >> > >>> >> grub-core/loader/arm64/fdt.c | 5 +
>> >> >> > >>> >> 2 files changed, 6 insertions(+)
>> >> >> > >>> >>
>> >> >> > >>> >> diff --git a/grub-core/Makefile.core.def
>> >> >> > >>> >> b/grub-core/Makefile.core.def
>> >> >> > >>> >> index 2ef10d1..3ea4e49 100644
>> >> >> > >>> >> --- a/grub-core/Makefile.core.def
>> >> >> > >>> >> +++ b/grub-core/Makefile.core.def
>> >> >> > >>> >> @@ -1674,6 +1674,7 @@ module = {
>> >> >> > >>> >> module = {
>> >> >> > >>> >>name = fdt;
>> >> >> > >>> >>arm64 = loader/arm64/fdt.c;
>> >> >> > >>> >> + fdt = lib/fdt.c;
>> >> >> > >>> >>enable = arm64;
>> >> >> > >>> >> };
>> >> >> > >>> >>
>> >> >> > >>> > Please don't add same file to 2 different modules. Remove it
>> >> >> > >>> > from
>> >> >> > >>> > Linux
>> >> >> > >>> > module
>> >> >> > >>>
>> >> >> > >>> AFAIK, for now , only arm and arm64 are using lib/fdt.c
>> >> >> > >>> So please allow me to separate all the fdt code from
>> >> >> > >>> loader/arm/linux.c; just like arm64.
>> >> >> > >>>
>> >> >> > >> I don't think it's necessary at this point. Is xen_boot going
>> >> >> > >> to
>> >> >> > >> be
>> >> >> > >> available for 32-bit arm as well?
>> >> >> >
>> >> >> > And I would like to help to sort out the code for your new fdt
>> >> >> > module
>> >> >> > , If you don't mind.
>> >> >> >
>>
Re: [PATCH] arm64: Fix the bug in fdt module
Le 5 nov. 2015 2:39 PM, "Fu Wei" a écrit :
>
> Hi Vladimir,
>
>
> On 5 November 2015 at 18:00, Vladimir 'phcoder' Serbinenko
> wrote:
> >
> > Le 5 nov. 2015 10:48 AM, "Fu Wei" a écrit :
> >>
> >> Hi Vladimir
> >>
> >> On 5 November 2015 at 17:45, Vladimir 'phcoder' Serbinenko
> >> wrote:
> >> >
> >> > Le 5 nov. 2015 9:51 AM, "Fu Wei" a écrit :
> >> >>
> >> >> Hi Vladimir
> >> >>
> >> >> Please correct me :-)
> >> >> Can I do this :
> >> >>
> >> >> diff --git a/grub-core/Makefile.core.def
b/grub-core/Makefile.core.def
> >> >> index 2ef10d1..ea5831f 100644
> >> >> --- a/grub-core/Makefile.core.def
> >> >> +++ b/grub-core/Makefile.core.def
> >> >> @@ -1665,7 +1665,6 @@ module = {
> >> >>ia64_efi = loader/ia64/efi/linux.c;
> >> >>arm = loader/arm/linux.c;
> >> >>arm64 = loader/arm64/linux.c;
> >> >> - fdt = lib/fdt.c;
> >> >>common = loader/linux.c;
> >> >>common = lib/cmdline.c;
> >> >>enable = noemu;
> >> >> @@ -1674,7 +1673,9 @@ module = {
> >> >> module = {
> >> >>name = fdt;
> >> >>arm64 = loader/arm64/fdt.c;
> >> >> - enable = arm64;
> >> >> + arm = loader/arm/fdt.c;
> >> > Where does this file come from?
> >> >
> >>
> >> that is the file I am working on
> >>
> >> i just tried to let you know my thought.
> >>
> >> Is this way Ok for you? Please correct me if I misunderstand you
> >>
> > What is the reason to reshuffle fdt code for 32-bit arm?
>
> reason:
> we can delete fdt = lib/fdt.c; in linux module,
> make this only in fdt module.
>
You don't need to actually shuffle any code. Just see the attached patch
>
> > Does it merge any code with arm64?
>
> No, because arm64 uses uefi runtime service, but arm doesn't
>
> > Will this code be used by anything besides linux module?
>
> For arm64, as you know, yes, by xen_boot,
> but for arm, NO
>
> So maybe we can do this:
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 3ea4e49..db36a68 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1664,8 +1664,8 @@ module = {
>sparc64_ieee1275 = loader/sparc64/ieee1275/linux.c;
>ia64_efi = loader/ia64/efi/linux.c;
>arm = loader/arm/linux.c;
> + arm = lib/fdt.c;
>arm64 = loader/arm64/linux.c;
> - fdt = lib/fdt.c;
>common = loader/linux.c;
>common = lib/cmdline.c;
>enable = noemu;
> @@ -1674,7 +1674,7 @@ module = {
> module = {
>name = fdt;
>arm64 = loader/arm64/fdt.c;
> - fdt = lib/fdt.c;
> + common = lib/fdt.c;
>enable = arm64;
> };
>
>
> >
> >>
> >> >
> >> >> + common = lib/fdt.c;
> >> >> + enable = fdt;
> >> >> };
> >> >>
> >> >> module = {
> >> >>
> >> >> On 4 November 2015 at 19:16, Vladimir 'phcoder' Serbinenko
> >> >> wrote:
> >> >> >
> >> >> > Le 4 nov. 2015 12:12 PM, "Vladimir 'phcoder' Serbinenko"
> >> >> >
> >> >> > a écrit :
> >> >> >>
> >> >> >>
> >> >> >> Le 4 nov. 2015 12:09 PM, "Fu Wei" a écrit :
> >> >> >> >
> >> >> >> > Hi Vladimir,
> >> >> >> >
> >> >> >> > On 4 November 2015 at 19:06, Fu Wei wrote:
> >> >> >> > > Hi Vladimir,
> >> >> >> > >
> >> >> >> > > On 4 November 2015 at 18:47, Vladimir 'phcoder' Serbinenko
> >> >> >> > > wrote:
> >> >> >> > >>
> >> >> >> > >> Le 4 nov. 2015 10:48 AM, "Fu Wei" a
écrit :
> >> >> >> > >>>
> >> >> >> > >>> Hi Vladimir,
> >> >> >> > >>>
> >> >> >> > >>> Great thanks for your help :-)
> >> >> >> > >>>
> >> >> >> > >>>
> >> >> >> > >>> On 4 November 2015 at 02:07, Vladimir 'phcoder' Serbinenko
> >> >> >> > >>> wrote:
> >> >> >> > >>> >
> >> >> >> > >>> > Le 3 nov. 2015 9:56 AM, a écrit :
> >> >> >> > >>> >>
> >> >> >> > >>> >> From: Fu Wei
> >> >> >> > >>> >>
> >> >> >> > >>> >> This patch goes with commit:
> >> >> >> > >>> >> 4d0cb755387d6f109b901386ed4d3d475df239fe
> >> >> >> > >>> >> arm64: Move FDT functions to separate module
> >> >> >> > >>> >>
> >> >> >> > >>> >> linux and xen_boot modules can't work without this
patch.
> >> >> >> > >>> >>
> >> >> >> > >>> >> Signed-off-by: Fu Wei
> >> >> >> > >>> >> ---
> >> >> >> > >>> >> grub-core/Makefile.core.def | 1 +
> >> >> >> > >>> >> grub-core/loader/arm64/fdt.c | 5 +
> >> >> >> > >>> >> 2 files changed, 6 insertions(+)
> >> >> >> > >>> >>
> >> >> >> > >>> >> diff --git a/grub-core/Makefile.core.def
> >> >> >> > >>> >> b/grub-core/Makefile.core.def
> >> >> >> > >>> >> index 2ef10d1..3ea4e49 100644
> >> >> >> > >>> >> --- a/grub-core/Makefile.core.def
> >> >> >> > >>> >> +++ b/grub-core/Makefile.core.def
> >> >> >> > >>> >> @@ -1674,6 +1674,7 @@ module = {
> >> >> >> > >>> >> module = {
> >> >> >> > >>> >>name = fdt;
> >> >> >> > >>> >>arm64 = loader/arm64/fdt.c;
> >> >> >> > >>> >> + fdt = lib/fdt.c;
> >> >> >> > >>> >>enable = arm64;
> >> >> >> > >>> >> };
> >> >> >> > >>> >>
> >> >> >> > >>> > Please don't add same file to 2 different modules.
Remove it
> >> >> >> > >>> > from
> >> >> >> > >>> > Linux
> >> >> >> > >>> > module
> >> >> >> > >>>
> >> >> >> > >>> AFAIK, for now , only arm and arm64 are using lib/fdt.c
> >> >> >> >
Re: [PATCH] arm64: Fix the bug in fdt module
Hi Vladimir,
Thanks for your help, I will test it on arm and arm64, then make a new patch
On 5 November 2015 at 22:53, Vladimir 'phcoder' Serbinenko
wrote:
>
> Le 5 nov. 2015 2:39 PM, "Fu Wei" a écrit :
>>
>> Hi Vladimir,
>>
>>
>
>> On 5 November 2015 at 18:00, Vladimir 'phcoder' Serbinenko
>> wrote:
>> >
>> > Le 5 nov. 2015 10:48 AM, "Fu Wei" a écrit :
>> >>
>> >> Hi Vladimir
>> >>
>> >> On 5 November 2015 at 17:45, Vladimir 'phcoder' Serbinenko
>> >> wrote:
>> >> >
>> >> > Le 5 nov. 2015 9:51 AM, "Fu Wei" a écrit :
>> >> >>
>> >> >> Hi Vladimir
>> >> >>
>> >> >> Please correct me :-)
>> >> >> Can I do this :
>> >> >>
>> >> >> diff --git a/grub-core/Makefile.core.def
>> >> >> b/grub-core/Makefile.core.def
>> >> >> index 2ef10d1..ea5831f 100644
>> >> >> --- a/grub-core/Makefile.core.def
>> >> >> +++ b/grub-core/Makefile.core.def
>> >> >> @@ -1665,7 +1665,6 @@ module = {
>> >> >>ia64_efi = loader/ia64/efi/linux.c;
>> >> >>arm = loader/arm/linux.c;
>> >> >>arm64 = loader/arm64/linux.c;
>> >> >> - fdt = lib/fdt.c;
>> >> >>common = loader/linux.c;
>> >> >>common = lib/cmdline.c;
>> >> >>enable = noemu;
>> >> >> @@ -1674,7 +1673,9 @@ module = {
>> >> >> module = {
>> >> >>name = fdt;
>> >> >>arm64 = loader/arm64/fdt.c;
>> >> >> - enable = arm64;
>> >> >> + arm = loader/arm/fdt.c;
>> >> > Where does this file come from?
>> >> >
>> >>
>> >> that is the file I am working on
>> >>
>> >> i just tried to let you know my thought.
>> >>
>> >> Is this way Ok for you? Please correct me if I misunderstand you
>> >>
>> > What is the reason to reshuffle fdt code for 32-bit arm?
>>
>> reason:
>> we can delete fdt = lib/fdt.c; in linux module,
>> make this only in fdt module.
>>
> You don't need to actually shuffle any code. Just see the attached patch
>
>>
>> > Does it merge any code with arm64?
>>
>> No, because arm64 uses uefi runtime service, but arm doesn't
>>
>> > Will this code be used by anything besides linux module?
>>
>> For arm64, as you know, yes, by xen_boot,
>> but for arm, NO
>>
>> So maybe we can do this:
>>
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 3ea4e49..db36a68 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -1664,8 +1664,8 @@ module = {
>>sparc64_ieee1275 = loader/sparc64/ieee1275/linux.c;
>>ia64_efi = loader/ia64/efi/linux.c;
>>arm = loader/arm/linux.c;
>> + arm = lib/fdt.c;
>>arm64 = loader/arm64/linux.c;
>> - fdt = lib/fdt.c;
>>common = loader/linux.c;
>>common = lib/cmdline.c;
>>enable = noemu;
>> @@ -1674,7 +1674,7 @@ module = {
>> module = {
>>name = fdt;
>>arm64 = loader/arm64/fdt.c;
>> - fdt = lib/fdt.c;
>> + common = lib/fdt.c;
>>enable = arm64;
>> };
>>
>>
>> >
>> >>
>> >> >
>> >> >> + common = lib/fdt.c;
>> >> >> + enable = fdt;
>> >> >> };
>> >> >>
>> >> >> module = {
>> >> >>
>> >> >> On 4 November 2015 at 19:16, Vladimir 'phcoder' Serbinenko
>> >> >> wrote:
>> >> >> >
>> >> >> > Le 4 nov. 2015 12:12 PM, "Vladimir 'phcoder' Serbinenko"
>> >> >> >
>> >> >> > a écrit :
>> >> >> >>
>> >> >> >>
>> >> >> >> Le 4 nov. 2015 12:09 PM, "Fu Wei" a écrit :
>> >> >> >> >
>> >> >> >> > Hi Vladimir,
>> >> >> >> >
>> >> >> >> > On 4 November 2015 at 19:06, Fu Wei wrote:
>> >> >> >> > > Hi Vladimir,
>> >> >> >> > >
>> >> >> >> > > On 4 November 2015 at 18:47, Vladimir 'phcoder' Serbinenko
>> >> >> >> > > wrote:
>> >> >> >> > >>
>> >> >> >> > >> Le 4 nov. 2015 10:48 AM, "Fu Wei" a
>> >> >> >> > >> écrit :
>> >> >> >> > >>>
>> >> >> >> > >>> Hi Vladimir,
>> >> >> >> > >>>
>> >> >> >> > >>> Great thanks for your help :-)
>> >> >> >> > >>>
>> >> >> >> > >>>
>> >> >> >> > >>> On 4 November 2015 at 02:07, Vladimir 'phcoder' Serbinenko
>> >> >> >> > >>> wrote:
>> >> >> >> > >>> >
>> >> >> >> > >>> > Le 3 nov. 2015 9:56 AM, a écrit :
>> >> >> >> > >>> >>
>> >> >> >> > >>> >> From: Fu Wei
>> >> >> >> > >>> >>
>> >> >> >> > >>> >> This patch goes with commit:
>> >> >> >> > >>> >> 4d0cb755387d6f109b901386ed4d3d475df239fe
>> >> >> >> > >>> >> arm64: Move FDT functions to separate module
>> >> >> >> > >>> >>
>> >> >> >> > >>> >> linux and xen_boot modules can't work without this
>> >> >> >> > >>> >> patch.
>> >> >> >> > >>> >>
>> >> >> >> > >>> >> Signed-off-by: Fu Wei
>> >> >> >> > >>> >> ---
>> >> >> >> > >>> >> grub-core/Makefile.core.def | 1 +
>> >> >> >> > >>> >> grub-core/loader/arm64/fdt.c | 5 +
>> >> >> >> > >>> >> 2 files changed, 6 insertions(+)
>> >> >> >> > >>> >>
>> >> >> >> > >>> >> diff --git a/grub-core/Makefile.core.def
>> >> >> >> > >>> >> b/grub-core/Makefile.core.def
>> >> >> >> > >>> >> index 2ef10d1..3ea4e49 100644
>> >> >> >> > >>> >> --- a/grub-core/Makefile.core.def
>> >> >> >> > >>> >> +++ b/grub-core/Makefile.core.def
>> >> >> >> > >>> >> @@ -1674,6 +1674,7 @@ module = {
>> >> >> >> > >>> >> module = {
>> >> >> >> > >>> >>name = fdt;
>> >> >> >> > >>> >>arm64 = loader/arm64/fdt.c;
>> >> >> >>
Re: Grub get and set efi variables
On Wednesday, November 4, 2015, Andrei Borzenkov wrote: > 04.11.2015 02:05, Mat Troi пишет: > >> Hi SevenBits, >> >> Thanks for letting me know. Out of curiosity, any particular reason why >> this patch did not get merged? It looks to be a useful feature. >> >> > First, this patch was reverse patch :) Yeah, I think I had accidentally passed the arguments to the diff command in the wrong order. > > I am not convinced making it easy to set EFI variable from within GRUB is > good thing, because there are known reports about systems rendered > unbootable by writing too much into EFI flash. What is your use case that > absolutely requires setting EFI variables? How are you going to implement > it on other platforms? I should probably note that I wrote this patch for a specific project of mine which required the ability to read UEFI variables. I added in write functionality for good measure because I could. But I agree, this would only encourage tinkering and users messing with their systems and potentially bricking it, which would of course be blamed on GRUB. > > Reading does not harm and may be useful, but then it should provide > generic interface to access arbitrary vendor namespace, not only EFI global > variables, and handle arbitrary binary data, even if initial implementation > handles only subset of them. Once someone starts to use it changing it will > be much more difficult. > > Maybe it should take hints how to interpret variable values, or have > format option akin to printf. I would be happy to resume work on this, and debase it on the current code, if GRUB has a clear need for such functionality. I would prefer to have it be clear what the patch should consist of, though. A key question would be how to e.g. handle arbitrary data stored in variables if it is not something easily represent able like a string. Right now, the patch stores it's value into an environment variable specified by the user. Unless the powers that be think otherwise, I think this is the best way to go. > > Thanks, >> Mat >> >> On Tue, Nov 3, 2015 at 12:12 PM, SevenBits >> wrote: >> >> On Tuesday, November 3, 2015, Mat Troi wrote: >>> >>> Hi, Searching through google, I see there was an email thread to add a patch for getting and setting efi variable in GRUB2. https://lists.gnu.org/archive/html/grub-devel/2013-11/msg00328.html However, looking at the tree, it doesn't look like this patch was added, am I missing something? Does anyone know if we have command to get/set efi variables in GRUB2? http://git.savannah.gnu.org/gitweb/?p=grub.git;a=tree;f=grub-core/commands/efi;h=005fd2efc9a2eede2a1eb1cab8081c360219107b;hb=HEAD I'm the author of that patch. No, it was never merged into the tree. As >>> far as I know, there is no equivalent functionality; GRUB does not >>> support >>> this feature. >>> >>> >>> Thanks, Mat >>> ___ >>> 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 >> >> > > ___ > 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
[PATCH] efinet: add efinet_multicast_filter command
We have some hardware that doesn't honor
EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST properly so we aren't getting
RA's that are multicasted properly (our switches respond to solicitations with a
multicast rather than a unicast). I don't want to add this filtering by
default, so add a new command to allow a user to specify a multicast receive
filter. We use it like this
efinet_multicast_filter efinet0 33:33:0:0:0:1
to get ipv6 multicasts which allows us to receive the router advertisements.
Thanks,
Signed-off-by: Josef Bacik
---
grub-core/kern/efi/efi.c | 12 -
grub-core/net/drivers/efi/efinet.c | 51 ++
grub-core/net/net.c| 43
include/grub/efi/api.h | 5 +++-
include/grub/net.h | 3 +++
5 files changed, 107 insertions(+), 7 deletions(-)
diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
index 2e77834..efc3d33 100644
--- a/grub-core/kern/efi/efi.c
+++ b/grub-core/kern/efi/efi.c
@@ -652,12 +652,12 @@ grub_efi_print_device_path (grub_efi_device_path_t *dp)
grub_efi_mac_address_device_path_t *mac
= (grub_efi_mac_address_device_path_t *) dp;
grub_printf ("/MacAddr(%02x:%02x:%02x:%02x:%02x:%02x,%x)",
-(unsigned) mac->mac_address[0],
-(unsigned) mac->mac_address[1],
-(unsigned) mac->mac_address[2],
-(unsigned) mac->mac_address[3],
-(unsigned) mac->mac_address[4],
-(unsigned) mac->mac_address[5],
+(unsigned) mac->mac_address.addr[0],
+(unsigned) mac->mac_address.addr[1],
+(unsigned) mac->mac_address.addr[2],
+(unsigned) mac->mac_address.addr[3],
+(unsigned) mac->mac_address.addr[4],
+(unsigned) mac->mac_address.addr[5],
(unsigned) mac->if_type);
}
break;
diff --git a/grub-core/net/drivers/efi/efinet.c
b/grub-core/net/drivers/efi/efinet.c
index c8f80a1..8c2c4f8 100644
--- a/grub-core/net/drivers/efi/efinet.c
+++ b/grub-core/net/drivers/efi/efinet.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
GRUB_MOD_LICENSE ("GPLv3+");
@@ -451,9 +452,58 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char
**device,
}
}
+static grub_err_t
+grub_cmd_multicast_filter (struct grub_command *cmd __attribute__ ((unused)),
+ int argc, char **args)
+{
+ struct grub_net_card *card;
+ grub_efi_simple_network_t *net;
+ grub_net_link_level_address_t hwaddr;
+ grub_efi_mac_address_t filter_mac[1];
+ grub_efi_status_t st;
+
+ if (argc != 2)
+return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments expected"));
+
+ FOR_NET_CARDS (card)
+if (grub_strcmp (card->name, args[0]) == 0)
+ break;
+
+ if (card == NULL)
+return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("card not found"));
+ if (card->driver != &efidriver)
+return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("card not an efi card"));
+
+ if (grub_net_str_to_hwaddr (args[1], &hwaddr) == 0)
+return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("couldn't parse hw address"));
+ grub_memset (filter_mac, 0, sizeof(filter_mac));
+ grub_memcpy (filter_mac, hwaddr.mac, 6);
+
+ net = card->efi_net;
+ if (!(net->mode->receive_filter_mask &
+ GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST))
+return grub_error (GRUB_ERR_IO,
+ N_("device doesn't support multicast filtering"));
+
+ st = efi_call_6 (net->receive_filters, net,
+ GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST, 0, 0, 1,
+ filter_mac);
+ if (st != GRUB_EFI_SUCCESS)
+return grub_error (GRUB_ERR_IO,
+ N_("could not set multicast filter address"));
+
+ return GRUB_ERR_NONE;
+}
+
+static grub_command_t cmd_multicast_filter;
+
GRUB_MOD_INIT(efinet)
{
grub_efinet_findcards ();
+ cmd_multicast_filter = grub_register_command ("efinet_multicast_filter",
+ grub_cmd_multicast_filter,
+ N_("CARD HWADDRESS"),
+ N_("Add a multicast filter"));
grub_efi_net_config = grub_efi_net_config_real;
}
@@ -464,5 +514,6 @@ GRUB_MOD_FINI(efinet)
FOR_NET_CARDS_SAFE (card, next)
if (card->driver == &efidriver)
grub_net_card_unregister (card);
+ grub_unregister_command(cmd_multicast_filter);
}
diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 65bea28..3a69f63 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -480,6 +480,49 @@ parse_ip6 (const char *val, grub_uint64_t *ip, const char
**rest)
return 1;
}
+int
+grub_net_str
Re: [PATCH] efinet: add efinet_multicast_filter command
I don't have EFI spec under my hand now. Can we get away with making it a
default or at least for the case when no interface overrides mac address.
Extra config to workaround firmware bugs is usually harmful
Le 5 nov. 2015 9:17 PM, "Josef Bacik" a écrit :
> We have some hardware that doesn't honor
> EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST properly so we aren't
> getting
> RA's that are multicasted properly (our switches respond to solicitations
> with a
> multicast rather than a unicast). I don't want to add this filtering by
> default, so add a new command to allow a user to specify a multicast
> receive
> filter. We use it like this
>
> efinet_multicast_filter efinet0 33:33:0:0:0:1
>
> to get ipv6 multicasts which allows us to receive the router
> advertisements.
> Thanks,
>
> Signed-off-by: Josef Bacik
> ---
> grub-core/kern/efi/efi.c | 12 -
> grub-core/net/drivers/efi/efinet.c | 51
> ++
> grub-core/net/net.c| 43
> include/grub/efi/api.h | 5 +++-
> include/grub/net.h | 3 +++
> 5 files changed, 107 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index 2e77834..efc3d33 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -652,12 +652,12 @@ grub_efi_print_device_path (grub_efi_device_path_t
> *dp)
> grub_efi_mac_address_device_path_t *mac
> = (grub_efi_mac_address_device_path_t *) dp;
> grub_printf ("/MacAddr(%02x:%02x:%02x:%02x:%02x:%02x,%x)",
> -(unsigned) mac->mac_address[0],
> -(unsigned) mac->mac_address[1],
> -(unsigned) mac->mac_address[2],
> -(unsigned) mac->mac_address[3],
> -(unsigned) mac->mac_address[4],
> -(unsigned) mac->mac_address[5],
> +(unsigned) mac->mac_address.addr[0],
> +(unsigned) mac->mac_address.addr[1],
> +(unsigned) mac->mac_address.addr[2],
> +(unsigned) mac->mac_address.addr[3],
> +(unsigned) mac->mac_address.addr[4],
> +(unsigned) mac->mac_address.addr[5],
> (unsigned) mac->if_type);
> }
> break;
> diff --git a/grub-core/net/drivers/efi/efinet.c
> b/grub-core/net/drivers/efi/efinet.c
> index c8f80a1..8c2c4f8 100644
> --- a/grub-core/net/drivers/efi/efinet.c
> +++ b/grub-core/net/drivers/efi/efinet.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
>
> GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -451,9 +452,58 @@ grub_efi_net_config_real (grub_efi_handle_t hnd, char
> **device,
>}
> }
>
> +static grub_err_t
> +grub_cmd_multicast_filter (struct grub_command *cmd __attribute__
> ((unused)),
> + int argc, char **args)
> +{
> + struct grub_net_card *card;
> + grub_efi_simple_network_t *net;
> + grub_net_link_level_address_t hwaddr;
> + grub_efi_mac_address_t filter_mac[1];
> + grub_efi_status_t st;
> +
> + if (argc != 2)
> +return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("two arguments
> expected"));
> +
> + FOR_NET_CARDS (card)
> +if (grub_strcmp (card->name, args[0]) == 0)
> + break;
> +
> + if (card == NULL)
> +return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("card not found"));
> + if (card->driver != &efidriver)
> +return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("card not an efi card"));
> +
> + if (grub_net_str_to_hwaddr (args[1], &hwaddr) == 0)
> +return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("couldn't parse hw
> address"));
> + grub_memset (filter_mac, 0, sizeof(filter_mac));
> + grub_memcpy (filter_mac, hwaddr.mac, 6);
> +
> + net = card->efi_net;
> + if (!(net->mode->receive_filter_mask &
> + GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST))
> +return grub_error (GRUB_ERR_IO,
> + N_("device doesn't support multicast filtering"));
> +
> + st = efi_call_6 (net->receive_filters, net,
> + GRUB_EFI_SIMPLE_NETWORK_RECEIVE_MULTICAST, 0, 0, 1,
> + filter_mac);
> + if (st != GRUB_EFI_SUCCESS)
> +return grub_error (GRUB_ERR_IO,
> + N_("could not set multicast filter address"));
> +
> + return GRUB_ERR_NONE;
> +}
> +
> +static grub_command_t cmd_multicast_filter;
> +
> GRUB_MOD_INIT(efinet)
> {
>grub_efinet_findcards ();
> + cmd_multicast_filter = grub_register_command ("efinet_multicast_filter",
> + grub_cmd_multicast_filter,
> + N_("CARD HWADDRESS"),
> + N_("Add a multicast
> filter"));
>grub_efi_net_config = grub_e
Re: [PATCH] efinet: add efinet_multicast_filter command
On 11/05/2015 03:28 PM, Vladimir 'phcoder' Serbinenko wrote: I don't have EFI spec under my hand now. Can we get away with making it a default or at least for the case when no interface overrides mac address. Extra config to workaround firmware bugs is usually harmful http://wiki.phoenix.com/wiki/index.php/EFI_SIMPLE_NETWORK_PROTOCOL That is what I've been using. The thing I worry about is that this is just the multicast address for ipv6. There's a wikipedia page that lists about 15 addresses for different things. Now how many do we care about for grub? I have no idea. I _think_ that we only really care about multicast traffic for ipv6 router advertisements, I can't think of when else we'd want to use it. So in that sense I think it would be ok to do this. But I have no way of knowing what would break in current working configurations which is why I went with the config option. I can test with the variety of hardware I have here and see how it does with it set by default. It's up to you, I'm good either way. Thanks, Josef ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH] efinet: add efinet_multicast_filter command
If we can't figure out which ones we need to filter out, then it's unrealistic to expect our users to be able to configure. We need to go with multicast filter per default and switch to promiscuous only when we actually need it: when we override mac address. You can add function to go to promiscuous mode to network interface structure On 11/05/2015 03:28 PM, Vladimir 'phcoder' Serbinenko wrote: > I don't have EFI spec under my hand now. Can we get away with making it > a default or at least for the case when no interface overrides mac > address. Extra config to workaround firmware bugs is usually harmful > > http://wiki.phoenix.com/wiki/index.php/EFI_SIMPLE_NETWORK_PROTOCOL That is what I've been using. The thing I worry about is that this is just the multicast address for ipv6. There's a wikipedia page that lists about 15 addresses for different things. Now how many do we care about for grub? I have no idea. I _think_ that we only really care about multicast traffic for ipv6 router advertisements, I can't think of when else we'd want to use it. So in that sense I think it would be ok to do this. But I have no way of knowing what would break in current working configurations which is why I went with the config option. I can test with the variety of hardware I have here and see how it does with it set by default. It's up to you, I'm good either way. Thanks, Josef ___ 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: Grub get and set efi variables
Actually, I submitted similar patch recently as well. It provides read
function for variables and accepts a hint on how to process them. The
original patch is here:
https://lists.gnu.org/archive/html/grub-devel/2015-09/msg00043.html.
Probably, I forgot to enable plain-text mode, so it got there as a
binary attachment. I will repeat it here for convenience.
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 9764cd2..49fa3ec 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -728,6 +728,12 @@ module = {
};
module = {
+ name = efivar;
+ efi = commands/efi/efivar.c;
+ enable = efi;
+};
+
+module = {
name = blocklist;
common = commands/blocklist.c;
};
diff --git a/grub-core/commands/efi/efivar.c b/grub-core/commands/efi/efivar.c
new file mode 100644
index 000..ca206eb
--- /dev/null
+++ b/grub-core/commands/efi/efivar.c
@@ -0,0 +1,146 @@
+/* efivar.c - Read EFI global variables. */
+/*
+ * GRUB -- GRand Unified Bootloader
+ * Copyright (C) 2015 Free Software Foundation, Inc.
+ * Copyright (C) 2015 CloudFlare, Inc.
+ *
+ * GRUB is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * GRUB is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with GRUB. If not, see
Re: [PATCH] efinet: add efinet_multicast_filter command
05.11.2015 22:23, Josef Bacik пишет: We have some hardware that doesn't honor EFI_SIMPLE_NETWORK_RECEIVE_PROMISCUOUS_MULTICAST properly so we aren't getting You mean that driver advertises promiscuous multicast support but does not implement it? Can you add debugging to efi_call_6 (net->receive_filters, net, filters, 0, 0, 0, NULL); whether it fails. May be need set each one separately and fall back to promiscuous. RA's that are multicasted properly (our switches respond to solicitations with a multicast rather than a unicast). Could you send packet trace? I don't want to add this filtering by default, so add a new command to allow a user to specify a multicast receive filter. We use it like this I do not think we need any IPv4 multicasts; for IPv6 we need all nodes and solicited address. But I would like to understand first whether receive_filters fail and we ignore it or it succeeds. ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
