diff mbox series

[kvm-unit-tests,v5,12/29] powerpc/sprs: Avoid taking async interrupts caused by register fuzzing

Message ID 20231216134257.1743345-13-npiggin@gmail.com (mailing list archive)
State Not Applicable
Headers show
Series powerpc: updates, P10, PNV support | expand

Commit Message

Nicholas Piggin Dec. 16, 2023, 1:42 p.m. UTC
Storing certain values in some registers can cause asynchronous
interrupts that can crash the test case, for example decrementer
or PMU interrupts.

Change the msleep to mdelay which does not enable MSR[EE] and so
avoids the problem. This allows removing some of the SPR special
casing.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 powerpc/sprs.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

Comments

Thomas Huth Dec. 19, 2023, 11:47 a.m. UTC | #1
On 16/12/2023 14.42, Nicholas Piggin wrote:
> Storing certain values in some registers can cause asynchronous
> interrupts that can crash the test case, for example decrementer
> or PMU interrupts.
> 
> Change the msleep to mdelay which does not enable MSR[EE] and so
> avoids the problem. This allows removing some of the SPR special
> casing.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   powerpc/sprs.c | 14 ++------------
>   1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/powerpc/sprs.c b/powerpc/sprs.c
> index 01041912..313698e0 100644
> --- a/powerpc/sprs.c
> +++ b/powerpc/sprs.c
> @@ -481,12 +481,7 @@ static void set_sprs(uint64_t val)
>   			continue;
>   		if (sprs[i].type & SPR_HARNESS)
>   			continue;
> -		if (!strcmp(sprs[i].name, "MMCR0")) {
> -			/* XXX: could use a comment or better abstraction! */
> -			__mtspr(i, (val & 0xfffffffffbab3fffULL) | 0xfa0b2070);
> -		} else {
> -			__mtspr(i, val);
> -		}
> +		__mtspr(i, val);
>   	}
>   }
>   
> @@ -536,12 +531,7 @@ int main(int argc, char **argv)
>   	if (pause) {
>   		migrate_once();
>   	} else {
> -		msleep(2000);
> -
> -		/* Taking a dec updates SRR0, SRR1, SPRG1, so don't fail. */
> -		sprs[26].type |= SPR_ASYNC;
> -		sprs[27].type |= SPR_ASYNC;
> -		sprs[273].type |= SPR_ASYNC;
> +		mdelay(2000);
>   	}

IIRC I used the H_CEDE stuff here on purpose to increase the possibility 
that the guest gets rescheduled onto another CPU core on the host, and thus 
that it uncovers sprs that are not saved and restored on the host more 
easily. So I'd rather keep the msleep() here.

  Thomas
Nicholas Piggin Dec. 22, 2023, 9:51 a.m. UTC | #2
On Tue Dec 19, 2023 at 9:47 PM AEST, Thomas Huth wrote:
> On 16/12/2023 14.42, Nicholas Piggin wrote:
> > Storing certain values in some registers can cause asynchronous
> > interrupts that can crash the test case, for example decrementer
> > or PMU interrupts.
> > 
> > Change the msleep to mdelay which does not enable MSR[EE] and so
> > avoids the problem. This allows removing some of the SPR special
> > casing.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   powerpc/sprs.c | 14 ++------------
> >   1 file changed, 2 insertions(+), 12 deletions(-)
> > 
> > diff --git a/powerpc/sprs.c b/powerpc/sprs.c
> > index 01041912..313698e0 100644
> > --- a/powerpc/sprs.c
> > +++ b/powerpc/sprs.c
> > @@ -481,12 +481,7 @@ static void set_sprs(uint64_t val)
> >   			continue;
> >   		if (sprs[i].type & SPR_HARNESS)
> >   			continue;
> > -		if (!strcmp(sprs[i].name, "MMCR0")) {
> > -			/* XXX: could use a comment or better abstraction! */
> > -			__mtspr(i, (val & 0xfffffffffbab3fffULL) | 0xfa0b2070);
> > -		} else {
> > -			__mtspr(i, val);
> > -		}
> > +		__mtspr(i, val);
> >   	}
> >   }
> >   
> > @@ -536,12 +531,7 @@ int main(int argc, char **argv)
> >   	if (pause) {
> >   		migrate_once();
> >   	} else {
> > -		msleep(2000);
> > -
> > -		/* Taking a dec updates SRR0, SRR1, SPRG1, so don't fail. */
> > -		sprs[26].type |= SPR_ASYNC;
> > -		sprs[27].type |= SPR_ASYNC;
> > -		sprs[273].type |= SPR_ASYNC;
> > +		mdelay(2000);
> >   	}
>
> IIRC I used the H_CEDE stuff here on purpose to increase the possibility 
> that the guest gets rescheduled onto another CPU core on the host, and thus 
> that it uncovers sprs that are not saved and restored on the host more 
> easily. So I'd rather keep the msleep() here.

Ah. Not a bad idea. I'll see about making it deal with H_CEDE
instead.

I wonder if there would be a way to stress host CPU migration
in the test harness. Instead of waiting for QEMU to exit, the
script could run taskset in a loop to bounce it around.
Conceptually easy, probably fiddly to wire things up nicely.

I may try to take a look at adding that... but after this
series so it doesn't get any bigger :/

Thanks,
Nick
diff mbox series

Patch

diff --git a/powerpc/sprs.c b/powerpc/sprs.c
index 01041912..313698e0 100644
--- a/powerpc/sprs.c
+++ b/powerpc/sprs.c
@@ -481,12 +481,7 @@  static void set_sprs(uint64_t val)
 			continue;
 		if (sprs[i].type & SPR_HARNESS)
 			continue;
-		if (!strcmp(sprs[i].name, "MMCR0")) {
-			/* XXX: could use a comment or better abstraction! */
-			__mtspr(i, (val & 0xfffffffffbab3fffULL) | 0xfa0b2070);
-		} else {
-			__mtspr(i, val);
-		}
+		__mtspr(i, val);
 	}
 }
 
@@ -536,12 +531,7 @@  int main(int argc, char **argv)
 	if (pause) {
 		migrate_once();
 	} else {
-		msleep(2000);
-
-		/* Taking a dec updates SRR0, SRR1, SPRG1, so don't fail. */
-		sprs[26].type |= SPR_ASYNC;
-		sprs[27].type |= SPR_ASYNC;
-		sprs[273].type |= SPR_ASYNC;
+		mdelay(2000);
 	}
 
 	get_sprs(after);