Re: [PATCH v2 (resend) 12/27] x86/mapcache: Initialise the mapcache for the idle domain

[Jan Beulich <jbeulich@xxxxxxxx>]

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