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

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-07-30 at 09:56 +0200, Jan Beulich wrote:
> 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.
But we can't modify an existent entry because we have
"ASSERT(!pte_is_valid(*entry))" in pmap_map() function and we are doing
TLB flush during pmap_unmap(). So when we will be in pmap_map() we are
sure that there is no TLB entry for the new pte.

> 
> > > > +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.
Thanks for clarifying. I will stick to the first option with __init.

~ Oleksii