[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 12/18] xen/riscv: Implement p2m_pte_from_mfn() and support PBMT configuration
- To: Jan Beulich <jbeulich@xxxxxxxx>
- From: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
- Date: Fri, 10 Oct 2025 10:29:48 +0200
- Cc: Alistair Francis <alistair.francis@xxxxxxx>, Bob Eshleman <bobbyeshleman@xxxxxxxxx>, Connor Davis <connojdavis@xxxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Anthony PERARD <anthony.perard@xxxxxxxxxx>, Michal Orzel <michal.orzel@xxxxxxx>, Julien Grall <julien@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
- Delivery-date: Fri, 10 Oct 2025 08:29:57 +0000
- List-id: Xen developer discussion <xen-devel.lists.xenproject.org>
On 10/9/25 2:06 PM, Jan Beulich wrote:
On 09.10.2025 11:21, Oleksii Kurochko wrote:
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.
Won't Xen need its own page fault handler anyway?
Of course, it will.
I just meant that it won’t need it solely for the purpose of setting the A and
D bits.
Considering that Svade is mandatory for RVAxx profiles, and that at some point
we may want to implement certain optimizations (mentioned below), it would make
sense to handle the A/D bits in the page fault handler.
However, for now, for the sake of simplicity and given that none of the
optimizations mentioned below are currently implemented and OpenSBI delegates
page fault handling to hypervisor so OpenSBI isn't planning to deal with A/D
bits, I think we can set the A/D bits during PTE creation with a comment
explaining why it’s done this way, as suggested before.
Later, when additional optimizations that rely on A/D bits are needed, we can
remove this initial setting and add proper A/D handling in the page fault
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.
What I don't really understand is what the intended use of that extension is.
I think this is mainly for software-managed PTE A/D bit updates, which could be
useful for several use cases such as demand paging, cache flushing optimizations,
and memory access tracking.
Also, from a hardware perspective, it’s probably simpler to let software manage
the PTE A/D bits (using the Svade extension) rather than implementing the
Svadu extension for hardware-managed updates.
~ Oleksii
Surely every entity should be responsible for its own A/D bits, with lower
layers coming into play only when certain things need e.g. emulating. This
lack of understanding on my part extends to ...
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.
... this point, which I was also wondering about before.
|
