On Mon, May 13, 2024 at 01:40:32PM +0000, Elias El Yandouzi wrote:
> From: Hongyan Xia <hongy...@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 <wa...@amazon.de>
> Signed-off-by: Hongyan Xia <hongy...@amazon.com>
> Signed-off-by: Julien Grall <jgr...@amazon.com>
> Signed-off-by: Elias El Yandouzi <elias...@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.
