* Re: [PATCH] arch: powerpc: Remove duplicate include of interrupt.h
From: Christophe Leroy @ 2021-03-23 5:41 UTC (permalink / raw)
To: Wan Jiabing, zhouchuangao
Cc: linux-kernel, Nicholas Piggin, Paul Mackerras, Aneesh Kumar K.V,
kael_w, Michal Suchanek, linuxppc-dev, Qiang Zhao
In-Reply-To: <20210323024126.237840-1-wanjiabing@vivo.com>
Le 23/03/2021 à 03:41, Wan Jiabing a écrit :
> asm/interrupt.h has been included at line 12. According to
> alphabetic order,we remove the duplicate one at line 10.
Could you please cook a single patch for all files in arch/powerpc/
Thanks
Christophe
>
> Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
> ---
> arch/powerpc/kernel/interrupt.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index c475a229a42a..11d456896772 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -7,7 +7,6 @@
> #include <asm/asm-prototypes.h>
> #include <asm/kup.h>
> #include <asm/cputime.h>
> -#include <asm/interrupt.h>
> #include <asm/hw_irq.h>
> #include <asm/interrupt.h>
> #include <asm/kprobes.h>
>
^ permalink raw reply
* Re: [PATCH] tools: testing: Remove duplicate include of sched.h
From: Christophe Leroy @ 2021-03-23 5:44 UTC (permalink / raw)
To: Wan Jiabing, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Shuah Khan, linuxppc-dev, linux-kselftest,
linux-kernel
Cc: kael_w
In-Reply-To: <20210323033413.284420-1-wanjiabing@vivo.com>
Le 23/03/2021 à 04:34, Wan Jiabing a écrit :
> sched.h has been included at line 33.
> So we remove the duplicate one at line 36.
Can you please send a single patch for all files in tools/testing/selftests/powerpc/
Thanks
Christophe
>
> Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
> ---
> tools/testing/selftests/powerpc/mm/tlbie_test.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/tools/testing/selftests/powerpc/mm/tlbie_test.c b/tools/testing/selftests/powerpc/mm/tlbie_test.c
> index f85a0938ab25..48344a74b212 100644
> --- a/tools/testing/selftests/powerpc/mm/tlbie_test.c
> +++ b/tools/testing/selftests/powerpc/mm/tlbie_test.c
> @@ -33,7 +33,6 @@
> #include <sched.h>
> #include <time.h>
> #include <stdarg.h>
> -#include <sched.h>
> #include <pthread.h>
> #include <signal.h>
> #include <sys/prctl.h>
>
^ permalink raw reply
* [PATCH] [v2] tools: testing: Remove duplicate includes
From: Wan Jiabing @ 2021-03-23 6:15 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
Shuah Khan, Wan Jiabing, linuxppc-dev, linux-kselftest,
linux-kernel
Cc: kael_w
sched.h has been included at line 33, so remove the
duplicate one at line 36.
inttypes.h has been included at line 19, so remove the
duplicate one at line 23.
pthread.h has been included at line 17,so remove the
duplicate one at line 20.
Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
---
tools/testing/selftests/powerpc/mm/tlbie_test.c | 1 -
tools/testing/selftests/powerpc/tm/tm-poison.c | 1 -
tools/testing/selftests/powerpc/tm/tm-vmx-unavail.c | 1 -
3 files changed, 3 deletions(-)
diff --git a/tools/testing/selftests/powerpc/mm/tlbie_test.c b/tools/testing/selftests/powerpc/mm/tlbie_test.c
index f85a0938ab25..48344a74b212 100644
--- a/tools/testing/selftests/powerpc/mm/tlbie_test.c
+++ b/tools/testing/selftests/powerpc/mm/tlbie_test.c
@@ -33,7 +33,6 @@
#include <sched.h>
#include <time.h>
#include <stdarg.h>
-#include <sched.h>
#include <pthread.h>
#include <signal.h>
#include <sys/prctl.h>
diff --git a/tools/testing/selftests/powerpc/tm/tm-poison.c b/tools/testing/selftests/powerpc/tm/tm-poison.c
index 29e5f26af7b9..27c083a03d1f 100644
--- a/tools/testing/selftests/powerpc/tm/tm-poison.c
+++ b/tools/testing/selftests/powerpc/tm/tm-poison.c
@@ -20,7 +20,6 @@
#include <sched.h>
#include <sys/types.h>
#include <signal.h>
-#include <inttypes.h>
#include "tm.h"
diff --git a/tools/testing/selftests/powerpc/tm/tm-vmx-unavail.c b/tools/testing/selftests/powerpc/tm/tm-vmx-unavail.c
index e2a0c07e8362..9ef37a9836ac 100644
--- a/tools/testing/selftests/powerpc/tm/tm-vmx-unavail.c
+++ b/tools/testing/selftests/powerpc/tm/tm-vmx-unavail.c
@@ -17,7 +17,6 @@
#include <pthread.h>
#include <sys/mman.h>
#include <unistd.h>
-#include <pthread.h>
#include "tm.h"
#include "utils.h"
--
2.25.1
^ permalink raw reply related
* Re: [PATCH -next] powerpc: kernel/time.c - cleanup warnings
From: heying (H) @ 2021-03-23 6:21 UTC (permalink / raw)
To: Christophe Leroy, mpe, benh, paulus, npiggin, msuchanek, peterz,
geert+renesas, kernelfans, frederic
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <5ee06736-7fc4-7993-a8b5-042e1890a6de@huawei.com>
Dear Christophe,
在 2021/3/18 10:28, heying (H) 写道:
>
> 在 2021/3/17 19:16, Christophe Leroy 写道:
>>
>>
>> Le 17/03/2021 à 11:34, He Ying a écrit :
>>> We found these warnings in arch/powerpc/kernel/time.c as follows:
>>> warning: symbol 'decrementer_max' was not declared. Should it be
>>> static?
>>> warning: symbol 'rtc_lock' was not declared. Should it be static?
>>> warning: symbol 'dtl_consumer' was not declared. Should it be static?
>>>
>>> Declare 'decrementer_max' in arch/powerpc/include/asm/time.h. And
>>> include
>>> proper header in which 'rtc_lock' is declared. Move 'dtl_consumer'
>>> definition behind "include <asm/dtl.h>" because 'dtl_consumer' is
>>> declared
>>> there.
>>>
>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>> Signed-off-by: He Ying <heying24@huawei.com>
>>> ---
>>> arch/powerpc/include/asm/time.h | 1 +
>>> arch/powerpc/kernel/time.c | 7 +++----
>>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/time.h
>>> b/arch/powerpc/include/asm/time.h
>>> index 8dd3cdb25338..2cd2b50bedda 100644
>>> --- a/arch/powerpc/include/asm/time.h
>>> +++ b/arch/powerpc/include/asm/time.h
>>> @@ -22,6 +22,7 @@ extern unsigned long tb_ticks_per_jiffy;
>>> extern unsigned long tb_ticks_per_usec;
>>> extern unsigned long tb_ticks_per_sec;
>>> extern struct clock_event_device decrementer_clockevent;
>>> +extern u64 decrementer_max;
>>> extern void generic_calibrate_decr(void);
>>> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
>>> index b67d93a609a2..409967713ca6 100644
>>> --- a/arch/powerpc/kernel/time.c
>>> +++ b/arch/powerpc/kernel/time.c
>>> @@ -55,6 +55,7 @@
>>> #include <linux/sched/cputime.h>
>>> #include <linux/sched/clock.h>
>>> #include <linux/processor.h>
>>> +#include <linux/mc146818rtc.h>
>>
>> I don't think that's the good place. It has no link to powerpc, it is
>> only by chance that it has the same name.
>>
>> As rtc_lock is defined in powerpc time.c, I think you should declare
>> it in powerpc asm/time.h
>
> My first thought was the same as yours. I tried to add declaration in
> powerpc asm/time.h, but got a compiling error:
>
> drivers/rtc/rtc-vr41xx.c:75:24: error: static declaration of
> ‘rtc_lock’ follows non-static declaration
> static DEFINE_SPINLOCK(rtc_lock);
>
> In file included from ./arch/powerpc/include/asm/delay.h:7:0,
> from ./arch/powerpc/include/asm/io.h:33,
> from ./include/linux/io.h:13,
> from drivers/rtc/rtc-vr41xx.c:11:
> ./arch/powerpc/include/asm/time.h:25:19: note: previous declaration of
> ‘rtc_lock’ was here
> extern spinlock_t rtc_lock;
>
> There's a conflict. Perhaps I can rename it in drivers/rtc/rtc-vr41xx.c.
>
>
> But I find an existing declaration in linux/mc146818rtc.h and there's
> only one definition for 'rtc_lock' in powerpc.
>
> There's some includes of mc146818rtc.h in powperc. I wonder they point
> to the same thing. But I'm not very sure
>
> because the header's name looks a bit strange.
How about including mc146818rtc.h in powperpc kernel/time.c? May I have
your opinions please?
Thanks.
^ permalink raw reply
* [PATCH] [v2] arch: powerpc: Remove duplicate includes
From: Wan Jiabing @ 2021-03-23 6:29 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
Nicholas Piggin, Aneesh Kumar K.V, Cédric Le Goater,
Randy Dunlap, Wan Jiabing, Ganesh Goudar, Christophe Leroy,
Michal Suchanek, Geert Uytterhoeven, Pingfan Liu,
Frederic Weisbecker, linuxppc-dev, linux-kernel
Cc: kael_w
mmu-hash.h: asm/bug.h has been included at line 12, so remove
the duplicate one at line 21.
interrupt.c: asm/interrupt.h has been included at line 12, so
remove the duplicate one at line 10.
time.c: linux/sched/clock.h has been included at line 33,so
remove the duplicate one at line 56 and move sched/cputime.h
under sched including segament.
Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
---
arch/powerpc/include/asm/book3s/64/mmu-hash.h | 1 -
arch/powerpc/kernel/interrupt.c | 1 -
arch/powerpc/kernel/time.c | 3 +--
3 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/book3s/64/mmu-hash.h b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
index f911bdb68d8b..3004f3323144 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu-hash.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu-hash.h
@@ -18,7 +18,6 @@
* complete pgtable.h but only a portion of it.
*/
#include <asm/book3s/64/pgtable.h>
-#include <asm/bug.h>
#include <asm/task_size_64.h>
#include <asm/cpu_has_feature.h>
diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index c475a229a42a..11d456896772 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -7,7 +7,6 @@
#include <asm/asm-prototypes.h>
#include <asm/kup.h>
#include <asm/cputime.h>
-#include <asm/interrupt.h>
#include <asm/hw_irq.h>
#include <asm/interrupt.h>
#include <asm/kprobes.h>
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index b67d93a609a2..e2766e0e2a3a 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -31,6 +31,7 @@
#include <linux/export.h>
#include <linux/sched.h>
#include <linux/sched/clock.h>
+#include <linux/sched/cputime.h>
#include <linux/kernel.h>
#include <linux/param.h>
#include <linux/string.h>
@@ -52,8 +53,6 @@
#include <linux/irq_work.h>
#include <linux/of_clk.h>
#include <linux/suspend.h>
-#include <linux/sched/cputime.h>
-#include <linux/sched/clock.h>
#include <linux/processor.h>
#include <asm/trace.h>
--
2.25.1
^ permalink raw reply related
* Re: [PATCH -next] powerpc: kernel/time.c - cleanup warnings
From: Christophe Leroy @ 2021-03-23 6:33 UTC (permalink / raw)
To: heying (H), mpe, benh, paulus, npiggin, msuchanek, peterz,
geert+renesas, kernelfans, frederic
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <c6908c07-1814-a7f8-5f48-a4c316fb567c@huawei.com>
Le 23/03/2021 à 07:21, heying (H) a écrit :
> Dear Christophe,
>
>
> 在 2021/3/18 10:28, heying (H) 写道:
>>
>> 在 2021/3/17 19:16, Christophe Leroy 写道:
>>>
>>>
>>> Le 17/03/2021 à 11:34, He Ying a écrit :
>>>> We found these warnings in arch/powerpc/kernel/time.c as follows:
>>>> warning: symbol 'decrementer_max' was not declared. Should it be static?
>>>> warning: symbol 'rtc_lock' was not declared. Should it be static?
>>>> warning: symbol 'dtl_consumer' was not declared. Should it be static?
>>>>
>>>> Declare 'decrementer_max' in arch/powerpc/include/asm/time.h. And include
>>>> proper header in which 'rtc_lock' is declared. Move 'dtl_consumer'
>>>> definition behind "include <asm/dtl.h>" because 'dtl_consumer' is declared
>>>> there.
>>>>
>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>> Signed-off-by: He Ying <heying24@huawei.com>
>>>> ---
>>>> arch/powerpc/include/asm/time.h | 1 +
>>>> arch/powerpc/kernel/time.c | 7 +++----
>>>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
>>>> index 8dd3cdb25338..2cd2b50bedda 100644
>>>> --- a/arch/powerpc/include/asm/time.h
>>>> +++ b/arch/powerpc/include/asm/time.h
>>>> @@ -22,6 +22,7 @@ extern unsigned long tb_ticks_per_jiffy;
>>>> extern unsigned long tb_ticks_per_usec;
>>>> extern unsigned long tb_ticks_per_sec;
>>>> extern struct clock_event_device decrementer_clockevent;
>>>> +extern u64 decrementer_max;
>>>> extern void generic_calibrate_decr(void);
>>>> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
>>>> index b67d93a609a2..409967713ca6 100644
>>>> --- a/arch/powerpc/kernel/time.c
>>>> +++ b/arch/powerpc/kernel/time.c
>>>> @@ -55,6 +55,7 @@
>>>> #include <linux/sched/cputime.h>
>>>> #include <linux/sched/clock.h>
>>>> #include <linux/processor.h>
>>>> +#include <linux/mc146818rtc.h>
>>>
>>> I don't think that's the good place. It has no link to powerpc, it is only by chance that it has
>>> the same name.
>>>
>>> As rtc_lock is defined in powerpc time.c, I think you should declare it in powerpc asm/time.h
>>
>> My first thought was the same as yours. I tried to add declaration in powerpc asm/time.h, but got
>> a compiling error:
>>
>> drivers/rtc/rtc-vr41xx.c:75:24: error: static declaration of ‘rtc_lock’ follows non-static
>> declaration
>> static DEFINE_SPINLOCK(rtc_lock);
>>
>> In file included from ./arch/powerpc/include/asm/delay.h:7:0,
>> from ./arch/powerpc/include/asm/io.h:33,
>> from ./include/linux/io.h:13,
>> from drivers/rtc/rtc-vr41xx.c:11:
>> ./arch/powerpc/include/asm/time.h:25:19: note: previous declaration of ‘rtc_lock’ was here
>> extern spinlock_t rtc_lock;
>>
>> There's a conflict. Perhaps I can rename it in drivers/rtc/rtc-vr41xx.c.
>>
>>
>> But I find an existing declaration in linux/mc146818rtc.h and there's only one definition for
>> 'rtc_lock' in powerpc.
>>
>> There's some includes of mc146818rtc.h in powperc. I wonder they point to the same thing. But I'm
>> not very sure
>>
>> because the header's name looks a bit strange.
>
> How about including mc146818rtc.h in powperpc kernel/time.c? May I have your opinions please?
>
As I said, mc146818rtc.h is not related to powerpc, and if it works that's just chance, and there is
no certainty that it will still work in the future.
If you can't find a clean solution, it is better to leave the warning.
Christophe
^ permalink raw reply
* Re: [PATCH -next] powerpc: kernel/time.c - cleanup warnings
From: heying (H) @ 2021-03-23 6:53 UTC (permalink / raw)
To: Christophe Leroy, mpe, benh, paulus, npiggin, msuchanek, peterz,
geert+renesas, kernelfans, frederic
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <5f4ec5e0-af9c-ba47-4a01-589b1b724cb9@csgroup.eu>
Dear Christophe,
在 2021/3/23 14:33, Christophe Leroy 写道:
>
>
> Le 23/03/2021 à 07:21, heying (H) a écrit :
>> Dear Christophe,
>>
>>
>> 在 2021/3/18 10:28, heying (H) 写道:
>>>
>>> 在 2021/3/17 19:16, Christophe Leroy 写道:
>>>>
>>>>
>>>> Le 17/03/2021 à 11:34, He Ying a écrit :
>>>>> We found these warnings in arch/powerpc/kernel/time.c as follows:
>>>>> warning: symbol 'decrementer_max' was not declared. Should it be
>>>>> static?
>>>>> warning: symbol 'rtc_lock' was not declared. Should it be static?
>>>>> warning: symbol 'dtl_consumer' was not declared. Should it be static?
>>>>>
>>>>> Declare 'decrementer_max' in arch/powerpc/include/asm/time.h. And
>>>>> include
>>>>> proper header in which 'rtc_lock' is declared. Move 'dtl_consumer'
>>>>> definition behind "include <asm/dtl.h>" because 'dtl_consumer' is
>>>>> declared
>>>>> there.
>>>>>
>>>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>>>> Signed-off-by: He Ying <heying24@huawei.com>
>>>>> ---
>>>>> arch/powerpc/include/asm/time.h | 1 +
>>>>> arch/powerpc/kernel/time.c | 7 +++----
>>>>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/arch/powerpc/include/asm/time.h
>>>>> b/arch/powerpc/include/asm/time.h
>>>>> index 8dd3cdb25338..2cd2b50bedda 100644
>>>>> --- a/arch/powerpc/include/asm/time.h
>>>>> +++ b/arch/powerpc/include/asm/time.h
>>>>> @@ -22,6 +22,7 @@ extern unsigned long tb_ticks_per_jiffy;
>>>>> extern unsigned long tb_ticks_per_usec;
>>>>> extern unsigned long tb_ticks_per_sec;
>>>>> extern struct clock_event_device decrementer_clockevent;
>>>>> +extern u64 decrementer_max;
>>>>> extern void generic_calibrate_decr(void);
>>>>> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
>>>>> index b67d93a609a2..409967713ca6 100644
>>>>> --- a/arch/powerpc/kernel/time.c
>>>>> +++ b/arch/powerpc/kernel/time.c
>>>>> @@ -55,6 +55,7 @@
>>>>> #include <linux/sched/cputime.h>
>>>>> #include <linux/sched/clock.h>
>>>>> #include <linux/processor.h>
>>>>> +#include <linux/mc146818rtc.h>
>>>>
>>>> I don't think that's the good place. It has no link to powerpc, it
>>>> is only by chance that it has the same name.
>>>>
>>>> As rtc_lock is defined in powerpc time.c, I think you should
>>>> declare it in powerpc asm/time.h
>>>
>>> My first thought was the same as yours. I tried to add declaration
>>> in powerpc asm/time.h, but got a compiling error:
>>>
>>> drivers/rtc/rtc-vr41xx.c:75:24: error: static declaration of
>>> ‘rtc_lock’ follows non-static declaration
>>> static DEFINE_SPINLOCK(rtc_lock);
>>>
>>> In file included from ./arch/powerpc/include/asm/delay.h:7:0,
>>> from ./arch/powerpc/include/asm/io.h:33,
>>> from ./include/linux/io.h:13,
>>> from drivers/rtc/rtc-vr41xx.c:11:
>>> ./arch/powerpc/include/asm/time.h:25:19: note: previous declaration
>>> of ‘rtc_lock’ was here
>>> extern spinlock_t rtc_lock;
>>>
>>> There's a conflict. Perhaps I can rename it in
>>> drivers/rtc/rtc-vr41xx.c.
>>>
>>>
>>> But I find an existing declaration in linux/mc146818rtc.h and
>>> there's only one definition for 'rtc_lock' in powerpc.
>>>
>>> There's some includes of mc146818rtc.h in powperc. I wonder they
>>> point to the same thing. But I'm not very sure
>>>
>>> because the header's name looks a bit strange.
>>
>> How about including mc146818rtc.h in powperpc kernel/time.c? May I
>> have your opinions please?
>>
>
> As I said, mc146818rtc.h is not related to powerpc, and if it works
> that's just chance, and there is no certainty that it will still work
> in the future.
>
> If you can't find a clean solution, it is better to leave the warning.
OK. I see. Thanks for you relpy. I'll try to find some other better way.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 19/41] KVM: PPC: Book3S HV P9: Stop handling hcalls in real-mode in the P9 path
From: Cédric Le Goater @ 2021-03-23 7:26 UTC (permalink / raw)
To: Nicholas Piggin, Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <1616436906.owrt3o4wh1.astroid@bobo.none>
On 3/22/21 7:22 PM, Nicholas Piggin wrote:
> Excerpts from Cédric Le Goater's message of March 23, 2021 2:01 am:
>> On 3/22/21 2:15 PM, Nicholas Piggin wrote:
>>> Excerpts from Alexey Kardashevskiy's message of March 22, 2021 5:30 pm:
>>>>
>>>>
>>>> On 06/03/2021 02:06, Nicholas Piggin wrote:
>>>>> In the interest of minimising the amount of code that is run in>>> "real-mode", don't handle hcalls in real mode in the P9 path.
>>>>>
>>>>> POWER8 and earlier are much more expensive to exit from HV real mode
>>>>> and switch to host mode, because on those processors HV interrupts get
>>>>> to the hypervisor with the MMU off, and the other threads in the core
>>>>> need to be pulled out of the guest, and SLBs all need to be saved,
>>>>> ERATs invalidated, and host SLB reloaded before the MMU is re-enabled
>>>>> in host mode. Hash guests also require a lot of hcalls to run. The
>>>>> XICS interrupt controller requires hcalls to run.
>>>>>
>>>>> By contrast, POWER9 has independent thread switching, and in radix mode
>>>>> the hypervisor is already in a host virtual memory mode when the HV
>>>>> interrupt is taken. Radix + xive guests don't need hcalls to handle
>>>>> interrupts or manage translations.
>>
>> Do we need to handle the host-is-a-P9-without-xive case ?
>
> I'm not sure really. Is there an intention for OPAL to be able to
> provide a fallback layer in the worst case?
yes. OPAL has a XICS-on-XIVE emulation for P9, implemented for bringup,
and it still boots, XICS guest can run. P10 doesn't have it though.
> Maybe microwatt grows HV capability before XIVE?
I don't know if we should develop the same XIVE logic for microwatt.
It's awfully complex and we have the XICS interface which works already.
>>>>> So it's much less important to handle hcalls in real mode in P9.
>>>>
>>>> So acde25726bc6034b (which added if(kvm_is_radix(vcpu->kvm))return
>>>> H_TOO_HARD) can be reverted, pretty much?
>>>
>>> Yes. Although that calls attention to the fact I missed doing
>>> a P9 h_random handler in this patch. I'll fix that, then I think
>>> acde2572 could be reverted entirely.
>>>
>>> [...]
>>>
>>>>> } else {
>>>>> kvmppc_xive_push_vcpu(vcpu);
>>>>> trap = kvmhv_load_hv_regs_and_go(vcpu, time_limit, lpcr);
>>>>> - kvmppc_xive_pull_vcpu(vcpu);
>>>>> + /* H_CEDE has to be handled now, not later */
>>>>> + /* XICS hcalls must be handled before xive is pulled */
>>>>> + if (trap == BOOK3S_INTERRUPT_SYSCALL &&
>>>>> + !(vcpu->arch.shregs.msr & MSR_PR)) {
>>>>> + unsigned long req = kvmppc_get_gpr(vcpu, 3);
>>>>>
>>>>> + if (req == H_CEDE) {
>>>>> + kvmppc_cede(vcpu);
>>>>> + kvmppc_xive_cede_vcpu(vcpu); /* may un-cede */
>>>>> + kvmppc_set_gpr(vcpu, 3, 0);
>>>>> + trap = 0;
>>>>> + }
>>>>> + if (req == H_EOI || req == H_CPPR ||
>>>>
>>>> else if (req == H_EOI ... ?
>>>
>>> Hummm, sure.
>>
>> you could integrate the H_CEDE in the switch statement below.
>
> Below is in a different file just for the emulation calls.
>
>>>
>>> [...]
>>>
>>>>> +void kvmppc_xive_cede_vcpu(struct kvm_vcpu *vcpu)
>>>>> +{
>>>>> + void __iomem *esc_vaddr = (void __iomem *)vcpu->arch.xive_esc_vaddr;
>>>>> +
>>>>> + if (!esc_vaddr)
>>>>> + return;
>>>>> +
>>>>> + /* we are using XIVE with single escalation */
>>>>> +
>>>>> + if (vcpu->arch.xive_esc_on) {
>>>>> + /*
>>>>> + * If we still have a pending escalation, abort the cede,
>>>>> + * and we must set PQ to 10 rather than 00 so that we don't
>>>>> + * potentially end up with two entries for the escalation
>>>>> + * interrupt in the XIVE interrupt queue. In that case
>>>>> + * we also don't want to set xive_esc_on to 1 here in
>>>>> + * case we race with xive_esc_irq().
>>>>> + */
>>>>> + vcpu->arch.ceded = 0;
>>>>> + /*
>>>>> + * The escalation interrupts are special as we don't EOI them.
>>>>> + * There is no need to use the load-after-store ordering offset
>>>>> + * to set PQ to 10 as we won't use StoreEOI.
>>>>> + */
>>>>> + __raw_readq(esc_vaddr + XIVE_ESB_SET_PQ_10);
>>>>> + } else {
>>>>> + vcpu->arch.xive_esc_on = true;
>>>>> + mb();
>>>>> + __raw_readq(esc_vaddr + XIVE_ESB_SET_PQ_00);
>>>>> + }
>>>>> + mb();
>>>>
>>>>
>>>> Uff. Thanks for cut-n-pasting the comments, helped a lot to match this c
>>>> to that asm!
>>>
>>> Glad it helped.
>>>>> +}
>>
>> I had to do the PowerNV models in QEMU to start understanding that stuff ...
>>
>>>>> +EXPORT_SYMBOL_GPL(kvmppc_xive_cede_vcpu);
>>>>> +
>>>>> /*
>>>>> * This is a simple trigger for a generic XIVE IRQ. This must
>>>>> * only be called for interrupts that support a trigger page
>>>>> @@ -2106,6 +2140,32 @@ static int kvmppc_xive_create(struct kvm_device *dev, u32 type)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +int kvmppc_xive_xics_hcall(struct kvm_vcpu *vcpu, u32 req)
>>>>> +{
>>>>> + struct kvmppc_vcore *vc = vcpu->arch.vcore;
>>>>
>>>>
>>>> Can a XIVE enabled guest issue these hcalls? Don't we want if
>>>> (!kvmppc_xics_enabled(vcpu)) and
>>>> if (xics_on_xive()) here, as kvmppc_rm_h_xirr() have? Some of these
>>>> hcalls do write to XIVE registers but some seem to change
>>>> kvmppc_xive_vcpu. Thanks,
>>>
>>> Yes I think you're right, good catch. I'm not completely sure about all
>>> the xive and xics modes but a guest certainly can make any kind of hcall
>>> it likes and we have to sanity check it.
>>
>> Yes.
>>
>>> We want to take the hcall here (in replacement of the real mode hcalls)
>>> with the same condition. So it would be:
>>>
>>> if (!kvmppc_xics_enabled(vcpu))
>>> return H_TOO_HARD;
>>
>> Yes.
>>
>> This test covers the case in which a vCPU does XICS hcalls without QEMU
>> having connected the vCPU to a XICS ICP. The ICP is the KVM XICS device
>> on P8 or XICS-on-XIVE on P9. It catches QEMU errors when the interrupt
>> mode is negotiated, we don't want the OS to do XICS hcalls after having
>> negotiated the XIVE interrupt mode.
>
> Okay.
>
>> It's different for the XIVE hcalls (when running under XICS) because they
>> are all handled in QEMU.
>
> XIVE guest hcalls running on XICS host?
What I meant is that in anycase, XICS or XIVE host, the XIVE hcalls are
trapped in KVM but always handled in QEMU. So We don't need to check
anything in KVM, as QEMU will take care of it.
( It also make the implementation cleaner since the hcall frontend is in
one place. )
C.
>>> if (!xics_on_xive())
>>> return H_TOO_HARD;
>>
>> I understand that this code is only called on P9 and with translation on.
>
> Yes.
>
>> On P9, we could have xics_on_xive() == 0 if XIVE is disabled at compile
>> time or with "xive=off" at boot time. But guests should be supported.
>> I don't see a reason to restrict the support even if these scenarios
>> are rather unusual if not very rare.
>>
>> on P10, it's the same but since we don't have the XICS emulation layer
>> in OPAL, the host will be pretty useless. We don't care.
>>
>> Since we are trying to handle hcalls, this is L0 and it can not be called
>> for nested guests, which would be another case of xics_on_xive() == 0.
>> We don't care either.
>
> Okay so no xics_on_xive() test. I'll change that.
>
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH 1/1] powerpc/iommu: Enable remaining IOMMU Pagesizes present in LoPAR
From: Alexey Kardashevskiy @ 2021-03-23 7:41 UTC (permalink / raw)
To: Leonardo Bras, Michael Ellerman, Benjamin Herrenschmidt,
Paul Mackerras, Christophe Leroy, Joel Stanley, brking
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <20210322190943.715368-1-leobras.c@gmail.com>
On 23/03/2021 06:09, Leonardo Bras wrote:
> According to LoPAR, ibm,query-pe-dma-window output named "IO Page Sizes"
> will let the OS know all possible pagesizes that can be used for creating a
> new DDW.
>
> Currently Linux will only try using 3 of the 8 available options:
> 4K, 64K and 16M. According to LoPAR, Hypervisor may also offer 32M, 64M,
> 128M, 256M and 16G.
>
> Enabling bigger pages would be interesting for direct mapping systems
> with a lot of RAM, while using less TCE entries.
> > Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
> ---
> arch/powerpc/include/asm/iommu.h | 8 ++++++++
> arch/powerpc/platforms/pseries/iommu.c | 28 +++++++++++++++++++-------
> 2 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> index deef7c94d7b6..c170048b7a1b 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -19,6 +19,14 @@
> #include <asm/pci-bridge.h>
> #include <asm/asm-const.h>
>
> +#define IOMMU_PAGE_SHIFT_16G 34
> +#define IOMMU_PAGE_SHIFT_256M 28
> +#define IOMMU_PAGE_SHIFT_128M 27
> +#define IOMMU_PAGE_SHIFT_64M 26
> +#define IOMMU_PAGE_SHIFT_32M 25
> +#define IOMMU_PAGE_SHIFT_16M 24
> +#define IOMMU_PAGE_SHIFT_64K 16
These are not very descriptive, these are just normal shifts, could be
as simple as __builtin_ctz(SZ_4K) (gcc will optimize this) and so on.
OTOH the PAPR page sizes need macros as they are the ones which are
weird and screaming for macros.
I'd steal/rework spapr_page_mask_to_query_mask() from QEMU. Thanks,
> +
> #define IOMMU_PAGE_SHIFT_4K 12
> #define IOMMU_PAGE_SIZE_4K (ASM_CONST(1) << IOMMU_PAGE_SHIFT_4K)
> #define IOMMU_PAGE_MASK_4K (~((1 << IOMMU_PAGE_SHIFT_4K) - 1))
> diff --git a/arch/powerpc/platforms/pseries/iommu.c b/arch/powerpc/platforms/pseries/iommu.c
> index 9fc5217f0c8e..02958e80aa91 100644
> --- a/arch/powerpc/platforms/pseries/iommu.c
> +++ b/arch/powerpc/platforms/pseries/iommu.c
> @@ -1099,6 +1099,24 @@ static void reset_dma_window(struct pci_dev *dev, struct device_node *par_dn)
> ret);
> }
>
> +/* Returns page shift based on "IO Page Sizes" output at ibm,query-pe-dma-window. SeeL LoPAR */
> +static int iommu_get_page_shift(u32 query_page_size)
> +{
> + const int shift[] = {IOMMU_PAGE_SHIFT_4K, IOMMU_PAGE_SHIFT_64K, IOMMU_PAGE_SHIFT_16M,
> + IOMMU_PAGE_SHIFT_32M, IOMMU_PAGE_SHIFT_64M, IOMMU_PAGE_SHIFT_128M,
> + IOMMU_PAGE_SHIFT_256M, IOMMU_PAGE_SHIFT_16G};
> + int i = ARRAY_SIZE(shift) - 1;
> +
> + /* Looks for the largest page size supported */
> + for (; i >= 0; i--) {
> + if (query_page_size & (1 << i))
> + return shift[i];
> + }
> +
> + /* No valid page size found. */
> + return 0;
> +}
> +
> /*
> * If the PE supports dynamic dma windows, and there is space for a table
> * that can map all pages in a linear offset, then setup such a table,
> @@ -1206,13 +1224,9 @@ static u64 enable_ddw(struct pci_dev *dev, struct device_node *pdn)
> goto out_failed;
> }
> }
> - if (query.page_size & 4) {
> - page_shift = 24; /* 16MB */
> - } else if (query.page_size & 2) {
> - page_shift = 16; /* 64kB */
> - } else if (query.page_size & 1) {
> - page_shift = 12; /* 4kB */
> - } else {
> +
> + page_shift = iommu_get_page_shift(query.page_size);
> + if (!page_shift) {
> dev_dbg(&dev->dev, "no supported direct page size in mask %x",
> query.page_size);
> goto out_failed;
>
--
Alexey
^ permalink raw reply
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
From: Shivaprasad G Bhat @ 2021-03-23 7:53 UTC (permalink / raw)
To: David Gibson
Cc: xiaoguangrong.eric, mst, aneesh.kumar, linux-nvdimm, qemu-devel,
kvm-ppc, Greg Kurz, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev
In-Reply-To: <20210208062122.GA40668@yekko.fritz.box>
Hi David,
Sorry about the delay.
On 2/8/21 11:51 AM, David Gibson wrote:
> On Tue, Jan 19, 2021 at 12:40:31PM +0530, Shivaprasad G Bhat wrote:
>> Thanks for the comments!
>>
>>
>> On 12/28/20 2:08 PM, David Gibson wrote:
>>
>>> On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
>> ...
>>>> The overall idea looks good but I think you should consider using
>>>> a thread pool to implement it. See below.
>>> I am not convinced, however. Specifically, attaching this to the DRC
>>> doesn't make sense to me. We're adding exactly one DRC related async
>>> hcall, and I can't really see much call for another one. We could
>>> have other async hcalls - indeed we already have one for HPT resizing
>>> - but attaching this to DRCs doesn't help for those.
>> The semantics of the hcall made me think, if this is going to be
>> re-usable for future if implemented at DRC level.
> It would only be re-usable for operations that are actually connected
> to DRCs. It doesn't seem to me particularly likely that we'll ever
> have more asynchronous hcalls that are also associated with DRCs.
Okay
>> Other option
>> is to move the async-hcall-state/list into the NVDIMMState structure
>> in include/hw/mem/nvdimm.h and handle it with machine->nvdimms_state
>> at a global level.
> I'm ok with either of two options:
>
> A) Implement this ad-hoc for this specific case, making whatever
> simplifications you can based on this specific case.
I am simplifying it to nvdimm use-case alone and limiting the scope.
> B) Implement a general mechanism for async hcalls that is *not* tied
> to DRCs. Then use that for the existing H_RESIZE_HPT_PREPARE call as
> well as this new one.
>
>> Hope you are okay with using the pool based approach that Greg
> Honestly a thread pool seems like it might be overkill for this
> application.
I think its appropriate here as that is what is being done by virtio-pmem
too for flush requests. The aio infrastructure simplifies lot of the
thread handling usage. Please suggest if you think there are better ways.
I am sending the next version addressing all the comments from you and Greg.
Thanks,
Shivaprasad
^ permalink raw reply
* Re: [PATCH v4 04/46] KVM: PPC: Book3S HV: Prevent radix guests from setting LPCR[TC]
From: Alexey Kardashevskiy @ 2021-03-23 8:36 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210323010305.1045293-5-npiggin@gmail.com>
On 23/03/2021 12:02, Nicholas Piggin wrote:
> This bit only applies to hash partitions.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/kvm/book3s_hv.c | 6 ++++++
> arch/powerpc/kvm/book3s_hv_nested.c | 3 +--
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index c5de7e3f22b6..1ffb0902e779 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1645,6 +1645,12 @@ static int kvm_arch_vcpu_ioctl_set_sregs_hv(struct kvm_vcpu *vcpu,
> */
> unsigned long kvmppc_filter_lpcr_hv(struct kvmppc_vcore *vc, unsigned long lpcr)
> {
> + struct kvm *kvm = vc->kvm;
> +
> + /* LPCR_TC only applies to HPT guests */
> + if (kvm_is_radix(kvm))
> + lpcr &= ~LPCR_TC;
> +
> /* On POWER8 and above, userspace can modify AIL */
> if (!cpu_has_feature(CPU_FTR_ARCH_207S))
> lpcr &= ~LPCR_AIL;
> diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
> index f7b441b3eb17..851e3f527eb2 100644
> --- a/arch/powerpc/kvm/book3s_hv_nested.c
> +++ b/arch/powerpc/kvm/book3s_hv_nested.c
> @@ -140,8 +140,7 @@ static void sanitise_hv_regs(struct kvm_vcpu *vcpu, struct hv_guest_state *hr)
> /*
> * Don't let L1 change LPCR bits for the L2 except these:
> */
> - mask = LPCR_DPFD | LPCR_ILE | LPCR_TC | LPCR_AIL | LPCR_LD |
> - LPCR_LPES | LPCR_MER;
> + mask = LPCR_DPFD | LPCR_ILE | LPCR_AIL | LPCR_LD | LPCR_LPES | LPCR_MER;
> hr->lpcr = kvmppc_filter_lpcr_hv(vc,
> (vc->lpcr & ~mask) | (hr->lpcr & mask));
>
>
--
Alexey
^ permalink raw reply
* Re: [PATCH v4 22/46] KVM: PPC: Book3S HV P9: Stop handling hcalls in real-mode in the P9 path
From: Alexey Kardashevskiy @ 2021-03-23 9:02 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210323010305.1045293-23-npiggin@gmail.com>
On 23/03/2021 12:02, Nicholas Piggin wrote:
> In the interest of minimising the amount of code that is run in
> "real-mode", don't handle hcalls in real mode in the P9 path.
>
> POWER8 and earlier are much more expensive to exit from HV real mode
> and switch to host mode, because on those processors HV interrupts get
> to the hypervisor with the MMU off, and the other threads in the core
> need to be pulled out of the guest, and SLBs all need to be saved,
> ERATs invalidated, and host SLB reloaded before the MMU is re-enabled
> in host mode. Hash guests also require a lot of hcalls to run. The
> XICS interrupt controller requires hcalls to run.
>
> By contrast, POWER9 has independent thread switching, and in radix mode
> the hypervisor is already in a host virtual memory mode when the HV
> interrupt is taken. Radix + xive guests don't need hcalls to handle
> interrupts or manage translations.
>
> So it's much less important to handle hcalls in real mode in P9.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/include/asm/kvm_ppc.h | 5 ++
> arch/powerpc/kvm/book3s_hv.c | 57 ++++++++++++++++----
> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 5 ++
> arch/powerpc/kvm/book3s_xive.c | 70 +++++++++++++++++++++++++
> 4 files changed, 127 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index 73b1ca5a6471..db6646c2ade2 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -607,6 +607,7 @@ extern void kvmppc_free_pimap(struct kvm *kvm);
> extern int kvmppc_xics_rm_complete(struct kvm_vcpu *vcpu, u32 hcall);
> extern void kvmppc_xics_free_icp(struct kvm_vcpu *vcpu);
> extern int kvmppc_xics_hcall(struct kvm_vcpu *vcpu, u32 cmd);
> +extern int kvmppc_xive_xics_hcall(struct kvm_vcpu *vcpu, u32 req);
> extern u64 kvmppc_xics_get_icp(struct kvm_vcpu *vcpu);
> extern int kvmppc_xics_set_icp(struct kvm_vcpu *vcpu, u64 icpval);
> extern int kvmppc_xics_connect_vcpu(struct kvm_device *dev,
> @@ -639,6 +640,8 @@ static inline int kvmppc_xics_enabled(struct kvm_vcpu *vcpu)
> static inline void kvmppc_xics_free_icp(struct kvm_vcpu *vcpu) { }
> static inline int kvmppc_xics_hcall(struct kvm_vcpu *vcpu, u32 cmd)
> { return 0; }
> +static inline int kvmppc_xive_xics_hcall(struct kvm_vcpu *vcpu, u32 req)
> + { return 0; }
> #endif
>
> #ifdef CONFIG_KVM_XIVE
> @@ -673,6 +676,7 @@ extern int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 irq,
> int level, bool line_status);
> extern void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu);
> extern void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu);
> +extern void kvmppc_xive_cede_vcpu(struct kvm_vcpu *vcpu);
>
> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
> {
> @@ -714,6 +718,7 @@ static inline int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 ir
> int level, bool line_status) { return -ENODEV; }
> static inline void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu) { }
> static inline void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu) { }
> +static inline void kvmppc_xive_cede_vcpu(struct kvm_vcpu *vcpu) { }
>
> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
> { return 0; }
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index fa7614c37e08..17739aaee3d8 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -1142,12 +1142,13 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
> }
>
> /*
> - * Handle H_CEDE in the nested virtualization case where we haven't
> - * called the real-mode hcall handlers in book3s_hv_rmhandlers.S.
> + * Handle H_CEDE in the P9 path where we don't call the real-mode hcall
> + * handlers in book3s_hv_rmhandlers.S.
> + *
> * This has to be done early, not in kvmppc_pseries_do_hcall(), so
> * that the cede logic in kvmppc_run_single_vcpu() works properly.
> */
> -static void kvmppc_nested_cede(struct kvm_vcpu *vcpu)
> +static void kvmppc_cede(struct kvm_vcpu *vcpu)
> {
> vcpu->arch.shregs.msr |= MSR_EE;
> vcpu->arch.ceded = 1;
> @@ -1403,9 +1404,15 @@ static int kvmppc_handle_exit_hv(struct kvm_vcpu *vcpu,
> /* hcall - punt to userspace */
> int i;
>
> - /* hypercall with MSR_PR has already been handled in rmode,
> - * and never reaches here.
> - */
> + if (unlikely(vcpu->arch.shregs.msr & MSR_PR)) {
> + /*
> + * Guest userspace executed sc 1, reflect it back as a
> + * privileged program check interrupt.
> + */
> + kvmppc_core_queue_program(vcpu, SRR1_PROGPRIV);
> + r = RESUME_GUEST;
> + break;
> + }
>
> run->papr_hcall.nr = kvmppc_get_gpr(vcpu, 3);
> for (i = 0; i < 9; ++i)
> @@ -3663,6 +3670,12 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
> return trap;
> }
>
> +static inline bool hcall_is_xics(unsigned long req)
> +{
> + return (req == H_EOI || req == H_CPPR || req == H_IPI ||
> + req == H_IPOLL || req == H_XIRR || req == H_XIRR_X);
Do not need braces :)
> +}
> +
> /*
> * Virtual-mode guest entry for POWER9 and later when the host and
> * guest are both using the radix MMU. The LPIDR has already been set.
> @@ -3774,15 +3787,36 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
> /* H_CEDE has to be handled now, not later */
> if (trap == BOOK3S_INTERRUPT_SYSCALL && !vcpu->arch.nested &&
> kvmppc_get_gpr(vcpu, 3) == H_CEDE) {
> - kvmppc_nested_cede(vcpu);
> + kvmppc_cede(vcpu);
> kvmppc_set_gpr(vcpu, 3, 0);
> trap = 0;
> }
> } else {
> kvmppc_xive_push_vcpu(vcpu);
> trap = kvmhv_load_hv_regs_and_go(vcpu, time_limit, lpcr);
> + if (trap == BOOK3S_INTERRUPT_SYSCALL && !vcpu->arch.nested &&
> + !(vcpu->arch.shregs.msr & MSR_PR)) {
> + unsigned long req = kvmppc_get_gpr(vcpu, 3);
> +
> + /* H_CEDE has to be handled now, not later */
> + if (req == H_CEDE) {
> + kvmppc_cede(vcpu);
> + kvmppc_xive_cede_vcpu(vcpu); /* may un-cede */
> + kvmppc_set_gpr(vcpu, 3, 0);
> + trap = 0;
> +
> + /* XICS hcalls must be handled before xive is pulled */
> + } else if (hcall_is_xics(req)) {
> + int ret;
> +
> + ret = kvmppc_xive_xics_hcall(vcpu, req);
> + if (ret != H_TOO_HARD) {
> + kvmppc_set_gpr(vcpu, 3, ret);
> + trap = 0;
> + }
> + }
> + }
> kvmppc_xive_pull_vcpu(vcpu);
> -
> }
>
> vcpu->arch.slb_max = 0;
> @@ -4442,8 +4476,11 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
> else
> r = kvmppc_run_vcpu(vcpu);
>
> - if (run->exit_reason == KVM_EXIT_PAPR_HCALL &&
> - !(vcpu->arch.shregs.msr & MSR_PR)) {
> + if (run->exit_reason == KVM_EXIT_PAPR_HCALL) {
> + if (WARN_ON_ONCE(vcpu->arch.shregs.msr & MSR_PR)) {
> + r = RESUME_GUEST;
> + continue;
> + }
> trace_kvm_hcall_enter(vcpu);
> r = kvmppc_pseries_do_hcall(vcpu);
> trace_kvm_hcall_exit(vcpu, r);
> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> index c11597f815e4..2d0d14ed1d92 100644
> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
> @@ -1397,9 +1397,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
> mr r4,r9
> bge fast_guest_return
> 2:
> + /* If we came in through the P9 short path, no real mode hcalls */
> + lwz r0, STACK_SLOT_SHORT_PATH(r1)
> + cmpwi r0, 0
> + bne no_try_real
btw is mmu on at this point? or it gets enabled by rfid at the end of
guest_exit_short_path?
anyway,
Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> /* See if this is an hcall we can handle in real mode */
> cmpwi r12,BOOK3S_INTERRUPT_SYSCALL
> beq hcall_try_real_mode
> +no_try_real:
>
> /* Hypervisor doorbell - exit only if host IPI flag set */
> cmpwi r12, BOOK3S_INTERRUPT_H_DOORBELL
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index 741bf1f4387a..dcc07ceaf5ca 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -158,6 +158,40 @@ void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu)
> }
> EXPORT_SYMBOL_GPL(kvmppc_xive_pull_vcpu);
>
> +void kvmppc_xive_cede_vcpu(struct kvm_vcpu *vcpu)
> +{
> + void __iomem *esc_vaddr = (void __iomem *)vcpu->arch.xive_esc_vaddr;
> +
> + if (!esc_vaddr)
> + return;
> +
> + /* we are using XIVE with single escalation */
> +
> + if (vcpu->arch.xive_esc_on) {
> + /*
> + * If we still have a pending escalation, abort the cede,
> + * and we must set PQ to 10 rather than 00 so that we don't
> + * potentially end up with two entries for the escalation
> + * interrupt in the XIVE interrupt queue. In that case
> + * we also don't want to set xive_esc_on to 1 here in
> + * case we race with xive_esc_irq().
> + */
> + vcpu->arch.ceded = 0;
> + /*
> + * The escalation interrupts are special as we don't EOI them.
> + * There is no need to use the load-after-store ordering offset
> + * to set PQ to 10 as we won't use StoreEOI.
> + */
> + __raw_readq(esc_vaddr + XIVE_ESB_SET_PQ_10);
> + } else {
> + vcpu->arch.xive_esc_on = true;
> + mb();
> + __raw_readq(esc_vaddr + XIVE_ESB_SET_PQ_00);
> + }
> + mb();
> +}
> +EXPORT_SYMBOL_GPL(kvmppc_xive_cede_vcpu);
> +
> /*
> * This is a simple trigger for a generic XIVE IRQ. This must
> * only be called for interrupts that support a trigger page
> @@ -2106,6 +2140,42 @@ static int kvmppc_xive_create(struct kvm_device *dev, u32 type)
> return 0;
> }
>
> +int kvmppc_xive_xics_hcall(struct kvm_vcpu *vcpu, u32 req)
> +{
> + struct kvmppc_vcore *vc = vcpu->arch.vcore;
> +
> + /*
> + * This test covers the case in which a vCPU does XICS hcalls without
> + * QEMU having connected the vCPU to a XICS ICP. The ICP is the KVM
> + * XICS device on P8 or XICS-on-XIVE on P9. It catches QEMU errors when
> + * the interrupt mode is negotiated, we don't want the OS to do XICS
> + * hcalls after having negotiated the XIVE interrupt mode.
> + */
> + if (!kvmppc_xics_enabled(vcpu))
> + return H_TOO_HARD;
> +
> + switch (req) {
> + case H_XIRR:
> + return xive_vm_h_xirr(vcpu);
> + case H_CPPR:
> + return xive_vm_h_cppr(vcpu, kvmppc_get_gpr(vcpu, 4));
> + case H_EOI:
> + return xive_vm_h_eoi(vcpu, kvmppc_get_gpr(vcpu, 4));
> + case H_IPI:
> + return xive_vm_h_ipi(vcpu, kvmppc_get_gpr(vcpu, 4),
> + kvmppc_get_gpr(vcpu, 5));
> + case H_IPOLL:
> + return xive_vm_h_ipoll(vcpu, kvmppc_get_gpr(vcpu, 4));
> + case H_XIRR_X:
> + xive_vm_h_xirr(vcpu);
> + kvmppc_set_gpr(vcpu, 5, get_tb() + vc->tb_offset);
> + return H_SUCCESS;
> + }
> +
> + return H_UNSUPPORTED;
> +}
> +EXPORT_SYMBOL_GPL(kvmppc_xive_xics_hcall);
> +
> int kvmppc_xive_debug_show_queues(struct seq_file *m, struct kvm_vcpu *vcpu)
> {
> struct kvmppc_xive_vcpu *xc = vcpu->arch.xive_vcpu;
>
--
Alexey
^ permalink raw reply
* [PATCH v2 -next] powerpc: kernel/time.c - cleanup warnings
From: He Ying @ 2021-03-23 9:12 UTC (permalink / raw)
To: mpe, benh, paulus, a.zummo, alexandre.belloni, christophe.leroy,
npiggin, msuchanek, heying24, tglx, peterz, geert+renesas,
kernelfans, frederic
Cc: linux-rtc, linuxppc-dev, linux-kernel
We found these warnings in arch/powerpc/kernel/time.c as follows:
warning: symbol 'decrementer_max' was not declared. Should it be static?
warning: symbol 'rtc_lock' was not declared. Should it be static?
warning: symbol 'dtl_consumer' was not declared. Should it be static?
Declare 'decrementer_max' and 'rtc_lock' in powerpc asm/time.h.
Rename 'rtc_lock' in drviers/rtc/rtc-vr41xx.c to 'vr41xx_rtc_lock' to
avoid the conflict with the variable in powerpc asm/time.h.
Move 'dtl_consumer' definition behind "include <asm/dtl.h>" because it
is declared there.
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: He Ying <heying24@huawei.com>
---
v2:
- Instead of including linux/mc146818rtc.h in powerpc kernel/time.c, declare
rtc_lock in powerpc asm/time.h.
arch/powerpc/include/asm/time.h | 3 +++
arch/powerpc/kernel/time.c | 6 ++----
drivers/rtc/rtc-vr41xx.c | 22 +++++++++++-----------
3 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/include/asm/time.h b/arch/powerpc/include/asm/time.h
index 8dd3cdb25338..64a3ef0b4270 100644
--- a/arch/powerpc/include/asm/time.h
+++ b/arch/powerpc/include/asm/time.h
@@ -12,6 +12,7 @@
#ifdef __KERNEL__
#include <linux/types.h>
#include <linux/percpu.h>
+#include <linux/spinlock.h>
#include <asm/processor.h>
#include <asm/cpu_has_feature.h>
@@ -22,6 +23,8 @@ extern unsigned long tb_ticks_per_jiffy;
extern unsigned long tb_ticks_per_usec;
extern unsigned long tb_ticks_per_sec;
extern struct clock_event_device decrementer_clockevent;
+extern u64 decrementer_max;
+extern spinlock_t rtc_lock;
extern void generic_calibrate_decr(void);
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index b67d93a609a2..60b6ac7d3685 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -150,10 +150,6 @@ bool tb_invalid;
u64 __cputime_usec_factor;
EXPORT_SYMBOL(__cputime_usec_factor);
-#ifdef CONFIG_PPC_SPLPAR
-void (*dtl_consumer)(struct dtl_entry *, u64);
-#endif
-
static void calc_cputime_factors(void)
{
struct div_result res;
@@ -179,6 +175,8 @@ static inline unsigned long read_spurr(unsigned long tb)
#include <asm/dtl.h>
+void (*dtl_consumer)(struct dtl_entry *, u64);
+
/*
* Scan the dispatch trace log and count up the stolen time.
* Should be called with interrupts disabled.
diff --git a/drivers/rtc/rtc-vr41xx.c b/drivers/rtc/rtc-vr41xx.c
index 5a9f9ad86d32..cc31db058197 100644
--- a/drivers/rtc/rtc-vr41xx.c
+++ b/drivers/rtc/rtc-vr41xx.c
@@ -72,7 +72,7 @@ static void __iomem *rtc2_base;
static unsigned long epoch = 1970; /* Jan 1 1970 00:00:00 */
-static DEFINE_SPINLOCK(rtc_lock);
+static DEFINE_SPINLOCK(vr41xx_rtc_lock);
static char rtc_name[] = "RTC";
static unsigned long periodic_count;
static unsigned int alarm_enabled;
@@ -101,13 +101,13 @@ static inline time64_t read_elapsed_second(void)
static inline void write_elapsed_second(time64_t sec)
{
- spin_lock_irq(&rtc_lock);
+ spin_lock_irq(&vr41xx_rtc_lock);
rtc1_write(ETIMELREG, (uint16_t)(sec << 15));
rtc1_write(ETIMEMREG, (uint16_t)(sec >> 1));
rtc1_write(ETIMEHREG, (uint16_t)(sec >> 17));
- spin_unlock_irq(&rtc_lock);
+ spin_unlock_irq(&vr41xx_rtc_lock);
}
static int vr41xx_rtc_read_time(struct device *dev, struct rtc_time *time)
@@ -139,14 +139,14 @@ static int vr41xx_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
unsigned long low, mid, high;
struct rtc_time *time = &wkalrm->time;
- spin_lock_irq(&rtc_lock);
+ spin_lock_irq(&vr41xx_rtc_lock);
low = rtc1_read(ECMPLREG);
mid = rtc1_read(ECMPMREG);
high = rtc1_read(ECMPHREG);
wkalrm->enabled = alarm_enabled;
- spin_unlock_irq(&rtc_lock);
+ spin_unlock_irq(&vr41xx_rtc_lock);
rtc_time64_to_tm((high << 17) | (mid << 1) | (low >> 15), time);
@@ -159,7 +159,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
alarm_sec = rtc_tm_to_time64(&wkalrm->time);
- spin_lock_irq(&rtc_lock);
+ spin_lock_irq(&vr41xx_rtc_lock);
if (alarm_enabled)
disable_irq(aie_irq);
@@ -173,7 +173,7 @@ static int vr41xx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *wkalrm)
alarm_enabled = wkalrm->enabled;
- spin_unlock_irq(&rtc_lock);
+ spin_unlock_irq(&vr41xx_rtc_lock);
return 0;
}
@@ -202,7 +202,7 @@ static int vr41xx_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long
static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
{
- spin_lock_irq(&rtc_lock);
+ spin_lock_irq(&vr41xx_rtc_lock);
if (enabled) {
if (!alarm_enabled) {
enable_irq(aie_irq);
@@ -214,7 +214,7 @@ static int vr41xx_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
alarm_enabled = 0;
}
}
- spin_unlock_irq(&rtc_lock);
+ spin_unlock_irq(&vr41xx_rtc_lock);
return 0;
}
@@ -296,7 +296,7 @@ static int rtc_probe(struct platform_device *pdev)
rtc->range_max = (1ULL << 33) - 1;
rtc->max_user_freq = MAX_PERIODIC_RATE;
- spin_lock_irq(&rtc_lock);
+ spin_lock_irq(&vr41xx_rtc_lock);
rtc1_write(ECMPLREG, 0);
rtc1_write(ECMPMREG, 0);
@@ -304,7 +304,7 @@ static int rtc_probe(struct platform_device *pdev)
rtc1_write(RTCL1LREG, 0);
rtc1_write(RTCL1HREG, 0);
- spin_unlock_irq(&rtc_lock);
+ spin_unlock_irq(&vr41xx_rtc_lock);
aie_irq = platform_get_irq(pdev, 0);
if (aie_irq <= 0) {
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v4 22/46] KVM: PPC: Book3S HV P9: Stop handling hcalls in real-mode in the P9 path
From: Nicholas Piggin @ 2021-03-23 9:16 UTC (permalink / raw)
To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <6901d698-f3d8-024b-3aa1-47b157bbd57d@ozlabs.ru>
Excerpts from Alexey Kardashevskiy's message of March 23, 2021 7:02 pm:
>
>
> On 23/03/2021 12:02, Nicholas Piggin wrote:
>> In the interest of minimising the amount of code that is run in
>> "real-mode", don't handle hcalls in real mode in the P9 path.
>>
>> POWER8 and earlier are much more expensive to exit from HV real mode
>> and switch to host mode, because on those processors HV interrupts get
>> to the hypervisor with the MMU off, and the other threads in the core
>> need to be pulled out of the guest, and SLBs all need to be saved,
>> ERATs invalidated, and host SLB reloaded before the MMU is re-enabled
>> in host mode. Hash guests also require a lot of hcalls to run. The
>> XICS interrupt controller requires hcalls to run.
>>
>> By contrast, POWER9 has independent thread switching, and in radix mode
>> the hypervisor is already in a host virtual memory mode when the HV
>> interrupt is taken. Radix + xive guests don't need hcalls to handle
>> interrupts or manage translations.
>>
>> So it's much less important to handle hcalls in real mode in P9.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/kvm_ppc.h | 5 ++
>> arch/powerpc/kvm/book3s_hv.c | 57 ++++++++++++++++----
>> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 5 ++
>> arch/powerpc/kvm/book3s_xive.c | 70 +++++++++++++++++++++++++
>> 4 files changed, 127 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
>> index 73b1ca5a6471..db6646c2ade2 100644
>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>> @@ -607,6 +607,7 @@ extern void kvmppc_free_pimap(struct kvm *kvm);
>> extern int kvmppc_xics_rm_complete(struct kvm_vcpu *vcpu, u32 hcall);
>> extern void kvmppc_xics_free_icp(struct kvm_vcpu *vcpu);
>> extern int kvmppc_xics_hcall(struct kvm_vcpu *vcpu, u32 cmd);
>> +extern int kvmppc_xive_xics_hcall(struct kvm_vcpu *vcpu, u32 req);
>> extern u64 kvmppc_xics_get_icp(struct kvm_vcpu *vcpu);
>> extern int kvmppc_xics_set_icp(struct kvm_vcpu *vcpu, u64 icpval);
>> extern int kvmppc_xics_connect_vcpu(struct kvm_device *dev,
>> @@ -639,6 +640,8 @@ static inline int kvmppc_xics_enabled(struct kvm_vcpu *vcpu)
>> static inline void kvmppc_xics_free_icp(struct kvm_vcpu *vcpu) { }
>> static inline int kvmppc_xics_hcall(struct kvm_vcpu *vcpu, u32 cmd)
>> { return 0; }
>> +static inline int kvmppc_xive_xics_hcall(struct kvm_vcpu *vcpu, u32 req)
>> + { return 0; }
>> #endif
>>
>> #ifdef CONFIG_KVM_XIVE
>> @@ -673,6 +676,7 @@ extern int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 irq,
>> int level, bool line_status);
>> extern void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu);
>> extern void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu);
>> +extern void kvmppc_xive_cede_vcpu(struct kvm_vcpu *vcpu);
>>
>> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
>> {
>> @@ -714,6 +718,7 @@ static inline int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 ir
>> int level, bool line_status) { return -ENODEV; }
>> static inline void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu) { }
>> static inline void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu) { }
>> +static inline void kvmppc_xive_cede_vcpu(struct kvm_vcpu *vcpu) { }
>>
>> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
>> { return 0; }
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index fa7614c37e08..17739aaee3d8 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -1142,12 +1142,13 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
>> }
>>
>> /*
>> - * Handle H_CEDE in the nested virtualization case where we haven't
>> - * called the real-mode hcall handlers in book3s_hv_rmhandlers.S.
>> + * Handle H_CEDE in the P9 path where we don't call the real-mode hcall
>> + * handlers in book3s_hv_rmhandlers.S.
>> + *
>> * This has to be done early, not in kvmppc_pseries_do_hcall(), so
>> * that the cede logic in kvmppc_run_single_vcpu() works properly.
>> */
>> -static void kvmppc_nested_cede(struct kvm_vcpu *vcpu)
>> +static void kvmppc_cede(struct kvm_vcpu *vcpu)
>> {
>> vcpu->arch.shregs.msr |= MSR_EE;
>> vcpu->arch.ceded = 1;
>> @@ -1403,9 +1404,15 @@ static int kvmppc_handle_exit_hv(struct kvm_vcpu *vcpu,
>> /* hcall - punt to userspace */
>> int i;
>>
>> - /* hypercall with MSR_PR has already been handled in rmode,
>> - * and never reaches here.
>> - */
>> + if (unlikely(vcpu->arch.shregs.msr & MSR_PR)) {
>> + /*
>> + * Guest userspace executed sc 1, reflect it back as a
>> + * privileged program check interrupt.
>> + */
>> + kvmppc_core_queue_program(vcpu, SRR1_PROGPRIV);
>> + r = RESUME_GUEST;
>> + break;
>> + }
>>
>> run->papr_hcall.nr = kvmppc_get_gpr(vcpu, 3);
>> for (i = 0; i < 9; ++i)
>> @@ -3663,6 +3670,12 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>> return trap;
>> }
>>
>> +static inline bool hcall_is_xics(unsigned long req)
>> +{
>> + return (req == H_EOI || req == H_CPPR || req == H_IPI ||
>> + req == H_IPOLL || req == H_XIRR || req == H_XIRR_X);
>
> Do not need braces :)
>
>
>> +}
>> +
>> /*
>> * Virtual-mode guest entry for POWER9 and later when the host and
>> * guest are both using the radix MMU. The LPIDR has already been set.
>> @@ -3774,15 +3787,36 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>> /* H_CEDE has to be handled now, not later */
>> if (trap == BOOK3S_INTERRUPT_SYSCALL && !vcpu->arch.nested &&
>> kvmppc_get_gpr(vcpu, 3) == H_CEDE) {
>> - kvmppc_nested_cede(vcpu);
>> + kvmppc_cede(vcpu);
>> kvmppc_set_gpr(vcpu, 3, 0);
>> trap = 0;
>> }
>> } else {
>> kvmppc_xive_push_vcpu(vcpu);
>> trap = kvmhv_load_hv_regs_and_go(vcpu, time_limit, lpcr);
>> + if (trap == BOOK3S_INTERRUPT_SYSCALL && !vcpu->arch.nested &&
>> + !(vcpu->arch.shregs.msr & MSR_PR)) {
>> + unsigned long req = kvmppc_get_gpr(vcpu, 3);
>> +
>> + /* H_CEDE has to be handled now, not later */
>> + if (req == H_CEDE) {
>> + kvmppc_cede(vcpu);
>> + kvmppc_xive_cede_vcpu(vcpu); /* may un-cede */
>> + kvmppc_set_gpr(vcpu, 3, 0);
>> + trap = 0;
>> +
>> + /* XICS hcalls must be handled before xive is pulled */
>> + } else if (hcall_is_xics(req)) {
>> + int ret;
>> +
>> + ret = kvmppc_xive_xics_hcall(vcpu, req);
>> + if (ret != H_TOO_HARD) {
>> + kvmppc_set_gpr(vcpu, 3, ret);
>> + trap = 0;
>> + }
>> + }
>> + }
>> kvmppc_xive_pull_vcpu(vcpu);
>> -
>> }
>>
>> vcpu->arch.slb_max = 0;
>> @@ -4442,8 +4476,11 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
>> else
>> r = kvmppc_run_vcpu(vcpu);
>>
>> - if (run->exit_reason == KVM_EXIT_PAPR_HCALL &&
>> - !(vcpu->arch.shregs.msr & MSR_PR)) {
>> + if (run->exit_reason == KVM_EXIT_PAPR_HCALL) {
>> + if (WARN_ON_ONCE(vcpu->arch.shregs.msr & MSR_PR)) {
>> + r = RESUME_GUEST;
>> + continue;
>> + }
>> trace_kvm_hcall_enter(vcpu);
>> r = kvmppc_pseries_do_hcall(vcpu);
>> trace_kvm_hcall_exit(vcpu, r);
>> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> index c11597f815e4..2d0d14ed1d92 100644
>> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>> @@ -1397,9 +1397,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> mr r4,r9
>> bge fast_guest_return
>> 2:
>> + /* If we came in through the P9 short path, no real mode hcalls */
>> + lwz r0, STACK_SLOT_SHORT_PATH(r1)
>> + cmpwi r0, 0
>> + bne no_try_real
>
>
> btw is mmu on at this point? or it gets enabled by rfid at the end of
> guest_exit_short_path?
Hash guest it's off. Radix guest it can be on or off depending on the
interrupt type and MSR and LPCR[AIL] values.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v4 22/46] KVM: PPC: Book3S HV P9: Stop handling hcalls in real-mode in the P9 path
From: Alexey Kardashevskiy @ 2021-03-23 9:24 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <1616490842.v369xyk7do.astroid@bobo.none>
On 23/03/2021 20:16, Nicholas Piggin wrote:
> Excerpts from Alexey Kardashevskiy's message of March 23, 2021 7:02 pm:
>>
>>
>> On 23/03/2021 12:02, Nicholas Piggin wrote:
>>> In the interest of minimising the amount of code that is run in
>>> "real-mode", don't handle hcalls in real mode in the P9 path.
>>>
>>> POWER8 and earlier are much more expensive to exit from HV real mode
>>> and switch to host mode, because on those processors HV interrupts get
>>> to the hypervisor with the MMU off, and the other threads in the core
>>> need to be pulled out of the guest, and SLBs all need to be saved,
>>> ERATs invalidated, and host SLB reloaded before the MMU is re-enabled
>>> in host mode. Hash guests also require a lot of hcalls to run. The
>>> XICS interrupt controller requires hcalls to run.
>>>
>>> By contrast, POWER9 has independent thread switching, and in radix mode
>>> the hypervisor is already in a host virtual memory mode when the HV
>>> interrupt is taken. Radix + xive guests don't need hcalls to handle
>>> interrupts or manage translations.
>>>
>>> So it's much less important to handle hcalls in real mode in P9.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> arch/powerpc/include/asm/kvm_ppc.h | 5 ++
>>> arch/powerpc/kvm/book3s_hv.c | 57 ++++++++++++++++----
>>> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 5 ++
>>> arch/powerpc/kvm/book3s_xive.c | 70 +++++++++++++++++++++++++
>>> 4 files changed, 127 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
>>> index 73b1ca5a6471..db6646c2ade2 100644
>>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>>> @@ -607,6 +607,7 @@ extern void kvmppc_free_pimap(struct kvm *kvm);
>>> extern int kvmppc_xics_rm_complete(struct kvm_vcpu *vcpu, u32 hcall);
>>> extern void kvmppc_xics_free_icp(struct kvm_vcpu *vcpu);
>>> extern int kvmppc_xics_hcall(struct kvm_vcpu *vcpu, u32 cmd);
>>> +extern int kvmppc_xive_xics_hcall(struct kvm_vcpu *vcpu, u32 req);
>>> extern u64 kvmppc_xics_get_icp(struct kvm_vcpu *vcpu);
>>> extern int kvmppc_xics_set_icp(struct kvm_vcpu *vcpu, u64 icpval);
>>> extern int kvmppc_xics_connect_vcpu(struct kvm_device *dev,
>>> @@ -639,6 +640,8 @@ static inline int kvmppc_xics_enabled(struct kvm_vcpu *vcpu)
>>> static inline void kvmppc_xics_free_icp(struct kvm_vcpu *vcpu) { }
>>> static inline int kvmppc_xics_hcall(struct kvm_vcpu *vcpu, u32 cmd)
>>> { return 0; }
>>> +static inline int kvmppc_xive_xics_hcall(struct kvm_vcpu *vcpu, u32 req)
>>> + { return 0; }
>>> #endif
>>>
>>> #ifdef CONFIG_KVM_XIVE
>>> @@ -673,6 +676,7 @@ extern int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 irq,
>>> int level, bool line_status);
>>> extern void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu);
>>> extern void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu);
>>> +extern void kvmppc_xive_cede_vcpu(struct kvm_vcpu *vcpu);
>>>
>>> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
>>> {
>>> @@ -714,6 +718,7 @@ static inline int kvmppc_xive_set_irq(struct kvm *kvm, int irq_source_id, u32 ir
>>> int level, bool line_status) { return -ENODEV; }
>>> static inline void kvmppc_xive_push_vcpu(struct kvm_vcpu *vcpu) { }
>>> static inline void kvmppc_xive_pull_vcpu(struct kvm_vcpu *vcpu) { }
>>> +static inline void kvmppc_xive_cede_vcpu(struct kvm_vcpu *vcpu) { }
>>>
>>> static inline int kvmppc_xive_enabled(struct kvm_vcpu *vcpu)
>>> { return 0; }
>>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>>> index fa7614c37e08..17739aaee3d8 100644
>>> --- a/arch/powerpc/kvm/book3s_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_hv.c
>>> @@ -1142,12 +1142,13 @@ int kvmppc_pseries_do_hcall(struct kvm_vcpu *vcpu)
>>> }
>>>
>>> /*
>>> - * Handle H_CEDE in the nested virtualization case where we haven't
>>> - * called the real-mode hcall handlers in book3s_hv_rmhandlers.S.
>>> + * Handle H_CEDE in the P9 path where we don't call the real-mode hcall
>>> + * handlers in book3s_hv_rmhandlers.S.
>>> + *
>>> * This has to be done early, not in kvmppc_pseries_do_hcall(), so
>>> * that the cede logic in kvmppc_run_single_vcpu() works properly.
>>> */
>>> -static void kvmppc_nested_cede(struct kvm_vcpu *vcpu)
>>> +static void kvmppc_cede(struct kvm_vcpu *vcpu)
>>> {
>>> vcpu->arch.shregs.msr |= MSR_EE;
>>> vcpu->arch.ceded = 1;
>>> @@ -1403,9 +1404,15 @@ static int kvmppc_handle_exit_hv(struct kvm_vcpu *vcpu,
>>> /* hcall - punt to userspace */
>>> int i;
>>>
>>> - /* hypercall with MSR_PR has already been handled in rmode,
>>> - * and never reaches here.
>>> - */
>>> + if (unlikely(vcpu->arch.shregs.msr & MSR_PR)) {
>>> + /*
>>> + * Guest userspace executed sc 1, reflect it back as a
>>> + * privileged program check interrupt.
>>> + */
>>> + kvmppc_core_queue_program(vcpu, SRR1_PROGPRIV);
>>> + r = RESUME_GUEST;
>>> + break;
>>> + }
>>>
>>> run->papr_hcall.nr = kvmppc_get_gpr(vcpu, 3);
>>> for (i = 0; i < 9; ++i)
>>> @@ -3663,6 +3670,12 @@ static int kvmhv_load_hv_regs_and_go(struct kvm_vcpu *vcpu, u64 time_limit,
>>> return trap;
>>> }
>>>
>>> +static inline bool hcall_is_xics(unsigned long req)
>>> +{
>>> + return (req == H_EOI || req == H_CPPR || req == H_IPI ||
>>> + req == H_IPOLL || req == H_XIRR || req == H_XIRR_X);
>>
>> Do not need braces :)
>>
>>
>>> +}
>>> +
>>> /*
>>> * Virtual-mode guest entry for POWER9 and later when the host and
>>> * guest are both using the radix MMU. The LPIDR has already been set.
>>> @@ -3774,15 +3787,36 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>>> /* H_CEDE has to be handled now, not later */
>>> if (trap == BOOK3S_INTERRUPT_SYSCALL && !vcpu->arch.nested &&
>>> kvmppc_get_gpr(vcpu, 3) == H_CEDE) {
>>> - kvmppc_nested_cede(vcpu);
>>> + kvmppc_cede(vcpu);
>>> kvmppc_set_gpr(vcpu, 3, 0);
>>> trap = 0;
>>> }
>>> } else {
>>> kvmppc_xive_push_vcpu(vcpu);
>>> trap = kvmhv_load_hv_regs_and_go(vcpu, time_limit, lpcr);
>>> + if (trap == BOOK3S_INTERRUPT_SYSCALL && !vcpu->arch.nested &&
>>> + !(vcpu->arch.shregs.msr & MSR_PR)) {
>>> + unsigned long req = kvmppc_get_gpr(vcpu, 3);
>>> +
>>> + /* H_CEDE has to be handled now, not later */
>>> + if (req == H_CEDE) {
>>> + kvmppc_cede(vcpu);
>>> + kvmppc_xive_cede_vcpu(vcpu); /* may un-cede */
>>> + kvmppc_set_gpr(vcpu, 3, 0);
>>> + trap = 0;
>>> +
>>> + /* XICS hcalls must be handled before xive is pulled */
>>> + } else if (hcall_is_xics(req)) {
>>> + int ret;
>>> +
>>> + ret = kvmppc_xive_xics_hcall(vcpu, req);
>>> + if (ret != H_TOO_HARD) {
>>> + kvmppc_set_gpr(vcpu, 3, ret);
>>> + trap = 0;
>>> + }
>>> + }
>>> + }
>>> kvmppc_xive_pull_vcpu(vcpu);
>>> -
>>> }
>>>
>>> vcpu->arch.slb_max = 0;
>>> @@ -4442,8 +4476,11 @@ static int kvmppc_vcpu_run_hv(struct kvm_vcpu *vcpu)
>>> else
>>> r = kvmppc_run_vcpu(vcpu);
>>>
>>> - if (run->exit_reason == KVM_EXIT_PAPR_HCALL &&
>>> - !(vcpu->arch.shregs.msr & MSR_PR)) {
>>> + if (run->exit_reason == KVM_EXIT_PAPR_HCALL) {
>>> + if (WARN_ON_ONCE(vcpu->arch.shregs.msr & MSR_PR)) {
>>> + r = RESUME_GUEST;
>>> + continue;
>>> + }
>>> trace_kvm_hcall_enter(vcpu);
>>> r = kvmppc_pseries_do_hcall(vcpu);
>>> trace_kvm_hcall_exit(vcpu, r);
>>> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>> index c11597f815e4..2d0d14ed1d92 100644
>>> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>> @@ -1397,9 +1397,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>>> mr r4,r9
>>> bge fast_guest_return
>>> 2:
>>> + /* If we came in through the P9 short path, no real mode hcalls */
>>> + lwz r0, STACK_SLOT_SHORT_PATH(r1)
>>> + cmpwi r0, 0
>>> + bne no_try_real
>>
>>
>> btw is mmu on at this point? or it gets enabled by rfid at the end of
>> guest_exit_short_path?
>
> Hash guest it's off. Radix guest it can be on or off depending on the
> interrupt type and MSR and LPCR[AIL] values.
What I meant was - what do we expect here on p9? mmu on? ^w^w^w^w^w^w^w^w^w
I just realized - it is radix so there is no problem with vmalloc
addresses in real mode as these do not use top 2 bits as on hash and the
exact mmu state is less important here. Cheers.
>
> Thanks,
> Nick
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 3/4] powerpc/rust: Add target.json for ppc64le
From: Miguel Ojeda @ 2021-03-23 9:28 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-kernel, rust-for-linux
In-Reply-To: <20210323032624.1039422-4-mpe@ellerman.id.au>
On Tue, Mar 23, 2021 at 4:27 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> ppc64le only for now. We'll eventually need to come up with some way to
> change the target.json that's used based on more than just $(ARCH).
Indeed, it is one reason I didn't tackle e.g. x86 32-bit, because I
wanted to figure out how to do the whole `target.json` cleanly (i.e.
likely have a script generate them on the fly), so I thought it was
better to wait post-RFC.
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH next v1 2/3] printk: remove safe buffers
From: Petr Mladek @ 2021-03-23 9:46 UTC (permalink / raw)
To: John Ogness
Cc: Rafael Aquini, Alexey Kardashevskiy, Paul Mackerras, Tiezhu Yang,
Peter Zijlstra, Yue Hu, Jordan Niethe, Kees Cook,
Paul E. McKenney, Alistair Popple, Guilherme G. Piccoli,
Nicholas Piggin, Steven Rostedt, Thomas Gleixner, kexec,
linux-kernel, Sergey Senozhatsky, Eric Biederman, Andrew Morton,
linuxppc-dev, Cédric Le Goater
In-Reply-To: <87ft0mg8a0.fsf@jogness.linutronix.de>
On Mon 2021-03-22 22:58:47, John Ogness wrote:
> On 2021-03-22, Petr Mladek <pmladek@suse.com> wrote:
> > On Mon 2021-03-22 12:16:15, John Ogness wrote:
> >> On 2021-03-21, Sergey Senozhatsky <senozhatsky@chromium.org> wrote:
> >> >> @@ -369,7 +70,10 @@ __printf(1, 0) int vprintk_func(const char *fmt, va_list args)
> >> >> * Use the main logbuf even in NMI. But avoid calling console
> >> >> * drivers that might have their own locks.
> >> >> */
> >> >> - if ((this_cpu_read(printk_context) & PRINTK_NMI_DIRECT_CONTEXT_MASK)) {
> >> >> + if (this_cpu_read(printk_context) &
> >> >> + (PRINTK_NMI_DIRECT_CONTEXT_MASK |
> >> >> + PRINTK_NMI_CONTEXT_MASK |
> >> >> + PRINTK_SAFE_CONTEXT_MASK)) {
> >> >
> >> But I suppose I could switch
> >> the 1 printk_nmi_direct_enter() user to printk_nmi_enter() so that
> >> PRINTK_NMI_DIRECT_CONTEXT_MASK can be removed now. I would do this in a
> >> 4th patch of the series.
> >
> > Yes, please unify the PRINTK_NMI_CONTEXT. One is enough.
>
> Agreed. (But I'll go even further. See below.)
>
> > I wonder if it would make sense to go even further at this stage.
> > What is possible?
> >
> > 1. We could get rid of printk_nmi_enter()/exit() and
> > PRINTK_NMI_CONTEXT completely already now. It is enough
> > to check in_nmi() in printk_func().
> >
>
> Agreed. in_nmi() within vprintk_emit() is enough to detect if the
> console code should be skipped:
>
> if (!in_sched && !in_nmi()) {
> ...
> }
Well, we also need to make sure that the irq work is scheduled to
call console later. We should keep this dicision in
printk_func(). I mean to replace the current
if (this_cpu_read(printk_context) &
(PRINTK_NMI_DIRECT_CONTEXT_MASK |
PRINTK_NMI_CONTEXT_MASK |
PRINTK_SAFE_CONTEXT_MASK)) {
with
/*
* Avoid calling console drivers in recursive printk()
* and in NMI context.
*/
if (this_cpu_read(printk_context) || in_nmi() {
That said, I am not sure how this fits your further rework.
I do not want to complicate it too much.
I am just afraid that the discussion about console rework might
take some time. And this would remove some complexity before we
started the more complicated or controversial changes.
> > 2. I thought about unifying printk_safe_enter()/exit() and
> > printk_enter()/exit(). They both count recursion with
> > IRQs disabled, have similar name. But they are used
> > different way.
> >
> > But better might be to rename printk_safe_enter()/exit() to
> > console_enter()/exit() or to printk_deferred_enter()/exit().
> > It would make more clear what it does now. And it might help
> > to better distinguish it from the new printk_enter()/exit().
> >
> > I am not sure if it is worth it.
>
> I am also not sure if it is worth the extra "noise" just to give the
> function a more appropriate name. The plan is to remove it completely
> soon anyway. My vote is to leave the name as it is.
OK, let's keep printk_safe() name. It was just an idea. I wrote it
primary to sort my thoughts.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH v4 22/46] KVM: PPC: Book3S HV P9: Stop handling hcalls in real-mode in the P9 path
From: Nicholas Piggin @ 2021-03-23 9:48 UTC (permalink / raw)
To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <994fb056-4445-4301-faca-b53394fb6b35@ozlabs.ru>
Excerpts from Alexey Kardashevskiy's message of March 23, 2021 7:24 pm:
>
>
> On 23/03/2021 20:16, Nicholas Piggin wrote:
>> Excerpts from Alexey Kardashevskiy's message of March 23, 2021 7:02 pm:
>>>
>>>
>>> On 23/03/2021 12:02, Nicholas Piggin wrote:
>>>> diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>>> index c11597f815e4..2d0d14ed1d92 100644
>>>> --- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>>> +++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
>>>> @@ -1397,9 +1397,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>>>> mr r4,r9
>>>> bge fast_guest_return
>>>> 2:
>>>> + /* If we came in through the P9 short path, no real mode hcalls */
>>>> + lwz r0, STACK_SLOT_SHORT_PATH(r1)
>>>> + cmpwi r0, 0
>>>> + bne no_try_real
>>>
>>>
>>> btw is mmu on at this point? or it gets enabled by rfid at the end of
>>> guest_exit_short_path?
>>
>> Hash guest it's off. Radix guest it can be on or off depending on the
>> interrupt type and MSR and LPCR[AIL] values.
>
> What I meant was - what do we expect here on p9? mmu on? ^w^w^w^w^w^w^w^w^w
P9 radix can be on or off. If the guest had MSR[IR] or MSR[DR] clear, or
if the guest is running AIL=0 mode, or if this is a machine check,
system reset, or HMI interrupt then the MMU will be off here.
> I just realized - it is radix so there is no problem with vmalloc
> addresses in real mode as these do not use top 2 bits as on hash and the
> exact mmu state is less important here. Cheers.
We still can't use vmalloc addresses in real mode on radix because they
don't translate with the page tables.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 0/4] Rust for Linux for ppc64le
From: Miguel Ojeda @ 2021-03-23 10:02 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, linux-kernel, rust-for-linux
In-Reply-To: <20210323032624.1039422-1-mpe@ellerman.id.au>
Hi Michael,
On Tue, Mar 23, 2021 at 4:27 AM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Hi all,
>
> Here's a first attempt at getting the kernel Rust support building on powerpc.
Thanks a *lot*! It is great to have more architectures rolling.
> It's powerpc64le only for now, as that's what I can easily test given the
> distros I have installed. Though powerpc and powerpc64 are also Tier 2 platforms
Even if it is just 64-bit, it is very good to have it!
> so in theory should work. Supporting those would require something more
> complicated than just pointing rustc at arch/$(ARCH)/rust/target.json.
Yeah, the arch/$(ARCH)/rust/target.json dance is a placeholder -- I
need to figure out how to do that more cleanly, likely generating them
on the fly.
> This is based on 832575d934a2 from the Rust-for-Linux tree. Anything newer gives
> me errors about symbol name lengths. I figured I'd send this anyway, as it seems
> like those errors are probably not powerpc specific.
Sure, feel free to send things even if they don't work completely.
I will take a look at the symbol name lengths -- I increased that
limit to 512 and added support for 2-byte lengths in the tables, but
perhaps something is missing. If I manage to make it work, I can add
ppc64le to our CI! :-)
> Michael Ellerman (4):
> rust: Export symbols in initialized data section
> rust: Add powerpc64 as a 64-bit target_arch in c_types.rs
> powerpc/rust: Add target.json for ppc64le
> rust: Enable for ppc64le
Regarding the development process: at least until the RFC we are
working with the usual GitHub PR workflow (for several reasons: having
a quick CI setup, getting new Rust developers on-board, having a list
of "issues", cross-reference with the Rust repo, etc.).
I can take patches from the list, of course, but since we are pre-RFC,
do you mind if they get rebased etc. through there?
Thanks again!
Cheers,
Miguel
^ permalink raw reply
* Re: [PATCH v4 28/46] KVM: PPC: Book3S HV P9: Reduce irq_work vs guest decrementer races
From: Alexey Kardashevskiy @ 2021-03-23 10:13 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <20210323010305.1045293-29-npiggin@gmail.com>
On 23/03/2021 12:02, Nicholas Piggin wrote:
> irq_work's use of the DEC SPR is racy with guest<->host switch and guest
> entry which flips the DEC interrupt to guest, which could lose a host
> work interrupt.
>
> This patch closes one race, and attempts to comment another class of
> races.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kvm/book3s_hv.c | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 1f38a0abc611..989a1ff5ad11 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -3745,6 +3745,18 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
> if (!(vcpu->arch.ctrl & 1))
> mtspr(SPRN_CTRLT, mfspr(SPRN_CTRLF) & ~1);
>
> + /*
> + * When setting DEC, we must always deal with irq_work_raise via NMI vs
> + * setting DEC. The problem occurs right as we switch into guest mode
> + * if a NMI hits and sets pending work and sets DEC, then that will
> + * apply to the guest and not bring us back to the host.
> + *
> + * irq_work_raise could check a flag (or possibly LPCR[HDICE] for
> + * example) and set HDEC to 1? That wouldn't solve the nested hv
> + * case which needs to abort the hcall or zero the time limit.
> + *
> + * XXX: Another day's problem.
> + */
> mtspr(SPRN_DEC, vcpu->arch.dec_expires - tb);
>
> if (kvmhv_on_pseries()) {
> @@ -3879,7 +3891,8 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
> vc->entry_exit_map = 0x101;
> vc->in_guest = 0;
>
> - mtspr(SPRN_DEC, local_paca->kvm_hstate.dec_expires - tb);
> + set_dec_or_work(local_paca->kvm_hstate.dec_expires - tb);
set_dec_or_work() will write local_paca->kvm_hstate.dec_expires - tb - 1
to SPRN_DEC which is not exactly the same, is this still alright?
I asked in v3 but it is probably lost :)
> +
> mtspr(SPRN_SPRG_VDSO_WRITE, local_paca->sprg_vdso);
>
> kvmhv_load_host_pmu();
>
--
Alexey
^ permalink raw reply
* [PATCH] sound:ppc: fix spelling typo of values
From: caizhichao @ 2021-03-23 8:55 UTC (permalink / raw)
To: geoff, perex, tiwai
Cc: alsa-devel, caizhichao, linux-kernel, paulus, linuxppc-dev
From: caizhichao <caizhichao@yulong.com>
vaules -> values
Signed-off-by: caizhichao <caizhichao@yulong.com>
---
sound/ppc/snd_ps3_reg.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/ppc/snd_ps3_reg.h b/sound/ppc/snd_ps3_reg.h
index 566a318..e2212b7 100644
--- a/sound/ppc/snd_ps3_reg.h
+++ b/sound/ppc/snd_ps3_reg.h
@@ -308,7 +308,7 @@
each interrupt in this register.
Writing 1b to a field containing 1b clears field and de-asserts interrupt.
Writing 0b to a field has no effect.
-Field vaules are the following:
+Field values are the following:
0 - Interrupt hasn't occurred.
1 - Interrupt has occurred.
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v4 28/46] KVM: PPC: Book3S HV P9: Reduce irq_work vs guest decrementer races
From: Nicholas Piggin @ 2021-03-23 10:36 UTC (permalink / raw)
To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <3ca0e504-70df-2a25-12af-a1addac842b6@ozlabs.ru>
Excerpts from Alexey Kardashevskiy's message of March 23, 2021 8:13 pm:
>
>
> On 23/03/2021 12:02, Nicholas Piggin wrote:
>> irq_work's use of the DEC SPR is racy with guest<->host switch and guest
>> entry which flips the DEC interrupt to guest, which could lose a host
>> work interrupt.
>>
>> This patch closes one race, and attempts to comment another class of
>> races.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/kvm/book3s_hv.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 1f38a0abc611..989a1ff5ad11 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -3745,6 +3745,18 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>> if (!(vcpu->arch.ctrl & 1))
>> mtspr(SPRN_CTRLT, mfspr(SPRN_CTRLF) & ~1);
>>
>> + /*
>> + * When setting DEC, we must always deal with irq_work_raise via NMI vs
>> + * setting DEC. The problem occurs right as we switch into guest mode
>> + * if a NMI hits and sets pending work and sets DEC, then that will
>> + * apply to the guest and not bring us back to the host.
>> + *
>> + * irq_work_raise could check a flag (or possibly LPCR[HDICE] for
>> + * example) and set HDEC to 1? That wouldn't solve the nested hv
>> + * case which needs to abort the hcall or zero the time limit.
>> + *
>> + * XXX: Another day's problem.
>> + */
>> mtspr(SPRN_DEC, vcpu->arch.dec_expires - tb);
>>
>> if (kvmhv_on_pseries()) {
>> @@ -3879,7 +3891,8 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>> vc->entry_exit_map = 0x101;
>> vc->in_guest = 0;
>>
>> - mtspr(SPRN_DEC, local_paca->kvm_hstate.dec_expires - tb);
>> + set_dec_or_work(local_paca->kvm_hstate.dec_expires - tb);
>
>
> set_dec_or_work() will write local_paca->kvm_hstate.dec_expires - tb - 1
> to SPRN_DEC which is not exactly the same, is this still alright?
>
> I asked in v3 but it is probably lost :)
Oh I did see that then forgot.
It will write dec_expires - tb, then it will write 1 if it found irq_work
was pending.
The change is intentional to fixes one of the lost irq_work races.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH next v1 2/3] printk: remove safe buffers
From: Petr Mladek @ 2021-03-23 10:47 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Peter Zijlstra, Paul Mackerras, Tiezhu Yang,
Rafael Aquini, Alexey Kardashevskiy, Yue Hu, Jordan Niethe,
Kees Cook, Paul E. McKenney, Alistair Popple,
Guilherme G. Piccoli, Nicholas Piggin, Steven Rostedt,
Thomas Gleixner, kexec, linux-kernel, Sergey Senozhatsky,
Eric Biederman, Andrew Morton, linuxppc-dev,
Cédric Le Goater
In-Reply-To: <20210316233326.10778-3-john.ogness@linutronix.de>
On Wed 2021-03-17 00:33:25, John Ogness wrote:
> With @logbuf_lock removed, the high level printk functions for
> storing messages are lockless. Messages can be stored from any
> context, so there is no need for the NMI and safe buffers anymore.
> Remove the NMI and safe buffers.
>
> Although the safe buffers are removed, the NMI and safe context
> tracking is still in place. In these contexts, store the message
> immediately but still use irq_work to defer the console printing.
>
> Since printk recursion tracking is in place, safe context tracking
> for most of printk is not needed. Remove it. Only safe context
> tracking relating to the console lock is left in place. This is
> because the console lock is needed for the actual printing.
I have two more questions after actually checking the entire patch.
See below.
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1084,7 +1069,6 @@ void __init setup_log_buf(int early)
> struct printk_record r;
> size_t new_descs_size;
> size_t new_infos_size;
> - unsigned long flags;
> char *new_log_buf;
> unsigned int free;
> u64 seq;
> @@ -1142,8 +1126,6 @@ void __init setup_log_buf(int early)
> new_descs, ilog2(new_descs_count),
> new_infos);
>
> - printk_safe_enter_irqsave(flags);
> -
> log_buf_len = new_log_buf_len;
> log_buf = new_log_buf;
> new_log_buf_len = 0;
> @@ -1159,8 +1141,6 @@ void __init setup_log_buf(int early)
> */
> prb = &printk_rb_dynamic;
>
> - printk_safe_exit_irqrestore(flags);
This will allow to add new messages from the IRQ context when we
are copying them to the new buffer. They might get lost in
the small race window.
Also the messages from NMI might get lost because they are not
longer stored in the per-CPU buffer.
A possible solution might be to do something like this:
prb_for_each_record(0, &printk_rb_static, seq, &r)
free -= add_to_rb(&printk_rb_dynamic, &r);
prb = &printk_rb_dynamic;
/*
* Copy the remaining messages that might have appeared
* from IRQ or NMI context after we ended copying and
* before we switched the buffers. They must be finalized
* because only one CPU is up at this stage.
*/
prb_for_each_record(seq, &printk_rb_static, seq, &r)
free -= add_to_rb(&printk_rb_dynamic, &r);
> -
> if (seq != prb_next_seq(&printk_rb_static)) {
> pr_err("dropped %llu messages\n",
> prb_next_seq(&printk_rb_static) - seq);
> @@ -2666,7 +2631,6 @@ void console_unlock(void)
> size_t ext_len = 0;
> size_t len;
>
> - printk_safe_enter_irqsave(flags);
> skip:
> if (!prb_read_valid(prb, console_seq, &r))
> break;
> @@ -2711,6 +2675,8 @@ void console_unlock(void)
> printk_time);
> console_seq++;
>
> + printk_safe_enter_irqsave(flags);
What is the purpose of the printk_safe context here, please?
I guess that you wanted to prevent calling console drivers
recursively. But it is already serialized by console_lock().
IMHO, the only risk is when manipulating console_sem->lock
or console_owner_lock. But they are already guarded by
printk_safe context, for example, in console_lock() or
console_lock_spinning_enable().
Do I miss something, please?
> +
> /*
> * While actively printing out messages, if another printk()
> * were to occur on another CPU, it may wait for this one to
> @@ -2745,8 +2711,6 @@ void console_unlock(void)
> * flush, no worries.
> */
> retry = prb_read_valid(prb, console_seq, NULL);
> - printk_safe_exit_irqrestore(flags);
> -
> if (retry && console_trylock())
> goto again;
> }
Heh, all these patches feels like stripping printk of an armour. I hope
that we trained it enough to be flexible and avoid any damage.
Best Regards,
Petr
^ permalink raw reply
* Re: [PATCH v4 28/46] KVM: PPC: Book3S HV P9: Reduce irq_work vs guest decrementer races
From: Nicholas Piggin @ 2021-03-23 11:15 UTC (permalink / raw)
To: Alexey Kardashevskiy, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <1616495617.6070udmp89.astroid@bobo.none>
Excerpts from Nicholas Piggin's message of March 23, 2021 8:36 pm:
> Excerpts from Alexey Kardashevskiy's message of March 23, 2021 8:13 pm:
>>
>>
>> On 23/03/2021 12:02, Nicholas Piggin wrote:
>>> irq_work's use of the DEC SPR is racy with guest<->host switch and guest
>>> entry which flips the DEC interrupt to guest, which could lose a host
>>> work interrupt.
>>>
>>> This patch closes one race, and attempts to comment another class of
>>> races.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> arch/powerpc/kvm/book3s_hv.c | 15 ++++++++++++++-
>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>>> index 1f38a0abc611..989a1ff5ad11 100644
>>> --- a/arch/powerpc/kvm/book3s_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_hv.c
>>> @@ -3745,6 +3745,18 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>>> if (!(vcpu->arch.ctrl & 1))
>>> mtspr(SPRN_CTRLT, mfspr(SPRN_CTRLF) & ~1);
>>>
>>> + /*
>>> + * When setting DEC, we must always deal with irq_work_raise via NMI vs
>>> + * setting DEC. The problem occurs right as we switch into guest mode
>>> + * if a NMI hits and sets pending work and sets DEC, then that will
>>> + * apply to the guest and not bring us back to the host.
>>> + *
>>> + * irq_work_raise could check a flag (or possibly LPCR[HDICE] for
>>> + * example) and set HDEC to 1? That wouldn't solve the nested hv
>>> + * case which needs to abort the hcall or zero the time limit.
>>> + *
>>> + * XXX: Another day's problem.
>>> + */
>>> mtspr(SPRN_DEC, vcpu->arch.dec_expires - tb);
>>>
>>> if (kvmhv_on_pseries()) {
>>> @@ -3879,7 +3891,8 @@ static int kvmhv_p9_guest_entry(struct kvm_vcpu *vcpu, u64 time_limit,
>>> vc->entry_exit_map = 0x101;
>>> vc->in_guest = 0;
>>>
>>> - mtspr(SPRN_DEC, local_paca->kvm_hstate.dec_expires - tb);
>>> + set_dec_or_work(local_paca->kvm_hstate.dec_expires - tb);
>>
>>
>> set_dec_or_work() will write local_paca->kvm_hstate.dec_expires - tb - 1
>> to SPRN_DEC which is not exactly the same, is this still alright?
>>
>> I asked in v3 but it is probably lost :)
>
> Oh I did see that then forgot.
>
> It will write dec_expires - tb, then it will write 1 if it found irq_work
> was pending.
Ah you were actually asking about set_dec writing val - 1. I totally
missed that.
Yes that was an unintentional change. This is the way timer.c code works
with respect to the decrementers_next_tb value, so it's probably better
to make them so it seems like it should be okay (and better to bring the
KVM code up to match timer code rather than be different or the other
way around). The difference should be noted in the changelog though.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 0/2] handle premature return from H_JOIN in pseries mobility code
From: Michael Ellerman @ 2021-03-23 12:02 UTC (permalink / raw)
To: linuxppc-dev, Nathan Lynch; +Cc: tyreld, brking, npiggin
In-Reply-To: <20210315080045.460331-1-nathanl@linux.ibm.com>
On Mon, 15 Mar 2021 03:00:43 -0500, Nathan Lynch wrote:
> pseries VMs in shared processor mode are susceptible to failed
> migrations becasue stray H_PRODs from the paravirt spinlock
> implementation can bump threads out of joining state before the
> suspend has occurred. Fix this by adding a small amount of shared
> state and ordering accesses to it with respect to H_PROD and H_JOIN.
>
> Nathan Lynch (2):
> powerpc/pseries/mobility: use struct for shared state
> powerpc/pseries/mobility: handle premature return from H_JOIN
>
> [...]
Applied to powerpc/fixes.
[1/2] powerpc/pseries/mobility: use struct for shared state
https://git.kernel.org/powerpc/c/e834df6cfc71d8e5ce2c27a0184145ea125c3f0f
[2/2] powerpc/pseries/mobility: handle premature return from H_JOIN
https://git.kernel.org/powerpc/c/274cb1ca2e7ce02cab56f5f4c61a74aeb566f931
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