On 17.11.2025 12:36, Oleksii Kurochko wrote:
> On 11/10/25 4:29 PM, Jan Beulich wrote:
>> On 20.10.2025 17:57, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/p2m.c
>>> +++ b/xen/arch/riscv/p2m.c
>>> @@ -17,6 +17,8 @@
>>> #include <asm/riscv_encoding.h>
>>> #include <asm/vmid.h>
>>>
>>> +#define P2M_SUPPORTED_LEVEL_MAPPING 2
>> I fear without a comment it's left unclear what this is / represents.
>
> Probably just renaming it to|P2M_MAX_SUPPORTED_LEVEL_MAPPING| would make it
> clearer,
> wouldn’t it?
> Otherwise, I can add the following comment:
> /*
> * At the moment, only 4K, 2M, and 1G mappings are supported for G-stage
> * translation. Therefore, the maximum supported page-table level is 2,
> * which corresponds to 1G mappings.
> */
Both the name change and the comment, if you ask me.
>>> @@ -403,11 +415,147 @@ static int p2m_next_level(struct p2m_domain *p2m,
>>> bool alloc_tbl,
>>> return P2M_TABLE_MAP_NONE;
>>> }
>>>
>>> +static void p2m_put_foreign_page(struct page_info *pg)
>>> +{
>>> + /*
>>> + * It’s safe to call put_page() here because arch_flush_tlb_mask()
>>> + * will be invoked if the page is reallocated, which will trigger a
>>> + * flush of the guest TLBs.
>>> + */
>>> + put_page(pg);
>>> +}
>>> +
>>> +/* Put any references on the single 4K page referenced by mfn. */
>> To me this and ...
>>
>>> +static void p2m_put_4k_page(mfn_t mfn, p2m_type_t type)
>>> +{
>>> + /* TODO: Handle other p2m types */
>>> +
>>> + if ( p2m_is_foreign(type) )
>>> + {
>>> + ASSERT(mfn_valid(mfn));
>>> + p2m_put_foreign_page(mfn_to_page(mfn));
>>> + }
>>> +}
>>> +
>>> +/* Put any references on the superpage referenced by mfn. */
>> ... to a lesser degree this comment are potentially misleading. Down here at
>> least there is something plural-ish (the 4k pages that the 2M one consists
>> of), but especially for the single page case above "any" could easily mean
>> "anything that's still outstanding, anywhere". I'm also not quite sure "on"
>> is really what you mean (I'm not a native speaker, so my gut feeling may be
>> wrong here).
>
> Then I could suggest the following instead:
> /* Put the reference associated with the 4K page identified by mfn. */
> and
> /* Put the references associated with the superpage identified by mfn. */
>
> I think the comments could be omitted, since the function names already make
> this clear.
Okay with me.
Jan
