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

[Jürgen Groß <jgross@xxxxxxxx>]

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