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

[Google]

On Tue, 10 Jan 2023 20:38:54 +0530
Souradeep Chowdhury <quic_schowdhu@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>> ---
> >>>>    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) <mhiramat@xxxxxxxxxx>