3:34 p.m.
On 03/25/2016 02:19 PM, Sam Ravnborg wrote:
Hi Dave.
A few commnets from reading the patch, as I have no knowledge of the code it patches.
And comments are biaes towards kernel ways of doing this.
Thanks for the review. I'll clean this up and resubmit.
Sam
On Wed, Mar 23, 2016 at 05:41:08PM -0500, Dave Kleikamp wrote:
> Based on original work by Karl Volz <karl.volz(a)oracle.com>
Should this patch follow the kernel standards, then
the description should be far more elaborated.
And I dunno if a s-o-b is appropriate too.
Fair enough. I could add a bigger description. I saw no other s-o-b
lines in the crash changesets, but I could put one in and let the
maintainer do what he wants with it.
> @@ -104,6 +104,7 @@ void add_extra_lib(char *);
> #undef X86_64
> #undef ARM
> #undef ARM64
> +#undef SPARC64
>
> #define UNKNOWN 0
> #define X86 1
> @@ -117,6 +118,7 @@ void add_extra_lib(char *);
> #define ARM 9
> #define ARM64 10
> #define MIPS 11
> +#define SPARC64 12
>
> #define TARGET_X86 "TARGET=X86"
> #define TARGET_ALPHA "TARGET=ALPHA"
> @@ -129,6 +131,7 @@ void add_extra_lib(char *);
> #define TARGET_ARM "TARGET=ARM"
> #define TARGET_ARM64 "TARGET=ARM64"
> #define TARGET_MIPS "TARGET=MIPS"
> +#define TARGET_SPARC64 "TARGET=SPARC64"
>
> #define TARGET_CFLAGS_X86 "TARGET_CFLAGS=-D_FILE_OFFSET_BITS=64"
> #define TARGET_CFLAGS_ALPHA "TARGET_CFLAGS="
> @@ -149,6 +152,7 @@ void add_extra_lib(char *);
> #define TARGET_CFLAGS_MIPS
"TARGET_CFLAGS=-D_FILE_OFFSET_BITS=64"
> #define TARGET_CFLAGS_MIPS_ON_X86
"TARGET_CFLAGS=-D_FILE_OFFSET_BITS=64"
> #define TARGET_CFLAGS_MIPS_ON_X86_64 "TARGET_CFLAGS=-m32
-D_FILE_OFFSET_BITS=64"
> +#define TARGET_CFLAGS_SPARC64 "TARGET_CFLAGS=-mcpu=v9 -fPIC
-fno-builtin"
Are you missing -m64 here? At least we use -m64 when building the kernel.
Now that I think about it. I probably have too much here. The installed
compiler should have the proper defaults for building user-space. I'm
sure this dates back to developing on an early toolchain.
> #define GDB_TARGET_DEFAULT "GDB_CONF_FLAGS="
> #define GDB_TARGET_ARM_ON_X86 "GDB_CONF_FLAGS=--target=arm-elf-linux"
> @@ -378,6 +382,9 @@ get_current_configuration(struct supported_gdb_version *sp)
> #ifdef __mips__
> target_data.target = MIPS;
> #endif
> +#if defined(__sparc__) && defined(__arch64__)
> + target_data.target = SPARC64;
> +#endif
In another place in the patch following is used to detect sparc64:
> +#ifdef __sparc_v9__
> +#define SPARC64
> +#endif
Looks strange that two different approaches are used.
Agreed. I think I'll use __sparc_v9__ in both places.
> +#ifdef NEED_ALIGNED_MEM_ACCESS
> +
> +#define DEF_LOADER(TYPE) \
> +static inline TYPE \
> +load_##TYPE (char *addr) \
> +{ \
> + TYPE ret; \
> + size_t i = sizeof(TYPE); \
> + while (i--) \
> + ((char *)&ret)[i] = addr[i]; \
> + return ret; \
> +}
> +
> +DEF_LOADER(int);
> +DEF_LOADER(uint);
> +DEF_LOADER(long);
> +DEF_LOADER(ulong);
> +DEF_LOADER(ulonglong);
> +DEF_LOADER(ushort);
> +DEF_LOADER(short);
> +typedef void *pointer_t;
> +DEF_LOADER(pointer_t);
> +
> +#define LOADER(TYPE) load_##TYPE
> +
> +#define INT(ADDR) LOADER(int) ((char *)(ADDR))
> +#define UINT(ADDR) LOADER(uint) ((char *)(ADDR))
> +#define LONG(ADDR) LOADER(long) ((char *)(ADDR))
> +#define ULONG(ADDR) LOADER(ulong) ((char *)(ADDR))
> +#define ULONGLONG(ADDR) LOADER(ulonglong) ((char *)(ADDR))
> +#define ULONG_PTR(ADDR) ((ulong *) (LOADER(pointer_t) ((char *)(ADDR))))
> +#define USHORT(ADDR) LOADER(ushort) ((char *)(ADDR))
> +#define SHORT(ADDR) LOADER(short) ((char *)(ADDR))
> +#define UCHAR(ADDR) *((unsigned char *)((char *)(ADDR)))
> +#define VOID_PTR(ADDR) ((void *) (LOADER(pointer_t) ((char *)(ADDR))))
> +
> +#else
> +
> #define INT(ADDR) *((int *)((char *)(ADDR)))
> #define UINT(ADDR) *((uint *)((char *)(ADDR)))
> #define LONG(ADDR) *((long *)((char *)(ADDR)))
> @@ -2195,6 +2246,8 @@ struct builtin_debug_table {
> #define UCHAR(ADDR) *((unsigned char *)((char *)(ADDR)))
> #define VOID_PTR(ADDR) *((void **)((char *)(ADDR)))
>
> +#endif /* NEED_ALIGNED_MEM_ACCESS */
> +
The NEED_ALIGNED_MEM_ACCESS is generic - and could have been
handled in a dedicated patch.
Makes sense.
>
> +#ifdef SPARC64
> +#define _64BIT_
> +#define MACHINE_TYPE "SPARC64"
> +
> +#define PTOV(X) \
> + ((unsigned long)(X)-(machdep->machspec->phys_offset)+(machdep->kvbase))
> +#define VTOP(X) \
> + ((unsigned long)(X)-(machdep->kvbase)+(machdep->machspec->phys_offset))
Some spaces would make wonders for readability.
agreed.
> +extern int sparc64_IS_VMALLOC_ADDR(ulong vaddr);
> +#define IS_VMALLOC_ADDR(X) sparc64_IS_VMALLOC_ADDR((ulong)(X))
> +#define PAGE_SHIFT (13)
> +#define PAGE_SIZE (1UL << PAGE_SHIFT)
> +#define PAGE_MASK (~(PAGE_SIZE-1))
> +#define PAGEBASE(X) (((ulong)(X)) & (ulong)machdep->pagemask)
> +#define THREAD_SIZE (2*PAGE_SIZE)
Spaces missing again.
okay
> +/* S3 Core
> + * Core 48-bit physical address supported.
> + * Bit 47 distinguishes memory or I/O. When set to "1" it is I/O.
> + */
> +#define PHYS_MASK_SHIFT (47)
> +#define PHYS_MASK (((1UL) << PHYS_MASK_SHIFT) - 1)
> +
> +typedef signed int s32;
> +
> +/*
> + * This next two defines are convenience defines for normal page table.
> + */
> +#define PTES_PER_PAGE (1UL << (PAGE_SHIFT - 3))
> +#define PTES_PER_PAGE_MASK (PTES_PER_PAGE - 1)
> +
> +/* 4-levels / 8K pages */
> +#define PG_LVL4_PDIRS_BITS (53)
> +#define PG_LVL4_PGDIR_SHIFT (43)
> +#define PG_LVL4_PTRS_PER_PGD (1024)
> +#define PG_LVL4_PUD_SHIFT (33)
> +#define PG_LVL4_PTRS_PER_PUD (1024)
> +#define PG_LVL4_PMD_SHIFT (23)
> +#define PG_LVL4_PTRS_PER_PMD (1024)
> +#define PG_LVL4_PTRS_PER_PTE (1UL << (PAGE_SHIFT-3))
If these definitions was a copy of the kernel definitions,
then it would be much easier to follow and check.
From the kernel:
/* PMD_SHIFT determines the size of the area a second-level page
* table can map
*/
#define PMD_SHIFT (PAGE_SHIFT + (PAGE_SHIFT-3))
#define PMD_SIZE (_AC(1,UL) << PMD_SHIFT)
#define PMD_MASK (~(PMD_SIZE-1))
#define PMD_BITS (PAGE_SHIFT - 3)
/* PUD_SHIFT determines the size of the area a third-level page
* table can map
*/
#define PUD_SHIFT (PMD_SHIFT + PMD_BITS)
#define PUD_SIZE (_AC(1,UL) << PUD_SHIFT)
#define PUD_MASK (~(PUD_SIZE-1))
#define PUD_BITS (PAGE_SHIFT - 3)
/* PGDIR_SHIFT determines what a fourth-level page table entry can map */
#define PGDIR_SHIFT (PUD_SHIFT + PUD_BITS)
#define PGDIR_SIZE (_AC(1,UL) << PGDIR_SHIFT)
#define PGDIR_MASK (~(PGDIR_SIZE-1))
#define PGDIR_BITS (PAGE_SHIFT - 3)
I'll redo those.
> +/* Down one huge page */
> +#define PG_LVL4_SPARC64_USERSPACE_TOP (-(1UL << 23UL))
If 23 is PG_LVL4_PMD_SHIFT - then use this constant.
yep
> +#define PAGE_PMD_HUGE (0x0100000000000000UL)
> +#define HPAGE_SHIFT (23)
> +
> +/* These are for SUN4V. */
> +#define _PAGE_VALID (0x8000000000000000UL)
> +#define _PAGE_NFO_4V (0x4000000000000000UL)
> +#define _PAGE_MODIFIED_4V (0x2000000000000000UL)
> +#define _PAGE_ACCESSED_4V (0x1000000000000000UL)
> +#define _PAGE_READ_4V (0x0800000000000000UL)
> +#define _PAGE_WRITE_4V (0x0400000000000000UL)
> +#define _PAGE_PADDR_4V (0x00FFFFFFFFFFE000UL)
> +#define _PAGE_PFN_MASK (_PAGE_PADDR_4V)
> +#define _PAGE_P_4V (0x0000000000000100UL)
> +#define _PAGE_EXEC_4V (0x0000000000000080UL)
> +#define _PAGE_W_4V (0x0000000000000040UL)
> +#define _PAGE_PRESENT_4V (0x0000000000000010UL)
> +#define _PAGE_SZALL_4V (0x0000000000000007UL)
> +/* There are other page sizes. Some supported. */
> +#define _PAGE_SZ4MB_4V (0x0000000000000003UL)
> +#define _PAGE_SZ512K_4V (0x0000000000000002UL)
> +#define _PAGE_SZ64K_4V (0x0000000000000001UL)
> +#define _PAGE_SZ8K_4V (0x0000000000000000UL)
> +
> +#define SPARC64_MODULES_VADDR (0x0000000010000000UL)
> +#define SPARC64_MODULES_END (0x00000000f0000000UL)
> +#define LOW_OBP_ADDRESS (0x00000000f0000000UL)
> +#define HI_OBP_ADDRESS (0x0000000100000000UL)
These two are not used and can be deleted.
Did not check all of tham but OBP stuff triggered me.
Missed these when cleaning out obsolete code.
> +#define SPARC64_VMALLOC_START (0x0000000100000000UL)
> +
> +#define SPARC64_STACK_SIZE 0x4000
> +#define PTRS_PER_PGD (1024)
> +
> +/* sparsemem */
> +#define _SECTION_SIZE_BITS 30
> +#define _MAX_PHYSMEM_BITS_LVL4 53
> +
> +#define STACK_BIAS 2047
> +
> +struct machine_specific {
> + ulong flags;
> + ulong userspace_top;
> + ulong page_offset;
> + ulong vmalloc_start;
> + ulong vmalloc_end;
> + ulong modules_vaddr;
> + ulong modules_end;
> + ulong phys_offset;
> + ulong __exception_text_start;
> + ulong __exception_text_end;
> + struct pt_regs *panic_task_regs;
> + ulong pte_protnone;
> + ulong pte_file;
> + uint pgd_shift;
> + uint pud_shift;
> + uint pmd_shift;
> + uint ptes_per_pte;
> +};
Most of the values in machine_specific are in reality constants.
Do we really gain anything colleting the various constants in a struct?
Look like this may have been copied from an arch where much more is dynamic.
At one time it made sense. I had crash supporting older kernels with
different page cache layouts, but I didn't think it would be worth the
complexity to leave all that in there, so I pulled a lot out. I should
go a little further to simplify it. Hopefully, we're done messing with
the page table format.
> +
> +#define TIF_SIGPENDING (2)
> +#define SWP_OFFSET(E) ((E) >> (13UL+8UL))
> +#define SWP_TYPE(E) (((E) >> (13UL)) & 0xffUL)
> +#define __swp_type(E) SWP_TYPE(E)
> +#define __swp_offset(E) SWP_OFFSET(E)
The SWP_OFFSET indirection is just obscufation - should be dropped.
Same with SWP_TYPE.
Maybe this is needed in generic code - but at least not in this patch.
It's this way for all the architectures, so I followed suit.
And the 13UL is PAGE_SHIFT hardcoded - replace this with PAGE_SHIFT.
That I will fix.
> /*
> + * sparc64.c
> + */
> +#ifdef SPARC64
> +void sparc64_init(int);
> +void sparc64_dump_machdep_table(ulong);
> +int sparc64_vmalloc_addr(ulong);
> +#define display_idt_table() \
> + error(FATAL, "The -d option is not applicable to sparc64.\n")
Is a macro really needed?
A static inline is preferable due to better typechecks etc.
Not that there are much to check here - but in general.
This is copied from what other architectures do.
> diff --git a/sparc64.c b/sparc64.c
> new file mode 100644
> index 0000000..7d26137
> --- /dev/null
> +++ b/sparc64.c
> @@ -0,0 +1,1186 @@
> +#ifdef SPARC64
> +
> +#include "defs.h"
> +#include <stdio.h>
> +#include <elf.h>
> +#include <asm/ptrace.h>
> +#include <linux/const.h>
> +
> +/* Things not done, debugged or tested at this point:
> + * 1) uvtop swap handling
> + * 2) uniform page table layout - like we had in 1st quarter of 2013
> + * 3) and whatever can't be thought of.
> + */
> +#define true (1)
> +#define false (0)
Irk - this should be provided by compiler/glibc.
The rest of the code uses TRUE and FALSE. I don't know we do this in
lower case. I'm going to change it to be consistent.
> +static const unsigned long not_valid_pte = ~0UL;
> +static struct machine_specific sparc64_machine_specific;
> +static unsigned long sparc64_ksp_offset;
> +#define MAGIC_TT (0x1ff)
I see this used in places like this:
(regs->magic & ~MAGIC_TT) != PT_REGS_MAGIC
But the name MAGIC_TT does not give much clue what this is for.
I'll come up with a better name.
And the mix of defines, then prototypes, then (even no empty line) a
define again
does not increase readability.
Agreed.
> +static uint page_size(void)
> +{
> + return machdep->pagesize;
> +}
I cannot see the vaule of hiding PAGE_SIZE first in a struct, and now in a static
function.
Unless this is required by code I cannot see in the patch of course.
The struct is used in generic code, but this static function is only
used once and needs to go.
> +static void
> +sparc64_display_machine_stats(void)
> +{
> + int c;
> + struct new_utsname *uts;
> + char buf[BUFSIZE];
> + ulong mhz;
> +
> + uts = &kt->utsname;
> +
> + fprintf(fp, " MACHINE TYPE: %s\n", uts->machine);
> + fprintf(fp, " MEMORY SIZE: %s\n", get_memory_size(buf));
> + fprintf(fp, " CPUS: %d\n", kt->cpus);
> + fprintf(fp, " PROCESSOR SPEED: ");
> + if ((mhz = machdep->processor_speed()))
> + fprintf(fp, "%ld Mhz\n", mhz);
> + else
> + fprintf(fp, "(unknown)\n");
> + fprintf(fp, " HZ: %d\n", machdep->hz);
> + fprintf(fp, " PAGE SIZE: %d\n", PAGESIZE());
PAGESIZE() is a function call?
Yes, in generic code. Which really makes the above page_size()
redundant. Whoever began this port must have disliked all caps.
> + fprintf(fp, " KERNEL VIRTUAL BASE: %lx\n", machdep->kvbase);
> + fprintf(fp, " KERNEL VMALLOC BASE: %lx\n", vt->vmalloc_start);
> + fprintf(fp, " KERNEL MODULES BASE: %lx\n", MODULES_VADDR);
> + fprintf(fp, " KERNEL STACK SIZE: %ld\n", STACKSIZE());
STACKSIZE is not defined. Assuming it comes from the generic part somehow.
yes
> +static void sparc64_display_memmap(void)
> +{
> + unsigned long iomem_resource;
> + unsigned long resource;
> + unsigned long start, end, nameptr;
> + int size = STRUCT_SIZE("resource");
> + char *buf = GETBUF(size);
> + char name[32];
In the kernel work we try to avoid mixing local variable definitions
and assignments - as code like the above is a little hard to read.
Good point. I just realized that there's a memory leak here since GETBUF
makes an allocation that is not freed.
> +static void sparc64_cmd_mach(void)
> +{
> + int c;
> + int mflag = 0;
> +
> + while ((c = getopt(argcnt, args, "cm")) != EOF) {
> + switch (c) {
> + case 'm':
> + mflag++;
> + sparc64_display_memmap();
> + break;
> + case 'c':
> + fprintf(fp, "SPARC64: '-%c' option is not supported\n",
> + c);
> + break;
> + default:
> + argerrs++;
> + break;
> + }
> + }
Was this a better place to handle that -d flag is not supported?
This is another case where I duplicated what other architectures do. It
looks like the -d or -x flags only make sense with the -c flag, which is
not supported (as yet). Of course, if I can make the sparc code a little
clearer, I don't have follow the other arches to the letter.
> +static int sparc64_phys_live_valid(unsigned long paddr)
> +{
> + unsigned int nr;
> + int rc = false;
> +
> + for (nr = 0U; nr != nr_phys_ranges; nr++) {
What is gained by 0U versus 0 here?
Nothing.
> + if (paddr >= phys_ranges[nr].start &&
> + paddr < phys_ranges[nr].end) {
> + rc = true;
> + break;
> + }
> + }
> + return rc;
> +}
> +
> +static int sparc64_phys_kdump_valid(unsigned long paddr)
> +{
> + return true;
> +}
> +
> +static void sparc6_phys_base_live_limits(void)
> +{
> + if (nr_phys_ranges >= NR_PHYS_RANGES)
> + error(FATAL, "sparc6_phys_base_live_limits: "
> + "NR_PHYS_RANGES exceeded.\n");
> + else if (nr_kimage_ranges >= NR_IMAGE_RANGES)
> + error(FATAL, "sparc6_phys_base_live_limits: "
> + "NR_IMAGE_RANGES exceeded.\n");
> +}
> +
> +static void sparc64_phys_base_live_valid(void)
> +{
> + if (!nr_phys_ranges)
> + error(FATAL, "No physical memory ranges.");
> + else if (!nr_kimage_ranges)
> + error(FATAL, "No vmlinux memory ranges.");
> +}
> +
> +static void sparc64_phys_base_live(void)
> +{
> + char line[BUFSIZE];
> + FILE *fp;
> +
> + fp = fopen("/proc/iomem", "r");
> + if (fp == NULL)
> + error(FATAL, "Can't open /proc/iomem. We can't proceed.");
> +
> + while (fgets(line, sizeof(line), fp) != 0) {
> + unsigned long start, end;
> + int count, consumed;
> + char *ch;
> +
> + sparc6_phys_base_live_limits();
If we exceed NR_PHYS_RANGES or NR_IMAGE_RANGES then we would fault below,
because the execution continue. (If error() returns that is).
Error handling looks a little ackward.
error(FATAL, ...) does not return.
> +
> +static int sparc64_is_linear_mapped(unsigned long vaddr)
> +{
> + int rc = 0;
> +
> + if ((vaddr & PAGE_OFFSET) == PAGE_OFFSET)
> + rc = 1;
> + return rc;
Just do a simple:
return (vaddr & PAGE_OFFSET) == PAGE_OFFSET;
Yeah. I'm thinking this evolved from something more complicated.
> +static unsigned long fetch_page_table_level(unsigned long pte_kva,
> + unsigned long vaddr, unsigned int shift,
> + unsigned int mask, const char *name,
> + int verbose)
> +{
Coding style nit.
In some places the return type is on a separate line,
other places on the same line as the function name.
And the extra parameters would be better located aligned with the first
parameter on the first line (using TABS and SPACES as appropriate).
But all this is from being used to kernel style.
General note that apply to the whole patch.
It seems that most of the crash code puts the return type on a separate
line, but it's not universally enforced. I'll fix it to be consistent.
> +int sparc64_IS_VMALLOC_ADDR(ulong vaddr)
> +{
> + int ret = (vaddr >= machdep->machspec->vmalloc_start &&
> + vaddr < machdep->machspec->vmalloc_end);
> +
> + return ret;
This could be a one-liner.
And if you insist on using a local variable then name it rc as in other palces.
okay
> +}
> +
> +static int sparc64_dis_filter(ulong vaddr, char *inbuf, unsigned int radix)
> +{
> + return false;
> +}
Two tab indent?
oops
> +{
> + struct {struct sparc_stackf sf; struct pt_regs pr; }
> + exception_frame_data;
Does this really need to be inside the function body?
No. It's pretty ugly there.
> + unsigned long exception_frame = bt->stacktop;
> + unsigned long first_frame;
> + struct reg_window one_down;
> + int rc;
> +
> + exception_frame = exception_frame - TRACEREG_SZ - STACKFRAME_SZ;
> + rc = readmem(exception_frame, KVADDR, &exception_frame_data,
> + sizeof(exception_frame_data), "EF fetch.", RETURN_ON_ERROR);
> + if (!rc)
> + goto out;
> + if (exception_frame_data.pr.magic != 0x57ac6c00)
Please use PT_REGS_MAGIC. Do you need to mask out the lower 9 bits as doen in other
places?
It looks like it.
> + goto out;
> + first_frame = exception_frame - sizeof(struct reg_window);
> +
> + rc = readmem(first_frame, KVADDR, &one_down, sizeof(struct reg_window),
> + "Stack fetch.", RETURN_ON_ERROR);
> + if (!rc)
> + goto out;
> + /* Extra arguments. */
> + first_frame = first_frame - (6 * 8);
Where did 6 * 8 suddenly come from?
I'm not sure. I'm going to have to figure out what this function is
supposed to do. :-)
> +
> + rc = readmem(first_frame, KVADDR, &one_down, sizeof(struct reg_window),
> + "Stack fetch.", RETURN_ON_ERROR);
> + if (!rc)
> + goto out;
> +out:
> + return rc;
> +}
> +
> +/* Need to handle hardirq and softirq stacks. */
> +static int kstack_valid(struct bt_info *bt, unsigned long sp)
> +{
> + unsigned long thread_info = SIZE(thread_info);
> + unsigned long base = bt->stackbase + thread_info;
> + unsigned long top = bt->stacktop - sizeof(struct sparc_stackf) -
> + sizeof(struct pt_regs);
> + int rc = false;
> +
> + if (sp & (16U - 1))
> + goto out;
Where did 16U come from?
I'm assuming valid stack frames must be 16-byte aligned.
> +
> + if ((sp >= base) && (sp <= top))
> + rc = true;
> +out:
> + return rc;
> +}
> +
> +static void sparc64_print_eframe(struct bt_info *bt, unsigned long stack_top)
> +{
> + struct {struct sparc_stackf sf; struct pt_regs pr; } k_entry;
Duplicated - was also in previous function.
Yeah. will fix.
> +static int sparc64_is_vmalloc_mapped(unsigned long vaddr)
> +{
> + struct machine_specific *ms = &sparc64_machine_specific;
> + int rc = 0;
> +
> + /* We exclude OBP range whose TTEs were captured by kernel in
> + * early boot. It is possible to fetch these but for what purpose?
> + */
Comment says OBP but code used modules_vaddr / modules_end, vmalloc_start, vmalloc_end
That comment clearly doesn't belong.
> + if ((vaddr >= ms->modules_vaddr && vaddr < ms->modules_end) ||
> + (vaddr >= ms->vmalloc_start && vaddr < ms->vmalloc_end))
> + rc = 1;
> + return rc;
> +}
> +
> +static int sparc64_is_kvaddr(ulong vaddr)
> +{
> + int rc = kimage_va_range(vaddr);
> +
> + if (rc)
> + goto out;
> + rc = sparc64_is_linear_mapped(vaddr);
> + if (rc)
> + goto out;
> + rc = sparc64_is_vmalloc_mapped(vaddr);
> +out:
> + return rc;
> +}
Use goto for error handling.
The above is not easy to read/parse.
I may just replace this with:
return kimage_va_range(vaddr) ||
sparc64_is_linear_mapped(vaddr) ||
sparc64_is_vmalloc_mapped(vaddr);
> +
> +static int sparc64_kvtop(struct task_context *tc, ulong vaddr,
> + physaddr_t *paddr, int verbose)
> +{
> + unsigned long phys_addr;
> + int rc = false;
> +
> + if (kimage_va_range(vaddr))
> + phys_addr = kimage_va_translate(vaddr);
> + else if (sparc64_is_vmalloc_mapped(vaddr)) {
> + phys_addr = sparc64_vmalloc_translate(vaddr, verbose);
> + if (phys_addr == not_valid_pte)
> + goto out;
> + } else if (sparc64_is_linear_mapped(vaddr))
> + phys_addr = sparc64_linear_translate(vaddr);
> + else {
> + error(WARNING,
> + "This is an invalid kernel virtual address=0x%lx.",
> + vaddr);
> + goto out;
> + }
Coding style nit.
If you need {} then use it for all statements, also single liners.
okay
So use:
if (foo()) {
bar();
} else {
baz();
baz2();
}
> diff --git a/task.c b/task.c
> index 7b01951..f21d0f8 100644
> --- a/task.c
> +++ b/task.c
> @@ -2368,7 +2369,11 @@ store_context(struct task_context *tc, ulong task, char *tp)
>
> tc->pid = (ulong)(*pid_addr);
> strlcpy(tc->comm, comm_addr, TASK_COMM_LEN);
> - tc->processor = *processor_addr;
> +#ifdef SPARC64
> + tc->processor = *(unsigned short *)processor_addr;
> +#else
> + tc->processor = *processor_addr;
> +#endif
This looks like it is papering over something that needs a proper fix.
And it does not look sparc64 specific, so should be a separate patch.
Only sparc has a 16-bit thread_info->cpu. A generic fix would be little
more complicated, but would avoid the ifdef.
> tc->ptask = *parent_addr;
> tc->mm_struct = *mm_addr;
> tc->task = task;
> @@ -5266,8 +5271,15 @@ task_flags(ulong task)
>
> fill_task_struct(task);
>
> - flags = tt->last_task_read ?
> - ULONG(tt->task_struct + OFFSET(task_struct_flags)) : 0;
> + if (tt->last_task_read) {
> + if (SIZE(task_struct_flags) == sizeof(unsigned int))
> + flags = UINT(tt->task_struct +
> + OFFSET(task_struct_flags));
> + else
> + flags = ULONG(tt->task_struct +
> + OFFSET(task_struct_flags));
> + } else
> + flags = 0;
This also looks like it is paerign over something that should see a proper fix.
And it does not look sparc64 specific, so should be in a separate patch.
It's not sparc64-specific, but fixes a generic bug I found with sparc64.
This is the proper fix because crash can't hard-code the type of
task_struct->flags.