| Message ID | 20230214010942.25143-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MIPS: vpe-mt: provide a default 'physical_memsize' | expand |
Hi Randy, On 14/2/23 02:09, Randy Dunlap wrote: > When neither LANTIQ nor MIPS_MALTA is set, 'physical_memsize' is not > declared. This causes the build to fail with: > > mips-linux-ld: arch/mips/kernel/vpe-mt.o: in function `vpe_run': > arch/mips/kernel/vpe-mt.c:(.text.vpe_run+0x280): undefined reference to `physical_memsize' > > Fix this by declaring a 0-value physical_memsize with neither LANTIQ > nor MIPS_MALTA is set, like LANTIQ does. > > Fixes: 1a2a6d7e8816 ("MIPS: APRP: Split VPE loader into separate files.") > Signed-off-by: Randy Dunlap <rdunlap@infradead.org> > Reported-by: kernel test robot <lkp@intel.com> > Link: https://lore.kernel.org/all/202302030625.2g3E98sY-lkp@intel.com/ > Cc: Dengcheng Zhu <dzhu@wavecomp.com> > Cc: John Crispin <john@phrozen.org> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: linux-mips@vger.kernel.org > --- > How has this build error not been detected for 10 years? > > arch/mips/kernel/vpe-mt.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff -- a/arch/mips/kernel/vpe-mt.c b/arch/mips/kernel/vpe-mt.c > --- a/arch/mips/kernel/vpe-mt.c > +++ b/arch/mips/kernel/vpe-mt.c > @@ -22,6 +22,15 @@ static int major; > /* The number of TCs and VPEs physically available on the core */ > static int hw_tcs, hw_vpes; > > +#if !defined(CONFIG_MIPS_MALTA) && !defined(CONFIG_LANTIQ) > +/* The 2 above provide their own 'physical_memsize' variable. */ Which seems dubious. The variable should be defined once, likely in arch/mips/kernel/vpe-mt.c, since arch/mips/include/asm/vpe.h declares it. I'm surprised CONFIG_MIPS_MALTA always links malta-dtshim.o, but malta-dtshim.o depends on MIPS_VPE_LOADER_MT, and I can't find a CONFIG_MIPS_MALTA -> MIPS_VPE_LOADER_MT Kconfig dep. > +/* > + * This is needed by the vpe-mt loader code, just set it to 0 and assume > + * that the firmware hardcodes this value to something useful. > + */ > +unsigned long physical_memsize = 0L; I agree this is where this variable has be be declared / initialized, but having this dependent on CONFIG_MIPS_MALTA/CONFIG_LANTIQ machines doesn't seem right. > +#endif > + > /* We are prepared so configure and start the VPE... */ > int vpe_run(struct vpe *v) > { Regards, Phil.
Hi, On 2/13/23 23:40, Philippe Mathieu-Daudé wrote: > Hi Randy, > > On 14/2/23 02:09, Randy Dunlap wrote: >> When neither LANTIQ nor MIPS_MALTA is set, 'physical_memsize' is not >> declared. This causes the build to fail with: >> >> mips-linux-ld: arch/mips/kernel/vpe-mt.o: in function `vpe_run': >> arch/mips/kernel/vpe-mt.c:(.text.vpe_run+0x280): undefined reference to `physical_memsize' >> >> Fix this by declaring a 0-value physical_memsize with neither LANTIQ >> nor MIPS_MALTA is set, like LANTIQ does. >> >> Fixes: 1a2a6d7e8816 ("MIPS: APRP: Split VPE loader into separate files.") >> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> >> Reported-by: kernel test robot <lkp@intel.com> >> Link: https://lore.kernel.org/all/202302030625.2g3E98sY-lkp@intel.com/ >> Cc: Dengcheng Zhu <dzhu@wavecomp.com> >> Cc: John Crispin <john@phrozen.org> >> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> >> Cc: linux-mips@vger.kernel.org >> --- >> How has this build error not been detected for 10 years? >> >> arch/mips/kernel/vpe-mt.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff -- a/arch/mips/kernel/vpe-mt.c b/arch/mips/kernel/vpe-mt.c >> --- a/arch/mips/kernel/vpe-mt.c >> +++ b/arch/mips/kernel/vpe-mt.c >> @@ -22,6 +22,15 @@ static int major; >> /* The number of TCs and VPEs physically available on the core */ >> static int hw_tcs, hw_vpes; >> +#if !defined(CONFIG_MIPS_MALTA) && !defined(CONFIG_LANTIQ) >> +/* The 2 above provide their own 'physical_memsize' variable. */ > > Which seems dubious. The variable should be defined once, likely in > arch/mips/kernel/vpe-mt.c, since arch/mips/include/asm/vpe.h declares > it. > > I'm surprised CONFIG_MIPS_MALTA always links malta-dtshim.o, but > malta-dtshim.o depends on MIPS_VPE_LOADER_MT, and I can't find a > CONFIG_MIPS_MALTA -> MIPS_VPE_LOADER_MT Kconfig dep. > >> +/* >> + * This is needed by the vpe-mt loader code, just set it to 0 and assume >> + * that the firmware hardcodes this value to something useful. >> + */ >> +unsigned long physical_memsize = 0L; > > I agree this is where this variable has be be declared / initialized, > but having this dependent on CONFIG_MIPS_MALTA/CONFIG_LANTIQ machines > doesn't seem right. > >> +#endif >> + >> /* We are prepared so configure and start the VPE... */ >> int vpe_run(struct vpe *v) >> { OK, I'll try to consolidate all of these into one location. Thanks.
Hi, On 2/13/23 23:40, Philippe Mathieu-Daudé wrote: > Hi Randy, > > On 14/2/23 02:09, Randy Dunlap wrote: >> When neither LANTIQ nor MIPS_MALTA is set, 'physical_memsize' is not >> declared. This causes the build to fail with: >> >> mips-linux-ld: arch/mips/kernel/vpe-mt.o: in function `vpe_run': >> arch/mips/kernel/vpe-mt.c:(.text.vpe_run+0x280): undefined reference to `physical_memsize' >> >> Fix this by declaring a 0-value physical_memsize with neither LANTIQ >> nor MIPS_MALTA is set, like LANTIQ does. >> >> Fixes: 1a2a6d7e8816 ("MIPS: APRP: Split VPE loader into separate files.") >> Signed-off-by: Randy Dunlap <rdunlap@infradead.org> >> Reported-by: kernel test robot <lkp@intel.com> >> Link: https://lore.kernel.org/all/202302030625.2g3E98sY-lkp@intel.com/ >> Cc: Dengcheng Zhu <dzhu@wavecomp.com> >> Cc: John Crispin <john@phrozen.org> >> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> >> Cc: linux-mips@vger.kernel.org >> --- >> How has this build error not been detected for 10 years? >> >> arch/mips/kernel/vpe-mt.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff -- a/arch/mips/kernel/vpe-mt.c b/arch/mips/kernel/vpe-mt.c >> --- a/arch/mips/kernel/vpe-mt.c >> +++ b/arch/mips/kernel/vpe-mt.c >> @@ -22,6 +22,15 @@ static int major; >> /* The number of TCs and VPEs physically available on the core */ >> static int hw_tcs, hw_vpes; >> +#if !defined(CONFIG_MIPS_MALTA) && !defined(CONFIG_LANTIQ) >> +/* The 2 above provide their own 'physical_memsize' variable. */ > > Which seems dubious. The variable should be defined once, likely in > arch/mips/kernel/vpe-mt.c, since arch/mips/include/asm/vpe.h declares > it. That doesn't work for CONFIG_MIPS_MALTA=y and MIPS_VPE_LOADER is not set. In the current (before a consolidation patch) code, mti-malta/malta-memory.c declares 'physical_memsize' and malta-dtshim.c uses it (thru an 'extern'), so MIPS_VPE_LOADER and MIPS_VPE_LOADER_MT are not required to be enabled. > I'm surprised CONFIG_MIPS_MALTA always links malta-dtshim.o, but > malta-dtshim.o depends on MIPS_VPE_LOADER_MT, and I can't find a > CONFIG_MIPS_MALTA -> MIPS_VPE_LOADER_MT Kconfig dep. Why does malta-dtshim.o depend on MIPS_VPE_LOADER_MT? MIPS_MALTA selects SUPPORTS_VPE_LOADER and SYS_SUPPORTS_MULTITHREADING but does not require MIPS_VPE_LOADER or MIPS_VPE_LOADER_MT. It builds fine with those symbols being enabled (before any patch). >> +/* >> + * This is needed by the vpe-mt loader code, just set it to 0 and assume >> + * that the firmware hardcodes this value to something useful. >> + */ >> +unsigned long physical_memsize = 0L; > > I agree this is where this variable has be be declared / initialized, > but having this dependent on CONFIG_MIPS_MALTA/CONFIG_LANTIQ machines > doesn't seem right. So far I have been able to consolidate the LANTIQ code into a general patch, but not MALTA. >> +#endif >> + >> /* We are prepared so configure and start the VPE... */ >> int vpe_run(struct vpe *v) >> { > > Regards, > > Phil. Thanks.
On Wed, Feb 15, 2023 at 10:59:35PM -0800, Randy Dunlap wrote: > > I agree this is where this variable has be be declared / initialized, > > but having this dependent on CONFIG_MIPS_MALTA/CONFIG_LANTIQ machines > > doesn't seem right. > > So far I have been able to consolidate the LANTIQ code into a general > patch, but not MALTA. if I didn't miss something physical_memory is always 0 for LANTIQ and something for MALTA depending on command line/DT. Now arch/mips/kernel/vpe-mt.c contains /* * The sde-kit passes 'memsize' to __start in $a3, so set something * here... Or set $a3 to zero and define DFLT_STACK_SIZE and * DFLT_HEAP_SIZE when you compile your program */ mttgpr(6, v->ntcs); mttgpr(7, physical_memsize); so the 0 for LANTIQ is fine with the correct VPE payload. But for MALTA could cause major problems, if the VPE payload uses the top of memory for it's stack. So I would guess nobody uses this "mode". Therefore let's get rid of physical_memory in vpe.c completly. Thomas.
On 2/17/23 03:57, Thomas Bogendoerfer wrote: > On Wed, Feb 15, 2023 at 10:59:35PM -0800, Randy Dunlap wrote: >>> I agree this is where this variable has be be declared / initialized, >>> but having this dependent on CONFIG_MIPS_MALTA/CONFIG_LANTIQ machines >>> doesn't seem right. >> >> So far I have been able to consolidate the LANTIQ code into a general >> patch, but not MALTA. > > if I didn't miss something physical_memory is always 0 for LANTIQ > and something for MALTA depending on command line/DT. Now > > arch/mips/kernel/vpe-mt.c contains > > /* > * The sde-kit passes 'memsize' to __start in $a3, so set something > * here... Or set $a3 to zero and define DFLT_STACK_SIZE and > * DFLT_HEAP_SIZE when you compile your program > */ > mttgpr(6, v->ntcs); > mttgpr(7, physical_memsize); > > so the 0 for LANTIQ is fine with the correct VPE payload. But for > MALTA could cause major problems, if the VPE payload uses the top > of memory for it's stack. So I would guess nobody uses this "mode". > Therefore let's get rid of physical_memory in vpe.c completly. Hi Thomas, I had already concluded that MALTA's dtshim and physical_memsize are independent so I should ignore those in any changes/patches. I'll check into your suggestion to see what that looks like. Thanks for the comments.
On 2/17/23 03:57, Thomas Bogendoerfer wrote: > On Wed, Feb 15, 2023 at 10:59:35PM -0800, Randy Dunlap wrote: >>> I agree this is where this variable has be be declared / initialized, >>> but having this dependent on CONFIG_MIPS_MALTA/CONFIG_LANTIQ machines >>> doesn't seem right. >> >> So far I have been able to consolidate the LANTIQ code into a general >> patch, but not MALTA. > > if I didn't miss something physical_memory is always 0 for LANTIQ > and something for MALTA depending on command line/DT. Now > > arch/mips/kernel/vpe-mt.c contains > > /* > * The sde-kit passes 'memsize' to __start in $a3, so set something > * here... Or set $a3 to zero and define DFLT_STACK_SIZE and > * DFLT_HEAP_SIZE when you compile your program > */ > mttgpr(6, v->ntcs); > mttgpr(7, physical_memsize); > > so the 0 for LANTIQ is fine with the correct VPE payload. But for > MALTA could cause major problems, if the VPE payload uses the top > of memory for it's stack. So I would guess nobody uses this "mode". > Therefore let's get rid of physical_memory in vpe.c completly. Hi Thomas, What is this line doing? mttgpr(6, v->ntcs); Does it need to stay? But the comment and mttgpr(7, physical_memsize); can be deleted? thanks.
On Fri, Feb 17, 2023 at 03:24:04PM -0800, Randy Dunlap wrote: > > > On 2/17/23 03:57, Thomas Bogendoerfer wrote: > > On Wed, Feb 15, 2023 at 10:59:35PM -0800, Randy Dunlap wrote: > >>> I agree this is where this variable has be be declared / initialized, > >>> but having this dependent on CONFIG_MIPS_MALTA/CONFIG_LANTIQ machines > >>> doesn't seem right. > >> > >> So far I have been able to consolidate the LANTIQ code into a general > >> patch, but not MALTA. > > > > if I didn't miss something physical_memory is always 0 for LANTIQ > > and something for MALTA depending on command line/DT. Now > > > > arch/mips/kernel/vpe-mt.c contains > > > > /* > > * The sde-kit passes 'memsize' to __start in $a3, so set something > > * here... Or set $a3 to zero and define DFLT_STACK_SIZE and > > * DFLT_HEAP_SIZE when you compile your program > > */ > > mttgpr(6, v->ntcs); > > mttgpr(7, physical_memsize); > > > > so the 0 for LANTIQ is fine with the correct VPE payload. But for > > MALTA could cause major problems, if the VPE payload uses the top > > of memory for it's stack. So I would guess nobody uses this "mode". > > Therefore let's get rid of physical_memory in vpe.c completly. > > Hi Thomas, > > What is this line doing? > mttgpr(6, v->ntcs); from a quick glance over the code, it's probably the number of thread contexts (TCs) the VPE is allowed to use. > Does it need to stay? yes. > But the comment and mttgpr(7, physical_memsize); can be deleted? replace it with /* * we don't pass the memsize here, so VPE programs need to be * compiled with DFLT_STACK_SIZE and DFLT_HEAP_SIZE defined */ mttgpr(7, 0); Thomas.
diff -- a/arch/mips/kernel/vpe-mt.c b/arch/mips/kernel/vpe-mt.c --- a/arch/mips/kernel/vpe-mt.c +++ b/arch/mips/kernel/vpe-mt.c @@ -22,6 +22,15 @@ static int major; /* The number of TCs and VPEs physically available on the core */ static int hw_tcs, hw_vpes; +#if !defined(CONFIG_MIPS_MALTA) && !defined(CONFIG_LANTIQ) +/* The 2 above provide their own 'physical_memsize' variable. */ +/* + * This is needed by the vpe-mt loader code, just set it to 0 and assume + * that the firmware hardcodes this value to something useful. + */ +unsigned long physical_memsize = 0L; +#endif + /* We are prepared so configure and start the VPE... */ int vpe_run(struct vpe *v) {
When neither LANTIQ nor MIPS_MALTA is set, 'physical_memsize' is not declared. This causes the build to fail with: mips-linux-ld: arch/mips/kernel/vpe-mt.o: in function `vpe_run': arch/mips/kernel/vpe-mt.c:(.text.vpe_run+0x280): undefined reference to `physical_memsize' Fix this by declaring a 0-value physical_memsize with neither LANTIQ nor MIPS_MALTA is set, like LANTIQ does. Fixes: 1a2a6d7e8816 ("MIPS: APRP: Split VPE loader into separate files.") Signed-off-by: Randy Dunlap <rdunlap@infradead.org> Reported-by: kernel test robot <lkp@intel.com> Link: https://lore.kernel.org/all/202302030625.2g3E98sY-lkp@intel.com/ Cc: Dengcheng Zhu <dzhu@wavecomp.com> Cc: John Crispin <john@phrozen.org> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: linux-mips@vger.kernel.org --- How has this build error not been detected for 10 years? arch/mips/kernel/vpe-mt.c | 9 +++++++++ 1 file changed, 9 insertions(+)