| Message ID | 20181031192843.13230-5-f.fainelli@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Get rid of __early_init_dt_declare_initrd() | expand |
On Wed, Oct 31, 2018 at 12:28:41PM -0700, Florian Fainelli wrote: > ARM64 is the only architecture that re-defines > __early_init_dt_declare_initrd() in order for that function to populate > initrd_start/initrd_end with physical addresses instead of virtual > addresses. Instead of having an override we can leverage > drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to > populate those variables for us. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > arch/arm64/mm/init.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > index 3cf87341859f..00ef2166bb73 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -72,8 +72,8 @@ static int __init early_initrd(char *p) > if (*endp == ',') { > size = memparse(endp + 1, NULL); > > - initrd_start = start; > - initrd_end = start + size; > + phys_initrd_start = start; > + phys_initrd_size = size; > } > return 0; > } > @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void) > memblock_add(__pa_symbol(_text), (u64)(_end - _text)); > } > > - if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) { > + if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) { > /* > * Add back the memory we just removed if it results in the > * initrd to become inaccessible via the linear mapping. > * Otherwise, this is a no-op > */ > - u64 base = initrd_start & PAGE_MASK; > - u64 size = PAGE_ALIGN(initrd_end) - base; > + u64 base = phys_initrd_start & PAGE_MASK; > + u64 size = PAGE_ALIGN(phys_initrd_size); > > /* > * We can only add back the initrd memory if we don't end up > @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void) > */ > memblock_reserve(__pa_symbol(_text), _end - _text); > #ifdef CONFIG_BLK_DEV_INITRD > - if (initrd_start) { > - memblock_reserve(initrd_start, initrd_end - initrd_start); > - > + if (phys_initrd_size) { Since we're touching the if anyways, perhaps convert the #ifdef to a C IS_ENABLED(). > /* the generic initrd code expects virtual addresses */ > - initrd_start = __phys_to_virt(initrd_start); > - initrd_end = __phys_to_virt(initrd_end); > + initrd_start = __phys_to_virt(phys_initrd_start); > + initrd_end = initrd_start + phys_initrd_size; > + initrd_below_start_ok = 0; > } > #endif > > -- > 2.17.1 >
Hi Florian, On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote: > ARM64 is the only architecture that re-defines > __early_init_dt_declare_initrd() in order for that function to populate > initrd_start/initrd_end with physical addresses instead of virtual > addresses. Instead of having an override we can leverage > drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to > populate those variables for us. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > arch/arm64/mm/init.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > index 3cf87341859f..00ef2166bb73 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -72,8 +72,8 @@ static int __init early_initrd(char *p) > if (*endp == ',') { > size = memparse(endp + 1, NULL); > > - initrd_start = start; > - initrd_end = start + size; > + phys_initrd_start = start; > + phys_initrd_size = size; > } > return 0; > } > @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void) > memblock_add(__pa_symbol(_text), (u64)(_end - _text)); > } > > - if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) { > + if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) { > /* > * Add back the memory we just removed if it results in the > * initrd to become inaccessible via the linear mapping. > * Otherwise, this is a no-op > */ > - u64 base = initrd_start & PAGE_MASK; > - u64 size = PAGE_ALIGN(initrd_end) - base; > + u64 base = phys_initrd_start & PAGE_MASK; > + u64 size = PAGE_ALIGN(phys_initrd_size); > > /* > * We can only add back the initrd memory if we don't end up > @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void) > */ > memblock_reserve(__pa_symbol(_text), _end - _text); > #ifdef CONFIG_BLK_DEV_INITRD > - if (initrd_start) { > - memblock_reserve(initrd_start, initrd_end - initrd_start); > - > + if (phys_initrd_size) { > /* the generic initrd code expects virtual addresses */ > - initrd_start = __phys_to_virt(initrd_start); > - initrd_end = __phys_to_virt(initrd_end); > + initrd_start = __phys_to_virt(phys_initrd_start); > + initrd_end = initrd_start + phys_initrd_size; > + initrd_below_start_ok = 0; Where is this assignment coming from? > } > #endif > > -- > 2.17.1 >
On 11/5/18 12:39 PM, Ard Biesheuvel wrote: > Hi Florian, > > On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote: >> ARM64 is the only architecture that re-defines >> __early_init_dt_declare_initrd() in order for that function to populate >> initrd_start/initrd_end with physical addresses instead of virtual >> addresses. Instead of having an override we can leverage >> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to >> populate those variables for us. >> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> arch/arm64/mm/init.c | 19 +++++++++---------- >> 1 file changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >> index 3cf87341859f..00ef2166bb73 100644 >> --- a/arch/arm64/mm/init.c >> +++ b/arch/arm64/mm/init.c >> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p) >> if (*endp == ',') { >> size = memparse(endp + 1, NULL); >> >> - initrd_start = start; >> - initrd_end = start + size; >> + phys_initrd_start = start; >> + phys_initrd_size = size; >> } >> return 0; >> } >> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void) >> memblock_add(__pa_symbol(_text), (u64)(_end - _text)); >> } >> >> - if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) { >> + if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) { >> /* >> * Add back the memory we just removed if it results in the >> * initrd to become inaccessible via the linear mapping. >> * Otherwise, this is a no-op >> */ >> - u64 base = initrd_start & PAGE_MASK; >> - u64 size = PAGE_ALIGN(initrd_end) - base; >> + u64 base = phys_initrd_start & PAGE_MASK; >> + u64 size = PAGE_ALIGN(phys_initrd_size); >> >> /* >> * We can only add back the initrd memory if we don't end up >> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void) >> */ >> memblock_reserve(__pa_symbol(_text), _end - _text); >> #ifdef CONFIG_BLK_DEV_INITRD >> - if (initrd_start) { >> - memblock_reserve(initrd_start, initrd_end - initrd_start); >> - >> + if (phys_initrd_size) { >> /* the generic initrd code expects virtual addresses */ >> - initrd_start = __phys_to_virt(initrd_start); >> - initrd_end = __phys_to_virt(initrd_end); >> + initrd_start = __phys_to_virt(phys_initrd_start); >> + initrd_end = initrd_start + phys_initrd_size; >> + initrd_below_start_ok = 0; > > Where is this assignment coming from? __early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though after patch #5 this is not necessary any more.
On 5 November 2018 at 21:41, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 11/5/18 12:39 PM, Ard Biesheuvel wrote: >> Hi Florian, >> >> On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote: >>> ARM64 is the only architecture that re-defines >>> __early_init_dt_declare_initrd() in order for that function to populate >>> initrd_start/initrd_end with physical addresses instead of virtual >>> addresses. Instead of having an override we can leverage >>> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to >>> populate those variables for us. >>> >>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>> --- >>> arch/arm64/mm/init.c | 19 +++++++++---------- >>> 1 file changed, 9 insertions(+), 10 deletions(-) >>> >>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >>> index 3cf87341859f..00ef2166bb73 100644 >>> --- a/arch/arm64/mm/init.c >>> +++ b/arch/arm64/mm/init.c >>> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p) >>> if (*endp == ',') { >>> size = memparse(endp + 1, NULL); >>> >>> - initrd_start = start; >>> - initrd_end = start + size; >>> + phys_initrd_start = start; >>> + phys_initrd_size = size; >>> } >>> return 0; >>> } >>> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void) >>> memblock_add(__pa_symbol(_text), (u64)(_end - _text)); >>> } >>> >>> - if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) { >>> + if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) { >>> /* >>> * Add back the memory we just removed if it results in the >>> * initrd to become inaccessible via the linear mapping. >>> * Otherwise, this is a no-op >>> */ >>> - u64 base = initrd_start & PAGE_MASK; >>> - u64 size = PAGE_ALIGN(initrd_end) - base; >>> + u64 base = phys_initrd_start & PAGE_MASK; >>> + u64 size = PAGE_ALIGN(phys_initrd_size); >>> >>> /* >>> * We can only add back the initrd memory if we don't end up >>> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void) >>> */ >>> memblock_reserve(__pa_symbol(_text), _end - _text); >>> #ifdef CONFIG_BLK_DEV_INITRD >>> - if (initrd_start) { >>> - memblock_reserve(initrd_start, initrd_end - initrd_start); >>> - >>> + if (phys_initrd_size) { >>> /* the generic initrd code expects virtual addresses */ >>> - initrd_start = __phys_to_virt(initrd_start); >>> - initrd_end = __phys_to_virt(initrd_end); >>> + initrd_start = __phys_to_virt(phys_initrd_start); >>> + initrd_end = initrd_start + phys_initrd_size; >>> + initrd_below_start_ok = 0; >> >> Where is this assignment coming from? > > __early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though > after patch #5 this is not necessary any more. Yes, but why? The original arm64 version of __early_init_dt_declare_initrd() does not set it but now you set to 1 in the IS_ENABLED(CONFIG_ARM64) section in the generic code and set it back to 0 here. Or am I missing something?
On 11/5/18 12:44 PM, Ard Biesheuvel wrote: > On 5 November 2018 at 21:41, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 11/5/18 12:39 PM, Ard Biesheuvel wrote: >>> Hi Florian, >>> >>> On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>> ARM64 is the only architecture that re-defines >>>> __early_init_dt_declare_initrd() in order for that function to populate >>>> initrd_start/initrd_end with physical addresses instead of virtual >>>> addresses. Instead of having an override we can leverage >>>> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to >>>> populate those variables for us. >>>> >>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>> --- >>>> arch/arm64/mm/init.c | 19 +++++++++---------- >>>> 1 file changed, 9 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >>>> index 3cf87341859f..00ef2166bb73 100644 >>>> --- a/arch/arm64/mm/init.c >>>> +++ b/arch/arm64/mm/init.c >>>> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p) >>>> if (*endp == ',') { >>>> size = memparse(endp + 1, NULL); >>>> >>>> - initrd_start = start; >>>> - initrd_end = start + size; >>>> + phys_initrd_start = start; >>>> + phys_initrd_size = size; >>>> } >>>> return 0; >>>> } >>>> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void) >>>> memblock_add(__pa_symbol(_text), (u64)(_end - _text)); >>>> } >>>> >>>> - if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) { >>>> + if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) { >>>> /* >>>> * Add back the memory we just removed if it results in the >>>> * initrd to become inaccessible via the linear mapping. >>>> * Otherwise, this is a no-op >>>> */ >>>> - u64 base = initrd_start & PAGE_MASK; >>>> - u64 size = PAGE_ALIGN(initrd_end) - base; >>>> + u64 base = phys_initrd_start & PAGE_MASK; >>>> + u64 size = PAGE_ALIGN(phys_initrd_size); >>>> >>>> /* >>>> * We can only add back the initrd memory if we don't end up >>>> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void) >>>> */ >>>> memblock_reserve(__pa_symbol(_text), _end - _text); >>>> #ifdef CONFIG_BLK_DEV_INITRD >>>> - if (initrd_start) { >>>> - memblock_reserve(initrd_start, initrd_end - initrd_start); >>>> - >>>> + if (phys_initrd_size) { >>>> /* the generic initrd code expects virtual addresses */ >>>> - initrd_start = __phys_to_virt(initrd_start); >>>> - initrd_end = __phys_to_virt(initrd_end); >>>> + initrd_start = __phys_to_virt(phys_initrd_start); >>>> + initrd_end = initrd_start + phys_initrd_size; >>>> + initrd_below_start_ok = 0; >>> >>> Where is this assignment coming from? >> >> __early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though >> after patch #5 this is not necessary any more. > > Yes, but why? The original arm64 version of > __early_init_dt_declare_initrd() does not set it but now you set to 1 > in the IS_ENABLED(CONFIG_ARM64) section in the generic code and set it > back to 0 here. Humm, it is an if (!IS_ENABLED(CONFIG_ARM64)) condition, so we would not be taking that branch on an ARM64 kernel. If you are saying the assignment is not necessary anymore after patch #5 , that is true, though this can only be done a part of part #5, not as part of patch #4 in order not to break initrd functionality in-between patches. > > Or am I missing something? > Not sure, I could be too, it's Monday after all :)
On 5 November 2018 at 21:51, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 11/5/18 12:44 PM, Ard Biesheuvel wrote: >> On 5 November 2018 at 21:41, Florian Fainelli <f.fainelli@gmail.com> wrote: >>> On 11/5/18 12:39 PM, Ard Biesheuvel wrote: >>>> Hi Florian, >>>> >>>> On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>>> ARM64 is the only architecture that re-defines >>>>> __early_init_dt_declare_initrd() in order for that function to populate >>>>> initrd_start/initrd_end with physical addresses instead of virtual >>>>> addresses. Instead of having an override we can leverage >>>>> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to >>>>> populate those variables for us. >>>>> >>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>>> --- >>>>> arch/arm64/mm/init.c | 19 +++++++++---------- >>>>> 1 file changed, 9 insertions(+), 10 deletions(-) >>>>> >>>>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >>>>> index 3cf87341859f..00ef2166bb73 100644 >>>>> --- a/arch/arm64/mm/init.c >>>>> +++ b/arch/arm64/mm/init.c >>>>> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p) >>>>> if (*endp == ',') { >>>>> size = memparse(endp + 1, NULL); >>>>> >>>>> - initrd_start = start; >>>>> - initrd_end = start + size; >>>>> + phys_initrd_start = start; >>>>> + phys_initrd_size = size; >>>>> } >>>>> return 0; >>>>> } >>>>> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void) >>>>> memblock_add(__pa_symbol(_text), (u64)(_end - _text)); >>>>> } >>>>> >>>>> - if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) { >>>>> + if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) { >>>>> /* >>>>> * Add back the memory we just removed if it results in the >>>>> * initrd to become inaccessible via the linear mapping. >>>>> * Otherwise, this is a no-op >>>>> */ >>>>> - u64 base = initrd_start & PAGE_MASK; >>>>> - u64 size = PAGE_ALIGN(initrd_end) - base; >>>>> + u64 base = phys_initrd_start & PAGE_MASK; >>>>> + u64 size = PAGE_ALIGN(phys_initrd_size); >>>>> >>>>> /* >>>>> * We can only add back the initrd memory if we don't end up >>>>> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void) >>>>> */ >>>>> memblock_reserve(__pa_symbol(_text), _end - _text); >>>>> #ifdef CONFIG_BLK_DEV_INITRD >>>>> - if (initrd_start) { >>>>> - memblock_reserve(initrd_start, initrd_end - initrd_start); >>>>> - >>>>> + if (phys_initrd_size) { >>>>> /* the generic initrd code expects virtual addresses */ >>>>> - initrd_start = __phys_to_virt(initrd_start); >>>>> - initrd_end = __phys_to_virt(initrd_end); >>>>> + initrd_start = __phys_to_virt(phys_initrd_start); >>>>> + initrd_end = initrd_start + phys_initrd_size; >>>>> + initrd_below_start_ok = 0; >>>> >>>> Where is this assignment coming from? >>> >>> __early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though >>> after patch #5 this is not necessary any more. >> >> Yes, but why? The original arm64 version of >> __early_init_dt_declare_initrd() does not set it but now you set to 1 >> in the IS_ENABLED(CONFIG_ARM64) section in the generic code and set it >> back to 0 here. > > Humm, it is an if (!IS_ENABLED(CONFIG_ARM64)) condition, so we would not > be taking that branch on an ARM64 kernel. > Right. So now that we are not setting it to 1 on arm64, there is no longer a reason to set it to 0 again, no? > If you are saying the assignment is not necessary anymore after patch #5 > , that is true, though this can only be done a part of part #5, not as > part of patch #4 in order not to break initrd functionality in-between > patches. > >> >> Or am I missing something? >> > > Not sure, I could be too, it's Monday after all :) Yeah :-)
On 11/5/18 1:00 PM, Ard Biesheuvel wrote: > On 5 November 2018 at 21:51, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 11/5/18 12:44 PM, Ard Biesheuvel wrote: >>> On 5 November 2018 at 21:41, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>> On 11/5/18 12:39 PM, Ard Biesheuvel wrote: >>>>> Hi Florian, >>>>> >>>>> On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>>>> ARM64 is the only architecture that re-defines >>>>>> __early_init_dt_declare_initrd() in order for that function to populate >>>>>> initrd_start/initrd_end with physical addresses instead of virtual >>>>>> addresses. Instead of having an override we can leverage >>>>>> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to >>>>>> populate those variables for us. >>>>>> >>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>>>> --- >>>>>> arch/arm64/mm/init.c | 19 +++++++++---------- >>>>>> 1 file changed, 9 insertions(+), 10 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >>>>>> index 3cf87341859f..00ef2166bb73 100644 >>>>>> --- a/arch/arm64/mm/init.c >>>>>> +++ b/arch/arm64/mm/init.c >>>>>> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p) >>>>>> if (*endp == ',') { >>>>>> size = memparse(endp + 1, NULL); >>>>>> >>>>>> - initrd_start = start; >>>>>> - initrd_end = start + size; >>>>>> + phys_initrd_start = start; >>>>>> + phys_initrd_size = size; >>>>>> } >>>>>> return 0; >>>>>> } >>>>>> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void) >>>>>> memblock_add(__pa_symbol(_text), (u64)(_end - _text)); >>>>>> } >>>>>> >>>>>> - if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) { >>>>>> + if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) { >>>>>> /* >>>>>> * Add back the memory we just removed if it results in the >>>>>> * initrd to become inaccessible via the linear mapping. >>>>>> * Otherwise, this is a no-op >>>>>> */ >>>>>> - u64 base = initrd_start & PAGE_MASK; >>>>>> - u64 size = PAGE_ALIGN(initrd_end) - base; >>>>>> + u64 base = phys_initrd_start & PAGE_MASK; >>>>>> + u64 size = PAGE_ALIGN(phys_initrd_size); >>>>>> >>>>>> /* >>>>>> * We can only add back the initrd memory if we don't end up >>>>>> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void) >>>>>> */ >>>>>> memblock_reserve(__pa_symbol(_text), _end - _text); >>>>>> #ifdef CONFIG_BLK_DEV_INITRD >>>>>> - if (initrd_start) { >>>>>> - memblock_reserve(initrd_start, initrd_end - initrd_start); >>>>>> - >>>>>> + if (phys_initrd_size) { >>>>>> /* the generic initrd code expects virtual addresses */ >>>>>> - initrd_start = __phys_to_virt(initrd_start); >>>>>> - initrd_end = __phys_to_virt(initrd_end); >>>>>> + initrd_start = __phys_to_virt(phys_initrd_start); >>>>>> + initrd_end = initrd_start + phys_initrd_size; >>>>>> + initrd_below_start_ok = 0; >>>>> >>>>> Where is this assignment coming from? >>>> >>>> __early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though >>>> after patch #5 this is not necessary any more. >>> >>> Yes, but why? The original arm64 version of >>> __early_init_dt_declare_initrd() does not set it but now you set to 1 >>> in the IS_ENABLED(CONFIG_ARM64) section in the generic code and set it >>> back to 0 here. >> >> Humm, it is an if (!IS_ENABLED(CONFIG_ARM64)) condition, so we would not >> be taking that branch on an ARM64 kernel. >> > > Right. So now that we are not setting it to 1 on arm64, there is no > longer a reason to set it to 0 again, no? Correct, and in fact, this is not a problem either at patch #4 (which has the custom __early_init_dt_declare_initrd()) or #5 (which removes it), any other feedback you would like me to address before addressing Rob's suggestion? > >> If you are saying the assignment is not necessary anymore after patch #5 >> , that is true, though this can only be done a part of part #5, not as >> part of patch #4 in order not to break initrd functionality in-between >> patches. >> >>> >>> Or am I missing something? >>> >> >> Not sure, I could be too, it's Monday after all :) > > Yeah :-) >
On 5 November 2018 at 22:05, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 11/5/18 1:00 PM, Ard Biesheuvel wrote: >> On 5 November 2018 at 21:51, Florian Fainelli <f.fainelli@gmail.com> wrote: >>> On 11/5/18 12:44 PM, Ard Biesheuvel wrote: >>>> On 5 November 2018 at 21:41, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>>> On 11/5/18 12:39 PM, Ard Biesheuvel wrote: >>>>>> Hi Florian, >>>>>> >>>>>> On 31 October 2018 at 20:28, Florian Fainelli <f.fainelli@gmail.com> wrote: >>>>>>> ARM64 is the only architecture that re-defines >>>>>>> __early_init_dt_declare_initrd() in order for that function to populate >>>>>>> initrd_start/initrd_end with physical addresses instead of virtual >>>>>>> addresses. Instead of having an override we can leverage >>>>>>> drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to >>>>>>> populate those variables for us. >>>>>>> >>>>>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >>>>>>> --- >>>>>>> arch/arm64/mm/init.c | 19 +++++++++---------- >>>>>>> 1 file changed, 9 insertions(+), 10 deletions(-) >>>>>>> >>>>>>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >>>>>>> index 3cf87341859f..00ef2166bb73 100644 >>>>>>> --- a/arch/arm64/mm/init.c >>>>>>> +++ b/arch/arm64/mm/init.c >>>>>>> @@ -72,8 +72,8 @@ static int __init early_initrd(char *p) >>>>>>> if (*endp == ',') { >>>>>>> size = memparse(endp + 1, NULL); >>>>>>> >>>>>>> - initrd_start = start; >>>>>>> - initrd_end = start + size; >>>>>>> + phys_initrd_start = start; >>>>>>> + phys_initrd_size = size; >>>>>>> } >>>>>>> return 0; >>>>>>> } >>>>>>> @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void) >>>>>>> memblock_add(__pa_symbol(_text), (u64)(_end - _text)); >>>>>>> } >>>>>>> >>>>>>> - if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) { >>>>>>> + if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) { >>>>>>> /* >>>>>>> * Add back the memory we just removed if it results in the >>>>>>> * initrd to become inaccessible via the linear mapping. >>>>>>> * Otherwise, this is a no-op >>>>>>> */ >>>>>>> - u64 base = initrd_start & PAGE_MASK; >>>>>>> - u64 size = PAGE_ALIGN(initrd_end) - base; >>>>>>> + u64 base = phys_initrd_start & PAGE_MASK; >>>>>>> + u64 size = PAGE_ALIGN(phys_initrd_size); >>>>>>> >>>>>>> /* >>>>>>> * We can only add back the initrd memory if we don't end up >>>>>>> @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void) >>>>>>> */ >>>>>>> memblock_reserve(__pa_symbol(_text), _end - _text); >>>>>>> #ifdef CONFIG_BLK_DEV_INITRD >>>>>>> - if (initrd_start) { >>>>>>> - memblock_reserve(initrd_start, initrd_end - initrd_start); >>>>>>> - >>>>>>> + if (phys_initrd_size) { >>>>>>> /* the generic initrd code expects virtual addresses */ >>>>>>> - initrd_start = __phys_to_virt(initrd_start); >>>>>>> - initrd_end = __phys_to_virt(initrd_end); >>>>>>> + initrd_start = __phys_to_virt(phys_initrd_start); >>>>>>> + initrd_end = initrd_start + phys_initrd_size; >>>>>>> + initrd_below_start_ok = 0; >>>>>> >>>>>> Where is this assignment coming from? >>>>> >>>>> __early_init_dt_declare_initrd() sets initrd_below_start_ok to 1 though >>>>> after patch #5 this is not necessary any more. >>>> >>>> Yes, but why? The original arm64 version of >>>> __early_init_dt_declare_initrd() does not set it but now you set to 1 >>>> in the IS_ENABLED(CONFIG_ARM64) section in the generic code and set it >>>> back to 0 here. >>> >>> Humm, it is an if (!IS_ENABLED(CONFIG_ARM64)) condition, so we would not >>> be taking that branch on an ARM64 kernel. >>> >> >> Right. So now that we are not setting it to 1 on arm64, there is no >> longer a reason to set it to 0 again, no? > > Correct, and in fact, this is not a problem either at patch #4 (which > has the custom __early_init_dt_declare_initrd()) or #5 (which removes > it), any other feedback you would like me to address before addressing > Rob's suggestion? > No, I think this is ok. The conditional on arm64 in generic code is a bit nasty, and it would be nicer generally if we only have to record a single memory range, but if this fixes the issue you were addressing originally, I'm fine with it.
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index 3cf87341859f..00ef2166bb73 100644 --- a/arch/arm64/mm/init.c +++ b/arch/arm64/mm/init.c @@ -72,8 +72,8 @@ static int __init early_initrd(char *p) if (*endp == ',') { size = memparse(endp + 1, NULL); - initrd_start = start; - initrd_end = start + size; + phys_initrd_start = start; + phys_initrd_size = size; } return 0; } @@ -408,14 +408,14 @@ void __init arm64_memblock_init(void) memblock_add(__pa_symbol(_text), (u64)(_end - _text)); } - if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && initrd_start) { + if (IS_ENABLED(CONFIG_BLK_DEV_INITRD) && phys_initrd_size) { /* * Add back the memory we just removed if it results in the * initrd to become inaccessible via the linear mapping. * Otherwise, this is a no-op */ - u64 base = initrd_start & PAGE_MASK; - u64 size = PAGE_ALIGN(initrd_end) - base; + u64 base = phys_initrd_start & PAGE_MASK; + u64 size = PAGE_ALIGN(phys_initrd_size); /* * We can only add back the initrd memory if we don't end up @@ -460,12 +460,11 @@ void __init arm64_memblock_init(void) */ memblock_reserve(__pa_symbol(_text), _end - _text); #ifdef CONFIG_BLK_DEV_INITRD - if (initrd_start) { - memblock_reserve(initrd_start, initrd_end - initrd_start); - + if (phys_initrd_size) { /* the generic initrd code expects virtual addresses */ - initrd_start = __phys_to_virt(initrd_start); - initrd_end = __phys_to_virt(initrd_end); + initrd_start = __phys_to_virt(phys_initrd_start); + initrd_end = initrd_start + phys_initrd_size; + initrd_below_start_ok = 0; } #endif
ARM64 is the only architecture that re-defines __early_init_dt_declare_initrd() in order for that function to populate initrd_start/initrd_end with physical addresses instead of virtual addresses. Instead of having an override we can leverage drivers/of/fdt.c populating phys_initrd_start/phys_initrd_size to populate those variables for us. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- arch/arm64/mm/init.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)