[v3,4/6] arm64: Utilize phys_initrd_start/phys_initrd_size

Message ID 20181031192843.13230-5-f.fainelli@gmail.com
State New
Headers show
Series
  • arm64: Get rid of __early_init_dt_declare_initrd()
Related show

Commit Message

Florian Fainelli Oct. 31, 2018, 7:28 p.m.
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(-)

Comments

Rob Herring Nov. 5, 2018, 8:33 p.m. | #1
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
>
Ard Biesheuvel Nov. 5, 2018, 8:39 p.m. | #2
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
>
Florian Fainelli Nov. 5, 2018, 8:41 p.m. | #3
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.
Ard Biesheuvel Nov. 5, 2018, 8:44 p.m. | #4
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?
Florian Fainelli Nov. 5, 2018, 8:51 p.m. | #5
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 :)
Ard Biesheuvel Nov. 5, 2018, 9 p.m. | #6
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 :-)
Florian Fainelli Nov. 5, 2018, 9:05 p.m. | #7
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 :-)
>
Ard Biesheuvel Nov. 5, 2018, 9:07 p.m. | #8
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.

Patch

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