Re: [kvm-unit-tests v2 06/10] powerpc/sprs: Specify SPRs with data rather than code
On Thu Mar 23, 2023 at 10:36 PM AEST, Thomas Huth wrote:
> On 20/03/2023 08.03, Nicholas Piggin wrote:
> > +/* SPRs common denominator back to PowerPC Operating Environment
> > Architecture */
> > +static const struct spr sprs_common[1024] = {
> > + [1] = {"XER",64, RW, SPR_HARNESS, }, /*
> > Compiler */
> > + [8] = {"LR", 64, RW, SPR_HARNESS, }, /*
> > Compiler, mfspr/mtspr */
> > + [9] = {"CTR",64, RW, SPR_HARNESS, }, /*
> > Compiler, mfspr/mtspr */
> > + [18] = {"DSISR", 32, OS_RW, SPR_INT, },
> > + [19] = {"DAR",64, OS_RW, SPR_INT, },
> > + [26] = {"SRR0", 64, OS_RW, SPR_INT, },
> > + [27] = {"SRR1", 64, OS_RW, SPR_INT, },
> > +[268] = {"TB", 64, RO , SPR_ASYNC, },
> > +[269] = {"TBU",32, RO, SPR_ASYNC, },
> > +[272] = {"SPRG0", 64, OS_RW, SPR_HARNESS, }, /* Int stack */
> > +[273] = {"SPRG1", 64, OS_RW, SPR_HARNESS, }, /* Scratch */
> > +[274] = {"SPRG2", 64, OS_RW, },
> > +[275] = {"SPRG3", 64, OS_RW, },
> > +[287] = {"PVR",32, OS_RO, },
> > +};
>
> Using a size of 1024 for each of these arrays looks weird. Why don't you add
> a "nr" field to struct spr and specify the register number via that field
> instead of using the index into the array as register number?
Oh I meant to reply to this. I did try it that way at first. When it
came manipulating the arrays like merging them or adding and
subtracing some SPRs, it required a bit of code to search, sort, add,
remove, etc. This way takes almost nothing. It is a dumb data structure
but it works okay here.
Thanks,
Nick
Re: [kvm-unit-tests v2 06/10] powerpc/sprs: Specify SPRs with data rather than code
On 20/03/2023 08.03, Nicholas Piggin wrote:
A significant rework that builds an array of 'struct spr', where each
element describes an SPR. This makes various metadata about the SPR
like name and access type easier to carry and use.
Hypervisor privileged registers are described despite not being used
at the moment for completeness, but also the code might one day be
reused for a hypervisor-privileged test.
Signed-off-by: Nicholas Piggin
This ended up a little over-engineered perhaps, but there are lots of
SPRs, lots of access types, lots of changes between processor and ISA
versions, and lots of places they are implemented and used, so lots of
room for mistakes. There is not a good system in place to easily
see that userspace, supervisor, etc., switches perform all the right
SPR context switching so this is a nice test case to have. The sprs test
quickly caught a few QEMU TCG SPR bugs which really motivated me to
improve the SPR coverage.
---
powerpc/sprs.c | 589 +
1 file changed, 394 insertions(+), 195 deletions(-)
diff --git a/powerpc/sprs.c b/powerpc/sprs.c
index db341a9..dd83dac 100644
--- a/powerpc/sprs.c
+++ b/powerpc/sprs.c
@@ -82,231 +82,407 @@ static void mtspr(unsigned spr, uint64_t val)
: "lr", "ctr", "xer");
}
-uint64_t before[1024], after[1024];
+static uint64_t before[1024], after[1024];
-/* Common SPRs for all PowerPC CPUs */
-static void set_sprs_common(uint64_t val)
-{
- // mtspr(9, val); /* CTR */ /* Used by mfspr/mtspr */
- // mtspr(273, val); /* SPRG1 */ /* Used by our exception handler */
- mtspr(274, val);/* SPRG2 */
- mtspr(275, val);/* SPRG3 */
-}
+#define SPR_PR_READ0x0001
+#define SPR_PR_WRITE 0x0002
+#define SPR_OS_READ0x0010
+#define SPR_OS_WRITE 0x0020
+#define SPR_HV_READ0x0100
+#define SPR_HV_WRITE 0x0200
+
+#define RW 0x333
+#define RO 0x111
+#define WO 0x222
+#define OS_RW 0x330
+#define OS_RO 0x110
+#define OS_WO 0x220
+#define HV_RW 0x300
+#define HV_RO 0x100
+#define HV_WO 0x200
+
+#define SPR_ASYNC 0x1000 /* May be updated asynchronously */
+#define SPR_INT0x2000 /* May be updated by synchronous
interrupt */
+#define SPR_HARNESS0x4000 /* Test harness uses the register */
+
+struct spr {
+ const char *name;
+ uint8_t width;
+ uint16_taccess;
+ uint16_ttype;
+};
+
+/* SPRs common denominator back to PowerPC Operating Environment Architecture
*/
+static const struct spr sprs_common[1024] = {
+ [1] = {"XER", 64, RW, SPR_HARNESS, }, /*
Compiler */
+ [8] = {"LR", 64, RW, SPR_HARNESS, }, /*
Compiler, mfspr/mtspr */
+ [9] = {"CTR", 64, RW, SPR_HARNESS, }, /*
Compiler, mfspr/mtspr */
+ [18] = {"DSISR",32, OS_RW, SPR_INT, },
+ [19] = {"DAR", 64, OS_RW, SPR_INT, },
+ [26] = {"SRR0", 64, OS_RW, SPR_INT, },
+ [27] = {"SRR1", 64, OS_RW, SPR_INT, },
+[268] = {"TB", 64, RO , SPR_ASYNC, },
+[269] = {"TBU", 32, RO, SPR_ASYNC, },
+[272] = {"SPRG0",64, OS_RW, SPR_HARNESS, }, /* Int stack */
+[273] = {"SPRG1",64, OS_RW, SPR_HARNESS, }, /* Scratch */
+[274] = {"SPRG2",64, OS_RW, },
+[275] = {"SPRG3",64, OS_RW, },
+[287] = {"PVR", 32, OS_RO, },
+};
Using a size of 1024 for each of these arrays looks weird. Why don't you add
a "nr" field to struct spr and specify the register number via that field
instead of using the index into the array as register number?
Thomas
[kvm-unit-tests v2 06/10] powerpc/sprs: Specify SPRs with data rather than code
A significant rework that builds an array of 'struct spr', where each
element describes an SPR. This makes various metadata about the SPR
like name and access type easier to carry and use.
Hypervisor privileged registers are described despite not being used
at the moment for completeness, but also the code might one day be
reused for a hypervisor-privileged test.
Signed-off-by: Nicholas Piggin
This ended up a little over-engineered perhaps, but there are lots of
SPRs, lots of access types, lots of changes between processor and ISA
versions, and lots of places they are implemented and used, so lots of
room for mistakes. There is not a good system in place to easily
see that userspace, supervisor, etc., switches perform all the right
SPR context switching so this is a nice test case to have. The sprs test
quickly caught a few QEMU TCG SPR bugs which really motivated me to
improve the SPR coverage.
---
powerpc/sprs.c | 589 +
1 file changed, 394 insertions(+), 195 deletions(-)
diff --git a/powerpc/sprs.c b/powerpc/sprs.c
index db341a9..dd83dac 100644
--- a/powerpc/sprs.c
+++ b/powerpc/sprs.c
@@ -82,231 +82,407 @@ static void mtspr(unsigned spr, uint64_t val)
: "lr", "ctr", "xer");
}
-uint64_t before[1024], after[1024];
+static uint64_t before[1024], after[1024];
-/* Common SPRs for all PowerPC CPUs */
-static void set_sprs_common(uint64_t val)
-{
- // mtspr(9, val); /* CTR */ /* Used by mfspr/mtspr */
- // mtspr(273, val); /* SPRG1 */ /* Used by our exception handler */
- mtspr(274, val);/* SPRG2 */
- mtspr(275, val);/* SPRG3 */
-}
+#define SPR_PR_READ0x0001
+#define SPR_PR_WRITE 0x0002
+#define SPR_OS_READ0x0010
+#define SPR_OS_WRITE 0x0020
+#define SPR_HV_READ0x0100
+#define SPR_HV_WRITE 0x0200
+
+#define RW 0x333
+#define RO 0x111
+#define WO 0x222
+#define OS_RW 0x330
+#define OS_RO 0x110
+#define OS_WO 0x220
+#define HV_RW 0x300
+#define HV_RO 0x100
+#define HV_WO 0x200
+
+#define SPR_ASYNC 0x1000 /* May be updated asynchronously */
+#define SPR_INT0x2000 /* May be updated by synchronous
interrupt */
+#define SPR_HARNESS0x4000 /* Test harness uses the register */
+
+struct spr {
+ const char *name;
+ uint8_t width;
+ uint16_taccess;
+ uint16_ttype;
+};
+
+/* SPRs common denominator back to PowerPC Operating Environment Architecture
*/
+static const struct spr sprs_common[1024] = {
+ [1] = {"XER",64, RW, SPR_HARNESS, }, /*
Compiler */
+ [8] = {"LR", 64, RW, SPR_HARNESS, }, /*
Compiler, mfspr/mtspr */
+ [9] = {"CTR",64, RW, SPR_HARNESS, }, /*
Compiler, mfspr/mtspr */
+ [18] = {"DSISR", 32, OS_RW, SPR_INT, },
+ [19] = {"DAR",64, OS_RW, SPR_INT, },
+ [26] = {"SRR0", 64, OS_RW, SPR_INT, },
+ [27] = {"SRR1", 64, OS_RW, SPR_INT, },
+[268] = {"TB", 64, RO , SPR_ASYNC, },
+[269] = {"TBU",32, RO, SPR_ASYNC, },
+[272] = {"SPRG0", 64, OS_RW, SPR_HARNESS, }, /* Int stack */
+[273] = {"SPRG1", 64, OS_RW, SPR_HARNESS, }, /* Scratch */
+[274] = {"SPRG2", 64, OS_RW, },
+[275] = {"SPRG3", 64, OS_RW, },
+[287] = {"PVR",32, OS_RO, },
+};
/* SPRs from PowerPC Operating Environment Architecture, Book III, Vers. 2.01
*/
-static void set_sprs_book3s_201(uint64_t val)
-{
- mtspr(18, val); /* DSISR */
- mtspr(19, val); /* DAR */
- mtspr(152, val);/* CTRL */
- mtspr(256, val);/* VRSAVE */
- mtspr(786, val);/* MMCRA */
- mtspr(795, val);/* MMCR0 */
- mtspr(798, val);/* MMCR1 */
-}
+static const struct spr sprs_201[1024] = {
+ [22] = {"DEC",32, OS_RW, SPR_ASYNC, },
+ [25] = {"SDR1", 64, HV_RW | OS_RO, },
+ [29] = {"ACCR", 64, OS_RW, },
+[136] = {"CTRL", 32, RO, },
+[152] = {"CTRL", 32, OS_WO, },
+[259] = {"SPRG3", 64, RO, },
+/* ASR, EAR omitted */
+[268] = {"TB", 64, RO, },
+[269] = {"TBU",32, RO, },
+[284] = {"TBL",32, HV_WO, },
+[285] = {"TBU",32, HV_WO, },
+[310] = {"HDEC", 32, HV_RW, },
+[1013]= {"DABR", 64, HV_RW | OS_RO, },
+[1023]= {"PIR",32, OS_RO, },
+};
+
+static const struct spr sprs_970_pmu[1024] = {
+/* POWER4+ PMU, should find PPC970 and confirm */
+[770] = {"MMCRA", 64, RO, },
+[771] = {"PMC1", 32, RO, },
+[772] = {"PMC2", 32, RO, },
+[773] = {"PMC3", 32, RO, },
+[774] =
