| Message ID | 1307379898-14256-5-git-send-email-will.deacon@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 6 Jun 2011, Will Deacon wrote: > Tools such as kexec and CPU hotplug require a way to reset the processor > and branch to some code in physical space. This requires various bits of > jiggery pokery with the caches and MMU which, when it goes wrong, tends > to lock up the system. > > This patch implements a new function, arm_machine_reset, for > consolidating this code in one place where it can be used by multiple > subsystems. > [ ... ] > +void arm_machine_reset(unsigned long reset_code_phys) > +{ > + unsigned long cpu_reset_end = PAGE_ALIGN((unsigned long)cpu_reset); > + /* This is stricter than necessary but better to be safe than sorry. */ > + BUG_ON(virt_to_phys((void *)cpu_reset_end) >= TASK_SIZE); > + > + /* Disable interrupts first */ > + local_irq_disable(); > + local_fiq_disable(); > + > + /* > + * Clean and invalidate L2. > + * This is racy, so we must be the last guy left. > + */ > + WARN_ON(num_online_cpus() > 1); > + /* Flush while we still have locking available to us. */ > + outer_flush_all(); > + outer_disable(); > + /* Data destroyed here will only be speculative. */ > + outer_inv_all(); > + > + prepare_for_reboot(0); > + > + /* Switch to the identity mapping. */ > + ((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys); void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset) > +} > + > /* > * Function pointers to optional machine specific functions > */ > -- > 1.7.0.4 > >
On Tue, 7 Jun 2011, Frank Hofmann wrote: > > > On Mon, 6 Jun 2011, Will Deacon wrote: > >> Tools such as kexec and CPU hotplug require a way to reset the processor >> and branch to some code in physical space. This requires various bits of >> jiggery pokery with the caches and MMU which, when it goes wrong, tends >> to lock up the system. >> >> This patch implements a new function, arm_machine_reset, for >> consolidating this code in one place where it can be used by multiple >> subsystems. >> > [ ... ] >> +void arm_machine_reset(unsigned long reset_code_phys) >> +{ >> + unsigned long cpu_reset_end = PAGE_ALIGN((unsigned long)cpu_reset); >> + /* This is stricter than necessary but better to be safe than sorry. >> */ >> + BUG_ON(virt_to_phys((void *)cpu_reset_end) >= TASK_SIZE); >> + >> + /* Disable interrupts first */ >> + local_irq_disable(); >> + local_fiq_disable(); >> + >> + /* >> + * Clean and invalidate L2. >> + * This is racy, so we must be the last guy left. >> + */ >> + WARN_ON(num_online_cpus() > 1); >> + /* Flush while we still have locking available to us. */ >> + outer_flush_all(); >> + outer_disable(); >> + /* Data destroyed here will only be speculative. */ >> + outer_inv_all(); >> + >> + prepare_for_reboot(0); >> + >> + /* Switch to the identity mapping. */ >> + ((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys); > > void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset) Sorry, pressed wrong key ... posted too early. I meant to say this line is magic, why not decouple the declaration bit from the invocation so that at least the function call looks "normal" ? Regarding the use of local_irq/fiq_disable - is it safe to call these multiple times, i.e. is it safe to invoke the reset function from a context where IRQ/FIQ are already off ? Another question regarding the MMU tables. prepare_for_reboot / setup_mm_for_reboot assume that current->mm is available _and_ lowmem / user area there can be blotted over. Wrt. to hibernation, that's a stretch unless some way is found to fully "fake a context". With that I mean to create a threadinfo and pagedir that's guaranteed to sit outside the target kernel heap. Currently, the hibernation code switches to swapper_pg_dir and puts the 1:1 mappings in there; I'm starting to think that is safe if for no other reason than swapper_pg_dir having _nothing_ in the user area ? Is it convention, design or accident that swapper_pg_dir doesn't map anything in the low range ? FrankH. > >> +} >> + >> /* >> * Function pointers to optional machine specific functions >> */ >> -- >> 1.7.0.4 >> >> >
Hi Frank, Thanks for looking at this. On Tue, Jun 07, 2011 at 12:37:23PM +0100, Frank Hofmann wrote: > >> +void arm_machine_reset(unsigned long reset_code_phys) > >> +{ > >> + unsigned long cpu_reset_end = PAGE_ALIGN((unsigned long)cpu_reset); > >> + /* This is stricter than necessary but better to be safe than sorry. > >> */ > >> + BUG_ON(virt_to_phys((void *)cpu_reset_end) >= TASK_SIZE); > >> + > >> + /* Disable interrupts first */ > >> + local_irq_disable(); > >> + local_fiq_disable(); > >> + > >> + /* > >> + * Clean and invalidate L2. > >> + * This is racy, so we must be the last guy left. > >> + */ > >> + WARN_ON(num_online_cpus() > 1); > >> + /* Flush while we still have locking available to us. */ > >> + outer_flush_all(); > >> + outer_disable(); > >> + /* Data destroyed here will only be speculative. */ > >> + outer_inv_all(); > >> + > >> + prepare_for_reboot(0); > >> + > >> + /* Switch to the identity mapping. */ > >> + ((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys); > > > > void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset) > > Sorry, pressed wrong key ... posted too early. No problem. > I meant to say this line is magic, why not decouple the declaration bit > from the invocation so that at least the function call looks "normal" ? You mean you don't like the LISP-ish look of it?! I take your point, I'll rework that. > Regarding the use of local_irq/fiq_disable - is it safe to call these > multiple times, i.e. is it safe to invoke the reset function from a > context where IRQ/FIQ are already off ? Yes, they just mask at the CPSR level. > > Another question regarding the MMU tables. > > prepare_for_reboot / setup_mm_for_reboot assume that current->mm is > available _and_ lowmem / user area there can be blotted over. > > Wrt. to hibernation, that's a stretch unless some way is found to fully > "fake a context". With that I mean to create a threadinfo and pagedir > that's guaranteed to sit outside the target kernel heap. Ouch, ok. > Currently, the hibernation code switches to swapper_pg_dir and puts the > 1:1 mappings in there; I'm starting to think that is safe if for no other > reason than swapper_pg_dir having _nothing_ in the user area ? That sounds ok providing that, when you come out of hibernate, the 1:1 mapping doesn't persist in swapper_pg_dir. However, stepping back a bit here, there is a bigger problem to tackle. If the physical address of cpu_reset is bigger than TASK_SIZE, then we fail to map it using the current identity mapping code [that piggy backs on the current task]. If PHYS_OFFSET > TASK_SIZE, this will *always* be the case and is probably quite common for a 2:2 split. I can think of two ways to get around this: (1) Use /another/ set of page tables for mapping the cpu_reset code only [and nothing else]. This is tricky since it will need to be set extremely late-on where we don't care about mapping in the rest of the kernel, data, stack etc. (2) Make the assumption that the physical address of cpu_reset will not conflict with a virtual address that points at the kernel text via the linear mapping. This is probably a reasonable thing to assume, given that the kernel lives near the start of memory on most platforms. We could then take out the ID map but leaving the kernel text alone. We'd probably have to change to a new stack, which we could place immediately after the kernel text. What do you reckon? > Is it convention, design or accident that swapper_pg_dir doesn't map > anything in the low range ? With the new switch_mm stuff, it's absolutely critical that we don't have user mappings in there during normal execution (going down for hibernate is probably ok since we'll clobber the caches and TLBs anyway). Will
On Tue, 7 Jun 2011, Will Deacon wrote: > Hi Frank, > > Thanks for looking at this. Thanks for posting it ;-) [ ... ] >>>> + /* Switch to the identity mapping. */ >>>> + ((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys); >>> >>> void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset) >> >> Sorry, pressed wrong key ... posted too early. > > No problem. > >> I meant to say this line is magic, why not decouple the declaration bit >> from the invocation so that at least the function call looks "normal" ? > > You mean you don't like the LISP-ish look of it?! I take your point, I'll rework > that. typeof(cpu_reset)(*phys_reset) = (typeof(cpu_reset) *)virt_to_phys(cpu_reset); reset_func(reset_code_phys); Works for me. Was experimenting with this when I pressed the wrong key. > >> Regarding the use of local_irq/fiq_disable - is it safe to call these >> multiple times, i.e. is it safe to invoke the reset function from a >> context where IRQ/FIQ are already off ? > > Yes, they just mask at the CPSR level. Good, thx for the confirmation. > >> >> Another question regarding the MMU tables. >> >> prepare_for_reboot / setup_mm_for_reboot assume that current->mm is >> available _and_ lowmem / user area there can be blotted over. >> >> Wrt. to hibernation, that's a stretch unless some way is found to fully >> "fake a context". With that I mean to create a threadinfo and pagedir >> that's guaranteed to sit outside the target kernel heap. > > Ouch, ok. > >> Currently, the hibernation code switches to swapper_pg_dir and puts the >> 1:1 mappings in there; I'm starting to think that is safe if for no other >> reason than swapper_pg_dir having _nothing_ in the user area ? > > That sounds ok providing that, when you come out of hibernate, the 1:1 mapping > doesn't persist in swapper_pg_dir. I can delete those mappings - as they're created by identity_mapping_add() they can be ditched with identity_mapping_del() - which might be sufficient, provided, and there's the critical bit why I ask, that swapper_pg_dir _normally_ has nothing in the range anyway. Thing is, just because something appears to work doesn't mean it has no unexpected/undesired side effects ... and I'm no ARM VM guru. > > However, stepping back a bit here, there is a bigger problem to tackle. If the > physical address of cpu_reset is bigger than TASK_SIZE, then we fail to map it > using the current identity mapping code [that piggy backs on the current task]. > If PHYS_OFFSET > TASK_SIZE, this will *always* be the case and is probably quite > common for a 2:2 split. > > I can think of two ways to get around this: > > (1) Use /another/ set of page tables for mapping the cpu_reset code only [and > nothing else]. This is tricky since it will need to be set extremely late-on > where we don't care about mapping in the rest of the kernel, data, stack etc. > > (2) Make the assumption that the physical address of cpu_reset will not conflict > with a virtual address that points at the kernel text via the linear mapping. > This is probably a reasonable thing to assume, given that the kernel lives > near the start of memory on most platforms. We could then take out the ID map > but leaving the kernel text alone. We'd probably have to change to a new stack, > which we could place immediately after the kernel text. > > What do you reckon? cpu_reset itself is relocatable, isn't it ? Maybe one could do the same thing with/for it as for the reset code itself. I.e. relocate to a "suitable" page if the 1:1 mapping for all of lowmem isn't possible. The catch with that is of course somehow, such a "1:1 candidate page" must be found. > >> Is it convention, design or accident that swapper_pg_dir doesn't map >> anything in the low range ? > > With the new switch_mm stuff, it's absolutely critical that we don't have > user mappings in there during normal execution (going down for hibernate > is probably ok since we'll clobber the caches and TLBs anyway). I'm using swapper_pg_dir for hibernate (restore) for the simple reason that it maps all of kernel text/data writeable. When invoking resume via uswsusp and/or tuxonice, it's triggered from a user context which is insufficient to _rewrite_ "universe". So when the time comes to do the cpu_reset / cpu_resume dance, the active mm context is swapper_pg_dir; hence creating the identity mappings in there. It's not hard to zap them afterwards; it'd be far more difficult to restore previous contents if there had been any. Short: Good news that the user pagedir section in swapper_pg_dir is empty by design. Thanks, FrankH.
On Tue, Jun 07, 2011 at 02:54:53PM +0100, Frank Hofmann wrote: > > > On Tue, 7 Jun 2011, Will Deacon wrote: > > >Hi Frank, > > > >Thanks for looking at this. > > Thanks for posting it ;-) > > [ ... ] > >>>>+ /* Switch to the identity mapping. */ > >>>>+ ((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys); > >>> > >>>void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset) > >> > >>Sorry, pressed wrong key ... posted too early. > > > >No problem. > > > >>I meant to say this line is magic, why not decouple the declaration bit > >>from the invocation so that at least the function call looks "normal" ? > > > >You mean you don't like the LISP-ish look of it?! I take your point, I'll rework > >that. > > typeof(cpu_reset)(*phys_reset) = > (typeof(cpu_reset) *)virt_to_phys(cpu_reset); This is a declaration, but the extra parentheses confuse me. How about: typeof(cpu_reset) *phys_reset = (typeof(cpu_reset) *)virt_to_phys(cpu_reset); or even: typedef typeof(cpu_reset) *phys_reset_t; phys_reset_t phys_reset = (phys_reset_t)virt_to_phys(cpu_reset); > > reset_func(reset_code_phys); Do you mean reset_func(phys_reset) ? > > Works for me. Was experimenting with this when I pressed the wrong key. > > > > >>Regarding the use of local_irq/fiq_disable - is it safe to call these > >>multiple times, i.e. is it safe to invoke the reset function from a > >>context where IRQ/FIQ are already off ? > > > >Yes, they just mask at the CPSR level. > > Good, thx for the confirmation. > > > > >> > >>Another question regarding the MMU tables. > >> > >>prepare_for_reboot / setup_mm_for_reboot assume that current->mm is > >>available _and_ lowmem / user area there can be blotted over. > >> > >>Wrt. to hibernation, that's a stretch unless some way is found to fully > >>"fake a context". With that I mean to create a threadinfo and pagedir > >>that's guaranteed to sit outside the target kernel heap. > > > >Ouch, ok. > > > >>Currently, the hibernation code switches to swapper_pg_dir and puts the > >>1:1 mappings in there; I'm starting to think that is safe if for no other > >>reason than swapper_pg_dir having _nothing_ in the user area ? > > > >That sounds ok providing that, when you come out of hibernate, the 1:1 mapping > >doesn't persist in swapper_pg_dir. > > I can delete those mappings - as they're created by > identity_mapping_add() they can be ditched with > identity_mapping_del() - which might be sufficient, provided, and > there's the critical bit why I ask, that swapper_pg_dir _normally_ > has nothing in the range anyway. > > Thing is, just because something appears to work doesn't mean it has > no unexpected/undesired side effects ... and I'm no ARM VM guru. > > > > > >However, stepping back a bit here, there is a bigger problem to tackle. If the > >physical address of cpu_reset is bigger than TASK_SIZE, then we fail to map it > >using the current identity mapping code [that piggy backs on the current task]. > >If PHYS_OFFSET > TASK_SIZE, this will *always* be the case and is probably quite > >common for a 2:2 split. > > > >I can think of two ways to get around this: > > > >(1) Use /another/ set of page tables for mapping the cpu_reset code only [and > > nothing else]. This is tricky since it will need to be set extremely late-on > > where we don't care about mapping in the rest of the kernel, data, stack etc. > > > >(2) Make the assumption that the physical address of cpu_reset will not conflict > > with a virtual address that points at the kernel text via the linear mapping. > > This is probably a reasonable thing to assume, given that the kernel lives > > near the start of memory on most platforms. We could then take out the ID map > > but leaving the kernel text alone. We'd probably have to change to a new stack, > > which we could place immediately after the kernel text. > > > >What do you reckon? > > cpu_reset itself is relocatable, isn't it ? Maybe one could do the > same thing with/for it as for the reset code itself. I.e. relocate > to a "suitable" page if the 1:1 mapping for all of lowmem isn't > possible. The catch with that is of course somehow, such a "1:1 > candidate page" must be found. If cpu_reset is guaranteed to be position-independent and self-contained, we could just copy it (with fncpy() for example). We would still need to know the length of that function somehow though. Maybe just assuming that it's not longer than a page would be safe enough. In this case we would just need to find an identity-mappable target location to copy the code to; we can find such a location in the same way as that used to find space for the alternate stack, i.e., somewhere after the end of the kernel image. Cheers ---Dave
On Tue, 7 Jun 2011, Dave Martin wrote: > On Tue, Jun 07, 2011 at 02:54:53PM +0100, Frank Hofmann wrote: >> >> >> On Tue, 7 Jun 2011, Will Deacon wrote: >> >>> Hi Frank, >>> >>> Thanks for looking at this. >> >> Thanks for posting it ;-) >> >> [ ... ] >>>>>> + /* Switch to the identity mapping. */ >>>>>> + ((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys); >>>>> >>>>> void (*reset_func)(unsigned long) = virt_to_phys(cpu_reset) >>>> >>>> Sorry, pressed wrong key ... posted too early. >>> >>> No problem. >>> >>>> I meant to say this line is magic, why not decouple the declaration bit >>>> from the invocation so that at least the function call looks "normal" ? >>> >>> You mean you don't like the LISP-ish look of it?! I take your point, I'll rework >>> that. >> >> typeof(cpu_reset)(*phys_reset) = >> (typeof(cpu_reset) *)virt_to_phys(cpu_reset); > > This is a declaration, but the extra parentheses confuse me. > > How about: > > typeof(cpu_reset) *phys_reset = > (typeof(cpu_reset) *)virt_to_phys(cpu_reset); Function pointers ;-) Thanks. > > or even: > > typedef typeof(cpu_reset) *phys_reset_t; > phys_reset_t phys_reset = (phys_reset_t)virt_to_phys(cpu_reset); >> >> reset_func(reset_code_phys); > > Do you mean reset_func(phys_reset) ? phys_reset(reset_code_phys); me and my copy & paste ... [ ... ] >> cpu_reset itself is relocatable, isn't it ? Maybe one could do the >> same thing with/for it as for the reset code itself. I.e. relocate >> to a "suitable" page if the 1:1 mapping for all of lowmem isn't >> possible. The catch with that is of course somehow, such a "1:1 >> candidate page" must be found. > > If cpu_reset is guaranteed to be position-independent and self-contained, we could > just copy it (with fncpy() for example). We would still need to know the length > of that function somehow though. Maybe just assuming that it's not longer than > a page would be safe enough. In all current CPU implementations it's position-dependent and self-contained; all they do are suitable control reg modifications, ending all with "mov pc, r0". > > In this case we would just need to find an identity-mappable target location to > copy the code to; we can find such a location in the same way as that used to > find space for the alternate stack, i.e., somewhere after the end of the kernel > image. If that's a normal thing to do, then this kind of 1:1-bounce-page seems a good way of avoiding the address range collision. best regards, FrankH. > > Cheers > ---Dave >
On Tue, 7 Jun 2011, Frank Hofmann wrote: > > > On Tue, 7 Jun 2011, Dave Martin wrote: [ ... ] >> How about: >> >> typeof(cpu_reset) *phys_reset = >> (typeof(cpu_reset) *)virt_to_phys(cpu_reset); > > Function pointers ;-) > Thanks. Hmmm ... Just found a problem with this. If you have a MULTI_CPU config, this doesn't compile. For two reasons: 1. you cannot use cpu_reset as argument to virt_to_phys because you can't take the address That bit can be fixed by changing the MULTI_CPU #define in <asm/proc-fns.h> not to include the macro argument. (There is no code in the arm tree using cpu_reset_whatever names which would break from that change ... still, not that nice) 2. even when you do that, you loose the "typeof()" information and the above still doesn't compile. Only a manual type override, void (*phys_reset)(unsigned long) = (void (*)(unsigned long))cpu_reset; is accepted then. FrankH. > >> >> or even: >> >> typedef typeof(cpu_reset) *phys_reset_t; >> phys_reset_t phys_reset = (phys_reset_t)virt_to_phys(cpu_reset); >>> >>> reset_func(reset_code_phys); >> >> Do you mean reset_func(phys_reset) ? > > phys_reset(reset_code_phys); > > me and my copy & paste ... > > [ ... ] >>> cpu_reset itself is relocatable, isn't it ? Maybe one could do the >>> same thing with/for it as for the reset code itself. I.e. relocate >>> to a "suitable" page if the 1:1 mapping for all of lowmem isn't >>> possible. The catch with that is of course somehow, such a "1:1 >>> candidate page" must be found. >> >> If cpu_reset is guaranteed to be position-independent and self-contained, >> we could >> just copy it (with fncpy() for example). We would still need to know the >> length >> of that function somehow though. Maybe just assuming that it's not longer >> than >> a page would be safe enough. > > In all current CPU implementations it's position-dependent and > self-contained; all they do are suitable control reg modifications, ending > all with "mov pc, r0". > >> >> In this case we would just need to find an identity-mappable target >> location to >> copy the code to; we can find such a location in the same way as that used >> to >> find space for the alternate stack, i.e., somewhere after the end of the >> kernel >> image. > > If that's a normal thing to do, then this kind of 1:1-bounce-page seems a > good way of avoiding the address range collision. > > best regards, > FrankH. > >> >> Cheers >> ---Dave >> >
On Wed, Jun 08, 2011 at 04:55:11PM +0100, Frank Hofmann wrote: > > > On Tue, 7 Jun 2011, Frank Hofmann wrote: > > > > > > > On Tue, 7 Jun 2011, Dave Martin wrote: > [ ... ] > >> How about: > >> > >> typeof(cpu_reset) *phys_reset = > >> (typeof(cpu_reset) *)virt_to_phys(cpu_reset); > > > > Function pointers ;-) > > Thanks. > > Hmmm ... > > Just found a problem with this. > > If you have a MULTI_CPU config, this doesn't compile. For two reasons: > > 1. you cannot use cpu_reset as argument to virt_to_phys because you can't > take the address > That bit can be fixed by changing the MULTI_CPU #define in > <asm/proc-fns.h> not to include the macro argument. > (There is no code in the arm tree using cpu_reset_whatever names which > would break from that change ... still, not that nice) > > 2. even when you do that, you loose the "typeof()" information and the > above still doesn't compile. > > Only a manual type override, > > void (*phys_reset)(unsigned long) = (void (*)(unsigned long))cpu_reset; > > is accepted then. Damn, yes, I assumed the MULTI_CPU case would just pointer at the structure field, but it takes the argument as parameter for the invocation. Oh well, I'll hardcode the type after all then! I'll send a v2 once I've finished cleaning up the code as I've tried to make it more useful following on from your earlier feedback. Cheers, Will
On Wed, 8 Jun 2011, Will Deacon wrote: > On Wed, Jun 08, 2011 at 04:55:11PM +0100, Frank Hofmann wrote: >> >> >> On Tue, 7 Jun 2011, Frank Hofmann wrote: >> >>> >>> >>> On Tue, 7 Jun 2011, Dave Martin wrote: >> [ ... ] >>>> How about: >>>> >>>> typeof(cpu_reset) *phys_reset = >>>> (typeof(cpu_reset) *)virt_to_phys(cpu_reset); >>> >>> Function pointers ;-) >>> Thanks. >> >> Hmmm ... >> >> Just found a problem with this. >> >> If you have a MULTI_CPU config, this doesn't compile. For two reasons: >> >> 1. you cannot use cpu_reset as argument to virt_to_phys because you can't >> take the address >> That bit can be fixed by changing the MULTI_CPU #define in >> <asm/proc-fns.h> not to include the macro argument. >> (There is no code in the arm tree using cpu_reset_whatever names which >> would break from that change ... still, not that nice) >> >> 2. even when you do that, you loose the "typeof()" information and the >> above still doesn't compile. >> >> Only a manual type override, >> >> void (*phys_reset)(unsigned long) = (void (*)(unsigned long))cpu_reset; >> >> is accepted then. > > Damn, yes, I assumed the MULTI_CPU case would just pointer at the structure field, > but it takes the argument as parameter for the invocation. Oh well, I'll hardcode > the type after all then! It's not just that - the worse bit is that as long as the #define looks like: #define cpu_reset(addr) processor.reset(addr) compile is being refused; one has to ditch the argument part of the macro to be able to take the address. I'm unsure how desirable that change is; it's got the unwanted consequence that people who decide to use function names like "graphics_cpu_reset" or "cpu_reset_specialregisters" would fall flat on their face. As said, not that anyone does so right now; just not nice to introduce such behaviour. FrankH. > > I'll send a v2 once I've finished cleaning up the code as I've tried to make it > more useful following on from your earlier feedback. > > Cheers, > > Will >
On Wed, Jun 08, 2011 at 05:10:05PM +0100, Frank Hofmann wrote: > >> Just found a problem with this. > >> > >> If you have a MULTI_CPU config, this doesn't compile. For two reasons: > >> > >> 1. you cannot use cpu_reset as argument to virt_to_phys because you can't > >> take the address > >> That bit can be fixed by changing the MULTI_CPU #define in > >> <asm/proc-fns.h> not to include the macro argument. > >> (There is no code in the arm tree using cpu_reset_whatever names which > >> would break from that change ... still, not that nice) > >> > >> 2. even when you do that, you loose the "typeof()" information and the > >> above still doesn't compile. > >> > >> Only a manual type override, > >> > >> void (*phys_reset)(unsigned long) = (void (*)(unsigned long))cpu_reset; > >> > >> is accepted then. > > > > Damn, yes, I assumed the MULTI_CPU case would just pointer at the structure field, > > but it takes the argument as parameter for the invocation. Oh well, I'll hardcode > > the type after all then! > > It's not just that - the worse bit is that as long as the #define looks > like: > > #define cpu_reset(addr) processor.reset(addr) > > compile is being refused; one has to ditch the argument part of the macro > to be able to take the address. > > I'm unsure how desirable that change is; it's got the unwanted consequence > that people who decide to use function names like "graphics_cpu_reset" or > "cpu_reset_specialregisters" would fall flat on their face. Isn't this already a problem in the !MULTI_CPU case? cpu_reset will be expanded to something like armv7_cpu_reset by the macro. > As said, not that anyone does so right now; just not nice to introduce > such behaviour. But if that behaviour is already there, I don't think it's a big deal. Will
On Wed, Jun 08, 2011 at 05:10:05PM +0100, Frank Hofmann wrote: > > > On Wed, 8 Jun 2011, Will Deacon wrote: > > >On Wed, Jun 08, 2011 at 04:55:11PM +0100, Frank Hofmann wrote: > >> > >> > >>On Tue, 7 Jun 2011, Frank Hofmann wrote: > >> > >>> > >>> > >>>On Tue, 7 Jun 2011, Dave Martin wrote: > >>[ ... ] > >>>>How about: > >>>> > >>>> typeof(cpu_reset) *phys_reset = > >>>> (typeof(cpu_reset) *)virt_to_phys(cpu_reset); > >>> > >>>Function pointers ;-) > >>>Thanks. > >> > >>Hmmm ... > >> > >>Just found a problem with this. > >> > >>If you have a MULTI_CPU config, this doesn't compile. For two reasons: > >> > >>1. you cannot use cpu_reset as argument to virt_to_phys because you can't > >> take the address > >> That bit can be fixed by changing the MULTI_CPU #define in > >> <asm/proc-fns.h> not to include the macro argument. > >> (There is no code in the arm tree using cpu_reset_whatever names which > >> would break from that change ... still, not that nice) > >> > >>2. even when you do that, you loose the "typeof()" information and the > >> above still doesn't compile. > >> > >>Only a manual type override, > >> > >> void (*phys_reset)(unsigned long) = (void (*)(unsigned long))cpu_reset; > >> > >>is accepted then. > > > >Damn, yes, I assumed the MULTI_CPU case would just pointer at the structure field, > >but it takes the argument as parameter for the invocation. Oh well, I'll hardcode > >the type after all then! > > It's not just that - the worse bit is that as long as the #define > looks like: > > #define cpu_reset(addr) processor.reset(addr) > > compile is being refused; one has to ditch the argument part of the > macro to be able to take the address. > > I'm unsure how desirable that change is; it's got the unwanted > consequence that people who decide to use function names like > "graphics_cpu_reset" or "cpu_reset_specialregisters" would fall flat > on their face. The preprocessor won't expand a macro name unless it appears as a separate token, surely? So if I have #define cpu_reset processor.reset then cpu_reset(graphics_cpu_reset) expands to processor.reset(graphics_cpu_reset) Which is the intention (even if it's a stupid example) Cheers ---Dave
On Wed, 8 Jun 2011, Dave Martin wrote: > On Wed, Jun 08, 2011 at 05:10:05PM +0100, Frank Hofmann wrote: [ ... ] >> It's not just that - the worse bit is that as long as the #define >> looks like: >> >> #define cpu_reset(addr) processor.reset(addr) >> >> compile is being refused; one has to ditch the argument part of the >> macro to be able to take the address. >> >> I'm unsure how desirable that change is; it's got the unwanted >> consequence that people who decide to use function names like >> "graphics_cpu_reset" or "cpu_reset_specialregisters" would fall flat >> on their face. > > The preprocessor won't expand a macro name unless it appears as a separate > token, surely? > > So if I have > > #define cpu_reset processor.reset > > then > > cpu_reset(graphics_cpu_reset) > > expands to > > processor.reset(graphics_cpu_reset) > > Which is the intention (even if it's a stupid example) Ah, I was concerned that it'd change graphics_cpu_reset ===> graphics_processor.reset but you're correct that kind of substitution is not happening. Anyway as Will mentioned, the !MULTI_CPU case has the #define the following way in any case: #define ____glue(name,fn) name##fn #define __glue(name,fn) ____glue(name,fn) #define cpu_reset __glue(CPU_NAME,_reset) I.e. in the !MULTI_CPU case cpu_reset is defined without the macro argument, while the MULTI_CPU define _has_ the macro argument. Would it be ok to remove the (args) part from the MULTI_CPU #define then ? Thanks, FrankH. > > Cheers > ---Dave > >
Hi Frank, On Thu, Jun 09, 2011 at 11:00:56AM +0100, Frank Hofmann wrote: > Ah, I was concerned that it'd change > > graphics_cpu_reset ===> graphics_processor.reset > > but you're correct that kind of substitution is not happening. > > Anyway as Will mentioned, the !MULTI_CPU case has the #define the > following way in any case: > > #define ____glue(name,fn) name##fn > #define __glue(name,fn) ____glue(name,fn) > #define cpu_reset __glue(CPU_NAME,_reset) > > I.e. in the !MULTI_CPU case cpu_reset is defined without the macro > argument, while the MULTI_CPU define _has_ the macro argument. > > Would it be ok to remove the (args) part from the MULTI_CPU #define then ? I was planning to do that in v2 of the patches. Cheers, Will
diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h index 832888d..cd2a3cd 100644 --- a/arch/arm/include/asm/system.h +++ b/arch/arm/include/asm/system.h @@ -108,6 +108,7 @@ extern int cpu_architecture(void); extern void cpu_init(void); void arm_machine_restart(char mode, const char *cmd); +void arm_machine_reset(unsigned long reset_code_phys); extern void (*arm_pm_restart)(char str, const char *cmd); #define UDBG_UNDEFINED (1 << 0) diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c index 5e1e541..0b46e9f 100644 --- a/arch/arm/kernel/process.c +++ b/arch/arm/kernel/process.c @@ -91,12 +91,8 @@ static int __init hlt_setup(char *__unused) __setup("nohlt", nohlt_setup); __setup("hlt", hlt_setup); -void arm_machine_restart(char mode, const char *cmd) +static void prepare_for_reboot(char mode) { - /* Disable interrupts first */ - local_irq_disable(); - local_fiq_disable(); - /* * Tell the mm system that we are going to reboot - * we may need it to insert some 1:1 mappings so that @@ -112,6 +108,15 @@ void arm_machine_restart(char mode, const char *cmd) /* Push out any further dirty data, and ensure cache is empty */ flush_cache_all(); +} + +void arm_machine_restart(char mode, const char *cmd) +{ + /* Disable interrupts first */ + local_irq_disable(); + local_fiq_disable(); + + prepare_for_reboot(mode); /* * Now call the architecture specific reboot code. @@ -127,6 +132,33 @@ void arm_machine_restart(char mode, const char *cmd) while (1); } +void arm_machine_reset(unsigned long reset_code_phys) +{ + unsigned long cpu_reset_end = PAGE_ALIGN((unsigned long)cpu_reset); + /* This is stricter than necessary but better to be safe than sorry. */ + BUG_ON(virt_to_phys((void *)cpu_reset_end) >= TASK_SIZE); + + /* Disable interrupts first */ + local_irq_disable(); + local_fiq_disable(); + + /* + * Clean and invalidate L2. + * This is racy, so we must be the last guy left. + */ + WARN_ON(num_online_cpus() > 1); + /* Flush while we still have locking available to us. */ + outer_flush_all(); + outer_disable(); + /* Data destroyed here will only be speculative. */ + outer_inv_all(); + + prepare_for_reboot(0); + + /* Switch to the identity mapping. */ + ((typeof(cpu_reset) *)virt_to_phys((void *)cpu_reset))(reset_code_phys); +} + /* * Function pointers to optional machine specific functions */
Tools such as kexec and CPU hotplug require a way to reset the processor and branch to some code in physical space. This requires various bits of jiggery pokery with the caches and MMU which, when it goes wrong, tends to lock up the system. This patch implements a new function, arm_machine_reset, for consolidating this code in one place where it can be used by multiple subsystems. Signed-off-by: Will Deacon <will.deacon@arm.com> --- arch/arm/include/asm/system.h | 1 + arch/arm/kernel/process.c | 42 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 38 insertions(+), 5 deletions(-)