|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v5 1/3] xen/riscv: always preset A/D bits in G-stage PTEs
Le 07/10/2026 à 10:24, Baptiste Le Duc a écrit : On 10/7/26 00:50, Teddy Astie wrote:Le 05/10/2026 à 18:02, Baptiste Le Duc a écrit :A RISC-V implementation can manage the PTE A/D bits in one of two ways: 1) Update the 'A' and 'D' PTE bits in hardware (ratified as Svadu).2) Generate a page fault when 'A' and/or 'D' is clear, so that softwarecan set them (ratified as Svade).p2m_set_pte_flags() presets A/D in G-stage PTEs only when the device tree advertises Svade. Platforms that use scheme (2) without advertising Svade, such as the HiFive Premier P550, then hit guest-page faults that Xen doesnot handle.I second what Jan said (this is likely a firmware bug, given that the enumerated behavior is not consistent with the specification, as existence of Svadu/Svade alters observable behavior).Thanks, agreed on both points.I'll reword the commit message: it shouldn't imply that Svade in the DT means faults will occur, since with Svadu also present that depends on henvcfg.ADUE.On the firmware-bug point: I don't think the P550 is really inconsistent with the spec. Svade and Svadu didn't exist when the P550 core was designed. They were only ratified later, to name the two behaviors that already existed in the wild. Before that, the privileged spec just allowed either: hardware updating A/D, or raising a page fault. The P550 implements the second, and there was no extension it could advertise for it. Therefore, the DT lacks svade because the extension didn't exist when the platform description was written, not because the vendor misdescribed anything. At least in latest specification, absence of Svade implies that A/D bit is updated on page-table access. The spec also insist on the no-Svade behavior. > 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. > > (... hardware updates A/D bit) The no-Svade behavior can then be re-introduced with Svadu with ADUE=1 > If the Svadu extension is implemented, the ADUE bit controls whether > hardware updating of PTE A/D bits is enabled for VS-stage address > translation. When ADUE=1, hardware updating of PTE A/D bits is enabled > during VS-stage address translation, and the implementation behaves as > though the Svade extension were not implemented for VS-mode address > translation. When ADUE=0, the implementation behaves as though Svade > were implemented for VS-stage address translation. If Svadu is not > implemented, ADUE is read-only zero.The specification is unfortunately written a bit sideways, the base behavior happens to be "opt-in" through another extension. But that's specification, and it's possible that firmware follows a earlier revision of it that has unspecified A/D semantics instead. But in this case, the firmware is out of sync with current specifications and probably wants to be updated. I agree that, where Svadu is available, Xen should ideally configure a consistent scheme (ADUE=1) and use ADUE=0 only for specific needs such as dirty tracking. But Xen doesn't support Svadu today, so that is a separate piece of work which I'd prefer to do later.In the meantime, presetting A/D in the G-stage PTEs is the only way to avoid the faults on hardware like the P550, and it is harmless on hardware that updates A/D itself. Looks good to me. For v6 I'll keep the unconditional preset and reword the commit message to say this, what do you think?Yet, existence of Svade alone doesn't mandate the actual behavior, it is also dependant on the value of ADUE bit in configuration registers.Always preset A/D in G-stage PTEs so scheme (2) never faults. This is harmless with (1): software just does what hardware would have done on first access.Drop the TODO suggesting to handle A/D faults: only Svade raises them, soit would not work on Svadu. Dirty/access tracking can restrict the RWX permissions of p2m entries, as x86 and Arm do.Ideally (in cases we can) we would want to configure hardware to follow a consistent scheme (with the one that updates A/D with hardware i.e ADUE=1, as it's consistent to without Svade) and only consider the ADUE=0 behavior for specific needs like dirty tracking.> Fixes: ff14053983b0 ("xen/riscv: Implement p2m_pte_from_mfn() andsupport PBMT configuration")Assisted-by: Claude:claude-opus-5 Signed-off-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx> --- Changes since v4: - restore svade and update the commit message accordingly. - rebase on top of staging. --- Changes since v3:- always preset A/D bits in G-stage mappings instead of keying off the DT:it may not reflect what the hardware actually does. - change commit title/message. - drop svade as it is now-unused. --- Changes since v2: - change commit title.- preset A/D bits in p2m_set_pte_flags() if (!svadu || svade), instead of setting RISCV_ISA_EXT_svade in the riscv_isa bitmap for cases 1, 2 and 4.- riscv_resolve_ad_scheme() now only warns when both Svade and Svadu are present and SBI FWFT is missing; drop the dead 'svadu && svade' operand. - repeat XENLOG_WARNING on each line of the warning. - drop ASSERT(svade != svadu) from p2m_set_pte_flags(). - only add the sbi_probe_extension() declaration to sbi.h. - rework the comment in p2m_set_pte_flags(). --- Changes since v1: - change commit title - expose RISCV_ISA_EXT_svadu so the two extensions can be told apart. - move the Svade/Svadu resolution to a new riscv_resolve_ad_scheme(), called once from riscv_fill_hwcap(). - expose sbi_probe_extension() (was static) to probe for SBI FWFT.- stop presetting A/D bits unconditionally in p2m_set_permission(), do itonly when Svade is present. --- xen/arch/riscv/p2m.c | 38 ++++++++------------------------------ 1 file changed, 8 insertions(+), 30 deletions(-) diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c index f7b380b90a..14cae7dcd7 100644 --- a/xen/arch/riscv/p2m.c +++ b/xen/arch/riscv/p2m.c@@ -591,38 +591,16 @@ static void p2m_set_pte_flags(pte_t *e, p2m_type_t t)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- * necessary to set A and D bits. + * A RISC-V implementation can either: + * 1) Update the 'A' and 'D' PTE bits in hardware. + * 2) Generate a page fault when 'A' and/or 'D' is clear, so that + * software can set them.*- * TODO: For now, it’s fine to simply set the A/D bits, since OpenSBI - * delegates page faults to a lower privilege mode and so OpenSBI- * isn't expect to handle page-faults occured in lower modes.- * By setting the A/D bits here, page faults that would otherwise- * be generated due to unset A/D bits will not occur in Xen. - *- * Currently, Xen on RISC-V does not make use of the information - * that could be obtained from handling such page faults, which - * could otherwise be useful for several use cases such as demand - * paging, cache-flushing optimizations, memory access tracking,etc.- *- * To support the more general case and the optimizations mentioned - * above, it would be better to stop setting the A/D bits here and - * instead handle page faults that occur due to unset A/D bits. + * Xen supports both, so set 'A' and 'D' up front to avoid the faults + * of (2). This is harmless with (1): software just does what hardware+ * would have done on first access. */ - if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ) - e->pte |= PTE_ACCESSED | PTE_DIRTY; + e->pte |= PTE_ACCESSED | PTE_DIRTY;On the code change itself, it looks good to me, as we don't make use of A/D bit anyway (and in case we would need to, we would need to clear them anyway). Attachment:
OpenPGP_signature.asc
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |