* [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 6:46 ` Nicholas Piggin
2020-02-26 4:07 ` [PATCH v3 02/14] powerpc: Define new SRR1 bits for a future ISA version Jordan Niethe
` (12 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, dja
From: Alistair Popple <alistair@popple.id.au>
Prefix instructions have their own FSCR bit which needs to enabled via
a CPU feature. The kernel will save the FSCR for problem state but it
needs to be enabled initially.
Signed-off-by: Alistair Popple <alistair@popple.id.au>
---
arch/powerpc/include/asm/reg.h | 3 +++
arch/powerpc/kernel/dt_cpu_ftrs.c | 23 +++++++++++++++++++++++
2 files changed, 26 insertions(+)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 1aa46dff0957..c7758c2ccc5f 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -397,6 +397,7 @@
#define SPRN_RWMR 0x375 /* Region-Weighting Mode Register */
/* HFSCR and FSCR bit numbers are the same */
+#define FSCR_PREFIX_LG 13 /* Enable Prefix Instructions */
#define FSCR_SCV_LG 12 /* Enable System Call Vectored */
#define FSCR_MSGP_LG 10 /* Enable MSGP */
#define FSCR_TAR_LG 8 /* Enable Target Address Register */
@@ -408,11 +409,13 @@
#define FSCR_VECVSX_LG 1 /* Enable VMX/VSX */
#define FSCR_FP_LG 0 /* Enable Floating Point */
#define SPRN_FSCR 0x099 /* Facility Status & Control Register */
+#define FSCR_PREFIX __MASK(FSCR_PREFIX_LG)
#define FSCR_SCV __MASK(FSCR_SCV_LG)
#define FSCR_TAR __MASK(FSCR_TAR_LG)
#define FSCR_EBB __MASK(FSCR_EBB_LG)
#define FSCR_DSCR __MASK(FSCR_DSCR_LG)
#define SPRN_HFSCR 0xbe /* HV=1 Facility Status & Control Register */
+#define HFSCR_PREFIX __MASK(FSCR_PREFIX_LG)
#define HFSCR_MSGP __MASK(FSCR_MSGP_LG)
#define HFSCR_TAR __MASK(FSCR_TAR_LG)
#define HFSCR_EBB __MASK(FSCR_EBB_LG)
diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 182b4047c1ef..396f2c6c588e 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -553,6 +553,28 @@ static int __init feat_enable_large_ci(struct dt_cpu_feature *f)
return 1;
}
+static int __init feat_enable_prefix(struct dt_cpu_feature *f)
+{
+ u64 fscr, hfscr;
+
+ if (f->usable_privilege & USABLE_HV) {
+ hfscr = mfspr(SPRN_HFSCR);
+ hfscr |= HFSCR_PREFIX;
+ mtspr(SPRN_HFSCR, hfscr);
+ }
+
+ if (f->usable_privilege & USABLE_OS) {
+ fscr = mfspr(SPRN_FSCR);
+ fscr |= FSCR_PREFIX;
+ mtspr(SPRN_FSCR, fscr);
+
+ if (f->usable_privilege & USABLE_PR)
+ current->thread.fscr |= FSCR_PREFIX;
+ }
+
+ return 1;
+}
+
struct dt_cpu_feature_match {
const char *name;
int (*enable)(struct dt_cpu_feature *f);
@@ -626,6 +648,7 @@ static struct dt_cpu_feature_match __initdata
{"vector-binary128", feat_enable, 0},
{"vector-binary16", feat_enable, 0},
{"wait-v3", feat_enable, 0},
+ {"prefix-instructions", feat_enable_prefix, 0},
};
static bool __initdata using_dt_cpu_ftrs;
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
2020-02-26 4:07 ` [PATCH v3 01/14] powerpc: Enable Prefixed Instructions Jordan Niethe
@ 2020-02-26 6:46 ` Nicholas Piggin
2020-02-28 2:52 ` Jordan Niethe
0 siblings, 1 reply; 36+ messages in thread
From: Nicholas Piggin @ 2020-02-26 6:46 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: alistair, dja, bala24
Jordan Niethe's on February 26, 2020 2:07 pm:
> From: Alistair Popple <alistair@popple.id.au>
>
> Prefix instructions have their own FSCR bit which needs to enabled via
> a CPU feature. The kernel will save the FSCR for problem state but it
> needs to be enabled initially.
>
> Signed-off-by: Alistair Popple <alistair@popple.id.au>
> ---
> arch/powerpc/include/asm/reg.h | 3 +++
> arch/powerpc/kernel/dt_cpu_ftrs.c | 23 +++++++++++++++++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 1aa46dff0957..c7758c2ccc5f 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -397,6 +397,7 @@
> #define SPRN_RWMR 0x375 /* Region-Weighting Mode Register */
>
> /* HFSCR and FSCR bit numbers are the same */
> +#define FSCR_PREFIX_LG 13 /* Enable Prefix Instructions */
> #define FSCR_SCV_LG 12 /* Enable System Call Vectored */
> #define FSCR_MSGP_LG 10 /* Enable MSGP */
> #define FSCR_TAR_LG 8 /* Enable Target Address Register */
> @@ -408,11 +409,13 @@
> #define FSCR_VECVSX_LG 1 /* Enable VMX/VSX */
> #define FSCR_FP_LG 0 /* Enable Floating Point */
> #define SPRN_FSCR 0x099 /* Facility Status & Control Register */
> +#define FSCR_PREFIX __MASK(FSCR_PREFIX_LG)
When you add a new FSCR, there's a couple more things to do, check
out traps.c.
> #define FSCR_SCV __MASK(FSCR_SCV_LG)
> #define FSCR_TAR __MASK(FSCR_TAR_LG)
> #define FSCR_EBB __MASK(FSCR_EBB_LG)
> #define FSCR_DSCR __MASK(FSCR_DSCR_LG)
> #define SPRN_HFSCR 0xbe /* HV=1 Facility Status & Control Register */
> +#define HFSCR_PREFIX __MASK(FSCR_PREFIX_LG)
> #define HFSCR_MSGP __MASK(FSCR_MSGP_LG)
> #define HFSCR_TAR __MASK(FSCR_TAR_LG)
> #define HFSCR_EBB __MASK(FSCR_EBB_LG)
> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> index 182b4047c1ef..396f2c6c588e 100644
> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> @@ -553,6 +553,28 @@ static int __init feat_enable_large_ci(struct dt_cpu_feature *f)
> return 1;
> }
>
> +static int __init feat_enable_prefix(struct dt_cpu_feature *f)
> +{
> + u64 fscr, hfscr;
> +
> + if (f->usable_privilege & USABLE_HV) {
> + hfscr = mfspr(SPRN_HFSCR);
> + hfscr |= HFSCR_PREFIX;
> + mtspr(SPRN_HFSCR, hfscr);
> + }
> +
> + if (f->usable_privilege & USABLE_OS) {
> + fscr = mfspr(SPRN_FSCR);
> + fscr |= FSCR_PREFIX;
> + mtspr(SPRN_FSCR, fscr);
> +
> + if (f->usable_privilege & USABLE_PR)
> + current->thread.fscr |= FSCR_PREFIX;
> + }
> +
> + return 1;
> +}
It would be good to be able to just use the default feature matching
for this, if possible? Do we not do the right thing with
init_thread.fscr?
> +
> struct dt_cpu_feature_match {
> const char *name;
> int (*enable)(struct dt_cpu_feature *f);
> @@ -626,6 +648,7 @@ static struct dt_cpu_feature_match __initdata
> {"vector-binary128", feat_enable, 0},
> {"vector-binary16", feat_enable, 0},
> {"wait-v3", feat_enable, 0},
> + {"prefix-instructions", feat_enable_prefix, 0},
That's reasonable to make that a feature, will it specify a minimum
base set of prefix instructions or just that prefix instructions
with the prefix/suffix arrangement exist?
You may not need "-instructions" on the end, none of the other
instructions do.
I would maybe just hold off upstreaming the dt_cpu_ftrs changes for
a bit. We have to do a pass over new CPU feature device tree, and
some compatibility questions have come up recently.
If you wouldn't mind just adding the new [H]FSCR bits and faults
upstream for now, that would be good.
Thanks,
Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
2020-02-26 6:46 ` Nicholas Piggin
@ 2020-02-28 2:52 ` Jordan Niethe
2020-02-28 4:48 ` Nicholas Piggin
0 siblings, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-28 2:52 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
On Wed, Feb 26, 2020 at 5:50 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on February 26, 2020 2:07 pm:
> > From: Alistair Popple <alistair@popple.id.au>
> >
> > Prefix instructions have their own FSCR bit which needs to enabled via
> > a CPU feature. The kernel will save the FSCR for problem state but it
> > needs to be enabled initially.
> >
> > Signed-off-by: Alistair Popple <alistair@popple.id.au>
> > ---
> > arch/powerpc/include/asm/reg.h | 3 +++
> > arch/powerpc/kernel/dt_cpu_ftrs.c | 23 +++++++++++++++++++++++
> > 2 files changed, 26 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> > index 1aa46dff0957..c7758c2ccc5f 100644
> > --- a/arch/powerpc/include/asm/reg.h
> > +++ b/arch/powerpc/include/asm/reg.h
> > @@ -397,6 +397,7 @@
> > #define SPRN_RWMR 0x375 /* Region-Weighting Mode Register */
> >
> > /* HFSCR and FSCR bit numbers are the same */
> > +#define FSCR_PREFIX_LG 13 /* Enable Prefix Instructions */
> > #define FSCR_SCV_LG 12 /* Enable System Call Vectored */
> > #define FSCR_MSGP_LG 10 /* Enable MSGP */
> > #define FSCR_TAR_LG 8 /* Enable Target Address Register */
> > @@ -408,11 +409,13 @@
> > #define FSCR_VECVSX_LG 1 /* Enable VMX/VSX */
> > #define FSCR_FP_LG 0 /* Enable Floating Point */
> > #define SPRN_FSCR 0x099 /* Facility Status & Control Register */
> > +#define FSCR_PREFIX __MASK(FSCR_PREFIX_LG)
>
> When you add a new FSCR, there's a couple more things to do, check
> out traps.c.
>
> > #define FSCR_SCV __MASK(FSCR_SCV_LG)
> > #define FSCR_TAR __MASK(FSCR_TAR_LG)
> > #define FSCR_EBB __MASK(FSCR_EBB_LG)
> > #define FSCR_DSCR __MASK(FSCR_DSCR_LG)
> > #define SPRN_HFSCR 0xbe /* HV=1 Facility Status & Control Register */
> > +#define HFSCR_PREFIX __MASK(FSCR_PREFIX_LG)
> > #define HFSCR_MSGP __MASK(FSCR_MSGP_LG)
> > #define HFSCR_TAR __MASK(FSCR_TAR_LG)
> > #define HFSCR_EBB __MASK(FSCR_EBB_LG)
> > diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > index 182b4047c1ef..396f2c6c588e 100644
> > --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
> > +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
> > @@ -553,6 +553,28 @@ static int __init feat_enable_large_ci(struct dt_cpu_feature *f)
> > return 1;
> > }
> >
> > +static int __init feat_enable_prefix(struct dt_cpu_feature *f)
> > +{
> > + u64 fscr, hfscr;
> > +
> > + if (f->usable_privilege & USABLE_HV) {
> > + hfscr = mfspr(SPRN_HFSCR);
> > + hfscr |= HFSCR_PREFIX;
> > + mtspr(SPRN_HFSCR, hfscr);
> > + }
> > +
> > + if (f->usable_privilege & USABLE_OS) {
> > + fscr = mfspr(SPRN_FSCR);
> > + fscr |= FSCR_PREFIX;
> > + mtspr(SPRN_FSCR, fscr);
> > +
> > + if (f->usable_privilege & USABLE_PR)
> > + current->thread.fscr |= FSCR_PREFIX;
> > + }
> > +
> > + return 1;
> > +}
>
> It would be good to be able to just use the default feature matching
> for this, if possible? Do we not do the right thing with
> init_thread.fscr?
The default feature matching do you mean feat_enable()?
I just tested using that again, within feat_enable() I can print and
see that the [h]fscr gets the bits set for enabling prefixed
instructions. However once I get to the shell and start xmon, the fscr
bit for prefixed instructions can be seen to be unset. What we are
doing in feat_enable_prefix() avoids that problem. So it seems maybe
something is not quite right with the init_thread.fscr. I will look
into further.
>
>
> > +
> > struct dt_cpu_feature_match {
> > const char *name;
> > int (*enable)(struct dt_cpu_feature *f);
> > @@ -626,6 +648,7 @@ static struct dt_cpu_feature_match __initdata
> > {"vector-binary128", feat_enable, 0},
> > {"vector-binary16", feat_enable, 0},
> > {"wait-v3", feat_enable, 0},
> > + {"prefix-instructions", feat_enable_prefix, 0},
>
> That's reasonable to make that a feature, will it specify a minimum
> base set of prefix instructions or just that prefix instructions
> with the prefix/suffix arrangement exist?
This was just going to be that they exist.
>
> You may not need "-instructions" on the end, none of the other
> instructions do.
Good point.
>
> I would maybe just hold off upstreaming the dt_cpu_ftrs changes for
> a bit. We have to do a pass over new CPU feature device tree, and
> some compatibility questions have come up recently.
>
> If you wouldn't mind just adding the new [H]FSCR bits and faults
> upstream for now, that would be good.
No problem.
>
> Thanks,
> Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
2020-02-28 2:52 ` Jordan Niethe
@ 2020-02-28 4:48 ` Nicholas Piggin
2020-03-03 23:20 ` Alistair Popple
0 siblings, 1 reply; 36+ messages in thread
From: Nicholas Piggin @ 2020-02-28 4:48 UTC (permalink / raw)
To: Jordan Niethe; +Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
Jordan Niethe's on February 28, 2020 12:52 pm:
> On Wed, Feb 26, 2020 at 5:50 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Jordan Niethe's on February 26, 2020 2:07 pm:
>> > From: Alistair Popple <alistair@popple.id.au>
>> >
>> > Prefix instructions have their own FSCR bit which needs to enabled via
>> > a CPU feature. The kernel will save the FSCR for problem state but it
>> > needs to be enabled initially.
>> >
>> > Signed-off-by: Alistair Popple <alistair@popple.id.au>
>> > ---
>> > arch/powerpc/include/asm/reg.h | 3 +++
>> > arch/powerpc/kernel/dt_cpu_ftrs.c | 23 +++++++++++++++++++++++
>> > 2 files changed, 26 insertions(+)
>> >
>> > diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
>> > index 1aa46dff0957..c7758c2ccc5f 100644
>> > --- a/arch/powerpc/include/asm/reg.h
>> > +++ b/arch/powerpc/include/asm/reg.h
>> > @@ -397,6 +397,7 @@
>> > #define SPRN_RWMR 0x375 /* Region-Weighting Mode Register */
>> >
>> > /* HFSCR and FSCR bit numbers are the same */
>> > +#define FSCR_PREFIX_LG 13 /* Enable Prefix Instructions */
>> > #define FSCR_SCV_LG 12 /* Enable System Call Vectored */
>> > #define FSCR_MSGP_LG 10 /* Enable MSGP */
>> > #define FSCR_TAR_LG 8 /* Enable Target Address Register */
>> > @@ -408,11 +409,13 @@
>> > #define FSCR_VECVSX_LG 1 /* Enable VMX/VSX */
>> > #define FSCR_FP_LG 0 /* Enable Floating Point */
>> > #define SPRN_FSCR 0x099 /* Facility Status & Control Register */
>> > +#define FSCR_PREFIX __MASK(FSCR_PREFIX_LG)
>>
>> When you add a new FSCR, there's a couple more things to do, check
>> out traps.c.
>>
>> > #define FSCR_SCV __MASK(FSCR_SCV_LG)
>> > #define FSCR_TAR __MASK(FSCR_TAR_LG)
>> > #define FSCR_EBB __MASK(FSCR_EBB_LG)
>> > #define FSCR_DSCR __MASK(FSCR_DSCR_LG)
>> > #define SPRN_HFSCR 0xbe /* HV=1 Facility Status & Control Register */
>> > +#define HFSCR_PREFIX __MASK(FSCR_PREFIX_LG)
>> > #define HFSCR_MSGP __MASK(FSCR_MSGP_LG)
>> > #define HFSCR_TAR __MASK(FSCR_TAR_LG)
>> > #define HFSCR_EBB __MASK(FSCR_EBB_LG)
>> > diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > index 182b4047c1ef..396f2c6c588e 100644
>> > --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> > @@ -553,6 +553,28 @@ static int __init feat_enable_large_ci(struct dt_cpu_feature *f)
>> > return 1;
>> > }
>> >
>> > +static int __init feat_enable_prefix(struct dt_cpu_feature *f)
>> > +{
>> > + u64 fscr, hfscr;
>> > +
>> > + if (f->usable_privilege & USABLE_HV) {
>> > + hfscr = mfspr(SPRN_HFSCR);
>> > + hfscr |= HFSCR_PREFIX;
>> > + mtspr(SPRN_HFSCR, hfscr);
>> > + }
>> > +
>> > + if (f->usable_privilege & USABLE_OS) {
>> > + fscr = mfspr(SPRN_FSCR);
>> > + fscr |= FSCR_PREFIX;
>> > + mtspr(SPRN_FSCR, fscr);
>> > +
>> > + if (f->usable_privilege & USABLE_PR)
>> > + current->thread.fscr |= FSCR_PREFIX;
>> > + }
>> > +
>> > + return 1;
>> > +}
>>
>> It would be good to be able to just use the default feature matching
>> for this, if possible? Do we not do the right thing with
>> init_thread.fscr?
> The default feature matching do you mean feat_enable()?
> I just tested using that again, within feat_enable() I can print and
> see that the [h]fscr gets the bits set for enabling prefixed
> instructions. However once I get to the shell and start xmon, the fscr
> bit for prefixed instructions can be seen to be unset. What we are
> doing in feat_enable_prefix() avoids that problem. So it seems maybe
> something is not quite right with the init_thread.fscr. I will look
> into further.
Okay it probably gets overwritten somewhere.
But hmm, the dt_cpu_ftrs parsing should be picking those up and setting
them by default I would think (or maybe not because you don't expect
FSCR bit if OS support is not required).
All the other FSCR bits are done similarly to this AFAIKS:
https://patchwork.ozlabs.org/patch/1244476/
I would do that for now rather than digging into it too far, but we
should look at cleaning that up and doing something more like what
you have here.
>> > struct dt_cpu_feature_match {
>> > const char *name;
>> > int (*enable)(struct dt_cpu_feature *f);
>> > @@ -626,6 +648,7 @@ static struct dt_cpu_feature_match __initdata
>> > {"vector-binary128", feat_enable, 0},
>> > {"vector-binary16", feat_enable, 0},
>> > {"wait-v3", feat_enable, 0},
>> > + {"prefix-instructions", feat_enable_prefix, 0},
>>
>> That's reasonable to make that a feature, will it specify a minimum
>> base set of prefix instructions or just that prefix instructions
>> with the prefix/suffix arrangement exist?
> This was just going to be that they exist.
>>
>> You may not need "-instructions" on the end, none of the other
>> instructions do.
> Good point.
>>
>> I would maybe just hold off upstreaming the dt_cpu_ftrs changes for
>> a bit. We have to do a pass over new CPU feature device tree, and
>> some compatibility questions have come up recently.
>>
>> If you wouldn't mind just adding the new [H]FSCR bits and faults
>> upstream for now, that would be good.
> No problem.
Thanks,
Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
2020-02-28 4:48 ` Nicholas Piggin
@ 2020-03-03 23:20 ` Alistair Popple
0 siblings, 0 replies; 36+ messages in thread
From: Alistair Popple @ 2020-03-03 23:20 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: Jordan Niethe, Daniel Axtens, linuxppc-dev, Balamuruhan S
> But hmm, the dt_cpu_ftrs parsing should be picking those up and setting
> them by default I would think (or maybe not because you don't expect
> FSCR bit if OS support is not required).
Right - the generic dt_cpu_ftrs didn't do the FSCR enablement which I assume
is because if OS support is required anyway we can just add it in a similar
way to the below patch. My thinking was that we could use it to disable prefix
with a firmware if required, however outside of a lab environment there isn't
much practical use for that so I'm ok with just doing something similar to the
below.
> All the other FSCR bits are done similarly to this AFAIKS:
>
> https://patchwork.ozlabs.org/patch/1244476/
>
> I would do that for now rather than digging into it too far, but we
> should look at cleaning that up and doing something more like what
> you have here.
>
> >> > struct dt_cpu_feature_match {
> >> >
> >> > const char *name;
> >> > int (*enable)(struct dt_cpu_feature *f);
> >> >
> >> > @@ -626,6 +648,7 @@ static struct dt_cpu_feature_match __initdata
> >> >
> >> > {"vector-binary128", feat_enable, 0},
> >> > {"vector-binary16", feat_enable, 0},
> >> > {"wait-v3", feat_enable, 0},
> >> >
> >> > + {"prefix-instructions", feat_enable_prefix, 0},
> >>
> >> That's reasonable to make that a feature, will it specify a minimum
> >> base set of prefix instructions or just that prefix instructions
> >> with the prefix/suffix arrangement exist?
> >
> > This was just going to be that they exist.
> >
> >> You may not need "-instructions" on the end, none of the other
> >> instructions do.
> >
> > Good point.
> >
> >> I would maybe just hold off upstreaming the dt_cpu_ftrs changes for
> >> a bit. We have to do a pass over new CPU feature device tree, and
> >> some compatibility questions have come up recently.
> >>
> >> If you wouldn't mind just adding the new [H]FSCR bits and faults
> >> upstream for now, that would be good.
> >
> > No problem.
>
> Thanks,
> Nick
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 02/14] powerpc: Define new SRR1 bits for a future ISA version
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
2020-02-26 4:07 ` [PATCH v3 01/14] powerpc: Enable Prefixed Instructions Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-03-03 23:31 ` Alistair Popple
2020-02-26 4:07 ` [PATCH v3 03/14] powerpc sstep: Prepare to support prefixed instructions Jordan Niethe
` (11 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
Add the BOUNDARY SRR1 bit definition for when the cause of an alignment
exception is a prefixed instruction that crosses a 64-byte boundary.
Add the PREFIXED SRR1 bit definition for exceptions caused by prefixed
instructions.
Bit 35 of SRR1 is called SRR1_ISI_N_OR_G. This name comes from it being
used to indicate that an ISI was due to the access being no-exec or
guarded. A future ISA version adds another purpose. It is also set if
there is an access in a cache-inhibited location for prefixed
instruction. Rename from SRR1_ISI_N_OR_G to SRR1_ISI_N_G_OR_CIP.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: Combined all the commits concerning SRR1 bits.
---
arch/powerpc/include/asm/reg.h | 4 +++-
arch/powerpc/kvm/book3s_hv_nested.c | 2 +-
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 2 +-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index c7758c2ccc5f..173f33df4fab 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -762,7 +762,7 @@
#endif
#define SRR1_ISI_NOPT 0x40000000 /* ISI: Not found in hash */
-#define SRR1_ISI_N_OR_G 0x10000000 /* ISI: Access is no-exec or G */
+#define SRR1_ISI_N_G_OR_CIP 0x10000000 /* ISI: Access is no-exec or G or CI for a prefixed instruction */
#define SRR1_ISI_PROT 0x08000000 /* ISI: Other protection fault */
#define SRR1_WAKEMASK 0x00380000 /* reason for wakeup */
#define SRR1_WAKEMASK_P8 0x003c0000 /* reason for wakeup on POWER8 and 9 */
@@ -789,6 +789,8 @@
#define SRR1_PROGADDR 0x00010000 /* SRR0 contains subsequent addr */
#define SRR1_MCE_MCP 0x00080000 /* Machine check signal caused interrupt */
+#define SRR1_BOUNDARY 0x10000000 /* Prefixed instruction crosses 64-byte boundary */
+#define SRR1_PREFIXED 0x20000000 /* Exception caused by prefixed instruction */
#define SPRN_HSRR0 0x13A /* Save/Restore Register 0 */
#define SPRN_HSRR1 0x13B /* Save/Restore Register 1 */
diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
index dc97e5be76f6..6ab685227574 100644
--- a/arch/powerpc/kvm/book3s_hv_nested.c
+++ b/arch/powerpc/kvm/book3s_hv_nested.c
@@ -1169,7 +1169,7 @@ static int kvmhv_translate_addr_nested(struct kvm_vcpu *vcpu,
} else if (vcpu->arch.trap == BOOK3S_INTERRUPT_H_INST_STORAGE) {
/* Can we execute? */
if (!gpte_p->may_execute) {
- flags |= SRR1_ISI_N_OR_G;
+ flags |= SRR1_ISI_N_G_OR_CIP;
goto forward_to_l1;
}
} else {
diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
index 220305454c23..b53a9f1c1a46 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
@@ -1260,7 +1260,7 @@ long kvmppc_hpte_hv_fault(struct kvm_vcpu *vcpu, unsigned long addr,
status &= ~DSISR_NOHPTE; /* DSISR_NOHPTE == SRR1_ISI_NOPT */
if (!data) {
if (gr & (HPTE_R_N | HPTE_R_G))
- return status | SRR1_ISI_N_OR_G;
+ return status | SRR1_ISI_N_G_OR_CIP;
if (!hpte_read_permission(pp, slb_v & key))
return status | SRR1_ISI_PROT;
} else if (status & DSISR_ISSTORE) {
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 02/14] powerpc: Define new SRR1 bits for a future ISA version
2020-02-26 4:07 ` [PATCH v3 02/14] powerpc: Define new SRR1 bits for a future ISA version Jordan Niethe
@ 2020-03-03 23:31 ` Alistair Popple
0 siblings, 0 replies; 36+ messages in thread
From: Alistair Popple @ 2020-03-03 23:31 UTC (permalink / raw)
To: Jordan Niethe; +Cc: dja, linuxppc-dev, bala24
On Wednesday, 26 February 2020 3:07:04 PM AEDT Jordan Niethe wrote:
> Add the BOUNDARY SRR1 bit definition for when the cause of an alignment
> exception is a prefixed instruction that crosses a 64-byte boundary.
> Add the PREFIXED SRR1 bit definition for exceptions caused by prefixed
> instructions.
>
> Bit 35 of SRR1 is called SRR1_ISI_N_OR_G. This name comes from it being
> used to indicate that an ISI was due to the access being no-exec or
> guarded. A future ISA version adds another purpose. It is also set if
> there is an access in a cache-inhibited location for prefixed
> instruction. Rename from SRR1_ISI_N_OR_G to SRR1_ISI_N_G_OR_CIP.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
Confirmed the definitions here match the specifications so:
Reviewed-by: Alistair Popple <alistair@popple.id.au>
> ---
> v2: Combined all the commits concerning SRR1 bits.
> ---
> arch/powerpc/include/asm/reg.h | 4 +++-
> arch/powerpc/kvm/book3s_hv_nested.c | 2 +-
> arch/powerpc/kvm/book3s_hv_rm_mmu.c | 2 +-
> 3 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index c7758c2ccc5f..173f33df4fab 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -762,7 +762,7 @@
> #endif
>
> #define SRR1_ISI_NOPT 0x40000000 /* ISI: Not found in hash */
> -#define SRR1_ISI_N_OR_G 0x10000000 /* ISI: Access is no-exec or G */
> +#define SRR1_ISI_N_G_OR_CIP 0x10000000 /* ISI: Access is no-exec or G or
> CI for a prefixed instruction */ #define SRR1_ISI_PROT 0x08000000 /*
> ISI: Other protection fault */ #define SRR1_WAKEMASK 0x00380000 /*
> reason for wakeup */
> #define SRR1_WAKEMASK_P8 0x003c0000 /* reason for wakeup on POWER8 and 9
> */ @@ -789,6 +789,8 @@
> #define SRR1_PROGADDR 0x00010000 /* SRR0 contains subsequent addr */
>
> #define SRR1_MCE_MCP 0x00080000 /* Machine check signal caused
interrupt
> */ +#define SRR1_BOUNDARY 0x10000000 /* Prefixed instruction crosses
> 64-byte boundary */ +#define SRR1_PREFIXED 0x20000000 /* Exception
> caused by prefixed instruction */
>
> #define SPRN_HSRR0 0x13A /* Save/Restore Register 0 */
> #define SPRN_HSRR1 0x13B /* Save/Restore Register 1 */
> diff --git a/arch/powerpc/kvm/book3s_hv_nested.c
> b/arch/powerpc/kvm/book3s_hv_nested.c index dc97e5be76f6..6ab685227574
> 100644
> --- a/arch/powerpc/kvm/book3s_hv_nested.c
> +++ b/arch/powerpc/kvm/book3s_hv_nested.c
> @@ -1169,7 +1169,7 @@ static int kvmhv_translate_addr_nested(struct kvm_vcpu
> *vcpu, } else if (vcpu->arch.trap == BOOK3S_INTERRUPT_H_INST_STORAGE) { /*
> Can we execute? */
> if (!gpte_p->may_execute) {
> - flags |= SRR1_ISI_N_OR_G;
> + flags |= SRR1_ISI_N_G_OR_CIP;
> goto forward_to_l1;
> }
> } else {
> diff --git a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> b/arch/powerpc/kvm/book3s_hv_rm_mmu.c index 220305454c23..b53a9f1c1a46
> 100644
> --- a/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> +++ b/arch/powerpc/kvm/book3s_hv_rm_mmu.c
> @@ -1260,7 +1260,7 @@ long kvmppc_hpte_hv_fault(struct kvm_vcpu *vcpu,
> unsigned long addr, status &= ~DSISR_NOHPTE; /* DSISR_NOHPTE ==
> SRR1_ISI_NOPT */
> if (!data) {
> if (gr & (HPTE_R_N | HPTE_R_G))
> - return status | SRR1_ISI_N_OR_G;
> + return status | SRR1_ISI_N_G_OR_CIP;
> if (!hpte_read_permission(pp, slb_v & key))
> return status | SRR1_ISI_PROT;
> } else if (status & DSISR_ISSTORE) {
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 03/14] powerpc sstep: Prepare to support prefixed instructions
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
2020-02-26 4:07 ` [PATCH v3 01/14] powerpc: Enable Prefixed Instructions Jordan Niethe
2020-02-26 4:07 ` [PATCH v3 02/14] powerpc: Define new SRR1 bits for a future ISA version Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 4:07 ` [PATCH v3 04/14] powerpc sstep: Add support for prefixed load/stores Jordan Niethe
` (10 subsequent siblings)
13 siblings, 0 replies; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
Currently all instructions are a single word long. A future ISA version
will include prefixed instructions which have a double word length. The
functions used for analysing and emulating instructions need to be
modified so that they can handle these new instruction types.
A prefixed instruction is a word prefix followed by a word suffix. All
prefixes uniquely have the primary op-code 1. Suffixes may be valid word
instructions or instructions that only exist as suffixes.
In handling prefixed instructions it will be convenient to treat the
suffix and prefix as separate words. To facilitate this modify
analyse_instr() and emulate_step() to take a suffix as a
parameter. For word instructions it does not matter what is passed in
here - it will be ignored.
We also define a new flag, PREFIXED, to be used in instruction_op:type.
This flag will indicate when emulating an analysed instruction if the
NIP should be advanced by word length or double word length.
The callers of analyse_instr() and emulate_step() will need their own
changes to be able to support prefixed instructions. For now modify them
to pass in 0 as a suffix.
Note that at this point no prefixed instructions are emulated or
analysed - this is just making it possible to do so.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Move definition of __get_user_instr() and
__get_user_instr_inatomic() to "powerpc: Support prefixed instructions
in alignment handler."
- Use a macro for returning the length of an op
- Rename sufx -> suffix
- Define and use PPC_NO_SUFFIX instead of 0
v3: - Define and use OP_PREFIX
- Rename OP_LENGTH() to GETLENGTH()
- Define IS_PREFIX() as 0 for non 64 bit ppc
---
arch/powerpc/include/asm/ppc-opcode.h | 13 ++++++++++++
arch/powerpc/include/asm/sstep.h | 9 ++++++--
arch/powerpc/kernel/align.c | 2 +-
arch/powerpc/kernel/hw_breakpoint.c | 4 ++--
arch/powerpc/kernel/kprobes.c | 2 +-
arch/powerpc/kernel/mce_power.c | 2 +-
arch/powerpc/kernel/optprobes.c | 3 ++-
arch/powerpc/kernel/uprobes.c | 2 +-
arch/powerpc/kvm/emulate_loadstore.c | 2 +-
arch/powerpc/lib/sstep.c | 12 ++++++-----
arch/powerpc/lib/test_emulate_step.c | 30 +++++++++++++--------------
arch/powerpc/xmon/xmon.c | 5 +++--
12 files changed, 54 insertions(+), 32 deletions(-)
diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index c1df75edde44..24dc193cd3ef 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -158,6 +158,9 @@
/* VMX Vector Store Instructions */
#define OP_31_XOP_STVX 231
+/* Prefixed Instructions */
+#define OP_PREFIX 1
+
#define OP_31 31
#define OP_LWZ 32
#define OP_STFS 52
@@ -377,6 +380,16 @@
#define PPC_INST_VCMPEQUD 0x100000c7
#define PPC_INST_VCMPEQUB 0x10000006
+/* macros for prefixed instructions */
+#ifdef __powerpc64__
+#define IS_PREFIX(x) (((x) >> 26) == OP_PREFIX)
+#else
+#define IS_PREFIX(x) (0)
+#endif
+
+#define PPC_NO_SUFFIX 0
+#define PPC_INST_LENGTH(x) (IS_PREFIX(x) ? 8 : 4)
+
/* macros to insert fields into opcodes */
#define ___PPC_RA(a) (((a) & 0x1f) << 16)
#define ___PPC_RB(b) (((b) & 0x1f) << 11)
diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index 769f055509c9..5539df5c50a4 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -89,11 +89,15 @@ enum instruction_type {
#define VSX_LDLEFT 4 /* load VSX register from left */
#define VSX_CHECK_VEC 8 /* check MSR_VEC not MSR_VSX for reg >= 32 */
+/* Prefixed flag, ORed in with type */
+#define PREFIXED 0x800
+
/* Size field in type word */
#define SIZE(n) ((n) << 12)
#define GETSIZE(w) ((w) >> 12)
#define GETTYPE(t) ((t) & INSTR_TYPE_MASK)
+#define GETLENGTH(t) (((t) & PREFIXED) ? 8 : 4)
#define MKOP(t, f, s) ((t) | (f) | SIZE(s))
@@ -132,7 +136,7 @@ union vsx_reg {
* otherwise.
*/
extern int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
- unsigned int instr);
+ unsigned int instr, unsigned int suffix);
/*
* Emulate an instruction that can be executed just by updating
@@ -149,7 +153,8 @@ void emulate_update_regs(struct pt_regs *reg, struct instruction_op *op);
* 0 if it could not be emulated, or -1 for an instruction that
* should not be emulated (rfid, mtmsrd clearing MSR_RI, etc.).
*/
-extern int emulate_step(struct pt_regs *regs, unsigned int instr);
+extern int emulate_step(struct pt_regs *regs, unsigned int instr,
+ unsigned int suffix);
/*
* Emulate a load or store instruction by reading/writing the
diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index 92045ed64976..ba3bf5c3ab62 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -334,7 +334,7 @@ int fix_alignment(struct pt_regs *regs)
if ((instr & 0xfc0006fe) == (PPC_INST_COPY & 0xfc0006fe))
return -EIO;
- r = analyse_instr(&op, regs, instr);
+ r = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
if (r < 0)
return -EINVAL;
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 2462cd7c565c..3a7ec6760dab 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -251,7 +251,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
goto fail;
- ret = analyse_instr(&op, regs, instr);
+ ret = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
type = GETTYPE(op.type);
size = GETSIZE(op.type);
@@ -275,7 +275,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
return false;
}
- if (!emulate_step(regs, instr))
+ if (!emulate_step(regs, instr, PPC_NO_SUFFIX))
goto fail;
return true;
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 337516df17d4..6b2e9e37f12b 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -228,7 +228,7 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
unsigned int insn = *p->ainsn.insn;
/* regs->nip is also adjusted if emulate_step returns 1 */
- ret = emulate_step(regs, insn);
+ ret = emulate_step(regs, insn, PPC_NO_SUFFIX);
if (ret > 0) {
/*
* Once this instruction has been boosted
diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
index 1cbf7f1a4e3d..824eda536f5d 100644
--- a/arch/powerpc/kernel/mce_power.c
+++ b/arch/powerpc/kernel/mce_power.c
@@ -374,7 +374,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr,
if (pfn != ULONG_MAX) {
instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK);
instr = *(unsigned int *)(instr_addr);
- if (!analyse_instr(&op, &tmp, instr)) {
+ if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
pfn = addr_to_pfn(regs, op.ea);
*addr = op.ea;
*phys_addr = (pfn << PAGE_SHIFT);
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 024f7aad1952..f908d9422557 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -100,7 +100,8 @@ static unsigned long can_optimize(struct kprobe *p)
* and that can be emulated.
*/
if (!is_conditional_branch(*p->ainsn.insn) &&
- analyse_instr(&op, ®s, *p->ainsn.insn) == 1) {
+ analyse_instr(&op, ®s, *p->ainsn.insn,
+ PPC_NO_SUFFIX) == 1) {
emulate_update_regs(®s, &op);
nip = regs.nip;
}
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index 1cfef0e5fec5..4ab40c4b576f 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -173,7 +173,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
* emulate_step() returns 1 if the insn was successfully emulated.
* For all other cases, we need to single-step in hardware.
*/
- ret = emulate_step(regs, auprobe->insn);
+ ret = emulate_step(regs, auprobe->insn, PPC_NO_SUFFIX);
if (ret > 0)
return true;
diff --git a/arch/powerpc/kvm/emulate_loadstore.c b/arch/powerpc/kvm/emulate_loadstore.c
index 1139bc56e004..2fc1951cdae5 100644
--- a/arch/powerpc/kvm/emulate_loadstore.c
+++ b/arch/powerpc/kvm/emulate_loadstore.c
@@ -95,7 +95,7 @@ int kvmppc_emulate_loadstore(struct kvm_vcpu *vcpu)
emulated = EMULATE_FAIL;
vcpu->arch.regs.msr = vcpu->arch.shared->msr;
- if (analyse_instr(&op, &vcpu->arch.regs, inst) == 0) {
+ if (analyse_instr(&op, &vcpu->arch.regs, inst, PPC_NO_SUFFIX) == 0) {
int type = op.type & INSTR_TYPE_MASK;
int size = GETSIZE(op.type);
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index c077acb983a1..efbe72370670 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1163,7 +1163,7 @@ static nokprobe_inline int trap_compare(long v1, long v2)
* otherwise.
*/
int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
- unsigned int instr)
+ unsigned int instr, unsigned int suffix)
{
unsigned int opcode, ra, rb, rc, rd, spr, u;
unsigned long int imm;
@@ -2756,7 +2756,8 @@ void emulate_update_regs(struct pt_regs *regs, struct instruction_op *op)
{
unsigned long next_pc;
- next_pc = truncate_if_32bit(regs->msr, regs->nip + 4);
+ next_pc = truncate_if_32bit(regs->msr,
+ regs->nip + GETLENGTH(op->type));
switch (GETTYPE(op->type)) {
case COMPUTE:
if (op->type & SETREG)
@@ -3101,14 +3102,14 @@ NOKPROBE_SYMBOL(emulate_loadstore);
* or -1 if the instruction is one that should not be stepped,
* such as an rfid, or a mtmsrd that would clear MSR_RI.
*/
-int emulate_step(struct pt_regs *regs, unsigned int instr)
+int emulate_step(struct pt_regs *regs, unsigned int instr, unsigned int suffix)
{
struct instruction_op op;
int r, err, type;
unsigned long val;
unsigned long ea;
- r = analyse_instr(&op, regs, instr);
+ r = analyse_instr(&op, regs, instr, suffix);
if (r < 0)
return r;
if (r > 0) {
@@ -3200,7 +3201,8 @@ int emulate_step(struct pt_regs *regs, unsigned int instr)
return 0;
instr_done:
- regs->nip = truncate_if_32bit(regs->msr, regs->nip + 4);
+ regs->nip = truncate_if_32bit(regs->msr,
+ regs->nip + GETLENGTH(op.type));
return 1;
}
NOKPROBE_SYMBOL(emulate_step);
diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index 42347067739c..3bc042e15d00 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -103,7 +103,7 @@ static void __init test_ld(void)
regs.gpr[3] = (unsigned long) &a;
/* ld r5, 0(r3) */
- stepped = emulate_step(®s, TEST_LD(5, 3, 0));
+ stepped = emulate_step(®s, TEST_LD(5, 3, 0), PPC_NO_SUFFIX);
if (stepped == 1 && regs.gpr[5] == a)
show_result("ld", "PASS");
@@ -121,7 +121,7 @@ static void __init test_lwz(void)
regs.gpr[3] = (unsigned long) &a;
/* lwz r5, 0(r3) */
- stepped = emulate_step(®s, TEST_LWZ(5, 3, 0));
+ stepped = emulate_step(®s, TEST_LWZ(5, 3, 0), PPC_NO_SUFFIX);
if (stepped == 1 && regs.gpr[5] == a)
show_result("lwz", "PASS");
@@ -141,7 +141,7 @@ static void __init test_lwzx(void)
regs.gpr[5] = 0x8765;
/* lwzx r5, r3, r4 */
- stepped = emulate_step(®s, TEST_LWZX(5, 3, 4));
+ stepped = emulate_step(®s, TEST_LWZX(5, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && regs.gpr[5] == a[2])
show_result("lwzx", "PASS");
else
@@ -159,7 +159,7 @@ static void __init test_std(void)
regs.gpr[5] = 0x5678;
/* std r5, 0(r3) */
- stepped = emulate_step(®s, TEST_STD(5, 3, 0));
+ stepped = emulate_step(®s, TEST_STD(5, 3, 0), PPC_NO_SUFFIX);
if (stepped == 1 || regs.gpr[5] == a)
show_result("std", "PASS");
else
@@ -184,7 +184,7 @@ static void __init test_ldarx_stdcx(void)
regs.gpr[5] = 0x5678;
/* ldarx r5, r3, r4, 0 */
- stepped = emulate_step(®s, TEST_LDARX(5, 3, 4, 0));
+ stepped = emulate_step(®s, TEST_LDARX(5, 3, 4, 0), PPC_NO_SUFFIX);
/*
* Don't touch 'a' here. Touching 'a' can do Load/store
@@ -202,7 +202,7 @@ static void __init test_ldarx_stdcx(void)
regs.gpr[5] = 0x9ABC;
/* stdcx. r5, r3, r4 */
- stepped = emulate_step(®s, TEST_STDCX(5, 3, 4));
+ stepped = emulate_step(®s, TEST_STDCX(5, 3, 4), PPC_NO_SUFFIX);
/*
* Two possible scenarios that indicates successful emulation
@@ -242,7 +242,7 @@ static void __init test_lfsx_stfsx(void)
regs.gpr[4] = 0;
/* lfsx frt10, r3, r4 */
- stepped = emulate_step(®s, TEST_LFSX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_LFSX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1)
show_result("lfsx", "PASS");
@@ -255,7 +255,7 @@ static void __init test_lfsx_stfsx(void)
c.a = 678.91;
/* stfsx frs10, r3, r4 */
- stepped = emulate_step(®s, TEST_STFSX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_STFSX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && c.b == cached_b)
show_result("stfsx", "PASS");
@@ -285,7 +285,7 @@ static void __init test_lfdx_stfdx(void)
regs.gpr[4] = 0;
/* lfdx frt10, r3, r4 */
- stepped = emulate_step(®s, TEST_LFDX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_LFDX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1)
show_result("lfdx", "PASS");
@@ -298,7 +298,7 @@ static void __init test_lfdx_stfdx(void)
c.a = 987654.32;
/* stfdx frs10, r3, r4 */
- stepped = emulate_step(®s, TEST_STFDX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_STFDX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && c.b == cached_b)
show_result("stfdx", "PASS");
@@ -344,7 +344,7 @@ static void __init test_lvx_stvx(void)
regs.gpr[4] = 0;
/* lvx vrt10, r3, r4 */
- stepped = emulate_step(®s, TEST_LVX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_LVX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1)
show_result("lvx", "PASS");
@@ -360,7 +360,7 @@ static void __init test_lvx_stvx(void)
c.b[3] = 498532;
/* stvx vrs10, r3, r4 */
- stepped = emulate_step(®s, TEST_STVX(10, 3, 4));
+ stepped = emulate_step(®s, TEST_STVX(10, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && cached_b[0] == c.b[0] && cached_b[1] == c.b[1] &&
cached_b[2] == c.b[2] && cached_b[3] == c.b[3])
@@ -401,7 +401,7 @@ static void __init test_lxvd2x_stxvd2x(void)
regs.gpr[4] = 0;
/* lxvd2x vsr39, r3, r4 */
- stepped = emulate_step(®s, TEST_LXVD2X(39, 3, 4));
+ stepped = emulate_step(®s, TEST_LXVD2X(39, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && cpu_has_feature(CPU_FTR_VSX)) {
show_result("lxvd2x", "PASS");
@@ -421,7 +421,7 @@ static void __init test_lxvd2x_stxvd2x(void)
c.b[3] = 4;
/* stxvd2x vsr39, r3, r4 */
- stepped = emulate_step(®s, TEST_STXVD2X(39, 3, 4));
+ stepped = emulate_step(®s, TEST_STXVD2X(39, 3, 4), PPC_NO_SUFFIX);
if (stepped == 1 && cached_b[0] == c.b[0] && cached_b[1] == c.b[1] &&
cached_b[2] == c.b[2] && cached_b[3] == c.b[3] &&
@@ -848,7 +848,7 @@ static int __init emulate_compute_instr(struct pt_regs *regs,
if (!regs || !instr)
return -EINVAL;
- if (analyse_instr(&op, regs, instr) != 1 ||
+ if (analyse_instr(&op, regs, instr, PPC_NO_SUFFIX) != 1 ||
GETTYPE(op.type) != COMPUTE) {
pr_info("emulation failed, instruction = 0x%08x\n", instr);
return -EFAULT;
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index e8c84d265602..897e512c6379 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -705,7 +705,8 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) == (MSR_IR|MSR_64BIT)) {
bp = at_breakpoint(regs->nip);
if (bp != NULL) {
- int stepped = emulate_step(regs, bp->instr[0]);
+ int stepped = emulate_step(regs, bp->instr[0],
+ PPC_NO_SUFFIX);
if (stepped == 0) {
regs->nip = (unsigned long) &bp->instr[0];
atomic_inc(&bp->ref_count);
@@ -1170,7 +1171,7 @@ static int do_step(struct pt_regs *regs)
/* check we are in 64-bit kernel mode, translation enabled */
if ((regs->msr & (MSR_64BIT|MSR_PR|MSR_IR)) == (MSR_64BIT|MSR_IR)) {
if (mread(regs->nip, &instr, 4) == 4) {
- stepped = emulate_step(regs, instr);
+ stepped = emulate_step(regs, instr, PPC_NO_SUFFIX);
if (stepped < 0) {
printf("Couldn't single-step %s instruction\n",
(IS_RFID(instr)? "rfid": "mtmsrd"));
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 04/14] powerpc sstep: Add support for prefixed load/stores
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
` (2 preceding siblings ...)
2020-02-26 4:07 ` [PATCH v3 03/14] powerpc sstep: Prepare to support prefixed instructions Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 4:07 ` [PATCH v3 05/14] powerpc sstep: Add support for prefixed fixed-point arithmetic Jordan Niethe
` (9 subsequent siblings)
13 siblings, 0 replies; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
This adds emulation support for the following prefixed integer
load/stores:
* Prefixed Load Byte and Zero (plbz)
* Prefixed Load Halfword and Zero (plhz)
* Prefixed Load Halfword Algebraic (plha)
* Prefixed Load Word and Zero (plwz)
* Prefixed Load Word Algebraic (plwa)
* Prefixed Load Doubleword (pld)
* Prefixed Store Byte (pstb)
* Prefixed Store Halfword (psth)
* Prefixed Store Word (pstw)
* Prefixed Store Doubleword (pstd)
* Prefixed Load Quadword (plq)
* Prefixed Store Quadword (pstq)
the follow prefixed floating-point load/stores:
* Prefixed Load Floating-Point Single (plfs)
* Prefixed Load Floating-Point Double (plfd)
* Prefixed Store Floating-Point Single (pstfs)
* Prefixed Store Floating-Point Double (pstfd)
and for the following prefixed VSX load/stores:
* Prefixed Load VSX Scalar Doubleword (plxsd)
* Prefixed Load VSX Scalar Single-Precision (plxssp)
* Prefixed Load VSX Vector [0|1] (plxv, plxv0, plxv1)
* Prefixed Store VSX Scalar Doubleword (pstxsd)
* Prefixed Store VSX Scalar Single-Precision (pstxssp)
* Prefixed Store VSX Vector [0|1] (pstxv, pstxv0, pstxv1)
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Combine all load/store patches
- Fix the name of Type 01 instructions
- Remove sign extension flag from pstd/pld
- Rename sufx -> suffix
v3: - Move prefixed loads and stores into the switch statement
---
arch/powerpc/lib/sstep.c | 159 +++++++++++++++++++++++++++++++++++++++
1 file changed, 159 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index efbe72370670..8e4ec953e279 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -187,6 +187,44 @@ static nokprobe_inline unsigned long xform_ea(unsigned int instr,
return ea;
}
+/*
+ * Calculate effective address for a MLS:D-form / 8LS:D-form
+ * prefixed instruction
+ */
+static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned int instr,
+ unsigned int suffix,
+ const struct pt_regs *regs)
+{
+ int ra, prefix_r;
+ unsigned int dd;
+ unsigned long ea, d0, d1, d;
+
+ prefix_r = instr & (1ul << 20);
+ ra = (suffix >> 16) & 0x1f;
+
+ d0 = instr & 0x3ffff;
+ d1 = suffix & 0xffff;
+ d = (d0 << 16) | d1;
+
+ /*
+ * sign extend a 34 bit number
+ */
+ dd = (unsigned int)(d >> 2);
+ ea = (signed int)dd;
+ ea = (ea << 2) | (d & 0x3);
+
+ if (!prefix_r && ra)
+ ea += regs->gpr[ra];
+ else if (!prefix_r && !ra)
+ ; /* Leave ea as is */
+ else if (prefix_r && !ra)
+ ea += regs->nip;
+ else if (prefix_r && ra)
+ ; /* Invalid form. Should already be checked for by caller! */
+
+ return ea;
+}
+
/*
* Return the largest power of 2, not greater than sizeof(unsigned long),
* such that x is a multiple of it.
@@ -1166,6 +1204,7 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
unsigned int instr, unsigned int suffix)
{
unsigned int opcode, ra, rb, rc, rd, spr, u;
+ unsigned int suffixopcode, prefixtype, prefix_r;
unsigned long int imm;
unsigned long int val, val2;
unsigned int mb, me, sh;
@@ -2648,6 +2687,126 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
break;
}
break;
+ case 1: /* Prefixed instructions */
+ prefix_r = instr & (1ul << 20);
+ ra = (suffix >> 16) & 0x1f;
+ op->update_reg = ra;
+ rd = (suffix >> 21) & 0x1f;
+ op->reg = rd;
+ op->val = regs->gpr[rd];
+
+ suffixopcode = suffix >> 26;
+ prefixtype = (instr >> 24) & 0x3;
+ switch (prefixtype) {
+ case 0: /* Type 00 Eight-Byte Load/Store */
+ if (prefix_r && ra)
+ break;
+ op->ea = mlsd_8lsd_ea(instr, suffix, regs);
+ switch (suffixopcode) {
+ case 41: /* plwa */
+ op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 4);
+ break;
+ case 42: /* plxsd */
+ op->reg = rd + 32;
+ op->type = MKOP(LOAD_VSX, PREFIXED, 8);
+ op->element_size = 8;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 43: /* plxssp */
+ op->reg = rd + 32;
+ op->type = MKOP(LOAD_VSX, PREFIXED, 4);
+ op->element_size = 8;
+ op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
+ break;
+ case 46: /* pstxsd */
+ op->reg = rd + 32;
+ op->type = MKOP(STORE_VSX, PREFIXED, 8);
+ op->element_size = 8;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 47: /* pstxssp */
+ op->reg = rd + 32;
+ op->type = MKOP(STORE_VSX, PREFIXED, 4);
+ op->element_size = 8;
+ op->vsx_flags = VSX_FPCONV | VSX_CHECK_VEC;
+ break;
+ case 51: /* plxv1 */
+ op->reg += 32;
+
+ /* fallthru */
+ case 50: /* plxv0 */
+ op->type = MKOP(LOAD_VSX, PREFIXED, 16);
+ op->element_size = 16;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 55: /* pstxv1 */
+ op->reg = rd + 32;
+
+ /* fallthru */
+ case 54: /* pstxv0 */
+ op->type = MKOP(STORE_VSX, PREFIXED, 16);
+ op->element_size = 16;
+ op->vsx_flags = VSX_CHECK_VEC;
+ break;
+ case 56: /* plq */
+ op->type = MKOP(LOAD, PREFIXED, 16);
+ break;
+ case 57: /* pld */
+ op->type = MKOP(LOAD, PREFIXED, 8);
+ break;
+ case 60: /* stq */
+ op->type = MKOP(STORE, PREFIXED, 16);
+ break;
+ case 61: /* pstd */
+ op->type = MKOP(STORE, PREFIXED, 8);
+ break;
+ }
+ break;
+ case 1: /* Type 01 Eight-Byte Register-to-Register */
+ break;
+ case 2: /* Type 10 Modified Load/Store */
+ if (prefix_r && ra)
+ break;
+ op->ea = mlsd_8lsd_ea(instr, suffix, regs);
+ switch (suffixopcode) {
+ case 32: /* plwz */
+ op->type = MKOP(LOAD, PREFIXED, 4);
+ break;
+ case 34: /* plbz */
+ op->type = MKOP(LOAD, PREFIXED, 1);
+ break;
+ case 36: /* pstw */
+ op->type = MKOP(STORE, PREFIXED, 4);
+ break;
+ case 38: /* pstb */
+ op->type = MKOP(STORE, PREFIXED, 1);
+ break;
+ case 40: /* plhz */
+ op->type = MKOP(LOAD, PREFIXED, 2);
+ break;
+ case 42: /* plha */
+ op->type = MKOP(LOAD, PREFIXED | SIGNEXT, 2);
+ break;
+ case 44: /* psth */
+ op->type = MKOP(STORE, PREFIXED, 2);
+ break;
+ case 48: /* plfs */
+ op->type = MKOP(LOAD_FP, PREFIXED | FPCONV, 4);
+ break;
+ case 50: /* plfd */
+ op->type = MKOP(LOAD_FP, PREFIXED, 8);
+ break;
+ case 52: /* pstfs */
+ op->type = MKOP(STORE_FP, PREFIXED | FPCONV, 4);
+ break;
+ case 54: /* pstfd */
+ op->type = MKOP(STORE_FP, PREFIXED, 8);
+ break;
+ }
+ break;
+ case 3: /* Type 11 Modified Register-to-Register */
+ break;
+ }
#endif /* __powerpc64__ */
}
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 05/14] powerpc sstep: Add support for prefixed fixed-point arithmetic
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
` (3 preceding siblings ...)
2020-02-26 4:07 ` [PATCH v3 04/14] powerpc sstep: Add support for prefixed load/stores Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 4:07 ` [PATCH v3 06/14] powerpc: Support prefixed instructions in alignment handler Jordan Niethe
` (8 subsequent siblings)
13 siblings, 0 replies; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
This adds emulation support for the following prefixed Fixed-Point
Arithmetic instructions:
* Prefixed Add Immediate (paddi)
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v3: Since we moved the prefixed loads/stores into the load/store switch
statement it no longer makes sense to have paddi in there, so move it
out.
---
arch/powerpc/lib/sstep.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 8e4ec953e279..f2010a3e1e06 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1331,6 +1331,26 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
switch (opcode) {
#ifdef __powerpc64__
+ case 1:
+ prefix_r = instr & (1ul << 20);
+ ra = (suffix >> 16) & 0x1f;
+ rd = (suffix >> 21) & 0x1f;
+ op->reg = rd;
+ op->val = regs->gpr[rd];
+ suffixopcode = suffix >> 26;
+ prefixtype = (instr >> 24) & 0x3;
+ switch (prefixtype) {
+ case 2:
+ if (prefix_r && ra)
+ return 0;
+ switch (suffixopcode) {
+ case 14: /* paddi */
+ op->type = COMPUTE | PREFIXED;
+ op->val = mlsd_8lsd_ea(instr, suffix, regs);
+ goto compute_done;
+ }
+ }
+ break;
case 2: /* tdi */
if (rd & trap_compare(regs->gpr[ra], (short) instr))
goto trap;
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 06/14] powerpc: Support prefixed instructions in alignment handler
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
` (4 preceding siblings ...)
2020-02-26 4:07 ` [PATCH v3 05/14] powerpc sstep: Add support for prefixed fixed-point arithmetic Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 4:07 ` [PATCH v3 07/14] powerpc/traps: Check for prefixed instructions in facility_unavailable_exception() Jordan Niethe
` (7 subsequent siblings)
13 siblings, 0 replies; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
Alignment interrupts can be caused by prefixed instructions accessing
memory. In the alignment handler the instruction that caused the
exception is loaded and attempted emulate. If the instruction is a
prefixed instruction load the prefix and suffix to emulate. After
emulating increment the NIP by 8.
Prefixed instructions are not permitted to cross 64-byte boundaries. If
they do the alignment interrupt is invoked with SRR1 BOUNDARY bit set.
If this occurs send a SIGBUS to the offending process if in user mode.
If in kernel mode call bad_page_fault().
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Move __get_user_instr() and __get_user_instr_inatomic() to this
commit (previously in "powerpc sstep: Prepare to support prefixed
instructions").
- Rename sufx to suffix
- Use a macro for calculating instruction length
v3: Move __get_user_{instr(), instr_inatomic()} up with the other
get_user definitions and remove nested if.
---
arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++++++++++++
arch/powerpc/kernel/align.c | 8 +++++---
arch/powerpc/kernel/traps.c | 21 ++++++++++++++++++++-
3 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 2f500debae21..8903a96cbb4b 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -105,6 +105,31 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
#define __put_user_inatomic(x, ptr) \
__put_user_nosleep((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
+/*
+ * When reading an instruction iff it is a prefix, the suffix needs to be also
+ * loaded.
+ */
+#define __get_user_instr(x, y, ptr) \
+({ \
+ long __gui_ret = 0; \
+ y = 0; \
+ __gui_ret = __get_user(x, ptr); \
+ if (!__gui_ret && IS_PREFIX(x)) \
+ __gui_ret = __get_user(y, ptr + 1); \
+ __gui_ret; \
+})
+
+#define __get_user_instr_inatomic(x, y, ptr) \
+({ \
+ long __gui_ret = 0; \
+ y = 0; \
+ __gui_ret = __get_user_inatomic(x, ptr); \
+ if (!__gui_ret && IS_PREFIX(x)) \
+ __gui_ret = __get_user_inatomic(y, ptr + 1); \
+ __gui_ret; \
+})
+
+
extern long __put_user_bad(void);
/*
diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index ba3bf5c3ab62..4984cf681215 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -293,7 +293,7 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,
int fix_alignment(struct pt_regs *regs)
{
- unsigned int instr;
+ unsigned int instr, suffix;
struct instruction_op op;
int r, type;
@@ -303,13 +303,15 @@ int fix_alignment(struct pt_regs *regs)
*/
CHECK_FULL_REGS(regs);
- if (unlikely(__get_user(instr, (unsigned int __user *)regs->nip)))
+ if (unlikely(__get_user_instr(instr, suffix,
+ (unsigned int __user *)regs->nip)))
return -EFAULT;
if ((regs->msr & MSR_LE) != (MSR_KERNEL & MSR_LE)) {
/* We don't handle PPC little-endian any more... */
if (cpu_has_feature(CPU_FTR_PPC_LE))
return -EIO;
instr = swab32(instr);
+ suffix = swab32(suffix);
}
#ifdef CONFIG_SPE
@@ -334,7 +336,7 @@ int fix_alignment(struct pt_regs *regs)
if ((instr & 0xfc0006fe) == (PPC_INST_COPY & 0xfc0006fe))
return -EIO;
- r = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
+ r = analyse_instr(&op, regs, instr, suffix);
if (r < 0)
return -EINVAL;
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 82a3438300fd..d80b82fc1ae3 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -583,6 +583,10 @@ static inline int check_io_access(struct pt_regs *regs)
#define REASON_ILLEGAL (ESR_PIL | ESR_PUO)
#define REASON_PRIVILEGED ESR_PPR
#define REASON_TRAP ESR_PTR
+#define REASON_PREFIXED 0
+#define REASON_BOUNDARY 0
+
+#define inst_length(reason) 4
/* single-step stuff */
#define single_stepping(regs) (current->thread.debug.dbcr0 & DBCR0_IC)
@@ -597,6 +601,10 @@ static inline int check_io_access(struct pt_regs *regs)
#define REASON_ILLEGAL SRR1_PROGILL
#define REASON_PRIVILEGED SRR1_PROGPRIV
#define REASON_TRAP SRR1_PROGTRAP
+#define REASON_PREFIXED SRR1_PREFIXED
+#define REASON_BOUNDARY SRR1_BOUNDARY
+
+#define inst_length(reason) (((reason) & REASON_PREFIXED) ? 8 : 4)
#define single_stepping(regs) ((regs)->msr & MSR_SE)
#define clear_single_step(regs) ((regs)->msr &= ~MSR_SE)
@@ -1593,11 +1601,20 @@ void alignment_exception(struct pt_regs *regs)
{
enum ctx_state prev_state = exception_enter();
int sig, code, fixed = 0;
+ unsigned long reason;
/* We restore the interrupt state now */
if (!arch_irq_disabled_regs(regs))
local_irq_enable();
+ reason = get_reason(regs);
+
+ if (reason & REASON_BOUNDARY) {
+ sig = SIGBUS;
+ code = BUS_ADRALN;
+ goto bad;
+ }
+
if (tm_abort_check(regs, TM_CAUSE_ALIGNMENT | TM_CAUSE_PERSISTENT))
goto bail;
@@ -1606,7 +1623,8 @@ void alignment_exception(struct pt_regs *regs)
fixed = fix_alignment(regs);
if (fixed == 1) {
- regs->nip += 4; /* skip over emulated instruction */
+ /* skip over emulated instruction */
+ regs->nip += inst_length(reason);
emulate_single_step(regs);
goto bail;
}
@@ -1619,6 +1637,7 @@ void alignment_exception(struct pt_regs *regs)
sig = SIGBUS;
code = BUS_ADRALN;
}
+bad:
if (user_mode(regs))
_exception(sig, regs, code, regs->dar);
else
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 07/14] powerpc/traps: Check for prefixed instructions in facility_unavailable_exception()
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
` (5 preceding siblings ...)
2020-02-26 4:07 ` [PATCH v3 06/14] powerpc: Support prefixed instructions in alignment handler Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 6:49 ` Nicholas Piggin
2020-02-26 4:07 ` [PATCH v3 08/14] powerpc/xmon: Remove store_inst() for patch_instruction() Jordan Niethe
` (6 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
If prefixed instructions are made unavailable by the [H]FSCR, attempting
to use them will cause a facility unavailable exception. Add "PREFIX" to
the facility_strings[].
Currently there are no prefixed instructions that are actually emulated
by emulate_instruction() within facility_unavailable_exception().
However, when caused by a prefixed instructions the SRR1 PREFIXED bit is
set. Prepare for dealing with emulated prefixed instructions by checking
for this bit.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
arch/powerpc/kernel/traps.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index d80b82fc1ae3..cd8b3043c268 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1739,6 +1739,7 @@ void facility_unavailable_exception(struct pt_regs *regs)
[FSCR_TAR_LG] = "TAR",
[FSCR_MSGP_LG] = "MSGP",
[FSCR_SCV_LG] = "SCV",
+ [FSCR_PREFIX_LG] = "PREFIX",
};
char *facility = "unknown";
u64 value;
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 07/14] powerpc/traps: Check for prefixed instructions in facility_unavailable_exception()
2020-02-26 4:07 ` [PATCH v3 07/14] powerpc/traps: Check for prefixed instructions in facility_unavailable_exception() Jordan Niethe
@ 2020-02-26 6:49 ` Nicholas Piggin
2020-02-26 23:52 ` Jordan Niethe
0 siblings, 1 reply; 36+ messages in thread
From: Nicholas Piggin @ 2020-02-26 6:49 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: alistair, dja, bala24
Jordan Niethe's on February 26, 2020 2:07 pm:
> If prefixed instructions are made unavailable by the [H]FSCR, attempting
> to use them will cause a facility unavailable exception. Add "PREFIX" to
> the facility_strings[].
>
> Currently there are no prefixed instructions that are actually emulated
> by emulate_instruction() within facility_unavailable_exception().
> However, when caused by a prefixed instructions the SRR1 PREFIXED bit is
> set. Prepare for dealing with emulated prefixed instructions by checking
> for this bit.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
Oh you've got it here, I would just squash this together with the first
patch.
Thanks,
Nick
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 07/14] powerpc/traps: Check for prefixed instructions in facility_unavailable_exception()
2020-02-26 6:49 ` Nicholas Piggin
@ 2020-02-26 23:52 ` Jordan Niethe
2020-02-28 1:57 ` Nicholas Piggin
0 siblings, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 23:52 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
On Wed, Feb 26, 2020 at 5:53 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on February 26, 2020 2:07 pm:
> > If prefixed instructions are made unavailable by the [H]FSCR, attempting
> > to use them will cause a facility unavailable exception. Add "PREFIX" to
> > the facility_strings[].
> >
> > Currently there are no prefixed instructions that are actually emulated
> > by emulate_instruction() within facility_unavailable_exception().
> > However, when caused by a prefixed instructions the SRR1 PREFIXED bit is
> > set. Prepare for dealing with emulated prefixed instructions by checking
> > for this bit.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>
> Oh you've got it here, I would just squash this together with the first
> patch.
Sure, I'll put them together. When you mentioned a couple more things
to do in traps.c, was it just this? Or is there still more to be done
adding an FSCR?
>
> Thanks,
> Nick
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v3 07/14] powerpc/traps: Check for prefixed instructions in facility_unavailable_exception()
2020-02-26 23:52 ` Jordan Niethe
@ 2020-02-28 1:57 ` Nicholas Piggin
0 siblings, 0 replies; 36+ messages in thread
From: Nicholas Piggin @ 2020-02-28 1:57 UTC (permalink / raw)
To: Jordan Niethe; +Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
Jordan Niethe's on February 27, 2020 9:52 am:
> On Wed, Feb 26, 2020 at 5:53 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Jordan Niethe's on February 26, 2020 2:07 pm:
>> > If prefixed instructions are made unavailable by the [H]FSCR, attempting
>> > to use them will cause a facility unavailable exception. Add "PREFIX" to
>> > the facility_strings[].
>> >
>> > Currently there are no prefixed instructions that are actually emulated
>> > by emulate_instruction() within facility_unavailable_exception().
>> > However, when caused by a prefixed instructions the SRR1 PREFIXED bit is
>> > set. Prepare for dealing with emulated prefixed instructions by checking
>> > for this bit.
>> >
>> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>>
>> Oh you've got it here, I would just squash this together with the first
>> patch.
> Sure, I'll put them together. When you mentioned a couple more things
> to do in traps.c, was it just this? Or is there still more to be done
> adding an FSCR?
Hmm... maybe it's just that one.
Thanks,
Nick
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 08/14] powerpc/xmon: Remove store_inst() for patch_instruction()
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
` (6 preceding siblings ...)
2020-02-26 4:07 ` [PATCH v3 07/14] powerpc/traps: Check for prefixed instructions in facility_unavailable_exception() Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 7:00 ` Nicholas Piggin
2020-02-26 4:07 ` [PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions Jordan Niethe
` (5 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
For modifying instructions in xmon, patch_instruction() can serve the
same role that store_inst() is performing with the advantage of not
being specific to xmon. In some places patch_instruction() is already
being using followed by store_inst(). In these cases just remove the
store_inst(). Otherwise replace store_inst() with patch_instruction().
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
arch/powerpc/xmon/xmon.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 897e512c6379..a673cf55641c 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -325,11 +325,6 @@ static inline void sync(void)
asm volatile("sync; isync");
}
-static inline void store_inst(void *p)
-{
- asm volatile ("dcbst 0,%0; sync; icbi 0,%0; isync" : : "r" (p));
-}
-
static inline void cflush(void *p)
{
asm volatile ("dcbf 0,%0; icbi 0,%0" : : "r" (p));
@@ -882,8 +877,7 @@ static struct bpt *new_breakpoint(unsigned long a)
for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
if (!bp->enabled && atomic_read(&bp->ref_count) == 0) {
bp->address = a;
- bp->instr[1] = bpinstr;
- store_inst(&bp->instr[1]);
+ patch_instruction(&bp->instr[1], bpinstr);
return bp;
}
}
@@ -913,7 +907,7 @@ static void insert_bpts(void)
bp->enabled = 0;
continue;
}
- store_inst(&bp->instr[0]);
+ patch_instruction(&bp->instr[0], bp->instr[0]);
if (bp->enabled & BP_CIABR)
continue;
if (patch_instruction((unsigned int *)bp->address,
@@ -923,7 +917,6 @@ static void insert_bpts(void)
bp->enabled &= ~BP_TRAP;
continue;
}
- store_inst((void *)bp->address);
}
}
@@ -958,8 +951,6 @@ static void remove_bpts(void)
(unsigned int *)bp->address, bp->instr[0]) != 0)
printf("Couldn't remove breakpoint at %lx\n",
bp->address);
- else
- store_inst((void *)bp->address);
}
}
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 08/14] powerpc/xmon: Remove store_inst() for patch_instruction()
2020-02-26 4:07 ` [PATCH v3 08/14] powerpc/xmon: Remove store_inst() for patch_instruction() Jordan Niethe
@ 2020-02-26 7:00 ` Nicholas Piggin
2020-02-26 23:54 ` Jordan Niethe
0 siblings, 1 reply; 36+ messages in thread
From: Nicholas Piggin @ 2020-02-26 7:00 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: alistair, dja, bala24
Jordan Niethe's on February 26, 2020 2:07 pm:
> For modifying instructions in xmon, patch_instruction() can serve the
> same role that store_inst() is performing with the advantage of not
> being specific to xmon. In some places patch_instruction() is already
> being using followed by store_inst(). In these cases just remove the
> store_inst(). Otherwise replace store_inst() with patch_instruction().
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> arch/powerpc/xmon/xmon.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index 897e512c6379..a673cf55641c 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -325,11 +325,6 @@ static inline void sync(void)
> asm volatile("sync; isync");
> }
>
> -static inline void store_inst(void *p)
> -{
> - asm volatile ("dcbst 0,%0; sync; icbi 0,%0; isync" : : "r" (p));
> -}
> -
> static inline void cflush(void *p)
> {
> asm volatile ("dcbf 0,%0; icbi 0,%0" : : "r" (p));
> @@ -882,8 +877,7 @@ static struct bpt *new_breakpoint(unsigned long a)
> for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
> if (!bp->enabled && atomic_read(&bp->ref_count) == 0) {
> bp->address = a;
> - bp->instr[1] = bpinstr;
> - store_inst(&bp->instr[1]);
> + patch_instruction(&bp->instr[1], bpinstr);
> return bp;
> }
> }
> @@ -913,7 +907,7 @@ static void insert_bpts(void)
> bp->enabled = 0;
> continue;
> }
> - store_inst(&bp->instr[0]);
> + patch_instruction(&bp->instr[0], bp->instr[0]);
Hmm that's a bit weird. Can you read instructions into a local variable
first, do the checks on them, then patch them into their execution
location?
Otherwise, good cleanup.
Thanks,
Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 08/14] powerpc/xmon: Remove store_inst() for patch_instruction()
2020-02-26 7:00 ` Nicholas Piggin
@ 2020-02-26 23:54 ` Jordan Niethe
0 siblings, 0 replies; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 23:54 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
On Wed, Feb 26, 2020 at 6:04 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on February 26, 2020 2:07 pm:
> > For modifying instructions in xmon, patch_instruction() can serve the
> > same role that store_inst() is performing with the advantage of not
> > being specific to xmon. In some places patch_instruction() is already
> > being using followed by store_inst(). In these cases just remove the
> > store_inst(). Otherwise replace store_inst() with patch_instruction().
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > arch/powerpc/xmon/xmon.c | 13 ++-----------
> > 1 file changed, 2 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index 897e512c6379..a673cf55641c 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -325,11 +325,6 @@ static inline void sync(void)
> > asm volatile("sync; isync");
> > }
> >
> > -static inline void store_inst(void *p)
> > -{
> > - asm volatile ("dcbst 0,%0; sync; icbi 0,%0; isync" : : "r" (p));
> > -}
> > -
> > static inline void cflush(void *p)
> > {
> > asm volatile ("dcbf 0,%0; icbi 0,%0" : : "r" (p));
> > @@ -882,8 +877,7 @@ static struct bpt *new_breakpoint(unsigned long a)
> > for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
> > if (!bp->enabled && atomic_read(&bp->ref_count) == 0) {
> > bp->address = a;
> > - bp->instr[1] = bpinstr;
> > - store_inst(&bp->instr[1]);
> > + patch_instruction(&bp->instr[1], bpinstr);
> > return bp;
> > }
> > }
> > @@ -913,7 +907,7 @@ static void insert_bpts(void)
> > bp->enabled = 0;
> > continue;
> > }
> > - store_inst(&bp->instr[0]);
> > + patch_instruction(&bp->instr[0], bp->instr[0]);
>
> Hmm that's a bit weird. Can you read instructions into a local variable
> first, do the checks on them, then patch them into their execution
> location?
I agree it is weird, local variables would be better.
>
> Otherwise, good cleanup.
>
> Thanks,
> Nick
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
` (7 preceding siblings ...)
2020-02-26 4:07 ` [PATCH v3 08/14] powerpc/xmon: Remove store_inst() for patch_instruction() Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 7:06 ` Nicholas Piggin
2020-02-26 4:07 ` [PATCH v3 10/14] powerpc/xmon: Dump " Jordan Niethe
` (4 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
A prefixed instruction is composed of a word prefix and a word suffix.
It does not make sense to be able to have a breakpoint on the suffix of
a prefixed instruction, so make this impossible.
When leaving xmon_core() we check to see if we are currently at a
breakpoint. If this is the case, the breakpoint needs to be proceeded
from. Initially emulate_step() is tried, but if this fails then we need
to execute the saved instruction out of line. The NIP is set to the
address of bpt::instr[] for the current breakpoint. bpt::instr[]
contains the instruction replaced by the breakpoint, followed by a trap
instruction. After bpt::instr[0] is executed and we hit the trap we
enter back into xmon_bpt(). We know that if we got here and the offset
indicates we are at bpt::instr[1] then we have just executed out of line
so we can put the NIP back to the instruction after the breakpoint
location and continue on.
Adding prefixed instructions complicates this as the bpt::instr[1] needs
to be used to hold the suffix. To deal with this make bpt::instr[] big
enough for three word instructions. bpt::instr[2] contains the trap,
and in the case of word instructions pad bpt::instr[1] with a noop.
No support for disassembling prefixed instructions.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: Rename sufx to suffix
v3: - Just directly use PPC_INST_NOP
- Typo: plac -> place
- Rename read_inst() to mread_inst(). Do not have it call mread().
---
arch/powerpc/xmon/xmon.c | 90 ++++++++++++++++++++++++++++++++++------
1 file changed, 78 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index a673cf55641c..a73a35aa4a75 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -97,7 +97,8 @@ static long *xmon_fault_jmp[NR_CPUS];
/* Breakpoint stuff */
struct bpt {
unsigned long address;
- unsigned int instr[2];
+ /* Prefixed instructions can not cross 64-byte boundaries */
+ unsigned int instr[3] __aligned(64);
atomic_t ref_count;
int enabled;
unsigned long pad;
@@ -120,6 +121,7 @@ static unsigned bpinstr = 0x7fe00008; /* trap */
static int cmds(struct pt_regs *);
static int mread(unsigned long, void *, int);
static int mwrite(unsigned long, void *, int);
+static int mread_instr(unsigned long, unsigned int *, unsigned int *);
static int handle_fault(struct pt_regs *);
static void byterev(unsigned char *, int);
static void memex(void);
@@ -701,7 +703,7 @@ static int xmon_core(struct pt_regs *regs, int fromipi)
bp = at_breakpoint(regs->nip);
if (bp != NULL) {
int stepped = emulate_step(regs, bp->instr[0],
- PPC_NO_SUFFIX);
+ bp->instr[1]);
if (stepped == 0) {
regs->nip = (unsigned long) &bp->instr[0];
atomic_inc(&bp->ref_count);
@@ -756,8 +758,8 @@ static int xmon_bpt(struct pt_regs *regs)
/* Are we at the trap at bp->instr[1] for some bp? */
bp = in_breakpoint_table(regs->nip, &offset);
- if (bp != NULL && offset == 4) {
- regs->nip = bp->address + 4;
+ if (bp != NULL && (offset == 4 || offset == 8)) {
+ regs->nip = bp->address + offset;
atomic_dec(&bp->ref_count);
return 1;
}
@@ -858,8 +860,9 @@ static struct bpt *in_breakpoint_table(unsigned long nip, unsigned long *offp)
if (off >= sizeof(bpts))
return NULL;
off %= sizeof(struct bpt);
- if (off != offsetof(struct bpt, instr[0])
- && off != offsetof(struct bpt, instr[1]))
+ if (off != offsetof(struct bpt, instr[0]) &&
+ off != offsetof(struct bpt, instr[1]) &&
+ off != offsetof(struct bpt, instr[2]))
return NULL;
*offp = off - offsetof(struct bpt, instr[0]);
return (struct bpt *) (nip - off);
@@ -876,8 +879,16 @@ static struct bpt *new_breakpoint(unsigned long a)
for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
if (!bp->enabled && atomic_read(&bp->ref_count) == 0) {
+ /*
+ * Prefixed instructions are two words, but regular
+ * instructions are only one. Use a nop to pad out the
+ * regular instructions so that we can place the trap
+ * at the same place. For prefixed instructions the nop
+ * will get overwritten during insert_bpts().
+ */
bp->address = a;
- patch_instruction(&bp->instr[1], bpinstr);
+ patch_instruction(&bp->instr[1], PPC_INST_NOP);
+ patch_instruction(&bp->instr[2], bpinstr);
return bp;
}
}
@@ -889,13 +900,15 @@ static struct bpt *new_breakpoint(unsigned long a)
static void insert_bpts(void)
{
int i;
- struct bpt *bp;
+ unsigned int prefix;
+ struct bpt *bp, *bp2;
bp = bpts;
for (i = 0; i < NBPTS; ++i, ++bp) {
if ((bp->enabled & (BP_TRAP|BP_CIABR)) == 0)
continue;
- if (mread(bp->address, &bp->instr[0], 4) != 4) {
+ if (!mread_instr(bp->address, &bp->instr[0],
+ &bp->instr[1])) {
printf("Couldn't read instruction at %lx, "
"disabling breakpoint there\n", bp->address);
bp->enabled = 0;
@@ -907,7 +920,34 @@ static void insert_bpts(void)
bp->enabled = 0;
continue;
}
+ /*
+ * Check the address is not a suffix by looking for a prefix in
+ * front of it.
+ */
+ if ((mread(bp->address - 4, &prefix, 4) == 4) &&
+ IS_PREFIX(prefix)) {
+ printf("Breakpoint at %lx is on the second word of a "
+ "prefixed instruction, disabling it\n",
+ bp->address);
+ bp->enabled = 0;
+ continue;
+ }
+ /*
+ * We might still be a suffix - if the prefix has already been
+ * replaced by a breakpoint we won't catch it with the above
+ * test.
+ */
+ bp2 = at_breakpoint(bp->address - 4);
+ if (bp2 && IS_PREFIX(bp2->instr[0])) {
+ printf("Breakpoint at %lx is on the second word of a "
+ "prefixed instruction, disabling it\n",
+ bp->address);
+ bp->enabled = 0;
+ continue;
+ }
patch_instruction(&bp->instr[0], bp->instr[0]);
+ if (IS_PREFIX(bp->instr[0]))
+ patch_instruction(&bp->instr[1], bp->instr[1]);
if (bp->enabled & BP_CIABR)
continue;
if (patch_instruction((unsigned int *)bp->address,
@@ -1155,14 +1195,14 @@ static int do_step(struct pt_regs *regs)
*/
static int do_step(struct pt_regs *regs)
{
- unsigned int instr;
+ unsigned int instr, suffix;
int stepped;
force_enable_xmon();
/* check we are in 64-bit kernel mode, translation enabled */
if ((regs->msr & (MSR_64BIT|MSR_PR|MSR_IR)) == (MSR_64BIT|MSR_IR)) {
- if (mread(regs->nip, &instr, 4) == 4) {
- stepped = emulate_step(regs, instr, PPC_NO_SUFFIX);
+ if (mread_instr(regs->nip, &instr, &suffix)) {
+ stepped = emulate_step(regs, instr, suffix);
if (stepped < 0) {
printf("Couldn't single-step %s instruction\n",
(IS_RFID(instr)? "rfid": "mtmsrd"));
@@ -2121,6 +2161,32 @@ mwrite(unsigned long adrs, void *buf, int size)
return n;
}
+static int
+mread_instr(unsigned long adrs, unsigned int *instr,
+ unsigned int *suffix)
+{
+ int n;
+
+ n = 0;
+ if (setjmp(bus_error_jmp) == 0) {
+ catch_memory_errors = 1;
+ sync();
+ *instr = *(unsigned int *)adrs;
+ sync();
+ if (!IS_PREFIX(*instr)) {
+ n = 4;
+ } else {
+ *suffix = *(unsigned int *)(adrs + 4);
+ n = 8;
+ }
+ sync();
+ /* wait a little while to see if we get a machine check */
+ __delay(200);
+ }
+ catch_memory_errors = 0;
+ return n;
+}
+
static int fault_type;
static int fault_except;
static char *fault_chars[] = { "--", "**", "##" };
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions
2020-02-26 4:07 ` [PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions Jordan Niethe
@ 2020-02-26 7:06 ` Nicholas Piggin
2020-02-27 0:11 ` Jordan Niethe
0 siblings, 1 reply; 36+ messages in thread
From: Nicholas Piggin @ 2020-02-26 7:06 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: alistair, dja, bala24
Jordan Niethe's on February 26, 2020 2:07 pm:
> A prefixed instruction is composed of a word prefix and a word suffix.
> It does not make sense to be able to have a breakpoint on the suffix of
> a prefixed instruction, so make this impossible.
>
> When leaving xmon_core() we check to see if we are currently at a
> breakpoint. If this is the case, the breakpoint needs to be proceeded
> from. Initially emulate_step() is tried, but if this fails then we need
> to execute the saved instruction out of line. The NIP is set to the
> address of bpt::instr[] for the current breakpoint. bpt::instr[]
> contains the instruction replaced by the breakpoint, followed by a trap
> instruction. After bpt::instr[0] is executed and we hit the trap we
> enter back into xmon_bpt(). We know that if we got here and the offset
> indicates we are at bpt::instr[1] then we have just executed out of line
> so we can put the NIP back to the instruction after the breakpoint
> location and continue on.
>
> Adding prefixed instructions complicates this as the bpt::instr[1] needs
> to be used to hold the suffix. To deal with this make bpt::instr[] big
> enough for three word instructions. bpt::instr[2] contains the trap,
> and in the case of word instructions pad bpt::instr[1] with a noop.
>
> No support for disassembling prefixed instructions.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v2: Rename sufx to suffix
> v3: - Just directly use PPC_INST_NOP
> - Typo: plac -> place
> - Rename read_inst() to mread_inst(). Do not have it call mread().
> ---
> arch/powerpc/xmon/xmon.c | 90 ++++++++++++++++++++++++++++++++++------
> 1 file changed, 78 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index a673cf55641c..a73a35aa4a75 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -97,7 +97,8 @@ static long *xmon_fault_jmp[NR_CPUS];
> /* Breakpoint stuff */
> struct bpt {
> unsigned long address;
> - unsigned int instr[2];
> + /* Prefixed instructions can not cross 64-byte boundaries */
> + unsigned int instr[3] __aligned(64);
This is pretty wild, I didn't realize xmon executes breakpoints out
of line like this.
IMO the break point entries here should correspond with a range of
reserved bytes in .text so we patch instructions into normal executable
pages rather than .data.
Anyway that's for patch.
Thanks,
Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions
2020-02-26 7:06 ` Nicholas Piggin
@ 2020-02-27 0:11 ` Jordan Niethe
2020-02-27 7:14 ` Christophe Leroy
0 siblings, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-27 0:11 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
On Wed, Feb 26, 2020 at 6:10 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on February 26, 2020 2:07 pm:
> > A prefixed instruction is composed of a word prefix and a word suffix.
> > It does not make sense to be able to have a breakpoint on the suffix of
> > a prefixed instruction, so make this impossible.
> >
> > When leaving xmon_core() we check to see if we are currently at a
> > breakpoint. If this is the case, the breakpoint needs to be proceeded
> > from. Initially emulate_step() is tried, but if this fails then we need
> > to execute the saved instruction out of line. The NIP is set to the
> > address of bpt::instr[] for the current breakpoint. bpt::instr[]
> > contains the instruction replaced by the breakpoint, followed by a trap
> > instruction. After bpt::instr[0] is executed and we hit the trap we
> > enter back into xmon_bpt(). We know that if we got here and the offset
> > indicates we are at bpt::instr[1] then we have just executed out of line
> > so we can put the NIP back to the instruction after the breakpoint
> > location and continue on.
> >
> > Adding prefixed instructions complicates this as the bpt::instr[1] needs
> > to be used to hold the suffix. To deal with this make bpt::instr[] big
> > enough for three word instructions. bpt::instr[2] contains the trap,
> > and in the case of word instructions pad bpt::instr[1] with a noop.
> >
> > No support for disassembling prefixed instructions.
> >
> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> > ---
> > v2: Rename sufx to suffix
> > v3: - Just directly use PPC_INST_NOP
> > - Typo: plac -> place
> > - Rename read_inst() to mread_inst(). Do not have it call mread().
> > ---
> > arch/powerpc/xmon/xmon.c | 90 ++++++++++++++++++++++++++++++++++------
> > 1 file changed, 78 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> > index a673cf55641c..a73a35aa4a75 100644
> > --- a/arch/powerpc/xmon/xmon.c
> > +++ b/arch/powerpc/xmon/xmon.c
> > @@ -97,7 +97,8 @@ static long *xmon_fault_jmp[NR_CPUS];
> > /* Breakpoint stuff */
> > struct bpt {
> > unsigned long address;
> > - unsigned int instr[2];
> > + /* Prefixed instructions can not cross 64-byte boundaries */
> > + unsigned int instr[3] __aligned(64);
>
> This is pretty wild, I didn't realize xmon executes breakpoints out
> of line like this.
>
> IMO the break point entries here should correspond with a range of
> reserved bytes in .text so we patch instructions into normal executable
> pages rather than .data.
Would it make sense to use vmalloc_exec() and use that like we are
going to do in kprobes()?
>
> Anyway that's for patch.
>
> Thanks,
> Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions
2020-02-27 0:11 ` Jordan Niethe
@ 2020-02-27 7:14 ` Christophe Leroy
2020-02-28 0:37 ` Jordan Niethe
0 siblings, 1 reply; 36+ messages in thread
From: Christophe Leroy @ 2020-02-27 7:14 UTC (permalink / raw)
To: Jordan Niethe, Nicholas Piggin
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Daniel Axtens
Le 27/02/2020 à 01:11, Jordan Niethe a écrit :
> On Wed, Feb 26, 2020 at 6:10 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Jordan Niethe's on February 26, 2020 2:07 pm:
>>> A prefixed instruction is composed of a word prefix and a word suffix.
>>> It does not make sense to be able to have a breakpoint on the suffix of
>>> a prefixed instruction, so make this impossible.
>>>
>>> When leaving xmon_core() we check to see if we are currently at a
>>> breakpoint. If this is the case, the breakpoint needs to be proceeded
>>> from. Initially emulate_step() is tried, but if this fails then we need
>>> to execute the saved instruction out of line. The NIP is set to the
>>> address of bpt::instr[] for the current breakpoint. bpt::instr[]
>>> contains the instruction replaced by the breakpoint, followed by a trap
>>> instruction. After bpt::instr[0] is executed and we hit the trap we
>>> enter back into xmon_bpt(). We know that if we got here and the offset
>>> indicates we are at bpt::instr[1] then we have just executed out of line
>>> so we can put the NIP back to the instruction after the breakpoint
>>> location and continue on.
>>>
>>> Adding prefixed instructions complicates this as the bpt::instr[1] needs
>>> to be used to hold the suffix. To deal with this make bpt::instr[] big
>>> enough for three word instructions. bpt::instr[2] contains the trap,
>>> and in the case of word instructions pad bpt::instr[1] with a noop.
>>>
>>> No support for disassembling prefixed instructions.
>>>
>>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>>> ---
>>> v2: Rename sufx to suffix
>>> v3: - Just directly use PPC_INST_NOP
>>> - Typo: plac -> place
>>> - Rename read_inst() to mread_inst(). Do not have it call mread().
>>> ---
>>> arch/powerpc/xmon/xmon.c | 90 ++++++++++++++++++++++++++++++++++------
>>> 1 file changed, 78 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>>> index a673cf55641c..a73a35aa4a75 100644
>>> --- a/arch/powerpc/xmon/xmon.c
>>> +++ b/arch/powerpc/xmon/xmon.c
>>> @@ -97,7 +97,8 @@ static long *xmon_fault_jmp[NR_CPUS];
>>> /* Breakpoint stuff */
>>> struct bpt {
>>> unsigned long address;
>>> - unsigned int instr[2];
>>> + /* Prefixed instructions can not cross 64-byte boundaries */
>>> + unsigned int instr[3] __aligned(64);
>>
>> This is pretty wild, I didn't realize xmon executes breakpoints out
>> of line like this.
Neither did I. That's problematic. Kernel data is mapped NX on some
platforms.
>>
>> IMO the break point entries here should correspond with a range of
>> reserved bytes in .text so we patch instructions into normal executable
>> pages rather than .data.
> Would it make sense to use vmalloc_exec() and use that like we are
> going to do in kprobes()?
As we are (already) doing in kprobes() you mean ?
In fact kprobes uses module_alloc(), and it works because kprobe depends
on module. On some platforms (i.e. book3s/32) vmalloc space is marked NX
in segment registers when CONFIG_MODULES is not set, see
mmu_mark_initmem_nx(). On other ones the Instruction TLB miss exception
does not manage misses at kernel addresses when CONFIG_MODULES is not
selected.
So if we want XMON to work at all time, we need to use some (linear)
text address and use patch_instruction() to change it.
Christophe
>>
>> Anyway that's for patch.
>>
>> Thanks,
>> Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions
2020-02-27 7:14 ` Christophe Leroy
@ 2020-02-28 0:37 ` Jordan Niethe
2020-02-28 1:23 ` Nicholas Piggin
0 siblings, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-28 0:37 UTC (permalink / raw)
To: Christophe Leroy
Cc: Alistair Popple, Balamuruhan S, linuxppc-dev, Nicholas Piggin,
Daniel Axtens
On Thu, Feb 27, 2020 at 6:14 PM Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>
>
>
> Le 27/02/2020 à 01:11, Jordan Niethe a écrit :
> > On Wed, Feb 26, 2020 at 6:10 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >>
> >> Jordan Niethe's on February 26, 2020 2:07 pm:
> >>> A prefixed instruction is composed of a word prefix and a word suffix.
> >>> It does not make sense to be able to have a breakpoint on the suffix of
> >>> a prefixed instruction, so make this impossible.
> >>>
> >>> When leaving xmon_core() we check to see if we are currently at a
> >>> breakpoint. If this is the case, the breakpoint needs to be proceeded
> >>> from. Initially emulate_step() is tried, but if this fails then we need
> >>> to execute the saved instruction out of line. The NIP is set to the
> >>> address of bpt::instr[] for the current breakpoint. bpt::instr[]
> >>> contains the instruction replaced by the breakpoint, followed by a trap
> >>> instruction. After bpt::instr[0] is executed and we hit the trap we
> >>> enter back into xmon_bpt(). We know that if we got here and the offset
> >>> indicates we are at bpt::instr[1] then we have just executed out of line
> >>> so we can put the NIP back to the instruction after the breakpoint
> >>> location and continue on.
> >>>
> >>> Adding prefixed instructions complicates this as the bpt::instr[1] needs
> >>> to be used to hold the suffix. To deal with this make bpt::instr[] big
> >>> enough for three word instructions. bpt::instr[2] contains the trap,
> >>> and in the case of word instructions pad bpt::instr[1] with a noop.
> >>>
> >>> No support for disassembling prefixed instructions.
> >>>
> >>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> >>> ---
> >>> v2: Rename sufx to suffix
> >>> v3: - Just directly use PPC_INST_NOP
> >>> - Typo: plac -> place
> >>> - Rename read_inst() to mread_inst(). Do not have it call mread().
> >>> ---
> >>> arch/powerpc/xmon/xmon.c | 90 ++++++++++++++++++++++++++++++++++------
> >>> 1 file changed, 78 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> >>> index a673cf55641c..a73a35aa4a75 100644
> >>> --- a/arch/powerpc/xmon/xmon.c
> >>> +++ b/arch/powerpc/xmon/xmon.c
> >>> @@ -97,7 +97,8 @@ static long *xmon_fault_jmp[NR_CPUS];
> >>> /* Breakpoint stuff */
> >>> struct bpt {
> >>> unsigned long address;
> >>> - unsigned int instr[2];
> >>> + /* Prefixed instructions can not cross 64-byte boundaries */
> >>> + unsigned int instr[3] __aligned(64);
> >>
> >> This is pretty wild, I didn't realize xmon executes breakpoints out
> >> of line like this.
>
> Neither did I. That's problematic. Kernel data is mapped NX on some
> platforms.
>
> >>
> >> IMO the break point entries here should correspond with a range of
> >> reserved bytes in .text so we patch instructions into normal executable
> >> pages rather than .data.
> > Would it make sense to use vmalloc_exec() and use that like we are
> > going to do in kprobes()?
>
> As we are (already) doing in kprobes() you mean ?
Sorry for the confusion, I was mainly thinking of the patch that you
pointed out before:
https://patchwork.ozlabs.org/patch/1232619/
>
> In fact kprobes uses module_alloc(), and it works because kprobe depends
> on module. On some platforms (i.e. book3s/32) vmalloc space is marked NX
> in segment registers when CONFIG_MODULES is not set, see
> mmu_mark_initmem_nx(). On other ones the Instruction TLB miss exception
> does not manage misses at kernel addresses when CONFIG_MODULES is not
> selected.
>
> So if we want XMON to work at all time, we need to use some (linear)
> text address and use patch_instruction() to change it.
Thank you for the detailed clarification, I will do it like that.
>
> Christophe
>
> >>
> >> Anyway that's for patch.
> >>
> >> Thanks,
> >> Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions
2020-02-28 0:37 ` Jordan Niethe
@ 2020-02-28 1:23 ` Nicholas Piggin
0 siblings, 0 replies; 36+ messages in thread
From: Nicholas Piggin @ 2020-02-28 1:23 UTC (permalink / raw)
To: Christophe Leroy, Jordan Niethe
Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
Jordan Niethe's on February 28, 2020 10:37 am:
> On Thu, Feb 27, 2020 at 6:14 PM Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
>>
>>
>>
>> Le 27/02/2020 à 01:11, Jordan Niethe a écrit :
>> > On Wed, Feb 26, 2020 at 6:10 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>> >>
>> >> Jordan Niethe's on February 26, 2020 2:07 pm:
>> >>> A prefixed instruction is composed of a word prefix and a word suffix.
>> >>> It does not make sense to be able to have a breakpoint on the suffix of
>> >>> a prefixed instruction, so make this impossible.
>> >>>
>> >>> When leaving xmon_core() we check to see if we are currently at a
>> >>> breakpoint. If this is the case, the breakpoint needs to be proceeded
>> >>> from. Initially emulate_step() is tried, but if this fails then we need
>> >>> to execute the saved instruction out of line. The NIP is set to the
>> >>> address of bpt::instr[] for the current breakpoint. bpt::instr[]
>> >>> contains the instruction replaced by the breakpoint, followed by a trap
>> >>> instruction. After bpt::instr[0] is executed and we hit the trap we
>> >>> enter back into xmon_bpt(). We know that if we got here and the offset
>> >>> indicates we are at bpt::instr[1] then we have just executed out of line
>> >>> so we can put the NIP back to the instruction after the breakpoint
>> >>> location and continue on.
>> >>>
>> >>> Adding prefixed instructions complicates this as the bpt::instr[1] needs
>> >>> to be used to hold the suffix. To deal with this make bpt::instr[] big
>> >>> enough for three word instructions. bpt::instr[2] contains the trap,
>> >>> and in the case of word instructions pad bpt::instr[1] with a noop.
>> >>>
>> >>> No support for disassembling prefixed instructions.
>> >>>
>> >>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>> >>> ---
>> >>> v2: Rename sufx to suffix
>> >>> v3: - Just directly use PPC_INST_NOP
>> >>> - Typo: plac -> place
>> >>> - Rename read_inst() to mread_inst(). Do not have it call mread().
>> >>> ---
>> >>> arch/powerpc/xmon/xmon.c | 90 ++++++++++++++++++++++++++++++++++------
>> >>> 1 file changed, 78 insertions(+), 12 deletions(-)
>> >>>
>> >>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> >>> index a673cf55641c..a73a35aa4a75 100644
>> >>> --- a/arch/powerpc/xmon/xmon.c
>> >>> +++ b/arch/powerpc/xmon/xmon.c
>> >>> @@ -97,7 +97,8 @@ static long *xmon_fault_jmp[NR_CPUS];
>> >>> /* Breakpoint stuff */
>> >>> struct bpt {
>> >>> unsigned long address;
>> >>> - unsigned int instr[2];
>> >>> + /* Prefixed instructions can not cross 64-byte boundaries */
>> >>> + unsigned int instr[3] __aligned(64);
>> >>
>> >> This is pretty wild, I didn't realize xmon executes breakpoints out
>> >> of line like this.
>>
>> Neither did I. That's problematic. Kernel data is mapped NX on some
>> platforms.
>>
>> >>
>> >> IMO the break point entries here should correspond with a range of
>> >> reserved bytes in .text so we patch instructions into normal executable
>> >> pages rather than .data.
>> > Would it make sense to use vmalloc_exec() and use that like we are
>> > going to do in kprobes()?
>>
>> As we are (already) doing in kprobes() you mean ?
> Sorry for the confusion, I was mainly thinking of the patch that you
> pointed out before:
> https://patchwork.ozlabs.org/patch/1232619/
>>
>> In fact kprobes uses module_alloc(), and it works because kprobe depends
>> on module. On some platforms (i.e. book3s/32) vmalloc space is marked NX
>> in segment registers when CONFIG_MODULES is not set, see
>> mmu_mark_initmem_nx(). On other ones the Instruction TLB miss exception
>> does not manage misses at kernel addresses when CONFIG_MODULES is not
>> selected.
>>
>> So if we want XMON to work at all time, we need to use some (linear)
>> text address and use patch_instruction() to change it.
> Thank you for the detailed clarification, I will do it like that.
Yeah I would just make a little array in .text.xmon_bpts or something,
which should do the trick.
That wouldn't depend on this series, but if you want to do it as a fix
up patch before it, that would be good.
Thanks,
Nick
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 10/14] powerpc/xmon: Dump prefixed instructions
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
` (8 preceding siblings ...)
2020-02-26 4:07 ` [PATCH v3 09/14] powerpc/xmon: Add initial support for prefixed instructions Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 4:07 ` [PATCH v3 11/14] powerpc/kprobes: Support kprobes on " Jordan Niethe
` (3 subsequent siblings)
13 siblings, 0 replies; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
Currently when xmon is dumping instructions it reads a word at a time
and then prints that instruction (either as a hex number or by
disassembling it). For prefixed instructions it would be nice to show
its prefix and suffix as together. Use read_instr() so that if a prefix
is encountered its suffix is loaded too. Then print these in the form:
prefix:suffix
Xmon uses the disassembly routines from GNU binutils. These currently do
not support prefixed instructions so we will not disassemble the
prefixed instructions yet.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: Rename sufx to suffix
v3: Simplify generic_inst_dump()
---
arch/powerpc/xmon/xmon.c | 38 ++++++++++++++++++++++++++++++--------
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index a73a35aa4a75..bf304189e33a 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -2900,6 +2900,21 @@ prdump(unsigned long adrs, long ndump)
}
}
+static bool instrs_are_equal(unsigned long insta, unsigned long suffixa,
+ unsigned long instb, unsigned long suffixb)
+{
+ if (insta != instb)
+ return false;
+
+ if (!IS_PREFIX(insta) && !IS_PREFIX(instb))
+ return true;
+
+ if (IS_PREFIX(insta) && IS_PREFIX(instb))
+ return suffixa == suffixb;
+
+ return false;
+}
+
typedef int (*instruction_dump_func)(unsigned long inst, unsigned long addr);
static int
@@ -2908,12 +2923,11 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
{
int nr, dotted;
unsigned long first_adr;
- unsigned int inst, last_inst = 0;
- unsigned char val[4];
+ unsigned int inst, suffix, last_inst = 0, last_suffix = 0;
dotted = 0;
- for (first_adr = adr; count > 0; --count, adr += 4) {
- nr = mread(adr, val, 4);
+ for (first_adr = adr; count > 0; --count, adr += nr) {
+ nr = mread_instr(adr, &inst, &suffix);
if (nr == 0) {
if (praddr) {
const char *x = fault_chars[fault_type];
@@ -2921,8 +2935,9 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
}
break;
}
- inst = GETWORD(val);
- if (adr > first_adr && inst == last_inst) {
+ if (adr > first_adr && instrs_are_equal(inst, suffix,
+ last_inst,
+ last_suffix)) {
if (!dotted) {
printf(" ...\n");
dotted = 1;
@@ -2931,10 +2946,17 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
}
dotted = 0;
last_inst = inst;
- if (praddr)
+ last_suffix = suffix;
+ if (praddr) {
printf(REG" %.8x", adr, inst);
+ if (IS_PREFIX(inst))
+ printf(":%.8x", suffix);
+ }
printf("\t");
- dump_func(inst, adr);
+ if (IS_PREFIX(inst))
+ printf("%.8x:%.8x", inst, suffix);
+ else
+ dump_func(inst, adr);
printf("\n");
}
return adr - first_adr;
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
` (9 preceding siblings ...)
2020-02-26 4:07 ` [PATCH v3 10/14] powerpc/xmon: Dump " Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 7:14 ` Nicholas Piggin
2020-02-26 4:07 ` [PATCH v3 12/14] powerpc/uprobes: Add support for " Jordan Niethe
` (2 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
A prefixed instruction is composed of a word prefix followed by a word
suffix. It does not make sense to be able to have a kprobe on the suffix
of a prefixed instruction, so make this impossible.
Kprobes work by replacing an instruction with a trap and saving that
instruction to be single stepped out of place later. Currently there is
not enough space allocated to keep a prefixed instruction for single
stepping. Increase the amount of space allocated for holding the
instruction copy.
kprobe_post_handler() expects all instructions to be 4 bytes long which
means that it does not function correctly for prefixed instructions.
Add checks for prefixed instructions which will use a length of 8 bytes
instead.
For optprobes we normally patch in loading the instruction we put a
probe on into r4 before calling emulate_step(). We now make space and
patch in loading the suffix into r5 as well.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v3: - Base on top of https://patchwork.ozlabs.org/patch/1232619/
- Change printing format to %x:%x
---
arch/powerpc/include/asm/kprobes.h | 5 ++--
arch/powerpc/kernel/kprobes.c | 43 +++++++++++++++++++++-------
arch/powerpc/kernel/optprobes.c | 32 ++++++++++++---------
arch/powerpc/kernel/optprobes_head.S | 6 ++++
4 files changed, 60 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h
index 66b3f2983b22..0d44ce8a3163 100644
--- a/arch/powerpc/include/asm/kprobes.h
+++ b/arch/powerpc/include/asm/kprobes.h
@@ -38,12 +38,13 @@ extern kprobe_opcode_t optprobe_template_entry[];
extern kprobe_opcode_t optprobe_template_op_address[];
extern kprobe_opcode_t optprobe_template_call_handler[];
extern kprobe_opcode_t optprobe_template_insn[];
+extern kprobe_opcode_t optprobe_template_suffix[];
extern kprobe_opcode_t optprobe_template_call_emulate[];
extern kprobe_opcode_t optprobe_template_ret[];
extern kprobe_opcode_t optprobe_template_end[];
-/* Fixed instruction size for powerpc */
-#define MAX_INSN_SIZE 1
+/* Prefixed instructions are two words */
+#define MAX_INSN_SIZE 2
#define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */
#define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry)
#define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 6b2e9e37f12b..9ccf1b9a1275 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -117,16 +117,28 @@ void *alloc_insn_page(void)
int arch_prepare_kprobe(struct kprobe *p)
{
int ret = 0;
+ struct kprobe *prev;
kprobe_opcode_t insn = *p->addr;
+ kprobe_opcode_t prefix = *(p->addr - 1);
+ preempt_disable();
if ((unsigned long)p->addr & 0x03) {
printk("Attempt to register kprobe at an unaligned address\n");
ret = -EINVAL;
} else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
ret = -EINVAL;
+ } else if (IS_PREFIX(prefix)) {
+ printk("Cannot register a kprobe on the second word of prefixed instruction\n");
+ ret = -EINVAL;
+ }
+ prev = get_kprobe(p->addr - 1);
+ if (prev && IS_PREFIX(*prev->ainsn.insn)) {
+ printk("Cannot register a kprobe on the second word of prefixed instruction\n");
+ ret = -EINVAL;
}
+
/* insn must be on a special executable page on ppc64. This is
* not explicitly required on ppc32 (right now), but it doesn't hurt */
if (!ret) {
@@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
}
if (!ret) {
- patch_instruction(p->ainsn.insn, *p->addr);
+ patch_instruction(&p->ainsn.insn[0], p->addr[0]);
+ if (IS_PREFIX(insn))
+ patch_instruction(&p->ainsn.insn[1], p->addr[1]);
p->opcode = *p->addr;
}
p->ainsn.boostable = 0;
+ preempt_enable_no_resched();
return ret;
}
NOKPROBE_SYMBOL(arch_prepare_kprobe);
@@ -225,10 +240,11 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe);
static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
{
int ret;
- unsigned int insn = *p->ainsn.insn;
+ unsigned int insn = p->ainsn.insn[0];
+ unsigned int suffix = p->ainsn.insn[1];
/* regs->nip is also adjusted if emulate_step returns 1 */
- ret = emulate_step(regs, insn, PPC_NO_SUFFIX);
+ ret = emulate_step(regs, insn, suffix);
if (ret > 0) {
/*
* Once this instruction has been boosted
@@ -242,7 +258,11 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
* So, we should never get here... but, its still
* good to catch them, just in case...
*/
- printk("Can't step on instruction %x\n", insn);
+ if (!IS_PREFIX(insn))
+ printk("Can't step on instruction %x\n", insn);
+ else
+ printk("Can't step on instruction %x:%x\n", insn,
+ suffix);
BUG();
} else {
/*
@@ -284,7 +304,7 @@ int kprobe_handler(struct pt_regs *regs)
if (kprobe_running()) {
p = get_kprobe(addr);
if (p) {
- kprobe_opcode_t insn = *p->ainsn.insn;
+ kprobe_opcode_t insn = p->ainsn.insn[0];
if (kcb->kprobe_status == KPROBE_HIT_SS &&
is_trap(insn)) {
/* Turn off 'trace' bits */
@@ -457,9 +477,10 @@ static int trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs)
* the link register properly so that the subsequent 'blr' in
* kretprobe_trampoline jumps back to the right instruction.
*
- * For nip, we should set the address to the previous instruction since
- * we end up emulating it in kprobe_handler(), which increments the nip
- * again.
+ * To keep the nip at the correct address we need to counter the
+ * increment that happens when we emulate the kretprobe_trampoline noop
+ * in kprobe_handler(). We do this by decrementing the address by the
+ * length of the noop which is always 4 bytes.
*/
regs->nip = orig_ret_address - 4;
regs->link = orig_ret_address;
@@ -487,12 +508,14 @@ int kprobe_post_handler(struct pt_regs *regs)
{
struct kprobe *cur = kprobe_running();
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+ kprobe_opcode_t insn;
if (!cur || user_mode(regs))
return 0;
+ insn = *cur->ainsn.insn;
/* make sure we got here for instruction we have a kprobe on */
- if (((unsigned long)cur->ainsn.insn + 4) != regs->nip)
+ if ((unsigned long)cur->ainsn.insn + PPC_INST_LENGTH(insn) != regs->nip)
return 0;
if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
@@ -501,7 +524,7 @@ int kprobe_post_handler(struct pt_regs *regs)
}
/* Adjust nip to after the single-stepped instruction */
- regs->nip = (unsigned long)cur->addr + 4;
+ regs->nip = (unsigned long)cur->addr + PPC_INST_LENGTH(insn);
regs->msr |= kcb->kprobe_saved_msr;
/*Restore back the original saved kprobes variables and continue. */
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index f908d9422557..60cf8e8485ab 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -27,6 +27,8 @@
(optprobe_template_op_address - optprobe_template_entry)
#define TMPL_INSN_IDX \
(optprobe_template_insn - optprobe_template_entry)
+#define TMPL_SUFX_IDX \
+ (optprobe_template_suffix - optprobe_template_entry)
#define TMPL_END_IDX \
(optprobe_template_end - optprobe_template_entry)
@@ -100,8 +102,8 @@ static unsigned long can_optimize(struct kprobe *p)
* and that can be emulated.
*/
if (!is_conditional_branch(*p->ainsn.insn) &&
- analyse_instr(&op, ®s, *p->ainsn.insn,
- PPC_NO_SUFFIX) == 1) {
+ analyse_instr(&op, ®s, p->ainsn.insn[0],
+ p->ainsn.insn[1]) == 1) {
emulate_update_regs(®s, &op);
nip = regs.nip;
}
@@ -141,27 +143,27 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
}
/*
- * emulate_step() requires insn to be emulated as
- * second parameter. Load register 'r4' with the
- * instruction.
+ * emulate_step() requires insn to be emulated as second parameter, and the
+ * suffix as the third parameter. Load these into registers.
*/
-void patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr)
+static void patch_imm32_load_insns(int reg, unsigned int val,
+ kprobe_opcode_t *addr)
{
- /* addis r4,0,(insn)@h */
- patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(4) |
+ /* addis reg,0,(insn)@h */
+ patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(reg) |
((val >> 16) & 0xffff));
addr++;
- /* ori r4,r4,(insn)@l */
- patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(4) |
- ___PPC_RS(4) | (val & 0xffff));
+ /* ori reg,reg,(insn)@l */
+ patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(reg) |
+ ___PPC_RS(reg) | (val & 0xffff));
}
/*
* Generate instructions to load provided immediate 64-bit value
* to register 'r3' and patch these instructions at 'addr'.
*/
-void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
+static void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
{
/* lis r3,(op)@highest */
patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(3) |
@@ -267,9 +269,11 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step);
/*
- * 3. load instruction to be emulated into relevant register, and
+ * 3. load instruction and suffix to be emulated into the relevant
+ * registers, and
*/
- patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX);
+ patch_imm32_load_insns(4, p->ainsn.insn[0], buff + TMPL_INSN_IDX);
+ patch_imm32_load_insns(5, p->ainsn.insn[1], buff + TMPL_SUFX_IDX);
/*
* 4. branch back from trampoline
diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S
index cf383520843f..395d1643f59d 100644
--- a/arch/powerpc/kernel/optprobes_head.S
+++ b/arch/powerpc/kernel/optprobes_head.S
@@ -95,6 +95,12 @@ optprobe_template_insn:
nop
nop
+ .global optprobe_template_suffix
+optprobe_template_suffix:
+ /* Pass suffix to be emulated in r5 */
+ nop
+ nop
+
.global optprobe_template_call_emulate
optprobe_template_call_emulate:
/* Branch to emulate_step() */
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
2020-02-26 4:07 ` [PATCH v3 11/14] powerpc/kprobes: Support kprobes on " Jordan Niethe
@ 2020-02-26 7:14 ` Nicholas Piggin
2020-02-27 0:58 ` Jordan Niethe
0 siblings, 1 reply; 36+ messages in thread
From: Nicholas Piggin @ 2020-02-26 7:14 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: alistair, dja, bala24
Jordan Niethe's on February 26, 2020 2:07 pm:
> @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
> }
>
> if (!ret) {
> - patch_instruction(p->ainsn.insn, *p->addr);
> + patch_instruction(&p->ainsn.insn[0], p->addr[0]);
> + if (IS_PREFIX(insn))
> + patch_instruction(&p->ainsn.insn[1], p->addr[1]);
> p->opcode = *p->addr;
Not to single out this hunk or this patch even, but what do you reckon
about adding an instruction data type, and then use that in all these
call sites rather than adding the extra arg or doing the extra copy
manually in each place depending on prefix?
instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
etc., would all take this new instr. Places that open code a memory
access like your MCE change need some accessor
instr = *(unsigned int *)(instr_addr);
- if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
+ if (IS_PREFIX(instr))
+ suffix = *(unsigned int *)(instr_addr + 4);
Becomes
read_instr(instr_addr, &instr);
if (!analyse_instr(&op, &tmp, instr)) ...
etc.
Thanks,
Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
2020-02-26 7:14 ` Nicholas Piggin
@ 2020-02-27 0:58 ` Jordan Niethe
2020-02-28 1:47 ` Nicholas Piggin
0 siblings, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-27 0:58 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on February 26, 2020 2:07 pm:
> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
> > }
> >
> > if (!ret) {
> > - patch_instruction(p->ainsn.insn, *p->addr);
> > + patch_instruction(&p->ainsn.insn[0], p->addr[0]);
> > + if (IS_PREFIX(insn))
> > + patch_instruction(&p->ainsn.insn[1], p->addr[1]);
> > p->opcode = *p->addr;
>
> Not to single out this hunk or this patch even, but what do you reckon
> about adding an instruction data type, and then use that in all these
> call sites rather than adding the extra arg or doing the extra copy
> manually in each place depending on prefix?
>
> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
> etc., would all take this new instr. Places that open code a memory
> access like your MCE change need some accessor
>
> instr = *(unsigned int *)(instr_addr);
> - if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
> + if (IS_PREFIX(instr))
> + suffix = *(unsigned int *)(instr_addr + 4);
>
> Becomes
> read_instr(instr_addr, &instr);
> if (!analyse_instr(&op, &tmp, instr)) ...
>
> etc.
Daniel Axtens also talked about this and my reasons not to do so were
pretty unconvincing, so I started trying something like this. One
thing I have been wondering is how pervasive should the new type be.
Below is data type I have started using, which I think works
reasonably for replacing unsigned ints everywhere (like within
code-patching.c). In a few architecture independent places such as
uprobes which want to do ==, etc the union type does not work so well.
I will have the next revision of the series start using a type.
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
new file mode 100644
index 000000000000..50adb3dbdeb4
--- /dev/null
+++ b/arch/powerpc/include/asm/inst.h
@@ -0,0 +1,87 @@
+
+#ifndef _ASM_INST_H
+#define _ASM_INST_H
+
+#ifdef __powerpc64__
+
+/* 64 bit Instruction */
+
+typedef struct {
+ unsigned int prefix;
+ unsigned int suffix;
+} __packed ppc_prefixed_inst;
+
+typedef union ppc_inst {
+ unsigned int w;
+ ppc_prefixed_inst p;
+} ppc_inst;
+
+#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
+#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
sizeof((inst).p) : sizeof((inst).w))
+
+#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
+#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
.p.suffix = (0x60000000) })
+#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
.p.suffix = (y) })
+
+#define PPC_INST_WORD(x) ((x).w)
+#define PPC_INST_PREFIX(x) (x.p.prefix)
+#define PPC_INST_SUFFIX(x) (x.p.suffix)
+#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
+
+#define DEREF_PPC_INST_PTR(ptr) \
+({ \
+ ppc_inst __inst; \
+ __inst.w = *(unsigned int *)(ptr); \
+ if (PPC_INST_IS_PREFIXED(__inst)) \
+ __inst.p = *(ppc_prefixed_inst *)(ptr); \
+ __inst; \
+})
+
+#define PPC_INST_NEXT(ptr) ((ptr) += PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
+#define PPC_INST_PREV(ptr) ((ptr) -= PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
+
+#define PPC_INST_EQ(x, y) \
+({ \
+ long pic_ret = 0; \
+ pic_ret = (PPC_INST_PREFIX(x) == PPC_INST_PREFIX(y)); \
+ if (pic_ret) { \
+ if (PPC_INST_IS_PREFIXED(x) && PPC_INST_IS_PREFIXED(y)) { \
+ pic_ret = (PPC_INST_SUFFIX(x) == PPC_INST_SUFFIX(y)); \
+ } else { \
+ pic_ret = 0; \
+ } \
+ } \
+ pic_ret; \
+})
+
+#else /* !__powerpc64__ */
+
+/* 32 bit Instruction */
+
+typedef unsigned int ppc_inst;
+
+#define PPC_INST_IS_PREFIXED(inst) (0)
+#define PPC_INST_LEN(inst) (4)
+
+#define PPC_INST_NEW_WORD(x) (x)
+#define PPC_INST_NEW_WORD_PAD(x) (x)
+#define PPC_INST_NEW_PREFIXED(x, y) (x)
+
+#define PPC_INST_WORD(x) (x)
+#define PPC_INST_PREFIX(x) (x)
+#define PPC_INST_SUFFIX(x) (0)
+#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
+
+#define DEREF_PPC_INST_PTR(ptr) (*ptr)
+
+#define PPC_INST_NEXT(ptr) ((ptr) += 4)
+#define PPC_INST_PREV(ptr) ((ptr) -= 4)
+
+#define PPC_INST_EQ(x, y) ((x) == (y))
+
+#endif /* __powerpc64__ */
+
+
+#endif /* _ASM_INST_H */
>
> Thanks,
> Nick
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
2020-02-27 0:58 ` Jordan Niethe
@ 2020-02-28 1:47 ` Nicholas Piggin
2020-02-28 1:56 ` Nicholas Piggin
2020-02-28 3:23 ` Jordan Niethe
0 siblings, 2 replies; 36+ messages in thread
From: Nicholas Piggin @ 2020-02-28 1:47 UTC (permalink / raw)
To: Jordan Niethe; +Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
Jordan Niethe's on February 27, 2020 10:58 am:
> On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Jordan Niethe's on February 26, 2020 2:07 pm:
>> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
>> > }
>> >
>> > if (!ret) {
>> > - patch_instruction(p->ainsn.insn, *p->addr);
>> > + patch_instruction(&p->ainsn.insn[0], p->addr[0]);
>> > + if (IS_PREFIX(insn))
>> > + patch_instruction(&p->ainsn.insn[1], p->addr[1]);
>> > p->opcode = *p->addr;
>>
>> Not to single out this hunk or this patch even, but what do you reckon
>> about adding an instruction data type, and then use that in all these
>> call sites rather than adding the extra arg or doing the extra copy
>> manually in each place depending on prefix?
>>
>> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
>> etc., would all take this new instr. Places that open code a memory
>> access like your MCE change need some accessor
>>
>> instr = *(unsigned int *)(instr_addr);
>> - if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
>> + if (IS_PREFIX(instr))
>> + suffix = *(unsigned int *)(instr_addr + 4);
>>
>> Becomes
>> read_instr(instr_addr, &instr);
>> if (!analyse_instr(&op, &tmp, instr)) ...
>>
>> etc.
> Daniel Axtens also talked about this and my reasons not to do so were
> pretty unconvincing, so I started trying something like this. One
Okay.
> thing I have been wondering is how pervasive should the new type be.
I wouldn't mind it being quite pervasive. We have to be careful not
to copy it directly to/from memory now, but if we have accessors to
do all that with, I think it should be okay.
> Below is data type I have started using, which I think works
> reasonably for replacing unsigned ints everywhere (like within
> code-patching.c). In a few architecture independent places such as
> uprobes which want to do ==, etc the union type does not work so well.
There will be some places you have to make the boundary. I would start
by just making it a powerpc thing, but possibly there is or could be
some generic helpers. How does something like x86 cope with this?
> I will have the next revision of the series start using a type.
Thanks for doing that.
>
> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> new file mode 100644
> index 000000000000..50adb3dbdeb4
> --- /dev/null
> +++ b/arch/powerpc/include/asm/inst.h
> @@ -0,0 +1,87 @@
> +
> +#ifndef _ASM_INST_H
> +#define _ASM_INST_H
> +
> +#ifdef __powerpc64__
> +
> +/* 64 bit Instruction */
> +
> +typedef struct {
> + unsigned int prefix;
> + unsigned int suffix;
u32?
> +} __packed ppc_prefixed_inst;
> +
> +typedef union ppc_inst {
> + unsigned int w;
> + ppc_prefixed_inst p;
> +} ppc_inst;
I'd make it a struct and use nameless structs/unions inside it (with
appropriate packed annotation):
struct ppc_inst {
union {
struct {
u32 word;
u32 pad;
};
struct {
u32 prefix;
u32 suffix;
};
};
};
> +
> +#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
> +#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
> sizeof((inst).p) : sizeof((inst).w))
Good accessors, I'd make them all C inline functions and lower case.
> +
> +#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
> +#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
> .p.suffix = (0x60000000) })
> +#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
> .p.suffix = (y) })
If these are widely used, I'd make them a bit shorter
#define PPC_INST(x)
#define PPC_INST_PREFIXED(x)
I'd also set padding to something invalid 0 or 0xffffffff for the first
case, and have your accessors check that rather than carrying around
another type of ppc_inst (prefixed, padded, non-padded).
> +
> +#define PPC_INST_WORD(x) ((x).w)
> +#define PPC_INST_PREFIX(x) (x.p.prefix)
> +#define PPC_INST_SUFFIX(x) (x.p.suffix)
> +#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
I'd avoid simple accessors like this completely. If they have any use
it would be to ensure you don't try to use prefix/suffix on a
non-prefixed instruction for example. If you want to do that I'd use
inline functions for it.
> +
> +#define DEREF_PPC_INST_PTR(ptr) \
> +({ \
> + ppc_inst __inst; \
> + __inst.w = *(unsigned int *)(ptr); \
> + if (PPC_INST_IS_PREFIXED(__inst)) \
> + __inst.p = *(ppc_prefixed_inst *)(ptr); \
> + __inst; \
> +})
I'd go an inline with shorter lowercase names. ppc_inst_read(&inst);
> +#define PPC_INST_NEXT(ptr) ((ptr) += PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
> +#define PPC_INST_PREV(ptr) ((ptr) -= PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
Wouldn't bother with this accessor, caller could ptr += ppc_inst_len(inst)
> +#define PPC_INST_EQ(x, y) \
> +({ \
> + long pic_ret = 0; \
> + pic_ret = (PPC_INST_PREFIX(x) == PPC_INST_PREFIX(y)); \
> + if (pic_ret) { \
> + if (PPC_INST_IS_PREFIXED(x) && PPC_INST_IS_PREFIXED(y)) { \
> + pic_ret = (PPC_INST_SUFFIX(x) == PPC_INST_SUFFIX(y)); \
> + } else { \
> + pic_ret = 0; \
> + } \
> + } \
> + pic_ret; \
> +})
static inline bool ppc_inst_eq(struct ppc_inst x, struct ppc_inst y)
{
return !memcmp(&x, &y, sizeof(struct ppc_inst));
}
If all accessors and initalisers are such that the padding gets set,
that'll work.
Thanks,
Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
2020-02-28 1:47 ` Nicholas Piggin
@ 2020-02-28 1:56 ` Nicholas Piggin
2020-02-28 3:23 ` Jordan Niethe
1 sibling, 0 replies; 36+ messages in thread
From: Nicholas Piggin @ 2020-02-28 1:56 UTC (permalink / raw)
To: Jordan Niethe; +Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
Nicholas Piggin's on February 28, 2020 11:47 am:
> Jordan Niethe's on February 27, 2020 10:58 am:
>> On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>> +
>> +#define DEREF_PPC_INST_PTR(ptr) \
>> +({ \
>> + ppc_inst __inst; \
>> + __inst.w = *(unsigned int *)(ptr); \
>> + if (PPC_INST_IS_PREFIXED(__inst)) \
>> + __inst.p = *(ppc_prefixed_inst *)(ptr); \
>> + __inst; \
>> +})
>
> I'd go an inline with shorter lowercase names. ppc_inst_read(&inst);
Probably inst = ppc_inst_read(mem), rather.
Thanks,
Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
2020-02-28 1:47 ` Nicholas Piggin
2020-02-28 1:56 ` Nicholas Piggin
@ 2020-02-28 3:23 ` Jordan Niethe
2020-02-28 4:53 ` Nicholas Piggin
1 sibling, 1 reply; 36+ messages in thread
From: Jordan Niethe @ 2020-02-28 3:23 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
On Fri, Feb 28, 2020 at 12:48 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> Jordan Niethe's on February 27, 2020 10:58 am:
> > On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >>
> >> Jordan Niethe's on February 26, 2020 2:07 pm:
> >> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
> >> > }
> >> >
> >> > if (!ret) {
> >> > - patch_instruction(p->ainsn.insn, *p->addr);
> >> > + patch_instruction(&p->ainsn.insn[0], p->addr[0]);
> >> > + if (IS_PREFIX(insn))
> >> > + patch_instruction(&p->ainsn.insn[1], p->addr[1]);
> >> > p->opcode = *p->addr;
> >>
> >> Not to single out this hunk or this patch even, but what do you reckon
> >> about adding an instruction data type, and then use that in all these
> >> call sites rather than adding the extra arg or doing the extra copy
> >> manually in each place depending on prefix?
> >>
> >> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
> >> etc., would all take this new instr. Places that open code a memory
> >> access like your MCE change need some accessor
> >>
> >> instr = *(unsigned int *)(instr_addr);
> >> - if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
> >> + if (IS_PREFIX(instr))
> >> + suffix = *(unsigned int *)(instr_addr + 4);
> >>
> >> Becomes
> >> read_instr(instr_addr, &instr);
> >> if (!analyse_instr(&op, &tmp, instr)) ...
> >>
> >> etc.
> > Daniel Axtens also talked about this and my reasons not to do so were
> > pretty unconvincing, so I started trying something like this. One
>
> Okay.
>
> > thing I have been wondering is how pervasive should the new type be.
>
> I wouldn't mind it being quite pervasive. We have to be careful not
> to copy it directly to/from memory now, but if we have accessors to
> do all that with, I think it should be okay.
>
> > Below is data type I have started using, which I think works
> > reasonably for replacing unsigned ints everywhere (like within
> > code-patching.c). In a few architecture independent places such as
> > uprobes which want to do ==, etc the union type does not work so well.
>
> There will be some places you have to make the boundary. I would start
> by just making it a powerpc thing, but possibly there is or could be
> some generic helpers. How does something like x86 cope with this?
One of the places I was thinking of was is_swbp_insn() in
kernel/events/uprobes.c. The problem was I wanted to typedef
uprobe_opcode_t as ppc_insn type which was probably the wrong thing to
do. x86 typedef's it as u8 (size of the trap they use). So we probably
can do the same thing and just keep it as a u32.
>
> > I will have the next revision of the series start using a type.
>
> Thanks for doing that.
>
> >
> > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> > new file mode 100644
> > index 000000000000..50adb3dbdeb4
> > --- /dev/null
> > +++ b/arch/powerpc/include/asm/inst.h
> > @@ -0,0 +1,87 @@
> > +
> > +#ifndef _ASM_INST_H
> > +#define _ASM_INST_H
> > +
> > +#ifdef __powerpc64__
> > +
> > +/* 64 bit Instruction */
> > +
> > +typedef struct {
> > + unsigned int prefix;
> > + unsigned int suffix;
>
> u32?
Sure.
>
> > +} __packed ppc_prefixed_inst;
> > +
> > +typedef union ppc_inst {
> > + unsigned int w;
> > + ppc_prefixed_inst p;
> > +} ppc_inst;
>
> I'd make it a struct and use nameless structs/unions inside it (with
> appropriate packed annotation):
Yeah that will be nicer.
>
> struct ppc_inst {
> union {
> struct {
> u32 word;
> u32 pad;
> };
> struct {
> u32 prefix;
> u32 suffix;
> };
> };
> };
>
> > +
> > +#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
> > +#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
> > sizeof((inst).p) : sizeof((inst).w))
>
> Good accessors, I'd make them all C inline functions and lower case.
Will change.
>
> > +
> > +#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
> > +#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
> > .p.suffix = (0x60000000) })
> > +#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
> > .p.suffix = (y) })
>
> If these are widely used, I'd make them a bit shorter
>
> #define PPC_INST(x)
> #define PPC_INST_PREFIXED(x)
Good idea, they do end up used quite widely.
>
> I'd also set padding to something invalid 0 or 0xffffffff for the first
> case, and have your accessors check that rather than carrying around
> another type of ppc_inst (prefixed, padded, non-padded).
>
> > +
> > +#define PPC_INST_WORD(x) ((x).w)
> > +#define PPC_INST_PREFIX(x) (x.p.prefix)
> > +#define PPC_INST_SUFFIX(x) (x.p.suffix)
> > +#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
>
> I'd avoid simple accessors like this completely. If they have any use
> it would be to ensure you don't try to use prefix/suffix on a
> non-prefixed instruction for example. If you want to do that I'd use
> inline functions for it.
What I was thinking with these macros was that they would let us keep
the instruction type as a simple u32 on 32 bit ppc. Is it worth trying
to do that?
>
> > +
> > +#define DEREF_PPC_INST_PTR(ptr) \
> > +({ \
> > + ppc_inst __inst; \
> > + __inst.w = *(unsigned int *)(ptr); \
> > + if (PPC_INST_IS_PREFIXED(__inst)) \
> > + __inst.p = *(ppc_prefixed_inst *)(ptr); \
> > + __inst; \
> > +})
>
> I'd go an inline with shorter lowercase names. ppc_inst_read(&inst);
Will do.
>
> > +#define PPC_INST_NEXT(ptr) ((ptr) += PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
> > +#define PPC_INST_PREV(ptr) ((ptr) -= PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
>
> Wouldn't bother with this accessor, caller could ptr += ppc_inst_len(inst)
Good point.
>
>
> > +#define PPC_INST_EQ(x, y) \
> > +({ \
> > + long pic_ret = 0; \
> > + pic_ret = (PPC_INST_PREFIX(x) == PPC_INST_PREFIX(y)); \
> > + if (pic_ret) { \
> > + if (PPC_INST_IS_PREFIXED(x) && PPC_INST_IS_PREFIXED(y)) { \
> > + pic_ret = (PPC_INST_SUFFIX(x) == PPC_INST_SUFFIX(y)); \
> > + } else { \
> > + pic_ret = 0; \
> > + } \
> > + } \
> > + pic_ret; \
> > +})
>
> static inline bool ppc_inst_eq(struct ppc_inst x, struct ppc_inst y)
> {
> return !memcmp(&x, &y, sizeof(struct ppc_inst));
> }
Well that is definitely cleaner!
Thank you for the feedback.
>
> If all accessors and initalisers are such that the padding gets set,
> that'll work.
>
> Thanks,
> Nick
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
2020-02-28 3:23 ` Jordan Niethe
@ 2020-02-28 4:53 ` Nicholas Piggin
0 siblings, 0 replies; 36+ messages in thread
From: Nicholas Piggin @ 2020-02-28 4:53 UTC (permalink / raw)
To: Jordan Niethe; +Cc: Alistair Popple, Daniel Axtens, linuxppc-dev, Balamuruhan S
Jordan Niethe's on February 28, 2020 1:23 pm:
> On Fri, Feb 28, 2020 at 12:48 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Jordan Niethe's on February 27, 2020 10:58 am:
>> > On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>> >>
>> >> Jordan Niethe's on February 26, 2020 2:07 pm:
>> >> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
>> >> > }
>> >> >
>> >> > if (!ret) {
>> >> > - patch_instruction(p->ainsn.insn, *p->addr);
>> >> > + patch_instruction(&p->ainsn.insn[0], p->addr[0]);
>> >> > + if (IS_PREFIX(insn))
>> >> > + patch_instruction(&p->ainsn.insn[1], p->addr[1]);
>> >> > p->opcode = *p->addr;
>> >>
>> >> Not to single out this hunk or this patch even, but what do you reckon
>> >> about adding an instruction data type, and then use that in all these
>> >> call sites rather than adding the extra arg or doing the extra copy
>> >> manually in each place depending on prefix?
>> >>
>> >> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
>> >> etc., would all take this new instr. Places that open code a memory
>> >> access like your MCE change need some accessor
>> >>
>> >> instr = *(unsigned int *)(instr_addr);
>> >> - if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
>> >> + if (IS_PREFIX(instr))
>> >> + suffix = *(unsigned int *)(instr_addr + 4);
>> >>
>> >> Becomes
>> >> read_instr(instr_addr, &instr);
>> >> if (!analyse_instr(&op, &tmp, instr)) ...
>> >>
>> >> etc.
>> > Daniel Axtens also talked about this and my reasons not to do so were
>> > pretty unconvincing, so I started trying something like this. One
>>
>> Okay.
>>
>> > thing I have been wondering is how pervasive should the new type be.
>>
>> I wouldn't mind it being quite pervasive. We have to be careful not
>> to copy it directly to/from memory now, but if we have accessors to
>> do all that with, I think it should be okay.
>>
>> > Below is data type I have started using, which I think works
>> > reasonably for replacing unsigned ints everywhere (like within
>> > code-patching.c). In a few architecture independent places such as
>> > uprobes which want to do ==, etc the union type does not work so well.
>>
>> There will be some places you have to make the boundary. I would start
>> by just making it a powerpc thing, but possibly there is or could be
>> some generic helpers. How does something like x86 cope with this?
>
> One of the places I was thinking of was is_swbp_insn() in
> kernel/events/uprobes.c. The problem was I wanted to typedef
> uprobe_opcode_t as ppc_insn type which was probably the wrong thing to
> do. x86 typedef's it as u8 (size of the trap they use). So we probably
> can do the same thing and just keep it as a u32.
Sounds good.
>> > I will have the next revision of the series start using a type.
>>
>> Thanks for doing that.
>>
>> >
>> > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
>> > new file mode 100644
>> > index 000000000000..50adb3dbdeb4
>> > --- /dev/null
>> > +++ b/arch/powerpc/include/asm/inst.h
>> > @@ -0,0 +1,87 @@
>> > +
>> > +#ifndef _ASM_INST_H
>> > +#define _ASM_INST_H
>> > +
>> > +#ifdef __powerpc64__
>> > +
>> > +/* 64 bit Instruction */
>> > +
>> > +typedef struct {
>> > + unsigned int prefix;
>> > + unsigned int suffix;
>>
>> u32?
> Sure.
>>
>> > +} __packed ppc_prefixed_inst;
>> > +
>> > +typedef union ppc_inst {
>> > + unsigned int w;
>> > + ppc_prefixed_inst p;
>> > +} ppc_inst;
>>
>> I'd make it a struct and use nameless structs/unions inside it (with
>> appropriate packed annotation):
> Yeah that will be nicer.
>>
>> struct ppc_inst {
>> union {
>> struct {
>> u32 word;
>> u32 pad;
>> };
>> struct {
>> u32 prefix;
>> u32 suffix;
>> };
>> };
>> };
>>
>> > +
>> > +#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
>> > +#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
>> > sizeof((inst).p) : sizeof((inst).w))
>>
>> Good accessors, I'd make them all C inline functions and lower case.
> Will change.
>>
>> > +
>> > +#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
>> > +#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
>> > .p.suffix = (0x60000000) })
>> > +#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
>> > .p.suffix = (y) })
>>
>> If these are widely used, I'd make them a bit shorter
>>
>> #define PPC_INST(x)
>> #define PPC_INST_PREFIXED(x)
> Good idea, they do end up used quite widely.
>>
>> I'd also set padding to something invalid 0 or 0xffffffff for the first
>> case, and have your accessors check that rather than carrying around
>> another type of ppc_inst (prefixed, padded, non-padded).
>>
>> > +
>> > +#define PPC_INST_WORD(x) ((x).w)
>> > +#define PPC_INST_PREFIX(x) (x.p.prefix)
>> > +#define PPC_INST_SUFFIX(x) (x.p.suffix)
>> > +#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
>>
>> I'd avoid simple accessors like this completely. If they have any use
>> it would be to ensure you don't try to use prefix/suffix on a
>> non-prefixed instruction for example. If you want to do that I'd use
>> inline functions for it.
> What I was thinking with these macros was that they would let us keep
> the instruction type as a simple u32 on 32 bit ppc. Is it worth trying
> to do that?
Hmm, it might be. On the other hand if ppc32 can use the same
struct ppc_insn struct it might help share code without too many
macros.
I guess it's up to Christophe and Michael and you I won't complain
any more (so long as they're lower case and inlines where possible :))
Thanks,
Nick
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v3 12/14] powerpc/uprobes: Add support for prefixed instructions
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
` (10 preceding siblings ...)
2020-02-26 4:07 ` [PATCH v3 11/14] powerpc/kprobes: Support kprobes on " Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 4:07 ` [PATCH v3 13/14] powerpc/hw_breakpoints: Initial " Jordan Niethe
2020-02-26 4:07 ` [PATCH v3 14/14] powerpc: Add prefix support to mce_find_instr_ea_and_pfn() Jordan Niethe
13 siblings, 0 replies; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
Uprobes can execute instructions out of line. Increase the size of the
buffer used for this so that this works for prefixed instructions. Take
into account the length of prefixed instructions when fixing up the nip.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Fix typo
- Use macro for instruction length
---
arch/powerpc/include/asm/uprobes.h | 16 ++++++++++++----
arch/powerpc/kernel/uprobes.c | 4 ++--
2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
index 2bbdf27d09b5..5516ab27db47 100644
--- a/arch/powerpc/include/asm/uprobes.h
+++ b/arch/powerpc/include/asm/uprobes.h
@@ -14,18 +14,26 @@
typedef ppc_opcode_t uprobe_opcode_t;
+/*
+ * Ensure we have enough space for prefixed instructions, which
+ * are double the size of a word instruction, i.e. 8 bytes.
+ */
#define MAX_UINSN_BYTES 4
-#define UPROBE_XOL_SLOT_BYTES (MAX_UINSN_BYTES)
+#define UPROBE_XOL_SLOT_BYTES (2 * MAX_UINSN_BYTES)
/* The following alias is needed for reference from arch-agnostic code */
#define UPROBE_SWBP_INSN BREAKPOINT_INSTRUCTION
#define UPROBE_SWBP_INSN_SIZE 4 /* swbp insn size in bytes */
struct arch_uprobe {
+ /*
+ * Ensure there is enough space for prefixed instructions. Prefixed
+ * instructions must not cross 64-byte boundaries.
+ */
union {
- u32 insn;
- u32 ixol;
- };
+ uprobe_opcode_t insn[2];
+ uprobe_opcode_t ixol[2];
+ } __aligned(64);
};
struct arch_uprobe_task {
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index 4ab40c4b576f..7e0334ad5cfe 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -111,7 +111,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
* support doesn't exist and have to fix-up the next instruction
* to be executed.
*/
- regs->nip = utask->vaddr + MAX_UINSN_BYTES;
+ regs->nip = utask->vaddr + PPC_INST_LENGTH(auprobe->insn[0]);
user_disable_single_step(current);
return 0;
@@ -173,7 +173,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
* emulate_step() returns 1 if the insn was successfully emulated.
* For all other cases, we need to single-step in hardware.
*/
- ret = emulate_step(regs, auprobe->insn, PPC_NO_SUFFIX);
+ ret = emulate_step(regs, auprobe->insn[0], auprobe->insn[1]);
if (ret > 0)
return true;
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 13/14] powerpc/hw_breakpoints: Initial support for prefixed instructions
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
` (11 preceding siblings ...)
2020-02-26 4:07 ` [PATCH v3 12/14] powerpc/uprobes: Add support for " Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
2020-02-26 4:07 ` [PATCH v3 14/14] powerpc: Add prefix support to mce_find_instr_ea_and_pfn() Jordan Niethe
13 siblings, 0 replies; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
Currently when getting an instruction to emulate in
hw_breakpoint_handler() we do not load the suffix of a prefixed
instruction. Ensure we load the suffix if the instruction we need to
emulate is a prefixed instruction.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: Rename sufx to suffix
v3: Add __user to type cast to remove sparse warning
---
arch/powerpc/kernel/hw_breakpoint.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 3a7ec6760dab..edf46356dfb2 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -243,15 +243,16 @@ dar_range_overlaps(unsigned long dar, int size, struct arch_hw_breakpoint *info)
static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
struct arch_hw_breakpoint *info)
{
- unsigned int instr = 0;
+ unsigned int instr = 0, suffix = 0;
int ret, type, size;
struct instruction_op op;
unsigned long addr = info->address;
- if (__get_user_inatomic(instr, (unsigned int *)regs->nip))
+ if (__get_user_instr_inatomic(instr, suffix,
+ (unsigned int __user *)regs->nip))
goto fail;
- ret = analyse_instr(&op, regs, instr, PPC_NO_SUFFIX);
+ ret = analyse_instr(&op, regs, instr, suffix);
type = GETTYPE(op.type);
size = GETSIZE(op.type);
@@ -275,7 +276,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
return false;
}
- if (!emulate_step(regs, instr, PPC_NO_SUFFIX))
+ if (!emulate_step(regs, instr, suffix))
goto fail;
return true;
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread* [PATCH v3 14/14] powerpc: Add prefix support to mce_find_instr_ea_and_pfn()
2020-02-26 4:07 [PATCH v3 00/14] Initial Prefixed Instruction support Jordan Niethe
` (12 preceding siblings ...)
2020-02-26 4:07 ` [PATCH v3 13/14] powerpc/hw_breakpoints: Initial " Jordan Niethe
@ 2020-02-26 4:07 ` Jordan Niethe
13 siblings, 0 replies; 36+ messages in thread
From: Jordan Niethe @ 2020-02-26 4:07 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alistair, bala24, Jordan Niethe, dja
mce_find_instr_ea_and_pfn analyses an instruction to determine the
effective address that caused the machine check. Update this to load and
pass the suffix to analyse_instr for prefixed instructions.
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
v2: - Rename sufx to suffix
---
arch/powerpc/kernel/mce_power.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
index 824eda536f5d..091bab4a5464 100644
--- a/arch/powerpc/kernel/mce_power.c
+++ b/arch/powerpc/kernel/mce_power.c
@@ -365,7 +365,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr,
* in real-mode is tricky and can lead to recursive
* faults
*/
- int instr;
+ int instr, suffix = 0;
unsigned long pfn, instr_addr;
struct instruction_op op;
struct pt_regs tmp = *regs;
@@ -374,7 +374,9 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr,
if (pfn != ULONG_MAX) {
instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK);
instr = *(unsigned int *)(instr_addr);
- if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
+ if (IS_PREFIX(instr))
+ suffix = *(unsigned int *)(instr_addr + 4);
+ if (!analyse_instr(&op, &tmp, instr, suffix)) {
pfn = addr_to_pfn(regs, op.ea);
*addr = op.ea;
*phys_addr = (pfn << PAGE_SHIFT);
--
2.17.1
^ permalink raw reply related [flat|nested] 36+ messages in thread