Re: [kvm-unit-tests v2 03/10] powerpc: abstract H_CEDE calls into a sleep functions
On Thu Mar 23, 2023 at 10:12 PM AEST, Thomas Huth wrote:
> On 20/03/2023 08.03, Nicholas Piggin wrote:
> > This consolidates several implementations, and it no longer leaves
> > MSR[EE] enabled after the decrementer interrupt is handled, but
> > rather disables it on return.
> >
> > The handler no longer allows a continuous ticking, but rather dec
> > has to be re-armed and EE re-enabled (e.g., via H_CEDE hcall) each
> > time.
> >
> > Signed-off-by: Nicholas Piggin
> > ---
> > lib/powerpc/asm/handlers.h | 2 +-
> > lib/powerpc/asm/ppc_asm.h | 1 +
> > lib/powerpc/asm/processor.h | 7 +++
> > lib/powerpc/handlers.c | 10 -
> > lib/powerpc/processor.c | 42 +
> > powerpc/sprs.c | 6 +-
> > powerpc/tm.c| 20 +-
> > 7 files changed, 57 insertions(+), 31 deletions(-)
> >
> > diff --git a/lib/powerpc/asm/handlers.h b/lib/powerpc/asm/handlers.h
> > index 64ba727..e4a0cd4 100644
> > --- a/lib/powerpc/asm/handlers.h
> > +++ b/lib/powerpc/asm/handlers.h
> > @@ -3,6 +3,6 @@
> >
> > #include
> >
> > -void dec_except_handler(struct pt_regs *regs, void *data);
> > +void dec_handler_oneshot(struct pt_regs *regs, void *data);
> >
> > #endif /* _ASMPOWERPC_HANDLERS_H_ */
> > diff --git a/lib/powerpc/asm/ppc_asm.h b/lib/powerpc/asm/ppc_asm.h
> > index 1b85f6b..6299ff5 100644
> > --- a/lib/powerpc/asm/ppc_asm.h
> > +++ b/lib/powerpc/asm/ppc_asm.h
> > @@ -36,6 +36,7 @@
> > #endif /* __BYTE_ORDER__ */
> >
> > /* Machine State Register definitions: */
> > +#define MSR_EE_BIT 15 /* External Interrupts Enable */
> > #define MSR_SF_BIT63 /* 64-bit mode */
> >
> > #endif /* _ASMPOWERPC_PPC_ASM_H */
> > diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h
> > index ac001e1..ebfeff2 100644
> > --- a/lib/powerpc/asm/processor.h
> > +++ b/lib/powerpc/asm/processor.h
> > @@ -20,6 +20,8 @@ static inline uint64_t get_tb(void)
> >
> > extern void delay(uint64_t cycles);
> > extern void udelay(uint64_t us);
> > +extern void sleep_tb(uint64_t cycles);
> > +extern void usleep(uint64_t us);
> >
> > static inline void mdelay(uint64_t ms)
> > {
> > @@ -27,4 +29,9 @@ static inline void mdelay(uint64_t ms)
> > udelay(1000);
> > }
> >
> > +static inline void msleep(uint64_t ms)
> > +{
> > + usleep(ms * 1000);
> > +}
> > +
> > #endif /* _ASMPOWERPC_PROCESSOR_H_ */
> > diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
> > index c8721e0..296f14f 100644
> > --- a/lib/powerpc/handlers.c
> > +++ b/lib/powerpc/handlers.c
> > @@ -9,15 +9,13 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > /*
> >* Generic handler for decrementer exceptions (0x900)
> > - * Just reset the decrementer back to the value specified when registering
> > the
> > - * handler
> > + * Return with MSR[EE] disabled.
> >*/
> > -void dec_except_handler(struct pt_regs *regs __unused, void *data)
> > +void dec_handler_oneshot(struct pt_regs *regs, void *data)
> > {
> > - uint64_t dec = *((uint64_t *) data);
> > -
> > - asm volatile ("mtdec %0" : : "r" (dec));
> > + regs->msr &= ~(1UL << MSR_EE_BIT);
> > }
> > diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
> > index ec85b9d..e77a240 100644
> > --- a/lib/powerpc/processor.c
> > +++ b/lib/powerpc/processor.c
> > @@ -10,6 +10,8 @@
> > #include
> > #include
> > #include
> > +#include
> > +#include
> >
> > static struct {
> > void (*func)(struct pt_regs *, void *data);
> > @@ -54,3 +56,43 @@ void udelay(uint64_t us)
> > {
> > delay((us * tb_hz) / 100);
> > }
> > +
> > +void sleep_tb(uint64_t cycles)
> > +{
> > + uint64_t start, end, now;
> > +
> > + start = now = get_tb();
> > + end = start + cycles;
> > +
> > + while (end > now) {
> > + uint64_t left = end - now;
> > +
> > + /* Could support large decrementer */
> > + if (left > 0x7fff)
> > + left = 0x7fff;
> > +
> > + asm volatile ("mtdec %0" : : "r" (left));
> > + handle_exception(0x900, _handler_oneshot, NULL);
>
> Wouldn't it be better to first call handle_exception() before moving
> something into the decrementer?
It shouldn't really matter, the drecrementer could be anything here
(probably it's already -ve here if it starts at zero). It only matters
when MSR[EE]=1 or we make the H_CEDE call.
I'll add a comment at least.
Thanks,
Nick
Re: [kvm-unit-tests v2 02/10] powerpc: add local variant of SPR test
On Thu Mar 23, 2023 at 9:26 PM AEST, Thomas Huth wrote: > On 20/03/2023 08.03, Nicholas Piggin wrote: > > This adds the non-migration variant of the SPR test to the matrix, > > which can be simpler to run and debug. > > > > Reviewed-by: Thomas Huth > > Signed-off-by: Nicholas Piggin > > --- > > powerpc/unittests.cfg | 4 > > 1 file changed, 4 insertions(+) > > > > diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg > > index 1e74948..3e41598 100644 > > --- a/powerpc/unittests.cfg > > +++ b/powerpc/unittests.cfg > > @@ -68,5 +68,9 @@ groups = h_cede_tm > > > > [sprs] > > file = sprs.elf > > +groups = sprs > > Looking at this again, I think you don't really need a "groups =" entry here > ... I'd suggest to drop that line. Okay, will do. Thanks, Nick
Re: [PATCH 0/2] KVM: PPC: support kvm selftests
On Thu Mar 23, 2023 at 3:41 AM AEST, Sean Christopherson wrote: > On Thu, Mar 16, 2023, Michael Ellerman wrote: > > Nicholas Piggin writes: > > > Hi, > > > > > > This series adds initial KVM selftests support for powerpc > > > (64-bit, BookS). > > > > Awesome. > > > > > It spans 3 maintainers but it does not really > > > affect arch/powerpc, and it is well contained in selftests > > > code, just touches some makefiles and a tiny bit headers so > > > conflicts should be unlikely and trivial. > > > > > > I guess Paolo is the best point to merge these, if no comments > > > or objections? > > > > Yeah. If it helps: > > > > Acked-by: Michael Ellerman (powerpc) > > What is the long term plan for KVM PPC maintenance? I was under the > impression > that KVM PPC was trending toward "bug fixes only", but the addition of > selftests > support suggests otherwise. We plan to continue maintaining it. New support and features has been a bit low in the past couple of years, hopefully that will pick up a bit though. > I ask primarily because routing KVM PPC patches through the PPC tree is going > to > be problematic if KVM PPC sees signficiant development. The current > situation is > ok because the volume of patches is low and KVM PPC isn't trying to drive > anything > substantial into common KVM code, but if that changes... Michael has done KVM topic branches to pull from a few times when such conflicts came up (at smaller scale). If we end up with larger changes or regular conflicts we might start up a kvm-ppc tree again I guess. > My other concern is that for selftests specifically, us KVM folks are taking > on > more maintenance burden by supporting PPC. AFAIK, none of the people that > focus > on KVM selftests in any meaningful capacity have access to PPC hardware, let > alone > know enough about the architecture to make intelligent code changes. > > Don't get me wrong, I'm very much in favor of more testing, I just don't want > KVM > to get left holding the bag. Understood. I'll be happy to maintain powerpc part of kvm selftests and do any fixes that are needed for core code changes.If support fell away you could leave it broken (or rm -rf it if you prefer) -- I wouldn't ask anybody to work on archs they don't know or aren't paid to. Not sure if anything more can be done to help your process or ease your mind. It (KVM and kvm-selftests) can run in QEMU at least. Thanks, Nick
Re: perf tools power9 JSON files build breakage on ubuntu 18.04 cross build
On 3/23/23 18:41, Arnaldo Carvalho de Melo wrote: > Exception processing pmu-events/arch/powerpc/power9/other.json > Traceback (most recent call last): > File "pmu-events/jevents.py", line 997, in > main() > File "pmu-events/jevents.py", line 979, in main > ftw(arch_path, [], preprocess_one_file) > File "pmu-events/jevents.py", line 935, in ftw > ftw(item.path, parents + [item.name], action) > File "pmu-events/jevents.py", line 933, in ftw > action(parents, item) > File "pmu-events/jevents.py", line 514, in preprocess_one_file > for event in read_json_events(item.path, topic): > File "pmu-events/jevents.py", line 388, in read_json_events > events = json.load(open(path), object_hook=JsonEvent) > File "/usr/lib/python3.6/json/__init__.py", line 296, in load > return loads(fp.read(), > File "/usr/lib/python3.6/encodings/ascii.py", line 26, in decode > return codecs.ascii_decode(input, self.errors)[0] > UnicodeDecodeError: 'ascii' codec can't decode byte 0xc2 in position 55090: > ordinal not in range(128) > CC /tmp/build/perf/tests/expr.o > pmu-events/Build:35: recipe for target > '/tmp/build/perf/pmu-events/pmu-events.c' failed > make[3]: *** [/tmp/build/perf/pmu-events/pmu-events.c] Error 1 > make[3]: *** Deleting file '/tmp/build/perf/pmu-events/pmu-events.c' > Makefile.perf:679: recipe for target > '/tmp/build/perf/pmu-events/pmu-events-in.o' failed > make[2]: *** [/tmp/build/perf/pmu-events/pmu-events-in.o] Error 2 > make[2]: *** Waiting for unfinished jobs > > > Now jevents is an opt-out feature so I'm noticing these problems. Hi Arnaldo, Thanks for raising it. I will check this issue. Thanks, Kajol Jain > > A similar fix for s390 was accepted today: > > > https://lore.kernel.org/r/20230323122532.2305847-1-tmri...@linux.ibm.com > https://lore.kernel.org/r/ZBwkl77/I31AQk12@osiris
Re: Memory coherency issue with IO thread offloading?
On Sat Mar 25, 2023 at 11:20 AM AEST, Jens Axboe wrote: > On 3/24/23 7:15?PM, Jens Axboe wrote: > >> Are there any CONFIG options I'd need to trip this? > > > > I don't think you need any special CONFIG options. I'll attach my config > > here, and I know the default distro one hits it too. But perhaps the > > mariadb version is not new enough? I think you need 10.6 or above, as > > will use io_uring by default. What version are you running? > > And here's the .config and the patch for using queue_work(). So if you *don't* apply this patch, the work gets queued up with an IO thread? In io-wq.c? Does that worker end up just doing an io_write() same as this one? Can the queueing cause the creation of an IO thread (if one does not exist, or all blocked?) I'm wondering what the practical differences are between this patch and upstream. kthread_use_mm() should be basically the same as context switching to an IO thread. There is maybe a difference in that kthread_switch_mm() has a 'sync' instruction *after* the MMU is switched to the new thread from the membarrier code, but a regular context switch might not. The MMU switch does have an isync() after it though, so loads *should* be prohibited from moving ahead of that. Something like this adds a sync roughly where kthread_use_mm() has one. It's a pretty unlikely shot in the dark though. I'm more inclined to think the work submission to the IO thread might have a problem. Thanks, Nick diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 488655f2319f..417c0652dc66 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -5180,6 +5180,7 @@ static struct rq *finish_task_switch(struct task_struct *prev) tick_nohz_task_switch(); finish_lock_switch(rq); finish_arch_post_lock_switch(); + smp_mb(); kcov_finish_switch(current); /* * kmap_local_sched_out() is invoked with rq::lock held and
[PATCH] powerpc: don't try to copy ppc for task with NULL pt_regs
Powerpc sets up PF_KTHREAD and PF_IO_WORKER with a NULL pt_regs, which
from my (arguably very short) checking is not commonly done for other
archs. This is fine, except when PF_IO_WORKER's have been created and
the task does something that causes a coredump to be generated. Then we
get this crash:
Kernel attempted to read user page (160) - exploit attempt? (uid: 1000)
BUG: Kernel NULL pointer dereference on read at 0x0160
Faulting instruction address: 0xc00c3a60
Oops: Kernel access of bad area, sig: 11 [#1]
LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=32 NUMA pSeries
Modules linked in: bochs drm_vram_helper drm_kms_helper xts binfmt_misc ecb ctr
syscopyarea sysfillrect cbc sysimgblt drm_ttm_helper aes_generic ttm sg libaes
evdev joydev virtio_balloon vmx_crypto gf128mul drm dm_mod fuse loop configfs
drm_panel_orientation_quirks ip_tables x_tables autofs4 hid_generic usbhid hid
xhci_pci xhci_hcd usbcore usb_common sd_mod
CPU: 1 PID: 1982 Comm: ppc-crash Not tainted 6.3.0-rc2+ #88
Hardware name: IBM pSeries (emulated by qemu) POWER9 (raw) 0x4e1202 0xf05
of:SLOF,HEAD hv:linux,kvm pSeries
NIP: c00c3a60 LR: c0039944 CTR: c00398e0
REGS: c41833b0 TRAP: 0300 Not tainted (6.3.0-rc2+)
MSR: 8280b033 CR: 88082828 XER:
200400f8
CFAR: c00c386c DAR: 0160 DSISR: 4000 IRQMASK: 0
GPR00: c0039920 c4183650 c175d600 c40f9800
GPR04: 0160 0008 c0039920
GPR08: 0003fe06 2000
GPR12: c00398e0 c003f200 c15edbc0 cba2f648
GPR16: cba2f600 c1616ea8 0004
GPR20: 0048 c4183918 c1410f00 c1410ef8
GPR24: c40f9800 c40f9800 c41837b8 c00398e0
GPR28: ccc4cb80 c40f9800 0008 0008
NIP [c00c3a60] memcpy_power7+0x200/0x7d0
LR [c0039944] ppr_get+0x64/0xb0
Call Trace:
[c4183650] [c0039920] ppr_get+0x40/0xb0 (unreliable)
[c4183690] [c01e5e80] __regset_get+0x180/0x1f0
[c4183700] [c01e5f94] regset_get_alloc+0x64/0x90
[c4183740] [c07ae638] elf_core_dump+0xb98/0x1b60
[c41839c0] [c07bb564] do_coredump+0x1c34/0x24a0
[c4183ba0] [c01acf0c] get_signal+0x71c/0x1410
[c4183ce0] [c00228a0] do_notify_resume+0x140/0x6f0
[c4183db0] [c00353bc]
interrupt_exit_user_prepare_main+0x29c/0x320
[c4183e20] [c003579c] interrupt_exit_user_prepare+0x6c/0xa0
[c4183e50] [c000c6f4] interrupt_return_srr_user+0x8/0x138
--- interrupt: 300 at 0x183ee09e0
NIP: 000183ee09e0 LR: 000183ee09dc CTR: 8280f033
REGS: c4183e80 TRAP: 0300 Not tainted (6.3.0-rc2+)
MSR: 8000d033 CR: 22002848 XER: 00f8
CFAR: 7ffe6d746aa8 DAR: DSISR: 4200 IRQMASK: 0
GPR00: 000183ee09dc 720d37c0 000183f07f00
GPR04: 720d37a8 7ffe6d9eae00
GPR08: 720d3710
GPR12: 7ffe6d9eae00
GPR16:
GPR20: 000183ee0860
GPR24: 7ffe6d9df820 7ffe6d9e 720d7d98 0001
GPR28: 000183ee0c60 720d7924 720d7820
NIP [000183ee09e0] 0x183ee09e0
LR [000183ee09dc] 0x183ee09dc
--- interrupt: 300
Code: f9030018 38630020 409f001c e804 e8c40008 38840010 f803 f8c30008
38630010 78a50720 7cb01120 409c001c <8004> 80c40004 38840008 9003
---[ end trace ]---
note: ppc-crash[1982] exited with irqs disabled
because ppr_get() is trying to copy from a PF_IO_WORKER with a NULL
pt_regs.
Check for a valid pt_regs in both ppc_get/ppr_set, and return an error
if not set. The actual error value doesn't seem to be important here,
so just pick -EINVAL.
Signed-off-by: Jens Axboe
diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c
b/arch/powerpc/kernel/ptrace/ptrace-view.c
index 2087a785f05f..80b699dd0d7f 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-view.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
@@ -290,6 +290,8 @@ static int gpr_set(struct task_struct *target, const struct
user_regset *regset,
static int ppr_get(struct task_struct *target, const struct user_regset
*regset,
struct membuf to)
{
+ if (!target->thread.regs)
+ return -EINVAL;
return membuf_write(, >thread.regs->ppr, sizeof(u64));
}
@@ -297,6 +299,8 @@ static int ppr_set(struct task_struct *target, const struct
user_regset *regset,
unsigned int pos, unsigned int count,
Re: [PATCH 01/10] locking/atomic: Add missing cast to try_cmpxchg() fallbacks
On Fri, Mar 24, 2023 at 5:33 PM Mark Rutland wrote:
>
> On Fri, Mar 24, 2023 at 04:14:22PM +, Mark Rutland wrote:
> > On Fri, Mar 24, 2023 at 04:43:32PM +0100, Uros Bizjak wrote:
> > > On Fri, Mar 24, 2023 at 3:13 PM Mark Rutland wrote:
> > > >
> > > > On Sun, Mar 05, 2023 at 09:56:19PM +0100, Uros Bizjak wrote:
> > > > > Cast _oldp to the type of _ptr to avoid incompatible-pointer-types
> > > > > warning.
> > > >
> > > > Can you give an example of where we are passing an incompatible pointer?
> > >
> > > An example is patch 10/10 from the series, which will fail without
> > > this fix when fallback code is used. We have:
> > >
> > > - } while (local_cmpxchg(>head, offset, head) != offset);
> > > + } while (!local_try_cmpxchg(>head, , head));
> > >
> > > where rb->head is defined as:
> > >
> > > typedef struct {
> > >atomic_long_t a;
> > > } local_t;
> > >
> > > while offset is defined as 'unsigned long'.
> >
> > Ok, but that's because we're doing the wrong thing to start with.
> >
> > Since local_t is defined in terms of atomic_long_t, we should define the
> > generic local_try_cmpxchg() in terms of atomic_long_try_cmpxchg(). We'll
> > still
> > have a mismatch between 'long *' and 'unsigned long *', but then we can fix
> > that in the callsite:
> >
> > while (!local_try_cmpxchg(>head, &(long *)offset, head))
>
> Sorry, that should be:
>
> while (!local_try_cmpxchg(>head, (long *), head))
The fallbacks are a bit more complicated than above, and are different
from atomic_try_cmpxchg.
Please note in patch 2/10, the falbacks when arch_try_cmpxchg_local
are not defined call arch_cmpxchg_local. Also in patch 2/10,
try_cmpxchg_local is introduced, where it calls
arch_try_cmpxchg_local. Targets (and generic code) simply define (e.g.
:
#define local_cmpxchg(l, o, n) \
(cmpxchg_local(&((l)->a.counter), (o), (n)))
+#define local_try_cmpxchg(l, po, n) \
+ (try_cmpxchg_local(&((l)->a.counter), (po), (n)))
which is part of the local_t API. Targets should either define all
these #defines, or none. There are no partial fallbacks as is the case
with atomic_t.
The core of the local_h API is in the local.h header. If the target
doesn't define its own local.h header, then asm-generic/local.h is
used that does exactly what you propose above regarding the usage of
atomic functions.
OTOH, when the target defines its own local.h, then the above
target-dependent #define path applies. The target should define its
own arch_try_cmpxchg_local, otherwise a "generic" target-dependent
fallback that calls target arch_cmpxchg_local applies. In the case of
x86, patch 9/10 enables new instruction by defining
arch_try_cmpxchg_local.
FYI, the patch sequence is carefully chosen so that x86 also exercises
fallback code between different patches in the series.
Targets are free to define local_t to whatever they like, but for some
reason they all define it to:
typedef struct {
atomic_long_t a;
} local_t;
so they have to dig the variable out of the struct like:
#define local_cmpxchg(l, o, n) \
(cmpxchg_local(&((l)->a.counter), (o), (n)))
Regarding the mismatch of 'long *' vs 'unsigned long *': x86
target-specific code does for try_cmpxchg:
#define __raw_try_cmpxchg(_ptr, _pold, _new, size, lock) \
({ \
bool success; \
__typeof__(_ptr) _old = (__typeof__(_ptr))(_pold); \
__typeof__(*(_ptr)) __old = *_old; \
__typeof__(*(_ptr)) __new = (_new); \
so, it *does* cast the "old" pointer to the type of "ptr". The generic
code does *not*. This difference is dangerous, since the compilation
of some code involving try_cmpxchg will compile OK for x86 but will
break for other targets that use try_cmpxchg fallback templates (I was
the unlucky one that tripped on this in the past). Please note that
this problem is not specific to the proposed local_try_cmpxchg series,
but affects the existing try_cmpxchg API.
Also, I don't think that "fixing" callsites is the right thing to do.
The generic code should follow x86 and cast the "old" pointer to the
type of "ptr" inside the fallback.
> The fundamenalthing I'm trying to say is that the
> atomic/atomic64/atomic_long/local/local64 APIs should be type-safe, and for
> their try_cmpxchg() implementations, the type signature should be:
>
> ${atomictype}_try_cmpxchg(${atomictype} *ptr, ${inttype} *old,
> ${inttype} new)
This conversion should be performed also for the cmpxchg family of
functions, if desired at all. try_cmpxchg fallback is just cmpxchg
with some extra code around.
Thanks,
Uros.
Re: [PATCH] rcu: torture: ppc: Remove duplicated argument --enable-kvm
On Sun, Mar 26, 2023 at 08:24:34AM +0800, zhouzho...@gmail.com wrote:
> From: Zhouyi Zhou
>
> The argument --enable-kvm is duplicated because qemu_args
> in kvm-test-1-run.sh has already give this.
>
> Signed-off-by: Zhouyi Zhou
Good catch! Applied, thank you!
Thanx, Paul
> ---
> Dear RCU and PPC developers
>
> I discover this possible minor flaw when I am performing RCU torture
> test in PPC VM of of Open Source Lab of Oregon State University.
>
> But I can't test my patch because I am in a VM.
>
> Thanks for your time
>
> Cheers ;-)
> Zhouyi
> --
> tools/testing/selftests/rcutorture/bin/functions.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/rcutorture/bin/functions.sh
> b/tools/testing/selftests/rcutorture/bin/functions.sh
> index b52d5069563c..48b9147e8c91 100644
> --- a/tools/testing/selftests/rcutorture/bin/functions.sh
> +++ b/tools/testing/selftests/rcutorture/bin/functions.sh
> @@ -250,7 +250,7 @@ identify_qemu_args () {
> echo -machine virt,gic-version=host -cpu host
> ;;
> qemu-system-ppc64)
> - echo -enable-kvm -M pseries -nodefaults
> + echo -M pseries -nodefaults
> echo -device spapr-vscsi
> if test -n "$TORTURE_QEMU_INTERACTIVE" -a -n "$TORTURE_QEMU_MAC"
> then
> --
> 2.34.1
>
