Re: [PATCH (skiboot)] dt: add /cpus/ibm, powerpc-cpu-features device tree bindings
On Mon, Feb 05, 2018 at 11:26:15AM +1000, Nicholas Piggin wrote:
> On Sat, 3 Feb 2018 09:36:20 -0600
> > On Sat, Feb 03, 2018 at 02:27:32PM +1000, Nicholas Piggin wrote:
> > > + /*
> > > + * ISAv3.0B branch instruction and register additions
> > > + * CA32, OV32, mcrxrx, setb
> > > + */
> > > + { "branch-v3",
> >
> > Those aren't branch instructions, they are integer instructions. Some
> > of which use the XER, some of which use CR fields.
>
> Okay, well the register changes are for branch facility registers, I
> guess instructions could move to integer. We have nothing relying on
> exact definition of these things at the moment, so these we can change.
XER is a fixed-point facility register (that's what the "X" means even!)
Thanks,
Segher
Re: [PATCH (skiboot)] dt: add /cpus/ibm, powerpc-cpu-features device tree bindings
On Sat, 3 Feb 2018 09:36:20 -0600
Segher Boessenkool wrote:
> Hi! Some remarks:
Hi Segher,
Thanks for looking, we're a bit constrained in what we should do because
Linux side of the patch is already merged.
>
> On Sat, Feb 03, 2018 at 02:27:32PM +1000, Nicholas Piggin wrote:
> > + /*
> > +* ISAv3.0B deliver a random number instruction (darn)
> > +*/
> > + { "random-number-generator",
>
> That's not such a great name... "darn-instruction" maybe? Just "wait"
> as a name is a bit too short too I guess (and there is "wait-v3", so
> that could be "wait-v2" or so? Or "wait-instruction").
You're probably right, I don't know if it's worth changing now though.
-vX is meant to be an indication of an ISA version that changed or
superseded the older function.
>
> > + /*
> > +* ISAv3.0B branch instruction and register additions
> > +* CA32, OV32, mcrxrx, setb
> > +*/
> > + { "branch-v3",
>
> Those aren't branch instructions, they are integer instructions. Some
> of which use the XER, some of which use CR fields.
Okay, well the register changes are for branch facility registers, I
guess instructions could move to integer. We have nothing relying on
exact definition of these things at the moment, so these we can change.
>
> > +- isa
> > + Usage: required
> > + Value type:
> > + Definition:
> > +
> > + isa that the CPU is currently running in. This provides instruction set
> > + compatibility, less the individual feature nodes. For example, an ISA
> > v3.0
> > + implementation that lacks the "transactional-memory" cpufeature node
> > + should not use transactional memory facilities.
> > +
> > + Value corresponds to the "Power ISA Version" multiplied by 1000.
> > + For example, <3000> corresponds to Version 3.0, <2070> to Version 2.07.
> > + The minor digit is available for revisions.
>
> So the 2.07B, 3.0B (in the code above) should really be 2071 and 3001
> (instead of 2070 and 3000)? Or you could make it hex and have 0x207B :-)
This I think we can change (Linux code is supposed to be largely version
agnostic,
and it looks like it is except one special case that will do the right thing if
we change it.
>
> > +The policy for seleting and configuring which features to advertise and
> > use
>
> Typo ("selecting").
Thanks,
Nick
Re: [PATCH (skiboot)] dt: add /cpus/ibm, powerpc-cpu-features device tree bindings
Hi! Some remarks:
On Sat, Feb 03, 2018 at 02:27:32PM +1000, Nicholas Piggin wrote:
> + /*
> + * ISAv3.0B deliver a random number instruction (darn)
> + */
> + { "random-number-generator",
That's not such a great name... "darn-instruction" maybe? Just "wait"
as a name is a bit too short too I guess (and there is "wait-v3", so
that could be "wait-v2" or so? Or "wait-instruction").
> + /*
> + * ISAv3.0B branch instruction and register additions
> + * CA32, OV32, mcrxrx, setb
> + */
> + { "branch-v3",
Those aren't branch instructions, they are integer instructions. Some
of which use the XER, some of which use CR fields.
> +- isa
> + Usage: required
> + Value type:
> + Definition:
> +
> + isa that the CPU is currently running in. This provides instruction set
> + compatibility, less the individual feature nodes. For example, an ISA v3.0
> + implementation that lacks the "transactional-memory" cpufeature node
> + should not use transactional memory facilities.
> +
> + Value corresponds to the "Power ISA Version" multiplied by 1000.
> + For example, <3000> corresponds to Version 3.0, <2070> to Version 2.07.
> + The minor digit is available for revisions.
So the 2.07B, 3.0B (in the code above) should really be 2071 and 3001
(instead of 2070 and 3000)? Or you could make it hex and have 0x207B :-)
> +The policy for seleting and configuring which features to advertise and use
Typo ("selecting").
Segher
Re: [PATCH (skiboot)] dt: add /cpus/ibm,powerpc-cpu-features device tree bindings
On Sat, 3 Feb 2018 14:27:32 +1000 Nicholas Piggin wrote: > diff --git a/core/cpufeatures.c b/core/cpufeatures.c > new file mode 100644 > index 0..ca9df91f0 > --- /dev/null > +++ b/core/cpufeatures.c > @@ -0,0 +1,932 @@ > +/* Copyright 2017 IBM Corp. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > + * implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +/* > + * This file deals with setup of /cpus/ibm,powerpc-cpu-features dt > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +#define DEBUG Sorry, didn't send the right version after testing. This line should be deleted before going upstream. Thanks, Nick
[PATCH (skiboot)] dt: add /cpus/ibm, powerpc-cpu-features device tree bindings
This is a new CPU feature advertising interface that is fine-grained,
extensible, aware of privilege levels, and gives control of features
to all levels of the stack (firmware, hypervisor, and OS).
The design and binding specification is described in detail in doc/.
Signed-off-by: Nicholas Piggin
---
core/Makefile.inc | 2 +-
core/cpufeatures.c | 932 +
core/device.c | 7 +
core/init.c| 1 +
.../ibm,powerpc-cpu-features/binding.txt | 245 ++
.../ibm,powerpc-cpu-features/design.txt| 157
include/device.h | 1 +
include/skiboot.h | 5 +
8 files changed, 1349 insertions(+), 1 deletion(-)
create mode 100644 core/cpufeatures.c
create mode 100644 doc/device-tree/ibm,powerpc-cpu-features/binding.txt
create mode 100644 doc/device-tree/ibm,powerpc-cpu-features/design.txt
diff --git a/core/Makefile.inc b/core/Makefile.inc
index d6a7269fa..5c1205646 100644
--- a/core/Makefile.inc
+++ b/core/Makefile.inc
@@ -9,7 +9,7 @@ CORE_OBJS += vpd.o hostservices.o platform.o nvram.o
nvram-format.o hmi.o
CORE_OBJS += console-log.o ipmi.o time-utils.o pel.o pool.o errorlog.o
CORE_OBJS += timer.o i2c.o rtc.o flash.o sensor.o ipmi-opal.o
CORE_OBJS += flash-subpartition.o bitmap.o buddy.o pci-quirk.o powercap.o psr.o
-CORE_OBJS += pci-dt-slot.o direct-controls.o
+CORE_OBJS += pci-dt-slot.o direct-controls.o cpufeatures.o
ifeq ($(SKIBOOT_GCOV),1)
CORE_OBJS += gcov-profiling.o
diff --git a/core/cpufeatures.c b/core/cpufeatures.c
new file mode 100644
index 0..ca9df91f0
--- /dev/null
+++ b/core/cpufeatures.c
@@ -0,0 +1,932 @@
+/* Copyright 2017 IBM Corp.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/*
+ * This file deals with setup of /cpus/ibm,powerpc-cpu-features dt
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#define DEBUG
+#ifdef DEBUG
+#define DBG(fmt, a...) prlog(PR_DEBUG, "CPUFT: " fmt, ##a)
+#else
+#define DBG(fmt, a...)
+#endif
+
+/* Device-tree visible constants follow */
+#define ISA_V2_07B 2070
+#define ISA_V3_0B 3000
+
+#define USABLE_PR (1U << 0)
+#define USABLE_OS (1U << 1)
+#define USABLE_HV (1U << 2)
+
+#define HV_SUPPORT_HFSCR (1U << 0)
+#define OS_SUPPORT_FSCR(1U << 0)
+
+/* Following are definitions for the match tables, not the DT binding itself */
+#define ISA_BASE 0
+
+#define HV_NONE0
+#define HV_CUSTOM 1
+#define HV_HFSCR 2
+
+#define OS_NONE0
+#define OS_CUSTOM 1
+#define OS_FSCR2
+
+/* CPU bitmasks for match table */
+#define CPU_P8_DD1 (1U << 0)
+#define CPU_P8_DD2 (1U << 1)
+#define CPU_P9_DD1 (1U << 2)
+#define CPU_P9_DD2 (1U << 3)
+
+#define CPU_P8 (CPU_P8_DD1|CPU_P8_DD2)
+#define CPU_P9 (CPU_P9_DD1|CPU_P9_DD2)
+#define CPU_ALL(CPU_P8|CPU_P9)
+
+struct cpu_feature {
+ const char *name;
+ uint32_t cpus_supported;
+ uint32_t isa;
+ uint32_t usable_privilege;
+ uint32_t hv_support;
+ uint32_t os_support;
+ uint32_t hfscr_bit_nr;
+ uint32_t fscr_bit_nr;
+ uint32_t hwcap_bit_nr;
+ const char *dependencies_names; /* space-delimited names */
+};
+
+/*
+ * The base (or NULL) cpu feature set is the CPU features available
+ * when no child nodes of the /cpus/ibm,powerpc-cpu-features node exist. The
+ * base feature set is POWER8 (ISAv2.07B), less features that are listed
+ * explicitly.
+ *
+ * XXX: currently, the feature dependencies are not necessarily captured
+ * exactly or completely. This is somewhat acceptable because all
+ * implementations must be aware of all these features.
+ */
+static const struct cpu_feature cpu_features_table[] = {
+ /*
+* Big endian as in ISAv2.07B, MSR_LE=0
+*/
+ { "big-endian",
+ CPU_ALL,
+ ISA_BASE, USABLE_HV|USABLE_OS|USABLE_PR,
+ HV_CUSTOM, OS_CUSTOM,
+ -1, -1, -1,
+ NULL, },
+
+ /*
+* Little endian as in ISAv2.07B, MSR_LE=1.
+*
+* When both big and little endian are defined, there is an LPCR ILE
+* bit and implementation specific way to switch HILE mode, MSR_SLE,
+* etc.
+*/
+ { "littl
