[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [PATCH 4/9] xen/arm: static memory initialization
Hi Julien and Jan > -----Original Message----- > From: Julien Grall <julien@xxxxxxx> > Sent: Thursday, July 1, 2021 1:46 AM > To: Jan Beulich <jbeulich@xxxxxxxx>; Penny Zheng <Penny.Zheng@xxxxxxx> > Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen > <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; > sstabellini@xxxxxxxxxx > 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |