Hi Julien Happy new year~~~~
> -----Original Message----- > From: Julien Grall <[email protected]> > Sent: Sunday, January 8, 2023 8:53 PM > To: Penny Zheng <[email protected]>; [email protected] > Cc: Wei Chen <[email protected]>; Stefano Stabellini > <[email protected]>; Bertrand Marquis <[email protected]>; > Volodymyr Babchuk <[email protected]> > Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner > when host address not provided > > Hi, > > On 15/11/2022 02:52, Penny Zheng wrote: > > @@ -922,33 +927,82 @@ static mfn_t __init > acquire_shared_memory_bank(struct domain *d, > > d->max_pages += nr_pfns; > > > > smfn = maddr_to_mfn(pbase); > > - res = acquire_domstatic_pages(d, smfn, nr_pfns, 0); > > - if ( res ) > > + page = mfn_to_page(smfn); > > + /* > > + * If page is allocated from heap as static shared memory, then we just > > + * assign it to the owner domain > > + */ > > + if ( page->count_info == (PGC_state_inuse | PGC_static) ) > I am a bit confused how this can help differentiating becaPGC_state_inuse is > 0. So effectively, you are checking that count_info is equal to PGC_static. > When host address is provided, the host address range defined in xen,static-mem will be stored as a "struct membank" with type of "MEMBANK_STATIC_DOMAIN" in "bootinfo.reserved_mem" Then it will be initialized as static memory through "init_staticmem_pages" So here its page->count_info is PGC_state_free |PGC_static. For pages allocated from heap, its page state is different, being PGC_state_inuse. So actually, we are checking page state to tell the difference . > But as I wrote in a previous patch, I don't think you should convert > {xen,dom}heap pages to a static pages. > I agree that taking reference could also prevent giving these pages back to heap. But may I ask what is your concern on converting {xen,dom}heap pages to a static pages? > [...] > > > +static int __init assign_shared_memory(struct domain *d, > > + struct shm_membank *shm_membank, > > + paddr_t gbase) { > > + int ret = 0; > > + unsigned long nr_pages, nr_borrowers; > > + struct page_info *page; > > + unsigned int i; > > + struct meminfo *meminfo; > > + > > + /* Host address is not provided in "xen,shared-mem" */ > > + if ( shm_membank->mem.banks.meminfo ) > > + { > > + meminfo = shm_membank->mem.banks.meminfo; > > + for ( i = 0; i < meminfo->nr_banks; i++ ) > > + { > > + ret = acquire_shared_memory(d, > > + meminfo->bank[i].start, > > + meminfo->bank[i].size, > > + gbase); > > + if ( ret ) > > + return ret; > > + > > + gbase += meminfo->bank[i].size; > > + } > > + } > > + else > > + { > > + ret = acquire_shared_memory(d, > > + shm_membank->mem.bank->start, > > + shm_membank->mem.bank->size, gbase); > > + if ( ret ) > > + return ret; > > + } > > Looking at this change and... > > > + > > /* > > * Get the right amount of references per page, which is the number of > > * borrower domains. > > @@ -984,23 +1076,37 @@ static int __init assign_shared_memory(struct > domain *d, > > * So if the borrower is created first, it will cause adding pages > > * in the P2M without reference. > > */ > > - page = mfn_to_page(smfn); > > - for ( i = 0; i < nr_pages; i++ ) > > + if ( shm_membank->mem.banks.meminfo ) > > { > > - if ( !get_page_nr(page + i, d, nr_borrowers) ) > > + meminfo = shm_membank->mem.banks.meminfo; > > + for ( i = 0; i < meminfo->nr_banks; i++ ) > > { > > - printk(XENLOG_ERR > > - "Failed to add %lu references to page %"PRI_mfn".\n", > > - nr_borrowers, mfn_x(smfn) + i); > > - goto fail; > > + page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start)); > > + nr_pages = PFN_DOWN(meminfo->bank[i].size); > > + ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers); > > + if ( ret ) > > + goto fail; > > } > > } > > + else > > + { > > + page = mfn_to_page( > > + maddr_to_mfn(shm_membank->mem.bank->start)); > > + nr_pages = shm_membank->mem.bank->size >> PAGE_SHIFT; > > + ret = add_shared_memory_ref(d, page, nr_pages, nr_borrowers); > > + if ( ret ) > > + return ret; > > + } > > ... this one. The code to deal with a bank is exactly the same. But you need > the duplication because you special case "one bank". > > As I wrote in a previous patch, I don't think we should special case it. > If the concern is memory usage, then we should look at reworking meminfo > instead (or using a different structure). > A few concerns explained why I didn't choose "struct meminfo" over two pointers "struct membank*" and "struct meminfo*". 1) memory usage is the main reason. If we use "struct meminfo" over the current "struct membank*" and "struct meminfo*", "struct shm_meminfo" will become a array of 256 "struct shm_membank", with "struct shm_membank" being also an 256-item array, that is 256 * 256, too big for a structure and If I remembered clearly, it will lead to "more than PAGE_SIZE" compiling error. FWIT, either reworking meminfo or using a different structure, are both leading to sizing down the array, hmmm, I don't know which size is suitable. That's why I prefer pointer and dynamic allocation. 2) If we use "struct meminfo*" over the current "struct membank*" and "struct meminfo*", we will need a new flag to differentiate two scenarios(host address provided or not), As the special case "struct membank*" is already helping us differentiate. And since when host address is provided, the related "struct membank" also needs to be reserved in "bootinfo.reserved_mem". That's why I used pointer " struct membank*" to avoid memory waste. For the duplication, I could extract the common codes to mitigate the impact. > > > > return 0; > > > > fail: > > while ( --i >= 0 ) > > - put_page_nr(page + i, nr_borrowers); > > + { > > + page = mfn_to_page(maddr_to_mfn(meminfo->bank[i].start)); > > + nr_pages = PFN_DOWN(meminfo->bank[i].size); > > + remove_shared_memory_ref(page, nr_pages, nr_borrowers); > > + } > > return ret; > > } > > > > Cheers, > > -- > Julien Grall
