* Re: [RFC PATCH v3 3/7] powerpc: Activate CONFIG_THREAD_INFO_IN_TASK
From: Christophe LEROY @ 2018-10-03 5:47 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <20181003153025.35b2dd5e@roar.ozlabs.ibm.com>
Le 03/10/2018 à 07:30, Nicholas Piggin a écrit :
> On Mon, 1 Oct 2018 12:30:23 +0000 (UTC)
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>> This patch activates CONFIG_THREAD_INFO_IN_TASK which
>> moves the thread_info into task_struct.
>>
>> Moving thread_info into task_struct has the following advantages:
>> - It protects thread_info from corruption in the case of stack
>> overflows.
>> - Its address is harder to determine if stack addresses are
>> leaked, making a number of attacks more difficult.
>>
>> This has the following consequences:
>> - thread_info is now located at the top of task_struct.
>
> "top"... I got confused for a minute thinking high address and
> wondering how you can change CURRENT_THREAD_INFO just to point
> to current :)
Would 'beginning' be less confusing ?
>
>
>
>> diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
>> index 07d9dce7eda6..4e98989b5512 100644
>> --- a/arch/powerpc/Makefile
>> +++ b/arch/powerpc/Makefile
>> @@ -422,3 +422,9 @@ checkbin:
>>
>> CLEAN_FILES += $(TOUT)
>>
>> +ifdef CONFIG_SMP
>> +prepare: task_cpu_prepare
>> +
>> +task_cpu_prepare: prepare0
>> + $(eval KBUILD_CFLAGS += -D_TASK_CPU=$(shell awk '{if ($$2 == "TI_CPU") print $$3;}' include/generated/asm-offsets.h))
>> +endif
>> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
>> index 447cbd1bee99..3a7e5561630b 100644
>> --- a/arch/powerpc/include/asm/ptrace.h
>> +++ b/arch/powerpc/include/asm/ptrace.h
>> @@ -120,7 +120,7 @@ extern int ptrace_put_reg(struct task_struct *task, int regno,
>> unsigned long data);
>>
>> #define current_pt_regs() \
>> - ((struct pt_regs *)((unsigned long)current_thread_info() + THREAD_SIZE) - 1)
>> + ((struct pt_regs *)((unsigned long)task_stack_page(current) + THREAD_SIZE) - 1)
>> /*
>> * We use the least-significant bit of the trap field to indicate
>> * whether we have saved the full set of registers, or only a
>> diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h
>> index 95b66a0c639b..df519b7322e5 100644
>> --- a/arch/powerpc/include/asm/smp.h
>> +++ b/arch/powerpc/include/asm/smp.h
>> @@ -83,7 +83,13 @@ int is_cpu_dead(unsigned int cpu);
>> /* 32-bit */
>> extern int smp_hw_index[];
>>
>> -#define raw_smp_processor_id() (current_thread_info()->cpu)
>> +/*
>> + * This is particularly ugly: it appears we can't actually get the definition
>> + * of task_struct here, but we need access to the CPU this task is running on.
>> + * Instead of using task_struct we're using _TASK_CPU which is extracted from
>> + * asm-offsets.h by kbuild to get the current processor ID.
>> + */
>> +#define raw_smp_processor_id() (*(unsigned int*)((void*)current + _TASK_CPU))
>
> This is clever but yes ugly. Can't you include asm-offsets.h? riscv
> seems to.
riscv has a clean asm-offsets.h . Our's defines constant with the same
name as those defined in other headers which are included in C files. So
including asm-offsets in C files does create conflicts like:
./include/generated/asm-offsets.h:71:0: warning: "TASK_SIZE" redefined
#define TASK_SIZE -2147483648 /* TASK_SIZE */
./arch/powerpc/include/asm/processor.h:95:0: note: this is the location
of the previous definition
#define TASK_SIZE (CONFIG_TASK_SIZE)
./include/generated/asm-offsets.h:98:0: warning: "NSEC_PER_SEC" redefined
#define NSEC_PER_SEC 1000000000 /* NSEC_PER_SEC */
./include/linux/time64.h:36:0: note: this is the location of the
previous definition
#define NSEC_PER_SEC 1000000000L
./arch/powerpc/include/asm/nohash/32/pgtable.h:34:0: warning:
"PGD_TABLE_SIZE" redefined
#define PGD_TABLE_SIZE (sizeof(pgd_t) << PGD_INDEX_SIZE)
./include/generated/asm-offsets.h:101:0: note: this is the location of
the previous definition
#define PGD_TABLE_SIZE 256 /* PGD_TABLE_SIZE */
...
In v2, I had a patch to fix those redundancies
(https://patchwork.ozlabs.org/patch/974363/) but I found it unconvenient.
>
> I'm not 100% sure on kgdb and kexec stuff but I think it seems okay.
> Looks like a pretty nice cleanup too aside from the features it brings,
> thanks for working on it.
Thanks for reviewing it.
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
Christophe
^ permalink raw reply
* Re: [RFC PATCH v3 2/7] powerpc: Prepare for moving thread_info into task_struct
From: Christophe LEROY @ 2018-10-03 5:49 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <20181003150253.5124e989@roar.ozlabs.ibm.com>
Le 03/10/2018 à 07:02, Nicholas Piggin a écrit :
> On Mon, 1 Oct 2018 12:30:21 +0000 (UTC)
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>> This patch cleans the powerpc kernel before activating
>> CONFIG_THREAD_INFO_IN_TASK:
>> - The purpose of the pointer given to call_do_softirq() and
>> call_do_irq() is to point the new stack ==> change it to void*
>> - Don't use CURRENT_THREAD_INFO() to locate the stack.
>> - Fixed a few comments.
>> - TI_CPU is only used when CONFIG_SMP is set.
>> - Replace current_thread_info()->task by current
>> - Remove unnecessary casts to thread_info, as they'll become invalid
>> once thread_info is not in stack anymore.
>> - Ensure task_struct 'cpu' fields is not used directly out of SMP code
>> - Rename THREAD_INFO to TASK_STASK: As it is in fact the offset of the
>> pointer to the stack in task_struct, this pointer will not be impacted
>> by the move of THREAD_INFO.
>> - Makes TASK_STACK available to PPC64 which will need it to the get
>> stack pointer from current once the thread_info have been moved.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/include/asm/irq.h | 4 ++--
>> arch/powerpc/include/asm/livepatch.h | 2 +-
>> arch/powerpc/include/asm/processor.h | 4 ++--
>> arch/powerpc/include/asm/reg.h | 2 +-
>> arch/powerpc/kernel/asm-offsets.c | 2 +-
>> arch/powerpc/kernel/entry_32.S | 2 +-
>> arch/powerpc/kernel/entry_64.S | 2 +-
>> arch/powerpc/kernel/head_32.S | 4 ++--
>> arch/powerpc/kernel/head_40x.S | 4 ++--
>> arch/powerpc/kernel/head_44x.S | 2 +-
>> arch/powerpc/kernel/head_8xx.S | 2 +-
>> arch/powerpc/kernel/head_booke.h | 4 ++--
>> arch/powerpc/kernel/head_fsl_booke.S | 6 ++++--
>> arch/powerpc/kernel/irq.c | 2 +-
>> arch/powerpc/kernel/misc_32.S | 8 ++++++--
>> arch/powerpc/kernel/process.c | 6 +++---
>> arch/powerpc/kernel/setup_32.c | 15 +++++----------
>> arch/powerpc/kernel/smp.c | 4 +++-
>> arch/powerpc/xmon/xmon.c | 2 +-
>> 19 files changed, 40 insertions(+), 37 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h
>> index ee39ce56b2a2..8108d1fe33ca 100644
>> --- a/arch/powerpc/include/asm/irq.h
>> +++ b/arch/powerpc/include/asm/irq.h
>> @@ -63,8 +63,8 @@ extern struct thread_info *hardirq_ctx[NR_CPUS];
>> extern struct thread_info *softirq_ctx[NR_CPUS];
>>
>> extern void irq_ctx_init(void);
>> -extern void call_do_softirq(struct thread_info *tp);
>> -extern void call_do_irq(struct pt_regs *regs, struct thread_info *tp);
>> +extern void call_do_softirq(void *tp);
>> +extern void call_do_irq(struct pt_regs *regs, void *tp);
>
> void *sp for these ?
Yes, why not but it means changing the code. I wanted to minimise the
changes and avoid cosmetic. Or maybe should add a cosmetic patch at the
end ?
>
> This all seems okay to me except the 32-bit code which I don't know.
> Would it be any trouble for you to put the TI_CPU bits into their own
> patch?
No problem, I can put the TI_CPU bits in a separate patch.
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
Thanks
Christophe
>
>> extern void do_IRQ(struct pt_regs *regs);
>> extern void __init init_IRQ(void);
>> extern void __do_irq(struct pt_regs *regs);
>> diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
>> index 47a03b9b528b..818451bf629c 100644
>> --- a/arch/powerpc/include/asm/livepatch.h
>> +++ b/arch/powerpc/include/asm/livepatch.h
>> @@ -49,7 +49,7 @@ static inline void klp_init_thread_info(struct thread_info *ti)
>> ti->livepatch_sp = (unsigned long *)(ti + 1) + 1;
>> }
>> #else
>> -static void klp_init_thread_info(struct thread_info *ti) { }
>> +static inline void klp_init_thread_info(struct thread_info *ti) { }
>> #endif /* CONFIG_LIVEPATCH */
>>
>> #endif /* _ASM_POWERPC_LIVEPATCH_H */
>> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
>> index 353879db3e98..31873614392f 100644
>> --- a/arch/powerpc/include/asm/processor.h
>> +++ b/arch/powerpc/include/asm/processor.h
>> @@ -40,7 +40,7 @@
>>
>> #ifndef __ASSEMBLY__
>> #include <linux/types.h>
>> -#include <asm/thread_info.h>
>> +#include <linux/thread_info.h>
>> #include <asm/ptrace.h>
>> #include <asm/hw_breakpoint.h>
>>
>> @@ -333,7 +333,7 @@ struct thread_struct {
>>
>> #define INIT_SP (sizeof(init_stack) + (unsigned long) &init_stack)
>> #define INIT_SP_LIMIT \
>> - (_ALIGN_UP(sizeof(init_thread_info), 16) + (unsigned long) &init_stack)
>> + (_ALIGN_UP(sizeof(struct thread_info), 16) + (unsigned long) &init_stack)
>>
>> #ifdef CONFIG_SPE
>> #define SPEFSCR_INIT \
>> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
>> index e5b314ed054e..f3a9cf19a986 100644
>> --- a/arch/powerpc/include/asm/reg.h
>> +++ b/arch/powerpc/include/asm/reg.h
>> @@ -1053,7 +1053,7 @@
>> * - SPRG9 debug exception scratch
>> *
>> * All 32-bit:
>> - * - SPRG3 current thread_info pointer
>> + * - SPRG3 current thread_struct physical addr pointer
>> * (virtual on BookE, physical on others)
>> *
>> * 32-bit classic:
>> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
>> index ba9d0fc98730..d1f161e48945 100644
>> --- a/arch/powerpc/kernel/asm-offsets.c
>> +++ b/arch/powerpc/kernel/asm-offsets.c
>> @@ -85,10 +85,10 @@ int main(void)
>> DEFINE(NMI_MASK, NMI_MASK);
>> OFFSET(TASKTHREADPPR, task_struct, thread.ppr);
>> #else
>> - OFFSET(THREAD_INFO, task_struct, stack);
>> DEFINE(THREAD_INFO_GAP, _ALIGN_UP(sizeof(struct thread_info), 16));
>> OFFSET(KSP_LIMIT, thread_struct, ksp_limit);
>> #endif /* CONFIG_PPC64 */
>> + OFFSET(TASK_STACK, task_struct, stack);
>>
>> #ifdef CONFIG_LIVEPATCH
>> OFFSET(TI_livepatch_sp, thread_info, livepatch_sp);
>> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
>> index e58c3f467db5..12c0721f65ea 100644
>> --- a/arch/powerpc/kernel/entry_32.S
>> +++ b/arch/powerpc/kernel/entry_32.S
>> @@ -1166,7 +1166,7 @@ ret_from_debug_exc:
>> mfspr r9,SPRN_SPRG_THREAD
>> lwz r10,SAVED_KSP_LIMIT(r1)
>> stw r10,KSP_LIMIT(r9)
>> - lwz r9,THREAD_INFO-THREAD(r9)
>> + lwz r9,TASK_STACK-THREAD(r9)
>> CURRENT_THREAD_INFO(r10, r1)
>> lwz r10,TI_PREEMPT(r10)
>> stw r10,TI_PREEMPT(r9)
>> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
>> index 77a888bfcb53..697406572592 100644
>> --- a/arch/powerpc/kernel/entry_64.S
>> +++ b/arch/powerpc/kernel/entry_64.S
>> @@ -680,7 +680,7 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
>> 2:
>> #endif /* CONFIG_PPC_BOOK3S_64 */
>>
>> - CURRENT_THREAD_INFO(r7, r8) /* base of new stack */
>> + clrrdi r7, r8, THREAD_SHIFT /* base of new stack */
>> /* Note: this uses SWITCH_FRAME_SIZE rather than INT_FRAME_SIZE
>> because we don't need to leave the 288-byte ABI gap at the
>> top of the kernel stack. */
>> diff --git a/arch/powerpc/kernel/head_32.S b/arch/powerpc/kernel/head_32.S
>> index 61ca27929355..dce6f2ff07e5 100644
>> --- a/arch/powerpc/kernel/head_32.S
>> +++ b/arch/powerpc/kernel/head_32.S
>> @@ -261,7 +261,7 @@ __secondary_hold_acknowledge:
>> tophys(r11,r1); /* use tophys(r1) if kernel */ \
>> beq 1f; \
>> mfspr r11,SPRN_SPRG_THREAD; \
>> - lwz r11,THREAD_INFO-THREAD(r11); \
>> + lwz r11,TASK_STACK-THREAD(r11); \
>> addi r11,r11,THREAD_SIZE; \
>> tophys(r11,r11); \
>> 1: subi r11,r11,INT_FRAME_SIZE /* alloc exc. frame */
>> @@ -841,7 +841,7 @@ __secondary_start:
>> bl init_idle_6xx
>> #endif /* CONFIG_6xx */
>>
>> - /* get current_thread_info and current */
>> + /* get current's stack and current */
>> lis r1,secondary_ti@ha
>> tophys(r1,r1)
>> lwz r1,secondary_ti@l(r1)
>> diff --git a/arch/powerpc/kernel/head_40x.S b/arch/powerpc/kernel/head_40x.S
>> index b19d78410511..3088c9f29f5e 100644
>> --- a/arch/powerpc/kernel/head_40x.S
>> +++ b/arch/powerpc/kernel/head_40x.S
>> @@ -115,7 +115,7 @@ _ENTRY(saved_ksp_limit)
>> andi. r11,r11,MSR_PR; \
>> beq 1f; \
>> mfspr r1,SPRN_SPRG_THREAD; /* if from user, start at top of */\
>> - lwz r1,THREAD_INFO-THREAD(r1); /* this thread's kernel stack */\
>> + lwz r1,TASK_STACK-THREAD(r1); /* this thread's kernel stack */\
>> addi r1,r1,THREAD_SIZE; \
>> 1: subi r1,r1,INT_FRAME_SIZE; /* Allocate an exception frame */\
>> tophys(r11,r1); \
>> @@ -158,7 +158,7 @@ _ENTRY(saved_ksp_limit)
>> beq 1f; \
>> /* COMING FROM USER MODE */ \
>> mfspr r11,SPRN_SPRG_THREAD; /* if from user, start at top of */\
>> - lwz r11,THREAD_INFO-THREAD(r11); /* this thread's kernel stack */\
>> + lwz r11,TASK_STACK-THREAD(r11); /* this thread's kernel stack */\
>> 1: addi r11,r11,THREAD_SIZE-INT_FRAME_SIZE; /* Alloc an excpt frm */\
>> tophys(r11,r11); \
>> stw r10,_CCR(r11); /* save various registers */\
>> diff --git a/arch/powerpc/kernel/head_44x.S b/arch/powerpc/kernel/head_44x.S
>> index 37e4a7cf0065..15d39b2499de 100644
>> --- a/arch/powerpc/kernel/head_44x.S
>> +++ b/arch/powerpc/kernel/head_44x.S
>> @@ -1020,7 +1020,7 @@ _GLOBAL(start_secondary_47x)
>>
>> /* Now we can get our task struct and real stack pointer */
>>
>> - /* Get current_thread_info and current */
>> + /* Get current's stack and current */
>> lis r1,secondary_ti@ha
>> lwz r1,secondary_ti@l(r1)
>> lwz r2,TI_TASK(r1)
>> diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
>> index 6582f824d620..e56e36aa2b3d 100644
>> --- a/arch/powerpc/kernel/head_8xx.S
>> +++ b/arch/powerpc/kernel/head_8xx.S
>> @@ -124,7 +124,7 @@ turn_on_mmu:
>> tophys(r11,r1); /* use tophys(r1) if kernel */ \
>> beq 1f; \
>> mfspr r11,SPRN_SPRG_THREAD; \
>> - lwz r11,THREAD_INFO-THREAD(r11); \
>> + lwz r11,TASK_STACK-THREAD(r11); \
>> addi r11,r11,THREAD_SIZE; \
>> tophys(r11,r11); \
>> 1: subi r11,r11,INT_FRAME_SIZE /* alloc exc. frame */
>> diff --git a/arch/powerpc/kernel/head_booke.h b/arch/powerpc/kernel/head_booke.h
>> index d0862a100d29..20fe0c93a0bd 100644
>> --- a/arch/powerpc/kernel/head_booke.h
>> +++ b/arch/powerpc/kernel/head_booke.h
>> @@ -44,7 +44,7 @@
>> mr r11, r1; \
>> beq 1f; \
>> /* if from user, start at top of this thread's kernel stack */ \
>> - lwz r11, THREAD_INFO-THREAD(r10); \
>> + lwz r11, TASK_STACK-THREAD(r10); \
>> ALLOC_STACK_FRAME(r11, THREAD_SIZE); \
>> 1 : subi r11, r11, INT_FRAME_SIZE; /* Allocate exception frame */ \
>> stw r13, _CCR(r11); /* save various registers */ \
>> @@ -130,7 +130,7 @@
>> DO_KVM BOOKE_INTERRUPT_##intno exc_level_srr1; \
>> andi. r11,r11,MSR_PR; \
>> mfspr r11,SPRN_SPRG_THREAD; /* if from user, start at top of */\
>> - lwz r11,THREAD_INFO-THREAD(r11); /* this thread's kernel stack */\
>> + lwz r11,TASK_STACK-THREAD(r11); /* this thread's kernel stack */\
>> addi r11,r11,EXC_LVL_FRAME_OVERHEAD; /* allocate stack frame */\
>> beq 1f; \
>> /* COMING FROM USER MODE */ \
>> diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S
>> index e2750b856c8f..239ad8a4754e 100644
>> --- a/arch/powerpc/kernel/head_fsl_booke.S
>> +++ b/arch/powerpc/kernel/head_fsl_booke.S
>> @@ -243,8 +243,10 @@ set_ivor:
>> li r0,0
>> stwu r0,THREAD_SIZE-STACK_FRAME_OVERHEAD(r1)
>>
>> +#ifdef CONFIG_SMP
>> CURRENT_THREAD_INFO(r22, r1)
>> stw r24, TI_CPU(r22)
>> +#endif
>>
>> bl early_init
>>
>> @@ -702,7 +704,7 @@ finish_tlb_load:
>>
>> /* Get the next_tlbcam_idx percpu var */
>> #ifdef CONFIG_SMP
>> - lwz r12, THREAD_INFO-THREAD(r12)
>> + lwz r12, TASK_STACK-THREAD(r12)
>> lwz r15, TI_CPU(r12)
>> lis r14, __per_cpu_offset@h
>> ori r14, r14, __per_cpu_offset@l
>> @@ -1074,7 +1076,7 @@ __secondary_start:
>> mr r4,r24 /* Why? */
>> bl call_setup_cpu
>>
>> - /* get current_thread_info and current */
>> + /* get current's stack and current */
>> lis r1,secondary_ti@ha
>> lwz r1,secondary_ti@l(r1)
>> lwz r2,TI_TASK(r1)
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index 916ddc4aac44..aa53db3ba6e7 100644
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -663,7 +663,7 @@ void do_IRQ(struct pt_regs *regs)
>> struct thread_info *curtp, *irqtp, *sirqtp;
>>
>> /* Switch to the irq stack to handle this */
>> - curtp = current_thread_info();
>> + curtp = (void*)(current_stack_pointer() & ~(THREAD_SIZE - 1));
>> irqtp = hardirq_ctx[raw_smp_processor_id()];
>> sirqtp = softirq_ctx[raw_smp_processor_id()];
>>
>> diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
>> index 695b24a2d954..24a7f18ea10c 100644
>> --- a/arch/powerpc/kernel/misc_32.S
>> +++ b/arch/powerpc/kernel/misc_32.S
>> @@ -60,7 +60,7 @@ _GLOBAL(call_do_softirq)
>> blr
>>
>> /*
>> - * void call_do_irq(struct pt_regs *regs, struct thread_info *irqtp);
>> + * void call_do_irq(struct pt_regs *regs, void *irqtp);
>> */
>> _GLOBAL(call_do_irq)
>> mflr r0
>> @@ -183,10 +183,14 @@ _GLOBAL(low_choose_750fx_pll)
>> or r4,r4,r5
>> mtspr SPRN_HID1,r4
>>
>> +#ifdef CONFIG_SMP
>> /* Store new HID1 image */
>> CURRENT_THREAD_INFO(r6, r1)
>> lwz r6,TI_CPU(r6)
>> slwi r6,r6,2
>> +#else
>> + li r6, 0
>> +#endif
>> addis r6,r6,nap_save_hid1@ha
>> stw r4,nap_save_hid1@l(r6)
>>
>> @@ -599,7 +603,7 @@ EXPORT_SYMBOL(__bswapdi2)
>> #ifdef CONFIG_SMP
>> _GLOBAL(start_secondary_resume)
>> /* Reset stack */
>> - CURRENT_THREAD_INFO(r1, r1)
>> + rlwinm r1, r1, 0, 0, 31 - THREAD_SHIFT
>> addi r1,r1,THREAD_SIZE-STACK_FRAME_OVERHEAD
>> li r3,0
>> stw r3,0(r1) /* Zero the stack frame pointer */
>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
>> index 03c2e1f134bc..111abb4df2ec 100644
>> --- a/arch/powerpc/kernel/process.c
>> +++ b/arch/powerpc/kernel/process.c
>> @@ -1240,8 +1240,8 @@ struct task_struct *__switch_to(struct task_struct *prev,
>> batch->active = 1;
>> }
>>
>> - if (current_thread_info()->task->thread.regs) {
>> - restore_math(current_thread_info()->task->thread.regs);
>> + if (current->thread.regs) {
>> + restore_math(current->thread.regs);
>>
>> /*
>> * The copy-paste buffer can only store into foreign real
>> @@ -1251,7 +1251,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
>> * mappings, we must issue a cp_abort to clear any state and
>> * prevent snooping, corruption or a covert channel.
>> */
>> - if (current_thread_info()->task->thread.used_vas)
>> + if (current->thread.used_vas)
>> asm volatile(PPC_CP_ABORT);
>> }
>> #endif /* CONFIG_PPC_BOOK3S_64 */
>> diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
>> index 8c507be12c3c..81ebf7d6f526 100644
>> --- a/arch/powerpc/kernel/setup_32.c
>> +++ b/arch/powerpc/kernel/setup_32.c
>> @@ -205,10 +205,8 @@ void __init irqstack_early_init(void)
>> /* interrupt stacks must be in lowmem, we get that for free on ppc32
>> * as the memblock is limited to lowmem by default */
>> for_each_possible_cpu(i) {
>> - softirq_ctx[i] = (struct thread_info *)
>> - __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
>> - hardirq_ctx[i] = (struct thread_info *)
>> - __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
>> + softirq_ctx[i] = __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
>> + hardirq_ctx[i] = __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
>> }
>> }
>>
>> @@ -226,13 +224,10 @@ void __init exc_lvl_early_init(void)
>> hw_cpu = 0;
>> #endif
>>
>> - critirq_ctx[hw_cpu] = (struct thread_info *)
>> - __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
>> + critirq_ctx[hw_cpu] = __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
>> #ifdef CONFIG_BOOKE
>> - dbgirq_ctx[hw_cpu] = (struct thread_info *)
>> - __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
>> - mcheckirq_ctx[hw_cpu] = (struct thread_info *)
>> - __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
>> + dbgirq_ctx[hw_cpu] = __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
>> + mcheckirq_ctx[hw_cpu] = __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
>> #endif
>> }
>> }
>> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
>> index 61c1fadbc644..19dd0ea55714 100644
>> --- a/arch/powerpc/kernel/smp.c
>> +++ b/arch/powerpc/kernel/smp.c
>> @@ -20,6 +20,7 @@
>> #include <linux/kernel.h>
>> #include <linux/export.h>
>> #include <linux/sched/mm.h>
>> +#include <linux/sched/task_stack.h>
>> #include <linux/sched/topology.h>
>> #include <linux/smp.h>
>> #include <linux/interrupt.h>
>> @@ -812,7 +813,8 @@ static void cpu_idle_thread_init(unsigned int cpu, struct task_struct *idle)
>>
>> #ifdef CONFIG_PPC64
>> paca_ptrs[cpu]->__current = idle;
>> - paca_ptrs[cpu]->kstack = (unsigned long)ti + THREAD_SIZE - STACK_FRAME_OVERHEAD;
>> + paca_ptrs[cpu]->kstack = (unsigned long)task_stack_page(idle) +
>> + THREAD_SIZE - STACK_FRAME_OVERHEAD;
>> #endif
>> ti->cpu = cpu;
>> secondary_ti = current_set[cpu] = ti;
>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> index 694c1d92e796..0d8d6fee892a 100644
>> --- a/arch/powerpc/xmon/xmon.c
>> +++ b/arch/powerpc/xmon/xmon.c
>> @@ -2988,7 +2988,7 @@ static void show_task(struct task_struct *tsk)
>> printf("%px %016lx %6d %6d %c %2d %s\n", tsk,
>> tsk->thread.ksp,
>> tsk->pid, tsk->parent->pid,
>> - state, task_thread_info(tsk)->cpu,
>> + state, task_cpu(tsk),
>> tsk->comm);
>> }
>>
>> --
>> 2.13.3
>>
^ permalink raw reply
* Re: [RFC PATCH v3 3/7] powerpc: Activate CONFIG_THREAD_INFO_IN_TASK
From: Nicholas Piggin @ 2018-10-03 5:52 UTC (permalink / raw)
To: Christophe LEROY; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <c1b9f2db-ebca-4a2d-4f91-2dce84da31f2@c-s.fr>
On Wed, 3 Oct 2018 07:47:05 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> Le 03/10/2018 à 07:30, Nicholas Piggin a écrit :
> > On Mon, 1 Oct 2018 12:30:23 +0000 (UTC)
> > Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >
> >> This patch activates CONFIG_THREAD_INFO_IN_TASK which
> >> moves the thread_info into task_struct.
> >>
> >> Moving thread_info into task_struct has the following advantages:
> >> - It protects thread_info from corruption in the case of stack
> >> overflows.
> >> - Its address is harder to determine if stack addresses are
> >> leaked, making a number of attacks more difficult.
> >>
> >> This has the following consequences:
> >> - thread_info is now located at the top of task_struct.
> >
> > "top"... I got confused for a minute thinking high address and
> > wondering how you can change CURRENT_THREAD_INFO just to point
> > to current :)
>
> Would 'beginning' be less confusing ?
Yes, good idea.
> >> @@ -83,7 +83,13 @@ int is_cpu_dead(unsigned int cpu);
> >> /* 32-bit */
> >> extern int smp_hw_index[];
> >>
> >> -#define raw_smp_processor_id() (current_thread_info()->cpu)
> >> +/*
> >> + * This is particularly ugly: it appears we can't actually get the definition
> >> + * of task_struct here, but we need access to the CPU this task is running on.
> >> + * Instead of using task_struct we're using _TASK_CPU which is extracted from
> >> + * asm-offsets.h by kbuild to get the current processor ID.
> >> + */
> >> +#define raw_smp_processor_id() (*(unsigned int*)((void*)current + _TASK_CPU))
> >
> > This is clever but yes ugly. Can't you include asm-offsets.h? riscv
> > seems to.
>
> riscv has a clean asm-offsets.h . Our's defines constant with the same
> name as those defined in other headers which are included in C files. So
> including asm-offsets in C files does create conflicts like:
>
> ./include/generated/asm-offsets.h:71:0: warning: "TASK_SIZE" redefined
> #define TASK_SIZE -2147483648 /* TASK_SIZE */
> ./arch/powerpc/include/asm/processor.h:95:0: note: this is the location
> of the previous definition
> #define TASK_SIZE (CONFIG_TASK_SIZE)
>
> ./include/generated/asm-offsets.h:98:0: warning: "NSEC_PER_SEC" redefined
> #define NSEC_PER_SEC 1000000000 /* NSEC_PER_SEC */
> ./include/linux/time64.h:36:0: note: this is the location of the
> previous definition
> #define NSEC_PER_SEC 1000000000L
>
> ./arch/powerpc/include/asm/nohash/32/pgtable.h:34:0: warning:
> "PGD_TABLE_SIZE" redefined
> #define PGD_TABLE_SIZE (sizeof(pgd_t) << PGD_INDEX_SIZE)
> ./include/generated/asm-offsets.h:101:0: note: this is the location of
> the previous definition
> #define PGD_TABLE_SIZE 256 /* PGD_TABLE_SIZE */
>
> ...
Okay.
>
> In v2, I had a patch to fix those redundancies
> (https://patchwork.ozlabs.org/patch/974363/) but I found it unconvenient.
Because of merge conflicts, or you did not like the new names?
Thanks,
Nick
^ permalink raw reply
* Re: [RFC PATCH v3 4/7] powerpc: regain entire stack space
From: Christophe LEROY @ 2018-10-03 5:52 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <20181003153448.3fb64e19@roar.ozlabs.ibm.com>
Le 03/10/2018 à 07:34, Nicholas Piggin a écrit :
> On Mon, 1 Oct 2018 12:30:25 +0000 (UTC)
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>> thread_info is not anymore in the stack, so the entire stack
>> can now be used.
>
> Nice.
>
>>
>> In the meantime, all pointers to the stacks are not anymore
>> pointers to thread_info so this patch changes them to void*
>
> Wasn't this previously effectively already the case with patch
> 3/7? You had thread_info sized space left there, but it was not
> used or initialized right? Does it make sense to move this part
> of it to the previous patch?
Not really.
In 3/7 I changed the prototypes of two functions that really used the
pointer as a task pointer only.
Here it change things that before 4/7 were really used as both stack
pointers and thread_info pointers.
Christophe
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [RFC PATCH v3 2/7] powerpc: Prepare for moving thread_info into task_struct
From: Nicholas Piggin @ 2018-10-03 5:57 UTC (permalink / raw)
To: Christophe LEROY; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <8c53e2fd-c97e-7968-130f-8b5728e20cab@c-s.fr>
On Wed, 3 Oct 2018 07:49:44 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> Le 03/10/2018 à 07:02, Nicholas Piggin a écrit :
> > On Mon, 1 Oct 2018 12:30:21 +0000 (UTC)
> > Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >
> >> This patch cleans the powerpc kernel before activating
> >> CONFIG_THREAD_INFO_IN_TASK:
> >> - The purpose of the pointer given to call_do_softirq() and
> >> call_do_irq() is to point the new stack ==> change it to void*
> >> - Don't use CURRENT_THREAD_INFO() to locate the stack.
> >> - Fixed a few comments.
> >> - TI_CPU is only used when CONFIG_SMP is set.
> >> - Replace current_thread_info()->task by current
> >> - Remove unnecessary casts to thread_info, as they'll become invalid
> >> once thread_info is not in stack anymore.
> >> - Ensure task_struct 'cpu' fields is not used directly out of SMP code
> >> - Rename THREAD_INFO to TASK_STASK: As it is in fact the offset of the
> >> pointer to the stack in task_struct, this pointer will not be impacted
> >> by the move of THREAD_INFO.
> >> - Makes TASK_STACK available to PPC64 which will need it to the get
> >> stack pointer from current once the thread_info have been moved.
> >>
> >> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> >> ---
> >> arch/powerpc/include/asm/irq.h | 4 ++--
> >> arch/powerpc/include/asm/livepatch.h | 2 +-
> >> arch/powerpc/include/asm/processor.h | 4 ++--
> >> arch/powerpc/include/asm/reg.h | 2 +-
> >> arch/powerpc/kernel/asm-offsets.c | 2 +-
> >> arch/powerpc/kernel/entry_32.S | 2 +-
> >> arch/powerpc/kernel/entry_64.S | 2 +-
> >> arch/powerpc/kernel/head_32.S | 4 ++--
> >> arch/powerpc/kernel/head_40x.S | 4 ++--
> >> arch/powerpc/kernel/head_44x.S | 2 +-
> >> arch/powerpc/kernel/head_8xx.S | 2 +-
> >> arch/powerpc/kernel/head_booke.h | 4 ++--
> >> arch/powerpc/kernel/head_fsl_booke.S | 6 ++++--
> >> arch/powerpc/kernel/irq.c | 2 +-
> >> arch/powerpc/kernel/misc_32.S | 8 ++++++--
> >> arch/powerpc/kernel/process.c | 6 +++---
> >> arch/powerpc/kernel/setup_32.c | 15 +++++----------
> >> arch/powerpc/kernel/smp.c | 4 +++-
> >> arch/powerpc/xmon/xmon.c | 2 +-
> >> 19 files changed, 40 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/irq.h b/arch/powerpc/include/asm/irq.h
> >> index ee39ce56b2a2..8108d1fe33ca 100644
> >> --- a/arch/powerpc/include/asm/irq.h
> >> +++ b/arch/powerpc/include/asm/irq.h
> >> @@ -63,8 +63,8 @@ extern struct thread_info *hardirq_ctx[NR_CPUS];
> >> extern struct thread_info *softirq_ctx[NR_CPUS];
> >>
> >> extern void irq_ctx_init(void);
> >> -extern void call_do_softirq(struct thread_info *tp);
> >> -extern void call_do_irq(struct pt_regs *regs, struct thread_info *tp);
> >> +extern void call_do_softirq(void *tp);
> >> +extern void call_do_irq(struct pt_regs *regs, void *tp);
> >
> > void *sp for these ?
>
> Yes, why not but it means changing the code. I wanted to minimise the
> changes and avoid cosmetic.
Fair enough.
> Or maybe should add a cosmetic patch at the
> end ?
Yeah that would be nice, I don't mind too much if it goes as part of
the patches or as a cleanup afterward. There's a few other places that
could use similar minor.
> > This all seems okay to me except the 32-bit code which I don't know.
> > Would it be any trouble for you to put the TI_CPU bits into their own
> > patch?
>
> No problem, I can put the TI_CPU bits in a separate patch.
Great thanks.
Thanks,
Nick
>
> >
> > Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
> >
>
> Thanks
> Christophe
>
> >
> >> extern void do_IRQ(struct pt_regs *regs);
> >> extern void __init init_IRQ(void);
> >> extern void __do_irq(struct pt_regs *regs);
> >> diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
> >> index 47a03b9b528b..818451bf629c 100644
> >> --- a/arch/powerpc/include/asm/livepatch.h
> >> +++ b/arch/powerpc/include/asm/livepatch.h
> >> @@ -49,7 +49,7 @@ static inline void klp_init_thread_info(struct thread_info *ti)
> >> ti->livepatch_sp = (unsigned long *)(ti + 1) + 1;
> >> }
> >> #else
> >> -static void klp_init_thread_info(struct thread_info *ti) { }
> >> +static inline void klp_init_thread_info(struct thread_info *ti) { }
> >> #endif /* CONFIG_LIVEPATCH */
> >>
> >> #endif /* _ASM_POWERPC_LIVEPATCH_H */
> >> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> >> index 353879db3e98..31873614392f 100644
> >> --- a/arch/powerpc/include/asm/processor.h
> >> +++ b/arch/powerpc/include/asm/processor.h
> >> @@ -40,7 +40,7 @@
> >>
> >> #ifndef __ASSEMBLY__
> >> #include <linux/types.h>
> >> -#include <asm/thread_info.h>
> >> +#include <linux/thread_info.h>
> >> #include <asm/ptrace.h>
> >> #include <asm/hw_breakpoint.h>
> >>
> >> @@ -333,7 +333,7 @@ struct thread_struct {
> >>
> >> #define INIT_SP (sizeof(init_stack) + (unsigned long) &init_stack)
> >> #define INIT_SP_LIMIT \
> >> - (_ALIGN_UP(sizeof(init_thread_info), 16) + (unsigned long) &init_stack)
> >> + (_ALIGN_UP(sizeof(struct thread_info), 16) + (unsigned long) &init_stack)
> >>
> >> #ifdef CONFIG_SPE
> >> #define SPEFSCR_INIT \
> >> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> >> index e5b314ed054e..f3a9cf19a986 100644
> >> --- a/arch/powerpc/include/asm/reg.h
> >> +++ b/arch/powerpc/include/asm/reg.h
> >> @@ -1053,7 +1053,7 @@
> >> * - SPRG9 debug exception scratch
> >> *
> >> * All 32-bit:
> >> - * - SPRG3 current thread_info pointer
> >> + * - SPRG3 current thread_struct physical addr pointer
> >> * (virtual on BookE, physical on others)
> >> *
> >> * 32-bit classic:
> >> diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
> >> index ba9d0fc98730..d1f161e48945 100644
> >> --- a/arch/powerpc/kernel/asm-offsets.c
> >> +++ b/arch/powerpc/kernel/asm-offsets.c
> >> @@ -85,10 +85,10 @@ int main(void)
> >> DEFINE(NMI_MASK, NMI_MASK);
> >> OFFSET(TASKTHREADPPR, task_struct, thread.ppr);
> >> #else
> >> - OFFSET(THREAD_INFO, task_struct, stack);
> >> DEFINE(THREAD_INFO_GAP, _ALIGN_UP(sizeof(struct thread_info), 16));
> >> OFFSET(KSP_LIMIT, thread_struct, ksp_limit);
> >> #endif /* CONFIG_PPC64 */
> >> + OFFSET(TASK_STACK, task_struct, stack);
> >>
> >> #ifdef CONFIG_LIVEPATCH
> >> OFFSET(TI_livepatch_sp, thread_info, livepatch_sp);
> >> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> >> index e58c3f467db5..12c0721f65ea 100644
> >> --- a/arch/powerpc/kernel/entry_32.S
> >> +++ b/arch/powerpc/kernel/entry_32.S
> >> @@ -1166,7 +1166,7 @@ ret_from_debug_exc:
> >> mfspr r9,SPRN_SPRG_THREAD
> >> lwz r10,SAVED_KSP_LIMIT(r1)
> >> stw r10,KSP_LIMIT(r9)
> >> - lwz r9,THREAD_INFO-THREAD(r9)
> >> + lwz r9,TASK_STACK-THREAD(r9)
> >> CURRENT_THREAD_INFO(r10, r1)
> >> lwz r10,TI_PREEMPT(r10)
> >> stw r10,TI_PREEMPT(r9)
> >> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> >> index 77a888bfcb53..697406572592 100644
> >> --- a/arch/powerpc/kernel/entry_64.S
> >> +++ b/arch/powerpc/kernel/entry_64.S
> >> @@ -680,7 +680,7 @@ END_FTR_SECTION_IFCLR(CPU_FTR_ARCH_207S)
> >> 2:
> >> #endif /* CONFIG_PPC_BOOK3S_64 */
> >>
> >> - CURRENT_THREAD_INFO(r7, r8) /* base of new stack */
> >> + clrrdi r7, r8, THREAD_SHIFT /* base of new stack */
> >> /* Note: this uses SWITCH_FRAME_SIZE rather than INT_FRAME_SIZE
> >> because we don't need to leave the 288-byte ABI gap at the
> >> top of the kernel stack. */
> >> diff --git a/arch/powerpc/kernel/head_32.S b/arch/powerpc/kernel/head_32.S
> >> index 61ca27929355..dce6f2ff07e5 100644
> >> --- a/arch/powerpc/kernel/head_32.S
> >> +++ b/arch/powerpc/kernel/head_32.S
> >> @@ -261,7 +261,7 @@ __secondary_hold_acknowledge:
> >> tophys(r11,r1); /* use tophys(r1) if kernel */ \
> >> beq 1f; \
> >> mfspr r11,SPRN_SPRG_THREAD; \
> >> - lwz r11,THREAD_INFO-THREAD(r11); \
> >> + lwz r11,TASK_STACK-THREAD(r11); \
> >> addi r11,r11,THREAD_SIZE; \
> >> tophys(r11,r11); \
> >> 1: subi r11,r11,INT_FRAME_SIZE /* alloc exc. frame */
> >> @@ -841,7 +841,7 @@ __secondary_start:
> >> bl init_idle_6xx
> >> #endif /* CONFIG_6xx */
> >>
> >> - /* get current_thread_info and current */
> >> + /* get current's stack and current */
> >> lis r1,secondary_ti@ha
> >> tophys(r1,r1)
> >> lwz r1,secondary_ti@l(r1)
> >> diff --git a/arch/powerpc/kernel/head_40x.S b/arch/powerpc/kernel/head_40x.S
> >> index b19d78410511..3088c9f29f5e 100644
> >> --- a/arch/powerpc/kernel/head_40x.S
> >> +++ b/arch/powerpc/kernel/head_40x.S
> >> @@ -115,7 +115,7 @@ _ENTRY(saved_ksp_limit)
> >> andi. r11,r11,MSR_PR; \
> >> beq 1f; \
> >> mfspr r1,SPRN_SPRG_THREAD; /* if from user, start at top of */\
> >> - lwz r1,THREAD_INFO-THREAD(r1); /* this thread's kernel stack */\
> >> + lwz r1,TASK_STACK-THREAD(r1); /* this thread's kernel stack */\
> >> addi r1,r1,THREAD_SIZE; \
> >> 1: subi r1,r1,INT_FRAME_SIZE; /* Allocate an exception frame */\
> >> tophys(r11,r1); \
> >> @@ -158,7 +158,7 @@ _ENTRY(saved_ksp_limit)
> >> beq 1f; \
> >> /* COMING FROM USER MODE */ \
> >> mfspr r11,SPRN_SPRG_THREAD; /* if from user, start at top of */\
> >> - lwz r11,THREAD_INFO-THREAD(r11); /* this thread's kernel stack */\
> >> + lwz r11,TASK_STACK-THREAD(r11); /* this thread's kernel stack */\
> >> 1: addi r11,r11,THREAD_SIZE-INT_FRAME_SIZE; /* Alloc an excpt frm */\
> >> tophys(r11,r11); \
> >> stw r10,_CCR(r11); /* save various registers */\
> >> diff --git a/arch/powerpc/kernel/head_44x.S b/arch/powerpc/kernel/head_44x.S
> >> index 37e4a7cf0065..15d39b2499de 100644
> >> --- a/arch/powerpc/kernel/head_44x.S
> >> +++ b/arch/powerpc/kernel/head_44x.S
> >> @@ -1020,7 +1020,7 @@ _GLOBAL(start_secondary_47x)
> >>
> >> /* Now we can get our task struct and real stack pointer */
> >>
> >> - /* Get current_thread_info and current */
> >> + /* Get current's stack and current */
> >> lis r1,secondary_ti@ha
> >> lwz r1,secondary_ti@l(r1)
> >> lwz r2,TI_TASK(r1)
> >> diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
> >> index 6582f824d620..e56e36aa2b3d 100644
> >> --- a/arch/powerpc/kernel/head_8xx.S
> >> +++ b/arch/powerpc/kernel/head_8xx.S
> >> @@ -124,7 +124,7 @@ turn_on_mmu:
> >> tophys(r11,r1); /* use tophys(r1) if kernel */ \
> >> beq 1f; \
> >> mfspr r11,SPRN_SPRG_THREAD; \
> >> - lwz r11,THREAD_INFO-THREAD(r11); \
> >> + lwz r11,TASK_STACK-THREAD(r11); \
> >> addi r11,r11,THREAD_SIZE; \
> >> tophys(r11,r11); \
> >> 1: subi r11,r11,INT_FRAME_SIZE /* alloc exc. frame */
> >> diff --git a/arch/powerpc/kernel/head_booke.h b/arch/powerpc/kernel/head_booke.h
> >> index d0862a100d29..20fe0c93a0bd 100644
> >> --- a/arch/powerpc/kernel/head_booke.h
> >> +++ b/arch/powerpc/kernel/head_booke.h
> >> @@ -44,7 +44,7 @@
> >> mr r11, r1; \
> >> beq 1f; \
> >> /* if from user, start at top of this thread's kernel stack */ \
> >> - lwz r11, THREAD_INFO-THREAD(r10); \
> >> + lwz r11, TASK_STACK-THREAD(r10); \
> >> ALLOC_STACK_FRAME(r11, THREAD_SIZE); \
> >> 1 : subi r11, r11, INT_FRAME_SIZE; /* Allocate exception frame */ \
> >> stw r13, _CCR(r11); /* save various registers */ \
> >> @@ -130,7 +130,7 @@
> >> DO_KVM BOOKE_INTERRUPT_##intno exc_level_srr1; \
> >> andi. r11,r11,MSR_PR; \
> >> mfspr r11,SPRN_SPRG_THREAD; /* if from user, start at top of */\
> >> - lwz r11,THREAD_INFO-THREAD(r11); /* this thread's kernel stack */\
> >> + lwz r11,TASK_STACK-THREAD(r11); /* this thread's kernel stack */\
> >> addi r11,r11,EXC_LVL_FRAME_OVERHEAD; /* allocate stack frame */\
> >> beq 1f; \
> >> /* COMING FROM USER MODE */ \
> >> diff --git a/arch/powerpc/kernel/head_fsl_booke.S b/arch/powerpc/kernel/head_fsl_booke.S
> >> index e2750b856c8f..239ad8a4754e 100644
> >> --- a/arch/powerpc/kernel/head_fsl_booke.S
> >> +++ b/arch/powerpc/kernel/head_fsl_booke.S
> >> @@ -243,8 +243,10 @@ set_ivor:
> >> li r0,0
> >> stwu r0,THREAD_SIZE-STACK_FRAME_OVERHEAD(r1)
> >>
> >> +#ifdef CONFIG_SMP
> >> CURRENT_THREAD_INFO(r22, r1)
> >> stw r24, TI_CPU(r22)
> >> +#endif
> >>
> >> bl early_init
> >>
> >> @@ -702,7 +704,7 @@ finish_tlb_load:
> >>
> >> /* Get the next_tlbcam_idx percpu var */
> >> #ifdef CONFIG_SMP
> >> - lwz r12, THREAD_INFO-THREAD(r12)
> >> + lwz r12, TASK_STACK-THREAD(r12)
> >> lwz r15, TI_CPU(r12)
> >> lis r14, __per_cpu_offset@h
> >> ori r14, r14, __per_cpu_offset@l
> >> @@ -1074,7 +1076,7 @@ __secondary_start:
> >> mr r4,r24 /* Why? */
> >> bl call_setup_cpu
> >>
> >> - /* get current_thread_info and current */
> >> + /* get current's stack and current */
> >> lis r1,secondary_ti@ha
> >> lwz r1,secondary_ti@l(r1)
> >> lwz r2,TI_TASK(r1)
> >> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> >> index 916ddc4aac44..aa53db3ba6e7 100644
> >> --- a/arch/powerpc/kernel/irq.c
> >> +++ b/arch/powerpc/kernel/irq.c
> >> @@ -663,7 +663,7 @@ void do_IRQ(struct pt_regs *regs)
> >> struct thread_info *curtp, *irqtp, *sirqtp;
> >>
> >> /* Switch to the irq stack to handle this */
> >> - curtp = current_thread_info();
> >> + curtp = (void*)(current_stack_pointer() & ~(THREAD_SIZE - 1));
> >> irqtp = hardirq_ctx[raw_smp_processor_id()];
> >> sirqtp = softirq_ctx[raw_smp_processor_id()];
> >>
> >> diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
> >> index 695b24a2d954..24a7f18ea10c 100644
> >> --- a/arch/powerpc/kernel/misc_32.S
> >> +++ b/arch/powerpc/kernel/misc_32.S
> >> @@ -60,7 +60,7 @@ _GLOBAL(call_do_softirq)
> >> blr
> >>
> >> /*
> >> - * void call_do_irq(struct pt_regs *regs, struct thread_info *irqtp);
> >> + * void call_do_irq(struct pt_regs *regs, void *irqtp);
> >> */
> >> _GLOBAL(call_do_irq)
> >> mflr r0
> >> @@ -183,10 +183,14 @@ _GLOBAL(low_choose_750fx_pll)
> >> or r4,r4,r5
> >> mtspr SPRN_HID1,r4
> >>
> >> +#ifdef CONFIG_SMP
> >> /* Store new HID1 image */
> >> CURRENT_THREAD_INFO(r6, r1)
> >> lwz r6,TI_CPU(r6)
> >> slwi r6,r6,2
> >> +#else
> >> + li r6, 0
> >> +#endif
> >> addis r6,r6,nap_save_hid1@ha
> >> stw r4,nap_save_hid1@l(r6)
> >>
> >> @@ -599,7 +603,7 @@ EXPORT_SYMBOL(__bswapdi2)
> >> #ifdef CONFIG_SMP
> >> _GLOBAL(start_secondary_resume)
> >> /* Reset stack */
> >> - CURRENT_THREAD_INFO(r1, r1)
> >> + rlwinm r1, r1, 0, 0, 31 - THREAD_SHIFT
> >> addi r1,r1,THREAD_SIZE-STACK_FRAME_OVERHEAD
> >> li r3,0
> >> stw r3,0(r1) /* Zero the stack frame pointer */
> >> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> >> index 03c2e1f134bc..111abb4df2ec 100644
> >> --- a/arch/powerpc/kernel/process.c
> >> +++ b/arch/powerpc/kernel/process.c
> >> @@ -1240,8 +1240,8 @@ struct task_struct *__switch_to(struct task_struct *prev,
> >> batch->active = 1;
> >> }
> >>
> >> - if (current_thread_info()->task->thread.regs) {
> >> - restore_math(current_thread_info()->task->thread.regs);
> >> + if (current->thread.regs) {
> >> + restore_math(current->thread.regs);
> >>
> >> /*
> >> * The copy-paste buffer can only store into foreign real
> >> @@ -1251,7 +1251,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
> >> * mappings, we must issue a cp_abort to clear any state and
> >> * prevent snooping, corruption or a covert channel.
> >> */
> >> - if (current_thread_info()->task->thread.used_vas)
> >> + if (current->thread.used_vas)
> >> asm volatile(PPC_CP_ABORT);
> >> }
> >> #endif /* CONFIG_PPC_BOOK3S_64 */
> >> diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
> >> index 8c507be12c3c..81ebf7d6f526 100644
> >> --- a/arch/powerpc/kernel/setup_32.c
> >> +++ b/arch/powerpc/kernel/setup_32.c
> >> @@ -205,10 +205,8 @@ void __init irqstack_early_init(void)
> >> /* interrupt stacks must be in lowmem, we get that for free on ppc32
> >> * as the memblock is limited to lowmem by default */
> >> for_each_possible_cpu(i) {
> >> - softirq_ctx[i] = (struct thread_info *)
> >> - __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
> >> - hardirq_ctx[i] = (struct thread_info *)
> >> - __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
> >> + softirq_ctx[i] = __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
> >> + hardirq_ctx[i] = __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
> >> }
> >> }
> >>
> >> @@ -226,13 +224,10 @@ void __init exc_lvl_early_init(void)
> >> hw_cpu = 0;
> >> #endif
> >>
> >> - critirq_ctx[hw_cpu] = (struct thread_info *)
> >> - __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
> >> + critirq_ctx[hw_cpu] = __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
> >> #ifdef CONFIG_BOOKE
> >> - dbgirq_ctx[hw_cpu] = (struct thread_info *)
> >> - __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
> >> - mcheckirq_ctx[hw_cpu] = (struct thread_info *)
> >> - __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
> >> + dbgirq_ctx[hw_cpu] = __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
> >> + mcheckirq_ctx[hw_cpu] = __va(memblock_alloc(THREAD_SIZE, THREAD_SIZE));
> >> #endif
> >> }
> >> }
> >> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> >> index 61c1fadbc644..19dd0ea55714 100644
> >> --- a/arch/powerpc/kernel/smp.c
> >> +++ b/arch/powerpc/kernel/smp.c
> >> @@ -20,6 +20,7 @@
> >> #include <linux/kernel.h>
> >> #include <linux/export.h>
> >> #include <linux/sched/mm.h>
> >> +#include <linux/sched/task_stack.h>
> >> #include <linux/sched/topology.h>
> >> #include <linux/smp.h>
> >> #include <linux/interrupt.h>
> >> @@ -812,7 +813,8 @@ static void cpu_idle_thread_init(unsigned int cpu, struct task_struct *idle)
> >>
> >> #ifdef CONFIG_PPC64
> >> paca_ptrs[cpu]->__current = idle;
> >> - paca_ptrs[cpu]->kstack = (unsigned long)ti + THREAD_SIZE - STACK_FRAME_OVERHEAD;
> >> + paca_ptrs[cpu]->kstack = (unsigned long)task_stack_page(idle) +
> >> + THREAD_SIZE - STACK_FRAME_OVERHEAD;
> >> #endif
> >> ti->cpu = cpu;
> >> secondary_ti = current_set[cpu] = ti;
> >> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> >> index 694c1d92e796..0d8d6fee892a 100644
> >> --- a/arch/powerpc/xmon/xmon.c
> >> +++ b/arch/powerpc/xmon/xmon.c
> >> @@ -2988,7 +2988,7 @@ static void show_task(struct task_struct *tsk)
> >> printf("%px %016lx %6d %6d %c %2d %s\n", tsk,
> >> tsk->thread.ksp,
> >> tsk->pid, tsk->parent->pid,
> >> - state, task_thread_info(tsk)->cpu,
> >> + state, task_cpu(tsk),
> >> tsk->comm);
> >> }
> >>
> >> --
> >> 2.13.3
> >>
^ permalink raw reply
* Re: [RFC PATCH v3 5/7] powerpc: 'current_set' is now a table of task_struct pointers
From: Christophe LEROY @ 2018-10-03 6:00 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <20181003154101.0b46956d@roar.ozlabs.ibm.com>
Le 03/10/2018 à 07:41, Nicholas Piggin a écrit :
> On Mon, 1 Oct 2018 12:30:27 +0000 (UTC)
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>> The table of pointers 'current_set' has been used for retrieving
>> the stack and current. They used to be thread_info pointers as
>> they were pointing to the stack and current was taken from the
>> 'task' field of the thread_info.
>>
>> Now, the pointers of 'current_set' table are now both pointers
>> to task_struct and pointers to thread_info.
>>
>> As they are used to get current, and the stack pointer is
>> retrieved from current's stack field, this patch changes
>> their type to task_struct, and renames secondary_ti to
>> secondary_current.
>
> I'm not sure if current_set is actually needed is it? Because
> 64-bit already initializes paca->ksave / PACAKSAVE. That might
> be a cleanup to do after your series.
head_64.S contains:
__secondary_start:
/* Set thread priority to MEDIUM */
HMT_MEDIUM
/* Initialize the kernel stack */
LOAD_REG_ADDR(r3, current_set)
sldi r28,r24,3 /* get current_set[cpu#] */
ldx r14,r3,r28
addi r14,r14,THREAD_SIZE-STACK_FRAME_OVERHEAD
std r14,PACAKSAVE(r13)
32-bit doesn't seem to use it, it only uses secondary_ti it seems.
>
> Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
Christophe
^ permalink raw reply
* Re: [RFC PATCH v3 7/7] powerpc/64: Modify CURRENT_THREAD_INFO()
From: Christophe LEROY @ 2018-10-03 6:01 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <20181003154442.5d77ad9c@roar.ozlabs.ibm.com>
Le 03/10/2018 à 07:44, Nicholas Piggin a écrit :
> On Mon, 1 Oct 2018 12:30:31 +0000 (UTC)
> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>> CURRENT_THREAD_INFO() now uses the PACA to retrieve 'current' pointer,
>> it doesn't use 'sp' anymore.
>
> Can you remove this too now? I think it will be clearer what's going on
> and easier to read once everyone remembers current is the same offset as
> current thread_info.
Ok I will.
Christophe
>
> Overall nice series, thanks for doing this.
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [RFC PATCH v3 3/7] powerpc: Activate CONFIG_THREAD_INFO_IN_TASK
From: Christophe LEROY @ 2018-10-03 6:04 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <20181003155243.70806497@roar.ozlabs.ibm.com>
Le 03/10/2018 à 07:52, Nicholas Piggin a écrit :
> On Wed, 3 Oct 2018 07:47:05 +0200
> Christophe LEROY <christophe.leroy@c-s.fr> wrote:
>
>> Le 03/10/2018 à 07:30, Nicholas Piggin a écrit :
>>> On Mon, 1 Oct 2018 12:30:23 +0000 (UTC)
>>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>>
>>>> This patch activates CONFIG_THREAD_INFO_IN_TASK which
>>>> moves the thread_info into task_struct.
>>>>
>>>> Moving thread_info into task_struct has the following advantages:
>>>> - It protects thread_info from corruption in the case of stack
>>>> overflows.
>>>> - Its address is harder to determine if stack addresses are
>>>> leaked, making a number of attacks more difficult.
>>>>
>>>> This has the following consequences:
>>>> - thread_info is now located at the top of task_struct.
>>>
>>> "top"... I got confused for a minute thinking high address and
>>> wondering how you can change CURRENT_THREAD_INFO just to point
>>> to current :)
>>
>> Would 'beginning' be less confusing ?
>
> Yes, good idea.
>
>>>> @@ -83,7 +83,13 @@ int is_cpu_dead(unsigned int cpu);
>>>> /* 32-bit */
>>>> extern int smp_hw_index[];
>>>>
>>>> -#define raw_smp_processor_id() (current_thread_info()->cpu)
>>>> +/*
>>>> + * This is particularly ugly: it appears we can't actually get the definition
>>>> + * of task_struct here, but we need access to the CPU this task is running on.
>>>> + * Instead of using task_struct we're using _TASK_CPU which is extracted from
>>>> + * asm-offsets.h by kbuild to get the current processor ID.
>>>> + */
>>>> +#define raw_smp_processor_id() (*(unsigned int*)((void*)current + _TASK_CPU))
>>>
>>> This is clever but yes ugly. Can't you include asm-offsets.h? riscv
>>> seems to.
>>
>> riscv has a clean asm-offsets.h . Our's defines constant with the same
>> name as those defined in other headers which are included in C files. So
>> including asm-offsets in C files does create conflicts like:
>>
>> ./include/generated/asm-offsets.h:71:0: warning: "TASK_SIZE" redefined
>> #define TASK_SIZE -2147483648 /* TASK_SIZE */
>> ./arch/powerpc/include/asm/processor.h:95:0: note: this is the location
>> of the previous definition
>> #define TASK_SIZE (CONFIG_TASK_SIZE)
>>
>> ./include/generated/asm-offsets.h:98:0: warning: "NSEC_PER_SEC" redefined
>> #define NSEC_PER_SEC 1000000000 /* NSEC_PER_SEC */
>> ./include/linux/time64.h:36:0: note: this is the location of the
>> previous definition
>> #define NSEC_PER_SEC 1000000000L
>>
>> ./arch/powerpc/include/asm/nohash/32/pgtable.h:34:0: warning:
>> "PGD_TABLE_SIZE" redefined
>> #define PGD_TABLE_SIZE (sizeof(pgd_t) << PGD_INDEX_SIZE)
>> ./include/generated/asm-offsets.h:101:0: note: this is the location of
>> the previous definition
>> #define PGD_TABLE_SIZE 256 /* PGD_TABLE_SIZE */
>>
>> ...
>
> Okay.
>
>>
>> In v2, I had a patch to fix those redundancies
>> (https://patchwork.ozlabs.org/patch/974363/) but I found it unconvenient.
>
> Because of merge conflicts, or you did not like the new names?
Both, because of the amount of changes it implies, and also because of
the new names. I find it quite convenient to be able to use same names
both in C and ASM. And I didn't want my serie to imply big-bangs in
unrelated or not directly related topics.
Christophe
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [RFC PATCH v3 5/7] powerpc: 'current_set' is now a table of task_struct pointers
From: Nicholas Piggin @ 2018-10-03 6:09 UTC (permalink / raw)
To: Christophe LEROY; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <0336ff43-55ac-067b-23a9-4cb4b2fd38f3@c-s.fr>
On Wed, 3 Oct 2018 08:00:43 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> Le 03/10/2018 à 07:41, Nicholas Piggin a écrit :
> > On Mon, 1 Oct 2018 12:30:27 +0000 (UTC)
> > Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >
> >> The table of pointers 'current_set' has been used for retrieving
> >> the stack and current. They used to be thread_info pointers as
> >> they were pointing to the stack and current was taken from the
> >> 'task' field of the thread_info.
> >>
> >> Now, the pointers of 'current_set' table are now both pointers
> >> to task_struct and pointers to thread_info.
> >>
> >> As they are used to get current, and the stack pointer is
> >> retrieved from current's stack field, this patch changes
> >> their type to task_struct, and renames secondary_ti to
> >> secondary_current.
> >
> > I'm not sure if current_set is actually needed is it? Because
> > 64-bit already initializes paca->ksave / PACAKSAVE. That might
> > be a cleanup to do after your series.
>
> head_64.S contains:
>
> __secondary_start:
> /* Set thread priority to MEDIUM */
> HMT_MEDIUM
>
> /* Initialize the kernel stack */
> LOAD_REG_ADDR(r3, current_set)
> sldi r28,r24,3 /* get current_set[cpu#] */
> ldx r14,r3,r28
> addi r14,r14,THREAD_SIZE-STACK_FRAME_OVERHEAD
> std r14,PACAKSAVE(r13)
Right, I don't *think* that's needed because boot CPU should already
have set PACAKSAVE before starting secondaries here. ld r14,PACAKSAVE
should have the same result I think.
But never mind that for your series, just something I saw that could
be cleaned up.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 28/33] KVM: PPC: Book3S HV: Sanitise hv_regs on nested guest entry
From: David Gibson @ 2018-10-03 6:07 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1538479892-14835-29-git-send-email-paulus@ozlabs.org>
[-- Attachment #1: Type: text/plain, Size: 3712 bytes --]
On Tue, Oct 02, 2018 at 09:31:27PM +1000, Paul Mackerras wrote:
> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>
> restore_hv_regs() is used to copy the hv_regs L1 wants to set to run the
> nested (L2) guest into the vcpu structure. We need to sanitise these
> values to ensure we don't let the L1 guest hypervisor do things we don't
> want it to.
>
> We don't let data address watchpoints or completed instruction address
> breakpoints be set to match in hypervisor state.
>
> We also don't let L1 enable features in the hypervisor facility status
> and control register (HFSCR) for L2 which we have disabled for L1. That
> is L2 will get the subset of features which the L0 hypervisor has
> enabled for L1 and the features L1 wants to enable for L2. This could
> mean we give L1 a hypervisor facility unavailable interrupt for a
> facility it thinks it has enabled, however it shouldn't have enabled a
> facility it itself doesn't have for the L2 guest.
>
> We sanitise the registers when copying in the L2 hv_regs. We don't need
> to sanitise when copying back the L1 hv_regs since these shouldn't be
> able to contain invalid values as they're just what was copied out.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> arch/powerpc/include/asm/reg.h | 1 +
> arch/powerpc/kvm/book3s_hv_nested.c | 17 +++++++++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 9c42abf..47489f6 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -415,6 +415,7 @@
> #define HFSCR_DSCR __MASK(FSCR_DSCR_LG)
> #define HFSCR_VECVSX __MASK(FSCR_VECVSX_LG)
> #define HFSCR_FP __MASK(FSCR_FP_LG)
> +#define HFSCR_INTR_CAUSE (ASM_CONST(0xFF) << 56) /* interrupt cause */
> #define SPRN_TAR 0x32f /* Target Address Register */
> #define SPRN_LPCR 0x13E /* LPAR Control Register */
> #define LPCR_VPM0 ASM_CONST(0x8000000000000000)
> diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
> index 7656cb3..7b1088a 100644
> --- a/arch/powerpc/kvm/book3s_hv_nested.c
> +++ b/arch/powerpc/kvm/book3s_hv_nested.c
> @@ -86,6 +86,22 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
> }
> }
>
> +static void sanitise_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
> +{
> + /*
> + * Don't let L1 enable features for L2 which we've disabled for L1,
> + * but preserve the interrupt cause field.
> + */
> + hr->hfscr &= (HFSCR_INTR_CAUSE | vcpu->arch.hfscr);
> +
> + /* Don't let data address watchpoint match in hypervisor state */
> + hr->dawrx0 &= ~DAWRX_HYP;
> +
> + /* Don't let completed instruction address breakpt match in HV state */
> + if ((hr->ciabr & CIABR_PRIV) == CIABR_PRIV_HYPER)
> + hr->ciabr &= ~CIABR_PRIV;
> +}
> +
> static void restore_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
> {
> struct kvmppc_vcore *vc = vcpu->arch.vcore;
> @@ -198,6 +214,7 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
> mask = LPCR_DPFD | LPCR_ILE | LPCR_TC | LPCR_AIL | LPCR_LD |
> LPCR_LPES | LPCR_MER;
> lpcr = (vc->lpcr & ~mask) | (l2_hv.lpcr & mask);
> + sanitise_hv_regs(vcpu, &l2_hv);
> restore_hv_regs(vcpu, &l2_hv);
>
> vcpu->arch.ret = RESUME_GUEST;
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 33/33] KVM: PPC: Book3S HV: Add a VM capability to enable nested virtualization
From: David Gibson @ 2018-10-03 6:21 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1538479892-14835-34-git-send-email-paulus@ozlabs.org>
[-- Attachment #1: Type: text/plain, Size: 5673 bytes --]
On Tue, Oct 02, 2018 at 09:31:32PM +1000, Paul Mackerras wrote:
> With this, userspace can enable a KVM-HV guest to run nested guests
> under it.
>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> ---
> Documentation/virtual/kvm/api.txt | 14 ++++++++++++++
> arch/powerpc/include/asm/kvm_ppc.h | 1 +
> arch/powerpc/kvm/book3s_hv.c | 17 +++++++++++++++++
> arch/powerpc/kvm/powerpc.c | 12 ++++++++++++
> include/uapi/linux/kvm.h | 1 +
> 5 files changed, 45 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 017d851..a2d4832 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -4522,6 +4522,20 @@ hpage module parameter is not set to 1, -EINVAL is returned.
> While it is generally possible to create a huge page backed VM without
> this capability, the VM will not be able to run.
>
> +7.15 KVM_CAP_PPC_NESTED_HV
> +
> +Architectures: ppc
> +Parameters: enable flag (0 to disable, non-zero to enable)
> +Returns: 0 on success, -EINVAL when the implementation doesn't support
> +nested-HV virtualization.
> +
> +HV-KVM on POWER9 and later systems allows for "nested-HV"
> +virtualization, which provides a way for a guest VM to run guests that
> +can run using the CPU's supervisor mode (privileged non-hypervisor
> +state). Enabling this capability on a VM depends on the CPU having
> +the necessary functionality and on the facility being enabled with a
> +kvm-hv module parameter.
> +
> 8. Other capabilities.
> ----------------------
>
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 245e564..80f0091 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -327,6 +327,7 @@ struct kvmppc_ops {
> int (*set_smt_mode)(struct kvm *kvm, unsigned long mode,
> unsigned long flags);
> void (*giveup_ext)(struct kvm_vcpu *vcpu, ulong msr);
> + int (*enable_nested)(struct kvm *kvm, bool enable);
> };
>
> extern struct kvmppc_ops *kvmppc_hv_ops;
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 196bff1..a5b3862 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -118,6 +118,11 @@ module_param_cb(h_ipi_redirect, &module_param_ops, &h_ipi_redirect, 0644);
> MODULE_PARM_DESC(h_ipi_redirect, "Redirect H_IPI wakeup to a free host core");
> #endif
>
> +/* If set, guests are allowed to create and control nested guests */
> +static bool enable_nested = true;
> +module_param(enable_nested, bool, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(enable_nested, "Enable nested virtualization (only on POWER9)");
I'd suggest calling the module parameter just "nested" to match x86.
> /* If set, the threads on each CPU core have to be in the same MMU mode */
> static bool no_mixing_hpt_and_radix;
>
> @@ -5188,6 +5193,17 @@ static int kvmhv_configure_mmu(struct kvm *kvm, struct kvm_ppc_mmuv3_cfg *cfg)
> return err;
> }
>
> +static int kvmhv_enable_nested(struct kvm *kvm, bool enable)
> +{
> + if (!(enable_nested && cpu_has_feature(CPU_FTR_ARCH_300)))
> + return -EINVAL;
Maybe EPERM, rather than EINVAL. There's nothing invalid about the
ioctl() parameters - we just can't do what they want.
> +
> + /* kvm == NULL means the caller is testing if the capability exists */
> + if (kvm)
> + kvm->arch.nested_enable = enable;
> + return 0;
> +}
> +
> static struct kvmppc_ops kvm_ops_hv = {
> .get_sregs = kvm_arch_vcpu_ioctl_get_sregs_hv,
> .set_sregs = kvm_arch_vcpu_ioctl_set_sregs_hv,
> @@ -5227,6 +5243,7 @@ static struct kvmppc_ops kvm_ops_hv = {
> .configure_mmu = kvmhv_configure_mmu,
> .get_rmmu_info = kvmhv_get_rmmu_info,
> .set_smt_mode = kvmhv_set_smt_mode,
> + .enable_nested = kvmhv_enable_nested,
> };
>
> static int kvm_init_subcore_bitmap(void)
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index eba5756..449ae1d 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -596,6 +596,10 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_PPC_MMU_HASH_V3:
> r = !!(hv_enabled && cpu_has_feature(CPU_FTR_ARCH_300));
> break;
> + case KVM_CAP_PPC_NESTED_HV:
> + r = !!(hv_enabled && kvmppc_hv_ops->enable_nested &&
> + !kvmppc_hv_ops->enable_nested(NULL, false));
> + break;
> #endif
> case KVM_CAP_SYNC_MMU:
> #ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> @@ -2114,6 +2118,14 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> r = kvm->arch.kvm_ops->set_smt_mode(kvm, mode, flags);
> break;
> }
> +
> + case KVM_CAP_PPC_NESTED_HV:
> + r = -EINVAL;
> + if (!is_kvmppc_hv_enabled(kvm) ||
> + !kvm->arch.kvm_ops->enable_nested)
> + break;
> + r = kvm->arch.kvm_ops->enable_nested(kvm, !!cap->args[0]);
> + break;
> #endif
> default:
> r = -EINVAL;
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 07548de..a6d5a46 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -952,6 +952,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_S390_HPAGE_1M 156
> #define KVM_CAP_NESTED_STATE 157
> #define KVM_CAP_ARM_INJECT_SERROR_ESR 158
> +#define KVM_CAP_PPC_NESTED_HV 160
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 32/33] KVM: PPC: Book3S HV: Use hypercalls for TLB invalidation when nested
From: David Gibson @ 2018-10-03 6:17 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1538479892-14835-33-git-send-email-paulus@ozlabs.org>
[-- Attachment #1: Type: text/plain, Size: 5873 bytes --]
On Tue, Oct 02, 2018 at 09:31:31PM +1000, Paul Mackerras wrote:
> This adds code to call the H_TLB_INVALIDATE hypercall when running as
> a guest, in the cases where we need to invalidate TLBs (or other MMU
> caches) as part of managing the mappings for a nested guest. Calling
> H_TLB_INVALIDATE is an alternative to doing the tlbie instruction and
> having it be emulated by our hypervisor.
Why is supporting two methods useful, rather than just settling on
one? Having both just sounds like asking for untested and therefore
broken code paths lying around.
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> ---
> arch/powerpc/include/asm/kvm_book3s_64.h | 5 +++++
> arch/powerpc/kvm/book3s_64_mmu_radix.c | 30 +++++++++++++++++++++++++++--
> arch/powerpc/kvm/book3s_hv_nested.c | 33 +++++++++++++++++++++++---------
> 3 files changed, 57 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
> index f0fa14c..95280e2 100644
> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
> @@ -24,6 +24,7 @@
> #include <asm/bitops.h>
> #include <asm/book3s/64/mmu-hash.h>
> #include <asm/cpu_has_feature.h>
> +#include <asm/ppc-opcode.h>
>
> #ifdef CONFIG_PPC_PSERIES
> static inline bool kvmhv_on_pseries(void)
> @@ -121,6 +122,10 @@ struct kvm_nested_guest *kvmhv_get_nested(struct kvm *kvm, int l1_lpid,
> void kvmhv_put_nested(struct kvm_nested_guest *gp);
> int kvmhv_nested_next_lpid(struct kvm *kvm, int lpid);
>
> +/* Encoding of first parameter for H_TLB_INVALIDATE */
> +#define H_TLBIE_P1_ENC(ric, prs, r) (___PPC_RIC(ric) | ___PPC_PRS(prs) | \
> + ___PPC_R(r))
> +
> /* Power architecture requires HPT is at least 256kiB, at most 64TiB */
> #define PPC_MIN_HPT_ORDER 18
> #define PPC_MAX_HPT_ORDER 46
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_radix.c b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> index b74abdd..6c93f5c 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_radix.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_radix.c
> @@ -201,17 +201,43 @@ static void kvmppc_radix_tlbie_page(struct kvm *kvm, unsigned long addr,
> unsigned int pshift, unsigned int lpid)
> {
> unsigned long psize = PAGE_SIZE;
> + int psi;
> + long rc;
> + unsigned long rb;
>
> if (pshift)
> psize = 1UL << pshift;
> + else
> + pshift = PAGE_SHIFT;
>
> addr &= ~(psize - 1);
> - radix__flush_tlb_lpid_page(lpid, addr, psize);
> +
> + if (!kvmhv_on_pseries()) {
> + radix__flush_tlb_lpid_page(lpid, addr, psize);
> + return;
> + }
> +
> + psi = shift_to_mmu_psize(pshift);
> + rb = addr | (mmu_get_ap(psi) << PPC_BITLSHIFT(58));
> + rc = plpar_hcall_norets(H_TLB_INVALIDATE, H_TLBIE_P1_ENC(0, 0, 1),
> + lpid, rb);
> + if (rc)
> + pr_err("KVM: TLB page invalidation hcall failed, rc=%ld\n", rc);
> }
>
> static void kvmppc_radix_flush_pwc(struct kvm *kvm, unsigned int lpid)
> {
> - radix__flush_pwc_lpid(lpid);
> + long rc;
> +
> + if (!kvmhv_on_pseries()) {
> + radix__flush_pwc_lpid(lpid);
> + return;
> + }
> +
> + rc = plpar_hcall_norets(H_TLB_INVALIDATE, H_TLBIE_P1_ENC(1, 0, 1),
> + lpid, TLBIEL_INVAL_SET_LPID);
> + if (rc)
> + pr_err("KVM: TLB PWC invalidation hcall failed, rc=%ld\n", rc);
> }
>
> static unsigned long kvmppc_radix_update_pte(struct kvm *kvm, pte_t *ptep,
> diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
> index 914ca78..81c81d51 100644
> --- a/arch/powerpc/kvm/book3s_hv_nested.c
> +++ b/arch/powerpc/kvm/book3s_hv_nested.c
> @@ -368,17 +368,32 @@ void kvmhv_nested_exit(void)
> }
> }
>
> +static void kvmhv_flush_lpid(unsigned int lpid)
> +{
> + long rc;
> +
> + if (!kvmhv_on_pseries()) {
> + radix__flush_tlb_lpid(lpid);
> + return;
> + }
> +
> + rc = plpar_hcall_norets(H_TLB_INVALIDATE, H_TLBIE_P1_ENC(2, 0, 1),
> + lpid, TLBIEL_INVAL_SET_LPID);
> + if (rc)
> + pr_err("KVM: TLB LPID invalidation hcall failed, rc=%ld\n", rc);
> +}
> +
> void kvmhv_set_ptbl_entry(unsigned int lpid, u64 dw0, u64 dw1)
> {
> - if (cpu_has_feature(CPU_FTR_HVMODE)) {
> + if (!kvmhv_on_pseries()) {
> mmu_partition_table_set_entry(lpid, dw0, dw1);
> - } else {
> - pseries_partition_tb[lpid].patb0 = cpu_to_be64(dw0);
> - pseries_partition_tb[lpid].patb1 = cpu_to_be64(dw1);
> - /* this will be emulated, L0 will do the necessary barriers */
> - asm volatile(PPC_TLBIE_5(%0, %1, 2, 0, 1) : :
> - "r" (TLBIEL_INVAL_SET_LPID), "r" (lpid));
> + return;
> }
> +
> + pseries_partition_tb[lpid].patb0 = cpu_to_be64(dw0);
> + pseries_partition_tb[lpid].patb1 = cpu_to_be64(dw1);
> + /* L0 will do the necessary barriers */
> + kvmhv_flush_lpid(lpid);
> }
>
> static void kvmhv_set_nested_ptbl(struct kvm_nested_guest *gp)
> @@ -543,7 +558,7 @@ static void kvmhv_flush_nested(struct kvm_nested_guest *gp)
> spin_lock(&kvm->mmu_lock);
> kvmppc_free_pgtable_radix(kvm, gp->shadow_pgtable, gp->shadow_lpid);
> spin_unlock(&kvm->mmu_lock);
> - radix__flush_tlb_lpid(gp->shadow_lpid);
> + kvmhv_flush_lpid(gp->shadow_lpid);
> kvmhv_update_ptbl_cache(gp);
> if (gp->l1_gr_to_hr == 0)
> kvmhv_remove_nested(gp);
> @@ -839,7 +854,7 @@ static void kvmhv_emulate_tlbie_lpid(struct kvm_vcpu *vcpu,
> spin_lock(&kvm->mmu_lock);
> kvmppc_free_pgtable_radix(kvm, gp->shadow_pgtable,
> gp->shadow_lpid);
> - radix__flush_tlb_lpid(gp->shadow_lpid);
> + kvmhv_flush_lpid(gp->shadow_lpid);
> spin_unlock(&kvm->mmu_lock);
> break;
> case 1:
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 23/33] KVM: PPC: Book3S HV: Introduce rmap to track nested guest mappings
From: David Gibson @ 2018-10-03 5:56 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1538479892-14835-24-git-send-email-paulus@ozlabs.org>
[-- Attachment #1: Type: text/plain, Size: 4548 bytes --]
On Tue, Oct 02, 2018 at 09:31:22PM +1000, Paul Mackerras wrote:
> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>
> When a host (L0) page which is mapped into a (L1) guest is in turn
> mapped through to a nested (L2) guest we keep a reverse mapping (rmap)
> so that these mappings can be retrieved later.
>
> Whenever we create an entry in a shadow_pgtable for a nested guest we
> create a corresponding rmap entry and add it to the list for the
> L1 guest memslot at the index of the L1 guest page it maps. This means
> at the L1 guest memslot we end up with lists of rmaps.
>
> When we are notified of a host page being invalidated which has been
> mapped through to a (L1) guest, we can then walk the rmap list for that
> guest page, and find and invalidate all of the corresponding
> shadow_pgtable entries.
>
> In order to reduce memory consumption, we compress the information for
> each rmap entry down to 52 bits -- 12 bits for the LPID and 40 bits
> for the guest real page frame number -- which will fit in a single
> unsigned long. To avoid a scenario where a guest can trigger
> unbounded memory allocations, we scan the list when adding an entry to
> see if there is already an entry with the contents we need. This can
> occur, because we don't ever remove entries from the middle of a list.
>
> A struct nested guest rmap is a list pointer and an rmap entry;
> ----------------
> | next pointer |
> ----------------
> | rmap entry |
> ----------------
>
> Thus the rmap pointer for each guest frame number in the memslot can be
> either NULL, a single entry, or a pointer to a list of nested rmap entries.
>
> gfn memslot rmap array
> -------------------------
> 0 | NULL | (no rmap entry)
> -------------------------
> 1 | single rmap entry | (rmap entry with low bit set)
> -------------------------
> 2 | list head pointer | (list of rmap entries)
> -------------------------
>
> The final entry always has the lowest bit set and is stored in the next
> pointer of the last list entry, or as a single rmap entry.
> With a list of rmap entries looking like;
>
> ----------------- ----------------- -------------------------
> | list head ptr | ----> | next pointer | ----> | single rmap entry |
> ----------------- ----------------- -------------------------
> | rmap entry | | rmap entry |
> ----------------- -------------------------
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> ---
> arch/powerpc/include/asm/kvm_book3s.h | 3 +
> arch/powerpc/include/asm/kvm_book3s_64.h | 70 ++++++++++++++++-
> arch/powerpc/kvm/book3s_64_mmu_radix.c | 44 +++++++----
> arch/powerpc/kvm/book3s_hv.c | 1 +
> arch/powerpc/kvm/book3s_hv_nested.c | 130 ++++++++++++++++++++++++++++++-
> 5 files changed, 233 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> index d983778..1d2286d 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -196,6 +196,9 @@ extern int kvmppc_mmu_radix_translate_table(struct kvm_vcpu *vcpu, gva_t eaddr,
> int table_index, u64 *pte_ret_p);
> extern int kvmppc_mmu_radix_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
> struct kvmppc_pte *gpte, bool data, bool iswrite);
> +extern void kvmppc_unmap_pte(struct kvm *kvm, pte_t *pte, unsigned long gpa,
> + unsigned int shift, struct kvm_memory_slot *memslot,
> + unsigned int lpid);
> extern bool kvmppc_hv_handle_set_rc(struct kvm *kvm, pgd_t *pgtable,
> bool writing, unsigned long gpa,
> unsigned int lpid);
> diff --git a/arch/powerpc/include/asm/kvm_book3s_64.h b/arch/powerpc/include/asm/kvm_book3s_64.h
> index 5496152..38614f0 100644
> --- a/arch/powerpc/include/asm/kvm_book3s_64.h
> +++ b/arch/powerpc/include/asm/kvm_book3s_64.h
> @@ -53,6 +53,66 @@ struct kvm_nested_guest {
> struct kvm_nested_guest *next;
> };
>
> +/*
> + * We define a nested rmap entry as a single 64-bit quantity
> + * 0xFFF0000000000000 12-bit lpid field
> + * 0x000FFFFFFFFFF000 40-bit guest physical address field
I thought we could potentially support guests with >1TiB of RAM..?
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 30/33] KVM: PPC: Book3S HV: Allow HV module to load without hypervisor mode
From: David Gibson @ 2018-10-03 6:15 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1538479892-14835-31-git-send-email-paulus@ozlabs.org>
[-- Attachment #1: Type: text/plain, Size: 2551 bytes --]
On Tue, Oct 02, 2018 at 09:31:29PM +1000, Paul Mackerras wrote:
> With this, the KVM-HV module can be loaded in a guest running under
> KVM-HV, and if the hypervisor supports nested virtualization, this
> guest can now act as a nested hypervisor and run nested guests.
>
> This also adds some checks to inform userspace that HPT guests are not
> supported by nested hypervisors, and to prevent userspace from
> configuring a guest to use HPT mode.
>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
> ---
> arch/powerpc/kvm/book3s_hv.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index f630e91..196bff1 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -4237,6 +4237,10 @@ static int kvm_vm_ioctl_get_smmu_info_hv(struct kvm *kvm,
> {
> struct kvm_ppc_one_seg_page_size *sps;
>
> + /* If we're a nested hypervisor, we only support radix guests */
> + if (kvmhv_on_pseries())
> + return -EINVAL;
> +
> /*
> * POWER7, POWER8 and POWER9 all support 32 storage keys for data.
> * POWER7 doesn't support keys for instruction accesses,
> @@ -4822,11 +4826,15 @@ static int kvmppc_core_emulate_mfspr_hv(struct kvm_vcpu *vcpu, int sprn,
>
> static int kvmppc_core_check_processor_compat_hv(void)
> {
> - if (!cpu_has_feature(CPU_FTR_HVMODE) ||
> - !cpu_has_feature(CPU_FTR_ARCH_206))
> - return -EIO;
> + if (cpu_has_feature(CPU_FTR_HVMODE) &&
> + cpu_has_feature(CPU_FTR_ARCH_206))
> + return 0;
>
> - return 0;
> + /* Can run as nested hypervisor on POWER9 in radix mode. */
> + if (cpu_has_feature(CPU_FTR_ARCH_300) && radix_enabled())
Shouldn't we probe the parent hypervisor for ability to support nested
guests before we say "yes" here?
> + return 0;
> +
> + return -EIO;
> }
>
> #ifdef CONFIG_KVM_XICS
> @@ -5144,6 +5152,10 @@ static int kvmhv_configure_mmu(struct kvm *kvm, struct kvm_ppc_mmuv3_cfg *cfg)
> if (radix && !radix_enabled())
> return -EINVAL;
>
> + /* If we're a nested hypervisor, we currently only support radix */
> + if (kvmhv_on_pseries() && !radix)
> + return -EINVAL;
> +
> mutex_lock(&kvm->lock);
> if (radix != kvm_is_radix(kvm)) {
> if (kvm->arch.mmu_ready) {
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 29/33] KVM: PPC: Book3S HV: Handle differing endianness for H_ENTER_NESTED
From: David Gibson @ 2018-10-03 6:13 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1538479892-14835-30-git-send-email-paulus@ozlabs.org>
[-- Attachment #1: Type: text/plain, Size: 4557 bytes --]
On Tue, Oct 02, 2018 at 09:31:28PM +1000, Paul Mackerras wrote:
> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>
> The hcall H_ENTER_NESTED takes as the two parameters the address in
> L1 guest memory of a hv_regs struct and a pt_regs struct which the
> L1 guest would like to use to run a L2 guest and in which are returned
> the exit state of the L2 guest. For efficiency, these are in the
> endianness of the L1 guest, rather than being always big-endian as is
> usually the case for PAPR hypercalls.
Does that actually make a difference for efficiency? I thought the
presence of the byte-reversing loads and stores meant byteswapping was
basically zero-cost on POWER.
> When reading/writing these structures, this patch handles the case
> where the endianness of the L1 guest differs from that of the L0
> hypervisor, by byteswapping the structures after reading and before
> writing them back.
>
> Since all the fields of the pt_regs are of the same type, i.e.,
> unsigned long, we treat it as an array of unsigned longs. The fields
> of struct hv_guest_state are not all the same, so its fields are
> byteswapped individually.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Looks correct, though, so
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> arch/powerpc/kvm/book3s_hv_nested.c | 51 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
> index 7b1088a..228dc11 100644
> --- a/arch/powerpc/kvm/book3s_hv_nested.c
> +++ b/arch/powerpc/kvm/book3s_hv_nested.c
> @@ -51,6 +51,48 @@ void kvmhv_save_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
> hr->ppr = vcpu->arch.ppr;
> }
>
> +static void byteswap_pt_regs(struct pt_regs *regs)
> +{
> + unsigned long *addr = (unsigned long *) regs;
> +
> + for (; addr < ((unsigned long *) (regs + 1)); addr++)
> + *addr = swab64(*addr);
> +}
> +
> +static void byteswap_hv_regs(struct hv_guest_state *hr)
> +{
> + hr->version = swab64(hr->version);
> + hr->lpid = swab32(hr->lpid);
> + hr->vcpu_token = swab32(hr->vcpu_token);
> + hr->lpcr = swab64(hr->lpcr);
> + hr->pcr = swab64(hr->pcr);
> + hr->amor = swab64(hr->amor);
> + hr->dpdes = swab64(hr->dpdes);
> + hr->hfscr = swab64(hr->hfscr);
> + hr->tb_offset = swab64(hr->tb_offset);
> + hr->dawr0 = swab64(hr->dawr0);
> + hr->dawrx0 = swab64(hr->dawrx0);
> + hr->ciabr = swab64(hr->ciabr);
> + hr->hdec_expiry = swab64(hr->hdec_expiry);
> + hr->purr = swab64(hr->purr);
> + hr->spurr = swab64(hr->spurr);
> + hr->ic = swab64(hr->ic);
> + hr->vtb = swab64(hr->vtb);
> + hr->hdar = swab64(hr->hdar);
> + hr->hdsisr = swab64(hr->hdsisr);
> + hr->heir = swab64(hr->heir);
> + hr->asdr = swab64(hr->asdr);
> + hr->srr0 = swab64(hr->srr0);
> + hr->srr1 = swab64(hr->srr1);
> + hr->sprg[0] = swab64(hr->sprg[0]);
> + hr->sprg[1] = swab64(hr->sprg[1]);
> + hr->sprg[2] = swab64(hr->sprg[2]);
> + hr->sprg[3] = swab64(hr->sprg[3]);
> + hr->pidr = swab64(hr->pidr);
> + hr->cfar = swab64(hr->cfar);
> + hr->ppr = swab64(hr->ppr);
> +}
> +
> static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
> struct hv_guest_state *hr)
> {
> @@ -175,6 +217,8 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
> sizeof(struct hv_guest_state));
> if (err)
> return H_PARAMETER;
> + if (kvmppc_need_byteswap(vcpu))
> + byteswap_hv_regs(&l2_hv);
> if (l2_hv.version != HV_GUEST_STATE_VERSION)
> return H_P2;
>
> @@ -183,7 +227,8 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
> sizeof(struct pt_regs));
> if (err)
> return H_PARAMETER;
> -
> + if (kvmppc_need_byteswap(vcpu))
> + byteswap_pt_regs(&l2_regs);
> if (l2_hv.vcpu_token >= NR_CPUS)
> return H_PARAMETER;
>
> @@ -255,6 +300,10 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
> kvmhv_put_nested(l2);
>
> /* copy l2_hv_state and regs back to guest */
> + if (kvmppc_need_byteswap(vcpu)) {
> + byteswap_hv_regs(&l2_hv);
> + byteswap_pt_regs(&l2_regs);
> + }
> err = kvm_vcpu_write_guest(vcpu, hv_ptr, &l2_hv,
> sizeof(struct hv_guest_state));
> if (err)
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 22/33] KVM: PPC: Book3S HV: Handle page fault for a nested guest
From: David Gibson @ 2018-10-03 5:42 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <20181003053913.GP1886@umbus.fritz.box>
[-- Attachment #1: Type: text/plain, Size: 3378 bytes --]
On Wed, Oct 03, 2018 at 03:39:13PM +1000, David Gibson wrote:
> On Tue, Oct 02, 2018 at 09:31:21PM +1000, Paul Mackerras wrote:
> > From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> >
> > Consider a normal (L1) guest running under the main hypervisor (L0),
> > and then a nested guest (L2) running under the L1 guest which is acting
> > as a nested hypervisor. L0 has page tables to map the address space for
> > L1 providing the translation from L1 real address -> L0 real address;
> >
> > L1
> > |
> > | (L1 -> L0)
> > |
> > ----> L0
> >
> > There are also page tables in L1 used to map the address space for L2
> > providing the translation from L2 real address -> L1 read address. Since
> > the hardware can only walk a single level of page table, we need to
> > maintain in L0 a "shadow_pgtable" for L2 which provides the translation
> > from L2 real address -> L0 real address. Which looks like;
> >
> > L2 L2
> > | |
> > | (L2 -> L1) |
> > | |
> > ----> L1 | (L2 -> L0)
> > | |
> > | (L1 -> L0) |
> > | |
> > ----> L0 --------> L0
> >
> > When a page fault occurs while running a nested (L2) guest we need to
> > insert a pte into this "shadow_pgtable" for the L2 -> L0 mapping. To
> > do this we need to:
> >
> > 1. Walk the pgtable in L1 memory to find the L2 -> L1 mapping, and
> > provide a page fault to L1 if this mapping doesn't exist.
> > 2. Use our L1 -> L0 pgtable to convert this L1 address to an L0 address,
> > or try to insert a pte for that mapping if it doesn't exist.
> > 3. Now we have a L2 -> L0 mapping, insert this into our shadow_pgtable
> >
> > Once this mapping exists we can take rc faults when hardware is unable
> > to automatically set the reference and change bits in the pte. On these
> > we need to:
> >
> > 1. Check the rc bits on the L2 -> L1 pte match, and otherwise reflect
> > the fault down to L1.
> > 2. Set the rc bits in the L1 -> L0 pte which corresponds to the same
> > host page.
> > 3. Set the rc bits in the L2 -> L0 pte.
> >
> > As we reuse a large number of functions in book3s_64_mmu_radix.c for
> > this we also needed to refactor a number of these functions to take
> > an lpid parameter so that the correct lpid is used for tlb invalidations.
> > The functionality however has remained the same.
> >
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
>
> Some comments below, but no showstoppers, so,
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
One more, again not a showstopper:
> > @@ -393,10 +396,20 @@ struct kvm_nested_guest *kvmhv_alloc_nested(struct kvm *kvm, unsigned int lpid)
> > */
> > static void kvmhv_release_nested(struct kvm_nested_guest *gp)
> > {
> > + struct kvm *kvm = gp->l1_host;
> > +
> > + if (gp->shadow_pgtable) {
> > + /*
> > + * No vcpu is using this struct and no call to
> > + * kvmhv_remove_nest_rmap can find this struct,
It's kind of dubious that you're referring to kvmhv_remove_nest_rmap()
a patch before it is introduced.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 24/33] KVM: PPC: Book3S HV: Emulate Privileged TLBIE for guest hypervisors
From: David Gibson @ 2018-10-03 5:56 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1538479892-14835-25-git-send-email-paulus@ozlabs.org>
[-- Attachment #1: Type: text/plain, Size: 11687 bytes --]
On Tue, Oct 02, 2018 at 09:31:23PM +1000, Paul Mackerras wrote:
> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
>
> When running a nested (L2) guest the guest (L1) hypervisor will use
> hypervisor privileged tlb invalidation instructions (to manage the
> partition scoped page tables) which will result in hypervisor
> emulation assistance interrupts. We emulate these instructions on behalf
> of the L1 guest.
>
> The tlbie instruction can invalidate different scopes:
>
> Invalidate TLB for a given target address:
> - This invalidates a single L2 -> L1 pte
> - We need to invalidate any L2 -> L0 shadow_pgtable ptes which map the L2
> address space which is being invalidated. This is because a single
> L2 -> L1 pte may have been mapped with more than one pte in the
> L2 -> L0 page tables.
>
> Invalidate the entire TLB for a given LPID or for all LPIDs:
> - Invalidate the entire shadow_pgtable for a given nested guest, or
> for all nested guests.
>
> Invalidate the PWC (page walk cache) for a given LPID or for all LPIDs:
> - We don't cache the PWC, so nothing to do
>
> Invalidate the entire TLB, PWC and partition table for a given/all LPIDs:
> - Here we re-read the partition table entry and remove the nested state
> for any nested guest for which the first doubleword of the partition
> table entry is now zero.
>
> This also implements the H_TLB_INVALIDATE hcall. It takes as parameters
> the tlbie instruction word (of which the RIC, PRS and R fields are used),
> the rS value (giving the lpid, where required) and the rB value (giving
> the IS, AP and EPN values).
>
> [paulus@ozlabs.org - adapted to having the partition table in guest
> memory, added the H_TLB_INVALIDATE implementation.]
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
Again, do we need this if we're moving to a paravirt tlbie?
> ---
> arch/powerpc/include/asm/book3s/64/mmu-hash.h | 12 ++
> arch/powerpc/include/asm/kvm_book3s.h | 1 +
> arch/powerpc/include/asm/ppc-opcode.h | 1 +
> arch/powerpc/kvm/book3s_emulate.c | 1 -
> arch/powerpc/kvm/book3s_hv.c | 3 +
> arch/powerpc/kvm/book3s_hv_nested.c | 210 +++++++++++++++++++++++++-
> 6 files changed, 225 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> index b3520b5..66db23e 100644
> --- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> +++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
> @@ -203,6 +203,18 @@ static inline unsigned int mmu_psize_to_shift(unsigned int mmu_psize)
> BUG();
> }
>
> +static inline unsigned int ap_to_shift(unsigned long ap)
> +{
> + int psize;
> +
> + for (psize = 0; psize < MMU_PAGE_COUNT; psize++) {
> + if (mmu_psize_defs[psize].ap == ap)
> + return mmu_psize_defs[psize].shift;
> + }
> +
> + return -1;
> +}
> +
> static inline unsigned long get_sllp_encoding(int psize)
> {
> unsigned long sllp;
> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> index 1d2286d..210e550 100644
> --- a/arch/powerpc/include/asm/kvm_book3s.h
> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> @@ -301,6 +301,7 @@ long kvmhv_set_partition_table(struct kvm_vcpu *vcpu);
> void kvmhv_set_ptbl_entry(unsigned int lpid, u64 dw0, u64 dw1);
> void kvmhv_release_all_nested(struct kvm *kvm);
> long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu);
> +long kvmhv_do_nested_tlbie(struct kvm_vcpu *vcpu);
> int kvmhv_run_single_vcpu(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu,
> u64 time_limit, unsigned long lpcr);
> void kvmhv_save_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr);
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> index 665af14..6093bc8 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -104,6 +104,7 @@
> #define OP_31_XOP_LHZUX 311
> #define OP_31_XOP_MSGSNDP 142
> #define OP_31_XOP_MSGCLRP 174
> +#define OP_31_XOP_TLBIE 306
> #define OP_31_XOP_MFSPR 339
> #define OP_31_XOP_LWAX 341
> #define OP_31_XOP_LHAX 343
> diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
> index 2654df2..8c7e933 100644
> --- a/arch/powerpc/kvm/book3s_emulate.c
> +++ b/arch/powerpc/kvm/book3s_emulate.c
> @@ -36,7 +36,6 @@
> #define OP_31_XOP_MTSR 210
> #define OP_31_XOP_MTSRIN 242
> #define OP_31_XOP_TLBIEL 274
> -#define OP_31_XOP_TLBIE 306
> /* Opcode is officially reserved, reuse it as sc 1 when sc 1 doesn't trap */
> #define OP_31_XOP_FAKE_SC1 308
> #define OP_31_XOP_SLBMTE 402
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 6629df4..3aa5d11e 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -974,6 +974,9 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
> break;
> case H_TLB_INVALIDATE:
> ret = H_FUNCTION;
> + if (!vcpu->kvm->arch.nested_enable)
> + break;
> + ret = kvmhv_do_nested_tlbie(vcpu);
> break;
>
> default:
> diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
> index 4efa1f7..7abcc2b 100644
> --- a/arch/powerpc/kvm/book3s_hv_nested.c
> +++ b/arch/powerpc/kvm/book3s_hv_nested.c
> @@ -465,7 +465,7 @@ void kvmhv_release_all_nested(struct kvm *kvm)
> }
>
> /* caller must hold gp->tlb_lock */
> -void kvmhv_flush_nested(struct kvm_nested_guest *gp)
> +static void kvmhv_flush_nested(struct kvm_nested_guest *gp)
> {
> struct kvm *kvm = gp->l1_host;
>
> @@ -685,10 +685,216 @@ static int kvmhv_emulate_priv_mfspr(struct kvm_run *run, struct kvm_vcpu *vcpu,
> return EMULATE_FAIL;
> }
>
> +static inline int get_ric(unsigned int instr)
> +{
> + return (instr >> 18) & 0x3;
> +}
> +
> +static inline int get_prs(unsigned int instr)
> +{
> + return (instr >> 17) & 0x1;
> +}
> +
> +static inline int get_r(unsigned int instr)
> +{
> + return (instr >> 16) & 0x1;
> +}
> +
> +static inline int get_lpid(unsigned long r_val)
> +{
> + return r_val & 0xffffffff;
> +}
> +
> +static inline int get_is(unsigned long r_val)
> +{
> + return (r_val >> 10) & 0x3;
> +}
> +
> +static inline int get_ap(unsigned long r_val)
> +{
> + return (r_val >> 5) & 0x7;
> +}
> +
> +static inline long get_epn(unsigned long r_val)
> +{
> + return r_val >> 12;
> +}
> +
> +static int kvmhv_emulate_tlbie_tlb_addr(struct kvm_vcpu *vcpu, int lpid,
> + int ap, long epn)
> +{
> + struct kvm *kvm = vcpu->kvm;
> + struct kvm_nested_guest *gp;
> + long npages;
> + int shift;
> + unsigned long addr;
> +
> + shift = ap_to_shift(ap);
> + addr = epn << 12;
> + if (shift < 0)
> + /* Invalid ap encoding */
> + return -EINVAL;
> +
> + addr &= ~((1UL << shift) - 1);
> + npages = 1UL << (shift - PAGE_SHIFT);
> +
> + gp = kvmhv_get_nested(kvm, lpid, false);
> + if (!gp) /* No such guest -> nothing to do */
> + return 0;
> + mutex_lock(&gp->tlb_lock);
> +
> + /* There may be more than one host page backing this single guest pte */
> + do {
> + kvmhv_invalidate_shadow_pte(vcpu, gp, addr, &shift);
> +
> + npages -= 1UL << (shift - PAGE_SHIFT);
> + addr += 1UL << shift;
> + } while (npages > 0);
> +
> + mutex_unlock(&gp->tlb_lock);
> + kvmhv_put_nested(gp);
> + return 0;
> +}
> +
> +static void kvmhv_emulate_tlbie_lpid(struct kvm_vcpu *vcpu,
> + struct kvm_nested_guest *gp, int ric)
> +{
> + struct kvm *kvm = vcpu->kvm;
> +
> + mutex_lock(&gp->tlb_lock);
> + switch (ric) {
> + case 0:
> + /* Invalidate TLB */
> + spin_lock(&kvm->mmu_lock);
> + kvmppc_free_pgtable_radix(kvm, gp->shadow_pgtable,
> + gp->shadow_lpid);
> + radix__flush_tlb_lpid(gp->shadow_lpid);
> + spin_unlock(&kvm->mmu_lock);
> + break;
> + case 1:
> + /*
> + * Invalidate PWC
> + * We don't cache this -> nothing to do
> + */
> + break;
> + case 2:
> + /* Invalidate TLB, PWC and caching of partition table entries */
> + kvmhv_flush_nested(gp);
> + break;
> + default:
> + break;
> + }
> + mutex_unlock(&gp->tlb_lock);
> +}
> +
> +static void kvmhv_emulate_tlbie_all_lpid(struct kvm_vcpu *vcpu, int ric)
> +{
> + struct kvm *kvm = vcpu->kvm;
> + struct kvm_nested_guest *gp;
> + int i;
> +
> + spin_lock(&kvm->mmu_lock);
> + for (i = 0; i <= kvm->arch.max_nested_lpid; i++) {
> + gp = kvm->arch.nested_guests[i];
> + if (gp) {
> + spin_unlock(&kvm->mmu_lock);
> + kvmhv_emulate_tlbie_lpid(vcpu, gp, ric);
> + spin_lock(&kvm->mmu_lock);
> + }
> + }
> + spin_unlock(&kvm->mmu_lock);
> +}
> +
> +static int kvmhv_emulate_priv_tlbie(struct kvm_vcpu *vcpu, unsigned int instr,
> + int rs, int rb)
> +{
> + struct kvm *kvm = vcpu->kvm;
> + struct kvm_nested_guest *gp;
> + int r, ric, prs, is, ap;
> + int lpid;
> + long epn;
> + int ret = 0;
> +
> + ric = get_ric(instr);
> + prs = get_prs(instr);
> + r = get_r(instr);
> + lpid = get_lpid(kvmppc_get_gpr(vcpu, rs));
> + is = get_is(kvmppc_get_gpr(vcpu, rb));
> +
> + /*
> + * These cases are invalid and __should__ have caused a machine check
> + * r != 1 -> Only radix supported
> + * prs == 1 -> Not HV privileged
> + * ric == 3 -> No clusted bombs for radix
> + * is == 1 -> Partition scoped translations not associated with pid
> + * (!is) && (ric == 1 || ric == 2) -> Not supported by ISA
> + */
> + if ((!r) || (prs) || (ric == 3) || (is == 1) ||
> + ((!is) && (ric == 1 || ric == 2)))
> + return -EINVAL;
> +
> + switch (is) {
> + case 0:
> + /*
> + * We know ric == 0
> + * Invalidate TLB for a given target address
> + */
> + epn = get_epn(kvmppc_get_gpr(vcpu, rb));
> + ap = get_ap(kvmppc_get_gpr(vcpu, rb));
> + ret = kvmhv_emulate_tlbie_tlb_addr(vcpu, lpid, ap, epn);
> + break;
> + case 2:
> + /* Invalidate matching LPID */
> + gp = kvmhv_get_nested(kvm, lpid, false);
> + if (gp) {
> + kvmhv_emulate_tlbie_lpid(vcpu, gp, ric);
> + kvmhv_put_nested(gp);
> + }
> + break;
> + case 3:
> + /* Invalidate ALL LPIDs */
> + kvmhv_emulate_tlbie_all_lpid(vcpu, ric);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> static int kvmhv_emulate_priv_op_31(struct kvm_run *run, struct kvm_vcpu *vcpu,
> unsigned int instr)
> {
> - return EMULATE_FAIL;
> + int ret;
> +
> + switch (get_xop(instr)) {
> + case OP_31_XOP_TLBIE:
> + ret = kvmhv_emulate_priv_tlbie(vcpu, instr, get_rs(instr),
> + get_rb(instr));
> + if (ret) {
> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> + return EMULATE_AGAIN;
> + }
> + return EMULATE_DONE;
> + default:
> + return EMULATE_FAIL;
> + }
> +}
> +
> +/*
> + * This handles the H_TLB_INVALIDATE hcall.
> + * Parameters are (r4) tlbie instruction code, (r5) rS contents,
> + * (r6) rB contents.
> + */
> +long kvmhv_do_nested_tlbie(struct kvm_vcpu *vcpu)
> +{
> + int ret;
> +
> + ret = kvmhv_emulate_priv_tlbie(vcpu, kvmppc_get_gpr(vcpu, 4), 5, 6);
> + if (ret)
> + return H_PARAMETER;
> + return H_SUCCESS;
> }
>
> static int kvmhv_emulate_priv_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc: use PTRRELOC during early init
From: Christophe LEROY @ 2018-10-03 6:22 UTC (permalink / raw)
To: Andreas Schwab, Michael Neuling
Cc: Michal Suchánek, linuxppc-dev, Haren Myneni, Nicholas Piggin
In-Reply-To: <87wor09dww.fsf_-_@igel.home>
Hi Andreas,
Le 03/10/2018 à 00:33, Andreas Schwab a écrit :
> This fixes a crash on powerpc32 when using global data during early init
> without relocating its address.
>
> Fixes: 51c3c62b58 (powerpc: Avoid code patching freed init sections)
> Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
> ---
> arch/powerpc/lib/code-patching.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 6ae2777c22..6192fdae36 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -29,7 +29,7 @@ static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
> int err;
>
> /* Make sure we aren't patching a freed init section */
> - if (init_mem_is_free && init_section_contains(exec_addr, 4)) {
> + if (*PTRRELOC(&init_mem_is_free) && init_section_contains(exec_addr, 4)) {
That's not the best solution. In the past we already did our best to
separate early use of patch_instruction() , that's how
raw_patch_instruction() was born, see
https://patchwork.ozlabs.org/patch/840974/
Here, it idea is similar, this test should not apply to
raw_patch_instruction()
Did you try my proposed fix https://patchwork.ozlabs.org/patch/977195/ ?
Christophe
> pr_debug("Skipping init section patching addr: 0x%px\n", exec_addr);
> return 0;
> }
>
^ permalink raw reply
* Re: [RFC PATCH v3 3/7] powerpc: Activate CONFIG_THREAD_INFO_IN_TASK
From: Nicholas Piggin @ 2018-10-03 6:23 UTC (permalink / raw)
To: Christophe LEROY; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <210d5ce2-1f62-2458-617b-fe604b95919e@c-s.fr>
On Wed, 3 Oct 2018 08:04:49 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> Le 03/10/2018 à 07:52, Nicholas Piggin a écrit :
> > On Wed, 3 Oct 2018 07:47:05 +0200
> > Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> >
> >> Le 03/10/2018 à 07:30, Nicholas Piggin a écrit :
> >>> On Mon, 1 Oct 2018 12:30:23 +0000 (UTC)
> >>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >>>
> >>>> This patch activates CONFIG_THREAD_INFO_IN_TASK which
> >>>> moves the thread_info into task_struct.
> >>>>
> >>>> Moving thread_info into task_struct has the following advantages:
> >>>> - It protects thread_info from corruption in the case of stack
> >>>> overflows.
> >>>> - Its address is harder to determine if stack addresses are
> >>>> leaked, making a number of attacks more difficult.
> >>>>
> >>>> This has the following consequences:
> >>>> - thread_info is now located at the top of task_struct.
> >>>
> >>> "top"... I got confused for a minute thinking high address and
> >>> wondering how you can change CURRENT_THREAD_INFO just to point
> >>> to current :)
> >>
> >> Would 'beginning' be less confusing ?
> >
> > Yes, good idea.
> >
> >>>> @@ -83,7 +83,13 @@ int is_cpu_dead(unsigned int cpu);
> >>>> /* 32-bit */
> >>>> extern int smp_hw_index[];
> >>>>
> >>>> -#define raw_smp_processor_id() (current_thread_info()->cpu)
> >>>> +/*
> >>>> + * This is particularly ugly: it appears we can't actually get the definition
> >>>> + * of task_struct here, but we need access to the CPU this task is running on.
> >>>> + * Instead of using task_struct we're using _TASK_CPU which is extracted from
> >>>> + * asm-offsets.h by kbuild to get the current processor ID.
> >>>> + */
> >>>> +#define raw_smp_processor_id() (*(unsigned int*)((void*)current + _TASK_CPU))
> >>>
> >>> This is clever but yes ugly. Can't you include asm-offsets.h? riscv
> >>> seems to.
> >>
> >> riscv has a clean asm-offsets.h . Our's defines constant with the same
> >> name as those defined in other headers which are included in C files. So
> >> including asm-offsets in C files does create conflicts like:
> >>
> >> ./include/generated/asm-offsets.h:71:0: warning: "TASK_SIZE" redefined
> >> #define TASK_SIZE -2147483648 /* TASK_SIZE */
> >> ./arch/powerpc/include/asm/processor.h:95:0: note: this is the location
> >> of the previous definition
> >> #define TASK_SIZE (CONFIG_TASK_SIZE)
> >>
> >> ./include/generated/asm-offsets.h:98:0: warning: "NSEC_PER_SEC" redefined
> >> #define NSEC_PER_SEC 1000000000 /* NSEC_PER_SEC */
> >> ./include/linux/time64.h:36:0: note: this is the location of the
> >> previous definition
> >> #define NSEC_PER_SEC 1000000000L
> >>
> >> ./arch/powerpc/include/asm/nohash/32/pgtable.h:34:0: warning:
> >> "PGD_TABLE_SIZE" redefined
> >> #define PGD_TABLE_SIZE (sizeof(pgd_t) << PGD_INDEX_SIZE)
> >> ./include/generated/asm-offsets.h:101:0: note: this is the location of
> >> the previous definition
> >> #define PGD_TABLE_SIZE 256 /* PGD_TABLE_SIZE */
> >>
> >> ...
> >
> > Okay.
> >
> >>
> >> In v2, I had a patch to fix those redundancies
> >> (https://patchwork.ozlabs.org/patch/974363/) but I found it unconvenient.
> >
> > Because of merge conflicts, or you did not like the new names?
>
> Both, because of the amount of changes it implies, and also because of
> the new names. I find it quite convenient to be able to use same names
> both in C and ASM.
Yeah that's true. I guess this is okay for a one-off hack.
Thanks,
Nick
^ permalink raw reply
* Re: [RFC PATCH v3 4/7] powerpc: regain entire stack space
From: Nicholas Piggin @ 2018-10-03 6:30 UTC (permalink / raw)
To: Christophe LEROY; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <6a539614-abb6-d468-ccba-2e3c1fb8b680@c-s.fr>
On Wed, 3 Oct 2018 07:52:59 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> Le 03/10/2018 à 07:34, Nicholas Piggin a écrit :
> > On Mon, 1 Oct 2018 12:30:25 +0000 (UTC)
> > Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >
> >> thread_info is not anymore in the stack, so the entire stack
> >> can now be used.
> >
> > Nice.
> >
> >>
> >> In the meantime, all pointers to the stacks are not anymore
> >> pointers to thread_info so this patch changes them to void*
> >
> > Wasn't this previously effectively already the case with patch
> > 3/7? You had thread_info sized space left there, but it was not
> > used or initialized right? Does it make sense to move this part
> > of it to the previous patch?
>
> Not really.
>
> In 3/7 I changed the prototypes of two functions that really used the
> pointer as a task pointer only.
>
> Here it change things that before 4/7 were really used as both stack
> pointers and thread_info pointers.
What uses it as a thread_info pointer? It seems more like a stack
with some amount of unused space in it but that's all.
That said I don't care to nitpick too much where things go exactly
if you like it better here that's fine.
Thanks,
Nick
^ permalink raw reply
* Re: [RFC PATCH v3 4/7] powerpc: regain entire stack space
From: Christophe LEROY @ 2018-10-03 6:45 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <20181003163024.281254bf@roar.ozlabs.ibm.com>
Le 03/10/2018 à 08:30, Nicholas Piggin a écrit :
> On Wed, 3 Oct 2018 07:52:59 +0200
> Christophe LEROY <christophe.leroy@c-s.fr> wrote:
>
>> Le 03/10/2018 à 07:34, Nicholas Piggin a écrit :
>>> On Mon, 1 Oct 2018 12:30:25 +0000 (UTC)
>>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>>
>>>> thread_info is not anymore in the stack, so the entire stack
>>>> can now be used.
>>>
>>> Nice.
>>>
>>>>
>>>> In the meantime, all pointers to the stacks are not anymore
>>>> pointers to thread_info so this patch changes them to void*
>>>
>>> Wasn't this previously effectively already the case with patch
>>> 3/7? You had thread_info sized space left there, but it was not
>>> used or initialized right? Does it make sense to move this part
>>> of it to the previous patch?
>>
>> Not really.
>>
>> In 3/7 I changed the prototypes of two functions that really used the
>> pointer as a task pointer only.
I meant 2/7 here sorry.
>>
>> Here it change things that before 4/7 were really used as both stack
>> pointers and thread_info pointers.
And here I meant 3/7
>
> What uses it as a thread_info pointer? It seems more like a stack
> with some amount of unused space in it but that's all.
Before 3/7, we have
void do_softirq_own_stack(void)
{
struct thread_info *curtp, *irqtp;
curtp = current_thread_info();
irqtp = softirq_ctx[smp_processor_id()];
irqtp->task = curtp->task;
irqtp->flags = 0;
call_do_softirq(irqtp);
irqtp->task = NULL;
/* Set any flag that may have been set on the
* alternate stack
*/
if (irqtp->flags)
set_bits(irqtp->flags, &curtp->flags);
}
After 3/7, we have
void do_softirq_own_stack(void)
{
struct thread_info *irqtp;
irqtp = softirq_ctx[smp_processor_id()];
call_do_softirq(irqtp);
}
So now only we can change irqtp to void* can't we ?
>
> That said I don't care to nitpick too much where things go exactly
> if you like it better here that's fine.
No worry, I may have missed something, your comments are always welcome.
Thanks
Christophe
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [RFC PATCH v3 4/7] powerpc: regain entire stack space
From: Nicholas Piggin @ 2018-10-03 7:07 UTC (permalink / raw)
To: Christophe LEROY; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <38d58749-c058-a6d7-1ca2-23abe9dd34a3@c-s.fr>
On Wed, 3 Oct 2018 08:45:25 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> Le 03/10/2018 à 08:30, Nicholas Piggin a écrit :
> > On Wed, 3 Oct 2018 07:52:59 +0200
> > Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> >
> >> Le 03/10/2018 à 07:34, Nicholas Piggin a écrit :
> >>> On Mon, 1 Oct 2018 12:30:25 +0000 (UTC)
> >>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >>>
> >>>> thread_info is not anymore in the stack, so the entire stack
> >>>> can now be used.
> >>>
> >>> Nice.
> >>>
> >>>>
> >>>> In the meantime, all pointers to the stacks are not anymore
> >>>> pointers to thread_info so this patch changes them to void*
> >>>
> >>> Wasn't this previously effectively already the case with patch
> >>> 3/7? You had thread_info sized space left there, but it was not
> >>> used or initialized right? Does it make sense to move this part
> >>> of it to the previous patch?
> >>
> >> Not really.
> >>
> >> In 3/7 I changed the prototypes of two functions that really used the
> >> pointer as a task pointer only.
>
> I meant 2/7 here sorry.
>
> >>
> >> Here it change things that before 4/7 were really used as both stack
> >> pointers and thread_info pointers.
>
> And here I meant 3/7
>
> >
> > What uses it as a thread_info pointer? It seems more like a stack
> > with some amount of unused space in it but that's all.
>
> Before 3/7, we have
>
> void do_softirq_own_stack(void)
> {
> struct thread_info *curtp, *irqtp;
>
> curtp = current_thread_info();
> irqtp = softirq_ctx[smp_processor_id()];
> irqtp->task = curtp->task;
> irqtp->flags = 0;
> call_do_softirq(irqtp);
> irqtp->task = NULL;
>
> /* Set any flag that may have been set on the
> * alternate stack
> */
> if (irqtp->flags)
> set_bits(irqtp->flags, &curtp->flags);
> }
>
> After 3/7, we have
>
> void do_softirq_own_stack(void)
> {
> struct thread_info *irqtp;
>
> irqtp = softirq_ctx[smp_processor_id()];
> call_do_softirq(irqtp);
> }
>
>
> So now only we can change irqtp to void* can't we ?
In patch 3 we can, right? That's what I mean by moving from
thread_info * to void * in patch 3 rather than 4.
But if you prefer not to, it's fine. Maybe it keeps patch 3
a little smaller.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] migration/mm: Add WARN_ON to try_offline_node
From: Michal Hocko @ 2018-10-03 7:03 UTC (permalink / raw)
To: Tyrel Datwyler
Cc: Thomas Falcon, Kees Cook, Mathieu Malaterre, linux-kernel,
Nicholas Piggin, Pavel Tatashin, linux-mm, Michael Bringmann,
Mauricio Faria de Oliveira, Juliet Kim, Thiago Jung Bauermann,
Nathan Fontenot, Andrew Morton, YASUAKI ISHIMATSU, linuxppc-dev,
Dan Williams, Oscar Salvador
In-Reply-To: <bbc5f219-614f-b024-0888-8ad216c5eaf8@linux.vnet.ibm.com>
On Tue 02-10-18 12:45:50, Tyrel Datwyler wrote:
> On 10/02/2018 11:13 AM, Michael Bringmann wrote:
> >
> >
> > On 10/02/2018 11:04 AM, Michal Hocko wrote:
> >> On Tue 02-10-18 10:14:49, Michael Bringmann wrote:
> >>> On 10/02/2018 09:59 AM, Michal Hocko wrote:
> >>>> On Tue 02-10-18 09:51:40, Michael Bringmann wrote:
> >>>> [...]
> >>>>> When the device-tree affinity attributes have changed for memory,
> >>>>> the 'nid' affinity calculated points to a different node for the
> >>>>> memory block than the one used to install it, previously on the
> >>>>> source system. The newly calculated 'nid' affinity may not yet
> >>>>> be initialized on the target system. The current memory tracking
> >>>>> mechanisms do not record the node to which a memory block was
> >>>>> associated when it was added. Nathan is looking at adding this
> >>>>> feature to the new implementation of LMBs, but it is not there
> >>>>> yet, and won't be present in earlier kernels without backporting a
> >>>>> significant number of changes.
> >>>>
> >>>> Then the patch you have proposed here just papers over a real issue, no?
> >>>> IIUC then you simply do not remove the memory if you lose the race.
> >>>
> >>> The problem occurs when removing memory after an affinity change
> >>> references a node that was previously unreferenced. Other code
> >>> in 'kernel/mm/memory_hotplug.c' deals with initializing an empty
> >>> node when adding memory to a system. The 'removing memory' case is
> >>> specific to systems that perform LPM and allow device-tree changes.
> >>> The powerpc kernel does not have the option of accepting some PRRN
> >>> requests and accepting others. It must perform them all.
> >>
> >> I am sorry, but you are still too cryptic for me. Either there is a
> >> correctness issue and the the patch doesn't really fix anything or the
> >> final race doesn't make any difference and then the ppc code should be
> >> explicit about that. Checking the node inside the hotplug core code just
> >> looks as a wrong layer to mitigate an arch specific problem. I am not
> >> saying the patch is a no-go but if anything we want a big fat comment
> >> explaining how this is possible because right now it just points to an
> >> incorrect API usage.
> >>
> >> That being said, this sounds pretty much ppc specific problem and I
> >> would _prefer_ it to be handled there (along with a big fat comment of
> >> course).
> >
> > Let me try again. Regardless of the path to which we get to this condition,
> > we currently crash the kernel. This patch changes that to a WARN_ON notice
> > and continues executing the kernel without shutting down the system. I saw
> > the problem during powerpc testing, because that is the focus of my work.
> > There are other paths to this function besides powerpc. I feel that the
> > kernel should keep running instead of halting.
>
> This is still basically a hack to get around a known race. In itself
> this patch is still worth while in that we shouldn't crash the kernel
> on a null pointer dereference. However, I think the actual problem
> still needs to be addressed. We shouldn't run any PRRN events for the
> source system on the target after a migration. The device tree update
> should have taken care of telling us about new affinities and what
> not. Can we just throw out any queued PRRN events when we wake up on
> the target?
And until a proper fix is developed can we have NODE_DATA test in the
affected code rather than pollute the generic code with something that
is essentially a wrong usage of the API? With a big fat warning
explaining what is going on here?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH kernel] cxl: Remove unused include
From: Alexey Kardashevskiy @ 2018-10-03 7:46 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: Andrew Donnellan, Frederic Barrat
In-Reply-To: <875zypu6vg.fsf@concordia.ellerman.id.au>
On 28/09/2018 22:46, Michael Ellerman wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>> The included opal.h gives a wrong idea that CXL makes PPC OPAL calls
>> while it does not so let's remote it.
>
> But it does use eg.
>
> OPAL_PHB_CAPI_MODE_SNOOP_ON
> OPAL_PHB_CAPI_MODE_CAPI
>
> Which come from opal-api.h via opal.h.
>
> So you should at least include opal-api.h.
I'd say that since it includes pnv-pci.h (and this is why this patch
compiles), it should be a different set of values for powernv (which
would map 1:1 to OPAL_PHB_xxx). The powernv platform knowledge is
already intimate enough for a driver to have. Dunno, I found this opal.h
inclusion confusing, that's all.
>
> cheers
>
>> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
>> index b66d832..8cbcbb7 100644
>> --- a/drivers/misc/cxl/pci.c
>> +++ b/drivers/misc/cxl/pci.c
>> @@ -17,7 +17,6 @@
>> #include <linux/pci.h>
>> #include <linux/of.h>
>> #include <linux/delay.h>
>> -#include <asm/opal.h>
>> #include <asm/msi_bitmap.h>
>> #include <asm/pnv-pci.h>
>> #include <asm/io.h>
>> --
>> 2.11.0
--
Alexey
^ permalink raw reply
* Re: [PATCH kernel] cxl: Remove unused include
From: Frederic Barrat @ 2018-10-03 8:29 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <548a710f-b542-b361-b056-74a8d58c5ceb@ozlabs.ru>
Le 03/10/2018 à 09:46, Alexey Kardashevskiy a écrit :
>
>
> On 28/09/2018 22:46, Michael Ellerman wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>> The included opal.h gives a wrong idea that CXL makes PPC OPAL calls
>>> while it does not so let's remote it.
>>
>> But it does use eg.
>>
>> OPAL_PHB_CAPI_MODE_SNOOP_ON
>> OPAL_PHB_CAPI_MODE_CAPI
>>
>> Which come from opal-api.h via opal.h.
>>
>> So you should at least include opal-api.h.
>
>
> I'd say that since it includes pnv-pci.h (and this is why this patch
> compiles), it should be a different set of values for powernv (which
> would map 1:1 to OPAL_PHB_xxx). The powernv platform knowledge is
> already intimate enough for a driver to have. Dunno, I found this opal.h
> inclusion confusing, that's all.
I agree, it would be the cleaner way of doing it. It's going to be
pretty low on my list though...
Fred
>
>
>>
>> cheers
>>
>>> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
>>> index b66d832..8cbcbb7 100644
>>> --- a/drivers/misc/cxl/pci.c
>>> +++ b/drivers/misc/cxl/pci.c
>>> @@ -17,7 +17,6 @@
>>> #include <linux/pci.h>
>>> #include <linux/of.h>
>>> #include <linux/delay.h>
>>> -#include <asm/opal.h>
>>> #include <asm/msi_bitmap.h>
>>> #include <asm/pnv-pci.h>
>>> #include <asm/io.h>
>>> --
>>> 2.11.0
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox