[v4,6/6] arch: Move initrd= parsing into do_mounts_initrd.c

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

Commit Message

Florian Fainelli Nov. 5, 2018, 10:54 p.m.
ARC, ARM, ARM64 and Unicore32 are all capable of parsing the "initrd="
command line parameter to allow specifying the physical address and size
of an initrd. Move that parsing into init/do_mounts_initrd.c such that
we no longer duplicate that logic.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arc/mm/init.c       | 25 +++++--------------------
 arch/arm/mm/init.c       | 17 -----------------
 arch/arm64/mm/init.c     | 18 ------------------
 arch/unicore32/mm/init.c | 18 ------------------
 init/do_mounts_initrd.c  | 17 +++++++++++++++++
 5 files changed, 22 insertions(+), 73 deletions(-)

Comments

Vineet Gupta Nov. 13, 2018, 12:34 a.m. | #1
On 11/5/18 2:58 PM, Florian Fainelli wrote:
> ARC, ARM, ARM64 and Unicore32 are all capable of parsing the "initrd="
> command line parameter to allow specifying the physical address and size
> of an initrd. Move that parsing into init/do_mounts_initrd.c such that
> we no longer duplicate that logic.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  arch/arc/mm/init.c       | 25 +++++--------------------
>  arch/arm/mm/init.c       | 17 -----------------
>  arch/arm64/mm/init.c     | 18 ------------------
>  arch/unicore32/mm/init.c | 18 ------------------
>  init/do_mounts_initrd.c  | 17 +++++++++++++++++
>  5 files changed, 22 insertions(+), 73 deletions(-)
>
> diff --git a/arch/arc/mm/init.c b/arch/arc/mm/init.c
> index f8fe5668b30f..43bf4c3a1290 100644
> --- a/arch/arc/mm/init.c
> +++ b/arch/arc/mm/init.c
> @@ -78,24 +78,6 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size)
>  		base, TO_MB(size), !in_use ? "Not used":"");
>  }
>  
> -#ifdef CONFIG_BLK_DEV_INITRD
> -static int __init early_initrd(char *p)
> -{
> -	unsigned long start, size;
> -	char *endp;
> -
> -	start = memparse(p, &endp);
> -	if (*endp == ',') {
> -		size = memparse(endp + 1, NULL);
> -
> -		initrd_start = (unsigned long)__va(start);
> -		initrd_end = (unsigned long)__va(start + size);
> -	}
> -	return 0;
> -}
> -early_param("initrd", early_initrd);
> -#endif
> -
>  /*
>   * First memory setup routine called from setup_arch()
>   * 1. setup swapper's mm @init_mm
> @@ -140,8 +122,11 @@ void __init setup_arch_memory(void)
>  	memblock_reserve(low_mem_start, __pa(_end) - low_mem_start);
>  
>  #ifdef CONFIG_BLK_DEV_INITRD
> -	if (initrd_start)
> -		memblock_reserve(__pa(initrd_start), initrd_end - initrd_start);
> +	if (phys_initrd_size) {
> +		memblock_reserve(phys_initrd_start, phys_initrd_size);
> +		initrd_start = (unsigned long)__va(phys_initrd_start);
> +		initrd_end = initrd_start + phys_initrd_size;
> +	}
>  #endif

The common code now uses phys_initrd*, and you also use the same in ARC code, do
we still need the initrd_* setting here ?
ARC semantics was using them as PA anyways.

[snip]...

>  /*
>   * This keeps memory configuration data used by a couple memory
>   * initialization functions, as well as show_mem() for the skipping
> diff --git a/init/do_mounts_initrd.c b/init/do_mounts_initrd.c
> index 45865b72f4ea..732d21f4a637 100644
> --- a/init/do_mounts_initrd.c
> +++ b/init/do_mounts_initrd.c
> @@ -27,6 +27,23 @@ static int __init no_initrd(char *str)
>  
>  __setup("noinitrd", no_initrd);
>  
> +static int __init early_initrd(char *p)
> +{
> +	phys_addr_t start;
> +	unsigned long size;
> +	char *endp;
> +
> +	start = memparse(p, &endp);
> +	if (*endp == ',') {
> +		size = memparse(endp + 1, NULL);
> +
> +		phys_initrd_start = start;
> +		phys_initrd_size = size;
> +	}
> +	return 0;
> +}
> +early_param("initrd", early_initrd);
> +
>  static int init_linuxrc(struct subprocess_info *info, struct cred *new)
>  {
>  	ksys_unshare(CLONE_FS | CLONE_FILES);
Florian Fainelli Nov. 13, 2018, 12:37 a.m. | #2
On 11/12/18 4:34 PM, Vineet Gupta wrote:
> On 11/5/18 2:58 PM, Florian Fainelli wrote:
>> ARC, ARM, ARM64 and Unicore32 are all capable of parsing the "initrd="
>> command line parameter to allow specifying the physical address and size
>> of an initrd. Move that parsing into init/do_mounts_initrd.c such that
>> we no longer duplicate that logic.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  arch/arc/mm/init.c       | 25 +++++--------------------
>>  arch/arm/mm/init.c       | 17 -----------------
>>  arch/arm64/mm/init.c     | 18 ------------------
>>  arch/unicore32/mm/init.c | 18 ------------------
>>  init/do_mounts_initrd.c  | 17 +++++++++++++++++
>>  5 files changed, 22 insertions(+), 73 deletions(-)
>>
>> diff --git a/arch/arc/mm/init.c b/arch/arc/mm/init.c
>> index f8fe5668b30f..43bf4c3a1290 100644
>> --- a/arch/arc/mm/init.c
>> +++ b/arch/arc/mm/init.c
>> @@ -78,24 +78,6 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size)
>>  		base, TO_MB(size), !in_use ? "Not used":"");
>>  }
>>  
>> -#ifdef CONFIG_BLK_DEV_INITRD
>> -static int __init early_initrd(char *p)
>> -{
>> -	unsigned long start, size;
>> -	char *endp;
>> -
>> -	start = memparse(p, &endp);
>> -	if (*endp == ',') {
>> -		size = memparse(endp + 1, NULL);
>> -
>> -		initrd_start = (unsigned long)__va(start);
>> -		initrd_end = (unsigned long)__va(start + size);
>> -	}
>> -	return 0;
>> -}
>> -early_param("initrd", early_initrd);
>> -#endif
>> -
>>  /*
>>   * First memory setup routine called from setup_arch()
>>   * 1. setup swapper's mm @init_mm
>> @@ -140,8 +122,11 @@ void __init setup_arch_memory(void)
>>  	memblock_reserve(low_mem_start, __pa(_end) - low_mem_start);
>>  
>>  #ifdef CONFIG_BLK_DEV_INITRD
>> -	if (initrd_start)
>> -		memblock_reserve(__pa(initrd_start), initrd_end - initrd_start);
>> +	if (phys_initrd_size) {
>> +		memblock_reserve(phys_initrd_start, phys_initrd_size);
>> +		initrd_start = (unsigned long)__va(phys_initrd_start);
>> +		initrd_end = initrd_start + phys_initrd_size;
>> +	}
>>  #endif
> 
> The common code now uses phys_initrd*, and you also use the same in ARC code, do
> we still need the initrd_* setting here ?
> ARC semantics was using them as PA anyways.

Yes, the generic initrd code expects initrd_start/end to be virtual
addresses, which we now directly derive from phys_initrd_start, that
should really be equivalent.
Vineet Gupta Nov. 13, 2018, 12:40 a.m. | #3
On 11/12/18 4:38 PM, Florian Fainelli wrote:
>>>  #ifdef CONFIG_BLK_DEV_INITRD
>>> -	if (initrd_start)
>>> -		memblock_reserve(__pa(initrd_start), initrd_end - initrd_start);
>>> +	if (phys_initrd_size) {
>>> +		memblock_reserve(phys_initrd_start, phys_initrd_size);
>>> +		initrd_start = (unsigned long)__va(phys_initrd_start);
>>> +		initrd_end = initrd_start + phys_initrd_size;
>>> +	}
>>>  #endif
>> The common code now uses phys_initrd*, and you also use the same in ARC code, do
>> we still need the initrd_* setting here ?
>> ARC semantics was using them as PA anyways.
> Yes, the generic initrd code expects initrd_start/end to be virtual
> addresses, which we now directly derive from phys_initrd_start, that
> should really be equivalent.

So we can skip this explicit setting above - ARC arch code doesn't access the virt
initrd_start
Florian Fainelli Nov. 13, 2018, 12:52 a.m. | #4
On 11/12/18 4:40 PM, Vineet Gupta wrote:
> On 11/12/18 4:38 PM, Florian Fainelli wrote:
>>>>  #ifdef CONFIG_BLK_DEV_INITRD
>>>> -	if (initrd_start)
>>>> -		memblock_reserve(__pa(initrd_start), initrd_end - initrd_start);
>>>> +	if (phys_initrd_size) {
>>>> +		memblock_reserve(phys_initrd_start, phys_initrd_size);
>>>> +		initrd_start = (unsigned long)__va(phys_initrd_start);
>>>> +		initrd_end = initrd_start + phys_initrd_size;
>>>> +	}
>>>>  #endif
>>> The common code now uses phys_initrd*, and you also use the same in ARC code, do
>>> we still need the initrd_* setting here ?
>>> ARC semantics was using them as PA anyways.
>> Yes, the generic initrd code expects initrd_start/end to be virtual
>> addresses, which we now directly derive from phys_initrd_start, that
>> should really be equivalent.
> 
> So we can skip this explicit setting above - ARC arch code doesn't access the virt
> initrd_start

OK, you are saying we could just have the generic initrd code do this
assignment instead of having each architecture do it, is that a correct
understanding? If so, I suppose it could be done, whether as of this
patch series or as a follow-up, either way is fine with me.

One possible caveat is if __va() and __phys_to_virt() behave differently
(e.g: because of CONFIG_DEBUG_VIRTUAL or other things).
Vineet Gupta Nov. 13, 2018, 12:57 a.m. | #5
On 11/12/18 4:52 PM, Florian Fainelli wrote:
> On 11/12/18 4:40 PM, Vineet Gupta wrote:
>> On 11/12/18 4:38 PM, Florian Fainelli wrote:
>>>>>  #ifdef CONFIG_BLK_DEV_INITRD
>>>>> -	if (initrd_start)
>>>>> -		memblock_reserve(__pa(initrd_start), initrd_end - initrd_start);
>>>>> +	if (phys_initrd_size) {
>>>>> +		memblock_reserve(phys_initrd_start, phys_initrd_size);
>>>>> +		initrd_start = (unsigned long)__va(phys_initrd_start);
>>>>> +		initrd_end = initrd_start + phys_initrd_size;
>>>>> +	}
>>>>>  #endif
>>>> The common code now uses phys_initrd*, and you also use the same in ARC code, do
>>>> we still need the initrd_* setting here ?
>>>> ARC semantics was using them as PA anyways.
>>> Yes, the generic initrd code expects initrd_start/end to be virtual
>>> addresses, which we now directly derive from phys_initrd_start, that
>>> should really be equivalent.
>> So we can skip this explicit setting above - ARC arch code doesn't access the virt
>> initrd_start
> OK, you are saying we could just have the generic initrd code do this
> assignment instead of having each architecture do it, is that a correct
> understanding? 

Correct !

> If so, I suppose it could be done, whether as of this
> patch series or as a follow-up, either way is fine with me.

If it is not too much trouble, I'd prefer this now. I should have chimed earlier.

> One possible caveat is if __va() and __phys_to_virt() behave differently
> (e.g: because of CONFIG_DEBUG_VIRTUAL or other things).


Thing is, after your patches, we don't use the vanilla initrd_xxx in arch code any
longer. So this becomes just an implementation detail, which core code may or
maynot need and if it does, this needs to work already w/o having to set anything
in arch code. Agree ?

Thx,
-Vineet
Florian Fainelli Nov. 15, 2018, 7:35 p.m. | #6
On 11/12/18 4:57 PM, Vineet Gupta wrote:
> On 11/12/18 4:52 PM, Florian Fainelli wrote:
>> On 11/12/18 4:40 PM, Vineet Gupta wrote:
>>> On 11/12/18 4:38 PM, Florian Fainelli wrote:
>>>>>>  #ifdef CONFIG_BLK_DEV_INITRD
>>>>>> -	if (initrd_start)
>>>>>> -		memblock_reserve(__pa(initrd_start), initrd_end - initrd_start);
>>>>>> +	if (phys_initrd_size) {
>>>>>> +		memblock_reserve(phys_initrd_start, phys_initrd_size);
>>>>>> +		initrd_start = (unsigned long)__va(phys_initrd_start);
>>>>>> +		initrd_end = initrd_start + phys_initrd_size;
>>>>>> +	}
>>>>>>  #endif
>>>>> The common code now uses phys_initrd*, and you also use the same in ARC code, do
>>>>> we still need the initrd_* setting here ?
>>>>> ARC semantics was using them as PA anyways.
>>>> Yes, the generic initrd code expects initrd_start/end to be virtual
>>>> addresses, which we now directly derive from phys_initrd_start, that
>>>> should really be equivalent.
>>> So we can skip this explicit setting above - ARC arch code doesn't access the virt
>>> initrd_start
>> OK, you are saying we could just have the generic initrd code do this
>> assignment instead of having each architecture do it, is that a correct
>> understanding? 
> 
> Correct !
> 
>> If so, I suppose it could be done, whether as of this
>> patch series or as a follow-up, either way is fine with me.
> 
> If it is not too much trouble, I'd prefer this now. I should have chimed earlier.
> 
>> One possible caveat is if __va() and __phys_to_virt() behave differently
>> (e.g: because of CONFIG_DEBUG_VIRTUAL or other things).
> 
> 
> Thing is, after your patches, we don't use the vanilla initrd_xxx in arch code any
> longer. So this becomes just an implementation detail, which core code may or
> maynot need and if it does, this needs to work already w/o having to set anything
> in arch code. Agree ?

If you do not mind, I would prefer this series to go in, as-is, and
clean up the initrd_start/initrd_end assignment as a follow up patch
series. The reason is mostly that I am not yet clear on the timing of
these operations between the architecture resolving the virtual address
and the initrd code starting to use it.

Would that sound reasonable to you?

Patch

diff --git a/arch/arc/mm/init.c b/arch/arc/mm/init.c
index f8fe5668b30f..43bf4c3a1290 100644
--- a/arch/arc/mm/init.c
+++ b/arch/arc/mm/init.c
@@ -78,24 +78,6 @@  void __init early_init_dt_add_memory_arch(u64 base, u64 size)
 		base, TO_MB(size), !in_use ? "Not used":"");
 }
 
-#ifdef CONFIG_BLK_DEV_INITRD
-static int __init early_initrd(char *p)
-{
-	unsigned long start, size;
-	char *endp;
-
-	start = memparse(p, &endp);
-	if (*endp == ',') {
-		size = memparse(endp + 1, NULL);
-
-		initrd_start = (unsigned long)__va(start);
-		initrd_end = (unsigned long)__va(start + size);
-	}
-	return 0;
-}
-early_param("initrd", early_initrd);
-#endif
-
 /*
  * First memory setup routine called from setup_arch()
  * 1. setup swapper's mm @init_mm
@@ -140,8 +122,11 @@  void __init setup_arch_memory(void)
 	memblock_reserve(low_mem_start, __pa(_end) - low_mem_start);
 
 #ifdef CONFIG_BLK_DEV_INITRD
-	if (initrd_start)
-		memblock_reserve(__pa(initrd_start), initrd_end - initrd_start);
+	if (phys_initrd_size) {
+		memblock_reserve(phys_initrd_start, phys_initrd_size);
+		initrd_start = (unsigned long)__va(phys_initrd_start);
+		initrd_end = initrd_start + phys_initrd_size;
+	}
 #endif
 
 	early_init_fdt_reserve_self();
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index a3b6f1f1cbaf..478ea8b7db87 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -51,23 +51,6 @@  unsigned long __init __clear_cr(unsigned long mask)
 #endif
 
 #ifdef CONFIG_BLK_DEV_INITRD
-static int __init early_initrd(char *p)
-{
-	phys_addr_t start;
-	unsigned long size;
-	char *endp;
-
-	start = memparse(p, &endp);
-	if (*endp == ',') {
-		size = memparse(endp + 1, NULL);
-
-		phys_initrd_start = start;
-		phys_initrd_size = size;
-	}
-	return 0;
-}
-early_param("initrd", early_initrd);
-
 static int __init parse_tag_initrd(const struct tag *tag)
 {
 	pr_warn("ATAG_INITRD is deprecated; "
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index a66ffcde5f13..7474093363bc 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -61,24 +61,6 @@ 
 s64 memstart_addr __ro_after_init = -1;
 phys_addr_t arm64_dma_phys_limit __ro_after_init;
 
-#ifdef CONFIG_BLK_DEV_INITRD
-static int __init early_initrd(char *p)
-{
-	unsigned long start, size;
-	char *endp;
-
-	start = memparse(p, &endp);
-	if (*endp == ',') {
-		size = memparse(endp + 1, NULL);
-
-		phys_initrd_start = start;
-		phys_initrd_size = size;
-	}
-	return 0;
-}
-early_param("initrd", early_initrd);
-#endif
-
 #ifdef CONFIG_KEXEC_CORE
 /*
  * reserve_crashkernel() - reserves memory for crash kernel
diff --git a/arch/unicore32/mm/init.c b/arch/unicore32/mm/init.c
index 02aa2c0b295e..85ef2c624090 100644
--- a/arch/unicore32/mm/init.c
+++ b/arch/unicore32/mm/init.c
@@ -30,24 +30,6 @@ 
 
 #include "mm.h"
 
-#ifdef CONFIG_BLK_DEV_INITRD
-static int __init early_initrd(char *p)
-{
-	unsigned long start, size;
-	char *endp;
-
-	start = memparse(p, &endp);
-	if (*endp == ',') {
-		size = memparse(endp + 1, NULL);
-
-		phys_initrd_start = start;
-		phys_initrd_size = size;
-	}
-	return 0;
-}
-early_param("initrd", early_initrd);
-#endif
-
 /*
  * This keeps memory configuration data used by a couple memory
  * initialization functions, as well as show_mem() for the skipping
diff --git a/init/do_mounts_initrd.c b/init/do_mounts_initrd.c
index 45865b72f4ea..732d21f4a637 100644
--- a/init/do_mounts_initrd.c
+++ b/init/do_mounts_initrd.c
@@ -27,6 +27,23 @@  static int __init no_initrd(char *str)
 
 __setup("noinitrd", no_initrd);
 
+static int __init early_initrd(char *p)
+{
+	phys_addr_t start;
+	unsigned long size;
+	char *endp;
+
+	start = memparse(p, &endp);
+	if (*endp == ',') {
+		size = memparse(endp + 1, NULL);
+
+		phys_initrd_start = start;
+		phys_initrd_size = size;
+	}
+	return 0;
+}
+early_param("initrd", early_initrd);
+
 static int init_linuxrc(struct subprocess_info *info, struct cred *new)
 {
 	ksys_unshare(CLONE_FS | CLONE_FILES);