| Message ID | 20200827080438.315345-4-aneesh.kumar@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | mm/debug_vm_pgtable fixes | expand |
On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: > ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in > random value. > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > mm/debug_vm_pgtable.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > index 086309fb9b6f..bbf9df0e64c6 100644 > --- a/mm/debug_vm_pgtable.c > +++ b/mm/debug_vm_pgtable.c > @@ -44,10 +44,17 @@ > * entry type. But these bits might affect the ability to clear entries with > * pxx_clear() because of how dynamic page table folding works on s390. So > * while loading up the entries do not change the lower 4 bits. It does not > - * have affect any other platform. > + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is > + * used to mark a pte entry. > */ > -#define S390_MASK_BITS 4 > -#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS) > +#define S390_SKIP_MASK GENMASK(3, 0) > +#ifdef CONFIG_PPC_BOOK3S_64 > +#define PPC64_SKIP_MASK GENMASK(62, 62) > +#else > +#define PPC64_SKIP_MASK 0x0 > +#endif Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip bits for a s390 platform requirement and can also do so for ppc64 as well. As mentioned before, please avoid adding any platform specific constructs in the test. > +#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK) > +#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK) > #define RANDOM_NZVALUE GENMASK(7, 0) Please fix the alignments here. Feel free to consider following changes after this patch. diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c index 122416464e0f..f969031152bb 100644 --- a/mm/debug_vm_pgtable.c +++ b/mm/debug_vm_pgtable.c @@ -48,14 +48,11 @@ * have affect any other platform. Also avoid the 62nd bit on ppc64 that is * used to mark a pte entry. */ -#define S390_SKIP_MASK GENMASK(3, 0) -#ifdef CONFIG_PPC_BOOK3S_64 -#define PPC64_SKIP_MASK GENMASK(62, 62) -#else -#define PPC64_SKIP_MASK 0x0 -#endif -#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK) -#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK) +#define S390_SKIP_MASK GENMASK(3, 0) +#define PPC64_SKIP_MASK GENMASK(62, 62) +#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK) +#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK) + #define RANDOM_NZVALUE GENMASK(7, 0) > > static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot) > Besides, there is also one checkpatch.pl warning here. WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #7: ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in total: 0 errors, 1 warnings, 20 lines checked
On 9/1/20 8:45 AM, Anshuman Khandual wrote: > > > On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in >> random value. >> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> mm/debug_vm_pgtable.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >> index 086309fb9b6f..bbf9df0e64c6 100644 >> --- a/mm/debug_vm_pgtable.c >> +++ b/mm/debug_vm_pgtable.c >> @@ -44,10 +44,17 @@ >> * entry type. But these bits might affect the ability to clear entries with >> * pxx_clear() because of how dynamic page table folding works on s390. So >> * while loading up the entries do not change the lower 4 bits. It does not >> - * have affect any other platform. >> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is >> + * used to mark a pte entry. >> */ >> -#define S390_MASK_BITS 4 >> -#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS) >> +#define S390_SKIP_MASK GENMASK(3, 0) >> +#ifdef CONFIG_PPC_BOOK3S_64 >> +#define PPC64_SKIP_MASK GENMASK(62, 62) >> +#else >> +#define PPC64_SKIP_MASK 0x0 >> +#endif > > Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip > bits for a s390 platform requirement and can also do so for ppc64 as well. As > mentioned before, please avoid adding any platform specific constructs in the > test. > that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux >> +#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK) >> +#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK) >> #define RANDOM_NZVALUE GENMASK(7, 0) > > Please fix the alignments here. Feel free to consider following changes after > this patch. > > diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c > index 122416464e0f..f969031152bb 100644 > --- a/mm/debug_vm_pgtable.c > +++ b/mm/debug_vm_pgtable.c > @@ -48,14 +48,11 @@ > * have affect any other platform. Also avoid the 62nd bit on ppc64 that is > * used to mark a pte entry. > */ > -#define S390_SKIP_MASK GENMASK(3, 0) > -#ifdef CONFIG_PPC_BOOK3S_64 > -#define PPC64_SKIP_MASK GENMASK(62, 62) > -#else > -#define PPC64_SKIP_MASK 0x0 > -#endif > -#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK) > -#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK) > +#define S390_SKIP_MASK GENMASK(3, 0) > +#define PPC64_SKIP_MASK GENMASK(62, 62) > +#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK) > +#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK) > + > #define RANDOM_NZVALUE GENMASK(7, 0) > >> >> static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot) >> > > Besides, there is also one checkpatch.pl warning here. > > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) > #7: > ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in > > total: 0 errors, 1 warnings, 20 lines checked > These warnings are not valid. They are mostly long lines (upto 100) . or some details mentioned in the () as above. -aneesh
On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote: > On 9/1/20 8:45 AM, Anshuman Khandual wrote: >> >> >> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in >>> random value. >>> >>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>> --- >>> mm/debug_vm_pgtable.c | 13 ++++++++++--- >>> 1 file changed, 10 insertions(+), 3 deletions(-) >>> >>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>> index 086309fb9b6f..bbf9df0e64c6 100644 >>> --- a/mm/debug_vm_pgtable.c >>> +++ b/mm/debug_vm_pgtable.c >>> @@ -44,10 +44,17 @@ >>> * entry type. But these bits might affect the ability to clear entries with >>> * pxx_clear() because of how dynamic page table folding works on s390. So >>> * while loading up the entries do not change the lower 4 bits. It does not >>> - * have affect any other platform. >>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is >>> + * used to mark a pte entry. >>> */ >>> -#define S390_MASK_BITS 4 >>> -#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS) >>> +#define S390_SKIP_MASK GENMASK(3, 0) >>> +#ifdef CONFIG_PPC_BOOK3S_64 >>> +#define PPC64_SKIP_MASK GENMASK(62, 62) >>> +#else >>> +#define PPC64_SKIP_MASK 0x0 >>> +#endif >> >> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip >> bits for a s390 platform requirement and can also do so for ppc64 as well. As >> mentioned before, please avoid adding any platform specific constructs in the >> test. >> > > > that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux Could not (#if __BITS_PER_LONG == 32) be used instead or something like that. But should be a generic conditional check identifying 32 bit arch not anything platform specific.
On 9/1/20 1:02 PM, Anshuman Khandual wrote: > > > On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote: >> On 9/1/20 8:45 AM, Anshuman Khandual wrote: >>> >>> >>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in >>>> random value. >>>> >>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>>> --- >>>> mm/debug_vm_pgtable.c | 13 ++++++++++--- >>>> 1 file changed, 10 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>>> index 086309fb9b6f..bbf9df0e64c6 100644 >>>> --- a/mm/debug_vm_pgtable.c >>>> +++ b/mm/debug_vm_pgtable.c >>>> @@ -44,10 +44,17 @@ >>>> * entry type. But these bits might affect the ability to clear entries with >>>> * pxx_clear() because of how dynamic page table folding works on s390. So >>>> * while loading up the entries do not change the lower 4 bits. It does not >>>> - * have affect any other platform. >>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is >>>> + * used to mark a pte entry. >>>> */ >>>> -#define S390_MASK_BITS 4 >>>> -#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS) >>>> +#define S390_SKIP_MASK GENMASK(3, 0) >>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>> +#define PPC64_SKIP_MASK GENMASK(62, 62) >>>> +#else >>>> +#define PPC64_SKIP_MASK 0x0 >>>> +#endif >>> >>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip >>> bits for a s390 platform requirement and can also do so for ppc64 as well. As >>> mentioned before, please avoid adding any platform specific constructs in the >>> test. >>> >> >> >> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux > > Could not (#if __BITS_PER_LONG == 32) be used instead or something like > that. But should be a generic conditional check identifying 32 bit arch > not anything platform specific. > that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64. Not sure why other architectures need to bothered about ignoring bit 62. -aneesh
On 09/01/2020 01:06 PM, Aneesh Kumar K.V wrote: > On 9/1/20 1:02 PM, Anshuman Khandual wrote: >> >> >> On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote: >>> On 9/1/20 8:45 AM, Anshuman Khandual wrote: >>>> >>>> >>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in >>>>> random value. >>>>> >>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>>>> --- >>>>> mm/debug_vm_pgtable.c | 13 ++++++++++--- >>>>> 1 file changed, 10 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>>>> index 086309fb9b6f..bbf9df0e64c6 100644 >>>>> --- a/mm/debug_vm_pgtable.c >>>>> +++ b/mm/debug_vm_pgtable.c >>>>> @@ -44,10 +44,17 @@ >>>>> * entry type. But these bits might affect the ability to clear entries with >>>>> * pxx_clear() because of how dynamic page table folding works on s390. So >>>>> * while loading up the entries do not change the lower 4 bits. It does not >>>>> - * have affect any other platform. >>>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is >>>>> + * used to mark a pte entry. >>>>> */ >>>>> -#define S390_MASK_BITS 4 >>>>> -#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS) >>>>> +#define S390_SKIP_MASK GENMASK(3, 0) >>>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>>> +#define PPC64_SKIP_MASK GENMASK(62, 62) >>>>> +#else >>>>> +#define PPC64_SKIP_MASK 0x0 >>>>> +#endif >>>> >>>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip >>>> bits for a s390 platform requirement and can also do so for ppc64 as well. As >>>> mentioned before, please avoid adding any platform specific constructs in the >>>> test. >>>> >>> >>> >>> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux >> >> Could not (#if __BITS_PER_LONG == 32) be used instead or something like >> that. But should be a generic conditional check identifying 32 bit arch >> not anything platform specific. >> > > that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64. Not sure why other architectures need to bothered about ignoring bit 62. Thats okay as long it does not adversely affect other architectures, ignoring some more bits is acceptable. Like existing S390_MASK_BITS gets ignored on all other platforms even if it is a s390 specific constraint. Not having platform specific #ifdef here, is essential.
On 9/1/20 1:16 PM, Anshuman Khandual wrote: > > > On 09/01/2020 01:06 PM, Aneesh Kumar K.V wrote: >> On 9/1/20 1:02 PM, Anshuman Khandual wrote: >>> >>> >>> On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote: >>>> On 9/1/20 8:45 AM, Anshuman Khandual wrote: >>>>> >>>>> >>>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>>>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in >>>>>> random value. >>>>>> >>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>>>>> --- >>>>>> mm/debug_vm_pgtable.c | 13 ++++++++++--- >>>>>> 1 file changed, 10 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>>>>> index 086309fb9b6f..bbf9df0e64c6 100644 >>>>>> --- a/mm/debug_vm_pgtable.c >>>>>> +++ b/mm/debug_vm_pgtable.c >>>>>> @@ -44,10 +44,17 @@ >>>>>> * entry type. But these bits might affect the ability to clear entries with >>>>>> * pxx_clear() because of how dynamic page table folding works on s390. So >>>>>> * while loading up the entries do not change the lower 4 bits. It does not >>>>>> - * have affect any other platform. >>>>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is >>>>>> + * used to mark a pte entry. >>>>>> */ >>>>>> -#define S390_MASK_BITS 4 >>>>>> -#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS) >>>>>> +#define S390_SKIP_MASK GENMASK(3, 0) >>>>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>>>> +#define PPC64_SKIP_MASK GENMASK(62, 62) >>>>>> +#else >>>>>> +#define PPC64_SKIP_MASK 0x0 >>>>>> +#endif >>>>> >>>>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip >>>>> bits for a s390 platform requirement and can also do so for ppc64 as well. As >>>>> mentioned before, please avoid adding any platform specific constructs in the >>>>> test. >>>>> >>>> >>>> >>>> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux >>> >>> Could not (#if __BITS_PER_LONG == 32) be used instead or something like >>> that. But should be a generic conditional check identifying 32 bit arch >>> not anything platform specific. >>> >> >> that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64. Not sure why other architectures need to bothered about ignoring bit 62. > > Thats okay as long it does not adversely affect other architectures, ignoring > some more bits is acceptable. Like existing S390_MASK_BITS gets ignored on all > other platforms even if it is a s390 specific constraint. Not having platform > specific #ifdef here, is essential. > Why is it essential? -aneesh
On 09/01/2020 01:25 PM, Aneesh Kumar K.V wrote: > On 9/1/20 1:16 PM, Anshuman Khandual wrote: >> >> >> On 09/01/2020 01:06 PM, Aneesh Kumar K.V wrote: >>> On 9/1/20 1:02 PM, Anshuman Khandual wrote: >>>> >>>> >>>> On 09/01/2020 11:51 AM, Aneesh Kumar K.V wrote: >>>>> On 9/1/20 8:45 AM, Anshuman Khandual wrote: >>>>>> >>>>>> >>>>>> On 08/27/2020 01:34 PM, Aneesh Kumar K.V wrote: >>>>>>> ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in >>>>>>> random value. >>>>>>> >>>>>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >>>>>>> --- >>>>>>> mm/debug_vm_pgtable.c | 13 ++++++++++--- >>>>>>> 1 file changed, 10 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c >>>>>>> index 086309fb9b6f..bbf9df0e64c6 100644 >>>>>>> --- a/mm/debug_vm_pgtable.c >>>>>>> +++ b/mm/debug_vm_pgtable.c >>>>>>> @@ -44,10 +44,17 @@ >>>>>>> * entry type. But these bits might affect the ability to clear entries with >>>>>>> * pxx_clear() because of how dynamic page table folding works on s390. So >>>>>>> * while loading up the entries do not change the lower 4 bits. It does not >>>>>>> - * have affect any other platform. >>>>>>> + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is >>>>>>> + * used to mark a pte entry. >>>>>>> */ >>>>>>> -#define S390_MASK_BITS 4 >>>>>>> -#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS) >>>>>>> +#define S390_SKIP_MASK GENMASK(3, 0) >>>>>>> +#ifdef CONFIG_PPC_BOOK3S_64 >>>>>>> +#define PPC64_SKIP_MASK GENMASK(62, 62) >>>>>>> +#else >>>>>>> +#define PPC64_SKIP_MASK 0x0 >>>>>>> +#endif >>>>>> >>>>>> Please drop the #ifdef CONFIG_PPC_BOOK3S_64 here. We already accommodate skip >>>>>> bits for a s390 platform requirement and can also do so for ppc64 as well. As >>>>>> mentioned before, please avoid adding any platform specific constructs in the >>>>>> test. >>>>>> >>>>> >>>>> >>>>> that is needed so that it can be built on 32 bit architectures.I did face build errors with arch-linux >>>> >>>> Could not (#if __BITS_PER_LONG == 32) be used instead or something like >>>> that. But should be a generic conditional check identifying 32 bit arch >>>> not anything platform specific. >>>> >>> >>> that _PAGE_PTE bit is pretty much specific to PPC BOOK3S_64. Not sure why other architectures need to bothered about ignoring bit 62. >> >> Thats okay as long it does not adversely affect other architectures, ignoring >> some more bits is acceptable. Like existing S390_MASK_BITS gets ignored on all >> other platforms even if it is a s390 specific constraint. Not having platform >> specific #ifdef here, is essential. >> > > Why is it essential? IIRC, I might have already replied on this couple of times. But let me try once more. It is a generic test aimed at finding inconsistencies between different architectures in terms of the page table helper semantics. Any platform specific construct here, to 'make things work' has the potential to hide such inconsistencies and defeat the very purpose. The test/file here follows this rule consistently i.e there is not a single platform specific #ifdef right now and would really like to continue maintaining this property, unless until absolutely necessary. Current situation here wrt 32 bit archs can easily be accommodated with a generic check such as __BITS_PER_LONG.
diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c index 086309fb9b6f..bbf9df0e64c6 100644 --- a/mm/debug_vm_pgtable.c +++ b/mm/debug_vm_pgtable.c @@ -44,10 +44,17 @@ * entry type. But these bits might affect the ability to clear entries with * pxx_clear() because of how dynamic page table folding works on s390. So * while loading up the entries do not change the lower 4 bits. It does not - * have affect any other platform. + * have affect any other platform. Also avoid the 62nd bit on ppc64 that is + * used to mark a pte entry. */ -#define S390_MASK_BITS 4 -#define RANDOM_ORVALUE GENMASK(BITS_PER_LONG - 1, S390_MASK_BITS) +#define S390_SKIP_MASK GENMASK(3, 0) +#ifdef CONFIG_PPC_BOOK3S_64 +#define PPC64_SKIP_MASK GENMASK(62, 62) +#else +#define PPC64_SKIP_MASK 0x0 +#endif +#define ARCH_SKIP_MASK (S390_SKIP_MASK | PPC64_SKIP_MASK) +#define RANDOM_ORVALUE (GENMASK(BITS_PER_LONG - 1, 0) & ~ARCH_SKIP_MASK) #define RANDOM_NZVALUE GENMASK(7, 0) static void __init pte_basic_tests(unsigned long pfn, pgprot_t prot)
ppc64 use bit 62 to indicate a pte entry (_PAGE_PTE). Avoid setting that bit in random value. Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- mm/debug_vm_pgtable.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)