Hi, Sorry for the late reply. > Umm, there's the commit description to document justification for such >changes made and it's not the reviewer's duty to chase every such detail >that has been omitted from a submission. > >FAOD this is a request to include this detail in v3. How does it sound integrate the v3 commit message with: CONFIG_CPU_MICROMIPS has been removed along with the nop instructions. There, NOPs are 2 byte in size, so change the code with 3 _ssnop which are always 4 byte and remove the ifdef. Added ehb to make sure the hazard is always cleared. something more accurate to suggest? Thank you On Tue, Feb 18, 2025 at 4:23 PM Maciej W. Rozycki <macro@xxxxxxxxxxx> wrote: > > On Tue, 18 Feb 2025, Thomas Bogendoerfer wrote: > > > > > > diff --git a/arch/mips/kernel/genex.S b/arch/mips/kernel/genex.S > > > > > index a572ce36a24f..9747b216648f 100644 > > > > > --- a/arch/mips/kernel/genex.S > > > > > +++ b/arch/mips/kernel/genex.S > > > > > @@ -104,25 +104,27 @@ handle_vcei: > > > > > > > > > > __FINIT > > > > > > > > > > - .align 5 /* 32 byte rollback region */ > > > > > + .align 5 > > > > > LEAF(__r4k_wait) > > > > > .set push > > > > > .set noreorder > > > > > - /* start of rollback region */ > > > > > - LONG_L t0, TI_FLAGS($28) > > > > > - nop > > > > > - andi t0, _TIF_NEED_RESCHED > > > > > - bnez t0, 1f > > > > > - nop > > > > > - nop > > > > > - nop > > > > > -#ifdef CONFIG_CPU_MICROMIPS > > > > > - nop > > > > > - nop > > > > > - nop > > > > > - nop > > > > > -#endif > > > > > > > > My quick search didnn't find the reason for the extra NOPs on MICROMIPS, but > > > > they are here for a purpose. I might still need them... > > > The original code needs #ifdef CONFIG_CPU_MICROMIPS because nop in > > > MICROMIPS is 2 bytes, so need another four nop to align. But _ssnop is > > > always 4 bytes, so we can remove #ifdefs. > > > > ic > > This was wrong anyway (and I had arguments about it with people back in > the time, but it was a hopeless battle). Instead `.align ...' or an > equivalent pseudo-op (`.balign', etc.) should have been used, removing the > fragility of this piece or the need for CONFIG_CPU_MICROMIPS conditional. > Here and in other places. > > > > > > + _ssnop > > > > > + _ssnop > > > > > + _ssnop > > > > > > > > instead of handcoded hazard nops, use __irq_enable_hazard for that > > > No, I don't think so, this region should make sure be 32 bytes on each > > > platform, but __irq_enable_hazard is not consistent, 3 _ssnop is the > > > fallback implementation but available for all MIPS. > > > > you are right for most cases, but there is one case > > > > #elif (defined(CONFIG_CPU_MIPSR1) && !defined(CONFIG_MIPS_ALCHEMY)) || \ > > defined(CONFIG_CPU_BMIPS) > > > > which uses > > > > #define __irq_enable_hazard \ > > ___ssnop; \ > > ___ssnop; \ > > ___ssnop; \ > > ___ehb > > > > if MIPSR1 || BMIPS needs "rollback" handler 3 ssnnop would be wrong as > > irq enable hazard. > > Indeed, EHB must always be included, because for R2+ CPUs SSNOP doesn't > clear the hazard (it never clears, but with earlier CPUs it just stalls > the pipeline long enough for the hazard to eventually clear by itself). > > > > > But I doubt this works, because the wait instruction is not aligned to > > > > a 32 byte boundary, but the code assuemes this, IMHO. > > > Why? After this patch we only use 4 byte instructions. > > > > I've should have looked at the compiled output, sorry for the noise > > Umm, there's the commit description to document justification for such > changes made and it's not the reviewer's duty to chase every such detail > that has been omitted from a submission. > > FAOD this is a request to include this detail in v3. > > > Still this construct feels rather fragile. > > I think `.align', etc. can still be used to ensure enough space has been > consumed and if you want to make a code sequence's size check, then a > piece such as: > > 0: > nop > nop > nop > nop > 1: > .ifne 1b - 0b - 32 > .error "code consistency verification failure" > .endif > > should do even where alignment does not matter. And you could possibly do > smarter stuff with `.rept' if (1b - 0b) turns out below rather than above > the threshold required, but I'm leaving it as an exercise for the reader. > > Maciej -- Marco Crivellari L3 Support Engineer, Technology & Product marco.crivellari@xxxxxxxx
