On 21.07.2025 11:43, Oleksii Kurochko wrote:
> On 7/2/25 1:44 PM, Jan Beulich wrote:
>> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>>> --- 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.
I hope you see what a mess you get if you have two flavors of "valid" for a
PTE.
>>> + }
>>> +
>>> +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.
Note how I also didn't suggest const there.
> 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|.
Correct.
Jan
