Re: [PATCH v2 06/17] xen/riscv: add root page table allocation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.06.2025 15:05, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/include/asm/p2m.h
> +++ b/xen/arch/riscv/include/asm/p2m.h
> @@ -26,6 +26,12 @@ struct p2m_domain {
>      /* Pages used to construct the p2m */
>      struct page_list_head pages;
>  
> +    /* The root of the p2m tree. May be concatenated */
> +    struct page_info *root;
> +
> +    /* Address Translation Table for the p2m */
> +    paddr_t hgatp;

Does this really need holding in a struct field? Can't is be re-created at
any time from "root" above? And such re-creation is apparently infrequent,
if happening at all after initial allocation. (But of course I don't know
what future patches of yours will bring.) This is even more so if ...

> --- a/xen/arch/riscv/include/asm/riscv_encoding.h
> +++ b/xen/arch/riscv/include/asm/riscv_encoding.h
> @@ -133,11 +133,13 @@
>  #define HGATP_MODE_SV48X4            _UL(9)
>  
>  #define HGATP32_MODE_SHIFT           31
> +#define HGATP32_MODE_MASK            _UL(0x80000000)
>  #define HGATP32_VMID_SHIFT           22
>  #define HGATP32_VMID_MASK            _UL(0x1FC00000)
>  #define HGATP32_PPN                  _UL(0x003FFFFF)
>  
>  #define HGATP64_MODE_SHIFT           60
> +#define HGATP64_MODE_MASK            _ULL(0xF000000000000000)
>  #define HGATP64_VMID_SHIFT           44
>  #define HGATP64_VMID_MASK            _ULL(0x03FFF00000000000)

... VMID management is going to change as previously discussed, at which
point the value to put in hgatp will need (partly) re-calculating at certain
points anyway.

> --- a/xen/arch/riscv/p2m.c
> +++ b/xen/arch/riscv/p2m.c
> @@ -41,6 +41,91 @@ void p2m_write_unlock(struct p2m_domain *p2m)
>      write_unlock(&p2m->lock);
>  }
>  
> +static void clear_and_clean_page(struct page_info *page)
> +{
> +    clean_dcache_va_range(page, PAGE_SIZE);
> +    clear_domain_page(page_to_mfn(page));
> +}

A function of this name can, imo, only clear and then clean. Question is why
it's the other way around, and what the underlying requirement is for the
cleaning part to be there in the first place. Maybe that's obvious for a
RISC-V person, but it's entirely non-obvious to me (Arm being different in
this regard because of running with caches disabled at certain points in
time).

> +static struct page_info *p2m_allocate_root(struct domain *d)
> +{
> +    struct page_info *page;
> +    unsigned int order = get_order_from_bytes(KB(16));

While better than a hard-coded order of 2, this still is lacking. Is there
a reason there can't be a suitable manifest constant in the header?

> +    unsigned int nr_pages = _AC(1,U) << order;

Nit (style): Missing blank after comma.

> +    /* Return back nr_pages necessary for p2m root table. */
> +
> +    if ( ACCESS_ONCE(d->arch.paging.p2m_total_pages) < nr_pages )
> +        panic("Specify more xen,domain-p2m-mem-mb\n");

You shouldn't panic() in anything involved in domain creation. You want to
return NULL in this case.

Further, to me the use of "more" looks misleading here. Do you perhaps mean
"larger" or "bigger"?

This also looks to be happening without any lock held. If that's intentional,
I think the "why" wants clarifying in a code comment.

> +    for ( unsigned int i = 0; i < nr_pages; i++ )
> +    {
> +        /* Return memory to domheap. */
> +        page = page_list_remove_head(&d->arch.paging.p2m_freelist);
> +        if( page )
> +        {
> +            ACCESS_ONCE(d->arch.paging.p2m_total_pages)--;
> +            free_domheap_page(page);
> +        }
> +        else
> +        {
> +            printk(XENLOG_ERR
> +                   "Failed to free P2M pages, P2M freelist is empty.\n");
> +            return NULL;
> +        }
> +    }

The reason for doing this may also want to be put in a comment.

> +    /* Allocate memory for p2m root table. */
> +
> +    /*
> +     * As mentioned in the Priviliged Architecture Spec (version 20240411)
> +     * As explained in Section 18.5.1, for the paged virtual-memory schemes

The first sentence didn't finish when the 2nd starts. Is there a piece missing?
Do the two sentences want to be joined together?

> +static unsigned long hgatp_from_page(struct p2m_domain *p2m)

Function name and parameter type/name don't fit together.

> +{
> +    struct page_info *p2m_root_page = p2m->root;

As always: pointer-to-const wherever possible, please. But: Is this local
variable really useful to have?

> +    unsigned long ppn;
> +    unsigned long hgatp_mode;
> +
> +    ppn = PFN_DOWN(page_to_maddr(p2m_root_page)) & HGATP_PPN;
> +
> +#if RV_STAGE1_MODE == SATP_MODE_SV39
> +    hgatp_mode = HGATP_MODE_SV39X4;
> +#elif RV_STAGE1_MODE == SATP_MODE_SV48
> +    hgatp_mode = HGATP_MODE_SV48X4;
> +#else
> +#   error "add HGATP_MODE"
> +#endif
> +
> +    return ppn | MASK_INSR(p2m->vmid, HGATP_VMID_MASK) |
> +           MASK_INSR(hgatp_mode, HGATP_MODE_MASK);
> +}
> +
> +static int p2m_alloc_root_table(struct domain *d)

As indicated earlier, in a wider context - this is a good candidate where
the caller rather wants to pass struct p2m_domain *. Once you get variations
on P2Ms (like x86'es altp2m or nestedp2m, the domain won't be meaningful
here to know which P2M to allocate the root for.

> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +
> +    p2m->root = p2m_allocate_root(d);
> +    if ( !p2m->root )
> +        return -ENOMEM;
> +
> +    p2m->hgatp = hgatp_from_page(p2m);
> +
> +    return 0;
> +}
> +
>  static spinlock_t vmid_alloc_lock = SPIN_LOCK_UNLOCKED;
>  
>  /*
> @@ -228,5 +313,14 @@ int p2m_set_allocation(struct domain *d, unsigned long 
> pages, bool *preempted)
>          }
>      }
>  
> +    /*
> +    * First, wait for the p2m pool to be initialized. Then allocate the root

Why "wait"? There's waiting here.

> +    * table so that the necessary pages can be returned from the p2m pool,
> +    * since the root table must be allocated using alloc_domheap_pages(...)
> +    * to meet its specific requirements.
> +    */
> +    if ( !d->arch.p2m.root )

Aren't you open-coding p2m_get_hostp2m() here?

Jan

> +        p2m_alloc_root_table(d);
> +
>      return 0;
>  }