Re: [RFC PATCH v2 02/12] powerpc/ptrace: drop unnecessary #ifdefs CONFIG_PPC64
Le 24/02/2020 à 11:48, Michael Ellerman a écrit :
Christophe Leroy writes:
Drop a bunch of #ifdefs CONFIG_PPC64 that are not vital.
Signed-off-by: Christophe Leroy
---
arch/powerpc/include/asm/ptrace.h | 9 -
arch/powerpc/include/uapi/asm/ptrace.h | 12
arch/powerpc/kernel/ptrace/ptrace.c| 24 +++-
3 files changed, 11 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/include/asm/ptrace.h
b/arch/powerpc/include/asm/ptrace.h
index faa5a338ac5a..1506a9c61d50 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -36,11 +36,10 @@ struct pt_regs
unsigned long link;
unsigned long xer;
unsigned long ccr;
-#ifdef CONFIG_PPC64
- unsigned long softe;
-#else
- unsigned long mq;
-#endif
+ union {
+ unsigned long softe;
+ unsigned long mq;
+ };
unsigned long trap;
unsigned long dar;
unsigned long dsisr;
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h
b/arch/powerpc/include/uapi/asm/ptrace.h
index f5f1ccc740fc..37d7befbb8dc 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -43,12 +43,11 @@ struct pt_regs
unsigned long link;
unsigned long xer;
unsigned long ccr;
-#ifdef __powerpc64__
- unsigned long softe;/* Soft enabled/disabled */
-#else
- unsigned long mq; /* 601 only (not used at present) */
+ union {
+ unsigned long softe;/* Soft enabled/disabled */
+ unsigned long mq; /* 601 only (not used at present) */
/* Used on APUS to hold IPL value. */
-#endif
+ };
As Andreas pointed out this is not safe as this is a uapi header.
Ok, dropped
unsigned long trap; /* Reason for being here */
/* N.B. for critical exceptions on 4xx, the dar and dsisr
fields are overloaded to hold srr0 and srr1. */
@@ -105,11 +104,8 @@ struct pt_regs
#define PT_LNK36
#define PT_XER37
#define PT_CCR38
-#ifndef __powerpc64__
#define PT_MQ 39
-#else
#define PT_SOFTE 39
-#endif
I'd also rather leave that as it is.
There's a slim chance it could break some code that already has either
of those defined.
If you need them both defined to make other code work in the kernel
that's fine, in the kernel header we can do:
// Ensure these are always defined inside the kernel to avoid #ifdefs
#ifdef CONFIG_PPC64
#define PT_MQ 39
#else
#define PT_SOFTE 39
#endif
Ok.
Only the PT_SOFTE is missing, I added the following in the relevant case:
#define PT_SOFTE PT_MQ
#define PT_TRAP 40
#define PT_DAR41
#define PT_DSISR 42
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c
b/arch/powerpc/kernel/ptrace/ptrace.c
index 684b0b315c32..0afb223c4d57 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -113,11 +113,8 @@ static const struct pt_regs_offset regoffset_table[] = {
REG_OFFSET_NAME(link),
REG_OFFSET_NAME(xer),
REG_OFFSET_NAME(ccr),
-#ifdef CONFIG_PPC64
REG_OFFSET_NAME(softe),
-#else
REG_OFFSET_NAME(mq),
-#endif
Pretty sure that will cause breakage. The offset is ABI.
Ok, dropped
Christophe
Re: [RFC PATCH v2 02/12] powerpc/ptrace: drop unnecessary #ifdefs CONFIG_PPC64
Christophe Leroy writes:
> Drop a bunch of #ifdefs CONFIG_PPC64 that are not vital.
>
> Signed-off-by: Christophe Leroy
> ---
> arch/powerpc/include/asm/ptrace.h | 9 -
> arch/powerpc/include/uapi/asm/ptrace.h | 12
> arch/powerpc/kernel/ptrace/ptrace.c| 24 +++-
> 3 files changed, 11 insertions(+), 34 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ptrace.h
> b/arch/powerpc/include/asm/ptrace.h
> index faa5a338ac5a..1506a9c61d50 100644
> --- a/arch/powerpc/include/asm/ptrace.h
> +++ b/arch/powerpc/include/asm/ptrace.h
> @@ -36,11 +36,10 @@ struct pt_regs
> unsigned long link;
> unsigned long xer;
> unsigned long ccr;
> -#ifdef CONFIG_PPC64
> - unsigned long softe;
> -#else
> - unsigned long mq;
> -#endif
> + union {
> + unsigned long softe;
> + unsigned long mq;
> + };
> unsigned long trap;
> unsigned long dar;
> unsigned long dsisr;
> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h
> b/arch/powerpc/include/uapi/asm/ptrace.h
> index f5f1ccc740fc..37d7befbb8dc 100644
> --- a/arch/powerpc/include/uapi/asm/ptrace.h
> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> @@ -43,12 +43,11 @@ struct pt_regs
> unsigned long link;
> unsigned long xer;
> unsigned long ccr;
> -#ifdef __powerpc64__
> - unsigned long softe;/* Soft enabled/disabled */
> -#else
> - unsigned long mq; /* 601 only (not used at present) */
> + union {
> + unsigned long softe;/* Soft enabled/disabled */
> + unsigned long mq; /* 601 only (not used at present) */
> /* Used on APUS to hold IPL value. */
> -#endif
> + };
As Andreas pointed out this is not safe as this is a uapi header.
> unsigned long trap; /* Reason for being here */
> /* N.B. for critical exceptions on 4xx, the dar and dsisr
> fields are overloaded to hold srr0 and srr1. */
> @@ -105,11 +104,8 @@ struct pt_regs
> #define PT_LNK 36
> #define PT_XER 37
> #define PT_CCR 38
> -#ifndef __powerpc64__
> #define PT_MQ39
> -#else
> #define PT_SOFTE 39
> -#endif
I'd also rather leave that as it is.
There's a slim chance it could break some code that already has either
of those defined.
If you need them both defined to make other code work in the kernel
that's fine, in the kernel header we can do:
// Ensure these are always defined inside the kernel to avoid #ifdefs
#ifdef CONFIG_PPC64
#define PT_MQ 39
#else
#define PT_SOFTE 39
#endif
> #define PT_TRAP 40
> #define PT_DAR 41
> #define PT_DSISR 42
> diff --git a/arch/powerpc/kernel/ptrace/ptrace.c
> b/arch/powerpc/kernel/ptrace/ptrace.c
> index 684b0b315c32..0afb223c4d57 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> @@ -113,11 +113,8 @@ static const struct pt_regs_offset regoffset_table[] = {
> REG_OFFSET_NAME(link),
> REG_OFFSET_NAME(xer),
> REG_OFFSET_NAME(ccr),
> -#ifdef CONFIG_PPC64
> REG_OFFSET_NAME(softe),
> -#else
> REG_OFFSET_NAME(mq),
> -#endif
Pretty sure that will cause breakage. The offset is ABI.
cheers
Re: [RFC PATCH v2 02/12] powerpc/ptrace: drop unnecessary #ifdefs CONFIG_PPC64
On Jun 28 2019, Christophe Leroy wrote:
> Le 28/06/2019 à 18:36, Andreas Schwab a écrit :
>> On Jun 28 2019, Christophe Leroy wrote:
>>
>>> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h
>>> b/arch/powerpc/include/uapi/asm/ptrace.h
>>> index f5f1ccc740fc..37d7befbb8dc 100644
>>> --- a/arch/powerpc/include/uapi/asm/ptrace.h
>>> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
>>> @@ -43,12 +43,11 @@ struct pt_regs
>>> unsigned long link;
>>> unsigned long xer;
>>> unsigned long ccr;
>>> -#ifdef __powerpc64__
>>> - unsigned long softe;/* Soft enabled/disabled */
>>> -#else
>>> - unsigned long mq; /* 601 only (not used at present) */
>>> + union {
>>> + unsigned long softe;/* Soft enabled/disabled */
>>> + unsigned long mq; /* 601 only (not used at present) */
>>> /* Used on APUS to hold IPL value. */
>>> -#endif
>>> + };
>>
>> Anonymous unions are a C11 feature.
>>
>
> Is that a problem ?
Yes, this is a UAPI header.
Andreas.
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Re: [RFC PATCH v2 02/12] powerpc/ptrace: drop unnecessary #ifdefs CONFIG_PPC64
Le 28/06/2019 à 18:36, Andreas Schwab a écrit :
On Jun 28 2019, Christophe Leroy wrote:
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h
b/arch/powerpc/include/uapi/asm/ptrace.h
index f5f1ccc740fc..37d7befbb8dc 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -43,12 +43,11 @@ struct pt_regs
unsigned long link;
unsigned long xer;
unsigned long ccr;
-#ifdef __powerpc64__
- unsigned long softe;/* Soft enabled/disabled */
-#else
- unsigned long mq; /* 601 only (not used at present) */
+ union {
+ unsigned long softe;/* Soft enabled/disabled */
+ unsigned long mq; /* 601 only (not used at present) */
/* Used on APUS to hold IPL value. */
-#endif
+ };
Anonymous unions are a C11 feature.
Is that a problem ?
Kernel has a minimum GCC requirement of version 4.6, doesn't 4.6 support
C11 ?
Christophe
Re: [RFC PATCH v2 02/12] powerpc/ptrace: drop unnecessary #ifdefs CONFIG_PPC64
On Jun 28 2019, Christophe Leroy wrote:
> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h
> b/arch/powerpc/include/uapi/asm/ptrace.h
> index f5f1ccc740fc..37d7befbb8dc 100644
> --- a/arch/powerpc/include/uapi/asm/ptrace.h
> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> @@ -43,12 +43,11 @@ struct pt_regs
> unsigned long link;
> unsigned long xer;
> unsigned long ccr;
> -#ifdef __powerpc64__
> - unsigned long softe;/* Soft enabled/disabled */
> -#else
> - unsigned long mq; /* 601 only (not used at present) */
> + union {
> + unsigned long softe;/* Soft enabled/disabled */
> + unsigned long mq; /* 601 only (not used at present) */
> /* Used on APUS to hold IPL value. */
> -#endif
> + };
Anonymous unions are a C11 feature.
Andreas.
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
[RFC PATCH v2 02/12] powerpc/ptrace: drop unnecessary #ifdefs CONFIG_PPC64
Drop a bunch of #ifdefs CONFIG_PPC64 that are not vital.
Signed-off-by: Christophe Leroy
---
arch/powerpc/include/asm/ptrace.h | 9 -
arch/powerpc/include/uapi/asm/ptrace.h | 12
arch/powerpc/kernel/ptrace/ptrace.c| 24 +++-
3 files changed, 11 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/include/asm/ptrace.h
b/arch/powerpc/include/asm/ptrace.h
index faa5a338ac5a..1506a9c61d50 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -36,11 +36,10 @@ struct pt_regs
unsigned long link;
unsigned long xer;
unsigned long ccr;
-#ifdef CONFIG_PPC64
- unsigned long softe;
-#else
- unsigned long mq;
-#endif
+ union {
+ unsigned long softe;
+ unsigned long mq;
+ };
unsigned long trap;
unsigned long dar;
unsigned long dsisr;
diff --git a/arch/powerpc/include/uapi/asm/ptrace.h
b/arch/powerpc/include/uapi/asm/ptrace.h
index f5f1ccc740fc..37d7befbb8dc 100644
--- a/arch/powerpc/include/uapi/asm/ptrace.h
+++ b/arch/powerpc/include/uapi/asm/ptrace.h
@@ -43,12 +43,11 @@ struct pt_regs
unsigned long link;
unsigned long xer;
unsigned long ccr;
-#ifdef __powerpc64__
- unsigned long softe;/* Soft enabled/disabled */
-#else
- unsigned long mq; /* 601 only (not used at present) */
+ union {
+ unsigned long softe;/* Soft enabled/disabled */
+ unsigned long mq; /* 601 only (not used at present) */
/* Used on APUS to hold IPL value. */
-#endif
+ };
unsigned long trap; /* Reason for being here */
/* N.B. for critical exceptions on 4xx, the dar and dsisr
fields are overloaded to hold srr0 and srr1. */
@@ -105,11 +104,8 @@ struct pt_regs
#define PT_LNK 36
#define PT_XER 37
#define PT_CCR 38
-#ifndef __powerpc64__
#define PT_MQ 39
-#else
#define PT_SOFTE 39
-#endif
#define PT_TRAP40
#define PT_DAR 41
#define PT_DSISR 42
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c
b/arch/powerpc/kernel/ptrace/ptrace.c
index 684b0b315c32..0afb223c4d57 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -113,11 +113,8 @@ static const struct pt_regs_offset regoffset_table[] = {
REG_OFFSET_NAME(link),
REG_OFFSET_NAME(xer),
REG_OFFSET_NAME(ccr),
-#ifdef CONFIG_PPC64
REG_OFFSET_NAME(softe),
-#else
REG_OFFSET_NAME(mq),
-#endif
REG_OFFSET_NAME(trap),
REG_OFFSET_NAME(dar),
REG_OFFSET_NAME(dsisr),
@@ -289,17 +286,15 @@ int ptrace_get_reg(struct task_struct *task, int regno,
unsigned long *data)
if (regno == PT_DSCR)
return get_user_dscr(task, data);
-#ifdef CONFIG_PPC64
/*
* softe copies paca->irq_soft_mask variable state. Since irq_soft_mask
is
* no more used as a flag, lets force usr to alway see the softe value
as 1
* which means interrupts are not soft disabled.
*/
- if (regno == PT_SOFTE) {
+ if (IS_ENABLED(CONFIG_PPC64) && regno == PT_SOFTE) {
*data = 1;
return 0;
}
-#endif
regs_max = sizeof(struct user_pt_regs) / sizeof(unsigned long);
if (regno < regs_max) {
@@ -2013,7 +2008,6 @@ static const struct user_regset_view user_ppc_native_view
= {
.regsets = native_regsets, .n = ARRAY_SIZE(native_regsets)
};
-#ifdef CONFIG_PPC64
#include
static int gpr32_get_common(struct task_struct *target,
@@ -2287,14 +2281,11 @@ static const struct user_regset_view
user_ppc_compat_view = {
.name = "ppc", .e_machine = EM_PPC, .ei_osabi = ELF_OSABI,
.regsets = compat_regsets, .n = ARRAY_SIZE(compat_regsets)
};
-#endif /* CONFIG_PPC64 */
const struct user_regset_view *task_user_regset_view(struct task_struct *task)
{
-#ifdef CONFIG_PPC64
- if (test_tsk_thread_flag(task, TIF_32BIT))
+ if (IS_ENABLED(CONFIG_PPC64) && test_tsk_thread_flag(task, TIF_32BIT))
return _ppc_compat_view;
-#endif
return _ppc_native_view;
}
@@ -3081,11 +3072,7 @@ long arch_ptrace(struct task_struct *child, long request,
else
dbginfo.num_data_bps = 0;
dbginfo.num_condition_regs = 0;
-#ifdef CONFIG_PPC64
- dbginfo.data_bp_alignment = 8;
-#else
- dbginfo.data_bp_alignment = 4;
-#endif
+ dbginfo.data_bp_alignment = sizeof(long);
dbginfo.sizeof_condition = 0;
#ifdef CONFIG_HAVE_HW_BREAKPOINT
dbginfo.features = PPC_DEBUG_FEATURE_DATA_BP_RANGE;
@@ -3322,12 +3309,10
