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?
> 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.
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.
Jan
