| Message ID | 20210824095045.2281500-8-penny.zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Domain on Static Allocation | expand |
On Tue, 24 Aug 2021, Penny Zheng wrote: > This commit introduces allocate_static_memory to allocate static memory as > guest RAM for Domain on Static Allocation. > > It uses acquire_domstatic_pages to acquire pre-configured static memory > for this domain, and uses guest_physmap_add_pages to set up P2M table. > These pre-defined static memory banks shall be mapped to the fixed guest RAM > banks. And only when they exhausts the current guest RAM bank, it will seek > to the next one. > > In order to deal with the trouble of count-to-order conversion when page number > is not in a power-of-two, this commit exports p2m_insert_mapping and introduce > a new function guest_physmap_add_pages to cope with adding guest RAM p2m > mapping with nr_pages. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > v5 changes: > - don't split comment over multi-line (even they are more than 80 characters) > - simply use dt_find_property(node, "xen,static-mem", NULL) to tell > whether using allocate_static_memory, and add error comment when > "xen,static-mem" is used but CONFIG_STATIC_MEMORY is not enabled. > - exporting p2m_insert_mapping() and introduce guest_physmap_add_pages > to cope with adding guest RAM p2m mapping with nr_pages. > - check both pbase and psize are page aligned > - simplify the code in the loops by moving append_static_memory_to_bank() > outside of the if/else. > --- > xen/arch/arm/domain_build.c | 156 +++++++++++++++++++++++++++++++++++- > xen/arch/arm/p2m.c | 7 +- > xen/include/asm-arm/p2m.h | 11 +++ > 3 files changed, 168 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > index 6c86d52781..843b8514c7 100644 > --- a/xen/arch/arm/domain_build.c > +++ b/xen/arch/arm/domain_build.c > @@ -480,6 +480,148 @@ fail: > (unsigned long)kinfo->unassigned_mem >> 10); > } > > +#ifdef CONFIG_STATIC_MEMORY > +static bool __init append_static_memory_to_bank(struct domain *d, > + struct membank *bank, > + mfn_t smfn, > + paddr_t size) > +{ > + int res; > + unsigned int nr_pages = size >> PAGE_SHIFT; ^ PFN_DOWN > + /* Infer next GFN. */ > + gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size); > + > + res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages); > + if ( res ) > + { > + dprintk(XENLOG_ERR, "Failed to map pages to DOMU: %d", res); > + return false; > + } > + > + bank->size = bank->size + size; > + > + return true; > +} > + > +/* Allocate memory from static memory as RAM for one specific domain d. */ > +static void __init allocate_static_memory(struct domain *d, > + struct kernel_info *kinfo, > + const struct dt_device_node *node) > +{ > + const struct dt_property *prop; > + u32 addr_cells, size_cells, reg_cells; > + unsigned int nr_banks, gbank = 0, bank = 0; > + const uint64_t rambase[] = GUEST_RAM_BANK_BASES; > + const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES; > + const __be32 *cell; > + u64 tot_size = 0; > + paddr_t pbase, psize, gsize; > + mfn_t smfn; > + int res; > + > + prop = dt_find_property(node, "xen,static-mem", NULL); > + if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells", > + &addr_cells) ) > + { > + printk(XENLOG_ERR > + "%pd: failed to read \"#xen,static-mem-address-cells\".\n", d); > + goto fail; > + } > + > + if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells", > + &size_cells) ) > + { > + printk(XENLOG_ERR > + "%pd: failed to read \"#xen,static-mem-size-cells\".\n", d); > + goto fail; > + } > + reg_cells = addr_cells + size_cells; > + > + /* Start with GUEST_RAM0. */ > + gsize = ramsize[gbank]; > + kinfo->mem.bank[gbank].start = rambase[gbank]; > + > + cell = (const __be32 *)prop->value; > + nr_banks = (prop->length) / (reg_cells * sizeof (u32)); > + > + for ( ; bank < nr_banks; bank++ ) > + { > + device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize); > + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE)); > + > + smfn = maddr_to_mfn(pbase); > + res = acquire_domstatic_pages(d, smfn, psize >> PAGE_SHIFT, 0); ^ PFN_DOWN(psize) > + if ( res ) > + { > + printk(XENLOG_ERR > + "%pd: failed to acquire static memory: %d.\n", d, res); > + goto fail; > + } > + > + printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-%#"PRIpaddr"\n", > + d, bank, pbase, pbase + psize); > + > + /* > + * It shall be mapped to the fixed guest RAM banks(GUEST_RAM_BANK_BASES), > + * And only when it exhausts the current guest RAM bank, it will seek > + * to the next. > + */ > + while ( 1 ) > + { > + /* Map as much as possible the static range to the guest bank */ > + if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank], smfn, > + min(psize, gsize)) ) > + goto fail; > + > + /* > + * The current physical bank is fully mapped. > + * Handle the next physical bank. > + */ > + if ( gsize >= psize ) > + { > + gsize = gsize - psize; > + break; > + } > + /* > + * When current guest bank size is not enough to map. > + * Before seeking to the next, check if we still have available > + * guest bank. > + */ > + else if ( (gbank + 1) >= GUEST_RAM_BANKS ) > + { > + printk(XENLOG_ERR "Exhausted all fixed guest banks.\n"); > + goto fail; > + } > + else > + { > + psize = psize - gsize; > + smfn = mfn_add(smfn, gsize >> PAGE_SHIFT); ^ PFN_DOWN > + /* Update to the next guest bank. */ > + gbank++; > + gsize = ramsize[gbank]; > + kinfo->mem.bank[gbank].start = rambase[gbank]; > + } > + } > + > + tot_size += psize; > + } > + > + kinfo->mem.nr_banks = ++gbank; > + kinfo->unassigned_mem -= tot_size; > + if ( kinfo->unassigned_mem ) > + { > + printk(XENLOG_ERR > + "Size of \"memory\" property doesn't match up with the sum-up of \"xen,static-mem\".\n"); > + goto fail; Do we need to make this a fatal failure? I am asking because unassigned_mem comes from the "memory" property of the domain in device tree which is kind of redundant with the introduction of xen,static-mem. In fact, I think it would be better to clarify the role of "memory" when "xen,static-mem" is also present. In that case, we could even make "memory" optional. In any case, even if we don't make "memory" optional, it might still be good to turn this error into a warning and ignore the remaining kinfo->unassigned_mem. > + } > + > + return; > + > +fail: > + panic("Failed to allocate requested static memory for domain %pd.", d); > +} > +#endif > + > static int __init write_properties(struct domain *d, struct kernel_info *kinfo, > const struct dt_device_node *node) > { > @@ -2452,7 +2594,19 @@ static int __init construct_domU(struct domain *d, > /* type must be set before allocate memory */ > d->arch.type = kinfo.type; > #endif > - allocate_memory(d, &kinfo); > + if ( !dt_find_property(node, "xen,static-mem", NULL) ) > + allocate_memory(d, &kinfo); > +#ifdef CONFIG_STATIC_MEMORY > + else > + allocate_static_memory(d, &kinfo, node); > +#else > + else > + { > + printk(XENLOG_ERR > + "CONFIG_STATIC_MEMORY must be enabled to use \"xen,static-mem\".\n"); > + return -EINVAL; > + } > +#endif To avoid the double else in the code, this part could be written like this which is a bit simpler I think: if ( !dt_find_property(node, "xen,static-mem", NULL) ) allocate_memory(d, &kinfo); else { #ifdef CONFIG_STATIC_MEMORY allocate_static_memory(d, &kinfo, node); #else printk(XENLOG_ERR "CONFIG_STATIC_MEMORY must be enabled to use \"xen,static-mem\".\n"); return -EINVAL; #endif } This is just a suggestion to improve readability, I am also OK with what you wrote. (Another alternative would be to provide a stub allocate_static_memory implementation for !CONFIG_STATIC_MEMORY.)
On Thu, 2 Sep 2021, Stefano Stabellini wrote: > On Tue, 24 Aug 2021, Penny Zheng wrote: > > This commit introduces allocate_static_memory to allocate static memory as > > guest RAM for Domain on Static Allocation. > > > > It uses acquire_domstatic_pages to acquire pre-configured static memory > > for this domain, and uses guest_physmap_add_pages to set up P2M table. > > These pre-defined static memory banks shall be mapped to the fixed guest RAM > > banks. And only when they exhausts the current guest RAM bank, it will seek > > to the next one. > > > > In order to deal with the trouble of count-to-order conversion when page number > > is not in a power-of-two, this commit exports p2m_insert_mapping and introduce > > a new function guest_physmap_add_pages to cope with adding guest RAM p2m > > mapping with nr_pages. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > v5 changes: > > - don't split comment over multi-line (even they are more than 80 characters) > > - simply use dt_find_property(node, "xen,static-mem", NULL) to tell > > whether using allocate_static_memory, and add error comment when > > "xen,static-mem" is used but CONFIG_STATIC_MEMORY is not enabled. > > - exporting p2m_insert_mapping() and introduce guest_physmap_add_pages > > to cope with adding guest RAM p2m mapping with nr_pages. > > - check both pbase and psize are page aligned > > - simplify the code in the loops by moving append_static_memory_to_bank() > > outside of the if/else. > > --- > > xen/arch/arm/domain_build.c | 156 +++++++++++++++++++++++++++++++++++- > > xen/arch/arm/p2m.c | 7 +- > > xen/include/asm-arm/p2m.h | 11 +++ > > 3 files changed, 168 insertions(+), 6 deletions(-) > > > > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c > > index 6c86d52781..843b8514c7 100644 > > --- a/xen/arch/arm/domain_build.c > > +++ b/xen/arch/arm/domain_build.c > > @@ -480,6 +480,148 @@ fail: > > (unsigned long)kinfo->unassigned_mem >> 10); > > } > > > > +#ifdef CONFIG_STATIC_MEMORY > > +static bool __init append_static_memory_to_bank(struct domain *d, > > + struct membank *bank, > > + mfn_t smfn, > > + paddr_t size) > > +{ > > + int res; > > + unsigned int nr_pages = size >> PAGE_SHIFT; > ^ PFN_DOWN > > > > + /* Infer next GFN. */ > > + gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size); > > + > > + res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages); > > + if ( res ) > > + { > > + dprintk(XENLOG_ERR, "Failed to map pages to DOMU: %d", res); > > + return false; > > + } > > + > > + bank->size = bank->size + size; > > + > > + return true; > > +} > > + > > +/* Allocate memory from static memory as RAM for one specific domain d. */ > > +static void __init allocate_static_memory(struct domain *d, > > + struct kernel_info *kinfo, > > + const struct dt_device_node *node) > > +{ > > + const struct dt_property *prop; > > + u32 addr_cells, size_cells, reg_cells; > > + unsigned int nr_banks, gbank = 0, bank = 0; > > + const uint64_t rambase[] = GUEST_RAM_BANK_BASES; > > + const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES; > > + const __be32 *cell; > > + u64 tot_size = 0; > > + paddr_t pbase, psize, gsize; > > + mfn_t smfn; > > + int res; > > + > > + prop = dt_find_property(node, "xen,static-mem", NULL); > > + if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells", > > + &addr_cells) ) > > + { > > + printk(XENLOG_ERR > > + "%pd: failed to read \"#xen,static-mem-address-cells\".\n", d); > > + goto fail; > > + } > > + > > + if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells", > > + &size_cells) ) > > + { > > + printk(XENLOG_ERR > > + "%pd: failed to read \"#xen,static-mem-size-cells\".\n", d); > > + goto fail; > > + } > > + reg_cells = addr_cells + size_cells; > > + > > + /* Start with GUEST_RAM0. */ > > + gsize = ramsize[gbank]; > > + kinfo->mem.bank[gbank].start = rambase[gbank]; > > + > > + cell = (const __be32 *)prop->value; > > + nr_banks = (prop->length) / (reg_cells * sizeof (u32)); > > + > > + for ( ; bank < nr_banks; bank++ ) > > + { > > + device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize); > > + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE)); > > + > > + smfn = maddr_to_mfn(pbase); > > + res = acquire_domstatic_pages(d, smfn, psize >> PAGE_SHIFT, 0); > ^ PFN_DOWN(psize) > > > > + if ( res ) > > + { > > + printk(XENLOG_ERR > > + "%pd: failed to acquire static memory: %d.\n", d, res); > > + goto fail; > > + } > > + > > + printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-%#"PRIpaddr"\n", > > + d, bank, pbase, pbase + psize); > > + > > + /* > > + * It shall be mapped to the fixed guest RAM banks(GUEST_RAM_BANK_BASES), > > + * And only when it exhausts the current guest RAM bank, it will seek > > + * to the next. > > + */ > > + while ( 1 ) > > + { > > + /* Map as much as possible the static range to the guest bank */ > > + if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank], smfn, > > + min(psize, gsize)) ) > > + goto fail; > > + > > + /* > > + * The current physical bank is fully mapped. > > + * Handle the next physical bank. > > + */ > > + if ( gsize >= psize ) > > + { > > + gsize = gsize - psize; > > + break; > > + } > > + /* > > + * When current guest bank size is not enough to map. > > + * Before seeking to the next, check if we still have available > > + * guest bank. > > + */ > > + else if ( (gbank + 1) >= GUEST_RAM_BANKS ) > > + { > > + printk(XENLOG_ERR "Exhausted all fixed guest banks.\n"); > > + goto fail; > > + } > > + else > > + { > > + psize = psize - gsize; > > + smfn = mfn_add(smfn, gsize >> PAGE_SHIFT); > ^ PFN_DOWN > > > > + /* Update to the next guest bank. */ > > + gbank++; > > + gsize = ramsize[gbank]; > > + kinfo->mem.bank[gbank].start = rambase[gbank]; > > + } > > + } > > + > > + tot_size += psize; > > + } > > + > > + kinfo->mem.nr_banks = ++gbank; > > + kinfo->unassigned_mem -= tot_size; > > + if ( kinfo->unassigned_mem ) > > + { > > + printk(XENLOG_ERR > > + "Size of \"memory\" property doesn't match up with the sum-up of \"xen,static-mem\".\n"); > > + goto fail; > > Do we need to make this a fatal failure? > > I am asking because unassigned_mem comes from the "memory" property of > the domain in device tree which is kind of redundant with the > introduction of xen,static-mem. In fact, I think it would be better to > clarify the role of "memory" when "xen,static-mem" is also present. > In that case, we could even make "memory" optional. > > In any case, even if we don't make "memory" optional, it might still be > good to turn this error into a warning and ignore the remaining > kinfo->unassigned_mem. One more thing: if we decide to make "memory" optional, we need to avoid failing if it is not present at the beginning of construct_domU (if xen,static-mem is present).
Hi Stefano, On 02/09/2021 22:32, Stefano Stabellini wrote: > On Tue, 24 Aug 2021, Penny Zheng wrote: >> This commit introduces allocate_static_memory to allocate static memory as >> guest RAM for Domain on Static Allocation. >> >> It uses acquire_domstatic_pages to acquire pre-configured static memory >> for this domain, and uses guest_physmap_add_pages to set up P2M table. >> These pre-defined static memory banks shall be mapped to the fixed guest RAM >> banks. And only when they exhausts the current guest RAM bank, it will seek >> to the next one. >> >> In order to deal with the trouble of count-to-order conversion when page number >> is not in a power-of-two, this commit exports p2m_insert_mapping and introduce >> a new function guest_physmap_add_pages to cope with adding guest RAM p2m >> mapping with nr_pages. >> >> Signed-off-by: Penny Zheng <penny.zheng@arm.com> >> --- >> v5 changes: >> - don't split comment over multi-line (even they are more than 80 characters) >> - simply use dt_find_property(node, "xen,static-mem", NULL) to tell >> whether using allocate_static_memory, and add error comment when >> "xen,static-mem" is used but CONFIG_STATIC_MEMORY is not enabled. >> - exporting p2m_insert_mapping() and introduce guest_physmap_add_pages >> to cope with adding guest RAM p2m mapping with nr_pages. >> - check both pbase and psize are page aligned >> - simplify the code in the loops by moving append_static_memory_to_bank() >> outside of the if/else. >> --- >> xen/arch/arm/domain_build.c | 156 +++++++++++++++++++++++++++++++++++- >> xen/arch/arm/p2m.c | 7 +- >> xen/include/asm-arm/p2m.h | 11 +++ >> 3 files changed, 168 insertions(+), 6 deletions(-) >> >> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >> index 6c86d52781..843b8514c7 100644 >> --- a/xen/arch/arm/domain_build.c >> +++ b/xen/arch/arm/domain_build.c >> @@ -480,6 +480,148 @@ fail: >> (unsigned long)kinfo->unassigned_mem >> 10); >> } >> >> +#ifdef CONFIG_STATIC_MEMORY >> +static bool __init append_static_memory_to_bank(struct domain *d, >> + struct membank *bank, >> + mfn_t smfn, >> + paddr_t size) >> +{ >> + int res; >> + unsigned int nr_pages = size >> PAGE_SHIFT; > ^ PFN_DOWN > > >> + /* Infer next GFN. */ >> + gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size); >> + >> + res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages); >> + if ( res ) >> + { >> + dprintk(XENLOG_ERR, "Failed to map pages to DOMU: %d", res); >> + return false; >> + } >> + >> + bank->size = bank->size + size; >> + >> + return true; >> +} >> + >> +/* Allocate memory from static memory as RAM for one specific domain d. */ >> +static void __init allocate_static_memory(struct domain *d, >> + struct kernel_info *kinfo, >> + const struct dt_device_node *node) >> +{ >> + const struct dt_property *prop; >> + u32 addr_cells, size_cells, reg_cells; >> + unsigned int nr_banks, gbank = 0, bank = 0; >> + const uint64_t rambase[] = GUEST_RAM_BANK_BASES; >> + const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES; >> + const __be32 *cell; >> + u64 tot_size = 0; >> + paddr_t pbase, psize, gsize; >> + mfn_t smfn; >> + int res; >> + >> + prop = dt_find_property(node, "xen,static-mem", NULL); >> + if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells", >> + &addr_cells) ) >> + { >> + printk(XENLOG_ERR >> + "%pd: failed to read \"#xen,static-mem-address-cells\".\n", d); >> + goto fail; >> + } >> + >> + if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells", >> + &size_cells) ) >> + { >> + printk(XENLOG_ERR >> + "%pd: failed to read \"#xen,static-mem-size-cells\".\n", d); >> + goto fail; >> + } >> + reg_cells = addr_cells + size_cells; >> + >> + /* Start with GUEST_RAM0. */ >> + gsize = ramsize[gbank]; >> + kinfo->mem.bank[gbank].start = rambase[gbank]; >> + >> + cell = (const __be32 *)prop->value; >> + nr_banks = (prop->length) / (reg_cells * sizeof (u32)); >> + >> + for ( ; bank < nr_banks; bank++ ) >> + { >> + device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize); >> + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE)); >> + >> + smfn = maddr_to_mfn(pbase); >> + res = acquire_domstatic_pages(d, smfn, psize >> PAGE_SHIFT, 0); > ^ PFN_DOWN(psize) > > >> + if ( res ) >> + { >> + printk(XENLOG_ERR >> + "%pd: failed to acquire static memory: %d.\n", d, res); >> + goto fail; >> + } >> + >> + printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-%#"PRIpaddr"\n", >> + d, bank, pbase, pbase + psize); >> + >> + /* >> + * It shall be mapped to the fixed guest RAM banks(GUEST_RAM_BANK_BASES), >> + * And only when it exhausts the current guest RAM bank, it will seek >> + * to the next. >> + */ >> + while ( 1 ) >> + { >> + /* Map as much as possible the static range to the guest bank */ >> + if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank], smfn, >> + min(psize, gsize)) ) >> + goto fail; >> + >> + /* >> + * The current physical bank is fully mapped. >> + * Handle the next physical bank. >> + */ >> + if ( gsize >= psize ) >> + { >> + gsize = gsize - psize; >> + break; >> + } >> + /* >> + * When current guest bank size is not enough to map. >> + * Before seeking to the next, check if we still have available >> + * guest bank. >> + */ >> + else if ( (gbank + 1) >= GUEST_RAM_BANKS ) >> + { >> + printk(XENLOG_ERR "Exhausted all fixed guest banks.\n"); >> + goto fail; >> + } >> + else >> + { >> + psize = psize - gsize; >> + smfn = mfn_add(smfn, gsize >> PAGE_SHIFT); > ^ PFN_DOWN > > >> + /* Update to the next guest bank. */ >> + gbank++; >> + gsize = ramsize[gbank]; >> + kinfo->mem.bank[gbank].start = rambase[gbank]; >> + } >> + } >> + >> + tot_size += psize; >> + } >> + >> + kinfo->mem.nr_banks = ++gbank; >> + kinfo->unassigned_mem -= tot_size; >> + if ( kinfo->unassigned_mem ) >> + { >> + printk(XENLOG_ERR >> + "Size of \"memory\" property doesn't match up with the sum-up of \"xen,static-mem\".\n"); >> + goto fail; > > Do we need to make this a fatal failure? > > I am asking because unassigned_mem comes from the "memory" property of > the domain in device tree which is kind of redundant with the > introduction of xen,static-mem. In fact, I think it would be better to > clarify the role of "memory" when "xen,static-mem" is also present. > In that case, we could even make "memory" optional. I requested to make it mandatory. Imagine you have a domU that has 1GB and now you want to switch to static memory. If we make the property optional, then there is a risk for the admin to not correctly pass the amount of memory. This may become unnoticed until late. So I think making it mandatory is cheap for us and an easy way to confirm you properly sized the region. It also has the benefits to easily find out how much memory you gave to the guest (but that's just because I am lazy :)). > In any case, even if we don't make "memory" optional, it might still be > good to turn this error into a warning and ignore the remaining > kinfo->unassigned_mem. The behavior is matching the existing function and I think this is a good idea. If you ask 10MB of memory and we only give you 9MB, then at some point your guest is not going to be happy. It is much better to know it in advance with a failure over discovering hours later when you see an OOM from your domain. > > >> + } >> + >> + return; >> + >> +fail: >> + panic("Failed to allocate requested static memory for domain %pd.", d); >> +} >> +#endif >> + >> static int __init write_properties(struct domain *d, struct kernel_info *kinfo, >> const struct dt_device_node *node) >> { >> @@ -2452,7 +2594,19 @@ static int __init construct_domU(struct domain *d, >> /* type must be set before allocate memory */ >> d->arch.type = kinfo.type; >> #endif >> - allocate_memory(d, &kinfo); >> + if ( !dt_find_property(node, "xen,static-mem", NULL) ) >> + allocate_memory(d, &kinfo); >> +#ifdef CONFIG_STATIC_MEMORY >> + else >> + allocate_static_memory(d, &kinfo, node); >> +#else >> + else >> + { >> + printk(XENLOG_ERR >> + "CONFIG_STATIC_MEMORY must be enabled to use \"xen,static-mem\".\n"); >> + return -EINVAL; >> + } >> +#endif > > To avoid the double else in the code, this part could be written like > this which is a bit simpler I think: > > if ( !dt_find_property(node, "xen,static-mem", NULL) ) > allocate_memory(d, &kinfo); > else > { > #ifdef CONFIG_STATIC_MEMORY > allocate_static_memory(d, &kinfo, node); > #else > printk(XENLOG_ERR > "CONFIG_STATIC_MEMORY must be enabled to use \"xen,static-mem\".\n"); > return -EINVAL; > #endif > } > > This is just a suggestion to improve readability, I am also OK with what > you wrote. > > (Another alternative would be to provide a stub allocate_static_memory > implementation for !CONFIG_STATIC_MEMORY.) My preference is 1) Stub function 2) your #ifdef proposal 3) Penny's approach. Cheers,
On Thu, 2 Sep 2021, Julien Grall wrote: > > > + kinfo->mem.nr_banks = ++gbank; > > > + kinfo->unassigned_mem -= tot_size; > > > + if ( kinfo->unassigned_mem ) > > > + { > > > + printk(XENLOG_ERR > > > + "Size of \"memory\" property doesn't match up with the > > > sum-up of \"xen,static-mem\".\n"); > > > + goto fail; > > > > Do we need to make this a fatal failure? > > > > I am asking because unassigned_mem comes from the "memory" property of > > the domain in device tree which is kind of redundant with the > > introduction of xen,static-mem. In fact, I think it would be better to > > clarify the role of "memory" when "xen,static-mem" is also present. > > In that case, we could even make "memory" optional. > > I requested to make it mandatory. Imagine you have a domU that has 1GB and now > you want to switch to static memory. If we make the property optional, then > there is a risk for the admin to not correctly pass the amount of memory. This > may become unnoticed until late. > > So I think making it mandatory is cheap for us and an easy way to confirm you > properly sized the region. It also has the benefits to easily find out how > much memory you gave to the guest (but that's just because I am lazy :)). > > > In any case, even if we don't make "memory" optional, it might still be > > good to turn this error into a warning and ignore the remaining > > kinfo->unassigned_mem. > > The behavior is matching the existing function and I think this is a good > idea. If you ask 10MB of memory and we only give you 9MB, then at some point > your guest is not going to be happy. > > It is much better to know it in advance with a failure over discovering hours > later when you see an OOM from your domain. OK, I didn't realize this was discussed already. Let's not revisit this then. My preference is primarily to make the device tree easier to write, but nowadays nobody I know is writing the device tree by hand anymore (they all use ImageBuilder). So if the device tree is generated then we are fine either way as long as the binding is clear. So I am OK to drop my suggestion of making "memory" optional. Let's think of a way to make "memory" and "xen,static-mem" coexist instead. There are two sides of the issue: - the Xen implementation - the binding The Xen implementation is fine to panic if memory != xen,static-mem. In that regard, the current patch is OK. From the binding perspective, I think it would be good to add a statement to clarify. The binding doesn't necessarily need to match exactly the implementation as the binding should be as future proof and as flexible as possible. From the binding perspective two properties should mean different things: memory the total memory amount and xen,static-mem the static memory. If one day we'll have more types of memory, memory will cover the total amount while xen,static-mem will cover a subset. So memory must be greater or equal to xen,static-mem (even if today Xen only supports memory == xen,static-mem). So I would add: """ As the memory property represents the total memory of the domain, hence the amount of memory selected by the memory property must be greater or equal to the total amount specified by xen,static-mem. Other configurations (memory amount less than xen,static-mem amount) are invalid. """ This sentence has the purpose of clarifying that "memory" still need to be populated and have a valid value. Then, it is OK for Xen to error out if memory doesn't match xen,static-mem because that's the only configuration supported. The error message could be: Size of "memory" property doesn't match up with the sum-up of "xen,static-mem". Unsupported configuration.
On 03/09/2021 01:39, Stefano Stabellini wrote: > On Thu, 2 Sep 2021, Julien Grall wrote: >>>> + kinfo->mem.nr_banks = ++gbank; >>>> + kinfo->unassigned_mem -= tot_size; >>>> + if ( kinfo->unassigned_mem ) >>>> + { >>>> + printk(XENLOG_ERR >>>> + "Size of \"memory\" property doesn't match up with the >>>> sum-up of \"xen,static-mem\".\n"); >>>> + goto fail; >>> >>> Do we need to make this a fatal failure? >>> >>> I am asking because unassigned_mem comes from the "memory" property of >>> the domain in device tree which is kind of redundant with the >>> introduction of xen,static-mem. In fact, I think it would be better to >>> clarify the role of "memory" when "xen,static-mem" is also present. >>> In that case, we could even make "memory" optional. >> >> I requested to make it mandatory. Imagine you have a domU that has 1GB and now >> you want to switch to static memory. If we make the property optional, then >> there is a risk for the admin to not correctly pass the amount of memory. This >> may become unnoticed until late. >> >> So I think making it mandatory is cheap for us and an easy way to confirm you >> properly sized the region. It also has the benefits to easily find out how >> much memory you gave to the guest (but that's just because I am lazy :)). >> >>> In any case, even if we don't make "memory" optional, it might still be >>> good to turn this error into a warning and ignore the remaining >>> kinfo->unassigned_mem. >> >> The behavior is matching the existing function and I think this is a good >> idea. If you ask 10MB of memory and we only give you 9MB, then at some point >> your guest is not going to be happy. >> >> It is much better to know it in advance with a failure over discovering hours >> later when you see an OOM from your domain. > > OK, I didn't realize this was discussed already. Let's not revisit this > then. > > My preference is primarily to make the device tree easier to write, but > nowadays nobody I know is writing the device tree by hand anymore (they > all use ImageBuilder).So if the device tree is generated then we are > fine either way as long as the binding is clear. So I am OK to drop my > suggestion of making "memory" optional. Let's think of a way to make > "memory" and "xen,static-mem" coexist instead. > > > There are two sides of the issue: > - the Xen implementation > - the binding > > > The Xen implementation is fine to panic if memory != xen,static-mem. In > that regard, the current patch is OK. > > > From the binding perspective, I think it would be good to add a > statement to clarify. The binding doesn't necessarily need to match > exactly the implementation as the binding should be as future proof > and as flexible as possible. So I agree that the binding doesn't have to match the implementation. But... the binding always have be more restrictive than the implementation. Otherwise, someone following the binding may be not able to use it with Xen. > > From the binding perspective two properties should mean different > things: memory the total memory amount and xen,static-mem the static > memory. If one day we'll have more types of memory, memory will cover > the total amount while xen,static-mem will cover a subset. So memory > must be greater or equal to xen,static-mem (even if today Xen only > supports memory == xen,static-mem). > > So I would add: > > """ > As the memory property represents the total memory of the domain, hence > the amount of memory selected by the memory property must be greater or > equal to the total amount specified by xen,static-mem. Other > configurations (memory amount less than xen,static-mem amount) are > invalid. > """ > > This sentence has the purpose of clarifying that "memory" still need to > be populated and have a valid value. Then, it is OK for Xen to error > out if memory doesn't match xen,static-mem because that's the only > configuration supported. How about writing something like "The property 'memory' should match the amount of memory given to the guest. Currently, it is only possible to either allocate static memory or let Xen chose. *Mixing* is not supported'. Then if we add the mixing, we could say 'From Xen XX.YY, mixing will be allowed'. Cheers,
Hi Julien and Stefano > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Friday, September 3, 2021 3:42 PM > To: Stefano Stabellini <sstabellini@kernel.org> > Cc: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org; > Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen > <Wei.Chen@arm.com>; nd <nd@arm.com> > Subject: Re: [PATCH v5 7/7] xen/arm: introduce allocate_static_memory > > > > On 03/09/2021 01:39, Stefano Stabellini wrote: > > On Thu, 2 Sep 2021, Julien Grall wrote: > >>>> + kinfo->mem.nr_banks = ++gbank; > >>>> + kinfo->unassigned_mem -= tot_size; > >>>> + if ( kinfo->unassigned_mem ) > >>>> + { > >>>> + printk(XENLOG_ERR > >>>> + "Size of \"memory\" property doesn't match up with > >>>> + the > >>>> sum-up of \"xen,static-mem\".\n"); > >>>> + goto fail; > >>> > >>> Do we need to make this a fatal failure? > >>> > >>> I am asking because unassigned_mem comes from the "memory" property > >>> of the domain in device tree which is kind of redundant with the > >>> introduction of xen,static-mem. In fact, I think it would be better > >>> to clarify the role of "memory" when "xen,static-mem" is also present. > >>> In that case, we could even make "memory" optional. > >> > >> I requested to make it mandatory. Imagine you have a domU that has > >> 1GB and now you want to switch to static memory. If we make the > >> property optional, then there is a risk for the admin to not > >> correctly pass the amount of memory. This may become unnoticed until > late. > >> > >> So I think making it mandatory is cheap for us and an easy way to > >> confirm you properly sized the region. It also has the benefits to > >> easily find out how much memory you gave to the guest (but that's just > because I am lazy :)). > >> > >>> In any case, even if we don't make "memory" optional, it might still > >>> be good to turn this error into a warning and ignore the remaining > >>> kinfo->unassigned_mem. > >> > >> The behavior is matching the existing function and I think this is a > >> good idea. If you ask 10MB of memory and we only give you 9MB, then > >> at some point your guest is not going to be happy. > >> > >> It is much better to know it in advance with a failure over > >> discovering hours later when you see an OOM from your domain. > > > > OK, I didn't realize this was discussed already. Let's not revisit > > this then. > > > > My preference is primarily to make the device tree easier to write, > > but nowadays nobody I know is writing the device tree by hand anymore > > (they all use ImageBuilder).So if the device tree is generated then we > > are fine either way as long as the binding is clear. So I am OK to > > drop my suggestion of making "memory" optional. Let's think of a way > > to make "memory" and "xen,static-mem" coexist instead. > > > > > > There are two sides of the issue: > > - the Xen implementation > > - the binding > > > > > > The Xen implementation is fine to panic if memory != xen,static-mem. > > In that regard, the current patch is OK. > > > > > > From the binding perspective, I think it would be good to add a > > statement to clarify. The binding doesn't necessarily need to match > > exactly the implementation as the binding should be as future proof > > and as flexible as possible. > > So I agree that the binding doesn't have to match the implementation. > But... the binding always have be more restrictive than the implementation. > Otherwise, someone following the binding may be not able to use it with Xen. > > > > > From the binding perspective two properties should mean different > > things: memory the total memory amount and xen,static-mem the static > > memory. If one day we'll have more types of memory, memory will cover > > the total amount while xen,static-mem will cover a subset. So memory > > must be greater or equal to xen,static-mem (even if today Xen only > > supports memory == xen,static-mem). > > > > So I would add: > > > > """ > > As the memory property represents the total memory of the domain, > > hence the amount of memory selected by the memory property must be > > greater or equal to the total amount specified by xen,static-mem. > > Other configurations (memory amount less than xen,static-mem amount) > > are invalid. > > """ > > > > This sentence has the purpose of clarifying that "memory" still need > > to be populated and have a valid value. Then, it is OK for Xen to > > error out if memory doesn't match xen,static-mem because that's the > > only configuration supported. > How about writing something like "The property 'memory' should match the > amount of memory given to the guest. Currently, it is only possible to either > allocate static memory or let Xen chose. *Mixing* is not supported'. > > Then if we add the mixing, we could say 'From Xen XX.YY, mixing will be > allowed'. > Ok. I'll add the statement to clarify the binding. > Cheers, > > -- > Julien Grall
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 6c86d52781..843b8514c7 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -480,6 +480,148 @@ fail: (unsigned long)kinfo->unassigned_mem >> 10); } +#ifdef CONFIG_STATIC_MEMORY +static bool __init append_static_memory_to_bank(struct domain *d, + struct membank *bank, + mfn_t smfn, + paddr_t size) +{ + int res; + unsigned int nr_pages = size >> PAGE_SHIFT; + /* Infer next GFN. */ + gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size); + + res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages); + if ( res ) + { + dprintk(XENLOG_ERR, "Failed to map pages to DOMU: %d", res); + return false; + } + + bank->size = bank->size + size; + + return true; +} + +/* Allocate memory from static memory as RAM for one specific domain d. */ +static void __init allocate_static_memory(struct domain *d, + struct kernel_info *kinfo, + const struct dt_device_node *node) +{ + const struct dt_property *prop; + u32 addr_cells, size_cells, reg_cells; + unsigned int nr_banks, gbank = 0, bank = 0; + const uint64_t rambase[] = GUEST_RAM_BANK_BASES; + const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES; + const __be32 *cell; + u64 tot_size = 0; + paddr_t pbase, psize, gsize; + mfn_t smfn; + int res; + + prop = dt_find_property(node, "xen,static-mem", NULL); + if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells", + &addr_cells) ) + { + printk(XENLOG_ERR + "%pd: failed to read \"#xen,static-mem-address-cells\".\n", d); + goto fail; + } + + if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells", + &size_cells) ) + { + printk(XENLOG_ERR + "%pd: failed to read \"#xen,static-mem-size-cells\".\n", d); + goto fail; + } + reg_cells = addr_cells + size_cells; + + /* Start with GUEST_RAM0. */ + gsize = ramsize[gbank]; + kinfo->mem.bank[gbank].start = rambase[gbank]; + + cell = (const __be32 *)prop->value; + nr_banks = (prop->length) / (reg_cells * sizeof (u32)); + + for ( ; bank < nr_banks; bank++ ) + { + device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize); + ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE)); + + smfn = maddr_to_mfn(pbase); + res = acquire_domstatic_pages(d, smfn, psize >> PAGE_SHIFT, 0); + if ( res ) + { + printk(XENLOG_ERR + "%pd: failed to acquire static memory: %d.\n", d, res); + goto fail; + } + + printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-%#"PRIpaddr"\n", + d, bank, pbase, pbase + psize); + + /* + * It shall be mapped to the fixed guest RAM banks(GUEST_RAM_BANK_BASES), + * And only when it exhausts the current guest RAM bank, it will seek + * to the next. + */ + while ( 1 ) + { + /* Map as much as possible the static range to the guest bank */ + if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank], smfn, + min(psize, gsize)) ) + goto fail; + + /* + * The current physical bank is fully mapped. + * Handle the next physical bank. + */ + if ( gsize >= psize ) + { + gsize = gsize - psize; + break; + } + /* + * When current guest bank size is not enough to map. + * Before seeking to the next, check if we still have available + * guest bank. + */ + else if ( (gbank + 1) >= GUEST_RAM_BANKS ) + { + printk(XENLOG_ERR "Exhausted all fixed guest banks.\n"); + goto fail; + } + else + { + psize = psize - gsize; + smfn = mfn_add(smfn, gsize >> PAGE_SHIFT); + /* Update to the next guest bank. */ + gbank++; + gsize = ramsize[gbank]; + kinfo->mem.bank[gbank].start = rambase[gbank]; + } + } + + tot_size += psize; + } + + kinfo->mem.nr_banks = ++gbank; + kinfo->unassigned_mem -= tot_size; + if ( kinfo->unassigned_mem ) + { + printk(XENLOG_ERR + "Size of \"memory\" property doesn't match up with the sum-up of \"xen,static-mem\".\n"); + goto fail; + } + + return; + +fail: + panic("Failed to allocate requested static memory for domain %pd.", d); +} +#endif + static int __init write_properties(struct domain *d, struct kernel_info *kinfo, const struct dt_device_node *node) { @@ -2452,7 +2594,19 @@ static int __init construct_domU(struct domain *d, /* type must be set before allocate memory */ d->arch.type = kinfo.type; #endif - allocate_memory(d, &kinfo); + if ( !dt_find_property(node, "xen,static-mem", NULL) ) + allocate_memory(d, &kinfo); +#ifdef CONFIG_STATIC_MEMORY + else + allocate_static_memory(d, &kinfo, node); +#else + else + { + printk(XENLOG_ERR + "CONFIG_STATIC_MEMORY must be enabled to use \"xen,static-mem\".\n"); + return -EINVAL; + } +#endif rc = prepare_dtb_domU(d, &kinfo); if ( rc < 0 ) diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index eff9a105e7..6e01e83967 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1293,11 +1293,8 @@ out: return resolved; } -static inline int p2m_insert_mapping(struct domain *d, - gfn_t start_gfn, - unsigned long nr, - mfn_t mfn, - p2m_type_t t) +int p2m_insert_mapping(struct domain *d, gfn_t start_gfn, unsigned long nr, + mfn_t mfn, p2m_type_t t) { struct p2m_domain *p2m = p2m_get_hostp2m(d); int rc; diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h index 6a2108398f..f885cc522b 100644 --- a/xen/include/asm-arm/p2m.h +++ b/xen/include/asm-arm/p2m.h @@ -300,6 +300,9 @@ int map_dev_mmio_region(struct domain *d, unsigned long nr, mfn_t mfn); +int p2m_insert_mapping(struct domain *d, gfn_t start_gfn, unsigned long nr, + mfn_t mfn, p2m_type_t t); + int guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn, @@ -315,6 +318,14 @@ static inline int guest_physmap_add_page(struct domain *d, return guest_physmap_add_entry(d, gfn, mfn, page_order, p2m_ram_rw); } +static inline int guest_physmap_add_pages(struct domain *d, + gfn_t gfn, + mfn_t mfn, + unsigned int nr_pages) +{ + return p2m_insert_mapping(d, gfn, nr_pages, mfn, p2m_ram_rw); +} + mfn_t gfn_to_mfn(struct domain *d, gfn_t gfn); /* Look up a GFN and take a reference count on the backing page. */
This commit introduces allocate_static_memory to allocate static memory as guest RAM for Domain on Static Allocation. It uses acquire_domstatic_pages to acquire pre-configured static memory for this domain, and uses guest_physmap_add_pages to set up P2M table. These pre-defined static memory banks shall be mapped to the fixed guest RAM banks. And only when they exhausts the current guest RAM bank, it will seek to the next one. In order to deal with the trouble of count-to-order conversion when page number is not in a power-of-two, this commit exports p2m_insert_mapping and introduce a new function guest_physmap_add_pages to cope with adding guest RAM p2m mapping with nr_pages. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- v5 changes: - don't split comment over multi-line (even they are more than 80 characters) - simply use dt_find_property(node, "xen,static-mem", NULL) to tell whether using allocate_static_memory, and add error comment when "xen,static-mem" is used but CONFIG_STATIC_MEMORY is not enabled. - exporting p2m_insert_mapping() and introduce guest_physmap_add_pages to cope with adding guest RAM p2m mapping with nr_pages. - check both pbase and psize are page aligned - simplify the code in the loops by moving append_static_memory_to_bank() outside of the if/else. --- xen/arch/arm/domain_build.c | 156 +++++++++++++++++++++++++++++++++++- xen/arch/arm/p2m.c | 7 +- xen/include/asm-arm/p2m.h | 11 +++ 3 files changed, 168 insertions(+), 6 deletions(-)