Messages in this thread |  | | Subject | Re: [RFC PATCH v3 3/7] powerpc: Activate CONFIG_THREAD_INFO_IN_TASK | From | Christophe LEROY <> | Date | Wed, 3 Oct 2018 08:04:49 +0200 |
| |
Le 03/10/2018 à 07:52, Nicholas Piggin a écrit : > On Wed, 3 Oct 2018 07:47:05 +0200 > Christophe LEROY <christophe.leroy@c-s.fr> wrote: > >> Le 03/10/2018 à 07:30, Nicholas Piggin a écrit : >>> On Mon, 1 Oct 2018 12:30:23 +0000 (UTC) >>> Christophe Leroy <christophe.leroy@c-s.fr> wrote: >>> >>>> This patch activates CONFIG_THREAD_INFO_IN_TASK which >>>> moves the thread_info into task_struct. >>>> >>>> Moving thread_info into task_struct has the following advantages: >>>> - It protects thread_info from corruption in the case of stack >>>> overflows. >>>> - Its address is harder to determine if stack addresses are >>>> leaked, making a number of attacks more difficult. >>>> >>>> This has the following consequences: >>>> - thread_info is now located at the top of task_struct. >>> >>> "top"... I got confused for a minute thinking high address and >>> wondering how you can change CURRENT_THREAD_INFO just to point >>> to current :) >> >> Would 'beginning' be less confusing ? > > Yes, good idea. > >>>> @@ -83,7 +83,13 @@ int is_cpu_dead(unsigned int cpu); >>>> /* 32-bit */ >>>> extern int smp_hw_index[]; >>>> >>>> -#define raw_smp_processor_id() (current_thread_info()->cpu) >>>> +/* >>>> + * This is particularly ugly: it appears we can't actually get the definition >>>> + * of task_struct here, but we need access to the CPU this task is running on. >>>> + * Instead of using task_struct we're using _TASK_CPU which is extracted from >>>> + * asm-offsets.h by kbuild to get the current processor ID. >>>> + */ >>>> +#define raw_smp_processor_id() (*(unsigned int*)((void*)current + _TASK_CPU)) >>> >>> This is clever but yes ugly. Can't you include asm-offsets.h? riscv >>> seems to. >> >> riscv has a clean asm-offsets.h . Our's defines constant with the same >> name as those defined in other headers which are included in C files. So >> including asm-offsets in C files does create conflicts like: >> >> ./include/generated/asm-offsets.h:71:0: warning: "TASK_SIZE" redefined >> #define TASK_SIZE -2147483648 /* TASK_SIZE */ >> ./arch/powerpc/include/asm/processor.h:95:0: note: this is the location >> of the previous definition >> #define TASK_SIZE (CONFIG_TASK_SIZE) >> >> ./include/generated/asm-offsets.h:98:0: warning: "NSEC_PER_SEC" redefined >> #define NSEC_PER_SEC 1000000000 /* NSEC_PER_SEC */ >> ./include/linux/time64.h:36:0: note: this is the location of the >> previous definition >> #define NSEC_PER_SEC 1000000000L >> >> ./arch/powerpc/include/asm/nohash/32/pgtable.h:34:0: warning: >> "PGD_TABLE_SIZE" redefined >> #define PGD_TABLE_SIZE (sizeof(pgd_t) << PGD_INDEX_SIZE) >> ./include/generated/asm-offsets.h:101:0: note: this is the location of >> the previous definition >> #define PGD_TABLE_SIZE 256 /* PGD_TABLE_SIZE */ >> >> ... > > Okay. > >> >> In v2, I had a patch to fix those redundancies >> (https://patchwork.ozlabs.org/patch/974363/) but I found it unconvenient. > > Because of merge conflicts, or you did not like the new names?
Both, because of the amount of changes it implies, and also because of the new names. I find it quite convenient to be able to use same names both in C and ASM. And I didn't want my serie to imply big-bangs in unrelated or not directly related topics.
Christophe
> > Thanks, > Nick >
|  |