[PATCH] xilinx: zynqmp: Do not use null as spl bss start address

[PATCH] xilinx: zynqmp: Do not use null as spl bss start address

[Stefan Herbrechtsmeier]

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Do not use null as address for memory because of the special meaning for
pointers. Change the spl bss start address to the second page.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

---
The problem was discovered with a static char array initialized with an
empty string.

 include/configs/xilinx_zynqmp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/configs/xilinx_zynqmp.h b/include/configs/xilinx_zynqmp.h
index f25d796a1e..21a5cf1617 100644
--- a/include/configs/xilinx_zynqmp.h
+++ b/include/configs/xilinx_zynqmp.h
@@ -209,7 +209,7 @@
 #define CONFIG_SPL_MAX_SIZE		0x40000
 
 /* Just random location in OCM */
-#define CONFIG_SPL_BSS_START_ADDR	0x0
+#define CONFIG_SPL_BSS_START_ADDR	0x1000
 #define CONFIG_SPL_BSS_MAX_SIZE		0x80000
 
 #if defined(CONFIG_SPL_SPI_FLASH_SUPPORT)
-- 
2.30.2

Re: [PATCH] xilinx: zynqmp: Do not use null as spl bss start address

[Michal Simek]

Hi,

first of all subject is not accurate. We are not using null as start but address 0.

On 6/1/22 12:55, Stefan Herbrechtsmeier wrote:
> [CAUTION: External Email]
> 
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> 
> Do not use null as address for memory because of the special meaning for
> pointers. Change the spl bss start address to the second page.
> 
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> 
> ---
> The problem was discovered with a static char array initialized with an
> empty string.

It means your code is doing wrong pointer arithmeticians which pointed to BSS 
section and overwrites there something. What is that code doing?

I have not a problem to move BSS section but I want to at least clean that 
message to be accurate.

Thanks,
Michal

Re: [PATCH] xilinx: zynqmp: Do not use null as spl bss start address

[Stefan Herbrechtsmeier]

Hi,

Am 01.06.2022 um 13:59 schrieb Michal Simek:
> Hi,
> 
> first of all subject is not accurate. We are not using null as start but 
> address 0.

I will replace null with 0.

> 
> On 6/1/22 12:55, Stefan Herbrechtsmeier wrote:
>> [CAUTION: External Email]
>>
>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>
>> Do not use null as address for memory because of the special meaning for
>> pointers. Change the spl bss start address to the second page.
>>
>> Signed-off-by: Stefan Herbrechtsmeier 
>> <stefan.herbrechtsmeier@weidmueller.com>
>>
>> ---
>> The problem was discovered with a static char array initialized with an
>> empty string.
> 
> It means your code is doing wrong pointer arithmeticians which pointed 
> to BSS section and overwrites there something. What is that code doing?

I like to call the zynqmp_get_silicon_idcode_name function from my board 
code and therefore rework the function to return static memory to avoid 
a memory allocation on each call.

-	char name[ZYNQMP_VERSION_SIZE];
+	static char name[ZYNQMP_VERSION_SIZE] = "";

+	if (name[0])
+		return name;

-	return strdup(name);
+	return name;

The name variable gets the address 0 which means that snprintf and 
strncat do nothing because the dest pointer and NULL are equal.

Regards
   Stefan

Re: [PATCH] xilinx: zynqmp: Do not use null as spl bss start address

[Michal Simek]

On 6/1/22 14:27, Stefan Herbrechtsmeier wrote:
> Hi,
> 
> Am 01.06.2022 um 13:59 schrieb Michal Simek:
>> Hi,
>>
>> first of all subject is not accurate. We are not using null as start but 
>> address 0.
> 
> I will replace null with 0.
> 
>>
>> On 6/1/22 12:55, Stefan Herbrechtsmeier wrote:
>>> [CAUTION: External Email]
>>>
>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>>
>>> Do not use null as address for memory because of the special meaning for
>>> pointers. Change the spl bss start address to the second page.
>>>
>>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>>
>>> ---
>>> The problem was discovered with a static char array initialized with an
>>> empty string.
>>
>> It means your code is doing wrong pointer arithmeticians which pointed to BSS 
>> section and overwrites there something. What is that code doing?
> 
> I like to call the zynqmp_get_silicon_idcode_name function from my board code 
> and therefore rework the function to return static memory to avoid a memory 
> allocation on each call.
> 
> -    char name[ZYNQMP_VERSION_SIZE];
> +    static char name[ZYNQMP_VERSION_SIZE] = "";

Try to remove = "".
Undefined variables should be placed to bss section.

> 
> +    if (name[0])
> +        return name;
> 
> -    return strdup(name);
> +    return name;
> 
> The name variable gets the address 0 which means that snprintf and strncat do 
> nothing because the dest pointer and NULL are equal.

Ok. I see. It become the first variable in bss section.

Thanks,
Michal

Re: [PATCH] xilinx: zynqmp: Do not use null as spl bss start address

[Stefan Herbrechtsmeier]

Hi,

Am 01.06.2022 um 14:59 schrieb Michal Simek:
> 
> 
> On 6/1/22 14:27, Stefan Herbrechtsmeier wrote:
>> Hi,
>>
>> Am 01.06.2022 um 13:59 schrieb Michal Simek:
>>> Hi,
>>>
>>> first of all subject is not accurate. We are not using null as start 
>>> but address 0.
>>
>> I will replace null with 0.
>>
>>>
>>> On 6/1/22 12:55, Stefan Herbrechtsmeier wrote:
>>>> [CAUTION: External Email]
>>>>
>>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>>>
>>>> Do not use null as address for memory because of the special meaning 
>>>> for
>>>> pointers. Change the spl bss start address to the second page.
>>>>
>>>> Signed-off-by: Stefan Herbrechtsmeier 
>>>> <stefan.herbrechtsmeier@weidmueller.com>
>>>>
>>>> ---
>>>> The problem was discovered with a static char array initialized with an
>>>> empty string.
>>>
>>> It means your code is doing wrong pointer arithmeticians which 
>>> pointed to BSS section and overwrites there something. What is that 
>>> code doing?
>>
>> I like to call the zynqmp_get_silicon_idcode_name function from my 
>> board code and therefore rework the function to return static memory 
>> to avoid a memory allocation on each call.
>>
>> -    char name[ZYNQMP_VERSION_SIZE];
>> +    static char name[ZYNQMP_VERSION_SIZE] = "";
> 
> Try to remove = "".

The = "" is needed to skip the processing on the second run.

> Undefined variables should be placed to bss section.

A = "zu" will also move the variable to an other section.

> 
>>
>> +    if (name[0])
>> +        return name;
>>
>> -    return strdup(name);
>> +    return name;
>>
>> The name variable gets the address 0 which means that snprintf and 
>> strncat do nothing because the dest pointer and NULL are equal.
> 
> Ok. I see. It become the first variable in bss section.

Should I resend the patch with null replaced by 0?

Regard
   Stefan

Re: [PATCH] xilinx: zynqmp: Do not use null as spl bss start address

[Michal Simek]

On 6/1/22 16:03, Stefan Herbrechtsmeier wrote:
> Hi,
> 
> Am 01.06.2022 um 14:59 schrieb Michal Simek:
>>
>>
>> On 6/1/22 14:27, Stefan Herbrechtsmeier wrote:
>>> Hi,
>>>
>>> Am 01.06.2022 um 13:59 schrieb Michal Simek:
>>>> Hi,
>>>>
>>>> first of all subject is not accurate. We are not using null as start but 
>>>> address 0.
>>>
>>> I will replace null with 0.
>>>
>>>>
>>>> On 6/1/22 12:55, Stefan Herbrechtsmeier wrote:
>>>>> [CAUTION: External Email]
>>>>>
>>>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>>>>
>>>>> Do not use null as address for memory because of the special meaning for
>>>>> pointers. Change the spl bss start address to the second page.
>>>>>
>>>>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>>>>
>>>>> ---
>>>>> The problem was discovered with a static char array initialized with an
>>>>> empty string.
>>>>
>>>> It means your code is doing wrong pointer arithmeticians which pointed to 
>>>> BSS section and overwrites there something. What is that code doing?
>>>
>>> I like to call the zynqmp_get_silicon_idcode_name function from my board code 
>>> and therefore rework the function to return static memory to avoid a memory 
>>> allocation on each call.
>>>
>>> -    char name[ZYNQMP_VERSION_SIZE];
>>> +    static char name[ZYNQMP_VERSION_SIZE] = "";
>>
>> Try to remove = "".
> 
> The = "" is needed to skip the processing on the second run.

I looked if this really change anything. Static variable is placed to BSS 
section which is zeroed by default at start. It means before you jump to this 
function name array is already zeroed.


> 
>> Undefined variables should be placed to bss section.
> 
> A = "zu" will also move the variable to an other section.
> 
>>
>>>
>>> +    if (name[0])
>>> +        return name;
>>>
>>> -    return strdup(name);
>>> +    return name;

this function is called only once now that's why I personally can't see to do 
any change there unless you have good reason to call it more times.

But that being said I think that would make sense to move the whole 
zynqmp_get_silicon_idcode_name() to soc_xilinx_zynqmp.c and do silicon detection 
there. Only option there is soc_get_machine() which can return silicon type.


>>>
>>> The name variable gets the address 0 which means that snprintf and strncat do 
>>> nothing because the dest pointer and NULL are equal.
>>
>> Ok. I see. It become the first variable in bss section.
> 
> Should I resend the patch with null replaced by 0?

Yes please.
M

Re: [PATCH] xilinx: zynqmp: Do not use null as spl bss start address

[Stefan Herbrechtsmeier]

Am 03.06.2022 um 14:12 schrieb Michal Simek:
> 
> 
> On 6/1/22 16:03, Stefan Herbrechtsmeier wrote:
>> Hi,
>>
>> Am 01.06.2022 um 14:59 schrieb Michal Simek:
>>>
>>>
>>> On 6/1/22 14:27, Stefan Herbrechtsmeier wrote:
>>>> Hi,
>>>>
>>>> Am 01.06.2022 um 13:59 schrieb Michal Simek:
>>>>> Hi,
>>>>>
>>>>> first of all subject is not accurate. We are not using null as 
>>>>> start but address 0.
>>>>
>>>> I will replace null with 0.
>>>>
>>>>>
>>>>> On 6/1/22 12:55, Stefan Herbrechtsmeier wrote:
>>>>>> [CAUTION: External Email]
>>>>>>
>>>>>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>>>>>
>>>>>> Do not use null as address for memory because of the special 
>>>>>> meaning for
>>>>>> pointers. Change the spl bss start address to the second page.
>>>>>>
>>>>>> Signed-off-by: Stefan Herbrechtsmeier 
>>>>>> <stefan.herbrechtsmeier@weidmueller.com>
>>>>>>
>>>>>> ---
>>>>>> The problem was discovered with a static char array initialized 
>>>>>> with an
>>>>>> empty string.
>>>>>
>>>>> It means your code is doing wrong pointer arithmeticians which 
>>>>> pointed to BSS section and overwrites there something. What is that 
>>>>> code doing?
>>>>
>>>> I like to call the zynqmp_get_silicon_idcode_name function from my 
>>>> board code and therefore rework the function to return static memory 
>>>> to avoid a memory allocation on each call.
>>>>
>>>> -    char name[ZYNQMP_VERSION_SIZE];
>>>> +    static char name[ZYNQMP_VERSION_SIZE] = "";
>>>
>>> Try to remove = "".
>>
>> The = "" is needed to skip the processing on the second run.
> 
> I looked if this really change anything. Static variable is placed to 
> BSS section which is zeroed by default at start. It means before you 
> jump to this function name array is already zeroed.

Without the empty default value it is placed in an other section.

General it is a bad idea to use memory address zero because if a pointer 
point to this place it is interpreted as NULL pointer.

>>> Undefined variables should be placed to bss section.
>>
>> A = "zu" will also move the variable to an other section.
>>
>>>
>>>>
>>>> +    if (name[0])
>>>> +        return name;
>>>>
>>>> -    return strdup(name);
>>>> +    return name;
> 
> this function is called only once now that's why I personally can't see 
> to do any change there unless you have good reason to call it more times.

I use the function to select the correct configuration (fpga image) from 
the u-boot.itb in the spl.

> But that being said I think that would make sense to move the whole 
> zynqmp_get_silicon_idcode_name() to soc_xilinx_zynqmp.c and do silicon 
> detection there. Only option there is soc_get_machine() which can return 
> silicon type.

That sounds good.

>>>> The name variable gets the address 0 which means that snprintf and 
>>>> strncat do nothing because the dest pointer and NULL are equal.
>>>
>>> Ok. I see. It become the first variable in bss section.
>>
>> Should I resend the patch with null replaced by 0?
> 
> Yes please.

I will resend the patch next week.

Regards
   Stefan