Re: [RFC 7/7] Add S390 support for linux-kernel target

Philipp Rudo
2017-01-13T11:46:30Z

Hi Luis

On Thu, 12 Jan 2017 11:08:59 -0600
Luis Machado <lgustavo@codesourcery.com> wrote:

> On 01/12/2017 05:32 AM, Philipp Rudo wrote:
> > Implement functions for the linux-kernel target hooks.
> >
> > gdb/ChangeLog:
> >
> > * s390-linux-tdep.h: Define macros for address translation.
> > * s390-linux-tdep.c: Include gdbthread.h and lk-low.h.
> > (s390_gregmap_lk): New array.
> > (s390_gregset_lk): New type.
> > (s390_lk_get_registers, s390_lk_get_percpu_offset)
> > (s390_lk_map_running_task_to_cpu, s390_lk_is_kvaddr)
> > (s390_lk_read_table_entry, s390_lk_vtop,
> > s390_lk_init_private): New function.
> > (s390_gdbarch_init): Adjust.
> > ---
> > gdb/s390-linux-tdep.c | 270
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> > gdb/s390-linux-tdep.h | 62 ++++++++++++ 2 files changed, 332
> > insertions(+)
> >
> > diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
> > index 054c6d5..f436a74 100644
> > --- a/gdb/s390-linux-tdep.c
> > +++ b/gdb/s390-linux-tdep.c
>
> I wonder if we should put the new lk-specific code in its own file,
> like s390-lk-tdep.c or something. It would make thing a bit more
> clear and would pollute the current s390-linux-tdep.c file?

It probably is better. I'll talk to Andreas as S390 maintainer what he
prefers.

> > @@ -29,6 +29,7 @@
> > #include "target.h"
> > #include "gdbcore.h"
> > #include "gdbcmd.h"
> > +#include "gdbthread.h"
> > #include "objfiles.h"
> > #include "floatformat.h"
> > #include "regcache.h"
> > @@ -49,6 +50,8 @@
> > #include "auxv.h"
> > #include "xml-syscall.h"
> >
> > +#include "lk-low.h"
> > +
> > #include "stap-probe.h"
> > #include "ax.h"
> > #include "ax-gdb.h"
> > @@ -783,6 +786,14 @@ static const struct regcache_map_entry
> > s390_gregmap[] = { 0 }
> > };
> >
> > +static const struct regcache_map_entry s390_gregmap_lk[] =
> > + {
> > + { 10, S390_R6_REGNUM }, /* r0-r5 volatile */
> > + { -2, REGCACHE_MAP_SKIP, 8 },
> > + { 1, S390_PSWA_REGNUM }, /* Use r14 for PSWA. */
> > + { 0 }
> > + };
> > +
> > static const struct regcache_map_entry s390_regmap_cr [] =
> > {
> > { 16, S390_CR0_REGNUM, 8 },
> > @@ -891,6 +902,12 @@ const struct regset s390_gregset = {
> > regcache_collect_regset
> > };
> >
> > +const struct regset s390_gregset_lk = {
> > + s390_gregmap_lk,
> > + regcache_supply_regset,
> > + regcache_collect_regset
> > +};
> > +
> > const struct regset s390_cr_regset = {
> > s390_regmap_cr,
> > regcache_supply_regset,
> > @@ -1080,6 +1097,256 @@ s390_core_read_description (struct gdbarch
> > *gdbarch, }
> > }
> >
> > +/* Funktion for Linux kernel target get_registers hook. Supplies
> > gprs for
>
> "Function..."

fixed.

>
> > + task TASK to REGCACHE. Uses r14 (back jump address) as current
> > pswa. */ +
> > +void
> > +s390_lk_get_registers (CORE_ADDR task, struct target_ops *target,
> > + struct regcache *regcache, int regnum)
> > +{
> > + const struct regset *regset;
> > + CORE_ADDR ksp, gprs, pswa;
> > + gdb_byte *buf;
> > + size_t size;
> > + struct cleanup *old_chain;
> > +
> > + regset = &s390_gregset_lk;
> > +
> > + ksp = lk_read_addr (task + LK_OFFSET (task_struct, thread)
> > + + LK_OFFSET (thread_struct, ksp));
> > + gprs = ksp + LK_OFFSET (stack_frame, gprs);
> > + size = FIELD_SIZE (LK_FIELD (stack_frame, gprs));
> > +
>
> Do we have a reasonable MAX for this size? If so, we may try to have
> a static array instead of allocating and deallocating in such a short
> time. Just a suggestion.

You are right this isn't perfect. Changed it to

gdb_byte buf[80]; /* 80 = 10 (#registers saved) * 8 (64 bit width) */

The number of registers never changed since there is a git history for
the kernel tree so this should be pretty save.

> > + buf = XCNEWVEC (gdb_byte, size);
> > + old_chain = make_cleanup (xfree, buf);
> > + read_memory (gprs, buf, size);
> > + regset->supply_regset (regset, regcache, -1, buf, size);
> > + do_cleanups (old_chain);
> > +}
> > +
> > +/* Function for Linux kernel target get_percpu_offset hook.
> > Returns the
>
> Two spaces after period.

fixed.

> > + percpu_offset from lowcore for cpu CPU. */
> > +
> > +CORE_ADDR
> > +s390_lk_get_percpu_offset (int cpu)
>
> Is cpu ever negative? If not, make it unsigned?

No it can't be negative. While looking into it i found that there are
more variables that could be made unsigned, like the size of an bitmap.
Will fix for v2.

> > +{
> > + CORE_ADDR lowcore_ptr, lowcore;
> > + int ptr_len = lk_builtin_type_size (unsigned_long);
> > +
> > + lowcore_ptr = LK_ADDR (lowcore_ptr) + (ptr_len * cpu);
> > + lowcore = lk_read_addr (lowcore_ptr);
> > +
> > + return lk_read_addr (lowcore + LK_OFFSET (lowcore,
> > percpu_offset)); +}
> > +
> > +/* Function for Linux kernel target map_running_task_to_cpu hook.
> > */ +
> > +int
> > +s390_lk_map_running_task_to_cpu (struct thread_info *ti)
> > +{
> > + struct regcache *regcache;
> > + enum register_status reg_status;
> > + CORE_ADDR lowcore;
> > + int cpu;
> > +
> > + regcache = get_thread_regcache (ti->ptid);
> > + reg_status = regcache_raw_read_unsigned (regcache,
> > S390_PREFIX_REGNUM,
> > + (ULONGEST *) &lowcore);
> > + if (reg_status != REG_VALID)
> > + error (_("Could not find prefix register for thread with pid
> > %d, lwp %li."),
> > + ti->ptid.pid, ti->ptid.lwp);
> > +
> > + cpu = lk_read_int (lowcore + LK_OFFSET (lowcore, cpu_nr));
> > +
> > + return cpu;
> > +}
> > +
> > +/* Funktion for Linux kernel target is_kvaddr hook. */
>
> "Function..."

fixed.

> > +
> > +int
> > +s390_lk_is_kvaddr (CORE_ADDR addr)
> > +{
> > + return addr >= LK_ADDR (high_memory);
> > +}
> > +
> > +/* Read table entry from TABLE at offset OFFSET. Helper for
> > s390_lk_vtop. */ +
> > +static inline ULONGEST
> > +s390_lk_read_table_entry (CORE_ADDR table, ULONGEST offset)
> > +{
> > + return lk_read_ulong (table + offset * lk_builtin_type_size
> > (unsigned_long)); +}
> > +
> > +/* Function for Linux kernel target vtop hook. Assume 64 bit
> > addresses. */ +
> > +CORE_ADDR
> > +s390_lk_vtop (CORE_ADDR table, CORE_ADDR vaddr)
> > +{
> > + ULONGEST entry, offset;
> > + CORE_ADDR paddr;
> > + int table_type;
> > +
> > + /* Read first entry in table to get its type. As the Table-Type
> > bits are
> > + the same in every table assume Region1-Table. */
> > + entry = s390_lk_read_table_entry (table, 0);
> > + table_type = (entry & S390_LK_RFTE_TT) >> 2;
> > +
> > + switch (table_type)
> > + {
> > + case S390_LK_DAT_TT_REGION1:
> > + {
> > + offset = (vaddr & S390_LK_VADDR_RFX) >> 53;
> > + entry = s390_lk_read_table_entry (table, offset);
> > +
> > + /* Do sanity checks. */
> > + if (!entry)
> > + warning (_("Trying to translate address %#lx with
> > empty region-first-table entry."),
> > + vaddr);
>
> Use paddress (...) to print the hex address of vaddr or phex (...).
> More occurrences of this throughout the patch.

I'm using phex (...) now. Also fixed in the other files.

> > + else if ((entry & S390_LK_RFTE_TT) >> 2 !=
> > S390_LK_DAT_TT_REGION1)
> > + warning (_("Trying to translate address %#lx with
> > corrupt table type in region-first-table entry."),
> > + vaddr);
> > + else if (entry & S390_LK_RFTE_I)
> > + warning (_("Translating address %#lx with invalid bit
> > set at region-first-table entry."),
> > + vaddr);
> > +
> > + table = entry & S390_LK_RFTE_O;
> > + }
> > + /* fall through */
> > + case S390_LK_DAT_TT_REGION2:
> > + {
> > + offset = (vaddr & S390_LK_VADDR_RSX) >> 42;
> > + entry = s390_lk_read_table_entry (table, offset);
> > +
> > + /* Do sanity checks. */
> > + if (!entry)
> > + warning (_("Trying to translate address %#lx with
> > empty region-second-table entry."),
> > + vaddr);
> > + else if ((entry & S390_LK_RSTE_TT) >> 2 !=
> > S390_LK_DAT_TT_REGION2)
> > + warning (_("Trying to translate address %#lx with
> > corrupt table type in region-second-table entry."),
> > + vaddr);
> > + else if (entry & S390_LK_RSTE_I)
> > + warning (_("Translating address %#lx with invalid bit
> > set at region-second-table entry."),
> > + vaddr);
> > +
> > + table = entry & S390_LK_RSTE_O;
> > + }
> > + /* fall through */
> > + case S390_LK_DAT_TT_REGION3:
> > + {
> > + offset = (vaddr & S390_LK_VADDR_RTX) >> 31;
> > + entry = s390_lk_read_table_entry (table, offset);
> > +
> > + /* Do sanity checks. */
> > + if (!entry)
> > + warning (_("Trying to translate address %#lx with
> > empty region-third-table entry."),
> > + vaddr);
> > + else if ((entry & S390_LK_RTTE_TT) >> 2 !=
> > S390_LK_DAT_TT_REGION3)
> > + warning (_("Trying to translate address %#lx with
> > corrupt table type in region-third-table entry."),
> > + vaddr);
> > + else if (entry & S390_LK_RTTE_I)
> > + warning (_("Translating address %#lx with invalid bit
> > set at region-third-table entry."),
> > + vaddr);
> > +
> > + /* Check for huge page. */
> > + if (entry & S390_LK_RTTE_FC)
> > + {
> > + paddr = ((entry & S390_LK_RTTE_RFAA)
> > + + (vaddr & ~S390_LK_RTTE_RFAA));
> > + return paddr;
> > + }
> > +
> > + table = entry & S390_LK_RTTE_O;
> > + }
> > + /* fall through */
> > + case S390_LK_DAT_TT_SEGMENT:
> > + {
> > + offset = (vaddr & S390_LK_VADDR_SX) >> 20;
> > + entry = s390_lk_read_table_entry (table, offset);
> > +
> > + /* Do sanity checks. */
> > + if (!entry)
> > + warning (_("Trying to translate address %#lx with
> > empty segment-table entry."),
> > + vaddr);
> > + else if ((entry & S390_LK_STE_TT) >> 2 !=
> > S390_LK_DAT_TT_SEGMENT)
> > + warning (_("Trying to translate address %#lx with
> > corrupt table type in segment-table entry."),
> > + vaddr);
> > + else if (entry & S390_LK_STE_I)
> > + warning (_("Translating address %#lx with invalid bit
> > set at segment-table entry."),
> > + vaddr);
> > +
> > + /* Check for large page. */
>
> Is large different from huge?

In principle its the same. Its just a nomenclature to distinguish 1 MB
and 2 GB pages.

> > + if (entry & S390_LK_STE_FC)
> > + {
> > + paddr = ((entry & S390_LK_STE_SFAA)
> > + + (vaddr & ~S390_LK_STE_SFAA));
> > + return paddr;
> > + }
> > +
> > + table = entry & S390_LK_STE_O;
> > + break;
> > + }
> > + } /* switch (table_type) */
> > +
> > + offset = (vaddr & S390_LK_VADDR_PX) >> 12;
> > + entry = s390_lk_read_table_entry (table, offset);
> > +
> > + /* Do sanity checks. */
> > + if (!entry)
> > + warning (_("Trying to translate address %#lx with empty
> > page-table entry."),
> > + vaddr);
> > + else if (entry & S390_LK_PTE_I)
> > + warning (_("Translating address %#lx with invalid bit set at
> > page-table entry."),
> > + vaddr);
> > +
> > + paddr = ((entry & S390_LK_PTE_PFAA) + (vaddr &
> > ~S390_LK_PTE_PFAA)); +
> > + return paddr;
> > +}
> > +
> > +/* Function for Linux kernel target get_module_text_offset hook.
> > */ +
> > +CORE_ADDR
> > +s390_lk_get_module_text_offset (CORE_ADDR mod)
> > +{
> > + CORE_ADDR offset, mod_arch;
> > +
> > + mod_arch = mod + LK_OFFSET (module, arch);
> > + offset = lk_read_ulong (mod_arch + LK_OFFSET (mod_arch_specific,
> > got_size));
> > + offset += lk_read_ulong (mod_arch + LK_OFFSET
> > (mod_arch_specific, plt_size)); +
> > + return offset;
> > +}
> > +
> > +/* Initialize s390 dependent private data for linux kernel
> > target. */
>
> Newline between comment and function declaration.

fixed.

> > +void
> > +s390_lk_init_private (struct gdbarch *gdbarch)
> > +{
> > + LK_DECLARE_FIELD (stack_frame, gprs);
> > +
> > + LK_DECLARE_FIELD (thread_struct, ksp);
> > +
> > + LK_DECLARE_STRUCT_ALIAS (_lowcore, lowcore); /* linux -4.4 */
> > + LK_DECLARE_STRUCT_ALIAS (lowcore, lowcore); /* linux 4.5+ */
> > + if (LK_STRUCT (lowcore) == NULL)
> > + error (_("Could not find struct lowcore. Abort."));
>
> Two spaces after period. Also, "Aborting" instead of "Abort"?

fixed and using Aborting now. Also changed in the other files.

> > + LK_DECLARE_FIELD (lowcore, percpu_offset);
> > + LK_DECLARE_FIELD (lowcore, current_pid);
> > + LK_DECLARE_FIELD (lowcore, cpu_nr);
> > +
> > + LK_DECLARE_FIELD (mod_arch_specific, got_size);
> > + LK_DECLARE_FIELD (mod_arch_specific, plt_size);
> > +
> > + LK_DECLARE_ADDR (lowcore_ptr);
> > + LK_DECLARE_ADDR (high_memory);
> > +
> > + LK_HOOK->get_registers = s390_lk_get_registers;
> > + LK_HOOK->is_kvaddr = s390_lk_is_kvaddr;
> > + LK_HOOK->vtop = s390_lk_vtop;
> > + LK_HOOK->get_percpu_offset = s390_lk_get_percpu_offset;
> > + LK_HOOK->map_running_task_to_cpu =
> > s390_lk_map_running_task_to_cpu;
> > + LK_HOOK->get_module_text_offset = s390_lk_get_module_text_offset;
> > +}
> > +
> >
> > /* Decoding S/390 instructions. */
> >
> > @@ -8220,6 +8487,9 @@ s390_gdbarch_init (struct gdbarch_info info,
> > struct gdbarch_list *arches) set_gdbarch_process_record (gdbarch,
> > s390_process_record); set_gdbarch_process_record_signal (gdbarch,
> > s390_linux_record_signal);
> >
> > + /* Support linux kernel debugging. */
> > + set_gdbarch_lk_init_private (gdbarch, s390_lk_init_private);
> > +
> > s390_init_linux_record_tdep (&s390_linux_record_tdep,
> > ABI_LINUX_S390); s390_init_linux_record_tdep
> > (&s390x_linux_record_tdep, ABI_LINUX_ZSERIES);
> >
> > diff --git a/gdb/s390-linux-tdep.h b/gdb/s390-linux-tdep.h
> > index fdb3c3d..8a16b0b 100644
> > --- a/gdb/s390-linux-tdep.h
> > +++ b/gdb/s390-linux-tdep.h
> > @@ -238,4 +238,66 @@ extern struct target_desc
> > *tdesc_s390x_te_linux64; extern struct target_desc
> > *tdesc_s390x_vx_linux64; extern struct target_desc
> > *tdesc_s390x_tevx_linux64;
> >
> > +/* Definitions for address translation. */
> > +/* DAT Table types. */
> > +#define S390_LK_DAT_TT_REGION1 3
> > +#define S390_LK_DAT_TT_REGION2 2
> > +#define S390_LK_DAT_TT_REGION3 1
> > +#define S390_LK_DAT_TT_SEGMENT 0
> > +
> > +/* Region-First-Table */
> > +#define S390_LK_RFTE_TL 0x3ULL /* Table-Length */
> > +#define S390_LK_RFTE_TT 0xcULL /* Table-Type */
> > +#define S390_LK_RFTE_I 0x20ULL /* Region-Invalid Bit */
> > +#define S390_LK_RFTE_TF 0xc0ULL /* Table Offset */
> > +#define S390_LK_RFTE_P 0x200ULL /* DAT-Protection Bit */
> > +#define S390_LK_RFTE_O ~0xfffULL /* Region-Second-Table
> > Origin */ +
> > +/* Region-Second-Table flags. */
> > +#define S390_LK_RSTE_TL 0x3ULL /* Table-Length */
> > +#define S390_LK_RSTE_TT 0xcULL /* Table-Type */
> > +#define S390_LK_RSTE_I 0x20ULL /* Region-Invalid Bit */
> > +#define S390_LK_RSTE_TF 0xc0ULL /* Table Offset */
> > +#define S390_LK_RSTE_P 0x200ULL /* DAT-Protection Bit */
> > +#define S390_LK_RSTE_O ~0xfffULL /* Region-Third-Table
> > Origin */ +
> > +/* Region-Third-Table flags. */
> > +#define S390_LK_RTTE_TL 0x3ULL /* Table-Length */
> > +#define S390_LK_RTTE_TT 0xcULL /* Table-Type */
> > +#define S390_LK_RTTE_CR 0x10ULL /* Common-Region Bit */
> > +#define S390_LK_RTTE_I 0x20ULL /* Region-Invalid Bit
> > */ +#define S390_LK_RTTE_TF 0xc0ULL /* Table Offset */
> > +#define S390_LK_RTTE_P 0x200ULL /* DAT-Protection Bit
> > */ +#define S390_LK_RTTE_FC 0x400ULL /* Format-Control
> > Bit */ +#define S390_LK_RTTE_F 0x800ULL /*
> > Fetch-Protection Bit */ +#define S390_LK_RTTE_ACC
> > 0xf000ULL /* Access-Control Bits */ +#define
> > S390_LK_RTTE_AV 0x10000ULL /* ACCF-Validy Control */
>
> "Validy" or "Validity"?

Its Validity ...
fixed.

Thanks a lot
Philipp

> > +#define S390_LK_RTTE_O ~0xfffULL /* Segment-Table
> > Origin */ +#define S390_LK_RTTE_RFAA ~0x7fffffffULL /*
> > Region-Frame Absolute Address */ +
> > +/* Segment-Table flags. */
> > +#define S390_LK_STE_TT 0xcULL /* Table-Type */
> > +#define S390_LK_STE_I 0x20ULL /* Segment-Invalid Bit */
> > +#define S390_LK_STE_TF 0xc0ULL /* Table Offset */
> > +#define S390_LK_STE_P 0x200ULL /* DAT-Protection Bit */
> > +#define S390_LK_STE_FC 0x400ULL /* Format-Control Bit */
> > +#define S390_LK_STE_F 0x800ULL /* Fetch-Protection Bit
> > */ +#define S390_LK_STE_ACC 0xf000ULL /* Access-Control
> > Bits */ +#define S390_LK_STE_AV 0x10000ULL /* ACCF-Validy
> > Control */ +#define S390_LK_STE_O ~0x7ffULL /* Page-Table
> > Origin */ +#define S390_LK_STE_SFAA ~0xfffffULL /*
> > Segment-Frame Absolute Address */ +
> > +/* Page-Table flags. */
> > +#define S390_LK_PTE_P 0x200ULL /* DAT-Protection Bit */
> > +#define S390_LK_PTE_I 0x400ULL /* Page-Invalid Bit */
> > +#define S390_LK_PTE_PFAA ~0xfffULL /* Page-Frame Absolute
> > Address */ +
> > +/* Virtual Address Fields. */
> > +#define S390_LK_VADDR_RFX 0xffe0000000000000ULL
> > +#define S390_LK_VADDR_RSX 0x001ffc0000000000ULL
> > +#define S390_LK_VADDR_RTX 0x000003ff80000000ULL
> > +#define S390_LK_VADDR_SX 0x000000007ff00000ULL
> > +#define S390_LK_VADDR_PX 0x00000000000ff000ULL
> > +#define S390_LK_VADDR_BX 0x0000000000000fffULL
> > +
> > #endif
> >
>