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

[Penny Zheng <penny.zheng@xxxxxxx>]

Hi Ayan

On 2023/7/5 21:33, Ayan Kumar Halder wrote:
> 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 ?
See this pair map_domain_page()/unmap_domain_page(), which is used to 
map/unmap page of guest RAM. vcpu in different mode is facing different 
scenario.
Taking usage in copy_guest() as example:
When vcpu in guest mode trying to access its own memory(e.g. copy
hypercall param), there is no need to do the mapping/unmapping in MPU,
as page is already mapped at both EL1/EL2. Checking if it is transient
region in unmap_domain_page() is definitely necessary to help avoid 
unmapping it here.
When vcpu in hypervisor mode at boot time copying kernel image to guest
memory, we need to map page as transient MPU region to do the copying 
and pasting.
> > 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