Re: [PATCH v1 2/6] xen/riscv: introduce things necessary for p2m initialization

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/20/25 3:37 PM, Jan Beulich wrote:

> On 09.05.2025 17:57, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/arch/riscv/p2m.c
> > +static void clear_and_clean_page(struct page_info *page)
> > +{
> > +    void *p = __map_domain_page(page);
> > +
> > +    clear_page(p);
> > +    unmap_domain_page(p);
> > +}
> >       
>     
> What's the "clean" about in the function name? The "clear" is referring
> to the clear_page() call afaict.
>     
Missed to add clean_dcache_va_range() between clear_page() and unmap_domain_page().
> Also aren't you largely open-coding
> clear_domain_page() here?
>     
Yes, missed that it is almost the sane as clear_domain_page(), so we could re-write
this function as:
  static void clear_and_clean_page(struct page_info *page)
  {
      clean_dcache_va_range(page, PAGE_SIZE);
      clear_domain_page(page_to_mfn(page));
  }

> > +static struct page_info *p2m_get_clean_page(struct domain *d)
> > +{
> > +    struct page_info *page;
> > +
> > +    /*
> > +     * As mentioned in the Priviliged Architecture Spec (version 20240411)
> > +     * As explained in Section 18.5.1, for the paged virtual-memory schemes
> > +     * (Sv32x4, Sv39x4, Sv48x4, and Sv57x4), the root page table is 16 KiB
> > +     * and must be aligned to a 16-KiB boundary.
> > +     */
> > +    page = alloc_domheap_pages(NULL, 2, 0);
> >       
> Shouldn't this allocation come from the domain's P2M pool (which is yet
> to be introduced)?
>     
First, I will drop p2m_get_clean_page() as it will be used only for p2m root page
table allocation.

p2m_init() is called by domain_create() [->arch_domain_create()->p2m_init()] from create_domUs():
[https://gitlab.com/xen-project/xen/-/blob/staging/xen/common/device-tree/dom0less-build.c?ref_type=heads#L984].

When p2m_init() is called, p2m pool isn't ready and domain isn't created yet. Last one
is also crucial for usage of p2m pool as p2m pool belongs to domain and thereby it is
using alloc_domheap_page(d, ...) (Not NULL as for allocation of p2m root table above),
so domain should be created first.

And only after domain_create() will created domain, p2m pool could be initialized during
domain construction:
  https://gitlab.com/xen-project/xen/-/blob/staging/xen/common/device-tree/dom0less-build.c?ref_type=heads#L756
and the size of p2m pool depends on the value from memory property of domain node in DT.
(line 748, the link the same as above).

Also, if CONFIG_ARCH_PAGING_MEMPOOL=n, then p2m pool isn't used. But it isn't a case for RISC-V
for the moment. Probably one day it would be useful if someone wanted to add support for MMU-less
case. Something like Arm is doing now for R-cores.

> Also hard-coding 2 here as order effectively builds
> in an assumption that PAGE_SIZE will only ever be 4k. I think to wants
> properly calculating instead.
>     
I haven't thought about that. I will update it with:
  page = alloc_domheap_pages(NULL, get_order_from_bytes(KB(16)), 0);

> > +    if ( page == NULL )
> > +        return NULL;
> > +
> > +    clear_and_clean_page(page);
> > +
> > +    return page;
> > +}
> >       
> Contrary to the function name you obtained 4 pages here, which is suitable
> for ...
>
> > +static struct page_info *p2m_allocate_root(struct domain *d)
> > +{
> > +    return p2m_get_clean_page(d);
> > +}
> >       
> ... this but - I expect - no anywhere else.
>     
Totally agree, as mentioned above this function is used only for p2m_allocate_root().
I will just open-code it in p2m_allocate_root().
> > +{
> > +    unsigned long ppn;
> > +    unsigned long hgatp_mode;
> > +
> > +    ppn = PFN_DOWN(page_to_maddr(page_info)) & HGATP_PPN;
> > +
> > +    /* ASID (VMID) not supported yet */
> > +
> > +#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"
> >       
> As before, please have the # of pre-processor directives in the first column.
>
> > +#endif
> > +
> > +    return ppn | (hgatp_mode << HGATP_MODE_SHIFT);
> >       
> Use MASK_INSR()?
>     
Do you mean MASK_INSR(hgatp_mode, HGATP_MODE_MASK)?
If yes, then I didn't get what is the point then?

> > +}
> > +
> > +static int p2m_alloc_table(struct domain *d)
> > +{
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +
> > +    p2m->root = p2m_allocate_root(d);
> > +    if ( !p2m->root )
> > +        return -ENOMEM;
> > +
> > +    p2m->hgatp = hgatp_from_page_info(p2m->root);
> > +
> > +    /*
> > +     * Make sure that all TLBs corresponding to the new VMID are flushed
> > +     * before using it.
> > +     */
> > +    p2m_write_lock(p2m);
> > +    p2m_force_tlb_flush_sync(p2m);
> > +    p2m_write_unlock(p2m);
> >       
> While Andrew directed you towards a better model in general, it won't be
> usable here then, as the guest didn't run on any pCPU(s) yet. Imo you
> want to do a single global flush e.g. when VMIDs wrap around. That'll be
> fewer global flushes than one per VM creation.
>     
I am not sure that I get a phrase 'VMIDs wrap around'.

I am going to implement, p2m_force_tlb_flush_sync() as:
 static void p2m_force_tlb_flush_sync(struct p2m_domain *p2m)
 {
   ...
     sbi_remote_hfence_gvma(d->dirty_cpumask, 0, 0);
   ...
 }

With such implementation if the guest didn't run on any pCPU(s) yet
then d->dirty_cpumask is empty, then sbi_remote_hfence_gvma() will do nothing
as hmask will be NULL (https://gitlab.com/xen-project/people/olkur/xen/-/blob/staging/xen/arch/riscv/sbi.c?ref_type=heads#L238).
I am not sure that it is a good idea as I can't find a guarantee in the spec
that TLB will be empty during boot time.

But if another VM is being created then we should flush stage2 before run a VM
so, the new VM won't re-use something from the old VM.
Or in case of VMID if VMID is reused by new VM in case if, for example, the
previous owner(domain) was destroyed and a new domain is reusing VMID, it is
needed to flush stage2.

p2m_alloc_table() looks a good place for that and I am not sure that we can
do a single global flush, and I don't really know in first glance where it
should be done.


> > +    p2m->default_access = p2m_access_rwx;
> > +
> > +    radix_tree_init(&p2m->p2m_type);
> > +
> > +#ifdef CONFIG_HAS_PASSTHROUGH
> > +    /*
> > +     * Some IOMMUs don't support coherent PT walk. When the p2m is
> > +     * shared with the CPU, Xen has to make sure that the PT changes have
> > +     * reached the memory
> > +     */
> > +    p2m->clean_pte = is_iommu_enabled(d) &&
> > +        !iommu_has_feature(d, IOMMU_FEAT_COHERENT_WALK);
> > +#else
> > +    p2m->clean_pte = true;
> >       
> When there's no IOMMU (in use), doesn't this want to be "false"?
>     
I think you are right, "false" is more correct here.

> > +#endif
> > +
> > +    /*
> > +     * "Trivial" initialisation is now complete.  Set the backpointer so
> > +     * p2m_teardown() and friends know to do something.
> > +     */
> > +    p2m->domain = d;
> >       
> And where is that p2m_teardown(), to cross-check the comment against?
>     
It is not introduced now as I expected it is need only when domain is needed to
be stop for some reason. And it isn't really needed now.

Anyway, it seems like it is a stale comment as on other arch-es p2m_teardown() has
an argument with struct domain *d.

I can update the commit to:
 "Trivial" initialisation is now complete.  Set the backpointer so the users of p2m
  could get an access to domain structure.

~ Oleksii