Re: [PATCH v4 12/18] xen/riscv: Implement p2m_pte_from_mfn() and support PBMT configuration

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.09.2025 15:30, Oleksii Kurochko wrote:
> 
> On 9/22/25 6:28 PM, Jan Beulich wrote:
>> On 17.09.2025 23:55, Oleksii Kurochko wrote:
>>> @@ -318,11 +331,87 @@ static inline void p2m_clean_pte(pte_t *p, bool 
>>> clean_pte)
>>>       p2m_write_pte(p, pte, clean_pte);
>>>   }
>>>   
>>> -static pte_t p2m_pte_from_mfn(mfn_t mfn, p2m_type_t t)
>>> +static void p2m_set_permission(pte_t *e, p2m_type_t t)
>>>   {
>>> -    panic("%s: hasn't been implemented yet\n", __func__);
>>> +    e->pte &= ~PTE_ACCESS_MASK;
>>> +
>>> +    e->pte |= PTE_USER;
>>> +
>>> +    /*
>>> +     * Two schemes to manage the A and D bits are defined:
>>> +     *   • The Svade extension: when a virtual page is accessed and the A 
>>> bit
>>> +     *     is clear, or is written and the D bit is clear, a page-fault
>>> +     *     exception is raised.
>>> +     *   • When the Svade extension is not implemented, the following 
>>> scheme
>>> +     *     applies.
>>> +     *     When a virtual page is accessed and the A bit is clear, the PTE 
>>> is
>>> +     *     updated to set the A bit. When the virtual page is written and 
>>> the
>>> +     *     D bit is clear, the PTE is updated to set the D bit. When 
>>> G-stage
>>> +     *     address translation is in use and is not Bare, the G-stage 
>>> virtual
>>> +     *     pages may be accessed or written by implicit accesses to 
>>> VS-level
>>> +     *     memory management data structures, such as page tables.
>>> +     * Thereby to avoid a page-fault in case of Svade is available, it is
>>> +     * necesssary to set A and D bits.
>>> +     */
>>> +    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>>> +        e->pte |= PTE_ACCESSED | PTE_DIRTY;
>> All of this depending on menvcfg.ADUE anyway, is this really needed? Isn't
>> machine mode software responsible for dealing with this kind of page faults
>> (just like the hypervisor is reponsible for dealing with ones resulting
>> from henvcfg.ADUE being clear)?
> 
> In general, I think you are right.
> 
> In this case, though, I just wanted to avoid unnecessary page faults for now.
> My understanding is that having such faults handled by the hypervisor can 
> indeed
> be useful, for example to track which pages are being accessed. However, 
> since we
> currently don’t track page usage, handling these traps would only result in
> setting the A and D bits and then returning control to the guest.

Yet that still be be machine-mode software aiui. By always setting the bits we'd
undermine whatever purpose _they_ have enabled the extension for, wouldn't we?

> To avoid this overhead, I chose to set the bits up front.

Irrespective to the answer to the question above, if you mean to do so, I think
all of this needs explaining better in the comment.

Jan