Re: [kvm-unit-tests PATCH v5 10/29] powerpc/sprs: Specify SPRs with data rather than code

["Nicholas Piggin" <npiggin@gmail.com>]

On Tue Dec 19, 2023 at 4:14 PM AEST, Thomas Huth wrote:
> On 16/12/2023 14.42, 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.
> > 
> > Acked-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   powerpc/sprs.c | 643 ++++++++++++++++++++++++++++++++++---------------
> >   1 file changed, 450 insertions(+), 193 deletions(-)
> > 
> > diff --git a/powerpc/sprs.c b/powerpc/sprs.c
> > index 57e487ce..cd8b472d 100644
> > --- a/powerpc/sprs.c
> > +++ b/powerpc/sprs.c
> > @@ -28,231 +28,465 @@
> >   #include <asm/processor.h>
> >   #include <asm/barrier.h>
> >   
> > -uint64_t before[1024], after[1024];
> > -
> > -/* Common SPRs for all PowerPC CPUs */
> > -static void set_sprs_common(uint64_t val)
> > +/* "Indirect" mfspr/mtspr which accept a non-constant spr number */
> > +static uint64_t __mfspr(unsigned spr)
> >   {
> > -	mtspr(9, val);		/* CTR */
> > -	// mtspr(273, val);	/* SPRG1 */  /* Used by our exception handler */
> > -	mtspr(274, val);	/* SPRG2 */
> > -	mtspr(275, val);	/* SPRG3 */
> > +	uint64_t tmp;
> > +	uint64_t ret;
> > +
> > +	asm volatile(
> > +"	bcl	20, 31, 1f		\n"
> > +"1:	mflr	%0			\n"
> > +"	addi	%0, %0, (2f-1b)		\n"
> > +"	add	%0, %0, %2		\n"
> > +"	mtctr	%0			\n"
> > +"	bctr				\n"
> > +"2:					\n"
> > +".LSPR=0				\n"
> > +".rept 1024				\n"
> > +"	mfspr	%1, .LSPR		\n"
> > +"	b	3f			\n"
> > +"	.LSPR=.LSPR+1			\n"
> > +".endr					\n"
> > +"3:					\n"
> > +	: "=&r"(tmp),
> > +	  "=r"(ret)
> > +	: "r"(spr*8) /* 8 bytes per 'mfspr ; b' block */
> > +	: "lr", "ctr");
> > +
> > +	return ret;
> >   }
> >   
> > -/* SPRs from PowerPC Operating Environment Architecture, Book III, Vers. 2.01 */
> > -static void set_sprs_book3s_201(uint64_t val)
> > +static void __mtspr(unsigned spr, 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 */
> > +	uint64_t tmp;
> > +
> > +	asm volatile(
> > +"	bcl	20, 31, 1f		\n"
> > +"1:	mflr	%0			\n"
> > +"	addi	%0, %0, (2f-1b)		\n"
> > +"	add	%0, %0, %2		\n"
> > +"	mtctr	%0			\n"
> > +"	bctr				\n"
> > +"2:					\n"
> > +".LSPR=0				\n"
> > +".rept 1024				\n"
> > +"	mtspr	.LSPR, %1		\n"
> > +"	b	3f			\n"
> > +"	.LSPR=.LSPR+1			\n"
> > +".endr					\n"
> > +"3:					\n"
> > +	: "=&r"(tmp)
> > +	: "r"(val),
> > +	  "r"(spr*8) /* 8 bytes per 'mfspr ; b' block */
> > +	: "lr", "ctr", "xer");
> >   }
> >   
> > +static uint64_t before[1024], after[1024];
> > +
> > +#define SPR_PR_READ	0x0001
> > +#define SPR_PR_WRITE	0x0002
> > +#define SPR_OS_READ	0x0010
> > +#define SPR_OS_WRITE	0x0020
> > +#define SPR_HV_READ	0x0100
> > +#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_INT		0x2000	/* May be updated by synchronous interrupt */
> > +#define SPR_HARNESS	0x4000	/* Test harness uses the register */
> > +
> > +struct spr {
> > +	const char	*name;
> > +	uint8_t		width;
> > +	uint16_t	access;
> > +	uint16_t	type;
> > +};
> > +
> > +/* 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, },
>
> Just a little stylish nit: You've got a space before the closing "}", but no 
> space after the opening "{". Looks a little bit weird to me. Maybe add a 
> space after the "{", too?

Yes that is inconsistent. I'll fix.

Thanks,
Nick