Re: [PATCH 10/22] x86/mapcache: initialise the mapcache for the idle domain

From: Jan Beulich <jbeulich@suse.com>
To: Elias El Yandouzi <eliasely@amazon.com>
Cc: "Hongyan Xia" <hongyxia@amazon.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"Wei Wang" <wawei@amazon.de>, "Julien Grall" <jgrall@amazon.com>,
	xen-devel@lists.xenproject.org, "Julien Grall" <julien@xen.org>
Subject: Re: [PATCH 10/22] x86/mapcache: initialise the mapcache for the idle domain
Date: Thu, 11 Jan 2024 08:53:13 +0100	[thread overview]
Message-ID: <88d4cb45-f2e6-4acd-8ec4-060f90a444fc@suse.com> (raw)
In-Reply-To: <88024314-245f-404e-a6b7-7ea859bae309@amazon.com>

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