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

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-07-29 at 16:29 +0200, Jan Beulich wrote:
> 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.
what do you mean by "negative" TLB? an old TLB entry which should be
invalidated?

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

> 
> > --- 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.
Thanks. I'll double check the namings and update the prototype of this
function.


~ Oleksii