Re: [PATCH v3 5/9] xen/riscv: introduce asm/pmap.h header

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.07.2024 18:22, oleksii.kurochko@xxxxxxxxx wrote:
> On Mon, 2024-07-29 at 16:29 +0200, Jan Beulich wrote:
>> On 24.07.2024 17:31, Oleksii Kurochko wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/riscv/include/asm/pmap.h
>>> @@ -0,0 +1,33 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef ASM_PMAP_H
>>> +#define ASM_PMAP_H
>>> +
>>> +#include <xen/bug.h>
>>> +#include <xen/mm.h>
>>> +#include <xen/page-size.h>
>>> +
>>> +#include <asm/fixmap.h>
>>> +#include <asm/flushtlb.h>
>>> +#include <asm/system.h>
>>> +
>>> +static inline void arch_pmap_map(unsigned int slot, mfn_t mfn)
>>> +{
>>> +    pte_t *entry = &xen_fixmap[slot];
>>> +    pte_t pte;
>>> +
>>> +    ASSERT(!pte_is_valid(*entry));
>>> +
>>> +    pte = mfn_to_xen_entry(mfn, PAGE_HYPERVISOR_RW);
>>> +    write_pte(entry, pte);
>>> +}
>>
>> Perhaps add a comment to clarify why it's safe to omit a TLB flush
>> here.
>> Note that arch_pmap_unmap() having one is a necessary but not
>> sufficient
>> condition to that. In principle hardware may also cache "negative"
>> TLB
>> entries; I have no idea how RISC-V behaves in this regard, or whether
>> that aspect is actually left to implementations.
> what do you mean by "negative" TLB? an old TLB entry which should be
> invalidated?

No, I mean TLB entries saying "no valid translation here". I.e. avoiding
recurring walks of something that was once found to have no translation.

>>> +static inline void arch_pmap_unmap(unsigned int slot)
>>> +{
>>> +    pte_t pte = {};
>>> +
>>> +    write_pte(&xen_fixmap[slot], pte);
>>> +
>>> +    flush_xen_tlb_range_va_local(FIXMAP_ADDR(slot), PAGE_SIZE);
>>> +}
>>
>> For both functions, even if mainly for documentation purposes, it may
>> be desirable to mark them __init. It's again a necessary (but not
>> sufficient) condition here, to validly use local TLB flushes only.
> Does __init in this context means that it will be called only by boot
> cpu at the start and that is it?

No, and I said so in my reply. __init is indicative of the function not
being suitable for runtime use, but it is not enough to indicate the
function is also unsuitable for use as soon as a 2nd CPU is being brought
up. Yet for the latter we have no proper annotation. Hence my suggestion
to use the former to have at least a limited indicator.

An alternative might be to add ASSERT(system_state < SYS_STATE_smp_boot).
That, however, exists in common/pmap.c already anyway.

Yet another alternative would be a clarifying comment.

Jan