20.03.2013, 01:37, "Sam Ravnborg" <sam@xxxxxxxxxxxx>:
> On Fri, Mar 08, 2013 at 08:19:46PM +0400, Kirill Tkhai wrote:
>
>> Hi!
>>
>> This patch is for hibernation support on sparc64.
>
> Hi Kirill.
> Some late nitpick feedback.
>
Hi, Sam.
>> Two steps to try:
>> 1)"echo disk > /sys/power/state" makes a system sleeping
>> 2)kernel parameter "resume=/dev/[swap_partition]" will say
>> the system which swap partition to use for restore.
>>
>> Use MMU bypass to copy data pages. This allowes not to
>> create temporary page table as on some other architectures.
>>
>> I changed mm/init_64.c because of restore may be called
>> from initcall. This moment is before free_initmem(), so
>> num_physpages must not be changed in it. Otherwise,
>> restore fails by the reason of mismatched sizes of the loaded
>> image and the system.
>>
>> I tested the patch on single-core mcst-r1000 variant of v9.
>> Everything work, but everybody is welcome to test it on any
>> more standard v9 :)
>>
>> Signed-off-by: Kirill Tkhai <tkhai@xxxxxxxxx>
>> CC: David Miller <davem@xxxxxxxxxxxxx>
>> ---
>> diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
>> index 9bff3db..9e1256f 100644
>> --- a/arch/sparc/Kconfig
>> +++ b/arch/sparc/Kconfig
>> @@ -103,6 +103,9 @@ config HAVE_LATENCYTOP_SUPPORT
>> bool
>> default y if SPARC64
>>
>> +config ARCH_HIBERNATION_POSSIBLE
>> + def_bool y if SPARC64
>> +
>> config AUDIT_ARCH
>> bool
>> default y
>> @@ -333,6 +336,10 @@ config ARCH_SPARSEMEM_DEFAULT
>>
>> source "mm/Kconfig"
>>
>> +if SPARC64
>> +source "kernel/power/Kconfig"
>> +endif
>
> The if SPARC64 is really not needed. As far as I could see we
> do not enable anything uin there for sparc32 anyway.
> So the only purpose of adding the "if" thing is for documentation.
Otherwise it is shown at xconfig when "Show all options" is turned
on. I copied the way how "drivers/cpufreq/Kconfig" is included.
>
>> +
>> config SCHED_SMT
>> bool "SMT (Hyperthreading) scheduler support"
>> depends on SPARC64 && SMP
>> diff --git a/arch/sparc/Makefile b/arch/sparc/Makefile
>> index 541b8b0..9ff4236 100644
>> --- a/arch/sparc/Makefile
>> +++ b/arch/sparc/Makefile
>> @@ -57,6 +57,7 @@ core-y += arch/sparc/
>> libs-y += arch/sparc/prom/
>> libs-y += arch/sparc/lib/
>>
>> +drivers-$(CONFIG_PM) += arch/sparc/power/
>> drivers-$(CONFIG_OPROFILE) += arch/sparc/oprofile/
>>
>> boot := arch/sparc/boot
>> diff --git a/arch/sparc/kernel/head_64.S b/arch/sparc/kernel/head_64.S
>> index 2feb15c..1a2ce1e 100644
>> diff --git a/arch/sparc/lib/Makefile b/arch/sparc/lib/Makefile
>> index 8410065..d4fc2f0 100644
>
> These looks bogus. Did you touch file permissions?
It's a wrong diff metadata. And one more below. Sorry.
>> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
>> index 5c2c6e6..a40a845 100644
>> --- a/arch/sparc/mm/init_64.c
>> +++ b/arch/sparc/mm/init_64.c
>> @@ -2061,7 +2061,7 @@ void __init mem_init(void)
>> * allocated below.
>> */
>> totalram_pages -= 1;
>> - num_physpages = totalram_pages;
>> + num_physpages = memblock_mem_size(last_valid_pfn) >> PAGE_SHIFT;
>>
>> /*
>> * Set up the zero page, mark it reserved, so that page count
>> @@ -2125,7 +2125,6 @@ void free_initmem(void)
>> ClearPageReserved(p);
>> init_page_count(p);
>> __free_page(p);
>> - num_physpages++;
>> totalram_pages++;
>> }
>> }
>> @@ -2142,7 +2141,6 @@ void free_initrd_mem(unsigned long start, unsigned long end)
>> ClearPageReserved(p);
>> init_page_count(p);
>> __free_page(p);
>> - num_physpages++;
>> totalram_pages++;
>> }
>> }
>
> Could you separate this out as an independent change?
> I must admit I do not see why what you do here is OK.
>
The arch-independent places where num_physpages
is used are
inet_frags_init()
sanitize_global_limit()
__online_page_set_limits()
and hibernation code.
It looks like everything is OK with all of them.
>> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
>> index 8ad21a2..380e1a1 100644
>
> Looks bogus.
>
>> diff --git a/arch/sparc/power/Makefile b/arch/sparc/power/Makefile
>> new file mode 100644
>> index 0000000..3201ace
>> --- /dev/null
>> +++ b/arch/sparc/power/Makefile
>> @@ -0,0 +1,3 @@
>> +# Makefile for Sparc-specific hibernate files.
>> +
>> +obj-$(CONFIG_HIBERNATION) += hibernate.o hibernate_asm.o
>> diff --git a/arch/sparc/power/hibernate.c b/arch/sparc/power/hibernate.c
>> new file mode 100644
>> index 0000000..89ca059
>> --- /dev/null
>> +++ b/arch/sparc/power/hibernate.c
>> @@ -0,0 +1,65 @@
>> +/*
>> + * hibernate.c: Hibernaton support specific for sparc64.
>> + *
>> + * Copyright (C) 2013 Kirill V Tkhai (tkhai@xxxxxxxxx)
>> + */
>> +
>> +#include <linux/mm.h>
>
> Empty line..
>
Yes, thanks for this and for other formatting points.
>> +#include <asm/page.h>
>> +#include <asm/tlb.h>
>> +#include <asm/visasm.h>
>
> Inverse christmas tree. Longest lines first.
>
>> +
>> +
>> +#include "hibernate.h"
>> +
>> +/* References to section boundaries */
>> +extern const void __nosave_begin, __nosave_end;
>
> These belong in asm-generic/sections.h
> Consider submitting a separate patch to Arnd.
>> +
>> +struct saved_context {
>> + unsigned long fp;
>> + unsigned long cwp;
>> + unsigned long wstate;
>> +
>> + unsigned long tick;
>> + unsigned long pstate;
>> +
>> + unsigned long g4;
>> + unsigned long g5;
>> + unsigned long g6;
>> +} saved_context;
>> +
>> +/*
>> + * pfn_is_nosave - check if given pfn is in the 'nosave' section
>> + */
>> +
>> +int pfn_is_nosave(unsigned long pfn)
>> +{
>> + unsigned long nosave_begin_pfn = ((unsigned long)&__nosave_begin) >> PAGE_SHIFT;
>> + unsigned long nosave_end_pfn = ((unsigned long)&__nosave_end) >> PAGE_SHIFT;
>
> Use PFN_DOWN
>
>> +
>> + BUILD_BUG_ON(SC_REG_FP != offsetof(struct saved_context, fp));
>> + BUILD_BUG_ON(SC_REG_CWP != offsetof(struct saved_context, cwp));
>> + BUILD_BUG_ON(SC_REG_WSTATE != offsetof(struct saved_context, wstate));
>> +
>> + BUILD_BUG_ON(SC_REG_TICK != offsetof(struct saved_context, tick));
>> + BUILD_BUG_ON(SC_REG_PSTATE != offsetof(struct saved_context, pstate));
>> +
>> + BUILD_BUG_ON(SC_REG_G4 != offsetof(struct saved_context, g4));
>> + BUILD_BUG_ON(SC_REG_G5 != offsetof(struct saved_context, g5));
>> + BUILD_BUG_ON(SC_REG_G6 != offsetof(struct saved_context, g6));
>
> Hmmm... See below.
>
>> +
>> + return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
>> +}
>> +
>> +void save_processor_state(void)
>> +{
>> + save_and_clear_fpu();
>> +}
>> +
>> +void restore_processor_state(void)
>> +{
>> + struct mm_struct *mm = current->active_mm;
>> +
>> + load_secondary_context(mm);
>> + tsb_context_switch(mm);
>> +}
>> diff --git a/arch/sparc/power/hibernate.h b/arch/sparc/power/hibernate.h
>> new file mode 100644
>> index 0000000..6c002f4
>> --- /dev/null
>> +++ b/arch/sparc/power/hibernate.h
>> @@ -0,0 +1,21 @@
>> +/*
>> + * hibernate.c: Hibernaton support specific for sparc64.
>> + *
>> + * Copyright (C) 2013 Kirill V Tkhai (tkhai@xxxxxxxxx)
>> + */
>> +
>> +#ifndef ___SPARC_HIBERNATE_H
>> +#define ___SPARC_HIBERNATE_H
>> +
>> +#define SC_REG_FP 0
>> +#define SC_REG_CWP 8
>> +#define SC_REG_WSTATE 16
>> +
>> +#define SC_REG_TICK 24
>> +#define SC_REG_PSTATE 32
>> +
>> +#define SC_REG_G4 40
>> +#define SC_REG_G5 48
>> +#define SC_REG_G6 56
>
> Use kernel/asm-offsets.c to generate these constants.
> This is relaiable and you can skip the run-time checks above.
>
Good advice. I'll follow it.
>> +
>> +#endif
>> diff --git a/arch/sparc/power/hibernate_asm.S b/arch/sparc/power/hibernate_asm.S
>> new file mode 100644
>> index 0000000..8cc5c4e
>> --- /dev/null
>> +++ b/arch/sparc/power/hibernate_asm.S
>> @@ -0,0 +1,129 @@
>> +/*
>> + * hibernate.c: Hibernaton support specific for sparc64.
>> + *
>> + * Copyright (C) 2013 Kirill V Tkhai (tkhai@xxxxxxxxx)
>> + */
>> +
>> +#include <linux/linkage.h>
>> +#include <asm/page.h>
>> +#include <asm/cpudata.h>
>> +#include "hibernate.h"
>
> Empty lines between the blocks.
> And inverse christmas tree again.
>
> For the asm part I did not look at it carefully.
> But a few comments anyway.
>
>> +
>> +ENTRY(swsusp_arch_suspend)
>> + save %sp, -192, %sp
>> + save %sp, -192, %sp
>
> This looks strange. Why two times?
It's because I want save only
one %o or %i register -- %i6 (fp). Othervise
it's necessary to save at least one more -- %i7.
>
>> + flushw
>> +
>> + setuw saved_context, %g3
>> +
>> + /* Save window regs */
>> + rdpr %cwp, %g2
>> + stx %g2, [%g3 + SC_REG_CWP]
>> + rdpr %wstate, %g2
>> + stx %g2, [%g3 + SC_REG_WSTATE]
>> + stx %fp, [%g3 + SC_REG_FP]
>> +
>> + /* Save state regs */
>> + rdpr %tick, %g2
>> + stx %g2, [%g3 + SC_REG_TICK]
>> + rdpr %pstate, %g2
>> + stx %g2, [%g3 + SC_REG_PSTATE]
>> +
>> + /* Save global regs */
>> + stx %g4, [%g3 + SC_REG_G4]
>> + stx %g5, [%g3 + SC_REG_G5]
>> + stx %g6, [%g3 + SC_REG_G6]
>> +
>> + call swsusp_save
>> + nop
>> +
>> + mov %o0, %i0
>> + restore
>> +
>> + mov %o0, %i0
>
> Uses space for indent => use tab
>
>> + ret
>> + restore
>> +
>> +ENTRY(swsusp_arch_resume)
>> + /* Write restore_pblist to %l0 */
>> + sethi %hi(restore_pblist), %l0
>> + ldx [%l0 + %lo(restore_pblist)], %l0
>
> Uses space for indent => use tab
>
>> +
>> + call __flush_tlb_all
>> + nop
>> +
>> + /* Write PAGE_OFFSET to %g7 */
>> + sethi %uhi(PAGE_OFFSET), %g7
>> + sllx %g7, 32, %g7
>> +
>> + setuw (PAGE_SIZE-8), %g3
>> +
>> + /* Use MMU Bypass */
>> + rd %asi, %g1
>> + wr %g0, ASI_PHYS_USE_EC, %asi
>> +
>> + /* Fill ITLB */
>> + ba 16f
>> + nop
>> +
>> +pbe_loop:
>> + cmp %l0, %g0
>> + be restore_ctx
>> + sub %l0, %g7, %l0
>
> Add empty line after delay slot
>
>> + ldxa [%l0 ] %asi, %l1 /* address */
>> + ldxa [%l0 + 8] %asi, %l2 /* orig_address */
>> +
>> + /* phys addr */
>> + sub %l1, %g7, %l1
>> + sub %l2, %g7, %l2
>> +
>> + mov %g3, %l3 /* PAGE_SIZE-8 */
>> +copy_loop:
>> + ldxa [%l1 + %l3] ASI_PHYS_USE_EC, %g2
>> + stxa %g2, [%l2 + %l3] ASI_PHYS_USE_EC
>> + cmp %l3, %g0
>> + bne copy_loop
>> + sub %l3, 8, %l3
>> +
>> + /* next pbe */
>> + ba pbe_loop
>> + ldxa [%l0 + 16] %asi, %l0
>> +
>> +restore_ctx:
>> + setuw saved_context, %g3
>> + wrpr %g0, 15, %pil
>> +
>> + /* Restore window regs */
>> + wrpr %g0, 0, %canrestore
>> + wrpr %g0, 0, %otherwin
>> + wrpr %g0, 6, %cansave
>> + wrpr %g0, 0, %cleanwin
>> +
>> + ldxa [%g3 + SC_REG_CWP] %asi, %g2
>> + wrpr %g2, %cwp
>> + ldxa [%g3 + SC_REG_WSTATE] %asi, %g2
>> + wrpr %g2, %wstate
>> + ldxa [%g3 + SC_REG_FP] %asi, %fp
>> +
>> + /* Restore state regs */
>> + ldxa [%g3 + SC_REG_PSTATE] %asi, %g2
>> + wrpr %g2, %pstate
>> + ldxa [%g3 + SC_REG_TICK] %asi, %g2
>> + wrpr %g2, %tick
>> +
>> + /* Restore global regs */
>> + ldxa [%g3 + SC_REG_G4] %asi, %g4
>> + ldxa [%g3 + SC_REG_G5] %asi, %g5
>> + ldxa [%g3 + SC_REG_G6] %asi, %g6
>> +
>> + wr %g1, %g0, %asi
>> +
>> + restore
>> + restore
>> +
>> + wrpr %g0, 14, %pil
>> +
>> + retl
>> + mov %g0, %o0
>> +
>> +16: ba,a pbe_loop
>
> All other labels have nice names - and then suddenly a label named "16"?
> And you are missing an instruction in the delay slot here.
Now, I think mask NMI earlier and put this action as delayed instruction
for this for the present unnamed branch. Looks like all copying must
processes with level=15.
Thanks for feedback, I will send new patch a little later.
Kirill
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
