Re: [PATCH v7 7/8] xen/riscv: page table handling

[oleksii.kurochko@gmail.com]

On Tue, 2024-09-24 at 12:49 +0200, Jan Beulich wrote:
> On 13.09.2024 17:57, Oleksii Kurochko wrote:
> 
> 
> > @@ -68,6 +108,52 @@ static inline bool pte_is_valid(pte_t p)
> >      return p.pte & PTE_VALID;
> >  }
> >  
> > +/*
> > + * From the RISC-V spec:
> > + *   The V bit indicates whether the PTE is valid; if it is 0, all
> > other bits
> > + *   in the PTE are don’t-cares and may be used freely by
> > software.
> > + *
> > + *   If V=1 the encoding of PTE R/W/X bits could be find in Table
> > 4.5.
> 
> Please avoid using table numbers when not also specifying the precise
> doc
> version. Numbering changes, and it's table 5.5 in the doc I'm looking
> at.
> Use table names instead (also elsewhere of course).
> 
> > + *   Table 4.5 summarizes the encoding of the permission bits.
> > + *      X W R Meaning
> > + *      0 0 0 Pointer to next level of page table.
> > + *      0 0 1 Read-only page.
> > + *      0 1 0 Reserved for future use.
> > + *      0 1 1 Read-write page.
> > + *      1 0 0 Execute-only page.
> > + *      1 0 1 Read-execute page.
> > + *      1 1 0 Reserved for future use.
> > + *      1 1 1 Read-write-execute page.
> > + */
> > +inline bool pte_is_table(const pte_t p)
> 
> Missing static?
static is missed. I'll add it, thanks.

> 
> We normally omit "const" on function arguments; the frequent request
> to add
> missing const is on pointed-to types. If you still want to have it,
> ...
> 
> > +{
> > +    /*
> > +     * According to the spec if V=1 and W=1 then R also needs to
> > be 1 as
> > +     * R = 0 is reserved for future use ( look at the Table 4.5 )
> > so check
> > +     * in ASSERT that if (V==1 && W==1) then R isn't 0.
> > +     *
> > +     * PAGE_HYPERVISOR_RW contains PTE_VALID too.
> > +     */
> > +    ASSERT(((p.pte & PAGE_HYPERVISOR_RW) != (PTE_VALID |
> > PTE_WRITABLE)));
> > +
> > +    return ((p.pte & (PTE_VALID | PTE_ACCESS_MASK)) == PTE_VALID);
> > +}
> > +
> > +static inline bool pte_is_mapping(/*const*/ pte_t p)
> 
> ... I wonder why it's then commented out here.
I just experimented and missed to uncomment it. If "const" could be
omit for none pointed-to types then I prefer to drop const here and in
pte_is_table().

> > +#define XEN_TABLE_MAP_FAILED 0
> > +#define XEN_TABLE_SUPER_PAGE 1
> > +#define XEN_TABLE_NORMAL 2
> > +
> > +/*
> > + * Take the currently mapped table, find the corresponding entry,
> > + * and map the next table, if available.
> > + *
> > + * The alloc_tbl parameters indicates whether intermediate tables
> > should
> > + * be allocated when not present.
> > + *
> > + * Return values:
> > + *  XEN_TABLE_MAP_FAILED: Either alloc_only was set and the entry
> > + *  was empty, or allocating a new page failed.
> > + *  XEN_TABLE_NORMAL: next level or leaf mapped normally
> > + *  XEN_TABLE_SUPER_PAGE: The next entry points to a superpage.
> > + */
> > +static int pt_next_level(bool alloc_tbl, pte_t **table, unsigned
> > int offset)
> > +{
> > +    pte_t *entry;
> > +    mfn_t mfn;
> > +
> > +    entry = *table + offset;
> > +
> > +    if ( !pte_is_valid(*entry) )
> > +    {
> > +        if ( !alloc_tbl )
> > +            return XEN_TABLE_MAP_FAILED;
> > +
> > +        if ( create_table(entry) )
> > +            return XEN_TABLE_MAP_FAILED;
> 
> You're still losing the -ENOMEM here.
Agree, I will save the return value of create_table and return it.

> > +        rc = -EOPNOTSUPP;
> > +        goto out;
> > +    }
> > +
> > +    entry = table + offsets[level];
> > +
> > +    rc = -EINVAL;
> > +    if ( !pt_check_entry(*entry, mfn, flags) )
> > +        goto out;
> > +
> > +    /* We are removing the page */
> > +    if ( !(flags & PTE_VALID) )
> > +        /*
> > +         * there is also a check in pt_check_entry() which check
> > that
> > +         * mfn=INVALID_MFN
> > +         */
> 
> Nit: Comments are to start with a capital letter.
> 
> > +        pte.pte = 0;
> > +    else
> > +    {
> > +        /* We are inserting a mapping => Create new pte. */
> > +        if ( !mfn_eq(mfn, INVALID_MFN) )
> > +            pte = pte_from_mfn(mfn, PTE_VALID);
> > +        else /* We are updating the permission => Copy the current
> > pte. */
> > +        {
> > +            pte = *entry;
> > +            pte.pte &= ~(flags & PTE_ACCESS_MASK);
> 
> Why does "flags" need using here? Simply clearing all PTE_ACCESS_MASK
> bits
> will do, won't it? And only that will guarantee that flags which are
> to be
> clear will actually be cleared.
Agree, there is no any necessity for using "flags" here, they should be
dropped.

> 
> > +/*
> > + * If `mfn` equals `INVALID_MFN`, it indicates that the following
> > page table
> > + * update operation might be related to either:
> > + *   - populating the table (PTE_POPULATE will be set
> > additionaly),
> > + *   - destroying a mapping (PTE_VALID=0),
> > + *   - modifying an existing mapping (PTE_VALID=1).
> > + *
> > + * If `mfn` != INVALID_MFN and flags has PTE_VALID bit set then it
> > means that
> > + * inserting will be done.
> > + */
> > +static int pt_update(unsigned long virt,
> 
> Don't you have vaddr_t for variables/parameters like this one?
Yes, I have. I will update update this and re-check other places.

> 
> > +                     mfn_t mfn,
> > +                     unsigned long nr_mfns,
> > +                     unsigned int flags)
> > +{
> > +    int rc = 0;
> > +    unsigned long vfn = PFN_DOWN(virt);
> > +    unsigned long left = nr_mfns;
> > +
> > +    const mfn_t root = get_root_page();
> 
> Why the blank line between adjacent declarations?
No specific reason, just wanted to group variables by usage ( but then
it should be one blank after rc ).
But I will just drop the blank for consistency with other local
variable of other functions.

~ Oleksii