linuxsh-dev

Re: [PATCH 4/10] Support for kernel+fs images

[Paul M. <le...@li...>]

On Thu, Jul 26, 2007 at 03:27:13PM +1000, David McCullough wrote:
> Add support for copying our filesystem out of the way when images are
> constructed by concatenating the root filesystem to the end of a linux.bin
> which has the BSS removed (to save space).
> 
That's an interesting abuse :-)

> --- a/arch/sh/kernel/setup.c
> +++ b/arch/sh/kernel/setup.c
> @@ -399,3 +399,66 @@ struct seq_operations cpuinfo_op = {
>  	.show	= show_cpuinfo,
>  };
>  #endif /* CONFIG_PROC_FS */
> +
> +#if defined(CONFIG_SH_CONCAT_FS)
> +/*
> + * do not call printk in here,  bad things will happen,  the kernel isn't
> + * actually up yet,  we are called from head.S before BSS is cleared.
> + */
> +
> +void __init __attribute__ ((weak))
> +arch_service_watchdog(void)
> +{
> +}
> +

I'm not sure if I like this watchdog thing..

> +extern void copy_romfs(void);
> +void __init copy_romfs()
> +{
> +	unsigned char	*sp, *dp;
> +	unsigned long	 len;
> +
> +	/*
> +	 * we used to use __bss_start,  but that is padded to 4K now and doesn't
> +	 * work, basically we need the last non-padded symbol before __bss_start
> +	 * and that is now __machvec_end
> +	 */
> +#define CURRENT_ROMFS_LOC	((unsigned long)(&__machvec_end))
> +
Please just add a non-padded symbol (ie, __davidms_strange_romfs_thing)
for this instead. Otherwise someone, most likely me, will forget that
there's some special bit of code that reads far too much in to what
__machvec_end really means.

> +#ifdef CONFIG_SH_ROMBOOT
> +	extern int _mem_start, _rom_store;
> +	sp = (unsigned char *) CURRENT_ROMFS_LOC - ((_mem_start - _rom_store) / 4);
> +#else
> +	sp = (unsigned char *) CURRENT_ROMFS_LOC;
> +#endif

CONFIG_SH_ROMBOOT is not added by any of your patches, likewise for
_mem_start and _rom_store. Presumably this should be sent as a follow up
patch if you want this functionality included.

> +	/* copy backwards to avoid writing over ourselves */
> +	arch_service_watchdog();
> +	while (dp >= ((unsigned char *) (&_end[0]))) {
> +		*dp-- = *sp--;
> +		if ((((unsigned long) dp) & 0x7ffff) == 0)
> +			arch_service_watchdog();
> +	}
> +	arch_service_watchdog();
> +}

Can you not simply disable the watchdog in your loader and start it up
from the board code? Having to do dummy reads from the thing in this
convoluted manner is rather undesirable.

Re: [PATCH 4/10] Support for kernel+fs images

[David M. <Dav...@se...>]

Jivin Paul Mundt lays it down ...
> On Thu, Jul 26, 2007 at 03:27:13PM +1000, David McCullough wrote:
> > Add support for copying our filesystem out of the way when images are
> > constructed by concatenating the root filesystem to the end of a linux.bin
> > which has the BSS removed (to save space).
>
> That's an interesting abuse :-)

It's a very old thing,  our images from the beginning of time have been
like this.  If you don't like it,  no problems, you can get around it by making
images with the BSS included in the linux.bin.

> > --- a/arch/sh/kernel/setup.c
> > +++ b/arch/sh/kernel/setup.c
> > @@ -399,3 +399,66 @@ struct seq_operations cpuinfo_op = {
> >  	.show	= show_cpuinfo,
> >  };
> >  #endif /* CONFIG_PROC_FS */
> > +
> > +#if defined(CONFIG_SH_CONCAT_FS)
> > +/*
> > + * do not call printk in here,  bad things will happen,  the kernel isn't
> > + * actually up yet,  we are called from head.S before BSS is cleared.
> > + */
> > +
> > +void __init __attribute__ ((weak))
> > +arch_service_watchdog(void)
> > +{
> > +}
> > +
> 
> I'm not sure if I like this watchdog thing..

What was there before was all ifdef'd,  I figured this might be more
acceptable,  I can go back to ifdef's if you like.

> > +extern void copy_romfs(void);
> > +void __init copy_romfs()
> > +{
> > +	unsigned char	*sp, *dp;
> > +	unsigned long	 len;
> > +
> > +	/*
> > +	 * we used to use __bss_start,  but that is padded to 4K now and doesn't
> > +	 * work, basically we need the last non-padded symbol before __bss_start
> > +	 * and that is now __machvec_end
> > +	 */
> > +#define CURRENT_ROMFS_LOC	((unsigned long)(&__machvec_end))
> > +
>
> Please just add a non-padded symbol (ie, __davidms_strange_romfs_thing)
> for this instead. Otherwise someone, most likely me, will forget that
> there's some special bit of code that reads far too much in to what
> __machvec_end really means.

ok.

> > +#ifdef CONFIG_SH_ROMBOOT
> > +	extern int _mem_start, _rom_store;
> > +	sp = (unsigned char *) CURRENT_ROMFS_LOC - ((_mem_start - _rom_store) / 4);
> > +#else
> > +	sp = (unsigned char *) CURRENT_ROMFS_LOC;
> > +#endif
> 
> CONFIG_SH_ROMBOOT is not added by any of your patches, likewise for
> _mem_start and _rom_store. Presumably this should be sent as a follow up
> patch if you want this functionality included.

My BAD,  the CONFIG_SH_ROMBOOT was contributed by someone else and since
we don't use it,  I didn't need all the other bits to make it go.

I'll fix this if you think the "strange romfs thing" is acceptable in
some shape or form.

> > +	/* copy backwards to avoid writing over ourselves */
> > +	arch_service_watchdog();
> > +	while (dp >= ((unsigned char *) (&_end[0]))) {
> > +		*dp-- = *sp--;
> > +		if ((((unsigned long) dp) & 0x7ffff) == 0)
> > +			arch_service_watchdog();
> > +	}
> > +	arch_service_watchdog();
> > +}
> 
> Can you not simply disable the watchdog in your loader and start it up
> from the board code? Having to do dummy reads from the thing in this
> convoluted manner is rather undesirable.

The watchdog is on from poweron, it cannot be disabled in any possible
way.  You have at most 1 second (or less on some versions) to service it or
it resets the hardware.

Again, if it's no good we'll just carry the needed changes in our tree.

Cheers,
Davidm

-- 
David McCullough,  dav...@se...,   Ph:+61 734352815
Secure Computing - SnapGear  http://www.uCdot.org http://www.cyberguard.com

Re: [PATCH 4/10] Support for kernel+fs images

[Paul M. <le...@li...>]

On Thu, Jul 26, 2007 at 04:21:01PM +1000, David McCullough wrote:
> Jivin Paul Mundt lays it down ...
> > On Thu, Jul 26, 2007 at 03:27:13PM +1000, David McCullough wrote:
> > > Add support for copying our filesystem out of the way when images are
> > > constructed by concatenating the root filesystem to the end of a linux.bin
> > > which has the BSS removed (to save space).
> >
> > That's an interesting abuse :-)
> 
> It's a very old thing,  our images from the beginning of time have been
> like this.  If you don't like it,  no problems, you can get around it
> by making images with the BSS included in the linux.bin.
> 
Besides the watchdog cruft, I don't particularly have a problem with it.
I'll certainly accept patches for the functionality, although it will be
for 2.6.24.

> > CONFIG_SH_ROMBOOT is not added by any of your patches, likewise for
> > _mem_start and _rom_store. Presumably this should be sent as a follow up
> > patch if you want this functionality included.
> 
> My BAD,  the CONFIG_SH_ROMBOOT was contributed by someone else and since
> we don't use it,  I didn't need all the other bits to make it go.
> 
> I'll fix this if you think the "strange romfs thing" is acceptable in
> some shape or form.
> 
If there are sufficient users of this feature, then I don't have a
problem with merging it. If you aren't using romboot at all, then just
drop that from the update.

> > > +	/* copy backwards to avoid writing over ourselves */
> > > +	arch_service_watchdog();
> > > +	while (dp >= ((unsigned char *) (&_end[0]))) {
> > > +		*dp-- = *sp--;
> > > +		if ((((unsigned long) dp) & 0x7ffff) == 0)
> > > +			arch_service_watchdog();
> > > +	}
> > > +	arch_service_watchdog();
> > > +}
> > 
> > Can you not simply disable the watchdog in your loader and start it up
> > from the board code? Having to do dummy reads from the thing in this
> > convoluted manner is rather undesirable.
> 
> The watchdog is on from poweron, it cannot be disabled in any possible
> way.  You have at most 1 second (or less on some versions) to service it or
> it resets the hardware.
> 
Then perhaps we should just be abstracting out the romfs copy. ie, have
the generic copy_romfs() (possibly renamed to something more reasonable)
take care of extracting all of the various image details, and then kick
the addresses down to a platform-specific symbol to do the actual copy.
This too can be a weak symbol, where the default implementation simply
does the copy without the watchdog noise.

This way you can keep all of your platform-specific cruft isolated
without making a compromise on functionality.

Re: [PATCH 4/10] Support for kernel+fs images

[David M. <Dav...@se...>]

Jivin Paul Mundt lays it down ...
> On Thu, Jul 26, 2007 at 04:21:01PM +1000, David McCullough wrote:
> > Jivin Paul Mundt lays it down ...
> > > On Thu, Jul 26, 2007 at 03:27:13PM +1000, David McCullough wrote:
> > > > Add support for copying our filesystem out of the way when images are
> > > > constructed by concatenating the root filesystem to the end of a linux.bin
> > > > which has the BSS removed (to save space).
> > >
> > > That's an interesting abuse :-)
> > 
> > It's a very old thing,  our images from the beginning of time have been
> > like this.  If you don't like it,  no problems, you can get around it
> > by making images with the BSS included in the linux.bin.
>
> Besides the watchdog cruft, I don't particularly have a problem with it.
> I'll certainly accept patches for the functionality, although it will be
> for 2.6.24.

No problems,  I'll go through and clean as much as this as I can and
keep dropping bits on you.  Is the format ok ?  Anything perferred ?

> > > CONFIG_SH_ROMBOOT is not added by any of your patches, likewise for
> > > _mem_start and _rom_store. Presumably this should be sent as a follow up
> > > patch if you want this functionality included.
> > 
> > My BAD,  the CONFIG_SH_ROMBOOT was contributed by someone else and since
> > we don't use it,  I didn't need all the other bits to make it go.
> > 
> > I'll fix this if you think the "strange romfs thing" is acceptable in
> > some shape or form.
>
> If there are sufficient users of this feature, then I don't have a
> problem with merging it. If you aren't using romboot at all, then just
> drop that from the update.

I have the code needed to make it work,  but we haven't used it at all.
I can contact the guy who sent it in and see if they would like to push
it in themselves.

> > > > +	/* copy backwards to avoid writing over ourselves */
> > > > +	arch_service_watchdog();
> > > > +	while (dp >= ((unsigned char *) (&_end[0]))) {
> > > > +		*dp-- = *sp--;
> > > > +		if ((((unsigned long) dp) & 0x7ffff) == 0)
> > > > +			arch_service_watchdog();
> > > > +	}
> > > > +	arch_service_watchdog();
> > > > +}
> > > 
> > > Can you not simply disable the watchdog in your loader and start it up
> > > from the board code? Having to do dummy reads from the thing in this
> > > convoluted manner is rather undesirable.
> > 
> > The watchdog is on from poweron, it cannot be disabled in any possible
> > way.  You have at most 1 second (or less on some versions) to service it or
> > it resets the hardware.
>
> Then perhaps we should just be abstracting out the romfs copy. ie, have
> the generic copy_romfs() (possibly renamed to something more reasonable)
> take care of extracting all of the various image details, and then kick
> the addresses down to a platform-specific symbol to do the actual copy.
> This too can be a weak symbol, where the default implementation simply
> does the copy without the watchdog noise.
> 
> This way you can keep all of your platform-specific cruft isolated
> without making a compromise on functionality.

That I can do. So putting all the special cases into the boards/snapgear
area is the preferred option ?  I'll have a go and see what it looks
like.

Hopefully some more patches soon ;-)

Thanks,
Davidm

-- 
David McCullough,  dav...@se...,   Ph:+61 734352815
Secure Computing - SnapGear  http://www.uCdot.org http://www.cyberguard.com