* Re: [PATCH next v1 2/3] printk: remove safe buffers
From: John Ogness @ 2021-03-22 11:16 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Rafael Aquini, Peter Zijlstra, Paul Mackerras, Tiezhu Yang,
Alexey Kardashevskiy, Yue Hu, Jordan Niethe, Petr Mladek,
Kees Cook, Paul E. McKenney, Alistair Popple,
Guilherme G. Piccoli, Nicholas Piggin, Steven Rostedt,
Thomas Gleixner, kexec, linux-kernel, Eric Biederman,
Andrew Morton, linuxppc-dev, Cédric Le Goater
In-Reply-To: <YFbY8kF7ilYoxvYp@google.com>
On 2021-03-21, Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
>> @@ -369,7 +70,10 @@ __printf(1, 0) int vprintk_func(const char *fmt, va_list args)
>> * Use the main logbuf even in NMI. But avoid calling console
>> * drivers that might have their own locks.
>> */
>> - if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) {
>> + if (this_cpu_read(printk_context) &
>> + (PRINTK_NMI_DIRECT_CONTEXT_MASK |
>> + PRINTK_NMI_CONTEXT_MASK |
>> + PRINTK_SAFE_CONTEXT_MASK)) {
>
> Do we need printk_nmi_direct_enter/exit() and
> PRINTK_NMI_DIRECT_CONTEXT_MASK? Seems like all printk_safe() paths
> are now DIRECT - we store messages to the prb, but don't call console
> drivers.
I was planning on waiting until the kthreads are introduced, in which
case printk_safe.c is completely removed. But I suppose I could switch
the 1 printk_nmi_direct_enter() user to printk_nmi_enter() so that
PRINTK_NMI_DIRECT_CONTEXT_MASK can be removed now. I would do this in a
4th patch of the series.
John Ogness
^ permalink raw reply
* Re: [PATCH v11 1/6] kasan: allow an architecture to disable inline instrumentation
From: Michael Ellerman @ 2021-03-22 11:08 UTC (permalink / raw)
To: Daniel Axtens, Balbir Singh
Cc: aneesh.kumar, linux-kernel, linux-mm, kasan-dev, linuxppc-dev
In-Reply-To: <87r1k8av4j.fsf@dja-thinkpad.axtens.net>
Daniel Axtens <dja@axtens.net> writes:
> Balbir Singh <bsingharora@gmail.com> writes:
>
>> On Sat, Mar 20, 2021 at 01:40:53AM +1100, Daniel Axtens wrote:
>>> For annoying architectural reasons, it's very difficult to support inline
>>> instrumentation on powerpc64.
>>
>> I think we can expand here and talk about how in hash mode, the vmalloc
>> address space is in a region of memory different than where kernel virtual
>> addresses are mapped. Did I recollect the reason correctly?
>
> I think that's _a_ reason, but for radix mode (which is all I support at
> the moment), the reason is a bit simpler.
Actually Aneesh fixed that in:
0034d395f89d ("powerpc/mm/hash64: Map all the kernel regions in the same 0xc range")
The problem we had prior to that was that the linear mapping was at
(0xc << 60), vmalloc was at (0xd << 60), and vmemap was at (0xf << 60).
Meaning our shadow region would need to be more than (3 << 60) in size.
cheers
^ permalink raw reply
* [PATCH] powerpc/iommu/debug: Remove redundant NULL check
From: Jiapeng Chong @ 2021-03-22 9:43 UTC (permalink / raw)
To: mpe; +Cc: Jiapeng Chong, paulus, linuxppc-dev, linux-kernel
Fix the following coccicheck warnings:
./fs/io_uring.c:5989:4-9: WARNING: NULL check before some freeing
functions is not needed.
Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
arch/powerpc/kernel/iommu.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
index c00214a..2168714 100644
--- a/arch/powerpc/kernel/iommu.c
+++ b/arch/powerpc/kernel/iommu.c
@@ -72,8 +72,7 @@ static void iommu_debugfs_del(struct iommu_table *tbl)
sprintf(name, "%08lx", tbl->it_index);
liobn_entry = debugfs_lookup(name, iommu_debugfs_dir);
- if (liobn_entry)
- debugfs_remove(liobn_entry);
+ debugfs_remove(liobn_entry);
}
#else
static void iommu_debugfs_add(struct iommu_table *tbl){}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v3 24/41] powerpc: add set_dec_or_work API for safely updating decrementer
From: Alexey Kardashevskiy @ 2021-03-22 9:38 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210305150638.2675513-25-npiggin@gmail.com>
On 06/03/2021 02:06, Nicholas Piggin wrote:
> 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.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/include/asm/time.h | 9 +++++++++
> arch/powerpc/kernel/time.c | 20 +++++++++++---------
> 2 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
> index 0128cd9769bc..d62bde57bf02 100644
> --- a/arch/powerpc/include/asm/time.h
> +++ b/arch/powerpc/include/asm/time.h
> @@ -78,6 +78,15 @@ static inline void set_dec(u64 val)
> mtspr(SPRN_DEC, val - 1);
> }
>
> +#ifdef CONFIG_IRQ_WORK
> +void set_dec_or_work(u64 val);
> +#else
> +static inline void set_dec_or_work(u64 val)
> +{
> + set_dec(val);
> +}
> +#endif
> +
> static inline unsigned long tb_ticks_since(unsigned long tstamp)
> {
> return mftb() - tstamp;
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index c5d524622c17..341cc8442e5e 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -562,6 +562,15 @@ void arch_irq_work_raise(void)
> preempt_enable();
> }
>
> +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);
> +}
> +EXPORT_SYMBOL_GPL(set_dec_or_work);
> +
> #else /* CONFIG_IRQ_WORK */
>
> #define test_irq_work_pending() 0
> @@ -629,10 +638,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);
> }
>
> @@ -874,11 +880,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;
> }
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 5/6] powerpc/mm/64s/hash: Add real-mode change_memory_range() for hash LPAR
From: Michael Ellerman @ 2021-03-22 9:07 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: aneesh.kumar
In-Reply-To: <1616382012.2m85hefs07.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Michael Ellerman's message of March 16, 2021 4:40 pm:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>>> Excerpts from Michael Ellerman's message of February 11, 2021 11:51 pm:
>>>> When we enabled STRICT_KERNEL_RWX we received some reports of boot
>>>> failures when using the Hash MMU and running under phyp. The crashes
>>>> are intermittent, and often exhibit as a completely unresponsive
>>>> system, or possibly an oops.
>> ...
>>>>
>>>> diff --git a/arch/powerpc/mm/book3s64/hash_pgtable.c b/arch/powerpc/mm/book3s64/hash_pgtable.c
>>>> index 3663d3cdffac..01de985df2c4 100644
>>>> --- a/arch/powerpc/mm/book3s64/hash_pgtable.c
>>>> +++ b/arch/powerpc/mm/book3s64/hash_pgtable.c
>>>> @@ -414,6 +428,73 @@ static void change_memory_range(unsigned long start, unsigned long end,
>>>> mmu_kernel_ssize);
>>>> }
>>>>
>>>> +static int notrace chmem_secondary_loop(struct change_memory_parms *parms)
>>>> +{
>>>> + unsigned long msr, tmp, flags;
>>>> + int *p;
>>>> +
>>>> + p = &parms->cpu_counter.counter;
>>>> +
>>>> + local_irq_save(flags);
>>>> + __hard_EE_RI_disable();
>>>> +
>>>> + asm volatile (
>>>> + // Switch to real mode and leave interrupts off
>>>> + "mfmsr %[msr] ;"
>>>> + "li %[tmp], %[MSR_IR_DR] ;"
>>>> + "andc %[tmp], %[msr], %[tmp] ;"
>>>> + "mtmsrd %[tmp] ;"
>>>> +
>>>> + // Tell the master we are in real mode
>>>> + "1: "
>>>> + "lwarx %[tmp], 0, %[p] ;"
>>>> + "addic %[tmp], %[tmp], -1 ;"
>>>> + "stwcx. %[tmp], 0, %[p] ;"
>>>> + "bne- 1b ;"
>>>> +
>>>> + // Spin until the counter goes to zero
>>>> + "2: ;"
>>>> + "lwz %[tmp], 0(%[p]) ;"
>>>> + "cmpwi %[tmp], 0 ;"
>>>> + "bne- 2b ;"
>>>> +
>>>> + // Switch back to virtual mode
>>>> + "mtmsrd %[msr] ;"
>>>
>>> Pity we don't have something that can switch to emergency stack and
>>> so we can write this stuff in C.
>>>
>>> How's something like this suit you?
>>
>> It looks like it would be really good for writing exploits :)
>
> Hmm. In that case maybe the callee function could be inlined into it
> like the interrupt wrappers, and the asm real-mode entry/exit gets
> added around it rather than have this little exploit stub. So similar to
> yours but with a stack switch as well so you can come back up in real
> mode.
Yeah inlining as much as possible would reduce the risk.
>> I think at the very least we would want the asm part to load the SP
>> from the paca itself, rather than taking it as a parameter.
>>
>> But I'm not sure writing these type of things in C is a big win, because
>> you have to be so careful about what you call anyway. It's almost better
>> in asm because it's so restrictive.
>>
>> Obviously having said that, my first attempt got the IRQ save/restore
>> wrong, so maybe we should at least have some macros to help with it.
>>
>> Did you have another user for this in mind? The only one that I can
>> think of at the moment is the subcore stuff.
>
> Possibly rtas entry/exit (although that has other issues). But I guess
> it's not a huge amount of asm compared with what I'm dealing with.
Ah yep, I hadn't thought of RTAS.
> I'm okay if you just put your thing in at the moment, we might or might
> not get keen and c-ify it later.
OK.
cheers
^ permalink raw reply
* Re: [PATCH v3 20/41] KVM: PPC: Book3S HV P9: Move setting HDEC after switching to guest LPCR
From: Alexey Kardashevskiy @ 2021-03-22 8:39 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210305150638.2675513-21-npiggin@gmail.com>
On 06/03/2021 02:06, Nicholas Piggin wrote:
> LPCR[HDICE]=0 suppresses hypervisor decrementer exceptions on some
> processors, so it must be enabled before HDEC is set.
Educating myself - is not it a processor bug when it does not suppress
hdec exceptions with HDICE=0?
Also, why do we want to enable interrupts before writing HDEC? Enabling
it may cause an interrupt right away a
Anyway whatever the answers are, this is not changed by this patch and
the change makes sense so
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> Rather than set it in the host LPCR then setting HDEC, move the HDEC
> update to after the guest MMU context (including LPCR) is loaded.
> There shouldn't be much concern with delaying HDEC by some 10s or 100s
> of nanoseconds by setting it a bit later.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kvm/book3s_hv.c | 19 +++++++------------
> 1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 1f2ba8955c6a..ffde1917ab68 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3505,20 +3505,9 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
> host_dawrx1 = mfspr(SPRN_DAWRX1);
> }
>
> - /*
> - * P8 and P9 suppress the HDEC exception when LPCR[HDICE] = 0,
> - * so set HDICE before writing HDEC.
> - */
> - mtspr(SPRN_LPCR, kvm->arch.host_lpcr | LPCR_HDICE);
> - isync();
> -
> hdec = time_limit - mftb();
> - if (hdec < 0) {
> - mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
> - isync();
> + if (hdec < 0)
> return BOOK3S_INTERRUPT_HV_DECREMENTER;
> - }
> - mtspr(SPRN_HDEC, hdec);
>
> if (vc->tb_offset) {
> u64 new_tb = mftb() + vc->tb_offset;
> @@ -3564,6 +3553,12 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>
> switch_mmu_to_guest_radix(kvm, vcpu, lpcr);
>
> + /*
> + * P9 suppresses the HDEC exception when LPCR[HDICE] = 0,
> + * so set guest LPCR (with HDICE) before writing HDEC.
> + */
> + mtspr(SPRN_HDEC, hdec);
> +
> mtspr(SPRN_SRR0, vcpu->arch.shregs.srr0);
> mtspr(SPRN_SRR1, vcpu->arch.shregs.srr1);
>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v11 1/6] kasan: allow an architecture to disable inline instrumentation
From: Marco Elver @ 2021-03-22 8:18 UTC (permalink / raw)
To: Daniel Axtens
Cc: aneesh.kumar, LKML, Linux Memory Management List, kasan-dev,
linuxppc-dev
In-Reply-To: <20210319144058.772525-2-dja@axtens.net>
On Fri, 19 Mar 2021 at 15:41, Daniel Axtens <dja@axtens.net> wrote:
>
> For annoying architectural reasons, it's very difficult to support inline
> instrumentation on powerpc64.
>
> Add a Kconfig flag to allow an arch to disable inline. (It's a bit
> annoying to be 'backwards', but I'm not aware of any way to have
> an arch force a symbol to be 'n', rather than 'y'.)
>
> We also disable stack instrumentation in this case as it does things that
> are functionally equivalent to inline instrumentation, namely adding
> code that touches the shadow directly without going through a C helper.
>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> ---
> lib/Kconfig.kasan | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/lib/Kconfig.kasan b/lib/Kconfig.kasan
> index cffc2ebbf185..7e237dbb6df3 100644
> --- a/lib/Kconfig.kasan
> +++ b/lib/Kconfig.kasan
> @@ -12,6 +12,9 @@ config HAVE_ARCH_KASAN_HW_TAGS
> config HAVE_ARCH_KASAN_VMALLOC
> bool
>
> +config ARCH_DISABLE_KASAN_INLINE
> + def_bool n
> +
Does just "bool" work here?
> config CC_HAS_KASAN_GENERIC
> def_bool $(cc-option, -fsanitize=kernel-address)
>
> @@ -130,6 +133,7 @@ config KASAN_OUTLINE
>
> config KASAN_INLINE
> bool "Inline instrumentation"
> + depends on !ARCH_DISABLE_KASAN_INLINE
> help
> Compiler directly inserts code checking shadow memory before
> memory accesses. This is faster than outline (in some workloads
> @@ -142,6 +146,7 @@ config KASAN_STACK
> bool "Enable stack instrumentation (unsafe)" if CC_IS_CLANG && !COMPILE_TEST
> depends on KASAN_GENERIC || KASAN_SW_TAGS
> default y if CC_IS_GCC
> + depends on !ARCH_DISABLE_KASAN_INLINE
Minor, but perhaps this 'depends on' line could be moved up 1 line to
be grouped with the other 'depends on'.
> help
> The LLVM stack address sanitizer has a know problem that
> causes excessive stack usage in a lot of functions, see
> @@ -154,6 +159,9 @@ config KASAN_STACK
> but clang users can still enable it for builds without
> CONFIG_COMPILE_TEST. On gcc it is assumed to always be safe
> to use and enabled by default.
> + If the architecture disables inline instrumentation, this is
> + also disabled as it adds inline-style instrumentation that
> + is run unconditionally.
>
> config KASAN_SW_TAGS_IDENTIFY
> bool "Enable memory corruption identification"
> --
> 2.27.0
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20210319144058.772525-2-dja%40axtens.net.
^ permalink raw reply
* Re: [PATCH v11 2/6] kasan: allow architectures to provide an outline readiness check
From: Marco Elver @ 2021-03-22 8:17 UTC (permalink / raw)
To: Daniel Axtens
Cc: aneesh.kumar, LKML, Linux Memory Management List, kasan-dev,
linuxppc-dev, Aneesh Kumar K . V
In-Reply-To: <20210319144058.772525-3-dja@axtens.net>
On Fri, 19 Mar 2021 at 15:41, Daniel Axtens <dja@axtens.net> wrote:
> Allow architectures to define a kasan_arch_is_ready() hook that bails
> out of any function that's about to touch the shadow unless the arch
> says that it is ready for the memory to be accessed. This is fairly
> uninvasive and should have a negligible performance penalty.
>
> This will only work in outline mode, so an arch must specify
> ARCH_DISABLE_KASAN_INLINE if it requires this.
>
> Cc: Balbir Singh <bsingharora@gmail.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Suggested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Signed-off-by: Daniel Axtens <dja@axtens.net>
>
> --
>
> I discuss the justfication for this later in the series. Also,
> both previous RFCs for ppc64 - by 2 different people - have
> needed this trick! See:
> - https://lore.kernel.org/patchwork/patch/592820/ # ppc64 hash series
> - https://patchwork.ozlabs.org/patch/795211/ # ppc radix series
> ---
> include/linux/kasan.h | 4 ++++
> mm/kasan/common.c | 4 ++++
> mm/kasan/generic.c | 3 +++
> mm/kasan/shadow.c | 4 ++++
> 4 files changed, 15 insertions(+)
>
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index 8b3b99d659b7..6bd8343f0033 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
Does kasan_arch_is_ready() need to be defined in the public interface
of KASAN? Could it instead be moved to mm/kasan/kasan.h?
> @@ -23,6 +23,10 @@ struct kunit_kasan_expectation {
>
> #endif
>
> +#ifndef kasan_arch_is_ready
> +static inline bool kasan_arch_is_ready(void) { return true; }
> +#endif
> +
> #if defined(CONFIG_KASAN_GENERIC) || defined(CONFIG_KASAN_SW_TAGS)
>
> #include <linux/pgtable.h>
> diff --git a/mm/kasan/common.c b/mm/kasan/common.c
> index 6bb87f2acd4e..f23a9e2dce9f 100644
> --- a/mm/kasan/common.c
> +++ b/mm/kasan/common.c
> @@ -345,6 +345,10 @@ static inline bool ____kasan_slab_free(struct kmem_cache *cache, void *object,
> if (unlikely(cache->flags & SLAB_TYPESAFE_BY_RCU))
> return false;
>
> + /* We can't read the shadow byte if the arch isn't ready */
> + if (!kasan_arch_is_ready())
> + return false;
> +
While it probably doesn't matter much, it seems this check could be
moved up, rather than having it in the middle here.
> if (!kasan_byte_accessible(tagged_object)) {
> kasan_report_invalid_free(tagged_object, ip);
> return true;
> diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c
> index 53cbf28859b5..c3f5ba7a294a 100644
> --- a/mm/kasan/generic.c
> +++ b/mm/kasan/generic.c
> @@ -163,6 +163,9 @@ static __always_inline bool check_region_inline(unsigned long addr,
> size_t size, bool write,
> unsigned long ret_ip)
> {
> + if (!kasan_arch_is_ready())
> + return true;
> +
> if (unlikely(size == 0))
> return true;
>
> diff --git a/mm/kasan/shadow.c b/mm/kasan/shadow.c
> index 727ad4629173..1f650c521037 100644
> --- a/mm/kasan/shadow.c
> +++ b/mm/kasan/shadow.c
> @@ -80,6 +80,10 @@ void kasan_poison(const void *addr, size_t size, u8 value, bool init)
> */
> addr = kasan_reset_tag(addr);
>
> + /* Don't touch the shadow memory if arch isn't ready */
> + if (!kasan_arch_is_ready())
> + return;
> +
> /* Skip KFENCE memory if called explicitly outside of sl*b. */
> if (is_kfence_address(addr))
> return;
> --
> 2.27.0
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20210319144058.772525-3-dja%40axtens.net.
^ permalink raw reply
* Re: [PATCH v3 21/41] KVM: PPC: Book3S HV P9: Use large decrementer for HDEC
From: Alexey Kardashevskiy @ 2021-03-22 7:58 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210305150638.2675513-22-npiggin@gmail.com>
On 06/03/2021 02:06, 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.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/include/asm/time.h | 2 ++
> arch/powerpc/kvm/book3s_hv.c | 3 ++-
> 2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
> index 8dd3cdb25338..68d94711811e 100644
> --- a/arch/powerpc/include/asm/time.h
> +++ b/arch/powerpc/include/asm/time.h
> @@ -18,6 +18,8 @@
> #include <asm/vdso/timebase.h>
>
> /* time.c */
> +extern u64 decrementer_max;
> +
> extern unsigned long tb_ticks_per_jiffy;
> extern unsigned long tb_ticks_per_usec;
> extern unsigned long tb_ticks_per_sec;
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index ffde1917ab68..24b0680f0ad7 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3623,7 +3623,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
> vc->tb_offset_applied = 0;
> }
>
> - mtspr(SPRN_HDEC, 0x7fffffff);
> + /* HDEC must be at least as large as DEC, so decrementer_max fits */
> + mtspr(SPRN_HDEC, decrementer_max);
>
> switch_mmu_to_host_radix(kvm, host_pidr);
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 1/3] powerpc/mm/hash: Avoid resizing-down HPT on first memory hotplug
From: David Gibson @ 2021-03-22 6:49 UTC (permalink / raw)
To: Leonardo Bras
Cc: Nathan Lynch, David Hildenbrand, Scott Cheloha, linux-kernel,
linuxppc-dev, Nicholas Piggin, Bharata B Rao, Paul Mackerras,
Sandipan Das, Aneesh Kumar K.V, Andrew Morton, Laurent Dufour,
Logan Gunthorpe, Dan Williams, Mike Rapoport
In-Reply-To: <20210312072940.598696-2-leobras.c@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4282 bytes --]
On Fri, Mar 12, 2021 at 04:29:39AM -0300, Leonardo Bras wrote:
> Because hypervisors may need to create HPTs without knowing the guest
> page size, the smallest used page-size (4k) may be chosen, resulting in
> a HPT that is possibly bigger than needed.
>
> On a guest with bigger page-sizes, the amount of entries for HTP may be
> too high, causing the guest to ask for a HPT resize-down on the first
> hotplug.
>
> This becomes a problem when HPT resize-down fails, and causes the
> HPT resize to be performed on every LMB added, until HPT size is
> compatible to guest memory size, causing a major slowdown.
>
> So, avoiding HPT resizing-down on hot-add significantly improves memory
> hotplug times.
>
> As an example, hotplugging 256GB on a 129GB guest took 710s without this
> patch, and 21s after applied.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
I don't love this approach. Adding the extra flag at this level seems
a bit inelegant, and it means we're passing up an easy opportunity to
reduce our resource footprint on the host.
But... maybe we'll have to do it. I'd like to see if we can get
things to work well enough with just the "batching" to avoid multiple
resize attempts first.
> ---
> arch/powerpc/mm/book3s64/hash_utils.c | 36 ++++++++++++++++-----------
> 1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
> index 73b06adb6eeb..cfb3ec164f56 100644
> --- a/arch/powerpc/mm/book3s64/hash_utils.c
> +++ b/arch/powerpc/mm/book3s64/hash_utils.c
> @@ -794,7 +794,7 @@ static unsigned long __init htab_get_table_size(void)
> }
>
> #ifdef CONFIG_MEMORY_HOTPLUG
> -static int resize_hpt_for_hotplug(unsigned long new_mem_size)
> +static int resize_hpt_for_hotplug(unsigned long new_mem_size, bool shrinking)
> {
> unsigned target_hpt_shift;
>
> @@ -803,19 +803,25 @@ static int resize_hpt_for_hotplug(unsigned long new_mem_size)
>
> target_hpt_shift = htab_shift_for_mem_size(new_mem_size);
>
> - /*
> - * To avoid lots of HPT resizes if memory size is fluctuating
> - * across a boundary, we deliberately have some hysterisis
> - * here: we immediately increase the HPT size if the target
> - * shift exceeds the current shift, but we won't attempt to
> - * reduce unless the target shift is at least 2 below the
> - * current shift
> - */
> - if (target_hpt_shift > ppc64_pft_size ||
> - target_hpt_shift < ppc64_pft_size - 1)
> - return mmu_hash_ops.resize_hpt(target_hpt_shift);
> + if (shrinking) {
>
> - return 0;
> + /*
> + * To avoid lots of HPT resizes if memory size is fluctuating
> + * across a boundary, we deliberately have some hysterisis
> + * here: we immediately increase the HPT size if the target
> + * shift exceeds the current shift, but we won't attempt to
> + * reduce unless the target shift is at least 2 below the
> + * current shift
> + */
> +
> + if (target_hpt_shift >= ppc64_pft_size - 1)
> + return 0;
> +
> + } else if (target_hpt_shift <= ppc64_pft_size) {
> + return 0;
> + }
> +
> + return mmu_hash_ops.resize_hpt(target_hpt_shift);
> }
>
> int hash__create_section_mapping(unsigned long start, unsigned long end,
> @@ -828,7 +834,7 @@ int hash__create_section_mapping(unsigned long start, unsigned long end,
> return -1;
> }
>
> - resize_hpt_for_hotplug(memblock_phys_mem_size());
> + resize_hpt_for_hotplug(memblock_phys_mem_size(), false);
>
> rc = htab_bolt_mapping(start, end, __pa(start),
> pgprot_val(prot), mmu_linear_psize,
> @@ -847,7 +853,7 @@ int hash__remove_section_mapping(unsigned long start, unsigned long end)
> int rc = htab_remove_mapping(start, end, mmu_linear_psize,
> mmu_kernel_ssize);
>
> - if (resize_hpt_for_hotplug(memblock_phys_mem_size()) == -ENOSPC)
> + if (resize_hpt_for_hotplug(memblock_phys_mem_size(), true) == -ENOSPC)
> pr_warn("Hash collision while resizing HPT\n");
>
> return rc;
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] powerpc/mm/hash: Avoid multiple HPT resize-ups on memory hotplug
From: David Gibson @ 2021-03-22 7:55 UTC (permalink / raw)
To: Leonardo Bras
Cc: Nathan Lynch, David Hildenbrand, Scott Cheloha, linux-kernel,
linuxppc-dev, Nicholas Piggin, Bharata B Rao, Paul Mackerras,
Sandipan Das, Aneesh Kumar K.V, Andrew Morton, Laurent Dufour,
Logan Gunthorpe, Dan Williams, Mike Rapoport
In-Reply-To: <20210312072940.598696-3-leobras.c@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6496 bytes --]
On Fri, Mar 12, 2021 at 04:29:40AM -0300, Leonardo Bras wrote:
> Every time a memory hotplug happens, and the memory limit crosses a 2^n
> value, it may be necessary to perform HPT resizing-up, which can take
> some time (over 100ms in my tests).
>
> It usually is not an issue, but it can take some time if a lot of memory
> is added to a guest with little starting memory:
> Adding 256G to a 2GB guest, for example will require 8 HPT resizes.
>
> Perform an HPT resize before memory hotplug, updating HPT to its
> final size (considering a successful hotplug), taking the number of
> HPT resizes to at most one per memory hotplug action.
>
> Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> ---
> arch/powerpc/include/asm/book3s/64/hash.h | 2 ++
> arch/powerpc/include/asm/sparsemem.h | 2 ++
> arch/powerpc/mm/book3s64/hash_utils.c | 14 ++++++++++++++
> arch/powerpc/mm/book3s64/pgtable.c | 6 ++++++
> arch/powerpc/platforms/pseries/hotplug-memory.c | 6 ++++++
> 5 files changed, 30 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/hash.h b/arch/powerpc/include/asm/book3s/64/hash.h
> index d959b0195ad9..843b0a178590 100644
> --- a/arch/powerpc/include/asm/book3s/64/hash.h
> +++ b/arch/powerpc/include/asm/book3s/64/hash.h
> @@ -255,6 +255,8 @@ int hash__create_section_mapping(unsigned long start, unsigned long end,
> int nid, pgprot_t prot);
> int hash__remove_section_mapping(unsigned long start, unsigned long end);
>
> +void hash_memory_batch_expand_prepare(unsigned long newsize);
> +
> #endif /* !__ASSEMBLY__ */
> #endif /* __KERNEL__ */
> #endif /* _ASM_POWERPC_BOOK3S_64_HASH_H */
> diff --git a/arch/powerpc/include/asm/sparsemem.h b/arch/powerpc/include/asm/sparsemem.h
> index d072866842e4..16b5f5300c84 100644
> --- a/arch/powerpc/include/asm/sparsemem.h
> +++ b/arch/powerpc/include/asm/sparsemem.h
> @@ -17,6 +17,8 @@ extern int remove_section_mapping(unsigned long start, unsigned long end);
> extern int memory_add_physaddr_to_nid(u64 start);
> #define memory_add_physaddr_to_nid memory_add_physaddr_to_nid
>
> +void memory_batch_expand_prepare(unsigned long newsize);
> +
> #ifdef CONFIG_NUMA
> extern int hot_add_scn_to_nid(unsigned long scn_addr);
> #else
> diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
> index cfb3ec164f56..1f6aa0bf27e7 100644
> --- a/arch/powerpc/mm/book3s64/hash_utils.c
> +++ b/arch/powerpc/mm/book3s64/hash_utils.c
> @@ -858,6 +858,20 @@ int hash__remove_section_mapping(unsigned long start, unsigned long end)
>
> return rc;
> }
> +
> +void hash_memory_batch_expand_prepare(unsigned long newsize)
> +{
> + /*
> + * Resizing-up HPT should never fail, but there are some cases system starts with higher
> + * SHIFT than required, and we go through the funny case of resizing HPT down while
> + * adding memory
> + */
> +
> + while (resize_hpt_for_hotplug(newsize, false) == -ENOSPC) {
> + newsize *= 2;
> + pr_warn("Hash collision while resizing HPT\n");
This unbounded increase in newsize makes me nervous - we should be
bounded by the current size of the HPT at least. In practice we
should be fine, since the resize should always succeed by the time we
reach our current HPT size, but that's far from obvious from this
point in the code.
And... you're doubling newsize which is a value which might not be a
power of 2. I'm wondering if there's an edge case where this could
actually cause us to skip the current size and erroneously resize to
one bigger than we have currently.
> + }
> +}
> #endif /* CONFIG_MEMORY_HOTPLUG */
>
> static void __init hash_init_partition_table(phys_addr_t hash_table,
> diff --git a/arch/powerpc/mm/book3s64/pgtable.c b/arch/powerpc/mm/book3s64/pgtable.c
> index 5b3a3bae21aa..f1cd8af0f67f 100644
> --- a/arch/powerpc/mm/book3s64/pgtable.c
> +++ b/arch/powerpc/mm/book3s64/pgtable.c
> @@ -193,6 +193,12 @@ int __meminit remove_section_mapping(unsigned long start, unsigned long end)
>
> return hash__remove_section_mapping(start, end);
> }
> +
> +void memory_batch_expand_prepare(unsigned long newsize)
This wrapper doesn't seem useful.
> +{
> + if (!radix_enabled())
> + hash_memory_batch_expand_prepare(newsize);
> +}
> #endif /* CONFIG_MEMORY_HOTPLUG */
>
> void __init mmu_partition_table_init(void)
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 8377f1f7c78e..353c71249214 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -671,6 +671,8 @@ static int dlpar_memory_add_by_count(u32 lmbs_to_add)
> if (lmbs_available < lmbs_to_add)
> return -EINVAL;
>
> + memory_batch_expand_prepare(memblock_phys_mem_size() + lmbs_to_add * drmem_lmb_size());
> +
> for_each_drmem_lmb(lmb) {
> if (lmb->flags & DRCONF_MEM_ASSIGNED)
> continue;
> @@ -734,6 +736,8 @@ static int dlpar_memory_add_by_index(u32 drc_index)
>
> pr_info("Attempting to hot-add LMB, drc index %x\n", drc_index);
>
> + memory_batch_expand_prepare(memblock_phys_mem_size() +
> + drmem_info->n_lmbs * drmem_lmb_size());
This doesn't look right. memory_add_by_index() is adding a *single*
LMB, I think using drmem_info->n_lmbs here means you're counting this
as adding again as much memory as you already have hotplugged.
> lmb_found = 0;
> for_each_drmem_lmb(lmb) {
> if (lmb->drc_index == drc_index) {
> @@ -788,6 +792,8 @@ static int dlpar_memory_add_by_ic(u32 lmbs_to_add, u32 drc_index)
> if (lmbs_available < lmbs_to_add)
> return -EINVAL;
>
> + memory_batch_expand_prepare(memblock_phys_mem_size() + lmbs_to_add * drmem_lmb_size());
> +
> for_each_drmem_lmb_in_range(lmb, start_lmb, end_lmb) {
> if (lmb->flags & DRCONF_MEM_ASSIGNED)
> continue;
I don't see memory_batch_expand_prepare() suppressing any existing HPT
resizes. Won't this just resize to the right size for the full add,
then resize several times again as we perform the add? Or.. I guess
that will be suppressed by patch 1/3. That's seems kinda fragile,
though.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 19/41] KVM: PPC: Book3S HV P9: Stop handling hcalls in real-mode in the P9 path
From: Alexey Kardashevskiy @ 2021-03-22 7:30 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210305150638.2675513-20-npiggin@gmail.com>
On 06/03/2021 02:06, Nicholas Piggin wrote:
> In the interest of minimising the amount of code that is run in
> "real-mode", don't handle hcalls in real mode in the P9 path.
>
> POWER8 and earlier are much more expensive to exit from HV real mode
> and switch to host mode, because on those processors HV interrupts get
> to the hypervisor with the MMU off, and the other threads in the core
> need to be pulled out of the guest, and SLBs all need to be saved,
> ERATs invalidated, and host SLB reloaded before the MMU is re-enabled
> in host mode. Hash guests also require a lot of hcalls to run. The
> XICS interrupt controller requires hcalls to run.
>
> By contrast, POWER9 has independent thread switching, and in radix mode
> the hypervisor is already in a host virtual memory mode when the HV
> interrupt is taken. Radix + xive guests don't need hcalls to handle
> interrupts or manage translations.
>
> So it's much less important to handle hcalls in real mode in P9.
So acde25726bc6034b (which added if(kvm_is_radix(vcpu->kvm))return
H_TOO_HARD) can be reverted, pretty much?
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/kvm_ppc.h | 5 +++
> arch/powerpc/kvm/book3s_hv.c | 46 +++++++++++++++----
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 5 +++
> arch/powerpc/kvm/book3s_xive.c | 60 +++++++++++++++++++++++++
> 4 files changed, 108 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 73b1ca5a6471..db6646c2ade2 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -607,6 +607,7 @@ extern void kvmppc_free_pimap(struct kvm *kvm);
> extern int kvmppc_xics_rm_complete(struct kvm_vcpu *vcpu, u32 hcall);
> extern void kvmppc_xics_free_icp(struct kvm_vcpu *vcpu);
> extern int kvmppc_xics_hcall(struct kvm_vcpu *vcpu, u32 cmd);
> +extern int kvmppc_xive_xics_hcall(struct kvm_vcpu *vcpu, u32 req);
> extern u64 kvmppc_xics_get_icp(struct kvm_vcpu *vcpu);
> extern int kvmppc_xics_set_icp(struct kvm_vcpu *vcpu, u64 icpval);
> extern int kvmppc_xics_connect_vcpu(struct kvm_device *dev,
> @@ -639,6 +640,8 @@ static inline int kvmppc_xics_enabled(struct kvm_vcpu *vcpu)
> static inline void kvmppc_xics_free_icp(struct kvm_vcpu *vcpu) { }
> static inline int kvmppc_xics_hcall(struct kvm_vcpu *vcpu, u32 cmd)
> { return 0; }
> +static inline int kvmppc_xive_xics_hcall(struct kvm_vcpu *vcpu, u32 req)
> + { return 0; }
> #endif
>
> #ifdef CONFIG_KVM_XIVE
> @@ -673,6 +676,7 @@ extern int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 irq,
> int level, bool line_status);
> extern void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu);
> extern void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu);
> +extern void kvmppc_xive_cede_vcpu(struct kvm_vcpu *vcpu);
>
> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
> {
> @@ -714,6 +718,7 @@ static inline int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 ir
> int level, bool line_status) { return -ENODEV; }
> static inline void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu) { }
> static inline void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu) { }
> +static inline void kvmppc_xive_cede_vcpu(struct kvm_vcpu *vcpu) { }
>
> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
> { return 0; }
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 497f216ad724..1f2ba8955c6a 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1147,7 +1147,7 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
> * This has to be done early, not in kvmppc_pseries_do_hcall(), so
> * that the cede logic in kvmppc_run_single_vcpu() works properly.
> */
> -static void kvmppc_nested_cede(struct kvm_vcpu *vcpu)
> +static void kvmppc_cede(struct kvm_vcpu *vcpu)
> {
> vcpu->arch.shregs.msr |= MSR_EE;
> vcpu->arch.ceded = 1;
> @@ -1403,9 +1403,15 @@ static int kvmppc_handle_exit_hv(struct kvm_vcpu *vcpu,
> /* hcall - punt to userspace */
> int i;
>
> - /* hypercall with MSR_PR has already been handled in rmode,
> - * and never reaches here.
> - */
> + if (unlikely(vcpu->arch.shregs.msr & MSR_PR)) {
> + /*
> + * Guest userspace executed sc 1, reflect it back as a
> + * privileged program check interrupt.
> + */
> + kvmppc_core_queue_program(vcpu, SRR1_PROGPRIV);
> + r = RESUME_GUEST;
> + break;
> + }
>
> run->papr_hcall.nr = kvmppc_get_gpr(vcpu, 3);
> for (i = 0; i < 9; ++i)
> @@ -3740,15 +3746,36 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
> /* H_CEDE has to be handled now, not later */
> if (trap == BOOK3S_INTERRUPT_SYSCALL && !vcpu->arch.nested &&
> kvmppc_get_gpr(vcpu, 3) == H_CEDE) {
> - kvmppc_nested_cede(vcpu);
> + kvmppc_cede(vcpu);
> kvmppc_set_gpr(vcpu, 3, 0);
> trap = 0;
> }
> } else {
> kvmppc_xive_push_vcpu(vcpu);
> trap = kvmhv_load_hv_regs_and_go(vcpu, time_limit, lpcr);
> - kvmppc_xive_pull_vcpu(vcpu);
> + /* H_CEDE has to be handled now, not later */
> + /* XICS hcalls must be handled before xive is pulled */
> + if (trap == BOOK3S_INTERRUPT_SYSCALL &&
> + !(vcpu->arch.shregs.msr & MSR_PR)) {
> + unsigned long req = kvmppc_get_gpr(vcpu, 3);
>
> + if (req == H_CEDE) {
> + kvmppc_cede(vcpu);
> + kvmppc_xive_cede_vcpu(vcpu); /* may un-cede */
> + kvmppc_set_gpr(vcpu, 3, 0);
> + trap = 0;
> + }
> + if (req == H_EOI || req == H_CPPR ||
else if (req == H_EOI ... ?
> + req == H_IPI || req == H_IPOLL ||
> + req == H_XIRR || req == H_XIRR_X) {
> + unsigned long ret;
> +
> + ret = kvmppc_xive_xics_hcall(vcpu, req);
> + kvmppc_set_gpr(vcpu, 3, ret);
> + trap = 0;
> + }
> + }
> + kvmppc_xive_pull_vcpu(vcpu);
> }
>
> vcpu->arch.slb_max = 0;
> @@ -4408,8 +4435,11 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
> else
> r = kvmppc_run_vcpu(vcpu);
>
> - if (run->exit_reason == KVM_EXIT_PAPR_HCALL &&
> - !(vcpu->arch.shregs.msr & MSR_PR)) {
> + if (run->exit_reason == KVM_EXIT_PAPR_HCALL) {
> + if (WARN_ON_ONCE(vcpu->arch.shregs.msr & MSR_PR)) {
> + r = RESUME_GUEST;
> + continue;
> + }
> trace_kvm_hcall_enter(vcpu);
> r = kvmppc_pseries_do_hcall(vcpu);
> trace_kvm_hcall_exit(vcpu, r);
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index c11597f815e4..2d0d14ed1d92 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -1397,9 +1397,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> mr r4,r9
> bge fast_guest_return
> 2:
> + /* If we came in through the P9 short path, no real mode hcalls */
> + lwz r0, STACK_SLOT_SHORT_PATH(r1)
> + cmpwi r0, 0
> + bne no_try_real
> /* See if this is an hcall we can handle in real mode */
> cmpwi r12,BOOK3S_INTERRUPT_SYSCALL
> beq hcall_try_real_mode
> +no_try_real:
>
> /* Hypervisor doorbell - exit only if host IPI flag set */
> cmpwi r12, BOOK3S_INTERRUPT_H_DOORBELL
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index 52cdb9e2660a..1e4871bbcad4 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -158,6 +158,40 @@ void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu)
> }
> EXPORT_SYMBOL_GPL(kvmppc_xive_pull_vcpu);
>
> +void kvmppc_xive_cede_vcpu(struct kvm_vcpu *vcpu)
> +{
> + void __iomem *esc_vaddr = (void __iomem *)vcpu->arch.xive_esc_vaddr;
> +
> + if (!esc_vaddr)
> + return;
> +
> + /* we are using XIVE with single escalation */
> +
> + if (vcpu->arch.xive_esc_on) {
> + /*
> + * If we still have a pending escalation, abort the cede,
> + * and we must set PQ to 10 rather than 00 so that we don't
> + * potentially end up with two entries for the escalation
> + * interrupt in the XIVE interrupt queue. In that case
> + * we also don't want to set xive_esc_on to 1 here in
> + * case we race with xive_esc_irq().
> + */
> + vcpu->arch.ceded = 0;
> + /*
> + * The escalation interrupts are special as we don't EOI them.
> + * There is no need to use the load-after-store ordering offset
> + * to set PQ to 10 as we won't use StoreEOI.
> + */
> + __raw_readq(esc_vaddr + XIVE_ESB_SET_PQ_10);
> + } else {
> + vcpu->arch.xive_esc_on = true;
> + mb();
> + __raw_readq(esc_vaddr + XIVE_ESB_SET_PQ_00);
> + }
> + mb();
Uff. Thanks for cut-n-pasting the comments, helped a lot to match this c
to that asm!
> +}
> +EXPORT_SYMBOL_GPL(kvmppc_xive_cede_vcpu);
> +
> /*
> * This is a simple trigger for a generic XIVE IRQ. This must
> * only be called for interrupts that support a trigger page
> @@ -2106,6 +2140,32 @@ static int kvmppc_xive_create(struct kvm_device *dev, u32 type)
> return 0;
> }
>
> +int kvmppc_xive_xics_hcall(struct kvm_vcpu *vcpu, u32 req)
> +{
> + struct kvmppc_vcore *vc = vcpu->arch.vcore;
Can a XIVE enabled guest issue these hcalls? Don't we want if
(!kvmppc_xics_enabled(vcpu)) and
if (xics_on_xive()) here, as kvmppc_rm_h_xirr() have? Some of these
hcalls do write to XIVE registers but some seem to change
kvmppc_xive_vcpu. Thanks,
> +
> + switch (req) {
> + case H_XIRR:
> + return xive_vm_h_xirr(vcpu);
> + case H_CPPR:
> + return xive_vm_h_cppr(vcpu, kvmppc_get_gpr(vcpu, 4));
> + case H_EOI:
> + return xive_vm_h_eoi(vcpu, kvmppc_get_gpr(vcpu, 4));
> + case H_IPI:
> + return xive_vm_h_ipi(vcpu, kvmppc_get_gpr(vcpu, 4),
> + kvmppc_get_gpr(vcpu, 5));
> + case H_IPOLL:
> + return xive_vm_h_ipoll(vcpu, kvmppc_get_gpr(vcpu, 4));
> + case H_XIRR_X:
> + xive_vm_h_xirr(vcpu);
> + kvmppc_set_gpr(vcpu, 5, get_tb() + vc->tb_offset);
> + return H_SUCCESS;
> + }
> +
> + return H_UNSUPPORTED;
> +}
> +EXPORT_SYMBOL_GPL(kvmppc_xive_xics_hcall);
> +
> int kvmppc_xive_debug_show_queues(struct seq_file *m, struct kvm_vcpu *vcpu)
> {
> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>
--
Alexey
^ permalink raw reply
* [PATCH] docs: powerpc: Fix a typo
From: Bhaskar Chowdhury @ 2021-03-22 6:22 UTC (permalink / raw)
To: mpe, benh, paulus, corbet, linuxppc-dev, linux-doc, linux-kernel
Cc: rdunlap, Bhaskar Chowdhury
s/struture/structure/
Signed-off-by: Bhaskar Chowdhury <unixbhaskar@gmail.com>
---
Documentation/powerpc/firmware-assisted-dump.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/powerpc/firmware-assisted-dump.rst b/Documentation/powerpc/firmware-assisted-dump.rst
index 20ea8cdee0aa..6c0ae070ba67 100644
--- a/Documentation/powerpc/firmware-assisted-dump.rst
+++ b/Documentation/powerpc/firmware-assisted-dump.rst
@@ -171,7 +171,7 @@ that were present in CMA region::
(meta area) |
|
|
- Metadata: This area holds a metadata struture whose
+ Metadata: This area holds a metadata structure whose
address is registered with f/w and retrieved in the
second kernel after crash, on platforms that support
tags (OPAL). Having such structure with info needed
--
2.31.0
^ permalink raw reply related
* Re: [PATCH v3 16/41] KVM: PPC: Book3S HV P9: Move radix MMU switching instructions together
From: Alexey Kardashevskiy @ 2021-03-22 6:21 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <1616390221.1zx2axnuhk.astroid@bobo.none>
On 22/03/2021 16:25, Nicholas Piggin wrote:
> Excerpts from Alexey Kardashevskiy's message of March 22, 2021 2:24 pm:
>>
>>
>> On 06/03/2021 02:06, Nicholas Piggin wrote:
>>> Switching the MMU from radix<->radix mode is tricky particularly as the
>>> MMU can remain enabled and requires a certain sequence of SPR updates.
>>> Move these together into their own functions.
>>>
>>> This also includes the radix TLB check / flush because it's tied in to
>>> MMU switching due to tlbiel getting LPID from LPIDR.
>>>
>>> (XXX: isync / hwsync synchronisation TBD)
>>
>>
>> Looks alright but what is this comment about? Is something missing or
>> just sub optimal?
>
> Ah, yeah the architecture says for example a CSI is required before +
> after each, but the fine print is that you only need those to separate
> between previous or subsequent accesses that may use those contexts
> being switched from/to.
>
> Then there is the question of CSI between the instructions so e.g., you
> don't get the TLB prefetch bug if the mtPIDR could go out of order ahead
> of the mtLPIDR, but those instructions are serialized so they wouldn't.
>
> There's possibly a few clarifications coming to the architecture around
> this as well.
>
> I think things are relatively okay but probably need a bit more
> commenting to justify where the isyncs() aren't. It's possible we might
> be able to even remove the isyncs that are there.
>
> Making a like-for-like conversion is a bit tricky because there are
> possible context synchronising instructions between them already.
>
> Maybe for the first series, I'll just put an isync between all of them,
> and then a later patch can replace some of them with comments.
>
>>
>>
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>
>>
>>> ---
>>> arch/powerpc/kvm/book3s_hv.c | 55 +++++++++++++++++++++---------------
>>> 1 file changed, 32 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>>> index f1230f9d98ba..b9cae42b9cd5 100644
>>> --- a/arch/powerpc/kvm/book3s_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_hv.c
>>> @@ -3449,12 +3449,38 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
>>> trace_kvmppc_run_core(vc, 1);
>>> }
>>>
>>> +static void switch_mmu_to_guest_radix(struct kvm *kvm, struct kvm_vcpu *vcpu, u64 lpcr)
>>> +{
>>> + struct kvmppc_vcore *vc = vcpu->arch.vcore;
>>> + struct kvm_nested_guest *nested = vcpu->arch.nested;
>>> + u32 lpid;
>>> +
>>> + lpid = nested ? nested->shadow_lpid : kvm->arch.lpid;
>>> +
>>> + mtspr(SPRN_LPID, lpid);
>>> + mtspr(SPRN_LPCR, lpcr);
>>> + mtspr(SPRN_PID, vcpu->arch.pid);
>>> + isync();
>>> +
>>> + /* TLBIEL must have LPIDR set, so set guest LPID before flushing. */
>>> + kvmppc_check_need_tlb_flush(kvm, vc->pcpu, nested);
>>> +}
>>> +
>>> +static void switch_mmu_to_host_radix(struct kvm *kvm, u32 pid)
>>> +{
>>> + mtspr(SPRN_PID, pid);
>>> + mtspr(SPRN_LPID, kvm->arch.host_lpid);
>>> + mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
>>> + isync();
>>> +}
>>> +
>>> /*
>>> * Load up hypervisor-mode registers on P9.
>>> */
>>> static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>> unsigned long lpcr)
>>> {
>>> + struct kvm *kvm = vcpu->kvm;
>>> struct kvmppc_vcore *vc = vcpu->arch.vcore;
>>> s64 hdec;
>>> u64 tb, purr, spurr;
>>> @@ -3477,12 +3503,12 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>> * P8 and P9 suppress the HDEC exception when LPCR[HDICE] = 0,
>>> * so set HDICE before writing HDEC.
>>> */
>>> - mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr | LPCR_HDICE);
>>> + mtspr(SPRN_LPCR, kvm->arch.host_lpcr | LPCR_HDICE);
>>> isync();
>>>
>>> hdec = time_limit - mftb();
>>> if (hdec < 0) {
>>> - mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr);
>>> + mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
>>> isync();
>>> return BOOK3S_INTERRUPT_HV_DECREMENTER;
>>> }
>>> @@ -3517,7 +3543,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>> }
>>> mtspr(SPRN_CIABR, vcpu->arch.ciabr);
>>> mtspr(SPRN_IC, vcpu->arch.ic);
>>> - mtspr(SPRN_PID, vcpu->arch.pid);
>>>
>>> mtspr(SPRN_PSSCR, vcpu->arch.psscr | PSSCR_EC |
>>> (local_paca->kvm_hstate.fake_suspend << PSSCR_FAKE_SUSPEND_LG));
>>> @@ -3531,8 +3556,7 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>>
>>> mtspr(SPRN_AMOR, ~0UL);
>>>
>>> - mtspr(SPRN_LPCR, lpcr);
>>> - isync();
>>> + switch_mmu_to_guest_radix(kvm, vcpu, lpcr);
>>>
>>> kvmppc_xive_push_vcpu(vcpu);
>>>
>>> @@ -3571,7 +3595,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>> mtspr(SPRN_DAWR1, host_dawr1);
>>> mtspr(SPRN_DAWRX1, host_dawrx1);
>>> }
>>> - mtspr(SPRN_PID, host_pidr);
>>>
>>> /*
>>> * Since this is radix, do a eieio; tlbsync; ptesync sequence in
>>> @@ -3586,9 +3609,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>> if (cpu_has_feature(CPU_FTR_ARCH_31))
>>> asm volatile(PPC_CP_ABORT);
>>>
>>> - mtspr(SPRN_LPID, vcpu->kvm->arch.host_lpid); /* restore host LPID */
>>> - isync();
>>> -
>>> vc->dpdes = mfspr(SPRN_DPDES);
>>> vc->vtb = mfspr(SPRN_VTB);
>>> mtspr(SPRN_DPDES, 0);
>>> @@ -3605,7 +3625,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>> }
>>>
>>> mtspr(SPRN_HDEC, 0x7fffffff);
>>> - mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr);
>>> +
>>> + switch_mmu_to_host_radix(kvm, host_pidr);
>>>
>>> return trap;
>>> }
>>> @@ -4138,7 +4159,7 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
>>> {
>>> struct kvm_run *run = vcpu->run;
>>> int trap, r, pcpu;
>>> - int srcu_idx, lpid;
>>> + int srcu_idx;
>>> struct kvmppc_vcore *vc;
>>> struct kvm *kvm = vcpu->kvm;
>>> struct kvm_nested_guest *nested = vcpu->arch.nested;
>>> @@ -4212,13 +4233,6 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
>>> vc->vcore_state = VCORE_RUNNING;
>>> trace_kvmppc_run_core(vc, 0);
>>>
>>> - if (cpu_has_feature(CPU_FTR_HVMODE)) {
>>
>>
>> The new location of mtspr(SPRN_LPID, lpid) does not check for
>> CPU_FTR_HVMODE anymore, is this going to work with HV KVM on pseries?
>
> Yes, these are moved to HVMODE specific code now.
ah right, kvmhv_on_pseries() is !cpu_has_feature(CPU_FTR_HVMODE).
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
--
Alexey
^ permalink raw reply
* Re: [PATCH v11 6/6] powerpc: Book3S 64-bit outline-only KASAN support
From: Daniel Axtens @ 2021-03-22 5:52 UTC (permalink / raw)
To: Balbir Singh
Cc: aneesh.kumar, linux-kernel, linux-mm, kasan-dev, linuxppc-dev
In-Reply-To: <20210322025934.GG77072@balbir-desktop>
Balbir Singh <bsingharora@gmail.com> writes:
> On Mon, Mar 22, 2021 at 11:55:08AM +1100, Daniel Axtens wrote:
>> Hi Balbir,
>>
>> > Could you highlight the changes from
>> > https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20170729140901.5887-1-bsingharora@gmail.com/?
>> >
>> > Feel free to use my signed-off-by if you need to and add/update copyright
>> > headers if appropriate.
>>
>> There's not really anything in common any more:
>>
>> - ppc32 KASAN landed, so there was already a kasan.h for powerpc, the
>> explicit memcpy changes, the support for non-instrumented files,
>> prom_check.sh, etc. all already landed.
>>
>> - I locate the shadow region differently and don't resize any virtual
>> memory areas.
>>
>> - The ARCH_DEFINES_KASAN_ZERO_PTE handling changed upstream and our
>> handling for that is now handled more by patch 3.
>>
>> - The outline hook is now an inline function rather than a #define.
>>
>> - The init function has been totally rewritten as it's gone from
>> supporting real mode to not supporting real mode and back.
>>
>> - The list of non-instrumented files has grown a lot.
>>
>> - There's new stuff: stack walking is now safe, KASAN vmalloc support
>> means modules are better supported now, ptdump works, and there's
>> documentation.
>>
>> It's been a while now, but I don't think when I started this process 2
>> years ago that I directly reused much of your code. So I'm not sure that
>> a signed-off-by makes sense here? Would a different tag (Originally-by?)
>> make more sense?
>>
>
> Sure
Will do.
>
>> >> + * The shadow ends before the highest accessible address
>> >> + * because we don't need a shadow for the shadow. Instead:
>> >> + * c00e000000000000 << 3 + a80e 0000 0000 0000 000 = c00fc00000000000
>> >
>> > The comment has one extra 0 in a80e.., I did the math and had to use
>> > the data from the defines :)
>>
>> 3 extra 0s, even! Fixed.
>>
>> >> +void __init kasan_init(void)
>> >> +{
>> >> + /*
>> >> + * We want to do the following things:
>> >> + * 1) Map real memory into the shadow for all physical memblocks
>> >> + * This takes us from c000... to c008...
>> >> + * 2) Leave a hole over the shadow of vmalloc space. KASAN_VMALLOC
>> >> + * will manage this for us.
>> >> + * This takes us from c008... to c00a...
>> >> + * 3) Map the 'early shadow'/zero page over iomap and vmemmap space.
>> >> + * This takes us up to where we start at c00e...
>> >> + */
>> >> +
>> >
>> > assuming we have
>> > #define VMEMMAP_END R_VMEMMAP_END
>> > and ditto for hash we probably need
>> >
>> > BUILD_BUG_ON(VMEMMAP_END + KASAN_SHADOW_OFFSET != KASAN_SHADOW_END);
>>
>> Sorry, I'm not sure what this is supposed to be testing? In what
>> situation would this trigger?
>>
>
> I am bit concerned that we have hard coded (IIR) 0xa80e... in the
> config, any changes to VMEMMAP_END, KASAN_SHADOW_OFFSET/END
> should be guarded.
>
Ah that makes sense. I'll come up with some test that should catch any
unsynchronised changes to VMEMMAP_END, KASAN_SHADOW_OFFSET or
KASAN_SHADOW_END.
Kind regards,
Daniel Axtens
> Balbir Singh.
^ permalink raw reply
* Re: [PATCH v3 18/41] KVM: PPC: Book3S HV P9: Move xive vcpu context management into kvmhv_p9_guest_entry
From: Alexey Kardashevskiy @ 2021-03-22 5:30 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210305150638.2675513-19-npiggin@gmail.com>
On 06/03/2021 02:06, Nicholas Piggin wrote:
> Move the xive management up so the low level register switching can be
> pushed further down in a later patch. XIVE MMIO CI operations can run in
> higher level code with machine checks, tracing, etc., available.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/kvm/book3s_hv.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index b265522fc467..497f216ad724 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3558,15 +3558,11 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>
> switch_mmu_to_guest_radix(kvm, vcpu, lpcr);
>
> - kvmppc_xive_push_vcpu(vcpu);
> -
> mtspr(SPRN_SRR0, vcpu->arch.shregs.srr0);
> mtspr(SPRN_SRR1, vcpu->arch.shregs.srr1);
>
> trap = __kvmhv_vcpu_entry_p9(vcpu);
>
> - kvmppc_xive_pull_vcpu(vcpu);
> -
> /* Advance host PURR/SPURR by the amount used by guest */
> purr = mfspr(SPRN_PURR);
> spurr = mfspr(SPRN_SPURR);
> @@ -3749,7 +3745,10 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
> trap = 0;
> }
> } else {
> + kvmppc_xive_push_vcpu(vcpu);
> trap = kvmhv_load_hv_regs_and_go(vcpu, time_limit, lpcr);
> + kvmppc_xive_pull_vcpu(vcpu);
> +
> }
>
> vcpu->arch.slb_max = 0;
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v3 16/41] KVM: PPC: Book3S HV P9: Move radix MMU switching instructions together
From: Nicholas Piggin @ 2021-03-22 5:25 UTC (permalink / raw)
To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <47284fdd-51ef-5ba7-487b-dfb46ec2816e@ozlabs.ru>
Excerpts from Alexey Kardashevskiy's message of March 22, 2021 2:24 pm:
>
>
> On 06/03/2021 02:06, Nicholas Piggin wrote:
>> Switching the MMU from radix<->radix mode is tricky particularly as the
>> MMU can remain enabled and requires a certain sequence of SPR updates.
>> Move these together into their own functions.
>>
>> This also includes the radix TLB check / flush because it's tied in to
>> MMU switching due to tlbiel getting LPID from LPIDR.
>>
>> (XXX: isync / hwsync synchronisation TBD)
>
>
> Looks alright but what is this comment about? Is something missing or
> just sub optimal?
Ah, yeah the architecture says for example a CSI is required before +
after each, but the fine print is that you only need those to separate
between previous or subsequent accesses that may use those contexts
being switched from/to.
Then there is the question of CSI between the instructions so e.g., you
don't get the TLB prefetch bug if the mtPIDR could go out of order ahead
of the mtLPIDR, but those instructions are serialized so they wouldn't.
There's possibly a few clarifications coming to the architecture around
this as well.
I think things are relatively okay but probably need a bit more
commenting to justify where the isyncs() aren't. It's possible we might
be able to even remove the isyncs that are there.
Making a like-for-like conversion is a bit tricky because there are
possible context synchronising instructions between them already.
Maybe for the first series, I'll just put an isync between all of them,
and then a later patch can replace some of them with comments.
>
>
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>
>
>> ---
>> arch/powerpc/kvm/book3s_hv.c | 55 +++++++++++++++++++++---------------
>> 1 file changed, 32 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index f1230f9d98ba..b9cae42b9cd5 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -3449,12 +3449,38 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
>> trace_kvmppc_run_core(vc, 1);
>> }
>>
>> +static void switch_mmu_to_guest_radix(struct kvm *kvm, struct kvm_vcpu *vcpu, u64 lpcr)
>> +{
>> + struct kvmppc_vcore *vc = vcpu->arch.vcore;
>> + struct kvm_nested_guest *nested = vcpu->arch.nested;
>> + u32 lpid;
>> +
>> + lpid = nested ? nested->shadow_lpid : kvm->arch.lpid;
>> +
>> + mtspr(SPRN_LPID, lpid);
>> + mtspr(SPRN_LPCR, lpcr);
>> + mtspr(SPRN_PID, vcpu->arch.pid);
>> + isync();
>> +
>> + /* TLBIEL must have LPIDR set, so set guest LPID before flushing. */
>> + kvmppc_check_need_tlb_flush(kvm, vc->pcpu, nested);
>> +}
>> +
>> +static void switch_mmu_to_host_radix(struct kvm *kvm, u32 pid)
>> +{
>> + mtspr(SPRN_PID, pid);
>> + mtspr(SPRN_LPID, kvm->arch.host_lpid);
>> + mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
>> + isync();
>> +}
>> +
>> /*
>> * Load up hypervisor-mode registers on P9.
>> */
>> static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>> unsigned long lpcr)
>> {
>> + struct kvm *kvm = vcpu->kvm;
>> struct kvmppc_vcore *vc = vcpu->arch.vcore;
>> s64 hdec;
>> u64 tb, purr, spurr;
>> @@ -3477,12 +3503,12 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>> * P8 and P9 suppress the HDEC exception when LPCR[HDICE] = 0,
>> * so set HDICE before writing HDEC.
>> */
>> - mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr | LPCR_HDICE);
>> + mtspr(SPRN_LPCR, kvm->arch.host_lpcr | LPCR_HDICE);
>> isync();
>>
>> hdec = time_limit - mftb();
>> if (hdec < 0) {
>> - mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr);
>> + mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
>> isync();
>> return BOOK3S_INTERRUPT_HV_DECREMENTER;
>> }
>> @@ -3517,7 +3543,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>> }
>> mtspr(SPRN_CIABR, vcpu->arch.ciabr);
>> mtspr(SPRN_IC, vcpu->arch.ic);
>> - mtspr(SPRN_PID, vcpu->arch.pid);
>>
>> mtspr(SPRN_PSSCR, vcpu->arch.psscr | PSSCR_EC |
>> (local_paca->kvm_hstate.fake_suspend << PSSCR_FAKE_SUSPEND_LG));
>> @@ -3531,8 +3556,7 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>
>> mtspr(SPRN_AMOR, ~0UL);
>>
>> - mtspr(SPRN_LPCR, lpcr);
>> - isync();
>> + switch_mmu_to_guest_radix(kvm, vcpu, lpcr);
>>
>> kvmppc_xive_push_vcpu(vcpu);
>>
>> @@ -3571,7 +3595,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>> mtspr(SPRN_DAWR1, host_dawr1);
>> mtspr(SPRN_DAWRX1, host_dawrx1);
>> }
>> - mtspr(SPRN_PID, host_pidr);
>>
>> /*
>> * Since this is radix, do a eieio; tlbsync; ptesync sequence in
>> @@ -3586,9 +3609,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>> if (cpu_has_feature(CPU_FTR_ARCH_31))
>> asm volatile(PPC_CP_ABORT);
>>
>> - mtspr(SPRN_LPID, vcpu->kvm->arch.host_lpid); /* restore host LPID */
>> - isync();
>> -
>> vc->dpdes = mfspr(SPRN_DPDES);
>> vc->vtb = mfspr(SPRN_VTB);
>> mtspr(SPRN_DPDES, 0);
>> @@ -3605,7 +3625,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>> }
>>
>> mtspr(SPRN_HDEC, 0x7fffffff);
>> - mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr);
>> +
>> + switch_mmu_to_host_radix(kvm, host_pidr);
>>
>> return trap;
>> }
>> @@ -4138,7 +4159,7 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
>> {
>> struct kvm_run *run = vcpu->run;
>> int trap, r, pcpu;
>> - int srcu_idx, lpid;
>> + int srcu_idx;
>> struct kvmppc_vcore *vc;
>> struct kvm *kvm = vcpu->kvm;
>> struct kvm_nested_guest *nested = vcpu->arch.nested;
>> @@ -4212,13 +4233,6 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
>> vc->vcore_state = VCORE_RUNNING;
>> trace_kvmppc_run_core(vc, 0);
>>
>> - if (cpu_has_feature(CPU_FTR_HVMODE)) {
>
>
> The new location of mtspr(SPRN_LPID, lpid) does not check for
> CPU_FTR_HVMODE anymore, is this going to work with HV KVM on pseries?
Yes, these are moved to HVMODE specific code now.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] powerpc/numa: Fix topology_physical_package_id() on pSeries
From: David Gibson @ 2021-03-22 5:19 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Nathan Lynch, Srikar Dronamraju, Daniel Henrique Barboza,
Greg Kurz, Vasant Hegde, linuxppc-dev
In-Reply-To: <20210316122437.3571843-1-clg@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 2860 bytes --]
On Tue, Mar 16, 2021 at 01:24:37PM +0100, Cédric Le Goater wrote:
> The topology-id of a CPU in a pSeries machine can be queried from
> sysfs but under PowerVM the value is always -1 even if NUMA nodes are
> defined. This is because the topology_physical_package_id() routine is
> using the "ibm,chip-id" property which is not specified in PAPR.
>
> Under QEMU/KVM, things are different because QEMU populates the CPU DT
> node with "ibm,chip-id" property. However, its value can be incorrect
> for uncommon SMT configuration and expose a bogus topology-id value in
> sysfs.
Incorrect in what sense? It's still indicating the (admittedly
arbitrary) qemu socket number, isn't it? And isn't that what it
should be?
> The use of cpu_to_node() guarantees to have a correct NUMA node id
> under both environments QEMU/KVM and PowerVM. This introduces a slight
> change for the QEMU/KVM guest, as the topology-id now matches the NUMA
> node and not the socket-id as before. Since QEMU also needs to remove
> "ibm,chip-id" property for the DT to follow the PAPR specs, both
> hypervisor environments will be in sync.
>
> On the PowerNV side, the NUMA node id returned by cpu_to_node() is
> computed from the "ibm,associativity" property of the CPU. Its value
> is built from the OPAL chip id and is equivalent to "ibm,chip-id".
Like mpe, I'm not convinced this is the right approach. "physical
packate" and NUMA node are not the same thing, except sometimes by
accident.
>
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Tested-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> arch/powerpc/include/asm/topology.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index 3beeb030cd78..887c42a4e43d 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -123,7 +123,7 @@ static inline int cpu_to_coregroup_id(int cpu)
> #ifdef CONFIG_PPC64
> #include <asm/smp.h>
>
> -#define topology_physical_package_id(cpu) (cpu_to_chip_id(cpu))
> +#define topology_physical_package_id(cpu) (cpu_to_node(cpu))
>
> #define topology_sibling_cpumask(cpu) (per_cpu(cpu_sibling_map, cpu))
> #define topology_core_cpumask(cpu) (cpu_cpu_mask(cpu))
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 17/41] KVM: PPC: Book3S HV P9: implement kvmppc_xive_pull_vcpu in C
From: Alexey Kardashevskiy @ 2021-03-22 5:05 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210305150638.2675513-18-npiggin@gmail.com>
On 06/03/2021 02:06, Nicholas Piggin wrote:
> This is more symmetric with kvmppc_xive_push_vcpu. The extra test in
> the asm will go away in a later change.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/kvm_ppc.h | 2 ++
> arch/powerpc/kvm/book3s_hv.c | 2 ++
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 5 ++++
> arch/powerpc/kvm/book3s_xive.c | 31 +++++++++++++++++++++++++
> 4 files changed, 40 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 9531b1c1b190..73b1ca5a6471 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -672,6 +672,7 @@ extern int kvmppc_xive_set_icp(struct kvm_vcpu *vcpu, u64 icpval);
> extern int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 irq,
> int level, bool line_status);
> extern void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu);
> +extern void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu);
>
> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
> {
> @@ -712,6 +713,7 @@ static inline int kvmppc_xive_set_icp(struct kvm_vcpu *vcpu, u64 icpval) { retur
> static inline int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 irq,
> int level, bool line_status) { return -ENODEV; }
> static inline void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu) { }
> +static inline void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu) { }
>
> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
> { return 0; }
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index b9cae42b9cd5..b265522fc467 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3565,6 +3565,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>
> trap = __kvmhv_vcpu_entry_p9(vcpu);
>
> + kvmppc_xive_pull_vcpu(vcpu);
> +
> /* Advance host PURR/SPURR by the amount used by guest */
> purr = mfspr(SPRN_PURR);
> spurr = mfspr(SPRN_SPURR);
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index 75405ef53238..c11597f815e4 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -1442,6 +1442,11 @@ guest_exit_cont: /* r9 = vcpu, r12 = trap, r13 = paca */
> bl kvmhv_accumulate_time
> #endif
> #ifdef CONFIG_KVM_XICS
> + /* If we came in through the P9 short path, xive pull is done in C */
> + lwz r0, STACK_SLOT_SHORT_PATH(r1)
> + cmpwi r0, 0
> + bne 1f
> +
> /* We are exiting, pull the VP from the XIVE */
> lbz r0, VCPU_XIVE_PUSHED(r9)
> cmpwi cr0, r0, 0
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index e7219b6f5f9a..52cdb9e2660a 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -127,6 +127,37 @@ void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu)
> }
> EXPORT_SYMBOL_GPL(kvmppc_xive_push_vcpu);
>
> +/*
> + * Pull a vcpu's context from the XIVE on guest exit.
> + * This assumes we are in virtual mode (MMU on)
> + */
> +void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu)
> +{
> + void __iomem *tima = local_paca->kvm_hstate.xive_tima_virt;
> +
> + if (!vcpu->arch.xive_pushed)
> + return;
> +
> + /*
> + * Sould not have been pushed if there is no tima
s/Sould/Should/
Otherwise good
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> + */
> + if (WARN_ON(!tima))
> + return;
> +
> + eieio();
> + /* First load to pull the context, we ignore the value */
> + __raw_readl(tima + TM_SPC_PULL_OS_CTX);
> + /* Second load to recover the context state (Words 0 and 1) */
> + vcpu->arch.xive_saved_state.w01 = __raw_readq(tima + TM_QW1_OS);
> +
> + /* Fixup some of the state for the next load */
> + vcpu->arch.xive_saved_state.lsmfb = 0;
> + vcpu->arch.xive_saved_state.ack = 0xff;
> + vcpu->arch.xive_pushed = 0;
> + eieio();
> +}
> +EXPORT_SYMBOL_GPL(kvmppc_xive_pull_vcpu);
> +
> /*
> * This is a simple trigger for a generic XIVE IRQ. This must
> * only be called for interrupts that support a trigger page
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v3 16/41] KVM: PPC: Book3S HV P9: Move radix MMU switching instructions together
From: Alexey Kardashevskiy @ 2021-03-22 4:24 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210305150638.2675513-17-npiggin@gmail.com>
On 06/03/2021 02:06, Nicholas Piggin wrote:
> Switching the MMU from radix<->radix mode is tricky particularly as the
> MMU can remain enabled and requires a certain sequence of SPR updates.
> Move these together into their own functions.
>
> This also includes the radix TLB check / flush because it's tied in to
> MMU switching due to tlbiel getting LPID from LPIDR.
>
> (XXX: isync / hwsync synchronisation TBD)
Looks alright but what is this comment about? Is something missing or
just sub optimal?
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kvm/book3s_hv.c | 55 +++++++++++++++++++++---------------
> 1 file changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index f1230f9d98ba..b9cae42b9cd5 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3449,12 +3449,38 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
> trace_kvmppc_run_core(vc, 1);
> }
>
> +static void switch_mmu_to_guest_radix(struct kvm *kvm, struct kvm_vcpu *vcpu, u64 lpcr)
> +{
> + struct kvmppc_vcore *vc = vcpu->arch.vcore;
> + struct kvm_nested_guest *nested = vcpu->arch.nested;
> + u32 lpid;
> +
> + lpid = nested ? nested->shadow_lpid : kvm->arch.lpid;
> +
> + mtspr(SPRN_LPID, lpid);
> + mtspr(SPRN_LPCR, lpcr);
> + mtspr(SPRN_PID, vcpu->arch.pid);
> + isync();
> +
> + /* TLBIEL must have LPIDR set, so set guest LPID before flushing. */
> + kvmppc_check_need_tlb_flush(kvm, vc->pcpu, nested);
> +}
> +
> +static void switch_mmu_to_host_radix(struct kvm *kvm, u32 pid)
> +{
> + mtspr(SPRN_PID, pid);
> + mtspr(SPRN_LPID, kvm->arch.host_lpid);
> + mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
> + isync();
> +}
> +
> /*
> * Load up hypervisor-mode registers on P9.
> */
> static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
> unsigned long lpcr)
> {
> + struct kvm *kvm = vcpu->kvm;
> struct kvmppc_vcore *vc = vcpu->arch.vcore;
> s64 hdec;
> u64 tb, purr, spurr;
> @@ -3477,12 +3503,12 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
> * P8 and P9 suppress the HDEC exception when LPCR[HDICE] = 0,
> * so set HDICE before writing HDEC.
> */
> - mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr | LPCR_HDICE);
> + mtspr(SPRN_LPCR, kvm->arch.host_lpcr | LPCR_HDICE);
> isync();
>
> hdec = time_limit - mftb();
> if (hdec < 0) {
> - mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr);
> + mtspr(SPRN_LPCR, kvm->arch.host_lpcr);
> isync();
> return BOOK3S_INTERRUPT_HV_DECREMENTER;
> }
> @@ -3517,7 +3543,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
> }
> mtspr(SPRN_CIABR, vcpu->arch.ciabr);
> mtspr(SPRN_IC, vcpu->arch.ic);
> - mtspr(SPRN_PID, vcpu->arch.pid);
>
> mtspr(SPRN_PSSCR, vcpu->arch.psscr | PSSCR_EC |
> (local_paca->kvm_hstate.fake_suspend << PSSCR_FAKE_SUSPEND_LG));
> @@ -3531,8 +3556,7 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>
> mtspr(SPRN_AMOR, ~0UL);
>
> - mtspr(SPRN_LPCR, lpcr);
> - isync();
> + switch_mmu_to_guest_radix(kvm, vcpu, lpcr);
>
> kvmppc_xive_push_vcpu(vcpu);
>
> @@ -3571,7 +3595,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
> mtspr(SPRN_DAWR1, host_dawr1);
> mtspr(SPRN_DAWRX1, host_dawrx1);
> }
> - mtspr(SPRN_PID, host_pidr);
>
> /*
> * Since this is radix, do a eieio; tlbsync; ptesync sequence in
> @@ -3586,9 +3609,6 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
> if (cpu_has_feature(CPU_FTR_ARCH_31))
> asm volatile(PPC_CP_ABORT);
>
> - mtspr(SPRN_LPID, vcpu->kvm->arch.host_lpid); /* restore host LPID */
> - isync();
> -
> vc->dpdes = mfspr(SPRN_DPDES);
> vc->vtb = mfspr(SPRN_VTB);
> mtspr(SPRN_DPDES, 0);
> @@ -3605,7 +3625,8 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
> }
>
> mtspr(SPRN_HDEC, 0x7fffffff);
> - mtspr(SPRN_LPCR, vcpu->kvm->arch.host_lpcr);
> +
> + switch_mmu_to_host_radix(kvm, host_pidr);
>
> return trap;
> }
> @@ -4138,7 +4159,7 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
> {
> struct kvm_run *run = vcpu->run;
> int trap, r, pcpu;
> - int srcu_idx, lpid;
> + int srcu_idx;
> struct kvmppc_vcore *vc;
> struct kvm *kvm = vcpu->kvm;
> struct kvm_nested_guest *nested = vcpu->arch.nested;
> @@ -4212,13 +4233,6 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
> vc->vcore_state = VCORE_RUNNING;
> trace_kvmppc_run_core(vc, 0);
>
> - if (cpu_has_feature(CPU_FTR_HVMODE)) {
The new location of mtspr(SPRN_LPID, lpid) does not check for
CPU_FTR_HVMODE anymore, is this going to work with HV KVM on pseries?
Thanks,
> - lpid = nested ? nested->shadow_lpid : kvm->arch.lpid;
> - mtspr(SPRN_LPID, lpid);
> - isync();
> - kvmppc_check_need_tlb_flush(kvm, pcpu, nested);
> - }
> -
> guest_enter_irqoff();
>
> srcu_idx = srcu_read_lock(&kvm->srcu);
> @@ -4237,11 +4251,6 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
>
> srcu_read_unlock(&kvm->srcu, srcu_idx);
>
> - if (cpu_has_feature(CPU_FTR_HVMODE)) {
> - mtspr(SPRN_LPID, kvm->arch.host_lpid);
> - isync();
> - }
> -
> set_irq_happened(trap);
>
> kvmppc_set_host_core(pcpu);
>
--
Alexey
^ permalink raw reply
* Re: Advice needed on SMP regression after cpu_core_mask change
From: David Gibson @ 2021-03-22 3:16 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: aneesh.kumar, Srikar Dronamraju, Greg Kurz, Cédric Le Goater,
linuxppc-dev
In-Reply-To: <04bf6d12-a806-d417-3d95-b6d315c44b58@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 7169 bytes --]
On Wed, Mar 17, 2021 at 01:05:21PM -0300, Daniel Henrique Barboza wrote:
>
>
> On 3/17/21 12:30 PM, Cédric Le Goater wrote:
> > On 3/17/21 2:00 PM, Daniel Henrique Barboza wrote:
> > > Hello,
> > >
> > > Patch 4bce545903fa ("powerpc/topology: Update topology_core_cpumask") introduced
> > > a regression in both upstream and RHEL downstream kernels [1]. The assumption made
> > > in the commit:
> > >
> > > "Further analysis shows that cpu_core_mask and cpu_cpu_mask for any CPU would be
> > > equal on Power"
> > >
> > > Doesn't seem to be true. After this commit, QEMU is now unable to set single NUMA
> > > node SMP topologies such as:
> > >
> > > -smp 8,maxcpus=8,cores=2,threads=2,sockets=2
> > >
> > > lscpu will give the following output in this case:
> > >
> > > # lscpu
> > > Architecture: ppc64le
> > > Byte Order: Little Endian
> > > CPU(s): 8
> > > On-line CPU(s) list: 0-7
> > > Thread(s) per core: 2
> > > Core(s) per socket: 4
> > > Socket(s): 1
> > > NUMA node(s): 1
> > > Model: 2.2 (pvr 004e 1202)
> > > Model name: POWER9 (architected), altivec supported
> > > Hypervisor vendor: KVM
> > > Virtualization type: para
> > > L1d cache: 32K
> > > L1i cache: 32K
> > > NUMA node0 CPU(s): 0-7
> > >
> > >
> > > This is happening because the macro cpu_cpu_mask(cpu) expands to
> > > cpumask_of_node(cpu_to_node(cpu)), which in turn expands to node_to_cpumask_map[node].
> > > node_to_cpumask_map is a NUMA array that maps CPUs to NUMA nodes (Aneesh is on CC to
> > > correct me if I'm wrong). We're now associating sockets to NUMA nodes directly.
> > >
> > > If I add a second NUMA node then I can get the intended smp topology:
> > >
> > > -smp 8,maxcpus=8,cores=2,threads=2,sockets=2
> > > -numa node,memdev=mem0,cpus=0-3,nodeid=0 \
> > > -numa node,memdev=mem1,cpus=4-7,nodeid=1 \
> > >
> > > # lscpu
> > > Architecture: ppc64le
> > > Byte Order: Little Endian
> > > CPU(s): 8
> > > On-line CPU(s) list: 0-7
> > > Thread(s) per core: 2
> > > Core(s) per socket: 2
> > > Socket(s): 2
> > > NUMA node(s): 2
> > > Model: 2.2 (pvr 004e 1202)
> > > Model name: POWER9 (architected), altivec supported
> > > Hypervisor vendor: KVM
> > > Virtualization type: para
> > > L1d cache: 32K
> > > L1i cache: 32K
> > > NUMA node0 CPU(s): 0-3
> > > NUMA node1 CPU(s): 4-7
> > >
> > >
> > > However, if I try a single socket with multiple NUMA nodes topology, which is the case
> > > of Power10, e.g.:
> > >
> > >
> > > -smp 8,maxcpus=8,cores=4,threads=2,sockets=1
> > > -numa node,memdev=mem0,cpus=0-3,nodeid=0 \
> > > -numa node,memdev=mem1,cpus=4-7,nodeid=1 \
> > >
> > >
> > > This is the result:
> > >
> > > # lscpu
> > > Architecture: ppc64le
> > > Byte Order: Little Endian
> > > CPU(s): 8
> > > On-line CPU(s) list: 0-7
> > > Thread(s) per core: 2
> > > Core(s) per socket: 2
> > > Socket(s): 2
> > > NUMA node(s): 2
> > > Model: 2.2 (pvr 004e 1202)
> > > Model name: POWER9 (architected), altivec supported
> > > Hypervisor vendor: KVM
> > > Virtualization type: para
> > > L1d cache: 32K
> > > L1i cache: 32K
> > > NUMA node0 CPU(s): 0-3
> > > NUMA node1 CPU(s): 4-7
> > >
> > >
> > > This confirms my suspicions that, at this moment, we're making sockets == NUMA nodes.
> >
> > Yes. I don't think we can do better on PAPR and the above examples
> > seem to confirm that the "sockets" definition is simply ignored.
> > > Cedric, the reason I'm CCing you is because this is related to ibm,chip-id. The commit
> > > after the one that caused the regression, 4ca234a9cbd7c3a65 ("powerpc/smp: Stop updating
> > > cpu_core_mask"), is erasing the code that calculated cpu_core_mask. cpu_core_mask, despite
> > > its shortcomings that caused its removal, was giving a precise SMP topology. And it was
> > > using physical_package_id/'ibm,chip-id' for that.
> >
> > ibm,chip-id is a no-no on pSeries. I guess this is inherent to PAPR which
> > is hiding a lot of the underlying HW and topology. May be we are trying
> > to reconcile two orthogonal views of machine virtualization ...
> >
> > > Checking in QEMU I can say that the ibm,chip-id calculation is the only place in the code
> > > that cares about cores per socket information. The kernel is now ignoring that, starting
> > > on 4bce545903fa, and now QEMU is unable to provide this info to the guest.
> > >
> > > If we're not going to use ibm,chip-id any longer, which seems sensible given that PAPR does
> > > not declare it, we need another way of letting the guest know how much cores per socket
> > > we want.
> > The RTAS call "ibm,get-system-parameter" with token "Processor Module
> > Information" returns that kind of information :
> >
> > 2 byte binary number (N) of module types followed by N module specifiers of the form:
> > 2 byte binary number (M) of sockets of this module type
> > 2 byte binary number (L) of chips per this module type
> > 2 byte binary number (K) of cores per chip in this module type.
> >
> > See the values in these sysfs files :
> >
> > cat /sys/devices/hv_24x7/interface/{sockets,chipspersocket,coresperchip}
> >
> > But I am afraid these are host level information and not guest/LPAR.
>
>
> I believe there might be some sort of reasoning behind not having this on
> PAPR, but I'll say in advance that the virtual machine should act as the
> real hardware, as close as possible. This is the kind of hcall that could
> be used in this situation.
In the case of POWER, that's pretty much a lost battle. The
virtualization features of the CPU don't really permit full hardware
virtualization - it has to be a paravirtualized environment. Once
that's the case, the value of keeping secondary things the same
between the bare metal and paravirt environments isn't that compelling
any more.
> > I didn't find any LPAR level properties or hcalls in the PAPR document.
> > They need to be specified.
> >
> > or
> >
> > We can add extra properties like ibm,chip-id but making sure it's only
> > used under the KVM hypervisor. My understanding is that's something we
> > are trying to avoid.
>
> We can change PAPR to add ibm,chip-id. Problem is that ibm,chip-id today, with
> the current kernel codebase, does not fix the issue because the code is
> ignoring it hehehe
>
>
> If we're going to change PAPR - and I believe we should, there's a clear
> lack of proper support for SMP topologies - we're better make sure that whatever
> attribute/hcall we add there fixes it in a robust way for the long term.
>
>
> Thanks,
>
>
> DHB
>
>
> >
> > C.
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 15/41] KVM: PPC: Book3S 64: Minimise hcall handler calling convention differences
From: Nicholas Piggin @ 2021-03-22 4:06 UTC (permalink / raw)
To: kvm-ppc; +Cc: Alexey Kardashevskiy, linuxppc-dev
In-Reply-To: <20210305150638.2675513-16-npiggin@gmail.com>
Excerpts from Nicholas Piggin's message of March 6, 2021 1:06 am:
> This sets up the same calling convention from interrupt entry to
> KVM interrupt handler for system calls as exists for other interrupt
> types.
>
> This is a better API, it uses a save area rather than SPR, and it has
> more registers free to use. Using a single common API helps maintain
> it, and it becomes easier to use in C in a later patch.
On second look I'm happy enough with this.
It does add some hcall setup code back into exception-64s.S and removes
most of the "fixup" code that was previously moved into
book3s_64_entry.S in patch 12. But if you take patch 12 and 13 and other
earlier patches together they are moving most KVM interrupt knowledge
into KVM which is a good change.
Once that is done, this final one then gets hcall into better shape for
the C code. If anything this patch could go together with patch 12 but
I guess I ended up writing it for the C code whereas the previous ones
were cleanups so the ordering didn't come out that way. It won't be
trivial to move now so I don't think I'd bother.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Revert "powerpc/mm: Remove DEBUG_VM_PGTABLE support on powerpc"
From: Aneesh Kumar K.V @ 2021-03-22 4:00 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev
In-Reply-To: <87lfak54lx.fsf@mpe.ellerman.id.au>
On 3/19/21 6:42 AM, Michael Ellerman wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
>> This reverts commit 675bceb097e6 ("powerpc/mm: Remove DEBUG_VM_PGTABLE support on powerpc")
>>
>> All the related issues are fixed by the series
>> https://lore.kernel.org/linux-mm/20200902114222.181353-1-aneesh.kumar@linux.ibm.com
>
> Was that series merged?
>
> If so this seems like this could be tagged as a Fix for the last commit
> in that series.
commit f14312e1ed1e ("mm/debug_vm_pgtable: avoid doing memory allocation
with pgtable_t mapped.")
>
> cheers
>
>> Hence enable it back
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>> Documentation/features/debug/debug-vm-pgtable/arch-support.txt | 2 +-
>> arch/powerpc/Kconfig | 1 +
>> 2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/features/debug/debug-vm-pgtable/arch-support.txt b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt
>> index 7aff505af706..fa83403b4aec 100644
>> --- a/Documentation/features/debug/debug-vm-pgtable/arch-support.txt
>> +++ b/Documentation/features/debug/debug-vm-pgtable/arch-support.txt
>> @@ -21,7 +21,7 @@
>> | nios2: | TODO |
>> | openrisc: | TODO |
>> | parisc: | TODO |
>> - | powerpc: | TODO |
>> + | powerpc: | ok |
>> | riscv: | ok |
>> | s390: | ok |
>> | sh: | TODO |
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 386ae12d8523..982c87d5c051 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -119,6 +119,7 @@ config PPC
>> #
>> select ARCH_32BIT_OFF_T if PPC32
>> select ARCH_HAS_DEBUG_VIRTUAL
>> + select ARCH_HAS_DEBUG_VM_PGTABLE
>> select ARCH_HAS_DEVMEM_IS_ALLOWED
>> select ARCH_HAS_ELF_RANDOMIZE
>> select ARCH_HAS_FORTIFY_SOURCE
>> --
>> 2.30.2
^ permalink raw reply
* Re: [PATCH v3 34/41] KVM: PPC: Book3S HV: Remove support for dependent threads mode on P9
From: Nicholas Piggin @ 2021-03-22 3:27 UTC (permalink / raw)
To: Aneesh Kumar K.V, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <87ft0tzug8.fsf@linux.ibm.com>
Excerpts from Aneesh Kumar K.V's message of March 18, 2021 1:11 am:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
>> Radix guest support will be removed from the P7/8 path, so disallow
>> dependent threads mode on P9.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/kvm_host.h | 1 -
>> arch/powerpc/kvm/book3s_hv.c | 27 +++++----------------------
>> 2 files changed, 5 insertions(+), 23 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
>> index 05fb00d37609..dd017dfa4e65 100644
>> --- a/arch/powerpc/include/asm/kvm_host.h
>> +++ b/arch/powerpc/include/asm/kvm_host.h
>> @@ -304,7 +304,6 @@ struct kvm_arch {
>> u8 fwnmi_enabled;
>> u8 secure_guest;
>> u8 svm_enabled;
>> - bool threads_indep;
>> bool nested_enable;
>> bool dawr1_enabled;
>> pgd_t *pgtable;
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index cb428e2f7140..928ed8180d9d 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -103,13 +103,9 @@ static int target_smt_mode;
>> module_param(target_smt_mode, int, 0644);
>> MODULE_PARM_DESC(target_smt_mode, "Target threads per core (0 = max)");
>>
>> -static bool indep_threads_mode = true;
>> -module_param(indep_threads_mode, bool, S_IRUGO | S_IWUSR);
>> -MODULE_PARM_DESC(indep_threads_mode, "Independent-threads mode (only on POWER9)");
>> -
>> static bool one_vm_per_core;
>> module_param(one_vm_per_core, bool, S_IRUGO | S_IWUSR);
>> -MODULE_PARM_DESC(one_vm_per_core, "Only run vCPUs from the same VM on a core (requires indep_threads_mode=N)");
>> +MODULE_PARM_DESC(one_vm_per_core, "Only run vCPUs from the same VM on a core (requires POWER8 or older)");
>
> Isn't this also a security feature, where there was an ask to make sure
> threads/vCPU from other VM won't run on this core? In that context isn't
> this applicable also for P9?
I'm not sure about an ask, but it is a possible security feature that
would be relevant to all SMT CPUs running KVM guests.
It doesn't make much sense to plumb P9 support all through the P8 path
just for that though, in my opinion? Is it tested? Who uses it? It's
lacking features of the P9 path.
It would be better added to KVM/QEMU in general (or until that is
available, disable SMT, or use CPU pinning and isolcpus to prevent host
code running on secondaries, and isolating VMs from one another, etc).
I think it's quite possible to rendezvous threads in kernel, move them
onto the threads of a core, and then have them all running in KVM code
before they enter the guest, without disabling SMT in the host.
You could do it with kernel threads on the secondaries even, but I
wouldn't like to have to plumb the vcore concept entirely through
everywhere so I would actually prefer to see QEMU grow an understanding
of it so it would know it has to call the ioctl on every guest SMT
thread.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 14/41] KVM: PPC: Book3S 64: move bad_host_intr check to HV handler
From: Nicholas Piggin @ 2021-03-22 3:18 UTC (permalink / raw)
To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <1f68b37c-7167-30d7-ee19-f6ebc69bd4a6@ozlabs.ru>
Excerpts from Alexey Kardashevskiy's message of March 20, 2021 7:07 pm:
>
>
> On 06/03/2021 02:06, Nicholas Piggin wrote:
>> This is not used by PR KVM.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>
>
> Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>
> a small tote - it probably makes sense to move this before 09/41 as this
> one removes what 09/41 added to book3s_64_entry.S. Thanks,
Thanks.
I do realise there's a bit of shuffling around in this part of the
series, I'm trying to see if that can be improved a bit. But 9/41
is just moving the code without change which I prefer to do first.
This one changes the calling convention for PR which I think is
better to do after we have the entry point in a common file.
Thanks,
Nick
>
>
>> ---
>> arch/powerpc/kvm/book3s_64_entry.S | 3 ---
>> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 4 +++-
>> arch/powerpc/kvm/book3s_segment.S | 7 +++++++
>> 3 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
>> index d06e81842368..7a6b060ceed8 100644
>> --- a/arch/powerpc/kvm/book3s_64_entry.S
>> +++ b/arch/powerpc/kvm/book3s_64_entry.S
>> @@ -78,11 +78,8 @@ do_kvm_interrupt:
>> 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
>> cmpwi r9,KVM_GUEST_MODE_GUEST
>> - ld r9,HSTATE_SCRATCH2(r13)
>> beq kvmppc_interrupt_pr
>> #endif
>> b kvmppc_interrupt_hv
>> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> index f976efb7e4a9..75405ef53238 100644
>> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> @@ -1265,6 +1265,7 @@ hdec_soon:
>> kvmppc_interrupt_hv:
>> /*
>> * Register contents:
>> + * R9 = HSTATE_IN_GUEST
>> * R12 = (guest CR << 32) | interrupt vector
>> * R13 = PACA
>> * guest R12 saved in shadow VCPU SCRATCH0
>> @@ -1272,6 +1273,8 @@ kvmppc_interrupt_hv:
>> * guest R9 saved in HSTATE_SCRATCH2
>> */
>> /* We're now back in the host but in guest MMU context */
>> + cmpwi r9,KVM_GUEST_MODE_HOST_HV
>> + beq kvmppc_bad_host_intr
>> li r9, KVM_GUEST_MODE_HOST_HV
>> stb r9, HSTATE_IN_GUEST(r13)
>>
>> @@ -3272,7 +3275,6 @@ END_FTR_SECTION_IFCLR(CPU_FTR_P9_TM_HV_ASSIST)
>> * cfar is saved in HSTATE_CFAR(r13)
>> * ppr is saved in HSTATE_PPR(r13)
>> */
>> -.global kvmppc_bad_host_intr
>> kvmppc_bad_host_intr:
>> /*
>> * Switch to the emergency stack, but start half-way down in
>> diff --git a/arch/powerpc/kvm/book3s_segment.S b/arch/powerpc/kvm/book3s_segment.S
>> index 1f492aa4c8d6..ef1d88b869bf 100644
>> --- a/arch/powerpc/kvm/book3s_segment.S
>> +++ b/arch/powerpc/kvm/book3s_segment.S
>> @@ -167,8 +167,15 @@ kvmppc_interrupt_pr:
>> * R12 = (guest CR << 32) | exit handler id
>> * R13 = PACA
>> * HSTATE.SCRATCH0 = guest R12
>> + *
>> + * If HV is possible, additionally:
>> + * R9 = HSTATE_IN_GUEST
>> + * HSTATE.SCRATCH2 = guest R9
>> */
>> #ifdef CONFIG_PPC64
>> +#ifdef CONFIG_KVM_BOOK3S_HV_POSSIBLE
>> + ld r9,HSTATE_SCRATCH2(r13)
>> +#endif
>> /* Match 32-bit entry */
>> rotldi r12, r12, 32 /* Flip R12 halves for stw */
>> stw r12, HSTATE_SCRATCH1(r13) /* CR is now in the low half */
>>
>
> --
> Alexey
>
^ 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