* [PATCH v5 01/48] KVM: PPC: Book3S HV: Nested move LPCR sanitising to sanitise_hv_regs
From: Nicholas Piggin @ 2021-04-01 15:02 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin, Fabiano Rosas
In-Reply-To: <20210401150325.442125-1-npiggin@gmail.com>
This will get a bit more complicated in future patches. Move it
into the helper function.
This change allows the L1 hypervisor to determine some of the LPCR
bits that the L0 is using to run it, which could be a privilege
violation (LPCR is HV-privileged), although the same problem exists
now for HFSCR for example. Discussion of the HV privilege issue is
ongoing and can be resolved with a later change.
Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kvm/book3s_hv_nested.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
index 0cd0e7aad588..3060e5deffc8 100644
--- a/arch/powerpc/kvm/book3s_hv_nested.c
+++ b/arch/powerpc/kvm/book3s_hv_nested.c
@@ -132,8 +132,27 @@ static void save_hv_return_state(struct kvm_vcpu *vcpu, int trap,
}
}
+/*
+ * This can result in some L0 HV register state being leaked to an L1
+ * hypervisor when the hv_guest_state is copied back to the guest after
+ * being modified here.
+ *
+ * There is no known problem with such a leak, and in many cases these
+ * register settings could be derived by the guest by observing behaviour
+ * and timing, interrupts, etc., but it is an issue to consider.
+ */
static void sanitise_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
{
+ struct kvmppc_vcore *vc = vcpu->arch.vcore;
+ u64 mask;
+
+ /*
+ * Don't let L1 change LPCR bits for the L2 except these:
+ */
+ mask = LPCR_DPFD | LPCR_ILE | LPCR_TC | LPCR_AIL | LPCR_LD |
+ LPCR_LPES | LPCR_MER;
+ hr->lpcr = (vc->lpcr & ~mask) | (hr->lpcr & mask);
+
/*
* Don't let L1 enable features for L2 which we've disabled for L1,
* but preserve the interrupt cause field.
@@ -271,8 +290,6 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
u64 hv_ptr, regs_ptr;
u64 hdec_exp;
s64 delta_purr, delta_spurr, delta_ic, delta_vtb;
- u64 mask;
- unsigned long lpcr;
if (vcpu->kvm->arch.l1_ptcr == 0)
return H_NOT_AVAILABLE;
@@ -321,9 +338,7 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
vcpu->arch.nested_vcpu_id = l2_hv.vcpu_token;
vcpu->arch.regs = l2_regs;
vcpu->arch.shregs.msr = vcpu->arch.regs.msr;
- mask = LPCR_DPFD | LPCR_ILE | LPCR_TC | LPCR_AIL | LPCR_LD |
- LPCR_LPES | LPCR_MER;
- lpcr = (vc->lpcr & ~mask) | (l2_hv.lpcr & mask);
+
sanitise_hv_regs(vcpu, &l2_hv);
restore_hv_regs(vcpu, &l2_hv);
@@ -335,7 +350,7 @@ long kvmhv_enter_nested_guest(struct kvm_vcpu *vcpu)
r = RESUME_HOST;
break;
}
- r = kvmhv_run_single_vcpu(vcpu, hdec_exp, lpcr);
+ r = kvmhv_run_single_vcpu(vcpu, hdec_exp, l2_hv.lpcr);
} while (is_kvmppc_resume_guest(r));
/* save L2 state for return */
--
2.23.0
^ permalink raw reply related
* [PATCH v5 00/48] KVM: PPC: Book3S: C-ify the P9 entry/exit code
From: Nicholas Piggin @ 2021-04-01 15:02 UTC (permalink / raw)
To: kvm-ppc; +Cc: linuxppc-dev, Nicholas Piggin
Git tree here
https://github.com/npiggin/linux/tree/kvm-in-c-v5
Now tested on P10 with hash host and radix host with hash and radix
guests with some basic kbuild stress testing.
Main changes since v4:
- Accounted for reviews so far. Comment and changelog fixes.
- Fix PR KVM register clobber [Paul]
- Fix HPT guest LPCR[TC] not set [Paul]
- Added comment about nested guest HV SPR leak issue [Paul]
- Improve misc comments and changelogs [Paul and Alexey]
- Fix decrementer_max export compile error [Alexey]
- Fixed issue with set_dec() not being identical to mtDEC causing
a patch to have an unintended change [Alexey]
- Re-add a patch to stop reflecting PR=1 sc 1, but only do it for radix.
Main changes since v3:
- Hopefully fixed LPCR sanising [from Fabiano review]
- Added MSR[HV] clearing of guest MSR (like MSR[ME] setting).
- Restored a lost comment about PPR saving, improved comments a bit
[Daniel review]
- Added isyncs, removed XXX isync comment.
- Fixed cede comment [Fabiano]
- In stop handling real mode handlers patch, fixed problem of nested
hcall handler calling cede and xics hcalls and consuming them
[Fabiano]
- Filter xics hcalls from being consumed if !kvmppc_xics_enabled
[Fabiano, Alexey noticed and Cedric provided the right recipe and
comment]
- Removed some more of radix, nested, and virt mode paths from the
P7/8 code [noticed by Alexey].
- Rebased on 5.12-rc4
- Hopefully I responded to all reviews and collected all reviewed-bys.
Thanks,
Nick
Nicholas Piggin (48):
KVM: PPC: Book3S HV: Nested move LPCR sanitising to sanitise_hv_regs
KVM: PPC: Book3S HV: Add a function to filter guest LPCR bits
KVM: PPC: Book3S HV: Disallow LPCR[AIL] to be set to 1 or 2
KVM: PPC: Book3S HV: Prevent radix guests setting LPCR[TC]
KVM: PPC: Book3S HV: Remove redundant mtspr PSPB
KVM: PPC: Book3S HV: remove unused kvmppc_h_protect argument
KVM: PPC: Book3S HV: Fix CONFIG_SPAPR_TCE_IOMMU=n default hcalls
powerpc/64s: Remove KVM handler support from CBE_RAS interrupts
powerpc/64s: remove KVM SKIP test from instruction breakpoint handler
KVM: PPC: Book3S HV: Ensure MSR[ME] is always set in guest MSR
KVM: PPC: Book3S HV: Ensure MSR[HV] is always clear in guest MSR
KVM: PPC: Book3S 64: move KVM interrupt entry to a common entry point
KVM: PPC: Book3S 64: Move GUEST_MODE_SKIP test into KVM
KVM: PPC: Book3S 64: add hcall interrupt handler
KVM: PPC: Book3S 64: Move hcall early register setup to KVM
KVM: PPC: Book3S 64: Move interrupt early register setup to KVM
KVM: PPC: Book3S 64: move bad_host_intr check to HV handler
KVM: PPC: Book3S 64: Minimise hcall handler calling convention
differences
KVM: PPC: Book3S HV P9: Move radix MMU switching instructions together
KVM: PPC: Book3S HV P9: implement kvmppc_xive_pull_vcpu in C
KVM: PPC: Book3S HV P9: Move xive vcpu context management into
kvmhv_p9_guest_entry
KVM: PPC: Book3S HV P9: Stop handling hcalls in real-mode in the P9
path
KVM: PPC: Book3S HV P9: Move setting HDEC after switching to guest
LPCR
KVM: PPC: Book3S HV P9: Use large decrementer for HDEC
KVM: PPC: Book3S HV P9: Use host timer accounting to avoid decrementer
read
KVM: PPC: Book3S HV P9: Reduce mftb per guest entry/exit
KVM: PPC: Book3S HV P9: Reduce irq_work vs guest decrementer races
KMV: PPC: Book3S HV: Use set_dec to set decrementer to host
powerpc: add set_dec_or_work API for safely updating decrementer
KVM: PPC: Book3S HV P9: Implement the rest of the P9 path in C
KVM: PPC: Book3S HV P9: inline kvmhv_load_hv_regs_and_go into
__kvmhv_vcpu_entry_p9
KVM: PPC: Book3S HV P9: Read machine check registers while MSR[RI] is
0
KVM: PPC: Book3S HV P9: Improve exit timing accounting coverage
KVM: PPC: Book3S HV P9: Move SPR loading after expiry time check
KVM: PPC: Book3S HV P9: Add helpers for OS SPR handling
KVM: PPC: Book3S HV P9: Switch to guest MMU context as late as
possible
KVM: PPC: Book3S HV: Implement radix prefetch workaround by disabling
MMU
KVM: PPC: Book3S HV: Remove support for dependent threads mode on P9
KVM: PPC: Book3S HV: Remove radix guest support from P7/8 path
KVM: PPC: Book3S HV: Remove virt mode checks from real mode handlers
KVM: PPC: Book3S HV: Remove unused nested HV tests in XICS emulation
KVM: PPC: Book3S HV: Radix guests should not have userspace hcalls
reflected to them
KVM: PPC: Book3S HV P9: Allow all P9 processors to enable nested HV
KVM: PPC: Book3S HV: small pseries_do_hcall cleanup
KVM: PPC: Book3S HV: add virtual mode handlers for HPT hcalls and page
faults
KVM: PPC: Book3S HV P9: implement hash guest support
KVM: PPC: Book3S HV P9: implement hash host / hash guest support
KVM: PPC: Book3S HV: remove ISA v3.0 and v3.1 support from P7/8 path
arch/powerpc/include/asm/asm-prototypes.h | 3 +-
arch/powerpc/include/asm/exception-64s.h | 13 +
arch/powerpc/include/asm/kvm_asm.h | 3 +-
arch/powerpc/include/asm/kvm_book3s.h | 2 +
arch/powerpc/include/asm/kvm_book3s_64.h | 8 +
arch/powerpc/include/asm/kvm_host.h | 1 -
arch/powerpc/include/asm/kvm_ppc.h | 21 +-
arch/powerpc/include/asm/mmu_context.h | 6 -
arch/powerpc/include/asm/time.h | 11 +
arch/powerpc/kernel/exceptions-64s.S | 257 ++------
arch/powerpc/kernel/security.c | 5 +-
arch/powerpc/kernel/time.c | 43 +-
arch/powerpc/kvm/Makefile | 6 +
arch/powerpc/kvm/book3s.c | 17 +-
arch/powerpc/kvm/book3s_64_entry.S | 402 ++++++++++++
arch/powerpc/kvm/book3s_64_vio_hv.c | 12 -
arch/powerpc/kvm/book3s_hv.c | 756 ++++++++++++----------
arch/powerpc/kvm/book3s_hv_builtin.c | 138 +---
arch/powerpc/kvm/book3s_hv_interrupt.c | 529 +++++++++++++++
arch/powerpc/kvm/book3s_hv_interrupts.S | 9 +-
arch/powerpc/kvm/book3s_hv_nested.c | 37 +-
arch/powerpc/kvm/book3s_hv_ras.c | 2 +
arch/powerpc/kvm/book3s_hv_rm_mmu.c | 15 +-
arch/powerpc/kvm/book3s_hv_rm_xics.c | 15 -
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 643 +-----------------
arch/powerpc/kvm/book3s_segment.S | 3 +
arch/powerpc/kvm/book3s_xive.c | 113 +++-
arch/powerpc/kvm/book3s_xive.h | 7 -
arch/powerpc/kvm/book3s_xive_native.c | 10 -
arch/powerpc/mm/book3s64/radix_pgtable.c | 27 +-
arch/powerpc/mm/book3s64/radix_tlb.c | 46 --
arch/powerpc/mm/mmu_context.c | 4 +-
arch/powerpc/platforms/powernv/idle.c | 52 +-
33 files changed, 1697 insertions(+), 1519 deletions(-)
create mode 100644 arch/powerpc/kvm/book3s_64_entry.S
create mode 100644 arch/powerpc/kvm/book3s_hv_interrupt.c
--
2.23.0
^ permalink raw reply
* Re: [PATCH printk v2 3/5] printk: remove NMI tracking
From: Petr Mladek @ 2021-04-01 14:37 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Peter Zijlstra, Alexei Starovoitov,
David Howells, Paul Mackerras, Will Deacon, Leonardo Bras,
Marc Zyngier, Masahiro Yamada, Russell King, Ingo Molnar,
Geert Uytterhoeven, Valentin Schneider, Kees Cook,
Paul E. McKenney, Anshuman Khandual, Frederic Weisbecker,
Johannes Weiner, Steven Rostedt, Nathan Chancellor, Nick Terrell,
Andy Lutomirski, Thomas Gleixner, linux-arm-kernel, linux-kernel,
Sergey Senozhatsky, Andrew Morton, linuxppc-dev, Mike Rapoport
In-Reply-To: <20210330153512.1182-4-john.ogness@linutronix.de>
On Tue 2021-03-30 17:35:10, John Ogness wrote:
> All NMI contexts are handled the same as the safe context: store the
> message and defer printing. There is no need to have special NMI
> context tracking for this. Using in_nmi() is enough.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
This is another great win!
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH printk v2 2/5] printk: remove safe buffers
From: Petr Mladek @ 2021-04-01 14:17 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Alexey Kardashevskiy, Paul Mackerras,
Tiezhu Yang, Rafael Aquini, Aneesh Kumar K.V, Peter Zijlstra,
Yue Hu, Jordan Niethe, Kees Cook, Paul E. McKenney,
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: <87a6qiqgzr.fsf@jogness.linutronix.de>
On Thu 2021-04-01 15:19:52, John Ogness wrote:
> On 2021-04-01, Petr Mladek <pmladek@suse.com> wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -1142,24 +1128,37 @@ void __init setup_log_buf(int early)
> >> new_descs, ilog2(new_descs_count),
> >> new_infos);
> >>
> >> - printk_safe_enter_irqsave(flags);
> >> + local_irq_save(flags);
> >
> > IMHO, we actually do not have to disable IRQ here. We already copy
> > messages that might appear in the small race window in NMI. It would
> > work the same way also for IRQ context.
>
> We do not have to, but why open up this window? We are still in early
> boot and interrupts have always been disabled here. I am not happy that
> this window even exists. I really prefer to keep it NMI-only.
Fair enough.
> >> --- a/lib/nmi_backtrace.c
> >> +++ b/lib/nmi_backtrace.c
> >> @@ -75,12 +75,6 @@ void nmi_trigger_cpumask_backtrace(const cpumask_t *mask,
> >> touch_softlockup_watchdog();
> >> }
> >>
> >> - /*
> >> - * Force flush any remote buffers that might be stuck in IRQ context
> >> - * and therefore could not run their irq_work.
> >> - */
> >> - printk_safe_flush();
> >
> > Sigh, this reminds me that the nmi_safe buffers serialized backtraces
> > from all CPUs.
> >
> > I am afraid that we have to put back the spinlock into
> > nmi_cpu_backtrace().
>
> Please no. That spinlock is a disaster. It can cause deadlocks with
> other cpu-locks (such as in kdb)
Could you please explain more the kdb case?
I am curious what locks might depend on each other here.
> and it will cause a major problem for atomic consoles.
AFAIK, you are going to add a special lock that would allow
nesting on the same CPU. It should possible and safe
to use is also for synchronizing the backtraces here.
> We need to be very careful about introducing locks
> where NMIs are waiting on other CPUs.
I agree.
> > It has been repeatedly added and removed depending
> > on whether the backtrace was printed into the main log buffer
> > or into the per-CPU buffers. Last time it was removed by
> > the commit 03fc7f9c99c1e7ae2925d ("printk/nmi: Prevent deadlock
> > when accessing the main log buffer in NMI").
> >
> > It should be safe because there should not be any other locks in the
> > code path. Note that only one backtrace might be triggered at the same
> > time, see @backtrace_flag in nmi_trigger_cpumask_backtrace().
>
> It is adding a lock around a lockless ringbuffer. For me that is a step
> backwards.
>
> > We _must_ serialize it somehow[*]. The lock in nmi_cpu_backtrace()
> > looks less evil than the nmi_safe machinery. nmi_safe() shrinks
> > too long backtraces, lose timestamps, needs to be explicitely
> > flushed here and there, is a non-trivial code.
> >
> > [*] Non-serialized bactraces are real mess. Caller-id is visible
> > only on consoles or via syslogd interface. And it is not much
> > convenient.
>
> Caller-id solves this problem and is easy to sort for anyone with
> `grep'. Yes, it is a shame that `dmesg' does not show it, but directly
> using any of the printk interfaces does show it (kmsg_dump, /dev/kmsg,
> syslog, console).
True but frankly, the current situation is _far_ from convenient:
+ consoles do not show it by default
+ none userspace tool (dmesg, journalctl, crash) is able to show it
+ grep is a nightmare, especially if you have more than handful of CPUs
Yes, everything is solvable but not easily.
> > I get this with "echo l >/proc/sysrq-trigger" and this patchset:
>
> Of course. Without caller-id, it is a mess. But this has nothing to do
> with NMI. The same problem exists for WARN_ON() on multiple CPUs
> simultaneously. If the user is not using caller-id, they are
> lost. Caller-id is the current solution to the interlaced logs.
Sure. But in reality, the risk of mixed WARN_ONs is small. While
this patch makes backtraces from all CPUs always unusable without
caller_id and non-trivial effort.
> 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.
My preference:
I most prefer 2nd solution until I see a realistic scenario
of a possible deadlock with the current kernel code.
I would still prefer 1st solution over 3rd one until we improve
kernel/userspace support for sorting the log by the caller id.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: Load modules closer to kernel text
From: Christophe Leroy @ 2021-04-01 13:43 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: Jordan Niethe, linuxppc-dev, linux-kernel
In-Reply-To: <87ft0a1v5i.fsf@mpe.ellerman.id.au>
Le 01/04/2021 à 06:33, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 31/03/2021 à 15:39, Michael Ellerman a écrit :
>>> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>>>> On the 8xx, TASK_SIZE is 0x80000000. The space between TASK_SIZE and
>>>> PAGE_OFFSET is not used.
>>>>
>>>> Use it to load modules in order to minimise the distance between
>>>> kernel text and modules and avoid trampolines in modules to access
>>>> kernel functions or other module functions.
>>>>
>>>> Define a 16Mbytes area for modules, that's more than enough.
>>>
>>> 16MB seems kind of small.
>>>
>>> At least on 64-bit we could potentially have hundreds of MBs of modules.
>>>
>>
>> Well, with a 16 MB kernel and 16 MB modules, my board is full :)
>
> Heh.
ARM set it to 16M or 8M.
>
>> Even on the more recent board that has 128 MB, I don't expect more than a few MBs of modules in
>> addition to the kernel which is approx 8M.
>>
>> But ok, I'll do something more generic, though it will conflict with Jordan's series.
>
> Don't feel you have to. You're the expert on 8xx, not me.
>
Doing it generic allows to also get it on book3s/32 for free.
Christophe
^ permalink raw reply
* Re: [PATCH 3/3] powerpc/sstep: Always test lmw and stmw
From: Michael Ellerman @ 2021-04-01 13:39 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: Jordan Niethe
In-Reply-To: <20210225032108.1458352-3-jniethe5@gmail.com>
Hi Jordan,
Jordan Niethe <jniethe5@gmail.com> writes:
> Load Multiple Word (lmw) and Store Multiple Word (stmw) will raise an
> Alignment Exception:
> - Little Endian mode: always
> - Big Endian mode: address not word aligned
>
> These conditions do not depend on cache inhibited memory. Test the
> alignment handler emulation of these instructions regardless of if there
> is cache inhibited memory available or not.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> .../powerpc/alignment/alignment_handler.c | 96 ++++++++++++++++++-
> 1 file changed, 94 insertions(+), 2 deletions(-)
Because of dd3a44c06f7b ("selftests/powerpc: Only test lwm/stmw on big endian")
this will need a respin sorry.
You'll need to add macros to generate lmw/stmw using .long, to avoid the
bug fixed in that commit.
cheers
^ permalink raw reply
* Re: [PATCH v10 03/10] powerpc: Always define MODULES_{VADDR,END}
From: Christophe Leroy @ 2021-04-01 13:36 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev; +Cc: ajd, npiggin, cmr, naveen.n.rao, dja
In-Reply-To: <20210330045132.722243-4-jniethe5@gmail.com>
Le 30/03/2021 à 06:51, Jordan Niethe a écrit :
> If MODULES_{VADDR,END} are not defined set them to VMALLOC_START and
> VMALLOC_END respectively. This reduces the need for special cases. For
> example, powerpc's module_alloc() was previously predicated on
> MODULES_VADDR being defined but now is unconditionally defined.
>
> This will be useful reducing conditional code in other places that need
> to allocate from the module region (i.e., kprobes).
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v10: New to series
> ---
> arch/powerpc/include/asm/pgtable.h | 5 +++++
> arch/powerpc/kernel/module.c | 5 +----
You probably also have changes to do in kernel/ptdump.c
In mm/book3s32/mmu.c and mm/kasan/kasan_init_32.c as well allthough that's harmless here.
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
> index 4eed82172e33..014c2921f26a 100644
> --- a/arch/powerpc/include/asm/pgtable.h
> +++ b/arch/powerpc/include/asm/pgtable.h
> @@ -167,6 +167,11 @@ struct seq_file;
> void arch_report_meminfo(struct seq_file *m);
> #endif /* CONFIG_PPC64 */
>
> +#ifndef MODULES_VADDR
> +#define MODULES_VADDR VMALLOC_START
> +#define MODULES_END VMALLOC_END
> +#endif
> +
> #endif /* __ASSEMBLY__ */
>
> #endif /* _ASM_POWERPC_PGTABLE_H */
> diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
> index a211b0253cdb..f1fb58389d58 100644
> --- a/arch/powerpc/kernel/module.c
> +++ b/arch/powerpc/kernel/module.c
> @@ -14,6 +14,7 @@
> #include <asm/firmware.h>
> #include <linux/sort.h>
> #include <asm/setup.h>
> +#include <linux/mm.h>
>
> static LIST_HEAD(module_bug_list);
>
> @@ -87,13 +88,9 @@ int module_finalize(const Elf_Ehdr *hdr,
> return 0;
> }
>
> -#ifdef MODULES_VADDR
> void *module_alloc(unsigned long size)
> {
> - BUILD_BUG_ON(TASK_SIZE > MODULES_VADDR);
> -
The above check is needed somewhere, if you remove it from here you have to perform the check
somewhere else.
> return __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END, GFP_KERNEL,
> PAGE_KERNEL_EXEC, VM_FLUSH_RESET_PERMS, NUMA_NO_NODE,
> __builtin_return_address(0));
> }
> -#endif
>
^ permalink raw reply
* [PATCH 1/3] powerpc/modules: Load modules closer to kernel text
From: Christophe Leroy @ 2021-04-01 13:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
jniethe5
Cc: linuxppc-dev, linux-kernel
On book3s/32, when STRICT_KERNEL_RWX is selected, modules are
allocated on the segment just before kernel text, ie on the
0xb0000000-0xbfffffff when PAGE_OFFSET is 0xc0000000.
On the 8xx, TASK_SIZE is 0x80000000. The space between TASK_SIZE and
PAGE_OFFSET is not used and could be used for modules.
The idea comes from ARM architecture.
Having modules just below PAGE_OFFSET offers an opportunity to
minimise the distance between kernel text and modules and avoid
trampolines in modules to access kernel functions or other module
functions.
When MODULES_VADDR is defined, powerpc has it's own module_alloc()
function. In that function, first try to allocate the module
above the limit defined by '_etext - 32M'. Then if the allocation
fails, fallback to the entire MODULES area.
DEBUG logs in module_32.c without the patch:
[ 1572.588822] module_32: Applying ADD relocate section 13 to 12
[ 1572.588891] module_32: Doing plt for call to 0xc00671a4 at 0xcae04024
[ 1572.588964] module_32: Initialized plt for 0xc00671a4 at cae04000
[ 1572.589037] module_32: REL24 value = CAE04000. location = CAE04024
[ 1572.589110] module_32: Location before: 48000001.
[ 1572.589171] module_32: Location after: 4BFFFFDD.
[ 1572.589231] module_32: ie. jump to 03FFFFDC+CAE04024 = CEE04000
[ 1572.589317] module_32: Applying ADD relocate section 15 to 14
[ 1572.589386] module_32: Doing plt for call to 0xc00671a4 at 0xcadfc018
[ 1572.589457] module_32: Initialized plt for 0xc00671a4 at cadfc000
[ 1572.589529] module_32: REL24 value = CADFC000. location = CADFC018
[ 1572.589601] module_32: Location before: 48000000.
[ 1572.589661] module_32: Location after: 4BFFFFE8.
[ 1572.589723] module_32: ie. jump to 03FFFFE8+CADFC018 = CEDFC000
With the patch:
[ 279.404671] module_32: Applying ADD relocate section 13 to 12
[ 279.404741] module_32: REL24 value = C00671B4. location = BF808024
[ 279.404814] module_32: Location before: 48000001.
[ 279.404874] module_32: Location after: 4885F191.
[ 279.404933] module_32: ie. jump to 0085F190+BF808024 = C00671B4
[ 279.405016] module_32: Applying ADD relocate section 15 to 14
[ 279.405085] module_32: REL24 value = C00671B4. location = BF800018
[ 279.405156] module_32: Location before: 48000000.
[ 279.405215] module_32: Location after: 4886719C.
[ 279.405275] module_32: ie. jump to 0086719C+BF800018 = C00671B4
We see that with the patch, no plt entries are set.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/module.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
index a211b0253cdb..fab84024650c 100644
--- a/arch/powerpc/kernel/module.c
+++ b/arch/powerpc/kernel/module.c
@@ -14,6 +14,7 @@
#include <asm/firmware.h>
#include <linux/sort.h>
#include <asm/setup.h>
+#include <asm/sections.h>
static LIST_HEAD(module_bug_list);
@@ -88,12 +89,28 @@ int module_finalize(const Elf_Ehdr *hdr,
}
#ifdef MODULES_VADDR
+static __always_inline void *
+__module_alloc(unsigned long size, unsigned long start, unsigned long end)
+{
+ return __vmalloc_node_range(size, 1, start, end, GFP_KERNEL,
+ PAGE_KERNEL_EXEC, VM_FLUSH_RESET_PERMS, NUMA_NO_NODE,
+ __builtin_return_address(0));
+}
+
void *module_alloc(unsigned long size)
{
+ unsigned long limit = (unsigned long)_etext - SZ_32M;
+ void *ptr = NULL;
+
BUILD_BUG_ON(TASK_SIZE > MODULES_VADDR);
- return __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END, GFP_KERNEL,
- PAGE_KERNEL_EXEC, VM_FLUSH_RESET_PERMS, NUMA_NO_NODE,
- __builtin_return_address(0));
+ /* First try within 32M limit from _etext to avoid branch trampolines */
+ if (MODULES_VADDR < PAGE_OFFSET && MODULES_END > limit)
+ ptr = __module_alloc(size, limit, MODULES_END);
+
+ if (!ptr)
+ ptr = __module_alloc(size, MODULES_VADDR, MODULES_END);
+
+ return ptr;
}
#endif
--
2.25.0
^ permalink raw reply related
* [PATCH 3/3] powerpc/32s: Define a MODULE area below kernel text all the time
From: Christophe Leroy @ 2021-04-01 13:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
jniethe5
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <0c3d5cb8a4dfdf6ca1b8aeb385c01470d6628d55.1617283827.git.christophe.leroy@csgroup.eu>
On book3s/32, the segment below kernel text is used for module
allocation when CONFIG_STRICT_KERNEL_RWX is defined.
In order to benefit from the powerpc specific module_alloc()
function which allocate modules with 32 Mbytes from
end of kernel text, use that segment below PAGE_OFFSET at all time.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/Kconfig | 2 +-
arch/powerpc/include/asm/book3s/32/pgtable.h | 2 --
arch/powerpc/mm/book3s32/mmu.c | 7 -------
3 files changed, 1 insertion(+), 10 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c1344c05226c..15a91202d5c3 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -1219,7 +1219,7 @@ config TASK_SIZE_BOOL
config TASK_SIZE
hex "Size of user task space" if TASK_SIZE_BOOL
default "0x80000000" if PPC_8xx
- default "0xb0000000" if PPC_BOOK3S_32 && STRICT_KERNEL_RWX
+ default "0xb0000000" if PPC_BOOK3S_32
default "0xc0000000"
endmenu
diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index 415ae29fa73a..83c65845a1a9 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -194,10 +194,8 @@ int map_kernel_page(unsigned long va, phys_addr_t pa, pgprot_t prot);
#define VMALLOC_END ioremap_bot
#endif
-#ifdef CONFIG_STRICT_KERNEL_RWX
#define MODULES_END ALIGN_DOWN(PAGE_OFFSET, SZ_256M)
#define MODULES_VADDR (MODULES_END - SZ_256M)
-#endif
#ifndef __ASSEMBLY__
#include <linux/sched.h>
diff --git a/arch/powerpc/mm/book3s32/mmu.c b/arch/powerpc/mm/book3s32/mmu.c
index a0db398b5c26..159930351d9f 100644
--- a/arch/powerpc/mm/book3s32/mmu.c
+++ b/arch/powerpc/mm/book3s32/mmu.c
@@ -184,17 +184,10 @@ static bool is_module_segment(unsigned long addr)
{
if (!IS_ENABLED(CONFIG_MODULES))
return false;
-#ifdef MODULES_VADDR
if (addr < ALIGN_DOWN(MODULES_VADDR, SZ_256M))
return false;
if (addr > ALIGN(MODULES_END, SZ_256M) - 1)
return false;
-#else
- if (addr < ALIGN_DOWN(VMALLOC_START, SZ_256M))
- return false;
- if (addr > ALIGN(VMALLOC_END, SZ_256M) - 1)
- return false;
-#endif
return true;
}
--
2.25.0
^ permalink raw reply related
* [PATCH 2/3] powerpc/8xx: Define a MODULE area below kernel text
From: Christophe Leroy @ 2021-04-01 13:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
jniethe5
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <0c3d5cb8a4dfdf6ca1b8aeb385c01470d6628d55.1617283827.git.christophe.leroy@csgroup.eu>
On the 8xx, TASK_SIZE is 0x80000000. The space between TASK_SIZE
and PAGE_OFFSET is not used.
In order to benefit from the powerpc specific module_alloc()
function which allocate modules with 32 Mbytes from
end of kernel text, define MODULES_VADDR and MODULES_END.
Set a 256Mb area just below PAGE_OFFSET, like book3s/32.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/include/asm/nohash/32/mmu-8xx.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/include/asm/nohash/32/mmu-8xx.h b/arch/powerpc/include/asm/nohash/32/mmu-8xx.h
index 478249959baa..6e4faa0a9b35 100644
--- a/arch/powerpc/include/asm/nohash/32/mmu-8xx.h
+++ b/arch/powerpc/include/asm/nohash/32/mmu-8xx.h
@@ -172,6 +172,9 @@
#define mmu_linear_psize MMU_PAGE_8M
+#define MODULES_VADDR (PAGE_OFFSET - SZ_256M)
+#define MODULES_END PAGE_OFFSET
+
#ifndef __ASSEMBLY__
#include <linux/mmdebug.h>
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v4 02/46] KVM: PPC: Book3S HV: Add a function to filter guest LPCR bits
From: Nicholas Piggin @ 2021-04-01 13:28 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, kvm-ppc
In-Reply-To: <1617269036.86nd07dbhp.astroid@bobo.none>
Excerpts from Nicholas Piggin's message of April 1, 2021 7:32 pm:
> Excerpts from Paul Mackerras's message of March 31, 2021 2:08 pm:
>> On Tue, Mar 23, 2021 at 11:02:21AM +1000, Nicholas Piggin wrote:
>>> Guest LPCR depends on hardware type, and future changes will add
>>> restrictions based on errata and guest MMU mode. Move this logic
>>> to a common function and use it for the cases where the guest
>>> wants to update its LPCR (or the LPCR of a nested guest).
>>
>> [snip]
>>
>>> @@ -4641,8 +4662,9 @@ void kvmppc_update_lpcr(struct kvm *kvm, unsigned long lpcr, unsigned long mask)
>>> struct kvmppc_vcore *vc = kvm->arch.vcores[i];
>>> if (!vc)
>>> continue;
>>> +
>>> spin_lock(&vc->lock);
>>> - vc->lpcr = (vc->lpcr & ~mask) | lpcr;
>>> + vc->lpcr = kvmppc_filter_lpcr_hv(vc, (vc->lpcr & ~mask) | lpcr);
>>
>> This change seems unnecessary, since kvmppc_update_lpcr is called only
>> to update MMU configuration bits, not as a result of any action by
>> userspace or a nested hypervisor. It's also beyond the scope of what
>> was mentioned in the commit message.
>
> I didn't think it was outside the spirit of the patch, but yes
> only the guest update LPCR case was enumerated. Would it be more
> consistent to add it to the changelog and leave it in here or would
> you prefer it left out until there is a real use?
On second thoughts, I already left at least one other place without
such a check, so I now tend to agree with you. But I instead added
a test that just ensures the host is not out of synch with itself in
terms of what it can set the LPCR to.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 16/18] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
From: Will Deacon @ 2021-04-01 13:26 UTC (permalink / raw)
To: Christoph Hellwig
Cc: kvm, Robin Murphy, linux-arm-msm, linuxppc-dev, dri-devel,
Li Yang, iommu, netdev, virtualization, freedreno,
David Woodhouse, linux-arm-kernel
In-Reply-To: <20210401095945.GA6726@lst.de>
On Thu, Apr 01, 2021 at 11:59:45AM +0200, Christoph Hellwig wrote:
> For now I'll just pass the iommu_domain to iommu_get_dma_strict,
> so that we can check for it. We can do additional cleanups on top
> of that later.
Sounds good to me, cheers!
Will
^ permalink raw reply
* Re: [PATCH printk v2 2/5] printk: remove safe buffers
From: John Ogness @ 2021-04-01 13:19 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Alexey Kardashevskiy, Paul Mackerras,
Tiezhu Yang, Rafael Aquini, Aneesh Kumar K.V, Peter Zijlstra,
Yue Hu, Jordan Niethe, Kees Cook, Paul E. McKenney,
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: <YGW63/elFr/gYW1u@alley>
On 2021-04-01, Petr Mladek <pmladek@suse.com> wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1142,24 +1128,37 @@ void __init setup_log_buf(int early)
>> new_descs, ilog2(new_descs_count),
>> new_infos);
>>
>> - printk_safe_enter_irqsave(flags);
>> + local_irq_save(flags);
>
> IMHO, we actually do not have to disable IRQ here. We already copy
> messages that might appear in the small race window in NMI. It would
> work the same way also for IRQ context.
We do not have to, but why open up this window? We are still in early
boot and interrupts have always been disabled here. I am not happy that
this window even exists. I really prefer to keep it NMI-only.
>> --- a/lib/nmi_backtrace.c
>> +++ b/lib/nmi_backtrace.c
>> @@ -75,12 +75,6 @@ void nmi_trigger_cpumask_backtrace(const cpumask_t *mask,
>> touch_softlockup_watchdog();
>> }
>>
>> - /*
>> - * Force flush any remote buffers that might be stuck in IRQ context
>> - * and therefore could not run their irq_work.
>> - */
>> - printk_safe_flush();
>
> Sigh, this reminds me that the nmi_safe buffers serialized backtraces
> from all CPUs.
>
> I am afraid that we have to put back the spinlock into
> nmi_cpu_backtrace().
Please no. That spinlock is a disaster. It can cause deadlocks with
other cpu-locks (such as in kdb) and it will cause a major problem for
atomic consoles. We need to be very careful about introducing locks
where NMIs are waiting on other CPUs.
> It has been repeatedly added and removed depending
> on whether the backtrace was printed into the main log buffer
> or into the per-CPU buffers. Last time it was removed by
> the commit 03fc7f9c99c1e7ae2925d ("printk/nmi: Prevent deadlock
> when accessing the main log buffer in NMI").
>
> It should be safe because there should not be any other locks in the
> code path. Note that only one backtrace might be triggered at the same
> time, see @backtrace_flag in nmi_trigger_cpumask_backtrace().
It is adding a lock around a lockless ringbuffer. For me that is a step
backwards.
> We _must_ serialize it somehow[*]. The lock in nmi_cpu_backtrace()
> looks less evil than the nmi_safe machinery. nmi_safe() shrinks
> too long backtraces, lose timestamps, needs to be explicitely
> flushed here and there, is a non-trivial code.
>
> [*] Non-serialized bactraces are real mess. Caller-id is visible
> only on consoles or via syslogd interface. And it is not much
> convenient.
Caller-id solves this problem and is easy to sort for anyone with
`grep'. Yes, it is a shame that `dmesg' does not show it, but directly
using any of the printk interfaces does show it (kmsg_dump, /dev/kmsg,
syslog, console).
> I get this with "echo l >/proc/sysrq-trigger" and this patchset:
Of course. Without caller-id, it is a mess. But this has nothing to do
with NMI. The same problem exists for WARN_ON() on multiple CPUs
simultaneously. If the user is not using caller-id, they are
lost. Caller-id is the current solution to the interlaced logs.
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.
John Ogness
[0] https://lore.kernel.org/lkml/472f2e553805b52d9834d64e4056db965edee329.camel@perches.com
[1] offlist message-id: 87d03k9ymz.fsf@jogness.linutronix.de
^ permalink raw reply
* Re: [PATCH v3 8/9] powerpc/xive: Map one IPI interrupt per node
From: Nicholas Piggin @ 2021-04-01 12:50 UTC (permalink / raw)
To: Cédric Le Goater, linuxppc-dev; +Cc: Thomas Gleixner, Greg Kurz
In-Reply-To: <20210331144514.892250-9-clg@kaod.org>
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)
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;
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v3 0/9] powerpc/xive: Map one IPI interrupt per node
From: Greg Kurz @ 2021-04-01 12:45 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: linuxppc-dev
In-Reply-To: <9b678c0d-2c39-62b8-8ff8-392b38ada6b3@kaod.org>
On Thu, 1 Apr 2021 11:18:10 +0200
Cédric Le Goater <clg@kaod.org> wrote:
> Hello,
>
> On 4/1/21 10:04 AM, Greg Kurz wrote:
> > On Wed, 31 Mar 2021 16:45:05 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> >
> >>
> >> Hello,
> >>
> >> 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.
> >>
> >
> > As explained by David and others, NUMA nodes happen to match sockets
> > with current POWER CPUs but these are really different concepts. NUMA
> > is about CPU memory accesses latency,
>
> This is exactly our problem. we have cache issues because hw threads
> on different chips are trying to access the same structure in memory.
> It happens on virtual platforms and baremetal platforms. This is not
> restricted to pseries.
>
Ok, I get it... the XIVE HW accesses structures in RAM, just like HW threads
do, so the closer, the better. This definitely looks NUMA related indeed. So
yes, the idea of having the XIVE HW to only access local in-RAM data when
handling IPIs between vCPUs in the same NUMA node makes sense.
What is less clear is the exact role of ibm,chip-id actually. This is
currently used on PowerNV only to pick up a default target on the same
"chip" as the source if possible. What is the detailed motivation behind
this ?
> > while in the case of XIVE you
> > really need to identify a XIVE chip localized in a given socket.
> >
> > PAPR doesn't know about sockets, only cores. In other words, a PAPR
> > compliant guest sees all vCPUs like they all sit in a single socket.
>
> There are also NUMA nodes on PAPR.
>
Yes but nothing prevents a NUMA node to span over multiple sockets
or having several NUMA nodes within the same socket, even if this
isn't the case in practice with current POWER hardware.
> > Same for the XIVE. Trying to introduce a concept of socket, either
> > by hijacking OPAL's ibm,chip-id or NUMA node ids, is a kind of
> > spec violation in this context. If the user cares for locality of
> > the vCPUs and XIVE on the same socket, then it should bind vCPU
> > threads to host CPUs from the same socket in the first place.
>
> Yes. that's a must have of course. You need to reflect the real HW
> topology in the guest or LPAR if you are after performance, or
> restrict the virtual machine to be on a single socket/chip/node.
>
> And this is not only a XIVE problem. XICS has the same problem with
> a shared single IPI interrupt descriptor but XICS doesn't scale well
> by design, so it doesn't show.
>
>
> > Isn't this enough to solve the performance issues this series
> > want to fix, without the need for virtual socket ids ?
> what are virtual socket ids ? A new concept ?
>
For now, we have virtual CPUs identified by a virtual CPU id.
It thus seems natural to speak of a virtual socket id, but
anyway, the wording isn't really important here and you
don't answer the question ;-)
> Thanks,
>
> C.
>
> >
> >> * 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
> >>
> >> Thanks,
> >>
> >> C.
> >>
> >> Changes in v3:
> >>
> >> - improved commit log for the misuse of "ibm,chip-id"
> >> - better error handling of xive_request_ipi()
> >> - use of a fwnode_handle to name the new domain
> >> - increased IPI name length
> >> - use of early_cpu_to_node() for hotplugged CPUs
> >> - filter CPU-less nodes
> >>
> >> Changes in v2:
> >>
> >> - extra simplification on xmon
> >> - fixes on issues reported by the kernel test robot
> >>
> >> Cédric Le Goater (9):
> >> powerpc/xive: Use cpu_to_node() instead of "ibm,chip-id" property
> >> powerpc/xive: Introduce an IPI interrupt domain
> >> powerpc/xive: Remove useless check on XIVE_IPI_HW_IRQ
> >> powerpc/xive: Simplify xive_core_debug_show()
> >> powerpc/xive: Drop check on irq_data in xive_core_debug_show()
> >> powerpc/xive: Simplify the dump of XIVE interrupts under xmon
> >> powerpc/xive: Fix xmon command "dxi"
> >> powerpc/xive: Map one IPI interrupt per node
> >> powerpc/xive: Modernize XIVE-IPI domain with an 'alloc' handler
> >>
> >> arch/powerpc/include/asm/xive.h | 1 +
> >> arch/powerpc/sysdev/xive/xive-internal.h | 2 -
> >> arch/powerpc/sysdev/xive/common.c | 211 +++++++++++++++--------
> >> arch/powerpc/xmon/xmon.c | 28 +--
> >> 4 files changed, 139 insertions(+), 103 deletions(-)
> >>
> >
>
^ permalink raw reply
* Re: [PATCH printk v2 2/5] printk: remove safe buffers
From: Petr Mladek @ 2021-04-01 12:21 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Alexey Kardashevskiy, Paul Mackerras,
Tiezhu Yang, Rafael Aquini, Aneesh Kumar K.V, Peter Zijlstra,
Yue Hu, Jordan Niethe, Kees Cook, Paul E. McKenney,
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: <20210330153512.1182-3-john.ogness@linutronix.de>
On Tue 2021-03-30 17:35:09, John Ogness wrote:
> With @logbuf_lock removed, the high level printk functions for
> storing messages are lockless. Messages can be stored from any
> context, so there is no need for the NMI and safe buffers anymore.
> Remove the NMI and safe buffers.
>
> Although the safe buffers are removed, the NMI and safe context
> tracking is still in place. In these contexts, store the message
> immediately but still use irq_work to defer the console printing.
>
> Since printk recursion tracking is in place, safe context tracking
> for most of printk is not needed. Remove it. Only safe context
> tracking relating to the console lock is left in place. This is
> because the console lock is needed for the actual printing.
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1142,24 +1128,37 @@ void __init setup_log_buf(int early)
> new_descs, ilog2(new_descs_count),
> new_infos);
>
> - printk_safe_enter_irqsave(flags);
> + local_irq_save(flags);
IMHO, we actually do not have to disable IRQ here. We already copy
messages that might appear in the small race window in NMI. It would work
the same way also for IRQ context.
> log_buf_len = new_log_buf_len;
> log_buf = new_log_buf;
> new_log_buf_len = 0;
>
> free = __LOG_BUF_LEN;
> - prb_for_each_record(0, &printk_rb_static, seq, &r)
> - free -= add_to_rb(&printk_rb_dynamic, &r);
> + prb_for_each_record(0, &printk_rb_static, seq, &r) {
> + text_size = add_to_rb(&printk_rb_dynamic, &r);
> + if (text_size > free)
> + free = 0;
> + else
> + free -= text_size;
> + }
>
> - /*
> - * This is early enough that everything is still running on the
> - * boot CPU and interrupts are disabled. So no new messages will
> - * appear during the transition to the dynamic buffer.
> - */
> prb = &printk_rb_dynamic;
>
> - printk_safe_exit_irqrestore(flags);
> + local_irq_restore(flags);
> +
> + /*
> + * Copy any remaining messages that might have appeared from
> + * NMI context after copying but before switching to the
> + * dynamic buffer.
The above comment would need to get updated if we keep also normal
IRQ enabled when copying the log buffers.
> + */
> + prb_for_each_record(seq, &printk_rb_static, seq, &r) {
> + text_size = add_to_rb(&printk_rb_dynamic, &r);
> + if (text_size > free)
> + free = 0;
> + else
> + free -= text_size;
> + }
>
> if (seq != prb_next_seq(&printk_rb_static)) {
> pr_err("dropped %llu messages\n",
> --- a/lib/nmi_backtrace.c
> +++ b/lib/nmi_backtrace.c
> @@ -75,12 +75,6 @@ void nmi_trigger_cpumask_backtrace(const cpumask_t *mask,
> touch_softlockup_watchdog();
> }
>
> - /*
> - * Force flush any remote buffers that might be stuck in IRQ context
> - * and therefore could not run their irq_work.
> - */
> - printk_safe_flush();
Sigh, this reminds me that the nmi_safe buffers serialized backtraces
from all CPUs.
I am afraid that we have to put back the spinlock into
nmi_cpu_backtrace(). It has been repeatedly added and removed depending
on whether the backtrace was printed into the main log buffer
or into the per-CPU buffers. Last time it was removed by
the commit 03fc7f9c99c1e7ae2925d ("printk/nmi: Prevent deadlock
when accessing the main log buffer in NMI").
It should be safe because there should not be any other locks in the
code path. Note that only one backtrace might be triggered at the same
time, see @backtrace_flag in nmi_trigger_cpumask_backtrace().
We _must_ serialize it somehow[*]. The lock in nmi_cpu_backtrace()
looks less evil than the nmi_safe machinery. nmi_safe() shrinks
too long backtraces, lose timestamps, needs to be explicitely
flushed here and there, is a non-trivial code.
[*] Non-serialized bactraces are real mess. Caller-id is visible
only on consoles or via syslogd interface. And it is not much
convenient.
I get this with "echo l >/proc/sysrq-trigger" and this patchset:
[ 95.642793] sysrq: Show backtrace of all active CPUs
[ 95.645202] NMI backtrace for cpu 3
[ 95.646778] CPU: 3 PID: 5095 Comm: bash Kdump: loaded Tainted: G W 5.11.0-default+ #231
[ 95.650397] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.12.0-59-gc9ba527-rebuilt.opensuse.org 04/01/2014
[ 95.656497] Call Trace:
[ 95.657937] dump_stack+0x88/0xab
[ 95.659888] nmi_cpu_backtrace+0xa4/0xc0
[ 95.661744] ? lapic_can_unplug_cpu+0xa0/0xa0
[ 95.663658] nmi_trigger_cpumask_backtrace+0xe6/0x120
[ 95.665657] arch_trigger_cpumask_backtrace+0x19/0x20
[ 95.667720] sysrq_handle_showallcpus+0x17/0x20
[ 95.670218] __handle_sysrq+0xe1/0x240
[ 95.672190] write_sysrq_trigger+0x51/0x60
[ 95.673993] proc_reg_write+0x62/0x90
[ 95.675319] vfs_write+0xed/0x380
[ 95.676636] ksys_write+0xad/0xf0
[ 95.677835] __x64_sys_write+0x1a/0x20
[ 95.678722] do_syscall_64+0x37/0x50
[ 95.679525] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 95.680571] RIP: 0033:0x7f3cbc2b3d44
[ 95.681380] Code: 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 80 00 00 00 00 8b 05 ea fa 2c 00 48 63 ff 85 c0 75 13 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 54 f3 c3 66 90 55 53 48 89 d5 48 89 f3 48 83
[ 95.684456] RSP: 002b:00007ffe29f06018 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[ 95.686029] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f3cbc2b3d44
[ 95.687346] RDX: 0000000000000002 RSI: 000055ad7117b420 RDI: 0000000000000001
[ 95.688690] RBP: 000055ad7117b420 R08: 000000000000000a R09: 0000000000000000
[ 95.690071] R10: 000000000000000a R11: 0000000000000246 R12: 0000000000000002
[ 95.691243] R13: 0000000000000001 R14: 00007f3cbc57f720 R15: 0000000000000002
[ 95.692318] Sending NMI from CPU 3 to CPUs 0-2:
[ 95.693014] NMI backtrace for cpu 2
[ 95.693014] NMI backtrace for cpu 1
[ 95.693016] CPU: 2 PID: 0 Comm: swapper/2 Kdump: loaded Tainted: G W 5.11.0-default+ #231
[ 95.693014] NMI backtrace for cpu 0 skipped: idling at native_safe_halt+0x12/0x20
[ 95.693016] CPU: 1 PID: 448 Comm: systemd-journal Kdump: loaded Tainted: G W 5.11.0-default+ #231
[ 95.693018] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.12.0-59-gc9ba527-rebuilt.opensuse.org 04/01/2014
[ 95.693019] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.12.0-59-gc9ba527-rebuilt.opensuse.org 04/01/2014
[ 95.693020] RIP: 0010:ttwu_do_wakeup+0x1aa/0x220
[ 95.693021] RIP: 0010:inode_permission+0x1d/0x150
[ 95.693025] Code: f0 48 39 c1 72 1b 48 89 83 c8 0b 00 00 48 c7 83 c0 0b 00 00 00 00 00 00 5b 41 5c 41 5d 41 5e 5d c3 48 89 8b c8 0b 00 00 eb e3 <48> 8d 7b 18 be ff ff ff ff e8 68 95 c4 00 85 c0 75 80 0f 0b e9 79
[ 95.693025] Code: ff ff 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 55 48 89 e5 41 56 41 55 41 89 f5 41 54 53 41 83 e5 02 75 43 f6 47 02 01 <41> 89 f4 48 89 fb 0f 84 80 00 00 00 44 89 e6 48 89 df e8 dc fd ff
[ 95.693027] RSP: 0018:ffffbae700120ef8 EFLAGS: 00000002
[ 95.693028] RSP: 0018:ffffbae7003ebea0 EFLAGS: 00000202
[ 95.693028] RAX: 0000000000000001 RBX: ffff9eb2ffbebec0 RCX: 0000000000000000
[ 95.693030] RDX: 0000000000010003 RSI: ffffffffaa6a9860 RDI: ffff9eb2803351d0
[ 95.693031] RAX: ffff9eb28229fa80 RBX: 0000000000000001 RCX: 0000000000000000
[ 95.693031] RBP: ffffbae700120f18 R08: 0000000000000001 R09: 0000000000000001
[ 95.693032] R10: ffff9eb282f00850 R11: 000000000000028d R12: ffff9eb282f00780
[ 95.693032] RDX: 0000000000000001 RSI: 0000000000000010 RDI: ffff9eb286df5298
[ 95.693033] R13: ffffbae700120f60 R14: ffffbae700120f60 R15: 0000000000000000
[ 95.693034] RBP: ffffbae7003ebec0 R08: 0000000000000001 R09: 0000000000000001
[ 95.693035] FS: 0000000000000000(0000) GS:ffff9eb2ffa00000(0000) knlGS:0000000000000000
[ 95.693036] R10: ffffbae7003ebea8 R11: 0000000000000001 R12: 0000000000000000
[ 95.693037] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 95.693037] R13: 0000000000000000 R14: 000055a9f3817900 R15: 0000000000000000
[ 95.693038] CR2: 000055e2be208280 CR3: 0000000103838003 CR4: 0000000000370ee0
[ 95.693039] FS: 00007f1682ccb1c0(0000) GS:ffff9eb2ff800000(0000) knlGS:0000000000000000
[ 95.693041] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 95.693042] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 95.693043] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 95.693043] CR2: 00007f3cbbfb36a8 CR3: 0000000102708005 CR4: 0000000000370ee0
[ 95.693044] Call Trace:
[ 95.693045] <IRQ>
[ 95.693047] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 95.693047] ttwu_do_activate+0x90/0x190
[ 95.693049] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 95.693050] Call Trace:
[ 95.693051] sched_ttwu_pending+0xe6/0x180
[ 95.693054] do_faccessat+0xbb/0x260
[ 95.693055] flush_smp_call_function_queue+0x117/0x220
[ 95.693058] generic_smp_call_function_single_interrupt+0x13/0x20
[ 95.693060] __x64_sys_access+0x1d/0x20
[ 95.693060] __sysvec_call_function_single+0x47/0x190
[ 95.693063] asm_call_irq_on_stack+0xf/0x20
[ 95.693064] do_syscall_64+0x37/0x50
[ 95.693066] </IRQ>
[ 95.693067] sysvec_call_function_single+0x6d/0xb0
[ 95.693068] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 95.693070] asm_sysvec_call_function_single+0x12/0x20
[ 95.693071] RIP: 0033:0x7f1681d6be1a
[ 95.693072] RIP: 0010:native_safe_halt+0x12/0x20
[ 95.693074] Code: 48 8b 15 81 a0 2c 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 63 f6 b8 15 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 06 f3 c3 0f 1f 40 00 48 8b 15 49 a0 2c 00 f7
[ 95.693074] Code: 00 0f 00 2d 92 4f 48 00 f4 5d c3 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 55 48 89 e5 e9 07 00 00 00 0f 00 2d 72 4f 48 00 fb f4 <5d> c3 cc cc cc cc cc cc cc cc cc cc cc cc 0f 1f 44 00 00 55 65 8b
[ 95.693076] RSP: 002b:00007fff61a07828 EFLAGS: 00000246
[ 95.693078] RSP: 0018:ffffbae7000b7e90 EFLAGS: 00000206
[ 95.693080] RAX: ffffffffa8d86240 RBX: 0000000000000002 RCX: 0000000000000000
[ 95.693080] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffffa8d865f5
[ 95.693078] ORIG_RAX: 0000000000000015
[ 95.693081] RBP: ffffbae7000b7e90 R08: 0000000000000001 R09: 0000000000000001
[ 95.693082] R10: 0000000000000000 R11: 0000000000000001 R12: ffffffffaa9bdf60
[ 95.693082] RAX: ffffffffffffffda RBX: 00007fff61a0a550 RCX: 00007f1681d6be1a
[ 95.693083] R13: 0000000000000000 R14: 0000000000000000 R15: ffff9eb280334400
[ 95.693084] RDX: 00007f16827d70e0 RSI: 0000000000000000 RDI: 000055a9f3817900
[ 95.693085] RBP: 00007fff61a07870 R08: 0000000000000000 R09: 0000000000000000
[ 95.693085] ? __cpuidle_text_start+0x8/0x8
[ 95.693086] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[ 95.693087] R13: 0000000000000000 R14: 00007fff61a07970 R15: 0000000000000000
[ 95.693087] ? default_idle_call+0x45/0x200
[ 95.693091] default_idle+0xe/0x20
[ 95.693093] arch_cpu_idle+0x15/0x20
[ 95.693094] default_idle_call+0x6c/0x200
[ 95.693096] do_idle+0x1fb/0x2e0
[ 95.693098] ? do_idle+0x1d9/0x2e0
[ 95.693100] cpu_startup_entry+0x1d/0x20
[ 95.693102] start_secondary+0x12b/0x160
[ 95.693105] secondary_startup_64_no_verify+0xc2/0xcb
Otherwise, I really love this patch removing a lot of tricky code.
Best Regards,
Petr
^ permalink raw reply
* [RFC 0/2] Define a new apporach to determine if an idle vCPU will be scheduled instantly or not
From: Parth Shah @ 2021-04-01 11:59 UTC (permalink / raw)
To: linuxppc-dev; +Cc: ego, mikey, srikar, npiggin, paulus, svaidy
Abstract:
=========
The Linux task scheduler tries to find an idle cpu for a wakee task
thereby lowering the wakeup latency as much as possible. The process
of determining if a cpu is idle or not has evolved over time.
Currently, a cpu is considered idle if
- there are no task running or enqueued to the runqueue of the cpu and
- in while running inside a guest, a cpu is not yielded (determined
via available_idle_cpu())
While inside the guest, there is no way to deterministically predict
if a vCPU that has been yielded/ceded to the hypervisor can be gotten
back. Hence currently the scheduler considers such CEDEd vCPU as not
"available" idle and would instead pick other busy CPUs for waking up
the wakee task.
In this patch-set we try to further classify idle cpus as instantly
available or not. This is achieved by taking hint from the hypervisor
by quering if the vCPU will be scheduled instantly or not. In most
cases, scheduler prefers prev_cpu of a waking task unless it is busy.
In this patchset, the hypervisor uses this information to figure out
if the prev_cpu used by the task (of the corresponding vCPU) is idle
or not, and passes this information to the guest.
Interface:
===========
This patchset introduces a new HCALL named H_IDLE_HINT for the guest
to query if a vCPU can be dispatched quickly or not. This is
currently a crude interface to demonstrate the efficacy of this
method. We are looking for feedback on any other mechanisms of
obtainining a hint from the hypervisor where the hint is still relevant.
But this patch series tries to emphasis the possible optimization for
task wakeup-latency and is open to accept any interface/architecture.
Internal working:
========
The code-flow of the current implementation is as follow:
- GuestOS scheduler searches for idle cpu using `avaialable_idle_cpu()`
which also looks if a vcpu_is_preempted() to see if vCPU is yielded or
not.
- If vCPU is yielded, then the GuestOS will additionally make hcall
H_IDLE_HINT to find if a vCPU can be scheduled instantly or not.
- The hypervisor services hcall by first finding the corresponding
task-p of the vCPU and returns 1 if the task_cpu(p) is
avaialable_idle_cpu or running SCHED_IDLE task. Else returns 0.
- GuestOS takes up this hint and considers the vCPU as idle if the hint
from hypervisor has value == 1.
The patch-set is based on v5.11 kernel.
Results:
========
- Baseline kernel = v5.11
- Patched kernel = v5.11 + this patch-set
Setup:
All the results are taken on IBM POWER9 baremetal system running patched
kernel. This system consists of 2 NUMA nodes, 22 cores per socket with
SMT-4 mode.
2 KVM guests are created sharing same set of physical CPUs, and each KVM
has identical CPU topology of 40 CPUs, 10 cores with SMT-4 support.
Scenarios:
----------
1. Under-commit case: Only one KVM is active at a time.
- Baseline (v5.11):
$> schbench -m 20 -t 2 -r 30
Latency percentiles (usec)
50.0000th: 67
75.0000th: 83
90.0000th: 115
95.0000th: 352
*99.0000th: 2260 <-----
99.5000th: 3580
99.9000th: 7128
min=0, max=9927
- With patch (v5.11 + patch):
$> schbench -m 20 -t 2 -r 30
Latency percentiles (usec)
50.0000th: 100
75.0000th: 113
90.0000th: 328
95.0000th: 360
*99.0000th: 434 (-80%) <----
99.5000th: 489
99.9000th: 2324
min=0, max=6054
We see a significant reduction in the tail latencies due to being able
to schedule on an yielded/ceded idle CPU with the patchset instead of
waking up the task on a busy CPU.
2. Over-commit case: Both KVMs sharing same set of CPUs. One KVM is
creating noise using `schbench -m 10 -t 2 -r 3000` while only the other
KVM is benchmarked.
- Baseline:
$> schbench -m 20 -t 2 -r 30
Latency percentiles (usec)
50.0000th: 73
75.0000th: 89
90.0000th: 115
95.0000th: 166
*99.0000th: 3084
99.5000th: 4044
99.9000th: 7656
min=0, max=18448
- With patch:
$> schbench -m 20 -t 2 -r 30
Latency percentiles (usec)
50.0000th: 114
75.0000th: 137
90.0000th: 170
95.0000th: 237
*99.0000th: 2828
99.5000th: 4168
99.9000th: 7528
min=0, max=15387
The results demonstrates that the proposed method of getting idle-hint
from the hypervisor to better find an idle cpu in the guestOS is very
helpful in under-commmit cases due to higher chance of finding the
previously used physical cpu as idle.
The results also confirms that there is no regression in the over-commit
case where the proposed methodlogy does not affect much.
Additionally, more tests were carried out with different combinations of
schbench threads and different numbers of KVM guest. The results for
these tests further confirmed that there is no major regression on the
workload performance.
Parth Shah (2):
KVM:PPC: Add new hcall to provide hint if a vcpu task will be
scheduled instantly.
sched: Use H_IDLE_HINT hcall to find if a vCPU can be wakeup target
arch/powerpc/include/asm/hvcall.h | 3 ++-
arch/powerpc/include/asm/paravirt.h | 21 +++++++++++++++++++--
arch/powerpc/kvm/book3s_hv.c | 13 +++++++++++++
arch/powerpc/kvm/trace_hv.h | 1 +
include/linux/kvm_host.h | 1 +
include/linux/sched.h | 1 +
kernel/sched/core.c | 13 +++++++++++++
kernel/sched/fair.c | 12 ++++++++++++
kernel/sched/sched.h | 1 +
virt/kvm/kvm_main.c | 17 +++++++++++++++++
10 files changed, 80 insertions(+), 3 deletions(-)
--
2.26.2
^ permalink raw reply
* [RFC 1/2] KVM:PPC: Add new hcall to provide hint if a vcpu task will be scheduled instantly.
From: Parth Shah @ 2021-04-01 11:59 UTC (permalink / raw)
To: linuxppc-dev; +Cc: ego, mikey, srikar, npiggin, paulus, svaidy
In-Reply-To: <20210401115922.1524705-1-parth@linux.ibm.com>
H_IDLE_HINT is a new hcall introduced to provide a hint to the guestOS
indicating if a given vCPU can be scheduled instantly or not.
The task scheduler generally prefers previous cpu of a task if it is
available_idle. So if a prev_cpu of the corresponding vCPU task_struct is
found to be available_idle or sched_idle then hint guestOS that the given
vCPU can be scheduled instantly by the hypervisor.
Signed-off-by: Parth Shah <parth@linux.ibm.com>
---
arch/powerpc/include/asm/hvcall.h | 3 ++-
arch/powerpc/kvm/book3s_hv.c | 13 +++++++++++++
arch/powerpc/kvm/trace_hv.h | 1 +
include/linux/kvm_host.h | 1 +
include/linux/sched.h | 1 +
kernel/sched/core.c | 13 +++++++++++++
kernel/sched/fair.c | 12 ++++++++++++
kernel/sched/sched.h | 1 +
virt/kvm/kvm_main.c | 17 +++++++++++++++++
9 files changed, 61 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index c98f5141e3fc..c91e27840c03 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -315,7 +315,8 @@
#define H_SCM_HEALTH 0x400
#define H_SCM_PERFORMANCE_STATS 0x418
#define H_RPT_INVALIDATE 0x448
-#define MAX_HCALL_OPCODE H_RPT_INVALIDATE
+#define H_IDLE_HINT 0x44C
+#define MAX_HCALL_OPCODE H_IDLE_HINT
/* Scope args for H_SCM_UNBIND_ALL */
#define H_UNBIND_SCOPE_ALL (0x1)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 6f612d240392..0472b8a1302f 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -931,6 +931,17 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
if (tvcpu->arch.ceded)
kvmppc_fast_vcpu_kick_hv(tvcpu);
break;
+ case H_IDLE_HINT:
+ target = kvmppc_get_gpr(vcpu, 4);
+ tvcpu = kvmppc_find_vcpu(vcpu->kvm, target);
+ if (!tvcpu) {
+ ret = H_PARAMETER;
+ break;
+ }
+ ret = kvm_vcpu_provide_idle_hint(tvcpu);
+ kvmppc_set_gpr(vcpu, 4, ret);
+ ret = H_SUCCESS;
+ break;
case H_CONFER:
target = kvmppc_get_gpr(vcpu, 4);
if (target == -1)
@@ -1145,6 +1156,7 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
case H_CEDE:
case H_PROD:
case H_CONFER:
+ case H_IDLE_HINT:
case H_REGISTER_VPA:
case H_SET_MODE:
case H_LOGICAL_CI_LOAD:
@@ -5359,6 +5371,7 @@ static unsigned int default_hcall_list[] = {
H_PROD,
H_CONFER,
H_REGISTER_VPA,
+ H_IDLE_HINT,
#ifdef CONFIG_KVM_XICS
H_EOI,
H_CPPR,
diff --git a/arch/powerpc/kvm/trace_hv.h b/arch/powerpc/kvm/trace_hv.h
index 830a126e095d..d0302a917eaf 100644
--- a/arch/powerpc/kvm/trace_hv.h
+++ b/arch/powerpc/kvm/trace_hv.h
@@ -46,6 +46,7 @@
{H_CEDE, "H_CEDE"}, \
{H_CONFER, "H_CONFER"}, \
{H_PROD, "H_PROD"}, \
+ {H_IDLE_HINT, "H_IDLE_HINT"}, \
{H_GET_PPP, "H_GET_PPP"}, \
{H_SET_PPP, "H_SET_PPP"}, \
{H_PURR, "H_PURR"}, \
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f3b1013fb22c..78fb0465cd65 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -843,6 +843,7 @@ bool kvm_vcpu_wake_up(struct kvm_vcpu *vcpu);
void kvm_vcpu_kick(struct kvm_vcpu *vcpu);
int kvm_vcpu_yield_to(struct kvm_vcpu *target);
void kvm_vcpu_on_spin(struct kvm_vcpu *vcpu, bool usermode_vcpu_not_eligible);
+unsigned long kvm_vcpu_provide_idle_hint(struct kvm_vcpu *target);
void kvm_flush_remote_tlbs(struct kvm *kvm);
void kvm_reload_remote_mmus(struct kvm *kvm);
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6e3a5eeec509..3dea2a4ff58d 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1688,6 +1688,7 @@ static inline int set_cpus_allowed_ptr(struct task_struct *p, const struct cpuma
extern int yield_to(struct task_struct *p, bool preempt);
extern void set_user_nice(struct task_struct *p, long nice);
extern int task_prio(const struct task_struct *p);
+extern unsigned long get_idle_hint(struct task_struct *p);
/**
* task_nice - return the nice value of a given task.
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index ff74fca39ed2..2962bf97ab13 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6812,6 +6812,19 @@ int __sched yield_to(struct task_struct *p, bool preempt)
}
EXPORT_SYMBOL_GPL(yield_to);
+/*
+ * Provide hint to the VM indicating if the previous vCPU can be scheduled
+ * instantly or not.
+ */
+unsigned long __sched get_idle_hint(struct task_struct *p)
+{
+ unsigned long ret = 0;
+ if (p->sched_class->get_idle_hint)
+ ret = p->sched_class->get_idle_hint(p);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(get_idle_hint);
+
int io_schedule_prepare(void)
{
int old_iowait = current->in_iowait;
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 04a3ce20da67..16701a3da5dc 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7236,6 +7236,16 @@ static bool yield_to_task_fair(struct rq *rq, struct task_struct *p)
return true;
}
+static unsigned long get_idle_hint_fair(struct task_struct *p)
+{
+ unsigned int prev_cpu = task_cpu(p);
+
+ if (available_idle_cpu(prev_cpu) || sched_idle_cpu(prev_cpu))
+ return 1;
+
+ return 0;
+}
+
#ifdef CONFIG_SMP
/**************************************************
* Fair scheduling class load-balancing methods.
@@ -11264,6 +11274,8 @@ DEFINE_SCHED_CLASS(fair) = {
.task_change_group = task_change_group_fair,
#endif
+ .get_idle_hint = get_idle_hint_fair,
+
#ifdef CONFIG_UCLAMP_TASK
.uclamp_enabled = 1,
#endif
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index bb09988451a0..09b1e35d8331 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1871,6 +1871,7 @@ struct sched_class {
#ifdef CONFIG_FAIR_GROUP_SCHED
void (*task_change_group)(struct task_struct *p, int type);
#endif
+ unsigned long (*get_idle_hint)(struct task_struct *p);
};
static inline void put_prev_task(struct rq *rq, struct task_struct *prev)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8367d88ce39b..5d750ae2fe0a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2907,6 +2907,23 @@ int kvm_vcpu_yield_to(struct kvm_vcpu *target)
}
EXPORT_SYMBOL_GPL(kvm_vcpu_yield_to);
+unsigned long kvm_vcpu_provide_idle_hint(struct kvm_vcpu *target)
+{
+ struct pid *pid;
+ struct task_struct *task = NULL;
+
+ rcu_read_lock();
+ pid = rcu_dereference(target->pid);
+ if (pid)
+ task = get_pid_task(pid, PIDTYPE_PID);
+ rcu_read_unlock();
+ if (!task)
+ return 0;
+
+ return get_idle_hint(task);
+}
+EXPORT_SYMBOL_GPL(kvm_vcpu_provide_idle_hint);
+
/*
* Helper that checks whether a VCPU is eligible for directed yield.
* Most eligible candidate to yield is decided by following heuristics:
--
2.26.2
^ permalink raw reply related
* [RFC 2/2] sched: Use H_IDLE_HINT hcall to find if a vCPU can be wakeup target
From: Parth Shah @ 2021-04-01 11:59 UTC (permalink / raw)
To: linuxppc-dev; +Cc: ego, mikey, srikar, npiggin, paulus, svaidy
In-Reply-To: <20210401115922.1524705-1-parth@linux.ibm.com>
During checking for an available_idle_cpu, if the vCPU is yielded then
check if it will be scheduled instantly by hypervisor or not. From guestOS,
use H_IDLE_HINT hcall to ask for this hint from the hypverisor, and
consider the yielded vCPU as target for wakeups iff it is hinted to be
scheduled instantly.
Signed-off-by: Parth Shah <parth@linux.ibm.com>
---
arch/powerpc/include/asm/paravirt.h | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
index edc08f04aef7..c7dd0368e1a4 100644
--- a/arch/powerpc/include/asm/paravirt.h
+++ b/arch/powerpc/include/asm/paravirt.h
@@ -41,6 +41,15 @@ static inline void yield_to_any(void)
{
plpar_hcall_norets(H_CONFER, -1, 0);
}
+
+/* Find if the previous physical CPU of this vcpu is available_idle or not */
+static inline void pcpu_available_instantly(int vcpu, unsigned long *is_idle)
+{
+ unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
+
+ if (plpar_hcall(H_IDLE_HINT, retbuf, vcpu) == H_SUCCESS)
+ *is_idle = retbuf[0];
+}
#else
static inline bool is_shared_processor(void)
{
@@ -75,6 +84,8 @@ static inline void prod_cpu(int cpu)
#define vcpu_is_preempted vcpu_is_preempted
static inline bool vcpu_is_preempted(int cpu)
{
+ unsigned long is_idle = 0;
+
if (!is_shared_processor())
return false;
@@ -92,8 +103,14 @@ static inline bool vcpu_is_preempted(int cpu)
}
#endif
- if (yield_count_of(cpu) & 1)
- return true;
+ if (yield_count_of(cpu) & 1) {
+#ifdef CONFIG_PPC_SPLPAR
+ pcpu_available_instantly(cpu, &is_idle);
+#endif
+
+ if (!is_idle)
+ return true;
+ }
return false;
}
--
2.26.2
^ permalink raw reply related
* Re: [PATCH v4 29/46] KVM: PPC: Book3S HV P9: Implement the rest of the P9 path in C
From: Nicholas Piggin @ 2021-04-01 10:35 UTC (permalink / raw)
To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <56dc4f3f-789d-0bbe-1b1f-508dbdfae487@ozlabs.ru>
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.
>
>
>> +
>> + 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).
>> +
>> + 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,
Nick
^ permalink raw reply
* Re: [PATCH] soc/fsl: qbman: fix conflicting alignment attributes
From: Arnd Bergmann @ 2021-04-01 10:23 UTC (permalink / raw)
To: Roy Pledge, Claudiu Manoil, Scott Wood, Arnd Bergmann, Li Yang
Cc: Arnd Bergmann, linuxppc-dev, Sebastian Andrzej Siewior,
YueHaibing, linux-kernel, soc, Jakub Kicinski, Lee Jones,
linux-arm-kernel
In-Reply-To: <20210323131530.2619900-1-arnd@kernel.org>
From: Arnd Bergmann <arnd@arndb.de>
On Tue, 23 Mar 2021 14:15:23 +0100, Arnd Bergmann wrote:
> When building with W=1, gcc points out that the __packed attribute
> on struct qm_eqcr_entry conflicts with the 8-byte alignment
> attribute on struct qm_fd inside it:
>
> drivers/soc/fsl/qbman/qman.c:189:1: error: alignment 1 of 'struct qm_eqcr_entry' is less than 8 [-Werror=packed-not-aligned]
>
> I assume that the alignment attribute is the correct one, and
> that qm_eqcr_entry cannot actually be unaligned in memory,
> so add the same alignment on the outer struct.
Applied to arm/fixes
[1/1] soc/fsl: qbman: fix conflicting alignment attributes
commit: b9ab2619047edc1c6c07dc68963d84a5979f0abe
Arnd
^ permalink raw reply
* Re: [PATCH v4 24/46] KVM: PPC: Book3S HV P9: Use large decrementer for HDEC
From: Nicholas Piggin @ 2021-04-01 10:14 UTC (permalink / raw)
To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <11ba0a43-a64a-ca06-581c-e8b7dc97b1d7@ozlabs.ru>
Excerpts from Alexey Kardashevskiy's message of March 26, 2021 12:05 pm:
>
>
> On 23/03/2021 12:02, Nicholas Piggin wrote:
>> On processors that don't suppress the HDEC exceptions when LPCR[HDICE]=0,
>> this could help reduce needless guest exits due to leftover exceptions on
>> entering the guest.
>>
>> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>
>
> ERROR: modpost: "decrementer_max" [arch/powerpc/kvm/kvm-hv.ko] undefined!
>
>
> need this:
>
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -89,6 +89,7 @@ static struct clocksource clocksource_timebase = {
>
> #define DECREMENTER_DEFAULT_MAX 0x7FFFFFFF
> u64 decrementer_max = DECREMENTER_DEFAULT_MAX;
> +EXPORT_SYMBOL_GPL(decrementer_max);
Added.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v4 15/46] KVM: PPC: Book3S 64: Move hcall early register setup to KVM
From: Nicholas Piggin @ 2021-04-01 10:03 UTC (permalink / raw)
To: Paul Mackerras; +Cc: linuxppc-dev, kvm-ppc
In-Reply-To: <YGVdFrsEtD88oB90@thinks.paulus.ozlabs.org>
Excerpts from Paul Mackerras's message of April 1, 2021 3:41 pm:
> On Tue, Mar 23, 2021 at 11:02:34AM +1000, Nicholas Piggin wrote:
>> System calls / hcalls have a different calling convention than
>> other interrupts, so there is code in the KVMTEST to massage these
>> into the same form as other interrupt handlers.
>>
>> Move this work into the KVM hcall handler. This means teaching KVM
>> a little more about the low level interrupt handler setup, PACA save
>> areas, etc., although that's not obviously worse than the current
>> approach of coming up with an entirely different interrupt register
>> / save convention.
>
> [snip]
>
>> @@ -1964,29 +1948,8 @@ EXC_VIRT_END(system_call, 0x4c00, 0x100)
>>
>> #ifdef CONFIG_KVM_BOOK3S_64_HANDLER
>> TRAMP_REAL_BEGIN(system_call_kvm)
>> - /*
>> - * This is a hcall, so register convention is as above, with these
>> - * differences:
>
> I haven't checked all the code changes in detail yet, but this comment
> at least is slightly misleading, since under PR KVM, system calls (to
> the guest kernel) and hypercalls both come through this path.
Yeah good point, I'll update the comment at its destination.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 16/18] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
From: Christoph Hellwig @ 2021-04-01 9:59 UTC (permalink / raw)
To: Robin Murphy
Cc: kvm, Will Deacon, linux-arm-msm, linuxppc-dev, dri-devel, Li Yang,
iommu, netdev, Christoph Hellwig, virtualization, freedreno,
David Woodhouse, linux-arm-kernel
In-Reply-To: <81dd27fe-28ee-c800-fe5d-aaa64cb93513@arm.com>
For now I'll just pass the iommu_domain to iommu_get_dma_strict,
so that we can check for it. We can do additional cleanups on top
of that later.
^ permalink raw reply
* Re: [PATCH v4 13/46] KVM: PPC: Book3S 64: Move GUEST_MODE_SKIP test into KVM
From: Nicholas Piggin @ 2021-04-01 9:53 UTC (permalink / raw)
To: Paul Mackerras; +Cc: Fabiano Rosas, linuxppc-dev, kvm-ppc, Daniel Axtens
In-Reply-To: <YGVbApPydgwAU8cP@thinks.paulus.ozlabs.org>
Excerpts from Paul Mackerras's message of April 1, 2021 3:32 pm:
> On Tue, Mar 23, 2021 at 11:02:32AM +1000, Nicholas Piggin wrote:
>> Move the GUEST_MODE_SKIP logic into KVM code. This is quite a KVM
>> internal detail that has no real need to be in common handlers.
>>
>> Also add a comment explaining why this thing exists.
>
> [snip]
>
>> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
>> index 7a039ea78f15..a5412e24cc05 100644
>> --- a/arch/powerpc/kvm/book3s_64_entry.S
>> +++ b/arch/powerpc/kvm/book3s_64_entry.S
>> @@ -1,6 +1,7 @@
>> /* SPDX-License-Identifier: GPL-2.0-only */
>> #include <asm/asm-offsets.h>
>> #include <asm/cache.h>
>> +#include <asm/exception-64s.h>
>> #include <asm/kvm_asm.h>
>> #include <asm/kvm_book3s_asm.h>
>> #include <asm/ppc_asm.h>
>> @@ -20,9 +21,12 @@ kvmppc_interrupt:
>> * guest R12 saved in shadow VCPU SCRATCH0
>> * guest R13 saved in SPRN_SCRATCH0
>> */
>> -#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>> std r9,HSTATE_SCRATCH2(r13)
>> lbz r9,HSTATE_IN_GUEST(r13)
>> + cmpwi r9,KVM_GUEST_MODE_SKIP
>> + beq- .Lmaybe_skip
>> +.Lno_skip:
>> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>> cmpwi r9,KVM_GUEST_MODE_HOST_HV
>> beq kvmppc_bad_host_intr
>> #ifdef CONFIG_KVM_BOOK3S_PR_POSSIBLE
>> @@ -34,3 +38,48 @@ kvmppc_interrupt:
>> #else
>> b kvmppc_interrupt_pr
>> #endif
>
> It's a bit hard to see without more context, but I think that in the
> PR-only case (CONFIG_KVM_BOOK3S_HV_POSSIBLE undefined), this will
> corrupt R9. You need to restore R9 before the unconditional branch to
> kvmppc_interrupt_pr. (I realize this code gets modified further, but
> I'd rather not break bisection.)
Very good catch, thanks.
Thanks,
Nick
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox