Hi,

On Mon, Mar 19, 2018 at 03:51:24AM +0800, Guo Ren wrote:

+inline static unsigned int
+get_regs_value(unsigned int rx, struct pt_regs *regs)
+{
+	unsigned int value;
+
+	if(rx == 0){
+		if(user_mode(regs)){
+			asm volatile("mfcr %0, ss1\n":"=r"(value));

Here you open code an MFCR instruction.

I guess MFCR stands for something like move-from-control-register, and MTCR
stands for move-to-control-register?

I see that later on you have helpers for specific registers, e.g. mfcr_cpuidrr().

You might want to follow the example of arm64's read_sysreg() and
write_sysreg(), and have general purpose helpers for thos instructions, e.g.

#define mfcr(reg)						\
({								\
	unsigned long __mfcr_val;				\
	asm volatile("mfcr %0, " #reg "\n" : "=r" (__mfr_val));	\
	__mfcr_val;						\
})

... which avoids needing helpers for each register, as you can do:

ss1_val = mfcr(ss1);
cpuidrr_val = mfcr(cpuidrr);

[...]

+static __init void setup_cpu_msa(void)
+{
+	if (memblock_start_of_DRAM() != (PHYS_OFFSET + CONFIG_RAM_BASE)) {
+		pr_err("C-SKY: dts-DRAM doesn't fit .config: %x-%x.\n",
+			memblock_start_of_DRAM(),
+			PHYS_OFFSET + CONFIG_RAM_BASE);
+		return;
+	}

If this is a problem, is it safe to continue at all?

Why does the base address of RAM matter?

+
+	mtcr_msa0(PHYS_OFFSET | 0xe);
+	mtcr_msa1(PHYS_OFFSET | 0x6);

As with MFCR, you could use a generic helper here, e.g.

#define mtcr(val, reg)								\
do {										\
	asm volatile("mtcr %0, " #reg "\n" : "=r" ((unsigned long)val));	\
} while (0);

mtcr(PHYS_OFFSET | 0xe, msa0)
mtcr(PHYS_OFFSET | 0x6, msa1)

+}
+
+__init void cpu_dt_probe(void)
+{
+	setup_cpu_msa();
+}
+
+static int c_show(struct seq_file *m, void *v)
+{
+	seq_printf(m, "C-SKY CPU : %s\n", CSKYCPU_DEF_NAME);
+	seq_printf(m, "revision  : 0x%08x\n", mfcr_cpuidrr());
+	seq_printf(m, "ccr reg   : 0x%08x\n", mfcr_ccr());
+	seq_printf(m, "ccr2 reg  : 0x%08x\n", mfcr_ccr2());
+	seq_printf(m, "hint reg  : 0x%08x\n", mfcr_hint());
+	seq_printf(m, "msa0 reg  : 0x%08x\n", mfcr_msa0());
+	seq_printf(m, "msa1 reg  : 0x%08x\n", mfcr_msa1());

Do these need to be exposed to userspace?

Does this arch support SMP? I see you don't log information per-cpu.

[...]

+#define THREADSIZE_MASK_BIT 13

You might want to define this as THREAD_SHIFT, and define THREAD_SIZE in terms
of it, so that they're guaranteed to be in sync

e.g. in your <asm/thread_info.h> have:

#define THREAD_SHIFT	13
#define THREAD_SIZE	(1 << THREAD_SHIFT)

[...]

+ENTRY(csky_systemcall)
+	SAVE_ALL_TRAP
+
+	psrset  ee, ie
+
+	/* Stack frame for syscall, origin call set_esp0 */
+	mov     r12, sp
+
+	bmaski  r11, 13
+	andn    r12, r11
+	bgeni   r11, 9
+	addi    r11, 32
+	addu    r12, r11
+	st      sp, (r12, 0)
+
+	lrw     r11, __NR_syscalls
+	cmphs   syscallid, r11                 /* Check nr of syscall */
+	bt      ret_from_exception
+
+	lrw     r13, sys_call_table
+	ixw     r13, syscallid                 /* Index into syscall table */
+	ldw     r11, (r13)               /* Get syscall function */
+	cmpnei  r11, 0                  /* Check for not null */
+	bf      ret_from_exception
+
+	mov     r9, sp				 /* Get task pointer */
+	bmaski  r10, THREADSIZE_MASK_BIT
+	andn    r9, r10                      /* Get thread_info */

If you have a spare register that you can point at the current task (or you
have preemption-safe percpu ops), I'd recommend moving the thread_info off of
the stack, and implementing THREAD_INFO_IN_TASK_STRUCT.

[...]

+ENTRY(csky_get_tls)
+	USPTOKSP
+
+	/* increase epc for continue */
+	mfcr	a0, epc
+	INCTRAP	a0
+	mtcr	a0, epc
+
+	/* get current task thread_info with kernel 8K stack */
+	bmaski   a0, (PAGE_SHIFT + 1)

For consistency, and in case you change your stack size in future, this should
be THREADSIZE_MASK_BIT.

[...]

+/*
+ * This routine handles page faults.  It determines the address,
+ * and the problem, and then passes it off to one of the appropriate
+ * routines.
+ */
+asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long write,
+                              unsigned long mmu_meh)
+{
+        struct vm_area_struct * vma = NULL;
+        struct task_struct *tsk = current;
+        struct mm_struct *mm = tsk->mm;
+        siginfo_t info;
+	int fault;
+	unsigned long address = mmu_meh & PAGE_MASK;
+
+        info.si_code = SEGV_MAPERR;
+
+        /*
+         * We fault-in kernel-space virtual memory on-demand. The
+         * 'reference' page table is init_mm.pgd.
+         *
+         * NOTE! We MUST NOT take any locks for this case. We may
+         * be in an interrupt or a critical region, and should
+         * only copy the information from the master page table,
+         * nothing more.
+         */
+        if (unlikely(address >= VMALLOC_START && address <= VMALLOC_END))
+                goto vmalloc_fault;

You might want to check if this was a user mode fault here, so that users can't trigger vmalloc faults.

Thanks,
Mark.

+vmalloc_fault:
+        {
+                /*
+                 * Synchronize this task's top level page-table
+                 * with the 'reference' page table.
+                 *
+                 * Do _not_ use "tsk" here. We might be inside
+                 * an interrupt in the middle of a task switch..
+                 */
+                int offset = __pgd_offset(address);
+                pgd_t *pgd, *pgd_k;
+                pud_t *pud, *pud_k;
+                pmd_t *pmd, *pmd_k;
+                pte_t *pte_k;
+
+                unsigned long pgd_base;
+		pgd_base = tlb_get_pgd();
+                pgd = (pgd_t *)pgd_base + offset;
+                pgd_k = init_mm.pgd + offset;
+
+                if (!pgd_present(*pgd_k))
+                        goto no_context;
+                set_pgd(pgd, *pgd_k);
+
+                pud = (pud_t *)pgd;
+                pud_k = (pud_t *)pgd_k;
+                if (!pud_present(*pud_k))
+                        goto no_context;
+
+                pmd = pmd_offset(pud, address);
+                pmd_k = pmd_offset(pud_k, address);
+                if (!pmd_present(*pmd_k))
+                        goto no_context;
+                set_pmd(pmd, *pmd_k);
+
+                pte_k = pte_offset_kernel(pmd_k, address);
+                if (!pte_present(*pte_k))
+                        goto no_context;
+                return;
+        }
+}
+
-- 
2.7.4