Hi Julien,
On Mon, Jul 22, 2024 at 7:27 PM Julien Grall <jul...@xen.org> 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).
>
Then I misunderstood you, if not to use {set/clear}_fixmap() in arch_pmap()
then everything should be fine.
Then I think it is needed to add the comment also above arch_pmap_*()
function why it isn't used {set/clear}_fixmap()
inside. ( or update the commit message )
>
> > /* 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".
But probably it would be better to add flush_xen_tlb_range_va_local() or
something similar here in case if someone will decide to update write_pte().
~ Oleksii
