* [PATCH V2] powerpc/kernel/sysfs: disable writing to purr in non-powernv
@ 2013-09-27 11:33 Madhavan Srinivasan
2013-10-01 6:31 ` Michael Ellerman
0 siblings, 1 reply; 5+ messages in thread
From: Madhavan Srinivasan @ 2013-09-27 11:33 UTC (permalink / raw)
To: benh; +Cc: Madhavan Srinivasan, linuxppc-dev
powerpc/kernel/sysfs.c exports purr with write permission.
This is only valid for kernel in hypervisor mode.
But writing to the file in PowerVM lpar causes crash.
# echo 0 > purr
cpu 0x0: Vector: 700 (Program Check) at [c000000000d072b0]
pc: c00000000001770c: .write_purr+0x1c/0x40
lr: c000000000017708: .write_purr+0x18/0x40
sp: c000000000d07530
msr: 8000000000049032
current = 0xc000000000c53de0
paca = 0xc00000000ec70000 softe: 0 irq_happened: 0x01
pid = 0, comm = swapper/0
enter ? for help
[c000000000d075b0] c0000000000fba64
.generic_smp_call_function_single_interrupt+0x104/0x190
[c000000000d07650] c000000000037748 .smp_ipi_demux+0xa8/0xf0
[c000000000d076e0] c000000000035314 .doorbell_exception+0x74/0xb0
[c000000000d07760] c000000000002950 doorbell_super_common+0x150/0x180
--- Exception: a01 (Doorbell) at c000000000060904
.plpar_hcall_norets+0x84/0xd4
[link register ] c00000000006dbd4 .check_and_cede_processor+0x24/0x40
[c000000000d07a50] c000000001002558 (unreliable)
[c000000000d07ac0] c00000000006dd0c .shared_cede_loop+0x2c/0x70
[c000000000d07b40] c0000000006ae954 .cpuidle_enter_state+0x64/0x150
[c000000000d07c00] c0000000006aeb30 .cpuidle_idle_call+0xf0/0x300
[c000000000d07cb0] c000000000062fa0 .pseries_lpar_idle+0x10/0x50
[c000000000d07d20] c000000000016d14 .arch_cpu_idle+0x64/0x150
[c000000000d07da0] c0000000000e0060 .cpu_startup_entry+0x1a0/0x2c0
[c000000000d07e80] c00000000000bca4 .rest_init+0x94/0xb0
[c000000000d07ef0] c000000000b54530 .start_kernel+0x478/0x494
[c000000000d07f90] c000000000009be0 .start_here_common+0x20/0x40
0:mon>
Changes:
1)Changed the test for to hypervisor mode instead of platform
Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
arch/powerpc/kernel/sysfs.c | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 27a90b9..42becfa 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -17,6 +17,7 @@
#include <asm/machdep.h>
#include <asm/smp.h>
#include <asm/pmc.h>
+#include <asm/reg.h>
#include "cacheinfo.h"
@@ -179,15 +180,27 @@ SYSFS_PMCSETUP(spurr, SPRN_SPURR);
SYSFS_PMCSETUP(dscr, SPRN_DSCR);
SYSFS_PMCSETUP(pir, SPRN_PIR);
+/*
+ Lets only enable Read for Hyp resources and
+ enable Write when needed with a separate function.
+ Lets be conservative and default to pseries.
+*/
static DEVICE_ATTR(mmcra, 0600, show_mmcra, store_mmcra);
static DEVICE_ATTR(spurr, 0400, show_spurr, NULL);
static DEVICE_ATTR(dscr, 0600, show_dscr, store_dscr);
-static DEVICE_ATTR(purr, 0600, show_purr, store_purr);
+static DEVICE_ATTR(purr, 0400, show_purr, store_purr);
static DEVICE_ATTR(pir, 0400, show_pir, NULL);
unsigned long dscr_default = 0;
EXPORT_SYMBOL(dscr_default);
+static void add_write_permission_dev_attr(void *ptr)
+{
+ struct device_attribute *attr = (struct device_attribute *)ptr;
+
+ attr->attr.mode |= (unsigned short) 0200;
+}
+
static ssize_t show_dscr_default(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -394,8 +407,11 @@ static void register_cpu_online(unsigned int cpu)
if (cpu_has_feature(CPU_FTR_MMCRA))
device_create_file(s, &dev_attr_mmcra);
- if (cpu_has_feature(CPU_FTR_PURR))
+ if (cpu_has_feature(CPU_FTR_PURR)) {
+ if (mfmsr() & MSR_HV)
+ add_write_permission_dev_attr((void *)&dev_attr_purr);
device_create_file(s, &dev_attr_purr);
+ }
if (cpu_has_feature(CPU_FTR_SPURR))
device_create_file(s, &dev_attr_spurr);
--
1.7.10.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH V2] powerpc/kernel/sysfs: disable writing to purr in non-powernv
2013-09-27 11:33 [PATCH V2] powerpc/kernel/sysfs: disable writing to purr in non-powernv Madhavan Srinivasan
@ 2013-10-01 6:31 ` Michael Ellerman
2013-10-01 7:50 ` Benjamin Herrenschmidt
2013-10-01 8:09 ` Madhavan Srinivasan
0 siblings, 2 replies; 5+ messages in thread
From: Michael Ellerman @ 2013-10-01 6:31 UTC (permalink / raw)
To: Madhavan Srinivasan; +Cc: linuxppc-dev
Hi Maddy,
On Fri, Sep 27, 2013 at 05:03:54PM +0530, Madhavan Srinivasan wrote:
> powerpc/kernel/sysfs.c exports purr with write permission.
PURR
> This is only valid for kernel in hypervisor mode.
> But writing to the file in PowerVM lpar causes crash.
In the kernel history/source we refer to it as "phyp". However in this
case it's not an issue with phyp, it's simply that you are not in
hypervisor mode, ie. the same crash would occur under KVM.
So you should just say "writing to the file in guest mode .."
> # echo 0 > purr
> cpu 0x0: Vector: 700 (Program Check) at [c000000000d072b0]
> pc: c00000000001770c: .write_purr+0x1c/0x40
> lr: c000000000017708: .write_purr+0x18/0x40
> sp: c000000000d07530
> msr: 8000000000049032
> current = 0xc000000000c53de0
> paca = 0xc00000000ec70000 softe: 0 irq_happened: 0x01
> pid = 0, comm = swapper/0
> enter ? for help
> [c000000000d075b0] c0000000000fba64
> .generic_smp_call_function_single_interrupt+0x104/0x190
> [c000000000d07650] c000000000037748 .smp_ipi_demux+0xa8/0xf0
> [c000000000d076e0] c000000000035314 .doorbell_exception+0x74/0xb0
> [c000000000d07760] c000000000002950 doorbell_super_common+0x150/0x180
> --- Exception: a01 (Doorbell) at c000000000060904
> .plpar_hcall_norets+0x84/0xd4
> [link register ] c00000000006dbd4 .check_and_cede_processor+0x24/0x40
> [c000000000d07a50] c000000001002558 (unreliable)
> [c000000000d07ac0] c00000000006dd0c .shared_cede_loop+0x2c/0x70
> [c000000000d07b40] c0000000006ae954 .cpuidle_enter_state+0x64/0x150
> [c000000000d07c00] c0000000006aeb30 .cpuidle_idle_call+0xf0/0x300
> [c000000000d07cb0] c000000000062fa0 .pseries_lpar_idle+0x10/0x50
> [c000000000d07d20] c000000000016d14 .arch_cpu_idle+0x64/0x150
> [c000000000d07da0] c0000000000e0060 .cpu_startup_entry+0x1a0/0x2c0
> [c000000000d07e80] c00000000000bca4 .rest_init+0x94/0xb0
> [c000000000d07ef0] c000000000b54530 .start_kernel+0x478/0x494
> [c000000000d07f90] c000000000009be0 .start_here_common+0x20/0x40
> 0:mon>
>
> Changes:
>
> 1)Changed the test for to hypervisor mode instead of platform
I think Ben's wrong about that.
Almost all existing code uses FW_FEATURE_LPAR to differentiate
hypervisor vs guest mode, so I think we should do the same here.
So it would be:
> + if (cpu_has_feature(CPU_FTR_PURR)) {
> + if (!firmware_has_feature(FW_FEATURE_LPAR))
> + add_write_permission_dev_attr((void *)&dev_attr_purr);
> device_create_file(s, &dev_attr_purr);
> + }
> +static void add_write_permission_dev_attr(void *ptr)
> +{
> + struct device_attribute *attr = (struct device_attribute *)ptr;
> +
> + attr->attr.mode |= (unsigned short) 0200;
> +}
Why does it take a void *, which then requires a cast at the call site?
And do you need the cast to short? If so shouldn't you use umode_t
directly?
cheers
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V2] powerpc/kernel/sysfs: disable writing to purr in non-powernv
2013-10-01 6:31 ` Michael Ellerman
@ 2013-10-01 7:50 ` Benjamin Herrenschmidt
2013-10-01 8:43 ` Michael Ellerman
2013-10-01 8:09 ` Madhavan Srinivasan
1 sibling, 1 reply; 5+ messages in thread
From: Benjamin Herrenschmidt @ 2013-10-01 7:50 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Madhavan Srinivasan, linuxppc-dev
On Tue, 2013-10-01 at 16:31 +1000, Michael Ellerman wrote:
> > 1)Changed the test for to hypervisor mode instead of platform
>
> I think Ben's wrong about that.
>
> Almost all existing code uses FW_FEATURE_LPAR to differentiate
> hypervisor vs guest mode, so I think we should do the same here.
I didn't object to using the FW test, it's a reasonable way to do it, I
objected to using the platform as an indication (powernv vs. pseries)
Ben.
> So it would be:
>
> > + if (cpu_has_feature(CPU_FTR_PURR)) {
> > + if (!firmware_has_feature(FW_FEATURE_LPAR))
> > + add_write_permission_dev_attr((void *)&dev_attr_purr);
> > device_create_file(s, &dev_attr_purr);
> > + }
>
>
> > +static void add_write_permission_dev_attr(void *ptr)
> > +{
> > + struct device_attribute *attr = (struct device_attribute *)ptr;
> > +
> > + attr->attr.mode |= (unsigned short) 0200;
> > +}
>
> Why does it take a void *, which then requires a cast at the call site?
>
> And do you need the cast to short? If so shouldn't you use umode_t
> directly?
>
> cheers
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V2] powerpc/kernel/sysfs: disable writing to purr in non-powernv
2013-10-01 7:50 ` Benjamin Herrenschmidt
@ 2013-10-01 8:43 ` Michael Ellerman
0 siblings, 0 replies; 5+ messages in thread
From: Michael Ellerman @ 2013-10-01 8:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Madhavan Srinivasan, linuxppc-dev
On Tue, Oct 01, 2013 at 05:50:30PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2013-10-01 at 16:31 +1000, Michael Ellerman wrote:
>
> > > 1)Changed the test for to hypervisor mode instead of platform
> >
> > I think Ben's wrong about that.
> >
> > Almost all existing code uses FW_FEATURE_LPAR to differentiate
> > hypervisor vs guest mode, so I think we should do the same here.
>
> I didn't object to using the FW test, it's a reasonable way to do it, I
> objected to using the platform as an indication (powernv vs. pseries)
Yeah sorry I wasn't clear. You are right that it shouldn't use the
platform. So I think we agree FW_FEATURE is the right way to go.
cheers
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH V2] powerpc/kernel/sysfs: disable writing to purr in non-powernv
2013-10-01 6:31 ` Michael Ellerman
2013-10-01 7:50 ` Benjamin Herrenschmidt
@ 2013-10-01 8:09 ` Madhavan Srinivasan
1 sibling, 0 replies; 5+ messages in thread
From: Madhavan Srinivasan @ 2013-10-01 8:09 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
On Tuesday 01 October 2013 12:01 PM, Michael Ellerman wrote:
> Hi Maddy,
>
> On Fri, Sep 27, 2013 at 05:03:54PM +0530, Madhavan Srinivasan wrote:
>> powerpc/kernel/sysfs.c exports purr with write permission.
>
> PURR
>
>> This is only valid for kernel in hypervisor mode.
>> But writing to the file in PowerVM lpar causes crash.
>
> In the kernel history/source we refer to it as "phyp". However in this
Will follow the convention.
> case it's not an issue with phyp, it's simply that you are not in
> hypervisor mode, ie. the same crash would occur under KVM.
>
> So you should just say "writing to the file in guest mode .."
>
Sure. Will make the changes.
>> # echo 0 > purr
>> cpu 0x0: Vector: 700 (Program Check) at [c000000000d072b0]
>> pc: c00000000001770c: .write_purr+0x1c/0x40
>> lr: c000000000017708: .write_purr+0x18/0x40
>> sp: c000000000d07530
>> msr: 8000000000049032
>> current = 0xc000000000c53de0
>> paca = 0xc00000000ec70000 softe: 0 irq_happened: 0x01
>> pid = 0, comm = swapper/0
>> enter ? for help
>> [c000000000d075b0] c0000000000fba64
>> .generic_smp_call_function_single_interrupt+0x104/0x190
>> [c000000000d07650] c000000000037748 .smp_ipi_demux+0xa8/0xf0
>> [c000000000d076e0] c000000000035314 .doorbell_exception+0x74/0xb0
>> [c000000000d07760] c000000000002950 doorbell_super_common+0x150/0x180
>> --- Exception: a01 (Doorbell) at c000000000060904
>> .plpar_hcall_norets+0x84/0xd4
>> [link register ] c00000000006dbd4 .check_and_cede_processor+0x24/0x40
>> [c000000000d07a50] c000000001002558 (unreliable)
>> [c000000000d07ac0] c00000000006dd0c .shared_cede_loop+0x2c/0x70
>> [c000000000d07b40] c0000000006ae954 .cpuidle_enter_state+0x64/0x150
>> [c000000000d07c00] c0000000006aeb30 .cpuidle_idle_call+0xf0/0x300
>> [c000000000d07cb0] c000000000062fa0 .pseries_lpar_idle+0x10/0x50
>> [c000000000d07d20] c000000000016d14 .arch_cpu_idle+0x64/0x150
>> [c000000000d07da0] c0000000000e0060 .cpu_startup_entry+0x1a0/0x2c0
>> [c000000000d07e80] c00000000000bca4 .rest_init+0x94/0xb0
>> [c000000000d07ef0] c000000000b54530 .start_kernel+0x478/0x494
>> [c000000000d07f90] c000000000009be0 .start_here_common+0x20/0x40
>> 0:mon>
>>
>> Changes:
>>
>> 1)Changed the test for to hypervisor mode instead of platform
>
> I think Ben's wrong about that.
>
> Almost all existing code uses FW_FEATURE_LPAR to differentiate
> hypervisor vs guest mode, so I think we should do the same here.
>
> So it would be:
>
>> + if (cpu_has_feature(CPU_FTR_PURR)) {
>> + if (!firmware_has_feature(FW_FEATURE_LPAR))
>> + add_write_permission_dev_attr((void *)&dev_attr_purr);
>> device_create_file(s, &dev_attr_purr);
>> + }
>
Will modify the check.
>
>> +static void add_write_permission_dev_attr(void *ptr)
>> +{
>> + struct device_attribute *attr = (struct device_attribute *)ptr;
>> +
>> + attr->attr.mode |= (unsigned short) 0200;
>> +}
>
> Why does it take a void *, which then requires a cast at the call site?
>
just prefered to send the address as void.
> And do you need the cast to short? If so shouldn't you use umode_t
> directly?
No, not really.
Will make the changes and will resend the patch.
>
> cheers
>
Thanks for feedback.
Maddy
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-10-01 8:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-27 11:33 [PATCH V2] powerpc/kernel/sysfs: disable writing to purr in non-powernv Madhavan Srinivasan
2013-10-01 6:31 ` Michael Ellerman
2013-10-01 7:50 ` Benjamin Herrenschmidt
2013-10-01 8:43 ` Michael Ellerman
2013-10-01 8:09 ` Madhavan Srinivasan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).