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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/7/25 3:09 PM, Jan Beulich wrote:

> 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?
>     
It’s a good point, and from an architectural perspective, it’s possible that
machine-mode software might want to handle page faults.
However, looking at OpenSBI, it delegates (otherwise all traps/interrupts by
default are going to machine-mode) page faults [1] to lower modes, and I expect
that other machine-mode software does the same (but of course there is no such
guarantee).
Therefore, considering that OpenSBI delegates page faults to lower modes and
does not set the A and D bits for p2m (guest) PTEs, this will result in a page
fault being handled by the hypervisor. As a result, we don’t affect the behavior
of machine-mode software at all.
If we want to avoid depending on how OpenSBI or other machine-mode software is
implemented, we might instead want to have our own page fault handler in Xen,
and then set the A and D bits within this handler.
Do you think it would be better to do in this way from the start? If yes, then
we also want drop setting of A and D bits for Xen's PTEs [3] to allow M-mode to
handle S/HS-mode page faults.
Interestingly, OpenSBI doesn’t allow hypervisor mode to decide whether to
support Svade or not [2]. By doing so, we can’t set henvcfg.adue = 1 to disable
it as menvcfg.adue=0 has more power, which is not very flexible.

[1] https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L209
[2] https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L168
[3] https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/riscv/pt.c?ref_type=heads#L343

> > 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.
>     
Sure, I will add the comment if the current one approach of setting A and D bits
will be chosen.

~ Oleksii