* [PATCH] parisc: Restore __ldcw_align for PA-RISC 2.0 processors
@ 2023-09-19 17:51 John David Anglin
2023-09-19 18:31 ` Sam James
0 siblings, 1 reply; 3+ messages in thread
From: John David Anglin @ 2023-09-19 17:51 UTC (permalink / raw)
To: linux-parisc; +Cc: Helge Deller, James Bottomley
[-- Attachment #1: Type: text/plain, Size: 4187 bytes --]
Back in 2005, Kyle McMartin removed the 16-byte alignment for ldcw
semaphores on PA 2.0 machines (CONFIG_PA20). This broke spinlocks
on pre PA8800 processors. The main symptom was random faults in
mmap'd memory (e.g., gcc compilations, etc).
Unfortunately, the errata for this ldcw change is lost.
The issue is the 16-byte alignment required for ldcw semaphore
instructions can only reduced to natural alignment when the ldcw
operation can be handled coherently in cache. Only PA8800 and
PA8900 processors actually support doing the operation in cache.
Aligning the spinlock dynamically adds two integer instructions
to each spinlock.
Tested on rp3440, c8000 and a500.
Signed-off-by: John David Anglin <dave.anglin@bell.net>
---
diff --git a/arch/parisc/include/asm/ldcw.h b/arch/parisc/include/asm/ldcw.h
index 6d28b5514699..ee9e071859b2 100644
--- a/arch/parisc/include/asm/ldcw.h
+++ b/arch/parisc/include/asm/ldcw.h
@@ -2,39 +2,42 @@
#ifndef __PARISC_LDCW_H
#define __PARISC_LDCW_H
-#ifndef CONFIG_PA20
/* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
and GCC only guarantees 8-byte alignment for stack locals, we can't
be assured of 16-byte alignment for atomic lock data even if we
specify "__attribute ((aligned(16)))" in the type declaration. So,
we use a struct containing an array of four ints for the atomic lock
type and dynamically select the 16-byte aligned int from the array
- for the semaphore. */
+ for the semaphore. */
+
+/* From: "Jim Hull" <jim.hull of hp.com>
+ I've attached a summary of the change, but basically, for PA 2.0, as
+ long as the ",CO" (coherent operation) completer is implemented, then the
+ 16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
+ they only require "natural" alignment (4-byte for ldcw, 8-byte for
+ ldcd).
+
+ Although the cache control hint is accepted by all PA 2.0 processors,
+ it is only implemented on PA8800/PA8900 CPUs. Prior PA8X00 CPUs still
+ require 16-byte alignment. If the address is unaligned, the operation
+ of the instruction is undefined. The ldcw instruction does not generate
+ unaligned data reference traps so misaligned accesses are not detected.
+ This hid the problem for years. So, restore the 16-byte alignment dropped
+ by Kyle McMartin in "Remove __ldcw_align for PA-RISC 2.0 processors". */
#define __PA_LDCW_ALIGNMENT 16
-#define __PA_LDCW_ALIGN_ORDER 4
#define __ldcw_align(a) ({ \
unsigned long __ret = (unsigned long) &(a)->lock[0]; \
__ret = (__ret + __PA_LDCW_ALIGNMENT - 1) \
& ~(__PA_LDCW_ALIGNMENT - 1); \
(volatile unsigned int *) __ret; \
})
-#define __LDCW "ldcw"
-#else /*CONFIG_PA20*/
-/* From: "Jim Hull" <jim.hull of hp.com>
- I've attached a summary of the change, but basically, for PA 2.0, as
- long as the ",CO" (coherent operation) completer is specified, then the
- 16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
- they only require "natural" alignment (4-byte for ldcw, 8-byte for
- ldcd). */
-
-#define __PA_LDCW_ALIGNMENT 4
-#define __PA_LDCW_ALIGN_ORDER 2
-#define __ldcw_align(a) (&(a)->slock)
+#ifdef CONFIG_PA20
#define __LDCW "ldcw,co"
-
-#endif /*!CONFIG_PA20*/
+#else
+#define __LDCW "ldcw"
+#endif
/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.
We don't explicitly expose that "*a" may be written as reload
diff --git a/arch/parisc/include/asm/spinlock_types.h b/arch/parisc/include/asm/spinlock_types.h
index efd06a897c6a..7b986b09dba8 100644
--- a/arch/parisc/include/asm/spinlock_types.h
+++ b/arch/parisc/include/asm/spinlock_types.h
@@ -9,15 +9,10 @@
#ifndef __ASSEMBLY__
typedef struct {
-#ifdef CONFIG_PA20
- volatile unsigned int slock;
-# define __ARCH_SPIN_LOCK_UNLOCKED { __ARCH_SPIN_LOCK_UNLOCKED_VAL }
-#else
volatile unsigned int lock[4];
# define __ARCH_SPIN_LOCK_UNLOCKED \
{ { __ARCH_SPIN_LOCK_UNLOCKED_VAL, __ARCH_SPIN_LOCK_UNLOCKED_VAL, \
__ARCH_SPIN_LOCK_UNLOCKED_VAL, __ARCH_SPIN_LOCK_UNLOCKED_VAL } }
-#endif
} arch_spinlock_t;
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] parisc: Restore __ldcw_align for PA-RISC 2.0 processors
2023-09-19 17:51 [PATCH] parisc: Restore __ldcw_align for PA-RISC 2.0 processors John David Anglin
@ 2023-09-19 18:31 ` Sam James
2023-09-19 19:36 ` John David Anglin
0 siblings, 1 reply; 3+ messages in thread
From: Sam James @ 2023-09-19 18:31 UTC (permalink / raw)
To: John David Anglin; +Cc: linux-parisc, Helge Deller, James Bottomley
John David Anglin <dave@parisc-linux.org> writes:
> [[PGP Signed Part:Undecided]]
> Back in 2005, Kyle McMartin removed the 16-byte alignment for ldcw
> semaphores on PA 2.0 machines (CONFIG_PA20). This broke spinlocks
> on pre PA8800 processors. The main symptom was random faults in
> mmap'd memory (e.g., gcc compilations, etc).
>
> Unfortunately, the errata for this ldcw change is lost.
>
> The issue is the 16-byte alignment required for ldcw semaphore
> instructions can only reduced to natural alignment when the ldcw
> operation can be handled coherently in cache. Only PA8800 and
> PA8900 processors actually support doing the operation in cache.
>
> Aligning the spinlock dynamically adds two integer instructions
> to each spinlock.
>
> Tested on rp3440, c8000 and a500.
Could you include a 'Fixes: <whatever the ancient commit hash is>'?
Also, what a find!
>
> Signed-off-by: John David Anglin <dave.anglin@bell.net>
> ---
>
> diff --git a/arch/parisc/include/asm/ldcw.h b/arch/parisc/include/asm/ldcw.h
> index 6d28b5514699..ee9e071859b2 100644
> --- a/arch/parisc/include/asm/ldcw.h
> +++ b/arch/parisc/include/asm/ldcw.h
> @@ -2,39 +2,42 @@
> #ifndef __PARISC_LDCW_H
> #define __PARISC_LDCW_H
>
> -#ifndef CONFIG_PA20
> /* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
> and GCC only guarantees 8-byte alignment for stack locals, we can't
> be assured of 16-byte alignment for atomic lock data even if we
> specify "__attribute ((aligned(16)))" in the type declaration. So,
> we use a struct containing an array of four ints for the atomic lock
> type and dynamically select the 16-byte aligned int from the array
> - for the semaphore. */
> + for the semaphore. */
> +
> +/* From: "Jim Hull" <jim.hull of hp.com>
> + I've attached a summary of the change, but basically, for PA 2.0, as
> + long as the ",CO" (coherent operation) completer is implemented, then the
> + 16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
> + they only require "natural" alignment (4-byte for ldcw, 8-byte for
> + ldcd).
> +
> + Although the cache control hint is accepted by all PA 2.0 processors,
> + it is only implemented on PA8800/PA8900 CPUs. Prior PA8X00 CPUs still
> + require 16-byte alignment. If the address is unaligned, the operation
> + of the instruction is undefined. The ldcw instruction does not generate
> + unaligned data reference traps so misaligned accesses are not detected.
> + This hid the problem for years. So, restore the 16-byte alignment dropped
> + by Kyle McMartin in "Remove __ldcw_align for PA-RISC 2.0 processors". */
>
> #define __PA_LDCW_ALIGNMENT 16
> -#define __PA_LDCW_ALIGN_ORDER 4
> #define __ldcw_align(a) ({ \
> unsigned long __ret = (unsigned long) &(a)->lock[0]; \
> __ret = (__ret + __PA_LDCW_ALIGNMENT - 1) \
> & ~(__PA_LDCW_ALIGNMENT - 1); \
> (volatile unsigned int *) __ret; \
> })
> -#define __LDCW "ldcw"
>
> -#else /*CONFIG_PA20*/
> -/* From: "Jim Hull" <jim.hull of hp.com>
> - I've attached a summary of the change, but basically, for PA 2.0, as
> - long as the ",CO" (coherent operation) completer is specified, then the
> - 16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
> - they only require "natural" alignment (4-byte for ldcw, 8-byte for
> - ldcd). */
> -
> -#define __PA_LDCW_ALIGNMENT 4
> -#define __PA_LDCW_ALIGN_ORDER 2
> -#define __ldcw_align(a) (&(a)->slock)
> +#ifdef CONFIG_PA20
> #define __LDCW "ldcw,co"
> -
> -#endif /*!CONFIG_PA20*/
> +#else
> +#define __LDCW "ldcw"
> +#endif
>
> /* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.
> We don't explicitly expose that "*a" may be written as reload
> diff --git a/arch/parisc/include/asm/spinlock_types.h b/arch/parisc/include/asm/spinlock_types.h
> index efd06a897c6a..7b986b09dba8 100644
> --- a/arch/parisc/include/asm/spinlock_types.h
> +++ b/arch/parisc/include/asm/spinlock_types.h
> @@ -9,15 +9,10 @@
> #ifndef __ASSEMBLY__
>
> typedef struct {
> -#ifdef CONFIG_PA20
> - volatile unsigned int slock;
> -# define __ARCH_SPIN_LOCK_UNLOCKED { __ARCH_SPIN_LOCK_UNLOCKED_VAL }
> -#else
> volatile unsigned int lock[4];
> # define __ARCH_SPIN_LOCK_UNLOCKED \
> { { __ARCH_SPIN_LOCK_UNLOCKED_VAL, __ARCH_SPIN_LOCK_UNLOCKED_VAL, \
> __ARCH_SPIN_LOCK_UNLOCKED_VAL, __ARCH_SPIN_LOCK_UNLOCKED_VAL } }
> -#endif
> } arch_spinlock_t;
>
>
>
> [[End of PGP Signed Part]]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] parisc: Restore __ldcw_align for PA-RISC 2.0 processors
2023-09-19 18:31 ` Sam James
@ 2023-09-19 19:36 ` John David Anglin
0 siblings, 0 replies; 3+ messages in thread
From: John David Anglin @ 2023-09-19 19:36 UTC (permalink / raw)
To: Sam James, John David Anglin; +Cc: linux-parisc, Helge Deller, James Bottomley
On 2023-09-19 2:31 p.m., Sam James wrote:
> John David Anglin <dave@parisc-linux.org> writes:
>
>> [[PGP Signed Part:Undecided]]
>> Back in 2005, Kyle McMartin removed the 16-byte alignment for ldcw
>> semaphores on PA 2.0 machines (CONFIG_PA20). This broke spinlocks
>> on pre PA8800 processors. The main symptom was random faults in
>> mmap'd memory (e.g., gcc compilations, etc).
>>
>> Unfortunately, the errata for this ldcw change is lost.
>>
>> The issue is the 16-byte alignment required for ldcw semaphore
>> instructions can only reduced to natural alignment when the ldcw
>> operation can be handled coherently in cache. Only PA8800 and
>> PA8900 processors actually support doing the operation in cache.
>>
>> Aligning the spinlock dynamically adds two integer instructions
>> to each spinlock.
>>
>> Tested on rp3440, c8000 and a500.
> Could you include a 'Fixes: <whatever the ancient commit hash is>'?
I looked but Kyle's commit predates git. It was done to a cvs archive that I don't have.
It's available in the mail archive here:
https://lore.kernel.org/linux-parisc/20050609050702.GB4641@roadwarrior.mcmartin.ca/
>
> Also, what a find!
>
>> Signed-off-by: John David Anglin <dave.anglin@bell.net>
>> ---
>>
>> diff --git a/arch/parisc/include/asm/ldcw.h b/arch/parisc/include/asm/ldcw.h
>> index 6d28b5514699..ee9e071859b2 100644
>> --- a/arch/parisc/include/asm/ldcw.h
>> +++ b/arch/parisc/include/asm/ldcw.h
>> @@ -2,39 +2,42 @@
>> #ifndef __PARISC_LDCW_H
>> #define __PARISC_LDCW_H
>>
>> -#ifndef CONFIG_PA20
>> /* Because kmalloc only guarantees 8-byte alignment for kmalloc'd data,
>> and GCC only guarantees 8-byte alignment for stack locals, we can't
>> be assured of 16-byte alignment for atomic lock data even if we
>> specify "__attribute ((aligned(16)))" in the type declaration. So,
>> we use a struct containing an array of four ints for the atomic lock
>> type and dynamically select the 16-byte aligned int from the array
>> - for the semaphore. */
>> + for the semaphore. */
>> +
>> +/* From: "Jim Hull" <jim.hull of hp.com>
>> + I've attached a summary of the change, but basically, for PA 2.0, as
>> + long as the ",CO" (coherent operation) completer is implemented, then the
>> + 16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
>> + they only require "natural" alignment (4-byte for ldcw, 8-byte for
>> + ldcd).
>> +
>> + Although the cache control hint is accepted by all PA 2.0 processors,
>> + it is only implemented on PA8800/PA8900 CPUs. Prior PA8X00 CPUs still
>> + require 16-byte alignment. If the address is unaligned, the operation
>> + of the instruction is undefined. The ldcw instruction does not generate
>> + unaligned data reference traps so misaligned accesses are not detected.
>> + This hid the problem for years. So, restore the 16-byte alignment dropped
>> + by Kyle McMartin in "Remove __ldcw_align for PA-RISC 2.0 processors". */
>>
>> #define __PA_LDCW_ALIGNMENT 16
>> -#define __PA_LDCW_ALIGN_ORDER 4
>> #define __ldcw_align(a) ({ \
>> unsigned long __ret = (unsigned long) &(a)->lock[0]; \
>> __ret = (__ret + __PA_LDCW_ALIGNMENT - 1) \
>> & ~(__PA_LDCW_ALIGNMENT - 1); \
>> (volatile unsigned int *) __ret; \
>> })
>> -#define __LDCW "ldcw"
>>
>> -#else /*CONFIG_PA20*/
>> -/* From: "Jim Hull" <jim.hull of hp.com>
>> - I've attached a summary of the change, but basically, for PA 2.0, as
>> - long as the ",CO" (coherent operation) completer is specified, then the
>> - 16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
>> - they only require "natural" alignment (4-byte for ldcw, 8-byte for
>> - ldcd). */
>> -
>> -#define __PA_LDCW_ALIGNMENT 4
>> -#define __PA_LDCW_ALIGN_ORDER 2
>> -#define __ldcw_align(a) (&(a)->slock)
>> +#ifdef CONFIG_PA20
>> #define __LDCW "ldcw,co"
>> -
>> -#endif /*!CONFIG_PA20*/
>> +#else
>> +#define __LDCW "ldcw"
>> +#endif
>>
>> /* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.
>> We don't explicitly expose that "*a" may be written as reload
>> diff --git a/arch/parisc/include/asm/spinlock_types.h b/arch/parisc/include/asm/spinlock_types.h
>> index efd06a897c6a..7b986b09dba8 100644
>> --- a/arch/parisc/include/asm/spinlock_types.h
>> +++ b/arch/parisc/include/asm/spinlock_types.h
>> @@ -9,15 +9,10 @@
>> #ifndef __ASSEMBLY__
>>
>> typedef struct {
>> -#ifdef CONFIG_PA20
>> - volatile unsigned int slock;
>> -# define __ARCH_SPIN_LOCK_UNLOCKED { __ARCH_SPIN_LOCK_UNLOCKED_VAL }
>> -#else
>> volatile unsigned int lock[4];
>> # define __ARCH_SPIN_LOCK_UNLOCKED \
>> { { __ARCH_SPIN_LOCK_UNLOCKED_VAL, __ARCH_SPIN_LOCK_UNLOCKED_VAL, \
>> __ARCH_SPIN_LOCK_UNLOCKED_VAL, __ARCH_SPIN_LOCK_UNLOCKED_VAL } }
>> -#endif
>> } arch_spinlock_t;
>>
>>
>>
>> [[End of PGP Signed Part]]
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-09-19 19:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-19 17:51 [PATCH] parisc: Restore __ldcw_align for PA-RISC 2.0 processors John David Anglin
2023-09-19 18:31 ` Sam James
2023-09-19 19:36 ` John David Anglin
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).