Re: [PATCH v2 5/8] xen/riscv: introduce asm/pmap.h header

[oleksii.kurochko@xxxxxxxxx]

On Tue, 2024-07-23 at 12:02 +0200, Jan Beulich wrote:
> On 23.07.2024 10:55, oleksii.kurochko@xxxxxxxxx wrote:
> > On Tue, 2024-07-23 at 10:36 +0200, Jan Beulich wrote:
> > > On 23.07.2024 10:02, Oleksii Kurochko wrote:
> > > > On Mon, Jul 22, 2024 at 7:27 PM Julien Grall <julien@xxxxxxx>
> > > > wrote:
> > > > > > > On 22/07/2024 15:44, Oleksii Kurochko wrote:
> > > > > >     /* Map a 4k page in a fixmap entry */
> > > > > >     void set_fixmap(unsigned map, mfn_t mfn, unsigned int
> > > > > > flags)
> > > > > >     {
> > > > > >         pte_t pte;
> > > > > > 
> > > > > >         pte = mfn_to_xen_entry(mfn, flags);
> > > > > >         pte.pte |= PTE_LEAF_DEFAULT;
> > > > > >         write_pte(&xen_fixmap[pt_index(0,
> > > > > > FIXMAP_ADDR(map))],
> > > > > > pte);
> > > > > 
> > > > > It would be saner to check if you are not overwriting any
> > > > > existing
> > > > > mapping as otherwise you will probably need a TLB flush.
> > > > > 
> > > > > >     }
> > > > > > 
> > > > > >     /* Remove a mapping from a fixmap entry */
> > > > > >     void clear_fixmap(unsigned map)
> > > > > >     {
> > > > > >         pte_t pte = {0};
> > > > > >         write_pte(&xen_fixmap[pt_index(0,
> > > > > > FIXMAP_ADDR(map))],
> > > > > > pte);
> > > > > 
> > > > > Don't you need a TLB flush?
> > > > > 
> > > > Inside write_pte() there is "sfence.vma".
> > > 
> > > That's just a fence though, not a TLB flush.
> > From the privileged doc:
> >    ```
> >    SFENCE.VMA is also used to invalidate entries in the
> >    address-translation cache associated with a hart (see Section
> > 4.3.2). 
> >    ...
> >    The SFENCE.VMA is used to flush any local hardware caches
> > related to
> >    address translation.
> >    It is specified as a fence rather than a TLB flush to provide
> > cleaner
> >    semantics with respect to
> >    which instructions are affected by the flush operation and to
> > support a
> >    wider variety of dynamic
> >    caching structures and memory-management schemes. SFENCE.VMA is
> > also
> >    used by higher
> >    privilege levels to synchronize page table writes and the
> > address
> >    translation hardware.
> >    ...
> >    ```
> > I read this as SFENCE.VMA is used not only for ordering of
> > load/stores,
> > but also to flush TLB ( which is a type of more general term as
> > address-translation cache, IIUIC ).
> 
> Oh, I see. Kind of unexpected for an instruction of that name. Yet
> note
> how they talk about the local hart only. You need a wider scope TLB
> flush here.
Could you please clarify why it is needed wider?

Arm Xen flushed only local TLB.
RISC-V Linux kernel for fixmap also uses: local_flush_tlb_page().

~ Oleksii