Re: [PATCH v3 14/20] xen/riscv: Implement p2m_pte_from_mfn() and support PBMT configuration

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.08.2025 16:44, Oleksii Kurochko wrote:
> On 8/11/25 1:36 PM, Jan Beulich wrote:
>> On 31.07.2025 17:58, Oleksii Kurochko wrote:
>>> +static pte_t p2m_pte_from_mfn(mfn_t mfn, p2m_type_t t, bool is_table)
>>> +{
>>> +    pte_t e = (pte_t) { PTE_VALID };
>> This and the rest of the function demand that mfn != INVALID_MFN, no matter
>> whether ...
>>
>>> +    switch ( t )
>>> +    {
>>> +    case p2m_mmio_direct_io:
>>> +        e.pte |= PTE_PBMT_IO;
>>> +        break;
>>> +
>>> +    default:
>>> +        break;
>>> +    }
>>> +
>>> +    pte_set_mfn(&e, mfn);
>>> +
>>> +    ASSERT(!(mfn_to_maddr(mfn) & ~PADDR_MASK));
>> ... PADDR_MASK is actually narrow enough to catch that case. Maybe best to
>> add an explicit assertion to that effect?
> 
> Then it should be enough instead of what we have now:
>    ASSERT(mfn_valid(mfn));

No, that would exclude MMIO living beyond max_page.

>>> +    if ( !is_table )
>>> +    {
>>> +        p2m_set_permission(&e, t);
>>> +
>>> +        if ( t < p2m_ext_storage )
>>> +            p2m_set_type(&e, t);
>>> +        else
>>> +            panic("unimplemeted\n");
>> The check is already done inside p2m_set_type() - why open-code it here?
> 
> It isn't really matters now (so could be dropped), but in further patch this 
> part
> of code will look like:
>          metadata[indx].pte = p2m_invalid;
> 
>          if ( t < p2m_ext_storage )
>              p2m_set_type(&e, t, indx);
>          else
>          {
>              e.pte |= MASK_INSR(p2m_ext_storage, P2M_TYPE_PTE_BITS_MASK);
>              p2m_set_type(metadata, t, indx);
>          }
> So my intention was to re-use p2m_set_type() without changing of a prototype. 
> So,
> if a type is stored in PTE bits then we pass PTE directly, if not - then pass
> metadata.

Then at the very least p2m_set_type() may not be a good name; a function of this
name imo should set the type, whatever it takes to do so. But I'm unconvinced of
the model as a whole.

Jan