* Re: [PATCH] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Nicholas Piggin @ 2018-04-24 7:31 UTC (permalink / raw)
To: Shilpasri G Bhat
Cc: rjw, viresh.kumar, benh, mpe, linux-pm, linuxppc-dev,
linux-kernel, ppaidipe, svaidy
In-Reply-To: <c88d51af-9eb8-6bb3-4201-8909cda0241b@linux.vnet.ibm.com>
On Tue, 24 Apr 2018 12:47:32 +0530
Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> wrote:
> Hi,
>
> On 04/24/2018 11:30 AM, Nicholas Piggin wrote:
> > On Tue, 24 Apr 2018 10:11:46 +0530
> > Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> wrote:
> >
> >> gpstate_timer_handler() uses synchronous smp_call to set the pstate
> >> on the requested core. This causes the below hard lockup:
> >>
> >> [c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
> >> [c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
> >> [c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
> >> [c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
> >> [c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
> >> [c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
> >> [c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
> >> [c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
> >> [c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
> >> [c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
> >> --- interrupt: 901 at doorbell_global_ipi+0x34/0x50
> >> LR = arch_send_call_function_ipi_mask+0x120/0x130
> >> [c000003fe566ba50] [c00000000004876c] arch_send_call_function_ipi_mask+0x4c/0x130 (unreliable)
> >> [c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
> >> [c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
> >> [c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
> >> [c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
> >> [c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
> >> [c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
> >> [c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
> >>
> >> Fix this by using the asynchronus smp_call in the timer interrupt handler.
> >> We don't have to wait in this handler until the pstates are changed on
> >> the core. This change will not have any impact on the global pstate
> >> ramp-down algorithm.
> >>
> >> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> >> Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
> >> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> >> ---
> >> drivers/cpufreq/powernv-cpufreq.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> >> index 0591874..7e0c752 100644
> >> --- a/drivers/cpufreq/powernv-cpufreq.c
> >> +++ b/drivers/cpufreq/powernv-cpufreq.c
> >> @@ -721,7 +721,7 @@ void gpstate_timer_handler(struct timer_list *t)
> >> spin_unlock(&gpstates->gpstate_lock);
> >>
> >> /* Timer may get migrated to a different cpu on cpu hot unplug */
> >> - smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
> >> + smp_call_function_any(policy->cpus, set_pstate, &freq_data, 0);
> >> }
> >>
> >> /*
> >
> > This can still deadlock because !wait case still ends up having to wait
> > if another !wait smp_call_function caller had previously used the
> > call single data for this cpu.
> >
> > If you go this way you would have to use smp_call_function_async, which
> > is more work.
> >
> > As a rule it would be better to avoid smp_call_function entirely if
> > possible. Can you ensure the timer is running on the right CPU? Use
> > add_timer_on and try again if the timer is on the wrong CPU, perhaps?
> >
>
> Yeah that is doable we can check for the cpu and re-queue it. We will only
> ramp-down slower in that case which is no harm.
Great, I'd be much happier avoiding that IPI. I guess it should happen
quite rarely that we have to queue on a different CPU. I would say just
do add_timer unless we have migrated to the wrong CPU, then do add_timer_on
in that case (it's a bit slower).
> (If the targeted core turns out to be offline then we will not queue the timer
> again as we would have already set the pstate to min in the cpu-down path.)
Something I noticed is that if we can not get the lock (trylock fails),
then the timer does not get queued again. Should it?
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Shilpasri G Bhat @ 2018-04-24 7:17 UTC (permalink / raw)
To: Nicholas Piggin
Cc: rjw, viresh.kumar, benh, mpe, linux-pm, linuxppc-dev,
linux-kernel, ppaidipe, svaidy
In-Reply-To: <20180424160034.6e9d2274@roar.ozlabs.ibm.com>
Hi,
On 04/24/2018 11:30 AM, Nicholas Piggin wrote:
> On Tue, 24 Apr 2018 10:11:46 +0530
> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> wrote:
>
>> gpstate_timer_handler() uses synchronous smp_call to set the pstate
>> on the requested core. This causes the below hard lockup:
>>
>> [c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
>> [c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
>> [c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
>> [c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
>> [c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
>> [c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
>> [c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
>> [c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
>> [c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
>> [c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
>> --- interrupt: 901 at doorbell_global_ipi+0x34/0x50
>> LR = arch_send_call_function_ipi_mask+0x120/0x130
>> [c000003fe566ba50] [c00000000004876c] arch_send_call_function_ipi_mask+0x4c/0x130 (unreliable)
>> [c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
>> [c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
>> [c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
>> [c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
>> [c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
>> [c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
>> [c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
>>
>> Fix this by using the asynchronus smp_call in the timer interrupt handler.
>> We don't have to wait in this handler until the pstates are changed on
>> the core. This change will not have any impact on the global pstate
>> ramp-down algorithm.
>>
>> Reported-by: Nicholas Piggin <npiggin@gmail.com>
>> Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>> ---
>> drivers/cpufreq/powernv-cpufreq.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
>> index 0591874..7e0c752 100644
>> --- a/drivers/cpufreq/powernv-cpufreq.c
>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>> @@ -721,7 +721,7 @@ void gpstate_timer_handler(struct timer_list *t)
>> spin_unlock(&gpstates->gpstate_lock);
>>
>> /* Timer may get migrated to a different cpu on cpu hot unplug */
>> - smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
>> + smp_call_function_any(policy->cpus, set_pstate, &freq_data, 0);
>> }
>>
>> /*
>
> This can still deadlock because !wait case still ends up having to wait
> if another !wait smp_call_function caller had previously used the
> call single data for this cpu.
>
> If you go this way you would have to use smp_call_function_async, which
> is more work.
>
> As a rule it would be better to avoid smp_call_function entirely if
> possible. Can you ensure the timer is running on the right CPU? Use
> add_timer_on and try again if the timer is on the wrong CPU, perhaps?
>
Yeah that is doable we can check for the cpu and re-queue it. We will only
ramp-down slower in that case which is no harm.
(If the targeted core turns out to be offline then we will not queue the timer
again as we would have already set the pstate to min in the cpu-down path.)
Thanks and Regards,
Shilpa
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Nicholas Piggin @ 2018-04-24 6:00 UTC (permalink / raw)
To: Shilpasri G Bhat
Cc: rjw, viresh.kumar, benh, mpe, linux-pm, linuxppc-dev,
linux-kernel, ppaidipe, svaidy
In-Reply-To: <1524544906-31512-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com>
On Tue, 24 Apr 2018 10:11:46 +0530
Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> wrote:
> gpstate_timer_handler() uses synchronous smp_call to set the pstate
> on the requested core. This causes the below hard lockup:
>
> [c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
> [c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
> [c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
> [c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
> [c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
> [c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
> [c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
> [c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
> [c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
> [c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
> --- interrupt: 901 at doorbell_global_ipi+0x34/0x50
> LR = arch_send_call_function_ipi_mask+0x120/0x130
> [c000003fe566ba50] [c00000000004876c] arch_send_call_function_ipi_mask+0x4c/0x130 (unreliable)
> [c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
> [c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
> [c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
> [c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
> [c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
> [c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
> [c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
>
> Fix this by using the asynchronus smp_call in the timer interrupt handler.
> We don't have to wait in this handler until the pstates are changed on
> the core. This change will not have any impact on the global pstate
> ramp-down algorithm.
>
> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
> drivers/cpufreq/powernv-cpufreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index 0591874..7e0c752 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -721,7 +721,7 @@ void gpstate_timer_handler(struct timer_list *t)
> spin_unlock(&gpstates->gpstate_lock);
>
> /* Timer may get migrated to a different cpu on cpu hot unplug */
> - smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
> + smp_call_function_any(policy->cpus, set_pstate, &freq_data, 0);
> }
>
> /*
This can still deadlock because !wait case still ends up having to wait
if another !wait smp_call_function caller had previously used the
call single data for this cpu.
If you go this way you would have to use smp_call_function_async, which
is more work.
As a rule it would be better to avoid smp_call_function entirely if
possible. Can you ensure the timer is running on the right CPU? Use
add_timer_on and try again if the timer is on the wrong CPU, perhaps?
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 1/6] powerpc/64s: Add barrier_nospec
From: Nicholas Piggin @ 2018-04-24 5:44 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, msuchanek, linux-kernel
In-Reply-To: <20180424041559.32410-1-mpe@ellerman.id.au>
On Tue, 24 Apr 2018 14:15:54 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:
> From: Michal Suchanek <msuchanek@suse.de>
>
> A no-op form of ori (or immediate of 0 into r31 and the result stored
> in r31) has been re-tasked as a speculation barrier. The instruction
> only acts as a barrier on newer machines with appropriate firmware
> support. On older CPUs it remains a harmless no-op.
>
> Implement barrier_nospec using this instruction.
>
> mpe: The semantics of the instruction are believed to be that it
> prevents execution of subsequent instructions until preceding branches
> have been fully resolved and are no longer executing speculatively.
> There is no further documentation available at this time.
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> mpe: Make it Book3S64 only, update comment & change log, add a
> memory clobber to the asm.
These all seem good to me. Thanks Michal.
We should (eventually) work on the module patching problem too.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 3/5] powerpc: use time64_t in read_persistent_clock
From: kbuild test robot @ 2018-04-24 5:29 UTC (permalink / raw)
To: Arnd Bergmann
Cc: kbuild-all, Michael Ellerman, Paul Mackerras,
Benjamin Herrenschmidt, linuxppc-dev, Arnd Bergmann,
Vitaly Bordug, Geoff Levand, linux-kernel
In-Reply-To: <20180423081114.1813726-3-arnd@arndb.de>
[-- Attachment #1: Type: text/plain, Size: 2623 bytes --]
Hi Arnd,
I love your patch! Yet something to improve:
[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.17-rc2 next-20180423]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/powerpc-always-enable-RTC_LIB/20180423-223504
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc64-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc64
All errors (new ones prefixed by >>):
arch/powerpc/platforms/maple/time.c: In function 'maple_get_boot_time':
>> arch/powerpc/platforms/maple/time.c:173:9: error: implicit declaration of function 'rtc_tm_to_time66'; did you mean 'rtc_tm_to_time64'? [-Werror=implicit-function-declaration]
return rtc_tm_to_time66(&tm);
^~~~~~~~~~~~~~~~
rtc_tm_to_time64
cc1: all warnings being treated as errors
vim +173 arch/powerpc/platforms/maple/time.c
139
140 time64_t __init maple_get_boot_time(void)
141 {
142 struct rtc_time tm;
143 struct device_node *rtcs;
144
145 rtcs = of_find_compatible_node(NULL, "rtc", "pnpPNP,b00");
146 if (rtcs) {
147 struct resource r;
148 if (of_address_to_resource(rtcs, 0, &r)) {
149 printk(KERN_EMERG "Maple: Unable to translate RTC"
150 " address\n");
151 goto bail;
152 }
153 if (!(r.flags & IORESOURCE_IO)) {
154 printk(KERN_EMERG "Maple: RTC address isn't PIO!\n");
155 goto bail;
156 }
157 maple_rtc_addr = r.start;
158 printk(KERN_INFO "Maple: Found RTC at IO 0x%x\n",
159 maple_rtc_addr);
160 }
161 bail:
162 if (maple_rtc_addr == 0) {
163 maple_rtc_addr = RTC_PORT(0); /* legacy address */
164 printk(KERN_INFO "Maple: No device node for RTC, assuming "
165 "legacy address (0x%x)\n", maple_rtc_addr);
166 }
167
168 rtc_iores.start = maple_rtc_addr;
169 rtc_iores.end = maple_rtc_addr + 7;
170 request_resource(&ioport_resource, &rtc_iores);
171
172 maple_get_rtc_time(&tm);
> 173 return rtc_tm_to_time66(&tm);
174 }
175
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23368 bytes --]
^ permalink raw reply
* Re: [PATCH] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Shilpasri G Bhat @ 2018-04-24 5:24 UTC (permalink / raw)
To: Stewart Smith, rjw, viresh.kumar
Cc: npiggin, benh, mpe, linux-pm, linuxppc-dev, linux-kernel,
ppaidipe, svaidy
In-Reply-To: <87y3hdb36b.fsf@linux.vnet.ibm.com>
Hi,
On 04/24/2018 10:40 AM, Stewart Smith wrote:
> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
>> gpstate_timer_handler() uses synchronous smp_call to set the pstate
>> on the requested core. This causes the below hard lockup:
>>
>> [c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
>> [c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
>> [c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
>> [c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
>> [c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
>> [c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
>> [c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
>> [c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
>> [c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
>> [c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
>> --- interrupt: 901 at doorbell_global_ipi+0x34/0x50
>> LR = arch_send_call_function_ipi_mask+0x120/0x130
>> [c000003fe566ba50] [c00000000004876c] arch_send_call_function_ipi_mask+0x4c/0x130 (unreliable)
>> [c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
>> [c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
>> [c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
>> [c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
>> [c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
>> [c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
>> [c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
>>
>> Fix this by using the asynchronus smp_call in the timer interrupt handler.
>> We don't have to wait in this handler until the pstates are changed on
>> the core. This change will not have any impact on the global pstate
>> ramp-down algorithm.
>>
>> Reported-by: Nicholas Piggin <npiggin@gmail.com>
>> Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>> ---
>> drivers/cpufreq/powernv-cpufreq.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
>> index 0591874..7e0c752 100644
>> --- a/drivers/cpufreq/powernv-cpufreq.c
>> +++ b/drivers/cpufreq/powernv-cpufreq.c
>> @@ -721,7 +721,7 @@ void gpstate_timer_handler(struct timer_list *t)
>> spin_unlock(&gpstates->gpstate_lock);
>>
>> /* Timer may get migrated to a different cpu on cpu hot unplug */
>> - smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
>> + smp_call_function_any(policy->cpus, set_pstate, &freq_data, 0);
>> }
>
> Should this have:
> Fixes: eaa2c3aeef83f
> and CC stable v4.7+ ?
>
Yeah this is required.
Fixes: eaa2c3aeef83 (cpufreq: powernv: Ramp-down global pstate slower than
local-pstate)
Thanks and Regards,
Shilpa
^ permalink raw reply
* Re: [PATCH] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Stewart Smith @ 2018-04-24 5:10 UTC (permalink / raw)
To: Shilpasri G Bhat, rjw, viresh.kumar
Cc: npiggin, benh, mpe, linux-pm, linuxppc-dev, linux-kernel,
ppaidipe, svaidy, Shilpasri G Bhat
In-Reply-To: <1524544906-31512-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com>
Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
> gpstate_timer_handler() uses synchronous smp_call to set the pstate
> on the requested core. This causes the below hard lockup:
>
> [c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
> [c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
> [c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
> [c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
> [c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
> [c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
> [c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
> [c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
> [c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
> [c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
> --- interrupt: 901 at doorbell_global_ipi+0x34/0x50
> LR = arch_send_call_function_ipi_mask+0x120/0x130
> [c000003fe566ba50] [c00000000004876c] arch_send_call_function_ipi_mask+0x4c/0x130 (unreliable)
> [c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
> [c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
> [c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
> [c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
> [c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
> [c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
> [c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
>
> Fix this by using the asynchronus smp_call in the timer interrupt handler.
> We don't have to wait in this handler until the pstates are changed on
> the core. This change will not have any impact on the global pstate
> ramp-down algorithm.
>
> Reported-by: Nicholas Piggin <npiggin@gmail.com>
> Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
> drivers/cpufreq/powernv-cpufreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> index 0591874..7e0c752 100644
> --- a/drivers/cpufreq/powernv-cpufreq.c
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -721,7 +721,7 @@ void gpstate_timer_handler(struct timer_list *t)
> spin_unlock(&gpstates->gpstate_lock);
>
> /* Timer may get migrated to a different cpu on cpu hot unplug */
> - smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
> + smp_call_function_any(policy->cpus, set_pstate, &freq_data, 0);
> }
Should this have:
Fixes: eaa2c3aeef83f
and CC stable v4.7+ ?
--
Stewart Smith
OPAL Architect, IBM.
^ permalink raw reply
* [PATCH] cpufreq: powernv: Fix the hardlockup by synchronus smp_call in timer interrupt
From: Shilpasri G Bhat @ 2018-04-24 4:41 UTC (permalink / raw)
To: rjw, viresh.kumar
Cc: npiggin, benh, mpe, linux-pm, linuxppc-dev, linux-kernel,
ppaidipe, svaidy, Shilpasri G Bhat
gpstate_timer_handler() uses synchronous smp_call to set the pstate
on the requested core. This causes the below hard lockup:
[c000003fe566b320] [c0000000001d5340] smp_call_function_single+0x110/0x180 (unreliable)
[c000003fe566b390] [c0000000001d55e0] smp_call_function_any+0x180/0x250
[c000003fe566b3f0] [c000000000acd3e8] gpstate_timer_handler+0x1e8/0x580
[c000003fe566b4a0] [c0000000001b46b0] call_timer_fn+0x50/0x1c0
[c000003fe566b520] [c0000000001b4958] expire_timers+0x138/0x1f0
[c000003fe566b590] [c0000000001b4bf8] run_timer_softirq+0x1e8/0x270
[c000003fe566b630] [c000000000d0d6c8] __do_softirq+0x158/0x3e4
[c000003fe566b710] [c000000000114be8] irq_exit+0xe8/0x120
[c000003fe566b730] [c000000000024d0c] timer_interrupt+0x9c/0xe0
[c000003fe566b760] [c000000000009014] decrementer_common+0x114/0x120
--- interrupt: 901 at doorbell_global_ipi+0x34/0x50
LR = arch_send_call_function_ipi_mask+0x120/0x130
[c000003fe566ba50] [c00000000004876c] arch_send_call_function_ipi_mask+0x4c/0x130 (unreliable)
[c000003fe566ba90] [c0000000001d59f0] smp_call_function_many+0x340/0x450
[c000003fe566bb00] [c000000000075f18] pmdp_invalidate+0x98/0xe0
[c000003fe566bb30] [c0000000003a1120] change_huge_pmd+0xe0/0x270
[c000003fe566bba0] [c000000000349278] change_protection_range+0xb88/0xe40
[c000003fe566bcf0] [c0000000003496c0] mprotect_fixup+0x140/0x340
[c000003fe566bdb0] [c000000000349a74] SyS_mprotect+0x1b4/0x350
[c000003fe566be30] [c00000000000b184] system_call+0x58/0x6c
Fix this by using the asynchronus smp_call in the timer interrupt handler.
We don't have to wait in this handler until the pstates are changed on
the core. This change will not have any impact on the global pstate
ramp-down algorithm.
Reported-by: Nicholas Piggin <npiggin@gmail.com>
Reported-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>
Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
drivers/cpufreq/powernv-cpufreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index 0591874..7e0c752 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -721,7 +721,7 @@ void gpstate_timer_handler(struct timer_list *t)
spin_unlock(&gpstates->gpstate_lock);
/* Timer may get migrated to a different cpu on cpu hot unplug */
- smp_call_function_any(policy->cpus, set_pstate, &freq_data, 1);
+ smp_call_function_any(policy->cpus, set_pstate, &freq_data, 0);
}
/*
--
1.8.3.1
^ permalink raw reply related
* [PATCH 6/6] powerpc/64: Use barrier_nospec in syscall entry
From: Michael Ellerman @ 2018-04-24 4:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin, msuchanek, linux-kernel
In-Reply-To: <20180424041559.32410-1-mpe@ellerman.id.au>
Our syscall entry is done in assembly so patch in an explicit
barrier_nospec.
Based on a patch by Michal Suchanek.
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
mpe: Move the barrier to immediately prior to the vulnerable load,
and add a comment trying to explain why. Drop the barrier from
syscall_dotrace, because that syscall number comes from the
kernel.
---
arch/powerpc/kernel/entry_64.S | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 51695608c68b..de30f9a34c0c 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -36,6 +36,7 @@
#include <asm/context_tracking.h>
#include <asm/tm.h>
#include <asm/ppc-opcode.h>
+#include <asm/barrier.h>
#include <asm/export.h>
#ifdef CONFIG_PPC_BOOK3S
#include <asm/exception-64s.h>
@@ -178,6 +179,15 @@ system_call: /* label this so stack traces look sane */
clrldi r8,r8,32
15:
slwi r0,r0,4
+
+ barrier_nospec_asm
+ /*
+ * Prevent the load of the handler below (based on the user-passed
+ * system call number) being speculatively executed until the test
+ * against NR_syscalls and branch to .Lsyscall_enosys above has
+ * committed.
+ */
+
ldx r12,r11,r0 /* Fetch system call handler [ptr] */
mtctr r12
bctrl /* Call handler */
--
2.14.1
^ permalink raw reply related
* [PATCH 5/6] powerpc: Use barrier_nospec in copy_from_user()
From: Michael Ellerman @ 2018-04-24 4:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin, msuchanek, linux-kernel
In-Reply-To: <20180424041559.32410-1-mpe@ellerman.id.au>
Based on the x86 commit doing the same.
See commit 304ec1b05031 ("x86/uaccess: Use __uaccess_begin_nospec()
and uaccess_try_nospec") and b3bbfb3fb5d2 ("x86: Introduce
__uaccess_begin_nospec() and uaccess_try_nospec") for more detail.
In all cases we are ordering the load from the potentially
user-controlled pointer vs a previous branch based on an access_ok()
check or similar.
Base on a patch from Michal Suchanek.
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/uaccess.h | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index a62ee663b2c8..6dc3d2eeea4a 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -252,6 +252,7 @@ do { \
__chk_user_ptr(ptr); \
if (!is_kernel_addr((unsigned long)__gu_addr)) \
might_fault(); \
+ barrier_nospec(); \
__get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
(x) = (__typeof__(*(ptr)))__gu_val; \
__gu_err; \
@@ -263,8 +264,10 @@ do { \
unsigned long __gu_val = 0; \
const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
might_fault(); \
- if (access_ok(VERIFY_READ, __gu_addr, (size))) \
+ if (access_ok(VERIFY_READ, __gu_addr, (size))) { \
+ barrier_nospec(); \
__get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
+ } \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
__gu_err; \
})
@@ -275,6 +278,7 @@ do { \
unsigned long __gu_val; \
const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \
__chk_user_ptr(ptr); \
+ barrier_nospec(); \
__get_user_size(__gu_val, __gu_addr, (size), __gu_err); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
__gu_err; \
@@ -302,15 +306,19 @@ static inline unsigned long raw_copy_from_user(void *to,
switch (n) {
case 1:
+ barrier_nospec();
__get_user_size(*(u8 *)to, from, 1, ret);
break;
case 2:
+ barrier_nospec();
__get_user_size(*(u16 *)to, from, 2, ret);
break;
case 4:
+ barrier_nospec();
__get_user_size(*(u32 *)to, from, 4, ret);
break;
case 8:
+ barrier_nospec();
__get_user_size(*(u64 *)to, from, 8, ret);
break;
}
@@ -318,6 +326,7 @@ static inline unsigned long raw_copy_from_user(void *to,
return 0;
}
+ barrier_nospec();
return __copy_tofrom_user((__force void __user *)to, from, n);
}
--
2.14.1
^ permalink raw reply related
* [PATCH 4/6] powerpc/64s: Enable barrier_nospec based on firmware settings
From: Michael Ellerman @ 2018-04-24 4:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin, msuchanek, linux-kernel
In-Reply-To: <20180424041559.32410-1-mpe@ellerman.id.au>
From: Michal Suchanek <msuchanek@suse.de>
Check what firmware told us and enable/disable the barrier_nospec as
appropriate.
We err on the side of enabling the barrier, as it's no-op on older
systems, see the comment for more detail.
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/setup.h | 1 +
arch/powerpc/kernel/security.c | 60 ++++++++++++++++++++++++++++++++++
arch/powerpc/platforms/powernv/setup.c | 1 +
arch/powerpc/platforms/pseries/setup.c | 1 +
4 files changed, 63 insertions(+)
diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index 4335cddc1cf2..aeb175e8a525 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -52,6 +52,7 @@ enum l1d_flush_type {
void setup_rfi_flush(enum l1d_flush_type, bool enable);
void do_rfi_flush_fixups(enum l1d_flush_type types);
+void setup_barrier_nospec(void);
void do_barrier_nospec_fixups(bool enable);
#ifdef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index b963eae0b0a0..d1b9639e5e24 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -8,6 +8,7 @@
#include <linux/device.h>
#include <linux/seq_buf.h>
+#include <asm/debugfs.h>
#include <asm/security_features.h>
#include <asm/setup.h>
@@ -22,6 +23,65 @@ static void enable_barrier_nospec(bool enable)
do_barrier_nospec_fixups(enable);
}
+void setup_barrier_nospec(void)
+{
+ bool enable;
+
+ /*
+ * It would make sense to check SEC_FTR_SPEC_BAR_ORI31 below as well.
+ * But there's a good reason not to. The two flags we check below are
+ * both are enabled by default in the kernel, so if the hcall is not
+ * functional they will be enabled.
+ * On a system where the host firmware has been updated (so the ori
+ * functions as a barrier), but on which the hypervisor (KVM/Qemu) has
+ * not been updated, we would like to enable the barrier. Dropping the
+ * check for SEC_FTR_SPEC_BAR_ORI31 achieves that. The only downside is
+ * we potentially enable the barrier on systems where the host firmware
+ * is not updated, but that's harmless as it's a no-op.
+ */
+ enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) &&
+ security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR);
+
+ enable_barrier_nospec(enable);
+}
+
+#ifdef CONFIG_DEBUG_FS
+static int barrier_nospec_set(void *data, u64 val)
+{
+ switch (val) {
+ case 0:
+ case 1:
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (!!val == !!barrier_nospec_enabled)
+ return 0;
+
+ enable_barrier_nospec(!!val);
+
+ return 0;
+}
+
+static int barrier_nospec_get(void *data, u64 *val)
+{
+ *val = barrier_nospec_enabled ? 1 : 0;
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(fops_barrier_nospec,
+ barrier_nospec_get, barrier_nospec_set, "%llu\n");
+
+static __init int barrier_nospec_debugfs_init(void)
+{
+ debugfs_create_file("barrier_nospec", 0600, powerpc_debugfs_root, NULL,
+ &fops_barrier_nospec);
+ return 0;
+}
+device_initcall(barrier_nospec_debugfs_init);
+#endif /* CONFIG_DEBUG_FS */
+
ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr, char *buf)
{
bool thread_priv;
diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index ef8c9ce53a61..e2ca5f77a55f 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -124,6 +124,7 @@ static void pnv_setup_rfi_flush(void)
security_ftr_enabled(SEC_FTR_L1D_FLUSH_HV));
setup_rfi_flush(type, enable);
+ setup_barrier_nospec();
}
static void __init pnv_setup_arch(void)
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index b55ad4286dc7..63b1f0d10ef0 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -534,6 +534,7 @@ void pseries_setup_rfi_flush(void)
security_ftr_enabled(SEC_FTR_L1D_FLUSH_PR);
setup_rfi_flush(types, enable);
+ setup_barrier_nospec();
}
#ifdef CONFIG_PCI_IOV
--
2.14.1
^ permalink raw reply related
* [PATCH 3/6] powerpc/64s: Patch barrier_nospec in modules
From: Michael Ellerman @ 2018-04-24 4:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin, msuchanek, linux-kernel
In-Reply-To: <20180424041559.32410-1-mpe@ellerman.id.au>
From: Michal Suchanek <msuchanek@suse.de>
Note that unlike RFI which is patched only in kernel the nospec state
reflects settings at the time the module was loaded.
Iterating all modules and re-patching every time the settings change
is not implemented.
Based on lwsync patching.
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/setup.h | 6 ++++++
arch/powerpc/kernel/module.c | 6 ++++++
arch/powerpc/lib/feature-fixups.c | 16 +++++++++++++---
3 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index afc7280cce3b..4335cddc1cf2 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -54,6 +54,12 @@ void setup_rfi_flush(enum l1d_flush_type, bool enable);
void do_rfi_flush_fixups(enum l1d_flush_type types);
void do_barrier_nospec_fixups(bool enable);
+#ifdef CONFIG_PPC_BOOK3S_64
+void do_barrier_nospec_fixups_range(bool enable, void *start, void *end);
+#else
+static inline void do_barrier_nospec_fixups_range(bool enable, void *start, void *end) { };
+#endif
+
#endif /* !__ASSEMBLY__ */
#endif /* _ASM_POWERPC_SETUP_H */
diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
index 3f7ba0f5bf29..a72698cd3dd0 100644
--- a/arch/powerpc/kernel/module.c
+++ b/arch/powerpc/kernel/module.c
@@ -72,6 +72,12 @@ int module_finalize(const Elf_Ehdr *hdr,
do_feature_fixups(powerpc_firmware_features,
(void *)sect->sh_addr,
(void *)sect->sh_addr + sect->sh_size);
+
+ sect = find_section(hdr, sechdrs, "__spec_barrier_fixup");
+ if (sect != NULL)
+ do_barrier_nospec_fixups_range(true,
+ (void *)sect->sh_addr,
+ (void *)sect->sh_addr + sect->sh_size);
#endif
sect = find_section(hdr, sechdrs, "__lwsync_fixup");
diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index 093c1d2ea5fd..3b37529f82f8 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -163,14 +163,14 @@ void do_rfi_flush_fixups(enum l1d_flush_type types)
: "unknown");
}
-void do_barrier_nospec_fixups(bool enable)
+void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, void *fixup_end)
{
unsigned int instr, *dest;
long *start, *end;
int i;
- start = PTRRELOC(&__start___barrier_nospec_fixup),
- end = PTRRELOC(&__stop___barrier_nospec_fixup);
+ start = fixup_start;
+ end = fixup_end;
instr = 0x60000000; /* nop */
@@ -189,6 +189,16 @@ void do_barrier_nospec_fixups(bool enable)
printk(KERN_DEBUG "barrier-nospec: patched %d locations\n", i);
}
+void do_barrier_nospec_fixups(bool enable)
+{
+ void *start, *end;
+
+ start = PTRRELOC(&__start___barrier_nospec_fixup),
+ end = PTRRELOC(&__stop___barrier_nospec_fixup);
+
+ do_barrier_nospec_fixups_range(enable, start, end);
+}
+
#endif /* CONFIG_PPC_BOOK3S_64 */
void do_lwsync_fixups(unsigned long value, void *fixup_start, void *fixup_end)
--
2.14.1
^ permalink raw reply related
* [PATCH 2/6] powerpc/64s: Add support for ori barrier_nospec patching
From: Michael Ellerman @ 2018-04-24 4:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin, msuchanek, linux-kernel
In-Reply-To: <20180424041559.32410-1-mpe@ellerman.id.au>
From: Michal Suchanek <msuchanek@suse.de>
Based on the RFI patching. This is required to be able to disable the
speculation barrier.
Only one barrier type is supported and it does nothing when the
firmware does not enable it. Also re-patching modules is not supported
So the only meaningful thing that can be done is patching out the
speculation barrier at boot when the user says it is not wanted.
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/barrier.h | 2 +-
arch/powerpc/include/asm/feature-fixups.h | 9 +++++++++
arch/powerpc/include/asm/setup.h | 1 +
arch/powerpc/kernel/security.c | 9 +++++++++
arch/powerpc/kernel/vmlinux.lds.S | 7 +++++++
arch/powerpc/lib/feature-fixups.c | 27 +++++++++++++++++++++++++++
6 files changed, 54 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index e582d2c88092..f67b3f6e36be 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -81,7 +81,7 @@ do { \
* Prevent execution of subsequent instructions until preceding branches have
* been fully resolved and are no longer executing speculatively.
*/
-#define barrier_nospec_asm ori 31,31,0
+#define barrier_nospec_asm NOSPEC_BARRIER_FIXUP_SECTION; nop
// This also acts as a compiler barrier due to the memory clobber.
#define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "memory")
diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h
index 1e82eb3caabd..86ac59e75f36 100644
--- a/arch/powerpc/include/asm/feature-fixups.h
+++ b/arch/powerpc/include/asm/feature-fixups.h
@@ -195,11 +195,20 @@ label##3: \
FTR_ENTRY_OFFSET 951b-952b; \
.popsection;
+#define NOSPEC_BARRIER_FIXUP_SECTION \
+953: \
+ .pushsection __barrier_nospec_fixup,"a"; \
+ .align 2; \
+954: \
+ FTR_ENTRY_OFFSET 953b-954b; \
+ .popsection;
+
#ifndef __ASSEMBLY__
#include <linux/types.h>
extern long __start___rfi_flush_fixup, __stop___rfi_flush_fixup;
+extern long __start___barrier_nospec_fixup, __stop___barrier_nospec_fixup;
void apply_feature_fixups(void);
void setup_feature_keys(void);
diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index 27fa52ed6d00..afc7280cce3b 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -52,6 +52,7 @@ enum l1d_flush_type {
void setup_rfi_flush(enum l1d_flush_type, bool enable);
void do_rfi_flush_fixups(enum l1d_flush_type types);
+void do_barrier_nospec_fixups(bool enable);
#endif /* !__ASSEMBLY__ */
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index bab5a27ea805..b963eae0b0a0 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -9,10 +9,19 @@
#include <linux/seq_buf.h>
#include <asm/security_features.h>
+#include <asm/setup.h>
unsigned long powerpc_security_features __read_mostly = SEC_FTR_DEFAULT;
+static bool barrier_nospec_enabled;
+
+static void enable_barrier_nospec(bool enable)
+{
+ barrier_nospec_enabled = enable;
+ do_barrier_nospec_fixups(enable);
+}
+
ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr, char *buf)
{
bool thread_priv;
diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index c8af90ff49f0..ff73f498568c 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -139,6 +139,13 @@ SECTIONS
*(__rfi_flush_fixup)
__stop___rfi_flush_fixup = .;
}
+
+ . = ALIGN(8);
+ __spec_barrier_fixup : AT(ADDR(__spec_barrier_fixup) - LOAD_OFFSET) {
+ __start___barrier_nospec_fixup = .;
+ *(__barrier_nospec_fixup)
+ __stop___barrier_nospec_fixup = .;
+ }
#endif
EXCEPTION_TABLE(0)
diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index 288fe4f0db4e..093c1d2ea5fd 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -162,6 +162,33 @@ void do_rfi_flush_fixups(enum l1d_flush_type types)
(types & L1D_FLUSH_MTTRIG) ? "mttrig type"
: "unknown");
}
+
+void do_barrier_nospec_fixups(bool enable)
+{
+ unsigned int instr, *dest;
+ long *start, *end;
+ int i;
+
+ start = PTRRELOC(&__start___barrier_nospec_fixup),
+ end = PTRRELOC(&__stop___barrier_nospec_fixup);
+
+ instr = 0x60000000; /* nop */
+
+ if (enable) {
+ pr_info("barrier-nospec: using ORI speculation barrier\n");
+ instr = 0x63ff0000; /* ori 31,31,0 speculation barrier */
+ }
+
+ for (i = 0; start < end; start++, i++) {
+ dest = (void *)start + *start;
+
+ pr_devel("patching dest %lx\n", (unsigned long)dest);
+ patch_instruction(dest, instr);
+ }
+
+ printk(KERN_DEBUG "barrier-nospec: patched %d locations\n", i);
+}
+
#endif /* CONFIG_PPC_BOOK3S_64 */
void do_lwsync_fixups(unsigned long value, void *fixup_start, void *fixup_end)
--
2.14.1
^ permalink raw reply related
* [PATCH 1/6] powerpc/64s: Add barrier_nospec
From: Michael Ellerman @ 2018-04-24 4:15 UTC (permalink / raw)
To: linuxppc-dev; +Cc: npiggin, msuchanek, linux-kernel
From: Michal Suchanek <msuchanek@suse.de>
A no-op form of ori (or immediate of 0 into r31 and the result stored
in r31) has been re-tasked as a speculation barrier. The instruction
only acts as a barrier on newer machines with appropriate firmware
support. On older CPUs it remains a harmless no-op.
Implement barrier_nospec using this instruction.
mpe: The semantics of the instruction are believed to be that it
prevents execution of subsequent instructions until preceding branches
have been fully resolved and are no longer executing speculatively.
There is no further documentation available at this time.
Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
mpe: Make it Book3S64 only, update comment & change log, add a
memory clobber to the asm.
---
arch/powerpc/include/asm/barrier.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index c7c63959ba91..e582d2c88092 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -76,6 +76,21 @@ do { \
___p1; \
})
+#ifdef CONFIG_PPC_BOOK3S_64
+/*
+ * Prevent execution of subsequent instructions until preceding branches have
+ * been fully resolved and are no longer executing speculatively.
+ */
+#define barrier_nospec_asm ori 31,31,0
+
+// This also acts as a compiler barrier due to the memory clobber.
+#define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "memory")
+
+#else /* !CONFIG_PPC_BOOK3S_64 */
+#define barrier_nospec_asm
+#define barrier_nospec()
+#endif
+
#include <asm-generic/barrier.h>
#endif /* _ASM_POWERPC_BARRIER_H */
--
2.14.1
^ permalink raw reply related
* Re: [PATCH RFC 1/1] KVM: PPC: Book3S HV: pack VCORE IDs to access full VCPU ID space
From: David Gibson @ 2018-04-24 3:48 UTC (permalink / raw)
To: Sam Bobroff; +Cc: Cédric Le Goater, linuxppc-dev, kvm, kvm-ppc, paulus
In-Reply-To: <20180424031914.GA25846@tungsten.ozlabs.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 12430 bytes --]
On Tue, Apr 24, 2018 at 01:19:15PM +1000, Sam Bobroff wrote:
> On Mon, Apr 23, 2018 at 11:06:35AM +0200, Cédric Le Goater wrote:
> > On 04/16/2018 06:09 AM, David Gibson wrote:
> > > On Thu, Apr 12, 2018 at 05:02:06PM +1000, Sam Bobroff wrote:
> > >> It is not currently possible to create the full number of possible
> > >> VCPUs (KVM_MAX_VCPUS) on Power9 with KVM-HV when the guest uses less
> > >> threads per core than it's core stride (or "VSMT mode"). This is
> > >> because the VCORE ID and XIVE offsets to grow beyond KVM_MAX_VCPUS
> > >> even though the VCPU ID is less than KVM_MAX_VCPU_ID.
> > >>
> > >> To address this, "pack" the VCORE ID and XIVE offsets by using
> > >> knowledge of the way the VCPU IDs will be used when there are less
> > >> guest threads per core than the core stride. The primary thread of
> > >> each core will always be used first. Then, if the guest uses more than
> > >> one thread per core, these secondary threads will sequentially follow
> > >> the primary in each core.
> > >>
> > >> So, the only way an ID above KVM_MAX_VCPUS can be seen, is if the
> > >> VCPUs are being spaced apart, so at least half of each core is empty
> > >> and IDs between KVM_MAX_VCPUS and (KVM_MAX_VCPUS * 2) can be mapped
> > >> into the second half of each core (4..7, in an 8-thread core).
> > >>
> > >> Similarly, if IDs above KVM_MAX_VCPUS * 2 are seen, at least 3/4 of
> > >> each core is being left empty, and we can map down into the second and
> > >> third quarters of each core (2, 3 and 5, 6 in an 8-thread core).
> > >>
> > >> Lastly, if IDs above KVM_MAX_VCPUS * 4 are seen, only the primary
> > >> threads are being used and 7/8 of the core is empty, allowing use of
> > >> the 1, 3, 5 and 7 thread slots.
> > >>
> > >> (Strides less than 8 are handled similarly.)
> > >>
> > >> This allows the VCORE ID or offset to be calculated quickly from the
> > >> VCPU ID or XIVE server numbers, without access to the VCPU structure.
> > >>
> > >> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> > >> ---
> > >> Hello everyone,
> > >>
> > >> I've tested this on P8 and P9, in lots of combinations of host and guest
> > >> threading modes and it has been fine but it does feel like a "tricky"
> > >> approach, so I still feel somewhat wary about it.
> >
> > Have you done any migration ?
>
> No, but I will :-)
>
> > >> I've posted it as an RFC because I have not tested it with guest native-XIVE,
> > >> and I suspect that it will take some work to support it.
> >
> > The KVM XIVE device will be different for XIVE exploitation mode, same structures
> > though. I will send a patchset shortly.
>
> Great. This is probably where conflicts between the host and guest
> numbers will show up. (See dwg's question below.)
>
> > >> arch/powerpc/include/asm/kvm_book3s.h | 19 +++++++++++++++++++
> > >> arch/powerpc/kvm/book3s_hv.c | 14 ++++++++++----
> > >> arch/powerpc/kvm/book3s_xive.c | 9 +++++++--
> > >> 3 files changed, 36 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> > >> index 376ae803b69c..1295056d564a 100644
> > >> --- a/arch/powerpc/include/asm/kvm_book3s.h
> > >> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> > >> @@ -368,4 +368,23 @@ extern int kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu);
> > >> #define SPLIT_HACK_MASK 0xff000000
> > >> #define SPLIT_HACK_OFFS 0xfb000000
> > >>
> > >> +/* Pack a VCPU ID from the [0..KVM_MAX_VCPU_ID) space down to the
> > >> + * [0..KVM_MAX_VCPUS) space, while using knowledge of the guest's core stride
> > >> + * (but not it's actual threading mode, which is not available) to avoid
> > >> + * collisions.
> > >> + */
> > >> +static inline u32 kvmppc_pack_vcpu_id(struct kvm *kvm, u32 id)
> > >> +{
> > >> + const int block_offsets[MAX_SMT_THREADS] = {0, 4, 2, 6, 1, 5, 3, 7};
> > >
> > > I'd suggest 1,3,5,7 at the end rather than 1,5,3,7 - accomplishes
> > > roughly the same thing, but I think makes the pattern more obvious.
>
> OK.
>
> > >> + int stride = kvm->arch.emul_smt_mode > 1 ?
> > >> + kvm->arch.emul_smt_mode : kvm->arch.smt_mode;
> > >
> > > AFAICT from BUG_ON()s etc. at the callsites, kvm->arch.smt_mode must
> > > always be 1 when this is called, so the conditional here doesn't seem
> > > useful.
>
> Ah yes, right. (That was an older version when I was thinking of using
> it for P8 as well but that didn't seem to be a good idea.)
>
> > >> + int block = (id / KVM_MAX_VCPUS) * (MAX_SMT_THREADS / stride);
> > >> + u32 packed_id;
> > >> +
> > >> + BUG_ON(block >= MAX_SMT_THREADS);
> > >> + packed_id = (id % KVM_MAX_VCPUS) + block_offsets[block];
> > >> + BUG_ON(packed_id >= KVM_MAX_VCPUS);
> > >> + return packed_id;
> > >> +}
> > >
> > > It took me a while to wrap my head around the packing function, but I
> > > think I got there in the end. It's pretty clever.
>
> Thanks, I'll try to add a better description as well :-)
>
> > > One thing bothers me, though. This certainly packs things under
> > > KVM_MAX_VCPUS, but not necessarily under the actual number of vcpus.
> > > e.g. KVM_MAC_VCPUS==16, 8 vcpus total, stride 8, 2 vthreads/vcore (as
> > > qemu sees it), gives both unpacked IDs (0, 1, 8, 9, 16, 17, 24, 25)
> > > and packed ids of (0, 1, 8, 9, 4, 5, 12, 13) - leaving 2, 3, 6, 7
> > > etc. unused.
>
> That's right. The property it provides is that all the numbers are under
> KVM_MAX_VCPUS (which, see below, is the size of the fixed areas) not
> that they are sequential.
>
> > > So again, the question is what exactly are these remapped IDs useful
> > > for. If we're indexing into a bare array of structures of size
> > > KVM_MAX_VCPUS then we're *already* wasting a bunch of space by having
> > > more entries than vcpus. If we're indexing into something sparser,
> > > then why is the remapping worthwhile?
>
> Well, here's my thinking:
>
> At the moment, kvm->vcores[] and xive->vp_base are both sized by NR_CPUS
> (via KVM_MAX_VCPUS and KVM_MAX_VCORES which are both NR_CPUS). This is
> enough space for the maximum number of VCPUs, and some space is wasted
> when the guest uses less than this (but KVM doesn't know how many will
> be created, so we can't do better easily). The problem is that the
> indicies overflow before all of those VCPUs can be created, not that
> more space is needed.
>
> We could fix the overflow by expanding these areas to KVM_MAX_VCPU_ID
> but that will use 8x the space we use now, and we know that no more than
> KVM_MAX_VCPUS will be used so all this new space is basically wasted.
>
> So remapping seems better if it will work. (Ben H. was strongly against
> wasting more XIVE space if possible.)
Hm, ok. Are the relevant arrays here per-VM, or global? Or some of both?
> In short, remapping provides a way to allow the guest to create it's full set
> of VCPUs without wasting any more space than we do currently, without
> having to do something more complicated like tracking used IDs or adding
> additional KVM CAPs.
>
> > >> +
> > >> #endif /* __ASM_KVM_BOOK3S_H__ */
> > >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> > >> index 9cb9448163c4..49165cc90051 100644
> > >> --- a/arch/powerpc/kvm/book3s_hv.c
> > >> +++ b/arch/powerpc/kvm/book3s_hv.c
> > >> @@ -1762,7 +1762,7 @@ static int threads_per_vcore(struct kvm *kvm)
> > >> return threads_per_subcore;
> > >> }
> > >>
> > >> -static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
> > >> +static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int id)
> > >> {
> > >> struct kvmppc_vcore *vcore;
> > >>
> > >> @@ -1776,7 +1776,7 @@ static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
> > >> init_swait_queue_head(&vcore->wq);
> > >> vcore->preempt_tb = TB_NIL;
> > >> vcore->lpcr = kvm->arch.lpcr;
> > >> - vcore->first_vcpuid = core * kvm->arch.smt_mode;
> > >> + vcore->first_vcpuid = id;
> > >> vcore->kvm = kvm;
> > >> INIT_LIST_HEAD(&vcore->preempt_list);
> > >>
> > >> @@ -1992,12 +1992,18 @@ static struct kvm_vcpu *kvmppc_core_vcpu_create_hv(struct kvm *kvm,
> > >> mutex_lock(&kvm->lock);
> > >> vcore = NULL;
> > >> err = -EINVAL;
> > >> - core = id / kvm->arch.smt_mode;
> > >> + if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> > >> + BUG_ON(kvm->arch.smt_mode != 1);
> > >> + core = kvmppc_pack_vcpu_id(kvm, id);
> > >> + } else {
> > >> + core = id / kvm->arch.smt_mode;
> > >> + }
> > >> if (core < KVM_MAX_VCORES) {
> > >> vcore = kvm->arch.vcores[core];
> > >> + BUG_ON(cpu_has_feature(CPU_FTR_ARCH_300) && vcore);
> > >> if (!vcore) {
> > >> err = -ENOMEM;
> > >> - vcore = kvmppc_vcore_create(kvm, core);
> > >> + vcore = kvmppc_vcore_create(kvm, id & ~(kvm->arch.smt_mode - 1));
> > >> kvm->arch.vcores[core] = vcore;
> > >> kvm->arch.online_vcores++;
> > >> }
> > >> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> > >> index f9818d7d3381..681dfe12a5f3 100644
> > >> --- a/arch/powerpc/kvm/book3s_xive.c
> > >> +++ b/arch/powerpc/kvm/book3s_xive.c
> > >> @@ -317,6 +317,11 @@ static int xive_select_target(struct kvm *kvm, u32 *server, u8 prio)
> > >> return -EBUSY;
> > >> }
> > >>
> > >> +static u32 xive_vp(struct kvmppc_xive *xive, u32 server)
> > >> +{
> > >> + return xive->vp_base + kvmppc_pack_vcpu_id(xive->kvm, server);
> > >> +}
> > >> +
> > >
> > > I'm finding the XIVE indexing really baffling. There are a bunch of
> > > other places where the code uses (xive->vp_base + NUMBER) directly.
>
> Ugh, yes. It looks like I botched part of my final cleanup and all the
> cases you saw in kvm/book3s_xive.c should have been replaced with a call to
> xive_vp(). I'll fix it and sorry for the confusion.
Ok.
> > This links the QEMU vCPU server NUMBER to a XIVE virtual processor number
> > in OPAL. So we need to check that all used NUMBERs are, first, consistent
> > and then, in the correct range.
>
> Right. My approach was to allow XIVE to keep using server numbers that
> are equal to VCPU IDs, and just pack down the ID before indexing into
> the vp_base area.
>
> > > If those are host side references, I guess they don't need updates for
> > > this.
>
> These are all guest side references.
>
> > > But if that's the case, then how does indexing into the same array
> > > with both host and guest server numbers make sense?
>
> Right, it doesn't make sense to mix host and guest server numbers when
> we're remapping only the guest ones, but in this case (without native
> guest XIVE support) it's just guest ones.
Right. Will this remapping be broken by guest-visible XIVE? That is
for the guest visible XIVE are we going to need to expose un-remapped
XIVE server IDs to the guest?
> > yes. VPs are allocated with KVM_MAX_VCPUS :
> >
> > xive->vp_base = xive_native_alloc_vp_block(KVM_MAX_VCPUS);
> >
> > but
> >
> > #define KVM_MAX_VCPU_ID (threads_per_subcore * KVM_MAX_VCORES)
> >
> > WE would need to change the allocation of the VPs I guess.
>
> Yes, this is one of the structures that overflow if we don't pack the IDs.
>
> > >> static u8 xive_lock_and_mask(struct kvmppc_xive *xive,
> > >> struct kvmppc_xive_src_block *sb,
> > >> struct kvmppc_xive_irq_state *state)
> > >> @@ -1084,7 +1089,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> > >> pr_devel("Duplicate !\n");
> > >> return -EEXIST;
> > >> }
> > >> - if (cpu >= KVM_MAX_VCPUS) {
> > >> + if (cpu >= KVM_MAX_VCPU_ID) {>>
> > >> pr_devel("Out of bounds !\n");
> > >> return -EINVAL;
> > >> }
> > >> @@ -1098,7 +1103,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> > >> xc->xive = xive;
> > >> xc->vcpu = vcpu;
> > >> xc->server_num = cpu;
> > >> - xc->vp_id = xive->vp_base + cpu;
> > >> + xc->vp_id = xive_vp(xive, cpu);
> > >> xc->mfrr = 0xff;
> > >> xc->valid = true;
> > >>
> > >
> >
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [1/2] powernv/npu: Do a PID GPU TLB flush when invalidating a large address range
From: Michael Ellerman @ 2018-04-24 3:48 UTC (permalink / raw)
To: Alistair Popple, linuxppc-dev; +Cc: Alistair Popple, mhairgrove, arbab
In-Reply-To: <20180417091129.23069-1-alistair@popple.id.au>
On Tue, 2018-04-17 at 09:11:28 UTC, Alistair Popple wrote:
> The NPU has a limited number of address translation shootdown (ATSD)
> registers and the GPU has limited bandwidth to process ATSDs. This can
> result in contention of ATSD registers leading to soft lockups on some
> threads, particularly when invalidating a large address range in
> pnv_npu2_mn_invalidate_range().
>
> At some threshold it becomes more efficient to flush the entire GPU TLB for
> the given MM context (PID) than individually flushing each address in the
> range. This patch will result in ranges greater than 2MB being converted
> from 32+ ATSDs into a single ATSD which will flush the TLB for the given
> PID on each GPU.
>
> Signed-off-by: Alistair Popple <alistair@popple.id.au>
> Acked-by: Balbir Singh <bsingharora@gmail.com>
> Tested-by: Balbir Singh <bsingharora@gmail.com>
Patch 1 applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/d0cf9b561ca97d5245bb9e0c4774b7
cheers
^ permalink raw reply
* Re: [1/2] powernv/npu: Add lock to prevent race in concurrent context init/destroy
From: Michael Ellerman @ 2018-04-24 3:48 UTC (permalink / raw)
To: Alistair Popple, linuxppc-dev; +Cc: Alistair Popple, mhairgrove, arbab
In-Reply-To: <20180411063855.5451-1-alistair@popple.id.au>
On Wed, 2018-04-11 at 06:38:54 UTC, Alistair Popple wrote:
> The pnv_npu2_init_context() and pnv_npu2_destroy_context() functions are
> used to allocate/free contexts to allow address translation and shootdown
> by the NPU on a particular GPU. Context initialisation is implicitly safe
> as it is protected by the requirement mmap_sem be held in write mode,
> however pnv_npu2_destroy_context() does not require mmap_sem to be held and
> it is not safe to call with a concurrent initialisation for a different
> GPU.
>
> It was assumed the driver would ensure destruction was not called
> concurrently with initialisation. However the driver may be simplified by
> allowing concurrent initialisation and destruction for different GPUs. As
> npu context creation/destruction is not a performance critical path and the
> critical section is not large a single spinlock is used for simplicity.
>
> Signed-off-by: Alistair Popple <alistair@popple.id.au>
> Reviewed-by: Mark Hairgrove <mhairgrove@nvidia.com>
> Tested-by: Mark Hairgrove <mhairgrove@nvidia.com>
> Reviewed-by: Balbir Singh <bsingharora@gmail.com>
Series applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/28a5933e8d362766462ea9e5f135e1
cheers
^ permalink raw reply
* Re: [1/2] powerpc/mm: Flush cache on memory hot(un)plug
From: Michael Ellerman @ 2018-04-24 3:48 UTC (permalink / raw)
To: Balbir Singh, linuxppc-dev; +Cc: rashmica.g
In-Reply-To: <20180406052424.29841-1-bsingharora@gmail.com>
On Fri, 2018-04-06 at 05:24:23 UTC, Balbir Singh wrote:
> This patch adds support for flushing potentially dirty
> cache lines when memory is hot-plugged/hot-un-plugged.
> The support is currently limited to 64 bit systems.
>
> The bug was exposed when mappings for a device were
> actually hot-unplugged and plugged in back later.
> A similar issue was observed during the development
> of memtrace, but memtrace does it's own flushing of
> region via a custom routine.
>
> These patches do a flush both on hotplug/unplug to
> clear any stale data in the cache w.r.t mappings,
> there is a small race window where a clean cache
> line may be created again just prior to tearing
> down the mapping.
>
> The patches were tested by disabling the flush
> routines in memtrace and doing I/O on the trace
> file. The system immediately checkstops (quite
> reliablly if prior to the hot-unplug of the memtrace
> region, we memset the regions we are about to
> hot unplug). After these patches no custom flushing
> is needed in the memtrace code.
>
> Signed-off-by: Balbir Singh <bsingharora@gmail.com>
> Acked-by: Reza Arbab <arbab@linux.ibm.com>
> Reviewed-by: Rashmica Gupta <rashmica.g@gmail.com>
Series applied to powerpc fixes, thanks.
https://git.kernel.org/powerpc/c/fb5924fddf9ee31db04da7ad4e8c34
cheers
^ permalink raw reply
* Re: [PATCH RFC 1/1] KVM: PPC: Book3S HV: pack VCORE IDs to access full VCPU ID space
From: Sam Bobroff @ 2018-04-24 3:19 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: David Gibson, linuxppc-dev, kvm, kvm-ppc, paulus
In-Reply-To: <1e01ea66-6103-94c8-ccb1-ed35b3a3104b@kaod.org>
[-- Attachment #1: Type: text/plain, Size: 11382 bytes --]
On Mon, Apr 23, 2018 at 11:06:35AM +0200, Cédric Le Goater wrote:
> On 04/16/2018 06:09 AM, David Gibson wrote:
> > On Thu, Apr 12, 2018 at 05:02:06PM +1000, Sam Bobroff wrote:
> >> It is not currently possible to create the full number of possible
> >> VCPUs (KVM_MAX_VCPUS) on Power9 with KVM-HV when the guest uses less
> >> threads per core than it's core stride (or "VSMT mode"). This is
> >> because the VCORE ID and XIVE offsets to grow beyond KVM_MAX_VCPUS
> >> even though the VCPU ID is less than KVM_MAX_VCPU_ID.
> >>
> >> To address this, "pack" the VCORE ID and XIVE offsets by using
> >> knowledge of the way the VCPU IDs will be used when there are less
> >> guest threads per core than the core stride. The primary thread of
> >> each core will always be used first. Then, if the guest uses more than
> >> one thread per core, these secondary threads will sequentially follow
> >> the primary in each core.
> >>
> >> So, the only way an ID above KVM_MAX_VCPUS can be seen, is if the
> >> VCPUs are being spaced apart, so at least half of each core is empty
> >> and IDs between KVM_MAX_VCPUS and (KVM_MAX_VCPUS * 2) can be mapped
> >> into the second half of each core (4..7, in an 8-thread core).
> >>
> >> Similarly, if IDs above KVM_MAX_VCPUS * 2 are seen, at least 3/4 of
> >> each core is being left empty, and we can map down into the second and
> >> third quarters of each core (2, 3 and 5, 6 in an 8-thread core).
> >>
> >> Lastly, if IDs above KVM_MAX_VCPUS * 4 are seen, only the primary
> >> threads are being used and 7/8 of the core is empty, allowing use of
> >> the 1, 3, 5 and 7 thread slots.
> >>
> >> (Strides less than 8 are handled similarly.)
> >>
> >> This allows the VCORE ID or offset to be calculated quickly from the
> >> VCPU ID or XIVE server numbers, without access to the VCPU structure.
> >>
> >> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> >> ---
> >> Hello everyone,
> >>
> >> I've tested this on P8 and P9, in lots of combinations of host and guest
> >> threading modes and it has been fine but it does feel like a "tricky"
> >> approach, so I still feel somewhat wary about it.
>
> Have you done any migration ?
No, but I will :-)
> >> I've posted it as an RFC because I have not tested it with guest native-XIVE,
> >> and I suspect that it will take some work to support it.
>
> The KVM XIVE device will be different for XIVE exploitation mode, same structures
> though. I will send a patchset shortly.
Great. This is probably where conflicts between the host and guest
numbers will show up. (See dwg's question below.)
> >> arch/powerpc/include/asm/kvm_book3s.h | 19 +++++++++++++++++++
> >> arch/powerpc/kvm/book3s_hv.c | 14 ++++++++++----
> >> arch/powerpc/kvm/book3s_xive.c | 9 +++++++--
> >> 3 files changed, 36 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
> >> index 376ae803b69c..1295056d564a 100644
> >> --- a/arch/powerpc/include/asm/kvm_book3s.h
> >> +++ b/arch/powerpc/include/asm/kvm_book3s.h
> >> @@ -368,4 +368,23 @@ extern int kvmppc_h_logical_ci_store(struct kvm_vcpu *vcpu);
> >> #define SPLIT_HACK_MASK 0xff000000
> >> #define SPLIT_HACK_OFFS 0xfb000000
> >>
> >> +/* Pack a VCPU ID from the [0..KVM_MAX_VCPU_ID) space down to the
> >> + * [0..KVM_MAX_VCPUS) space, while using knowledge of the guest's core stride
> >> + * (but not it's actual threading mode, which is not available) to avoid
> >> + * collisions.
> >> + */
> >> +static inline u32 kvmppc_pack_vcpu_id(struct kvm *kvm, u32 id)
> >> +{
> >> + const int block_offsets[MAX_SMT_THREADS] = {0, 4, 2, 6, 1, 5, 3, 7};
> >
> > I'd suggest 1,3,5,7 at the end rather than 1,5,3,7 - accomplishes
> > roughly the same thing, but I think makes the pattern more obvious.
OK.
> >> + int stride = kvm->arch.emul_smt_mode > 1 ?
> >> + kvm->arch.emul_smt_mode : kvm->arch.smt_mode;
> >
> > AFAICT from BUG_ON()s etc. at the callsites, kvm->arch.smt_mode must
> > always be 1 when this is called, so the conditional here doesn't seem
> > useful.
Ah yes, right. (That was an older version when I was thinking of using
it for P8 as well but that didn't seem to be a good idea.)
> >> + int block = (id / KVM_MAX_VCPUS) * (MAX_SMT_THREADS / stride);
> >> + u32 packed_id;
> >> +
> >> + BUG_ON(block >= MAX_SMT_THREADS);
> >> + packed_id = (id % KVM_MAX_VCPUS) + block_offsets[block];
> >> + BUG_ON(packed_id >= KVM_MAX_VCPUS);
> >> + return packed_id;
> >> +}
> >
> > It took me a while to wrap my head around the packing function, but I
> > think I got there in the end. It's pretty clever.
Thanks, I'll try to add a better description as well :-)
> > One thing bothers me, though. This certainly packs things under
> > KVM_MAX_VCPUS, but not necessarily under the actual number of vcpus.
> > e.g. KVM_MAC_VCPUS==16, 8 vcpus total, stride 8, 2 vthreads/vcore (as
> > qemu sees it), gives both unpacked IDs (0, 1, 8, 9, 16, 17, 24, 25)
> > and packed ids of (0, 1, 8, 9, 4, 5, 12, 13) - leaving 2, 3, 6, 7
> > etc. unused.
That's right. The property it provides is that all the numbers are under
KVM_MAX_VCPUS (which, see below, is the size of the fixed areas) not
that they are sequential.
> > So again, the question is what exactly are these remapped IDs useful
> > for. If we're indexing into a bare array of structures of size
> > KVM_MAX_VCPUS then we're *already* wasting a bunch of space by having
> > more entries than vcpus. If we're indexing into something sparser,
> > then why is the remapping worthwhile?
Well, here's my thinking:
At the moment, kvm->vcores[] and xive->vp_base are both sized by NR_CPUS
(via KVM_MAX_VCPUS and KVM_MAX_VCORES which are both NR_CPUS). This is
enough space for the maximum number of VCPUs, and some space is wasted
when the guest uses less than this (but KVM doesn't know how many will
be created, so we can't do better easily). The problem is that the
indicies overflow before all of those VCPUs can be created, not that
more space is needed.
We could fix the overflow by expanding these areas to KVM_MAX_VCPU_ID
but that will use 8x the space we use now, and we know that no more than
KVM_MAX_VCPUS will be used so all this new space is basically wasted.
So remapping seems better if it will work. (Ben H. was strongly against
wasting more XIVE space if possible.)
In short, remapping provides a way to allow the guest to create it's full set
of VCPUs without wasting any more space than we do currently, without
having to do something more complicated like tracking used IDs or adding
additional KVM CAPs.
> >> +
> >> #endif /* __ASM_KVM_BOOK3S_H__ */
> >> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> >> index 9cb9448163c4..49165cc90051 100644
> >> --- a/arch/powerpc/kvm/book3s_hv.c
> >> +++ b/arch/powerpc/kvm/book3s_hv.c
> >> @@ -1762,7 +1762,7 @@ static int threads_per_vcore(struct kvm *kvm)
> >> return threads_per_subcore;
> >> }
> >>
> >> -static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
> >> +static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int id)
> >> {
> >> struct kvmppc_vcore *vcore;
> >>
> >> @@ -1776,7 +1776,7 @@ static struct kvmppc_vcore *kvmppc_vcore_create(struct kvm *kvm, int core)
> >> init_swait_queue_head(&vcore->wq);
> >> vcore->preempt_tb = TB_NIL;
> >> vcore->lpcr = kvm->arch.lpcr;
> >> - vcore->first_vcpuid = core * kvm->arch.smt_mode;
> >> + vcore->first_vcpuid = id;
> >> vcore->kvm = kvm;
> >> INIT_LIST_HEAD(&vcore->preempt_list);
> >>
> >> @@ -1992,12 +1992,18 @@ static struct kvm_vcpu *kvmppc_core_vcpu_create_hv(struct kvm *kvm,
> >> mutex_lock(&kvm->lock);
> >> vcore = NULL;
> >> err = -EINVAL;
> >> - core = id / kvm->arch.smt_mode;
> >> + if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> >> + BUG_ON(kvm->arch.smt_mode != 1);
> >> + core = kvmppc_pack_vcpu_id(kvm, id);
> >> + } else {
> >> + core = id / kvm->arch.smt_mode;
> >> + }
> >> if (core < KVM_MAX_VCORES) {
> >> vcore = kvm->arch.vcores[core];
> >> + BUG_ON(cpu_has_feature(CPU_FTR_ARCH_300) && vcore);
> >> if (!vcore) {
> >> err = -ENOMEM;
> >> - vcore = kvmppc_vcore_create(kvm, core);
> >> + vcore = kvmppc_vcore_create(kvm, id & ~(kvm->arch.smt_mode - 1));
> >> kvm->arch.vcores[core] = vcore;
> >> kvm->arch.online_vcores++;
> >> }
> >> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> >> index f9818d7d3381..681dfe12a5f3 100644
> >> --- a/arch/powerpc/kvm/book3s_xive.c
> >> +++ b/arch/powerpc/kvm/book3s_xive.c
> >> @@ -317,6 +317,11 @@ static int xive_select_target(struct kvm *kvm, u32 *server, u8 prio)
> >> return -EBUSY;
> >> }
> >>
> >> +static u32 xive_vp(struct kvmppc_xive *xive, u32 server)
> >> +{
> >> + return xive->vp_base + kvmppc_pack_vcpu_id(xive->kvm, server);
> >> +}
> >> +
> >
> > I'm finding the XIVE indexing really baffling. There are a bunch of
> > other places where the code uses (xive->vp_base + NUMBER) directly.
Ugh, yes. It looks like I botched part of my final cleanup and all the
cases you saw in kvm/book3s_xive.c should have been replaced with a call to
xive_vp(). I'll fix it and sorry for the confusion.
> This links the QEMU vCPU server NUMBER to a XIVE virtual processor number
> in OPAL. So we need to check that all used NUMBERs are, first, consistent
> and then, in the correct range.
Right. My approach was to allow XIVE to keep using server numbers that
are equal to VCPU IDs, and just pack down the ID before indexing into
the vp_base area.
> > If those are host side references, I guess they don't need updates for
> > this.
These are all guest side references.
> > But if that's the case, then how does indexing into the same array
> > with both host and guest server numbers make sense?
Right, it doesn't make sense to mix host and guest server numbers when
we're remapping only the guest ones, but in this case (without native
guest XIVE support) it's just guest ones.
> yes. VPs are allocated with KVM_MAX_VCPUS :
>
> xive->vp_base = xive_native_alloc_vp_block(KVM_MAX_VCPUS);
>
> but
>
> #define KVM_MAX_VCPU_ID (threads_per_subcore * KVM_MAX_VCORES)
>
> WE would need to change the allocation of the VPs I guess.
Yes, this is one of the structures that overflow if we don't pack the IDs.
> >> static u8 xive_lock_and_mask(struct kvmppc_xive *xive,
> >> struct kvmppc_xive_src_block *sb,
> >> struct kvmppc_xive_irq_state *state)
> >> @@ -1084,7 +1089,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> >> pr_devel("Duplicate !\n");
> >> return -EEXIST;
> >> }
> >> - if (cpu >= KVM_MAX_VCPUS) {
> >> + if (cpu >= KVM_MAX_VCPU_ID) {>>
> >> pr_devel("Out of bounds !\n");
> >> return -EINVAL;
> >> }
> >> @@ -1098,7 +1103,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
> >> xc->xive = xive;
> >> xc->vcpu = vcpu;
> >> xc->server_num = cpu;
> >> - xc->vp_id = xive->vp_base + cpu;
> >> + xc->vp_id = xive_vp(xive, cpu);
> >> xc->mfrr = 0xff;
> >> xc->valid = true;
> >>
> >
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/mce: Fix a bug where mce loops on memory UE.
From: Balbir Singh @ 2018-04-23 23:41 UTC (permalink / raw)
To: Mahesh J Salgaonkar, linuxppc-dev
In-Reply-To: <152445952887.3244.567606806755236868.stgit@jupiter.in.ibm.com>
On Mon, 2018-04-23 at 10:29 +0530, Mahesh J Salgaonkar wrote:
> From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>
> The current code extracts the physical address for UE errors and then
> hooks it up into memory failure infrastructure. On successful extraction
> of physical address it wrongly sets "handled = 1" which means this UE error
> has been recovered. Since MCE handler gets return value as handled = 1, it
> assumes that error has been recovered and goes back to same NIP. This causes
> MCE interrupt again and again in a loop leading to hard lockup.
>
> Also, initialize phys_addr to ULONG_MAX so that we don't end up queuing
> undesired page to hwpoison.
>
> Without this patch we see:
> [ 1476.541984] Severe Machine check interrupt [Recovered]
> [ 1476.541985] NIP: [000000001002588c] PID: 7109 Comm: find
> [ 1476.541986] Initiator: CPU
> [ 1476.541987] Error type: UE [Load/Store]
> [ 1476.541988] Effective address: 00007fffd2755940
> [ 1476.541989] Physical address: 000020181a080000
> [...]
> [ 1476.542003] Severe Machine check interrupt [Recovered]
> [ 1476.542004] NIP: [000000001002588c] PID: 7109 Comm: find
> [ 1476.542005] Initiator: CPU
> [ 1476.542006] Error type: UE [Load/Store]
> [ 1476.542006] Effective address: 00007fffd2755940
> [ 1476.542007] Physical address: 000020181a080000
> [ 1476.542010] Severe Machine check interrupt [Recovered]
> [ 1476.542012] NIP: [000000001002588c] PID: 7109 Comm: find
> [ 1476.542013] Initiator: CPU
> [ 1476.542014] Error type: UE [Load/Store]
> [ 1476.542015] Effective address: 00007fffd2755940
> [ 1476.542016] Physical address: 000020181a080000
> [ 1476.542448] Memory failure: 0x20181a08: recovery action for dirty LRU page: Recovered
> [ 1476.542452] Memory failure: 0x20181a08: already hardware poisoned
> [ 1476.542453] Memory failure: 0x20181a08: already hardware poisoned
> [ 1476.542454] Memory failure: 0x20181a08: already hardware poisoned
> [ 1476.542455] Memory failure: 0x20181a08: already hardware poisoned
> [ 1476.542456] Memory failure: 0x20181a08: already hardware poisoned
> [ 1476.542457] Memory failure: 0x20181a08: already hardware poisoned
> [...]
> [ 1490.972174] Watchdog CPU:38 Hard LOCKUP
>
> After this patch we see:
>
> [ 325.384336] Severe Machine check interrupt [Not recovered]
> [ 325.384338] NIP: [00007fffaae585f4] PID: 7168 Comm: find
> [ 325.384339] Initiator: CPU
> [ 325.384341] Error type: UE [Load/Store]
> [ 325.384343] Effective address: 00007fffaafe28ac
> [ 325.384345] Physical address: 00002017c0bd0000
> [ 325.384350] find[7168]: unhandled signal 7 at 00007fffaae585f4 nip 00007fffaae585f4 lr 00007fffaae585e0 code 4
> [ 325.388574] Memory failure: 0x2017c0bd: recovery action for dirty LRU page: Recovered
>
> Fixes: 01eaac2b0591 ("powerpc/mce: Hookup ierror (instruction) UE errors")
> Fixes: ba41e1e1ccb9 ("powerpc/mce: Hookup derror (load/store) UE errors")
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/mce_power.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
> index fe6fc63251fe..63b58ae5d601 100644
> --- a/arch/powerpc/kernel/mce_power.c
> +++ b/arch/powerpc/kernel/mce_power.c
> @@ -441,7 +441,6 @@ static int mce_handle_ierror(struct pt_regs *regs,
> if (pfn != ULONG_MAX) {
> *phys_addr =
> (pfn << PAGE_SHIFT);
> - handled = 1;
> }
> }
> }
> @@ -532,9 +531,8 @@ static int mce_handle_derror(struct pt_regs *regs,
> * kernel/exception-64s.h
> */
> if (get_paca()->in_mce < MAX_MCE_DEPTH)
> - if (!mce_find_instr_ea_and_pfn(regs, addr,
> - phys_addr))
> - handled = 1;
> + mce_find_instr_ea_and_pfn(regs, addr,
> + phys_addr);
> }
> found = 1;
> }
> @@ -572,7 +570,7 @@ static long mce_handle_error(struct pt_regs *regs,
> const struct mce_ierror_table itable[])
> {
> struct mce_error_info mce_err = { 0 };
> - uint64_t addr, phys_addr;
> + uint64_t addr, phys_addr = ULONG_MAX;
> uint64_t srr1 = regs->msr;
> long handled;
>
>
Reviewed-by: Balbir Singh <bsingharora@gmail.com>
^ permalink raw reply
* Re: [PATCH] powerpc/mce: Fix a bug where mce loops on memory UE.
From: Balbir Singh @ 2018-04-23 23:00 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: Mahesh Jagannath Salgaonkar, linuxppc-dev
In-Reply-To: <20180423230116.0d3e9fc5@roar.ozlabs.ibm.com>
On Mon, 2018-04-23 at 23:01 +1000, Nicholas Piggin wrote:
> On Mon, 23 Apr 2018 21:14:12 +1000
> Balbir Singh <bsingharora@gmail.com> wrote:
>
> > On Mon, Apr 23, 2018 at 8:33 PM, Mahesh Jagannath Salgaonkar
> > <mahesh@linux.vnet.ibm.com> wrote:
> > > On 04/23/2018 12:21 PM, Balbir Singh wrote:
> > > > On Mon, Apr 23, 2018 at 2:59 PM, Mahesh J Salgaonkar
> > > > <mahesh@linux.vnet.ibm.com> wrote:
> > > > > From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> > > > >
> > > > > The current code extracts the physical address for UE errors and then
> > > > > hooks it up into memory failure infrastructure. On successful extraction
> > > > > of physical address it wrongly sets "handled = 1" which means this UE error
> > > > > has been recovered. Since MCE handler gets return value as handled = 1, it
> > > > > assumes that error has been recovered and goes back to same NIP. This causes
> > > > > MCE interrupt again and again in a loop leading to hard lockup.
> > > > >
> > > > > Also, initialize phys_addr to ULONG_MAX so that we don't end up queuing
> > > > > undesired page to hwpoison.
> > > > >
> > > > > Without this patch we see:
> > > > > [ 1476.541984] Severe Machine check interrupt [Recovered]
> > > > > [ 1476.541985] NIP: [000000001002588c] PID: 7109 Comm: find
> > > > > [ 1476.541986] Initiator: CPU
> > > > > [ 1476.541987] Error type: UE [Load/Store]
> > > > > [ 1476.541988] Effective address: 00007fffd2755940
> > > > > [ 1476.541989] Physical address: 000020181a080000
> > > > > [...]
> > > > > [ 1476.542003] Severe Machine check interrupt [Recovered]
> > > > > [ 1476.542004] NIP: [000000001002588c] PID: 7109 Comm: find
> > > > > [ 1476.542005] Initiator: CPU
> > > > > [ 1476.542006] Error type: UE [Load/Store]
> > > > > [ 1476.542006] Effective address: 00007fffd2755940
> > > > > [ 1476.542007] Physical address: 000020181a080000
> > > > > [ 1476.542010] Severe Machine check interrupt [Recovered]
> > > > > [ 1476.542012] NIP: [000000001002588c] PID: 7109 Comm: find
> > > > > [ 1476.542013] Initiator: CPU
> > > > > [ 1476.542014] Error type: UE [Load/Store]
> > > > > [ 1476.542015] Effective address: 00007fffd2755940
> > > > > [ 1476.542016] Physical address: 000020181a080000
> > > > > [ 1476.542448] Memory failure: 0x20181a08: recovery action for dirty LRU page: Recovered
> > > > > [ 1476.542452] Memory failure: 0x20181a08: already hardware poisoned
> > > > > [ 1476.542453] Memory failure: 0x20181a08: already hardware poisoned
> > > > > [ 1476.542454] Memory failure: 0x20181a08: already hardware poisoned
> > > > > [ 1476.542455] Memory failure: 0x20181a08: already hardware poisoned
> > > > > [ 1476.542456] Memory failure: 0x20181a08: already hardware poisoned
> > > > > [ 1476.542457] Memory failure: 0x20181a08: already hardware poisoned
> > > > > [...]
> > > > > [ 1490.972174] Watchdog CPU:38 Hard LOCKUP
> > > > >
> > > > > After this patch we see:
> > > > >
> > > > > [ 325.384336] Severe Machine check interrupt [Not recovered]
> > > >
> > > > How did you test for this?
> > >
> > > By injecting cache SUE using L2 FIR register (0x1001080c).
> > >
> > > > If the error was recovered, shouldn't the
> > > > process have gotten
> > > > a SIGBUS and we should have prevented further access as a part of the handling
> > > > (memory_failure()). Do we just need a MF_MUST_KILL in the flags?
> > >
> > > We hook it up to memory_failure() through a work queue and by the time
> > > work queue kicks in, the application continues to restart and hit same
> > > NIP again and again. Every MCE again hooks the same address to memory
> > > failure work queue and throws multiple recovered MCE messages for same
> > > address. Once the memory_failure() hwpoisons the page, application gets
> > > SIGBUS and then we are fine.
> > >
> >
> > That seems quite broken and not recovered is very confusing. So effectively
> > we can never recover from a MCE UE. I think we need a notion of delayed
> > recovery then? Where we do recover, but mark is as recovered with delays?
> > We might want to revisit our recovery process and see if the recovery requires
> > to turn the MMU on, but that is for later, I suppose.
>
> The notion of being handled in the machine check return value is not
> whether the failing resource is later de-allocated or fixed, but if
> *this* particular exception was able to be corrected / processing
> resume as normal without further action.
>
> The MCE UE is not recovered just by finding its address here, so I think
> Mahesh's patch is right.
>
OK, It would nice to see a "recovered" in the output as opposed to process
killed, but the MCE is "not recovered"? The kernel bits do sound sane
Balbir Singh.
^ permalink raw reply
* Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
From: Randy Dunlap @ 2018-04-23 21:17 UTC (permalink / raw)
To: Greg KH
Cc: Bryant G. Ly, benh, mpe, arnd, corbet, seroyer, mrochs, adreznec,
fbarrat, davem, linus.walleij, akpm, mikey, pombredanne, tlfalcon,
msuchanek, linux-doc, linuxppc-dev
In-Reply-To: <20180423195310.GA10693@kroah.com>
On 04/23/18 12:53, Greg KH wrote:
> On Mon, Apr 23, 2018 at 11:38:18AM -0700, Randy Dunlap wrote:
>> On 04/23/18 07:46, Bryant G. Ly wrote:
>>> This driver is a logical device which provides an
>>> interface between the hypervisor and a management
>>> partition.
>>>
>>> This driver is to be used for the POWER Virtual
>>> Management Channel Virtual Adapter on the PowerVM
>>> platform. It provides both request/response and
>>> async message support through the /dev/ibmvmc node.
>>>
>>> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
>>> Reviewed-by: Steven Royer <seroyer@linux.vnet.ibm.com>
>>> Reviewed-by: Adam Reznechek <adreznec@linux.vnet.ibm.com>
>>> Tested-by: Taylor Jakobson <tjakobs@us.ibm.com>
>>> Tested-by: Brad Warrum <bwarrum@us.ibm.com>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>> Documentation/ioctl/ioctl-number.txt | 1 +
>>> Documentation/misc-devices/ibmvmc.txt | 161 +++
>>> MAINTAINERS | 6 +
>>> arch/powerpc/include/asm/hvcall.h | 1 +
>>> drivers/misc/Kconfig | 14 +
>>> drivers/misc/Makefile | 1 +
>>> drivers/misc/ibmvmc.c | 2415 +++++++++++++++++++++++++++++++++
>>> drivers/misc/ibmvmc.h | 209 +++
>>> 8 files changed, 2808 insertions(+)
>>> create mode 100644 Documentation/misc-devices/ibmvmc.txt
>>> create mode 100644 drivers/misc/ibmvmc.c
>>> create mode 100644 drivers/misc/ibmvmc.h
>>
>>> diff --git a/Documentation/misc-devices/ibmvmc.txt b/Documentation/misc-devices/ibmvmc.txt
>>> new file mode 100644
>>> index 0000000..bae1064
>>> --- /dev/null
>>> +++ b/Documentation/misc-devices/ibmvmc.txt
>
> Aren't we doing new documentation in .rst format instead of .txt?
I am not aware that .rst format is a *requirement* for new documentation.
??
--
~Randy
^ permalink raw reply
* Re: [PATCH v2] powerpc/mm/radix: use do/while(0) trick for single statement block
From: Randy Dunlap @ 2018-04-23 21:13 UTC (permalink / raw)
To: Mathieu Malaterre, Michael Ellerman
Cc: Benjamin Herrenschmidt, Paul Mackerras, linuxppc-dev,
linux-kernel
In-Reply-To: <20180423193638.10535-1-malat@debian.org>
On 04/23/18 12:36, Mathieu Malaterre wrote:
> In commit 7a22d6321c3d ("powerpc/mm/radix: Update command line parsing for
> disable_radix") an `if` statement was added for a possible empty body
> (prom_debug).
>
> Fix the following warning, treated as error with W=1:
>
> arch/powerpc/kernel/prom_init.c:656:46: error: suggest braces around empty body in an ‘if’ statement [-Werror=empty-body]
>
> Suggested-by: Randy Dunlap <rdunlap@infradead.org>
> Signed-off-by: Mathieu Malaterre <malat@debian.org>
Acked-by: Randy Dunlap <rdunlap@infradead.org>
Thanks.
> ---
> v2: update macro directly
>
> arch/powerpc/kernel/prom_init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index 5ae153b97d0a..7edf3aa5bc6d 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -103,7 +103,7 @@ int of_workarounds;
> #ifdef DEBUG_PROM
> #define prom_debug(x...) prom_printf(x)
> #else
> -#define prom_debug(x...)
> +#define prom_debug(x...) do { } while (0)
> #endif
>
>
>
--
~Randy
^ permalink raw reply
* Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
From: Bryant G. Ly @ 2018-04-23 21:06 UTC (permalink / raw)
To: Greg KH, Randy Dunlap
Cc: benh, mpe, arnd, corbet, seroyer, mrochs, adreznec, fbarrat,
davem, linus.walleij, akpm, mikey, pombredanne, tlfalcon,
msuchanek, linux-doc, linuxppc-dev
In-Reply-To: <20180423195310.GA10693@kroah.com>
On 4/23/18 2:53 PM, Greg KH wrote:
> On Mon, Apr 23, 2018 at 11:38:18AM -0700, Randy Dunlap wrote:
>> On 04/23/18 07:46, Bryant G. Ly wrote:
>>> This driver is a logical device which provides an
>>> interface between the hypervisor and a management
>>> partition.
>>>
>>> This driver is to be used for the POWER Virtual
>>> Management Channel Virtual Adapter on the PowerVM
>>> platform. It provides both request/response and
>>> async message support through the /dev/ibmvmc node.
>>>
>>> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
>>> Reviewed-by: Steven Royer <seroyer@linux.vnet.ibm.com>
>>> Reviewed-by: Adam Reznechek <adreznec@linux.vnet.ibm.com>
>>> Tested-by: Taylor Jakobson <tjakobs@us.ibm.com>
>>> Tested-by: Brad Warrum <bwarrum@us.ibm.com>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>>> ---
>>> Documentation/ioctl/ioctl-number.txt | 1 +
>>> Documentation/misc-devices/ibmvmc.txt | 161 +++
>>> MAINTAINERS | 6 +
>>> arch/powerpc/include/asm/hvcall.h | 1 +
>>> drivers/misc/Kconfig | 14 +
>>> drivers/misc/Makefile | 1 +
>>> drivers/misc/ibmvmc.c | 2415 +++++++++++++++++++++++++++++++++
>>> drivers/misc/ibmvmc.h | 209 +++
>>> 8 files changed, 2808 insertions(+)
>>> create mode 100644 Documentation/misc-devices/ibmvmc.txt
>>> create mode 100644 drivers/misc/ibmvmc.c
>>> create mode 100644 drivers/misc/ibmvmc.h
>>> diff --git a/Documentation/misc-devices/ibmvmc.txt b/Documentation/misc-devices/ibmvmc.txt
>>> new file mode 100644
>>> index 0000000..bae1064
>>> --- /dev/null
>>> +++ b/Documentation/misc-devices/ibmvmc.txt
> Aren't we doing new documentation in .rst format instead of .txt?
>
> thanks,
>
> greg k-h
>
I will convert to .rst and fix the comments from Randy in the next version.
Thanks,
Bryant
^ permalink raw reply
* Re: [PATCH v1 1/1] misc: IBM Virtual Management Channel Driver
From: Greg KH @ 2018-04-23 19:53 UTC (permalink / raw)
To: Randy Dunlap
Cc: Bryant G. Ly, benh, mpe, arnd, corbet, seroyer, mrochs, adreznec,
fbarrat, davem, linus.walleij, akpm, mikey, pombredanne, tlfalcon,
msuchanek, linux-doc, linuxppc-dev
In-Reply-To: <c07b14b7-8f34-2ba5-536f-85896ff21661@infradead.org>
On Mon, Apr 23, 2018 at 11:38:18AM -0700, Randy Dunlap wrote:
> On 04/23/18 07:46, Bryant G. Ly wrote:
> > This driver is a logical device which provides an
> > interface between the hypervisor and a management
> > partition.
> >
> > This driver is to be used for the POWER Virtual
> > Management Channel Virtual Adapter on the PowerVM
> > platform. It provides both request/response and
> > async message support through the /dev/ibmvmc node.
> >
> > Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> > Reviewed-by: Steven Royer <seroyer@linux.vnet.ibm.com>
> > Reviewed-by: Adam Reznechek <adreznec@linux.vnet.ibm.com>
> > Tested-by: Taylor Jakobson <tjakobs@us.ibm.com>
> > Tested-by: Brad Warrum <bwarrum@us.ibm.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > ---
> > Documentation/ioctl/ioctl-number.txt | 1 +
> > Documentation/misc-devices/ibmvmc.txt | 161 +++
> > MAINTAINERS | 6 +
> > arch/powerpc/include/asm/hvcall.h | 1 +
> > drivers/misc/Kconfig | 14 +
> > drivers/misc/Makefile | 1 +
> > drivers/misc/ibmvmc.c | 2415 +++++++++++++++++++++++++++++++++
> > drivers/misc/ibmvmc.h | 209 +++
> > 8 files changed, 2808 insertions(+)
> > create mode 100644 Documentation/misc-devices/ibmvmc.txt
> > create mode 100644 drivers/misc/ibmvmc.c
> > create mode 100644 drivers/misc/ibmvmc.h
>
> > diff --git a/Documentation/misc-devices/ibmvmc.txt b/Documentation/misc-devices/ibmvmc.txt
> > new file mode 100644
> > index 0000000..bae1064
> > --- /dev/null
> > +++ b/Documentation/misc-devices/ibmvmc.txt
Aren't we doing new documentation in .rst format instead of .txt?
thanks,
greg k-h
^ 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