| Message ID | 20221115025235.1378931-7-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Follow-up static shared memory | expand |
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. But as I wrote in a previous patch, I don't think you should convert {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). > > 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,
Hi Julien Happy new year~~~~ > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Sunday, January 8, 2023 8:53 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > 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
On 09/01/2023 07:49, Penny Zheng wrote: > Hi Julien Hi Penny, > Happy new year~~~~ Happy new year too! >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Sunday, January 8, 2023 8:53 PM >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini >> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> 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 . Ok. This is definitely not obvious from the code. But I think this is a very fragile assumption. Instead, it would be better if we allocate the memory in acquire_shared_memory_bank() when the host address is not provided. > >> 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? A few reasons: 1) I consider them as two distinct allocators. So far they have the same behavior, but in the future this may change. 2) If the page is freed you really don't want the domain to be able to re-use the page for a different purpose. I realize that 2) is already a problem today with static pages. So I think the best is to ensure that pages allocated for shared memory never reach the any of the allocators. > >> [...] >> >>> +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. I am not aware of any place where we would restrict the size of kinfo in upstream. Can you give me a pointer? > 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. I would expect that in most cases, you will need only one bank when the host address is not provided. So I think this is a bit odd to me to impose a "large" allocation for them. > 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. I am confused... Today you are defining as: struct { struct membank *; struct { struct meminfo *; unsigned long total_size; } } So on arm64 host, you would use 24 bytes. If we were using an union instead. We would use 16 bytes. Adding a 32-bit fields, would bring to 20 bytes (assuming we can re-use a padding). Therefore, there is no memory waste here with a flag... Cheers,
> -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Monday, January 9, 2023 6:58 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner > when host address not provided > > > > On 09/01/2023 07:49, Penny Zheng wrote: > > Hi Julien > > Hi Penny, > > > Happy new year~~~~ > > Happy new year too! > > >> -----Original Message----- > >> From: Julien Grall <julien@xen.org> > >> Sent: Sunday, January 8, 2023 8:53 PM > >> To: Penny Zheng <Penny.Zheng@arm.com>; xen- > devel@lists.xenproject.org > >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > >> <sstabellini@kernel.org>; Bertrand Marquis > >> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > >> <Volodymyr_Babchuk@epam.com> > >> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner > >> when host address not provided > >> > >> Hi, > >> > > > > 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. > > I am not aware of any place where we would restrict the size of kinfo in > upstream. Can you give me a pointer? > If I remembered correctly, my first version of "struct shm_meminfo" is this "big"(256 * 256) structure, and it leads to the whole xen binary is bigger than 2MB. ;\ > > 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. > > I would expect that in most cases, you will need only one bank when the host > address is not provided. So I think this is a bit odd to me to impose a "large" > allocation for them. > Only if user is not defining size as something like (2^a + 2^b + 2^c + ...). ;\ So maybe 8 or 16 is enough? struct new_meminfo { unsigned int nr_banks; struct membank bank[8]; }; Correct me if I'm wrong: The "struct shm_membank" you are suggesting is looking like this, right? struct shm_membank { char shm_id[MAX_SHM_ID_LENGTH]; unsigned int nr_shm_borrowers; struct new_meminfo shm_banks; unsigned long total_size; }; Let me try~ > > 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. > > I am confused... Today you are defining as: > > struct { > struct membank *; > struct { > struct meminfo *; > unsigned long total_size; > } > } > > So on arm64 host, you would use 24 bytes. If we were using an union instead. > We would use 16 bytes. Adding a 32-bit fields, would bring to > 20 bytes (assuming we can re-use a padding). > > Therefore, there is no memory waste here with a flag... > > Cheers, > > -- > Julien Grall
Hi Penny, On 09/01/2023 11:58, Penny Zheng wrote: >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Monday, January 9, 2023 6:58 PM >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini >> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner >> when host address not provided >> >> >> >> On 09/01/2023 07:49, Penny Zheng wrote: >>> Hi Julien >> >> Hi Penny, >> >>> Happy new year~~~~ >> >> Happy new year too! >> >>>> -----Original Message----- >>>> From: Julien Grall <julien@xen.org> >>>> Sent: Sunday, January 8, 2023 8:53 PM >>>> To: Penny Zheng <Penny.Zheng@arm.com>; xen- >> devel@lists.xenproject.org >>>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini >>>> <sstabellini@kernel.org>; Bertrand Marquis >>>> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk >>>> <Volodymyr_Babchuk@epam.com> >>>> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner >>>> when host address not provided >>>> >>>> Hi, >>>> >>> >>> 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. >> >> I am not aware of any place where we would restrict the size of kinfo in >> upstream. Can you give me a pointer? >> > > If I remembered correctly, my first version of "struct shm_meminfo" is this > "big"(256 * 256) structure, and it leads to the whole xen binary is bigger than 2MB. ;\ Ah so the problem is because shm_mem is used in bootinfo. Then I think we should create a distinct structure when dealing with domain information. > >>> 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. >> >> I would expect that in most cases, you will need only one bank when the host >> address is not provided. So I think this is a bit odd to me to impose a "large" >> allocation for them. >> > > Only if user is not defining size as something like (2^a + 2^b + 2^c + ...). ;\ > So maybe 8 or 16 is enough? > struct new_meminfo { "new" is a bit strange. The name would want to be changed. Or maybe better the structure been defined within the next structure and anonymized. > unsigned int nr_banks; > struct membank bank[8]; > }; > > Correct me if I'm wrong: > The "struct shm_membank" you are suggesting is looking like this, right? > struct shm_membank { > char shm_id[MAX_SHM_ID_LENGTH]; > unsigned int nr_shm_borrowers; > struct new_meminfo shm_banks; > unsigned long total_size; > }; AFAIU, shm_membank would still be used to get the information from the host device-tree. If so, then I am afraid this is not an option to me because it would make the code to reserve memory more complex. Instead, we should create a separate structure that will only be used for domain shared memory information. Cheers,
> -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Tuesday, January 10, 2023 2:23 AM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner > when host address not provided > > Hi Penny, Hi Julien, > > On 09/01/2023 11:58, Penny Zheng wrote: > >> -----Original Message----- > >> From: Julien Grall <julien@xen.org> > >> Sent: Monday, January 9, 2023 6:58 PM > >> To: Penny Zheng <Penny.Zheng@arm.com>; xen- > devel@lists.xenproject.org > >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > >> <sstabellini@kernel.org>; Bertrand Marquis > >> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > >> <Volodymyr_Babchuk@epam.com> > >> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner > >> when host address not provided > >> > >> > >> > >> On 09/01/2023 07:49, Penny Zheng wrote: > >>> Hi Julien > >> > >> Hi Penny, > >> > >>> Happy new year~~~~ > >> > >> Happy new year too! > >> > >>>> -----Original Message----- > >>>> From: Julien Grall <julien@xen.org> > >>>> Sent: Sunday, January 8, 2023 8:53 PM > >>>> To: Penny Zheng <Penny.Zheng@arm.com>; xen- > >> devel@lists.xenproject.org > >>>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > >>>> <sstabellini@kernel.org>; Bertrand Marquis > >>>> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > >>>> <Volodymyr_Babchuk@epam.com> > >>>> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to > >>>> owner when host address not provided > >>>> > >>>> Hi, > >>>> > >>> > >>> 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. > >> > >> I am not aware of any place where we would restrict the size of kinfo > >> in upstream. Can you give me a pointer? > >> > > > > If I remembered correctly, my first version of "struct shm_meminfo" is > > this > > "big"(256 * 256) structure, and it leads to the whole xen binary is > >. ;\ > > Ah so the problem is because shm_mem is used in bootinfo. Then I think we > should create a distinct structure when dealing with domain information. > Yes, If I use the latter "struct shm_info", keeping the shm memory info out of the bootinfo, I think we could avoid "bigger than 2MB" error. Hmm, out of curiosity, The way to create "distinct" structure is like creating another section for these distinct structures in lds, just like the existing .dtb section? > > > >>> 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. > >> > >> I would expect that in most cases, you will need only one bank when > >> the host address is not provided. So I think this is a bit odd to me to > impose a "large" > >> allocation for them. > >> > > > > Only if user is not defining size as something like (2^a + 2^b + 2^c + > > ...). ;\ So maybe 8 or 16 is enough? > > struct new_meminfo { > > "new" is a bit strange. The name would want to be changed. Or maybe better > the structure been defined within the next structure and anonymized. > > > unsigned int nr_banks; > > struct membank bank[8]; > > }; > > > > Correct me if I'm wrong: > > The "struct shm_membank" you are suggesting is looking like this, right? > > struct shm_membank { > > char shm_id[MAX_SHM_ID_LENGTH]; > > unsigned int nr_shm_borrowers; > > struct new_meminfo shm_banks; > > unsigned long total_size; > > }; > > AFAIU, shm_membank would still be used to get the information from the > host device-tree. If so, then I am afraid this is not an option to me because it > would make the code to reserve memory more complex. > > Instead, we should create a separate structure that will only be used for > domain shared memory information. > Ah, so you are suggesting we should extract the domain shared memory information only when dealing with the information from the host device-tree, something like this: struct shm_info { char shm_id[MAX_SHM_ID_LENGTH]; unsigned int nr_shm_borrowers; } I'll try and may *bother* you when encountering any problem~ > Cheers, > > -- > Julien Grall
Hi Penny, On 10/01/2023 03:38, Penny Zheng wrote: >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: Tuesday, January 10, 2023 2:23 AM >> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini >> <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; >> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner >> when host address not provided >> >> Hi Penny, > > Hi Julien, > >> >> On 09/01/2023 11:58, Penny Zheng wrote: >>>> -----Original Message----- >>>> From: Julien Grall <julien@xen.org> >>>> Sent: Monday, January 9, 2023 6:58 PM >>>> To: Penny Zheng <Penny.Zheng@arm.com>; xen- >> devel@lists.xenproject.org >>>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini >>>> <sstabellini@kernel.org>; Bertrand Marquis >>>> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk >>>> <Volodymyr_Babchuk@epam.com> >>>> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner >>>> when host address not provided >>>> >>>> >>>> >>>> On 09/01/2023 07:49, Penny Zheng wrote: >>>>> Hi Julien >>>> >>>> Hi Penny, >>>> >>>>> Happy new year~~~~ >>>> >>>> Happy new year too! >>>> >>>>>> -----Original Message----- >>>>>> From: Julien Grall <julien@xen.org> >>>>>> Sent: Sunday, January 8, 2023 8:53 PM >>>>>> To: Penny Zheng <Penny.Zheng@arm.com>; xen- >>>> devel@lists.xenproject.org >>>>>> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini >>>>>> <sstabellini@kernel.org>; Bertrand Marquis >>>>>> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk >>>>>> <Volodymyr_Babchuk@epam.com> >>>>>> Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to >>>>>> owner when host address not provided >>>>>> >>>>>> Hi, >>>>>> >>>>> >>>>> 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. >>>> >>>> I am not aware of any place where we would restrict the size of kinfo >>>> in upstream. Can you give me a pointer? >>>> >>> >>> If I remembered correctly, my first version of "struct shm_meminfo" is >>> this >>> "big"(256 * 256) structure, and it leads to the whole xen binary is >>> . ;\ >> >> Ah so the problem is because shm_mem is used in bootinfo. Then I think we >> should create a distinct structure when dealing with domain information. >> > > Yes, If I use the latter "struct shm_info", keeping the shm memory info out of the bootinfo, > I think we could avoid "bigger than 2MB" error. > > Hmm, out of curiosity, The way to create "distinct" structure is like creating another section > for these distinct structures in lds, just like the existing .dtb section? No. I meant defining a new structure (i.e. struct {}) that would be used in kernel_info. So you don't grow the one used by bootinfo. > >>> >>>>> 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. >>>> >>>> I would expect that in most cases, you will need only one bank when >>>> the host address is not provided. So I think this is a bit odd to me to >> impose a "large" >>>> allocation for them. >>>> >>> >>> Only if user is not defining size as something like (2^a + 2^b + 2^c + >>> ...). ;\ So maybe 8 or 16 is enough? >>> struct new_meminfo { >> >> "new" is a bit strange. The name would want to be changed. Or maybe better >> the structure been defined within the next structure and anonymized. >> >>> unsigned int nr_banks; >>> struct membank bank[8]; >>> }; >>> >>> Correct me if I'm wrong: >>> The "struct shm_membank" you are suggesting is looking like this, right? >>> struct shm_membank { >>> char shm_id[MAX_SHM_ID_LENGTH]; >>> unsigned int nr_shm_borrowers; >>> struct new_meminfo shm_banks; >>> unsigned long total_size; >>> }; >> >> AFAIU, shm_membank would still be used to get the information from the >> host device-tree. If so, then I am afraid this is not an option to me because it >> would make the code to reserve memory more complex. >> >> Instead, we should create a separate structure that will only be used for >> domain shared memory information. >> > > Ah, so you are suggesting we should extract the domain shared memory information only > when dealing with the information from the host device-tree, something like this: > struct shm_info { > char shm_id[MAX_SHM_ID_LENGTH]; > unsigned int nr_shm_borrowers; > } I am not entirely sure what you are suggesting. So I will wait for the code to understand. Cheers,
Hi Julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Monday, January 9, 2023 6:58 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Bertrand Marquis <Bertrand.Marquis@arm.com>; > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Subject: Re: [PATCH v1 06/13] xen/arm: assign shared memory to owner > when host address not provided > > > > On 09/01/2023 07:49, Penny Zheng wrote: > > Hi Julien > > Hi Penny, > > > Happy new year~~~~ > > Happy new year too! > > >> -----Original Message----- > >> From: Julien Grall <julien@xen.org> > >> Sent: Sunday, January 8, 2023 8:53 PM > >> To: Penny Zheng <Penny.Zheng@arm.com>; xen- > devel@lists.xenproject.org > >> Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > >> <sstabellini@kernel.org>; Bertrand Marquis > >> <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > >> <Volodymyr_Babchuk@epam.com> > >> 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 . > > Ok. This is definitely not obvious from the code. But I think this is a very > fragile assumption. > > Instead, it would be better if we allocate the memory in > acquire_shared_memory_bank() when the host address is not provided. > Right now, acquire_shared_memory_bank is called only when domain is owner domain. It is applicable when host address is provided, we could still do guest physmap when borrower accessed before owner, as address is provided. However when host address is not provided, we must allocate memory at first domain. So allocating the memory shall be called outside acquire_shared_memory_bank > > > >> 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? > > A few reasons: > 1) I consider them as two distinct allocators. So far they have the same > behavior, but in the future this may change. > 2) If the page is freed you really don't want the domain to be able to re-use > the page for a different purpose. > > > I realize that 2) is already a problem today with static pages. So I think the > best is to ensure that pages allocated for shared memory never reach the > any of the allocators. > > > Cheers, > > -- > Julien Grall Cheers, -- Julien Grall
Hi Penny, On 11/14/22 21:52, Penny Zheng wrote: > With the introduction of new scenario where host address is not provided > in "xen,shared-mem", the function "assign_shared_memory" shall be adapted > to it too. > > Shared memory will already be allocated from heap, when calling > "assign_shared_memory" with unprovided host address. > So in "assign_shared_memory", we just need to assign these static shared pages > to its owner domain using function "assign_pages", and add as many > additional reference as the number of borrowers. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > xen/arch/arm/domain_build.c | 160 ++++++++++++++++++++++++++++++------ > 1 file changed, 133 insertions(+), 27 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 3de96882a5..faf0784bb0 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -817,8 +817,12 @@ static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank) > { > struct page_info *page; > struct domain *d; > + paddr_t pbase; > > - page = maddr_to_page(shm_membank->mem.bank->start); > + pbase = shm_membank->mem.banks.meminfo ? > + shm_membank->mem.banks.meminfo->bank[0].start : > + shm_membank->mem.bank->start; > + page = maddr_to_page(pbase); > d = page_get_owner_and_reference(page); > if ( d == NULL ) > return false; > @@ -907,6 +911,7 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, > mfn_t smfn; > unsigned long nr_pfns; > int res; > + struct page_info *page; > > /* > * Pages of statically shared memory shall be included > @@ -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) ) > { > - printk(XENLOG_ERR > - "%pd: failed to acquire static memory: %d.\n", d, res); > - d->max_pages -= nr_pfns; > - return INVALID_MFN; > + res = assign_pages(page, nr_pfns, d, 0); > + if ( res ) > + { > + printk(XENLOG_ERR > + "%pd: failed to assign static memory: %d.\n", d, res); > + return INVALID_MFN; > + } > + } > + else > + { > + res = acquire_domstatic_pages(d, smfn, nr_pfns, 0); > + if ( res ) > + { > + printk(XENLOG_ERR > + "%pd: failed to acquire static memory: %d.\n", d, res); > + d->max_pages -= nr_pfns; > + return INVALID_MFN; > + } > } > > return smfn; > } > > -static int __init assign_shared_memory(struct domain *d, > - struct shm_membank *shm_membank, > - paddr_t gbase) > +static void __init remove_shared_memory_ref(struct page_info *page, > + unsigned long nr_pages, > + unsigned long nr_borrowers) > { > - mfn_t smfn; > - int ret = 0; > - unsigned long nr_pages, nr_borrowers, i; > - struct page_info *page; > - paddr_t pbase, psize; > + while ( --nr_pages >= 0 ) arch/arm/domain_build.c: In function ‘remove_shared_memory_ref’: arch/arm/domain_build.c:969:24: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits] 969 | while ( --nr_pages >= 0 ) | ^~ > + put_page_nr(page + nr_pages, nr_borrowers); > +} > > - pbase = shm_membank->mem.bank->start; > - psize = shm_membank->mem.bank->size; > +static int __init add_shared_memory_ref(struct domain *d, struct page_info *page, > + unsigned long nr_pages, > + unsigned long nr_borrowers) > +{ > + unsigned int i; > > - printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n", > - d, pbase, pbase + psize); > + /* > + * Instead of letting borrower domain get a page ref, we add as many > + * additional reference as the number of borrowers when the owner > + * is allocated, since there is a chance that owner is created > + * after borrower. > + * So if the borrower is created first, it will cause adding pages > + * in the P2M without reference. > + */ > + for ( i = 0; i < nr_pages; i++ ) > + { > + if ( !get_page_nr(page + i, d, nr_borrowers) ) > + { > + printk(XENLOG_ERR > + "Failed to add %lu references to page %"PRI_mfn".\n", > + nr_borrowers, mfn_x(page_to_mfn(page)) + i); > + goto fail; > + } > + } > + return 0; > + > + fail: > + remove_shared_memory_ref(page, i, nr_borrowers); > + return -EINVAL; > +} > + > +static int __init acquire_shared_memory(struct domain *d, > + paddr_t pbase, paddr_t psize, > + paddr_t gbase) > +{ > + mfn_t smfn; > + int ret = 0; > + unsigned long nr_pages; When building with EXTRA_CFLAGS_XEN_CORE="-Wunused-but-set-variable -Wno-error=unused-but-set-variable" arch/arm/domain_build.c: In function ‘acquire_shared_memory’: arch/arm/domain_build.c:1010:19: warning: variable ‘nr_pages’ set but not used [-Wunused-but-set-variable] 1010 | unsigned long nr_pages; | ^~~~~~~~ > > smfn = acquire_shared_memory_bank(d, pbase, psize); > if ( mfn_eq(smfn, INVALID_MFN) ) > @@ -970,6 +1024,44 @@ static int __init assign_shared_memory(struct domain *d, > } > } > > + return 0; > +} > + > +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; > + } > + > /* > * 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; > + } > > return 0; > > fail: > while ( --i >= 0 ) I'm aware that this line is unchanged, and I'm not trying to introduce scope creep, but I still wanted to point this out because (1) it is similar in nature to other occurrences introduced in this series and (2) the body of the loop is changed: arch/arm/domain_build.c: In function ‘assign_shared_memory’: arch/arm/domain_build.c:1109:17: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits] 1109 | 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; > } > > -- > 2.25.1 > >
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 3de96882a5..faf0784bb0 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -817,8 +817,12 @@ static bool __init is_shm_allocated_to_domio(struct shm_membank *shm_membank) { struct page_info *page; struct domain *d; + paddr_t pbase; - page = maddr_to_page(shm_membank->mem.bank->start); + pbase = shm_membank->mem.banks.meminfo ? + shm_membank->mem.banks.meminfo->bank[0].start : + shm_membank->mem.bank->start; + page = maddr_to_page(pbase); d = page_get_owner_and_reference(page); if ( d == NULL ) return false; @@ -907,6 +911,7 @@ static mfn_t __init acquire_shared_memory_bank(struct domain *d, mfn_t smfn; unsigned long nr_pfns; int res; + struct page_info *page; /* * Pages of statically shared memory shall be included @@ -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) ) { - printk(XENLOG_ERR - "%pd: failed to acquire static memory: %d.\n", d, res); - d->max_pages -= nr_pfns; - return INVALID_MFN; + res = assign_pages(page, nr_pfns, d, 0); + if ( res ) + { + printk(XENLOG_ERR + "%pd: failed to assign static memory: %d.\n", d, res); + return INVALID_MFN; + } + } + else + { + res = acquire_domstatic_pages(d, smfn, nr_pfns, 0); + if ( res ) + { + printk(XENLOG_ERR + "%pd: failed to acquire static memory: %d.\n", d, res); + d->max_pages -= nr_pfns; + return INVALID_MFN; + } } return smfn; } -static int __init assign_shared_memory(struct domain *d, - struct shm_membank *shm_membank, - paddr_t gbase) +static void __init remove_shared_memory_ref(struct page_info *page, + unsigned long nr_pages, + unsigned long nr_borrowers) { - mfn_t smfn; - int ret = 0; - unsigned long nr_pages, nr_borrowers, i; - struct page_info *page; - paddr_t pbase, psize; + while ( --nr_pages >= 0 ) + put_page_nr(page + nr_pages, nr_borrowers); +} - pbase = shm_membank->mem.bank->start; - psize = shm_membank->mem.bank->size; +static int __init add_shared_memory_ref(struct domain *d, struct page_info *page, + unsigned long nr_pages, + unsigned long nr_borrowers) +{ + unsigned int i; - printk("%pd: allocate static shared memory BANK %#"PRIpaddr"-%#"PRIpaddr".\n", - d, pbase, pbase + psize); + /* + * Instead of letting borrower domain get a page ref, we add as many + * additional reference as the number of borrowers when the owner + * is allocated, since there is a chance that owner is created + * after borrower. + * So if the borrower is created first, it will cause adding pages + * in the P2M without reference. + */ + for ( i = 0; i < nr_pages; i++ ) + { + if ( !get_page_nr(page + i, d, nr_borrowers) ) + { + printk(XENLOG_ERR + "Failed to add %lu references to page %"PRI_mfn".\n", + nr_borrowers, mfn_x(page_to_mfn(page)) + i); + goto fail; + } + } + return 0; + + fail: + remove_shared_memory_ref(page, i, nr_borrowers); + return -EINVAL; +} + +static int __init acquire_shared_memory(struct domain *d, + paddr_t pbase, paddr_t psize, + paddr_t gbase) +{ + mfn_t smfn; + int ret = 0; + unsigned long nr_pages; smfn = acquire_shared_memory_bank(d, pbase, psize); if ( mfn_eq(smfn, INVALID_MFN) ) @@ -970,6 +1024,44 @@ static int __init assign_shared_memory(struct domain *d, } } + return 0; +} + +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; + } + /* * 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; + } 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; }
With the introduction of new scenario where host address is not provided in "xen,shared-mem", the function "assign_shared_memory" shall be adapted to it too. Shared memory will already be allocated from heap, when calling "assign_shared_memory" with unprovided host address. So in "assign_shared_memory", we just need to assign these static shared pages to its owner domain using function "assign_pages", and add as many additional reference as the number of borrowers. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- xen/arch/arm/domain_build.c | 160 ++++++++++++++++++++++++++++++------ 1 file changed, 133 insertions(+), 27 deletions(-)