| Message ID | 20240513134046.82605-6-eliasely@amazon.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Remove the directmap | expand |
On Mon, May 13, 2024 at 01:40:32PM +0000, Elias El Yandouzi wrote: > From: Hongyan Xia <hongyxia@amazon.com> > > 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(). Oh, so this is the reason for the remark on the previous commit message: idle domain init gets short-circuited earlier in arch_domain_create() and never gets to the mapcache_domain_init() call. > 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. > > Signed-off-by: Wei Wang <wawei@amazon.de> > Signed-off-by: Hongyan Xia <hongyxia@amazon.com> > Signed-off-by: Julien Grall <jgrall@amazon.com> > Signed-off-by: Elias El Yandouzi <eliasely@amazon.com> > > ---- > > Changes in V2: > * Free resources if mapcache initialisation fails > * Remove `is_idle_domain()` check from `create_perdomain_mappings()` I'm not seeing any is_idle_domain() in create_perdomain_mapping(), and neither anything removed by this patch. > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c > index 507d704f16..3303bdb53e 100644 > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -758,9 +758,16 @@ int arch_domain_create(struct domain *d, > > spin_lock_init(&d->arch.e820_lock); > > + if ( (rc = mapcache_domain_init(d)) != 0) > + { > + free_perdomain_mappings(d); > + return rc; What about all the error paths below here that don't use the fail label, don't you need to also call free_perdomain_mappings() on them? Or alternatively arrange the fail label to be suitable to be used this early if it's not already the case. > + } > + > /* Minimal initialisation for the idle domain. */ > if ( unlikely(is_idle_domain(d)) ) > { > + struct page_info *pg = d->arch.perdomain_l3_pg; const? > static const struct arch_csw idle_csw = { > .from = paravirt_ctxt_switch_from, > .to = paravirt_ctxt_switch_to, > @@ -771,6 +778,9 @@ int arch_domain_create(struct domain *d, > > d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */ > > + idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] = > + l4e_from_page(pg, __PAGE_HYPERVISOR_RW); Albeit I think you could just use d->arch.perdomain_l3_pg directly here and avoid the local pg variable? Would you mind adding: /* Slot 260: Per-domain mappings. */ I wonder if it won't be better to just use init_xen_l4_slots() and special case the idle domain in there, instead of open-coding the L4 population for the idle domain like this. Thanks, Roger.
On 14.05.2024 10:42, Roger Pau Monné wrote: > On Mon, May 13, 2024 at 01:40:32PM +0000, Elias El Yandouzi wrote: >> @@ -771,6 +778,9 @@ int arch_domain_create(struct domain *d, >> >> d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */ >> >> + idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] = >> + l4e_from_page(pg, __PAGE_HYPERVISOR_RW); > > Albeit I think you could just use d->arch.perdomain_l3_pg directly > here and avoid the local pg variable? > > Would you mind adding: > > /* Slot 260: Per-domain mappings. */ > > I wonder if it won't be better to just use init_xen_l4_slots() and > special case the idle domain in there, instead of open-coding the L4 > population for the idle domain like this. That would require changes to init_xen_l4_slots(), afaics, which I'm not sure we actually want (for, perhaps, being more intrusive than adding the two lines here). However, with this I'd like to get back to my earlier remark regarding the (further) moving of the mapcache_domain_init() invocation: You need to touch this idle domain specific path anyway. With that I view your earlier argument as yet more weak, even leaving aside the previously indicated fallout that this further movement causes. Jan
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 507d704f16..3303bdb53e 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -758,9 +758,16 @@ int arch_domain_create(struct domain *d, spin_lock_init(&d->arch.e820_lock); + if ( (rc = mapcache_domain_init(d)) != 0) + { + free_perdomain_mappings(d); + return rc; + } + /* Minimal initialisation for the idle domain. */ if ( unlikely(is_idle_domain(d)) ) { + struct page_info *pg = d->arch.perdomain_l3_pg; static const struct arch_csw idle_csw = { .from = paravirt_ctxt_switch_from, .to = paravirt_ctxt_switch_to, @@ -771,6 +778,9 @@ int arch_domain_create(struct domain *d, d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */ + idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] = + l4e_from_page(pg, __PAGE_HYPERVISOR_RW); + return 0; } @@ -851,8 +861,6 @@ int arch_domain_create(struct domain *d, psr_domain_init(d); - mapcache_domain_init(d); - if ( is_hvm_domain(d) ) { if ( (rc = hvm_domain_initialise(d, config)) != 0 )