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

[Julien Grall <julien@xxxxxxx>]

Hi Oleksii,

On 23/07/2024 16:36, oleksii.kurochko@xxxxxxxxx wrote:
> 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 ).
I have to admit, I am a little because concerned with calling sfence.vma 
in write_pte() (this may only be because I am not very familiar with 
RISC-V).
We have cases where multiple entry will be written in a single 
map_pages_to_xen() call. So wouldn't this means that the local TLBs 
would be nuked for every write rather than once?

> > 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.
Which code are you looking at? set_fixmap() will propagate the TLB flush 
to all innershareable CPUs.
The PMAP interface will do a local TLB flush because the interface can 
only be used during early boot where there is a single CPU running.
> RISC-V Linux kernel for fixmap also uses: local_flush_tlb_page().
I don't know how Linux is using set_fixmap(). But what matters is how 
Xen is using set_fixmap(). We have a couple of places in Xen where the 
fixmap needs to be accessed by all the CPUs.
Given this is a common interface in Xen, I think it makes sense to 
follow the same approach to avoid any confusion.
Cheers,

--
Julien Grall