Re: [RFC PATCH 02/13] powerpc: Add initial Dynamic Execution Control Register (DEXCR) support

On Tue, 2023-03-07 at 14:45 +1000, Nicholas Piggin wrote:

On Mon Nov 28, 2022 at 12:44 PM AEST, Benjamin Gray wrote:

quoted

diff --git a/arch/powerpc/include/asm/cputable.h

b/arch/powerpc/include/asm/cputable.h
index 757dbded11dc..03bc192f2d8b 100644

--- a/arch/powerpc/include/asm/cputable.h
+++ b/arch/powerpc/include/asm/cputable.h

@@ -192,6 +192,10 @@ static inline void cpu_feature_keys_init(void)

{ }
 #define
CPU_FTR_P9_RADIX_PREFETCH_BUG  LONG_ASM_CONST(0x0002000000000000)
 #define
CPU_FTR_ARCH_31                        LONG_ASM_CONST(0x00040000000
00000)
 #define
CPU_FTR_DAWR1                  LONG_ASM_CONST(0x0008000000000000)
+#define
CPU_FTR_DEXCR_SBHE             LONG_ASM_CONST(0x0010000000000000)
+#define
CPU_FTR_DEXCR_IBRTPD           LONG_ASM_CONST(0x0020000000000000)
+#define
CPU_FTR_DEXCR_SRAPD            LONG_ASM_CONST(0x0040000000000000)
+#define
CPU_FTR_DEXCR_NPHIE            LONG_ASM_CONST(0x0080000000000000)

We potentially don't need to use CPU_FTR bits for each of these. We
only really want them to use instruction patching and make feature
tests fast. But we have been a bit liberal with using them and they
are kind of tied into cpu feature parsing code so maybe it's easier
to go with them for now.

For the static only DEXCR series I've only got CPU_FTR_DEXCR_NPHIE
because that's needed for hashkey updates. The others don't really
matter; they are only interesting for masking out unsupported bits.
Masking itself seems to be unnecessary; the DEXCR will just ignore
unsupported bits. Attempting to set all bits on a P10 showed the first
8 were set and the remainder stayed 0'd, and the kernel worked fine.

It's definitely easier to use CPU_FTR_* for feature detection from the
PAPR specified blob though. Maybe it would be possible to support a
callback on a match instead of setting a feature flag.

@@ -1802,7 +1809,7 @@ int copy_thread(struct task_struct *p, const

quoted

@@ -1802,7 +1809,7 @@ int copy_thread(struct task_struct *p, const

struct kernel_clone_args *args)
 
        setup_ksp_vsid(p, sp);
 
-#ifdef CONFIG_PPC64 
+#ifdef CONFIG_PPC64
        if (cpu_has_feature(CPU_FTR_DSCR)) {
                p->thread.dscr_inherit = current-

quoted

thread.dscr_inherit;

                p->thread.dscr = mfspr(SPRN_DSCR);

@@ -1939,6 +1946,10 @@ void start_thread(struct pt_regs *regs,

unsigned long start, unsigned long sp)
        current->thread.tm_tfiar = 0;
        current->thread.load_tm = 0;
 #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
+#ifdef CONFIG_PPC_BOOK3S_64
+       if (cpu_has_feature(CPU_FTR_ARCH_31))
+               mtspr(SPRN_DEXCR, get_thread_dexcr(&current-

quoted

thread));

+#endif /* CONFIG_PPC_BOOK3S_64 */

You possibly don't need the ifdef here because CPU_FTR_ARCH_31 should
fold away. Some of the others do because they're using open-coded
access to struct members, but if you're using accessor functions to
get and set such things, there may be no need to.

I think my preference is for your style.

I've been revisiting where the DEXCR is initialised and updated. With
the static DEXCR, the thread value is just a field on the task struct
like the others.