| Message ID | 20210607024318.3988467-5-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Domain on Static Allocation | expand |
On 07.06.2021 04:43, Penny Zheng wrote: > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -611,6 +611,30 @@ static void __init init_pdx(void) > } > } > > +/* Static memory initialization */ > +static void __init init_staticmem_pages(void) > +{ > + int bank; While I'm not a maintainer of this code, I'd still like to point out that wherever possible we prefer "unsigned int" when dealing with only non-negative values, and even more so when using them as array indexes. > + /* > + * TODO: Considering NUMA-support scenario. > + */ Nit: Comment style. > @@ -872,6 +896,9 @@ void __init start_xen(unsigned long boot_phys_offset, > cmdline_parse(cmdline); > > setup_mm(); > + /* If exists, Static Memory Initialization. */ > + if ( bootinfo.static_mem.nr_banks > 0 ) > + init_staticmem_pages(); I don't think the conditional is really needed here? > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void) > return node_to_scrub(false) != NUMA_NO_NODE; > } > > +static void free_page(struct page_info *pg, bool need_scrub) > +{ > + mfn_t mfn = page_to_mfn(pg); With pdx compression this is a non-trivial conversion. The function being an internal helper and the caller already holding the MFN, I think it would be preferable if the MFN was passed in here. If done this way, you may want to consider adding an ASSERT() to double check both passed in arguments match up. > + /* If a page has no owner it will need no safety TLB flush. */ > + pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL); > + if ( pg->u.free.need_tlbflush ) > + page_set_tlbflush_timestamp(pg); > + > + /* This page is not a guest frame any more. */ > + page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */ > + set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); > + > +#ifdef CONFIG_ARM If avoidable there should be no arch-specific code added to this file. Assuming another arch gained PGC_reserved, what's wrong with enabling this code right away for them as well? I.e. use PGC_reserved here instead of CONFIG_ARM? Alternatively this may want to be CONFIG_STATIC_ALLOCATION, assuming we consider PGC_reserved tied to it. > + if ( pg->count_info & PGC_reserved ) > + { > + /* TODO: asynchronous scrubbing. */ > + if ( need_scrub ) > + scrub_one_page(pg); > + return; > + } > +#endif > + if ( need_scrub ) Nit: Please have a blank line between these last two. > + { > + pg->count_info |= PGC_need_scrub; > + poison_one_page(pg); > + } > + > + return; Please omit return statements at the end of functions returning void. > +} On the whole, bike shedding or not, I'm afraid the function's name doesn't match what it does: There's no freeing of a page here. What gets done is marking of a page as free. Hence maybe mark_page_free() or mark_free_page() or some such? > @@ -1512,6 +1530,38 @@ static void free_heap_pages( > spin_unlock(&heap_lock); > } > > +#ifdef CONFIG_STATIC_ALLOCATION > +/* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */ > +void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, > + bool need_scrub) > +{ > + mfn_t mfn = page_to_mfn(pg); > + unsigned long i; > + > + for ( i = 0; i < nr_mfns; i++ ) > + { > + switch ( pg[i].count_info & PGC_state ) > + { > + case PGC_state_inuse: > + BUG_ON(pg[i].count_info & PGC_broken); > + /* Mark it free and reserved. */ > + pg[i].count_info = PGC_state_free | PGC_reserved; > + break; > + > + default: > + printk(XENLOG_ERR > + "Page state shall be only in PGC_state_inuse. " Why? A page (static or not) can become broken while in use. IOW I don't think you can avoid handling PGC_state_offlining here. At which point this code will match free_heap_pages()'es, and hence likely will want folding as well. > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -85,6 +85,12 @@ bool scrub_free_pages(void); > } while ( false ) > #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) > > +#ifdef CONFIG_ARM ITYM CONFIG_STATIC_ALLOCATION here? Jan
Hi, On 10/06/2021 10:35, Jan Beulich wrote: > On 07.06.2021 04:43, Penny Zheng wrote: >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -611,6 +611,30 @@ static void __init init_pdx(void) >> } >> } >> >> +/* Static memory initialization */ >> +static void __init init_staticmem_pages(void) >> +{ >> + int bank; > > While I'm not a maintainer of this code, I'd still like to point out > that wherever possible we prefer "unsigned int" when dealing with > only non-negative values, and even more so when using them as array > indexes. +1. > >> + /* >> + * TODO: Considering NUMA-support scenario. >> + */ > > Nit: Comment style. > >> @@ -872,6 +896,9 @@ void __init start_xen(unsigned long boot_phys_offset, >> cmdline_parse(cmdline); >> >> setup_mm(); >> + /* If exists, Static Memory Initialization. */ >> + if ( bootinfo.static_mem.nr_banks > 0 ) >> + init_staticmem_pages(); > > I don't think the conditional is really needed here? > >> --- a/xen/common/page_alloc.c >> +++ b/xen/common/page_alloc.c >> @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void) >> return node_to_scrub(false) != NUMA_NO_NODE; >> } >> >> +static void free_page(struct page_info *pg, bool need_scrub) >> +{ >> + mfn_t mfn = page_to_mfn(pg); > > With pdx compression this is a non-trivial conversion. The function > being an internal helper and the caller already holding the MFN, I > think it would be preferable if the MFN was passed in here. If done > this way, you may want to consider adding an ASSERT() to double > check both passed in arguments match up. > >> + /* If a page has no owner it will need no safety TLB flush. */ >> + pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL); >> + if ( pg->u.free.need_tlbflush ) >> + page_set_tlbflush_timestamp(pg); >> + >> + /* This page is not a guest frame any more. */ >> + page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */ >> + set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); >> + >> +#ifdef CONFIG_ARM > > If avoidable there should be no arch-specific code added to this > file. Assuming another arch gained PGC_reserved, what's wrong with > enabling this code right away for them as well? I.e. use > PGC_reserved here instead of CONFIG_ARM? Alternatively this may > want to be CONFIG_STATIC_ALLOCATION, assuming we consider > PGC_reserved tied to it. > >> + if ( pg->count_info & PGC_reserved ) >> + { >> + /* TODO: asynchronous scrubbing. */ >> + if ( need_scrub ) >> + scrub_one_page(pg); >> + return; >> + } >> +#endif >> + if ( need_scrub ) > > Nit: Please have a blank line between these last two. > >> + { >> + pg->count_info |= PGC_need_scrub; >> + poison_one_page(pg); >> + } >> + >> + return; > > Please omit return statements at the end of functions returning void. > >> +} > > On the whole, bike shedding or not, I'm afraid the function's name > doesn't match what it does: There's no freeing of a page here. What > gets done is marking of a page as free. Hence maybe mark_page_free() > or mark_free_page() or some such? > >> @@ -1512,6 +1530,38 @@ static void free_heap_pages( >> spin_unlock(&heap_lock); >> } >> >> +#ifdef CONFIG_STATIC_ALLOCATION >> +/* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */ >> +void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, >> + bool need_scrub) >> +{ >> + mfn_t mfn = page_to_mfn(pg); >> + unsigned long i; >> + >> + for ( i = 0; i < nr_mfns; i++ ) >> + { >> + switch ( pg[i].count_info & PGC_state ) >> + { >> + case PGC_state_inuse: >> + BUG_ON(pg[i].count_info & PGC_broken); >> + /* Mark it free and reserved. */ >> + pg[i].count_info = PGC_state_free | PGC_reserved; >> + break; >> + >> + default: >> + printk(XENLOG_ERR >> + "Page state shall be only in PGC_state_inuse. " > > Why? A page (static or not) can become broken while in use. IOW I > don't think you can avoid handling PGC_state_offlining here. At which > point this code will match free_heap_pages()'es, and hence likely > will want folding as well. > >> --- a/xen/include/xen/mm.h >> +++ b/xen/include/xen/mm.h >> @@ -85,6 +85,12 @@ bool scrub_free_pages(void); >> } while ( false ) >> #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) >> >> +#ifdef CONFIG_ARM > > ITYM CONFIG_STATIC_ALLOCATION here? > > Jan >
Hi Penny, On 07/06/2021 03:43, Penny Zheng wrote: > This patch introduces static memory initialization, during system RAM boot up. The word "RAM" looks spurious. > New func init_staticmem_pages is responsible for static memory initialization. s/New func/The new function/ > Helper free_staticmem_pages is the equivalent of free_heap_pages, to free > nr_mfns pages of static memory. > > This commit defines a new helper free_page to extract common code between > free_heap_pages and free_staticmem_pages, like following the same cache/TLB > coherency policy. > > For each page, free_staticmem_pages includes the following extra steps to > initialize: > 1. change page state from inuse to free state and grant PGC_reserved. I think you mean "set" rather than "grant". > 2. scrub the page in need synchronously. Can you explain why this is necessary? > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > changes v2: > - rename to nr_mfns > - extract common code from free_heap_pages and free_staticmem_pages > - remove dead codes in other archs, including move some to arm-specific file, > and put some under CONFIG_ARM > - mark free_staticmem_pages __init > --- > xen/arch/arm/setup.c | 27 ++++++++++++++ I think it would be best to split the arm use in a separate patch. > xen/common/page_alloc.c | 78 +++++++++++++++++++++++++++++++++-------- > xen/include/xen/mm.h | 6 ++++ > 3 files changed, 97 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 00aad1c194..daafea0abb 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -611,6 +611,30 @@ static void __init init_pdx(void) > } > } > > +/* Static memory initialization */ > +static void __init init_staticmem_pages(void) > +{ > + int bank; > + > + /* > + * TODO: Considering NUMA-support scenario. > + */ > + for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ ) > + { > + paddr_t bank_start = bootinfo.static_mem.bank[bank].start; > + paddr_t bank_size = bootinfo.static_mem.bank[bank].size; > + paddr_t bank_end = bank_start + bank_size; > + > + bank_start = round_pgup(bank_start); > + bank_end = round_pgdown(bank_end); > + if ( bank_end <= bank_start ) > + return; > + > + free_staticmem_pages(maddr_to_page(bank_start), > + (bank_end - bank_start) >> PAGE_SHIFT, false); > + } > +} > + > #ifdef CONFIG_ARM_32 > static void __init setup_mm(void) > { > @@ -872,6 +896,9 @@ void __init start_xen(unsigned long boot_phys_offset, > cmdline_parse(cmdline); > > setup_mm(); > + /* If exists, Static Memory Initialization. */ > + if ( bootinfo.static_mem.nr_banks > 0 ) This check seems a pointless because init_staticmem_pages() is already able to cope with nr_banks == 0. > + init_staticmem_pages(); I would prefer if this is folded in setup_mm(). > > /* Parse the ACPI tables for possible boot-time configuration */ > acpi_boot_table_init(); > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 958ba0cd92..8c00262c04 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void) > return node_to_scrub(false) != NUMA_NO_NODE; > } > > +static void free_page(struct page_info *pg, bool need_scrub) > +{ > + mfn_t mfn = page_to_mfn(pg); > + > + /* If a page has no owner it will need no safety TLB flush. */ > + pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL); > + if ( pg->u.free.need_tlbflush ) > + page_set_tlbflush_timestamp(pg); > + > + /* This page is not a guest frame any more. */ > + page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */ > + set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); > + > +#ifdef CONFIG_ARM To echo what Jan already wrote, I am not in favor of adding new #ifdef CONFIG_<arch> in common code. I would expect the logic for static memory to be the same for each arch, so this should be protected with a generic Kconfig. > + if ( pg->count_info & PGC_reserved ) > + { > + /* TODO: asynchronous scrubbing. */ > + if ( need_scrub ) > + scrub_one_page(pg); > + return; > + } > +#endif > + if ( need_scrub ) > + { > + pg->count_info |= PGC_need_scrub; > + poison_one_page(pg); > + } > + > + return; > +} > + > /* Free 2^@order set of pages. */ > static void free_heap_pages( > struct page_info *pg, unsigned int order, bool need_scrub) > @@ -1425,20 +1456,7 @@ static void free_heap_pages( > BUG(); > } > > - /* If a page has no owner it will need no safety TLB flush. */ > - pg[i].u.free.need_tlbflush = (page_get_owner(&pg[i]) != NULL); > - if ( pg[i].u.free.need_tlbflush ) > - page_set_tlbflush_timestamp(&pg[i]); > - > - /* This page is not a guest frame any more. */ > - page_set_owner(&pg[i], NULL); /* set_gpfn_from_mfn snoops pg owner */ > - set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY); > - > - if ( need_scrub ) > - { > - pg[i].count_info |= PGC_need_scrub; > - poison_one_page(&pg[i]); > - } > + free_page(&pg[i], need_scrub); > } > > avail[node][zone] += 1 << order; > @@ -1512,6 +1530,38 @@ static void free_heap_pages( > spin_unlock(&heap_lock); > } > > +#ifdef CONFIG_STATIC_ALLOCATION > +/* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */ > +void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, > + bool need_scrub) > +{ > + mfn_t mfn = page_to_mfn(pg); > + unsigned long i; > + > + for ( i = 0; i < nr_mfns; i++ ) > + { > + switch ( pg[i].count_info & PGC_state ) > + { > + case PGC_state_inuse: > + BUG_ON(pg[i].count_info & PGC_broken); > + /* Mark it free and reserved. */ > + pg[i].count_info = PGC_state_free | PGC_reserved; > + break; > + > + default: > + printk(XENLOG_ERR > + "Page state shall be only in PGC_state_inuse. " > + "pg[%lu] MFN %"PRI_mfn" count_info=%#lx tlbflush_timestamp=%#x.\n", > + i, mfn_x(mfn) + i, > + pg[i].count_info, > + pg[i].tlbflush_timestamp); > + BUG(); > + } > + > + free_page(&pg[i], need_scrub); > + } > +} > +#endif > > /* > * Following rules applied for page offline: > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index 667f9dac83..df25e55966 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -85,6 +85,12 @@ bool scrub_free_pages(void); > } while ( false ) > #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) > > +#ifdef CONFIG_ARM > +/* Static Allocation */ > +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, > + bool need_scrub); > +#endif > + > /* Map machine page range in Xen virtual address space. */ > int map_pages_to_xen( > unsigned long virt, >
Hi Julien and Jan > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Thursday, July 1, 2021 1:46 AM > To: Jan Beulich <jbeulich@suse.com>; Penny Zheng <Penny.Zheng@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Subject: Re: [PATCH 4/9] xen/arm: static memory initialization > > Hi, > > On 10/06/2021 10:35, Jan Beulich wrote: > > On 07.06.2021 04:43, Penny Zheng wrote: > >> --- a/xen/arch/arm/setup.c > >> +++ b/xen/arch/arm/setup.c > >> @@ -611,6 +611,30 @@ static void __init init_pdx(void) > >> } > >> } > >> > >> +/* Static memory initialization */ > >> +static void __init init_staticmem_pages(void) { > >> + int bank; > > > > While I'm not a maintainer of this code, I'd still like to point out > > that wherever possible we prefer "unsigned int" when dealing with only > > non-negative values, and even more so when using them as array > > indexes. > > +1. > Understood. Thx. > > > >> + /* > >> + * TODO: Considering NUMA-support scenario. > >> + */ > > > > Nit: Comment style. > > Sure, thx. > >> @@ -872,6 +896,9 @@ void __init start_xen(unsigned long > boot_phys_offset, > >> cmdline_parse(cmdline); > >> > >> setup_mm(); > >> + /* If exists, Static Memory Initialization. */ > >> + if ( bootinfo.static_mem.nr_banks > 0 ) > >> + init_staticmem_pages(); > > > > I don't think the conditional is really needed here? > > Sure, right. Like what Julien suggests, init_staticmem_pages() is already able to cope with nr_banks == 0. > >> --- a/xen/common/page_alloc.c > >> +++ b/xen/common/page_alloc.c > >> @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void) > >> return node_to_scrub(false) != NUMA_NO_NODE; > >> } > >> > >> +static void free_page(struct page_info *pg, bool need_scrub) { > >> + mfn_t mfn = page_to_mfn(pg); > > > > With pdx compression this is a non-trivial conversion. The function > > being an internal helper and the caller already holding the MFN, I > > think it would be preferable if the MFN was passed in here. If done > > this way, you may want to consider adding an ASSERT() to double check > > both passed in arguments match up. > > Thank for the suggestion~ > >> + /* If a page has no owner it will need no safety TLB flush. */ > >> + pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL); > >> + if ( pg->u.free.need_tlbflush ) > >> + page_set_tlbflush_timestamp(pg); > >> + > >> + /* This page is not a guest frame any more. */ > >> + page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner > */ > >> + set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); > >> + > >> +#ifdef CONFIG_ARM > > > > If avoidable there should be no arch-specific code added to this file. > > Assuming another arch gained PGC_reserved, what's wrong with enabling > > this code right away for them as well? I.e. use PGC_reserved here > > instead of CONFIG_ARM? Alternatively this may want to be > > CONFIG_STATIC_ALLOCATION, assuming we consider PGC_reserved tied to > > it. > > To not bring dead codes in other archs, I'll use more generic option CONFIG_STATIC_ALLOCATION. > >> + if ( pg->count_info & PGC_reserved ) > >> + { > >> + /* TODO: asynchronous scrubbing. */ > >> + if ( need_scrub ) > >> + scrub_one_page(pg); > >> + return; > >> + } > >> +#endif > >> + if ( need_scrub ) > > > > Nit: Please have a blank line between these last two. > > Sure. Will do. > >> + { > >> + pg->count_info |= PGC_need_scrub; > >> + poison_one_page(pg); > >> + } > >> + > >> + return; > > > > Please omit return statements at the end of functions returning void. > > Sure, thx > >> +} > > > > On the whole, bike shedding or not, I'm afraid the function's name > > doesn't match what it does: There's no freeing of a page here. What > > gets done is marking of a page as free. Hence maybe mark_page_free() > > or mark_free_page() or some such? > > Ok. Thx. Always not good at giving names. I'll take mark_page_free() > >> @@ -1512,6 +1530,38 @@ static void free_heap_pages( > >> spin_unlock(&heap_lock); > >> } > >> > >> +#ifdef CONFIG_STATIC_ALLOCATION > >> +/* Equivalent of free_heap_pages to free nr_mfns pages of static > >> +memory. */ void __init free_staticmem_pages(struct page_info *pg, > unsigned long nr_mfns, > >> + bool need_scrub) { > >> + mfn_t mfn = page_to_mfn(pg); > >> + unsigned long i; > >> + > >> + for ( i = 0; i < nr_mfns; i++ ) > >> + { > >> + switch ( pg[i].count_info & PGC_state ) > >> + { > >> + case PGC_state_inuse: > >> + BUG_ON(pg[i].count_info & PGC_broken); > >> + /* Mark it free and reserved. */ > >> + pg[i].count_info = PGC_state_free | PGC_reserved; > >> + break; > >> + > >> + default: > >> + printk(XENLOG_ERR > >> + "Page state shall be only in PGC_state_inuse. " > > > > Why? A page (static or not) can become broken while in use. IOW I > > don't think you can avoid handling PGC_state_offlining here. At which > > point this code will match free_heap_pages()'es, and hence likely will > > want folding as well. > > Yeah, I was following the logic in free_heap_pages. Hmmm, I could not think of any scenario that will lead to PGC_state_offlining, that's why I was not including it at the first place. For broken issue, tbh, I just copy the bug_on from free_heap_pages, after quite a time thinking, I also could not find any scenario when a page(static or not) can become broken while in use. ;/ > >> --- a/xen/include/xen/mm.h > >> +++ b/xen/include/xen/mm.h > >> @@ -85,6 +85,12 @@ bool scrub_free_pages(void); > >> } while ( false ) > >> #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) > >> > >> +#ifdef CONFIG_ARM > > > > ITYM CONFIG_STATIC_ALLOCATION here? > > > > Jan > > > > -- > Julien Grall Cheers -- Penny Zheng
Hi Jan > -----Original Message----- > From: Penny Zheng > Sent: Monday, July 5, 2021 1:22 PM > To: Julien Grall <julien@xen.org>; Jan Beulich <jbeulich@suse.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Subject: RE: [PATCH 4/9] xen/arm: static memory initialization > > Hi Julien and Jan > > > -----Original Message----- > > From: Julien Grall <julien@xen.org> > > Sent: Thursday, July 1, 2021 1:46 AM > > To: Jan Beulich <jbeulich@suse.com>; Penny Zheng > <Penny.Zheng@arm.com> > > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > > <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > > sstabellini@kernel.org > > Subject: Re: [PATCH 4/9] xen/arm: static memory initialization > > > > Hi, > > > > On 10/06/2021 10:35, Jan Beulich wrote: > > > On 07.06.2021 04:43, Penny Zheng wrote: > > >> --- a/xen/arch/arm/setup.c > > >> +++ b/xen/arch/arm/setup.c > > >> @@ -611,6 +611,30 @@ static void __init init_pdx(void) > > >> } > > >> } > > >> > > >> +/* Static memory initialization */ static void __init > > >> +init_staticmem_pages(void) { > > >> + int bank; > > > > > > While I'm not a maintainer of this code, I'd still like to point out > > > that wherever possible we prefer "unsigned int" when dealing with > > > only non-negative values, and even more so when using them as array > > > indexes. > > > > +1. > > > > Understood. Thx. > > > > > > >> + /* > > >> + * TODO: Considering NUMA-support scenario. > > >> + */ > > > > > > Nit: Comment style. > > > > > Sure, thx. > > > >> @@ -872,6 +896,9 @@ void __init start_xen(unsigned long > > boot_phys_offset, > > >> cmdline_parse(cmdline); > > >> > > >> setup_mm(); > > >> + /* If exists, Static Memory Initialization. */ > > >> + if ( bootinfo.static_mem.nr_banks > 0 ) > > >> + init_staticmem_pages(); > > > > > > I don't think the conditional is really needed here? > > > > > Sure, right. > Like what Julien suggests, init_staticmem_pages() is already able to cope > with nr_banks == 0. > > > >> --- a/xen/common/page_alloc.c > > >> +++ b/xen/common/page_alloc.c > > >> @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void) > > >> return node_to_scrub(false) != NUMA_NO_NODE; > > >> } > > >> > > >> +static void free_page(struct page_info *pg, bool need_scrub) { > > >> + mfn_t mfn = page_to_mfn(pg); > > > > > > With pdx compression this is a non-trivial conversion. The function > > > being an internal helper and the caller already holding the MFN, I > > > think it would be preferable if the MFN was passed in here. If done > > > this way, you may want to consider adding an ASSERT() to double > > > check both passed in arguments match up. > > > > > Thank for the suggestion~ > While applying your suggestion here, if adding an ASSERT() to double check both passed-in arguments match up, either use like page_to_mfn to establish connection, which is absolutely unacceptable, or pass more parameters like base page/mfn to compare the offset. Hmmm, I am not in favor of this, since both extra parameters are only used in assertion only. > > >> + /* If a page has no owner it will need no safety TLB flush. */ > > >> + pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL); > > >> + if ( pg->u.free.need_tlbflush ) > > >> + page_set_tlbflush_timestamp(pg); > > >> + > > >> + /* This page is not a guest frame any more. */ > > >> + page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner > > */ > > >> + set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); > > >> + > > >> +#ifdef CONFIG_ARM > > > > > > If avoidable there should be no arch-specific code added to this file. > > > Assuming another arch gained PGC_reserved, what's wrong with > > > enabling this code right away for them as well? I.e. use > > > PGC_reserved here instead of CONFIG_ARM? Alternatively this may want > > > to be CONFIG_STATIC_ALLOCATION, assuming we consider PGC_reserved > > > tied to it. > > > > > To not bring dead codes in other archs, I'll use more generic option > CONFIG_STATIC_ALLOCATION. > > > >> + if ( pg->count_info & PGC_reserved ) > > >> + { > > >> + /* TODO: asynchronous scrubbing. */ > > >> + if ( need_scrub ) > > >> + scrub_one_page(pg); > > >> + return; > > >> + } > > >> +#endif > > >> + if ( need_scrub ) > > > > > > Nit: Please have a blank line between these last two. > > > > > Sure. Will do. > > > >> + { > > >> + pg->count_info |= PGC_need_scrub; > > >> + poison_one_page(pg); > > >> + } > > >> + > > >> + return; > > > > > > Please omit return statements at the end of functions returning void. > > > > > Sure, thx > > > >> +} > > > > > > On the whole, bike shedding or not, I'm afraid the function's name > > > doesn't match what it does: There's no freeing of a page here. What > > > gets done is marking of a page as free. Hence maybe mark_page_free() > > > or mark_free_page() or some such? > > > > > Ok. Thx. Always not good at giving names. I'll take mark_page_free() > > > >> @@ -1512,6 +1530,38 @@ static void free_heap_pages( > > >> spin_unlock(&heap_lock); > > >> } > > >> > > >> +#ifdef CONFIG_STATIC_ALLOCATION > > >> +/* Equivalent of free_heap_pages to free nr_mfns pages of static > > >> +memory. */ void __init free_staticmem_pages(struct page_info *pg, > > unsigned long nr_mfns, > > >> + bool need_scrub) { > > >> + mfn_t mfn = page_to_mfn(pg); > > >> + unsigned long i; > > >> + > > >> + for ( i = 0; i < nr_mfns; i++ ) > > >> + { > > >> + switch ( pg[i].count_info & PGC_state ) > > >> + { > > >> + case PGC_state_inuse: > > >> + BUG_ON(pg[i].count_info & PGC_broken); > > >> + /* Mark it free and reserved. */ > > >> + pg[i].count_info = PGC_state_free | PGC_reserved; > > >> + break; > > >> + > > >> + default: > > >> + printk(XENLOG_ERR > > >> + "Page state shall be only in PGC_state_inuse. " > > > > > > Why? A page (static or not) can become broken while in use. IOW I > > > don't think you can avoid handling PGC_state_offlining here. At > > > which point this code will match free_heap_pages()'es, and hence > > > likely will want folding as well. > > > > > Yeah, I was following the logic in free_heap_pages. > Hmmm, I could not think of any scenario that will lead to PGC_state_offlining, > that's why I was not including it at the first place. > For broken issue, tbh, I just copy the bug_on from free_heap_pages, after > quite a time thinking, I also could not find any scenario when a page(static or > not) can become broken while in use. ;/ > > > >> --- a/xen/include/xen/mm.h > > >> +++ b/xen/include/xen/mm.h > > >> @@ -85,6 +85,12 @@ bool scrub_free_pages(void); > > >> } while ( false ) > > >> #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) > > >> > > >> +#ifdef CONFIG_ARM > > > > > > ITYM CONFIG_STATIC_ALLOCATION here? > > > > > > Jan > > > > > > > -- > > Julien Grall > > Cheers > > -- > Penny Zheng
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Thursday, July 1, 2021 2:10 AM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; jbeulich@suse.com > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com> > Subject: Re: [PATCH 4/9] xen/arm: static memory initialization > > Hi Penny, > > On 07/06/2021 03:43, Penny Zheng wrote: > > This patch introduces static memory initialization, during system RAM boot > up. > > The word "RAM" looks spurious. > Thx. I check the "spurious" in dictionary, it means fake? So I will leave "during system boot up" here. > > New func init_staticmem_pages is responsible for static memory > initialization. > > s/New func/The new function/ > Sure. thx > > Helper free_staticmem_pages is the equivalent of free_heap_pages, to > > free nr_mfns pages of static memory. > > > > This commit defines a new helper free_page to extract common code > > between free_heap_pages and free_staticmem_pages, like following the > > same cache/TLB coherency policy. > > > > For each page, free_staticmem_pages includes the following extra steps > > to > > initialize: > > 1. change page state from inuse to free state and grant PGC_reserved. > > I think you mean "set" rather than "grant". > Yeah. I'll change to set here~ > > 2. scrub the page in need synchronously. > > Can you explain why this is necessary? > Since I'm borrowing the logic in free_heap_pages, I'm also trying to cover all the scenarios here like it does. So I assume that free_staticmem_page will not only be used on initialization, but also when destroying/rebooting the domain. On these cases, it is necessary to scrub the page, ig. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > changes v2: > > - rename to nr_mfns > > - extract common code from free_heap_pages and free_staticmem_pages > > - remove dead codes in other archs, including move some to > > arm-specific file, and put some under CONFIG_ARM > > - mark free_staticmem_pages __init > > --- > > xen/arch/arm/setup.c | 27 ++++++++++++++ > > I think it would be best to split the arm use in a separate patch. Sure, I'll move them to another commit. > > > xen/common/page_alloc.c | 78 +++++++++++++++++++++++++++++++++--- > ----- > > xen/include/xen/mm.h | 6 ++++ > > 3 files changed, 97 insertions(+), 14 deletions(-) > > > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index > > 00aad1c194..daafea0abb 100644 > > --- a/xen/arch/arm/setup.c > > +++ b/xen/arch/arm/setup.c > > @@ -611,6 +611,30 @@ static void __init init_pdx(void) > > } > > } > > > > +/* Static memory initialization */ > > +static void __init init_staticmem_pages(void) { > > + int bank; > > + > > + /* > > + * TODO: Considering NUMA-support scenario. > > + */ > > + for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ ) > > + { > > + paddr_t bank_start = bootinfo.static_mem.bank[bank].start; > > + paddr_t bank_size = bootinfo.static_mem.bank[bank].size; > > + paddr_t bank_end = bank_start + bank_size; > > + > > + bank_start = round_pgup(bank_start); > > + bank_end = round_pgdown(bank_end); > > + if ( bank_end <= bank_start ) > > + return; > > + > > + free_staticmem_pages(maddr_to_page(bank_start), > > + (bank_end - bank_start) >> PAGE_SHIFT, false); > > + } > > +} > > + > > #ifdef CONFIG_ARM_32 > > static void __init setup_mm(void) > > { > > @@ -872,6 +896,9 @@ void __init start_xen(unsigned long > boot_phys_offset, > > cmdline_parse(cmdline); > > > > setup_mm(); > > + /* If exists, Static Memory Initialization. */ > > + if ( bootinfo.static_mem.nr_banks > 0 ) > > This check seems a pointless because init_staticmem_pages() is already able > to cope with nr_banks == 0. > > > + init_staticmem_pages(); > I would prefer if this is folded in setup_mm(). > Sure. > > > > /* Parse the ACPI tables for possible boot-time configuration */ > > acpi_boot_table_init(); > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index > > 958ba0cd92..8c00262c04 100644 > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void) > > return node_to_scrub(false) != NUMA_NO_NODE; > > } > > > > +static void free_page(struct page_info *pg, bool need_scrub) { > > + mfn_t mfn = page_to_mfn(pg); > > + > > + /* If a page has no owner it will need no safety TLB flush. */ > > + pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL); > > + if ( pg->u.free.need_tlbflush ) > > + page_set_tlbflush_timestamp(pg); > > + > > + /* This page is not a guest frame any more. */ > > + page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */ > > + set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); > > + > > +#ifdef CONFIG_ARM > > To echo what Jan already wrote, I am not in favor of adding new #ifdef > CONFIG_<arch> in common code. I would expect the logic for static memory > to be the same for each arch, so this should be protected with a generic > Kconfig. > > > + if ( pg->count_info & PGC_reserved ) > > + { > > + /* TODO: asynchronous scrubbing. */ > > + if ( need_scrub ) > > + scrub_one_page(pg); > > + return; > > + } > > +#endif > > + if ( need_scrub ) > > + { > > + pg->count_info |= PGC_need_scrub; > > + poison_one_page(pg); > > + } > > + > > + return; > > +} > > + > > /* Free 2^@order set of pages. */ > > static void free_heap_pages( > > struct page_info *pg, unsigned int order, bool need_scrub) @@ > > -1425,20 +1456,7 @@ static void free_heap_pages( > > BUG(); > > } > > > > - /* If a page has no owner it will need no safety TLB flush. */ > > - pg[i].u.free.need_tlbflush = (page_get_owner(&pg[i]) != NULL); > > - if ( pg[i].u.free.need_tlbflush ) > > - page_set_tlbflush_timestamp(&pg[i]); > > - > > - /* This page is not a guest frame any more. */ > > - page_set_owner(&pg[i], NULL); /* set_gpfn_from_mfn snoops pg > owner */ > > - set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY); > > - > > - if ( need_scrub ) > > - { > > - pg[i].count_info |= PGC_need_scrub; > > - poison_one_page(&pg[i]); > > - } > > + free_page(&pg[i], need_scrub); > > } > > > > avail[node][zone] += 1 << order; @@ -1512,6 +1530,38 @@ static > > void free_heap_pages( > > spin_unlock(&heap_lock); > > } > > > > +#ifdef CONFIG_STATIC_ALLOCATION > > +/* Equivalent of free_heap_pages to free nr_mfns pages of static > > +memory. */ void __init free_staticmem_pages(struct page_info *pg, > unsigned long nr_mfns, > > + bool need_scrub) { > > + mfn_t mfn = page_to_mfn(pg); > > + unsigned long i; > > + > > + for ( i = 0; i < nr_mfns; i++ ) > > + { > > + switch ( pg[i].count_info & PGC_state ) > > + { > > + case PGC_state_inuse: > > + BUG_ON(pg[i].count_info & PGC_broken); > > + /* Mark it free and reserved. */ > > + pg[i].count_info = PGC_state_free | PGC_reserved; > > + break; > > + > > + default: > > + printk(XENLOG_ERR > > + "Page state shall be only in PGC_state_inuse. " > > + "pg[%lu] MFN %"PRI_mfn" count_info=%#lx > tlbflush_timestamp=%#x.\n", > > + i, mfn_x(mfn) + i, > > + pg[i].count_info, > > + pg[i].tlbflush_timestamp); > > + BUG(); > > + } > > + > > + free_page(&pg[i], need_scrub); > > + } > > +} > > +#endif > > > > /* > > * Following rules applied for page offline: > > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index > > 667f9dac83..df25e55966 100644 > > --- a/xen/include/xen/mm.h > > +++ b/xen/include/xen/mm.h > > @@ -85,6 +85,12 @@ bool scrub_free_pages(void); > > } while ( false ) > > #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) > > > > +#ifdef CONFIG_ARM > > +/* Static Allocation */ > > +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, > > + bool need_scrub); #endif > > + > > /* Map machine page range in Xen virtual address space. */ > > int map_pages_to_xen( > > unsigned long virt, > > > > -- > Julien Grall Cheers Penny Zheng
On 05.07.2021 07:22, Penny Zheng wrote: >> From: Julien Grall <julien@xen.org> >> Sent: Thursday, July 1, 2021 1:46 AM >> >> On 10/06/2021 10:35, Jan Beulich wrote: >>> On 07.06.2021 04:43, Penny Zheng wrote: >>>> @@ -1512,6 +1530,38 @@ static void free_heap_pages( >>>> spin_unlock(&heap_lock); >>>> } >>>> >>>> +#ifdef CONFIG_STATIC_ALLOCATION >>>> +/* Equivalent of free_heap_pages to free nr_mfns pages of static >>>> +memory. */ void __init free_staticmem_pages(struct page_info *pg, >> unsigned long nr_mfns, >>>> + bool need_scrub) { >>>> + mfn_t mfn = page_to_mfn(pg); >>>> + unsigned long i; >>>> + >>>> + for ( i = 0; i < nr_mfns; i++ ) >>>> + { >>>> + switch ( pg[i].count_info & PGC_state ) >>>> + { >>>> + case PGC_state_inuse: >>>> + BUG_ON(pg[i].count_info & PGC_broken); >>>> + /* Mark it free and reserved. */ >>>> + pg[i].count_info = PGC_state_free | PGC_reserved; >>>> + break; >>>> + >>>> + default: >>>> + printk(XENLOG_ERR >>>> + "Page state shall be only in PGC_state_inuse. " >>> >>> Why? A page (static or not) can become broken while in use. IOW I >>> don't think you can avoid handling PGC_state_offlining here. At which >>> point this code will match free_heap_pages()'es, and hence likely will >>> want folding as well. >>> > > Yeah, I was following the logic in free_heap_pages. > Hmmm, I could not think of any scenario that will lead to PGC_state_offlining, that's why > I was not including it at the first place. > For broken issue, tbh, I just copy the bug_on from free_heap_pages, after quite a time thinking, > I also could not find any scenario when a page(static or not) can become broken while in use. ;/ I can see that what you say may be true for Arm, but we're in generic code here with an arch-independent CONFIG_STATIC_ALLOCATION conditional around. Hence you want to avoid deliberately not handling a case that can occur on e.g. x86 (see mark_page_offline() and further related handling elsewhere). I'd perhaps view this differently if you were introducing completely new code, but you've specifically said you're cloning existing code (where the case is being handled). Plus, as said, you'll likely be able to actually share code by not excluding the case here. Jan
On 05.07.2021 09:14, Penny Zheng wrote: >> From: Penny Zheng >> Sent: Monday, July 5, 2021 1:22 PM >> >>> From: Julien Grall <julien@xen.org> >>> Sent: Thursday, July 1, 2021 1:46 AM >>> >>> On 10/06/2021 10:35, Jan Beulich wrote: >>>> On 07.06.2021 04:43, Penny Zheng wrote: >>>>> @@ -1512,6 +1530,38 @@ static void free_heap_pages( >>>>> spin_unlock(&heap_lock); >>>>> } >>>>> >>>>> +#ifdef CONFIG_STATIC_ALLOCATION >>>>> +/* Equivalent of free_heap_pages to free nr_mfns pages of static >>>>> +memory. */ void __init free_staticmem_pages(struct page_info *pg, >>> unsigned long nr_mfns, >>>>> + bool need_scrub) { >>>>> + mfn_t mfn = page_to_mfn(pg); >>>>> + unsigned long i; >>>>> + >>>>> + for ( i = 0; i < nr_mfns; i++ ) >>>>> + { >>>>> + switch ( pg[i].count_info & PGC_state ) >>>>> + { >>>>> + case PGC_state_inuse: >>>>> + BUG_ON(pg[i].count_info & PGC_broken); >>>>> + /* Mark it free and reserved. */ >>>>> + pg[i].count_info = PGC_state_free | PGC_reserved; >>>>> + break; >>>>> + >>>>> + default: >>>>> + printk(XENLOG_ERR >>>>> + "Page state shall be only in PGC_state_inuse. " >>>> >>>> Why? A page (static or not) can become broken while in use. IOW I >>>> don't think you can avoid handling PGC_state_offlining here. At >>>> which point this code will match free_heap_pages()'es, and hence >>>> likely will want folding as well. >>>> >> >> Yeah, I was following the logic in free_heap_pages. >> Hmmm, I could not think of any scenario that will lead to PGC_state_offlining, >> that's why I was not including it at the first place. >> For broken issue, tbh, I just copy the bug_on from free_heap_pages, after >> quite a time thinking, I also could not find any scenario when a page(static or >> not) can become broken while in use. ;/ I'm, afraid I don't understand. Using page_to_mfn(), expensive or not, in ASSERT() is quite fine. The (expensive) expression won't be evaluated in release builds. This is specifically different from BUG_ON(). Jan
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Monday, July 5, 2021 3:51 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; Julien Grall <julien@xen.org> > Subject: Re: [PATCH 4/9] xen/arm: static memory initialization > > On 05.07.2021 09:14, Penny Zheng wrote: > >> From: Penny Zheng > >> Sent: Monday, July 5, 2021 1:22 PM > >> > >>> From: Julien Grall <julien@xen.org> > >>> Sent: Thursday, July 1, 2021 1:46 AM > >>> > >>> On 10/06/2021 10:35, Jan Beulich wrote: > >>>> On 07.06.2021 04:43, Penny Zheng wrote: > >>>>> @@ -1512,6 +1530,38 @@ static void free_heap_pages( > >>>>> spin_unlock(&heap_lock); > >>>>> } > >>>>> > >>>>> +#ifdef CONFIG_STATIC_ALLOCATION > >>>>> +/* Equivalent of free_heap_pages to free nr_mfns pages of static > >>>>> +memory. */ void __init free_staticmem_pages(struct page_info *pg, > >>> unsigned long nr_mfns, > >>>>> + bool need_scrub) { > >>>>> + mfn_t mfn = page_to_mfn(pg); > >>>>> + unsigned long i; > >>>>> + > >>>>> + for ( i = 0; i < nr_mfns; i++ ) > >>>>> + { > >>>>> + switch ( pg[i].count_info & PGC_state ) > >>>>> + { > >>>>> + case PGC_state_inuse: > >>>>> + BUG_ON(pg[i].count_info & PGC_broken); > >>>>> + /* Mark it free and reserved. */ > >>>>> + pg[i].count_info = PGC_state_free | PGC_reserved; > >>>>> + break; > >>>>> + > >>>>> + default: > >>>>> + printk(XENLOG_ERR > >>>>> + "Page state shall be only in PGC_state_inuse. " > >>>> > >>>> Why? A page (static or not) can become broken while in use. IOW I > >>>> don't think you can avoid handling PGC_state_offlining here. At > >>>> which point this code will match free_heap_pages()'es, and hence > >>>> likely will want folding as well. > >>>> > >> > >> Yeah, I was following the logic in free_heap_pages. > >> Hmmm, I could not think of any scenario that will lead to > >> PGC_state_offlining, that's why I was not including it at the first place. > >> For broken issue, tbh, I just copy the bug_on from free_heap_pages, > >> after quite a time thinking, I also could not find any scenario when > >> a page(static or > >> not) can become broken while in use. ;/ > > I'm, afraid I don't understand. Using page_to_mfn(), expensive or not, in > ASSERT() is quite fine. The (expensive) expression won't be evaluated in > release builds. This is specifically different from BUG_ON(). > Thanks for the explanation.
On 05/07/2021 08:28, Penny Zheng wrote: > Hi Julien Hi Penny, >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Thursday, July 1, 2021 2:10 AM >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; >> sstabellini@kernel.org; jbeulich@suse.com >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen >> <Wei.Chen@arm.com> >> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization >> >> Hi Penny, >> >> On 07/06/2021 03:43, Penny Zheng wrote: >>> This patch introduces static memory initialization, during system RAM boot >> up. >> >> The word "RAM" looks spurious. >> > > Thx. I check the "spurious" in dictionary, it means fake? So I will leave "during system boot up" > here. Yes, this reads better. >>> 2. scrub the page in need synchronously. >> >> Can you explain why this is necessary? >> > > Since I'm borrowing the logic in free_heap_pages, I'm also trying to cover all the scenarios here like it does. > So I assume that free_staticmem_page will not only be used on initialization, but also when destroying/rebooting the domain. > On these cases, it is necessary to scrub the page, ig. I wasn't asking about scrubbing specifically but instead why it is synchronous. Sorry for the confusion. Cheers,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Tuesday, July 6, 2021 5:10 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org; jbeulich@suse.com > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com> > Subject: Re: [PATCH 4/9] xen/arm: static memory initialization > > > > On 05/07/2021 08:28, Penny Zheng wrote: > > Hi Julien > > Hi Penny, > > >> -----Original Message----- > >> From: Julien Grall <julien@xen.org> > >> Sent: Thursday, July 1, 2021 2:10 AM > >> To: Penny Zheng <Penny.Zheng@arm.com>; > >> xen-devel@lists.xenproject.org; sstabellini@kernel.org; > >> jbeulich@suse.com > >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > >> <Wei.Chen@arm.com> > >> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization > >> > >> Hi Penny, > >> > >> On 07/06/2021 03:43, Penny Zheng wrote: > >>> This patch introduces static memory initialization, during system > >>> RAM boot > >> up. > >> > >> The word "RAM" looks spurious. > >> > > > > Thx. I check the "spurious" in dictionary, it means fake? So I will leave > "during system boot up" > > here. > > Yes, this reads better. > > >>> 2. scrub the page in need synchronously. > >> > >> Can you explain why this is necessary? > >> > > > > Since I'm borrowing the logic in free_heap_pages, I'm also trying to cover all > the scenarios here like it does. > > So I assume that free_staticmem_page will not only be used on initialization, > but also when destroying/rebooting the domain. > > On these cases, it is necessary to scrub the page, ig. > > I wasn't asking about scrubbing specifically but instead why it is synchronous. > Sorry for the confusion. > I've read asynchronous scrubbing in buddy allocator, pages that need a scrub are added to tail, and specific working thread is working on it, hmm, imo, I don't think it could be simply applied into static memory. :/ So I put asynchronous scrubbing in #TODO. > Cheers, > > -- > Julien Grall Cheers -- Penny Zheng
On 06/07/2021 10:20, Penny Zheng wrote: > Hi Julien Hi Penny, >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Tuesday, July 6, 2021 5:10 PM >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; >> sstabellini@kernel.org; jbeulich@suse.com >> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen >> <Wei.Chen@arm.com> >> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization >> >> >> >> On 05/07/2021 08:28, Penny Zheng wrote: >>> Hi Julien >> >> Hi Penny, >> >>>> -----Original Message----- >>>> From: Julien Grall <julien@xen.org> >>>> Sent: Thursday, July 1, 2021 2:10 AM >>>> To: Penny Zheng <Penny.Zheng@arm.com>; >>>> xen-devel@lists.xenproject.org; sstabellini@kernel.org; >>>> jbeulich@suse.com >>>> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen >>>> <Wei.Chen@arm.com> >>>> Subject: Re: [PATCH 4/9] xen/arm: static memory initialization >>>> >>>> Hi Penny, >>>> >>>> On 07/06/2021 03:43, Penny Zheng wrote: >>>>> This patch introduces static memory initialization, during system >>>>> RAM boot >>>> up. >>>> >>>> The word "RAM" looks spurious. >>>> >>> >>> Thx. I check the "spurious" in dictionary, it means fake? So I will leave >> "during system boot up" >>> here. >> >> Yes, this reads better. >> >>>>> 2. scrub the page in need synchronously. >>>> >>>> Can you explain why this is necessary? >>>> >>> >>> Since I'm borrowing the logic in free_heap_pages, I'm also trying to cover all >> the scenarios here like it does. >>> So I assume that free_staticmem_page will not only be used on initialization, >> but also when destroying/rebooting the domain. >>> On these cases, it is necessary to scrub the page, ig. >> >> I wasn't asking about scrubbing specifically but instead why it is synchronous. >> Sorry for the confusion. >> > > I've read asynchronous scrubbing in buddy allocator, pages that need a scrub are added to tail, > and specific working thread is working on it, hmm, imo, I don't think it could be simply applied into static > memory. :/ I am not sure to understand why the same concept can't be applied. Anyway, what I am asking is to clarify in the commit message why this is synchronous (e.g. this just temporary). > > So I put asynchronous scrubbing in #TODO. We seem to have quite a few TODOs (some of them are implicit). Can we start to gather a list of what's missing? A first good place would be the cover lette.r Cheers,
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 00aad1c194..daafea0abb 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -611,6 +611,30 @@ static void __init init_pdx(void) } } +/* Static memory initialization */ +static void __init init_staticmem_pages(void) +{ + int bank; + + /* + * TODO: Considering NUMA-support scenario. + */ + for ( bank = 0 ; bank < bootinfo.static_mem.nr_banks; bank++ ) + { + paddr_t bank_start = bootinfo.static_mem.bank[bank].start; + paddr_t bank_size = bootinfo.static_mem.bank[bank].size; + paddr_t bank_end = bank_start + bank_size; + + bank_start = round_pgup(bank_start); + bank_end = round_pgdown(bank_end); + if ( bank_end <= bank_start ) + return; + + free_staticmem_pages(maddr_to_page(bank_start), + (bank_end - bank_start) >> PAGE_SHIFT, false); + } +} + #ifdef CONFIG_ARM_32 static void __init setup_mm(void) { @@ -872,6 +896,9 @@ void __init start_xen(unsigned long boot_phys_offset, cmdline_parse(cmdline); setup_mm(); + /* If exists, Static Memory Initialization. */ + if ( bootinfo.static_mem.nr_banks > 0 ) + init_staticmem_pages(); /* Parse the ACPI tables for possible boot-time configuration */ acpi_boot_table_init(); diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 958ba0cd92..8c00262c04 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -1376,6 +1376,37 @@ bool scrub_free_pages(void) return node_to_scrub(false) != NUMA_NO_NODE; } +static void free_page(struct page_info *pg, bool need_scrub) +{ + mfn_t mfn = page_to_mfn(pg); + + /* If a page has no owner it will need no safety TLB flush. */ + pg->u.free.need_tlbflush = (page_get_owner(pg) != NULL); + if ( pg->u.free.need_tlbflush ) + page_set_tlbflush_timestamp(pg); + + /* This page is not a guest frame any more. */ + page_set_owner(pg, NULL); /* set_gpfn_from_mfn snoops pg owner */ + set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY); + +#ifdef CONFIG_ARM + if ( pg->count_info & PGC_reserved ) + { + /* TODO: asynchronous scrubbing. */ + if ( need_scrub ) + scrub_one_page(pg); + return; + } +#endif + if ( need_scrub ) + { + pg->count_info |= PGC_need_scrub; + poison_one_page(pg); + } + + return; +} + /* Free 2^@order set of pages. */ static void free_heap_pages( struct page_info *pg, unsigned int order, bool need_scrub) @@ -1425,20 +1456,7 @@ static void free_heap_pages( BUG(); } - /* If a page has no owner it will need no safety TLB flush. */ - pg[i].u.free.need_tlbflush = (page_get_owner(&pg[i]) != NULL); - if ( pg[i].u.free.need_tlbflush ) - page_set_tlbflush_timestamp(&pg[i]); - - /* This page is not a guest frame any more. */ - page_set_owner(&pg[i], NULL); /* set_gpfn_from_mfn snoops pg owner */ - set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY); - - if ( need_scrub ) - { - pg[i].count_info |= PGC_need_scrub; - poison_one_page(&pg[i]); - } + free_page(&pg[i], need_scrub); } avail[node][zone] += 1 << order; @@ -1512,6 +1530,38 @@ static void free_heap_pages( spin_unlock(&heap_lock); } +#ifdef CONFIG_STATIC_ALLOCATION +/* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */ +void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, + bool need_scrub) +{ + mfn_t mfn = page_to_mfn(pg); + unsigned long i; + + for ( i = 0; i < nr_mfns; i++ ) + { + switch ( pg[i].count_info & PGC_state ) + { + case PGC_state_inuse: + BUG_ON(pg[i].count_info & PGC_broken); + /* Mark it free and reserved. */ + pg[i].count_info = PGC_state_free | PGC_reserved; + break; + + default: + printk(XENLOG_ERR + "Page state shall be only in PGC_state_inuse. " + "pg[%lu] MFN %"PRI_mfn" count_info=%#lx tlbflush_timestamp=%#x.\n", + i, mfn_x(mfn) + i, + pg[i].count_info, + pg[i].tlbflush_timestamp); + BUG(); + } + + free_page(&pg[i], need_scrub); + } +} +#endif /* * Following rules applied for page offline: diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 667f9dac83..df25e55966 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -85,6 +85,12 @@ bool scrub_free_pages(void); } while ( false ) #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) +#ifdef CONFIG_ARM +/* Static Allocation */ +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, + bool need_scrub); +#endif + /* Map machine page range in Xen virtual address space. */ int map_pages_to_xen( unsigned long virt,
This patch introduces static memory initialization, during system RAM boot up. New func init_staticmem_pages is responsible for static memory initialization. Helper free_staticmem_pages is the equivalent of free_heap_pages, to free nr_mfns pages of static memory. This commit defines a new helper free_page to extract common code between free_heap_pages and free_staticmem_pages, like following the same cache/TLB coherency policy. For each page, free_staticmem_pages includes the following extra steps to initialize: 1. change page state from inuse to free state and grant PGC_reserved. 2. scrub the page in need synchronously. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- changes v2: - rename to nr_mfns - extract common code from free_heap_pages and free_staticmem_pages - remove dead codes in other archs, including move some to arm-specific file, and put some under CONFIG_ARM - mark free_staticmem_pages __init --- xen/arch/arm/setup.c | 27 ++++++++++++++ xen/common/page_alloc.c | 78 +++++++++++++++++++++++++++++++++-------- xen/include/xen/mm.h | 6 ++++ 3 files changed, 97 insertions(+), 14 deletions(-)