* Re: [PATCH printk v2 2/5] printk: remove safe buffers
From: Sergey Senozhatsky @ 2021-04-02 2:14 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Peter Zijlstra, Paul Mackerras, Tiezhu Yang,
Rafael Aquini, Paul E. McKenney, Aneesh Kumar K.V,
Alexey Kardashevskiy, Yue Hu, Jordan Niethe, Kees Cook,
John Ogness, Alistair Popple, Guilherme G. Piccoli,
Nicholas Piggin, Steven Rostedt, Thomas Gleixner, kexec,
linux-kernel, Sergey Senozhatsky, Eric Biederman, Andrew Morton,
linuxppc-dev
In-Reply-To: <YGXV8LJarjUJDhvy@alley>
On (21/04/01 16:17), Petr Mladek wrote:
> > For the long term, we should introduce a printk-context API that allows
> > callers to perfectly pack their multi-line output into a single
> > entry. We discussed [0][1] this back in August 2020.
>
> We need a "short" term solution. There are currently 3 solutions:
>
> 1. Keep nmi_safe() and all the hacks around.
>
> 2. Serialize nmi_cpu_backtrace() by a spin lock and later by
> the special lock used also by atomic consoles.
>
> 3. Tell complaining people how to sort the messed logs.
Are we talking about nmi_cpu_backtrace()->dump_stack() or some
other path?
dump_stack() seems to be already serialized by `dump_lock`. Hmm,
show_regs() is not serialized, seems like it should be under the
same `dump_lock` as dump_stack().
^ permalink raw reply
* [PATCH] powerpc/powernv: Enable HAIL (HV AIL) for ISA v3.1 processors
From: Nicholas Piggin @ 2021-04-02 2:41 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin
Starting with ISA v3.1, LPCR[AIL] no longer controls the interrupt
mode for HV=1 interrupts. Instead, a new LPCR[HAIL] bit is defined
which behaves like AIL=3 for HV interrupts when set.
Set HAIL on bare metal to give us mmu-on interrupts and improve
performance.
This also fixes an scv bug: we don't implement scv real mode (AIL=0)
vectors because they are at an inconvenient location, so we just
disable scv support when AIL can not be set. However powernv assumes
that LPCR[AIL] will enable AIL mode so it enables scv support despite
HV interrupts being AIL=0, which causes scv interrupts to go off into
the weeds.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/reg.h | 1 +
arch/powerpc/kernel/setup_64.c | 19 ++++++++++++++++---
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 1be20bc8dce2..9086a2644c89 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -441,6 +441,7 @@
#define LPCR_VRMA_LP1 ASM_CONST(0x0000800000000000)
#define LPCR_RMLS 0x1C000000 /* Implementation dependent RMO limit sel */
#define LPCR_RMLS_SH 26
+#define LPCR_HAIL ASM_CONST(0x0000000004000000) /* HV AIL (ISAv3.1) */
#define LPCR_ILE ASM_CONST(0x0000000002000000) /* !HV irqs set MSR:LE */
#define LPCR_AIL ASM_CONST(0x0000000001800000) /* Alternate interrupt location */
#define LPCR_AIL_0 ASM_CONST(0x0000000000000000) /* MMU off exception offset 0x0 */
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 04a31586f760..671192afcdfd 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -233,10 +233,23 @@ static void cpu_ready_for_interrupts(void)
* If we are not in hypervisor mode the job is done once for
* the whole partition in configure_exceptions().
*/
- if (cpu_has_feature(CPU_FTR_HVMODE) &&
- cpu_has_feature(CPU_FTR_ARCH_207S)) {
+ if (cpu_has_feature(CPU_FTR_HVMODE)) {
unsigned long lpcr = mfspr(SPRN_LPCR);
- mtspr(SPRN_LPCR, lpcr | LPCR_AIL_3);
+ unsigned long new_lpcr = lpcr;
+
+ if (cpu_has_feature(CPU_FTR_ARCH_31)) {
+ /* P10 DD1 does not have HAIL */
+ if (pvr_version_is(PVR_POWER10) &&
+ (mfspr(SPRN_PVR) & 0xf00) == 0x100)
+ new_lpcr |= LPCR_AIL_3;
+ else
+ new_lpcr |= LPCR_HAIL;
+ } else if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
+ new_lpcr |= LPCR_AIL_3;
+ }
+
+ if (new_lpcr != lpcr)
+ mtspr(SPRN_LPCR, new_lpcr);
}
/*
--
2.23.0
^ permalink raw reply related
* Re: [PATCH 1/1] powerpc/smp: Set numa node before updating mask
From: Srikar Dronamraju @ 2021-04-02 3:18 UTC (permalink / raw)
To: Nathan Lynch
Cc: Gautham R Shenoy, Peter Zijlstra, Scott Cheloha,
Geetika Moolchandani, Valentin Schneider, Laurent Dufour,
linuxppc-dev, Ingo Molnar
In-Reply-To: <87czvdbova.fsf@linux.ibm.com>
* Nathan Lynch <nathanl@linux.ibm.com> [2021-04-01 17:51:05]:
Thanks Nathan for reviewing.
> > - set_numa_node(numa_cpu_lookup_table[cpu]);
> > - set_numa_mem(local_memory_node(numa_cpu_lookup_table[cpu]));
> > -
>
> Regardless of your change: at boot time, this set of calls to
> set_numa_node() and set_numa_mem() is redundant, right? Because
> smp_prepare_cpus() has:
>
> for_each_possible_cpu(cpu) {
> ...
> if (cpu_present(cpu)) {
> set_cpu_numa_node(cpu, numa_cpu_lookup_table[cpu]);
> set_cpu_numa_mem(cpu,
> local_memory_node(numa_cpu_lookup_table[cpu]));
> }
>
> I would rather that, when onlining a CPU that happens to have been
> dynamically added after boot, we enter start_secondary() with conditions
> equivalent to those at boot time. Or as close to that as is practical.
>
> So I'd suggest that pseries_add_processor() be made to update
> these things when the CPUs are marked present, before onlining them.
In pseries_add_processor, we are only marking the cpu as present. i.e
I believe numa_setup_cpu() would not have been called. So we may not have a
way to associate the CPU to the node. Otherwise we will have to call
numa_setup_cpu() or the hcall_vphn.
We could try calling numa_setup_cpu() immediately after we set the
CPU to be present, but that would be one more extra hcall + I dont know if
there are any more steps needed before CPU being made present and
associating the CPU to the node. Are we sure the node is already online? For
the numa_mem, we are better of if the zonelists for the node are built.
or the other solution would be to call this in map_cpu_to_node().
Here also we have to be sure the zonelists for the node are already built.
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH v5 25/48] KVM: PPC: Book3S HV P9: Use host timer accounting to avoid decrementer read
From: kernel test robot @ 2021-04-02 4:19 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev, kbuild-all, Nicholas Piggin
In-Reply-To: <20210401150325.442125-26-npiggin@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 5557 bytes --]
Hi Nicholas,
I love your patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.12-rc5 next-20210401]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Nicholas-Piggin/KVM-PPC-Book3S-C-ify-the-P9-entry-exit-code/20210401-232743
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-s031-20210401 (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.3-279-g6d5d9b42-dirty
# https://github.com/0day-ci/linux/commit/4c3e837b20a4ebe406d7b8fddbec2f6fa86755a2
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Nicholas-Piggin/KVM-PPC-Book3S-C-ify-the-P9-entry-exit-code/20210401-232743
git checkout 4c3e837b20a4ebe406d7b8fddbec2f6fa86755a2
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> arch/powerpc/kernel/time.c:877:9: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:877:9: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:877:9: sparse: got unsigned long long *
>> arch/powerpc/kernel/time.c:877:9: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:877:9: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:877:9: sparse: got unsigned long long *
>> arch/powerpc/kernel/time.c:877:9: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:877:9: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:877:9: sparse: got unsigned long long *
>> arch/powerpc/kernel/time.c:877:9: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:877:9: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:877:9: sparse: got unsigned long long *
>> arch/powerpc/kernel/time.c:877:9: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:877:9: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:877:9: sparse: got unsigned long long *
arch/powerpc/kernel/time.c:130:1: sparse: sparse: symbol 'rtc_lock' was not declared. Should it be static?
arch/powerpc/kernel/time.c:649:24: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:649:24: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:649:24: sparse: got unsigned long long *
arch/powerpc/kernel/time.c:580:24: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected void const [noderef] __percpu *__vpp_verify @@ got unsigned long long * @@
arch/powerpc/kernel/time.c:580:24: sparse: expected void const [noderef] __percpu *__vpp_verify
arch/powerpc/kernel/time.c:580:24: sparse: got unsigned long long *
vim +877 arch/powerpc/kernel/time.c
4a4cfe3836916e Tony Breeds 2007-09-22 873
d831d0b83f2058 Tony Breeds 2007-09-21 874 static int decrementer_set_next_event(unsigned long evt,
d831d0b83f2058 Tony Breeds 2007-09-21 875 struct clock_event_device *dev)
d831d0b83f2058 Tony Breeds 2007-09-21 876 {
6601ec1c2ba929 Christophe Leroy 2020-09-29 @877 __this_cpu_write(decrementers_next_tb, get_tb() + evt);
d831d0b83f2058 Tony Breeds 2007-09-21 878 set_dec(evt);
0215f7d8c53fb1 Benjamin Herrenschmidt 2014-01-14 879
0215f7d8c53fb1 Benjamin Herrenschmidt 2014-01-14 880 /* We may have raced with new irq work */
0215f7d8c53fb1 Benjamin Herrenschmidt 2014-01-14 881 if (test_irq_work_pending())
0215f7d8c53fb1 Benjamin Herrenschmidt 2014-01-14 882 set_dec(1);
0215f7d8c53fb1 Benjamin Herrenschmidt 2014-01-14 883
d831d0b83f2058 Tony Breeds 2007-09-21 884 return 0;
d831d0b83f2058 Tony Breeds 2007-09-21 885 }
d831d0b83f2058 Tony Breeds 2007-09-21 886
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34768 bytes --]
^ permalink raw reply
* Re: [PATCH v4 29/46] KVM: PPC: Book3S HV P9: Implement the rest of the P9 path in C
From: Alexey Kardashevskiy @ 2021-04-02 4:36 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <1617272101.bcglven6fh.astroid@bobo.none>
On 01/04/2021 21:35, Nicholas Piggin wrote:
> Excerpts from Alexey Kardashevskiy's message of April 1, 2021 3:30 pm:
>>
>>
>> On 3/23/21 12:02 PM, Nicholas Piggin wrote:
>>> Almost all logic is moved to C, by introducing a new in_guest mode that
>>> selects and branches very early in the interrupt handler to the P9 exit
>>> code.
>
> [...]
>
>>> +/*
>>> + * kvmppc_p9_exit_hcall and kvmppc_p9_exit_interrupt are branched to from
>>> + * above if the interrupt was taken for a guest that was entered via
>>> + * kvmppc_p9_enter_guest().
>>> + *
>>> + * This code recovers the host stack and vcpu pointer, saves all GPRs and
>>> + * CR, LR, CTR, XER as well as guest MSR and NIA into the VCPU, then re-
>>> + * establishes the host stack and registers to return from the
>>> + * kvmppc_p9_enter_guest() function.
>>
>> What does "this code" refer to? If it is the asm below, then it does not
>> save CTR, it is in the c code. Otherwise it is confusing (to me) :)
>
> Yes you're right, CTR is saved in C.
>
>>> + */
>>> +.balign IFETCH_ALIGN_BYTES
>>> +kvmppc_p9_exit_hcall:
>>> + mfspr r11,SPRN_SRR0
>>> + mfspr r12,SPRN_SRR1
>>> + li r10,0xc00
>>> + std r10,HSTATE_SCRATCH0(r13)
>>> +
>>> +.balign IFETCH_ALIGN_BYTES
>>> +kvmppc_p9_exit_interrupt:
>
> [...]
>
>>> +static inline void slb_invalidate(unsigned int ih)
>>> +{
>>> + asm volatile("slbia %0" :: "i"(ih));
>>> +}
>>
>> This one is not used.
>
> It gets used in a later patch, I guess I should move it there.
>
> [...]
>
>>> +int __kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu)
>>> +{
>>> + u64 *exsave;
>>> + unsigned long msr = mfmsr();
>>> + int trap;
>>> +
>>> + start_timing(vcpu, &vcpu->arch.rm_entry);
>>> +
>>> + vcpu->arch.ceded = 0;
>>> +
>>> + WARN_ON_ONCE(vcpu->arch.shregs.msr & MSR_HV);
>>> + WARN_ON_ONCE(!(vcpu->arch.shregs.msr & MSR_ME));
>>> +
>>> + mtspr(SPRN_HSRR0, vcpu->arch.regs.nip);
>>> + mtspr(SPRN_HSRR1, (vcpu->arch.shregs.msr & ~MSR_HV) | MSR_ME);
>>> +
>>> + /*
>>> + * On POWER9 DD2.1 and below, sometimes on a Hypervisor Data Storage
>>> + * Interrupt (HDSI) the HDSISR is not be updated at all.
>>> + *
>>> + * To work around this we put a canary value into the HDSISR before
>>> + * returning to a guest and then check for this canary when we take a
>>> + * HDSI. If we find the canary on a HDSI, we know the hardware didn't
>>> + * update the HDSISR. In this case we return to the guest to retake the
>>> + * HDSI which should correctly update the HDSISR the second time HDSI
>>> + * entry.
>>> + *
>>> + * Just do this on all p9 processors for now.
>>> + */
>>> + mtspr(SPRN_HDSISR, HDSISR_CANARY);
>>> +
>>> + accumulate_time(vcpu, &vcpu->arch.guest_time);
>>> +
>>> + local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_GUEST_HV_FAST;
>>> + kvmppc_p9_enter_guest(vcpu);
>>> + // Radix host and guest means host never runs with guest MMU state
>>> + local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_NONE;
>>> +
>>> + accumulate_time(vcpu, &vcpu->arch.rm_intr);
>>> +
>>> + /* Get these from r11/12 and paca exsave */
>>> + vcpu->arch.shregs.srr0 = mfspr(SPRN_SRR0);
>>> + vcpu->arch.shregs.srr1 = mfspr(SPRN_SRR1);
>>> + vcpu->arch.shregs.dar = mfspr(SPRN_DAR);
>>> + vcpu->arch.shregs.dsisr = mfspr(SPRN_DSISR);
>>> +
>>> + /* 0x2 bit for HSRR is only used by PR and P7/8 HV paths, clear it */
>>> + trap = local_paca->kvm_hstate.scratch0 & ~0x2;
>>> + if (likely(trap > BOOK3S_INTERRUPT_MACHINE_CHECK)) {
>>> + exsave = local_paca->exgen;
>>> + } else if (trap == BOOK3S_INTERRUPT_SYSTEM_RESET) {
>>> + exsave = local_paca->exnmi;
>>> + } else { /* trap == 0x200 */
>>> + exsave = local_paca->exmc;
>>> + }
>>> +
>>> + vcpu->arch.regs.gpr[1] = local_paca->kvm_hstate.scratch1;
>>> + vcpu->arch.regs.gpr[3] = local_paca->kvm_hstate.scratch2;
>>> + vcpu->arch.regs.gpr[9] = exsave[EX_R9/sizeof(u64)];
>>> + vcpu->arch.regs.gpr[10] = exsave[EX_R10/sizeof(u64)];
>>> + vcpu->arch.regs.gpr[11] = exsave[EX_R11/sizeof(u64)];
>>> + vcpu->arch.regs.gpr[12] = exsave[EX_R12/sizeof(u64)];
>>> + vcpu->arch.regs.gpr[13] = exsave[EX_R13/sizeof(u64)];
>>> + vcpu->arch.ppr = exsave[EX_PPR/sizeof(u64)];
>>> + vcpu->arch.cfar = exsave[EX_CFAR/sizeof(u64)];
>>> + vcpu->arch.regs.ctr = exsave[EX_CTR/sizeof(u64)];
>>> +
>>> + vcpu->arch.last_inst = KVM_INST_FETCH_FAILED;
>>> +
>>> + if (unlikely(trap == BOOK3S_INTERRUPT_MACHINE_CHECK)) {
>>> + vcpu->arch.fault_dar = exsave[EX_DAR/sizeof(u64)];
>>> + vcpu->arch.fault_dsisr = exsave[EX_DSISR/sizeof(u64)];
>>> + kvmppc_realmode_machine_check(vcpu);
>>> +
>>> + } else if (unlikely(trap == BOOK3S_INTERRUPT_HMI)) {
>>> + kvmppc_realmode_hmi_handler();
>>> +
>>> + } else if (trap == BOOK3S_INTERRUPT_H_EMUL_ASSIST) {
>>> + vcpu->arch.emul_inst = mfspr(SPRN_HEIR);
>>> +
>>> + } else if (trap == BOOK3S_INTERRUPT_H_DATA_STORAGE) {
>>> + vcpu->arch.fault_dar = exsave[EX_DAR/sizeof(u64)];
>>> + vcpu->arch.fault_dsisr = exsave[EX_DSISR/sizeof(u64)];
>>> + vcpu->arch.fault_gpa = mfspr(SPRN_ASDR);
>>> +
>>> + } else if (trap == BOOK3S_INTERRUPT_H_INST_STORAGE) {
>>> + vcpu->arch.fault_gpa = mfspr(SPRN_ASDR);
>>> +
>>> + } else if (trap == BOOK3S_INTERRUPT_H_FAC_UNAVAIL) {
>>> + vcpu->arch.hfscr = mfspr(SPRN_HFSCR);
>>> +
>>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>> + /*
>>> + * Softpatch interrupt for transactional memory emulation cases
>>> + * on POWER9 DD2.2. This is early in the guest exit path - we
>>> + * haven't saved registers or done a treclaim yet.
>>> + */
>>> + } else if (trap == BOOK3S_INTERRUPT_HV_SOFTPATCH) {
>>> + vcpu->arch.emul_inst = mfspr(SPRN_HEIR);
>>> +
>>> + /*
>>> + * The cases we want to handle here are those where the guest
>>> + * is in real suspend mode and is trying to transition to
>>> + * transactional mode.
>>> + */
>>> + if (local_paca->kvm_hstate.fake_suspend &&
>>> + (vcpu->arch.shregs.msr & MSR_TS_S)) {
>>> + if (kvmhv_p9_tm_emulation_early(vcpu)) {
>>> + /* Prevent it being handled again. */
>>> + trap = 0;
>>> + }
>>> + }
>>> +#endif
>>> + }
>>> +
>>> + radix_clear_slb();
>>> +
>>> + __mtmsrd(msr, 0);
>>
>>
>> The asm code only sets RI but this potentially sets more bits including
>> MSR_EE, is it expected to be 0 when __kvmhv_vcpu_entry_p9() is called?
>
> Yes.
>
>>> + mtspr(SPRN_CTRLT, 1);
>>
>> What is this for? ISA does not shed much light:
>> ===
>> 63 RUN This bit controls an external I/O pin.
>> ===
>
> I don't think it even does that these days. It interacts with the PMU.
> I was looking whether it's feasible to move it into PMU code entirely,
> but apparently some tool or something might sample it. I'm a bit
> suspicious about that because an untrusted guest could be running and
> claim not to so I don't know what said tool really achieves, but I'll
> go through that fight another day.
>
> But KVM has to set it to 1 at exit because Linux host has it set to 1
> except in CPU idle.
It this CTRLT setting a new thing or the asm does it too? I could not
spot it.
>>
>>
>>> +
>>> + accumulate_time(vcpu, &vcpu->arch.rm_exit);
>>
>> This should not compile without CONFIG_KVM_BOOK3S_HV_EXIT_TIMING.
>
> It has an ifdef wrapper so it should work (it does on my local tree
> which is slightly newer than what you have but I don't think I fixed
> anything around this recently).
You are absolutely right, my bad.
>
>>> +
>>> + end_timing(vcpu);
>>> +
>>> + return trap;
>>
>>
>> The asm does "For hash guest, read the guest SLB and save it away", this
>> code does not. Is this new fast-path-in-c only for radix-on-radix or
>> hash VMs are supported too?
>
> That asm code does not run for "guest_exit_short_path" case (aka the
> p9 path aka the fast path).
>
> Upstream code only supports radix host and radix guest in this path.
> The old path supports hash and radix. That's unchanged with this patch.
>
> After the series, the new path supports all P9 modes (hash/hash,
> radix/radix, and radix/hash), and the old path supports P7 and P8 only.
Thanks for clarification. Besides that CTRLT, I checked if the new c
code matches the old asm code (which made diving into ISA incredible fun
:) ) so fwiw
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
I'd really like to see longer commit logs clarifying all intended
changes but it is probably just me.
>
> Thanks,
> Nick
>
--
Alexey
^ permalink raw reply
* Re: [PATCH] powerpc/powernv: Enable HAIL (HV AIL) for ISA v3.1 processors
From: Cédric Le Goater @ 2021-04-02 6:10 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20210402024124.545826-1-npiggin@gmail.com>
On 4/2/21 4:41 AM, Nicholas Piggin wrote:
> Starting with ISA v3.1, LPCR[AIL] no longer controls the interrupt
> mode for HV=1 interrupts. Instead, a new LPCR[HAIL] bit is defined
> which behaves like AIL=3 for HV interrupts when set.
Will QEMU need an update ?
Thanks,
C.
> Set HAIL on bare metal to give us mmu-on interrupts and improve
> performance.
>
> This also fixes an scv bug: we don't implement scv real mode (AIL=0)
> vectors because they are at an inconvenient location, so we just
> disable scv support when AIL can not be set. However powernv assumes
> that LPCR[AIL] will enable AIL mode so it enables scv support despite
> HV interrupts being AIL=0, which causes scv interrupts to go off into
> the weeds.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/reg.h | 1 +
> arch/powerpc/kernel/setup_64.c | 19 ++++++++++++++++---
> 2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 1be20bc8dce2..9086a2644c89 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -441,6 +441,7 @@
> #define LPCR_VRMA_LP1 ASM_CONST(0x0000800000000000)
> #define LPCR_RMLS 0x1C000000 /* Implementation dependent RMO limit sel */
> #define LPCR_RMLS_SH 26
> +#define LPCR_HAIL ASM_CONST(0x0000000004000000) /* HV AIL (ISAv3.1) */
> #define LPCR_ILE ASM_CONST(0x0000000002000000) /* !HV irqs set MSR:LE */
> #define LPCR_AIL ASM_CONST(0x0000000001800000) /* Alternate interrupt location */
> #define LPCR_AIL_0 ASM_CONST(0x0000000000000000) /* MMU off exception offset 0x0 */
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index 04a31586f760..671192afcdfd 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -233,10 +233,23 @@ static void cpu_ready_for_interrupts(void)
> * If we are not in hypervisor mode the job is done once for
> * the whole partition in configure_exceptions().
> */
> - if (cpu_has_feature(CPU_FTR_HVMODE) &&
> - cpu_has_feature(CPU_FTR_ARCH_207S)) {
> + if (cpu_has_feature(CPU_FTR_HVMODE)) {
> unsigned long lpcr = mfspr(SPRN_LPCR);
> - mtspr(SPRN_LPCR, lpcr | LPCR_AIL_3);
> + unsigned long new_lpcr = lpcr;
> +
> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
> + /* P10 DD1 does not have HAIL */
> + if (pvr_version_is(PVR_POWER10) &&
> + (mfspr(SPRN_PVR) & 0xf00) == 0x100)
> + new_lpcr |= LPCR_AIL_3;
> + else
> + new_lpcr |= LPCR_HAIL;
> + } else if (cpu_has_feature(CPU_FTR_ARCH_207S)) {
> + new_lpcr |= LPCR_AIL_3;
> + }
> +
> + if (new_lpcr != lpcr)
> + mtspr(SPRN_LPCR, new_lpcr);
> }
>
> /*
>
^ permalink raw reply
* [RFC/PATCH] powerpc/smp: Add SD_SHARE_PKG_RESOURCES flag to MC sched-domain
From: Gautham R. Shenoy @ 2021-04-02 5:37 UTC (permalink / raw)
To: Michael Ellerman, Michael Neuling, Mel Gorman, Rik van Riel,
Srikar Dronamraju, Valentin Schneider, Vincent Guittot,
Dietmar Eggemann, Nicholas Piggin, Anton Blanchard, Parth Shah,
Vaidyanathan Srinivasan
Cc: Gautham R. Shenoy, linuxppc-dev, LKML
From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
On POWER10 systems, the L2 cache is at the SMT4 small core level. The
following commits ensure that L2 cache gets correctly discovered and
the Last-Level-Cache domain (LLC) is set to the SMT sched-domain.
790a166 powerpc/smp: Parse ibm,thread-groups with multiple properties
1fdc1d6 powerpc/smp: Rename cpu_l1_cache_map as thread_group_l1_cache_map
fbd2b67 powerpc/smp: Rename init_thread_group_l1_cache_map() to make
it generic
538abe powerpc/smp: Add support detecting thread-groups sharing L2 cache
0be4763 powerpc/cacheinfo: Print correct cache-sibling map/list for L2 cache
However, with the LLC now on the SMT sched-domain, we are seeing some
regressions in the performance of applications that requires
single-threaded performance. The reason for this is as follows:
Prior to the change (we call this P9-sched below), the sched-domain
hierarchy was:
SMT (SMT4) --> CACHE (SMT8)[LLC] --> MC (Hemisphere) --> DIE
where the CACHE sched-domain is defined to be the Last Level Cache (LLC).
On the upstream kernel, with the aforementioned commmits (P10-sched),
the sched-domain hierarchy is:
SMT (SMT4)[LLC] --> MC (Hemisphere) --> DIE
with the SMT sched-domain as the LLC.
When the scheduler tries to wakeup a task, it chooses between the
waker-CPU and the wakee's previous-CPU. Suppose this choice is called
the "target", then in the target's LLC domain, the scheduler
a) tries to find an idle core in the LLC. This helps exploit the
SMT folding that the wakee task can benefit from. If an idle
core is found, the wakee is woken up on it.
b) Failing to find an idle core, the scheduler tries to find an idle
CPU in the LLC. This helps minimise the wakeup latency for the
wakee since it gets to run on the CPU immediately.
c) Failing this, it will wake it up on target CPU.
Thus, with P9-sched topology, since the CACHE domain comprises of two
SMT4 cores, there is a decent chance that we get an idle core, failing
which there is a relatively higher probability of finding an idle CPU
among the 8 threads in the domain.
However, in P10-sched topology, since the SMT domain is the LLC and it
contains only a single SMT4 core, the probability that we find that
core to be idle is less. Furthermore, since there are only 4 CPUs to
search for an idle CPU, there is lower probability that we can get an
idle CPU to wake up the task on.
Thus applications which require single threaded performance will end
up getting woken up on potentially busy core, even though there are
idle cores in the system.
To remedy this, this patch proposes that the LLC be moved to the MC
level which is a group of cores in one half of the chip.
SMT (SMT4) --> MC (Hemisphere)[LLC] --> DIE
While there is no cache being shared at this level, this is still the
level where some amount of cache-snooping takes place and it is
relatively faster to access the data from the caches of the cores
within this domain. With this change, we no longer see regressions on
P10 for applications which require single threaded performance.
The patch also improves the tail latencies on schbench and the
usecs/op on "perf bench sched pipe"
On a 10 core P10 system with 80 CPUs,
schbench
============
(https://git.kernel.org/pub/scm/linux/kernel/git/mason/schbench.git/)
Values : Lower the better.
99th percentile is the tail latency.
99th percentile
~~~~~~~~~~~~~~~~~~
No. messenger
threads 5.12-rc4 5.12-rc4
P10-sched MC-LLC
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 70 us 85 us
2 81 us 101 us
3 92 us 107 us
4 96 us 110 us
5 103 us 123 us
6 3412 us ----> 122 us
7 1490 us 136 us
8 6200 us 3572 us
Hackbench
============
(perf bench sched pipe)
values: lower the better
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
No. of
parallel
instances 5.12-rc4 5.12-rc4
P10-sched MC-LLC
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 24.04 us/op 18.72 us/op
2 24.04 us/op 18.65 us/op
4 24.01 us/op 18.76 us/op
8 24.10 us/op 19.11 us/op
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
arch/powerpc/kernel/smp.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 5a4d59a..c75dbd4 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -976,6 +976,13 @@ static bool has_coregroup_support(void)
return coregroup_enabled;
}
+static int powerpc_mc_flags(void)
+{
+ if(has_coregroup_support())
+ return SD_SHARE_PKG_RESOURCES;
+ return 0;
+}
+
static const struct cpumask *cpu_mc_mask(int cpu)
{
return cpu_coregroup_mask(cpu);
@@ -986,7 +993,7 @@ static const struct cpumask *cpu_mc_mask(int cpu)
{ cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) },
#endif
{ shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) },
- { cpu_mc_mask, SD_INIT_NAME(MC) },
+ { cpu_mc_mask, powerpc_mc_flags, SD_INIT_NAME(MC) },
{ cpu_cpu_mask, SD_INIT_NAME(DIE) },
{ NULL, },
};
--
1.9.4
^ permalink raw reply related
* Re: [RFC/PATCH] powerpc/smp: Add SD_SHARE_PKG_RESOURCES flag to MC sched-domain
From: Gautham R Shenoy @ 2021-04-02 7:36 UTC (permalink / raw)
To: Gautham R. Shenoy
Cc: Parth Shah, Michael Neuling, Vaidyanathan Srinivasan,
Vincent Guittot, Srikar Dronamraju, Rik van Riel, Peter Zijlstra,
linuxppc-dev, LKML, Nicholas Piggin, Dietmar Eggemann, Mel Gorman,
Valentin Schneider
In-Reply-To: <1617341874-1205-1-git-send-email-ego@linux.vnet.ibm.com>
(Missed cc'ing Cc Peter in the original posting)
On Fri, Apr 02, 2021 at 11:07:54AM +0530, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
>
> On POWER10 systems, the L2 cache is at the SMT4 small core level. The
> following commits ensure that L2 cache gets correctly discovered and
> the Last-Level-Cache domain (LLC) is set to the SMT sched-domain.
>
> 790a166 powerpc/smp: Parse ibm,thread-groups with multiple properties
> 1fdc1d6 powerpc/smp: Rename cpu_l1_cache_map as thread_group_l1_cache_map
> fbd2b67 powerpc/smp: Rename init_thread_group_l1_cache_map() to make
> it generic
> 538abe powerpc/smp: Add support detecting thread-groups sharing L2 cache
> 0be4763 powerpc/cacheinfo: Print correct cache-sibling map/list for L2 cache
>
> However, with the LLC now on the SMT sched-domain, we are seeing some
> regressions in the performance of applications that requires
> single-threaded performance. The reason for this is as follows:
>
> Prior to the change (we call this P9-sched below), the sched-domain
> hierarchy was:
>
> SMT (SMT4) --> CACHE (SMT8)[LLC] --> MC (Hemisphere) --> DIE
>
> where the CACHE sched-domain is defined to be the Last Level Cache (LLC).
>
> On the upstream kernel, with the aforementioned commmits (P10-sched),
> the sched-domain hierarchy is:
>
> SMT (SMT4)[LLC] --> MC (Hemisphere) --> DIE
>
> with the SMT sched-domain as the LLC.
>
> When the scheduler tries to wakeup a task, it chooses between the
> waker-CPU and the wakee's previous-CPU. Suppose this choice is called
> the "target", then in the target's LLC domain, the scheduler
>
> a) tries to find an idle core in the LLC. This helps exploit the
> SMT folding that the wakee task can benefit from. If an idle
> core is found, the wakee is woken up on it.
>
> b) Failing to find an idle core, the scheduler tries to find an idle
> CPU in the LLC. This helps minimise the wakeup latency for the
> wakee since it gets to run on the CPU immediately.
>
> c) Failing this, it will wake it up on target CPU.
>
> Thus, with P9-sched topology, since the CACHE domain comprises of two
> SMT4 cores, there is a decent chance that we get an idle core, failing
> which there is a relatively higher probability of finding an idle CPU
> among the 8 threads in the domain.
>
> However, in P10-sched topology, since the SMT domain is the LLC and it
> contains only a single SMT4 core, the probability that we find that
> core to be idle is less. Furthermore, since there are only 4 CPUs to
> search for an idle CPU, there is lower probability that we can get an
> idle CPU to wake up the task on.
>
> Thus applications which require single threaded performance will end
> up getting woken up on potentially busy core, even though there are
> idle cores in the system.
>
> To remedy this, this patch proposes that the LLC be moved to the MC
> level which is a group of cores in one half of the chip.
>
> SMT (SMT4) --> MC (Hemisphere)[LLC] --> DIE
>
> While there is no cache being shared at this level, this is still the
> level where some amount of cache-snooping takes place and it is
> relatively faster to access the data from the caches of the cores
> within this domain. With this change, we no longer see regressions on
> P10 for applications which require single threaded performance.
>
> The patch also improves the tail latencies on schbench and the
> usecs/op on "perf bench sched pipe"
>
> On a 10 core P10 system with 80 CPUs,
>
> schbench
> ============
> (https://git.kernel.org/pub/scm/linux/kernel/git/mason/schbench.git/)
>
> Values : Lower the better.
> 99th percentile is the tail latency.
>
>
> 99th percentile
> ~~~~~~~~~~~~~~~~~~
> No. messenger
> threads 5.12-rc4 5.12-rc4
> P10-sched MC-LLC
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 70 us 85 us
> 2 81 us 101 us
> 3 92 us 107 us
> 4 96 us 110 us
> 5 103 us 123 us
> 6 3412 us ----> 122 us
> 7 1490 us 136 us
> 8 6200 us 3572 us
>
>
> Hackbench
> ============
> (perf bench sched pipe)
> values: lower the better
>
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> No. of
> parallel
> instances 5.12-rc4 5.12-rc4
> P10-sched MC-LLC
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 1 24.04 us/op 18.72 us/op
> 2 24.04 us/op 18.65 us/op
> 4 24.01 us/op 18.76 us/op
> 8 24.10 us/op 19.11 us/op
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/smp.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 5a4d59a..c75dbd4 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -976,6 +976,13 @@ static bool has_coregroup_support(void)
> return coregroup_enabled;
> }
>
> +static int powerpc_mc_flags(void)
> +{
> + if(has_coregroup_support())
> + return SD_SHARE_PKG_RESOURCES;
> + return 0;
> +}
> +
> static const struct cpumask *cpu_mc_mask(int cpu)
> {
> return cpu_coregroup_mask(cpu);
> @@ -986,7 +993,7 @@ static const struct cpumask *cpu_mc_mask(int cpu)
> { cpu_smt_mask, powerpc_smt_flags, SD_INIT_NAME(SMT) },
> #endif
> { shared_cache_mask, powerpc_shared_cache_flags, SD_INIT_NAME(CACHE) },
> - { cpu_mc_mask, SD_INIT_NAME(MC) },
> + { cpu_mc_mask, powerpc_mc_flags, SD_INIT_NAME(MC) },
> { cpu_cpu_mask, SD_INIT_NAME(DIE) },
> { NULL, },
> };
> --
> 1.9.4
>
^ permalink raw reply
* Re: [PATCH v4 29/46] KVM: PPC: Book3S HV P9: Implement the rest of the P9 path in C
From: Nicholas Piggin @ 2021-04-02 7:58 UTC (permalink / raw)
To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <e2fc29aa-f38c-4650-06e2-d918c59547bf@ozlabs.ru>
Excerpts from Alexey Kardashevskiy's message of April 2, 2021 2:36 pm:
>
>
> On 01/04/2021 21:35, Nicholas Piggin wrote:
>> Excerpts from Alexey Kardashevskiy's message of April 1, 2021 3:30 pm:
>>>
>>>
>>> On 3/23/21 12:02 PM, Nicholas Piggin wrote:
>>>> Almost all logic is moved to C, by introducing a new in_guest mode that
>>>> selects and branches very early in the interrupt handler to the P9 exit
>>>> code.
>>
>> [...]
>>
>>>> +/*
>>>> + * kvmppc_p9_exit_hcall and kvmppc_p9_exit_interrupt are branched to from
>>>> + * above if the interrupt was taken for a guest that was entered via
>>>> + * kvmppc_p9_enter_guest().
>>>> + *
>>>> + * This code recovers the host stack and vcpu pointer, saves all GPRs and
>>>> + * CR, LR, CTR, XER as well as guest MSR and NIA into the VCPU, then re-
>>>> + * establishes the host stack and registers to return from the
>>>> + * kvmppc_p9_enter_guest() function.
>>>
>>> What does "this code" refer to? If it is the asm below, then it does not
>>> save CTR, it is in the c code. Otherwise it is confusing (to me) :)
>>
>> Yes you're right, CTR is saved in C.
>>
>>>> + */
>>>> +.balign IFETCH_ALIGN_BYTES
>>>> +kvmppc_p9_exit_hcall:
>>>> + mfspr r11,SPRN_SRR0
>>>> + mfspr r12,SPRN_SRR1
>>>> + li r10,0xc00
>>>> + std r10,HSTATE_SCRATCH0(r13)
>>>> +
>>>> +.balign IFETCH_ALIGN_BYTES
>>>> +kvmppc_p9_exit_interrupt:
>>
>> [...]
>>
>>>> +static inline void slb_invalidate(unsigned int ih)
>>>> +{
>>>> + asm volatile("slbia %0" :: "i"(ih));
>>>> +}
>>>
>>> This one is not used.
>>
>> It gets used in a later patch, I guess I should move it there.
>>
>> [...]
>>
>>>> +int __kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu)
>>>> +{
>>>> + u64 *exsave;
>>>> + unsigned long msr = mfmsr();
>>>> + int trap;
>>>> +
>>>> + start_timing(vcpu, &vcpu->arch.rm_entry);
>>>> +
>>>> + vcpu->arch.ceded = 0;
>>>> +
>>>> + WARN_ON_ONCE(vcpu->arch.shregs.msr & MSR_HV);
>>>> + WARN_ON_ONCE(!(vcpu->arch.shregs.msr & MSR_ME));
>>>> +
>>>> + mtspr(SPRN_HSRR0, vcpu->arch.regs.nip);
>>>> + mtspr(SPRN_HSRR1, (vcpu->arch.shregs.msr & ~MSR_HV) | MSR_ME);
>>>> +
>>>> + /*
>>>> + * On POWER9 DD2.1 and below, sometimes on a Hypervisor Data Storage
>>>> + * Interrupt (HDSI) the HDSISR is not be updated at all.
>>>> + *
>>>> + * To work around this we put a canary value into the HDSISR before
>>>> + * returning to a guest and then check for this canary when we take a
>>>> + * HDSI. If we find the canary on a HDSI, we know the hardware didn't
>>>> + * update the HDSISR. In this case we return to the guest to retake the
>>>> + * HDSI which should correctly update the HDSISR the second time HDSI
>>>> + * entry.
>>>> + *
>>>> + * Just do this on all p9 processors for now.
>>>> + */
>>>> + mtspr(SPRN_HDSISR, HDSISR_CANARY);
>>>> +
>>>> + accumulate_time(vcpu, &vcpu->arch.guest_time);
>>>> +
>>>> + local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_GUEST_HV_FAST;
>>>> + kvmppc_p9_enter_guest(vcpu);
>>>> + // Radix host and guest means host never runs with guest MMU state
>>>> + local_paca->kvm_hstate.in_guest = KVM_GUEST_MODE_NONE;
>>>> +
>>>> + accumulate_time(vcpu, &vcpu->arch.rm_intr);
>>>> +
>>>> + /* Get these from r11/12 and paca exsave */
>>>> + vcpu->arch.shregs.srr0 = mfspr(SPRN_SRR0);
>>>> + vcpu->arch.shregs.srr1 = mfspr(SPRN_SRR1);
>>>> + vcpu->arch.shregs.dar = mfspr(SPRN_DAR);
>>>> + vcpu->arch.shregs.dsisr = mfspr(SPRN_DSISR);
>>>> +
>>>> + /* 0x2 bit for HSRR is only used by PR and P7/8 HV paths, clear it */
>>>> + trap = local_paca->kvm_hstate.scratch0 & ~0x2;
>>>> + if (likely(trap > BOOK3S_INTERRUPT_MACHINE_CHECK)) {
>>>> + exsave = local_paca->exgen;
>>>> + } else if (trap == BOOK3S_INTERRUPT_SYSTEM_RESET) {
>>>> + exsave = local_paca->exnmi;
>>>> + } else { /* trap == 0x200 */
>>>> + exsave = local_paca->exmc;
>>>> + }
>>>> +
>>>> + vcpu->arch.regs.gpr[1] = local_paca->kvm_hstate.scratch1;
>>>> + vcpu->arch.regs.gpr[3] = local_paca->kvm_hstate.scratch2;
>>>> + vcpu->arch.regs.gpr[9] = exsave[EX_R9/sizeof(u64)];
>>>> + vcpu->arch.regs.gpr[10] = exsave[EX_R10/sizeof(u64)];
>>>> + vcpu->arch.regs.gpr[11] = exsave[EX_R11/sizeof(u64)];
>>>> + vcpu->arch.regs.gpr[12] = exsave[EX_R12/sizeof(u64)];
>>>> + vcpu->arch.regs.gpr[13] = exsave[EX_R13/sizeof(u64)];
>>>> + vcpu->arch.ppr = exsave[EX_PPR/sizeof(u64)];
>>>> + vcpu->arch.cfar = exsave[EX_CFAR/sizeof(u64)];
>>>> + vcpu->arch.regs.ctr = exsave[EX_CTR/sizeof(u64)];
>>>> +
>>>> + vcpu->arch.last_inst = KVM_INST_FETCH_FAILED;
>>>> +
>>>> + if (unlikely(trap == BOOK3S_INTERRUPT_MACHINE_CHECK)) {
>>>> + vcpu->arch.fault_dar = exsave[EX_DAR/sizeof(u64)];
>>>> + vcpu->arch.fault_dsisr = exsave[EX_DSISR/sizeof(u64)];
>>>> + kvmppc_realmode_machine_check(vcpu);
>>>> +
>>>> + } else if (unlikely(trap == BOOK3S_INTERRUPT_HMI)) {
>>>> + kvmppc_realmode_hmi_handler();
>>>> +
>>>> + } else if (trap == BOOK3S_INTERRUPT_H_EMUL_ASSIST) {
>>>> + vcpu->arch.emul_inst = mfspr(SPRN_HEIR);
>>>> +
>>>> + } else if (trap == BOOK3S_INTERRUPT_H_DATA_STORAGE) {
>>>> + vcpu->arch.fault_dar = exsave[EX_DAR/sizeof(u64)];
>>>> + vcpu->arch.fault_dsisr = exsave[EX_DSISR/sizeof(u64)];
>>>> + vcpu->arch.fault_gpa = mfspr(SPRN_ASDR);
>>>> +
>>>> + } else if (trap == BOOK3S_INTERRUPT_H_INST_STORAGE) {
>>>> + vcpu->arch.fault_gpa = mfspr(SPRN_ASDR);
>>>> +
>>>> + } else if (trap == BOOK3S_INTERRUPT_H_FAC_UNAVAIL) {
>>>> + vcpu->arch.hfscr = mfspr(SPRN_HFSCR);
>>>> +
>>>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>>>> + /*
>>>> + * Softpatch interrupt for transactional memory emulation cases
>>>> + * on POWER9 DD2.2. This is early in the guest exit path - we
>>>> + * haven't saved registers or done a treclaim yet.
>>>> + */
>>>> + } else if (trap == BOOK3S_INTERRUPT_HV_SOFTPATCH) {
>>>> + vcpu->arch.emul_inst = mfspr(SPRN_HEIR);
>>>> +
>>>> + /*
>>>> + * The cases we want to handle here are those where the guest
>>>> + * is in real suspend mode and is trying to transition to
>>>> + * transactional mode.
>>>> + */
>>>> + if (local_paca->kvm_hstate.fake_suspend &&
>>>> + (vcpu->arch.shregs.msr & MSR_TS_S)) {
>>>> + if (kvmhv_p9_tm_emulation_early(vcpu)) {
>>>> + /* Prevent it being handled again. */
>>>> + trap = 0;
>>>> + }
>>>> + }
>>>> +#endif
>>>> + }
>>>> +
>>>> + radix_clear_slb();
>>>> +
>>>> + __mtmsrd(msr, 0);
>>>
>>>
>>> The asm code only sets RI but this potentially sets more bits including
>>> MSR_EE, is it expected to be 0 when __kvmhv_vcpu_entry_p9() is called?
>>
>> Yes.
>>
>>>> + mtspr(SPRN_CTRLT, 1);
>>>
>>> What is this for? ISA does not shed much light:
>>> ===
>>> 63 RUN This bit controls an external I/O pin.
>>> ===
>>
>> I don't think it even does that these days. It interacts with the PMU.
>> I was looking whether it's feasible to move it into PMU code entirely,
>> but apparently some tool or something might sample it. I'm a bit
>> suspicious about that because an untrusted guest could be running and
>> claim not to so I don't know what said tool really achieves, but I'll
>> go through that fight another day.
>>
>> But KVM has to set it to 1 at exit because Linux host has it set to 1
>> except in CPU idle.
>
>
> It this CTRLT setting a new thing or the asm does it too? I could not
> spot it.
It's quite old actually. Earlier processors (maybe POWER6) you had to
even read-modify-write but new ones you can just store 1:
Guest exit:
/* Save guest CTRL register, set runlatch to 1 */
mfspr r6,SPRN_CTRLF
stw r6,VCPU_CTRL(r9)
andi. r0,r6,1
bne 4f
ori r6,r6,1
mtspr SPRN_CTRLT,r6
4:
entry:
/* Restore state of CTRL run bit; assume 1 on entry */
lwz r5,VCPU_CTRL(r4)
andi. r5,r5,1
bne 4f
mfspr r6,SPRN_CTRLF
clrrdi r6,r6,1
mtspr SPRN_CTRLT,r6
It used to light an indicator on the front of the system once upon a
time and I think on some processors (Cell maybe?) it actually controlled
SMT threads in some way. But certainly in P9 it does almost nothing and
we'll probably try to phase it out.
>>> The asm does "For hash guest, read the guest SLB and save it away", this
>>> code does not. Is this new fast-path-in-c only for radix-on-radix or
>>> hash VMs are supported too?
>>
>> That asm code does not run for "guest_exit_short_path" case (aka the
>> p9 path aka the fast path).
>>
>> Upstream code only supports radix host and radix guest in this path.
>> The old path supports hash and radix. That's unchanged with this patch.
>>
>> After the series, the new path supports all P9 modes (hash/hash,
>> radix/radix, and radix/hash), and the old path supports P7 and P8 only.
>
>
> Thanks for clarification. Besides that CTRLT, I checked if the new c
> code matches the old asm code (which made diving into ISA incredible fun
> :) ) so fwiw
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Thanks for reviewing.
> I'd really like to see longer commit logs clarifying all intended
> changes but it is probably just me.
I'm not sure what the best balance is, at some point code is a more
precise description. For this particular patch I probably do need to go
over the changelog again and try to make sure it makes sense and covers
things. If you have specifics that are missing or changes you'd like
I would definitely consider them.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] powerpc/powernv: Enable HAIL (HV AIL) for ISA v3.1 processors
From: Nicholas Piggin @ 2021-04-02 8:00 UTC (permalink / raw)
To: Cédric Le Goater, linuxppc-dev
In-Reply-To: <0f41c067-7b9f-5b98-0993-c81cbaeea265@kaod.org>
Excerpts from Cédric Le Goater's message of April 2, 2021 4:10 pm:
> On 4/2/21 4:41 AM, Nicholas Piggin wrote:
>> Starting with ISA v3.1, LPCR[AIL] no longer controls the interrupt
>> mode for HV=1 interrupts. Instead, a new LPCR[HAIL] bit is defined
>> which behaves like AIL=3 for HV interrupts when set.
>
> Will QEMU need an update ?
Yes you're right it will, we need to do that.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v5 29/48] powerpc: add set_dec_or_work API for safely updating decrementer
From: Nicholas Piggin @ 2021-04-02 11:04 UTC (permalink / raw)
To: kvm-ppc; +Cc: Alexey Kardashevskiy, linuxppc-dev
In-Reply-To: <20210401150325.442125-30-npiggin@gmail.com>
Excerpts from Nicholas Piggin's message of April 2, 2021 1:03 am:
> Decrementer updates must always check for new irq work to avoid an
> irq work decrementer interrupt being lost.
>
> Add an API for this in the timer code so callers don't have to care
> about details.
Oh I forgot to update the changelog for this, it's significantly
changed, so I better withdraw the Reviewed-by as well. I think this
re-arm API is the better one, there's no reason it can't all be in
the host time.c code.
This implementation also avoids what used to be inevitable double
interrupt to take a host timer from guest (first hdec to get into
the host and set dec to some -ve value, then taking that dec as
soon as we enable interrupts) by just marking the dec pending in
that case, so it gets replayed when we enable irqs.
Thanks,
Nick
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/time.h | 4 ++++
> arch/powerpc/kernel/time.c | 41 +++++++++++++++++++++++++--------
> arch/powerpc/kvm/book3s_hv.c | 6 +----
> 3 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
> index 0128cd9769bc..924b2157882f 100644
> --- a/arch/powerpc/include/asm/time.h
> +++ b/arch/powerpc/include/asm/time.h
> @@ -106,6 +106,10 @@ static inline u64 timer_get_next_tb(void)
> return __this_cpu_read(decrementers_next_tb);
> }
>
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> +void timer_rearm_host_dec(u64 now);
> +#endif
> +
> /* Convert timebase ticks to nanoseconds */
> unsigned long long tb_to_ns(unsigned long long tb_ticks);
>
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index 8b9b38a8ce57..8bbcc6be40c0 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -563,13 +563,43 @@ void arch_irq_work_raise(void)
> preempt_enable();
> }
>
> +static void set_dec_or_work(u64 val)
> +{
> + set_dec(val);
> + /* We may have raced with new irq work */
> + if (unlikely(test_irq_work_pending()))
> + set_dec(1);
> +}
> +
> #else /* CONFIG_IRQ_WORK */
>
> #define test_irq_work_pending() 0
> #define clear_irq_work_pending()
>
> +static void set_dec_or_work(u64 val)
> +{
> + set_dec(val);
> +}
> #endif /* CONFIG_IRQ_WORK */
>
> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
> +void timer_rearm_host_dec(u64 now)
> +{
> + u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
> +
> + WARN_ON_ONCE(!arch_irqs_disabled());
> + WARN_ON_ONCE(mfmsr() & MSR_EE);
> +
> + if (now >= *next_tb) {
> + now = *next_tb - now;
> + set_dec_or_work(now);
> + } else {
> + local_paca->irq_happened |= PACA_IRQ_DEC;
> + }
> +}
> +EXPORT_SYMBOL_GPL(timer_rearm_host_dec);
> +#endif
> +
> /*
> * timer_interrupt - gets called when the decrementer overflows,
> * with interrupts disabled.
> @@ -630,10 +660,7 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(timer_interrupt)
> } else {
> now = *next_tb - now;
> if (now <= decrementer_max)
> - set_dec(now);
> - /* We may have raced with new irq work */
> - if (test_irq_work_pending())
> - set_dec(1);
> + set_dec_or_work(now);
> __this_cpu_inc(irq_stat.timer_irqs_others);
> }
>
> @@ -875,11 +902,7 @@ static int decrementer_set_next_event(unsigned long evt,
> struct clock_event_device *dev)
> {
> __this_cpu_write(decrementers_next_tb, get_tb() + evt);
> - set_dec(evt);
> -
> - /* We may have raced with new irq work */
> - if (test_irq_work_pending())
> - set_dec(1);
> + set_dec_or_work(evt);
>
> return 0;
> }
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 8c8df88eec8c..287042b4afb5 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3901,11 +3901,7 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
> vc->entry_exit_map = 0x101;
> vc->in_guest = 0;
>
> - next_timer = timer_get_next_tb();
> - set_dec(next_timer - tb);
> - /* We may have raced with new irq work */
> - if (test_irq_work_pending())
> - set_dec(1);
> + timer_rearm_host_dec(tb);
> mtspr(SPRN_SPRG_VDSO_WRITE, local_paca->sprg_vdso);
>
> kvmhv_load_host_pmu();
> --
> 2.23.0
>
>
^ permalink raw reply
* Re: [PATCH v3 8/9] powerpc/xive: Map one IPI interrupt per node
From: Cédric Le Goater @ 2021-04-02 11:31 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Thomas Gleixner, Greg Kurz
In-Reply-To: <1617279896.hl1bn8g3eg.astroid@bobo.none>
On 4/1/21 2:50 PM, Nicholas Piggin wrote:
> Excerpts from Cédric Le Goater's message of April 1, 2021 12:45 am:
>> ipistorm [*] can be used to benchmark the raw interrupt rate of an
>> interrupt controller by measuring the number of IPIs a system can
>> sustain. When applied to the XIVE interrupt controller of POWER9 and
>> POWER10 systems, a significant drop of the interrupt rate can be
>> observed when crossing the second node boundary.
>>
>> This is due to the fact that a single IPI interrupt is used for all
>> CPUs of the system. The structure is shared and the cache line updates
>> impact greatly the traffic between nodes and the overall IPI
>> performance.
>>
>> As a workaround, the impact can be reduced by deactivating the IRQ
>> lockup detector ("noirqdebug") which does a lot of accounting in the
>> Linux IRQ descriptor structure and is responsible for most of the
>> performance penalty.
>>
>> As a fix, this proposal allocates an IPI interrupt per node, to be
>> shared by all CPUs of that node. It solves the scaling issue, the IRQ
>> lockup detector still has an impact but the XIVE interrupt rate scales
>> linearly. It also improves the "noirqdebug" case as showed in the
>> tables below.
>>
>> * P9 DD2.2 - 2s * 64 threads
>>
>> "noirqdebug"
>> Mint/s Mint/s
>> chips cpus IPI/sys IPI/chip IPI/chip IPI/sys
>> --------------------------------------------------------------
>> 1 0-15 4.984023 4.875405 4.996536 5.048892
>> 0-31 10.879164 10.544040 10.757632 11.037859
>> 0-47 15.345301 14.688764 14.926520 15.310053
>> 0-63 17.064907 17.066812 17.613416 17.874511
>> 2 0-79 11.768764 21.650749 22.689120 22.566508
>> 0-95 10.616812 26.878789 28.434703 28.320324
>> 0-111 10.151693 31.397803 31.771773 32.388122
>> 0-127 9.948502 33.139336 34.875716 35.224548
>>
>> * P10 DD1 - 4s (not homogeneous) 352 threads
>>
>> "noirqdebug"
>> Mint/s Mint/s
>> chips cpus IPI/sys IPI/chip IPI/chip IPI/sys
>> --------------------------------------------------------------
>> 1 0-15 2.409402 2.364108 2.383303 2.395091
>> 0-31 6.028325 6.046075 6.089999 6.073750
>> 0-47 8.655178 8.644531 8.712830 8.724702
>> 0-63 11.629652 11.735953 12.088203 12.055979
>> 0-79 14.392321 14.729959 14.986701 14.973073
>> 0-95 12.604158 13.004034 17.528748 17.568095
>> 2 0-111 9.767753 13.719831 19.968606 20.024218
>> 0-127 6.744566 16.418854 22.898066 22.995110
>> 0-143 6.005699 19.174421 25.425622 25.417541
>> 0-159 5.649719 21.938836 27.952662 28.059603
>> 0-175 5.441410 24.109484 31.133915 31.127996
>> 3 0-191 5.318341 24.405322 33.999221 33.775354
>> 0-207 5.191382 26.449769 36.050161 35.867307
>> 0-223 5.102790 29.356943 39.544135 39.508169
>> 0-239 5.035295 31.933051 42.135075 42.071975
>> 0-255 4.969209 34.477367 44.655395 44.757074
>> 4 0-271 4.907652 35.887016 47.080545 47.318537
>> 0-287 4.839581 38.076137 50.464307 50.636219
>> 0-303 4.786031 40.881319 53.478684 53.310759
>> 0-319 4.743750 43.448424 56.388102 55.973969
>> 0-335 4.709936 45.623532 59.400930 58.926857
>> 0-351 4.681413 45.646151 62.035804 61.830057
>>
>> [*] https://github.com/antonblanchard/ipistorm
>>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>
> Very nice result but the default-on irqdebug code is quite a slowdown
> even with your improvements.
>
> Is the main cacheline bouncing in the fast path coming from
> desc->irq_count++ of the percpu handler? Can we do something quick and
> dirty like the attached patch?
>
> All this stuff seems totally racy with percpu handler but maybe that
> doesn't matter too much (and anyway it would be a much bigger change)
I gave the patch below a try and we are reaching the same results,
even better. The simplest solution is always the best. Nick, you
should send that single patch.
Thanks,
C.
> Thanks,
> Nick
>
> ---
> kernel/irq/spurious.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
> index f865e5f4d382..6b17b737ee6c 100644
> --- a/kernel/irq/spurious.c
> +++ b/kernel/irq/spurious.c
> @@ -378,7 +378,8 @@ void note_interrupt(struct irq_desc *desc, irqreturn_t action_ret)
> * then we merily delay the spurious detection
> * by one hard interrupt. Not a real problem.
> */
> - desc->threads_handled_last &= ~SPURIOUS_DEFERRED;
> + if (desc->threads_handled_last & SPURIOUS_DEFERRED)
> + desc->threads_handled_last &= ~SPURIOUS_DEFERRED;
> }
> }
>
> @@ -403,6 +404,10 @@ void note_interrupt(struct irq_desc *desc, irqreturn_t action_ret)
> desc->irqs_unhandled -= ok;
> }
>
> + if (likely(!desc->irqs_unhandled))
> + return;
> +
> + /* Now getting into unhandled irq detection */
> desc->irq_count++;
> if (likely(desc->irq_count < 100000))
> return;
>
^ permalink raw reply
* Re: [PATCH v3 8/9] powerpc/xive: Map one IPI interrupt per node
From: Cédric Le Goater @ 2021-04-02 12:19 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Thomas Gleixner, Greg Kurz
In-Reply-To: <ae512e1b-91ee-1cc8-c6a8-36e014ed8788@kaod.org>
> I gave the patch below a try and we are reaching the same results,
> even better. The simplest solution is always the best. Nick, you
> should send that single patch.
FYI, here are results in a KVM guests with pinned vCPUs.
* P9 DD2.2 - 2s * 64 threads - KVM guest :
IPI/sys IPI/chip
----------- -------------------------------- --------------------------------
unhandled unhandled
chips cpus noirqdebug detection noirqdebug detection
----------- -------------------------------- --------------------------------
1 0-15 4.152813 4.084240 4.061028 4.097700 4.042539 4.008314
0-31 8.186328 8.157970 7.937127 8.277942 8.019539 7.831189
0-47 11.391635 11.232960 11.017530 11.278048 10.994501 10.889347
0-63 13.907476 14.022307 11.460280 13.933946 13.506828 11.369188
2 0-79 18.105276 18.084463 8.376047 18.069176 17.587916 15.477006
0-95 22.100683 22.265763 7.338229 22.084006 21.588463 19.502192
0-111 25.305948 25.473068 6.716235 25.429261 24.607570 22.733253
0-127 27.814449 28.029029 6.222736 27.960119 27.253432 23.884916
The three columns "IPI/chip" are results with this series. "IPI/sys" are
without. The "unhandled detection" columns are with Nick's patch.
C.
^ permalink raw reply
* Re: [PATCH v2] pseries: prevent free CPU ids to be reused on another node
From: Nathan Lynch @ 2021-04-02 13:34 UTC (permalink / raw)
To: Laurent Dufour
Cc: Srikar Dronamraju, cheloha, linux-kernel, paulus, linuxppc-dev
In-Reply-To: <20210325093512.57856-1-ldufour@linux.ibm.com>
Hi Laurent,
Laurent Dufour <ldufour@linux.ibm.com> writes:
> When a CPU is hot added, the CPU ids are taken from the available mask from
> the lower possible set. If that set of values was previously used for CPU
> attached to a different node, this seems to application like if these CPUs
> have migrated from a node to another one which is not expected in real
> life.
This seems like a problem that could affect other architectures or
platforms? I guess as long as arch code is responsible for placing new
CPUs in cpu_present_mask, that code will have the responsibility of
ensuring CPU IDs' NUMA assignments remain stable.
[...]
> The effect of this patch can be seen by removing and adding CPUs using the
> Qemu monitor. In the following case, the first CPU from the node 2 is
> removed, then the first one from the node 1 is removed too. Later, the
> first CPU of the node 2 is added back. Without that patch, the kernel will
> numbered these CPUs using the first CPU ids available which are the ones
> freed when removing the second CPU of the node 0. This leads to the CPU ids
> 16-23 to move from the node 1 to the node 2. With the patch applied, the
> CPU ids 32-39 are used since they are the lowest free ones which have not
> been used on another node.
>
> At boot time:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
>
> Vanilla kernel, after the CPU hot unplug/plug operations:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 24 25 26 27 28 29 30 31
> node 2 cpus: 16 17 18 19 20 21 22 23 40 41 42 43 44 45 46 47
>
> Patched kernel, after the CPU hot unplug/plug operations:
> [root@vm40 ~]# numactl -H | grep cpus
> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
> node 1 cpus: 24 25 26 27 28 29 30 31
> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
Good demonstration of the problem. CPUs 16-23 "move" from node 1 to
node 2.
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 12cbffd3c2e3..48c7943b25b0 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -39,6 +39,8 @@
> /* This version can't take the spinlock, because it never returns */
> static int rtas_stop_self_token = RTAS_UNKNOWN_SERVICE;
>
> +static cpumask_var_t node_recorded_ids_map[MAX_NUMNODES];
I guess this should have documentation that it must be
accessed/manipulated with cpu_add_remove_lock held?
> +
> static void rtas_stop_self(void)
> {
> static struct rtas_args args;
> @@ -151,29 +153,61 @@ static void pseries_cpu_die(unsigned int cpu)
> */
> static int pseries_add_processor(struct device_node *np)
> {
> - unsigned int cpu;
> + unsigned int cpu, node;
> cpumask_var_t candidate_mask, tmp;
> - int err = -ENOSPC, len, nthreads, i;
> + int err = -ENOSPC, len, nthreads, i, nid;
From eight local vars to ten, and the two new variables' names are
"node" and "nid". More distinctive names would help readers.
> const __be32 *intserv;
> + bool force_reusing = false;
>
> intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
> if (!intserv)
> return 0;
>
> - zalloc_cpumask_var(&candidate_mask, GFP_KERNEL);
> - zalloc_cpumask_var(&tmp, GFP_KERNEL);
> + alloc_cpumask_var(&candidate_mask, GFP_KERNEL);
> + alloc_cpumask_var(&tmp, GFP_KERNEL);
> +
> + /*
> + * Fetch from the DT nodes read by dlpar_configure_connector() the NUMA
> + * node id the added CPU belongs to.
> + */
> + nid = of_node_to_nid(np);
> + if (nid < 0 || !node_possible(nid))
> + nid = first_online_node;
>
> nthreads = len / sizeof(u32);
> - for (i = 0; i < nthreads; i++)
> - cpumask_set_cpu(i, tmp);
>
> cpu_maps_update_begin();
>
> BUG_ON(!cpumask_subset(cpu_present_mask, cpu_possible_mask));
>
> +again:
> + cpumask_clear(candidate_mask);
> + cpumask_clear(tmp);
> + for (i = 0; i < nthreads; i++)
> + cpumask_set_cpu(i, tmp);
> +
> /* Get a bitmap of unoccupied slots. */
> cpumask_xor(candidate_mask, cpu_possible_mask, cpu_present_mask);
> +
> + /*
> + * Remove free ids previously assigned on the other nodes. We can walk
> + * only online nodes because once a node became online it is not turned
> + * offlined back.
> + */
> + if (!force_reusing)
> + for_each_online_node(node) {
> + if (node == nid) /* Keep our node's recorded ids */
> + continue;
> + cpumask_andnot(candidate_mask, candidate_mask,
> + node_recorded_ids_map[node]);
> + }
> +
> if (cpumask_empty(candidate_mask)) {
> + if (!force_reusing) {
> + force_reusing = true;
> + goto again;
> + }
> +
Hmm I'd encourage you to work toward a solution that doesn't involve
adding backwards jumps and a bool flag to this code.
The function already mixes concerns and this change makes it a bit more
difficult to follow. I'd suggest that you first factor out into a
separate function the parts that allocate a suitable range from
cpu_possible_mask, and only then introduce the behavior change
constraining the results.
> /* If we get here, it most likely means that NR_CPUS is
> * less than the partition's max processors setting.
> */
> @@ -191,12 +225,36 @@ static int pseries_add_processor(struct device_node *np)
> cpumask_shift_left(tmp, tmp, nthreads);
>
> if (cpumask_empty(tmp)) {
> + if (!force_reusing) {
> + force_reusing = true;
> + goto again;
> + }
> printk(KERN_ERR "Unable to find space in cpu_present_mask for"
> " processor %pOFn with %d thread(s)\n", np,
> nthreads);
> goto out_unlock;
> }
>
> + /* Record the newly used CPU ids for the associate node. */
> + cpumask_or(node_recorded_ids_map[nid], node_recorded_ids_map[nid], tmp);
> +
> + /*
> + * If we force reusing the id, remove these ids from any node which was
> + * previously using it.
> + */
> + if (force_reusing) {
> + cpu = cpumask_first(tmp);
> + pr_warn("Reusing free CPU ids %d-%d from another node\n",
> + cpu, cpu + nthreads - 1);
> +
> + for_each_online_node(node) {
> + if (node == nid)
> + continue;
> + cpumask_andnot(node_recorded_ids_map[node],
> + node_recorded_ids_map[node], tmp);
> + }
> + }
> +
I don't know, should we not fail the request instead of doing the
ABI-breaking thing the code in this change is trying to prevent? I don't
think a warning in the kernel log is going to help any application that
would be affected by this.
> for_each_cpu(cpu, tmp) {
> BUG_ON(cpu_present(cpu));
> set_cpu_present(cpu, true);
> @@ -889,6 +947,7 @@ static struct notifier_block pseries_smp_nb = {
> static int __init pseries_cpu_hotplug_init(void)
> {
> int qcss_tok;
> + unsigned int node;
>
> #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
> ppc_md.cpu_probe = dlpar_cpu_probe;
> @@ -910,8 +969,18 @@ static int __init pseries_cpu_hotplug_init(void)
> smp_ops->cpu_die = pseries_cpu_die;
>
> /* Processors can be added/removed only on LPAR */
> - if (firmware_has_feature(FW_FEATURE_LPAR))
> + if (firmware_has_feature(FW_FEATURE_LPAR)) {
> + for_each_node(node) {
> + alloc_bootmem_cpumask_var(&node_recorded_ids_map[node]);
> +
> + /* Record ids of CPU added at boot time */
> + cpumask_or(node_recorded_ids_map[node],
> + node_recorded_ids_map[node],
> + node_to_cpumask_map[node]);
> + }
> +
> of_reconfig_notifier_register(&pseries_smp_nb);
> + }
This looks OK.
^ permalink raw reply
* Re: [PATCH v2] pseries: prevent free CPU ids to be reused on another node
From: Laurent Dufour @ 2021-04-02 14:42 UTC (permalink / raw)
To: Nathan Lynch
Cc: Srikar Dronamraju, cheloha, linux-kernel, paulus, linuxppc-dev
In-Reply-To: <87a6qgbyk6.fsf@linux.ibm.com>
Thanks Nathan for reviewing this.
Le 02/04/2021 à 15:34, Nathan Lynch a écrit :
> Hi Laurent,
>
> Laurent Dufour <ldufour@linux.ibm.com> writes:
>> When a CPU is hot added, the CPU ids are taken from the available mask from
>> the lower possible set. If that set of values was previously used for CPU
>> attached to a different node, this seems to application like if these CPUs
>> have migrated from a node to another one which is not expected in real
>> life.
>
> This seems like a problem that could affect other architectures or
> platforms? I guess as long as arch code is responsible for placing new
> CPUs in cpu_present_mask, that code will have the responsibility of
> ensuring CPU IDs' NUMA assignments remain stable.
Actually, x86 is already handling this issue in the arch code specific code, see
8f54969dc8d6 ("x86/acpi: Introduce persistent storage for cpuid <-> apicid
mapping"). I didn't check for other architectures but as CPU id allocation is in
the arch part, I believe this is up to each arch to deal with this issue.
Making the CPU id allocation common to all arch is outside the scope of this patch.
> [...]
>
>> The effect of this patch can be seen by removing and adding CPUs using the
>> Qemu monitor. In the following case, the first CPU from the node 2 is
>> removed, then the first one from the node 1 is removed too. Later, the
>> first CPU of the node 2 is added back. Without that patch, the kernel will
>> numbered these CPUs using the first CPU ids available which are the ones
>> freed when removing the second CPU of the node 0. This leads to the CPU ids
>> 16-23 to move from the node 1 to the node 2. With the patch applied, the
>> CPU ids 32-39 are used since they are the lowest free ones which have not
>> been used on another node.
>>
>> At boot time:
>> [root@vm40 ~]# numactl -H | grep cpus
>> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
>> node 1 cpus: 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31
>> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
>>
>> Vanilla kernel, after the CPU hot unplug/plug operations:
>> [root@vm40 ~]# numactl -H | grep cpus
>> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
>> node 1 cpus: 24 25 26 27 28 29 30 31
>> node 2 cpus: 16 17 18 19 20 21 22 23 40 41 42 43 44 45 46 47
>>
>> Patched kernel, after the CPU hot unplug/plug operations:
>> [root@vm40 ~]# numactl -H | grep cpus
>> node 0 cpus: 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
>> node 1 cpus: 24 25 26 27 28 29 30 31
>> node 2 cpus: 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47
>
> Good demonstration of the problem. CPUs 16-23 "move" from node 1 to
> node 2.
Thanks
>
>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index 12cbffd3c2e3..48c7943b25b0 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -39,6 +39,8 @@
>> /* This version can't take the spinlock, because it never returns */
>> static int rtas_stop_self_token = RTAS_UNKNOWN_SERVICE;
>>
>> +static cpumask_var_t node_recorded_ids_map[MAX_NUMNODES];
>
> I guess this should have documentation that it must be
> accessed/manipulated with cpu_add_remove_lock held?
I'll add a comment before the declaration to make this clear.
>
>> +
>> static void rtas_stop_self(void)
>> {
>> static struct rtas_args args;
>> @@ -151,29 +153,61 @@ static void pseries_cpu_die(unsigned int cpu)
>> */
>> static int pseries_add_processor(struct device_node *np)
>> {
>> - unsigned int cpu;
>> + unsigned int cpu, node;
>> cpumask_var_t candidate_mask, tmp;
>> - int err = -ENOSPC, len, nthreads, i;
>> + int err = -ENOSPC, len, nthreads, i, nid;
>
> From eight local vars to ten, and the two new variables' names are
> "node" and "nid". More distinctive names would help readers.
I agree that's confusing, I'll do some cleanup.
>
>
>> const __be32 *intserv;
>> + bool force_reusing = false;
>>
>> intserv = of_get_property(np, "ibm,ppc-interrupt-server#s", &len);
>> if (!intserv)
>> return 0;
>>
>> - zalloc_cpumask_var(&candidate_mask, GFP_KERNEL);
>> - zalloc_cpumask_var(&tmp, GFP_KERNEL);
>> + alloc_cpumask_var(&candidate_mask, GFP_KERNEL);
>> + alloc_cpumask_var(&tmp, GFP_KERNEL);
>> +
>> + /*
>> + * Fetch from the DT nodes read by dlpar_configure_connector() the NUMA
>> + * node id the added CPU belongs to.
>> + */
>> + nid = of_node_to_nid(np);
>> + if (nid < 0 || !node_possible(nid))
>> + nid = first_online_node;
>>
>> nthreads = len / sizeof(u32);
>> - for (i = 0; i < nthreads; i++)
>> - cpumask_set_cpu(i, tmp);
>>
>> cpu_maps_update_begin();
>>
>> BUG_ON(!cpumask_subset(cpu_present_mask, cpu_possible_mask));
>>
>> +again:
>> + cpumask_clear(candidate_mask);
>> + cpumask_clear(tmp);
>> + for (i = 0; i < nthreads; i++)
>> + cpumask_set_cpu(i, tmp);
>> +
>> /* Get a bitmap of unoccupied slots. */
>> cpumask_xor(candidate_mask, cpu_possible_mask, cpu_present_mask);
>> +
>> + /*
>> + * Remove free ids previously assigned on the other nodes. We can walk
>> + * only online nodes because once a node became online it is not turned
>> + * offlined back.
>> + */
>> + if (!force_reusing)
>> + for_each_online_node(node) {
>> + if (node == nid) /* Keep our node's recorded ids */
>> + continue;
>> + cpumask_andnot(candidate_mask, candidate_mask,
>> + node_recorded_ids_map[node]);
>> + }
>> +
>> if (cpumask_empty(candidate_mask)) {
>> + if (!force_reusing) {
>> + force_reusing = true;
>> + goto again;
>> + }
>> +
>
> Hmm I'd encourage you to work toward a solution that doesn't involve
> adding backwards jumps and a bool flag to this code.
>
> The function already mixes concerns and this change makes it a bit more
> difficult to follow. I'd suggest that you first factor out into a
> separate function the parts that allocate a suitable range from
> cpu_possible_mask, and only then introduce the behavior change
> constraining the results.
Fair enough, I'll try to factor some part of the code and avoid a backward jumps.
>
>> /* If we get here, it most likely means that NR_CPUS is
>> * less than the partition's max processors setting.
>> */
>> @@ -191,12 +225,36 @@ static int pseries_add_processor(struct device_node *np)
>> cpumask_shift_left(tmp, tmp, nthreads);
>>
>> if (cpumask_empty(tmp)) {
>> + if (!force_reusing) {
>> + force_reusing = true;
>> + goto again;
>> + }
>> printk(KERN_ERR "Unable to find space in cpu_present_mask for"
>> " processor %pOFn with %d thread(s)\n", np,
>> nthreads);
>> goto out_unlock;
>> }
>>
>> + /* Record the newly used CPU ids for the associate node. */
>> + cpumask_or(node_recorded_ids_map[nid], node_recorded_ids_map[nid], tmp);
>> +
>> + /*
>> + * If we force reusing the id, remove these ids from any node which was
>> + * previously using it.
>> + */
>> + if (force_reusing) {
>> + cpu = cpumask_first(tmp);
>> + pr_warn("Reusing free CPU ids %d-%d from another node\n",
>> + cpu, cpu + nthreads - 1);
>> +
>> + for_each_online_node(node) {
>> + if (node == nid)
>> + continue;
>> + cpumask_andnot(node_recorded_ids_map[node],
>> + node_recorded_ids_map[node], tmp);
>> + }
>> + }
>> +
>
> I don't know, should we not fail the request instead of doing the
> ABI-breaking thing the code in this change is trying to prevent? I don't
> think a warning in the kernel log is going to help any application that
> would be affected by this.
That's a really good question. One should argue that the most important is to
satisfy the CPU add operation, assuming that only few are interested in the CPU
numbering, while others would prefer the CPU adding to fail (which may prevent
adding CPUs on another nodes if the whole operation is aborted as soon as a CPU
add is failing).
I was conservative here, but if failing the operation is the best option, then
this will make that code simpler, removing the backward jump.
Who is deciding?
>
>
>> for_each_cpu(cpu, tmp) {
>> BUG_ON(cpu_present(cpu));
>> set_cpu_present(cpu, true);
>> @@ -889,6 +947,7 @@ static struct notifier_block pseries_smp_nb = {
>> static int __init pseries_cpu_hotplug_init(void)
>> {
>> int qcss_tok;
>> + unsigned int node;
>>
>> #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
>> ppc_md.cpu_probe = dlpar_cpu_probe;
>> @@ -910,8 +969,18 @@ static int __init pseries_cpu_hotplug_init(void)
>> smp_ops->cpu_die = pseries_cpu_die;
>>
>> /* Processors can be added/removed only on LPAR */
>> - if (firmware_has_feature(FW_FEATURE_LPAR))
>> + if (firmware_has_feature(FW_FEATURE_LPAR)) {
>> + for_each_node(node) {
>> + alloc_bootmem_cpumask_var(&node_recorded_ids_map[node]);
>> +
>> + /* Record ids of CPU added at boot time */
>> + cpumask_or(node_recorded_ids_map[node],
>> + node_recorded_ids_map[node],
>> + node_to_cpumask_map[node]);
>> + }
>> +
>> of_reconfig_notifier_register(&pseries_smp_nb);
>> + }
>
> This looks OK.
>
^ permalink raw reply
* [PATCH v4 02/20] drivers: of: use cmdline building function
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
In-Reply-To: <cover.1617375802.git.christophe.leroy@csgroup.eu>
This patch uses the new cmdline building function to
concatenate the of provided cmdline with built-in parts
based on compile-time options.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
drivers/of/fdt.c | 23 ++++-------------------
1 file changed, 4 insertions(+), 19 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index dcc1dd96911a..7c5e9fb6039b 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -25,6 +25,7 @@
#include <linux/serial_core.h>
#include <linux/sysfs.h>
#include <linux/random.h>
+#include <linux/cmdline.h>
#include <asm/setup.h> /* for COMMAND_LINE_SIZE */
#include <asm/page.h>
@@ -1050,26 +1051,10 @@ int __init early_init_dt_scan_chosen(unsigned long node, const char *uname,
/* Retrieve command line */
p = of_get_flat_dt_prop(node, "bootargs", &l);
- if (p != NULL && l > 0)
- strlcpy(data, p, min(l, COMMAND_LINE_SIZE));
+ if (l <= 0)
+ p = NULL;
- /*
- * CONFIG_CMDLINE is meant to be a default in case nothing else
- * managed to set the command line, unless CONFIG_CMDLINE_FORCE
- * is set in which case we override whatever was found earlier.
- */
-#ifdef CONFIG_CMDLINE
-#if defined(CONFIG_CMDLINE_EXTEND)
- strlcat(data, " ", COMMAND_LINE_SIZE);
- strlcat(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#elif defined(CONFIG_CMDLINE_FORCE)
- strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#else
- /* No arguments from boot loader, use kernel's cmdl*/
- if (!((char *)data)[0])
- strlcpy(data, CONFIG_CMDLINE, COMMAND_LINE_SIZE);
-#endif
-#endif /* CONFIG_CMDLINE */
+ cmdline_build(data, p);
pr_debug("Command line is: %s\n", (char *)data);
--
2.25.0
^ permalink raw reply related
* [PATCH v4 00/20] Implement GENERIC_CMDLINE
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
The purpose of this series is to improve and enhance the
handling of kernel boot arguments.
Current situation is that most if not all architectures are using
similar options to do some manupulation on command line arguments:
- Prepend built-in arguments in front of bootloader provided arguments
- Append built-in arguments after bootloader provided arguments
- Replace bootloader provided arguments by built-in arguments
- Use built-in arguments when none is provided by bootloader.
On some architectures, all the options are possible. On other ones,
only a subset are available.
The purpose of this series is to refactor and enhance the
handling of kernel boot arguments so that every architecture can
benefit from all possibilities.
It is first focussed on powerpc but also extends the capability
for other arches.
The work has been focussed on minimising the churn in architectures
by keeping the most commonly used namings.
Main changes in V4:
- Included patch from Daniel to replace powerpc's strcpy() by strlcpy()
- Using strlcpy() instead of zeroing first char + strlcat() (idea taken frm Daniel's series)
- Reworked the convertion of EFI which was wrong in V3
- Added "too long" command line handling
- Changed cmdline macro into a function
- Done a few fixes in arch (NIOS2, SH, ARM)
- Taken comments into account (see individual responses for details)
- Tested on powerpc, build tested on ARM64, X86_64.
Main changes in V3:
- Also accept destination equal to source in cmdline_build() by setting a tmp buffer in __initdata. Powerpc provides different source and destination and call __cmdline_build() directly.
- Taken comments received from Will and Rob
- Converted all architectures (Only tested on powerpc)
Christophe Leroy (19):
cmdline: Add generic function to build command line.
drivers: of: use cmdline building function
x86/efi: Replace CONFIG_CMDLINE_OVERRIDE by CONFIG_CMDLINE_FORCE
drivers: firmware: efi: use cmdline building function
cmdline: Gives architectures opportunity to use generically defined
boot cmdline manipulation
powerpc: Convert to GENERIC_CMDLINE
arm: Convert to GENERIC_CMDLINE
arm64: Convert to GENERIC_CMDLINE
hexagon: Convert to GENERIC_CMDLINE
microblaze: Convert to GENERIC_CMDLINE
nios2: Convert to GENERIC_CMDLINE
openrisc: Convert to GENERIC_CMDLINE
riscv: Convert to GENERIC_CMDLINE
sh: Convert to GENERIC_CMDLINE
sparc: Convert to GENERIC_CMDLINE
xtensa: Convert to GENERIC_CMDLINE
x86: Convert to GENERIC_CMDLINE
mips: Convert to GENERIC_CMDLINE
cmdline: Remove CONFIG_CMDLINE_EXTEND
Daniel Walker (1):
powerpc: convert strcpy to strlcpy in prom_init
arch/arm/Kconfig | 38 +--------
arch/arm/kernel/atags_parse.c | 13 +--
arch/arm64/Kconfig | 33 +-------
arch/arm64/kernel/idreg-override.c | 9 +--
arch/hexagon/Kconfig | 11 +--
arch/hexagon/kernel/setup.c | 10 +--
arch/microblaze/Kconfig | 24 +-----
arch/microblaze/configs/mmu_defconfig | 2 +-
arch/microblaze/kernel/head.S | 4 +-
arch/mips/Kconfig | 1 +
arch/mips/Kconfig.debug | 44 -----------
arch/mips/configs/ar7_defconfig | 1 -
arch/mips/configs/bcm47xx_defconfig | 1 -
arch/mips/configs/bcm63xx_defconfig | 1 -
arch/mips/configs/bmips_be_defconfig | 1 -
arch/mips/configs/bmips_stb_defconfig | 1 -
arch/mips/configs/capcella_defconfig | 1 -
arch/mips/configs/ci20_defconfig | 1 -
arch/mips/configs/cu1000-neo_defconfig | 1 -
arch/mips/configs/cu1830-neo_defconfig | 1 -
arch/mips/configs/e55_defconfig | 1 -
arch/mips/configs/generic_defconfig | 1 -
arch/mips/configs/gpr_defconfig | 1 -
arch/mips/configs/loongson3_defconfig | 1 -
arch/mips/configs/mpc30x_defconfig | 1 -
arch/mips/configs/rt305x_defconfig | 1 -
arch/mips/configs/tb0219_defconfig | 1 -
arch/mips/configs/tb0226_defconfig | 1 -
arch/mips/configs/tb0287_defconfig | 1 -
arch/mips/configs/workpad_defconfig | 1 -
arch/mips/configs/xway_defconfig | 1 -
arch/mips/kernel/relocate.c | 4 +-
arch/mips/kernel/setup.c | 40 +---------
arch/mips/pic32/pic32mzda/early_console.c | 2 +-
arch/mips/pic32/pic32mzda/init.c | 2 -
arch/nios2/Kconfig | 25 +-----
arch/nios2/kernel/setup.c | 13 +--
arch/openrisc/Kconfig | 10 +--
arch/powerpc/Kconfig | 37 +--------
arch/powerpc/kernel/prom_init.c | 46 ++++++-----
arch/riscv/Kconfig | 44 +----------
arch/riscv/kernel/setup.c | 7 +-
arch/sh/Kconfig | 28 +------
arch/sh/configs/ap325rxa_defconfig | 2 +-
arch/sh/configs/dreamcast_defconfig | 2 +-
arch/sh/configs/ecovec24-romimage_defconfig | 2 +-
arch/sh/configs/ecovec24_defconfig | 2 +-
arch/sh/configs/edosk7760_defconfig | 2 +-
arch/sh/configs/espt_defconfig | 2 +-
arch/sh/configs/j2_defconfig | 2 +-
arch/sh/configs/kfr2r09-romimage_defconfig | 2 +-
arch/sh/configs/kfr2r09_defconfig | 2 +-
arch/sh/configs/lboxre2_defconfig | 2 +-
arch/sh/configs/microdev_defconfig | 2 +-
arch/sh/configs/migor_defconfig | 2 +-
arch/sh/configs/polaris_defconfig | 2 +-
arch/sh/configs/r7780mp_defconfig | 2 +-
arch/sh/configs/r7785rp_defconfig | 2 +-
arch/sh/configs/rsk7201_defconfig | 2 +-
arch/sh/configs/rsk7203_defconfig | 2 +-
arch/sh/configs/rts7751r2d1_defconfig | 2 +-
arch/sh/configs/rts7751r2dplus_defconfig | 2 +-
arch/sh/configs/sdk7780_defconfig | 2 +-
arch/sh/configs/sdk7786_defconfig | 2 +-
arch/sh/configs/se7206_defconfig | 2 +-
arch/sh/configs/se7343_defconfig | 2 +-
arch/sh/configs/se7712_defconfig | 2 +-
arch/sh/configs/se7721_defconfig | 2 +-
arch/sh/configs/se7724_defconfig | 2 +-
arch/sh/configs/se7751_defconfig | 2 +-
arch/sh/configs/se7780_defconfig | 2 +-
arch/sh/configs/sh03_defconfig | 2 +-
arch/sh/configs/sh2007_defconfig | 2 +-
arch/sh/configs/sh7757lcr_defconfig | 2 +-
arch/sh/configs/sh7763rdp_defconfig | 2 +-
arch/sh/configs/shmin_defconfig | 2 +-
arch/sh/configs/shx3_defconfig | 2 +-
arch/sh/configs/titan_defconfig | 2 +-
arch/sh/configs/ul2_defconfig | 2 +-
arch/sh/kernel/setup.c | 11 +--
arch/sparc/Kconfig | 18 +----
arch/sparc/prom/bootstr_64.c | 2 +-
arch/x86/Kconfig | 45 +----------
arch/x86/kernel/setup.c | 17 +---
arch/xtensa/Kconfig | 15 +---
arch/xtensa/configs/audio_kc705_defconfig | 1 -
arch/xtensa/configs/common_defconfig | 1 -
arch/xtensa/configs/generic_kc705_defconfig | 1 -
arch/xtensa/configs/iss_defconfig | 1 -
arch/xtensa/configs/nommu_kc705_defconfig | 1 -
arch/xtensa/configs/smp_lx200_defconfig | 1 -
arch/xtensa/configs/virt_defconfig | 1 -
arch/xtensa/configs/xip_kc705_defconfig | 1 -
arch/xtensa/kernel/setup.c | 10 +--
.../firmware/efi/libstub/efi-stub-helper.c | 35 ++++----
drivers/firmware/efi/libstub/efi-stub.c | 23 ++----
drivers/firmware/efi/libstub/efistub.h | 2 +-
drivers/firmware/efi/libstub/x86-stub.c | 18 +----
drivers/of/fdt.c | 23 +-----
include/linux/cmdline.h | 79 +++++++++++++++++++
init/Kconfig | 46 +++++++++++
usr/Kconfig | 2 +-
102 files changed, 265 insertions(+), 628 deletions(-)
create mode 100644 include/linux/cmdline.h
--
2.25.0
^ permalink raw reply
* [PATCH v4 03/20] x86/efi: Replace CONFIG_CMDLINE_OVERRIDE by CONFIG_CMDLINE_FORCE
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
In-Reply-To: <cover.1617375802.git.christophe.leroy@csgroup.eu>
During boot, x86 uses EFI driver. That driver is also used
by ARM.
In order to refactor the command line processing in that driver
in a following patch, rename CONFIG_CMDLINE_OVERRIDE by
CONFIG_CMDLINE_FORCE on the x86 in order to be similar
to ARM (and most other architectures).
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v4: New
---
arch/x86/Kconfig | 4 ++--
arch/x86/kernel/setup.c | 2 +-
drivers/firmware/efi/libstub/x86-stub.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2792879d398e..a20684d56b4b 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2384,14 +2384,14 @@ config CMDLINE
command line at boot time, it is appended to this string to
form the full kernel command line, when the system boots.
- However, you can use the CONFIG_CMDLINE_OVERRIDE option to
+ However, you can use the CONFIG_CMDLINE_FORCE option to
change this behavior.
In most cases, the command line (whether built-in or provided
by the boot loader) should specify the device for the root
file system.
-config CMDLINE_OVERRIDE
+config CMDLINE_FORCE
bool "Built-in command line overrides boot loader arguments"
depends on CMDLINE_BOOL && CMDLINE != ""
help
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index d883176ef2ce..6f2de58eeb54 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -884,7 +884,7 @@ void __init setup_arch(char **cmdline_p)
bss_resource.end = __pa_symbol(__bss_stop)-1;
#ifdef CONFIG_CMDLINE_BOOL
-#ifdef CONFIG_CMDLINE_OVERRIDE
+#ifdef CONFIG_CMDLINE_FORCE
strlcpy(boot_command_line, builtin_cmdline, COMMAND_LINE_SIZE);
#else
if (builtin_cmdline[0]) {
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index f14c4ff5839f..28659276b6ba 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -742,7 +742,7 @@ unsigned long efi_main(efi_handle_t handle,
goto fail;
}
#endif
- if (!IS_ENABLED(CONFIG_CMDLINE_OVERRIDE)) {
+ if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) {
unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
((u64)boot_params->ext_cmd_line_ptr << 32));
status = efi_parse_options((char *)cmdline_paddr);
--
2.25.0
^ permalink raw reply related
* [PATCH v4 04/20] drivers: firmware: efi: use cmdline building function
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
In-Reply-To: <cover.1617375802.git.christophe.leroy@csgroup.eu>
Use cmdline building function in EFI.
EFI cannot use pr_err() and doesn't have a .init.data section,
so use the __cmdline_build() internal function and provides
both a source and a destination.
Remove the handling of too long command lines as it is handled
by the generic CMDLINE.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v4: New
---
.../firmware/efi/libstub/efi-stub-helper.c | 35 ++++++++++---------
drivers/firmware/efi/libstub/efi-stub.c | 23 +++---------
drivers/firmware/efi/libstub/efistub.h | 2 +-
drivers/firmware/efi/libstub/x86-stub.c | 18 +++-------
4 files changed, 29 insertions(+), 49 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index aa8da0a49829..9f60d471d650 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -13,6 +13,7 @@
#include <linux/efi.h>
#include <linux/kernel.h>
#include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */
+#include <linux/cmdline.h>
#include <asm/efi.h>
#include <asm/setup.h>
@@ -339,13 +340,13 @@ void efi_apply_loadoptions_quirk(const void **load_options, int *load_options_si
* Size of memory allocated return in *cmd_line_len.
* Returns NULL on error.
*/
-char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
+char *efi_convert_cmdline(efi_loaded_image_t *image)
{
const u16 *s2;
- unsigned long cmdline_addr = 0;
+ unsigned long cmdline_addr = 0, options_addr = 0;
int options_chars = efi_table_attr(image, load_options_size);
const u16 *options = efi_table_attr(image, load_options);
- int options_bytes = 0, safe_options_bytes = 0; /* UTF-8 bytes */
+ int options_bytes = 0; /* UTF-8 bytes */
bool in_quote = false;
efi_status_t status;
@@ -354,16 +355,12 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
if (options) {
s2 = options;
- while (options_bytes < COMMAND_LINE_SIZE && options_chars--) {
+ while (options_chars--) {
u16 c = *s2++;
if (c < 0x80) {
if (c == L'\0' || c == L'\n')
break;
- if (c == L'"')
- in_quote = !in_quote;
- else if (!in_quote && isspace((char)c))
- safe_options_bytes = options_bytes;
options_bytes++;
continue;
@@ -395,24 +392,30 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
}
}
}
- if (options_bytes >= COMMAND_LINE_SIZE) {
- options_bytes = safe_options_bytes;
- efi_err("Command line is too long: truncated to %d bytes\n",
- options_bytes);
- }
}
options_bytes++; /* NUL termination */
status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, options_bytes,
- (void **)&cmdline_addr);
+ (void **)&options_addr);
if (status != EFI_SUCCESS)
return NULL;
- snprintf((char *)cmdline_addr, options_bytes, "%.*ls",
+ snprintf((char *)options_addr, options_bytes, "%.*ls",
options_bytes - 1, options);
- *cmd_line_len = options_bytes;
+ status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, COMMAND_LINE_SIZE,
+ (void **)&cmdline_addr);
+ if (status != EFI_SUCCESS) {
+ efi_bs_call(free_pool, (void *)options_addr);
+ return NULL;
+ }
+
+ if (!__cmdline_build((char *)cmdline_addr, (char *)options_addr))
+ efi_err("Command line is too long\n");
+
+ efi_bs_call(free_pool, (void *)cmdline_addr);
+
return (char *)cmdline_addr;
}
diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index 26e69788f27a..b79ee76f2e95 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -127,7 +127,6 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
unsigned long fdt_addr = 0; /* Original DTB */
unsigned long fdt_size = 0;
char *cmdline_ptr = NULL;
- int cmdline_size = 0;
efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
unsigned long reserve_addr = 0;
unsigned long reserve_size = 0;
@@ -165,29 +164,17 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
* protocol. We are going to copy the command line into the
* device tree, so this can be allocated anywhere.
*/
- cmdline_ptr = efi_convert_cmdline(image, &cmdline_size);
+ cmdline_ptr = efi_convert_cmdline(image);
if (!cmdline_ptr) {
efi_err("getting command line via LOADED_IMAGE_PROTOCOL\n");
status = EFI_OUT_OF_RESOURCES;
goto fail;
}
- if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) ||
- IS_ENABLED(CONFIG_CMDLINE_FORCE) ||
- cmdline_size == 0) {
- status = efi_parse_options(CONFIG_CMDLINE);
- if (status != EFI_SUCCESS) {
- efi_err("Failed to parse options\n");
- goto fail_free_cmdline;
- }
- }
-
- if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && cmdline_size > 0) {
- status = efi_parse_options(cmdline_ptr);
- if (status != EFI_SUCCESS) {
- efi_err("Failed to parse options\n");
- goto fail_free_cmdline;
- }
+ status = efi_parse_options(cmdline_ptr);
+ if (status != EFI_SUCCESS) {
+ efi_err("Failed to parse options\n");
+ goto fail_free_cmdline;
}
efi_info("Booting Linux Kernel...\n");
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index cde0a2ef507d..2f872c48b20b 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -780,7 +780,7 @@ void efi_free(unsigned long size, unsigned long addr);
void efi_apply_loadoptions_quirk(const void **load_options, int *load_options_size);
-char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len);
+char *efi_convert_cmdline(efi_loaded_image_t *image);
efi_status_t efi_get_memory_map(struct efi_boot_memmap *map);
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 28659276b6ba..0f6a33149ef7 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -359,7 +359,6 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
struct setup_header *hdr;
void *image_base;
efi_guid_t proto = LOADED_IMAGE_PROTOCOL_GUID;
- int options_size = 0;
efi_status_t status;
char *cmdline_ptr;
@@ -404,7 +403,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
hdr->type_of_loader = 0x21;
/* Convert unicode cmdline to ascii */
- cmdline_ptr = efi_convert_cmdline(image, &options_size);
+ cmdline_ptr = efi_convert_cmdline(image);
if (!cmdline_ptr)
goto fail;
@@ -674,6 +673,8 @@ unsigned long efi_main(efi_handle_t handle,
unsigned long buffer_start, buffer_end;
struct setup_header *hdr = &boot_params->hdr;
efi_status_t status;
+ unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
+ ((u64)boot_params->ext_cmd_line_ptr << 32));
efi_system_table = sys_table_arg;
@@ -735,22 +736,11 @@ unsigned long efi_main(efi_handle_t handle,
image_offset = 0;
}
-#ifdef CONFIG_CMDLINE_BOOL
- status = efi_parse_options(CONFIG_CMDLINE);
+ status = efi_parse_options((char *)cmdline_paddr);
if (status != EFI_SUCCESS) {
efi_err("Failed to parse options\n");
goto fail;
}
-#endif
- if (!IS_ENABLED(CONFIG_CMDLINE_FORCE)) {
- unsigned long cmdline_paddr = ((u64)hdr->cmd_line_ptr |
- ((u64)boot_params->ext_cmd_line_ptr << 32));
- status = efi_parse_options((char *)cmdline_paddr);
- if (status != EFI_SUCCESS) {
- efi_err("Failed to parse options\n");
- goto fail;
- }
- }
/*
* At this point, an initrd may already have been loaded by the
--
2.25.0
^ permalink raw reply related
* [PATCH v4 01/20] cmdline: Add generic function to build command line.
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
In-Reply-To: <cover.1617375802.git.christophe.leroy@csgroup.eu>
This code provides architectures with a way to build command line
based on what is built in the kernel and what is handed over by the
bootloader, based on selected compile-time options.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3:
- Addressed comments from Will
- Added capability to have src == dst
v4:
- Add cmdline_strlcpy()
- Removed cmdline_build() macro, __cmdline_build() becomes cmdline_build()
- Fixed the destination length to COMMAND_LINE_SIZE
- Truncate at a space not in a quote when too long
- Added a message when forcing the command line
---
include/linux/cmdline.h | 79 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
create mode 100644 include/linux/cmdline.h
diff --git a/include/linux/cmdline.h b/include/linux/cmdline.h
new file mode 100644
index 000000000000..a0773dc365c7
--- /dev/null
+++ b/include/linux/cmdline.h
@@ -0,0 +1,79 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_CMDLINE_H
+#define _LINUX_CMDLINE_H
+
+#include <linux/string.h>
+#include <linux/printk.h>
+#include <asm/setup.h>
+
+/* Allow users to override strlcat/strlcpy, powerpc can't use strings so early*/
+#ifndef cmdline_strlcat
+#define cmdline_strlcat strlcat
+#define cmdline_strlcpy strlcpy
+#endif
+
+/*
+ * This function will append or prepend a builtin command line to the command
+ * line provided by the bootloader. Kconfig options can be used to alter
+ * the behavior of this builtin command line.
+ * @dst: The destination of the final command line (Min. size COMMAND_LINE_SIZE)
+ * @src: The starting string or NULL if there isn't one. Must not equal dst.
+ * Returns: false if the string was truncated, true otherwise
+ */
+static __always_inline bool __cmdline_build(char *dst, const char *src)
+{
+ int len;
+ char *ptr, *last_space;
+ bool in_quote = false;
+
+ if (IS_ENABLED(CONFIG_CMDLINE_FORCE))
+ src = NULL;
+
+ dst[0] = 0;
+
+ if (IS_ENABLED(CONFIG_CMDLINE_PREPEND))
+ len = cmdline_strlcat(dst, CONFIG_CMDLINE " ", COMMAND_LINE_SIZE);
+
+ len = cmdline_strlcat(dst, src, COMMAND_LINE_SIZE);
+
+ if (IS_ENABLED(CONFIG_CMDLINE_EXTEND))
+ len = cmdline_strlcat(dst, " " CONFIG_CMDLINE, COMMAND_LINE_SIZE);
+
+ if (len < COMMAND_LINE_SIZE - 1)
+ return true;
+
+ for (ptr = dst; *ptr; ptr++) {
+ if (*ptr == '"')
+ in_quote = !in_quote;
+ if (*ptr == ' ' && !in_quote)
+ last_space = ptr;
+ }
+ if (last_space)
+ *last_space = 0;
+
+ return false;
+}
+
+/*
+ * This function will append or prepend a builtin command line to the command
+ * line provided by the bootloader. Kconfig options can be used to alter
+ * the behavior of this builtin command line.
+ * @dst: The destination of the final command line (Min. size COMMAND_LINE_SIZE)
+ * @src: The starting string or NULL if there isn't one.
+ */
+static __always_inline void cmdline_build(char *dst, const char *src)
+{
+ static char tmp[COMMAND_LINE_SIZE] __initdata;
+
+ if (src == dst) {
+ cmdline_strlcpy(tmp, src, COMMAND_LINE_SIZE);
+ src = tmp;
+ }
+ if (!__cmdline_build(dst, src))
+ pr_warn("Command line is too long, it has been truncated\n");
+
+ if (IS_ENABLED(CONFIG_CMDLINE_FORCE))
+ pr_info("Forcing kernel command line to: %s\n", dst);
+}
+
+#endif /* _LINUX_CMDLINE_H */
--
2.25.0
^ permalink raw reply related
* [PATCH v4 05/20] cmdline: Gives architectures opportunity to use generically defined boot cmdline manipulation
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
In-Reply-To: <cover.1617375802.git.christophe.leroy@csgroup.eu>
Most architectures have similar boot command line manipulation
options. This patchs adds the definition in init/Kconfig, gated by
CONFIG_HAVE_CMDLINE that the architectures can select to use them.
CONFIG_CMDLINE_EXTEND is understood differently by architectures.
For some of them it appends built-in CMDLINE to bootloader provided
line. For others it appends the bootloader provided CMDLINE to the
built-in one.
To avoid confusion, this commit brings to different options:
- CONFIG_CMDLINE_APPEND to append the built-in CMDLINE to the
bootloader line.
- CONFIG_CMDLINE_PREPEND to prepend the built-in CMDLINE in front of
the bootloader line.
For compatibility with existing architecture which uses CONFIG_OF, as
OF has already been converted to generic cmdline, we keep
CONFIG_CMDLINE_EXTEND as a synonym to CONFIG_CMDLINE_APPEND until
arm, powerpc, riscv and sh architectures have been converted.
A few differences are identified and will have to be taken
into account when converting the architecture to generic cmdline:
- riscv has CMDLINE_FALLBACK instead of CMDLINE_FROM_BOOTLOADER
- Some architectures are using CONFIG_CMDLINE_OVERRIDE or
CONFIG_CMDLINE_OVERWRITE instead of CONFIG_CMDLINE_FORCE.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v3:
- Comments from Will.
- Remove CONFIG_CMDLINE_BOOL. Using CONFIG_CMDLINE != "" instead, like arm and powerpc.
- Changed EXTEND to APPEND. Keep EXTEND for backward compatibility.
v4:
- Added CMDLINE_PREPEND as a dependency for INITRAMFS_FORCE
- Removed the EXTEND || APPEND in cmdline.h, keep EXTEND as it
will always exit and switch to APPEND in last patch.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
init/Kconfig | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
usr/Kconfig | 2 +-
2 files changed, 53 insertions(+), 1 deletion(-)
diff --git a/init/Kconfig b/init/Kconfig
index 5f5c776ef192..af0d84662cc2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -117,6 +117,58 @@ config INIT_ENV_ARG_LIMIT
Maximum of each of the number of arguments and environment
variables passed to init from the kernel command line.
+config GENERIC_CMDLINE
+ bool
+
+config CMDLINE
+ string "Default kernel command string" if GENERIC_CMDLINE
+ default ""
+ help
+ Defines a default kernel command string.
+ If this string is not empty, additional choices are proposed
+ below to determine how it will be used by the kernel.
+
+choice
+ prompt "Kernel command line type" if CMDLINE != ""
+ default CMDLINE_PREPEND if ARCH_WANT_CMDLINE_PREPEND_BY_DEFAULT
+ default CMDLINE_FROM_BOOTLOADER
+ depends on GENERIC_CMDLINE
+ help
+ Determine how the default kernel arguments are combined with any
+ arguments passed by the bootloader if any.
+
+config CMDLINE_FROM_BOOTLOADER
+ bool "Use bootloader kernel arguments if available"
+ help
+ Uses the command-line options passed by the boot loader. If
+ the boot loader doesn't provide any, the default kernel command
+ string provided in CMDLINE will be used.
+
+config CMDLINE_APPEND
+ bool "Append to the bootloader kernel arguments"
+ help
+ The default kernel command string will be appended to the
+ command-line arguments provided by the bootloader.
+
+config CMDLINE_PREPEND
+ bool "Prepend to the bootloader kernel arguments"
+ help
+ The default kernel command string will be prepended to the
+ command-line arguments provided by the bootloader.
+
+config CMDLINE_FORCE
+ bool "Always use the default kernel command string"
+ help
+ Always use the default kernel command string, ignoring any
+ arguments provided by the bootloader.
+endchoice
+
+config CMDLINE_EXTEND
+ bool
+ default CMDLINE_APPEND
+ help
+ To be removed once all architectures are converted to generic CMDLINE
+
config COMPILE_TEST
bool "Compile also drivers which will not load"
depends on HAS_IOMEM
diff --git a/usr/Kconfig b/usr/Kconfig
index 8bbcf699fe3b..b397e6b114d1 100644
--- a/usr/Kconfig
+++ b/usr/Kconfig
@@ -24,7 +24,7 @@ config INITRAMFS_SOURCE
config INITRAMFS_FORCE
bool "Ignore the initramfs passed by the bootloader"
- depends on CMDLINE_EXTEND || CMDLINE_FORCE
+ depends on CMDLINE_PREPEND || CMDLINE_APPEND || CMDLINE_FORCE
help
This option causes the kernel to ignore the initramfs image
(or initrd image) passed to it by the bootloader. This is
--
2.25.0
^ permalink raw reply related
* [PATCH v4 08/20] arm: Convert to GENERIC_CMDLINE
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
In-Reply-To: <cover.1617375802.git.christophe.leroy@csgroup.eu>
This converts the architecture to GENERIC_CMDLINE.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v4:
- Removed dependency to ATAGS
- Removed the message when forcing
---
arch/arm/Kconfig | 38 +----------------------------------
arch/arm/kernel/atags_parse.c | 13 +++---------
2 files changed, 4 insertions(+), 47 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 5da96f5df48f..1a1808b0eef7 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -50,6 +50,7 @@ config ARM
select GENERIC_ARCH_TOPOLOGY if ARM_CPU_TOPOLOGY
select GENERIC_ATOMIC64 if CPU_V7M || CPU_V6 || !CPU_32v6K || !AEABI
select GENERIC_CLOCKEVENTS_BROADCAST if SMP
+ select GENERIC_CMDLINE
select GENERIC_IRQ_IPI if SMP
select GENERIC_CPU_AUTOPROBE
select GENERIC_EARLY_IOREMAP
@@ -1740,43 +1741,6 @@ config ARM_ATAG_DTB_COMPAT_CMDLINE_EXTEND
endchoice
-config CMDLINE
- string "Default kernel command string"
- default ""
- help
- On some architectures (e.g. CATS), there is currently no way
- for the boot loader to pass arguments to the kernel. For these
- architectures, you should supply some command-line options at build
- time by entering them here. As a minimum, you should specify the
- memory size and the root device (e.g., mem=64M root=/dev/nfs).
-
-choice
- prompt "Kernel command line type" if CMDLINE != ""
- default CMDLINE_FROM_BOOTLOADER
- depends on ATAGS
-
-config CMDLINE_FROM_BOOTLOADER
- bool "Use bootloader kernel arguments if available"
- help
- Uses the command-line options passed by the boot loader. If
- the boot loader doesn't provide any, the default kernel command
- string provided in CMDLINE will be used.
-
-config CMDLINE_EXTEND
- bool "Extend bootloader kernel arguments"
- help
- The command-line arguments provided by the boot loader will be
- appended to the default kernel command string.
-
-config CMDLINE_FORCE
- bool "Always use the default kernel command string"
- help
- Always use the default kernel command string, even if the boot
- loader passes other arguments to the kernel.
- This is useful if you cannot or don't want to change the
- command-line options your boot loader passes to the kernel.
-endchoice
-
config XIP_KERNEL
bool "Kernel Execute-In-Place from ROM"
depends on !ARM_LPAE && !ARCH_MULTIPLATFORM
diff --git a/arch/arm/kernel/atags_parse.c b/arch/arm/kernel/atags_parse.c
index 373b61f9a4f0..ef97ec015c51 100644
--- a/arch/arm/kernel/atags_parse.c
+++ b/arch/arm/kernel/atags_parse.c
@@ -14,6 +14,7 @@
* is not parsed in any way).
*/
+#include <linux/cmdline.h>
#include <linux/init.h>
#include <linux/initrd.h>
#include <linux/kernel.h>
@@ -120,16 +121,8 @@ __tagtable(ATAG_REVISION, parse_tag_revision);
static int __init parse_tag_cmdline(const struct tag *tag)
{
-#if defined(CONFIG_CMDLINE_EXTEND)
- strlcat(default_command_line, " ", COMMAND_LINE_SIZE);
- strlcat(default_command_line, tag->u.cmdline.cmdline,
- COMMAND_LINE_SIZE);
-#elif defined(CONFIG_CMDLINE_FORCE)
- pr_warn("Ignoring tag cmdline (using the default kernel command line)\n");
-#else
- strlcpy(default_command_line, tag->u.cmdline.cmdline,
- COMMAND_LINE_SIZE);
-#endif
+ cmdline_build(default_command_line, tag->u.cmdline.cmdline);
+
return 0;
}
--
2.25.0
^ permalink raw reply related
* [PATCH v4 07/20] powerpc: Convert to GENERIC_CMDLINE
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
In-Reply-To: <cover.1617375802.git.christophe.leroy@csgroup.eu>
This updates the powerpc code to use the new cmdline building function.
The cmdline_build() defines a temporary string in __initdata.
powerpc uses __prombss instead at the moment, so we must
call cmdline_build() with destination different from the source
to allow GCC to optimise the temporary string out.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v4:
- Use cmdline_build() instead of __cmdline_build()
- Add cmdline_strlcpy() define
---
arch/powerpc/Kconfig | 37 +--------------------------------
arch/powerpc/kernel/prom_init.c | 21 +++++++++----------
2 files changed, 11 insertions(+), 47 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c1344c05226c..6723f10ac246 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -168,6 +168,7 @@ config PPC
select EDAC_SUPPORT
select GENERIC_ATOMIC64 if PPC32
select GENERIC_CLOCKEVENTS_BROADCAST if SMP
+ select GENERIC_CMDLINE
select GENERIC_CMOS_UPDATE
select GENERIC_CPU_AUTOPROBE
select GENERIC_CPU_VULNERABILITIES if PPC_BARRIER_NOSPEC
@@ -888,42 +889,6 @@ config PPC_DENORMALISATION
Add support for handling denormalisation of single precision
values. Useful for bare metal only. If unsure say Y here.
-config CMDLINE
- string "Initial kernel command string"
- default ""
- help
- On some platforms, there is currently no way for the boot loader to
- pass arguments to the kernel. For these platforms, you can supply
- some command-line options at build time by entering them here. In
- most cases you will need to specify the root device here.
-
-choice
- prompt "Kernel command line type" if CMDLINE != ""
- default CMDLINE_FROM_BOOTLOADER
-
-config CMDLINE_FROM_BOOTLOADER
- bool "Use bootloader kernel arguments if available"
- help
- Uses the command-line options passed by the boot loader. If
- the boot loader doesn't provide any, the default kernel command
- string provided in CMDLINE will be used.
-
-config CMDLINE_EXTEND
- bool "Extend bootloader kernel arguments"
- help
- The command-line arguments provided by the boot loader will be
- appended to the default kernel command string.
-
-config CMDLINE_FORCE
- bool "Always use the default kernel command string"
- help
- Always use the default kernel command string, even if the boot
- loader passes other arguments to the kernel.
- This is useful if you cannot or don't want to change the
- command-line options your boot loader passes to the kernel.
-
-endchoice
-
config EXTRA_TARGETS
string "Additional default image types"
help
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 33316ee55265..704afd028213 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -152,7 +152,7 @@ static struct prom_t __prombss prom;
static unsigned long __prombss prom_entry;
static char __prombss of_stdout_device[256];
-static char __prombss prom_scratch[256];
+static char __prombss prom_scratch[COMMAND_LINE_SIZE];
static unsigned long __prombss dt_header_start;
static unsigned long __prombss dt_struct_start, dt_struct_end;
@@ -770,24 +770,23 @@ static unsigned long prom_memparse(const char *ptr, const char **retptr)
* Early parsing of the command line passed to the kernel, used for
* "mem=x" and the options that affect the iommu
*/
+#define cmdline_strlcat prom_strlcat
+#define cmdline_strlcpy prom_strlcpy
+#include <linux/cmdline.h>
+
static void __init early_cmdline_parse(void)
{
const char *opt;
-
- char *p;
int l = 0;
-
- prom_cmd_line[0] = 0;
- p = prom_cmd_line;
+ bool truncated;
if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && (long)prom.chosen > 0)
- l = prom_getprop(prom.chosen, "bootargs", p, COMMAND_LINE_SIZE-1);
+ l = prom_getprop(prom.chosen, "bootargs", prom_scratch,
+ COMMAND_LINE_SIZE - 1);
- if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || l <= 0 || p[0] == '\0')
- prom_strlcat(prom_cmd_line, " " CONFIG_CMDLINE,
- sizeof(prom_cmd_line));
+ truncated = !__cmdline_build(prom_cmd_line, l > 0 ? prom_scratch : NULL);
- prom_printf("command line: %s\n", prom_cmd_line);
+ prom_printf("command line: %s %s\n", prom_cmd_line, truncated ? "[truncated]" : "");
#ifdef CONFIG_PPC64
opt = prom_strstr(prom_cmd_line, "iommu=");
--
2.25.0
^ permalink raw reply related
* [PATCH v4 06/20] powerpc: convert strcpy to strlcpy in prom_init
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
In-Reply-To: <cover.1617375802.git.christophe.leroy@csgroup.eu>
From: Daniel Walker <danielwa@cisco.com>
There's only two users of strcpy and one is the command
line handling. The generic command line handling uses strlcpy
and it makes sense to convert this one other user to strlcpy to
keep prom_init size consistent.
Cc: xe-linux-external@cisco.com
Signed-off-by: Daniel Walker <danielwa@cisco.com>
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/prom_init.c | 25 +++++++++++++++----------
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 41ed7e33d897..33316ee55265 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -242,15 +242,6 @@ static int __init prom_strcmp(const char *cs, const char *ct)
return 0;
}
-static char __init *prom_strcpy(char *dest, const char *src)
-{
- char *tmp = dest;
-
- while ((*dest++ = *src++) != '\0')
- /* nothing */;
- return tmp;
-}
-
static int __init prom_strncmp(const char *cs, const char *ct, size_t count)
{
unsigned char c1, c2;
@@ -276,6 +267,20 @@ static size_t __init prom_strlen(const char *s)
return sc - s;
}
+static size_t __init prom_strlcpy(char *dest, const char *src, size_t size)
+{
+ size_t ret = prom_strlen(src);
+
+ if (size) {
+ size_t len = (ret >= size) ? size - 1 : ret;
+
+ memcpy(dest, src, len);
+ dest[len] = '\0';
+ }
+ return ret;
+}
+
+
static int __init prom_memcmp(const void *cs, const void *ct, size_t count)
{
const unsigned char *su1, *su2;
@@ -2702,7 +2707,7 @@ static void __init flatten_device_tree(void)
/* Add "phandle" in there, we'll need it */
namep = make_room(&mem_start, &mem_end, 16, 1);
- prom_strcpy(namep, "phandle");
+ prom_strlcpy(namep, "phandle", 8);
mem_start = (unsigned long)namep + prom_strlen(namep) + 1;
/* Build string array */
--
2.25.0
^ permalink raw reply related
* [PATCH v4 09/20] arm64: Convert to GENERIC_CMDLINE
From: Christophe Leroy @ 2021-04-02 15:18 UTC (permalink / raw)
To: will, danielwa, robh, daniel, arnd, akpm
Cc: linux-arch, devicetree, microblaze, linux-xtensa, linux-sh,
linux-hexagon, x86, linux-kernel, nios2, linux-mips, linux-mm,
openrisc, sparclinux, linux-riscv, linuxppc-dev, linux-arm-kernel
In-Reply-To: <cover.1617375802.git.christophe.leroy@csgroup.eu>
This converts the architecture to GENERIC_CMDLINE.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/arm64/Kconfig | 33 +-----------------------------
arch/arm64/kernel/idreg-override.c | 9 ++++----
2 files changed, 5 insertions(+), 37 deletions(-)
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index e4e1b6550115..9781ba3758b1 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -105,6 +105,7 @@ config ARM64
select GENERIC_ALLOCATOR
select GENERIC_ARCH_TOPOLOGY
select GENERIC_CLOCKEVENTS_BROADCAST
+ select GENERIC_CMDLINE
select GENERIC_CPU_AUTOPROBE
select GENERIC_CPU_VULNERABILITIES
select GENERIC_EARLY_IOREMAP
@@ -1841,38 +1842,6 @@ config ARM64_ACPI_PARKING_PROTOCOL
protocol even if the corresponding data is present in the ACPI
MADT table.
-config CMDLINE
- string "Default kernel command string"
- default ""
- help
- Provide a set of default command-line options at build time by
- entering them here. As a minimum, you should specify the the
- root device (e.g. root=/dev/nfs).
-
-choice
- prompt "Kernel command line type" if CMDLINE != ""
- default CMDLINE_FROM_BOOTLOADER
- help
- Choose how the kernel will handle the provided default kernel
- command line string.
-
-config CMDLINE_FROM_BOOTLOADER
- bool "Use bootloader kernel arguments if available"
- help
- Uses the command-line options passed by the boot loader. If
- the boot loader doesn't provide any, the default kernel command
- string provided in CMDLINE will be used.
-
-config CMDLINE_FORCE
- bool "Always use the default kernel command string"
- help
- Always use the default kernel command string, even if the boot
- loader passes other arguments to the kernel.
- This is useful if you cannot or don't want to change the
- command-line options your boot loader passes to the kernel.
-
-endchoice
-
config EFI_STUB
bool
diff --git a/arch/arm64/kernel/idreg-override.c b/arch/arm64/kernel/idreg-override.c
index 83f1c4b92095..96a4e96a80a2 100644
--- a/arch/arm64/kernel/idreg-override.c
+++ b/arch/arm64/kernel/idreg-override.c
@@ -9,6 +9,7 @@
#include <linux/ctype.h>
#include <linux/kernel.h>
#include <linux/libfdt.h>
+#include <linux/cmdline.h>
#include <asm/cacheflush.h>
#include <asm/cpufeature.h>
@@ -187,12 +188,10 @@ static __init const u8 *get_bootargs_cmdline(void)
static __init void parse_cmdline(void)
{
const u8 *prop = get_bootargs_cmdline();
+ static char __initdata cmdline[COMMAND_LINE_SIZE];
- if (IS_ENABLED(CONFIG_CMDLINE_FORCE) || !prop)
- __parse_cmdline(CONFIG_CMDLINE, true);
-
- if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && prop)
- __parse_cmdline(prop, true);
+ cmdline_build(cmdline, prop);
+ __parse_cmdline(cmdline, true);
}
/* Keep checkers quiet */
--
2.25.0
^ permalink raw reply related
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