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

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-07-30 at 14:15 +0200, Jan Beulich wrote:
> On 30.07.2024 13:39, oleksii.kurochko@xxxxxxxxx wrote:
> > 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().
> 
> You _always_ modify an existing entry. That may be a not-present one,
> but
> that's still an entry. And that's part of why I think you still
> didn't
> understand what I said. A particular implementation, if permitted by
> the
> spec, may very well put in place a TLB entry when the result of a
> page
> walk was a non-present entry. So ...
> 
> > So when we will be in pmap_map() we are
> > sure that there is no TLB entry for the new pte.
> 
> ..., can you point me at the part of the spec saying that such
> "negative"
> TLB entries are not permitted?
I double-checked the spec and it seems to me you are right and it could
be an issue. Absense of "negative" TLB entrues will be guaranted only
in the case when Svvptc extension is present (
https://github.com/riscv/riscv-svvptc/blob/main/svvptc.adoc?plain=1#L85
):
   Depending on the microarchitecture, some possible ways to facilitate
   implementation of Svvptc include: not having any address-translation
   caches, not
   storing Invalid PTEs in the address-translation caches, automatically
   evicting
   Invalid PTEs using a bounded timer, or making address-translation
   caches
   coherent with store instructions that modify PTEs.

If the mentioned above extension isn't present. Also there is the
following words in the spec 
( 
https://github.com/riscv/riscv-isa-manual/blob/main/src/supervisor.adoc?plain=1#L1213
):
   For the special cases of increasing the permissions on a leaf PTE and
   changing an invalid
   PTE to a valid leaf, software may choose to execute the SFENCE.VMA
   lazily. After
   modifying the PTE but before executing SFENCE.VMA, either the new or
   old permissions
   will be used. In the latter case, a page-fault exception might occur,
   at which point software
   should execute SFENCE.VMA in accordance with the previous bullet point.

Probably in the future we will need also similar to work Linux kernel
does:
https://patchwork.kernel.org/project/linux-mips/cover/20240131155929.169961-1-alexghiti@xxxxxxxxxxxx/

So I'll add flush_xen_tlb_range_va_local(FIXMAP_ADDR(slot), PAGE_SIZE)
in arch_pmap_map.

~ Oleksii