| Message ID | 20220623122922.640980-3-aneesh.kumar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] powerpc/memhotplug: Add add_pages override for PPC | expand |
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: > Instead of high_memory use VMALLOC_START to validate that the address is > not in the vmalloc range. > > Cc: Kefeng Wang <wangkefeng.wang@huawei.com> > Cc: Christophe Leroy <christophe.leroy@csgroup.eu> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> Isn't this really the fix for ffa0b64e3be5 ("powerpc: Fix virt_addr_valid() for 64-bit Book3E & 32-bit") ? cheers > diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h > index e5f75c70eda8..256cad69e42e 100644 > --- a/arch/powerpc/include/asm/page.h > +++ b/arch/powerpc/include/asm/page.h > @@ -134,7 +134,7 @@ static inline bool pfn_valid(unsigned long pfn) > > #define virt_addr_valid(vaddr) ({ \ > unsigned long _addr = (unsigned long)vaddr; \ > - _addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory && \ > + _addr >= PAGE_OFFSET && _addr < (unsigned long)VMALLOC_START && \ > pfn_valid(virt_to_pfn(_addr)); \ > }) > > -- > 2.36.1
Le 23/06/2022 à 14:29, Aneesh Kumar K.V a écrit : > Instead of high_memory use VMALLOC_START to validate that the address is > not in the vmalloc range. What's the reason for using VMALLOC_START instead ? The gap between high_memory and VMALLOC_START should not be seen as valid memory either, should it ? If the problem is book3s/64, commit ffa0b64e3be5 ("powerpc: Fix virt_addr_valid() for 64-bit Book3E & 32-bit") says that those additional tests are superfluous for boo3s/64. Maybe it's time to drop unnecessary tests for book3s/64 ? > > Cc: Kefeng Wang <wangkefeng.wang@huawei.com> > Cc: Christophe Leroy <christophe.leroy@csgroup.eu> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > arch/powerpc/include/asm/page.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h > index e5f75c70eda8..256cad69e42e 100644 > --- a/arch/powerpc/include/asm/page.h > +++ b/arch/powerpc/include/asm/page.h > @@ -134,7 +134,7 @@ static inline bool pfn_valid(unsigned long pfn) > > #define virt_addr_valid(vaddr) ({ \ > unsigned long _addr = (unsigned long)vaddr; \ > - _addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory && \ > + _addr >= PAGE_OFFSET && _addr < (unsigned long)VMALLOC_START && \ > pfn_valid(virt_to_pfn(_addr)); \ > }) >
Michael Ellerman <mpe@ellerman.id.au> writes: > "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes: >> Instead of high_memory use VMALLOC_START to validate that the address is >> not in the vmalloc range. >> >> Cc: Kefeng Wang <wangkefeng.wang@huawei.com> >> Cc: Christophe Leroy <christophe.leroy@csgroup.eu> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > > Isn't this really the fix for ffa0b64e3be5 ("powerpc: Fix > virt_addr_valid() for 64-bit Book3E & 32-bit") ? If we are looking for a simpler backport yet. But what commit ffa0b64e3be5 does is correct. high_memory is suppose to be the top of direct mapped address range. Hence checking for high_meory will also avoid vmalloc address. If we take patch 1 then patch 3 is not really a fix. I would consider it a cleanup to switch to a more familiar VMALLOC_START variable. > > cheers > >> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h >> index e5f75c70eda8..256cad69e42e 100644 >> --- a/arch/powerpc/include/asm/page.h >> +++ b/arch/powerpc/include/asm/page.h >> @@ -134,7 +134,7 @@ static inline bool pfn_valid(unsigned long pfn) >> >> #define virt_addr_valid(vaddr) ({ \ >> unsigned long _addr = (unsigned long)vaddr; \ >> - _addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory && \ >> + _addr >= PAGE_OFFSET && _addr < (unsigned long)VMALLOC_START && \ >> pfn_valid(virt_to_pfn(_addr)); \ >> }) >> >> -- >> 2.36.1
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 23/06/2022 à 14:29, Aneesh Kumar K.V a écrit : >> Instead of high_memory use VMALLOC_START to validate that the address is >> not in the vmalloc range. > > What's the reason for using VMALLOC_START instead ? > The gap between high_memory and VMALLOC_START should not be seen as > valid memory either, should it ? Yes and that invalid range should be captured by the pfn_valid check. Commit ffa0b64e3be5 intended to skip the vmalloc range. Unfortunately, that resulted in kernel crash due to architecture not updating high_memory after a memory hotplug. That should be fixed by patch 1 in this series. patch 3 was added merely as a cleanup to switch from high_memory to a more familiar VMALLOC_START variable. > > If the problem is book3s/64, commit ffa0b64e3be5 ("powerpc: Fix > virt_addr_valid() for 64-bit Book3E & 32-bit") says that those > additional tests are superfluous for boo3s/64. Maybe it's time to drop > unnecessary tests for book3s/64 ? They are not specific book3s/64. IIUC virt_addr_valid will return false for an addr after memory hotplug on other platforms too. Patch 1 describe those details. > >> >> Cc: Kefeng Wang <wangkefeng.wang@huawei.com> >> Cc: Christophe Leroy <christophe.leroy@csgroup.eu> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> arch/powerpc/include/asm/page.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h >> index e5f75c70eda8..256cad69e42e 100644 >> --- a/arch/powerpc/include/asm/page.h >> +++ b/arch/powerpc/include/asm/page.h >> @@ -134,7 +134,7 @@ static inline bool pfn_valid(unsigned long pfn) >> >> #define virt_addr_valid(vaddr) ({ \ >> unsigned long _addr = (unsigned long)vaddr; \ >> - _addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory && \ >> + _addr >= PAGE_OFFSET && _addr < (unsigned long)VMALLOC_START && \ >> pfn_valid(virt_to_pfn(_addr)); \ >> }) >>
Le 23/06/2022 à 14:29, Aneesh Kumar K.V a écrit : > Instead of high_memory use VMALLOC_START to validate that the address is > not in the vmalloc range. > > Cc: Kefeng Wang <wangkefeng.wang@huawei.com> > Cc: Christophe Leroy <christophe.leroy@csgroup.eu> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > --- > arch/powerpc/include/asm/page.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h > index e5f75c70eda8..256cad69e42e 100644 > --- a/arch/powerpc/include/asm/page.h > +++ b/arch/powerpc/include/asm/page.h > @@ -134,7 +134,7 @@ static inline bool pfn_valid(unsigned long pfn) > > #define virt_addr_valid(vaddr) ({ \ > unsigned long _addr = (unsigned long)vaddr; \ > - _addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory && \ > + _addr >= PAGE_OFFSET && _addr < (unsigned long)VMALLOC_START && \ > pfn_valid(virt_to_pfn(_addr)); \ > }) > What about booke/64 ? The test will be _addr >= 0xc000000000000000 && _addr < 0x8000000000000000 so the test will be always false. Christophe
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 23/06/2022 à 14:29, Aneesh Kumar K.V a écrit : >> Instead of high_memory use VMALLOC_START to validate that the address is >> not in the vmalloc range. >> >> Cc: Kefeng Wang <wangkefeng.wang@huawei.com> >> Cc: Christophe Leroy <christophe.leroy@csgroup.eu> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> >> --- >> arch/powerpc/include/asm/page.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h >> index e5f75c70eda8..256cad69e42e 100644 >> --- a/arch/powerpc/include/asm/page.h >> +++ b/arch/powerpc/include/asm/page.h >> @@ -134,7 +134,7 @@ static inline bool pfn_valid(unsigned long pfn) >> >> #define virt_addr_valid(vaddr) ({ \ >> unsigned long _addr = (unsigned long)vaddr; \ >> - _addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory && \ >> + _addr >= PAGE_OFFSET && _addr < (unsigned long)VMALLOC_START && \ >> pfn_valid(virt_to_pfn(_addr)); \ >> }) >> > > What about booke/64 ? > > The test will be _addr >= 0xc000000000000000 && _addr < > 0x8000000000000000 so the test will be always false. > Ok, I didn't realize that booke/64 have vmalloc range below direct map. I guess we should drop patch 3. -aneesh
diff --git a/arch/powerpc/include/asm/page.h b/arch/powerpc/include/asm/page.h index e5f75c70eda8..256cad69e42e 100644 --- a/arch/powerpc/include/asm/page.h +++ b/arch/powerpc/include/asm/page.h @@ -134,7 +134,7 @@ static inline bool pfn_valid(unsigned long pfn) #define virt_addr_valid(vaddr) ({ \ unsigned long _addr = (unsigned long)vaddr; \ - _addr >= PAGE_OFFSET && _addr < (unsigned long)high_memory && \ + _addr >= PAGE_OFFSET && _addr < (unsigned long)VMALLOC_START && \ pfn_valid(virt_to_pfn(_addr)); \ })
Instead of high_memory use VMALLOC_START to validate that the address is not in the vmalloc range. Cc: Kefeng Wang <wangkefeng.wang@huawei.com> Cc: Christophe Leroy <christophe.leroy@csgroup.eu> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> --- arch/powerpc/include/asm/page.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)