Thanks for your detailed explanation! Please see my comments below. On 05/23/2012 06:12 AM, Maciej W. Rozycki wrote:
On Tue, 22 May 2012, Deng-Cheng Zhu wrote:Hmm, there's nothing platform-specific there, the file is pretty generic, it could be moved to arch/mips/kernel/ or thereabouts. That applies to <asm/mips-boards/launch.h> too, before you askYeah, agree with you. I didn't do it simply because I'm not sure :)I can see you've copied the whole contents over to arch/mips/kernel/vpe.c now. Please don't do that. This code is modular for a reason. Either modify original code to suit your needs while preserving functionality or, if infeasible, copy it over to a new module selected based on configuration. Common parts should be abstracted and extracted to a common dependency, either a shared header or another module, as applicable.
OK. Good advice!
(you may want to use alloc_bootmem or suchlike instead of hardcoding the trampoline page, though it's probably pretty safe to assume the end of the exception handler page is available everywhere).I'm not quite clear about this. Do you mean to bypass the arbitrary monitor in vpe_run() (in other words, to directly bring up the vpe in vpe_run())? Why do we need to worry about writing to the cpulaunch data?The location of RAM is platform-specific, CKSEG0ADDR mustn't be used to "allocate" kernel memory. But as I say the exception handlers' page is generally pretty safe, although the addition of the CP0 EBase register to the architecture stopped it being guaranteed at one point. Ultimately I think this memory should be properly allocated, but this is preexisting code, so there is no requirement that you fix that on this occasion (or at all), unless you'd really like to.
OK. I'll let it be for now.
There's nothing platform-specific referred to from arch/mips/kernel/vpe.c AFAICT (and I trust in Beth having got this piece right). I reckon it used to work with CONFIG_MIPS_SIM too, though I could imagine the configuration got neglected a bit as it is somewhat unusual.Oh, When I said IRQ related stuff I meant the interrupt specific changes in rtlx.c (not vpe.c) which correspond to those in malta-int.c. They are there to resolve some issues (Please refer to the code changes and added comments in these 2 files in PATCH #1 and #2.).I still can't see anything platform-specific in rtlx.c (also written by Beth, BTW) -- it's all software interrupts that are architectural. What pieces of code and comments are you specifically referring to?
I meant some interrupt specific changes in rtlx.c correspond to platform- specific ones in malta-int.c. You may simply refer to the latter for the issues I addressed. The changes to malta-int.c led to the platform dependency and it seems the issues could not be tackled in generic layer.
Also in some places you do stuff like:
#ifdef CONFIG_MIPS_CMP
int foo(void)
{
[something...]
}
#else
int foo(void)
{
[something else...]
}
#endif
Please don't. Again these pieces should be separate modules selected by
configuration, e.g. rtlx.c, rtlx-mt.c and rtlx-cmp.c with the former
holding the common stuff, and the two latters non-CMP- and CMP-specific
pieces, respectively (assuming that they are mutually exclusive and can't
be enabled both at a time in a single kernel binary for some reason).
Thanks, good point.
It may make sense to move this whole stuff to a subdirectory at one point.
Do you mean to move things like vpe.c, kspd.c and rtlx.c (and the proposed -mt/-cmp variants) into a directory such as arch/mips/kernel/aprp/? Deng-Cheng
