|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v11 03/12] xen/arm: permit non direct-mapped Dom0 construction
Hi Michal, Julien
On Sat, Dec 7, 2024 at 4:05 PM Michal Orzel <michal.orzel@xxxxxxx> wrote:
>
> On 06/12/2024 19:37, Julien Grall wrote:
> >
> >
> > Hi,
> >
> > Sorry for the late answer.
> >
> > On 05/12/2024 09:40, Michal Orzel wrote:
> >>
> >>
> >> On 02/12/2024 17:59, Carlo Nonato wrote:
> >>>
> >>>
> >>> Cache coloring requires Dom0 not to be direct-mapped because of its non
> >>> contiguous mapping nature, so allocate_memory() is needed in this case.
> >>> 8d2c3ab18cc1 ("arm/dom0less: put dom0less feature code in a separate
> >>> module")
> >>> moved allocate_memory() in dom0less_build.c. In order to use it
> >>> in Dom0 construction bring it back to domain_build.c and declare it in
> >>> domain_build.h.
> >>>
> >>> Take the opportunity to adapt the implementation of allocate_memory() so
> >>> that it uses the host layout when called on the hwdom, via
> >>> find_unallocated_memory().
> >>>
> >>> Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
> >>> ---
> >>> v11:
> >>> - GUEST_RAM_BANKS instead of hardcoding the number of banks in
> >>> allocate_memory()
> >>> - hwdom_ext_regions -> hwdom_free_mem in allocate_memory()
> >>> - added a comment in allocate_memory() when skipping small banks
> >>> v10:
> >>> - fixed a compilation bug that happened when dom0less support was disabled
> >>> v9:
> >>> - no changes
> >>> v8:
> >>> - patch adapted to new changes to allocate_memory()
> >>> v7:
> >>> - allocate_memory() now uses the host layout when called on the hwdom
> >>> v6:
> >>> - new patch
> >>> ---
> >>> xen/arch/arm/dom0less-build.c | 44 -----------
> >>> xen/arch/arm/domain_build.c | 97 ++++++++++++++++++++++++-
> >>> xen/arch/arm/include/asm/domain_build.h | 1 +
> >>> 3 files changed, 94 insertions(+), 48 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> >>> index d93a85434e..67b1503647 100644
> >>> --- a/xen/arch/arm/dom0less-build.c
> >>> +++ b/xen/arch/arm/dom0less-build.c
> >>> @@ -49,50 +49,6 @@ bool __init is_dom0less_mode(void)
> >>> return ( !dom0found && domUfound );
> >>> }
> >>>
> >>> -static void __init allocate_memory(struct domain *d, struct kernel_info
> >>> *kinfo)
> >>> -{
> >>> - struct membanks *mem = kernel_info_get_mem(kinfo);
> >>> - unsigned int i;
> >>> - paddr_t bank_size;
> >>> -
> >>> - printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
> >>> - /* Don't want format this as PRIpaddr (16 digit hex) */
> >>> - (unsigned long)(kinfo->unassigned_mem >> 20), d);
> >>> -
> >>> - mem->nr_banks = 0;
> >>> - bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
> >>> - if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
> >>> - bank_size) )
> >>> - goto fail;
> >>> -
> >>> - bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
> >>> - if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
> >>> - bank_size) )
> >>> - goto fail;
> >>> -
> >>> - if ( kinfo->unassigned_mem )
> >>> - goto fail;
> >>> -
> >>> - for( i = 0; i < mem->nr_banks; i++ )
> >>> - {
> >>> - printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr"
> >>> (%ldMB)\n",
> >>> - d,
> >>> - i,
> >>> - mem->bank[i].start,
> >>> - mem->bank[i].start + mem->bank[i].size,
> >>> - /* Don't want format this as PRIpaddr (16 digit hex) */
> >>> - (unsigned long)(mem->bank[i].size >> 20));
> >>> - }
> >>> -
> >>> - return;
> >>> -
> >>> -fail:
> >>> - panic("Failed to allocate requested domain memory."
> >>> - /* Don't want format this as PRIpaddr (16 digit hex) */
> >>> - " %ldKB unallocated. Fix the VMs configurations.\n",
> >>> - (unsigned long)kinfo->unassigned_mem >> 10);
> >>> -}
> >>> -
> >>> #ifdef CONFIG_VGICV2
> >>> static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
> >>> {
> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> >>> index 2c30792de8..2b8cba9b2f 100644
> >>> --- a/xen/arch/arm/domain_build.c
> >>> +++ b/xen/arch/arm/domain_build.c
> >>> @@ -416,7 +416,6 @@ static void __init allocate_memory_11(struct domain
> >>> *d,
> >>> }
> >>> }
> >>>
> >>> -#ifdef CONFIG_DOM0LESS_BOOT
> >>> bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size,
> >>> alloc_domheap_mem_cb cb, void
> >>> *extra)
> >>> {
> >>> @@ -508,7 +507,6 @@ bool __init allocate_bank_memory(struct kernel_info
> >>> *kinfo, gfn_t sgfn,
> >>>
> >>> return true;
> >>> }
> >>> -#endif
> >>>
> >>> /*
> >>> * When PCI passthrough is available we want to keep the
> >>> @@ -1003,6 +1001,94 @@ out:
> >>> return res;
> >>> }
> >>>
> >>> +void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
> >>> +{
> >>> + struct membanks *mem = kernel_info_get_mem(kinfo);
> >>> + unsigned int i, nr_banks = GUEST_RAM_BANKS;
> >>> + paddr_t bank_start, bank_size;
> >> Limit the scope
> >>
> >>> + struct membanks *hwdom_free_mem = NULL;
> >>> + const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> >>> + const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> >> Limit the scope
> >>
> >>> +
> >>> + printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
> >>> + /* Don't want format this as PRIpaddr (16 digit hex) */
> >>> + (unsigned long)(kinfo->unassigned_mem >> 20), d);
> >>> +
> >>> + mem->nr_banks = 0;
> >>> + /*
> >>> + * Use host memory layout for hwdom. Only case for this is when LLC
> >>> coloring
> >>> + * is enabled.
> >>> + */
> >>> + if ( is_hardware_domain(d) )
> >>> + {
> >>> + ASSERT(llc_coloring_enabled);
> >> This patch does not build because of declaration not being visible. You
> >> must include <xen/llc-coloring.h>.
> >
> > Piggying back on this comment. AFAICT, the code below would work also in
> > the non cache coloring case. So what's the assert is for?
> >
> >>
> >>> +
> >>> + hwdom_free_mem = xzalloc_flex_struct(struct membanks, bank,
> >>> + NR_MEM_BANKS);
> >>> + if ( !hwdom_free_mem )
> >>> + goto fail;
> >>> +
> >>> + hwdom_free_mem->max_banks = NR_MEM_BANKS;
> >>> +
> >>> + if ( find_unallocated_memory(kinfo, hwdom_free_mem) )
> >> My remarks for the use of find_unallocated_memory() 1:1 have not been
> >> addressed. You did not even
> >> change the comments inside the function. The problem is that the function
> >> is specifically designed
> >> for finding extended regions and assumes being called at certain point
> >> i.e. dom0 RAM allocated, gnttab
> >> region allocated, etc.
Answering Michal. Sorry about it, since we were waiting for comments and I
wanted to keep the revision alive (it happend too many times that we
(minervasys) left the discussion hanging for too long) I sent the v11
even if it was incomplete. I should have at least added commens, you're
right.
> > So I agree that the function should be updated if we plan to use it for
> > other purpose.
> >
> > My opinion is that we should attempt to make the function generic so
> > that in your
> >> case you can choose which regions to exclude, define even your own
> >> function to grab free regions (at the moment
> >> add_ext_regions grabs banks >= 64M but you still discards banks >= 128M,
> >> so it's a bit wasteful.
> >>
> >> My very short attempt to make the function as generic as possible in the
> >> first iteration:
> >> https://paste.debian.net/1338334/
> >
> > This looks better, but I wonder why we need still need to exclude the
> > static regions? Wouldn't it be sufficient to exclude just reserved regions?
> Static shared memory banks are not part of reserved memory (i.e.
> bootinfo.reserved_mem) if that's what you're asking.
> They are stored in bootinfo.shmem, hence we need to take them into account
> when searching for unused address space.
>
> If you and Carlo are ok with my proposed solution for making the function
> generic, I can send a patch as a prerequisite
> patch for Carlo series.
I'm ok with that.
> ~Michal
Thanks both.
- Carlo
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |