Instead of open coding a page table walk, use walk_pt() in need_pgt().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/mm.c | 66 +++++++++++++++++++--------------------------------
 1 file changed, 24 insertions(+), 42 deletions(-)

diff --git a/arch/x86/mm.c b/arch/x86/mm.c
index cc4d41e9..accde291 100644
--- a/arch/x86/mm.c
+++ b/arch/x86/mm.c
@@ -518,57 +518,39 @@ static pgentry_t *get_pgt(unsigned long va)
  * return a valid PTE for a given virtual address. If PTE does not exist,
  * allocate page-table pages.
  */
-pgentry_t *need_pgt(unsigned long va)
+static int need_pgt_func(unsigned long va, unsigned int lvl, bool is_leaf,
+                         pgentry_t *pte, void *par)
 {
+    pgentry_t **result = par;
     unsigned long pt_mfn;
-    pgentry_t *tab;
     unsigned long pt_pfn;
-    unsigned offset;
+    unsigned int idx;
 
-    tab = pt_base;
-    pt_mfn = virt_to_mfn(pt_base);
+    if ( !is_leaf )
+        return 0;
 
-#if defined(__x86_64__)
-    offset = l4_table_offset(va);
-    if ( !(tab[offset] & _PAGE_PRESENT) )
-    {
-        pt_pfn = virt_to_pfn(alloc_page());
-        if ( !pt_pfn )
-            return NULL;
-        new_pt_frame(&pt_pfn, pt_mfn, offset, L3_FRAME);
-    }
-    ASSERT(tab[offset] & _PAGE_PRESENT);
-    pt_mfn = pte_to_mfn(tab[offset]);
-    tab = mfn_to_virt(pt_mfn);
-#endif
-    offset = l3_table_offset(va);
-    if ( !(tab[offset] & _PAGE_PRESENT) ) 
-    {
-        pt_pfn = virt_to_pfn(alloc_page());
-        if ( !pt_pfn )
-            return NULL;
-        new_pt_frame(&pt_pfn, pt_mfn, offset, L2_FRAME);
-    }
-    ASSERT(tab[offset] & _PAGE_PRESENT);
-    pt_mfn = pte_to_mfn(tab[offset]);
-    tab = mfn_to_virt(pt_mfn);
-    offset = l2_table_offset(va);
-    if ( !(tab[offset] & _PAGE_PRESENT) )
+    if ( lvl == L1_FRAME || (*pte & _PAGE_PRESENT) )
     {
-        pt_pfn = virt_to_pfn(alloc_page());
-        if ( !pt_pfn )
-            return NULL;
-        new_pt_frame(&pt_pfn, pt_mfn, offset, L1_FRAME);
+        *result = pte;
+        return 1;
     }
-    ASSERT(tab[offset] & _PAGE_PRESENT);
-    if ( tab[offset] & _PAGE_PSE )
-        return &tab[offset];
 
-    pt_mfn = pte_to_mfn(tab[offset]);
-    tab = mfn_to_virt(pt_mfn);
+    pt_mfn = virt_to_mfn(pte);
+    pt_pfn = virt_to_pfn(alloc_page());
+    if ( !pt_pfn )
+        return -1;
+    idx = (va >> ptdata[lvl].shift) & (ptdata[lvl].entries - 1);
+    new_pt_frame(&pt_pfn, pt_mfn, idx, lvl - 1);
 
-    offset = l1_table_offset(va);
-    return &tab[offset];
+    return 0;
+}
+
+pgentry_t *need_pgt(unsigned long va)
+{
+    pgentry_t *tab = NULL;
+
+    walk_pt(va, va, need_pgt_func, &tab);
+    return tab;
 }
 EXPORT_SYMBOL(need_pgt);
 
-- 
2.43.0

Hello,

Juergen Gross, le mer. 31 juil. 2024 15:00:25 +0200, a ecrit:
> -pgentry_t *need_pgt(unsigned long va)
> +static int need_pgt_func(unsigned long va, unsigned int lvl, bool is_leaf,
> +                         pgentry_t *pte, void *par)
>  {
[...]
> +    if ( lvl == L1_FRAME || (*pte & _PAGE_PRESENT) )

Did you mean (*pte & _PAGE_PSE)?

>      {
> +        *result = pte;
> +        return 1;
>      }

Samuel

On 31.07.24 23:27, Samuel Thibault wrote:
> Hello,
> 
> Juergen Gross, le mer. 31 juil. 2024 15:00:25 +0200, a ecrit:
>> -pgentry_t *need_pgt(unsigned long va)
>> +static int need_pgt_func(unsigned long va, unsigned int lvl, bool is_leaf,
>> +                         pgentry_t *pte, void *par)
>>   {
> [...]
>> +    if ( lvl == L1_FRAME || (*pte & _PAGE_PRESENT) )
> 
> Did you mean (*pte & _PAGE_PSE)?

No. I want to bail out if the PTE does not require a page table to be
added to it. This is the case if the PTE is valid or if it is at the
lowest page table level already.

An invalid PTE with PSE set is still invalid, so the PSE bit has no
real meaning.


Juergen

Jürgen Groß, le jeu. 01 août 2024 07:56:36 +0200, a ecrit:
> On 31.07.24 23:27, Samuel Thibault wrote:
> > Hello,
> > 
> > Juergen Gross, le mer. 31 juil. 2024 15:00:25 +0200, a ecrit:
> > > -pgentry_t *need_pgt(unsigned long va)
> > > +static int need_pgt_func(unsigned long va, unsigned int lvl, bool is_leaf,
> > > +                         pgentry_t *pte, void *par)
> > >   {
> > [...]
> > > +    if ( lvl == L1_FRAME || (*pte & _PAGE_PRESENT) )
> > 
> > Did you mean (*pte & _PAGE_PSE)?
> 
> No.

I don't understand: it doesn't map what I know of need_pgt and what the
existing code is doing.

AIKI, the point of need_pgt is to make sure there is a L1 page table
entry for a VA, and return it, so the caller can put in it at pte for a
mfn or such. In the case a PSE is met, we don't go further, and it's up
to the caller to decide what it wants to do (most often it's actually
unexpected and asserted out). In both cases, the PRESENT bit of the
pte whose address is returned does not matter, most often it's the
caller which will set it.

The existing code for need_pgt thus always adds page table entries down
to level1 (except if _PAGE_PSE is met, i.e. a large page was already set
up): the termination was:

[... walk down to level 2]
-    if ( tab[offset] & _PAGE_PSE )
-        return &tab[offset];
[... walk down to level 1]
-    return &tab[offset];

i.e. we always return either a PSE L2 entry, or an L1 entry, and never
above (while keeping sure that the entries above are present).

> I want to bail out if the PTE does not require a page table to be
> added to it. This is the case if the PTE is valid

There being an entry at e.g. level 3 (without PSE) does not mean that
we have an entry at level 1 for the VA, so we have to continue adding
levels for the VA in the sparse page table tree.

> An invalid PTE with PSE set is still invalid, so the PSE bit has no
> real meaning.

Mmm, I don't think we ever create entries with PSE but not PRESENT (i.e.
we don't "plan" to have PSE entries), but even if we did, if we "plan"
to have PSE entries, we should return that entry, and it's up to the
caller to fill it (and make it PRESENT)

Samuel

On 01.08.24 09:39, Samuel Thibault wrote:
> Jürgen Groß, le jeu. 01 août 2024 07:56:36 +0200, a ecrit:
>> On 31.07.24 23:27, Samuel Thibault wrote:
>>> Hello,
>>>
>>> Juergen Gross, le mer. 31 juil. 2024 15:00:25 +0200, a ecrit:
>>>> -pgentry_t *need_pgt(unsigned long va)
>>>> +static int need_pgt_func(unsigned long va, unsigned int lvl, bool is_leaf,
>>>> +                         pgentry_t *pte, void *par)
>>>>    {
>>> [...]
>>>> +    if ( lvl == L1_FRAME || (*pte & _PAGE_PRESENT) )
>>>
>>> Did you mean (*pte & _PAGE_PSE)?
>>
>> No.
> 
> I don't understand: it doesn't map what I know of need_pgt and what the
> existing code is doing.
> 
> AIKI, the point of need_pgt is to make sure there is a L1 page table
> entry for a VA, and return it, so the caller can put in it at pte for a
> mfn or such. In the case a PSE is met, we don't go further, and it's up
> to the caller to decide what it wants to do (most often it's actually
> unexpected and asserted out). In both cases, the PRESENT bit of the
> pte whose address is returned does not matter, most often it's the
> caller which will set it.
> 
> The existing code for need_pgt thus always adds page table entries down
> to level1 (except if _PAGE_PSE is met, i.e. a large page was already set
> up): the termination was:
> 
> [... walk down to level 2]

You have omitted:

        ASSERT(tab[offset] & _PAGE_PRESENT);

> -    if ( tab[offset] & _PAGE_PSE )
> -        return &tab[offset];
> [... walk down to level 1]
> -    return &tab[offset];


Juergen

Jürgen Groß, le jeu. 01 août 2024 10:13:07 +0200, a ecrit:
> On 01.08.24 09:39, Samuel Thibault wrote:
> > Jürgen Groß, le jeu. 01 août 2024 07:56:36 +0200, a ecrit:
> > > On 31.07.24 23:27, Samuel Thibault wrote:
> > > > Hello,
> > > > 
> > > > Juergen Gross, le mer. 31 juil. 2024 15:00:25 +0200, a ecrit:
> > > > > -pgentry_t *need_pgt(unsigned long va)
> > > > > +static int need_pgt_func(unsigned long va, unsigned int lvl, bool is_leaf,
> > > > > +                         pgentry_t *pte, void *par)
> > > > >    {
> > > > [...]
> > > > > +    if ( lvl == L1_FRAME || (*pte & _PAGE_PRESENT) )
> > > > 
> > > > Did you mean (*pte & _PAGE_PSE)?
> > > 
> > > No.
> > 
> > I don't understand: it doesn't map what I know of need_pgt and what the
> > existing code is doing.
> > 
> > AIKI, the point of need_pgt is to make sure there is a L1 page table
> > entry for a VA, and return it, so the caller can put in it at pte for a
> > mfn or such. In the case a PSE is met, we don't go further, and it's up
> > to the caller to decide what it wants to do (most often it's actually
> > unexpected and asserted out). In both cases, the PRESENT bit of the
> > pte whose address is returned does not matter, most often it's the
> > caller which will set it.
> > 
> > The existing code for need_pgt thus always adds page table entries down
> > to level1 (except if _PAGE_PSE is met, i.e. a large page was already set
> > up): the termination was:
> > 
> > [... walk down to level 2]
> 
> You have omitted:
> 
>        ASSERT(tab[offset] & _PAGE_PRESENT);

Right, that confirms that we never set PSE without PRESENT. We probably
want to keep that assertion in the PSE case.

But we still want to continue walking down to level1 (or pse) even when
L4/L3/L2 have PRESENT.

> > -    if ( tab[offset] & _PAGE_PSE )
> > -        return &tab[offset];
> > [... walk down to level 1]
> > -    return &tab[offset];

Samuel

On 01.08.24 11:15, Samuel Thibault wrote:
> Jürgen Groß, le jeu. 01 août 2024 10:13:07 +0200, a ecrit:
>> On 01.08.24 09:39, Samuel Thibault wrote:
>>> Jürgen Groß, le jeu. 01 août 2024 07:56:36 +0200, a ecrit:
>>>> On 31.07.24 23:27, Samuel Thibault wrote:
>>>>> Hello,
>>>>>
>>>>> Juergen Gross, le mer. 31 juil. 2024 15:00:25 +0200, a ecrit:
>>>>>> -pgentry_t *need_pgt(unsigned long va)
>>>>>> +static int need_pgt_func(unsigned long va, unsigned int lvl, bool is_leaf,
>>>>>> +                         pgentry_t *pte, void *par)
>>>>>>     {
>>>>> [...]
>>>>>> +    if ( lvl == L1_FRAME || (*pte & _PAGE_PRESENT) )
>>>>>
>>>>> Did you mean (*pte & _PAGE_PSE)?
>>>>
>>>> No.
>>>
>>> I don't understand: it doesn't map what I know of need_pgt and what the
>>> existing code is doing.
>>>
>>> AIKI, the point of need_pgt is to make sure there is a L1 page table
>>> entry for a VA, and return it, so the caller can put in it at pte for a
>>> mfn or such. In the case a PSE is met, we don't go further, and it's up
>>> to the caller to decide what it wants to do (most often it's actually
>>> unexpected and asserted out). In both cases, the PRESENT bit of the
>>> pte whose address is returned does not matter, most often it's the
>>> caller which will set it.
>>>
>>> The existing code for need_pgt thus always adds page table entries down
>>> to level1 (except if _PAGE_PSE is met, i.e. a large page was already set
>>> up): the termination was:
>>>
>>> [... walk down to level 2]
>>
>> You have omitted:
>>
>>         ASSERT(tab[offset] & _PAGE_PRESENT);
> 
> Right, that confirms that we never set PSE without PRESENT. We probably
> want to keep that assertion in the PSE case.

Hmm, I'm not sure.

The hardware allows to have any bit set in the PTE when PRESENT isn't set.
It will never look at any other bit in this case. Why should the software
require something different here?

> But we still want to continue walking down to level1 (or pse) even when
> L4/L3/L2 have PRESENT.

We do that. In this case the is_leaf flag won't be set and need_pgt_func()
will return early with the return value 0, meaning that the walk will be
continued.


Juergen

Jürgen Groß, le jeu. 01 août 2024 11:25:31 +0200, a ecrit:
> On 01.08.24 11:15, Samuel Thibault wrote:
> > Jürgen Groß, le jeu. 01 août 2024 10:13:07 +0200, a ecrit:
> > > On 01.08.24 09:39, Samuel Thibault wrote:
> > > > Jürgen Groß, le jeu. 01 août 2024 07:56:36 +0200, a ecrit:
> > > > > On 31.07.24 23:27, Samuel Thibault wrote:
> > > > > > Hello,
> > > > > > 
> > > > > > Juergen Gross, le mer. 31 juil. 2024 15:00:25 +0200, a ecrit:
> > > > > > > -pgentry_t *need_pgt(unsigned long va)
> > > > > > > +static int need_pgt_func(unsigned long va, unsigned int lvl, bool is_leaf,
> > > > > > > +                         pgentry_t *pte, void *par)
> > > > > > >     {
> > > > > > [...]
> > > > > > > +    if ( lvl == L1_FRAME || (*pte & _PAGE_PRESENT) )
> > > > > > 
> > > > > > Did you mean (*pte & _PAGE_PSE)?
> > > > > 
> > > > > No.
> > > > 
> > > > I don't understand: it doesn't map what I know of need_pgt and what the
> > > > existing code is doing.
> > > > 
> > > > AIKI, the point of need_pgt is to make sure there is a L1 page table
> > > > entry for a VA, and return it, so the caller can put in it at pte for a
> > > > mfn or such. In the case a PSE is met, we don't go further, and it's up
> > > > to the caller to decide what it wants to do (most often it's actually
> > > > unexpected and asserted out). In both cases, the PRESENT bit of the
> > > > pte whose address is returned does not matter, most often it's the
> > > > caller which will set it.
> > > > 
> > > > The existing code for need_pgt thus always adds page table entries down
> > > > to level1 (except if _PAGE_PSE is met, i.e. a large page was already set
> > > > up): the termination was:
> > > > 
> > > > [... walk down to level 2]
> > > 
> > > You have omitted:
> > > 
> > >         ASSERT(tab[offset] & _PAGE_PRESENT);
> > 
> > Right, that confirms that we never set PSE without PRESENT. We probably
> > want to keep that assertion in the PSE case.
> 
> Hmm, I'm not sure.
> 
> The hardware allows to have any bit set in the PTE when PRESENT isn't set.
> It will never look at any other bit in this case. Why should the software
> require something different here?

To document&check what we expect to be producing.

> > But we still want to continue walking down to level1 (or pse) even when
> > L4/L3/L2 have PRESENT.
> 
> We do that. In this case the is_leaf flag won't be set and need_pgt_func()
> will return early with the return value 0, meaning that the walk will be
> continued.

Ah, I missed that indeed, I had not integrated that is_leaf is 1 also in
the case of missing level>1 page. It'd probably be useful to emphasize
in patch 1:

+ * is_leaf: true if PTE doesn't address another page table

it should explicitly say (L1 PTE, or PSE, or not present yet)

The *pte & _PAGE_PRESENT test alone still looks surprising to the
reader, though: we don't return because the page is present, but because
we reached the last possible level that we could fill, which is either
L1, or an existing PSE. It is true that when is_leaf is 1, if it's not
L1 but present, it's necessarily a PSE, but it's clearer to the reader
to write what we meant: L1 or PSE, so something like:

if ( lvl == L1_FRAME || (*pte & _PAGE_PRESENT) ) {
    assert ( lvl == L1_FRAME || (*pte & _PAGE_PSE) ); // we are at last possible level
    [...]
}

Samuel