Re: [PATCH v3 33/52] xen/mpu: initialize frametable in MPU system

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi，

On 2023/7/3 17:31, Julien Grall wrote:
> Hi,
>
> On 03/07/2023 07:10, Penny Zheng wrote:
> > On 2023/6/30 23:19, Ayan Kumar Halder wrote:
> > > Hi Penny,
> > >
> > > On 26/06/2023 04:34, Penny Zheng wrote:
> > > > CAUTION: This message has originated from an External Source. Please 
> > > > use proper judgment and caution when opening attachments, clicking 
> > > > links, or responding to this email.
> > > >
> > > > Xen is using page as the smallest granularity for memory managment.
> > > > And we want to follow the same concept in MPU system.
> > > > That is, structure page_info and the frametable which is used for 
> > > > storing
> > > > and managing the smallest memory managment unit is also required in 
> > > > MPU system.
> > > > In MPU system, since we can not use a fixed VA 
> > > > address(FRAMETABLE_VIRT_START)
> > > > to map frametable like MMU system does and everything is 1:1 
> > > > mapping, we
> > > > instead define a variable "struct page_info *frame_table" as frametable
> > > > pointer, and ask boot allocator to allocate appropriate memory for 
> > > > frametable.
> > > > As frametable is successfully initialized, the convertion between 
> > > > machine frame
> > > > number/machine address/"virtual address" and page-info structure is
> > > > ready too, like mfn_to_page/maddr_to_page/virt_to_page, etc
> > > >
> > > > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > > ---
> > > > v3:
> > > > - add ASSERT() to confirm the MFN you pass is covered by the 
> > > > frametable.
> > > > ---
> > > >   xen/arch/arm/include/asm/mm.h     | 14 ++++++++++++++
> > > >   xen/arch/arm/include/asm/mpu/mm.h |  3 +++
> > > >   xen/arch/arm/mpu/mm.c             | 27 +++++++++++++++++++++++++++
> > > >   3 files changed, 44 insertions(+)
> > > >
> > > > diff --git a/xen/arch/arm/include/asm/mm.h 
> > > > b/xen/arch/arm/include/asm/mm.h
> > > > index daa6329505..66d98b9a29 100644
> > > > --- a/xen/arch/arm/include/asm/mm.h
> > > > +++ b/xen/arch/arm/include/asm/mm.h
> > > > @@ -341,6 +341,19 @@ static inline uint64_t gvirt_to_maddr(vaddr_t 
> > > > va, paddr_t *pa,
> > > >   #define virt_to_mfn(va)     __virt_to_mfn(va)
> > > >   #define mfn_to_virt(mfn)    __mfn_to_virt(mfn)
> > > >
> > > > +#ifdef CONFIG_HAS_MPU
> > > > +/* Convert between virtual address to page-info structure. */
> > > > +static inline struct page_info *virt_to_page(const void *v)
> > > > +{
> > > > +    unsigned long pdx;
> > > > +
> > > > +    pdx = paddr_to_pdx(virt_to_maddr(v));
> > > > +    ASSERT(pdx >= frametable_base_pdx);
> > > > +    ASSERT(pdx < frametable_pdx_end);
> > > > +
> > > > +    return frame_table + pdx - frametable_base_pdx;
> > > > +}
> > > This should go in xen/arch/arm/include/asm/mpu/mm.h. Then you don't 
> > > need ifdef
> > > > +#else
> > > >   /* Convert between Xen-heap virtual addresses and page-info 
> > > > structures. */
> > > >   static inline struct page_info *virt_to_page(const void *v)
> > > >   {
> > > > @@ -354,6 +367,7 @@ static inline struct page_info 
> > > > *virt_to_page(const void *v)
> > > >       pdx += mfn_to_pdx(directmap_mfn_start);
> > > >       return frame_table + pdx - frametable_base_pdx;
> > > >   }
> > > Consequently, this should be in xen/arch/arm/include/asm/mmu/mm.h
> > The reason why I don't put virt_to_page()/page_to_virt() in specific 
> > header is that we are using some helpers, which are defined just
> > a few lines before, like mfn_to_virt(), etc.
> > If you are moving mfn_to_virt() to specific header too, that will
> > result in a lot dulplication.
> >
> > > > +#endif
> > > >
> > > >   static inline void *page_to_virt(const struct page_info *pg)
> > > >   {
> > > > diff --git a/xen/arch/arm/include/asm/mpu/mm.h 
> > > > b/xen/arch/arm/include/asm/mpu/mm.h
> > > > index e26bd4f975..98f6df65b8 100644
> > > > --- a/xen/arch/arm/include/asm/mpu/mm.h
> > > > +++ b/xen/arch/arm/include/asm/mpu/mm.h
> > > > @@ -2,6 +2,9 @@
> > > >   #ifndef __ARCH_ARM_MM_MPU__
> > > >   #define __ARCH_ARM_MM_MPU__
> > > >
> > > > +extern struct page_info *frame_table;
> > > If you define frame_table in xen/arch/arm/include/asm/mm.h , then you 
> > > don't need the above declaration.
> > It is a variable only in MPU. In MMU, it is a fixed value.
> > "#define frame_table ((struct page_info *)FRAMETABLE_VIRT_START)"
> >
> > > > +extern unsigned long frametable_pdx_end;
> > > Also you don't need extern as this is only used by 
> > > xen/arch/arm/mpu/mm.c.
> > sure.
> >
> > > > +
> > > >   extern int xen_mpumap_update(paddr_t base, paddr_t limit, unsigned 
> > > > int flags);
> > > You don't need extern here as it should be used only in 
> > > xen/arch/arm/mpu/mm.c
> > > Currently, I see the following in xen/arch/arm/mm.c,
> > >
> > > int map_pages_to_xen(unsigned long virt,
> > >                       mfn_t mfn,
> > >                       unsigned long nr_mfns,
> > >                       unsigned int flags)
> > > {
> > > #ifndef CONFIG_HAS_MPU
> > >      return xen_pt_update(virt, mfn, nr_mfns, flags);
> > > #else
> > >      return xen_mpumap_update(mfn_to_maddr(mfn),
> > >                               mfn_to_maddr(mfn_add(mfn, nr_mfns)), 
> You are ignoring 'virt' here. Shouldn't you at least check that 'virt == 
> mfn_to_maddr(mfn)'?
Sure, I'll do the check.

> > > flags);
> > > #endif
> > > }
> > >
> > > int destroy_xen_mappings(unsigned long s, unsigned long e)
> > > {
> > >      ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> > >      ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> > >      ASSERT(s <= e);
> > > #ifndef CONFIG_HAS_MPU
> > >      return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, 0);
> > > #else
> > >      return xen_mpumap_update(virt_to_maddr((void *)s),
> > >                               virt_to_maddr((void *)e), 0);
> > > #endif
> > > }
> > >
> > > int modify_xen_mappings(unsigned long s, unsigned long e, unsigned 
> > > int flags)
> > > {
> > >      ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> > >      ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> > >      ASSERT(s <= e);
> > > #ifndef CONFIG_HAS_MPU
> > >      return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
> > > #else
> > >      return xen_mpumap_update(virt_to_maddr((void *)s),
> > >                               virt_to_maddr((void *)e), flags);
> > > #endif
> > > }
> > >
> > > It would be better to have 2 implementations for map_pages_to_xen(), 
> > > destroy_xen_mappings() and modify_xen_mappings() in 
> > > xen/arch/arm/mpu/mm.c and xen/arch/arm/mmu/mm.c.
> > I prefer them staying in the common file, using #ifdef to go to the
> > different path.
> I don't like the #ifdef solution in this situation. You are only doing 
> it for the benefits of the ASSERT(). But they don't seem to have any 
> value for xen_mpumap_update() (you are still passing an address rather 
> than a frame).
Okay， I will pass a page or a valid mfn next version.

> > Since if not and when anyone wants to update map_pages_to_xen(), 
> > destroy_xen_mappings() and modify_xen_mappings() in the future, it is 
> > possible for them to leave changes in only one file.
> The helpers are just wrappers. I doubt they will change in the future. 
> So I think it would be OK to duplicate.
> The alternative would to have a common prototype for xen_pt_update() and 
> xen_mpumap_update() and avoid any #ifdery. That said, this is not my 
> preference at least if they are not static inline.
Correct me if I'm wrong, you are suggesting something like this:
A more-generic wrapper like xen_mm_update, and we introduce static 
inline implementation in mmu/mm.h with xen_pt_update(), and static
inline implementation in mpu/mm.h with xen_mpumap_update().

> Cheers,