Re: [for 4.22 v5 18/18] xen/riscv: introduce metadata table to store P2M type

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2025 16:38, Oleksii Kurochko wrote:
> On 11/18/25 7:58 AM, Jan Beulich wrote:
>> On 17.11.2025 20:51, Oleksii Kurochko wrote:
>>> On 11/12/25 12:49 PM, Jan Beulich wrote:
>>>> On 20.10.2025 17:58, Oleksii Kurochko wrote:
>>>>> +    if ( *md_pg )
>>>>> +        metadata = __map_domain_page(*md_pg);
>>>>> +
>>>>> +    if ( t < p2m_first_external )
>>>>> +    {
>>>>>            pte->pte |= MASK_INSR(t, P2M_TYPE_PTE_BITS_MASK);
>>>>>    -    return rc;
>>>>> +        if ( metadata )
>>>>> +            metadata[ctx->index].pte = p2m_invalid;
>>>> Shouldn't this be accompanied with a BUILD_BUG_ON(p2m_invalid), as 
>>>> otherwise
>>>> p2m_alloc_page()'s clearing of the page won't have the intended effect?
>>> I think that, at least, at the moment we are always explicitly set p2m type 
>>> and
>>> do not rely on that by default 0==p2m_invalid.
>> You don't, and ...
>>
>>> Just to be safe, I will add after "if ( metadata )" suggested
>>> BUILD_BUG_ON(p2m_invalid):
>>>          if ( metadata )
>>>              metadata[ctx->index].type = p2m_invalid;
>>>                  /*
>>>           * metadata.type is expected to be p2m_invalid (0) after the page 
>>> is
>>>           * allocated and zero-initialized in p2m_alloc_page().
>>>           */
>>>          BUILD_BUG_ON(p2m_invalid);
>>> ...
>> ... this leaves me with the impression that you didn't read my reply 
>> correctly.
>> p2m_alloc_page() clear the page, thus _implicitly_ setting all entries to
>> p2m_invalid. That's where the BUILD_BUG_ON() would want to go (the call site,
>> ftaod).
> 
> I think I still don’t fully understand what the issue would be 
> if|p2m_invalid| were
> ever equal to 1 instead of 0 in the context of a metadata page.
> 
> Yes, if|p2m_invalid| were 1, there would be a problem if someone tried to 
> read this
> metadata pagebefore it was assigned any type. They would find a value of 0, 
> which
> corresponds to a valid type rather than to|p2m_invalid|, as one might expect.
> However, I’m not sure I currently see a scenario in which the metadata page 
> would
> be read before being initialized.

Are you sure walks can only happen for GFNs that were set up? What you need to
do walks on is under guest control, after all.

> But just to be safe when such case will occur I am okay with putting
> BUILD_BUG_ON(p2m_invalid) before p2m_alloc_page() in p2m_set_type() function.

Thanks.

>>>>> +static void p2m_free_page(struct p2m_domain *p2m, struct page_info *pg);
>>>>> +
>>>>> +/*
>>>>> + * Free page table's page and metadata page linked to page table's page.
>>>>> + */
>>>>> +static void p2m_free_table(struct p2m_domain *p2m, struct page_info 
>>>>> *tbl_pg)
>>>>> +{
>>>>> +    if ( tbl_pg->v.md.pg )
>>>>> +        p2m_free_page(p2m, tbl_pg->v.md.pg);
>>>> To play safe, maybe better also clear tbl_pg->v.md.pg?
>>> I thought it would be enough to clear it during allocation in 
>>> p2m_alloc_page(),
>>> since I'm not sure it is critical if md.pg data were somehow leaked and 
>>> read.
>>> But to be safer, we can add this here:
>>>     clear_and_clean_page(tbl_pg->v.md.pg, p2m->clean_dcache);
>> I didn't say clear what tbl_pg->v.md.pg points to, though. I suggested to 
>> clear
>> the struct field itself.
> 
> Won't be enough just tbl_pg->v.md.pg = NULL; ?

Exactly that, yes.

Jan