Le 21/02/2024 à 13:09, Michael Ellerman a écrit :
> Christophe Leroy <[email protected]> writes:
>> set_memory_p() and set_memory_np() can fail.
>>
>> As mentioned in linux/mm.h:
>>
>> /*
>> * To support DEBUG_PAGEALLOC architecture must ensure that
>> * __kernel_map_pages() never fails
>> */
>>
>> So panic in case set_memory_p() or set_memory_np() fail
>> in __kernel_map_pages().
>>
>> Link: https://github.com/KSPP/linux/issues/7
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>> arch/powerpc/include/asm/book3s/64/hash.h | 2 +-
>> arch/powerpc/mm/book3s64/hash_utils.c | 3 ++-
>> arch/powerpc/mm/pageattr.c | 10 +++++++---
>> 3 files changed, 10 insertions(+), 5 deletions(-)
>>
> ...
>> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
>> index 16b8d20d6ca8..62b678585878 100644
>> --- a/arch/powerpc/mm/pageattr.c
>> +++ b/arch/powerpc/mm/pageattr.c
>> @@ -106,17 +106,21 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>> #ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>> void __kernel_map_pages(struct page *page, int numpages, int enable)
>> {
>> + int err;
>> unsigned long addr = (unsigned long)page_address(page);
>>
>> if (PageHighMem(page))
>> return;
>>
>> if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
>> - hash__kernel_map_pages(page, numpages, enable);
>> + err = hash__kernel_map_pages(page, numpages, enable);
>> else if (enable)
>> - set_memory_p(addr, numpages);
>> + err = set_memory_p(addr, numpages);
>> else
>> - set_memory_np(addr, numpages);
>> + err = set_memory_np(addr, numpages);
>> +
>> + if (err)
>> + panic("%s: set_memory_%sp() failed\n", enable ? "" : "n");
>
> This doesn't compile, it's missing __func__ I guess.
Damn, I was too quick when I took into account checkpatch's feedback,
sorry for that.
>
> Seems like we could keep it simpler though, it should hopefully never
> happen anyway, eg:
>
> panic("%s: changing memory protections failed\n", __func__);
Sure, let's do that. Do you want a v2 or you do the change directly ?
Thanks
Christophe
Christophe Leroy <[email protected]> writes:
> Le 21/02/2024 à 13:09, Michael Ellerman a écrit :
>> Christophe Leroy <[email protected]> writes:
>>> set_memory_p() and set_memory_np() can fail.
>>>
>>> As mentioned in linux/mm.h:
>>>
>>> /*
>>> * To support DEBUG_PAGEALLOC architecture must ensure that
>>> * __kernel_map_pages() never fails
>>> */
>>>
>>> So panic in case set_memory_p() or set_memory_np() fail
>>> in __kernel_map_pages().
>>>
>>> Link: https://github.com/KSPP/linux/issues/7
>>> Signed-off-by: Christophe Leroy <[email protected]>
>>> ---
>>> arch/powerpc/include/asm/book3s/64/hash.h | 2 +-
>>> arch/powerpc/mm/book3s64/hash_utils.c | 3 ++-
>>> arch/powerpc/mm/pageattr.c | 10 +++++++---
>>> 3 files changed, 10 insertions(+), 5 deletions(-)
>>>
>> ...
>>> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
>>> index 16b8d20d6ca8..62b678585878 100644
>>> --- a/arch/powerpc/mm/pageattr.c
>>> +++ b/arch/powerpc/mm/pageattr.c
>>> @@ -106,17 +106,21 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
>>> #ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>>> void __kernel_map_pages(struct page *page, int numpages, int enable)
>>> {
>>> + int err;
>>> unsigned long addr = (unsigned long)page_address(page);
>>>
>>> if (PageHighMem(page))
>>> return;
>>>
>>> if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
>>> - hash__kernel_map_pages(page, numpages, enable);
>>> + err = hash__kernel_map_pages(page, numpages, enable);
>>> else if (enable)
>>> - set_memory_p(addr, numpages);
>>> + err = set_memory_p(addr, numpages);
>>> else
>>> - set_memory_np(addr, numpages);
>>> + err = set_memory_np(addr, numpages);
>>> +
>>> + if (err)
>>> + panic("%s: set_memory_%sp() failed\n", enable ? "" : "n");
>>
>> This doesn't compile, it's missing __func__ I guess.
>
> Damn, I was too quick when I took into account checkpatch's feedback,
> sorry for that.
>
>>
>> Seems like we could keep it simpler though, it should hopefully never
>> happen anyway, eg:
>>
>> panic("%s: changing memory protections failed\n", __func__);
>
> Sure, let's do that. Do you want a v2 or you do the change directly ?
No need for a v2, I'll just fix it up when applying.
cheers