On 10.01.2024 17:24, Elias El Yandouzi wrote: > On 22/12/2022 13:06, Jan Beulich wrote: >> On 16.12.2022 12:48, Julien Grall wrote: >>> --- 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.
Not sure what to add to what I said: create_perdomain_mapping() gaining such a check is a layering violation to me. arch_domain_create() otoh special cases the idle domain already. Jan
