Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
> 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
Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
Jordan Niethe's on February 28, 2020 12:52 pm:
> On Wed, Feb 26, 2020 at 5:50 PM Nicholas Piggin wrote:
>>
>> Jordan Niethe's on February 26, 2020 2:07 pm:
>> > From: Alistair Popple
>> >
>> > 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
>> > ---
>> > 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_RWMR0x375 /* 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_FSCR0x099 /* 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
Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
On Wed, Feb 26, 2020 at 5:50 PM Nicholas Piggin wrote:
>
> Jordan Niethe's on February 26, 2020 2:07 pm:
> > From: Alistair Popple
> >
> > 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
> > ---
> > 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_RWMR0x375 /* 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_FSCR0x099 /* 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
Re: [PATCH v3 01/14] powerpc: Enable Prefixed Instructions
Jordan Niethe's on February 26, 2020 2:07 pm:
> From: Alistair Popple
>
> 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
> ---
> 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_RWMR0x375 /* 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_FSCR0x099 /* 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
[PATCH v3 01/14] powerpc: Enable Prefixed Instructions
From: Alistair Popple
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
---
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_LG12 /* Enable System Call Vectored */
#define FSCR_MSGP_LG 10 /* Enable MSGP */
#define FSCR_TAR_LG8 /* 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
