x86/mm/pat: let level meaningful even NULL return in, lookup_address_in_pgd
PATCH 1/1 x86/mm/pat: let level meaningful even NULL return in, lookup_address_in_pgd
If pmd or pud is not set, we may set a wrong page mapping level.
Base knowledge:
1) If we have passed pgd_none() check, then current level is at least PG_LEVEL_1G.
2) If we have passed pud_large() ||!pud_present() check, then current level is at least PG_LEVEL_2M.
This patch does not change the behavior when a non-NULL value is returned.
Let's take bask knowledge 2 as an example.
We know *address* belongs to *pud*, however for some reasons *pmd* is NULL. For example, this address has no physical pages mapped. What we could benefit from this patch are below:
1) We can walk memory range easier.
If addressA passed to lookup_address(), and NULL returned. We can pass addressA + level_to_size(level) to lookup_address() in next loop.
...
if (!pte) {
/* level_to_size has not been implemented in upstream*/
address += level_to_size(level);
continue;
}...
2) keep same behavior because level is set to PG_LEVEL_4K even when pte is NULL.
So let *level* meaningful even lookup_address_in_pgd return NULL by moving the assignment before pud/pmd_offset.
--- arch/x86/mm/pageattr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/mm/pageattr.c +2 -2
@@ -336,19 +336,19 @@ pte_t *lookup_address_in_pgd(pgd_t *pgd, unsigned long address,if (pgd_none (*pgd))return NULL ; + *level =PG_LEVEL_1G ; pud =pud_offset (pgd, address);if (pud_none (*pud))return NULL ; - *level =PG_LEVEL_1G ;if (pud_large (*pud) || !pud_present (*pud))return (pte_t *)pud; + *level =PG_LEVEL_2M ; pmd =pmd_offset (pud, address);if (pmd_none (*pmd))return NULL ; - *level =PG_LEVEL_2M ;if (pmd_large (*pmd) || !pmd_present (*pmd))return (pte_t *)pmd;
Signature · 1 line
1.9.1
Quoted 1 line · Pan Xinhui
If pmd or pud is not set, we may set a wrong page mapping level.
No. The behaviour is simply undefined, if the return value of the function is NULL.
So what you are trying to do is to make the level information accurate even for the failure case.
Quoted 7 lines
We know *address* belongs to *pud*, however for some reasons *pmd* is NULL. For example, this address has no physical pages mapped. What we could benefit from this patch are below:
1) We can walk memory range easier.
If addressA passed tolookup_address(), and NULL returned. We can pass addressA +level_to_size(level)tolookup_address()in next loop.
...
Quoted 5 lines
if (!pte) {
/* level_to_size has not been implemented in upstream*/
address += level_to_size(level);
continue;
}This example is completely useless because we do not see how the loop itself looks like and how that improves anything. The proper way to do this is to post:
- the patch which changes the function
- another patch which makes use of the change
But so far I cannot see any reason why we want to change it.
Quoted 3 lines
...
2) keep same behavior because level is set toPG_LEVEL_4Keven when pte is NULL.
And what's the actual benefit of #2? Keeping the same behaviour is a requirement if you don't want to break any users of that function.
Thanks,
tglx
hi, tglx
thanks for your reply.
Quoted 9 lines · Thomas Gleixner
Quoted 1 line · Pan Xinhui
If pmd or pud is not set, we may set a wrong page mapping level.
No. The behaviour is simply undefined, if the return value of the function is NULL.
So what you are trying to do is to make the level information accurate even for the failure case.
yes. it's good to report level information. then we can handle some errors.
Quoted 13 lines
Quoted 7 lines
We know *address* belongs to *pud*, however for some reasons *pmd* is NULL. For example, this address has no physical pages mapped. What we could benefit from this patch are below:
1) We can walk memory range easier.
If addressA passed tolookup_address(), and NULL returned. We can pass addressA +level_to_size(level)tolookup_address()in next loop.
...Quoted 5 lines
if (!pte) { /* level_to_size has not been implemented in upstream*/ address += level_to_size(level); continue; }
Quoted 9 lines
This example is completely useless because we do not see how the loop itself looks like and how that improves anything. The proper way to do this is to post:
- the patch which changes the function
- another patch which makes use of the changeBut so far I cannot see any reason why we want to change it.
sorry for that. There are some debug patches protected. I will try to make a simple example in other mails.
Quoted 7 lines
Quoted 3 lines
...
2) keep same behavior because level is set toPG_LEVEL_4Keven when pte is NULL.And what's the actual benefit of #2? Keeping the same behaviour is a requirement if you don't want to break any users of that function.
agree with you. :)
I did not explain it in correct ways.
When pte is NULL, lookup_address will return NULL on failure. however the level is correct and set to PG_LEVEL_4K. So what I am trying to do is that if lookup_address return NULL on *pud* or *pmd* NULL failure, level is still correct or more correct. A correct level information is very useful when we walk a large range of memory.
thanks
xinhui
Quoted 4 lines
Thanks,
tglx