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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Julien,

On Sun, 2024-07-21 at 09:51 +0100, Julien Grall wrote:
> Hi Oleksii,
> 
> On 12/07/2024 17:22, Oleksii Kurochko wrote:
> > Introduces arch_pmap_{un}map functions and select HAS_PMAP
> > for CONFIG_RISCV.
> > 
> > Additionaly it was necessary to introduce functions:
> >   - mfn_to_xen_entry
> >   - mfn_to_pte
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in V2:
> >   - newly introduced patch
> > ---
> >   xen/arch/riscv/Kconfig            |  1 +
> >   xen/arch/riscv/include/asm/page.h |  2 ++
> >   xen/arch/riscv/include/asm/pmap.h | 28
> > ++++++++++++++++++++++++++++
> >   xen/arch/riscv/mm.c               | 14 ++++++++++++++
> >   4 files changed, 45 insertions(+)
> >   create mode 100644 xen/arch/riscv/include/asm/pmap.h
> > 
> > diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
> > index 259eea8d3b..0112aa8778 100644
> > --- a/xen/arch/riscv/Kconfig
> > +++ b/xen/arch/riscv/Kconfig
> > @@ -3,6 +3,7 @@ config RISCV
> >     select FUNCTION_ALIGNMENT_16B
> >     select GENERIC_BUG_FRAME
> >     select HAS_DEVICE_TREE
> > +   select HAS_PMAP
> >   
> >   config RISCV_64
> >     def_bool y
> > diff --git a/xen/arch/riscv/include/asm/page.h
> > b/xen/arch/riscv/include/asm/page.h
> > index cbbf3656d1..339074d502 100644
> > --- a/xen/arch/riscv/include/asm/page.h
> > +++ b/xen/arch/riscv/include/asm/page.h
> > @@ -51,6 +51,8 @@ typedef struct {
> >   #endif
> >   } pte_t;
> >   
> > +pte_t mfn_to_xen_entry(mfn_t mfn, unsigned int attr);
> > +
> >   static inline pte_t paddr_to_pte(paddr_t paddr,
> >                                    unsigned int permissions)
> >   {
> > diff --git a/xen/arch/riscv/include/asm/pmap.h
> > b/xen/arch/riscv/include/asm/pmap.h
> > new file mode 100644
> > index 0000000000..eb4c48515c
> > --- /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);
> > +}
> > +
> > +#endif /* __ASM_PMAP_H__ */
> > diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> > index d69a174b5d..445319af08 100644
> > --- a/xen/arch/riscv/mm.c
> > +++ b/xen/arch/riscv/mm.c
> > @@ -370,3 +370,17 @@ int map_pages_to_xen(unsigned long virt,
> >       BUG_ON("unimplemented");
> >       return -1;
> >   }
> > +
> > +static inline pte_t mfn_to_pte(mfn_t mfn)
> > +{
> > +    unsigned long pte = mfn_x(mfn) << PTE_PPN_SHIFT;
> > +    return (pte_t){ .pte = pte};
> > +}
> > +
> > +inline pte_t mfn_to_xen_entry(mfn_t mfn, unsigned int attr)
> > +{
> > +    /* there is no attr field in RISC-V's pte */
> > +    (void) attr;
> 
> Surely you have a way to say indicate whether an entry is
> readable/writable?
Sure, there is a way. But probably I misinterpreted attr for Arm and
decided not to use them here. By that I mean, that Arm has MT_NORMAL,
MT_DEVICE which RISC-V doesn't have.

If it is about readable/writable then for sure, I will start to use
attr.

~ Oleksii

> 
> > +
> > +    return mfn_to_pte(mfn);
> > +}
> 
> Cheers,
>