Re: [PATCH v2 17/17] xen/riscv: add support of page lookup by GFN

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 7/2/25 1:44 PM, Jan Beulich wrote:

> On 10.06.2025 15:05, Oleksii Kurochko wrote:
> > Introduce helper functions for safely querying the P2M (physical-to-machine)
> > mapping:
> >  - add p2m_read_lock(), p2m_read_unlock(), and p2m_is_locked() for managing
> >    P2M lock state.
> >  - Implement p2m_get_entry() to retrieve mapping details for a given GFN,
> >    including MFN, page order, and validity.
> >  - Add p2m_lookup() to encapsulate read-locked MFN retrieval.
> >  - Introduce p2m_get_page_from_gfn() to convert a GFN into a page_info
> >    pointer, acquiring a reference to the page if valid.
> >
> > Implementations are based on Arm's functions with some minor modifications:
> > - p2m_get_entry():
> >   - Reverse traversal of page tables, as RISC-V uses the opposite order
> >     compared to Arm.
> >   - Removed the return of p2m_access_t from p2m_get_entry() since
> >     mem_access_settings is not introduced for RISC-V.
> >       
> Didn't I see uses of p2m_access in earlier patches? If you don't mean to have
> that, then please consistently {every,no}where.
>     
Yes, it was used. I think it would be better just usage of p2m_access from earlier
patches.

> >   - Updated BUILD_BUG_ON() to check using the level 0 mask, which corresponds
> >     to Arm's THIRD_MASK.
> >   - Replaced open-coded bit shifts with the BIT() macro.
> >   - Other minor changes, such as using RISC-V-specific functions to validate
> >     P2M PTEs, and replacing Arm-specific GUEST_* macros with their RISC-V
> >     equivalents.
> > - p2m_get_page_from_gfn():
> >   - Removed p2m_is_foreign() and related logic, as this functionality is not
> >     implemented for RISC-V.
> >       
> Yet I expect you'll need this, sooner or later.
>     
Then I'll add correspondent code in this patch.
> > --- a/xen/arch/riscv/p2m.c
> > +++ b/xen/arch/riscv/p2m.c
> > @@ -1055,3 +1055,134 @@ int guest_physmap_add_entry(struct domain *d,
> >  {
> >      return p2m_insert_mapping(d, gfn, (1 << page_order), mfn, t);
> >  }
> > +
> > +/*
> > + * Get the details of a given gfn.
> > + *
> > + * If the entry is present, the associated MFN will be returned and the
> > + * access and type filled up. The page_order will correspond to the
> >       
> You removed p2m_access_t * from the parameters; you need to also update
> the comment then accordingly.
>
> > + * order of the mapping in the page table (i.e it could be a superpage).
> > + *
> > + * If the entry is not present, INVALID_MFN will be returned and the
> > + * page_order will be set according to the order of the invalid range.
> > + *
> > + * valid will contain the value of bit[0] (e.g valid bit) of the
> > + * entry.
> > + */
> > +static mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
> > +                           p2m_type_t *t,
> > +                           unsigned int *page_order,
> > +                           bool *valid)
> > +{
> > +    paddr_t addr = gfn_to_gaddr(gfn);
> > +    unsigned int level = 0;
> > +    pte_t entry, *table;
> > +    int rc;
> > +    mfn_t mfn = INVALID_MFN;
> > +    p2m_type_t _t;
> >       
> Please no local variables with leading underscores. In x86 we commonly
> name such variables p2mt.
>
> > +    DECLARE_OFFSETS(offsets, addr);
> >       
> This is the sole use of "addr". Is such a local variable really worth having?
>     
Not really, I'll drop it.

> > +    ASSERT(p2m_is_locked(p2m));
> > +    BUILD_BUG_ON(XEN_PT_LEVEL_MAP_MASK(0) != PAGE_MASK);
> > +
> > +    /* Allow t to be NULL */
> > +    t = t ?: &_t;
> > +
> > +    *t = p2m_invalid;
> > +
> > +    if ( valid )
> > +        *valid = false;
> > +
> > +    /* XXX: Check if the mapping is lower than the mapped gfn */
> > +
> > +    /* This gfn is higher than the highest the p2m map currently holds */
> > +    if ( gfn_x(gfn) > gfn_x(p2m->max_mapped_gfn) )
> > +    {
> > +        for ( level = P2M_ROOT_LEVEL; level ; level-- )
> >       
> Nit: Stray blank before the 2nd semicolon. (Again at least once below.)
>
> > +            if ( (gfn_x(gfn) & (XEN_PT_LEVEL_MASK(level) >> PAGE_SHIFT)) >
> > +                 gfn_x(p2m->max_mapped_gfn) )
> > +                break;
> > +
> > +        goto out;
> > +    }
> > +
> > +    table = p2m_get_root_pointer(p2m, gfn);
> > +
> > +    /*
> > +     * the table should always be non-NULL because the gfn is below
> > +     * p2m->max_mapped_gfn and the root table pages are always present.
> > +     */
> > +    if ( !table )
> > +    {
> > +        ASSERT_UNREACHABLE();
> > +        level = P2M_ROOT_LEVEL;
> > +        goto out;
> > +    }
> > +
> > +    for ( level = P2M_ROOT_LEVEL; level ; level-- )
> > +    {
> > +        rc = p2m_next_level(p2m, true, level, &table, offsets[level]);
> > +        if ( (rc == GUEST_TABLE_MAP_NONE) && (rc != GUEST_TABLE_MAP_NOMEM) )
> >       
> This condition looks odd. As written the rhs of the && is redundant.
>     
And it is wrong. It should:
 if ( (rc == P2M_TABLE_MAP_NONE) || (rc == P2M_TABLE_MAP_NOMEM) )
> > +            goto out_unmap;
> > +        else if ( rc != GUEST_TABLE_NORMAL )
> >       
> As before, no real need for "else" in such cases.
>
> > +            break;
> > +    }
> > +
> > +    entry = table[offsets[level]];
> > +
> > +    if ( p2me_is_valid(p2m, entry) )
> > +    {
> > +        *t = p2m_type_radix_get(p2m, entry);
> >       
> If the incoming argument is NULL, the somewhat expensive radix tree lookup
> is unnecessary here.
>     
Good point.

> > +        mfn = pte_get_mfn(entry);
> > +        /*
> > +         * The entry may point to a superpage. Find the MFN associated
> > +         * to the GFN.
> > +         */
> > +        mfn = mfn_add(mfn,
> > +                      gfn_x(gfn) & (BIT(XEN_PT_LEVEL_ORDER(level), UL) - 1));
> > +
> > +        if ( valid )
> > +            *valid = pte_is_valid(entry);
> >       
> Interesting. Why not the P2M counterpart of the function? Yes, the comment
> ahead of the function says so, but I don't see why the valid bit suddenly
> is relevant here (besides the P2M type).
>     
This valid variable is expected to be used in the caller (something what Arm does here
https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/arm/p2m.c#L375) to check if
it is needed to do flush_page_to_ram(), so if the the valid bit in PTE was set to 0
then it means nothing should be flushed as entry wasn't used as it marked invalid.

> > +    }
> > +
> > +out_unmap:
> > +    unmap_domain_page(table);
> > +
> > +out:
> >       
> Nit: Style (bot labels).
>
> > +    if ( page_order )
> > +        *page_order = XEN_PT_LEVEL_ORDER(level);
> > +
> > +    return mfn;
> > +}
> > +
> > +static mfn_t p2m_lookup(struct domain *d, gfn_t gfn, p2m_type_t *t)
> >       
> pointer-to-const for the 1st arg? But again more likely struct p2m_domain *
> anyway?
>     
struct p2_domain would be better, but I’m not really sure that a
pointer-to-const can be used here. I expect that, in that case,
p2m_read_lock() would also need to accept a pointer-to-const, and since it
calls read_lock() internally, that could be a problem because read_lock()
accepts a rwlock_t *l.

> > +{
> > +    mfn_t mfn;
> > +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +
> > +    p2m_read_lock(p2m);
> > +    mfn = p2m_get_entry(p2m, gfn, t, NULL, NULL);
> > +    p2m_read_unlock(p2m);
> > +
> > +    return mfn;
> > +}
> > +
> > +struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
> >       
> Same here - likely you mean struct p2m_domain * instead.
>
> > +                                        p2m_type_t *t)
> > +{
> > +    p2m_type_t p2mt = {0};
> >       
> Why a compound initializer for something that isn't a compound object?
> And why plain 0 for something that is an enumerated type?
>     
Agree, it should be a compound initializer. I'll drop it.

> > +    struct page_info *page;
> > +
> > +    mfn_t mfn = p2m_lookup(d, gfn, &p2mt);
> > +
> > +    if ( t )
> > +        *t = p2mt;
> >       
> What's wrong with passing t directly to p2m_lookup()?
>     
It was needed before when the code of p2m_get_page_from_gfn() looked like:
  struct page_info *p2m_get_page_from_gfn(struct domain *d, gfn_t gfn,
                                          p2m_type_t *t)
  {
      struct page_info *page;
      p2m_type_t p2mt;
      mfn_t mfn = p2m_lookup(d, gfn, &p2mt);
  
      if ( t )
          *t = p2mt;
  
      if ( !p2m_is_any_ram(p2mt) )
          return NULL;
So it was needed to make sure that p2m_is_any_ram(*t) doesn't try to dereference
a NULL pointer.

Even with the current one implementation the similar issue could be with if use
*t instead of p2mt:
  struct page_info *p2m_get_page_from_gfn(struct p2m_domain *p2m, gfn_t gfn,
                                          p2m_type_t *t)
  {
      ...
      if ( p2m_is_foreign(p2mt) )
      {
          struct domain *fdom = page_get_owner_and_reference(page);

And the second reason it is because of p2m_get_entry() (which is used inside
p2m_lookup()) could return for `t` a pointer to local variable:
  static mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
                             p2m_type_t *t,
                             unsigned int *page_order,
                             bool *valid)
  {
      ...
      p2m_type_t p2mt;
      ...  
      /* Allow t to be NULL */
      t = t ?: &p2mt;
      ...
What looks wrong. I will remove this part of the code and pass
`t` directly to p2m_lookup().

And after p2m_lookup() call will just check if t argument is NULL then init
it with p2mt:
   struct page_info *p2m_get_page_from_gfn(struct p2m_domain *p2m, gfn_t gfn,
                                           p2m_type_t *t)
   {
       struct page_info *page;
       p2m_type_t p2mt = p2m_invalid;
       mfn_t mfn = p2m_lookup(p2m, gfn, t);
   
       if ( !mfn_valid(mfn) )
           return NULL;
   
       if ( !t )
           p2mt = *t;
   
       ...
   }

Thanks.

~ Oleksii