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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/1/25 8:29 AM, Jan Beulich wrote:

> On 30.06.2025 18:18, Oleksii Kurochko wrote:
> > On 6/30/25 5:22 PM, Jan Beulich wrote:
> > > 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?
> > >         
> > Yes, with the current one implementation, I agree it would be enough only
> > root. But as you noticed below...
> >
> > > 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.
> > >         
> > ... after VMID management will changed to per-CPU base then it will be needed
> > to update re-calculate hgatp each time vCPU on pCPU is changed.
> > In this case I prefer to have partially calculated 'hgatp'.
> >       
> But why, when you need to do some recalculation anyway?
>     
Less operations will be needed to do.
If we have partially prepared 'hgatp' then we have to only update VMID bits
instead of getting ppn for page, then calculate hgatp_mode each time.
But if you think it isn't really needed I can add vmid argument for hgatp_from_page()
and just call this function when an update of hgatp is needed.

> > > > --- 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).
> > >         
> > You're right, the current name|clear_and_clean_page()| implies that clearing
> > should come before cleaning, which contradicts the current implementation.
> > The intent here is to ensure that the page contents are consistent in RAM
> > (not just in cache) before use by other entities (guests or devices).
> >
> > The clean must follow the clear — so yes, the order needs to be reversed.
> >       
> What you don't address though - why's the cleaning needed in the first place?
>     
If we clean the data cache first, we flush the d-cache and then use the page to
perform the clear operation. As a result, the "cleared" value will be written into
the d-cache. To avoid polluting the d-cache with the "cleared" value, the correct
sequence is to clear the page first, then clean the data cache.

> > > > +    unsigned int nr_pages = _AC(1,U) << order;
> > > >           
> > > Nit (style): Missing blank after comma.
> > >         
> > I've changed that to BIT(order, U)
> >
> > > > +    /* 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.
> > >         
> > It makes sense in this case just to return NULL.
> >
> > > 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.
> > >         
> > Agree, returning back pages necessary for p2m root table should be done under
> > spin_lock(&d->arch.paging.lock).
> >       
> Which should be acquired at the paging_*() layer then, not at the p2m_*() layer.
> (As long as you mean to have that separation, that is. See the earlier discussion
> on that matter.)
>     
Then partly p2m_set_allocation() should be moved to paging_*() too.

> > > > +    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.
> > >         
> > I thought it would be enough the comment above: /* Return back nr_pages necessary for p2m root table. */
> >       
> That describes what the code does, but not why.
>     
I will add to the comment: "... to get the memory accounting right".

> > > > +{
> > > > +    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.
> > >         
> > I am not really get your question.
> >
> > "wait" here is about the initialization of the pool which happens above this comment.
> >       
> But there's no "waiting" involved. What you talk about is one thing needing to
> happen after the other.
>     
Okay, then I will just reword comment.

Thanks.

~ Oleksii