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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.07.2024 17:31, Oleksii Kurochko wrote:
> Introduces arch_pmap_{un}map functions and select HAS_PMAP
> for CONFIG_RISCV.
> 
> Additionaly it was necessary to introduce functions:
>  - mfn_from_xen_entry
>  - mfn_to_pte
> 
> Also flush_xen_tlb_range_va_local() and flush_xen_tlb_one_local()
> are introduced and use in arch_pmap_unmap().

Just flush_xen_tlb_one_local() would suffice for the purposes here.
flush_xen_tlb_range_va_local() will need some kind of cutoff, to
avoid looping for an excessivly long time.

> --- /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.

> +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.

> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -382,3 +382,18 @@ int map_pages_to_xen(unsigned long virt,
>      BUG_ON("unimplemented");
>      return -1;
>  }
> +
> +static inline pte_t mfn_from_pte(mfn_t mfn)
> +{
> +    unsigned long pte = mfn_x(mfn) << PTE_PPN_SHIFT;
> +    return (pte_t){ .pte = pte };
> +}

My earlier naming related comment may not have been here, but it
was certainly meant to apply to any similar functions: A function
of this name should imo take a pte_t and produce an mfn_t. IOW I'd
expect this function to be pte_from_mfn(). However, I question it being
a good idea to do it this way. The resulting pte_t isn't valid yet, aiui,
as it still needs at least the access controls filled in. Such a partial
pte_t would better not be floating around at any time, imo. IOW the
function likely wants to take a 2nd parameter.

Jan