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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 22/07/2024 18:09, Oleksii Kurochko wrote:
> On Mon, 2024-07-22 at 15:48 +0100, Julien Grall wrote:
> > Hi,
> >
> > On 22/07/2024 15:44, Oleksii Kurochko wrote:
> > > On Mon, 2024-07-22 at 14:54 +0200, Jan Beulich wrote:
> > > > On 12.07.2024 18:22, Oleksii Kurochko wrote:
> > > > > --- /dev/null
> > > > > +++ b/xen/arch/riscv/include/asm/pmap.h
> > > > > @@ -0,0 +1,28 @@
> > > > > +#ifndef __ASM_PMAP_H__
> > > > > +#define __ASM_PMAP_H__
> > > > > +
> > > > > +#include <xen/bug.h>
> > > > > +#include <xen/mm.h>
> > > > > +
> > > > > +#include <asm/fixmap.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);
> > > > > +    pte.pte |= PTE_LEAF_DEFAULT;
> > > > > +    write_pte(entry, pte);
> > > > > +}
> > > > > +
> > > > > +static inline void arch_pmap_unmap(unsigned int slot)
> > > > > +{
> > > > > +    pte_t pte = {};
> > > > > +
> > > > > +    write_pte(&xen_fixmap[slot], pte);
> > > > > +}
> > > > Why are these not using set_fixmap() / clear_fixmap()
> > > > respectively?
> > > They haven't been introduced yet. And I thought that these fucntion
> > > are
> > > used only in pmap_{un}map() and that is the reason why I decided to
> > > not
> > > introduce them. But while writing the answer on another comment, I
> > > found other places where set_fixmap() / clear_fixmap() are used, so
> > > I
> > > will introduce them and reuse here.
> > I am guessing you are going to implement set_fixmap()/clear_fixmap()
> > using map_pages_to_xen(). If so, for early boot you are going to end
> > up
> > in a circular loop because map_pages_to_xen() will likely use pmap()
> > which will call set_fixmap().
> I am going to implement that in the following way as I faced the
> described by you issue when I first time tried to implement it using
> map_pages_to_xen():
What's wrong with keeping the arch_pmap_*() as-is and call 
map_pages_to_xen() in the fixmap?
At least this would be consistent with what other architectures does and 
less risky (see below).
>     /* 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?

Cheers,

--
Julien Grall