Re: [PATCH v3 35/52] xen/arm: map static memory on demand

[Ayan Kumar Halder <ayankuma@xxxxxxx>]

On 05/07/2023 11:16, Penny Zheng wrote:
> Hi Ayan
Hi Penny,
> On 2023/7/4 23:10, 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.
> > >
> > > In function init_staticmem_pages, we need the access to static memory
> > > for proper initialization.
> > > It is not a problem in MMU system, as Xen map the whole RAM in
> > > setup_mm(). However, with limited MPU memory regions, it is too luxury
> > > to map the whole RAM.
> > > As a result, we follow the rule of "map on demand", to map static 
> > > memory
> > > temporarily before its initialization, and unmap immediately after its
> > > initialization.
> > I could see that you are using _PAGE_TRANSIENT  to map memory 
> > temporarily. However, I don't see this being translated to any of the 
> > MPU hardware features (ie _PAGE_TRANSIENT does not seem to translate 
> > to any of the attributes in PRBAR, PRLAR, PRENR, etc). Thus, how is 
> > it different from mapping the memory in "non temporary" way ?
> It is only software feature.
> It is designed for implementing functions like ioremap_xxx(), or 
> map_staticmem_pages_to_xen() here, which are always occuring with its 
> reverse unmapping function nearby like iounmap(), or 
> unmap_staticmem_pages_to_xen(), to map a chunk of memory 
> *temporarily*, for a very short time.
I understand that it is a software only feature. But why does the 
software need to know if the memory is mapped temporarily or not ? What 
difference does it make ?
> I want to check this flag in the unmapping function, to ensure that we 
> are unmapping the proper MPU region.
I had a look at unmap_staticmem_pages_to_xen() --> xen_mpumap_update() 
--> control_mpu_region_from_index() and I don't see this flag used anywhere.
- Ayan

> Fixed MPU regions are like Xen text section, Xen data section, etc.
>
> > Please let me know what I am missing.
> >
> > - Ayan
> >
> > > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > > Signed-off-by: Wei Chen <wei.chen@xxxxxxx>
> > > ---
> > > v3:
> > > - new commit
> > > ---
> > >   xen/arch/arm/include/asm/mm.h |  2 ++
> > >   xen/arch/arm/mmu/mm.c         | 10 ++++++++++
> > >   xen/arch/arm/mpu/mm.c         | 10 ++++++++++
> > >   xen/arch/arm/setup.c          | 21 +++++++++++++++++++++
> > >   4 files changed, 43 insertions(+)
> > >
> > > diff --git a/xen/arch/arm/include/asm/mm.h 
> > > b/xen/arch/arm/include/asm/mm.h
> > > index 66d98b9a29..cffbf8a595 100644
> > > --- a/xen/arch/arm/include/asm/mm.h
> > > +++ b/xen/arch/arm/include/asm/mm.h
> > > @@ -224,6 +224,8 @@ extern void mm_init_secondary_cpu(void);
> > >   extern void setup_frametable_mappings(paddr_t ps, paddr_t pe);
> > >   /* map a physical range in virtual memory */
> > >   void __iomem *ioremap_attr(paddr_t start, size_t len, unsigned int 
> > > attributes);
> > > +extern int map_staticmem_pages_to_xen(paddr_t start, paddr_t end);
> > > +extern int unmap_staticmem_pages_to_xen(paddr_t start, paddr_t end);
> > >
> > >   static inline void __iomem *ioremap_nocache(paddr_t start, size_t 
> > > len)
> > >   {
> > > diff --git a/xen/arch/arm/mmu/mm.c b/xen/arch/arm/mmu/mm.c
> > > index 2f29cb53fe..4196a55c32 100644
> > > --- a/xen/arch/arm/mmu/mm.c
> > > +++ b/xen/arch/arm/mmu/mm.c
> > > @@ -1113,6 +1113,16 @@ int populate_pt_range(unsigned long virt, 
> > > unsigned long nr_mfns)
> > >       return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
> > >   }
> > >
> > > +int __init map_staticmem_pages_to_xen(paddr_t start, paddr_t end)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > > +int __init unmap_staticmem_pages_to_xen(paddr_t start, paddr_t end)
> > > +{
> > > +    return 0;
> > > +}
> > > +
> > >   /*
> > >    * Local variables:
> > >    * mode: C
> > > diff --git a/xen/arch/arm/mpu/mm.c b/xen/arch/arm/mpu/mm.c
> > > index a40055ae5e..9d5c1da39c 100644
> > > --- a/xen/arch/arm/mpu/mm.c
> > > +++ b/xen/arch/arm/mpu/mm.c
> > > @@ -614,6 +614,16 @@ void __init setup_frametable_mappings(paddr_t 
> > > ps, paddr_t pe)
> > >              frametable_size - (nr_pdxs * sizeof(struct page_info)));
> > >   }
> > >
> > > +int __init map_staticmem_pages_to_xen(paddr_t start, paddr_t end)
> > > +{
> > > +    return xen_mpumap_update(start, end, PAGE_HYPERVISOR | 
> > > _PAGE_TRANSIENT);
> > > +}
> > > +
> > > +int __init unmap_staticmem_pages_to_xen(paddr_t start, paddr_t end)
> > > +{
> > > +    return xen_mpumap_update(start, end, 0);
> > > +}
> > > +
> > >   /*
> > >    * Local variables:
> > >    * mode: C
> > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> > > index f42b53d17b..c21d1db763 100644
> > > --- a/xen/arch/arm/setup.c
> > > +++ b/xen/arch/arm/setup.c
> > > @@ -637,12 +637,33 @@ void __init init_staticmem_pages(void)
> > >               mfn_t bank_start = 
> > > _mfn(PFN_UP(bootinfo.reserved_mem.bank[bank].start));
> > >               unsigned long bank_pages = 
> > > PFN_DOWN(bootinfo.reserved_mem.bank[bank].size);
> > >               mfn_t bank_end = mfn_add(bank_start, bank_pages);
> > > +            int res;
> > >
> > >               if ( mfn_x(bank_end) <= mfn_x(bank_start) )
> > >                   return;
> > >
> > > +            /* Map temporarily before initialization */
> > > +            res = map_staticmem_pages_to_xen(mfn_to_maddr(bank_start),
> > > + mfn_to_maddr(bank_end));
> > > +            if ( res )
> > > +            {
> > > +                printk(XENLOG_ERR "Failed to map static memory to 
> > > Xen: %d\n",
> > > +                       res);
> > > +                return;
> > > +            }
> > > +
> > > unprepare_staticmem_pages(mfn_to_page(bank_start),
> > >                                         bank_pages, false);
> > > +
> > > +            /* Unmap immediately after initialization */
> > > +            res = 
> > > unmap_staticmem_pages_to_xen(mfn_to_maddr(bank_start),
> > > + mfn_to_maddr(bank_end));
> > > +            if ( res )
> > > +            {
> > > +                printk(XENLOG_ERR "Failed to unmap static memory to 
> > > Xen: %d\n",
> > > +                       res);
> > > +                return;
> > > +            }
> > >           }
> > >       }
> > >   #endif
> > > --
> > > 2.25.1