Re: [PATCH v4 1/2] Revert "powerpc: Set max_mapnr correctly"

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 01/04/2022 à 13:23, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> Le 28/03/2022 à 12:37, Michael Ellerman a écrit :
>>> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
>>>> Hi maintainers，
>>>>
>>>> I saw the patches has been reviewed[1], could they be merged?
>>>
>>> Maybe I'm just misreading the change log, but it seems wrong that we
>>> need to add extra checks. pfn_valid() shouldn't return true for vmalloc
>>> addresses in the first place, shouldn't we fix that instead? Who knows
>>> what else that might be broken because of that.
>>
>> pfn_valid() doesn't take an address but a PFN
> 
> Yeah sorry that was unclear wording on my part.
> 
> What I mean is that pfn_valid(virt_to_pfn(some_vmalloc_addr)) should be
> false, because virt_to_pfn(vmalloc_addr) should fail.

Yes that's probably how it should be but none of the main architectures 
do that.

The best we find is some architecture that WARN_ON(some valloc addr) in 
virt_to_pfn(). That's wouldn't help in our case, as it would then WARN_ON()


> 
> The right way to convert a vmalloc address to a pfn is with
> vmalloc_to_pfn(), which walks the page tables to find the actual pfn
> backing that vmalloc addr.
> 
>> If you have 1Gbyte of memory you have 256k PFNs.
>>
>> In a generic config the kernel will map 768 Mbytes of mémory (From
>> 0xc0000000 to 0xe0000000) and will use 0xf0000000-0xffffffff for
>> everything else including vmalloc.
>>
>> If you take a page above that 768 Mbytes limit, and tries to linarly
>> convert it's PFN to a va, you'll hip vmalloc space. Anyway that PFN is
>> valid.
> 
> That's true, but it's just some random page in vmalloc space, there's no
> guarantee that it's the same page as the PFN you started with.

Yes sure, what I meant however is that pfn_valid(some_valid_pfn) should 
return true, even if virt_to_pfn(some_vmalloc_address) profides a valid PFN.

> 
> Note it's not true on 64-bit Book3S. There if you take a valid PFN (ie.
> backed by RAM) and convert it to a virtual address (with __va()), you
> will never get a vmalloc address.
> 
>> So the check really needs to be done in virt_addr_valid().
> 
> I don't think it has to, but with the way our virt_to_pfn()/__pa() works
> I guess for now it's the easiest solution.
> 

At least other architectures do it that way. See for instance how ARM 
does it:

#define virt_addr_valid(kaddr)	(((unsigned long)(kaddr) >= PAGE_OFFSET 
&& (unsigned long)(kaddr) < (unsigned long)high_memory) \
					&& pfn_valid(virt_to_pfn(kaddr)))



high_memory being the top of linear RAM mapping

Christophe