[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 10/22] x86/mapcache: initialise the mapcache for the idle domain
Hi, On 22/12/2022 13:06, Jan Beulich wrote: On 16.12.2022 12:48, Julien Grall wrote:From: Hongyan Xia <hongyxia@xxxxxxxxxx> In order to use the mapcache in the idle domain, we also have to populate its page tables in the PERDOMAIN region, and we need to move mapcache_domain_init() earlier in arch_domain_create(). Note, commit 'x86: lift mapcache variable to the arch level' has initialised the mapcache for HVM domains. With this patch, PV, HVM, idle domains now all initialise the mapcache.But they can't use it yet, can they? This needs saying explicitly, or else one is going to make wrong implications. Yes, I tried to use the mapcache right after the idle vCPU gets scheduled and it worked. So, I believe it is enough. --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -732,6 +732,8 @@ int arch_domain_create(struct domain *d,spin_lock_init(&d->arch.e820_lock); + mapcache_domain_init(d);+ /* Minimal initialisation for the idle domain. */ if ( unlikely(is_idle_domain(d)) ) { @@ -829,8 +831,6 @@ int arch_domain_create(struct domain *d,psr_domain_init(d); - mapcache_domain_init(d);You move this ahead of error paths taking the "goto out" route, so adjustments to affected error paths are going to be needed to avoid memory leaks. Correct, I'll fix that. --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -5963,6 +5963,9 @@ int create_perdomain_mapping(struct domain *d, unsigned long va, l3tab = __map_domain_page(pg); clear_page(l3tab); d->arch.perdomain_l3_pg = pg; + if ( is_idle_domain(d) ) + idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] = + l4e_from_page(pg, __PAGE_HYPERVISOR_RW);Hmm, having an idle domain check here isn't very nice. I agree putting it in arch_domain_create()'s respective conditional isn't very neat either, but personally I'd consider this at least a little less bad. And the layering violation aspect isn't much worse than that of setting d->arch.ctxt_switch there as well. Why do you think it would be less bad to move it in arch_domain_create()? To me, it would make things worse as it would spread the mapping stuff across different functions. -- Elias
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |