On 13.05.2024 11:35, Elias El Yandouzi wrote:
> On 20/02/2024 10:51, Jan Beulich wrote:
>> On 16.01.2024 20:25, Elias El Yandouzi wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -750,9 +750,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,
>>> @@ -763,6 +770,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;
>>> }
>>
>> Why not add another call to mapcache_domain_init() right here, allowing
>> a more specific panic() to be invoked in case of failure (compared to
>> the BUG_ON() upon failure of creation of the idle domain as a whole)?
>> Then the other mapcache_domain_init() call doesn't need moving a 2nd
>> time in close succession.
>
> To be honest, I don't really like the idea of having twice the same call
> just for the benefit of having a panic() call in case of failure for the
> idle domain.
Resulting in the problem Roger has now validly pointed out in reply to v3.
IOW the (more specific) panic() isn't the only reason; it would merely be
an imo desirable side effect.
Jan
