Re: [PATCH 2/3] mini-os: mm: switch need_pgt() to use walk_pt()

[Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>]

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