diff mbox series

[V0,1/1] bootconfig: Increase max size of bootconfig from 32 KB to 256 KB for DCC support

Message ID 654357bcbfd3974072a558c494a51edafaa73e1a.1673261071.git.quic_schowdhu@quicinc.com
State New
Headers
Series bootconfig: Increase size and node limit of bootconfig for DCC support |

Commit Message

Souradeep Chowdhury Jan. 9, 2023, 2:31 p.m. UTC 
  Increasing the memory size of bootconfig to be able to handle a max number of
8192 nodes to be fitted in memory size of 256KB.

Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
---
 include/linux/bootconfig.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Bjorn Andersson Jan. 9, 2023, 3:09 p.m. UTC | #1 
On Mon, Jan 09, 2023 at 08:01:05PM +0530, Souradeep Chowdhury wrote:
> Increasing the memory size of bootconfig to be able to handle a max number of
> 8192 nodes to be fitted in memory size of 256KB.
> 

This states what the patch does, but not why.

The description you put in the cover letter does capture the why, but
the cover-letter won't be part of the git history (and if it was,
there's no reason to keep the motivation separate from the change). So
please move the motivation into the commit message.

Also, there's generally no reason to have a cover-letter for a
single patch "series". So please skip the --cover-letter.

Regards,
Bjorn

> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  include/linux/bootconfig.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
> index 1611f9d..64d233b 100644
> --- a/include/linux/bootconfig.h
> +++ b/include/linux/bootconfig.h
> @@ -55,11 +55,11 @@ struct xbc_node {
>  } __attribute__ ((__packed__));
>  
>  #define XBC_KEY		0
> -#define XBC_VALUE	(1 << 15)
> -/* Maximum size of boot config is 32KB - 1 */
> +#define XBC_VALUE	(1 << 18)
> +/* Maximum size of boot config is 256KB - 1 */
>  #define XBC_DATA_MAX	(XBC_VALUE - 1)
>  
> -#define XBC_NODE_MAX	1024
> +#define XBC_NODE_MAX	8192
>  #define XBC_KEYLEN_MAX	256
>  #define XBC_DEPTH_MAX	16
>  
> -- 
> 2.7.4
>
Masami Hiramatsu (Google) Jan. 9, 2023, 3:18 p.m. UTC | #2 
On Mon, 9 Jan 2023 20:01:05 +0530
Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:

> Increasing the memory size of bootconfig to be able to handle a max number of
> 8192 nodes to be fitted in memory size of 256KB.

Sorry, but you missed the 'xbc_node::data' stores the index of the data and
that is uint16_t. So the XBC_DATA_MAX is fixed limitation.

The number of nodes (XBC_NODE_MAX) can be expanded because I just decided it
to keep the pre-compiled array size ~8KB. Maybe expanding it to 64KB just
increase the size of kernel on init memory (and freed after boot).

Could you tell me why you need such a big data for your DCC?

Thank you,

> 
> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> ---
>  include/linux/bootconfig.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
> index 1611f9d..64d233b 100644
> --- a/include/linux/bootconfig.h
> +++ b/include/linux/bootconfig.h
> @@ -55,11 +55,11 @@ struct xbc_node {
>  } __attribute__ ((__packed__));
>  
>  #define XBC_KEY		0
> -#define XBC_VALUE	(1 << 15)
> -/* Maximum size of boot config is 32KB - 1 */
> +#define XBC_VALUE	(1 << 18)
> +/* Maximum size of boot config is 256KB - 1 */
>  #define XBC_DATA_MAX	(XBC_VALUE - 1)
>  
> -#define XBC_NODE_MAX	1024
> +#define XBC_NODE_MAX	8192
>  #define XBC_KEYLEN_MAX	256
>  #define XBC_DEPTH_MAX	16
>  
> -- 
> 2.7.4
>
Souradeep Chowdhury Jan. 10, 2023, 10:57 a.m. UTC | #3 
On 1/9/2023 8:39 PM, Bjorn Andersson wrote:
> On Mon, Jan 09, 2023 at 08:01:05PM +0530, Souradeep Chowdhury wrote:
>> Increasing the memory size of bootconfig to be able to handle a max number of
>> 8192 nodes to be fitted in memory size of 256KB.
>>
> 
> This states what the patch does, but not why.
> 
> The description you put in the cover letter does capture the why, but
> the cover-letter won't be part of the git history (and if it was,
> there's no reason to keep the motivation separate from the change). So
> please move the motivation into the commit message.
> 
> Also, there's generally no reason to have a cover-letter for a
> single patch "series". So please skip the --cover-letter.
> 
> Regards,
> Bjorn

Ack

> 
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   include/linux/bootconfig.h | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
>> index 1611f9d..64d233b 100644
>> --- a/include/linux/bootconfig.h
>> +++ b/include/linux/bootconfig.h
>> @@ -55,11 +55,11 @@ struct xbc_node {
>>   } __attribute__ ((__packed__));
>>   
>>   #define XBC_KEY		0
>> -#define XBC_VALUE	(1 << 15)
>> -/* Maximum size of boot config is 32KB - 1 */
>> +#define XBC_VALUE	(1 << 18)
>> +/* Maximum size of boot config is 256KB - 1 */
>>   #define XBC_DATA_MAX	(XBC_VALUE - 1)
>>   
>> -#define XBC_NODE_MAX	1024
>> +#define XBC_NODE_MAX	8192
>>   #define XBC_KEYLEN_MAX	256
>>   #define XBC_DEPTH_MAX	16
>>   
>> -- 
>> 2.7.4
>>
Souradeep Chowdhury Jan. 10, 2023, 11:56 a.m. UTC | #4 
On 1/9/2023 8:48 PM, Masami Hiramatsu (Google) wrote:
> On Mon, 9 Jan 2023 20:01:05 +0530
> Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
> 
>> Increasing the memory size of bootconfig to be able to handle a max number of
>> 8192 nodes to be fitted in memory size of 256KB.
> 
> Sorry, but you missed the 'xbc_node::data' stores the index of the data and
> that is uint16_t. So the XBC_DATA_MAX is fixed limitation.
> 
> The number of nodes (XBC_NODE_MAX) can be expanded because I just decided it
> to keep the pre-compiled array size ~8KB. Maybe expanding it to 64KB just
> increase the size of kernel on init memory (and freed after boot).
> 
> Could you tell me why you need such a big data for your DCC?
> 
> Thank you,

DCC is a debugging tool used in qcom which is needed to debug crashes
that can happen at boot-time. For debugging purposes a large number of
registers need to be configured in DCC driver which is to be fed via the
bootconfig file. For that we need to expand the nodes as well as memory
for using bootconfig.
Can you let us know the changes that you suggest for doing the same? Is 
it fine to just increase the XBC_NODE_MAX, do we also need to
change the uint16_t to u32 for proper storing of index values?


> 
>>
>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>> ---
>>   include/linux/bootconfig.h | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
>> index 1611f9d..64d233b 100644
>> --- a/include/linux/bootconfig.h
>> +++ b/include/linux/bootconfig.h
>> @@ -55,11 +55,11 @@ struct xbc_node {
>>   } __attribute__ ((__packed__));
>>   
>>   #define XBC_KEY		0
>> -#define XBC_VALUE	(1 << 15)
>> -/* Maximum size of boot config is 32KB - 1 */
>> +#define XBC_VALUE	(1 << 18)
>> +/* Maximum size of boot config is 256KB - 1 */
>>   #define XBC_DATA_MAX	(XBC_VALUE - 1)
>>   
>> -#define XBC_NODE_MAX	1024
>> +#define XBC_NODE_MAX	8192
>>   #define XBC_KEYLEN_MAX	256
>>   #define XBC_DEPTH_MAX	16
>>   
>> -- 
>> 2.7.4
>>
> 
>
Masami Hiramatsu (Google) Jan. 10, 2023, 2:46 p.m. UTC | #5 
On Tue, 10 Jan 2023 17:26:07 +0530
Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:

> 
> 
> On 1/9/2023 8:48 PM, Masami Hiramatsu (Google) wrote:
> > On Mon, 9 Jan 2023 20:01:05 +0530
> > Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
> > 
> >> Increasing the memory size of bootconfig to be able to handle a max number of
> >> 8192 nodes to be fitted in memory size of 256KB.
> > 
> > Sorry, but you missed the 'xbc_node::data' stores the index of the data and
> > that is uint16_t. So the XBC_DATA_MAX is fixed limitation.
> > 
> > The number of nodes (XBC_NODE_MAX) can be expanded because I just decided it
> > to keep the pre-compiled array size ~8KB. Maybe expanding it to 64KB just
> > increase the size of kernel on init memory (and freed after boot).
> > 
> > Could you tell me why you need such a big data for your DCC?
> > 
> > Thank you,
> 
> DCC is a debugging tool used in qcom which is needed to debug crashes
> that can happen at boot-time. For debugging purposes a large number of
> registers need to be configured in DCC driver which is to be fed via the
> bootconfig file. For that we need to expand the nodes as well as memory
> for using bootconfig.

Hmm, how many registers does DCC usually use? And how big the bootconfig
file is usually? I have no idea about that.

> Can you let us know the changes that you suggest for doing the same? Is 
> it fine to just increase the XBC_NODE_MAX, do we also need to
> change the uint16_t to u32 for proper storing of index values?

Expanding the number of max nodes is easy, just increase the XBC_NODE_MAX
(must be less than 64k). That will also increase the memory consumption
during the boot time even if the bootconfig is small. Anyway, it will be
freed after boot, so it maybe OK.

But expanding the size of max bootconfig needs to change the type of
the 'data' field to uint32_t (since that will be used for building 
bootconfig tool) and you also must confirm that `tools/bootconfig/bootconfig`
can be built and pass the test-bootconfig.sh.
Hmm, comparing with expanding the max number of XBC node, changing the
'data' type to uint32_t may not have much impact on memory consumption point
of view, because it may increase only 20% of memory, but expanding the
MAX_XBC_NODE always increases more than double.

Thus, if we can accept increasing the number of node, it should be OK to
change the 'data' type.

BTW, I think now we don't need the ' __attribute__ ((__packed__))' for
struct xbc_node. It was packed for reducing the size of array and able to
pass 'compiled' bootconfig, but now it is just passed as a text data for
safety.

Thank you,

> 
> 
> > 
> >>
> >> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> >> ---
> >>   include/linux/bootconfig.h | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
> >> index 1611f9d..64d233b 100644
> >> --- a/include/linux/bootconfig.h
> >> +++ b/include/linux/bootconfig.h
> >> @@ -55,11 +55,11 @@ struct xbc_node {
> >>   } __attribute__ ((__packed__));
> >>   
> >>   #define XBC_KEY		0
> >> -#define XBC_VALUE	(1 << 15)
> >> -/* Maximum size of boot config is 32KB - 1 */
> >> +#define XBC_VALUE	(1 << 18)
> >> +/* Maximum size of boot config is 256KB - 1 */
> >>   #define XBC_DATA_MAX	(XBC_VALUE - 1)
> >>   
> >> -#define XBC_NODE_MAX	1024
> >> +#define XBC_NODE_MAX	8192
> >>   #define XBC_KEYLEN_MAX	256
> >>   #define XBC_DEPTH_MAX	16
> >>   
> >> -- 
> >> 2.7.4
> >>
> > 
> >
Souradeep Chowdhury Jan. 10, 2023, 3:08 p.m. UTC | #6 
On 1/10/2023 8:16 PM, Masami Hiramatsu (Google) wrote:
> On Tue, 10 Jan 2023 17:26:07 +0530
> Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
> 
>>
>>
>> On 1/9/2023 8:48 PM, Masami Hiramatsu (Google) wrote:
>>> On Mon, 9 Jan 2023 20:01:05 +0530
>>> Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
>>>
>>>> Increasing the memory size of bootconfig to be able to handle a max number of
>>>> 8192 nodes to be fitted in memory size of 256KB.
>>>
>>> Sorry, but you missed the 'xbc_node::data' stores the index of the data and
>>> that is uint16_t. So the XBC_DATA_MAX is fixed limitation.
>>>
>>> The number of nodes (XBC_NODE_MAX) can be expanded because I just decided it
>>> to keep the pre-compiled array size ~8KB. Maybe expanding it to 64KB just
>>> increase the size of kernel on init memory (and freed after boot).
>>>
>>> Could you tell me why you need such a big data for your DCC?
>>>
>>> Thank you,
>>
>> DCC is a debugging tool used in qcom which is needed to debug crashes
>> that can happen at boot-time. For debugging purposes a large number of
>> registers need to be configured in DCC driver which is to be fed via the
>> bootconfig file. For that we need to expand the nodes as well as memory
>> for using bootconfig.
> 
> Hmm, how many registers does DCC usually use? And how big the bootconfig
> file is usually? I have no idea about that.

So a typical bootconfig file for consumption of DCC looks like as follows

dcc_config {
         link_list_0 {
                 qcom-curr-link-list = 6
                 qcom-link-list = R_0x1781005c_1_apb,
                                  R_0x1782005c_1_apb
         }
         link_list_1 {
                 qcom-curr-link-list = 5
                 qcom-link-list = R_0x1784005c_1_apb
         }
}

The "qcom-link-list" field can have 1000s of register , based on that 
max nodes is increased to 8192.

> 
>> Can you let us know the changes that you suggest for doing the same? Is
>> it fine to just increase the XBC_NODE_MAX, do we also need to
>> change the uint16_t to u32 for proper storing of index values?
> 
> Expanding the number of max nodes is easy, just increase the XBC_NODE_MAX
> (must be less than 64k). That will also increase the memory consumption
> during the boot time even if the bootconfig is small. Anyway, it will be
> freed after boot, so it maybe OK.

So since the limit is 64K, 8192 is a valid value for max nodes.

> 
> But expanding the size of max bootconfig needs to change the type of
> the 'data' field to uint32_t (since that will be used for building
> bootconfig tool) and you also must confirm that `tools/bootconfig/bootconfig`
> can be built and pass the test-bootconfig.sh.
> Hmm, comparing with expanding the max number of XBC node, changing the
> 'data' type to uint32_t may not have much impact on memory consumption point
> of view, because it may increase only 20% of memory, but expanding the
> MAX_XBC_NODE always increases more than double.
> 
> Thus, if we can accept increasing the number of node, it should be OK to
> change the 'data' type.

That means from DCC point of view only increasing the max nodes is 
enough as increasing the data size is unrelated to increasing the max nodes?

> 
> BTW, I think now we don't need the ' __attribute__ ((__packed__))' for
> struct xbc_node. It was packed for reducing the size of array and able to
> pass 'compiled' bootconfig, but now it is just passed as a text data for
> safety.

> 
> Thank you,
> 
>>
>>
>>>
>>>>
>>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>>>> ---
>>>>    include/linux/bootconfig.h | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
>>>> index 1611f9d..64d233b 100644
>>>> --- a/include/linux/bootconfig.h
>>>> +++ b/include/linux/bootconfig.h
>>>> @@ -55,11 +55,11 @@ struct xbc_node {
>>>>    } __attribute__ ((__packed__));
>>>>    
>>>>    #define XBC_KEY		0
>>>> -#define XBC_VALUE	(1 << 15)
>>>> -/* Maximum size of boot config is 32KB - 1 */
>>>> +#define XBC_VALUE	(1 << 18)
>>>> +/* Maximum size of boot config is 256KB - 1 */
>>>>    #define XBC_DATA_MAX	(XBC_VALUE - 1)
>>>>    
>>>> -#define XBC_NODE_MAX	1024
>>>> +#define XBC_NODE_MAX	8192
>>>>    #define XBC_KEYLEN_MAX	256
>>>>    #define XBC_DEPTH_MAX	16
>>>>    
>>>> -- 
>>>> 2.7.4
>>>>
>>>
>>>
> 
>
Masami Hiramatsu (Google) Jan. 12, 2023, 7:01 a.m. UTC | #7 
On Tue, 10 Jan 2023 20:38:54 +0530
Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:

> 
> 
> On 1/10/2023 8:16 PM, Masami Hiramatsu (Google) wrote:
> > On Tue, 10 Jan 2023 17:26:07 +0530
> > Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
> > 
> >>
> >>
> >> On 1/9/2023 8:48 PM, Masami Hiramatsu (Google) wrote:
> >>> On Mon, 9 Jan 2023 20:01:05 +0530
> >>> Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
> >>>
> >>>> Increasing the memory size of bootconfig to be able to handle a max number of
> >>>> 8192 nodes to be fitted in memory size of 256KB.
> >>>
> >>> Sorry, but you missed the 'xbc_node::data' stores the index of the data and
> >>> that is uint16_t. So the XBC_DATA_MAX is fixed limitation.
> >>>
> >>> The number of nodes (XBC_NODE_MAX) can be expanded because I just decided it
> >>> to keep the pre-compiled array size ~8KB. Maybe expanding it to 64KB just
> >>> increase the size of kernel on init memory (and freed after boot).
> >>>
> >>> Could you tell me why you need such a big data for your DCC?
> >>>
> >>> Thank you,
> >>
> >> DCC is a debugging tool used in qcom which is needed to debug crashes
> >> that can happen at boot-time. For debugging purposes a large number of
> >> registers need to be configured in DCC driver which is to be fed via the
> >> bootconfig file. For that we need to expand the nodes as well as memory
> >> for using bootconfig.
> > 
> > Hmm, how many registers does DCC usually use? And how big the bootconfig
> > file is usually? I have no idea about that.
> 
> So a typical bootconfig file for consumption of DCC looks like as follows
> 
> dcc_config {
>          link_list_0 {
>                  qcom-curr-link-list = 6
>                  qcom-link-list = R_0x1781005c_1_apb,
>                                   R_0x1782005c_1_apb
>          }
>          link_list_1 {
>                  qcom-curr-link-list = 5
>                  qcom-link-list = R_0x1784005c_1_apb
>          }
> }
> 
> The "qcom-link-list" field can have 1000s of register , based on that 
> max nodes is increased to 8192.

OK, then the number of fields can be larger than 1000. I got it.

> 
> > 
> >> Can you let us know the changes that you suggest for doing the same? Is
> >> it fine to just increase the XBC_NODE_MAX, do we also need to
> >> change the uint16_t to u32 for proper storing of index values?
> > 
> > Expanding the number of max nodes is easy, just increase the XBC_NODE_MAX
> > (must be less than 64k). That will also increase the memory consumption
> > during the boot time even if the bootconfig is small. Anyway, it will be
> > freed after boot, so it maybe OK.
> 
> So since the limit is 64K, 8192 is a valid value for max nodes.

Yes. Expanding the number of node is OK to me.

> 
> > 
> > But expanding the size of max bootconfig needs to change the type of
> > the 'data' field to uint32_t (since that will be used for building
> > bootconfig tool) and you also must confirm that `tools/bootconfig/bootconfig`
> > can be built and pass the test-bootconfig.sh.
> > Hmm, comparing with expanding the max number of XBC node, changing the
> > 'data' type to uint32_t may not have much impact on memory consumption point
> > of view, because it may increase only 20% of memory, but expanding the
> > MAX_XBC_NODE always increases more than double.
> > 
> > Thus, if we can accept increasing the number of node, it should be OK to
> > change the 'data' type.
> 
> That means from DCC point of view only increasing the max nodes is 
> enough as increasing the data size is unrelated to increasing the max nodes?

Yes, if it is less than 32KB, you just need to increase the XBC_NODE_MAX.
But if you think the size of bootconfig, we have to change the type of
xbc_node::data field.

Can you check the DCC also need to expand the size of bootconfig limitation?

Thank you!

> 
> > 
> > BTW, I think now we don't need the ' __attribute__ ((__packed__))' for
> > struct xbc_node. It was packed for reducing the size of array and able to
> > pass 'compiled' bootconfig, but now it is just passed as a text data for
> > safety.
> 
> > 
> > Thank you,
> > 
> >>
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
> >>>> ---
> >>>>    include/linux/bootconfig.h | 6 +++---
> >>>>    1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
> >>>> index 1611f9d..64d233b 100644
> >>>> --- a/include/linux/bootconfig.h
> >>>> +++ b/include/linux/bootconfig.h
> >>>> @@ -55,11 +55,11 @@ struct xbc_node {
> >>>>    } __attribute__ ((__packed__));
> >>>>    
> >>>>    #define XBC_KEY		0
> >>>> -#define XBC_VALUE	(1 << 15)
> >>>> -/* Maximum size of boot config is 32KB - 1 */
> >>>> +#define XBC_VALUE	(1 << 18)
> >>>> +/* Maximum size of boot config is 256KB - 1 */
> >>>>    #define XBC_DATA_MAX	(XBC_VALUE - 1)
> >>>>    
> >>>> -#define XBC_NODE_MAX	1024
> >>>> +#define XBC_NODE_MAX	8192
> >>>>    #define XBC_KEYLEN_MAX	256
> >>>>    #define XBC_DEPTH_MAX	16
> >>>>    
> >>>> -- 
> >>>> 2.7.4
> >>>>
> >>>
> >>>
> > 
> >
Souradeep Chowdhury Jan. 19, 2023, 4:51 p.m. UTC | #8 
On 1/12/2023 12:31 PM, Masami Hiramatsu (Google) wrote:
> On Tue, 10 Jan 2023 20:38:54 +0530
> Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
> 
>>
>>
>> On 1/10/2023 8:16 PM, Masami Hiramatsu (Google) wrote:
>>> On Tue, 10 Jan 2023 17:26:07 +0530
>>> Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
>>>
>>>>
>>>>
>>>> On 1/9/2023 8:48 PM, Masami Hiramatsu (Google) wrote:
>>>>> On Mon, 9 Jan 2023 20:01:05 +0530
>>>>> Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
>>>>>
>>>>>> Increasing the memory size of bootconfig to be able to handle a max number of
>>>>>> 8192 nodes to be fitted in memory size of 256KB.
>>>>>
>>>>> Sorry, but you missed the 'xbc_node::data' stores the index of the data and
>>>>> that is uint16_t. So the XBC_DATA_MAX is fixed limitation.
>>>>>
>>>>> The number of nodes (XBC_NODE_MAX) can be expanded because I just decided it
>>>>> to keep the pre-compiled array size ~8KB. Maybe expanding it to 64KB just
>>>>> increase the size of kernel on init memory (and freed after boot).
>>>>>
>>>>> Could you tell me why you need such a big data for your DCC?
>>>>>
>>>>> Thank you,
>>>>
>>>> DCC is a debugging tool used in qcom which is needed to debug crashes
>>>> that can happen at boot-time. For debugging purposes a large number of
>>>> registers need to be configured in DCC driver which is to be fed via the
>>>> bootconfig file. For that we need to expand the nodes as well as memory
>>>> for using bootconfig.
>>>
>>> Hmm, how many registers does DCC usually use? And how big the bootconfig
>>> file is usually? I have no idea about that.
>>
>> So a typical bootconfig file for consumption of DCC looks like as follows
>>
>> dcc_config {
>>           link_list_0 {
>>                   qcom-curr-link-list = 6
>>                   qcom-link-list = R_0x1781005c_1_apb,
>>                                    R_0x1782005c_1_apb
>>           }
>>           link_list_1 {
>>                   qcom-curr-link-list = 5
>>                   qcom-link-list = R_0x1784005c_1_apb
>>           }
>> }
>>
>> The "qcom-link-list" field can have 1000s of register , based on that
>> max nodes is increased to 8192.
> 
> OK, then the number of fields can be larger than 1000. I got it.
> 
>>
>>>
>>>> Can you let us know the changes that you suggest for doing the same? Is
>>>> it fine to just increase the XBC_NODE_MAX, do we also need to
>>>> change the uint16_t to u32 for proper storing of index values?
>>>
>>> Expanding the number of max nodes is easy, just increase the XBC_NODE_MAX
>>> (must be less than 64k). That will also increase the memory consumption
>>> during the boot time even if the bootconfig is small. Anyway, it will be
>>> freed after boot, so it maybe OK.
>>
>> So since the limit is 64K, 8192 is a valid value for max nodes.
> 
> Yes. Expanding the number of node is OK to me.
> 
>>
>>>
>>> But expanding the size of max bootconfig needs to change the type of
>>> the 'data' field to uint32_t (since that will be used for building
>>> bootconfig tool) and you also must confirm that `tools/bootconfig/bootconfig`
>>> can be built and pass the test-bootconfig.sh.
>>> Hmm, comparing with expanding the max number of XBC node, changing the
>>> 'data' type to uint32_t may not have much impact on memory consumption point
>>> of view, because it may increase only 20% of memory, but expanding the
>>> MAX_XBC_NODE always increases more than double.
>>>
>>> Thus, if we can accept increasing the number of node, it should be OK to
>>> change the 'data' type.
>>
>> That means from DCC point of view only increasing the max nodes is
>> enough as increasing the data size is unrelated to increasing the max nodes?
> 
> Yes, if it is less than 32KB, you just need to increase the XBC_NODE_MAX.
> But if you think the size of bootconfig, we have to change the type of
> xbc_node::data field.
> 
> Can you check the DCC also need to expand the size of bootconfig limitation?
> 
> Thank you!

Yes, I don't think the index needs to be increased from u16 to u32 for 
dcc. Will be sending out the next version accordingly.

Thanks
> 
>>
>>>
>>> BTW, I think now we don't need the ' __attribute__ ((__packed__))' for
>>> struct xbc_node. It was packed for reducing the size of array and able to
>>> pass 'compiled' bootconfig, but now it is just passed as a text data for
>>> safety.
>>
>>>
>>> Thank you,
>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Souradeep Chowdhury <quic_schowdhu@quicinc.com>
>>>>>> ---
>>>>>>     include/linux/bootconfig.h | 6 +++---
>>>>>>     1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
>>>>>> index 1611f9d..64d233b 100644
>>>>>> --- a/include/linux/bootconfig.h
>>>>>> +++ b/include/linux/bootconfig.h
>>>>>> @@ -55,11 +55,11 @@ struct xbc_node {
>>>>>>     } __attribute__ ((__packed__));
>>>>>>     
>>>>>>     #define XBC_KEY		0
>>>>>> -#define XBC_VALUE	(1 << 15)
>>>>>> -/* Maximum size of boot config is 32KB - 1 */
>>>>>> +#define XBC_VALUE	(1 << 18)
>>>>>> +/* Maximum size of boot config is 256KB - 1 */
>>>>>>     #define XBC_DATA_MAX	(XBC_VALUE - 1)
>>>>>>     
>>>>>> -#define XBC_NODE_MAX	1024
>>>>>> +#define XBC_NODE_MAX	8192
>>>>>>     #define XBC_KEYLEN_MAX	256
>>>>>>     #define XBC_DEPTH_MAX	16
>>>>>>     
>>>>>> -- 
>>>>>> 2.7.4
>>>>>>
>>>>>
>>>>>
>>>
>>>
> 
>
Masami Hiramatsu (Google) Jan. 19, 2023, 11:37 p.m. UTC | #9 
On Thu, 19 Jan 2023 22:21:27 +0530
Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:

> 
> 
> On 1/12/2023 12:31 PM, Masami Hiramatsu (Google) wrote:
> > On Tue, 10 Jan 2023 20:38:54 +0530
> > Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
> > 
> >>
> >>
> >> On 1/10/2023 8:16 PM, Masami Hiramatsu (Google) wrote:
> >>> On Tue, 10 Jan 2023 17:26:07 +0530
> >>> Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
> >>>
> >>>>
> >>>>
> >>>> On 1/9/2023 8:48 PM, Masami Hiramatsu (Google) wrote:
> >>>>> On Mon, 9 Jan 2023 20:01:05 +0530
> >>>>> Souradeep Chowdhury <quic_schowdhu@quicinc.com> wrote:
> >>>>>
> >>>>>> Increasing the memory size of bootconfig to be able to handle a max number of
> >>>>>> 8192 nodes to be fitted in memory size of 256KB.
> >>>>>
> >>>>> Sorry, but you missed the 'xbc_node::data' stores the index of the data and
> >>>>> that is uint16_t. So the XBC_DATA_MAX is fixed limitation.
> >>>>>
> >>>>> The number of nodes (XBC_NODE_MAX) can be expanded because I just decided it
> >>>>> to keep the pre-compiled array size ~8KB. Maybe expanding it to 64KB just
> >>>>> increase the size of kernel on init memory (and freed after boot).
> >>>>>
> >>>>> Could you tell me why you need such a big data for your DCC?
> >>>>>
> >>>>> Thank you,
> >>>>
> >>>> DCC is a debugging tool used in qcom which is needed to debug crashes
> >>>> that can happen at boot-time. For debugging purposes a large number of
> >>>> registers need to be configured in DCC driver which is to be fed via the
> >>>> bootconfig file. For that we need to expand the nodes as well as memory
> >>>> for using bootconfig.
> >>>
> >>> Hmm, how many registers does DCC usually use? And how big the bootconfig
> >>> file is usually? I have no idea about that.
> >>
> >> So a typical bootconfig file for consumption of DCC looks like as follows
> >>
> >> dcc_config {
> >>           link_list_0 {
> >>                   qcom-curr-link-list = 6
> >>                   qcom-link-list = R_0x1781005c_1_apb,
> >>                                    R_0x1782005c_1_apb
> >>           }
> >>           link_list_1 {
> >>                   qcom-curr-link-list = 5
> >>                   qcom-link-list = R_0x1784005c_1_apb
> >>           }
> >> }
> >>
> >> The "qcom-link-list" field can have 1000s of register , based on that
> >> max nodes is increased to 8192.
> > 
> > OK, then the number of fields can be larger than 1000. I got it.
> > 
> >>
> >>>
> >>>> Can you let us know the changes that you suggest for doing the same? Is
> >>>> it fine to just increase the XBC_NODE_MAX, do we also need to
> >>>> change the uint16_t to u32 for proper storing of index values?
> >>>
> >>> Expanding the number of max nodes is easy, just increase the XBC_NODE_MAX
> >>> (must be less than 64k). That will also increase the memory consumption
> >>> during the boot time even if the bootconfig is small. Anyway, it will be
> >>> freed after boot, so it maybe OK.
> >>
> >> So since the limit is 64K, 8192 is a valid value for max nodes.
> > 
> > Yes. Expanding the number of node is OK to me.
> > 
> >>
> >>>
> >>> But expanding the size of max bootconfig needs to change the type of
> >>> the 'data' field to uint32_t (since that will be used for building
> >>> bootconfig tool) and you also must confirm that `tools/bootconfig/bootconfig`
> >>> can be built and pass the test-bootconfig.sh.
> >>> Hmm, comparing with expanding the max number of XBC node, changing the
> >>> 'data' type to uint32_t may not have much impact on memory consumption point
> >>> of view, because it may increase only 20% of memory, but expanding the
> >>> MAX_XBC_NODE always increases more than double.
> >>>
> >>> Thus, if we can accept increasing the number of node, it should be OK to
> >>> change the 'data' type.
> >>
> >> That means from DCC point of view only increasing the max nodes is
> >> enough as increasing the data size is unrelated to increasing the max nodes?
> > 
> > Yes, if it is less than 32KB, you just need to increase the XBC_NODE_MAX.
> > But if you think the size of bootconfig, we have to change the type of
> > xbc_node::data field.
> > 
> > Can you check the DCC also need to expand the size of bootconfig limitation?
> > 
> > Thank you!
> 
> Yes, I don't think the index needs to be increased from u16 to u32 for 
> dcc. Will be sending out the next version accordingly.

OK, thanks for the confirmation!
diff mbox series

Patch

diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h
index 1611f9d..64d233b 100644
--- a/include/linux/bootconfig.h
+++ b/include/linux/bootconfig.h
@@ -55,11 +55,11 @@  struct xbc_node {
 } __attribute__ ((__packed__));
 
 #define XBC_KEY		0
-#define XBC_VALUE	(1 << 15)
-/* Maximum size of boot config is 32KB - 1 */
+#define XBC_VALUE	(1 << 18)
+/* Maximum size of boot config is 256KB - 1 */
 #define XBC_DATA_MAX	(XBC_VALUE - 1)
 
-#define XBC_NODE_MAX	1024
+#define XBC_NODE_MAX	8192
 #define XBC_KEYLEN_MAX	256
 #define XBC_DEPTH_MAX	16