| Message ID | 20211003012210.1165606-4-npiggin@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | various fixes | expand |
Hello, On 10/3/21 03:22, Nicholas Piggin wrote: > If cpu_relax() is called when not at medium SMT priority, it will lose > the prior priority and return at medium. Add a debug check to catch > this, which would have flagged the previous bug. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > core/cpu.c | 6 ++++++ > include/processor.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/core/cpu.c b/core/cpu.c > index 5c10fc6e8..0f2da1524 100644 > --- a/core/cpu.c > +++ b/core/cpu.c > @@ -80,6 +80,12 @@ unsigned long __attrconst cpu_emergency_stack_top(unsigned int pir) > > void __nomcount cpu_relax(void) > { > + if ((mfspr(SPR_PPR32) >> 18) != 0x4) { Why not use SPR_PPR ? SPR_PPR32 is for the embedded world according to ISA. Thanks, C. > + printf("cpu_relax called when not at medium SMT priority: " > + "PPR[PRI]=0x%lx\n", mfspr(SPR_PPR32) >> 18); > + backtrace(); > + } > + > /* Relax a bit to give sibling threads some breathing space */ > smt_lowest(); > asm volatile("nop; nop; nop; nop;\n" > diff --git a/include/processor.h b/include/processor.h > index 973d7e77b..7a9c49994 100644 > --- a/include/processor.h > +++ b/include/processor.h > @@ -71,6 +71,7 @@ > #define SPR_USRR1 0x1fb /* RW: Ultravisor Save/Restore Register 1 */ > #define SPR_SMFCTRL 0x1ff /* RW: Secure Memory Facility Control */ > #define SPR_PSSCR 0x357 /* RW: Stop status and control (ISA 3) */ > +#define SPR_PPR32 0x382 > #define SPR_TSCR 0x399 > #define SPR_HID0 0x3f0 > #define SPR_HID1 0x3f1 >
Excerpts from Cédric Le Goater's message of November 26, 2021 8:39 pm: > Hello, > > On 10/3/21 03:22, Nicholas Piggin wrote: >> If cpu_relax() is called when not at medium SMT priority, it will lose >> the prior priority and return at medium. Add a debug check to catch >> this, which would have flagged the previous bug. >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> core/cpu.c | 6 ++++++ >> include/processor.h | 1 + >> 2 files changed, 7 insertions(+) >> >> diff --git a/core/cpu.c b/core/cpu.c >> index 5c10fc6e8..0f2da1524 100644 >> --- a/core/cpu.c >> +++ b/core/cpu.c >> @@ -80,6 +80,12 @@ unsigned long __attrconst cpu_emergency_stack_top(unsigned int pir) >> >> void __nomcount cpu_relax(void) >> { >> + if ((mfspr(SPR_PPR32) >> 18) != 0x4) { > > > Why not use SPR_PPR ? SPR_PPR32 is for the embedded world according > to ISA. Hmm I don't see that mentioned in ISA 3.1. I used PPR32 because of the programming note in 3.1 Program Priority Registers which says "The ability to access the low-order half of the PPR (and thus the use of mfppr and mtppr) might be phased out in a future version of the architecture". Thanks, Nick
On 11/27/21 08:39, Nicholas Piggin wrote: > Excerpts from Cédric Le Goater's message of November 26, 2021 8:39 pm: >> Hello, >> >> On 10/3/21 03:22, Nicholas Piggin wrote: >>> If cpu_relax() is called when not at medium SMT priority, it will lose >>> the prior priority and return at medium. Add a debug check to catch >>> this, which would have flagged the previous bug. >>> >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >>> --- >>> core/cpu.c | 6 ++++++ >>> include/processor.h | 1 + >>> 2 files changed, 7 insertions(+) >>> >>> diff --git a/core/cpu.c b/core/cpu.c >>> index 5c10fc6e8..0f2da1524 100644 >>> --- a/core/cpu.c >>> +++ b/core/cpu.c >>> @@ -80,6 +80,12 @@ unsigned long __attrconst cpu_emergency_stack_top(unsigned int pir) >>> >>> void __nomcount cpu_relax(void) >>> { >>> + if ((mfspr(SPR_PPR32) >> 18) != 0x4) { >> >> >> Why not use SPR_PPR ? SPR_PPR32 is for the embedded world according >> to ISA. > > Hmm I don't see that mentioned in ISA 3.1. I used PPR32 because of the > programming note in 3.1 Program Priority Registers which says "The > ability to access the low-order half of the PPR (and thus the use of > mfppr and mtppr) might be phased out in a future version of the > architecture". I was looking at v2.07 because the QEMU powernv8 machine started complaining. We might want to model the PPR reg and the 'or' insn for it. No big deal either, it's just a warning. Thanks, C.
Excerpts from Cédric Le Goater's message of November 27, 2021 7:40 pm: > On 11/27/21 08:39, Nicholas Piggin wrote: >> Excerpts from Cédric Le Goater's message of November 26, 2021 8:39 pm: >>> Hello, >>> >>> On 10/3/21 03:22, Nicholas Piggin wrote: >>>> If cpu_relax() is called when not at medium SMT priority, it will lose >>>> the prior priority and return at medium. Add a debug check to catch >>>> this, which would have flagged the previous bug. >>>> >>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >>>> --- >>>> core/cpu.c | 6 ++++++ >>>> include/processor.h | 1 + >>>> 2 files changed, 7 insertions(+) >>>> >>>> diff --git a/core/cpu.c b/core/cpu.c >>>> index 5c10fc6e8..0f2da1524 100644 >>>> --- a/core/cpu.c >>>> +++ b/core/cpu.c >>>> @@ -80,6 +80,12 @@ unsigned long __attrconst cpu_emergency_stack_top(unsigned int pir) >>>> >>>> void __nomcount cpu_relax(void) >>>> { >>>> + if ((mfspr(SPR_PPR32) >> 18) != 0x4) { >>> >>> >>> Why not use SPR_PPR ? SPR_PPR32 is for the embedded world according >>> to ISA. >> >> Hmm I don't see that mentioned in ISA 3.1. I used PPR32 because of the >> programming note in 3.1 Program Priority Registers which says "The >> ability to access the low-order half of the PPR (and thus the use of >> mfppr and mtppr) might be phased out in a future version of the >> architecture". > > I was looking at v2.07 because the QEMU powernv8 machine started > complaining. We might want to model the PPR reg and the 'or' insn > for it. No big deal either, it's just a warning. Hmm. We can change skiboot to PPR if it's causing issues with existing code. We should update qemu, and some point after that we might eventually change Linux over to use PPR32. Linux of course is the bigger problem than skiboot if the ISA ever wants to deprecate PPR. Thanks, Nick
diff --git a/core/cpu.c b/core/cpu.c index 5c10fc6e8..0f2da1524 100644 --- a/core/cpu.c +++ b/core/cpu.c @@ -80,6 +80,12 @@ unsigned long __attrconst cpu_emergency_stack_top(unsigned int pir) void __nomcount cpu_relax(void) { + if ((mfspr(SPR_PPR32) >> 18) != 0x4) { + printf("cpu_relax called when not at medium SMT priority: " + "PPR[PRI]=0x%lx\n", mfspr(SPR_PPR32) >> 18); + backtrace(); + } + /* Relax a bit to give sibling threads some breathing space */ smt_lowest(); asm volatile("nop; nop; nop; nop;\n" diff --git a/include/processor.h b/include/processor.h index 973d7e77b..7a9c49994 100644 --- a/include/processor.h +++ b/include/processor.h @@ -71,6 +71,7 @@ #define SPR_USRR1 0x1fb /* RW: Ultravisor Save/Restore Register 1 */ #define SPR_SMFCTRL 0x1ff /* RW: Secure Memory Facility Control */ #define SPR_PSSCR 0x357 /* RW: Stop status and control (ISA 3) */ +#define SPR_PPR32 0x382 #define SPR_TSCR 0x399 #define SPR_HID0 0x3f0 #define SPR_HID1 0x3f1
If cpu_relax() is called when not at medium SMT priority, it will lose the prior priority and return at medium. Add a debug check to catch this, which would have flagged the previous bug. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- core/cpu.c | 6 ++++++ include/processor.h | 1 + 2 files changed, 7 insertions(+)