* Re: [PATCH] powerpc/32: Fix vmap stack - Properly set r1 before activating MMU on syscall too
From: Michael Ellerman @ 2020-12-22 13:11 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Christophe Leroy,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <a3d819d5c348cee9783a311d5d3f3ba9b48fd219.1608531452.git.christophe.leroy@csgroup.eu>
On Mon, 21 Dec 2020 06:18:03 +0000 (UTC), Christophe Leroy wrote:
> We need r1 to be properly set before activating MMU, otherwise any new
> exception taken while saving registers into the stack in syscall
> prologs will use the user stack, which is wrong and will even lockup
> or crash when KUAP is selected.
>
> Do that by switching the meaning of r11 and r1 until we have saved r1
> to the stack: copy r1 into r11 and setup the new stack pointer in r1.
> To avoid complicating and impacting all generic and specific prolog
> code (and more), copy back r1 into r11 once r11 is save onto
> the stack.
>
> [...]
Applied to powerpc/fixes.
[1/1] powerpc/32: Fix vmap stack - Properly set r1 before activating MMU on syscall too
https://git.kernel.org/powerpc/c/d5c243989fb0cb03c74d7340daca3b819f706ee7
cheers
^ permalink raw reply
* Re: GIT kernel with the PowerPC updates 5.11-1 doesn't boot on a FSL P5040 board and in a virtual e5500 QEMU machine
From: Christian Zigotzky @ 2020-12-22 12:15 UTC (permalink / raw)
To: Christophe Leroy, Denis Kirjanov
Cc: Darren Stevens, linuxppc-dev, R.T.Dickinson, mad skateman
In-Reply-To: <8d25f58b-a7bf-4413-b8cc-ed3bd0107263@xenosoft.de>
Hello,
I compiled the latest Git kernel today and unfortunately the boot issue
still exists.
I was able to reduce the patch for reverting the changes. In this way we
know the problematic code now.
vdso-v2.patch:
diff -rupN a/arch/powerpc/kernel/vdso32/vgettimeofday.c
b/arch/powerpc/kernel/vdso32/vgettimeofday.c
--- a/arch/powerpc/kernel/vdso32/vgettimeofday.c 2020-12-19
00:01:16.829846652 +0100
+++ b/arch/powerpc/kernel/vdso32/vgettimeofday.c 2020-12-19
00:00:37.817369691 +0100
@@ -10,12 +10,6 @@ int __c_kernel_clock_gettime(clockid_t c
return __cvdso_clock_gettime32_data(vd, clock, ts);
}
-int __c_kernel_clock_gettime64(clockid_t clock, struct
__kernel_timespec *ts,
- const struct vdso_data *vd)
-{
- return __cvdso_clock_gettime_data(vd, clock, ts);
-}
-
int __c_kernel_gettimeofday(struct __kernel_old_timeval *tv, struct
timezone *tz,
const struct vdso_data *vd)
{
----
With this patch, the uImage boots without any problems on my FSL P5040
board and in a virtual e5500 QEMU machine. Please check the problematic
code.
Thanks,
Christian
On 19 December 2020 at 01:33pm, Christian Zigotzky wrote:
> On 19 December 2020 at 07:49am, Christophe Leroy wrote:
>>
>>
>> Le 18/12/2020 à 23:49, Christian Zigotzky a écrit :
>>> On 18 December 2020 at 10:25pm, Denis Kirjanov wrote:
>>> >
>>> >
>>> > On Friday, December 18, 2020, Christian Zigotzky
>>> <chzigotzky@xenosoft.de> wrote:
>>> >
>>> > Hello,
>>> >
>>> > I compiled the latest Git kernel with the new PowerPC updates
>>> 5.11-1 [1] today. Unfortunately this kernel doesn't boot on my FSL
>>> P5040 board [2] and in a virtual e5500 QEMU machine [3].
>>> >
>>> > I was able to revert the new PowerPC updates 5.11-1 [4] and
>>> after a new compiling, the kernel boots without any problems on my
>>> FSL P5040 board.
>>> >
>>> > Please check the new PowerPC updates 5.11-1.
>>> >
>>> >
>>> > Can you bisect the bad commit?
>>> >
>>> Hello Denis,
>>>
>>> I have bisected [5] and d0e3fc69d00d1f50d22d6b6acfc555ccda80ad1e
>>> (powerpc/vdso: Provide __kernel_clock_gettime64() on vdso32) [6] is
>>> the first bad commit.
>>>
>>> I was able to revert this bad commit and after a new compiling, the
>>> kernel boots without any problems.
>>
>> That's puzzling.
>>
>> Can you describe the symptoms exactly ? What do you mean by "the
>> kernel doesn't boot" ? Where and how does it stops booting ?
> It stops during the disk initialisation.
>>
>> This commit only adds a new VDSO call, for getting y2038 compliant
>> time. At the time I implemented it there was no libc using it yet. Is
>> your libc using it ?
> I tested it with ubuntu MATE 16.04.7 LTS (32-bit userland + 64-bit
> kernel) and with Debian Sid (MintPPC and Fienix 32-bit userland +
> 64-bit kernel) on my FSL P5040 board and in a virtual e5500 QEMU
> machine. How can I figure out if the libc use it?
>>
>> Where can I find all the elements you are using to boot with QEMU ?
>> Especially the file MintPPC32-X5000.img
> Download: http://www.xenosoft.de/MintPPC32-X5000.tar.gz (md5sum:
> b31c1c1ca1fcf5d4cdf110c4bce11654) The password for both 'root' and
> 'mintppc' is 'mintppc'.
>
> QEMU command with KVM on my P5040 board: qemu-system-ppc64 -M ppce500
> -cpu e5500 -enable-kvm -m 1024 -kernel uImage -drive
> format=raw,file=MintPPC32-X5000.img,index=0,if=virtio -netdev
> user,id=mynet0 -device e1000,netdev=mynet0 -append "rw root=/dev/vda"
> -device virtio-vga -device virtio-mouse-pci -device
> virtio-keyboard-pci -device pci-ohci,id=newusb -device
> usb-audio,bus=newusb.0 -smp 4
>
> QEMU command without KVM on macOS Intel: qemu-system-ppc64 -M ppce500
> -cpu e5500 -m 1024 -kernel uImage -drive
> format=raw,file=MintPPC32-X5000.img,index=0,if=virtio -netdev
> user,id=mynet0 -device virtio-net-pci,netdev=mynet0 -append "rw
> root=/dev/vda" -device virtio-vga -usb -device usb-ehci,id=ehci
> -device usb-tablet -device virtio-keyboard-pci -smp 4 -vnc :1
>>
>> Can you also share you kernel config
> See attachment.
>>
>> Thanks
>> Christophe
> Thanks
> Christian
>
^ permalink raw reply
* [PATCH] powerpc/32s: Fix RTAS machine check with VMAP stack
From: Christophe Leroy @ 2020-12-22 7:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
When we have VMAP stack, exception prolog 1 sets r1, not r11.
Fixes: da7bb43ab9da ("powerpc/32: Fix vmap stack - Properly set r1 before activating MMU")
Fixes: d2e006036082 ("powerpc/32: Use SPRN_SPRG_SCRATCH2 in exception prologs")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/head_book3s_32.S | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/powerpc/kernel/head_book3s_32.S b/arch/powerpc/kernel/head_book3s_32.S
index 349bf3f0c3af..fbc48a500846 100644
--- a/arch/powerpc/kernel/head_book3s_32.S
+++ b/arch/powerpc/kernel/head_book3s_32.S
@@ -260,9 +260,16 @@ __secondary_hold_acknowledge:
MachineCheck:
EXCEPTION_PROLOG_0
#ifdef CONFIG_PPC_CHRP
+#ifdef CONFIG_VMAP_STACK
+ mtspr SPRN_SPRG_SCRATCH2,r1
+ mfspr r1, SPRN_SPRG_THREAD
+ lwz r1, RTAS_SP(r1)
+ cmpwi cr1, r1, 0
+#else
mfspr r11, SPRN_SPRG_THREAD
lwz r11, RTAS_SP(r11)
cmpwi cr1, r11, 0
+#endif
bne cr1, 7f
#endif /* CONFIG_PPC_CHRP */
EXCEPTION_PROLOG_1 for_rtas=1
--
2.25.0
^ permalink raw reply related
* [Bug 210749] sysfs: cannot create duplicate filename '/bus/nvmem/devices/module-vpd'
From: bugzilla-daemon @ 2020-12-22 6:51 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-210749-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=210749
--- Comment #2 from Erhard F. (erhard_f@mailbox.org) ---
Created attachment 294285
--> https://bugzilla.kernel.org/attachment.cgi?id=294285&action=edit
dmesg (kernel 5.9.16, Talos II)
Hmm... does not seem to happen on kernel 5.9.x.
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH 3/3] ibmvfc: use correlation token to tag commands
From: Nathan Chancellor @ 2020-12-22 6:24 UTC (permalink / raw)
To: Tyrel Datwyler
Cc: martin.petersen, linux-scsi, linux-kernel, james.bottomley,
clang-built-linux, brking, linuxppc-dev
In-Reply-To: <20201117185031.129939-3-tyreld@linux.ibm.com>
On Tue, Nov 17, 2020 at 12:50:31PM -0600, Tyrel Datwyler wrote:
> The vfcFrame correlation field is 64bit handle that is intended to trace
> I/O operations through both the client stack and VIOS stack when the
> underlying physical FC adapter supports tagging.
>
> Tag vfcFrames with the associated ibmvfc_event pointer handle.
>
> Signed-off-by: Tyrel Datwyler <tyreld@linux.ibm.com>
> ---
> drivers/scsi/ibmvscsi/ibmvfc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.c b/drivers/scsi/ibmvscsi/ibmvfc.c
> index 0cab4b852b48..3922441a117d 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.c
> @@ -1693,6 +1693,8 @@ static int ibmvfc_queuecommand_lck(struct scsi_cmnd *cmnd,
> vfc_cmd->iu.pri_task_attr = IBMVFC_SIMPLE_TASK;
> }
>
> + vfc_cmd->correlation = cpu_to_be64(evt);
> +
> if (likely(!(rc = ibmvfc_map_sg_data(cmnd, evt, vfc_cmd, vhost->dev))))
> return ibmvfc_send_event(evt, vhost, 0);
>
> @@ -2370,6 +2372,8 @@ static int ibmvfc_abort_task_set(struct scsi_device *sdev)
> tmf->iu.tmf_flags = IBMVFC_ABORT_TASK_SET;
> evt->sync_iu = &rsp_iu;
>
> + tmf->correlation = cpu_to_be64(evt);
> +
> init_completion(&evt->comp);
> rsp_rc = ibmvfc_send_event(evt, vhost, default_timeout);
> }
> --
> 2.27.0
>
This patch introduces a clang warning, is this intentional behavior?
$ make -skj"$(nproc)" ARCH=powerpc CROSS_COMPILE=powerpc64le-linux-gnu- LLVM=1 O=out distclean ppc64le_defconfig drivers/scsi/ibmvscsi/ibmvfc.o
Using ../arch/powerpc/configs/ppc64_defconfig as base
Merging ../arch/powerpc/configs/le.config
#
# merged configuration written to .config (needs make)
#
../drivers/scsi/ibmvscsi/ibmvfc.c:1747:25: warning: incompatible pointer to integer conversion passing 'struct ibmvfc_event *' to parameter of type '__u64' (aka 'unsigned long long') [-Wint-conversion]
vfc_cmd->correlation = cpu_to_be64(evt);
^~~~~~~~~~~~~~~~
../include/linux/byteorder/generic.h:92:21: note: expanded from macro 'cpu_to_be64'
#define cpu_to_be64 __cpu_to_be64
^
../include/uapi/linux/byteorder/little_endian.h:37:52: note: expanded from macro '__cpu_to_be64'
#define __cpu_to_be64(x) ((__force __be64)__swab64((x)))
~~~~~~~~~^~~~
../include/uapi/linux/swab.h:133:12: note: expanded from macro '__swab64'
__fswab64(x))
^
../include/uapi/linux/swab.h:66:57: note: passing argument to parameter 'val' here
static inline __attribute_const__ __u64 __fswab64(__u64 val)
^
../drivers/scsi/ibmvscsi/ibmvfc.c:2421:22: warning: incompatible pointer to integer conversion passing 'struct ibmvfc_event *' to parameter of type '__u64' (aka 'unsigned long long') [-Wint-conversion]
tmf->correlation = cpu_to_be64(evt);
^~~~~~~~~~~~~~~~
../include/linux/byteorder/generic.h:92:21: note: expanded from macro 'cpu_to_be64'
#define cpu_to_be64 __cpu_to_be64
^
../include/uapi/linux/byteorder/little_endian.h:37:52: note: expanded from macro '__cpu_to_be64'
#define __cpu_to_be64(x) ((__force __be64)__swab64((x)))
~~~~~~~~~^~~~
../include/uapi/linux/swab.h:133:12: note: expanded from macro '__swab64'
__fswab64(x))
^
../include/uapi/linux/swab.h:66:57: note: passing argument to parameter 'val' here
static inline __attribute_const__ __u64 __fswab64(__u64 val)
^
2 warnings generated.
Cheers,
Nathan
^ permalink raw reply
* Re: [PATCH 3/3] powerpc: rewrite atomics to use ARCH_ATOMIC
From: Nicholas Piggin @ 2020-12-22 3:52 UTC (permalink / raw)
To: Boqun Feng
Cc: Christophe Leroy, linux-arch, Arnd Bergmann, Alexey Kardashevskiy,
linux-kernel, Peter Zijlstra, Will Deacon, linuxppc-dev
In-Reply-To: <20201113153012.GD286534@boqun-archlinux>
Excerpts from Boqun Feng's message of November 14, 2020 1:30 am:
> Hi Nicholas,
>
> On Wed, Nov 11, 2020 at 09:07:23PM +1000, Nicholas Piggin wrote:
>> All the cool kids are doing it.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/atomic.h | 681 ++++++++++-------------------
>> arch/powerpc/include/asm/cmpxchg.h | 62 +--
>> 2 files changed, 248 insertions(+), 495 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/atomic.h b/arch/powerpc/include/asm/atomic.h
>> index 8a55eb8cc97b..899aa2403ba7 100644
>> --- a/arch/powerpc/include/asm/atomic.h
>> +++ b/arch/powerpc/include/asm/atomic.h
>> @@ -11,185 +11,285 @@
>> #include <asm/cmpxchg.h>
>> #include <asm/barrier.h>
>>
>> +#define ARCH_ATOMIC
>> +
>> +#ifndef CONFIG_64BIT
>> +#include <asm-generic/atomic64.h>
>> +#endif
>> +
>> /*
>> * Since *_return_relaxed and {cmp}xchg_relaxed are implemented with
>> * a "bne-" instruction at the end, so an isync is enough as a acquire barrier
>> * on the platform without lwsync.
>> */
>> #define __atomic_acquire_fence() \
>> - __asm__ __volatile__(PPC_ACQUIRE_BARRIER "" : : : "memory")
>> + asm volatile(PPC_ACQUIRE_BARRIER "" : : : "memory")
>>
>> #define __atomic_release_fence() \
>> - __asm__ __volatile__(PPC_RELEASE_BARRIER "" : : : "memory")
>> + asm volatile(PPC_RELEASE_BARRIER "" : : : "memory")
>>
>> -static __inline__ int atomic_read(const atomic_t *v)
>> -{
>> - int t;
>> +#define __atomic_pre_full_fence smp_mb
>>
>> - __asm__ __volatile__("lwz%U1%X1 %0,%1" : "=r"(t) : "m"(v->counter));
>> +#define __atomic_post_full_fence smp_mb
>>
Thanks for the review.
> Do you need to define __atomic_{pre,post}_full_fence for PPC? IIRC, they
> are default smp_mb__{before,atomic}_atomic(), so are smp_mb() defautly
> on PPC.
Okay I didn't realise that's not required.
>> - return t;
>> +#define arch_atomic_read(v) __READ_ONCE((v)->counter)
>> +#define arch_atomic_set(v, i) __WRITE_ONCE(((v)->counter), (i))
>> +#ifdef CONFIG_64BIT
>> +#define ATOMIC64_INIT(i) { (i) }
>> +#define arch_atomic64_read(v) __READ_ONCE((v)->counter)
>> +#define arch_atomic64_set(v, i) __WRITE_ONCE(((v)->counter), (i))
>> +#endif
>> +
> [...]
>>
>> +#define ATOMIC_FETCH_OP_UNLESS_RELAXED(name, type, dtype, width, asm_op) \
>> +static inline int arch_##name##_relaxed(type *v, dtype a, dtype u) \
>
> I don't think we have atomic_fetch_*_unless_relaxed() at atomic APIs,
> ditto for:
>
> atomic_fetch_add_unless_relaxed()
> atomic_inc_not_zero_relaxed()
> atomic_dec_if_positive_relaxed()
>
> , and we don't have the _acquire() and _release() variants for them
> either, and if you don't define their fully-ordered version (e.g.
> atomic_inc_not_zero()), atomic-arch-fallback.h will use read and cmpxchg
> to implement them, and I think not what we want.
Okay. How can those be added? The atoimc generation is pretty
complicated.
> [...]
>>
>> #endif /* __KERNEL__ */
>> #endif /* _ASM_POWERPC_ATOMIC_H_ */
>> diff --git a/arch/powerpc/include/asm/cmpxchg.h b/arch/powerpc/include/asm/cmpxchg.h
>> index cf091c4c22e5..181f7e8b3281 100644
>> --- a/arch/powerpc/include/asm/cmpxchg.h
>> +++ b/arch/powerpc/include/asm/cmpxchg.h
>> @@ -192,7 +192,7 @@ __xchg_relaxed(void *ptr, unsigned long x, unsigned int size)
>> (unsigned long)_x_, sizeof(*(ptr))); \
>> })
>>
>> -#define xchg_relaxed(ptr, x) \
>> +#define arch_xchg_relaxed(ptr, x) \
>> ({ \
>> __typeof__(*(ptr)) _x_ = (x); \
>> (__typeof__(*(ptr))) __xchg_relaxed((ptr), \
>> @@ -448,35 +448,7 @@ __cmpxchg_relaxed(void *ptr, unsigned long old, unsigned long new,
>> return old;
>> }
>>
>> -static __always_inline unsigned long
>> -__cmpxchg_acquire(void *ptr, unsigned long old, unsigned long new,
>> - unsigned int size)
>> -{
>> - switch (size) {
>> - case 1:
>> - return __cmpxchg_u8_acquire(ptr, old, new);
>> - case 2:
>> - return __cmpxchg_u16_acquire(ptr, old, new);
>> - case 4:
>> - return __cmpxchg_u32_acquire(ptr, old, new);
>> -#ifdef CONFIG_PPC64
>> - case 8:
>> - return __cmpxchg_u64_acquire(ptr, old, new);
>> -#endif
>> - }
>> - BUILD_BUG_ON_MSG(1, "Unsupported size for __cmpxchg_acquire");
>> - return old;
>> -}
>> -#define cmpxchg(ptr, o, n) \
>> - ({ \
>> - __typeof__(*(ptr)) _o_ = (o); \
>> - __typeof__(*(ptr)) _n_ = (n); \
>> - (__typeof__(*(ptr))) __cmpxchg((ptr), (unsigned long)_o_, \
>> - (unsigned long)_n_, sizeof(*(ptr))); \
>> - })
>> -
>> -
>
> If you remove {atomic_}_cmpxchg_{,_acquire}() and use the version
> provided by atomic-arch-fallback.h, then a fail cmpxchg or
> cmpxchg_acquire() will still result into a full barrier or a acquire
> barrier after the RMW operation, the barrier is not necessary and
> probably this is not what we want?
Why is that done? That seems like a very subtle difference. Shouldn't
the fallback version skip the barrier?
Thanks,
Nick
^ permalink raw reply
* Re: [RFC PATCH 3/5] powerpc/64s: add CONFIG_PPC_NMMU for nest MMU support
From: Nicholas Piggin @ 2020-12-22 3:37 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <7860a2b3-205f-9b27-42cb-e298264f8253@csgroup.eu>
Excerpts from Christophe Leroy's message of December 20, 2020 9:43 pm:
>
>
> Le 20/12/2020 à 00:48, Nicholas Piggin a écrit :
>> This allows some nest MMU features to be compiled away if coprocessor
>> support is not selected.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/Kconfig | 1 +
>> arch/powerpc/include/asm/book3s/64/mmu.h | 2 ++
>> arch/powerpc/include/asm/book3s/64/tlbflush.h | 2 ++
>> arch/powerpc/include/asm/mmu_context.h | 5 +++--
>> arch/powerpc/platforms/Kconfig | 3 +++
>> arch/powerpc/platforms/powernv/Kconfig | 1 +
>> 6 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index ae7391627054..4376bf4c53b4 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -888,6 +888,7 @@ config PPC_PROT_SAO_LPAR
>>
>> config PPC_COPRO_BASE
>> bool
>> + select PPC_NMMU if PPC_BOOK3S_64
>>
>> config SCHED_SMT
>> bool "SMT (Hyperthreading) scheduler support"
>> diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h b/arch/powerpc/include/asm/book3s/64/mmu.h
>> index 995bbcdd0ef8..07850d68a624 100644
>> --- a/arch/powerpc/include/asm/book3s/64/mmu.h
>> +++ b/arch/powerpc/include/asm/book3s/64/mmu.h
>> @@ -103,8 +103,10 @@ typedef struct {
>> /* Number of bits in the mm_cpumask */
>> atomic_t active_cpus;
>>
>> +#ifdef CONFIG_PPC_NMMU
>> /* Number of users of the external (Nest) MMU */
>> atomic_t copros;
>> +#endif
>>
>> /* Number of user space windows opened in process mm_context */
>> atomic_t vas_windows;
>> diff --git a/arch/powerpc/include/asm/book3s/64/tlbflush.h b/arch/powerpc/include/asm/book3s/64/tlbflush.h
>> index 0a7431e954c6..c70a82851f78 100644
>> --- a/arch/powerpc/include/asm/book3s/64/tlbflush.h
>> +++ b/arch/powerpc/include/asm/book3s/64/tlbflush.h
>> @@ -16,8 +16,10 @@ enum {
>>
>> static inline bool mm_has_nmmu(struct mm_struct *mm)
>> {
>> +#ifdef CONFIG_PPC_NMMU
>> if (unlikely(atomic_read(&mm->context.copros) > 0))
>> return true;
>> +#endif
>> return false;
>> }
>>
>> diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
>> index d5821834dba9..53eac0cc4929 100644
>> --- a/arch/powerpc/include/asm/mmu_context.h
>> +++ b/arch/powerpc/include/asm/mmu_context.h
>> @@ -143,6 +143,7 @@ static inline void dec_mm_active_cpus(struct mm_struct *mm)
>> atomic_dec(&mm->context.active_cpus);
>> }
>>
>> +#ifdef CONFIG_PPC_NMMU
>> static inline void mm_context_add_copro(struct mm_struct *mm)
>> {
>> /*
>> @@ -187,6 +188,7 @@ static inline void mm_context_remove_copro(struct mm_struct *mm)
>> dec_mm_active_cpus(mm);
>> }
>> }
>> +#endif
>>
>> /*
>> * vas_windows counter shows number of open windows in the mm
>> @@ -218,8 +220,7 @@ static inline void mm_context_remove_vas_window(struct mm_struct *mm)
>> #else
>> static inline void inc_mm_active_cpus(struct mm_struct *mm) { }
>> static inline void dec_mm_active_cpus(struct mm_struct *mm) { }
>> -static inline void mm_context_add_copro(struct mm_struct *mm) { }
>> -static inline void mm_context_remove_copro(struct mm_struct *mm) { }
>
> Are you sure you can remove those ?
> If so, I think it belongs to another patch, I can't see how the new PPC_NMMU would allow that by itself.
Yeah possibly a separate patch. Nothing except 64s should compile such
code though, I think?
Thanks,
Nick
^ permalink raw reply
* Re: [RFC PATCH 1/5] powerpc/64s: update_mmu_cache inline the radix test
From: Nicholas Piggin @ 2020-12-22 3:32 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <7190cf34-af03-ca35-d2b5-aa152d300ec0@csgroup.eu>
Excerpts from Christophe Leroy's message of December 20, 2020 9:37 pm:
>
>
> Le 20/12/2020 à 00:48, Nicholas Piggin a écrit :
>> This allows the function to be entirely noped if hash support is
>> compiled out (not possible yet).
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/book3s/pgtable.h | 11 ++++++++++-
>> arch/powerpc/mm/book3s32/mmu.c | 4 ++--
>> arch/powerpc/mm/book3s64/hash_utils.c | 7 ++-----
>> 3 files changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/pgtable.h b/arch/powerpc/include/asm/book3s/pgtable.h
>> index 0e1263455d73..914e9fc7b069 100644
>> --- a/arch/powerpc/include/asm/book3s/pgtable.h
>> +++ b/arch/powerpc/include/asm/book3s/pgtable.h
>> @@ -35,7 +35,16 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>> * corresponding HPTE into the hash table ahead of time, instead of
>> * waiting for the inevitable extra hash-table miss exception.
>> */
>> -void update_mmu_cache(struct vm_area_struct *vma, unsigned long address, pte_t *ptep);
>> +void hash__update_mmu_cache(struct vm_area_struct *vma, unsigned long address, pte_t *ptep);
>> +
>> +static inline void update_mmu_cache(struct vm_area_struct *vma, unsigned long address, pte_t *ptep)
>> +{
>> +#ifdef CONFIG_PPC64
>
> You shouldn't need that ifdef. radix_enabled() is always defined.
True, thanks.
>> + if (radix_enabled())
>> + return;
>> +#endif
>> + hash__update_mmu_cache(vma, address, ptep);
>> +}
>>
>> #endif /* __ASSEMBLY__ */
>> #endif
>> diff --git a/arch/powerpc/mm/book3s32/mmu.c b/arch/powerpc/mm/book3s32/mmu.c
>> index 859e5bd603ac..c5a570ca37ff 100644
>> --- a/arch/powerpc/mm/book3s32/mmu.c
>> +++ b/arch/powerpc/mm/book3s32/mmu.c
>> @@ -325,8 +325,8 @@ static void hash_preload(struct mm_struct *mm, unsigned long ea)
>> *
>> * This must always be called with the pte lock held.
>> */
>> -void update_mmu_cache(struct vm_area_struct *vma, unsigned long address,
>> - pte_t *ptep)
>> +void hash__update_mmu_cache(struct vm_area_struct *vma, unsigned long address,
>> + pte_t *ptep)
>
> Now the limit is 100 chars per line. This should fit on a single line I think.
I never quite know what to do here. The Linux limit is 100 but 80 is
still preferred AFAIK (e.g., don't make lots of lines beyond 80), but
80-100 can be used in some cases when splitting the line doesn't improve
readability on 80 colums.
This does (slightly) improve readability.
Thanks,
Nick
>
>> {
>> if (!mmu_has_feature(MMU_FTR_HPTE_TABLE))
>> return;
>> diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
>> index 73b06adb6eeb..d52a3dee7cf2 100644
>> --- a/arch/powerpc/mm/book3s64/hash_utils.c
>> +++ b/arch/powerpc/mm/book3s64/hash_utils.c
>> @@ -1667,8 +1667,8 @@ static void hash_preload(struct mm_struct *mm, pte_t *ptep, unsigned long ea,
>> *
>> * This must always be called with the pte lock held.
>> */
>> -void update_mmu_cache(struct vm_area_struct *vma, unsigned long address,
>> - pte_t *ptep)
>> +void hash__update_mmu_cache(struct vm_area_struct *vma, unsigned long address,
>> + pte_t *ptep)
>
> Now the limit is 100 chars per line. This should fit on a single line I think.
>
>> {
>> /*
>> * We don't need to worry about _PAGE_PRESENT here because we are
>> @@ -1677,9 +1677,6 @@ void update_mmu_cache(struct vm_area_struct *vma, unsigned long address,
>> unsigned long trap;
>> bool is_exec;
>>
>> - if (radix_enabled())
>> - return;
>> -
>> /* We only want HPTEs for linux PTEs that have _PAGE_ACCESSED set */
>> if (!pte_young(*ptep) || address >= TASK_SIZE)
>> return;
>>
>
^ permalink raw reply
* Re: [PATCH] powerpc: always enable queued spinlocks for 64s, disable for others
From: Nicholas Piggin @ 2020-12-22 3:28 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <769ec5dd-8e74-56cb-a3fe-3b657bb3d14c@csgroup.eu>
Excerpts from Christophe Leroy's message of December 21, 2020 4:04 pm:
>
>
> Le 21/12/2020 à 04:22, Nicholas Piggin a écrit :
>> Queued spinlocks have shown to have good performance and fairness
>> properties even on smaller (2 socket) POWER systems. This selects
>> them automatically for 64s. For other platforms they are de-selected,
>> the standard spinlock is far simpler and smaller code, and single
>> chips with a handful of cores is unlikely to show any improvement.
>>
>> CONFIG_EXPERT still allows this to be changed, e.g., to help debug
>> performance or correctness issues.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/Kconfig | 8 +++-----
>> 1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index ae7391627054..1f9f9e64d638 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -255,6 +255,7 @@ config PPC
>> select PCI_MSI_ARCH_FALLBACKS if PCI_MSI
>> select PCI_SYSCALL if PCI
>> select PPC_DAWR if PPC64
>> + select PPC_QUEUED_SPINLOCKS if !EXPERT && PPC_BOOK3S_64 && SMP
>
> The condition is a bit complicated, and it doesn't set it to Y by default when EXPERT is selected.
Yeah, I don't know how to do that (switch people's oldconfig from =N to
=Y) otherwise (without renaming the option). I think it's enough though,
experts should have said yes already :)
>
>> select RTC_LIB
>> select SPARSE_IRQ
>> select SYSCTL_EXCEPTION_TRACE
>> @@ -506,16 +507,13 @@ config HOTPLUG_CPU
>> config PPC_QUEUED_SPINLOCKS
>> bool "Queued spinlocks"
>> depends on SMP
>> + depends on EXPERT || PPC_BOOK3S_64
>> +
>
> I would do:
>
> config PPC_QUEUED_SPINLOCKS
> bool "Queued spinlocks" if EXPERT
> depends on SMP
> default PPC_BOOK3S_64
That's nicer.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()
From: Segher Boessenkool @ 2020-12-21 17:12 UTC (permalink / raw)
To: David Laight
Cc: ravi.bangoria@linux.ibm.com, mikey@neuling.org,
yanaijie@huawei.com, wangle6@huawei.com,
linuxppc-dev@lists.ozlabs.org, haren@linux.ibm.com,
linux-kernel@vger.kernel.org, paulus@samba.org, npiggin@gmail.com,
aneesh.kumar@linux.ibm.com, Xiaoming Ni
In-Reply-To: <ad814ccf34c14c76b45e50b6e7741c3a@AcuMS.aculab.com>
On Mon, Dec 21, 2020 at 04:42:23PM +0000, David Laight wrote:
> From: Segher Boessenkool
> > Sent: 21 December 2020 16:32
> >
> > On Mon, Dec 21, 2020 at 04:17:21PM +0100, Christophe Leroy wrote:
> > > Le 21/12/2020 à 04:27, Xiaoming Ni a écrit :
> > > >Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
> > > >infrastructure"), the powerpc system is ready to support KASLR.
> > > >To reduces the risk of invalidating address randomization, don't print the
> > > >EIP/LR hex values in dump_stack() and show_regs().
> >
> > > I think your change is not enough to hide EIP address, see below a dump
> > > with you patch, you get "Faulting instruction address: 0xc03a0c14"
> >
> > As far as I can see the patch does nothing to the GPR printout. Often
> > GPRs contain code addresses. As one example, the LR is moved via a GPR
> > (often GPR0, but not always) for storing on the stack.
> >
> > So this needs more work.
>
> If the dump_stack() is from an oops you need the real EIP value
> on order to stand any chance of making headway.
Or at least the function name + offset, yes.
> Otherwise you might just as well just print 'borked - tough luck'.
Yes. ASLR is a house of cards. But that isn't constructive wrt this
patch :-)
Segher
^ permalink raw reply
* RE: [PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()
From: David Laight @ 2020-12-21 16:42 UTC (permalink / raw)
To: 'Segher Boessenkool', Christophe Leroy
Cc: ravi.bangoria@linux.ibm.com, mikey@neuling.org,
yanaijie@huawei.com, wangle6@huawei.com,
linuxppc-dev@lists.ozlabs.org, haren@linux.ibm.com,
linux-kernel@vger.kernel.org, npiggin@gmail.com, paulus@samba.org,
aneesh.kumar@linux.ibm.com, Xiaoming Ni
In-Reply-To: <20201221163130.GZ2672@gate.crashing.org>
From: Segher Boessenkool
> Sent: 21 December 2020 16:32
>
> On Mon, Dec 21, 2020 at 04:17:21PM +0100, Christophe Leroy wrote:
> > Le 21/12/2020 à 04:27, Xiaoming Ni a écrit :
> > >Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
> > >infrastructure"), the powerpc system is ready to support KASLR.
> > >To reduces the risk of invalidating address randomization, don't print the
> > >EIP/LR hex values in dump_stack() and show_regs().
>
> > I think your change is not enough to hide EIP address, see below a dump
> > with you patch, you get "Faulting instruction address: 0xc03a0c14"
>
> As far as I can see the patch does nothing to the GPR printout. Often
> GPRs contain code addresses. As one example, the LR is moved via a GPR
> (often GPR0, but not always) for storing on the stack.
>
> So this needs more work.
If the dump_stack() is from an oops you need the real EIP value
on order to stand any chance of making headway.
Otherwise you might just as well just print 'borked - tough luck'.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()
From: Segher Boessenkool @ 2020-12-21 16:31 UTC (permalink / raw)
To: Christophe Leroy
Cc: Xiaoming Ni, ravi.bangoria, mikey, yanaijie, haren, linux-kernel,
npiggin, wangle6, paulus, aneesh.kumar, linuxppc-dev
In-Reply-To: <2279fc96-1f10-0c3f-64d9-734f18758620@csgroup.eu>
On Mon, Dec 21, 2020 at 04:17:21PM +0100, Christophe Leroy wrote:
> Le 21/12/2020 à 04:27, Xiaoming Ni a écrit :
> >Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
> >infrastructure"), the powerpc system is ready to support KASLR.
> >To reduces the risk of invalidating address randomization, don't print the
> >EIP/LR hex values in dump_stack() and show_regs().
> I think your change is not enough to hide EIP address, see below a dump
> with you patch, you get "Faulting instruction address: 0xc03a0c14"
As far as I can see the patch does nothing to the GPR printout. Often
GPRs contain code addresses. As one example, the LR is moved via a GPR
(often GPR0, but not always) for storing on the stack.
So this needs more work.
Segher
^ permalink raw reply
* Re: [PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()
From: Christophe Leroy @ 2020-12-21 15:17 UTC (permalink / raw)
To: Xiaoming Ni, linux-kernel, benh, mpe, paulus, linuxppc-dev,
yanaijie, npiggin, ravi.bangoria, mikey, aneesh.kumar, haren
Cc: wangle6
In-Reply-To: <20201221032758.12143-1-nixiaoming@huawei.com>
Le 21/12/2020 à 04:27, Xiaoming Ni a écrit :
> Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
> infrastructure"), the powerpc system is ready to support KASLR.
> To reduces the risk of invalidating address randomization, don't print the
> EIP/LR hex values in dump_stack() and show_regs().
I think this change is worth providing more details. Explain how the change improves debugging.
>
> This patch follows x86 and arm64's lead:
> commit a25ffd3a6302a6 ("arm64: traps: Don't print stack or raw
> PC/LR values in backtraces")
> commit bb5e5ce545f203 ("x86/dumpstack: Remove kernel text
> addresses from stack dump")
I think your change is not enough to hide EIP address, see below a dump with you patch, you get
"Faulting instruction address: 0xc03a0c14"
Also, you replace a 8 digits value by a potentially long ling. Should should therefore put NIP and
LR on different lines, and put CTR somewhere else (next to XER ?)
Now, the NIP and LR before the REGS and after the GPRs are exactly the same. No need to duplicate.
root@vgoip:~# busybox echo ACCESS_USERSPACE > /sys/kernel/debug/provoke-crash/DI
RECT
[ 198.698395] lkdtm: Performing direct entry ACCESS_USERSPACE
[ 198.698742] lkdtm: attempting bad read at 77ce8000
[ 198.698844] Kernel attempted to read user page (77ce8000) - exploit attempt? (uid: 0)
[ 198.706489] BUG: Unable to handle kernel data access on read at 0x77ce8000
[ 198.713274] Faulting instruction address: 0xc03a0c14
[ 198.718187] Oops: Kernel access of bad area, sig: 11 [#1]
[ 198.723516] BE PAGE_SIZE=16K PREEMPT CMPC885
[ 198.731272] CPU: 0 PID: 370 Comm: busybox Not tainted 5.10.0-s3k-dev-13158-g8e389861badd #4386
[ 198.739785] NIP: lkdtm_ACCESS_USERSPACE+0xbc/0x110 LR: lkdtm_ACCESS_USERSPACE+0xbc/0x110 CTR:
00000000
[ 198.748994] REGS: cad83d30 TRAP: 0300 Not tainted (5.10.0-s3k-dev-13158-g8e389861badd)
[ 198.757081] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 24002222 XER: 00000000
[ 198.763881] DAR: 77ce8000 DSISR: 88000000
[ 198.763881] GPR00: c03a0c14 cad83df0 c28a8000 00000026 c093e518 00000001 00000027 00000027
[ 198.763881] GPR08: c09a26dc 00000000 00000000 3ffff000 24002228 100d3dd6 100a2ffc 00000000
[ 198.763881] GPR16: 100cd280 100b0000 107e42ec 107e54b5 100d0000 100d0000 00000000 100a2fdc
[ 198.763881] GPR24: ffffffef ffffffff cad83f10 00000011 c078298c c2930000 c084b980 77ce8000
[ 198.802865] NIP lkdtm_ACCESS_USERSPACE+0xbc/0x110
[ 198.807514] LR lkdtm_ACCESS_USERSPACE+0xbc/0x110
[ 198.812077] Call Trace:
[ 198.814485] [cad83df0] lkdtm_ACCESS_USERSPACE+0xbc/0x110 (unreliable)
[ 198.820940] [cad83e20] direct_entry+0xe0/0x164
[ 198.825415] [cad83e50] full_proxy_write+0x78/0xbc
[ 198.830148] [cad83e80] vfs_write+0x12c/0x478
[ 198.834452] [cad83f00] ksys_write+0x78/0x128
[ 198.838754] [cad83f30] ret_from_syscall+0x0/0x34
[ 198.843401] --- interrupt: c01 at 0xfd51d0c
[ 198.847532] NIP: 0xfd51d0c LR: 0x10008404 CTR: 0fcff380
[ 198.852696] REGS: cad83f40 TRAP: 0c01 Not tainted (5.10.0-s3k-dev-13158-g8e389861badd)
[ 198.860784] MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 44002424 XER: 00000000
[ 198.867928]
[ 198.867928] GPR00: 00000004 7fde21f0 77cf34d0 00000001 10640008 00000011 00000000 0fd5b36c
[ 198.867928] GPR08: 00000000 00024000 00000000 00000009 84022222
[ 198.884193] NIP 0xfd51d0c
[ 198.886775] LR 0x10008404
[ 198.889357] --- interrupt: c01
[ 198.892373] Instruction dump:
[ 198.895298] 7d295279 39400000 40820078 80010034 83e1002c 7c0803a6 38210030 4e800020
[ 198.903214] 3c60c085 7fe4fb78 3863c874 4bcc5805 <813f0000> 3c60c085 3d29c0df 3929c0de
[ 198.911316] ---[ end trace ba4047052f99b7bf ]---
[ 198.915865]
Segmentation fault
>
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> ---
> arch/powerpc/kernel/process.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index a66f435dabbf..913cf1ea702e 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1455,8 +1455,8 @@ static void __show_regs(struct pt_regs *regs)
> {
> int i, trap;
>
> - printk("NIP: "REG" LR: "REG" CTR: "REG"\n",
> - regs->nip, regs->link, regs->ctr);
> + printk("NIP: %pS LR: %pS CTR: "REG"\n",
> + (void *)regs->nip, (void *)regs->link, regs->ctr);
> printk("REGS: %px TRAP: %04lx %s (%s)\n",
> regs, regs->trap, print_tainted(), init_utsname()->release);
> printk("MSR: "REG" ", regs->msr);
> @@ -1493,8 +1493,8 @@ static void __show_regs(struct pt_regs *regs)
> * above info out without failing
> */
> if (IS_ENABLED(CONFIG_KALLSYMS)) {
> - printk("NIP ["REG"] %pS\n", regs->nip, (void *)regs->nip);
> - printk("LR ["REG"] %pS\n", regs->link, (void *)regs->link);
> + printk("NIP %pS\n", (void *)regs->nip);
> + printk("LR %pS\n", (void *)regs->link);
> }
> }
>
> @@ -2160,8 +2160,8 @@ void show_stack(struct task_struct *tsk, unsigned long *stack,
> newsp = stack[0];
> ip = stack[STACK_FRAME_LR_SAVE];
> if (!firstframe || ip != lr) {
> - printk("%s["REG"] ["REG"] %pS",
> - loglvl, sp, ip, (void *)ip);
> + printk("%s ["REG"] %pS",
> + loglvl, sp, (void *)ip);
> ret_addr = ftrace_graph_ret_addr(current,
> &ftrace_idx, ip, stack);
> if (ret_addr != ip)
>
^ permalink raw reply
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
From: Greg Kurz @ 2020-12-21 14:37 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, linux-nvdimm, aneesh.kumar, mst, qemu-devel,
kvm-ppc, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev, david
In-Reply-To: <20201221130853.15c8ddfd@bahia.lan>
On Mon, 21 Dec 2020 13:08:53 +0100
Greg Kurz <groug@kaod.org> wrote:
> Hi Shiva,
>
> On Mon, 30 Nov 2020 09:16:39 -0600
> Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
>
> > The patch adds support for async hcalls at the DRC level for the
> > spapr devices. To be used by spapr-scm devices in the patch/es to follow.
> >
> > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> > ---
>
> The overall idea looks good but I think you should consider using
> a thread pool to implement it. See below.
>
Some more comments.
> > hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_drc.h | 25 +++++++
> > 2 files changed, 174 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 77718cde1f..4ecd04f686 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -15,6 +15,7 @@
> > #include "qapi/qmp/qnull.h"
> > #include "cpu.h"
> > #include "qemu/cutils.h"
> > +#include "qemu/guest-random.h"
> > #include "hw/ppc/spapr_drc.h"
> > #include "qom/object.h"
> > #include "migration/vmstate.h"
> > @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> > spapr_drc_release(drc);
> > }
> >
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + *
> > + * All subsequent requests to run/query the status should use the
> > + * unique token returned here.
> > + */
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> > +{
> > + Error *err = NULL;
> > + uint64_t token;
> > + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> > +
> > + state = g_malloc0(sizeof(*state));
> > + state->pending = true;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > +retry:
> > + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> > + error_report_err(err);
> > + g_free(state);
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > + return 0;
> > + }
> > +
> > + if (!token) /* Token should be non-zero */
> > + goto retry;
> > +
> > + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> > + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> > + if (tmp->continue_token == token) {
> > + /* If the token already in use, get a new one */
> > + goto retry;
> > + }
> > + }
> > + }
> > +
> > + state->continue_token = token;
> > + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> > +
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return state->continue_token;
> > +}
> > +
> > +static void *spapr_drc_async_hcall_runner(void *opaque)
> > +{
> > + int response = -1;
It feels wrong since the return value of func() is supposed to be
opaque to this function. And anyway it isn't needed since response
is always set a few lines below.
> > + SpaprDrcDeviceAsyncHCallState *state = opaque;
> > +
> > + /*
> > + * state is freed only after this thread finishes(after pthread_join()),
> > + * don't worry about it becoming NULL.
> > + */
> > +
> > + response = state->func(state->data);
> > +
> > + state->hcall_ret = response;
> > + state->pending = 0;
s/0/false/
> > +
> > + return NULL;
> > +}
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + * token : The continue token to be used for tracking as recived from
> > + * spapr_drc_get_new_async_hcall_token
> > + * @func() : the worker function which needs to be executed asynchronously
> > + * @data : data to be passed to the asynchronous function. Worker is supposed
> > + * to free/cleanup the data that is passed here
>
> It'd be cleaner to pass a completion callback and have free/cleanup handled there.
>
> > + */
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc *func, void *data)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> > + if (state->continue_token == token) {
> > + state->func = func;
> > + state->data = data;
> > + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> > + spapr_drc_async_hcall_runner, state,
> > + QEMU_THREAD_JOINABLE);
>
> qemu_thread_create() exits on failure, it shouldn't be called on
> a guest triggerable path, eg. a buggy guest could call it up to
> the point that pthread_create() returns EAGAIN.
>
> Please use a thread pool (see thread_pool_submit_aio()). This takes care
> of all the thread housekeeping for you in a safe way, and it provides a
> completion callback API. The implementation could then be just about
> having two lists: one for pending requests (fed here) and one for
> completed requests (fed by the completion callback).
>
> > + break;
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_finish_async_hcalls
> > + * Waits for all pending async requests to complete
> > + * thier execution and free the states
> > + */
> > +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state, *next;
> > +
> > + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> > + return;
> > + }
> > +
This is called during machine reset and there won't be contention
in the large majority of cases. If the list is empty QLIST_FOREACH_SAFE()
won't iterate. So I don't think special casing the empty list brings much.
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> > + qemu_thread_join(&state->thread);
>
> With a thread-pool, you'd just need to aio_poll() until the pending list
> is empty and then clear the completed list.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_get_async_hcall_status
> > + * Fetches the status of the hcall worker and returns H_BUSY
> > + * if the worker is still running.
> > + */
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
> > +{
> > + int ret = H_PARAMETER;
> > + SpaprDrcDeviceAsyncHCallState *state, *node;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
> > + if (state->continue_token == token) {
> > + if (state->pending) {
> > + ret = H_BUSY;
> > + break;
> > + } else {
> > + ret = state->hcall_ret;
> > + qemu_thread_join(&state->thread);
>
> Like for qemu_thread_create(), the guest shouldn't be responsible for
> thread housekeeping. Getting the hcall status should just be about
> finding the token in the pending or completed lists.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + break;
> > + }
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return ret;
> > +}
> > +
> > void spapr_drc_reset(SpaprDrc *drc)
> > {
> > SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > @@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> > drc->ccs_offset = -1;
> > drc->ccs_depth = -1;
> > }
> > + spapr_drc_finish_async_hcalls(drc);
> > }
> >
> > static bool spapr_drc_unplug_requested_needed(void *opaque)
> > @@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> > drc->owner = owner;
> > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> > spapr_drc_index(drc));
> > +
>
> Unrelated change.
>
> > object_property_add_child(owner, prop_name, OBJECT(drc));
> > object_unref(OBJECT(drc));
> > qdev_realize(DEVICE(drc), NULL, NULL);
> > @@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> > object_property_add(obj, "fdt", "struct", prop_get_fdt,
> > NULL, NULL, NULL);
> > drc->state = drck->empty_state;
> > +
> > + qemu_mutex_init(&drc->async_hcall_states_lock);
> > + QLIST_INIT(&drc->async_hcall_states);
> > +
>
> Empty line not needed.
>
> > }
> >
> > static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 165b281496..77f6e4386c 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -18,6 +18,7 @@
> > #include "sysemu/runstate.h"
> > #include "hw/qdev-core.h"
> > #include "qapi/error.h"
> > +#include "block/thread-pool.h"
> >
> > #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> > #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> > @@ -168,6 +169,21 @@ typedef enum {
> > SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> > } SpaprDrcState;
> >
> > +typedef struct SpaprDrc SpaprDrc;
> > +
> > +typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
> > +typedef struct SpaprDrcDeviceAsyncHCallState {
> > + uint64_t continue_token;
> > + bool pending;
> > +
> > + int hcall_ret;
> > + SpaprDrcAsyncHcallWorkerFunc *func;
> > + void *data;
> > +
> > + QemuThread thread;
> > +
> > + QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
> > +} SpaprDrcDeviceAsyncHCallState;
> > typedef struct SpaprDrc {
> > /*< private >*/
> > DeviceState parent;
> > @@ -182,6 +198,10 @@ typedef struct SpaprDrc {
> > int ccs_offset;
> > int ccs_depth;
> >
> > + /* async hcall states */
> > + QemuMutex async_hcall_states_lock;
> > + QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
> > +
> > /* device pointer, via link property */
> > DeviceState *dev;
> > bool unplug_requested;
> > @@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
> > /* Returns true if a hot plug/unplug request is pending */
> > bool spapr_drc_transient(SpaprDrc *drc);
> >
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc, void *data);
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
> > +
> > static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
> > {
> > return drc->unplug_requested;
> >
> >
> >
>
>
^ permalink raw reply
* Re: [RFC Qemu PATCH v2 0/2] spapr: nvdimm: Asynchronus flush hcall support
From: Greg Kurz @ 2020-12-21 13:07 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, aneesh.kumar, linux-nvdimm, qemu-devel,
kvm-ppc, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev, david
In-Reply-To: <160674929554.2492771.17651548703390170573.stgit@lep8c.aus.stglabs.ibm.com>
On Mon, 30 Nov 2020 09:16:14 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> The nvdimm devices are expected to ensure write persistent during power
> failure kind of scenarios.
>
> The libpmem has architecture specific instructions like dcbf on power
> to flush the cache data to backend nvdimm device during normal writes.
>
> Qemu - virtual nvdimm devices are memory mapped. The dcbf in the guest
> doesn't traslate to actual flush to the backend file on the host in case
> of file backed vnvdimms. This is addressed by virtio-pmem in case of x86_64
> by making asynchronous flushes.
>
> On PAPR, issue is addressed by adding a new hcall to
> request for an explicit asynchronous flush requests from the guest ndctl
> driver when the backend nvdimm cannot ensure write persistence with dcbf
> alone. So, the approach here is to convey when the asynchronous flush is
> required in a device tree property. The guest makes the hcall when the
> property is found, instead of relying on dcbf.
>
> The first patch adds the necessary asynchronous hcall support infrastructure
> code at the DRC level. Second patch implements the hcall using the
> infrastructure.
>
> Hcall semantics are in review and not final.
>
> A new device property sync-dax is added to the nvdimm device. When the
> sync-dax is off(default), the asynchronous hcalls will be called.
>
> With respect to save from new qemu to restore on old qemu, having the
> sync-dax by default off(when not specified) causes IO errors in guests as
> the async-hcall would not be supported on old qemu. The new hcall
> implementation being supported only on the new pseries machine version,
> the current machine version checks may be sufficient to prevent
> such migration. Please suggest what should be done.
>
First, all requests that are still not completed from the guest POV,
ie. the hcall hasn't returned H_SUCCESS yet, are state that we should
migrate in theory. In this case, I guess we rather want to drain all
pending requests on the source in some pre-save handler.
Then, as explained in another mail, you should enforce stable behavior
for existing machine types with some hw_compat magic.
> The below demonstration shows the map_sync behavior with sync-dax on & off.
> (https://github.com/avocado-framework-tests/avocado-misc-tests/blob/master/memory/ndctl.py.data/map_sync.c)
>
> The pmem0 is from nvdimm with With sync-dax=on, and pmem1 is from nvdimm with syn-dax=off, mounted as
> /dev/pmem0 on /mnt1 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize=32k,noquota)
> /dev/pmem1 on /mnt2 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize=32k,noquota)
>
> [root@atest-guest ~]# ./mapsync /mnt1/newfile ----> When sync-dax=off
> [root@atest-guest ~]# ./mapsync /mnt2/newfile ----> when sync-dax=on
> Failed to mmap with Operation not supported
>
> ---
> v1 - https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06330.html
> Changes from v1
> - Fixed a missed-out unlock
> - using QLIST_FOREACH instead of QLIST_FOREACH_SAFE while generating token
>
> Shivaprasad G Bhat (2):
> spapr: drc: Add support for async hcalls at the drc level
> spapr: nvdimm: Implement async flush hcalls
>
>
> hw/mem/nvdimm.c | 1
> hw/ppc/spapr_drc.c | 146 ++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/spapr_nvdimm.c | 79 ++++++++++++++++++++++++
> include/hw/mem/nvdimm.h | 10 +++
> include/hw/ppc/spapr.h | 3 +
> include/hw/ppc/spapr_drc.h | 25 ++++++++
> 6 files changed, 263 insertions(+), 1 deletion(-)
>
> --
> Signature
>
>
^ permalink raw reply
* Re: [RFC Qemu PATCH v2 2/2] spapr: nvdimm: Implement async flush hcalls
From: Greg Kurz @ 2020-12-21 13:07 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, aneesh.kumar, linux-nvdimm, qemu-devel,
kvm-ppc, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev, david
In-Reply-To: <160674940727.2492771.7855399693883710135.stgit@lep8c.aus.stglabs.ibm.com>
On Mon, 30 Nov 2020 09:17:24 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> When the persistent memory beacked by a file, a cpu cache flush instruction
> is not sufficient to ensure the stores are correctly flushed to the media.
>
> The patch implements the async hcalls for flush operation on demand from the
> guest kernel.
>
> The device option sync-dax is by default off and enables explicit asynchronous
> flush requests from guest. It can be disabled by setting syn-dax=on.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
> hw/mem/nvdimm.c | 1 +
> hw/ppc/spapr_nvdimm.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/mem/nvdimm.h | 10 ++++++
> include/hw/ppc/spapr.h | 3 +-
> 4 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 03c2201b56..37a4db0135 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -220,6 +220,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
>
> static Property nvdimm_properties[] = {
> DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
> + DEFINE_PROP_BOOL(NVDIMM_SYNC_DAX_PROP, NVDIMMDevice, sync_dax, false),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5e..557e36aa98 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -22,6 +22,7 @@
> * THE SOFTWARE.
> */
> #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
> #include "qapi/error.h"
> #include "hw/ppc/spapr_drc.h"
> #include "hw/ppc/spapr_nvdimm.h"
> @@ -155,6 +156,11 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
> "operating-system")));
> _FDT(fdt_setprop(fdt, child_offset, "ibm,cache-flush-required", NULL, 0));
>
> + if (!nvdimm->sync_dax) {
So this is done unconditionally for all machine types. This means that a
guest started on a newer QEMU cannot be migrated to an older QEMU. This
is annoying because people legitimately expect an existing machine type
to be migratable to any QEMU version that supports it.
This means that something like the following should be added in hw_compat_5_2[]
to fix the property for pre-6.0 machine types:
{ "nvdimm", "sync-dax", "on" },
> + _FDT(fdt_setprop(fdt, child_offset, "ibm,async-flush-required",
> + NULL, 0));
> + }
> +
> return child_offset;
> }
>
> @@ -370,6 +376,78 @@ static target_ulong h_scm_bind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
> return H_SUCCESS;
> }
>
> +typedef struct SCMAsyncFlushData {
> + int fd;
> + uint64_t token;
> +} SCMAsyncFlushData;
> +
> +static int flush_worker_cb(void *opaque)
> +{
> + int ret = H_SUCCESS;
> + SCMAsyncFlushData *req_data = opaque;
> +
> + /* flush raw backing image */
> + if (qemu_fdatasync(req_data->fd) < 0) {
> + error_report("papr_scm: Could not sync nvdimm to backend file: %s",
> + strerror(errno));
> + ret = H_HARDWARE;
> + }
> +
> + g_free(req_data);
> +
> + return ret;
> +}
> +
> +static target_ulong h_scm_async_flush(PowerPCCPU *cpu, SpaprMachineState *spapr,
> + target_ulong opcode, target_ulong *args)
> +{
> + int ret;
> + uint32_t drc_index = args[0];
> + uint64_t continue_token = args[1];
> + SpaprDrc *drc = spapr_drc_by_index(drc_index);
> + PCDIMMDevice *dimm;
> + HostMemoryBackend *backend = NULL;
> + SCMAsyncFlushData *req_data = NULL;
> +
> + if (!drc || !drc->dev ||
> + spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PMEM) {
> + return H_PARAMETER;
> + }
> +
> + if (continue_token != 0) {
> + ret = spapr_drc_get_async_hcall_status(drc, continue_token);
> + if (ret == H_BUSY) {
> + args[0] = continue_token;
> + return H_LONG_BUSY_ORDER_1_SEC;
> + }
> +
> + return ret;
> + }
> +
> + dimm = PC_DIMM(drc->dev);
> + backend = MEMORY_BACKEND(dimm->hostmem);
> +
> + req_data = g_malloc0(sizeof(SCMAsyncFlushData));
> + req_data->fd = memory_region_get_fd(&backend->mr);
> +
> + continue_token = spapr_drc_get_new_async_hcall_token(drc);
> + if (!continue_token) {
> + g_free(req_data);
> + return H_P2;
> + }
> + req_data->token = continue_token;
> +
> + spapr_drc_run_async_hcall(drc, continue_token, &flush_worker_cb, req_data);
> +
> + ret = spapr_drc_get_async_hcall_status(drc, continue_token);
> + if (ret == H_BUSY) {
> + args[0] = req_data->token;
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> static target_ulong h_scm_unbind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> @@ -486,6 +564,7 @@ static void spapr_scm_register_types(void)
> spapr_register_hypercall(H_SCM_BIND_MEM, h_scm_bind_mem);
> spapr_register_hypercall(H_SCM_UNBIND_MEM, h_scm_unbind_mem);
> spapr_register_hypercall(H_SCM_UNBIND_ALL, h_scm_unbind_all);
> + spapr_register_hypercall(H_SCM_ASYNC_FLUSH, h_scm_async_flush);
> }
>
> type_init(spapr_scm_register_types)
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index c699842dd0..9e8795766e 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -51,6 +51,7 @@ OBJECT_DECLARE_TYPE(NVDIMMDevice, NVDIMMClass, NVDIMM)
> #define NVDIMM_LABEL_SIZE_PROP "label-size"
> #define NVDIMM_UUID_PROP "uuid"
> #define NVDIMM_UNARMED_PROP "unarmed"
> +#define NVDIMM_SYNC_DAX_PROP "sync-dax"
>
> struct NVDIMMDevice {
> /* private */
> @@ -85,6 +86,15 @@ struct NVDIMMDevice {
> */
> bool unarmed;
>
> + /*
> + * On PPC64,
> + * The 'off' value results in the async-flush-required property set
> + * in the device tree for pseries machines. When 'off', the guest
> + * initiates explicity flush requests to the backend device ensuring
> + * write persistence.
> + */
> + bool sync_dax;
> +
> /*
> * The PPC64 - spapr requires each nvdimm device have a uuid.
> */
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2e89e36cfb..6d7110b7dc 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -535,8 +535,9 @@ struct SpaprMachineState {
> #define H_SCM_BIND_MEM 0x3EC
> #define H_SCM_UNBIND_MEM 0x3F0
> #define H_SCM_UNBIND_ALL 0x3FC
> +#define H_SCM_ASYNC_FLUSH 0x4A0
>
> -#define MAX_HCALL_OPCODE H_SCM_UNBIND_ALL
> +#define MAX_HCALL_OPCODE H_SCM_ASYNC_FLUSH
>
> /* The hcalls above are standardized in PAPR and implemented by pHyp
> * as well.
>
>
>
^ permalink raw reply
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
From: Greg Kurz @ 2020-12-21 12:08 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, aneesh.kumar, linux-nvdimm, qemu-devel,
kvm-ppc, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev, david
In-Reply-To: <160674938210.2492771.1728601884822491679.stgit@lep8c.aus.stglabs.ibm.com>
Hi Shiva,
On Mon, 30 Nov 2020 09:16:39 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> The patch adds support for async hcalls at the DRC level for the
> spapr devices. To be used by spapr-scm devices in the patch/es to follow.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
The overall idea looks good but I think you should consider using
a thread pool to implement it. See below.
> hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr_drc.h | 25 +++++++
> 2 files changed, 174 insertions(+)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 77718cde1f..4ecd04f686 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -15,6 +15,7 @@
> #include "qapi/qmp/qnull.h"
> #include "cpu.h"
> #include "qemu/cutils.h"
> +#include "qemu/guest-random.h"
> #include "hw/ppc/spapr_drc.h"
> #include "qom/object.h"
> #include "migration/vmstate.h"
> @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> spapr_drc_release(drc);
> }
>
> +
> +/*
> + * @drc : device DRC targetting which the async hcalls to be made.
> + *
> + * All subsequent requests to run/query the status should use the
> + * unique token returned here.
> + */
> +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> +{
> + Error *err = NULL;
> + uint64_t token;
> + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> +
> + state = g_malloc0(sizeof(*state));
> + state->pending = true;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> +retry:
> + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> + error_report_err(err);
> + g_free(state);
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> + return 0;
> + }
> +
> + if (!token) /* Token should be non-zero */
> + goto retry;
> +
> + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> + if (tmp->continue_token == token) {
> + /* If the token already in use, get a new one */
> + goto retry;
> + }
> + }
> + }
> +
> + state->continue_token = token;
> + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> +
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +
> + return state->continue_token;
> +}
> +
> +static void *spapr_drc_async_hcall_runner(void *opaque)
> +{
> + int response = -1;
> + SpaprDrcDeviceAsyncHCallState *state = opaque;
> +
> + /*
> + * state is freed only after this thread finishes(after pthread_join()),
> + * don't worry about it becoming NULL.
> + */
> +
> + response = state->func(state->data);
> +
> + state->hcall_ret = response;
> + state->pending = 0;
> +
> + return NULL;
> +}
> +
> +/*
> + * @drc : device DRC targetting which the async hcalls to be made.
> + * token : The continue token to be used for tracking as recived from
> + * spapr_drc_get_new_async_hcall_token
> + * @func() : the worker function which needs to be executed asynchronously
> + * @data : data to be passed to the asynchronous function. Worker is supposed
> + * to free/cleanup the data that is passed here
It'd be cleaner to pass a completion callback and have free/cleanup handled there.
> + */
> +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> + SpaprDrcAsyncHcallWorkerFunc *func, void *data)
> +{
> + SpaprDrcDeviceAsyncHCallState *state;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> + if (state->continue_token == token) {
> + state->func = func;
> + state->data = data;
> + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> + spapr_drc_async_hcall_runner, state,
> + QEMU_THREAD_JOINABLE);
qemu_thread_create() exits on failure, it shouldn't be called on
a guest triggerable path, eg. a buggy guest could call it up to
the point that pthread_create() returns EAGAIN.
Please use a thread pool (see thread_pool_submit_aio()). This takes care
of all the thread housekeeping for you in a safe way, and it provides a
completion callback API. The implementation could then be just about
having two lists: one for pending requests (fed here) and one for
completed requests (fed by the completion callback).
> + break;
> + }
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +}
> +
> +/*
> + * spapr_drc_finish_async_hcalls
> + * Waits for all pending async requests to complete
> + * thier execution and free the states
> + */
> +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> +{
> + SpaprDrcDeviceAsyncHCallState *state, *next;
> +
> + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> + return;
> + }
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> + qemu_thread_join(&state->thread);
With a thread-pool, you'd just need to aio_poll() until the pending list
is empty and then clear the completed list.
> + QLIST_REMOVE(state, node);
> + g_free(state);
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +}
> +
> +/*
> + * spapr_drc_get_async_hcall_status
> + * Fetches the status of the hcall worker and returns H_BUSY
> + * if the worker is still running.
> + */
> +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
> +{
> + int ret = H_PARAMETER;
> + SpaprDrcDeviceAsyncHCallState *state, *node;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
> + if (state->continue_token == token) {
> + if (state->pending) {
> + ret = H_BUSY;
> + break;
> + } else {
> + ret = state->hcall_ret;
> + qemu_thread_join(&state->thread);
Like for qemu_thread_create(), the guest shouldn't be responsible for
thread housekeeping. Getting the hcall status should just be about
finding the token in the pending or completed lists.
> + QLIST_REMOVE(state, node);
> + g_free(state);
> + break;
> + }
> + }
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +
> + return ret;
> +}
> +
> void spapr_drc_reset(SpaprDrc *drc)
> {
> SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> drc->ccs_offset = -1;
> drc->ccs_depth = -1;
> }
> + spapr_drc_finish_async_hcalls(drc);
> }
>
> static bool spapr_drc_unplug_requested_needed(void *opaque)
> @@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> drc->owner = owner;
> prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> spapr_drc_index(drc));
> +
Unrelated change.
> object_property_add_child(owner, prop_name, OBJECT(drc));
> object_unref(OBJECT(drc));
> qdev_realize(DEVICE(drc), NULL, NULL);
> @@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> object_property_add(obj, "fdt", "struct", prop_get_fdt,
> NULL, NULL, NULL);
> drc->state = drck->empty_state;
> +
> + qemu_mutex_init(&drc->async_hcall_states_lock);
> + QLIST_INIT(&drc->async_hcall_states);
> +
Empty line not needed.
> }
>
> static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 165b281496..77f6e4386c 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -18,6 +18,7 @@
> #include "sysemu/runstate.h"
> #include "hw/qdev-core.h"
> #include "qapi/error.h"
> +#include "block/thread-pool.h"
>
> #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> @@ -168,6 +169,21 @@ typedef enum {
> SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> } SpaprDrcState;
>
> +typedef struct SpaprDrc SpaprDrc;
> +
> +typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
> +typedef struct SpaprDrcDeviceAsyncHCallState {
> + uint64_t continue_token;
> + bool pending;
> +
> + int hcall_ret;
> + SpaprDrcAsyncHcallWorkerFunc *func;
> + void *data;
> +
> + QemuThread thread;
> +
> + QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
> +} SpaprDrcDeviceAsyncHCallState;
> typedef struct SpaprDrc {
> /*< private >*/
> DeviceState parent;
> @@ -182,6 +198,10 @@ typedef struct SpaprDrc {
> int ccs_offset;
> int ccs_depth;
>
> + /* async hcall states */
> + QemuMutex async_hcall_states_lock;
> + QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
> +
> /* device pointer, via link property */
> DeviceState *dev;
> bool unplug_requested;
> @@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
> /* Returns true if a hot plug/unplug request is pending */
> bool spapr_drc_transient(SpaprDrc *drc);
>
> +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
> +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> + SpaprDrcAsyncHcallWorkerFunc, void *data);
> +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
> +
> static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
> {
> return drc->unplug_requested;
>
>
>
^ permalink raw reply
* Re: [PATCH] KVM: PPC: fix comparison to bool warning
From: Michael Ellerman @ 2020-12-21 11:03 UTC (permalink / raw)
To: xiakaixu1987@gmail.com, paulus; +Cc: Kaixu Xia, linuxppc-dev, kvm-ppc
In-Reply-To: <1604764178-8087-1-git-send-email-kaixuxia@tencent.com>
On Sat, 7 Nov 2020 23:49:38 +0800, xiakaixu1987@gmail.com wrote:
> Fix the following coccicheck warning:
>
> ./arch/powerpc/kvm/booke.c:503:6-16: WARNING: Comparison to bool
> ./arch/powerpc/kvm/booke.c:505:6-17: WARNING: Comparison to bool
> ./arch/powerpc/kvm/booke.c:507:6-16: WARNING: Comparison to bool
Applied to powerpc/next.
[1/1] KVM: PPC: fix comparison to bool warning
https://git.kernel.org/powerpc/c/a300bf8c5f24bdeaa84925d1e0ec6221cbdc7597
cheers
^ permalink raw reply
* Re: [PATCH] KVM: PPC: Book3S: Assign boolean values to a bool variable
From: Michael Ellerman @ 2020-12-21 11:03 UTC (permalink / raw)
To: xiakaixu1987@gmail.com, paulus
Cc: Kaixu Xia, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <1604730382-5810-1-git-send-email-kaixuxia@tencent.com>
On Sat, 7 Nov 2020 14:26:22 +0800, xiakaixu1987@gmail.com wrote:
> Fix the following coccinelle warnings:
>
> ./arch/powerpc/kvm/book3s_xics.c:476:3-15: WARNING: Assignment of 0/1 to bool variable
> ./arch/powerpc/kvm/book3s_xics.c:504:3-15: WARNING: Assignment of 0/1 to bool variable
Applied to powerpc/next.
[1/1] KVM: PPC: Book3S: Assign boolean values to a bool variable
https://git.kernel.org/powerpc/c/13751f8747519fe3bdc738fa6d802fbd94a85ac4
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/ps3: use dma_mapping_error()
From: Michael Ellerman @ 2020-12-21 11:03 UTC (permalink / raw)
To: linuxppc-dev, Vincent Stehlé, linux-kernel
Cc: Geoff Levand, Geert Uytterhoeven
In-Reply-To: <20201213182622.23047-1-vincent.stehle@laposte.net>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 356 bytes --]
On Sun, 13 Dec 2020 19:26:22 +0100, Vincent Stehlé wrote:
> The DMA address returned by dma_map_single() should be checked with
> dma_mapping_error(). Fix the ps3stor_setup() function accordingly.
Applied to powerpc/next.
[1/1] powerpc/ps3: use dma_mapping_error()
https://git.kernel.org/powerpc/c/d0edaa28a1f7830997131cbce87b6c52472825d1
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/configs: Add ppc64le_allnoconfig target
From: Michael Ellerman @ 2020-12-21 11:03 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman; +Cc: dja
In-Reply-To: <20201125031551.2112715-1-mpe@ellerman.id.au>
On Wed, 25 Nov 2020 14:15:51 +1100, Michael Ellerman wrote:
> Add a phony target for ppc64le_allnoconfig, which tests some
> combinations of CONFIG symbols that aren't covered by any of our
> defconfigs.
Applied to powerpc/next.
[1/1] powerpc/configs: Add ppc64le_allnoconfig target
https://git.kernel.org/powerpc/c/5d82344795dbd3fcd74c974ab60b2845970dc5e3
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: Add config fragment for disabling -Werror
From: Michael Ellerman @ 2020-12-21 11:03 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman
In-Reply-To: <20201023040002.3313371-1-mpe@ellerman.id.au>
On Fri, 23 Oct 2020 15:00:02 +1100, Michael Ellerman wrote:
> This makes it easy to disable building with -Werror:
>
> $ make defconfig
> $ grep WERROR .config
> # CONFIG_PPC_DISABLE_WERROR is not set
> CONFIG_PPC_WERROR=y
>
> [...]
Applied to powerpc/next.
[1/1] powerpc: Add config fragment for disabling -Werror
https://git.kernel.org/powerpc/c/c15d1f9d03a0f4f68bf52dffdd541c8054e6de35
cheers
^ permalink raw reply
* Re: [PATCH v2 1/1] powerpc/kvm: Fix mask size for emulated msgsndp
From: Michael Ellerman @ 2020-12-21 11:03 UTC (permalink / raw)
To: Paul Mackerras, Leonardo Bras, Benjamin Herrenschmidt,
Michael Ellerman
Cc: linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <20201208215707.31149-1-leobras.c@gmail.com>
On Tue, 8 Dec 2020 18:57:08 -0300, Leonardo Bras wrote:
> According to ISAv3.1 and ISAv3.0b, the msgsndp is described to split RB in:
> msgtype <- (RB) 32:36
> payload <- (RB) 37:63
> t <- (RB) 57:63
>
> The current way of getting 'msgtype', and 't' is missing their MSB:
> msgtype: ((arg >> 27) & 0xf) : Gets (RB) 33:36, missing bit 32
> t: (arg &= 0x3f) : Gets (RB) 58:63, missing bit 57
>
> [...]
Applied to powerpc/next.
[1/1] KVM: PPC: Book3S HV: Fix mask size for emulated msgsndp
https://git.kernel.org/powerpc/c/87fb4978ef8f7e3d6f51ea8e259638c4e96f2fc0
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/memhotplug: quieting some DLPAR operations
From: Michael Ellerman @ 2020-12-21 11:03 UTC (permalink / raw)
To: linuxppc-dev, Laurent Dufour; +Cc: nathanl, cheloha, paulus, linux-kernel
In-Reply-To: <20201211145954.90143-1-ldufour@linux.ibm.com>
On Fri, 11 Dec 2020 15:59:54 +0100, Laurent Dufour wrote:
> When attempting to remove by index a set of LMB a lot of messages are
> displayed on the console, even when everything goes fine:
>
> pseries-hotplug-mem: Attempting to hot-remove LMB, drc index 8000002d
> Offlined Pages 4096
> pseries-hotplug-mem: Memory at 2d0000000 was hot-removed
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/pseries/memhotplug: Quieten some DLPAR operations
https://git.kernel.org/powerpc/c/20e9de85edae3a5866f29b6cce87c9ec66d62a1b
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/mm: Fix hugetlb_free_pmd_range() and hugetlb_free_pud_range()
From: Michael Ellerman @ 2020-12-21 11:03 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Michael Ellerman, qcai,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <56feccd7b6fcd98e353361a233fa7bb8e67c3164.1607780469.git.christophe.leroy@csgroup.eu>
On Sat, 12 Dec 2020 13:41:25 +0000 (UTC), Christophe Leroy wrote:
> Commit 7bfe54b5f165 ("powerpc/mm: Refactor the floor/ceiling check in
> hugetlb range freeing functions") inadvertely removed the mask
> applied to start parameter in those two functions, leading to the
> following crash on power9.
>
> [ 7703.114640][T58070] LTP: starting hugemmap05_1 (hugemmap05 -m)
> [ 7703.157792][ C99] ------------[ cut here ]------------
> [ 7703.158279][ C99] kernel BUG at arch/powerpc/mm/book3s64/pgtable.c:387!
> [ 7703.158306][ C99] Oops: Exception in kernel mode, sig: 5 [#1]
> [ 7703.158330][ C99] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256 NUMA PowerNV
> [ 7703.158343][ C99] Modules linked in: vfio_pci vfio_virqfd vfio_iommu_spapr_tce vfio vfio_spapr_eeh loop kvm_hv kvm ip_tables x_tables sd_mod ahci libahci tg3 libata firmware_class libphy dm_mirror dm_region_hash dm_log dm_mod [last unloaded: dummy_del_mod]
> [ 7703.158435][ C99] CPU: 99 PID: 308 Comm: ksoftirqd/99 Tainted: G O 5.10.0-rc7-next-20201211 #1
> [ 7703.158464][ C99] NIP: c00000000005dbec LR: c0000000003352f4 CTR: 0000000000000000
> [ 7703.158489][ C99] REGS: c00020000bb6f830 TRAP: 0700 Tainted: G O (5.10.0-rc7-next-20201211)
> [ 7703.158528][ C99] MSR: 900000000282b033 <SF,HV,VEC,VSX,EE,FP,ME,IR,DR,RI,LE> CR: 24002284 XER: 20040000
> [ 7703.158570][ C99] GPR00: c0000000003352f4 c00020000bb6fad0 c000000007f70b00 c0002000385b3ff0
> [ 7703.158570][ C99] GPR04: 0000000000000000 0000000000000003 c00020000bb6f8b4 0000000000000001
> [ 7703.158570][ C99] GPR08: 0000000000000001 0000000000000009 0000000000000008 0000000000000002
> [ 7703.158570][ C99] GPR12: 0000000024002488 c000201fff649c00 c000000007f2a20c 0000000000000000
> [ 7703.158570][ C99] GPR16: 0000000000000007 0000000000000000 c000000000194d10 c000000000194d10
> [ 7703.158570][ C99] GPR24: 0000000000000014 0000000000000015 c000201cc6e72398 c000000007fac4b4
> [ 7703.158570][ C99] GPR28: c000000007f2bf80 c000000007fac2f8 0000000000000008 c000200033870000
> [ 7703.158766][ C99] NIP [c00000000005dbec] __tlb_remove_table+0x1dc/0x1e0
> pgtable_free at arch/powerpc/mm/book3s64/pgtable.c:387
> (inlined by) __tlb_remove_table at arch/powerpc/mm/book3s64/pgtable.c:405
> [ 7703.158805][ C99] LR [c0000000003352f4] tlb_remove_table_rcu+0x54/0xa0
> [ 7703.158853][ C99] Call Trace:
> [ 7703.158872][ C99] [c00020000bb6fad0] [c00000000005db4c] __tlb_remove_table+0x13c/0x1e0 (unreliable)
> [ 7703.158890][ C99] [c00020000bb6fb00] [c0000000003352f4] tlb_remove_table_rcu+0x54/0xa0
> __tlb_remove_table_free at mm/mmu_gather.c:101
> (inlined by) tlb_remove_table_rcu at mm/mmu_gather.c:156
> [ 7703.158927][ C99] [c00020000bb6fb30] [c000000000194d7c] rcu_core+0x35c/0xbb0
> rcu_do_batch at kernel/rcu/tree.c:2502
> (inlined by) rcu_core at kernel/rcu/tree.c:2737
> [ 7703.158966][ C99] [c00020000bb6fbf0] [c00000000095a3d0] __do_softirq+0x480/0x704
> [ 7703.159006][ C99] [c00020000bb6fd10] [c0000000000cc1f4] run_ksoftirqd+0x74/0xd0
> run_ksoftirqd at kernel/softirq.c:651
> (inlined by) run_ksoftirqd at kernel/softirq.c:642
> [ 7703.159046][ C99] [c00020000bb6fd30] [c0000000001040c8] smpboot_thread_fn+0x278/0x320
> [ 7703.159096][ C99] [c00020000bb6fda0] [c0000000000fc8a4] kthread+0x1c4/0x1d0
> [ 7703.159145][ C99] [c00020000bb6fe10] [c00000000000d9fc] ret_from_kernel_thread+0x5c/0x80
> [ 7703.159183][ C99] Instruction dump:
> [ 7703.159204][ C99] 60000000 7c0802a6 3c82f8b4 7fe3fb78 38847470 f8010040 482b4fc5 60000000
> [ 7703.159248][ C99] 0fe00000 7c0802a6 fbe10028 f8010040 <0fe00000> 3c4c07f1 38422f10 7c0802a6
> [ 7703.159293][ C99] ---[ end trace 1d92a5231ba6a0d5 ]---
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/mm: Fix hugetlb_free_pmd_range() and hugetlb_free_pud_range()
https://git.kernel.org/powerpc/c/2198d4934ee8b81341a84c9ec8bb25b4b0d02522
cheers
^ 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