* [PATCH 2/5] powerpc, process: Remove the unused extern dscr_default
2014-12-08 6:30 [PATCH 1/5] powerpc: Fix handling of DSCR related facility unavailable exception Anshuman Khandual
@ 2014-12-08 6:30 ` Anshuman Khandual
2014-12-08 6:30 ` [PATCH 3/5] powerpc, offset: Change PACA_DSCR to PACA_DSCR_DEFAULT Anshuman Khandual
` (3 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Anshuman Khandual @ 2014-12-08 6:30 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, anton
The process context switch code no longer uses dscr_default variable
from the sysfs.c file. The variable became unused when we started
storing the CPU specific DSCR value in the PACA structure instead.
This patch just removes this extern declaration. It was originally
added by the following commit.
efcac658: powerpc: Per process DSCR + some fixes (try#4)
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
arch/powerpc/kernel/process.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 923cd2d..73925a9 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1116,8 +1116,6 @@ static void setup_ksp_vsid(struct task_struct *p, unsigned long sp)
/*
* Copy a thread..
*/
-extern unsigned long dscr_default; /* defined in arch/powerpc/kernel/sysfs.c */
-
int copy_thread(unsigned long clone_flags, unsigned long usp,
unsigned long arg, struct task_struct *p)
{
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/5] powerpc, offset: Change PACA_DSCR to PACA_DSCR_DEFAULT
2014-12-08 6:30 [PATCH 1/5] powerpc: Fix handling of DSCR related facility unavailable exception Anshuman Khandual
2014-12-08 6:30 ` [PATCH 2/5] powerpc, process: Remove the unused extern dscr_default Anshuman Khandual
@ 2014-12-08 6:30 ` Anshuman Khandual
2014-12-08 6:30 ` [PATCH 4/5] powerpc, dscr: Added some in-code documentation Anshuman Khandual
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Anshuman Khandual @ 2014-12-08 6:30 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, anton
PACA_DSCR offset macro tracks dscr_default element in the paca
structure. Better change the name of this macro to match that
of the data element it tracks. Makes the code more readable.
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
arch/powerpc/kernel/asm-offsets.c | 2 +-
arch/powerpc/kernel/entry_64.S | 2 +-
arch/powerpc/kernel/tm.S | 4 ++--
arch/powerpc/kvm/book3s_hv_rmhandlers.S | 2 +-
4 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/kernel/asm-offsets.c b/arch/powerpc/kernel/asm-offsets.c
index 9d7dede..b6fd715 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -246,7 +246,7 @@ int main(void)
#endif
DEFINE(PACAHWCPUID, offsetof(struct paca_struct, hw_cpu_id));
DEFINE(PACAKEXECSTATE, offsetof(struct paca_struct, kexec_state));
- DEFINE(PACA_DSCR, offsetof(struct paca_struct, dscr_default));
+ DEFINE(PACA_DSCR_DEFAULT, offsetof(struct paca_struct, dscr_default));
DEFINE(PACA_STARTTIME, offsetof(struct paca_struct, starttime));
DEFINE(PACA_STARTTIME_USER, offsetof(struct paca_struct, starttime_user));
DEFINE(PACA_USER_TIME, offsetof(struct paca_struct, user_time));
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 0905c8d..a2a1b58 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -564,7 +564,7 @@ BEGIN_FTR_SECTION
ld r0,THREAD_DSCR(r4)
cmpwi r6,0
bne 1f
- ld r0,PACA_DSCR(r13)
+ ld r0,PACA_DSCR_DEFAULT(r13)
1:
BEGIN_FTR_SECTION_NESTED(70)
mfspr r8, SPRN_FSCR
diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
index 2a324f4..dbd2c18 100644
--- a/arch/powerpc/kernel/tm.S
+++ b/arch/powerpc/kernel/tm.S
@@ -293,7 +293,7 @@ dont_backup_fp:
ld r2, STK_GOT(r1)
/* Load CPU's default DSCR */
- ld r0, PACA_DSCR(r13)
+ ld r0, PACA_DSCR_DEFAULT(r13)
mtspr SPRN_DSCR, r0
blr
@@ -473,7 +473,7 @@ restore_gprs:
ld r2, STK_GOT(r1)
/* Load CPU's default DSCR */
- ld r0, PACA_DSCR(r13)
+ ld r0, PACA_DSCR_DEFAULT(r13)
mtspr SPRN_DSCR, r0
blr
diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index edb2ccd..6ed2cf4 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -294,7 +294,7 @@ kvm_start_guest:
beq kvm_no_guest
/* Set HSTATE_DSCR(r13) to something sensible */
- ld r6, PACA_DSCR(r13)
+ ld r6, PACA_DSCR_DEFAULT(r13)
std r6, HSTATE_DSCR(r13)
bl kvmppc_hv_entry
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/5] powerpc, dscr: Added some in-code documentation
2014-12-08 6:30 [PATCH 1/5] powerpc: Fix handling of DSCR related facility unavailable exception Anshuman Khandual
2014-12-08 6:30 ` [PATCH 2/5] powerpc, process: Remove the unused extern dscr_default Anshuman Khandual
2014-12-08 6:30 ` [PATCH 3/5] powerpc, offset: Change PACA_DSCR to PACA_DSCR_DEFAULT Anshuman Khandual
@ 2014-12-08 6:30 ` Anshuman Khandual
2014-12-09 10:03 ` [4/5] " Michael Ellerman
2014-12-08 6:30 ` [PATCH 5/5] documentation, powerpc: Add documentation for DSCR support Anshuman Khandual
2014-12-09 10:11 ` [1/5] powerpc: Fix handling of DSCR related facility unavailable exception Michael Ellerman
4 siblings, 1 reply; 10+ messages in thread
From: Anshuman Khandual @ 2014-12-08 6:30 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, anton
This patch adds some in-code documentation to the DSCR related
code to make it more readable without having any functional
change to it.
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/processor.h | 8 ++++++++
arch/powerpc/kernel/sysfs.c | 13 +++++++++++++
2 files changed, 21 insertions(+)
diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index dda7ac4..81c1aeb 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -295,6 +295,14 @@ struct thread_struct {
#endif
#ifdef CONFIG_PPC64
unsigned long dscr;
+ /*
+ * XXX: dscr_inherit indicates that the process has explicitly
+ * attempted and changed the DSCR register value for itself.
+ * Hence kernel wont use the default CPU DSCR value contained
+ * in the PACA structure anymore during process context switch.
+ * Once this variable is set, this behaviour will be inherited
+ * to all the children of this process from that point onwards.
+ */
int dscr_inherit;
unsigned long ppr; /* used to save/restore SMT priority */
#endif
diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 67fd2fd..edde3f0 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -496,8 +496,21 @@ static DEVICE_ATTR(spurr, 0400, show_spurr, NULL);
static DEVICE_ATTR(purr, 0400, show_purr, store_purr);
static DEVICE_ATTR(pir, 0400, show_pir, NULL);
+/*
+ * XXX: This is the system wide DSCR register default value.
+ * Any change to this value through the sysfs interface will
+ * update all per-cpu DSCR default values across the system
+ * stored in their respective PACA structures.
+ */
static unsigned long dscr_default;
+/*
+ * XXX: read_dscr and write_dscr are the functions for the
+ * per-cpu DSCR default sysfs files present for each cpu.
+ * Though updates to per-cpu DSCR value also gets called
+ * for all the CPUs on the system when the system wide
+ * global dscr_default gets changed.
+ */
static void read_dscr(void *val)
{
*(unsigned long *)val = get_paca()->dscr_default;
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [4/5] powerpc, dscr: Added some in-code documentation
2014-12-08 6:30 ` [PATCH 4/5] powerpc, dscr: Added some in-code documentation Anshuman Khandual
@ 2014-12-09 10:03 ` Michael Ellerman
2014-12-09 13:03 ` Anshuman Khandual
0 siblings, 1 reply; 10+ messages in thread
From: Michael Ellerman @ 2014-12-09 10:03 UTC (permalink / raw)
To: Anshuman Khandual, linuxppc-dev; +Cc: mikey, anton
On Mon, 2014-08-12 at 06:30:11 UTC, Anshuman Khandual wrote:
> This patch adds some in-code documentation to the DSCR related
> code to make it more readable without having any functional
> change to it.
Adding documentation is always good, but ...
> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
> index dda7ac4..81c1aeb 100644
> --- a/arch/powerpc/include/asm/processor.h
> +++ b/arch/powerpc/include/asm/processor.h
> @@ -295,6 +295,14 @@ struct thread_struct {
> #endif
> #ifdef CONFIG_PPC64
> unsigned long dscr;
> + /*
> + * XXX: dscr_inherit indicates that the process has explicitly
Please don't use XXX as a matter of practice.
It should be saved for *really* tricky/complicated code, and this isn't that.
> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> index 67fd2fd..edde3f0 100644
> --- a/arch/powerpc/kernel/sysfs.c
> +++ b/arch/powerpc/kernel/sysfs.c
> @@ -496,8 +496,21 @@ static DEVICE_ATTR(spurr, 0400, show_spurr, NULL);
> static DEVICE_ATTR(purr, 0400, show_purr, store_purr);
> static DEVICE_ATTR(pir, 0400, show_pir, NULL);
>
> +/*
> + * XXX: This is the system wide DSCR register default value.
> + * Any change to this value through the sysfs interface will
> + * update all per-cpu DSCR default values across the system
> + * stored in their respective PACA structures.
> + */
> static unsigned long dscr_default;
Yeah it seems you're right, writing updates the values in all pacas, reading
returns the value in the current cpu's paca. So why do we need this copy of the
value?
> +/*
> + * XXX: read_dscr and write_dscr are the functions for the
> + * per-cpu DSCR default sysfs files present for each cpu.
> + * Though updates to per-cpu DSCR value also gets called
> + * for all the CPUs on the system when the system wide
> + * global dscr_default gets changed.
> + */
> static void read_dscr(void *val)
> {
Please make these proper kernel-doc comments. I've definitely asked you to do
that at least once before on a different patch, to check you can do:
$ ./scripts/kernel-doc -text arch/powerpc/kernel/sysfs.c
The comments for write_dscr() should be attached to that function.
cheers
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [4/5] powerpc, dscr: Added some in-code documentation
2014-12-09 10:03 ` [4/5] " Michael Ellerman
@ 2014-12-09 13:03 ` Anshuman Khandual
0 siblings, 0 replies; 10+ messages in thread
From: Anshuman Khandual @ 2014-12-09 13:03 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: mikey, anton
On 12/09/2014 03:33 PM, Michael Ellerman wrote:
> On Mon, 2014-08-12 at 06:30:11 UTC, Anshuman Khandual wrote:
>> This patch adds some in-code documentation to the DSCR related
>> code to make it more readable without having any functional
>> change to it.
>
> Adding documentation is always good, but ...
>
>> diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
>> index dda7ac4..81c1aeb 100644
>> --- a/arch/powerpc/include/asm/processor.h
>> +++ b/arch/powerpc/include/asm/processor.h
>> @@ -295,6 +295,14 @@ struct thread_struct {
>> #endif
>> #ifdef CONFIG_PPC64
>> unsigned long dscr;
>> + /*
>> + * XXX: dscr_inherit indicates that the process has explicitly
>
> Please don't use XXX as a matter of practice.
>
> It should be saved for *really* tricky/complicated code, and this isn't that.
Sure, got it. Will remove them.
>
>> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
>> index 67fd2fd..edde3f0 100644
>> --- a/arch/powerpc/kernel/sysfs.c
>> +++ b/arch/powerpc/kernel/sysfs.c
>> @@ -496,8 +496,21 @@ static DEVICE_ATTR(spurr, 0400, show_spurr, NULL);
>> static DEVICE_ATTR(purr, 0400, show_purr, store_purr);
>> static DEVICE_ATTR(pir, 0400, show_pir, NULL);
>>
>> +/*
>> + * XXX: This is the system wide DSCR register default value.
>> + * Any change to this value through the sysfs interface will
>> + * update all per-cpu DSCR default values across the system
>> + * stored in their respective PACA structures.
>> + */
>> static unsigned long dscr_default;
>
> Yeah it seems you're right, writing updates the values in all pacas, reading
> returns the value in the current cpu's paca. So why do we need this copy of the
> value?
My comment here might be little confusing. The read_dscr/write_dscr functions
are used for per-CPU PACA DSCR values which can read/update the per-CPU PACA
variable directly. The functions show_dscr_default/store_dscr_default are used
to read/update the system wide DSCR default which is the above 'dscr_default'
variable. Function store_dscr_default also calls write_dscr to update PACA on
every CPU present on the system. I will re-write the code comment to make more
sense.
>
>> +/*
>> + * XXX: read_dscr and write_dscr are the functions for the
>> + * per-cpu DSCR default sysfs files present for each cpu.
>> + * Though updates to per-cpu DSCR value also gets called
>> + * for all the CPUs on the system when the system wide
>> + * global dscr_default gets changed.
>> + */
>> static void read_dscr(void *val)
>> {
>
> Please make these proper kernel-doc comments. I've definitely asked you to do
> that at least once before on a different patch, to check you can do:
Yes you had. Thought that this function is too small but as you said if we
are writing documentation for it we should write kernel-doc format only.
Will make sure about this now onward.
>
> $ ./scripts/kernel-doc -text arch/powerpc/kernel/sysfs.c
>
> The comments for write_dscr() should be attached to that function.
Sure.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 5/5] documentation, powerpc: Add documentation for DSCR support
2014-12-08 6:30 [PATCH 1/5] powerpc: Fix handling of DSCR related facility unavailable exception Anshuman Khandual
` (2 preceding siblings ...)
2014-12-08 6:30 ` [PATCH 4/5] powerpc, dscr: Added some in-code documentation Anshuman Khandual
@ 2014-12-08 6:30 ` Anshuman Khandual
2014-12-09 10:11 ` [1/5] powerpc: Fix handling of DSCR related facility unavailable exception Michael Ellerman
4 siblings, 0 replies; 10+ messages in thread
From: Anshuman Khandual @ 2014-12-08 6:30 UTC (permalink / raw)
To: linuxppc-dev; +Cc: mikey, anton
This patch adds a new documentation file explaining the DSCR
support on powerpc platforms. This explains DSCR related data
structure, code paths and also available user interfaces. Any
further functional changes to the DSCR support in the kernel
should definitely update the documentation here.
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
Documentation/powerpc/00-INDEX | 2 +
Documentation/powerpc/dscr.txt | 83 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 85 insertions(+)
create mode 100644 Documentation/powerpc/dscr.txt
diff --git a/Documentation/powerpc/00-INDEX b/Documentation/powerpc/00-INDEX
index 6fd0e8b..9dc845c 100644
--- a/Documentation/powerpc/00-INDEX
+++ b/Documentation/powerpc/00-INDEX
@@ -30,3 +30,5 @@ ptrace.txt
- Information on the ptrace interfaces for hardware debug registers.
transactional_memory.txt
- Overview of the Power8 transactional memory support.
+dscr.txt
+ - Overview DSCR (Data Stream Control Register) support.
diff --git a/Documentation/powerpc/dscr.txt b/Documentation/powerpc/dscr.txt
new file mode 100644
index 0000000..1ff4400
--- /dev/null
+++ b/Documentation/powerpc/dscr.txt
@@ -0,0 +1,83 @@
+ DSCR (Data Stream Control Register)
+ ================================================
+
+DSCR register in powerpc allows user to have some control of prefetch of data
+stream in the processor. Please refer to the ISA documents or related manual
+for more detailed information regarding how to use this DSCR to attain this
+control of the pefetches . This document here provides an overview of kernel
+support for DSCR, related kernel objects, it's functionalities and exported
+user interface.
+
+(A) Data Structures:
+
+ (1) thread_struct:
+ dscr /* Thread DSCR value */
+ dscr_inherit /* Thread has changed default DSCR */
+
+ (2) PACA:
+ dscr_default /* per-CPU DSCR default value */
+
+ (3) sysfs.c:
+ dscr_default /* System DSCR default value */
+
+(B) Scheduler Changes:
+
+ Scheduler will write the per-CPU DSCR default which is stored in the
+ CPU's PACA value into the register if the thread has dscr_inherit value
+ cleared which means that it has not changed the default DSCR till now.
+ If the dscr_inherit value is set which means that it has changed the
+ default DSCR value, scheduler will write the changed value which will
+ now be contained in thread struct's dscr into the register instead of
+ the per-CPU default PACA based DSCR value.
+
+ NOTE: Please note here that the system wide global DSCR value never
+ gets used directly in the scheduler process context switch at all.
+
+(C) SYSFS Interface:
+
+ Global DSCR default: /sys/devices/system/cpu/dscr_default
+ CPU specific DSCR default: /sys/devices/system/cpu/cpuN/dscr
+
+ Changing the global DSCR default in the sysfs will change all the CPU
+ specific DSCR defaults immediately in their PACA structures. Again if
+ the current process has the dscr_inherit clear, it also writes the new
+ value into every CPU's DSCR register right away and updates the current
+ thread's DSCR value as well.
+
+ Changing the CPU specif DSCR default value in the sysfs does exactly
+ the same thing as above but unlike the global one above, it just changes
+ stuff for that particular CPU instead for all the CPUs on the system.
+
+(D) User Space Instructions:
+
+ The DSCR register can be accessed in the user space using any of these
+ two SPR numbers available for that purpose.
+
+ (1) Problem state SPR: 0x03 (Un-privileged, POWER8 only)
+ (2) Privileged state SPR: 0x11 (Privileged)
+
+ Accessing DSCR through privileged SPR number (0x11) from user space
+ works, as it is emulated following an illegal instruction exception
+ inside the kernel. Both mfspr and mtspr instructions are emulated.
+
+ Accessing DSCR through user level SPR (0x03) from user space will first
+ create a facility unavailable exception. Inside this exception handler
+ all mfspr isntruction based read attempts will get emulated and returned
+ where as the first mtspr instruction based write attempts will enable
+ the DSCR facility for the next time around (both for read and write) by
+ setting DSCR facility in the FSCR register.
+
+(E) Specifics about 'dscr_inherit':
+
+ The thread struct element 'dscr_inherit' represents whether the thread
+ in question has attempted and changed the DSCR itself using any of the
+ following methods. This element signifies whether the thread wants to
+ use the CPU default DSCR value or its own changed DSCR value in the
+ kernel.
+
+ (1) mtspr instruction (SPR number 0x03)
+ (2) mtspr instruction (SPR number 0x11)
+ (3) ptrace interface (Explicitly set user DSCR value)
+
+ Any child of the process created after this event in the process inherits
+ this same behaviour as well.
--
1.9.3
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [1/5] powerpc: Fix handling of DSCR related facility unavailable exception
2014-12-08 6:30 [PATCH 1/5] powerpc: Fix handling of DSCR related facility unavailable exception Anshuman Khandual
` (3 preceding siblings ...)
2014-12-08 6:30 ` [PATCH 5/5] documentation, powerpc: Add documentation for DSCR support Anshuman Khandual
@ 2014-12-09 10:11 ` Michael Ellerman
2014-12-09 13:15 ` Anshuman Khandual
4 siblings, 1 reply; 10+ messages in thread
From: Michael Ellerman @ 2014-12-09 10:11 UTC (permalink / raw)
To: Anshuman Khandual, linuxppc-dev; +Cc: mikey, anton
On Mon, 2014-08-12 at 06:30:08 UTC, Anshuman Khandual wrote:
> Currently DSCR (Data Stream Control Register) can be accessed with
> mfspr or mtspr instructions inside a thread via two different SPR
> numbers. One being the user accessible problem state SPR number 0x03
> and the other being the privilege state SPR number 0x11. All access
> through the privilege state SPR number get emulated through illegal
> instruction exception. Any access through the problem state SPR number
> raises one facility unavailable exception which sets the thread based
> dscr_inherit bit and enables DSCR facility through FSCR register thus
> allowing direct access to DSCR without going through this exception in
> the future. We set the thread.dscr_inherit bit whether the access was
> with mfspr or mtspr instruction which is neither correct nor does it
> match the behaviour through the instruction emulation code path driven
> from privilege state SPR number. User currently observes two different
> kind of behaviour when accessing the DSCR through these two SPR numbers.
> This problem can be observed through these two test cases by replacing
> the privilege state SPR number with the problem state SPR number.
>
> (1) http://ozlabs.org/~anton/junkcode/dscr_default_test.c
> (2) http://ozlabs.org/~anton/junkcode/dscr_explicit_test.c
Can you convert those into a selftest please?
cheers
^ permalink raw reply [flat|nested] 10+ messages in thread