interrupt_exit_kernel_prepare() on booke/32 has a useless 'mfmsr' and two 'wrteei 0'

interrupt_exit_kernel_prepare() on booke/32 has a useless 'mfmsr' and two 'wrteei 0'

[Christophe Leroy]

44x/bamboo_defconfig

For the mfmsr, that's because mfmsr is defined as 'asm volatile'. Is that correct ? Reading MSR 
doesn't have any side effects as far as I know, should we remove the volatile ?

For the wrteei, that's because we are calling __hard_EE_RI_disable() after local_irq_save(). On 
booke those two fonctions do exactly the same because RI doesn't exist. Could we replace that by a 
__hard_RI_disable() that would be a nop on booke ?


00000364 <interrupt_exit_kernel_prepare>:
  364:	81 23 00 84 	lwz     r9,132(r3)
  368:	55 29 04 62 	rlwinm  r9,r9,0,17,17
  36c:	0f 09 00 00 	twnei   r9,0
  370:	81 22 00 4c 	lwz     r9,76(r2)
  374:	75 23 00 01 	andis.  r3,r9,1
  378:	40 82 00 14 	bne     38c <interrupt_exit_kernel_prepare+0x28>
  37c:	7d 20 00 a6 	mfmsr   r9
  380:	7c 00 01 46 	wrteei  0
  384:	7c 00 01 46 	wrteei  0
  388:	4e 80 00 20 	blr
  38c:	38 e2 00 4c 	addi    r7,r2,76
  390:	3d 20 00 01 	lis     r9,1
  394:	7c c0 38 28 	lwarx   r6,0,r7
  398:	7c c6 48 78 	andc    r6,r6,r9
  39c:	7c c0 39 2d 	stwcx.  r6,0,r7
  3a0:	40 a2 ff f4 	bne     394 <interrupt_exit_kernel_prepare+0x30>
  3a4:	38 60 00 01 	li      r3,1
  3a8:	4b ff ff d4 	b       37c <interrupt_exit_kernel_prepare+0x18>

Re: interrupt_exit_kernel_prepare() on booke/32 has a useless 'mfmsr' and two 'wrteei 0'

[Nicholas Piggin]

Excerpts from Christophe Leroy's message of February 10, 2021 7:21 pm:
> 44x/bamboo_defconfig
> 
> For the mfmsr, that's because mfmsr is defined as 'asm volatile'. Is that correct ? Reading MSR 
> doesn't have any side effects as far as I know, should we remove the volatile ?

Well I'm not really sure. It depends on the MSR value, so it must not 
assume it's unchanging.

If you just have asm ("mfmsr %0" : "=r"(msr)) then that's wrong because 
it will omit the second mfmsr in a mfmsr() ; mtmsr() ; mfmsr() sequence.

Adding a "memory" clobber there makes gcc produce the second mfmsr, but 
I don't know if that's really the right thing to do.

> 
> For the wrteei, that's because we are calling __hard_EE_RI_disable() after local_irq_save(). On 
> booke those two fonctions do exactly the same because RI doesn't exist. Could we replace that by a 
> __hard_RI_disable() that would be a nop on booke ?

Not on 64-bit because local_irq_disable() doesn't disable EE there.
You could have something like __hard_EE_RI_disable_irqoff() that is to 
be called only in irq disabled region. But is that just adding too much 
complexity to try to keep 32 and 64 bit unified?

Maybe just making different code paths for 32 and 64 would be best. 
32-bit seems fairly simple

	if (!arch_irq_disabled_regs(regs)) {
                /* Returning to a kernel context with local irqs enabled. */
                WARN_ON_ONCE(!(regs->msr & MSR_EE));

       		local_irq_disable();
                if (IS_ENABLED(CONFIG_PREEMPT)) {
                        /* Return to preemptible kernel context */
                        if (unlikely(*ti_flagsp & _TIF_NEED_RESCHED)) {
                                if (preempt_count() == 0)
                                        preempt_schedule_irq();
                        }
                }
		trace_hardirqs_on();
		__hard_RI_disable();
        } else {
		__hard_EE_RI_disable();
	}

You could get rid of that entirely if no preempt or irq tracing and just
have __hard_EE_RI_disable even AFAIKS.

Thanks,
Nick