* Re: [PATCH 1/2] lockdep: improve current->(hard|soft)irqs_enabled synchronisation with actual irq state
From: Nicholas Piggin @ 2020-07-23 13:11 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-arch, Alexey Kardashevskiy, Will Deacon, linux-kernel,
Ingo Molnar, linuxppc-dev
In-Reply-To: <20200723114010.GO5523@worktop.programming.kicks-ass.net>
Excerpts from Peter Zijlstra's message of July 23, 2020 9:40 pm:
> On Thu, Jul 23, 2020 at 08:56:14PM +1000, Nicholas Piggin wrote:
>
>> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
>> index 3a0db7b0b46e..35060be09073 100644
>> --- a/arch/powerpc/include/asm/hw_irq.h
>> +++ b/arch/powerpc/include/asm/hw_irq.h
>> @@ -200,17 +200,14 @@ static inline bool arch_irqs_disabled(void)
>> #define powerpc_local_irq_pmu_save(flags) \
>> do { \
>> raw_local_irq_pmu_save(flags); \
>> - trace_hardirqs_off(); \
>> + if (!raw_irqs_disabled_flags(flags)) \
>> + trace_hardirqs_off(); \
>> } while(0)
>> #define powerpc_local_irq_pmu_restore(flags) \
>> do { \
>> - if (raw_irqs_disabled_flags(flags)) { \
>> - raw_local_irq_pmu_restore(flags); \
>> - trace_hardirqs_off(); \
>> - } else { \
>> + if (!raw_irqs_disabled_flags(flags)) \
>> trace_hardirqs_on(); \
>> - raw_local_irq_pmu_restore(flags); \
>> - } \
>> + raw_local_irq_pmu_restore(flags); \
>> } while(0)
>
> You shouldn't be calling lockdep from NMI context!
After this patch it doesn't.
trace_hardirqs_on/off implementation appears to expect to be called in NMI
context though, for some reason.
> That is, I recently
> added suport for that on x86:
>
> https://lkml.kernel.org/r/20200623083721.155449112@infradead.org
> https://lkml.kernel.org/r/20200623083721.216740948@infradead.org
>
> But you need to be very careful on how you order things, as you can see
> the above relies on preempt_count() already having been incremented with
> NMI_MASK.
Hmm. My patch seems simpler.
I don't know this stuff very well, I don't really understand what your patch
enables for x86 but at least it shouldn't be incompatible with this one
AFAIKS.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCHv3 2/2] powerpc/pseries: update device tree before ejecting hotplug uevents
From: Nathan Lynch @ 2020-07-23 13:27 UTC (permalink / raw)
To: Pingfan Liu; +Cc: cheloha, kexec, ldufour, linuxppc-dev, Hari Bathini
In-Reply-To: <1595382730-10565-2-git-send-email-kernelfans@gmail.com>
Pingfan Liu <kernelfans@gmail.com> writes:
> A bug is observed on pseries by taking the following steps on rhel:
> -1. drmgr -c mem -r -q 5
> -2. echo c > /proc/sysrq-trigger
>
> And then, the failure looks like:
> kdump: saving to /sysroot//var/crash/127.0.0.1-2020-01-16-02:06:14/
> kdump: saving vmcore-dmesg.txt
> kdump: saving vmcore-dmesg.txt complete
> kdump: saving vmcore
> Checking for memory holes : [ 0.0 %] / Checking for memory holes : [100.0 %] | Excluding unnecessary pages : [100.0 %] \ Copying data : [ 0.3 %] - eta: 38s[ 44.337636] hash-mmu: mm: Hashing failure ! EA=0x7fffba400000 access=0x8000000000000004 current=makedumpfile
> [ 44.337663] hash-mmu: trap=0x300 vsid=0x13a109c ssize=1 base psize=2 psize 2 pte=0xc000000050000504
> [ 44.337677] hash-mmu: mm: Hashing failure ! EA=0x7fffba400000 access=0x8000000000000004 current=makedumpfile
> [ 44.337692] hash-mmu: trap=0x300 vsid=0x13a109c ssize=1 base psize=2 psize 2 pte=0xc000000050000504
> [ 44.337708] makedumpfile[469]: unhandled signal 7 at 00007fffba400000 nip 00007fffbbc4d7fc lr 000000011356ca3c code 2
> [ 44.338548] Core dump to |/bin/false pipe failed
> /lib/kdump-lib-initramfs.sh: line 98: 469 Bus error $CORE_COLLECTOR /proc/vmcore $_mp/$KDUMP_PATH/$HOST_IP-$DATEDIR/vmcore-incomplete
> kdump: saving vmcore failed
>
> * Root cause *
> After analyzing, it turns out that in the current implementation,
> when hot-removing lmb, the KOBJ_REMOVE event ejects before the dt updating as
> the code __remove_memory() comes before drmem_update_dt().
> So in kdump kernel, when read_from_oldmem() resorts to
> pSeries_lpar_hpte_insert() to install hpte, but fails with -2 due to
> non-exist pfn. And finally, low_hash_fault() raise SIGBUS to process, as it
> can be observed "Bus error"
>
> From a viewpoint of listener and publisher, the publisher notifies the
> listener before data is ready. This introduces a problem where udev
> launches kexec-tools (due to KOBJ_REMOVE) and loads a stale dt before
> updating. And in capture kernel, makedumpfile will access the memory based
> on the stale dt info, and hit a SIGBUS error due to an un-existed lmb.
>
> * Fix *
> In order to fix this issue, update dt before __remove_memory(), and
> accordingly the same rule in hot-add path.
>
> This will introduce extra dt updating payload for each involved lmb when hotplug.
> But it should be fine since drmem_update_dt() is memory based operation and
> hotplug is not a hot path.
This is great analysis but the performance implications of the change
are grave. The add/remove paths here are already O(n) where n is the
quantity of memory assigned to the LP, this change would make it O(n^2):
dlpar_memory_add_by_count
for_each_drmem_lmb <--
dlpar_add_lmb
drmem_update_dt(_v1|_v2)
for_each_drmem_lmb <--
Memory add/remove isn't a hot path but quadratic runtime complexity
isn't acceptable. Its current performance is bad enough that I have
internal bugs open on it.
Not to mention we leak memory every time drmem_update_dt is called
because we can't safely free device tree properties :-(
Also note that this sort of reverts (fixes?) 063b8b1251fd
("powerpc/pseries/memory-hotplug: Only update DT once per memory DLPAR
request").
^ permalink raw reply
* Re: [PATCH v3 0/6] powerpc: queued spinlocks and rwlocks
From: Nicholas Piggin @ 2020-07-23 13:30 UTC (permalink / raw)
To: Waiman Long, Peter Zijlstra
Cc: linux-arch, Will Deacon, Boqun Feng, linux-kernel, kvm-ppc,
virtualization, Ingo Molnar, linuxppc-dev
In-Reply-To: <eaabf501-80fe-dd15-c03c-f75ce4f75877@redhat.com>
Excerpts from Waiman Long's message of July 22, 2020 12:36 am:
> On 7/21/20 7:08 AM, Nicholas Piggin wrote:
>> diff --git a/arch/powerpc/include/asm/qspinlock.h b/arch/powerpc/include/asm/qspinlock.h
>> index b752d34517b3..26d8766a1106 100644
>> --- a/arch/powerpc/include/asm/qspinlock.h
>> +++ b/arch/powerpc/include/asm/qspinlock.h
>> @@ -31,16 +31,57 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
>>
>> #else
>> extern void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
>> +extern void queued_spin_lock_slowpath_queue(struct qspinlock *lock);
>> #endif
>>
>> static __always_inline void queued_spin_lock(struct qspinlock *lock)
>> {
>> - u32 val = 0;
>> -
>> - if (likely(atomic_try_cmpxchg_lock(&lock->val, &val, _Q_LOCKED_VAL)))
>> + atomic_t *a = &lock->val;
>> + u32 val;
>> +
>> +again:
>> + asm volatile(
>> +"1:\t" PPC_LWARX(%0,0,%1,1) " # queued_spin_lock \n"
>> + : "=&r" (val)
>> + : "r" (&a->counter)
>> + : "memory");
>> +
>> + if (likely(val == 0)) {
>> + asm_volatile_goto(
>> + " stwcx. %0,0,%1 \n"
>> + " bne- %l[again] \n"
>> + "\t" PPC_ACQUIRE_BARRIER " \n"
>> + :
>> + : "r"(_Q_LOCKED_VAL), "r" (&a->counter)
>> + : "cr0", "memory"
>> + : again );
>> return;
>> -
>> - queued_spin_lock_slowpath(lock, val);
>> + }
>> +
>> + if (likely(val == _Q_LOCKED_VAL)) {
>> + asm_volatile_goto(
>> + " stwcx. %0,0,%1 \n"
>> + " bne- %l[again] \n"
>> + :
>> + : "r"(_Q_LOCKED_VAL | _Q_PENDING_VAL), "r" (&a->counter)
>> + : "cr0", "memory"
>> + : again );
>> +
>> + atomic_cond_read_acquire(a, !(VAL & _Q_LOCKED_MASK));
>> +// clear_pending_set_locked(lock);
>> + WRITE_ONCE(lock->locked_pending, _Q_LOCKED_VAL);
>> +// lockevent_inc(lock_pending);
>> + return;
>> + }
>> +
>> + if (val == _Q_PENDING_VAL) {
>> + int cnt = _Q_PENDING_LOOPS;
>> + val = atomic_cond_read_relaxed(a,
>> + (VAL != _Q_PENDING_VAL) || !cnt--);
>> + if (!(val & ~_Q_LOCKED_MASK))
>> + goto again;
>> + }
>> + queued_spin_lock_slowpath_queue(lock);
>> }
>> #define queued_spin_lock queued_spin_lock
>>
>
> I am fine with the arch code override some part of the generic code.
Cool.
>> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
>> index b9515fcc9b29..ebcc6f5d99d5 100644
>> --- a/kernel/locking/qspinlock.c
>> +++ b/kernel/locking/qspinlock.c
>> @@ -287,10 +287,14 @@ static __always_inline u32 __pv_wait_head_or_lock(struct qspinlock *lock,
>>
>> #ifdef CONFIG_PARAVIRT_SPINLOCKS
>> #define queued_spin_lock_slowpath native_queued_spin_lock_slowpath
>> +#define queued_spin_lock_slowpath_queue native_queued_spin_lock_slowpath_queue
>> #endif
>>
>> #endif /* _GEN_PV_LOCK_SLOWPATH */
>>
>> +void queued_spin_lock_slowpath_queue(struct qspinlock *lock);
>> +static void __queued_spin_lock_slowpath_queue(struct qspinlock *lock);
>> +
>> /**
>> * queued_spin_lock_slowpath - acquire the queued spinlock
>> * @lock: Pointer to queued spinlock structure
>> @@ -314,12 +318,6 @@ static __always_inline u32 __pv_wait_head_or_lock(struct qspinlock *lock,
>> */
>> void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>> {
>> - struct mcs_spinlock *prev, *next, *node;
>> - u32 old, tail;
>> - int idx;
>> -
>> - BUILD_BUG_ON(CONFIG_NR_CPUS >= (1U << _Q_TAIL_CPU_BITS));
>> -
>> if (pv_enabled())
>> goto pv_queue;
>>
>> @@ -397,6 +395,26 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>> queue:
>> lockevent_inc(lock_slowpath);
>> pv_queue:
>> + __queued_spin_lock_slowpath_queue(lock);
>> +}
>> +EXPORT_SYMBOL(queued_spin_lock_slowpath);
>> +
>> +void queued_spin_lock_slowpath_queue(struct qspinlock *lock)
>> +{
>> + lockevent_inc(lock_slowpath);
>> + __queued_spin_lock_slowpath_queue(lock);
>> +}
>> +EXPORT_SYMBOL(queued_spin_lock_slowpath_queue);
>> +
>> +static void __queued_spin_lock_slowpath_queue(struct qspinlock *lock)
>> +{
>> + struct mcs_spinlock *prev, *next, *node;
>> + u32 old, tail;
>> + u32 val;
>> + int idx;
>> +
>> + BUILD_BUG_ON(CONFIG_NR_CPUS >= (1U << _Q_TAIL_CPU_BITS));
>> +
>> node = this_cpu_ptr(&qnodes[0].mcs);
>> idx = node->count++;
>> tail = encode_tail(smp_processor_id(), idx);
>> @@ -559,7 +577,6 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>> */
>> __this_cpu_dec(qnodes[0].mcs.count);
>> }
>> -EXPORT_SYMBOL(queued_spin_lock_slowpath);
>>
>> /*
>> * Generate the paravirt code for queued_spin_unlock_slowpath().
>>
> I would prefer to extract out the pending bit handling code out into a
> separate helper function which can be overridden by the arch code
> instead of breaking the slowpath into 2 pieces.
You mean have the arch provide a queued_spin_lock_slowpath_pending
function that the slow path calls?
I would actually prefer the pending handling can be made inline in
the queued_spin_lock function, especially with out-of-line locks it
makes sense to put it there.
We could ifdef out queued_spin_lock_slowpath_queue if it's not used,
then __queued_spin_lock_slowpath_queue would be inlined into the
caller so there would be no split?
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 2/5] powerpc: Allow 4096 bytes of stack expansion for the signal frame
From: Daniel Axtens @ 2020-07-23 13:35 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: linux-arch, hughd, linux-kernel
In-Reply-To: <20200703141327.1732550-2-mpe@ellerman.id.au>
Hi Michael,
Unfortunately, this patch doesn't completely solve the problem.
Trying the original reproducer, I'm still able to trigger the crash even
with this patch, although not 100% of the time. (If I turn ASLR off
outside of tmux it reliably crashes, if I turn ASLR off _inside_ of tmux
it reliably succeeds; all of this is on a serial console.)
./foo 1241000 & sleep 1; killall -USR1 foo; echo ok
If I add some debugging information, I see that I'm getting
address + 4096 = 7fffffed0fa0
gpr1 = 7fffffed1020
So address + 4096 is 0x80 bytes below the 4k window. I haven't been able
to figure out why, gdb gives me a NIP in __kernel_sigtramp_rt64 but I
don't know what to make of that.
Kind regards,
Daniel
P.S. I don't know what your policy on linking to kernel bugzilla is, but
if you want:
Link: https://bugzilla.kernel.org/show_bug.cgi?id=205183
> Reported-by: Tom Lane <tgl@sss.pgh.pa.us>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/mm/fault.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 641fc5f3d7dd..ed01329dd12b 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -274,7 +274,7 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
> /*
> * N.B. The POWER/Open ABI allows programs to access up to
> * 288 bytes below the stack pointer.
> - * The kernel signal delivery code writes up to about 1.5kB
> + * The kernel signal delivery code writes up to 4KB
> * below the stack pointer (r1) before decrementing it.
> * The exec code can write slightly over 640kB to the stack
> * before setting the user r1. Thus we allow the stack to
> @@ -299,7 +299,7 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
> * between the last mapped region and the stack will
> * expand the stack rather than segfaulting.
> */
> - if (address + 2048 >= uregs->gpr[1])
> + if (address + 4096 >= uregs->gpr[1])
> return false;
>
> if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH v2 1/3] module: Rename module_alloc() to text_alloc() and move to kernel proper
From: Ard Biesheuvel @ 2020-07-23 12:42 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Catalin Marinas, Kefeng Wang, Paul Mackerras, Zong Li, Andi Kleen,
Paul Burton, Vincent Whitchurch, Petr Mladek, Brian Gerst,
Andy Lutomirski, Thomas Gleixner, Jiri Kosina, Anup Patel,
Linux Kernel Mailing List, Philipp Rudo, Torsten Duwe,
Masami Hiramatsu, Andrew Morton, Mark Rutland,
James E.J. Bottomley, Vincent Chen, Omar Sandoval, open list:S390,
Joe Lawrence, Helge Deller, John Fastabend, Anil S Keshavamurthy,
Yonghong Song, Iurii Zaikin, Andrii Nakryiko, Thomas Huth,
Vasily Gorbik, moderated list:ARM PORT, Daniel Axtens,
Damien Le Moal, Martin KaFai Lau, Song Liu, Josh Poimboeuf,
Heiko Carstens, Alexei Starovoitov, Atish Patra, Will Deacon,
Daniel Borkmann, Masahiro Yamada, Nayna Jain, Ley Foon Tan,
Christian Borntraeger, Sami Tolvanen, Naveen N. Rao, Mao Han,
Marco Elver, Steven Rostedt, Babu Moger, Borislav Petkov,
Greentime Hu, Ben Dooks, Guan Xuetao, Thomas Bogendoerfer,
open list:PARISC ARCHITECTURE, Jessica Yu,
open list:BPF JIT for MIPS 32-BIT AND 64-BIT, David S. Miller,
Thiago Jung Bauermann, Peter Zijlstra, David Howells,
Amit Daniel Kachhap, Sandipan Das, H. Peter Anvin,
open list:SPARC + UltraSPARC sparc/sparc64,
open list:RISC-V ARCHITECTURE, Miroslav Benes, Jiri Olsa,
Tiezhu Yang, Vincenzo Frascino, Anders Roxell, Sven Schnelle,
maintainer:X86 ARCHITECTURE 32-BIT AND 64-BIT, Russell King,
Mike Rapoport, Ingo Molnar, Albert Ou, Paul E. McKenney,
Paul Walmsley, KP Singh, Dmitry Vyukov, Nick Hu,
open list:BPF JIT for MIPS 32-BIT AND 64-BIT, open list:MIPS,
Palmer Dabbelt, open list:LINUX FOR POWERPC 32-BIT AND 64-BIT
In-Reply-To: <20200723015127.GE45081@linux.intel.com>
On Thu, 23 Jul 2020 at 04:52, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Thu, Jul 16, 2020 at 06:49:09PM +0200, Christophe Leroy wrote:
> > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> a écrit :
> >
> > > Rename module_alloc() to text_alloc() and module_memfree() to
> > > text_memfree(), and move them to kernel/text.c, which is unconditionally
> > > compiled to the kernel proper. This allows kprobes, ftrace and bpf to
> > > allocate space for executable code without requiring to compile the modules
> > > support (CONFIG_MODULES=y) in.
> >
> > You are not changing enough in powerpc to have this work.
> > On powerpc 32 bits (6xx), when STRICT_KERNEL_RWX is selected, the vmalloc
> > space is set to NX (no exec) at segment level (ie by 256Mbytes zone) unless
> > CONFIG_MODULES is selected.
> >
> > Christophe
>
> This has been deduced down to:
>
> https://lore.kernel.org/lkml/20200717030422.679972-1-jarkko.sakkinen@linux.intel.com/
>
> I.e. not intruding PPC anymore :-)
>
Ok, so after the elaborate discussion we had between Jessica, Russell,
Peter, Will, Mark, you and myself, where we pointed out that
a) a single text_alloc() abstraction for bpf, kprobes and ftrace does
not fit other architectures very well, and
b) that module_alloc() is not suitable as a default to base text_alloc() on,
you went ahead and implemented that anyway, but only cc'ing Peter,
akpm, Masami and the mm list this time?
Sorry, but that is not how it works. Once people get pulled into a
discussion, you cannot dismiss them or their feedback like that and go
off and do your own thing anyway. Generic features like this are
tricky to get right, and it will likely take many iterations and input
from many different people.
^ permalink raw reply
* Re: [PATCH v3 5/6] powerpc/pseries: implement paravirt qspinlocks for SPLPAR
From: Peter Zijlstra @ 2020-07-23 14:00 UTC (permalink / raw)
To: Waiman Long
Cc: linux-arch, Boqun Feng, virtualization, linuxppc-dev,
Nicholas Piggin, linux-kernel, Ingo Molnar, kvm-ppc, Will Deacon
In-Reply-To: <8265d782-4e50-a9b2-a908-0cb588ffa09c@redhat.com>
On Thu, Jul 09, 2020 at 12:06:13PM -0400, Waiman Long wrote:
> We don't really need to do a pv_spinlocks_init() if pv_kick() isn't
> supported.
Waiman, if you cannot explain how not having kick is a sane thing, what
are you saying here?
^ permalink raw reply
* Re: [PATCH v2 2/2] KVM: PPC: Book3S HV: rework secure mem slot dropping
From: Laurent Dufour @ 2020-07-23 14:06 UTC (permalink / raw)
To: bharata, linuxram
Cc: linux-kernel, kvm-ppc, paulus, sukadev, linuxppc-dev, bauerman
In-Reply-To: <4a3caeaf-cd0c-fcd7-0a97-f367a5f78dac@linux.ibm.com>
Le 23/07/2020 à 14:32, Laurent Dufour a écrit :
> Le 23/07/2020 à 05:36, Bharata B Rao a écrit :
>> On Tue, Jul 21, 2020 at 12:42:02PM +0200, Laurent Dufour wrote:
>>> When a secure memslot is dropped, all the pages backed in the secure device
>>> (aka really backed by secure memory by the Ultravisor) should be paged out
>>> to a normal page. Previously, this was achieved by triggering the page
>>> fault mechanism which is calling kvmppc_svm_page_out() on each pages.
>>>
>>> This can't work when hot unplugging a memory slot because the memory slot
>>> is flagged as invalid and gfn_to_pfn() is then not trying to access the
>>> page, so the page fault mechanism is not triggered.
>>>
>>> Since the final goal is to make a call to kvmppc_svm_page_out() it seems
>>> simpler to directly calling it instead of triggering such a mechanism. This
>>> way kvmppc_uvmem_drop_pages() can be called even when hot unplugging a
>>> memslot.
>>>
>>> Since kvmppc_uvmem_drop_pages() is already holding kvm->arch.uvmem_lock,
>>> the call to __kvmppc_svm_page_out() is made.
>>> As __kvmppc_svm_page_out needs the vma pointer to migrate the pages, the
>>> VMA is fetched in a lazy way, to not trigger find_vma() all the time. In
>>> addition, the mmap_sem is help in read mode during that time, not in write
>>> mode since the virual memory layout is not impacted, and
>>> kvm->arch.uvmem_lock prevents concurrent operation on the secure device.
>>>
>>> Cc: Ram Pai <linuxram@us.ibm.com>
>>> Cc: Bharata B Rao <bharata@linux.ibm.com>
>>> Cc: Paul Mackerras <paulus@ozlabs.org>
>>> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
>>> ---
>>> arch/powerpc/kvm/book3s_hv_uvmem.c | 54 ++++++++++++++++++++----------
>>> 1 file changed, 37 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c
>>> b/arch/powerpc/kvm/book3s_hv_uvmem.c
>>> index 5a4b02d3f651..ba5c7c77cc3a 100644
>>> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
>>> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
>>> @@ -624,35 +624,55 @@ static inline int kvmppc_svm_page_out(struct
>>> vm_area_struct *vma,
>>> * fault on them, do fault time migration to replace the device PTEs in
>>> * QEMU page table with normal PTEs from newly allocated pages.
>>> */
>>> -void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *free,
>>> +void kvmppc_uvmem_drop_pages(const struct kvm_memory_slot *slot,
>>> struct kvm *kvm, bool skip_page_out)
>>> {
>>> int i;
>>> struct kvmppc_uvmem_page_pvt *pvt;
>>> - unsigned long pfn, uvmem_pfn;
>>> - unsigned long gfn = free->base_gfn;
>>> + struct page *uvmem_page;
>>> + struct vm_area_struct *vma = NULL;
>>> + unsigned long uvmem_pfn, gfn;
>>> + unsigned long addr, end;
>>> +
>>> + mmap_read_lock(kvm->mm);
>>> +
>>> + addr = slot->userspace_addr;
>>
>> We typically use gfn_to_hva() for that, but that won't work for a
>> memslot that is already marked INVALID which is the case here.
>> I think it is ok to access slot->userspace_addr here of an INVALID
>> memslot, but just thought of explictly bringing this up.
>
> Which explicitly mentioned above in the patch's description:
>
> This can't work when hot unplugging a memory slot because the memory slot
> is flagged as invalid and gfn_to_pfn() is then not trying to access the
> page, so the page fault mechanism is not triggered.
>
>>
>>> + end = addr + (slot->npages * PAGE_SIZE);
>>> - for (i = free->npages; i; --i, ++gfn) {
>>> - struct page *uvmem_page;
>>> + gfn = slot->base_gfn;
>>> + for (i = slot->npages; i; --i, ++gfn, addr += PAGE_SIZE) {
>>> +
>>> + /* Fetch the VMA if addr is not in the latest fetched one */
>>> + if (!vma || (addr < vma->vm_start || addr >= vma->vm_end)) {
>>> + vma = find_vma_intersection(kvm->mm, addr, end);
>>> + if (!vma ||
>>> + vma->vm_start > addr || vma->vm_end < end) {
>>> + pr_err("Can't find VMA for gfn:0x%lx\n", gfn);
>>> + break;
>>> + }
>>> + }
>>
>> In Ram's series, kvmppc_memslot_page_merge() also walks the VMAs spanning
>> the memslot, but it uses a different logic for the same. Why can't these
>> two cases use the same method to walk the VMAs? Is there anything subtly
>> different between the two cases?
>
> This is probably doable. At the time I wrote that patch, the
> kvmppc_memslot_page_merge() was not yet introduced AFAIR.
>
> This being said, I'd help a lot to factorize that code... I let Ram dealing with
> that ;)
Indeed I don't think this is relevant, the loop in kvmppc_memslot_page_merge()
deals with one call (to ksm_advise) per VMA, while this code is dealing with one
call per page of the VMA, which completely different.
I don't think merging the both will be a good idea.
Cheers,
Laurent.
^ permalink raw reply
* Re: [PATCH v3 5/6] powerpc/pseries: implement paravirt qspinlocks for SPLPAR
From: Nicholas Piggin @ 2020-07-23 14:09 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman
Cc: linux-arch, Peter Zijlstra, Boqun Feng, linux-kernel, kvm-ppc,
virtualization, Ingo Molnar, Waiman Long, Will Deacon
In-Reply-To: <874kqhvu1v.fsf@mpe.ellerman.id.au>
Excerpts from Michael Ellerman's message of July 9, 2020 8:53 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/paravirt.h | 28 ++++++++
>> arch/powerpc/include/asm/qspinlock.h | 66 +++++++++++++++++++
>> arch/powerpc/include/asm/qspinlock_paravirt.h | 7 ++
>> arch/powerpc/platforms/pseries/Kconfig | 5 ++
>> arch/powerpc/platforms/pseries/setup.c | 6 +-
>> include/asm-generic/qspinlock.h | 2 +
>
> Another ack?
>
>> diff --git a/arch/powerpc/include/asm/paravirt.h b/arch/powerpc/include/asm/paravirt.h
>> index 7a8546660a63..f2d51f929cf5 100644
>> --- a/arch/powerpc/include/asm/paravirt.h
>> +++ b/arch/powerpc/include/asm/paravirt.h
>> @@ -45,6 +55,19 @@ static inline void yield_to_preempted(int cpu, u32 yield_count)
>> {
>> ___bad_yield_to_preempted(); /* This would be a bug */
>> }
>> +
>> +extern void ___bad_yield_to_any(void);
>> +static inline void yield_to_any(void)
>> +{
>> + ___bad_yield_to_any(); /* This would be a bug */
>> +}
>
> Why do we do that rather than just not defining yield_to_any() at all
> and letting the build fail on that?
>
> There's a condition somewhere that we know will false at compile time
> and drop the call before linking?
Mainly so you could use it in if (IS_ENABLED()) blocks, but would still
catch the (presumably buggy) case where something calls it without the
option set.
I think I had it arranged a different way that was using IS_ENABLED
earlier and changed it but might as well keep it this way.
>
>> diff --git a/arch/powerpc/include/asm/qspinlock_paravirt.h b/arch/powerpc/include/asm/qspinlock_paravirt.h
>> new file mode 100644
>> index 000000000000..750d1b5e0202
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/qspinlock_paravirt.h
>> @@ -0,0 +1,7 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +#ifndef __ASM_QSPINLOCK_PARAVIRT_H
>> +#define __ASM_QSPINLOCK_PARAVIRT_H
>
> _ASM_POWERPC_QSPINLOCK_PARAVIRT_H please.
>
>> +
>> +EXPORT_SYMBOL(__pv_queued_spin_unlock);
>
> Why's that in a header? Should that (eventually) go with the generic implementation?
Yeah the qspinlock_paravirt.h header is a bit weird and only gets
included into kernel/locking/qspinlock.c
>> diff --git a/arch/powerpc/platforms/pseries/Kconfig b/arch/powerpc/platforms/pseries/Kconfig
>> index 24c18362e5ea..756e727b383f 100644
>> --- a/arch/powerpc/platforms/pseries/Kconfig
>> +++ b/arch/powerpc/platforms/pseries/Kconfig
>> @@ -25,9 +25,14 @@ config PPC_PSERIES
>> select SWIOTLB
>> default y
>>
>> +config PARAVIRT_SPINLOCKS
>> + bool
>> + default n
>
> default n is the default.
>
>> diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
>> index 2db8469e475f..747a203d9453 100644
>> --- a/arch/powerpc/platforms/pseries/setup.c
>> +++ b/arch/powerpc/platforms/pseries/setup.c
>> @@ -771,8 +771,12 @@ static void __init pSeries_setup_arch(void)
>> if (firmware_has_feature(FW_FEATURE_LPAR)) {
>> vpa_init(boot_cpuid);
>>
>> - if (lppaca_shared_proc(get_lppaca()))
>> + if (lppaca_shared_proc(get_lppaca())) {
>> static_branch_enable(&shared_processor);
>> +#ifdef CONFIG_PARAVIRT_SPINLOCKS
>> + pv_spinlocks_init();
>> +#endif
>> + }
>
> We could avoid the ifdef with this I think?
Yes I think so.
Thanks,
Nick
^ permalink raw reply
* Re: [RFC PATCH 4/5] powerpc/mm: Remove custom stack expansion checking
From: Daniel Axtens @ 2020-07-23 14:11 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: linux-arch, hughd, linux-kernel
In-Reply-To: <20200703141327.1732550-4-mpe@ellerman.id.au>
Hi Michael,
> We have powerpc specific logic in our page fault handling to decide if
> an access to an unmapped address below the stack pointer should expand
> the stack VMA.
>
> The logic aims to prevent userspace from doing bad accesses below the
> stack pointer. However as long as the stack is < 1MB in size, we allow
> all accesses without further checks. Adding some debug I see that I
> can do a full kernel build and LTP run, and not a single process has
> used more than 1MB of stack. So for the majority of processes the
> logic never even fires.
>
> We also recently found a nasty bug in this code which could cause
> userspace programs to be killed during signal delivery. It went
> unnoticed presumably because most processes use < 1MB of stack.
>
> The generic mm code has also grown support for stack guard pages since
> this code was originally written, so the most heinous case of the
> stack expanding into other mappings is now handled for us.
>
> Finally although some other arches have special logic in this path,
> from what I can tell none of x86, arm64, arm and s390 impose any extra
> checks other than those in expand_stack().
>
> So drop our complicated logic and like other architectures just let
> the stack expand as long as its within the rlimit.
>
I applied and tested this. While I wouldn't call my testing
comprehensive, I have not been able to reproduce the crash with this
patch applied.
Kind regards,
Daniel
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/mm/fault.c | 106 ++--------------------------------------
> 1 file changed, 5 insertions(+), 101 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index ed01329dd12b..925a7231abb3 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -42,39 +42,7 @@
> #include <asm/kup.h>
> #include <asm/inst.h>
>
> -/*
> - * Check whether the instruction inst is a store using
> - * an update addressing form which will update r1.
> - */
> -static bool store_updates_sp(struct ppc_inst inst)
> -{
> - /* check for 1 in the rA field */
> - if (((ppc_inst_val(inst) >> 16) & 0x1f) != 1)
> - return false;
> - /* check major opcode */
> - switch (ppc_inst_primary_opcode(inst)) {
> - case OP_STWU:
> - case OP_STBU:
> - case OP_STHU:
> - case OP_STFSU:
> - case OP_STFDU:
> - return true;
> - case OP_STD: /* std or stdu */
> - return (ppc_inst_val(inst) & 3) == 1;
> - case OP_31:
> - /* check minor opcode */
> - switch ((ppc_inst_val(inst) >> 1) & 0x3ff) {
> - case OP_31_XOP_STDUX:
> - case OP_31_XOP_STWUX:
> - case OP_31_XOP_STBUX:
> - case OP_31_XOP_STHUX:
> - case OP_31_XOP_STFSUX:
> - case OP_31_XOP_STFDUX:
> - return true;
> - }
> - }
> - return false;
> -}
> +
> /*
> * do_page_fault error handling helpers
> */
> @@ -267,54 +235,6 @@ static bool bad_kernel_fault(struct pt_regs *regs, unsigned long error_code,
> return false;
> }
>
> -static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
> - struct vm_area_struct *vma, unsigned int flags,
> - bool *must_retry)
> -{
> - /*
> - * N.B. The POWER/Open ABI allows programs to access up to
> - * 288 bytes below the stack pointer.
> - * The kernel signal delivery code writes up to 4KB
> - * below the stack pointer (r1) before decrementing it.
> - * The exec code can write slightly over 640kB to the stack
> - * before setting the user r1. Thus we allow the stack to
> - * expand to 1MB without further checks.
> - */
> - if (address + 0x100000 < vma->vm_end) {
> - struct ppc_inst __user *nip = (struct ppc_inst __user *)regs->nip;
> - /* get user regs even if this fault is in kernel mode */
> - struct pt_regs *uregs = current->thread.regs;
> - if (uregs == NULL)
> - return true;
> -
> - /*
> - * A user-mode access to an address a long way below
> - * the stack pointer is only valid if the instruction
> - * is one which would update the stack pointer to the
> - * address accessed if the instruction completed,
> - * i.e. either stwu rs,n(r1) or stwux rs,r1,rb
> - * (or the byte, halfword, float or double forms).
> - *
> - * If we don't check this then any write to the area
> - * between the last mapped region and the stack will
> - * expand the stack rather than segfaulting.
> - */
> - if (address + 4096 >= uregs->gpr[1])
> - return false;
> -
> - if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) &&
> - access_ok(nip, sizeof(*nip))) {
> - struct ppc_inst inst;
> -
> - if (!probe_user_read_inst(&inst, nip))
> - return !store_updates_sp(inst);
> - *must_retry = true;
> - }
> - return true;
> - }
> - return false;
> -}
> -
> #ifdef CONFIG_PPC_MEM_KEYS
> static bool access_pkey_error(bool is_write, bool is_exec, bool is_pkey,
> struct vm_area_struct *vma)
> @@ -480,7 +400,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
> int is_user = user_mode(regs);
> int is_write = page_fault_is_write(error_code);
> vm_fault_t fault, major = 0;
> - bool must_retry = false;
> bool kprobe_fault = kprobe_page_fault(regs, 11);
>
> if (unlikely(debugger_fault_handler(regs) || kprobe_fault))
> @@ -569,30 +488,15 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
> vma = find_vma(mm, address);
> if (unlikely(!vma))
> return bad_area(regs, address);
> - if (likely(vma->vm_start <= address))
> - goto good_area;
> - if (unlikely(!(vma->vm_flags & VM_GROWSDOWN)))
> - return bad_area(regs, address);
>
> - /* The stack is being expanded, check if it's valid */
> - if (unlikely(bad_stack_expansion(regs, address, vma, flags,
> - &must_retry))) {
> - if (!must_retry)
> + if (unlikely(vma->vm_start > address)) {
> + if (unlikely(!(vma->vm_flags & VM_GROWSDOWN)))
> return bad_area(regs, address);
>
> - mmap_read_unlock(mm);
> - if (fault_in_pages_readable((const char __user *)regs->nip,
> - sizeof(unsigned int)))
> - return bad_area_nosemaphore(regs, address);
> - goto retry;
> + if (unlikely(expand_stack(vma, address)))
> + return bad_area(regs, address);
> }
>
> - /* Try to expand it */
> - if (unlikely(expand_stack(vma, address)))
> - return bad_area(regs, address);
> -
> -good_area:
> -
> #ifdef CONFIG_PPC_MEM_KEYS
> if (unlikely(access_pkey_error(is_write, is_exec,
> (error_code & DSISR_KEYFAULT), vma)))
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH v3 0/6] powerpc: queued spinlocks and rwlocks
From: Waiman Long @ 2020-07-23 14:29 UTC (permalink / raw)
To: Nicholas Piggin, Peter Zijlstra
Cc: linux-arch, Will Deacon, Boqun Feng, linux-kernel, kvm-ppc,
virtualization, Ingo Molnar, linuxppc-dev
In-Reply-To: <1595510571.u39qfc8d1o.astroid@bobo.none>
On 7/23/20 9:30 AM, Nicholas Piggin wrote:
>> I would prefer to extract out the pending bit handling code out into a
>> separate helper function which can be overridden by the arch code
>> instead of breaking the slowpath into 2 pieces.
> You mean have the arch provide a queued_spin_lock_slowpath_pending
> function that the slow path calls?
>
> I would actually prefer the pending handling can be made inline in
> the queued_spin_lock function, especially with out-of-line locks it
> makes sense to put it there.
>
> We could ifdef out queued_spin_lock_slowpath_queue if it's not used,
> then __queued_spin_lock_slowpath_queue would be inlined into the
> caller so there would be no split?
The pending code is an optimization for lightly contended locks. That is
why I think it is appropriate to extract it into a helper function and
mark it as such.
You can certainly put the code in the arch's spin_lock code, you just
has to override the generic pending code by a null function.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v3 4/6] powerpc/64s: implement queued spinlocks and rwlocks
From: Michal Suchánek @ 2020-07-23 14:37 UTC (permalink / raw)
To: Nicholas Piggin
Cc: linux-arch, Peter Zijlstra, Will Deacon, Boqun Feng, linux-kernel,
kvm-ppc, virtualization, Ingo Molnar, Waiman Long, linuxppc-dev
In-Reply-To: <20200706043540.1563616-5-npiggin@gmail.com>
On Mon, Jul 06, 2020 at 02:35:38PM +1000, Nicholas Piggin wrote:
> These have shown significantly improved performance and fairness when
> spinlock contention is moderate to high on very large systems.
>
> [ Numbers hopefully forthcoming after more testing, but initial
> results look good ]
>
> Thanks to the fast path, single threaded performance is not noticably
> hurt.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/Kconfig | 13 ++++++++++++
> arch/powerpc/include/asm/Kbuild | 2 ++
> arch/powerpc/include/asm/qspinlock.h | 25 +++++++++++++++++++++++
> arch/powerpc/include/asm/spinlock.h | 5 +++++
> arch/powerpc/include/asm/spinlock_types.h | 5 +++++
> arch/powerpc/lib/Makefile | 3 +++
> include/asm-generic/qspinlock.h | 2 ++
> 7 files changed, 55 insertions(+)
> create mode 100644 arch/powerpc/include/asm/qspinlock.h
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 24ac85c868db..17663ea57697 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -146,6 +146,8 @@ config PPC
> select ARCH_SUPPORTS_ATOMIC_RMW
> select ARCH_USE_BUILTIN_BSWAP
> select ARCH_USE_CMPXCHG_LOCKREF if PPC64
> + select ARCH_USE_QUEUED_RWLOCKS if PPC_QUEUED_SPINLOCKS
> + select ARCH_USE_QUEUED_SPINLOCKS if PPC_QUEUED_SPINLOCKS
> select ARCH_WANT_IPC_PARSE_VERSION
> select ARCH_WEAK_RELEASE_ACQUIRE
> select BINFMT_ELF
> @@ -492,6 +494,17 @@ config HOTPLUG_CPU
>
> Say N if you are unsure.
>
> +config PPC_QUEUED_SPINLOCKS
> + bool "Queued spinlocks"
> + depends on SMP
> + default "y" if PPC_BOOK3S_64
> + help
> + Say Y here to use to use queued spinlocks which are more complex
> + but give better salability and fairness on large SMP and NUMA
^ +c?
Thanks
Michal
> + systems.
> +
> + If unsure, say "Y" if you have lots of cores, otherwise "N".
> +
> config ARCH_CPU_PROBE_RELEASE
> def_bool y
> depends on HOTPLUG_CPU
> diff --git a/arch/powerpc/include/asm/Kbuild b/arch/powerpc/include/asm/Kbuild
> index dadbcf3a0b1e..1dd8b6adff5e 100644
> --- a/arch/powerpc/include/asm/Kbuild
> +++ b/arch/powerpc/include/asm/Kbuild
> @@ -6,5 +6,7 @@ generated-y += syscall_table_spu.h
> generic-y += export.h
> generic-y += local64.h
> generic-y += mcs_spinlock.h
> +generic-y += qrwlock.h
> +generic-y += qspinlock.h
> generic-y += vtime.h
> generic-y += early_ioremap.h
> diff --git a/arch/powerpc/include/asm/qspinlock.h b/arch/powerpc/include/asm/qspinlock.h
> new file mode 100644
> index 000000000000..c49e33e24edd
> --- /dev/null
> +++ b/arch/powerpc/include/asm/qspinlock.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_POWERPC_QSPINLOCK_H
> +#define _ASM_POWERPC_QSPINLOCK_H
> +
> +#include <asm-generic/qspinlock_types.h>
> +
> +#define _Q_PENDING_LOOPS (1 << 9) /* not tuned */
> +
> +#define smp_mb__after_spinlock() smp_mb()
> +
> +static __always_inline int queued_spin_is_locked(struct qspinlock *lock)
> +{
> + /*
> + * This barrier was added to simple spinlocks by commit 51d7d5205d338,
> + * but it should now be possible to remove it, asm arm64 has done with
> + * commit c6f5d02b6a0f.
> + */
> + smp_mb();
> + return atomic_read(&lock->val);
> +}
> +#define queued_spin_is_locked queued_spin_is_locked
> +
> +#include <asm-generic/qspinlock.h>
> +
> +#endif /* _ASM_POWERPC_QSPINLOCK_H */
> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
> index 21357fe05fe0..434615f1d761 100644
> --- a/arch/powerpc/include/asm/spinlock.h
> +++ b/arch/powerpc/include/asm/spinlock.h
> @@ -3,7 +3,12 @@
> #define __ASM_SPINLOCK_H
> #ifdef __KERNEL__
>
> +#ifdef CONFIG_PPC_QUEUED_SPINLOCKS
> +#include <asm/qspinlock.h>
> +#include <asm/qrwlock.h>
> +#else
> #include <asm/simple_spinlock.h>
> +#endif
>
> #endif /* __KERNEL__ */
> #endif /* __ASM_SPINLOCK_H */
> diff --git a/arch/powerpc/include/asm/spinlock_types.h b/arch/powerpc/include/asm/spinlock_types.h
> index 3906f52dae65..c5d742f18021 100644
> --- a/arch/powerpc/include/asm/spinlock_types.h
> +++ b/arch/powerpc/include/asm/spinlock_types.h
> @@ -6,6 +6,11 @@
> # error "please don't include this file directly"
> #endif
>
> +#ifdef CONFIG_PPC_QUEUED_SPINLOCKS
> +#include <asm-generic/qspinlock_types.h>
> +#include <asm-generic/qrwlock_types.h>
> +#else
> #include <asm/simple_spinlock_types.h>
> +#endif
>
> #endif
> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
> index 5e994cda8e40..d66a645503eb 100644
> --- a/arch/powerpc/lib/Makefile
> +++ b/arch/powerpc/lib/Makefile
> @@ -41,7 +41,10 @@ obj-$(CONFIG_PPC_BOOK3S_64) += copyuser_power7.o copypage_power7.o \
> obj64-y += copypage_64.o copyuser_64.o mem_64.o hweight_64.o \
> memcpy_64.o memcpy_mcsafe_64.o
>
> +ifndef CONFIG_PPC_QUEUED_SPINLOCKS
> obj64-$(CONFIG_SMP) += locks.o
> +endif
> +
> obj64-$(CONFIG_ALTIVEC) += vmx-helper.o
> obj64-$(CONFIG_KPROBES_SANITY_TEST) += test_emulate_step.o \
> test_emulate_step_exec_instr.o
> diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
> index fde943d180e0..fb0a814d4395 100644
> --- a/include/asm-generic/qspinlock.h
> +++ b/include/asm-generic/qspinlock.h
> @@ -12,6 +12,7 @@
>
> #include <asm-generic/qspinlock_types.h>
>
> +#ifndef queued_spin_is_locked
> /**
> * queued_spin_is_locked - is the spinlock locked?
> * @lock: Pointer to queued spinlock structure
> @@ -25,6 +26,7 @@ static __always_inline int queued_spin_is_locked(struct qspinlock *lock)
> */
> return atomic_read(&lock->val);
> }
> +#endif
>
> /**
> * queued_spin_value_unlocked - is the spinlock structure unlocked?
> --
> 2.23.0
>
^ permalink raw reply
* Re: [PATCHv3 1/2] powerpc/pseries: group lmb operation and memblock's
From: Nathan Lynch @ 2020-07-23 14:41 UTC (permalink / raw)
To: Pingfan Liu; +Cc: kexec, linuxppc-dev, Hari Bathini
In-Reply-To: <1595382730-10565-1-git-send-email-kernelfans@gmail.com>
Pingfan Liu <kernelfans@gmail.com> writes:
> This patch prepares for the incoming patch which swaps the order of KOBJ_
> uevent and dt's updating.
>
> It has no functional effect, just groups lmb operation and memblock's in
> order to insert dt updating operation easily, and makes it easier to
> review.
...
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 5d545b7..1a3ac3b 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -355,7 +355,8 @@ static int dlpar_add_lmb(struct drmem_lmb *);
> static int dlpar_remove_lmb(struct drmem_lmb *lmb)
> {
> unsigned long block_sz;
> - int rc;
> + phys_addr_t base_addr;
> + int rc, nid;
>
> if (!lmb_is_removable(lmb))
> return -EINVAL;
> @@ -364,17 +365,19 @@ static int dlpar_remove_lmb(struct drmem_lmb *lmb)
> if (rc)
> return rc;
>
> + base_addr = lmb->base_addr;
> + nid = lmb->nid;
> block_sz = pseries_memory_block_size();
>
> - __remove_memory(lmb->nid, lmb->base_addr, block_sz);
> -
> - /* Update memory regions for memory remove */
> - memblock_remove(lmb->base_addr, block_sz);
> -
> invalidate_lmb_associativity_index(lmb);
> lmb_clear_nid(lmb);
> lmb->flags &= ~DRCONF_MEM_ASSIGNED;
>
> + __remove_memory(nid, base_addr, block_sz);
> +
> + /* Update memory regions for memory remove */
> + memblock_remove(base_addr, block_sz);
> +
> return 0;
> }
I don't understand; the commit message should not claim this has no
functional effect when it changes the order of operations like
this. Maybe this is an improvement over the current behavior, but it's
not explained why it would be.
^ permalink raw reply
* Re: [v3 12/15] powerpc/perf: Add support for outputting extended regs in perf intr_regs
From: Arnaldo Carvalho de Melo @ 2020-07-23 14:56 UTC (permalink / raw)
To: kajoljain
Cc: ego, mikey, Athira Rajeev, kvm, kvm-ppc, svaidyan, maddy, jolsa,
linuxppc-dev
In-Reply-To: <489dcd01-5570-bfd4-8b46-10cf15c1e3ab@linux.ibm.com>
Em Thu, Jul 23, 2020 at 11:14:16AM +0530, kajoljain escreveu:
>
>
> On 7/21/20 11:32 AM, kajoljain wrote:
> >
> >
> > On 7/17/20 8:08 PM, Athira Rajeev wrote:
> >> From: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> >>
> >> Add support for perf extended register capability in powerpc.
> >> The capability flag PERF_PMU_CAP_EXTENDED_REGS, is used to indicate the
> >> PMU which support extended registers. The generic code define the mask
> >> of extended registers as 0 for non supported architectures.
> >>
> >> Patch adds extended regs support for power9 platform by
> >> exposing MMCR0, MMCR1 and MMCR2 registers.
> >>
> >> REG_RESERVED mask needs update to include extended regs.
> >> `PERF_REG_EXTENDED_MASK`, contains mask value of the supported registers,
> >> is defined at runtime in the kernel based on platform since the supported
> >> registers may differ from one processor version to another and hence the
> >> MASK value.
> >>
> >> with patch
> >> ----------
> >>
> >> available registers: r0 r1 r2 r3 r4 r5 r6 r7 r8 r9 r10 r11
> >> r12 r13 r14 r15 r16 r17 r18 r19 r20 r21 r22 r23 r24 r25 r26
> >> r27 r28 r29 r30 r31 nip msr orig_r3 ctr link xer ccr softe
> >> trap dar dsisr sier mmcra mmcr0 mmcr1 mmcr2
> >>
> >> PERF_RECORD_SAMPLE(IP, 0x1): 4784/4784: 0 period: 1 addr: 0
> >> ... intr regs: mask 0xffffffffffff ABI 64-bit
> >> .... r0 0xc00000000012b77c
> >> .... r1 0xc000003fe5e03930
> >> .... r2 0xc000000001b0e000
> >> .... r3 0xc000003fdcddf800
> >> .... r4 0xc000003fc7880000
> >> .... r5 0x9c422724be
> >> .... r6 0xc000003fe5e03908
> >> .... r7 0xffffff63bddc8706
> >> .... r8 0x9e4
> >> .... r9 0x0
> >> .... r10 0x1
> >> .... r11 0x0
> >> .... r12 0xc0000000001299c0
> >> .... r13 0xc000003ffffc4800
> >> .... r14 0x0
> >> .... r15 0x7fffdd8b8b00
> >> .... r16 0x0
> >> .... r17 0x7fffdd8be6b8
> >> .... r18 0x7e7076607730
> >> .... r19 0x2f
> >> .... r20 0xc00000001fc26c68
> >> .... r21 0xc0002041e4227e00
> >> .... r22 0xc00000002018fb60
> >> .... r23 0x1
> >> .... r24 0xc000003ffec4d900
> >> .... r25 0x80000000
> >> .... r26 0x0
> >> .... r27 0x1
> >> .... r28 0x1
> >> .... r29 0xc000000001be1260
> >> .... r30 0x6008010
> >> .... r31 0xc000003ffebb7218
> >> .... nip 0xc00000000012b910
> >> .... msr 0x9000000000009033
> >> .... orig_r3 0xc00000000012b86c
> >> .... ctr 0xc0000000001299c0
> >> .... link 0xc00000000012b77c
> >> .... xer 0x0
> >> .... ccr 0x28002222
> >> .... softe 0x1
> >> .... trap 0xf00
> >> .... dar 0x0
> >> .... dsisr 0x80000000000
> >> .... sier 0x0
> >> .... mmcra 0x80000000000
> >> .... mmcr0 0x82008090
> >> .... mmcr1 0x1e000000
> >> .... mmcr2 0x0
> >> ... thread: perf:4784
> >>
> >> Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> >> [Defined PERF_REG_EXTENDED_MASK at run time to add support for different platforms ]
> >> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> >> Reviewed-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> >> ---
> >
> > Patch looks good to me.
> >
> > Reviewed-by: Kajol Jain <kjain@linux.ibm.com>
>
> Hi Arnaldo and Jiri,
> Please let me know if you have any comments on these patches. Can you pull/ack these
> patches if they seems fine to you.
Can you please clarify something here, I think I saw a kernel build bot
complaint followed by a fix, in these cases I think, for reviewer's
sake, that this would entail a v4 patchkit? One that has no such build
issues?
Or have I got something wrong?
- Arnaldo
^ permalink raw reply
* Re: [PATCH 1/2] lockdep: improve current->(hard|soft)irqs_enabled synchronisation with actual irq state
From: Peter Zijlstra @ 2020-07-23 14:59 UTC (permalink / raw)
To: Nicholas Piggin
Cc: linux-arch, Alexey Kardashevskiy, Will Deacon, linux-kernel,
Ingo Molnar, linuxppc-dev
In-Reply-To: <1595506730.3mvrxktem5.astroid@bobo.none>
On Thu, Jul 23, 2020 at 11:11:03PM +1000, Nicholas Piggin wrote:
> Excerpts from Peter Zijlstra's message of July 23, 2020 9:40 pm:
> > On Thu, Jul 23, 2020 at 08:56:14PM +1000, Nicholas Piggin wrote:
> >
> >> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> >> index 3a0db7b0b46e..35060be09073 100644
> >> --- a/arch/powerpc/include/asm/hw_irq.h
> >> +++ b/arch/powerpc/include/asm/hw_irq.h
> >> @@ -200,17 +200,14 @@ static inline bool arch_irqs_disabled(void)
> >> #define powerpc_local_irq_pmu_save(flags) \
> >> do { \
> >> raw_local_irq_pmu_save(flags); \
> >> - trace_hardirqs_off(); \
> >> + if (!raw_irqs_disabled_flags(flags)) \
> >> + trace_hardirqs_off(); \
> >> } while(0)
> >> #define powerpc_local_irq_pmu_restore(flags) \
> >> do { \
> >> - if (raw_irqs_disabled_flags(flags)) { \
> >> - raw_local_irq_pmu_restore(flags); \
> >> - trace_hardirqs_off(); \
> >> - } else { \
> >> + if (!raw_irqs_disabled_flags(flags)) \
> >> trace_hardirqs_on(); \
> >> - raw_local_irq_pmu_restore(flags); \
> >> - } \
> >> + raw_local_irq_pmu_restore(flags); \
> >> } while(0)
> >
> > You shouldn't be calling lockdep from NMI context!
>
> After this patch it doesn't.
You sure, trace_hardirqs_{on,off}() calls into lockdep. (FWIW they're
also broken vs entry ordering, but that's another story).
> trace_hardirqs_on/off implementation appears to expect to be called in NMI
> context though, for some reason.
Hurpm, not sure.. I'll have to go grep arch code now :/ The generic NMI
code didn't touch that stuff.
Argh, yes, there might be broken there... damn! I'll go frob around.
^ permalink raw reply
* Re: [PATCH v3 0/6] powerpc: queued spinlocks and rwlocks
From: Nicholas Piggin @ 2020-07-23 16:12 UTC (permalink / raw)
To: Waiman Long, Peter Zijlstra
Cc: linux-arch, Will Deacon, Boqun Feng, linux-kernel, kvm-ppc,
virtualization, Ingo Molnar, linuxppc-dev
In-Reply-To: <af825bce-ecf3-66e4-ad63-a844dbd2e775@redhat.com>
Excerpts from Waiman Long's message of July 24, 2020 12:29 am:
> On 7/23/20 9:30 AM, Nicholas Piggin wrote:
>>> I would prefer to extract out the pending bit handling code out into a
>>> separate helper function which can be overridden by the arch code
>>> instead of breaking the slowpath into 2 pieces.
>> You mean have the arch provide a queued_spin_lock_slowpath_pending
>> function that the slow path calls?
>>
>> I would actually prefer the pending handling can be made inline in
>> the queued_spin_lock function, especially with out-of-line locks it
>> makes sense to put it there.
>>
>> We could ifdef out queued_spin_lock_slowpath_queue if it's not used,
>> then __queued_spin_lock_slowpath_queue would be inlined into the
>> caller so there would be no split?
>
> The pending code is an optimization for lightly contended locks. That is
> why I think it is appropriate to extract it into a helper function and
> mark it as such.
>
> You can certainly put the code in the arch's spin_lock code, you just
> has to override the generic pending code by a null function.
I see what you mean. I guess that would work fine.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 1/2] lockdep: improve current->(hard|soft)irqs_enabled synchronisation with actual irq state
From: Nicholas Piggin @ 2020-07-23 16:20 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-arch, Alexey Kardashevskiy, Will Deacon, linux-kernel,
Ingo Molnar, linuxppc-dev
In-Reply-To: <20200723145904.GU5523@worktop.programming.kicks-ass.net>
Excerpts from Peter Zijlstra's message of July 24, 2020 12:59 am:
> On Thu, Jul 23, 2020 at 11:11:03PM +1000, Nicholas Piggin wrote:
>> Excerpts from Peter Zijlstra's message of July 23, 2020 9:40 pm:
>> > On Thu, Jul 23, 2020 at 08:56:14PM +1000, Nicholas Piggin wrote:
>> >
>> >> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
>> >> index 3a0db7b0b46e..35060be09073 100644
>> >> --- a/arch/powerpc/include/asm/hw_irq.h
>> >> +++ b/arch/powerpc/include/asm/hw_irq.h
>> >> @@ -200,17 +200,14 @@ static inline bool arch_irqs_disabled(void)
>> >> #define powerpc_local_irq_pmu_save(flags) \
>> >> do { \
>> >> raw_local_irq_pmu_save(flags); \
>> >> - trace_hardirqs_off(); \
>> >> + if (!raw_irqs_disabled_flags(flags)) \
>> >> + trace_hardirqs_off(); \
>> >> } while(0)
>> >> #define powerpc_local_irq_pmu_restore(flags) \
>> >> do { \
>> >> - if (raw_irqs_disabled_flags(flags)) { \
>> >> - raw_local_irq_pmu_restore(flags); \
>> >> - trace_hardirqs_off(); \
>> >> - } else { \
>> >> + if (!raw_irqs_disabled_flags(flags)) \
>> >> trace_hardirqs_on(); \
>> >> - raw_local_irq_pmu_restore(flags); \
>> >> - } \
>> >> + raw_local_irq_pmu_restore(flags); \
>> >> } while(0)
>> >
>> > You shouldn't be calling lockdep from NMI context!
>>
>> After this patch it doesn't.
>
> You sure, trace_hardirqs_{on,off}() calls into lockdep.
At least for irq enable/disable functions yes. NMI runs with
interrupts disabled so these will never call trace_hardirqs_on/off
after this patch.
> (FWIW they're
> also broken vs entry ordering, but that's another story).
>
>> trace_hardirqs_on/off implementation appears to expect to be called in NMI
>> context though, for some reason.
>
> Hurpm, not sure.. I'll have to go grep arch code now :/ The generic NMI
> code didn't touch that stuff.
>
> Argh, yes, there might be broken there... damn! I'll go frob around.
>
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 2/2] powerpc/64s: system call support for scv/rfscv instructions
From: Christophe Leroy @ 2020-07-23 16:48 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linux-api, musl, linuxppc-dev, Nicholas Piggin, libc-dev
In-Reply-To: <871rl2ralk.fsf@mpe.ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> a écrit :
> Nicholas Piggin <npiggin@gmail.com> writes:
>> diff --git a/arch/powerpc/include/asm/ppc-opcode.h
>> b/arch/powerpc/include/asm/ppc-opcode.h
>> index 2a39c716c343..b2bdc4de1292 100644
>> --- a/arch/powerpc/include/asm/ppc-opcode.h
>> +++ b/arch/powerpc/include/asm/ppc-opcode.h
>> @@ -257,6 +257,7 @@
>> #define PPC_INST_MFVSRD 0x7c000066
>> #define PPC_INST_MTVSRD 0x7c000166
>> #define PPC_INST_SC 0x44000002
>> +#define PPC_INST_SCV 0x44000001
> ...
>> @@ -411,6 +412,7 @@
> ...
>> +#define __PPC_LEV(l) (((l) & 0x7f) << 5)
>
> These conflicted and didn't seem to be used so I dropped them.
>
>> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
>> index 5abe98216dc2..161bfccbc309 100644
>> --- a/arch/powerpc/lib/sstep.c
>> +++ b/arch/powerpc/lib/sstep.c
>> @@ -3378,6 +3382,16 @@ int emulate_step(struct pt_regs *regs,
>> struct ppc_inst instr)
>> regs->msr = MSR_KERNEL;
>> return 1;
>>
>> + case SYSCALL_VECTORED_0: /* scv 0 */
>> + regs->gpr[9] = regs->gpr[13];
>> + regs->gpr[10] = MSR_KERNEL;
>> + regs->gpr[11] = regs->nip + 4;
>> + regs->gpr[12] = regs->msr & MSR_MASK;
>> + regs->gpr[13] = (unsigned long) get_paca();
>> + regs->nip = (unsigned long) &system_call_vectored_emulate;
>> + regs->msr = MSR_KERNEL;
>> + return 1;
>> +
>
> This broke the ppc64e build:
>
> ld: arch/powerpc/lib/sstep.o:(.toc+0x0): undefined reference to
> `system_call_vectored_emulate'
> make[1]: *** [/home/michael/linux/Makefile:1139: vmlinux] Error 1
>
> I wrapped it in #ifdef CONFIG_PPC64_BOOK3S.
You mean CONFIG_PPC_BOOK3S_64 ?
Christophe
>
> cheers
^ permalink raw reply
* Re: [PATCH v3 5/6] powerpc/pseries: implement paravirt qspinlocks for SPLPAR
From: Waiman Long @ 2020-07-23 18:32 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-arch, Boqun Feng, virtualization, linuxppc-dev,
Nicholas Piggin, linux-kernel, Ingo Molnar, kvm-ppc, Will Deacon
In-Reply-To: <20200723140011.GR5523@worktop.programming.kicks-ass.net>
On 7/23/20 10:00 AM, Peter Zijlstra wrote:
> On Thu, Jul 09, 2020 at 12:06:13PM -0400, Waiman Long wrote:
>> We don't really need to do a pv_spinlocks_init() if pv_kick() isn't
>> supported.
> Waiman, if you cannot explain how not having kick is a sane thing, what
> are you saying here?
>
The current PPC paravirt spinlock code doesn't do any cpu kick. It does
an equivalence of pv_wait by yielding the cpu to the lock holder only.
The pv_spinlocks_init() is for setting up the hash table for doing
pv_kick. If we don't need to do pv_kick, we don't need the hash table.
I am not saying that pv_kick is not needed for the PPC environment. I
was just trying to adapt the pvqspinlock code to such an environment
first. Further investigation on how to implement some kind of pv_kick
will be something that we may want to do as a follow on.
BTW, do you have any comment on my v2 lock holder cpu info qspinlock
patch? I will have to update the patch to fix the reported 0-day test
problem, but I want to collect other feedback before sending out v3.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v2 0/3] powerpc/pseries: IPI doorbell improvements
From: Cédric Le Goater @ 2020-07-23 17:18 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
Cc: Anton Blanchard, kvm-ppc, Paul Mackerras, David Gibson
In-Reply-To: <20200630115034.137050-1-npiggin@gmail.com>
On 6/30/20 1:50 PM, Nicholas Piggin wrote:
> Since v1:
> - Fixed SMP compile error.
> - Fixed EPAPR / KVM_GUEST breakage.
> - Expanded patch 3 changelog a bit.
>
> Thanks,
> Nick
I gave the patchset a try on KVM guests (P9 and P8) and PowerVM LPARs (P9).
Looks good.
Tested-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
>
> Nicholas Piggin (3):
> powerpc: inline doorbell sending functions
> powerpc/pseries: Use doorbells even if XIVE is available
> powerpc/pseries: Add KVM guest doorbell restrictions
>
> arch/powerpc/include/asm/dbell.h | 63 ++++++++++++++++++++++++++--
> arch/powerpc/include/asm/firmware.h | 6 +++
> arch/powerpc/include/asm/kvm_para.h | 26 ++----------
> arch/powerpc/kernel/Makefile | 5 +--
> arch/powerpc/kernel/dbell.c | 55 ------------------------
> arch/powerpc/kernel/firmware.c | 19 +++++++++
> arch/powerpc/platforms/pseries/smp.c | 62 +++++++++++++++++++--------
> 7 files changed, 134 insertions(+), 102 deletions(-)
>
^ permalink raw reply
* Re: [PATCH v3 5/6] powerpc/pseries: implement paravirt qspinlocks for SPLPAR
From: peterz @ 2020-07-23 18:47 UTC (permalink / raw)
To: Waiman Long
Cc: linux-arch, Boqun Feng, virtualization, linuxppc-dev,
Nicholas Piggin, linux-kernel, Ingo Molnar, kvm-ppc, Will Deacon
In-Reply-To: <845de183-56f5-2958-3159-faa131d46401@redhat.com>
On Thu, Jul 23, 2020 at 02:32:36PM -0400, Waiman Long wrote:
> BTW, do you have any comment on my v2 lock holder cpu info qspinlock patch?
> I will have to update the patch to fix the reported 0-day test problem, but
> I want to collect other feedback before sending out v3.
I want to say I hate it all, it adds instructions to a path we spend an
aweful lot of time optimizing without really getting anything back for
it.
Will, how do you feel about it?
^ permalink raw reply
* Re: [PATCH v3 5/6] powerpc/pseries: implement paravirt qspinlocks for SPLPAR
From: Waiman Long @ 2020-07-23 19:04 UTC (permalink / raw)
To: peterz
Cc: linux-arch, Boqun Feng, virtualization, linuxppc-dev,
Nicholas Piggin, linux-kernel, Ingo Molnar, kvm-ppc, Will Deacon
In-Reply-To: <20200723184759.GS119549@hirez.programming.kicks-ass.net>
On 7/23/20 2:47 PM, peterz@infradead.org wrote:
> On Thu, Jul 23, 2020 at 02:32:36PM -0400, Waiman Long wrote:
>> BTW, do you have any comment on my v2 lock holder cpu info qspinlock patch?
>> I will have to update the patch to fix the reported 0-day test problem, but
>> I want to collect other feedback before sending out v3.
> I want to say I hate it all, it adds instructions to a path we spend an
> aweful lot of time optimizing without really getting anything back for
> it.
It does add some extra instruction that may slow it down slightly, but I
don't agree that it gives nothing back. The cpu lock holder information
can be useful in analyzing crash dumps and in some debugging situation.
I think it can be useful in RHEL for this readon. How about an x86
config option to allow distros to decide if they want to have it
enabled? I will make sure that it will have no performance degradation
if the option is not enabled.
Cheers,
Longman
^ permalink raw reply
* Re: [PATCH v3 5/6] powerpc/pseries: implement paravirt qspinlocks for SPLPAR
From: peterz @ 2020-07-23 19:58 UTC (permalink / raw)
To: Waiman Long
Cc: linux-arch, Boqun Feng, virtualization, linuxppc-dev,
Nicholas Piggin, linux-kernel, Ingo Molnar, kvm-ppc, Will Deacon
In-Reply-To: <6d6279ad-7432-63c1-14c3-18c4cff30bf8@redhat.com>
On Thu, Jul 23, 2020 at 03:04:13PM -0400, Waiman Long wrote:
> On 7/23/20 2:47 PM, peterz@infradead.org wrote:
> > On Thu, Jul 23, 2020 at 02:32:36PM -0400, Waiman Long wrote:
> > > BTW, do you have any comment on my v2 lock holder cpu info qspinlock patch?
> > > I will have to update the patch to fix the reported 0-day test problem, but
> > > I want to collect other feedback before sending out v3.
> > I want to say I hate it all, it adds instructions to a path we spend an
> > aweful lot of time optimizing without really getting anything back for
> > it.
>
> It does add some extra instruction that may slow it down slightly, but I
> don't agree that it gives nothing back. The cpu lock holder information can
> be useful in analyzing crash dumps and in some debugging situation. I think
> it can be useful in RHEL for this readon. How about an x86 config option to
> allow distros to decide if they want to have it enabled? I will make sure
> that it will have no performance degradation if the option is not enabled.
Config knobs suck too; they create a maintenance burden (we get to make
sure all the permutations works/build/etc..) and effectively nobody uses
them, since world+dog uses what distros pick.
Anyway, instead of adding a second per-cpu variable, can you see how
horrible something like this is:
unsigned char adds(unsigned char var, unsigned char val)
{
unsigned short sat = 0xff, tmp = var;
asm ("addb %[val], %b[var];"
"cmovc %[sat], %[var];"
: [var] "+r" (tmp)
: [val] "ir" (val), [sat] "r" (sat)
);
return tmp;
}
Another thing to try is, instead of threading that lockval throughout
the thing, simply:
#define _Q_LOCKED_VAL this_cpu_read_stable(cpu_sat)
or combined with the above
#define _Q_LOCKED_VAL adds(this_cpu_read_stable(cpu_number), 2)
and see if the compiler really makes a mess of things.
^ permalink raw reply
* [PATCH v5 0/7] Migrate non-migrated pages of a SVM.
From: Ram Pai @ 2020-07-23 20:07 UTC (permalink / raw)
To: kvm-ppc, linuxppc-dev
Cc: ldufour, linuxram, cclaudio, bharata, sathnaga, aneesh.kumar,
sukadev, bauerman, david
The time to switch a VM to Secure-VM, increases by the size of the VM.
A 100GB VM takes about 7minutes. This is unacceptable. This linear
increase is caused by a suboptimal behavior by the Ultravisor and the
Hypervisor. The Ultravisor unnecessarily migrates all the GFN of the
VM from normal-memory to secure-memory. It has to just migrate the
necessary and sufficient GFNs.
However when the optimization is incorporated in the Ultravisor, the
Hypervisor starts misbehaving. The Hypervisor has a inbuilt assumption
that the Ultravisor will explicitly request to migrate, each and every
GFN of the VM. If only necessary and sufficient GFNs are requested for
migration, the Hypervisor continues to manage the remaining GFNs as
normal GFNs. This leads to memory corruption; manifested
consistently when the SVM reboots.
The same is true, when a memory slot is hotplugged into a SVM. The
Hypervisor expects the ultravisor to request migration of all GFNs to
secure-GFN. But the hypervisor cannot handle any H_SVM_PAGE_IN
requests from the Ultravisor, done in the context of
UV_REGISTER_MEM_SLOT ucall. This problem manifests as random errors
in the SVM, when a memory-slot is hotplugged.
This patch series automatically migrates the non-migrated pages of a
SVM, and thus solves the problem.
Testing: Passed rigorous testing using various sized SVMs.
Changelog:
v5: . This patch series includes Laurent's fix for memory hotplug/unplug
. drop pages first and then delete the memslot. Otherwise
the memslot does not get cleanly deleted, causing
problems during reboot.
. recreatable through the following set of commands
. device_add pc-dimm,id=dimm1,memdev=mem1
. device_del dimm1
. device_add pc-dimm,id=dimm1,memdev=mem1
Further incorporates comments from Bharata:
. fix for off-by-one while disabling migration.
. code-reorganized to maximize sharing in init_start path
and in memory-hotplug path
. locking adjustments in mass-page migration during H_SVM_INIT_DONE.
. improved recovery on error paths.
. additional comments in the code for better understanding.
. removed the retry-on-migration-failure code.
. re-added the initial patch that adjust some prototype to overcome
a git problem, where it messes up the code context. Had
accidently dropped the patch in the last version.
v4: . Incorported Bharata's comments:
- Optimization -- replace write mmap semaphore with read mmap semphore.
- disable page-merge during memory hotplug.
- rearranged the patches. consolidated the page-migration-retry logic
in a single patch.
v3: . Optimized the page-migration retry-logic.
. Relax and relinquish the cpu regularly while bulk migrating
the non-migrated pages. This issue was causing soft-lockups.
Fixed it.
. Added a new patch, to retry page-migration a couple of times
before returning H_BUSY in H_SVM_PAGE_IN. This issue was
seen a few times in a 24hour continuous reboot test of the SVMs.
v2: . fixed a bug observed by Laurent. The state of the GFN's associated
with Secure-VMs were not reset during memslot flush.
. Re-organized the code, for easier review.
. Better description of the patch series.
v1: . fixed a bug observed by Bharata. Pages that where paged-in and later
paged-out must also be skipped from migration during H_SVM_INIT_DONE.
Laurent Dufour (3):
KVM: PPC: Book3S HV: migrate hot plugged memory
KVM: PPC: Book3S HV: move kvmppc_svm_page_out up
KVM: PPC: Book3S HV: rework secure mem slot dropping
Ram Pai (4):
KVM: PPC: Book3S HV: Fix function definition in book3s_hv_uvmem.c
KVM: PPC: Book3S HV: Disable page merging in H_SVM_INIT_START
KVM: PPC: Book3S HV: track the state GFNs associated with secure VMs
KVM: PPC: Book3S HV: in H_SVM_INIT_DONE, migrate remaining normal-GFNs
to secure-GFNs.
Documentation/powerpc/ultravisor.rst | 3 +
arch/powerpc/include/asm/kvm_book3s_uvmem.h | 16 +
arch/powerpc/kvm/book3s_hv.c | 10 +-
arch/powerpc/kvm/book3s_hv_uvmem.c | 690 +++++++++++++++++++++-------
4 files changed, 548 insertions(+), 171 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH v5 1/7] KVM: PPC: Book3S HV: Fix function definition in book3s_hv_uvmem.c
From: Ram Pai @ 2020-07-23 20:07 UTC (permalink / raw)
To: kvm-ppc, linuxppc-dev
Cc: ldufour, linuxram, cclaudio, bharata, sathnaga, aneesh.kumar,
sukadev, bauerman, david
In-Reply-To: <1595534844-16188-1-git-send-email-linuxram@us.ibm.com>
Without this fix, git is confused. It generates wrong
function context for code changes in subsequent patches.
Weird, but true.
Cc: Paul Mackerras <paulus@ozlabs.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Bharata B Rao <bharata@linux.ibm.com>
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Cc: Laurent Dufour <ldufour@linux.ibm.com>
Cc: Thiago Jung Bauermann <bauerman@linux.ibm.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Claudio Carvalho <cclaudio@linux.ibm.com>
Cc: kvm-ppc@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
arch/powerpc/kvm/book3s_hv_uvmem.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
index 09d8119..e6f76bc 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -382,8 +382,7 @@ static struct page *kvmppc_uvmem_get_page(unsigned long gpa, struct kvm *kvm)
* Alloc a PFN from private device memory pool and copy page from normal
* memory to secure memory using UV_PAGE_IN uvcall.
*/
-static int
-kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
+static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
unsigned long end, unsigned long gpa, struct kvm *kvm,
unsigned long page_shift, bool *downgrade)
{
@@ -450,8 +449,8 @@ static struct page *kvmppc_uvmem_get_page(unsigned long gpa, struct kvm *kvm)
* In the former case, uses dev_pagemap_ops.migrate_to_ram handler
* to unmap the device page from QEMU's page tables.
*/
-static unsigned long
-kvmppc_share_page(struct kvm *kvm, unsigned long gpa, unsigned long page_shift)
+static unsigned long kvmppc_share_page(struct kvm *kvm, unsigned long gpa,
+ unsigned long page_shift)
{
int ret = H_PARAMETER;
@@ -500,9 +499,9 @@ static struct page *kvmppc_uvmem_get_page(unsigned long gpa, struct kvm *kvm)
* H_PAGE_IN_SHARED flag makes the page shared which means that the same
* memory in is visible from both UV and HV.
*/
-unsigned long
-kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gpa,
- unsigned long flags, unsigned long page_shift)
+unsigned long kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gpa,
+ unsigned long flags,
+ unsigned long page_shift)
{
bool downgrade = false;
unsigned long start, end;
@@ -559,10 +558,10 @@ static struct page *kvmppc_uvmem_get_page(unsigned long gpa, struct kvm *kvm)
* Provision a new page on HV side and copy over the contents
* from secure memory using UV_PAGE_OUT uvcall.
*/
-static int
-kvmppc_svm_page_out(struct vm_area_struct *vma, unsigned long start,
- unsigned long end, unsigned long page_shift,
- struct kvm *kvm, unsigned long gpa)
+static int kvmppc_svm_page_out(struct vm_area_struct *vma,
+ unsigned long start,
+ unsigned long end, unsigned long page_shift,
+ struct kvm *kvm, unsigned long gpa)
{
unsigned long src_pfn, dst_pfn = 0;
struct migrate_vma mig;
--
1.8.3.1
^ permalink raw reply related
* [PATCH v5 2/7] KVM: PPC: Book3S HV: Disable page merging in H_SVM_INIT_START
From: Ram Pai @ 2020-07-23 20:07 UTC (permalink / raw)
To: kvm-ppc, linuxppc-dev
Cc: ldufour, linuxram, cclaudio, bharata, sathnaga, aneesh.kumar,
sukadev, bauerman, david
In-Reply-To: <1595534844-16188-1-git-send-email-linuxram@us.ibm.com>
Page-merging of pages in memory-slots associated with a Secure VM,
is disabled in H_SVM_PAGE_IN handler.
This operation should have been done the much earlier; the moment the VM
is initiated for secure-transition. Delaying this operation, increases
the probability for those pages to acquire new references , making it
impossible to migrate those pages in H_SVM_PAGE_IN handler.
Disable page-migration in H_SVM_INIT_START handling.
Reviewed-by: Bharata B Rao <bharata@linux.ibm.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
---
Documentation/powerpc/ultravisor.rst | 1 +
arch/powerpc/kvm/book3s_hv_uvmem.c | 123 +++++++++++++++++++++++++----------
2 files changed, 89 insertions(+), 35 deletions(-)
diff --git a/Documentation/powerpc/ultravisor.rst b/Documentation/powerpc/ultravisor.rst
index df136c8..a1c8c37 100644
--- a/Documentation/powerpc/ultravisor.rst
+++ b/Documentation/powerpc/ultravisor.rst
@@ -895,6 +895,7 @@ Return values
One of the following values:
* H_SUCCESS on success.
+ * H_STATE if the VM is not in a position to switch to secure.
Description
~~~~~~~~~~~
diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
index e6f76bc..533b608 100644
--- a/arch/powerpc/kvm/book3s_hv_uvmem.c
+++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
@@ -211,10 +211,79 @@ static bool kvmppc_gfn_is_uvmem_pfn(unsigned long gfn, struct kvm *kvm,
return false;
}
+static int kvmppc_memslot_page_merge(struct kvm *kvm,
+ const struct kvm_memory_slot *memslot, bool merge)
+{
+ unsigned long gfn = memslot->base_gfn;
+ unsigned long end, start = gfn_to_hva(kvm, gfn);
+ int ret = 0;
+ struct vm_area_struct *vma;
+ int merge_flag = (merge) ? MADV_MERGEABLE : MADV_UNMERGEABLE;
+
+ if (kvm_is_error_hva(start))
+ return H_STATE;
+
+ end = start + (memslot->npages << PAGE_SHIFT);
+
+ mmap_write_lock(kvm->mm);
+ do {
+ vma = find_vma_intersection(kvm->mm, start, end);
+ if (!vma) {
+ ret = H_STATE;
+ break;
+ }
+ ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
+ merge_flag, &vma->vm_flags);
+ if (ret) {
+ ret = H_STATE;
+ break;
+ }
+ start = vma->vm_end;
+ } while (end > vma->vm_end);
+
+ mmap_write_unlock(kvm->mm);
+ return ret;
+}
+
+static void kvmppc_uvmem_memslot_delete(struct kvm *kvm,
+ const struct kvm_memory_slot *memslot)
+{
+ uv_unregister_mem_slot(kvm->arch.lpid, memslot->id);
+ kvmppc_uvmem_slot_free(kvm, memslot);
+ kvmppc_memslot_page_merge(kvm, memslot, true);
+}
+
+static int kvmppc_uvmem_memslot_create(struct kvm *kvm,
+ const struct kvm_memory_slot *memslot)
+{
+ int ret = H_PARAMETER;
+
+ if (kvmppc_memslot_page_merge(kvm, memslot, false))
+ return ret;
+
+ if (kvmppc_uvmem_slot_init(kvm, memslot))
+ goto out1;
+
+ ret = uv_register_mem_slot(kvm->arch.lpid,
+ memslot->base_gfn << PAGE_SHIFT,
+ memslot->npages * PAGE_SIZE,
+ 0, memslot->id);
+ if (ret < 0) {
+ ret = H_PARAMETER;
+ goto out;
+ }
+ return 0;
+out:
+ kvmppc_uvmem_slot_free(kvm, memslot);
+out1:
+ kvmppc_memslot_page_merge(kvm, memslot, true);
+ return ret;
+}
+
unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
{
struct kvm_memslots *slots;
- struct kvm_memory_slot *memslot;
+ struct kvm_memory_slot *memslot, *m;
int ret = H_SUCCESS;
int srcu_idx;
@@ -232,23 +301,24 @@ unsigned long kvmppc_h_svm_init_start(struct kvm *kvm)
return H_AUTHORITY;
srcu_idx = srcu_read_lock(&kvm->srcu);
+
+ /* register the memslot */
slots = kvm_memslots(kvm);
kvm_for_each_memslot(memslot, slots) {
- if (kvmppc_uvmem_slot_init(kvm, memslot)) {
- ret = H_PARAMETER;
- goto out;
- }
- ret = uv_register_mem_slot(kvm->arch.lpid,
- memslot->base_gfn << PAGE_SHIFT,
- memslot->npages * PAGE_SIZE,
- 0, memslot->id);
- if (ret < 0) {
- kvmppc_uvmem_slot_free(kvm, memslot);
- ret = H_PARAMETER;
- goto out;
+ ret = kvmppc_uvmem_memslot_create(kvm, memslot);
+ if (ret)
+ break;
+ }
+
+ if (ret) {
+ slots = kvm_memslots(kvm);
+ kvm_for_each_memslot(m, slots) {
+ if (m == memslot)
+ break;
+ kvmppc_uvmem_memslot_delete(kvm, memslot);
}
}
-out:
+
srcu_read_unlock(&kvm->srcu, srcu_idx);
return ret;
}
@@ -384,7 +454,7 @@ static struct page *kvmppc_uvmem_get_page(unsigned long gpa, struct kvm *kvm)
*/
static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
unsigned long end, unsigned long gpa, struct kvm *kvm,
- unsigned long page_shift, bool *downgrade)
+ unsigned long page_shift)
{
unsigned long src_pfn, dst_pfn = 0;
struct migrate_vma mig;
@@ -400,18 +470,6 @@ static int kvmppc_svm_page_in(struct vm_area_struct *vma, unsigned long start,
mig.src = &src_pfn;
mig.dst = &dst_pfn;
- /*
- * We come here with mmap_lock write lock held just for
- * ksm_madvise(), otherwise we only need read mmap_lock.
- * Hence downgrade to read lock once ksm_madvise() is done.
- */
- ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
- MADV_UNMERGEABLE, &vma->vm_flags);
- mmap_write_downgrade(kvm->mm);
- *downgrade = true;
- if (ret)
- return ret;
-
ret = migrate_vma_setup(&mig);
if (ret)
return ret;
@@ -503,7 +561,6 @@ unsigned long kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gpa,
unsigned long flags,
unsigned long page_shift)
{
- bool downgrade = false;
unsigned long start, end;
struct vm_area_struct *vma;
int srcu_idx;
@@ -524,7 +581,7 @@ unsigned long kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gpa,
ret = H_PARAMETER;
srcu_idx = srcu_read_lock(&kvm->srcu);
- mmap_write_lock(kvm->mm);
+ mmap_read_lock(kvm->mm);
start = gfn_to_hva(kvm, gfn);
if (kvm_is_error_hva(start))
@@ -540,16 +597,12 @@ unsigned long kvmppc_h_svm_page_in(struct kvm *kvm, unsigned long gpa,
if (!vma || vma->vm_start > start || vma->vm_end < end)
goto out_unlock;
- if (!kvmppc_svm_page_in(vma, start, end, gpa, kvm, page_shift,
- &downgrade))
+ if (!kvmppc_svm_page_in(vma, start, end, gpa, kvm, page_shift))
ret = H_SUCCESS;
out_unlock:
mutex_unlock(&kvm->arch.uvmem_lock);
out:
- if (downgrade)
- mmap_read_unlock(kvm->mm);
- else
- mmap_write_unlock(kvm->mm);
+ mmap_read_unlock(kvm->mm);
srcu_read_unlock(&kvm->srcu, srcu_idx);
return ret;
}
--
1.8.3.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox